feat(npm): shed the rebuild when the store refuses an upload session (upload-session-concurrency-cap plan: 10/12)

Folds the store's upload-session refusal into errRebuildShed and corrects every site that writes down what that sentinel means.

openRebuildBlob now recognises storage.ErrUploadSessionsBusy and sheds over it instead of returning a fault, so a refused session meters result="dropped" rather than error, and a cache miss waiting on that rebuild answers 503 inline_build_timeout rather than a 500. The refusal logs one WARN naming the package, the kind and the cause, which on the detached path is the only record of the package left stale.

The read path does not re-dispatch a rebuild on this shape. enqueueRebuild is justified by "an exhausted budget can mean contention a later attempt clears", and on this shape a later attempt does not clear it: a refill re-runs ForceExpireNpmMetadata, which has no kind filter, so every refused attempt expires all three kinds again and parks on the same full gate while holding a rendering slot. The gate does raise the same sentinel for an acquire past the whole capacity, which no retry clears, but this arm's single-unit open cannot ask for more units than the gate holds, so the refusal it can see is contention. storeRefusedRebuildErr splits the two shapes.

Size

929 reviewable LOC at 42e56b025, past the 500 ceiling development-model.md sets. Splitting does not help: the arm and the prose describing it have to land together, or the doc claims are false on main for as long as the two are apart. The split by group is 90 lines production Go, 543 test, 296 Markdown, so the reviewable surface is much smaller than the total. 400 of the test lines are one file, packument_cache_shed_internal_test.go, and most of it is the shed suite moved out of packument_cache_internal_test.go rather than new assertions.

What this does not reach

The arm sits at openRebuildBlob's NewSession call, and openRebuildBlob is the one function all three opens call, so the arm covers all three. TestRebuildPackumentCache_ShedsWhenTheStoreRefusesASession pins the shed at each of them. What belongs to one kind is not the arm's reach but where the sentinel can come from. Step 11 will charge the dist-tags session its own unit inside NewSession, so NewSession will be what refuses that open. The full and abbreviated sessions will be handed a reservation acquired in a single call before the first open, so NewSession cannot refuse them and their refusal arrives from that acquire, a call openRebuildBlob does not make. Folding that one belongs to whichever function ends up holding the acquire, and until it lands a refused reservation travels bare, meters result="error" or "timeout", and answers a read the generic 500. packument_cache.md says this at the point it describes the arm, and metrics.md points at it.

That single unit is also what the gate's two refusal conditions reduce to here. A one-unit acquire cannot exceed a capacity newUploadAdmissionGate keeps positive, so the over-capacity refusal is out of reach on this path and the reachable one is the spent wait budget. packument_cache.md and docs/runbooks/npm-packument-cache-invalidation.md both say so, and both say the generic 500 is what an operator sees while the gate stays full.

Nothing here is reachable in production yet. No production caller invokes the gate's Acquire, so NewSession raises no refusal and this arm cannot fire.

One site keeps the refill summary unqualified and this branch leaves it there. writeInlineBuildTimeout's doc comment (internal/format/npm/inline_build.go:242-244) says the packument path dispatches a refill and the dist-tags path does not, without the store-refused exception storeRefusedRebuildErr carves out. scripts/ci/check-comment-caps.sh charges the whole 20-line block the unexported cap of one line as soon as any of it is touched, so the qualifier is a block compression rather than a one-line edit. The block already hands the shape decision to its call sites and packument_get.go carries the guard, so the summary misleads only a reader who stops there. inline_build.go appears in one plan step's Files list, Step 4's, which is behind this one, so no later step is scheduled to reopen that block and the compression is owed by whoever next edits it.

Corrections to the plan

Four, all owed a docs(plans) amendment rather than an edit from this branch, which guardrail 4 bars.

Step 10's Acceptance says a shed at the first session open "leaves the cache row exactly as it found it, asserted on the row". That is false and this branch asserts the opposite. ForceExpireNpmMetadata runs unconditionally before every session open, so no refused open leaves the row untouched.

Step 10's Files list still carries the rebuildMaxRendering clamp, which its own Scope, Acceptance and Forecast all move to Step 11. This branch implements the three-out-of-four reading and does not touch the clamp.

Step 10's Acceptance sets no refill policy. It says only that "the inline-miss path serving the same package answers 503 and books the existing outcome", and nothing in the plan decides whether the read path re-dispatches on the folded refusal. This branch decides it at the first opportunity and skips the refill, in PackumentHandler.writeReadError at internal/format/npm/packument_get.go:281. That is a third behavior deviation, and the reason it takes is the second one above: a refill pays the kind-blind force-expiry again and parks on the same full gate.

Step 10's Files and Tests entries name three source files and one test file: packument_cache.go, packument_cache.md, metrics.md and packument_cache_internal_test.go. The branch touches twelve more, because the sentinel's meaning is written down in more places than the plan's research found: docs/dev/observability.md, docs/dev/storage.md, docs/runbooks/npm-packument-cache-invalidation.md, docs/specs/S11-npm-hosted.md, internal/format/npm/hosted_document_read.go, internal/format/npm/hosted_document_read_stage_internal_test.go, internal/format/npm/inline_build.go, internal/format/npm/packument_cache_shed_internal_test.go, internal/format/npm/packument_get.go, internal/format/npm/virtual_read_errors.go, internal/format/npm/virtual_read_errors.md and internal/format/npm/virtual_read_errors_test.go. Two of those are this round's: the S11 Error Cases row said two situations answer inline_build_timeout and that both refill, which the fold falsifies on both counts; and the shed suite moved into its own file, which the Tests entry's single (Modify) does not cover.

Merge order

Re-derived at 94498cc9e, after merging origin/main into this branch. git diff --name-only main...HEAD names sixteen files and overlaps four open merge requests. The merge moved the merge base to 3411f6e59, and a moved merge base is what can change a conflict scan, so every answer below was re-checked against it. None of them moved.

!2514 (merged), Step 7 of this same plan, shares docs/dev/storage.md. It merges cleanly against main and conflicts with this branch, so neither order is blocked and whichever lands second resolves that one file.

!1598, !2496 (merged) and !2545 (merged) each conflict with main as well — all three in docs/dev/observability.md, and !2496 (merged) in seven further files — so each needs a rebase whatever this branch does and the order is not this branch's to set.

e2e scenarios

No scenario is added or affected. The arm is unreachable until an acquire is wired inside NewSession, so there is no behaviour an e2e run can reach. The scenario belongs with the step that wires it.

Related to #1035

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading