feat(npm): bound a rebuild's open blob sessions and outlive its budget

🎯 What this MR does

Closes two bounds part 3 left open, both consequences of a render that holds blob sessions across its whole walk rather than opening one per kind and closing it.

Part 4 of 5 of Step 2 in the npm packument streaming generation plan. Depends on !1719 (merged), !1720 (merged) and !1721 (merged) — merge in that order.

📚 The stack:

Part MR What it delivers
2a !1719 (merged) the bounded reads the rebuild will walk
2b !1720 (merged) blob writes routed through a hashing tee
2c !1721 (merged) the rebuild streams into its blob sessions
2d · this MR 👉 this one session lifetime and the two-session bound
2e !1723 (merged) fence re-checked per page

🧩 The three changes

Sessions outlive the rebuild's budget by rebuildSessionGrace. storage.Session.Cancel refuses outright once the context the session was opened with is done, returning before it touches either the staging object or the upload_sessions row. Opening the sessions on the rebuild's own rebuildTimeout-bounded context therefore made a budget expiry mid-render — the one failure the budget exists to cause — the single case where nothing is reclaimed. Nothing collects those: ADR 025's gc:staging-sweep is still proposed.

The grace is measured from the budget's deadline, not from the call. A window opened at the call would close during the render on any rebuild whose reads are quick, which is the leak arriving earlier and on exactly the packages streaming was built to serve.

The store phase moves onto that same context. Session.Commit runs on the context its session was opened with, so leaving the fence pre-check and the upsert on the budget put a deadline boundary in the middle of the three steps: a budget expiring during the Commit's storage move let the commit finish and then refused the upsert, stranding a committed blob that nothing references and nothing reclaims (#498).

The streamed kinds move into their own function, and that is a memory bound. Committing a session does not release its driver-side staging buffer — pgSession keeps its writer and the GCS writer keeps the chunk-sized slice it allocated at open — so a rebuildBlob still reachable from the caller's frame keeps 64 MiB alive with it. Holding the two streamed kinds in a function that returns before the dist-tags session opens is what makes the two-open-sessions bound real rather than nominal, and takes rebuildMaxRendering's ceiling from three buffers per rebuild to two.

📊 Worth knowing

A worker's real lifetime is now rebuildTimeout + rebuildSessionGrace, not rebuildTimeout, because the store phase and the teardown both run inside the worker on the grace context. The capacity arithmetic pairing the timeout with the in-flight cap derives from that sum, so every comment that asserted the old bound moved with it.

rebuildSessionGrace is 30 s, derived from pg_session.go's cleanupTimeout rather than picked. The grace covers the store phase plus up to two Cancel calls, and cleanupTimeout gives one session's identical detached teardown 30 s; internal/remote/fetch.go already mirrors that constant for the same class of work. A worker therefore lives up to 60 s, which is the window rebuildMaxPerPackage now states.

The reasoning behind the caps moved into a new internal/format/npm/packument_cache.md, next to metrics.md: the occupancy model, the grace sizing, and the chunk-size ceiling arithmetic that the comment-caps trim had left with no home in-tree. Each affected Go block is one line pointing there, because editing any of them in place makes check-comment-caps.sh require that whole block be cut to its cap, 59 lines down to 1 for rebuildMaxPerPackage.

Deliberately not done here: the file header's two pointers at lines 10 and 16, and packument_get.go:96. Both sit inside blocks the same gate would collapse, 48 lines in the header's case.

📏 Size

Roughly 690 reviewable LOC. Production Go is about 130 lines and shrank by 125 more as comment blocks moved out; the new packument_cache.md is 230 lines of relocated prose, and the tests are the rest. Splitting would separate a constant from the doc that sizes it, so it stays together.

Testing

  • Unit, -race, and integration suites green; golangci-lint 0 issues, including a --build-tags=integration run.
  • npm conformance (mise run conformance:npm) passes on the head of this stack.
  • Covers the grace anchored on the budget's deadline rather than the call, budget expiry mid-render still reclaiming its sessions, at most two sessions open, and a dist-tags session failure leaving the streamed kinds stamped.
  • No e2e scenario is added or affected: the grace is an internal deadline on a detached rebuild goroutine, not a request-path behavior the catalogs in docs/testing/ describe.

Related to #241

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading