Repository navigation
Check the feed hash before parsing - #1672
Merged
Merged
Conversation
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 29 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
This was referenced Oct 20, 2026
aaronbrethorst
changed the base branch from
ab/1657-release-parsed-feed
to
ab/1657-stream-stop-times
October 20, 2026 07:39
aaronbrethorst
force-pushed
the
ab/1657-hash-first
branch
from
October 20, 2026 07:39
c595935 to
749706f
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
aaronbrethorst
added
the
skip-template-check
burma-shave
reviewed
Oct 21, 2026
This was referenced Oct 25, 2026
aaronbrethorst
force-pushed
the
ab/1657-hash-first
branch
from
October 25, 2026 04:34
749706f to
adb85b5
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
reviewed
Nov 7, 2026
aaronbrethorst
force-pushed
the
ab/1657-hash-first
branch
from
November 7, 2026 18:20
adb85b5 to
bd37cb1
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
aaronbrethorst
force-pushed
the
ab/1657-hash-first
branch
from
November 7, 2026 18:53
bd37cb1 to
285fe94
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
force-pushed
the
ab/1657-hash-first
branch
from
November 7, 2026 21:56
285fe94 to
fdac212
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
force-pushed
the
ab/1657-hash-first
branch
from
November 7, 2026 22:03
fdac212 to
c4fd923
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
force-pushed
the
ab/1657-hash-first
branch
from
November 7, 2026 23:26
c4fd923 to
ad81198
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
force-pushed
the
ab/1657-hash-first
branch
from
November 8, 2026 02:46
ad81198 to
35d7343
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
force-pushed
the
ab/1657-hash-first
branch
from
November 8, 2026 02:49
35d7343 to
06eae3b
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
ReloadStatic held the parsed feed in a local variable that it read again after the import, so the whole feed stayed reachable while stop directions were precomputed. For NJ Transit bus that kept several hundred MB alive through a stage that needs about a hundred. storeConfiguredFeed now loads, parses and stores the feed, and the parsed copy is reachable only from there. ReloadStatic precomputes directions after it returns, first calling debug.FreeOSMemory so the feed's memory is collected and returned to the OS instead of waiting for the GC to get around to it. importStaticIntoDB, which did the store and the precompute together, is replaced by precomputeStopDirections. On NJ Transit, the precompute starts at 113 MiB of heap instead of about 630 MiB, and the process's peak RSS falls from 1.0 GiB to about 950 MiB. The peak is now the parse itself (about 555 MiB of heap), which only a streaming parser (#1657, step 3) can lower.
Every startup and every daily reload read the whole static feed into memory and parsed it, and only then did StoreGtfsData compare its hash with import_metadata. So a restart with an unchanged feed and an existing database reached nearly the same memory peak as a first import. fetchFeedFile now puts the feed in a local file while hashing it: a local feed is read where it is, and a remote one is downloaded next to the database rather than to a temporary directory that may be held in memory. Downloads left behind by a crash are removed first. storeConfiguredFeed asks the new Client.HasImported whether the database already holds that hash from that source, and parses the feed only when it doesn't. HasImported and StoreGtfsData share one isSameFeed rule. The hash is still the SHA-256 of the feed bytes, so existing databases match and don't re-import. parse hashes the bytes it actually reads, so the stored hash always describes the stored data. With gtfstidy on, the hash is marked "+gtfstidy" only when tidying succeeded: turning tidy on or off causes a re-import, and a failed tidy is retried on the next reload instead of being recorded as done. A database imported with tidy on re-imports once. The static download now clones http.DefaultTransport, like the realtime client, so it honors proxy settings from the environment. For NJ Transit bus with an existing database, a restart is healthy in 1 s with a peak RSS of 194 MB. On main it takes 4 s and 825 MB.
With gtfstidy on, a feed was stored under its plain hash when tidying failed, but every reload looked for the hash marked "+gtfstidy". The check never matched, so a feed whose tidy always fails was read, tidied again and parsed on every restart and reload, and only then skipped by StoreGtfsData. Hash-first never helped such a feed. The import hash now depends only on the feed's content and whether tidy is on. A failed tidy is stored under the marked hash, so the next reload skips the feed, and tidy is retried when the feed changes.
The hash-first check in storeConfiguredFeed and the check inside StoreGtfsData each compared import_metadata and refreshed the feed expiry on their own, and logged the skip differently: one with the full hash, the other with its first 8 characters. A later change to one path would make an unchanged feed behave differently depending on which check caught it. Client.SkipIfImported replaces HasImported and, like StoreGtfsData, goes through skipUnchangedFeed, which compares the feed, refreshes the expiry and logs gtfs_data_unchanged_skipping_import with the short hash. RefreshFeedExpiry is no longer needed outside gtfsdb, so it is unexported.
Before each download, every gtfs-static-*.zip in the database's directory, or in the temporary directory for an in-memory database, was removed. Reloads are serialized only within one Manager, so a second process whose database shares that directory, or a second in-memory process, could remove a download between its fetch and its parse and fail the reload. Any operator file that matched the pattern was removed too. A download's name now carries its owner: the database file's name, or the process ID for an in-memory database. A process removes only its own leftovers, matched by the exact name os.CreateTemp gives them. The database path is also read as SQLite reads it. A "file:" URI, such as file:/data/gtfs.db?cache=shared, was taken as a directory named "file:", so every remote reload failed, and file::memory:?cache=shared sent downloads to the working directory.
downloadFeedFile hashes the feed while it writes it to disk, and parse then hashed the same bytes again, about a second of extra work on a 200 MB feed. Only a local feed can change between the fetch and the parse; a download is a file private to this import. feedFile now records whether it is private, and parse hashes again only a local feed.
With gtfstidy on, parse read the whole feed from disk into memory only so tidyGTFSData could write it back out to a temporary file for gtfstidy. That added feed-sized copies at the memory peak this change set out to lower. tidyGTFSFile now gives gtfstidy the feed's own path, made absolute because gtfstidy runs in its temporary directory. The feed is read into memory untidied only when tidying fails.
Each reload built a new client with a new transport and never closed it, so the keep-alive connection and its goroutines stayed idle for 90 seconds on a transport nothing would use again. A package-level client, like the realtime one, is shared by every reload instead. The download timeouts are now named constants, and the static and realtime clients clone http.DefaultTransport through one cloneDefaultTransport helper.
burma-shave
force-pushed
the
ab/1657-hash-first
branch
from
November 8, 2026 02:54
06eae3b to
bebd526
Compare
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
approved these changes
Nov 8, 2026
9 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #1657 (steps 2b and 2c). Stacked on #1669.
What it does
Two commits:
storeConfiguredFeedloads, parses and stores the feed, so the parsed copy is reachable only inside it.ReloadStaticcallsdebug.FreeOSMemoryand then precomputes stop directions. This runs only when the data changed, at most once per daily reload.fetchFeedFiledownloads a remote feed next to the database while hashing it (a local feed is hashed in place); downloads left behind by a crash are removed first.storeConfiguredFeedasks the newClient.HasImportedand parses only when the database does not already hold the feed.StoreGtfsDatakeeps its own check; both use oneisSameFeedrule. The download cloneshttp.DefaultTransport, so proxy environment variables apply.When the feed is unchanged, both paths call
Client.RefreshFeedExpiry, so a database written by an older build still picks up the corrected expiry from #1686 without a re-import.Root cause
Every startup and daily reload read the whole feed into memory and parsed it before
StoreGtfsDatacompared hashes, so a restart with an unchanged feed reached nearly the same memory peak as a first import. Separately,ReloadStatickept the parsed feed alive in a local variable while directions were precomputed.Testing
mainbefore this change: n/a (memory change). The expiry-refresh interaction is pinned byTestReloadStaticUnchangedFeedRefreshesExpiry, which fails without theRefreshFeedExpirycall in the short-circuit:TestImportMemory, at the previous PR in the stack and at this branch:precomputing_stop_directions_started)Full per-stage output is in the
TestImportMemorylog; the table shows the stages this PR moves.Tests:
TestFetchFeedFileLocal: a local feed is hashed in place with the expected hash.TestFetchFeedFileDownload: the auth header is sent, the file lands next to the database andcleanupremoves it; a 403 leaves no file behind.TestFetchFeedFileTruncatedDownloadLeavesNoFile: a server that promises more bytes than it sends; the fetch fails and the database directory is empty.TestFetchFeedFileServerUnreachable: a closed server gives the download error.TestFetchFeedFileRemovesStaleDownloads: a stale download next to the database is removed before a new one starts.TestDownloadDir:"",:memory:and a file path.TestFeedHashIncludesGTFSTidy,TestParseHashMarksTidyOnlyWhenItRan,TestParseHashMarksTidyWhenItRan: the+gtfstidymarker is added only when tidying ran (the success case uses a fakegtfstidyonPATH).TestHasImported: empty database, same feed, other source, other hash.TestReloadStaticSkipsParseForImportedFeed: a reload of the same feed does not parse again.TestReloadStaticPrecomputesOnlyWhenChanged: directions are precomputed after a changed import and not after an unchanged reload.TestReloadStaticUnchangedFeedRefreshesExpiry,TestReloadStaticUnchangedFeedExpiryFailure: the skipped parse still refreshesfeed_expires_at; a refresh failure fails the reload without parsing.TestReloadStaticHasImportedError: a failing metadata query fails the reload with its error.Checklist
mainand passes on this branch.gtfsdb/(db.go,models.go,query.sql.go) ortestdata/openapi.yml(synced from upstream).r.Context()to DB/service calls, notcontext.Background().testdata/openapi.yml, has an issue open on OneBusAway/sdk-config, or is outside the SDK and the description says why.Notes for reviewers
Size. 660 added, 134 removed; 367 of the additions are tests. The two commits are separable in principle, but the second moves the code the first introduced, so splitting them would mean reviewing the same lines twice.
Compatibility of the hash.
+gtfstidy, only when tidying succeeded. Turning tidy on or off still forces a re-import. A database imported with tidy on re-imports once after this change. A failed tidy is retried on the next reload instead of being recorded as done; while tidy keeps failing, every reload parses the feed again, as before this PR.Expiry refresh failure. A refresh that fails (for example an agency row with an invalid timezone) fails the reload, in both paths. That matches what #1686 does inside
StoreGtfsData.Follow-up. With gtfstidy on,
tidyGTFSDatastill copies the feed into the default temp directory; it should work in the download directory instead.