feat(npm): converge the packument cache miss on the fenced rebuild (S11 npm packument streaming generation plan: 3/7)
🎯 What this MR does
A cache miss no longer renders the packument in the handler. It runs the same fenced rebuild the write path dispatches, on the request goroutine, then serves the row that rebuild committed through the existing cache-hit path.
Step 3 of 3 in the npm packument streaming generation plan.
Steps 1 and 2 are merged, so this one targets main directly.
| Step | MR | What it delivered |
|---|---|---|
| 1 | !1646 (merged) | the streaming packument renderer |
| 2a | !1719 (merged) | the bounded reads the rebuild walks |
| 2b | !1720 (merged) | blob writes routed through a hashing tee |
| 2c | !1721 (merged) | the rebuild streams into its blob sessions |
| 2d | !1722 (merged) | session lifetime and the two-session bound |
| 2e | !1723 (merged) | the fence re-checked per page |
| 3 · this MR | the cache miss converges on that rebuild |
🧩 Why this is the step that closes finding 01
Step 2 made the background rebuild stream page by page. The read path did
not change: on main before this MR, a cold GET still called
allNpmVersions and GeneratePackuments, so an unauthenticated cache miss
still loaded the package's whole version set into memory. That is the code
#241's
finding 01 names.
A miss now holds one keyset page, so inlineBuildMaxInFlight becomes a memory
bound (slots x O(page)) rather than only a concurrency bound, and
NewPackumentHandler stops taking a version reader, a tag lister and a base URL.
The materialized generator stays, and that is a rebase correction, not an
oversight. This MR was written when GeneratePackuments, GenerateDistTags,
buildVersions, resolveDistTags, latestVersionID, Packuments and
PackumentInput went callerless once both miss paths stopped rendering, and it
deleted them. They are not callerless on today's main:
internal/format/npm/hosted_document_read.go, merged meanwhile for #889 (closed)
(S31 Step 8), calls them from HostedDocumentReader.render. Rebasing restored
all seven in internal/format/npm/packument.go, with their doc comments
rewritten to name that one remaining caller. Nothing about the served hosted
read path changed: the packument GET's cache miss still converges on the fenced
rebuild, which is what finding 01 asks for.
What that leaves open is tracked in
#966.
That seam has no production caller while git grep NewHostedDocumentReader
finds only its own file and test, so the materialized build is unreachable
today; #894 (S31 Step 13) is the change that wires it, and #966 asks for
buildInline to be re-expressed on the fenced fill before then.
It also closes the ETag window Step 2 opened. Both paths now serve the same keyset-ordered bytes, so a miss and a hit agree on the validator.
🔍 Worth a close read
A supersede is retried once, then answered. A writer rotating the fence
mid-fill is routine, not exceptional: enqueueRebuildAfterWrite retires the
singleflight key, so every write dispatches its own rebuild. The miss therefore
re-reads the cache after a refusal and retries only if nothing committed. A
second supersede answers 503 inline_build_superseded, a new code that joins
inline_build_capacity_exceeded (a fill the cap refused to start) and
inline_build_timeout (a fill that ran out of budget). Its Retry-After is the
shed's few seconds, not the timeout's budget-sized wait, because a supersede
clears as soon as the superseding writer's own rebuild commits.
The dist-tags blob-missing self-heal is now bounded. serveCacheHit falls
through to the fill when a fresh kind=1 row's blob reads back
ErrBlobNotFound. That terminated by construction before, because the inline
build opened no blob. Now the fall-through ends in serveCacheHit, so the two
call each other and storage that keeps reporting ErrBlobNotFound would recurse
them, each turn a full version walk and three blob writes on the request
goroutine. One attempt per request, then 500.
Rebuild outcome counting moved into RebuildPackumentCache. ok, error,
superseded and timeout now count one attempt each whichever path invoked it,
so a synchronous miss is indistinguishable from a dispatched refill. dropped
follows the same rule for one of its two sources: a rendering-cap shed returns
errRebuildShed, which rebuildOutcome maps to dropped on whichever
goroutine ran the rebuild, so a miss that waits out its budget there meters
dropped while answering its client a 503. Only a dispatch-time shed (metered
directly in enqueueRebuild) and panic are the worker's alone. The
named-return guard is load-bearing: a panic unwinds through the new defer, and
without it the same panic would meter as both ok and panic.
internal/format/npm/metrics.md carries the per-label ownership, because a
dropped spike is no longer evidence that only background work was refused.
An inherited shed answers the over-budget 503, not a 500.
rebuildCacheForMiss joins the same per-package group the dispatched worker
leads, so a routine post-write miss collapses onto the write's rebuild. When
that leader waits out its own budget at the rendering cap, the joiner receives
errRebuildShed with its own buildCtx still live, so
collapseInlineBuild's budget check cannot classify it.
inlineBuildOverBudgetErr is now the classification both handlers key their
inline_build_timeout 503 on, and it stays wider than the
inlineBuildTimedOut counter, which still counts only the builds this package
abandoned itself.
The response write deadline covers the store phase, not just the render. A
fill's store phase runs on a session context rebuildSessionGrace past the
render budget, so it can commit at up to inlineBuildTimeout + rebuildSessionGrace (60s), while the arm was inlineBuildTimeout + inlineBuildResponseGrace (35s). A fill landing in that window warmed the cache
and then failed every flush, handing the leader and each blocked waiter a dead
connection instead of the 200 it had just earned. The arm is now the three-term
sum (65s), and the deadline tests assert against a budget derived from the
rebuild's own two constants, so dropping the grace term fails them.
A client that leaves during its own fill gets a 499. The fill is detached,
so it completes and commits after the client hangs up, but the serve that
follows runs on r.Context(). Both handlers now guard that serve on
writeIfClientClosed, the helper the remote read paths use, so an abandoned
miss no longer books an ERROR line and an internal_server_error per
disconnect during exactly the cold bursts where fills run long.
⚠️ Accepted costs
- Cold-miss availability is now coupled to object-storage writes. A miss serves only once the blob commits and the fenced upserts succeed, so storage degradation turns cold reads into 500s where it previously only left the cache cold. Accepted because the unauthenticated memory DoS is the standing risk and cache hits already depend on storage reads.
- Cold reads are sensitive to the write rate on their own package. While writes land faster than a rebuild completes, every attempt supersedes and the miss answers 503 until the writes pause. Self-healing, and reachable only by someone who can write to the package.
- The widened write deadline is not sized for a large body. The first two costs are the plan's; this one comes out of review. 65s covers the latest a fill can commit plus the fixed tail before the first byte, but a packument that took most of its budget to render is also a large document to stream, so a write starting that late can still outrun the 5s margin. Bounding the read path's store phase to the response budget closes it by construction and changes when a cold read may succeed, so it is deferred to #952.
🧪 Tests
packument_superseded_test.go(new): the retry, the give-up 503 with its headers, and the re-read that serves the superseding writer's row.packument_fill_test.go(new): the miss-path harness.fillStoreis a real in-memorynpm_metadata_filestable the rebuild writes and the handlers read, so a "the miss filled the cache and served the row" assertion runs the production sequence rather than a scripted one.packument_disttags_internal_test.go(new): the dist-tags binding rules and the{}byte pin thatTestGenerateDistTagscarried, kept againstbindDistTagsnow that their old subject is deleted.TestRebuildPackumentCache_MetersItsOwnOutcome(new): the metric move.TestPackument_MissFillsTheCacheAndServesTheRowreplacesTestPackument_InlineBuildNoBlobOpen, whose premise inverted: a miss now opens exactly one blob.TestPackument_MissThenHitServeIdenticalBytespins the miss and the hit agreeing.TestDistTagsHandler_CacheHit_BlobGone_RebuildsOnceThen500replaces..._RebuildsInlineas the bounded-attempt case.TestPackument_InheritedShedIs503and its dist-tags twin seat a leader with a budget of its own in the group the miss joins, then assert the abandoned-build counter stays flat, so a request that led its own build cannot pass by answering the same code for a different reason.TestPackument_ClientGoneDuringFillIs499drives the abandoned miss through a cache seam that honors its context, because the fill fixtures ignore cancellation and would otherwise serve a departed client a 200.packument_test.gois deleted with its subject. Every case it held has a counterpart inpackument_render_test.gofrom Step 1, exceptTestGeneratePackuments_FullRevIsLatestVersionID, whose counterpart Step 2 added (TestRebuildPackumentCache_RevIsTheValueTheRevisionQueryReturned), and the Step 1 equivalence test, which goes with its comparator and leaves the golden-byte fixtures as the byte-level pin.
One behaviour change a test now records. In
TestDistTagsHandler_Singleflight_CanceledLeaderDoesNotFailGroup the canceled
leader used to get a 200 from bytes it had already built. It now gets a 499: its
fill still completes on the detached context and still fills the cache for every
waiter, but serving needs the request's own context for the blob read, and that
one is canceled. The client is gone either way, and 499 says so without charging
this service a 5xx.
📏 Size
1729 insertions, 2695 deletions, past the 500-line guidance in
development-model.md. It does not split. Roughly
620 lines are production, and most of the rest is the deletion of
packument_test.go (1051 lines) plus the test rework the deletion forces. The
production halves cannot land separately either: the generator's callers and the
generator have to go in one commit or main is left with dead code that the
unused linter fails on, and the new 503 code must reach
internal/metrics/cardinality.go in the same MR or the cardinality audit rejects
it on first emission.
✅ Testing
go test ./internal/format/npm/... ./internal/metrics/... ./internal/datastore/...green.- Every test in
disttags_test.go,packument_get_test.goand their new siblings also run green in a process of their own. That check earned its place: the rebuild dependencies are package-level, so several dist-tags tests were passing only because a parallel sibling had left them wired, and they fail alone. Each such test now wires its own and is marked non-parallel. golangci-lintclean, including a--build-tags=integrationpass overinternal/format/npm/...andinternal/managementapi/....scripts/ci/check-comment-caps.shclean.-tags=integrationrun against a local PostgreSQL:internal/format/npm/...andinternal/managementapi/...both green as of b1c729e7. The first run found the three packument read harnesses never callingSetRebuildDependencies, so every cache-miss read answered 500 (16 failures). That commit wires them and moves the miss-path expectations onto the fill, per the plan's Step 3 Tests entry: the "inline build opens no blob" assertions become one OpenBlob, and the cases that seed a tag or version change directly now carry the force-expire their real handler commits.
🚧 Not done yet
- The S11 spec sweep. The plan's Step 3 entry lists every place
docs/specs/S11-npm-hosted.mddescribes the inline-build miss mechanism, and none of them are edited here. That is the largest remaining piece. - The npm conformance suite (
mise run conformance:npm) has not been run.
🧪 e2e scenarios
None change. docs/testing/e2e/npm.md covers user journeys, and installs still succeed on cold and warm packages; no merged scenario asserts the server-side miss mechanism this MR changes.
Related to #241