feat(npm): thread the per-namespace delivery-mode override

What this changes

npm.Resolution carried no delivery-mode override, so every npm blob read resolved against the instance default alone. ADR-005 makes the per-namespace column available regardless of that default, and Maven (maven.Resolution.DeliveryModeOverride) and Container/OCI already honor it. On an instance defaulting to redirect, a namespace whose clients cannot reach object storage could use Maven and Container but not npm.

Resolution now carries namespaces.delivery_mode_override as a typed storage.DeliveryMode, mapped in the resolver off the row NamespaceFinder already returns. No new query, no migration, no finder signature change.

Four read paths thread it into their blob open:

Path Site How
Hosted tarball download internal/format/npm/download.go blobOpenOptions into serveBlob's OpenBlob
Cached remote tarball internal/format/npm/remote_tarball.go carried on remoteTarballRead, applied in serveCached
Remote packument internal/format/npm/remote_packument.go remote.StandaloneOptions.BlobOpenOptions
Remote dist-tags internal/format/npm/remote_disttags.go remote.StandaloneOptions.BlobOpenOptions

What is deliberately left out

The hosted packument and dist-tags reads still answer from the instance default. Their OpenBlob calls sit on the cache-hit arm alone (DistTagsHandler.serveCacheHit, PackumentHandler.serveCacheHit), so a fresh cache row answers with the instance default while a cache miss opens no blob at all and serveInlineBuild streams a 200 under either setting.

That 200-on-miss, 307-on-hit split is not caused by the override: with the column NULL on a redirect-default instance those two routes already behave that way, so leaving the override unset does not make them uniform. An earlier revision of this description recommended leaving it unset for exactly that reason. The reason does not hold, so the recommendation is withdrawn. A namespace that needs proxy delivery gets it on every npm read except those two, which is the state #708 (closed) closes.

Two properties are worth knowing before setting the column, and the sidecar and the e2e row now carry both. A proxied hosted tarball streams its whole body on server.timeouts.write without re-arming that deadline, unlike the three kind=2 reads, which arm armRemoteReadResponseDeadline first. The server.timeouts.write row in the configuration reference already documents this for proxied delivery generally, and the missing re-arm is shared with the Maven download and OCI blob handlers rather than introduced here. And delivery_mode_override is a column on namespaces, which the Maven and Container read paths consult too, so a namespace holding more than one format cannot set it for npm alone.

Two open merge requests rewrite exactly those files:

  • !1985 (merged) changes disttags.go and packument_get.go, and routes both handlers through HostedDocumentReader.Read, which already accepts ...storage.BlobOpenOption.
  • !1990 (merged) changes disttags.go and packument_get.go too.

Threading them here would conflict with both and be deleted by one of them. Once !1985 (merged) merges, the remainder is passing the options to two Read calls. #708 (closed) stays open for it, and the sidecar and the e2e catalog row both say so.

The in-code pointer to #708 (closed) sits on npm.Resolution.DeliveryModeOverride, the exported field whose contract it qualifies, which is where AGENTS.md asks for it. It is not repeated beside the two OpenBlob calls that carry the gap: touching either doc comment would reflow the whole block to its cap under check-comment-caps.sh, and both files are rewritten by !1985 (merged) and !1990 (merged), so the call-site pointer moves into them with !1985 (merged).

File overlap with open merge requests

Per the guardrail, git diff --name-only main...HEAD was checked against every open MR. Three share a file; none shares a changed line:

MR Shared file Hunks
!1916 (merged) docs/testing/e2e/npm.md theirs 49-55, 182-188, 259-265; mine 125, 171, 258 (adjacent table rows)
!1916 (merged) internal/format/npm/download.go, download_test.go disjoint

The npm.md usage-data row is one line from !1916 (merged)'s, so whichever lands second may need a one-line resolution there.

export_test.go no longer appears: the opts parameter it had gained went unused, so it was dropped and the file is byte-identical to main again. That removes the overlap with !1985 (merged) and !1990 (merged) entirely.

Tests

  • TestResolver_Resolve_CarriesDeliveryModeOverride, mirroring maven.TestResolver_ResolveRepository_CarriesDeliveryModeOverride: NULL, redirect, proxy, and an out-of-range column, across all three repository kinds.
  • TestDownloadGET_DeliveryModeThreading and TestDownloadHEAD_ThreadsNoDeliveryMode, mirroring maven's TestPrimaryGET_DeliveryModeThreading. The HEAD case pins that a HEAD reads BlobInfo and signs no redirect whatever the override says.
  • TestDownloadGET_DeliveryModeSelectsTheServedShape, the hosted counterpart of the remote shape test below: 4 subtests over both instance defaults, asserting a 307 carrying the storage URL under a redirect-resolved mode and a streamed 200 under a proxy-resolved one.
  • TestRemoteReads_ThreadTheDeliveryModeOverride, new file: all three kind=remote reads driven to a fresh cache hit. The two metadata proxies reach storage through remote.Standalone; the tarball route opens the blob itself, so one test would not have covered the other.
  • TestRemoteReads_DeliveryModeSelectsTheServedShape, same file: the three routes again, asserting the response a client receives rather than the option the serve threaded. 15 subtests over 3 routes and 5 combinations of instance default and override, including both diagonals (a proxy override under a redirect default, a redirect override under a proxy default). The fake resolves the override against an explicit instance default the way the storage layer does, so a NULL row and a redirect override are distinguishable rather than the same assertion twice. Both redirect arms were already reached on main: remote_tarball_test.go's "storage redirect" subtest drives serveRemoteTarballRedirect, and TestServeRemoteCached_RedirectOmitsTheValidator drives serveRemoteCached's. What was missing is a redirect arm resolved from a threaded option rather than from the fake's own redirectURL field, which is what makes this test discriminate between the two axes.

TestDownloadGET_DeliveryModeThreading still asserts the threaded option alone, because its fake delegates to storage/stub, whose OpenBlob documents that it always streams and that open options have no effect. Asserting a shape needs a fake that resolves a mode, so dmShapeBlobStore does that and nothing else, leaving the counting fake unchanged for the tests that only care about the option.

Each of the five threading sites was reverted in turn and the matching test failed, so no site is asserted only by a test that would pass without it. With blobOpenOptions stubbed to return nil, both shape tests fail on both diagonals, which the earlier single-default fake could not detect.

Verification

End to end, against a local AR built from this branch. driver.sh configures DELIVERY_MODE_PROXY as the instance default. A real npm publish followed by a tarball read gives:

namespaces.delivery_mode_override Status
NULL 200 streamed (instance default)
1 (proxy) 200 streamed
0 (redirect) 307 to storage

The 307 row is the behavior this MR adds; on main that read is a 200.

What did not run. scripts/conformance/npm-e2e.sh hardcodes its readiness probe at :9090 (PROBE_URL), and a concurrent local AR owns that port, so the harness could not boot its own instance. The end-to-end run above uses ephemeral ports instead and covers the same hosted tarball read. CI's conformance:npm:s3-garage job is unaffected: it sets no per-namespace override, and with the column NULL blobOpenOptions returns no options and OpenBlob is called exactly as before.

Also run: the full ./internal/... unit suite, golangci-lint run ./internal/format/npm/... (0 issues), the same with --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false (no finding in any file this branch touches), and scripts/ci/check-comment-caps.sh.

Delivery mode is not an npm telemetry dimension

The npm request-completion line and npm_request_total carry handler and code, so a 307 and a 200 book identically and whether an override took effect is not attributable to the npm handler set. The intended signal is the HTTP status on the service-wide LabKit access log and metrics; no npm-level dimension is added here, because that would be a new label on a bounded vocabulary and belongs with its own cardinality-budget change. An unlabeled counter recording that an override was resolved was considered and left out for the same reason the call-site pointer was: metrics.go and metrics.md are both rewritten by !1985 (merged) and !1990 (merged).

The artifact_pulled event does carry delivery_mode and stays truthful under an override, because serveBlob derives it from what was delivered rather than from what was requested. That is ADR-012 product analytics, not operational telemetry.

Who can set the column today

Nothing writes namespaces.delivery_mode_override. It is projected by namespaceColumns() and read by the three format resolvers, but no management-API, gRPC, or gitlabapi surface sets it (ADR-005 assigns that to S17, unimplemented), so only a hand-written UPDATE produces a non-NULL row. The status-code flip this MR introduces is therefore unreachable on a live instance until that surface lands.

Why there is no plan MR

This is a single-MR conformance fix traceable to one issue, not a multi-step initiative with a docs/plans/ status table. It introduces no new step numbering and nothing depends on it landing in a particular order.

Notes for the reviewer

The serveRemoteCached doc comment. Its second paragraph explained the absence this MR removes, so it had to change. check-comment-caps.sh takes a touched unexported top-level doc to one line and counts the whole block, so the surrounding prose (the redirect rationale and the result.BlobRef versus result.ETag validator rule) could not stay in the comment. It moved to a new internal/format/npm/remote_document_serve.md, the sidecar pattern packument_cache.md and metrics.md already use. Nothing about that prose changed except the delivery-mode paragraph.

The repo-type::* label is scoped to one value, but this change spans hosted and remote. It is labeled repo-type::hosted because the resolver field and the hosted tarball read are the foundation the remote arms thread; the remote sites are in the table above.

Diff size

776 changed lines, over the 500 the development model asks about, split:

Group Lines
Production +50 -26
Tests +626 -6
Docs +67 -1

Production is 76 lines. Splitting would put the resolver field in one MR and its consumers in another, which is the shape AGENTS.md's "State the merge order when a change widens a predicate ahead of the code that serves it" guardrail warns about: the field would be readable by every npm handler while only some honored it, with nothing in the tree saying which. The tests are the bulk and they follow the sites.

Governing ADRs

Related to #708 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading