historyarchive: bound the cache writer and cache only completed downloads

[karthikiyer56]

TL;DR

Three fixes in historyarchive:

1. The cache can no longer truncate what you're reading. Before: the caller read a file out of the cache while a goroutine filled it — so when the fill stopped early (size limit, error, closed reader), the caller's file silently ended mid-way, with no error. Now the caller reads the download directly, and the cache keeps a copy on the side. That copy can be abandoned at any moment and the caller never notices.
2. An archive pool shares one cache. N mirrors used to build N caches over the same directory — N× the size budget, and no member could see the others' files to evict them. Now there's one cache, built only after at least one mirror actually connects.
3. One bad request no longer panics scan/mirror, and no longer tricks repair into re-uploading a bucket that was never missing.

Two rules make the cache fix hold everywhere:

• Only whole files are ever served from the cache. A file mid-download is invisible to everyone but its downloader.
• Size limits mean "don't keep a copy" — never "cut the download short".

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:

• A miss hands the caller a reader over the upstream itself (cacheFillingReader); each chunk the caller reads is also written into the cache entry. The caller's data path never depends on the cache.
• When caching must stop — file outgrows MaxCacheFileSize, directory hits the 10 GiB budget, a cache write fails — the entry is deleted and the reader keeps streaming.
• While a path is mid-fill, concurrent requests bypass the cache and download their own copy. (fscache cannot error-out readers of an abandoned in-flight stream — they'd see a clean EOF — so isolation is the only safe option.)
• Gzip consumers stop reading right after the gzip trailer, before the raw io.EOF. Close on 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, so Close cannot hang.
• LRU eviction now trims to 8 GiB, deliberately below the 10 GiB budget. The two limits are measured differently (eviction skips open files and metadata), so sharing one number let a full cache evict nothing, permanently disabling caching.
• Cache-hit readers tolerate a double Close (the fscache reader underneath panics on the second one).
• Budget bookkeeping uses atomic counters, re-measures the directory at most once a second without holding a lock across the walk, and checks the budget every 4 MiB written instead of every Read.

One cache per pool, built only on success

• Every pool member used to build its own cache over the shared 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.
• Construction order matters: building the cache wipes CachePath and 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 failing NewArchivePool no longer wipes the directory or leaks a timer per attempt.

scan/mirror workers stop panicking — and stop misclassifying

• Worker goroutines in Scan and Mirror panicked the whole process when one request failed (has.Buckets(), BucketExists, CategoryCheckpointExists). They now feed the error accumulator sitting next to them and move on.
• De-panicking BucketExists exposed a second bug: the bucket was marked referenced before the failed check, which left it permanently classified "missing" — and repair would re-upload it. The reference is now forgotten on error, so the next checkpoint referencing the bucket rechecks it.

Behaviour changes

• Abandoning a stream mid-file discards its cache fill; the next request is a miss. (A file nobody finished reading shouldn't occupy budget.)
• Concurrent requests for a mid-fill path each download their own copy — bandwidth traded for correctness. The first completed fill serves everyone afterwards.
• A file too big for the cache, or arriving while the cache is full, is served in full and simply not cached.

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; Close is idempotent on hit and miss paths; a pool fetches each path once; a pool that fails to construct leaves CachePath untouched.
• historyarchive/archive_test.go: a bucket whose existence check failed is neither reported missing nor blocked from a recheck.

[Copilot]

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.

[Shaptic]

This seems like a complicated fix for a non-issue, but LGTM nonetheless.

[Shaptic]

How is this no longer fatal?

[Shaptic]

Why would a download be abandoned mid-write with any more likelihood than the direct fetch?

[Shaptic]

You can use support/collections/set for this

[Shaptic]

Idk what this even means - the path is the same across all members of the pool so there already is a single cache, no?