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.goandpackument_get.go, and routes both handlers throughHostedDocumentReader.Read, which already accepts...storage.BlobOpenOption. - !1990 (merged) changes
disttags.goandpackument_get.gotoo.
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, mirroringmaven.TestResolver_ResolveRepository_CarriesDeliveryModeOverride:NULL, redirect, proxy, and an out-of-range column, across all three repository kinds.TestDownloadGET_DeliveryModeThreadingandTestDownloadHEAD_ThreadsNoDeliveryMode, mirroring maven'sTestPrimaryGET_DeliveryModeThreading. The HEAD case pins that a HEAD readsBlobInfoand 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 a307carrying the storage URL under a redirect-resolved mode and a streamed200under a proxy-resolved one.TestRemoteReads_ThreadTheDeliveryModeOverride, new file: all threekind=remotereads driven to a fresh cache hit. The two metadata proxies reach storage throughremote.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 aNULLrow and a redirect override are distinguishable rather than the same assertion twice. Both redirect arms were already reached onmain:remote_tarball_test.go's "storage redirect" subtest drivesserveRemoteTarballRedirect, andTestServeRemoteCached_RedirectOmitsTheValidatordrivesserveRemoteCached's. What was missing is a redirect arm resolved from a threaded option rather than from the fake's ownredirectURLfield, 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
- ADR-005 Artifact Delivery Mode:
this MR implements its second axis for npm. The instance default is untouched,
and the CDN bypass ADR-005 requires under proxy is
internal/storage's, not this layer's. - ADR-008 Content-Addressable Storage and ADR-022 Namespace Decoupling: read for conflict; none. The override is read from the namespace row the handler already loads, which is what ADR-005 says it costs.
Related to #708 (closed)