fix(npm): re-arm the publish read deadline as bytes arrive part 2/2 (upload-inactivity-deadline plan: 4/4)

What

Installs server.DeadlineReader on the npm publish envelope body, so the read deadline is re-armed as bytes arrive and bounds the gap between two reads rather than only the transfer as a whole. Part 2 of 2 of the plan's step 4.

The re-arm is amortized to one arm per half window, so the observed cut lands between half npm.publish_inactivity_timeout and the whole of it.

Stack, in merge order

Step MR State
1, a re-arming reader in internal/server !2345 (merged) merged
2, the OCI blob upload arm !2346 (merged) merged
3, the Maven artifact PUT arm !2351 (merged) merged
4 part 1, the configuration key !2459 (merged) merged
4 part 2, this handler arm !2460 (merged) (this MR) open

Depended on !2459 (merged), and the order was not optional. This MR reads config.NpmConfig.PublishInactivityTimeout, which part 1 introduces, so merged first it would not have compiled. Guardrail 25 asks for the order to be stated on the dependent step, and this is it. !2459 (merged) has since merged, main carries it squashed as 60a0df082, and this branch is rebased past it, so the order is satisfied and no other merge request gates this one.

Merge order against an open MR outside the stack

None to coordinate. Re-derived at 9f4726fa5 against the 61 other open merge requests: eight touch a file in this branch's 17-file list. None is a correctness dependency, so none is this branch's to order; each is a textual conflict whose only cost is a rebase for whichever branch lands second.

  • Conflict with this branch while merging cleanly into main: !2052 and !2470 (merged) (both docs/testing/e2e/npm.md). Either order works and the second to land rebases.
  • Conflict with main as well as with this branch, so each needs a rebase whatever the merge order: !2332 (docs/dev/configuration-reference.md) and !2464 (merged) (docs/specs/S11-npm-hosted.md).
  • Touch a file in the list but merge cleanly against main and against this branch: !2427 (merged), !2465 (merged), !2469 (merged) and !2471 (merged).

Two things moved since the last revision of this section. Dropping the AGENTS.md hunk on review took !2020 (merged) and !2077 (merged) out of the set entirely, since that file was their only overlap with this branch. And !2464 (merged) moved the other way: it merged cleanly at the previous derivation and now conflicts with main and with this branch alike, over docs/specs/S11-npm-hosted.md. The field-number check an earlier revision ran is moot either way: part 1 carried NpmConfig.publish_inactivity_timeout itself, so with !2459 (merged) merged this branch adds no proto field and only rewrites tag 15's comment.

Rebase onto merged part 1

Rebased with git rebase --onto origin/main ae4b021597, which replays this branch's own two commits and drops part 1's, since main already carries them squashed. Two configuration surfaces needed a decision, and both take this branch's wording, because the behavior exists once this lands.

  • docs/dev/configuration-reference.md conflicted, on the publish_read_timeout and publish_inactivity_timeout rows. !2459 (merged) had shortened the first to "Read deadline for a publish request." on a Duo suggestion, because at part 1 the ceiling clause was a forward reference to behavior only part 2 delivers, and had written the second to say no code path reads the field. Taking this branch's rows restores both, and publish_read_timeout then matches maven.upload_read_timeout's wording in the same file, which is the precedent review cited.
  • config.example.yaml merged clean and still needed the same edit. !2459 (merged) gained one commit after this branch forked from it, rewriting the publish_inactivity_timeout inline comment to "no arm reads this yet, so no read deadline moves". Part 2 never touched that line, so git kept main's version with nothing to report, and the file would have shipped a comment this MR falsifies. It is restored to the settled wording by hand, which is the whole two-line difference between this revision's diff and the one before the rebase.

Behavior change on an existing route

This is the one client-visible change, and the plan's scope sentence ("changes no size cap, no status code, and no upload protocol") does not name it: a publish that pauses more than about five minutes between two reads is now cut where it previously survived to the 30m ceiling.

The opt-out is setting npm.publish_inactivity_timeout equal to npm.publish_read_timeout, which is accepted and leaves the bound inert.

A second change is internal but worth naming: a publish carrying a body now flushes its response under npm.publish_read_timeout rather than server.timeouts.write, because the reader pins the write half at the ceiling once, at construction. That is the npm half of #31 and what lets a read allowed to run for many minutes write its own response, as long as that read ends with margin before the ceiling. The cost is that a peer which completes a valid envelope and then stops reading parks the serve goroutine at the ceiling instead of at ten seconds. The OCI blob upload and Maven artifact PUT routes already take the same trade at their 1h ceilings. internal/format/npm/publish_deadlines.md prices it, and # Known residual below names what the pin does not reach.

NewPublishHandler substitutes compiled-in defaults for a non-positive read or inactivity timeout, mirroring oci.NewUploadHandler, so the declarable zero value of the exported config.NpmConfig cannot reach server.NewDeadlineReader's panic. A deployed config never reaches that arm, because config.parseDurationField refuses a non-positive duration.

Deviation from the merged plan

The plan's ## Dependencies states, at docs/plans/2026-09-03-upload-inactivity-deadline.md:1042:

No spec amendment in any step. S01, S11 and S12 each describe arming a deadline without stating whether it is absolute, so nothing in them becomes false.

This MR amends S11 anyway, in three places (the publish-flow deadline paragraph, the 499 failure row, and the ## Configuration NpmConfig mirror). The plan's premise does not hold for S11's proto mirror: the new field makes three of that message-level comment's claims false for one field, and the real proto was rewritten while its spec copy was not, so leaving it would have left the two disagreeing. Correcting the plan text belongs in the batch docs(plans) MR, because guardrail 4 forbids a step MR editing the plan file.

Files

16 reviewable files, 1798 reviewable LOC, plus config.pb.go (regenerated rather than hand-edited; buf:generate-check diffs gen/). Both figures are git diff --numstat 2b94b878e...9f4726fa5 with gen/ excluded.

The plan's Step 4 Files list names 9 files across both parts. Extras carried here, each ruled on rather than left out:

  • internal/format/npm/publish_deadline_test.go (new) and internal/format/npm/publish_deadlines.md (new sidecar), plus internal/server/body_size.md taking more than its plan entry lists: recorded as deliberate divergences in the test-first commit's body.
  • docs/specs/S11-npm-hosted.md: the amendment above.
  • docs/dev/api-style.md: its 499 section justified never declaring the status on no client being able to observe it. A body-inactivity cut fires on a still-connected idle client and the write half is pinned, so the status is delivered and read. This is the third route to reach that and the first to measure it.
  • internal/format/npm/unpublish_deadlines.md and internal/format/oci/manifest_deadlines.md: sibling sidecars whose cross-references to this route's posture move with it. Owned here because no later step touches them.
  • Four test-config helpers that construct a config.NpmConfig gain the new field, so each fixture carries the shipped pair. keyOrderConfig (publish_key_order_internal_test.go), publishConfig (publish_stream_integration_test.go) and pushConfig (publish_usagedata_test.go) already set PublishReadTimeout and gain its sibling; baseTestConfig (publish_precheck_integration_test.go) set neither and gains both. All four build through NewPublishHandler, which substitutes the compiled-in defaults, so none of them needs the field to avoid a panic. An earlier revision listed five sites and justified them with a panic the constructor prevents. 8ce6766f8 drops the two where that was the only stated reason: the bare literal in TestPublish_DeclaredOverLimitRefusedBeforeStaging, and publishWindowConfig, whose handlers are &PublishHandler literals that no case in its file drives through ServeHTTP. Deleting all four of the unit-side additions leaves go test ./internal/format/npm/ green, which is the measurement that says the reason was inoperative.
  • config.example.yaml, proto/.../config.proto and docs/dev/configuration-reference.md: 54 lines re-touched, rewriting the condition-and-event wording part 1 shipped into its settled form now that the arm exists. This is the only overlap between the two parts.

AGENTS.md was on that list until review. Its write-deadline census says this route "set the read deadline only ... so the session opens with the server's original instant still in force", which this MR makes false, and an earlier revision corrected it here. Review asked for that rule to live in a dev guideline rather than in the general AGENTS.md, so 9f4726fa drops the hunk and the relocation ships as its own docs MR. The sentence stays wrong on main until that MR lands. The corrected census is in this MR either way: the write row of docs/dev/configuration-reference.md, internal/server/body_size.md, and internal/format/npm/publish_deadlines.md.

Diff size

1798 reviewable LOC against the 500-LOC guideline in docs/dev/development-model.md. Split by group, at 9f4726fa5, counting insertions plus deletions and excluding the generated files under gen/:

Group LOC
Tests and test-config fixups 1040
Markdown and example config 656
Production Go and the proto schema 102

Production Go alone is 68 lines, all in internal/format/npm/publish.go.

Splitting further would not help. The configuration half is already split out as part 1; what remains is one 1034-line test file and one 473-line sidecar, and the test file's 474 lines of fixtures cannot land after the cases that use them. The test-first commit's intentionally-failing tests also have to sit on the same branch as the implementation that satisfies them, which is the condition guardrail 15's one --no-verify carve-out rests on.

Testing

  • TestPublishDeadline_* cover the paced completion, the stall past the window, the stall inside half the window, headers-then-nothing, an envelope-prefix stall, a slow session open, the ceiling clamp, the write-half pin, both FinishRead exits, the bodyless and chunked shapes, and the three arm-failure positions.
  • TestNewPublishHandler_NonPositiveDeadlinesTakeTheDefaults pins the zero-value contract of the exported config.NpmConfig on this route. It builds through the constructor, because the suite's own harness assembles a &PublishHandler literal and so reaches neither the substitution nor the constructor's two wiring guards.

Which FinishRead exit a publish takes depends on how it framed its body, and both rows are reachable. A publish that declared a Content-Length and whose envelope the walk completed hands the read half back cleared, because the closing brace is the last declared byte and net/http returns io.EOF together with it once the io.LimitedReader drains; that is what the npm CLI, yarn and pnpm all produce. A publish the walk abandoned, and a chunked one whose terminal chunk it never reads, leave a fresh gap bound for the post-handler drain. Each row has its own case, and each reddens when the other's mechanism is broken.

golangci-lint run --build-tags=integration --new-from-rev=origin/main over internal/format/npm, internal/config and internal/server reports 0 new findings, re-run at 8ce6766f8. CI lint never compiles //go:build integration files, so that run is the only thing covering the two integration-tagged files this MR edits, internal/format/npm/publish_precheck_integration_test.go and internal/format/npm/publish_stream_integration_test.go.

Conformance and e2e

No conformance run applies: the change moves no npm protocol behavior, keeping the same statuses, envelope shape and size caps.

docs/testing/e2e/npm.md gains e2e.npm.publish.stalled-publish-cut and its usage-event row. The scenario stalls the body after the _attachments key on purpose, for the reason below.

Known residual

sniffPublishEnvelope runs before either handler on the shared PUT /{package_name} route, to tell a publish from a deprecate, so a client that goes idle in the envelope metadata ahead of _attachments is bounded by server.timeouts.read and not by either npm key. How much that leaves uncovered is the client's key order: the npm CLI puts _attachments last, yarn puts it first. Closing it means arming above the split handler, newPublishOrDeprecateHandler.

internal/format/npm/publish_deadlines.md records this and a note on #1230 (closed) carries the measurement. Because that work item is this step's own, this MR uses Related to rather than Closes so the residual keeps an open owner.

A second residual is the write half's own, and it is why the claim above is bounded at "with margin". The pin puts both deadlines on one instant, so a read that runs all the way to npm.publish_read_timeout leaves nothing for the work between the end of the read and the response: the CAS commit, which carries no deadline of its own because storage.PgBlobStore.NewSession captures the request context and neither storage driver sets a client timeout, and then CommitPublish under publishCommitTimeout. Such a publish commits and then loses its 201, the client gets a closed connection, and its retry of the same version answers 409 version_exists. No fixed grace added to the ceiling covers it, because the CAS commit is the unbounded part.

This is not this route's alone: all five routes in internal/server/body_size.md's write-deadline census put both halves on one instant, and each does some work between the end of its read and its response. Closing it means moving the write half when the read ends rather than pinning an instant computed before it, which is a change to server.DeadlineReader and to the four handler arms, so it is out of scope here. #31 owns it, and internal/format/npm/publish_deadlines.md and internal/server/body_size.md now state it.

Related to #1230 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading