historyarchive: bound the cache writer and cache only completed downloads - #5975
historyarchive: bound the cache writer and cache only completed downloads#5975karthikiyer56 wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Enforces canonical StrKey payload lengths and bounds/cancels history archive cache writes.
Changes:
- Adds fixed-length validation for StrKey validators.
- Cancels cache downloads when readers close.
- Adds per-file and total cache limits with tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
strkey/main.go |
Enforces canonical payload lengths. |
strkey/isvalid_length_test.go |
Tests valid, short, and oversized payloads. |
historyarchive/archive.go |
Adds cancellation and cache size enforcement. |
historyarchive/archive_cache_test.go |
Tests cancellation, idempotent closing, and limits. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Archive.cachedGet starts a goroutine that copies an archive response into the on-disk cache file, and returns a reader over that file to the caller. The two are independent: the goroutine runs to upstream EOF no matter what the caller does, and it has no size limit. Closing the reader does not stop it, so a response the caller rejected is still copied to disk in full. The 10 GiB LRU budget configured in Connect cannot apply to a copy that is still running either. lruHaunter.Scrub skips entries where InUse() is true, and the writer holds a handle for the duration, so a growing entry is counted in neither the size tally nor the reap list. Each download now gets a cancellable context. The reader handed back to the caller cancels it on Close, and the copy goroutine closes the upstream body on cancellation, since a blocked Read does not observe a context otherwise. Bytes written by in-flight copies are counted against the cache budget, so the configured size is enforced while a copy is running rather than only after it finishes. A new ArchiveOptions.MaxCacheFileSize caps any single cached file, defaulting well above the largest file the pubnet archive currently serves. Copies that are cancelled or exceed a limit return an error, which routes to the existing removal branch, so a partial file is not left as a valid entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NewArchivePool calls Connect once per URL with the same options, so every archive in the pool builds its own cache over the one CachePath. Each cache carries its own size budget, and each only knows about the files it downloaded itself. Two consequences. The directory's real ceiling is the budget multiplied by the number of archives rather than the budget. And because eviction only considers the files one archive downloaded, a pool splitting its downloads between members can hold a full directory while no single member's tally is large enough to evict anything, so the cache fills and then stops accepting new files. Sharing one cache across the pool gives it one budget and one view of the directory, so eviction sees everything. It also means a bucket downloaded through one archive is a cache hit on the others, instead of being fetched again once per member. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…king The worker goroutines in Scan and Mirror panic when a call fails, which takes down the whole process rather than the one checkpoint being worked on. Every one of these sites sits next to the error accumulator the surrounding loop already uses for its other failures. Route them through that accumulator and move on to the next checkpoint, which is how the rest of both loops already handle a failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cache fill no longer sits between the caller and the data. A miss now hands the caller a reader over the upstream itself, which copies each chunk into the cache entry as a side effect. When caching must stop -- per-file limit, total budget, write error -- the entry is discarded and the caller keeps streaming, instead of getting a clean EOF mid-file. Fills are private to the caller that starts them: concurrent requests for the same path bypass the cache, because in-flight fscache readers see abandoned fills as silent truncation. Since gzip consumers stop reading right before the raw EOF, Close on a still-active fill does one bounded probe read to distinguish "consumed everything" (commit) from "abandoned mid-file" (discard). The LRU eviction target now sits below the hard budget so the two differently-measured limits cannot wedge each other, cache-hit readers tolerate double Close, usage accounting no longer holds a lock across directory walks, and the pool builds its shared cache only after at least one archive connects, so failed construction cannot wipe the cache directory or leak eviction timers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ScanBuckets marked a bucket referenced before checking its existence, so a transient BucketExists error left the bucket permanently classified as missing: nothing ever marked it existing, and later checkpoints referencing it skipped the recheck. Repair would then re-download and re-upload a bucket that was present all along. Forget the reference on error so the status stays unknown and the next reference rechecks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0c1baa5 to
035fdbe
Compare
The only function that actually collides with the path package, ListCategoryCheckpoints, takes subpath instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9557064 to
be518fd
Compare
Shaptic
left a comment
There was a problem hiding this comment.
This seems like a complicated fix for a non-issue, but LGTM nonetheless.
| if err != nil { | ||
| panic(errors.Wrap(err, "error getting buckets")) | ||
| atomic.AddUint32(&errs, noteError(errors.Wrap(err, "error getting buckets"))) | ||
| continue |
There was a problem hiding this comment.
How is this no longer fatal?
| // Another goroutine is filling this entry — download independently | ||
| // rather than read a file that may be abandoned mid-write (see acquire). |
There was a problem hiding this comment.
Why would a download be abandoned mid-write with any more likelihood than the direct fetch?
| // Paths whose entries are being written right now. Only completed entries | ||
| // are ever served; see acquire. | ||
| fillMu sync.Mutex | ||
| filling map[string]struct{} |
There was a problem hiding this comment.
You can use support/collections/set for this
| // One cache for the whole pool: per-member caches over the same directory | ||
| // would multiply the size budget and hide each member's files from the | ||
| // others' eviction. |
There was a problem hiding this comment.
Idk what this even means - the path is the same across all members of the pool so there already is a single cache, no?
TL;DR
Three fixes in
historyarchive:scan/mirror, and no longer tricksrepairinto re-uploading a bucket that was never missing.Two rules make the cache fix hold everywhere:
Follow-up: #5976
The cache stores exactly what a mirror sent — it cannot tell a good file from a bad one. So a file that streams cleanly end-to-end but then fails its content-hash check still gets cached, and every retry is served that same cached copy instead of asking a different mirror. Fixing that needs a purge hook in the interface plus caller changes here and in horizon/rpc/galexie — spec'd out in #5976 rather than grown into this PR.
The cache fill no longer sits between caller and data
On
main, a cache miss returns a reader over the cache file while a detached goroutine copies the upstream into it — forever, with no size limit, no matter what the caller does. Just bounding that copy (this PR's first iteration) made things worse: every abort committed a partial file, and its readers saw a clean EOF at the truncation point.The redesign removes the coupling instead of managing it:
cacheFillingReader); each chunk the caller reads is also written into the cache entry. The caller's data path never depends on the cache.MaxCacheFileSize, directory hits the 10 GiB budget, a cache write fails — the entry is deleted and the reader keeps streaming.io.EOF.Closeon a still-active fill therefore does one bounded probe read: immediate EOF → the file was fully consumed → commit; anything else → discard. A timer closes the upstream if the probe stalls, soClosecannot hang.Close(the fscache reader underneath panics on the second one).Read.One cache per pool, built only on success
CachePath: budget × pool size, blind eviction, and each member re-downloading the same buckets. The pool now builds one cache and attaches it to every member.CachePathand starts an eviction timer that can never be stopped. The pool now connects first and builds the cache only if something succeeded — a retry loop around a failingNewArchivePoolno longer wipes the directory or leaks a timer per attempt.scan/mirror workers stop panicking — and stop misclassifying
ScanandMirrorpanicked the whole process when one request failed (has.Buckets(),BucketExists,CategoryCheckpointExists). They now feed the error accumulator sitting next to them and move on.BucketExistsexposed a second bug: the bucket was marked referenced before the failed check, which left it permanently classified "missing" — andrepairwould re-upload it. The reference is now forgotten on error, so the next checkpoint referencing the bucket rechecks it.Behaviour changes
Tests
historyarchive/archive_cache_test.go: readers can consume files past both size limits and past a full cache, untruncated; the cache stops growing when a reader closes or a stream is rejected; in-flight requests bypass the cache and survive an abandoned fill;Closeis idempotent on hit and miss paths; a pool fetches each path once; a pool that fails to construct leavesCachePathuntouched.historyarchive/archive_test.go: a bucket whose existence check failed is neither reported missing nor blocked from a recheck.