feat(npm): thread the delivery-mode override into hosted metadata reads

What this changes

The hosted packument and dist-tags reads passed no open options to their cache-blob read, so a namespace that set namespaces.delivery_mode_override was answered on the instance default by those two routes while every other npm read honored the column. ADR-005 makes the per-namespace setting available regardless of that default. !2043 (merged) closed the resolver field and four of the six npm reads that open a blob; this closes the remaining two, and with them #708 (closed).

Both routes resolve their document through HostedDocumentReader.Read, whose signature already ends in opts ...storage.BlobOpenOption and which already forwards them to OpenBlob. Each call site now appends blobOpenOptions(res), the helper the tarball and kind=remote reads already use:

Route Site
Hosted packument GET internal/format/npm/packument_get.go:221
Hosted dist-tags GET internal/format/npm/disttags.go:720

No new query, no finder or constructor signature change, no migration. The production diff is two argument lists and one doc comment.

Both arms honor it, which is a change from what the tree documented

HostedDocumentReader opens the cache blob at one place (hosted_document_read.go:265), reached from both of its arms: the fresh row, and the row a cache miss fills before answering from it. So threading the option at the two Read call sites covers both arms, and a namespace gets one answer whatever the cache's warmth.

That contradicts what remote_document_serve.md and the e2e.npm.consume.delivery-mode-override row said. Both described a cache-hit-only blob read, naming DistTagsHandler.serveCacheHit, PackumentHandler.serveCacheHit and serveInlineBuild. Those three symbols no longer exist in production code: !1985 (merged) routed both handlers through HostedDocumentReader, and !1990 (merged) converged the packument cache miss on the fenced rebuild. The two documents are corrected here rather than only losing their #708 pointer, because the caveat they carried is now false in a way that would mislead an e2e run into controlling cache warmth it does not need to control.

Those same two merge requests are why !2043 (merged) left this out: both were open against these files at the time, and an implementation opened alongside them would have conflicted without either pipeline reporting it. Both have since merged, which is what unblocks this.

Governing ADRs

ADR-005 is the one ADR this touches, and it is the reason for the change rather than a constraint on it: the two-axis model it specifies is what the hosted metadata reads were not implementing. Nothing here deviates from it, so no handbook amendment MR is opened. The local mirror was read, not inferred from its title.

Tests

New internal/format/npm/hosted_delivery_mode_test.go, mirroring remote_delivery_mode_test.go rather than growing either handler suite, which sit at 81 KB and 69 KB and lose per-line diff anchors as they approach 100 KB.

  • TestHostedMetadataReads_ThreadTheDeliveryModeOverride: 6 subtests, both routes over a proxy, a redirect and a NULL column, asserting the option that reaches OpenBlob. Its fake always streams, so a threading assertion is not also an assertion about the shape.
  • TestHostedMetadataReads_DeliveryModeSelectsTheServedShape: 8 subtests, both routes over both instance defaults, asserting the response a client receives. Both diagonals are covered (a proxy override under a redirect default, a redirect override under a proxy default), so a NULL row and an override naming the instance default are distinguishable rather than the same assertion twice.
  • TestHostedMetadataReads_MissArmSelectsTheServedShape: 4 subtests driving a real miss through its fenced fill, which is what pins the second arm. It wires the process-global rebuild dependencies, so it is not parallel and says so. Its 307 branch asserts the validator per route, because the two 307s are not shaped alike: hdmRoute.redirectCarriesValidator records which route carries the strong ETag, and the dist-tags assertion is taken over the row the fill committed rather than over a fixture. Inverting both expectations fails both redirect subtests.
  • TestHostedMetadataReads_DistTagsSelfHealThreadsTheOverride: one test over the path a gone cache blob takes, where a fresh row's blob is missing and the read falls through to the rebuild before answering. It asserts the rebuild ran once, the read still answers 200, and the last OpenBlob still carried the override. It needs fillStore.seedRow, which is why internal/format/npm/packument_fill_test.go is in the diff. The self-heal shares its buildInline call with the miss arm (hosted_document_read.go:224), so this is path coverage rather than a mutation nothing else catches: dropping the opts at that call, or at buildInline's post-fill fromCacheRow, fails this test and the four miss-arm subtests, while dropping it on the cache-hit fromCacheRow fails only the fresh-row tests.

Each of the two threading sites was reverted in turn and the suite failed, so neither is asserted only by a test that would pass without it.

go test ./internal/format/npm/... is green. golangci-lint run --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/format/npm/... reports 0 issues. The --build-tags=integration run reports 66 findings across the package's integration files, none of them in a file this MR touches, so they are pre-existing.

e2e scenarios

docs/testing/e2e/npm.md's e2e.npm.consume.delivery-mode-override row is updated: it now tells a run to assert all three reads rather than the tarball GET alone, and to assert the validator per route, because a dist-tags 307 carries the strong ETag and a packument 307 carries none (cachedDelivery sets it for NpmMetadataKindDistTags only).

Reviewable LOC

589 changed lines, split:

File group Lines
internal/format/npm/hosted_delivery_mode_test.go (new test) 514
internal/format/npm/remote_document_serve.md (sidecar prose) 51
internal/format/npm/packument_fill_test.go (seedRow helper) 16
docs/testing/e2e/npm.md (one table row) 2
Production Go (disttags.go, packument_get.go, resolver.go) 6

Splitting would not help: the production change is 6 lines across three files and the two threading sites are the same defect, so a split would separate a one-line change from the test that proves it. The growth over the original 500 is review-requested coverage on the same two routes, which belongs with the change it constrains.

File overlap with open merge requests

git diff --name-only main...HEAD was checked against every open MR (70 at the time of writing). One shares a file and neither shares a changed line: !1916 (merged) touches docs/testing/e2e/npm.md at rows 49-55, 182-188 and 259-265, and this MR touches row 130. No open MR touches the Go files.

Why this closes the issue rather than relating to it

#708 (closed)'s stated work is four items. Three landed in !2043 (merged) and are on main: the npm.Resolution.DeliveryModeOverride field and its resolver mapping, the hosted tarball GET, and the three kind=remote reads. The fourth is the two hosted metadata reads, which is this MR. After it merges nothing the issue asks for is left, so the form is Closes.

The evidence for the three earlier items is on main rather than in this diff. A reviewer can check it with git grep -n 'blobOpenOptions' -- internal/format/npm/, which after this MR lists all six threading sites and the helper.

Closes #708 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading