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