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) (bothdocs/testing/e2e/npm.md). Either order works and the second to land rebases. - Conflict with
mainas 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
mainand 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.mdconflicted, on thepublish_read_timeoutandpublish_inactivity_timeoutrows. !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, andpublish_read_timeoutthen matchesmaven.upload_read_timeout's wording in the same file, which is the precedent review cited.config.example.yamlmerged clean and still needed the same edit. !2459 (merged) gained one commit after this branch forked from it, rewriting thepublish_inactivity_timeoutinline comment to "no arm reads this yet, so no read deadline moves". Part 2 never touched that line, so git keptmain'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) andinternal/format/npm/publish_deadlines.md(new sidecar), plusinternal/server/body_size.mdtaking 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: its499section 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.mdandinternal/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.NpmConfiggain the new field, so each fixture carries the shipped pair.keyOrderConfig(publish_key_order_internal_test.go),publishConfig(publish_stream_integration_test.go) andpushConfig(publish_usagedata_test.go) already setPublishReadTimeoutand gain its sibling;baseTestConfig(publish_precheck_integration_test.go) set neither and gains both. All four build throughNewPublishHandler, 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.8ce6766f8drops the two where that was the only stated reason: the bare literal inTestPublish_DeclaredOverLimitRefusedBeforeStaging, andpublishWindowConfig, whose handlers are&PublishHandlerliterals that no case in its file drives throughServeHTTP. Deleting all four of the unit-side additions leavesgo test ./internal/format/npm/green, which is the measurement that says the reason was inoperative. config.example.yaml,proto/.../config.protoanddocs/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, bothFinishReadexits, the bodyless and chunked shapes, and the three arm-failure positions.TestNewPublishHandler_NonPositiveDeadlinesTakeTheDefaultspins the zero-value contract of the exportedconfig.NpmConfigon this route. It builds through the constructor, because the suite's own harness assembles a&PublishHandlerliteral 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)