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.

📚 The stack:

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 👉 this one 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. fillStore is a real in-memory npm_metadata_files table 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 that TestGenerateDistTags carried, kept against bindDistTags now that their old subject is deleted.
  • TestRebuildPackumentCache_MetersItsOwnOutcome (new): the metric move.
  • TestPackument_MissFillsTheCacheAndServesTheRow replaces TestPackument_InlineBuildNoBlobOpen, whose premise inverted: a miss now opens exactly one blob. TestPackument_MissThenHitServeIdenticalBytes pins the miss and the hit agreeing.
  • TestDistTagsHandler_CacheHit_BlobGone_RebuildsOnceThen500 replaces ..._RebuildsInline as the bounded-attempt case.
  • TestPackument_InheritedShedIs503 and 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_ClientGoneDuringFillIs499 drives 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.go is deleted with its subject. Every case it held has a counterpart in packument_render_test.go from Step 1, except TestGeneratePackuments_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.go and 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-lint clean, including a --build-tags=integration pass over internal/format/npm/... and internal/managementapi/....
  • scripts/ci/check-comment-caps.sh clean.
  • -tags=integration run against a local PostgreSQL: internal/format/npm/... and internal/managementapi/... both green as of b1c729e7. The first run found the three packument read harnesses never calling SetRebuildDependencies, 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.md describes 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

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading