fix(npm): re-arm the hosted body write deadline from the document size (npm-hosted-fill-body-write-deadline plan: 2/2)

Dzmitry (Dima) Meshcharakourequested to merge
dmeshcharakou/npm-hosted-fill-body-write-deadline-step-2 into main

What

Step 2 of docs/plans/2026-08-31-npm-hosted-fill-body-write-deadline.md, and the fix step 1 made observable.

A cold hosted npm metadata read arms one 65 s write deadline before its inline fill, then streams the document it just committed under whatever is left of that instant. A packument that took most of the budget to render is also a large document to stream, so the body is written against a 5 s tail and a deadline firing mid-io.Copy truncates a committed 200: the status line is already out, and no 503 with a Retry-After is reachable.

buildInline now arms a second time, on the stream arm alone, once the document is resolved:

min(inlineBuildResponseGrace + size / inlineBuildBodyFloorRate, inlineBuildBodyMaxBudget)

512 KiB/s floor rate, 5 minute ceiling, measured from the HostedDocument.ReadyAt the same function has just stamped. The arm records max(firstArm, readyAt + budget), so it can only ever extend: below the 60 s x 512 KiB/s break-even, roughly 30 MiB, a fast fill's remaining margin is the later instant and the arm is inert. That max is the whole design. Arming readyAt + budget outright would replace about 64 s of margin with 5.195 s on a 100 KiB document and cut slow clients that succeed today, and it is also what makes the ceiling safe, since a capped document is never given less than it had before this step.

The redirect and 304 arms do not re-arm. A Location header and an empty 304 are the envelope inlineBuildResponseGrace already covers, and a 304 resolved no body to derive a window from.

Worked budgets

Document Rate term plus grace After the cap
100 KiB 5.195 s 5.195 s
4 MiB 13 s 13 s
64 MiB 2 m 13 s 2 m 13 s
147 MiB 4 m 59 s 4 m 59 s
147.5 MiB 5 m 5 m, where the cap begins to bind
500 MB (477 MiB, the configured ceiling) 15 m 59 s 5 m

Both columns are budgets, not armed instants: under the break-even the first arm's instant is the later one and is what the row is actually held for. The size is clamped to 150 MiB before the multiply, because time.Duration(size) * time.Second wraps negative past roughly 8.5 GiB and a wrapped instant is one already in the past. That clamp is an overflow guard and not the ceiling; the min is what binds from 147.5 MiB up.

packument_cache.md carries the derivation, what would falsify the floor rate, what the cap costs and buys, the ceilings that bind before it (the 25 s shutdown drain above 12.5 MiB, and a platform-managed GKE Gateway timeout), and the store-phase bound this deliberately is not.

What moves for an operator

Step 1's kill_outcome split keeps its two values and its cardinality, but the reading changes on the stream arm: it now says which of the two arms set the window a response was cut on, rather than naming two comparable populations. A successful second arm on a document of positive size puts budget_exhausted_before_body out of reach there, so a tick on it means the fill spent close to its whole 60 s and then either the second arm failed or the store reported no length. Both halves need the slow fill: after a fast one the max keeps the first arm's instant. metrics.md, observability.md and packument_cache.md all carry that reading. Nothing in this repository alerts on the label, and the family shipped with step 1, so no dashboard predates the change.

On the dist-tags route the label's meaning changes the same way, and only the magnitude of the widening is route-specific. npm.max_tags_per_package bounds that document at tens of KiB against a 30 MiB break-even, so the derived window is about 5 s and the first arm's instant is the later one on the ordinary path. That is not unconditional: a fill that spent close to its whole 60 s leaves about 5 s, which the derived window beats, and the re-arm moves the instant there too.

Review findings fixed in this MR

A branch review found five, fixed in 44e546446 and 9cf6224f5 before this MR opened. One was a real defect in the first commits:

armInlineBuildBodyDeadline read the clock a second time instead of measuring from the ReadyAt buildInline had just stamped, and inlineBuildKillOutcome subtracts that same pair. The two therefore disagreed by the nanoseconds between the reads. On a document whose Size a store could not report the window is exactly the grace term, and those nanoseconds pushed it across the <= the label turns on, so the response booked body_write_overran (which says a window was available and the write did not fit) when the body in fact never had one. All three documents described that ending as reachable, so the code was the half that was wrong. Measured across fill elapsed 0 s, 30 s, 59 s, 62 s and 64.5 s before the fix: body_write_overran every time, window floor 5.000000041 s.

Nothing composed the arm with the classifier, which is why it shipped green: the fallback and the classification were each pinned alone. TestInlineBuildKillOutcome_ReadsTheWindowTheArmActuallyLeft now composes them the way buildInline does, over the two documented paths to budget_exhausted_before_body with a fast-fill negative beside each, and is mutation-checked against the previous behaviour. assertArmedAt drops its 500 ms tolerance, because the arm no longer takes a clock reading of its own.

metrics.md also told an operator to rule the non-positive-size case out on the route's document-size histogram. buildInline observes a size only where it is positive, so that case leaves no sample there and its absence settles nothing; the Warn the arm emits is what separates the two.

The remaining three are reference accuracy. The new helper's doc comment linked #912, which is scoped to internal/format/oci by title, label and proposal and would close without touching any npm file, leaving this package's three arm-and-log helpers untracked; #1138 is filed for the npm side and is what the comment and the plan now link. The HostedDocument.WriteDeadline field comment enumerated the zero cases without the redirect and 304 arms. And packument_cache.md cited ADR-004 as committing to "concurrent-session limits", where its #### Concurrent Upload Sessions limit caps in-flight uploads and bounds nothing on this read path.

Second review round

A branch review of the pushed state found five more, fixed in 75ab24316, 46876a3ff and e54df427b. None changes what the service does; four are claims a reader can check and the fifth is a missing assertion.

Three passages said the size-derived arm leaves the kill_outcome split alone below the 30 MiB break-even. It does not. The max compares against what the first arm has left rather than against its whole 65 s, so below the break-even which of the two instants wins is conditional on what the fill spent, and this branch's own TestArmInlineBuildBodyDeadline_ExtendsPastTheFirstInstant is the counterexample: its "a fill that spent nearly all of its window" row is a 100 KiB document and the derived instant wins. metrics.md's ### The two kill counters and observability.md's dist-tags row now carry the condition, and the dist-tags row says what metrics.md's label entry already said correctly for both routes: a successful second arm on a positive size puts budget_exhausted_before_body out of reach whatever the route, because the derived window exceeds the grace from one byte up. What stays route-specific is the magnitude of the widening, not the label's meaning.

Two smaller over-claims went with them. The split cannot say which of the two arms set the window, since a max that picked firstArm and one that picked the derived instant both book body_write_overran. And budget_exhausted_before_body is a partial metric half of a failed second arm rather than a whole one: after a fast fill a failed arm leaves the first arm's window standing, so a kill there books body_write_overran and reads exactly like a successful arm.

packument_cache.md's drain paragraph said a drain-cut body books in neither kill counter and stopped there. It also books code="success", because hostedStreamRecorder.record reaches that write through an arm calling no recordOutcomeCode, and widening the window widens the population landing there rather than introducing it. Both halves are now stated beside the drain. The warm-arm bullet described the #1002 (closed) gap without naming it, so nothing in this package reached the issue; it is linked once, on that bullet.

The second arm's Warn had no test, though metrics.md and packument_cache.md both make it the only thing that says a widening was lost: a grep for the message found one hit, the emit site. It moves to logMsgInlineBuildBodyArmFailed, beside the logMsgSuperseded pair the file already holds for the same reason, and the arm-failure test asserts level and message together. Mutation-checked by replacing the log call with a no-op: both rows fail.

assertArmedFromTheDocumentSize compared the armed instant against the hand-over with a one-second InDelta, justified by a clock read "within microseconds" of the stamp. 44e546446 removed that clock read, so a second of slack sat over exactly the invariant that commit established. It is an equality now, matching the sibling assertArmedAt, which had dropped its own tolerance for this reason. Mutation-checked by restoring the clock read: the equality fails on a 41 ns drift, which the InDelta passed.

And 44e546446 updated four of the five places the plan states the arm's formula. The fifth, the opening line of ### The second arm takes the later of the two instants, still read now +. The plan also cited config.example.yaml:91 for max_tags_per_package, which is at :92.

One finding was raised and deliberately not taken. deadlineRecorder's type doc still opens its last paragraph on a single arm. Correcting it drags an eight-line block that carries real content into the comment-caps gate, which would force the whole block to two lines, and the sentence is incomplete rather than false: the writeDeadlines field and firstArm immediately below it both carry the correction.

Review rounds after this MR opened

Every finding below was raised on this MR and answered on the branch. No production behavior changed in any of them; the two Go commits in the first round add coverage, and the rest correct claims.

Round Findings Commits
First 2 e1411d9c per-arm failure split driven through Read; 9a33911b the body-arm WARN asserts its cause
Second 9 d32ccf15 three fixture-claim fixes; febca84f outcome split folded into the derived-window rows; e98f2557 dist-tags budget test says what it proves; 9a6c7e69 remote_document_serve.md claim bounded; 467f22ae an arm-failure tick reads as the blind spot being open, plus the 12.5 MiB and slow-fill corrections
Second, CI — eb311b16 gofmt blank line left by the fold in febca84f
Third 3 118a62cf arm-failure HELP scoped to the first arm; 34d86dd0 the plan's dist-tags inert claim conditioned; this description

The second round's largest finding is worth naming here because it reverses something the branch had asserted. Both arms resolve the same request's ResponseWriter chain, so a first arm that failed is followed by a second failing the same way: first-fail plus second-succeed is unreachable, and a tick on ..._inline_build_deadline_arm_failures_total always means the blind spot is open rather than may mean it. The max over a zero first arm stays in the code as a defensive contract, and is now documented as one rather than as a production state.

ADR conformance

ADR-005 ## Constraints on the system ("Timeouts") is the one ADR bearing on a response write deadline: the service must support extended timeouts for proxy downloads without affecting redirect performance. This step is what makes the service conform more closely, because the size-derived window is armed on the stream arm alone and the redirect arm keeps the tight first arm. No amendment is owed. The mirror was checked fresh with scripts/adr-freshness.sh (exit 0).

Diff size

1414 reviewable LOC, over the 500 ceiling in docs/dev/development-model.md. Splitting would not help: the production change is 61 LOC across three files and does not divide. The rest is the test table the derivation needs and the prose that prices the two constants.

Group LOC Files
Tests 856 inline_build_internal_test.go 451, hosted_document_read_internal_test.go 240, packument_get_test.go 92, inline_build_deadline_test.go 34, disttags_test.go 33, export_test.go 6
Package docs 370 packument_cache.md 257, metrics.md 95, remote_document_serve.md 18
Plan 116 docs/plans/2026-08-31-npm-hosted-fill-body-write-deadline.md
Production Go 61 inline_build.go 49, hosted_document_read.go 10, metrics.go 2
Repo docs 11 docs/testing/e2e/npm.md 7, observability.md 3, configuration-reference.md 1

Plan-file edits in a step MR

Four commits here touch the plan file, which AGENTS.md guardrail 4 tells a step MR not to do. Three are docs(plans). The fourth is 467f22ae, a docs(npm) commit that corrects one figure in packument_cache.md and the plan's copy of the same sentence together, at a reviewer's request; by the path rule in AGENTS.md that plan hunk should have been its own docs(plans) commit, and splitting it now would need a force-push mid-review. The guardrail's stated rationale is Status-table collisions between parallel step MRs, and that does not apply: the Status table is untouched, step 1 has merged, and no other step MR is in flight against this plan, so there is no second writer to collide with.

The docs(plans) edits are prose. They resolve the plan's ## Open Questions into ## Resolutions, name ADR-005 in ## Dependencies, pin the 5.195 s rate term, and retarget the arming-helper smell from #912 to #1138. One of them reverses this plan's own ruling that the GKE Gateway backend response timeout had to be answered before this MR opened; that reversal quotes the ruling it replaces in ## Resolutions' third entry, with the two reasons, so it is reviewable in place rather than by reference.

Merge order

No dependency. Thirteen open MRs touch a file this one touches and all but two merge cleanly into this branch, tested with git merge-tree:

Testing

  • go test ./internal/format/npm/ passes.
  • go vet ./internal/format/npm/ clean, and the full pre-commit chain (golangci-lint, Go comment caps, gitlint, go-test) passed on both commits.
  • No conformance run is owed. No route, status code, header, response body or error code changes: the same bytes go out under the same committed status, and only the instant the connection is held to moves.
  • docs/testing/e2e/npm.md gains e2e.npm.consume.cold-read-slow-client, recorded as blocked: the suite has no throttled npm client, and the scenario is only meaningful against one, because what it asserts is which of two write deadlines cut the body. #841 asks for the same harness on the OCI path.

Known gap, not fixed here

The warm arm still writes its whole body under server.timeouts.write, 10 s at the config.example.yaml figure, with no arm at all. After this step a large packument therefore succeeds on the cold read that builds it and is truncated on every warm read afterwards, booking no kill counter and no outcome code. That is #1002 (closed), which ## Research Findings scopes out on purpose and which this step does not regress; it is worth landing close behind. A flat kill counter after this merges is not evidence the truncation class is closed, and metrics.md says so from the counter's side.

Related to #952 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports