chore(npm): answer a staging-refused read with the shed 503 (upload-session-concurrency-cap plan: 8/12)
Step 8 of the upload-session concurrency cap: the npm read routes answer a
refused staging session as 503 upload_session_capacity_exceeded instead of
letting it fall through to their 500.
Three arms, all keyed on storage.ErrUploadSessionsBusy through errors.Is,
all reusing the publish route's code, message and jittered Retry-After
rather than minting constants of their own:
writeRemoteProxyError— the packument and dist-tags routes.RemoteTarballHandler.writeReadFailure— the tarball route, which reaches the shared mapping only on its received-refusal arm, so an arm added towriteRemoteProxyErroralone would leave this route on its500.virtualReadAnswerFor— thekind=virtualwinner's cache fill.
Both new writers go through envelope writers that already stamp
Cache-Control: no-store, and all three log at WARN with the cause passed
through redactedForLog.
What this does not reach
admission_source has six values and this step gives a status half to one of
them, remote_fill, on npm's read routes. Two npm-adjacent gaps stay open and
are named in docs/dev/storage.md's upload_session_admission_sheds_total
row rather than left to inference:
npm_rebuildhas no status half at all.openRebuildBlobopens that session and a refusal falls through toPackumentHandler.writeReadError's default500. Step 10 (#1240 (closed), !2513 (merged)) owns it.- The virtual arm is written but unreachable, for two independent reasons:
nothing calls
uploadAdmissionGate.Acquire, andwriteVirtualReadErrorhas no caller outsideexport_test.goand its tests. Wiring either alone leaves the arm unreachable.
The virtual arm deliberately excludes errHostedDocumentInlineBuild. S31
splits the two producers of the sentinel across two rows: the winner's cache
fill takes upload_session_capacity_exceeded, and a hosted position whose
rebuild is refused takes the inline_build_* row. openRebuildBlob raises
the same sentinel and HostedDocumentReader.fillCache wraps it
errors.Is-transparently, so without the exclusion this arm would answer a
refused rebuild with the fill's code. Step 10's fold does not fix that on its
own, because it wraps errRebuildShed with a second %w and the sentinel
stays matchable.
Merge order
This MR must merge before Step 11 (#1241), which wires the acquire into
storage.NewSession. Step 11 arms the sentinel's only producer; until the
read arms are in, a refused fill answers 500 with an ERROR record and no
Retry-After. The order is recorded on #1238 (closed) and is not implied by the plan
graph alone.
Two open siblings edit the same prose and conflict with this branch while
merging cleanly into main, so the order between them matters and no
pipeline reports it:
- !2513 (merged) (Step 10) — conflicts in
docs/dev/storage.mdandinternal/format/npm/metrics.md. The conflict is semantic: both rewrite the same per-format roster instorage.md, and !2513 (merged) narrows the twometrics.mdpassages to the publish path where this MR scopes them to the publish route plus a read route's cache fill. - !2514 (merged) (Step 7) — conflicts in
docs/dev/storage.md. This MR writes "OCI has no arm yet"; !2514 (merged) lands that arm.
Whichever merges second should re-derive the roster against the merged text rather than resolving the markers, because either side taken whole ships a false statement.
The stack
Every step targets main; merge order is step order. Steps 9 and 8 have
already merged out of order, which is why the order above is stated rather
than inferred from the stack's existence.
| Step | Issue | MR | State |
|---|---|---|---|
| 1 | #1231 (closed) | !2471 (merged) | merged |
| 2 | #1232 (closed) | !2408 (merged) | merged |
| 3 | #1233 (closed) | !2507 (merged) | open |
| 4 | #1234 (closed) | !2465 (merged) | merged |
| 5 | #1235 (closed) | !2462 (merged) | merged |
| 6 | #1236 (closed) | !2463 (merged) | merged |
| 7 | #1237 (closed) | !2514 (merged) | open |
| 8 | #1238 (closed) | this MR | open |
| 9 | #1239 (closed) | !2509 (merged) | merged |
| 10 | #1240 (closed) | !2513 (merged) | open |
| 11 | #1241 | not opened | — |
| 12 | #1242 | not opened | — |
Specs
S15-npm-remote.md— the staging-refusal row, which covers every standalone read route since packument, dist-tags and tarball all drive the fill.S31-npm-virtual.md— the winner's-cache-fill row, and the adjacent hosted rebuild row this arm is kept off.S11-npm-hosted.md— the code, the message rule, and the 35s ±20% window drawn per response, relayed unchanged.
Scope, against the plan's Files list
The plan scopes Step 8 to remote_read_errors.go and remote_tarball.go and
argues "Shared seams: none" from that two-file set. The realized set is
twelve files. The widening is deliberate and recorded on #1238 (closed): the scope note
of 2026-09-10 gives this step the kind=virtual arm, which no step's Files
list named. The rest follows from it — the sidecars that describe the arms,
and the two comment-only sites (internal/format/npm/metrics.go,
internal/metrics/cardinality.go) that carry the code's meaning. No new
Code, no new label, and "code" is still 41 in both requestCodeLabels
and closedSetValues.
The plan's "Shared seams: none" sentence is now stale, because
internal/format/npm/metrics.md is in Step 10's set. A step MR does not edit
the plan, so that sentence is left to the plan's single writer; the live
consequence is the !2513 (merged) conflict recorded above.
Size
800 reviewable LOC, over the 500 the development model asks to justify, and 3.1x the plan's "~260 total" forecast for this step. The split is 54 production Go, 532 test, 214 docs. Splitting would not help: the production change is three switch arms and one writer, and what the diff is made of is the per-arm tests and the sidecar prose that pays for them.
Testing
Every arm has a positive case driven four ways (bare sentinel, the real
production wrap remote: open cache-fill session: %w, a sentinel carrying a
cause, and both route policies), a neighbour control that reddens if an
adjacent arm moves onto the jittered window, and a budget test pinning the
code against the request metric.
Falsified rather than assumed, each in a scratch copy:
- Removing the virtual arm's
errHostedDocumentInlineBuildexclusion reddensTestVirtualReadError_StagingShedLeavesTheHostedRebuildAlone. - Replacing
uploadShedRetryAfterSeconds()with35in either new writer reddens its repeated-draw case. The shared bounds reader admits a flat constant by design, so the draw needed its own case on each writer. - Moving the shed arm ahead of
writeIfClientClosedin the tarball switch reddens the departed-client case.
No e2e scenario is added. Guardrail 12 binds feat and fix; this is a
plan-declared chore, the merged siblings !2465 (merged) and !2509 (merged) touched no catalog
either, and the sentinel has no producer to drive a scenario with until Step
11 lands.
Closes #1238 (closed)