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

🎯 Summary

Adds npmremote.FlightRegistry, one long-lived remote.SingleFlight per remote repository, and the RemoteOperations seam the read handlers resolve a request through.

📋 S15 Step 10 of the npm remote plan · tracked as #699 (closed) 🔗 Blocked by nothing, Steps 8 and 9 are merged · Blocks !1682 (merged) (step 12), !1686 (merged) (step 14)

🧩 What it decides

Why a registry at all. remote.SingleFlight's contract is one instance per remote repository. Nothing in a constructor signature enforces it: a per-request flight compiles, and its only symptom is halved coalescing and a fill cap multiplied by the number of wired handlers, which no unit test sees and only upstream traffic shows.

The seam is the concrete flight. RemoteOperations.Fetcher is *remote.SingleFlight, not a one-method fetch interface. That is the decision internal/remote/standalone.go already records for its own flight field: the one-instance contract is remote.SingleFlight's, and an interface cannot carry it. The packument route composes remote.Standalone, whose constructor takes the concrete type, so an interface here bought a conversion back at the composition root and nothing else.

The key. Entries key on (namespace_id, npm_remote_repositories.id). Without the namespace, that table declares no uniqueness for its id alone, since its primary key is (id, namespace_id). Without the binding id, a binding deleted and recreated against the same parent repository inherits the old flight, whose cache seam was built from the row that went away.

What is not cached. The registry holds the flight and nothing else. The cache seam is rebuilt per request, because npmremote.NewCacheStore captures both validity windows at construction. The URL and bearer are per request too, through LiveUpstreamConfigSource, so a rotation lands on the next request rather than the next restart.

Retention. Nothing is evicted, and the deferral rests on this MR mounting nothing rather than on a bound being hard. Maven's mavenRemoteFlights already bounds the identical structure with an idle sweep, and remote.SingleFlight prices two live flights for one repository — one uncoalesced fetch per in-flight path, a doubled fill cap until the old one settles — rather than forbidding them. That sweep is the shape #672 takes.

Mounts nothing. Every kind=remote read still answers Step 8's interim 501.

🔎 Review feedback carried from the closed !1673 (closed)

Finding Outcome
Key on the pair, not the parent id Taken. !1685 keyed on repositories.id; it now keys on the binding beside its namespace.
Constructor-bypass check read outside the mutex Moved inside, in both Flight and Len. Safe outside only because nothing re-initialises the field, and a later reset would surface as a race at the write site.

🔎 Review feedback from !1685 (merged) review round 2

Finding Outcome
LiveUpstreamConfigSource panics on a bypassed constructor Taken. Sentinel twin plus a s == nil || s.repos == nil guard, pinned over both the declared value and the boxed nil pointer.
RemoteOperationsProvider panics on a nil receiver Taken. if p == nil at the top, pinned over both shapes; the zero value's fail-closed-via-FlightRegistry route is now stated and held by a test.
The freshness bump does read a validity window Taken. Both comments restated on the reason that holds — freshWithinHoursExpr keeps the window in the fresh projection — and pinned by TestRemoteCacheReads_WindowStaysOutOfTheRowPredicates.
The retention deferral rests on a non-blocker Taken in the comment and in Retention above. One correction: #672 does not give that reason; its Notes already price a revived entry at one uncoalesced round of fills.
Present tense for a composition root that does not exist Taken. remote_operations.go and remote_testsupport_test.go now use future tense and name the packument proxy, matching the two sibling files.

🔎 Review feedback from !1685 (merged) review round 3

Finding Outcome
RemoteFetcher contradicts standalone.go's concrete-type decision Taken, option 1. RemoteOperations.Fetcher is *remote.SingleFlight and RemoteFetcher is deleted. The flagged sentence was the inversion of what this MR exists for: the registry is here because no signature can force one instance, and the interface re-opened that hole a layer up.
The window-invariance test measures less than its claim Taken, option 1. splitRenderedStatement now cuts the FROM-with-JOINs half out too and holds it window-invariant, because containerRemoteCacheTagStmt joins four tables and takes the value carrying the window. Verified by moving the comparison into its first ON arm: both new assertions fire, every old one still passes. Test renamed for what it measures.
The secret-lifetime claim names the wrong site Taken. The row read runs through NpmRemoteRepositoryStore.FindRemoteByRepositoryID, the single conversion site, so this call is itself a materialization site. Rewritten to keep only the bare-copy claim.
Two NotContains assertions cannot fail Taken, option 1. Option 2 does not reach it either: with err set the fake returns no row, and no arm of UpstreamConfig holds a row and errors. Replaced with ErrorIs on the cause and Contains on each id; test renamed. FlightRegistry's nil-receiver arms now covered on both Flight and Len.
Nothing ties the provider's CacheStores to the registry's Taken as option 2, stated on NewRemoteOperationsProvider. Option 1 is deferred to the MR that writes buildNpmRemoteOperations, where the shape that removes the second set (the provider taking the registry alone) can be judged against real wiring.
The keying comment reads as though maven did not differ Taken. The FlightRegistry doc now says the namespace half is defense against what the schema declares, and that the mounted mavenRemoteFlights keys on the binding id alone over the same composite primary key. One correction: the ids are UUIDv7 from datastore.newID, not random UUIDs.
The status-table hunk is now forbidden Taken, and one step further. Dropped here, and !1683 (merged) drops its row-11 hunk in ceb00501, because a batch MR filling row 10 alone would have collided with it on the adjacent line. Both rows are recorded in !1768 (merged).

📐 Governing ADRs

  • ADR-023 Code structure — conforms. The registry lives in the npmremote building-blocks subpackage; the handler-facing seam (RemoteOperations, RemoteOperationsFinder) lives in npm, so neither layer's dependency reads as a dependency on the other.
  • No other ADR governs this change: it adds no API surface, no schema, and no delivery path. Mirror checked with ./scripts/adr-freshness.sh: current. One internal ADR is never mirrored and was not read.

Testing

TestFlightRegistry_RecreatedBindingGetsANewFlight is new and pins the keying change; it was confirmed to fail against a parent-keyed registry. TestFlightRegistry_SameBindingInTwoNamespacesGetsTwoFlights pins the namespace half.

TestRemoteCacheReads_WindowStaysOutOfTheRowPredicates now holds the FROM-with-JOINs half window-invariant as well as the WHERE clause, checked load-bearing by moving the comparison into containerRemoteCacheTagStmt's first ON arm. TestFlightRegistry_ZeroValueIsRefusedNotDereferenced covers the nil pointer alongside the declared value, on both Flight and Len.

go test ./internal/format/npm/... ./internal/datastore/ ./internal/remote/... green. golangci-lint run ./internal/datastore/... ./internal/format/npm/... 0 issues; no integration-tagged file is touched.

📏 Diff size

~2,037 reviewable LOC, past the 500 line in docs/dev/development-model.md.

Group LOC
Production Go 619
Tests 1,418

Why not split. The provider exists to hand out the registry's flights, so splitting them lands a constructor with no caller in one MR and its only caller in the next, and the keying invariant the Acceptance pins can only be asserted across both. Production is 30% of the diff and sits in six files, four of them doc-only edits from review; the bulk is the table-driven suite that pins the keying, the build race, and the constructor bypass.

🧪 E2E scenario impact

None. No route is mounted, so no journey changes.

Related to #699 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading