feat(npm): serve remote tarballs through the caching proxy (S15 plan: 14/16)

🎯 Summary

Adds npm.RemoteTarballHandler, the GET/HEAD tarball read for a kind=remote repository, mounts it on the dispatcher's remote download slot, and lands the spec amendment, the read-path and integration suites, and the run recipe that this step owns.

📋 S15 Step 14 of the npm remote plan, the handler half. 🧱 !1821 (merged) is stacked on this one and carries the mount. This MR merges first; the route answers the interim 501 until !1821 (merged) lands, exactly as main does today. 🔗 Supersedes the closed !1687 (closed) (read-path suite), whose content this MR absorbed. !1688 (closed)'s content is what !1821 (merged) now carries.

🧩 What the handler decides

1️⃣ Why not remote.Standalone

Corrected in review. This section previously said Standalone "exposes no way to suppress" the fresh-hit download record. That was false: remote.StandaloneOptions.SkipDownloadRecord does exactly that, and the sibling packument route already passes it (remote_packument.go). Thanks to @mkhalifa3 for catching it.

Four things this route does have no shape in Standalone's surface today, and the download record is not one of them:

Gap	Why the fork needs it
remote.ServeResult carries no content length	The relay declares the upstream's own, which is what ends a dying transfer early instead of framing it as complete
Standalone's cache serve always opens the blob	This route answers a matching If-None-Match from the row's content address with no storage read, and a HEAD through BlobInfo with no stream opened and no redirect signed
Standalone records the retention bump before it returns	This route records only after the body copied through in full, so a short transfer books no download
A status outside 4xx/5xx	Classified here as no usable response; Standalone propagates a 3xx as UpstreamReportsError

That is a fork of a state machine rather than a variant of one, and closing it is tracked in #779 (closed), now on milestone %19.4 and assigned, per the round-3 decision that it lands as its own MR. Widening StandaloneOptions and ServeResult by four shapes at once, in internal/remote, which the maven and OCI remote paths also compose, is not something this MR should carry. #779 (closed) records the merge position: it is independent of this MR and of !1821 (merged), so it holds neither up.

What is shared rather than re-derived, by calling the pieces Standalone composes: remote.RevalidationFor for the freshness verdict, remote.CacheFallback for the degraded read, remote.HealthStatusReader for the fetch-path gate, and this package's own writeRemoteProxyError for every client-visible envelope. What stays this route's own is the leader-loss retry and the backpressure partition that feeds its retry budget, which the shared surface has no equivalent of.

2️⃣ A truncated relay must not read as complete

The status commits before the first byte, so a copy that dies mid-transfer can only be logged. With no declared length, net/http frames the relay chunked and writes the terminating chunk when the handler returns, so the client reads a truncated tarball back as a well-formed 200.

The relay declares remote.FetchResult.ContentLength whenever the fetch resolved it, which ends a short transfer early so the client reports unexpected EOF. A length is declared only when the fetch knows it: -1 covers a chunked upstream, a body the client decompressed, and one whose declared length outran the response cap.

The undeclared-length half is now closed too, which the first revision left open and described as pre-existing. It is not pre-existing: nothing relayed a blob body on this route before, and chunked is the default framing for Node-based registries, so it is the common case. abortRemoteTarballRelay backdates the response write deadline on an undelivered relay, which denies the response its terminating chunk.

panic(http.ErrAbortHandler), the documented idiom and what httputil.ReverseProxy uses, does not work in this service, measured against the real middleware stack:

variant	what the client sees
return normally (today, before this change)	200, 65536 bytes, no error
panic(http.ErrAbortHandler) under LabKit's recovery	200, 65536 bytes, no error, plus a stack-trace ERROR log
panic(http.ErrAbortHandler) with no recovery middleware	connection aborted
backdated ResponseController.SetWriteDeadline	200, 65536 bytes, unexpected EOF

LabKit's httpserver.PanicRecoveryMiddleware wraps every handler innermost (httpserver/server.go:159-162) and recovers that panic without re-panicking (httpserver/recovery.go:27-47, v2.34.0), so nothing in internal/server.buildMiddleware can get outside it. Teaching it to re-panic is filed as #781, and abortRemoteTarballRelay's comment points there as the change that deletes it.

The abort flushes the committed head before backdating, and that is what makes the promised outcome hold for a short tarball. io.Copy dispatches to (*http.response).ReadFrom, which relays the first sniffLen (512) bytes and only then flushes; a body that never reaches 512 is therefore still entirely inside net/http when the copy fails, and backdating the deadline first discards the status line and the headers along with it. Measured on this route: a 200-byte upstream body reaches the client as a bare EOF with no status line without the flush, and as the committed 200 plus unexpected EOF with it. A 1000-byte one was already fine, because the sniff copy had flushed. Both sides are rows of TestRemoteTarballHandler_UnframedTruncatedRelay_IsNotFramedAsComplete, each asserting its own premise against the boundary.

Corrected in round 3. This section previously called the undeclared-length restriction load-bearing, on the grounds that aborting a declared-length relay would lose a small body's response head. The head loss was real but is now fixed by the flush, and it was never specific to the declared-length framing. The restriction is an economy: net/http already refuses to finish a response whose body ran short of its declared length, so there is nothing left for the abort to add. The threshold is also 512 bytes rather than the buffer sizes, for the ReadFrom reason above. Thanks to @mkhalifa3 for finding it.

3️⃣ Rejections, refusals, and what is not a server fault

Three rejections are answered before any cache lookup or upstream request, each against the segment carrying the fault: a package name failing the npm charset is 422, a file name that is not one safe path segment is 400, and a safe name that is not {name}-{version}.tgz is 404.

An upstream refusal keeps the upstream's own status; no upstream header or body is relayed, so an upstream WWW-Authenticate never challenges the npm CLI for this registry's own credentials. A caller's own cancellation is stamped 499 and logged at debug. Fill-slot exhaustion, a twice-lost leader, and a remote the probe has already failed answer 503 with a Retry-After derived from health_check.scheduled_interval.

4️⃣ Validators and cache separation

A cache serve carries ETag: "sha256-<hex>" over the bytes it is serving, derived from the cached blob's content address and never the stored upstream_etag. A matching If-None-Match answers 304 before any BlobStore read. Vary: Authorization and an explicit Cache-Control ride every serve shape, including the relay. Last-Modified stays unset, because the row's creation instant is not carried through the lookup, so If-Modified-Since is not evaluated either rather than answered from a substitute.

🔍 What review changed

Area	Change
Error envelopes	Every one now carries Cache-Control: no-store. A bare relayed 404 is heuristically cacheable per RFC 9111, so a shared cache could pin an upstream miss past the package's publication.
Retry-After	Derived from health_check.scheduled_interval (300 at the default sweep), replacing a fixed 30 that returned clients ten times sooner than the health verdict can move.
Write deadline	Armed before any write. A follower can wait single_flight_wait_timeout behind a leader, well past the connection's 10s server.timeouts.write, and the 503 designed to stop the hammering was the response that never landed.
Health gate	A remote the probe failed answers 503 without dialling out or holding a fill slot. The packument route already gated; the two routes of one repository disagreed.
Failure bookkeeping	A relay or cache serve that broke after the status committed recorded no outcome code, so npm_request_total read a truncated transfer as code=success. Now booked against its source, with the written and declared byte counts.
Client disconnect	Stamped 499 rather than left unstamped, which the instrumentation defaults to 200.
Short cache copy	Compared against content.Size(); a short read is no longer reported delivered and no longer advances last_downloaded_at.
Log hygiene	Every logged error passes through redactedForLog; a transport failure arrives as a *url.Error that serializes its URL raw.

📏 Diff size

6,417 added, 97 removed across 25 files, against origin/main, at 30b8f3e0.

It was 7,849 before the split. The mount half moved to !1821 (merged) (1,802 lines, 8 files), which is the seam the closed !1688 (closed) drew:

Where	Lines	What
This MR	6,514	The handler, its four unit suites, and two review rounds' consolidations
!1821 (merged)	1,802	The dispatcher slot, wiring tests, the integration suite, the spec amendment, the run recipe

Still past the 500-line bound in development-model.md, so here is what remains and why it does not split further.

Group	Changed lines
Go tests	4,465
Go production	1,936
Docs	113

Tests are 69% of it (4,465 of 6,514). remote_tarball.go is 1,592 lines: 859 comment, 586 code, 147 blank.

Corrected in review. The first revision said "of the 1,379 production lines, 464 are comments", which was wrong by 212 at the time (the real split was 676/557/146) and the size waiver rested on it. Both figures above are produced by counting rather than by estimate. Thanks to @mkhalifa3 for checking the arithmetic.

Why it does not split further. What is left is remote_tarball.go (1,592) and its four unit suites. The only seam inside that is the one !1687 (closed) drew, separating the read-path suite from the code it covers, and !1687 (closed)'s own description admitted where it leads: "12b should not merge without 12c: the read-path arms are uncovered until it lands." In a stack the lower MR reaches main first, so that shape lands untested arms on main. The cached serve, the fill, the revalidation and blob repoint, and the HEAD arms also all share remoteTarballRead and the same serve-decision enum, so cutting between them lands half a state machine in each MR.

Splitting mount from implementation, which is what !1821 (merged) does, leaves both halves covered. That is the split that was available, and it is taken.

🧪 Testing

57 top-level tests, counted at 30b8f3e0: 21 + 18 + 13 + 2 across the four internal/format/npm/remote_tarball*_test.go suites, and 3 in internal/format/npm/npmremote/tarball_target_test.go.

• go build ./..., go vet, gofmt -l: clean.
• go test -race -count=1 ./internal/format/npm/... ./internal/format/maven/... ./internal/managementapi/... ./internal/metrics/... ./cmd/artifact-registry/...: green.
• go test ./...: green.
• golangci-lint run --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false over the same four package trees: 0 issues.
• The same command with --build-tags=integration: 134 issues, every one of them in a file this MR does not touch, 0 in any file it does.
• go test -tags=integration -race -run 'RemoteTarball|NpmRemote': green against a real PostgreSQL and BlobStore.

Every new test was mutation-checked: the production line it covers was deleted or inverted, the test confirmed red, and the line restored. That includes two mutations that add a retention bump where none belongs, so the four "records no download" assertions are not vacuous, and a positive control on the truncated-upstream test.

driver.sh smoke runs in !1821 (merged), which is where the recipe and the two new checks now live. It is 43 passed, 0 failed there, in both upstream modes, with the full output in that MR's description.

Worth carrying here because it settles a review thread against this MR: the first revision derived the new checks' expected statuses by reading the handler rather than by running them, and one was wrong for the workflow SKILL.md documents. Re-seeded against registry.npmjs.org the tarball check answers 404, not the hard-coded 503. The expectation now branches on the seeded upstream, and the packument check had the same defect and was fixed with it.

The count is 40 on main, 41 after !1674 (merged) added its dist-tags check, and 43 with this route's two.

🧭 E2E scenario impact

The catalog update travels with !1821 (merged), not this MR, and that is deliberate: every e2e.npm.remote.tarball.* scenario describes a route a client can reach, and until the mount lands none of them is runnable. !1821 (merged) adds nine of them plus an install-from-upstream journey, and removes the two sentences saying this route was not mounted and still answered 501.

Two scenario edits there are about behaviour this MR changes, so they are called out for the reviewer of both:

• e2e.npm.remote.tarball-truncated-relay no longer says a chunked upstream is a known gap. It was true when written and this MR's relay abort closes it.
• The usage-data disclosure gains the issue link it never had, #783.

The dist-tags exclusion stays: that route is still on its interim 501.

⚠️ Notes for the reviewer

• internal/metrics/cardinality.go moves the column budget 10 → 11, and now pins the label's exact value set. Raising the count on its own enforces nothing, which the first revision's own comment conceded: the audit's count bound is per-gather and every AuditCardinality call site is a per-package test, so the largest gather any of them produces is npm's seven against a budget of eleven. column is pinned in closedSetValues alongside code and OCI's error_code, so a twelfth value fails the audit on its first emission. Maven and managementapi gained the audit test they never had, and two tests hold the budget, the pinned set, and the four owning packages' declared columns to the same eleven.
• !1674 (merged) will conflict on cmd/artifact-registry/wire_npm_remote.go when it appends the dist-tags slot. Expected and unavoidable; the plan names the file as shared.
• The fill-slot budget is shared across the two routes, and the wiring comment now says what that costs. A fill slot is held for the whole client transfer, so a cold-tarball burst can occupy the repository's budget and a packument read behind it answers 503, failing the install at the metadata step. Recorded on #756 alongside the abandoned-fill cycling the same budget admits.

🔁 Review round 2

26 blocking threads from @mkhalifa3. Everything below is in this revision; the threads carry the per-item detail.

Corrections to claims this MR made. The Standalone rationale (false, see above), the comment-count figure (off by 212), metrics.md's stanza saying a budget raise would resolve the union, and the Cache-Control: private comment implying the response is not storable at all. Three sibling comments outside the diff still called this route future work; all three are fixed.

Behaviour. The undeclared-length relay is no longer framed as complete. A response this proxy kills on its own write deadline is booked as an internal error instead of being counted a success alongside client disconnects.

Consolidations. checksum.SHA256SumFromDigest replaces three copies of the same four digest checks; npmremote.NewTarballTarget replaces three copies of the 422/400/404 ordering, with ErrUnsafeSegment split by segment so a caller can map instead of re-deriving. blobReadTracker is renamed sourceReadTracker.

Coverage. Six unreached error arms, the two subtests dropped from the hosted twin, and scoped packages, which had no positive case anywhere in the three suites. The scoped integration test settles the outbound %2F question against a real HTTP server. Each new test was verified by deleting the line it covers and watching it go red.

Filed rather than built, with the operator-visible cost written where a reader will look: #779 (closed) (compose Standalone), #781 (ErrAbortHandler swallowed by LabKit's recovery), #782 (the fill commits an unframed truncated body), #783 (no artifact_pulled on npm remote reads), plus comments on #756 (fill-slot starvation and abandoned-fill cycling) and #269 (the transfer deadline and the disconnect-discards-fill rule).

Rebased onto main; the SKILL.md conflict with the container remote blob read arm is resolved and the check count re-derived (42).

🔁 Review round 3

11 blocking threads from @mkhalifa3, all in 30b8f3e0. The threads carry the per-item detail; two of them changed behaviour and are worth reading here.

A truncated relay is attributed by the clock, not by the error's shape. armRemoteReadResponseDeadline now returns the instant it armed, RemoteTarballHandler carries it on the read, and the deadline-kill arm is decided before the arms that were absorbing the same event. Over HTTP/1.1 the breach arrives as a *net.OpError wrapping os.ErrDeadlineExceeded and was already classified. Over HTTP/2 the expiry unblocks the write with an internal stream-closed error and cancels the request context, so every h2 relay this service truncated was counted a success and logged as the client's fault. The errors.Is arm on the sentinel is gone with it: os.ErrDeadlineExceeded satisfies net.Error with Timeout() true, so it matched nothing errors.As did not.

The abort flushes first. See the corrected note in section 2; the threshold is 512 bytes, not 4 KB, and a body under it lost its whole response rather than just its terminating chunk.

What the deadline-kill 5xx actually means. Its doc no longer attributes the class to a budget too small. A client that stalls its TCP reads produces the same event, and nothing observable at the io.Copy boundary separates the two. The consumption-rate evidence that would is written up on #269; until it lands the doc points an operator at the written-bytes field and says to deduplicate alerts by repository.

Coverage, each checked by deleting the line it covers:

• the trailing-fault arm, through a source returning (n>0, err) with the full declared count, mirroring the hosted twin's errWithData reader. The round-2 evidence for this arm pointed at the wrong one: both rows of the deadline-kill table leave the source reader clean;
• serveCached's third call site, the serve a 304 confirmed, whose generic storage fault must answer 500 rather than fall through to the re-fetch or the 503;
• both sides of net/http's sniff copy on the truncated relay;
• npm's seven buffered column values, which no audit run had ever emitted. TestRegisterMetrics_PassesCardinalityAudit emitted handler and code only, so the closed-set pinning was enforced for maven, OCI and the management API and by transcription alone for npm. Dropping npm_remote_last_downloaded_at from closedSetValues now fails npm's audit; before this it passed. The hosted download's bare "last_downloaded_at" literal became colNpmLastDownloadedAt so the loop emits what production passes.

Claims corrected. The file header, the ServeHTTP doc and the instrument note still stated a production mount this MR removed, and remote_operations.go stated the opposite pairing; all four now read as unmounted-until-!1821. The Cache-Control comment invoked heuristic freshness where the absent Last-Modified is what forbids it (RFC 9111 section 4.2.2), so private with no lifetime already forces revalidation. writeUnavailable carried a hand-synced count of its own callers. metrics.md said "the ceiling of 10" beside a union of 11.

driver.sh smoke is unchanged by this round. Nothing here alters how the service builds, boots or dispatches, and this MR still mounts no route; !1821 (merged) is where the smoke run exercises the tarball path.

Related to #349 (closed)