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 asserts 1 for all three sentinels. Against the old code it fails with expected: "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 with expected: "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.md around lines 422 and 1085; this MR edits 850 to 903.
  • !1807 (merged) edits internal/format/npm/remote_tarball.go inside resolve, around line 440; this MR edits the file preamble, writeReadFailure, remoteTarballBackpressure's doc, and writeUnavailable. The preamble edit shifts every line below it by six, which is a pure offset and not an overlapping hunk.

Related to #825 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading