feat(npm): serve the packument read path of a remote repository (S15 plan: 12/16)

🎯 Summary

Adds npm.RemotePackumentHandler, the GET/HEAD packument read for a kind=2 repository, and the composition root that mounts it. This is the first npm remote route to leave the dispatcher's interim 501.

📋 S15 Step 12 of the npm remote plan · tracked as #347 (closed) 🔗 Blocked by !1685 (merged) (step 10) and !1683 (merged) (step 11), both merged · Blocks !1674 (merged) (step 13), !1686 (merged) (step 14)

♻️ Re-cut against main after !1685 (merged) and !1683 (merged) merged. The previous head carried a stale copy of step 10 (npm.RemoteProxy and its two suites, 1096 lines) that duplicated the merged npmremote.FlightRegistry. The branch is rewritten so that copy never lands rather than being added and deleted; remote_proxy.go does not appear in this diff at all.

🧩 The three pieces

1️⃣ One URL, two documents

The client's Accept selects the full or the abbreviated variant. The header parse is delegated to negotiatePackumentKind, the same RFC 9110 token walk the hosted packument read uses, so the two paths cannot disagree about which Accept selects the abbreviated form. All Accept field lines are joined into one comma-separated list per RFC 9110 sections 5.2-5.3: Header.Get would see only the first line, hiding the abbreviated type from clients that send one media type per line. The target kind is then named explicitly rather than relying on the two enumerations happening to share their numbering.

2️⃣ Two serve shapes, and a third answer

A fresh cached document is served with no upstream call. A stale or missing one is fetched under the variant's canonical media type, rewritten in-stream so every dist.tarball points back at this repository, teed into the cache, and streamed on. The upstream body is read through io.EOF, which is what commits the cache fill: a consumer that stopped short would serve every byte and still abandon the fill.

BaseURL for the rewrite is assembled from the DB-resolved slug and repository name, never from raw request-path bytes, because it is embedded verbatim in every rewritten dist.tarball and then persisted in the cached document.

The third answer is 304. A cache serve whose validator matches the request's If-None-Match short-circuits with no body, closing the ServeResult on the way out because remote.Standalone has already opened the blob by the time the source is known. Every response carries Cache-Control: private, max-age=0, so a client must revalidate on each request; without the short-circuit that revalidation re-downloads the whole packument every time, and the ETag the branch advertises would name a representation no request could ever be spared.

A cache serve also reaches the redirect branch when the deployment is configured for redirect delivery, exactly as the hosted npm reads reach theirs. The pre-signed URL rides only in Location, never in an HTML body as http.Redirect would emit, so it cannot leak into proxy, CDN, or log body capture; nosniff is defense in depth, and no ETag travels because an ETag describes a representation and a redirect carries none.

3️⃣ Headers the handler owns

No upstream response header is copied. The fetch layer keeps the upstream map unexported behind SafeHeaders precisely so a proxy read path cannot forward one, and this path reads none of it. What it does set: the variant's Content-Type, Vary: Authorization, Accept on every response including errors, Cache-Control: private, max-age=0, and — on a cache serve only — the served bytes' content address as ETag plus a Content-Length so a client can detect a blob read that truncates the document mid-stream. An upstream stream can declare neither, because neither its length nor its content address is known when its headers are written.

The package name is judged at the HTTP boundary, before any cache read or upstream request. An unvalidated name would otherwise reach the cache query, the outbound URL builder, and the log fields, turning a clean 422 into a 500 or a misleading upstream 404.

🚦 What a failed read answers

Every failure is mapped through !1683 (merged)'s writeRemoteProxyError with package_not_found as this route's not-found code, rather than flattened into a generic 500.

Condition Status Code Retry-After
Upstream 404 404 package_not_found
Upstream 401/403/429 relayed upstream_rejected
Upstream 500 500 internal_server_error
Upstream 503 503 upstream_unavailable
Upstream other 5xx (502, 504) relayed upstream_unavailable
Transport failure, nothing cached 503 upstream_unavailable
Transport failure, cached copy exists 200 serves the cached document
Remote gated off as unhealthy 503 upstream_unavailable
Coalescing wait timed out 503 upstream_unavailable
Document past max_remote_packument_size, fetching request 200 truncated
Document past max_remote_packument_size, coalesced follower 503 upstream_unavailable
Client already disconnected 499 no envelope

The distinction is not cosmetic. npm retries a 5xx (fetch-retries, default 2, 10-60s backoff) and treats a 404 as terminal, so a miss reported as a server error costs three upstream fetches, tens of seconds of hang, and fails an install that an optional or peer dependency would otherwise have skipped. Retry-After is derived from health_check.scheduled_interval and is scoped to the 503 status rather than to who chose it: every 503 this route maps carries it, a relayed upstream 503 included, while a relayed 502, 504, or 429 carries none. That is S15's invariant ("Every 503 upstream_unavailable response carries a Retry-After header"), and it is what Maven's MapFetchOutcome already does on its propagated-5xx arm. Maven diverges on the value: it prefers the upstream's own Retry-After where the upstream sent one, and this path cannot, because ServeResult.SafeHeaders is a closed allowlist holding no Retry-After. The relayed-503 corner is the one S31 answers differently; writeRemoteProxyEnvelope's doc comment records the disagreement and this route follows S15, the spec that governs it.

🔒 Two refusals worth naming

An unset npm.public_registry_url refuses the read. packumentBaseURL appends the slug and repository segments, so an unset base still yields a non-empty host-relative prefix and NewTarballURLRewriter's own empty-base check never fires. The rewritten dist.tarball values would be persisted into the cached document and survive the configuration being corrected. The guard is handler-local because packumentBaseURL has two hosted callers, and per request rather than at construction because the field is optional and a hosted-only deployment must keep booting.

A HEAD records no download. remote.StandaloneOptions gains SkipDownloadRecord for it. last_downloaded_at drives the retention sweep, and a monitoring probe or a CDN revalidating with HEADs would otherwise hold a row perpetually fresh with no client having received the bytes. The zero value still records, so a caller that does not think about retention keeps the accounting. One framing difference is deliberate and documented on ServeHTTP: a warm HEAD declares Content-Length and a cold one does not, because a document still streaming from the upstream has no known length.

🔧 Composition root

cmd/artifact-registry/wire_npm_remote.go builds one npmremote.FlightRegistry and one npm.RemoteOperationsProvider per process and threads an npmRemoteWiring through mountSlugAnchoredFormats into buildNpmDispatcher as a DispatchOption. A boot with no upstream client supplies the zero value and every remote route keeps its 501, which is what the DB-less unit stubs and the isolated wireNPM seam rely on — wire_npm_boot_integration_test.go still asserts that 501, now labelled as the unwired seam's contract rather than the route's.

One provider serves every slot the builder returns, which is what will put the packument, dist-tags, and tarball routes of one repository on one flight rather than one each.

npm.max_remote_packument_size stops being inert here, so internal/config/npm.go and docs/dev/configuration-reference.md are rewritten together. The reference row records that a cap breach is not answered from the cache, because remote.Standalone excludes the cap sentinels from its fallback set, and the public_registry_url row records that a remote read needs it.

Tests

Suite What it pins
remote_packument_internal_test.go One constructor refusal per required field; the Retry-After derivation including its sub-second floor; the two wiring-fault 500s; the unset-base refusal landing before the operations lookup (an unreachableOperationsFinder panics if it does not).
remote_packument_integration_test.go Cold miss, variant independence, fresh hit, stale 304 revalidation, stale 200 refill, coalescing, invalid name, header ownership, bearer never served, redirect delivery.
remote_packument_errors_integration_test.go New. Every arm of the table above, end to end.
remote_packument_conditional_integration_test.go New. If-None-Match exact, weak, list, and wildcard forms; the non-matching control; HEAD recording no download with a GET control beside it; multi-line Accept selecting the abbreviated variant through to the upstream Accept, the cache row kind, and the served Content-Type.
remote_packument_harness_integration_test.go New sharing test. Concurrent requests for one path across two handlers over one provider must produce exactly one upstream GET and one registry entry; two repositories through one registry must produce two of each. The previous assert.NotSame check compared two separately-constructed proxies and could not fail.
wire_npm_remote_boot_integration_test.go New. The route through the production mount against a real database: upstream 404404 package_not_found (with the upstream call count proving the proxy answered and not a hosted handler), transport failure → 503 + Retry-After, upstream 200 → the rewrite applied, and an unwired composition still on 501.
internal/remote/standalone_test.go SkipDownloadRecord withholds the write while still serving, with the recording control beside it.

Live verification. driver.sh smoke passes 25/25 in both ephemeral and fixed-port modes, including a new check that the remote packument route no longer answers 501. Driven by hand against the real registry.npmjs.org: cold read 200 with every dist.tarball rewritten to point back at the local registry, warm read 200 with ETag and Content-Length, conditional read 304 with an empty body, a two-field-line Accept selecting the abbreviated variant (8968 bytes against the full document's 23053), and HEAD 200.

Lint. golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false is clean on every file this MR touches. Three //nolint tokens carried over from the superseded harness (contextcheck ×2, gosec ×1) were removed and the tagged run re-measured: no finding appeared, so they suppressed nothing and are gone.

📐 Governing ADRs

  • ADR-009 API design — conforms. The proxy read builds its own response headers and never blanket-copies an upstream one; error responses use the npm envelope; a relayed status keeps the upstream's own code class.
  • ADR-005 Artifact delivery modepartial, pre-existing deviation. The cache serve honours the instance default, but no npm read path threads the per-namespace override ADR-005 requires be "always available": npm.Resolution carries no field for one, while maven.Resolution.DeliveryModeOverride does. Inherited from the hosted npm reads, not widened here. Tracked in #708 (closed).

⚠️ One spec divergence, tracked

A cold or refill serve carries no ETag, because the CAS blob has not committed when the headers are written. docs/specs/S15-npm-remote.md asks both served variants to carry one. Filed as #749 with the two designs that would close it (amend the spec, or produce a validator before the headers are written); the comment on the ServeFromUpstream arm points at it.

📏 Diff size

3425 insertions across 30 files, past the 500 reviewable-LOC line, so per development-model.md here is the split and why it is not further divided:

Group LOC Notes
Production Go 716 remote_packument.go 382, wire_npm_remote.go 205, the rest are ≤ 40-line edits to existing files.
Tests 2622 The harness (1010) and the behaviour suite (506) are new against main but were reviewed on the closed !1681 (closed); the genuinely new material is the three suites and the boot test.
Docs and run recipe 150 Configuration reference, e2e scenario catalog, driver.sh seeding and its SKILL.md gotchas.

Splitting the handler from its mount was the obvious cut and it makes things worse, not better: an unmounted handler is 492 lines of unreachable production Go, which is exactly the state the previous head was in and the first thing the review flagged. The mount is what makes every assertion in the boot test and the run recipe possible.

Related to #347 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading