feat(npm): count a hosted fill's mid-body deadline kill (npm-hosted-fill-body-write-deadline plan: 1/2)

What

Step 1 of docs/plans/2026-08-31-npm-hosted-fill-body-write-deadline.md.

A hosted npm metadata fill writes its body under the 65 s instant armInlineBuildResponseDeadline arms. Until now a deadline firing inside that write was indistinguishable from a client hanging up: one Warn naming a disconnect, no counter and no outcome code, so a truncated 200 counted as a success on npm_request_total.

This makes the ending attributable and countable:

  • armInlineBuildResponseDeadline returns the instant it armed, and the zero time.Time plus an inlineBuildDeadlineArmFailures increment when the arm fails.
  • buildInline records that instant on HostedDocument.WriteDeadline for every fill-arm delivery, the redirect and the 304 included, and observes the route's document-size histogram on every fill-arm delivery carrying a size.
  • Both hosted streamDocument arms wrap the cache blob in sourceReadTracker when an instant was armed, then classify the ending.

Five new label-free metric families, all hosted-path only: ..._npm_packument_inline_build_deadline_kills_total, ..._npm_disttags_inline_build_deadline_kills_total, ..._npm_inline_build_deadline_arm_failures_total, ..._npm_packument_inline_build_document_bytes and ..._npm_disttags_inline_build_document_bytes.

What each ending books

Ending Counter Outcome code Log
The armed instant elapsed mid-body the route's kill counter internal_server_error Error, rationed per repository per minute
The client disconnected none none Warn
The cache blob's own read faulted none internal_server_error Error
Any other failed write on a clean read none none Warn

inlineBuildWriteDeadlineKill is clock-only. It drops remoteTarballWriteDeadlineKill's net.Error fallback rather than guarding its zero-deadline entry, because a zero instant here means a warm read armed nothing, and a warm read stalling past server.timeouts.write is exactly the shape that fallback matches. packument_cache.md carries the departure, the four blind spots it opens and how each is closed or bounded, and links #913. metrics.md carries what the five families count, the population each histogram observes, and the bucket derivation.

Review findings fixed in this MR

A branch review found one blocking defect in the first four commits, fixed in 29fa86ea0 before this MR opened. Both classifiers had introduced readErr into the decision without the context.Canceled guard that recordCopyOutcome (download.go) and recordRemoteTarballCopy both carry. The cache blob is opened with the request context, net/http cancels that context when the client goes away, and io.Copy reads before it writes, so a plain client disconnect on a cache-miss delivery was booked as internal_server_error with an unrationed Error line naming storage. The mirrored suites already pinned that ending; this suite had dropped it, which is why it shipped green. assertClientDisconnectIsChurn now pins it on both routes at both clock states and is mutation-checked.

The same commit corrects five documentation claims a reader acts on: the written-bytes arithmetic (borrowed from the ErrContentLength mechanism the same paragraph excludes), the histograms' population under collapse, a #269 citation that points at unrelated container work, the log volume of the unrationed records, and the dist-tags histogram's saturation of the first bucket.

Deviation from the plan

This MR reverses one ruling of the merged plan, deliberately and in one place. The plan's ### The observability table does not carry the inline-build families concluded:

no observability.md row is added, because adding rows for these while the four existing ones stay out would read as coverage the table does not have.

The five rows are added anyway. The asymmetry the plan names is real: the four pre-existing families (packument_inline_build_collapsed_total, disttags_inline_build_collapsed_total, inline_build_capacity_exceeded_total, inline_build_timed_out_total) are still absent from that table. The answer is to backfill those four, which is a docs change worth its own MR, rather than to withhold accurate rows for five new families whose proxy-side twins already sit in the same table. Raising it here so the reversal is reviewed rather than discovered.

Open question for step 2

The plan's Files: entry writes the last bucket edge as 477 MiB while its ## Research Findings calls 477 MiB an approximation of the exact product npm.max_package_json_size x npm.max_versions_per_package = 500,000,000 bytes. The two readings differ by 170,752 bytes. This MR ships 500_000_000, the "configured ceiling" reading, and TestInlineBuildDocumentBytesBuckets_BracketTheBudgetDerivation pins it. Step 2 reads this edge for its size-derived budget, so flip the literal and correct the plan text if the binary 477 << 20 was meant.

Diff size

1524 reviewable LOC (added + removed against origin/main), past the 500 in docs/dev/development-model.md. Splitting would not help review it. The production core is 275 LOC and is one change: the instant has to be returned, carried and classified together, and each half is unreachable without the other.

Group LOC Files
Production Go 275 metrics.go 68, packument_get.go 58, disttags.go 56, inline_build.go 47, hosted_document_read.go 24, remote_tarball_kill_log.go 22
Tests 915 packument_get_test.go 370, hosted_document_read_internal_test.go 256, inline_build_internal_test.go 95, disttags_test.go 75, export_test.go 49, metrics_test.go 37, inline_build_deadline_test.go 33
Docs 334 metrics.md 232, packument_cache.md 93, observability.md 9

Merge order

Depends on !2256 (merged) (docs(plans): reverse the ruling against observability.md rows). It amends the plan ruling the section above reverses, so it must merge first; a hard merge block is set on this MR for it.

Three open MRs of mine append to files this one appends to, and whichever merges second resolves the conflict. No pipeline reports it, so it is recorded here:

  • !2197 (merged) overlaps on five files: docs/dev/observability.md, internal/format/npm/metrics.go, metrics.md, metrics_test.go and export_test.go. This is the one that matters, because it appends to the same npmCollectors list and the same expected-name list.
  • !2204 (merged) and !2193 (merged) overlap on docs/dev/observability.md and internal/format/npm/metrics.md.

Testing

  • go test ./internal/format/npm/ passes.
  • golangci-lint run at the pinned 2.13 reports 0 issues for the package.
  • No conformance run is owed: no route, status code, header, response body or error code changes, so no npm protocol behaviour moves. The same bytes go out under the same committed status.
  • No e2e scenario in docs/testing/ is added or affected, for the same reason: every ending here is past a committed 200 and is visible only in metrics and logs, which the scenario catalogs do not assert.

Known gap, not fixed here

remoteTarballKillLog's type doc still names RemoteTarballHandler as the owner and recordRemoteTarballCopy as the writer. Two more owners and two more writers have made that an incomplete list. Correcting it means touching a 35-line block whose cap is one line, so the prose has to move to a sidecar that does not exist yet, dragging tarball-owned text into an npm-hosted MR. The file header this branch already rewrote names both windows, so a reader of the file is not misled.

Related to #952 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading