fix(npm): a coalescing 503 on the remote tarball answers Retry-After 1
What this changes
When several npm clients ask for the same tarball from a remote repository at once, one of them
fetches it from the upstream and the rest wait on that fetch. If the fetching client goes away, or if
the wait for a fill slot runs out, the waiting clients are answered 503 with a Retry-After header
telling them when to come back.
On the tarball route that header said 300: the health-probe sweep interval, at the 5m default. On
the packument and dist-tags routes the same condition answers 1. Nothing about the condition is
route-specific, and none of the three conditions records a health verdict, so no probe sweep has to
run before a retry can succeed. That makes 1 the right answer, and it is the value
docs/specs/S13-virtual-remote-foundation.md fixes for every format. This MR makes the tarball route
answer 1 too.
Issue #825 (closed) set out two ways to reconcile the difference. This takes the first: make the tarball route agree with S13 and with the other two routes.
What does not change
The tarball route still classifies the three conditions itself, ahead of the shared error mapping, so
its log line, its 503 and error code, and its deliberate refusal to serve a stale cached copy in
their place are all as before. Only the header value moves.
Spec
docs/specs/S15-npm-remote.md recorded the difference as two Error Cases rows plus a paragraph. They
become one row and two paragraphs: one recording the value all three routes now answer, one
answering the cost #825 (closed) named for it.
Tests
TestRemoteTarballHandler_BackpressureAndLeaderLoss_Return503(unit) now asserts1for all three sentinels. Against the old code it fails withexpected: "1",actual: "300"on each of them.TestMountSlugAnchoredFormats_NpmRemoteRoutesShareOneFlightRegistry(integration, composition root) now asserts the tarball follower and the dist-tags follower against the same constant. Against the old code it fails withexpected: "1",actual: "30".
The header is no longer what separates the two mappings in that integration test, since both
followers assert the same constant. The status still separates them: dropping the
remoteTarballBackpressure arm sends all three sentinels to writeReadFailure's default arm, which
answers 500, and both require.Equalf calls on the status catch that.
Run locally: go build ./..., go vet, go test ./internal/format/npm/, and both touched packages
with -tags=integration against a local PostgreSQL and Redis.
golangci-lint run --build-tags=integration over ./internal/format/npm/ ./cmd/artifact-registry/
reports nothing in the files this MR touches; the findings it does report are pre-existing ones in
other integration-tagged files.
Why prose is deleted in this diff
scripts/ci/check-comment-caps.sh caps every comment block a diff touches, and five blocks here sat
above their cap.
Three are routine: the shared remoteCoalescingRetryAfterSeconds doc,
RemoteTarballHandler.writeUnavailable's doc, and the composition-root test's doc comment. What they
explained now lives in the spec paragraph above and in the assertion messages.
remoteTarballBackpressure's doc is the one worth a reviewer's eye, because its cap is one line and
that costs the enumeration of the three sentinels and the remoteTarballFallbackEligible exclusion
rationale. The sentinels are the three errors.Is calls in the body. The exclusion is recorded in the
S15 Error Cases row, which says the condition is never served from cache, and in
internal/remote/errors.go. The clause that had to go either way said the three sentinels "are what
the shared mapping has no answer for", which writeRemoteProxyError has contradicted since before
this branch: its arms answer all three.
The fifth is one paragraph of the internal/format/npm/remote_tarball.go file preamble, which said
the shared surface puts "the health-interval Retry-After on every 503". This MR makes that false
for the coalescing one. Correcting it looked as though it meant rewriting all 51 preamble lines under
the 2-line cap, which would have deleted architecture notes that have nothing to do with this change.
It does not: the per-segment ratchet in docs/dev/go-style.md charges only the segments a diff
touches, so splitting the run at its paragraph boundaries brings the one corrected paragraph under
the cap on its own and leaves the rest of the preamble alone.
E2E scenarios
No scenario in docs/testing/ is added or affected. The two npm remote scenarios that mention
Retry-After, e2e.npm.remote.packument-upstream-unreachable and
e2e.npm.remote.tarball-upstream-unreachable, describe an unreachable upstream, which still answers
the sweep interval. The catalog has no coalescing-failure scenario: driving one needs a stalled
leader and a fill cap of one, which the unit and integration tests above hold rather than an e2e
flow.
Overlap with open MRs
Two open MRs touch a file this branch touches, neither in the same region:
- !1941 (merged) edits
docs/specs/S15-npm-remote.mdaround lines 422 and 1085; this MR edits 850 to 903. - !1807 (merged) edits
internal/format/npm/remote_tarball.goinsideresolve, around line 440; this MR edits the file preamble,writeReadFailure,remoteTarballBackpressure's doc, andwriteUnavailable. The preamble edit shifts every line below it by six, which is a pure offset and not an overlapping hunk.
Related to #825 (closed)