feat(npm): hold one coalescing flight per remote npm repository

🎯 What this MR does

Adds npm.RemoteFlights, the per-repository coalescing layer every kind=2 npm read serves through, plus the additive remote.ServeResult.BlobRef the handler above it needs. Step 11a of a 3-MR stack splitting an oversized branch.

Mounts nothing. No route changes behavior here; the dispatcher's remote slots still serve the interim 501.

📚 The stack:

Step MR What it delivers
11a · this MR 👉 !1673 (closed) npm.RemoteFlights + ServeResult.BlobRef
11b !1674 (merged) npm.RemoteDistTagsHandler and its suite
11c !1675 (closed) Composition root: mount, metric label, docs

🧩 The two pieces

1️⃣ RemoteFlights

remote.SingleFlight's usage contract asks for one long-lived instance per remote repository, shared by every flow that fetches from it. Two instances for one repository coalesce nothing between them and grant it two independent max_concurrent_fills_per_repo budgets. A handler cannot honour that alone: it is one process-wide value serving every repository, and the repository arrives per request. Composing per request would have shipped the coalescing inert and the fill cap enforcing nothing — invisible except as upstream traffic.

Entries are keyed on npm_remote_repositories.id, the binding surrogate, so a binding deleted and recreated cannot inherit the old flight. The id alone is the whole key even though the primary key is (id, namespace_id) on a hash-partitioned table; what makes it globally unique is that it is server-minted, and the field comment says so, because a retained flow captures its resolution's namespace and a shared key would serve one tenant from another's cache rows.

Two things had to move for a flow to outlive the request that built it:

  • npmremote.CacheStore captures the freshness windows at construction, so the registry compares them per request and rebuilds when an operator narrows one, rather than judging against the old window until restart.
  • The upstream URL and bearer can no longer come from one request's Resolution, so durableUpstreamConfig re-reads the row per outbound request. One indexed read, on the fetch path only, bought for a rotated credential taking effect on the next request rather than the next restart. That is the scope npmremote.UpstreamConfigSource documents as valid.

The registry is bounded at 1024 entries so it cannot grow with every repository the process has ever served. Overflow evicts an arbitrary entry, and that is not free: an evicted repository with requests still in flight briefly holds two flights, the exact condition the contract warns about. gitlab_artifact_registry_npm_remote_flight_evictions_total carries the rate and one Warn names the bound the first time it is reached.

2️⃣ ServeResult.BlobRef

Carries the served blob's content address out of the same Lookup that produced the bytes. Standalone.openCached is its only producer, so the fresh hit, the revalidated hit, and the fallback serve all carry it. The handler in !1674 (merged) needs it to advertise a strong ETag over the bytes it actually serves; ServeResult.ETag beside it is the upstream's own validator, which describes the upstream's representation and diverges the moment a transform rewrites one.

🔎 Review feedback

Finding Outcome
AppSec: eviction defeats the per-repository fill cap (medium) Real, and the MR already named it. Kept arbitrary eviction over LRU — an eviction costs continuity for one repository, and the accounting an LRU needs buys nothing while the rate is zero — but took AppSec's third suggestion: logFirstEviction emits one Warn naming max_remote_flights the first time the bound is reached. Deliberately not per eviction: past the bound every new repository evicts another, so a line each would arrive at request rate and bury what it reports. The counter carries the rate. TestRemoteFlights_FirstEvictionLogsOnceNamingTheBound pins both halves. Raising the bound stays a one-constant change once a real working-set number exists.
AppSec: f.entries nil-check outside the mutex (low) Moved inside the lock. Safe before — the field is written once, by the constructor, before the registry is shared — but only because nothing re-initialises it, and the comment now says that is what the placement rests on.
Duo: new(*base.Remote) does not compile, and does not copy Both false. new(expr) is Go 1.26's pointer-to-a-copy form and this repo uses it (.golangci.yaml go: "1.26"); the branch builds and the suite passes. Verified independently on go1.26.6: the expression compiles, the result is a distinct pointer, and mutating it leaves the original at its old value — so the parallel subtests do get their own copies. No change.
Duo: is the validity-window rebuild's two-live-flights window deliberately unmetered? Deliberate, and now said in the code. An eviction is load-triggered, repeats while the working set exceeds the bound, and is invisible without a counter. A rebuild is operator-triggered, happens once per window change per repository, and is already attributable to the write that caused it; metering it would add a series whose rate restates how often someone edits a remote.

Verification

  • go build ./... and go vet ./...: clean.
  • go test -race -count=1 ./internal/format/npm/... ./internal/remote/... ./internal/metrics/...: all green.
  • golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 ./internal/format/npm/... ./internal/remote/... ./internal/metrics/...: 0 findings on any line this MR adds or changes. The pre-existing findings it reports elsewhere in those packages are unchanged from main.

📏 Diff size

932 LOC, past the 500 reviewable-LOC line in docs/dev/development-model.md. Split by file group:

Group Files LOC
Production remote_flights.go 377, metrics.go +28, internal/remote/standalone.go +22, internal/remote/serve.go +11 438
Tests remote_flights_test.go 453, export_test.go +24, standalone_test.go +16, metrics_test.go +1 494

Tests are 53% of it. The registry and the BlobRef field could split further, but BlobRef is 33 lines across three files and would leave a one-field MR whose only consumer is two MRs away.

🧪 E2E scenario impact

None: nothing routes to this package yet, and docs/testing/e2e/npm.md scopes remote npm repositories out until the capability ships.

Related to #348 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading