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:

  1. S15 already stated the accurate rule twice, in ## API Contracts and in Validators on a proxied metadata read under ## 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-ups below).
  2. The existing integration tests already pin the behavior the amended sentence describes, for both the cold read and the refill read: see the ETag assertions in internal/format/npm/remote_packument_integration_test.go. Nothing had to change to make the spec true.
  3. 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-ups section 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 of serveRemoteCached answers 307 with no validator, which TestRemotePackument_FreshHitRedirectDelivery pins, 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 in Validators on a proxied metadata read.
  • Both enumerations completed. The cached row carries a validator on the fresh hit, on the serve a 304 confirms, 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 revalidation 200, and on the full re-fetch of a stale row that stored no upstream_etag.
  • The ## Follow-ups tracking 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 read each closed their list at three shapes, but the vanished-blob recovery reaches a live upstream serve with no validator two more ways: StandaloneFlow.Deliver falls through to an unconditional Fetch when ServeCached reports storage.ErrBlobNotFound on a fresh row, and serveRevalidated recovers the same way when the blob has gone on the 304 re-read. Neither passes an If-None-Match, so both dispatch to ServeUpstream. The governing sentence above each list already covered them; the lists now say so. The cached-row lists are untouched, because CacheServeVerdict.Valid() admits exactly the three they name.
  • The ## Follow-ups bullet 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's ETag sentences, the validator clause in ## API Contracts, the Validators on a proxied metadata read section, and one ## Follow-ups entry.
  • 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.sh caps a block in this position at 2 lines, so the spec pointer could not stay here. The rule itself survives in prose on serveRemoteUpstream and setRemoteDocumentHeaders in internal/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.serveCached as the function answering the conditional short-circuit. That method belongs to RemoteTarballHandler. The packument's short-circuit is the package-level serveRemoteCachedConditional in internal/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 Contracts says explicitly. Fixing the Serve paragraph therefore covers dist-tags too.

Checks

  • pre-commit run --files on 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 in remote_packument.go or remote_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_etag only as an input to an upstream If-None-Match, which is what the spec and the code already say.
  • The spec's **Status:** Approved field 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

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading