fix(npm): stream metadata documents so they carry their JSON type
Why
Under DELIVERY_MODE_REDIRECT an npm packument read answers 307 to a signed
object-storage URL, and the stored object carries
Content-Type: application/octet-stream. Every storage driver writes that type
at commit time (internal/storage/driver/gcs/gcs.go:491,
internal/storage/driver/gcs/writer.go:275, internal/storage/driver/s3/s3.go:1439)
and the blob layer has no channel for a per-object media type
(storage.BlobUserMetadata carries SHA-1 and nothing else), so a redirected
document can only ever be served under it.
A packument is JSON and a client is entitled to check the media type before
parsing. The 307 arms set no Content-Type at all, which is why requesting the
abbreviated form changed nothing: the request never reaches the code where the
two media types differ.
Merge gate: the ADR-005 amendment
namespaces.delivery_mode_override and storage.delivery_mode stop reaching
the four npm metadata routes. That is a deviation from
ADR-005,
which is handbook-owned, so per AGENTS.md it needs an amendment there.
The amendment merged on 2026-09-07: handbook!20993.
It scopes the two axes to artifact content (container layers and blobs, Maven
artifacts, npm tarballs) and records that format metadata documents are always
streamed. The mechanism it gives is that a pre-signed storage URL serves the
object's stored Content-Type, every driver writes application/octet-stream
at commit time, and the blob layer has no per-object media-type channel, so
only a streamed response can declare the JSON type. The reason it gives for
streaming rather than storing a correct type is that content type is a
property of how a format interprets a blob and not of the blob itself, which
is why blob_storage_blobs carries no content_type column; deduplication
is what turns that into a constraint, because a commit that finds the object
already present skips the write, so the first format to write the bytes owns
the object's type from then on. It also adds the negative consequence
(neither axis is observable on a metadata read) and the two alternatives
rejected in its place: the response-content-type override on the signed
URL, and setting the correct content type on the object at write time.
The amendment covers the OCI manifest deviation from !991 (merged) as well as this one, so it closes both rather than only the npm half. That was the point of not tracking a second one separately.
The gate has cleared. handbook!20993 merged on 2026-09-07, and the
adr-conformance thread that carried the gate on this MR is resolved. Nothing
here waits on the ADR amendment any more; the remaining blocker is review
approval.
What
Four routes had the defect, not the one reported: the hosted packument
(packument_get.go) and dist-tags (disttags.go) reads, and their two
kind=remote twins (remote_packument.go, remote_disttags.go). All four now
open with documentBlobOpenOptions, which appends storage.WithForceStream to
whatever ADR-005 resolved. The force outranks both of that ADR's axes
(internal/storage/pg_blobstore.go resolves it first), so a metadata document
streams on every deployment and the handler declares the media type.
The option is added per call site rather than inside blobOpenOptions, because
that helper is shared with download.go and remote_tarball.go. The two
tarball routes keep both axes: a tarball's bytes are opaque, so
application/octet-stream is the right type for them and the redirect is worth
keeping. That split is what the new rdmRoute.servesADocument field asserts on
the remote side, and download_test.go's existing 307 cases on the hosted side.
The force needs a HEAD branch, on all four routes
Forcing the stream regresses HEAD. All four routes are served for HEAD
through their GET pattern (net/http.ServeMux matches one for the other, as
handler.go:548 records), and none of them branched on the method, so a HEAD
read the whole cached document out of storage and handed it to net/http, which
discards every byte of a HEAD response body. Before the force, a redirect-mode
HEAD cost one URLFor and answered 307 with no object I/O.
The fix follows the pattern the hosted tarball route already uses at
download.go serveHead. On the hosted side,
HostedDocumentRequest.HeadOnly makes the cache arm resolve the row's blob
through BlobInfo instead of OpenBlob and return a new
HostedDocumentHeadersOnly delivery; each route answers 200 with the
validator, directive, media type and length its GET carries, and
armWarmReadBodyDeadline leaves on it, so nothing is armed. On the remote side,
serveRemoteCached branches ahead of result.Blob.Reader(), writes
Content-Length from result.Blob.Size(), and skips streamRemoteDocument.
Three placements are load-bearing rather than incidental. The branch sits
after the If-None-Match match, so a conditional HEAD still answers 304
off the row alone and reaches storage for neither bytes nor length. It sits
before the open, which is the point. And presence is settled by BlobInfo
rather than by the row's digest, so a HEAD cannot report a 200 with a
Content-Length for a document whose GET would fail; an absent blob takes the
same two kind arms the open path takes, so the dist-tags map self-heals through
the fill and a packument fails closed.
HeadOnly also rides into buildInline's own read-back, since that arm hands
the same request to fromCacheRow. A HEAD that misses the cache therefore
still pays for the fill -- that is what commits the row -- but reads no object
back afterwards.
This is the same fix internal/format/oci took for manifests in !991 (merged), and it
was chosen there over a response-content-type override on the signed URL for
reasons that still hold: that option needs the URLFor(ctx, path) interface,
both drivers, three URL middleware decorators and CDN query-parameter
forwarding to change.
The redirect arm is removed, not left dead
HostedDocumentRedirect and HostedDocument.RedirectURL are gone, both hosted
handlers lose their case, and cachedDelivery and serveRemoteCached fail
closed on a redirect instead of serving one, mirroring errRemoteCacheRedirect
in internal/format/oci. Serving the 307 would be the defect; a 500 is what
says the storage layer disagreed with the option it was handed. Nothing outside
internal/format/npm referenced either symbol.
What this does not reach
Hosted Maven maven-metadata.xml keeps both axes and can still answer a
302. It is a metadata document with the same defect, but it is served by the
ordinary Maven download path (internal/format/maven/download.go), which opens
with openOptions and calls serveRedirect on a redirect blob. This MR does not
touch it, and docs/dev/configuration-reference.md now says so rather than
claiming no metadata document reads either setting.
The kind=virtual metadata routes are orInterimStub today, so the defect
cannot reach them; the S31 virtual read path will need the same force when it
wires h.virtualPackument and h.virtualDistTags. Nothing fills
WithVirtualPackumentHandler or WithVirtualDistTagsHandler outside tests, so
they carry no force and are not a fifth and sixth forced route.
A remote HEAD on the live-upstream arm still reads the body through
io.EOF. serveRemoteUpstream is left alone deliberately: reaching EOF is
what commits the cache fill, so a HEAD that stopped short would abandon the
fill and leave the next reader to pay for it again. The same holds on the hosted
side for the fill itself, as the HEAD section above says.
Hosted Maven maven-metadata.xml keeps its HEAD shape too, for the same
reason it keeps both delivery axes: this MR does not touch that route.
What this gives up
CDN offload on warm metadata reads, which is most of the metadata reads there
are: a cold read already streamed, so the redirect covered the common arm, not
a residue. What makes the loss small is not the arm split but the deployment —
the cloudcdn and cloudfront middleware are real but are not enabled in
config.example.yaml, so on the shipped configuration the redirect target is
object storage rather than an edge. The service therefore takes on the egress
that went direct from GCS before, bounded per document by
npm.max_package_json_size x npm.max_versions_per_package, and the warm body
write deadline !2306 (merged) added is what keeps a large one from being truncated.
The ..._inline_build_document_bytes histograms lose their redirect
observations; a fill that fails closed on a redirect now observes nothing,
because it delivered no document.
Diff size
1238 insertions, 489 deletions across 28 files, measured at 0eceb0aba. Counted
as a sum it is 1727 reviewable LOC, over the 500 bar
docs/dev/development-model.md
sets, so it needs the justification that document asks for. (Earlier revisions
of this description gave 654, then 750, then 1703. The first two were the sum
before the HEAD branch landed, and 1703 the sum before this round's test and
docs fixes; re-deriving at each push is what moved the figure, not a
correction to the earlier arithmetic.)
The split: 153/116 across 8 non-test files under internal/format/npm/,
889/286 across 12 test files in the same package, and 196/87 across 8 Markdown
files (packument_cache.md, metrics.md, remote_document_serve.md, S11, S15,
configuration-reference.md, observability.md, e2e/npm.md).
Splitting would not help, and the shape says why: 1458 of the 1727 lines are tests and prose, against a production change of 153 added and 116 removed.
The diff carries two facts, and the second is the first one's own regression: a
metadata document streams, so a HEAD for one must not pay for the stream.
Both are asserted at every site that used to say otherwise. Landing the force
without the tests would leave the precedence unpinned; landing it without the
HEAD branch would ship a per-request storage read for zero delivered bytes on
all four routes, which is why that branch is here rather than in a follow-up;
and landing either without the docs would leave four documents describing an arm
the same MR deleted.
Test plan
| Acceptance criterion | Test |
|---|---|
| Hosted packument forces the stream | TestPackument_CacheHitForcesTheStream |
| Hosted dist-tags forces the stream | TestDistTagsHandler_CacheHitForcesTheStream |
| A hosted redirect is a 500 on both kinds | TestHostedDocumentReader_Read_FailsClosedOnARedirectPerKind |
| That 500 arms no write window of its own | the same test's w.writeDeadlines assertion; inverting it reddens all three kinds |
| A hosted redirect never leaks the signed URL | TestPackument_CacheHitRedirectFailsClosed, TestDistTagsHandler_CacheHit_RedirectFailsClosed |
| A fill that re-reads a redirecting blob fails closed | a fill whose committed blob redirects fails closed and arms once |
| Neither ADR-005 axis reaches a hosted metadata shape | TestHostedMetadataReads_ForceStreamOutranksEveryDeliveryMode, all four diagonals |
| The same on the cache-miss arm | TestHostedMetadataReads_MissArmSelectsTheServedShape |
| Neither axis reaches a remote metadata shape, and the tarball keeps both | TestRemoteReads_ForceStreamOutranksEveryDeliveryMode, three routes x five combinations |
| A remote cached redirect is a 500 | TestServeRemoteCached_RedirectFailsClosed |
| The forced open still threads the override | TestHostedDocumentReader_Read_PassesBlobOptionsThrough |
A warm HEAD measures once and opens nothing, on all three kinds |
TestHostedDocumentReader_Read_HeadMeasuresAndOpensNoBlobPerKind |
That HEAD arms no write window of its own |
the same test's w.writeDeadlines and w.armAttempts assertions |
A conditional HEAD still answers 304, and measures nothing |
TestHostedDocumentReader_Read_HeadStillAnswers304PerKind |
A HEAD on a gone packument blob fails closed on both kinds |
TestHostedDocumentReader_Read_HeadOnAGonePackumentBlobFailsClosed |
A HEAD on a gone dist-tags blob self-heals, and still opens nothing |
TestHostedDocumentReader_Read_HeadOnAGoneDistTagsBlobSelfHeals |
| A failed measure names its own stage | TestHostedDocumentReader_Read_HeadMeasureFailureNamesItsStage |
The hosted packument route's HEAD carries its GET's headers and no body |
TestPackumentHEAD_AnswersFromTheMeasureAndOpensNoBlob, both kinds |
The same route's conditional HEAD is a 304 off the row |
TestPackumentHEAD_ConditionalStillAnswers304 |
A HEAD never reports a 200 for a document its GET could not serve |
TestPackumentHEAD_BlobMissingFromCASIs500 |
The hosted dist-tags route's HEAD carries all six of its GET's headers |
TestDistTagsHandler_HEAD_AnswersFromTheMeasureAndOpensNoBlob |
The same route's conditional HEAD is a 304 off the row |
TestDistTagsHandler_HEAD_ConditionalStillAnswers304 |
A cached remote HEAD reads no byte of the object and still closes it |
TestServeRemoteCached_HeadDeclaresTheLengthAndReadsNothing |
Both remote twins answer a HEAD that way end to end |
TestRemoteDocumentReads_HeadServesTheLengthAndReadsNoObject, two routes |
hdmShapeStore did not resolve ForceStream; it now does, the way
PgBlobStore.effectiveMode and the remote fake already did, so the fixture
answers the precedence the production path implements.
go test ./internal/format/npm/...green.golangci-lint run ./internal/format/npm/(pinned 2.13.2, isolated cache,--max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false): 0 issues.scripts/ci/check-comment-caps.sh --base origin/main: OK.markdownlint-cli2,lychee,valeover every changed Markdown file: pass.- npm conformance is a merge gate on this MR, not a pre-Draft one: the MR
left Draft before conformance was reported green, so keying the obligation on
that transition read as satisfied when the gate is still open. It has to be
green before merge. The reported failing row is
npm.http.content-type.
Docs
docs/specs/S11-npm-hosted.md and docs/specs/S15-npm-remote.md lose their
metadata 307 response rows and the validator prose that hung off them;
remote_document_serve.md and packument_cache.md are rewritten where they
described the redirect arm, and each gains a section on what a HEAD costs
(## What a HEAD costs on either hosted metadata route and
## What a HEAD costs on a cached serve); docs/dev/configuration-reference.md names the
three response shapes that read neither delivery_mode setting and says
maven-metadata.xml is not one of them; docs/testing/e2e/npm.md gains
e2e.npm.consume.metadata-content-type and e2e.npm.consume.metadata-head,
and its delivery-mode-override row now asserts the split rather than the old
uniform behaviour.
Neither spec changes for the HEAD branch. The observable response is the same
one HEAD already promised -- same status, same headers, no body -- so no S11
or S15 statement is falsified by it; what changed is only what the service reads
from storage to produce it.
setDistTagsHeaders and cacheControlForVisibility each carried a doc comment
far past the one-line cap an unexported symbol gets, and both named the 307.
Editing either in place would have forced the whole block to that cap, so the
rationale moves to packument_cache.md under
## The response headers a hosted metadata success carries.
Closes #1128 (closed)
Closes #1125 (closed)