fix(npm): bound and ration the remote tarball's truncated-relay abort

🎯 Summary

Closes all five discussions from #793 (closed), the follow-up from !1686 (merged) (feat(npm): serve remote tarballs through the caching proxy). Everything is in the remote tarball proxy's abort path and the shared deadline arm that feeds it.

No plan MR: this is review follow-up on merged behavior rather than a feature or initiative, so it has no plan file and no step marker.

🧩 What each discussion asked for, and what landed

1️⃣ Make the alert mitigation real (per-repository dedup, bounded Error line)

note_3716553615

remoteTarballKillLog (new file, remote_tarball_kill_log.go) rations the deadline-kill Error line to one per repository per minute per process. The key is repositories.id, which is the dimension the arm's old comment recommended deduplicating on and the only one available: npm_request_total carries handler and code only, and a repository label is banned by S03-B.

The count lives on a metric, not on the line. A new label-less counter, gitlab_artifact_registry_npm_remote_tarball_deadline_kills_total, is incremented on every kill before the window is consulted, so the rationing can never reach it. The line that survives the window is a sample of the event, carrying the target path, the written-bytes count, the body source and the write error.

The outcome code cannot serve that purpose on its own, which is why the counter exists: CodeInternalServerError on handler="remote_download" is shared with the generic internalError writer and with a cache-sourced body fault, so npm_request_total is a complete record of the route's 500s and not of its truncations.

The rationing never hides that a request failed. Every kill still books a 500, so instrument's per-request npm request served Error line is written either way, and so is the request's access-log completion. What a suppressed kill costs is the second, more detailed line.

Two notes on how the limiter diverges from internal/usagedata/limiter.go, the only sibling in the tree, because a reviewer will diff them:

• It takes a mutex and grows on demand where the sibling is lock-free. It has to: the sibling's keys are a compile-time catalog, and a repositories.id is not.
• Both degenerate receivers (nil pointer, zero struct) admit every line rather than suppressing every line, and that is pinned by a test. Failing towards more Error lines is the direction that cannot hide a truncation.

The map is swept at 1024 entries, and a sweep that reclaims nothing raises the bar to twice what it could not reclaim, so N arrivals inside one window cost one scan per doubling rather than N scans of a map N long. Nothing expires an entry on its own, so the resident set is what has truncated since the last sweep, bounded by the threshold plus the arrivals taken since; that is what the constant's doc says, rather than claiming a window-based residency the code does not implement.

The access-log sampler's "never drop an error entry" invariant is not bent here: it is scoped to "access" records, and this is an application log. Worth naming anyway, because the access record for a kill says 200 (the status commits before the first relayed byte), so it is the duration rule that keeps it, not the status rule.

2️⃣ The abort's Flush() can pin the handler until the budget elapses

note_3716553631

boundRemoteTarballAbortFlush shortens the armed write deadline to remoteTarballAbortFlushBudget (5s) around the flush. Before it, a client that stayed connected but stopped reading could hold the handler for what remained of the 5m35s response budget.

It only ever shortens, and the guard is load-bearing rather than defensive. The kill arm arrives with the deadline already elapsed, and over HTTP/1.1 net.Conn accepts any instant, so an unguarded now + budget would revive a connection the abort exists to leave poisoned and hand the kill arm a flush that can now block. A zero deadline (failed arm) is left alone for the same reason: there is no instant to compare against.

All four branches of that guard, and the abort's shorten → flush → backdate ordering, are now covered by tests. They were not before: every test that reached the abort carried a zero deadline, so only the early return ever ran and inverting the comparison left CI green.

The SetWriteDeadline error is dropped rather than logged. The backdating call directly below fails for the same and only reason (a ResponseWriter chain that cannot set deadlines) and already logs that at Error.

3️⃣ Scope the flushed-prefix guarantee to HTTP/1.1

note_3716553645

⚠️ A live h2 probe corrected this note rather than confirming it. The note asked for the promise to be scoped to h1 "unless a live h2 probe shows otherwise", so I wrote one: TestRemoteTarballHandler_UnframedTruncatedRelay_OverHTTP2, driving the same truncated chunked relay over a real h2 connection on both sides of the sniff-copy boundary the h1 rows straddle.

The flushed prefix survives on HTTP/2. Both rows deliver every relayed byte (200/200 and 8200/8200) and then fail. The note's reading of the write scheduler is right as far as it goes (control and RST_STREAM pop ahead of queued DATA, and DATA for a closed stream is dropped). What it leaves out is that an h2 flush does not queue and return: it goes through writeDataFromHandler, which blocks until the frame write completes, so the prefix is in the connection's write buffer ahead of any later RST_STREAM, and the buffer drains in the order it was filled.

Two corrections to how that was first written up, both from review:

• The source is net/http's bundled h2_bundle.go, not golang.org/x/net v0.58.0. The bundle is regenerated per Go release, so it moves on a toolchain bump and not on the go.mod version; the module's http2 reaches only the GCS client transport here.
• The h2 half is latent, not live. The application listener terminates no TLS and sets no http.Server.Protocols (internal/server/server.go), so nothing negotiates h2 with this service today and every request reaching the relay is HTTP/1.1. The doc comment and the e2e catalog row now say which half is reachable end to end.

The claim is also now enforced: the h2 test asserted only bytes.HasPrefix(prefix, got), which passes when got is empty, so it passed in exactly the world the note predicted. It now asserts the prefix is non-empty.

What the protocol changes is the error's shape, and that half of the note was right: h1 ends in an unexpected EOF, h2 in an INTERNAL_ERROR stream error.

4️⃣ The flush-failure Warn is a guaranteed emission on the common aborts

note_3716553683

remoteTarballAbortFlushLevel drops the line to Debug on a deadline kill and on client churn, and keeps Warn otherwise. It recognises those two by calling remoteTarballWriteDeadlineKill and remoteTarballClientGone, the same predicates the arms use, so one event cannot be a kill to one of them and a finding to the other. remoteTarballClientGone is extracted for that reason and the churn arm now calls it too.

The level is computed before the flush runs. Reading the clock afterwards would downgrade a connection that was healthy when the flush started, since the flush can wait out the bound from 2️⃣.

One residual is deliberate and documented: a write that failed on a broken pipe with the source read clean stays at Warn. It is neither of the two endings the note named, and nothing at that boundary separates it from a writer refusing the bytes for a reason worth reading.

5️⃣ Pin the zero-deadline blind spot with monitoring

note_3716553702

The note asked for a Grafana alert on the arm-failure warning rate. Alerts are defined in the monitoring repository, not here (S03-A), and there was no metric to alert on, so this MR adds the alertable signal: gitlab_artifact_registry_npm_remote_response_deadline_arm_failures_total, a label-less counter incremented by armRemoteReadResponseDeadline.

Its expected value is zero forever, so the recommendation documented alongside it is to alert on any sustained rate rather than on a threshold. The counter is registered at boot, before any handler is mounted, which is what puts the series on the scrape path at zero from startup and lets an alert read a rate off it.

armRemoteReadResponseDeadline's own doc comment is the single home for what a failed arm costs. An earlier draft copied that rationale to five sites and two had already drifted apart ("extended budget" against "shortened budget"); the counter, the catalogs and the spec now point at it instead.

📐 Spec amendment: split out

S15's Observability table gains a row for each of the two counters. That change now rides in !1882 (merged) rather than here, because S15 is Status: Approved and guardrail 5 makes merging a spec MR the approval signal, which an amendment inside an implementation MR does not give the spec author.

docs/dev/observability.md and internal/format/npm/metrics.md keep their rows in this MR: those are the catalogs the code is required to update alongside itself.

Neither MR blocks the other and they no longer share a file, so the order is a choice rather than a constraint. Make it: !1882 (merged) first. If this merges alone, main emits two npm_remote_* series S15 does not declare, and S15's Observability section opens by claiming the whole npm-remote surface, so the next reader takes its four-family table as complete. Nothing in the branch changes either way; the cost is entirely in what the spec says while the gap is open.

🧹 One correction outside the five

remote_tarball.go's doc comments still said "Nothing constructs it yet: !1821 (merged) is the MR that mounts it". !1821 (merged) merged (e4647d4b), and cmd/artifact-registry/wire_npm_remote.go:187,216 builds and mounts the handler today. Corrected in the file header, the handler doc, and resolveTarget, which had been missed on the first pass and still said the opposite of the header two hunks above it.

🔁 Review round 2 (368d54db)

Seven anchored notes, all non-blocking, all answered. Every one was a comment, doc or test-coverage correction; the only production change is a deleted branch that could not be reached.

The counter's description was narrower than its arm. deadlineKill calls remoteTarballWriteDeadlineKill, whose net.Error fallback answers whenever the clock has not, so an upstream body stalling out upstream.request_total_timeout (30s) books a kill with the armed instant (5m35s) still minutes away. The Help, the docs/dev/observability.md row and the metrics.md paragraph all said "their own write deadline elapsed", which sends an operator to raise a response budget that was never the cause. All three now state what the arm books. Two comments also named different pairs of producers; recordRemoteTarballCopy's deadline-kill arm now carries the one list of three endings, and the window constant and the kill-log file doc point at it. remoteTarballAbortFlushLevel gains the kill its stated reason (the connection has stopped accepting writes) does not cover.

Nothing held any description to the predicate. TestRemoteTarballWriteDeadlineKill_IgnoresTheErrorShapePastTheInstant gains the row that does: a timeout with the instant still ahead.

boundRemoteTarballAbortFlush's deadline.IsZero() guard was unreachable. flushBy is always now-plus-budget, which is never before the year 1, so the comparison below already declined and the row named for the branch could not catch its deletion. The branch is gone; the row stays as an input, and both docs now say one comparison covers the zero Time.

remoteTarballAbortFlushBudget's paragraph named the wrong ending. A stalled reader killed on the armed deadline reaches the abort with the deadline already elapsed, so the bound declines and the flush fails in microseconds; the copy is what held the handler. The paragraph now names the ending the bound does protect, a copy that failed before the instant with the client connected and not reading.

repository_id is not on S03-B's disallowed-label list. The comment and metrics.md said it was, which invites a later author to grep the list, not find it, and add the label. What actually excludes it is the rule that every label carry a documented expected distinct-value count.

The h2 rows straddle 4 KiB, not 512 bytes. http2responseWriter implements no ReadFrom in go1.26.6, so io.Copy writes into a bufio.Writer of http2handlerChunkWriteSize. The 8200-byte and 200-byte rows are either side of that.

## Cache and publish-path counters held neither counter. Both move to ## Remote-proxy series, the name metrics.go's package doc uses. And the internal-test package doc splits its tail: the verdict guard takes a remote.CacheServeVerdict rather than an error shape, and the ordering tests turn on a wall clock, so one cause did not fit all four subjects.

Two items in those threads are deliberately not in this push. The predicate's own doc comment is byte-identical on main and the reviewer scoped it out. And the outcome-code misattribution (this arm books CodeInternalServerError where the source-fault arm it pre-empts books CodeUpstreamUnavailable) is pre-existing and gets a follow-up issue rather than a hunk here.

✅ Testing

Area	Coverage
remoteTarballKillLog	remote_tarball_kill_log_internal_test.go: window, per-repository isolation, both degenerate receivers, the sweep (reclaims, never refuses), the raised bar after a barren sweep, and 64 concurrent kills admitting exactly one under -race
Shared-instance seam	TestRemoteTarballHandler_ConcurrentKills_ShareOneLine drives 8 concurrent requests through one handler and asserts one line. A per-request log passes every unit test and fails only this one
Rationing at the arm	TestRecordRemoteTarballCopy_CountsEveryKillAndRationsOnlyTheLine (line rationed, outcome and counter not) and ..._RationsPerRepository
Flush bound	TestBoundRemoteTarballAbortFlush_OnlyEverShortens, all four inputs the guard has to answer, three of which one comparison declines, plus TestAbortRemoteTarballRelay_ShortensThenFlushesThenBackdates for the ordering
Flush level	TestRemoteTarballAbortFlushLevel_QuietsTheFlushesThatCannotSucceed, 8 rows including both kill shapes, plus TestRemoteTarballHandler_AbortFlushFailure_IsQuietWhereItCannotSucceed end to end
HTTP/2 relay	TestRemoteTarballHandler_UnframedTruncatedRelay_OverHTTP2, a real h2 connection, both sides of h2's own 4 KiB http2handlerChunkWriteSize buffer, now asserting the prefix arrives
Both counters	TestArmRemoteReadResponseDeadline_MetersAFailedArm asserts both directions; the kill counter is asserted as a delta across a rationed pair
The kill predicate	TestRemoteTarballWriteDeadlineKill_IgnoresTheErrorShapePastTheInstant, now including a timeout with the armed instant still ahead, which is the ending the counter's description had been missing
Catalog drift	TestRegisterMetrics_ExposesAllOperationalVectors gains both names and its length assert covers npmCollectors

Local: go test ./... green, -race green on the npm package, golangci-lint run ./internal/format/npm/... reports 0 issues, and golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/format/npm/... reports nothing on any file this MR touches. markdownlint-cli2 clean on the Markdown files.

Guardrail 12 (e2e catalogs): docs/testing/e2e/npm.md's e2e.npm.remote.tarball-truncated-relay row is updated, since its claim about the client-visible failure is what 3️⃣ corrects, and it now scopes the h2 half as not reachable against the deployed listener. No new scenario: nothing here changes what a client can request or receive, only how the failure reads and how it is logged.

Guardrail 11 (conformance): no conformance run. mise run conformance:npm drives the real npm CLI against a healthy upstream and exercises the happy path; it has no way to produce an upstream that truncates a chunked body mid-transfer, which is the only path this MR changes. Client-visible behavior is unchanged in any case.

Guardrail 22 (ADRs): checked against the mirror. ADR 005 (Artifact Delivery Mode) is the only one that governs, on its Monitoring and Timeouts clauses, and all five changes conform: everything is proxy-path only, and the deadline shortening in 2️⃣ happens after the copy has already failed and never touches the transfer bound. The freshness script reports the mirror stale by one upstream commit, to ADR 007 (database schema), which governs nothing here.

📏 Size (guardrail 18)

1636 reviewable LOC, past the 500 ceiling, and split rather than justified would not help: the three findings that drive the diff are one change to one decision (what counts a truncation and what merely samples it), and separating the counter from the limiter from the doc comments would produce parts that each read as incomplete.

The number is also mostly not code:

Group	LOC
Production Go	650, of which 107 are added lines that are neither blank nor comment
Tests	933
Docs	53

The other 543 production lines are doc comment, which is where this MR's review feedback mostly landed. Measured with git diff --numstat 9dfd41404 HEAD, added plus deleted, so re-derive it after any push rather than carrying it forward.

⚠️ For the reviewer

Open MRs touching files this branch also touches, at different lines. Whichever lands second rebases; no pipeline reports it. Re-derived at 368d54db, because the first version of this list had gone stale.

MR	Overlapping files	Why it matters
!1878 (merged)	docs/dev/observability.md, metrics.go, metrics.md, metrics_test.go	The sharpest one. It appends four entries to npmCollectors and four names to the same want list this branch appends two to, and TestRegisterMetrics_ExposesAllOperationalVectors asserts assert.Lenf(t, want, len(npmCollectors)). Whichever lands second has a red pipeline until it rebases.
!1880 (merged)	docs/dev/observability.md, metrics.md, remote_packument.go, remote_tarball.go	Four files, two of them the ones this MR edits most.
!1879 (merged)	docs/dev/observability.md, metrics.md	Catalog rows only.
!1834 (merged)	internal/format/npm/remote_tarball.go	Not spec-only, contrary to the earlier version of this list.
!1761 (merged)	docs/testing/e2e/npm.md	One row.
!1882 (merged)	none	The S15 rows. Merge it first; see the spec-amendment section.

!1798 (merged) and !1789 (merged) have merged and are no longer overlaps. The earlier claim that !1789 (merged) and !1834 (merged) touch docs/specs/S15-npm-remote.md only was wrong on both counts: !1789 (merged) touched fifteen files, and !1834 (merged) changes remote_tarball.go.

S15 plan Step 15 ("observability hooks", Status row empty) owns the gitlab_artifact_registry_npm_remote_* metric surface and the docs/dev/observability.md and internal/format/npm/metrics.md catalog entries, and its Files list still reads "four new metric families". This MR and !1882 (merged) land two of those families ahead of it. Recorded on #793 (closed) so Step 15 opens against reality; the plan file itself is untouched here, per guardrail 4's single-writer rule.

Related to #793 (closed)