docs: say what validator a cold npm remote packument serve carries
What this changes
The npm remote spec (docs/specs/S15-npm-remote.md) said that both served packument variants carry
"the ETag from the served body". The service cannot do that on the first read of a package. The
response headers go out before the body has finished streaming, and the ETag is derived from the
hash of the stored copy, which only exists once the whole body has arrived. So a serve streamed live
from the upstream carries no ETag, and a serve from the cached row carries one unless the deployment
delivers blobs by storage redirect, where the response is a 307 that describes no representation.
This MR changes the spec sentence rather than the handler.
Which side changed, and why
The spec. Three reasons:
- S15 already stated the accurate rule twice, in
## API Contractsand inValidators on a proxied metadata readunder## Error Cases. The Serve paragraph was the one place left saying otherwise, so the spec contradicted itself. Amending it makes the document self-consistent; changing the handler would mean amending the other two passages in the opposite direction. All three now state the rule per serve shape and name every shape, so a reader landing on any one of them gets the whole rule (see## Review follow-upsbelow). - The existing integration tests already pin the behavior the amended sentence describes, for both
the cold read and the refill read: see the
ETagassertions ininternal/format/npm/remote_packument_integration_test.go. Nothing had to change to make the spec true. - Making a live serve carry a validator is a design change, not a fix. Issue #749 lists the ways to
do it and says none is free: hash while streaming, buffer the document (which stops the packument
size cap from being a bound on a stream), or redirect the cold read to the just-committed copy
(which costs an extra round trip). Under this repo's rules that needs its own plan MR and a
decision from the spec owner, so it is out of scope here. The spec's
## Follow-upssection now records the option and its costs and cites #749 as where it is weighed against this amendment, so the door stays open and a reader who wants the option knows where it lives.
Related precedent, not absorbed: !1929 (merged) does the equivalent thing for Maven remote metadata (drain the
fill, then serve the committed copy). It was filed as a type::feature MR with its own spec amendment
and plan step, which is the shape the design option above would take for npm.
Review follow-ups
The second commit (docs: cover the redirect serve shape in the npm validator rule) closes three gaps
in the first one:
- The redirect serve shape. The amended Serve sentence read as an exhaustive rule over serve
shapes but named only two, so it claimed every cache serve carries an
ETag. A cache serve under redirect delivery does not: the redirect arm ofserveRemoteCachedanswers307with no validator, whichTestRemotePackument_FreshHitRedirectDeliverypins, and S15's tarball Validators paragraph already said so for its own route. The rule now names all three shapes, in the Serve paragraph, in## API Contracts, and inValidators on a proxied metadata read. - Both enumerations completed. The cached row carries a validator on the fresh hit, on the serve a
304confirms, and on the copy served when the upstream fails (the third was Duo's finding). The live stream carries none on a cold serve, on a revalidation200, and on the full re-fetch of a stale row that stored noupstream_etag. - The
## Follow-upstracking line. It said the design option is not tracked by a work item while #749 is open and holds exactly that option, which would have sent the next reader looking for nothing, or filing a duplicate. It cites #749 instead.
The dist-tags ServeFromUpstream comment also claimed the absence of a validator "is confined to the
first read of a document". Redirect delivery and a refill once the freshness window lapses both break
that, so it drops to the mechanism, which is what the packument arm carries. Touching the block brings
it under the 2-line cap, which is why the spec pointer under it goes as well. Nothing is lost by
dropping it: serveRemoteUpstream and setRemoteDocumentHeaders both state the validator rule in
prose, on both routes.
The third and fourth commits close two more review findings, both in the spec and neither behavioral:
- The live-stream enumerations were not exhaustive. The Serve paragraph and
Validators on a proxied metadata readeach closed their list at three shapes, but the vanished-blob recovery reaches a live upstream serve with no validator two more ways:StandaloneFlow.Deliverfalls through to an unconditionalFetchwhenServeCachedreportsstorage.ErrBlobNotFoundon a fresh row, andserveRevalidatedrecovers the same way when the blob has gone on the304re-read. Neither passes anIf-None-Match, so both dispatch toServeUpstream. The governing sentence above each list already covered them; the lists now say so. The cached-row lists are untouched, becauseCacheServeVerdict.Valid()admits exactly the three they name. - The
## Follow-upsbullet repeated the claim this MR deleted from Go. It said a client re-downloads the document "until some later read lands on the cache", which is the same implication as the dist-tags comment's "confined to the first read of a document", removed one commit earlier for the same reason. It re-opens on every refill once the freshness window lapses, and wherever blobs are delivered by storage redirect no metadata serve of either shape carries a validator, so a client there never obtains one at all. The bullet is what a reader uses to size #749, so as written it under-sized the work. It is now conditional on the delivery shape.
Files
docs/specs/S15-npm-remote.md: the Serve paragraph'sETagsentences, the validator clause in## API Contracts, theValidators on a proxied metadata readsection, and one## Follow-upsentry.internal/format/npm/remote_packument.go: comment only, no code. That comment existed to flag the contradiction and point at #749; with the spec fixed, it drops to the two lines stating the mechanism.scripts/ci/check-comment-caps.shcaps a block in this position at 2 lines, so the spec pointer could not stay here. The rule itself survives in prose onserveRemoteUpstreamandsetRemoteDocumentHeadersininternal/format/npm/remote_document_serve.go, which is what a reader landing on either route reads.internal/format/npm/remote_disttags.go: comment only, no code. The same arm on the sibling route, trimmed for the same reason and to the same two lines, minus a claim about later reads that redirect delivery and a lapsed freshness window both break.
Where I read the code differently from the issue
Two details in #749 do not match what is on main:
- It names
RemotePackumentHandler.serveCachedas the function answering the conditional short-circuit. That method belongs toRemoteTarballHandler. The packument's short-circuit is the package-levelserveRemoteCachedConditionalininternal/format/npm/remote_document_serve.go. - It says the same question applies to the dist-tags proxy and the tarball proxy. Neither needs a spec
change. The tarball proxy's Validators paragraph already states that a relay of a live upstream
body carries no validator, and dist-tags inherits the packument's rule, which
## API Contractssays explicitly. Fixing the Serve paragraph therefore covers dist-tags too.
Checks
pre-commit run --fileson all three changed files: every hook passes, including Vale, markdownlint, lychee,go-test,golangci-lint, and Go comment caps. The two review-follow-up commits touch only the spec, and the commit hook ran Vale, markdownlint, and lychee over it again.go build ./...,go vet ./internal/format/npm/,go test ./internal/format/npm/: pass.golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/format/npm/: 67 findings, all pre-existing in integration test files this MR does not touch. Zero inremote_packument.goorremote_disttags.go.- Overlap: !1945 (merged) also edits
docs/specs/S15-npm-remote.md, in three hunks spanning lines 850-896 under## Error Cases. This MR's four hunks in that file span 422-438, 631-639, 923-938, and 1094-1111, all outside that range, and neither change alters the other's premise. No open MR touches either Go file. !1890 (merged) touches the S15 plan file, not the spec. - No behavior change, so no scenario in
docs/testing/is added or affected. - 60 reviewable lines, so no split justification is needed.
- No ADR governs the response validator on a proxied read. ADR-007 describes
upstream_etagonly as an input to an upstreamIf-None-Match, which is what the spec and the code already say. - The spec's
**Status:** Approvedfield is untouched. - Duo will flag the spec's title-case headings under its sentence-case rule. Those are the spec template's headings and are kept as-is. This MR adds no heading of its own.
Related to #749