feat(npm): add npm.publish_inactivity_timeout part 1/2 (upload-inactivity-deadline plan: 4/4)
What
Adds the npm.publish_inactivity_timeout configuration key: schema, loader,
cross-field validation, example config and reference row. Part 1 of 2 of the
plan's step 4.
Nothing reads the key on this MR. The handler arm that reads it is part 2, so the two review separately: this one is the configuration contract, that one is the request behavior.
ADR-004 asks for this inactivity timeout by name and cites Slowloris.
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, this configuration key | !2459 (merged) (this MR) | open |
| 4 part 2, the npm publish arm | !2460 (merged) | open |
Merge order: this MR first. Part 2 reads
config.NpmConfig.PublishInactivityTimeout, so it does not compile without
this one. Guardrail 25 asks for that order to be stated where a reader will
look, and !2460 (merged)'s description names it too.
What the key does
It bounds the gap between two reads of a publish envelope body rather than the
transfer as a whole, so a client that keeps sending is not cut while an idle
one is. npm.publish_read_timeout stays the absolute ceiling.
The loader's three behaviors, each with a test:
- An explicitly set value above the ceiling is refused at startup, naming both fields.
- An omitted key takes the compiled-in
5mdefault and is then clamped down to a lower ceiling, so a deployment that only lowerspublish_read_timeoutstill boots rather than failing validation over a key it never wrote. - A value equal to the ceiling is accepted and leaves the bound inert, because every arm part 2 makes then resolves to the ceiling.
The clamp logs nothing, and that silence is deliberate: the identical clamps
at internal/config/container.go:120 and internal/config/maven.go:99 are
silent too. It cannot hide a misconfiguration, because an explicitly set value
above the ceiling is refused by NpmConfig.validate rather than clamped. The
clamp fires only on a key nobody wrote, and it leaves the window equal to
publish_read_timeout rather than unbounded, so part 2's arm still cuts a
stalled publish, at the read deadline instead of earlier.
The field pairs features.field_presence = EXPLICIT with
(buf.validate.field).string.min_len = 1, the same pattern container and
maven already use. That lets the wire boundary separate three cases: absent
defaults, explicitly blank is refused naming the field, and a set value is
parsed and then checked against the cross-field invariant in
NpmConfig.validate.
It is the only field in NpmConfig carrying a protovalidate constraint;
checked against all 14 others.
Claims that only hold before part 2 lands
The key is inert here, so the three surfaces that describe it are written as a condition and its event rather than asserting behavior that does not exist yet: they say the value is parsed, clamped and validated and no read deadline moves, and what happens once the publish handler arms the wrap. Each therefore reads correctly on both sides of part 2's merge.
Part 2 rewrites those three passages to their settled wording. That is 52 lines of re-touched text and the only overlap between the two parts.
Files
| File | Reviewable LOC |
|---|---|
internal/config/npm_test.go |
183 |
internal/config/npm.go |
47 |
proto/artifactregistry/config/v1/config.proto |
33 |
config.example.yaml |
21 |
docs/dev/configuration-reference.md |
3 |
internal/config/testdata/npm_happy_path.yaml |
1 |
288 reviewable LOC, comfortably under the 500-LOC guideline, plus
gen/artifactregistry/config/v1/config.pb.go (45 lines, regenerated rather
than hand-edited). That file is in the diff because buf:generate-check
diffs gen/, so an unlisted regeneration is a red pipeline rather than a tidy
omission.
Guardrail 19 is satisfied: the proto, the loader, config.example.yaml and
the configuration reference all move together, and the new reference row names
no spec, plan or sprint identifier.
Testing
internal/config/npm_test.go:
TestLoad_Npm_ExplicitEmptyStringRejected— themin_len = 1boundary.TestLoad_Npm_PublishInactivityAboveReadTimeoutInvariant— the cross-field refusal, in both directions and at equality.TestLoad_Npm_DefaultedInactivityYieldsToALowerCeiling— the one cross-field casevalidatemust not reach, on a key that was never written.TestLoad_Npm_ConfigExampleDefault— the value this file ships.TestLoad_Npm_InvalidDurationFields— every npm duration field crossed with{unparseable, zero, negative}, matchingTestLoad_Container_InvalidDurationFields. The zero and negative branches are what pinparseDurationField's positivity arm from the npm side, and that arm is what keeps a non-positive window out of part 2's handler, whereserver.NewDeadlineReaderpanics on one.
internal/config/testdata/npm_happy_path.yaml carries the key at 6m,
distinct from the 5m default, so the happy-path test reads a parsed value
rather than a defaulted one.
No conformance run applies and no e2e scenario changes: this MR adds no route
and moves no protocol behavior. Part 2 carries the
e2e.npm.publish.stalled-publish-cut scenario.
Related to #1230 (closed)