feat(oci): flight registry and per-call request state (S32 plan: 8/19)
What
Moves all four of the container request builder's per-call values off the instance onto two channels, and adds the container flight registry.
The four values split two ways. accept and imageName are uniform across a
resolution, so they ride one request-scoped context value, RemoteRequestState.
The row's url/auth_status/auth_url snapshot and its *UpstreamURLBuilder
belong to one container_remote_repositories row, so each position supplies its
own through RemoteUpstreamConfigSource. A resolution holds up to 20 rows
against one context, so a second field on the request-scoped value would send
every position to the first row's host.
FlightRegistry then hands out one shared *remote.SingleFlight per remote
repository, bounded by a cap, an idle window, and a throttled sweep. The
registry is impossible without the channel move, because remote.NewSingleFlight
captures its RequestBuilder for the repository's lifetime.
imageName is the value that must not be missed. target refuses every path
outside it, so a registry-held builder that kept it frozen would serve the first
image pulled through a remote and fail every other one.
Why the registry is bounded
The cap, idle window, and sweep interval are package constants, copying Maven's
maxRemoteFlights / remoteFlightIdleTimeout / remoteFlightSweepInterval
trio. Without them the map is an unbounded per-process cache of semaphores on a
multi-tenant service. npm's registry has no bound and defers it to #672; this
slice bounds where npm deferred.
Eviction removes only the map entry, so a caller already holding a flight finishes. While the two coexist each path pays one uncoalesced fetch.
The frozen cache seam
FlightRegistry.build freezes one cache seam per entry for the flight's life.
That is safe on the two paths the flight reaches: the fill write and the
freshness bump. Neither reads cache_validity_hours, so no held row pins a
freshness verdict. Lookup and the cache-fallback route stay on the caller's
own per-fetch store, which is what keeps RemoteOperations per fetch.
Each constructor validates what it reads
NewRemoteOperationsWithFlight first demanded a request builder, a doer, a blob
store, and both ids, then discarded all five: a flight carries its own, and only
the cache store and the two bounds reach the glue. A caller had to populate five
fields that changed nothing, and a reader could not tell which of the eight
influence a fetch.
The validator is now split along that line. The direct constructor runs both halves, so its contract is unchanged. The flight-backed one runs the glue half, which is what its own test already pinned.
Behavior
Every standalone remote read behaves identically before and after. Both install
sites — the manifest arm's buildUpstreamFill and the blob arm's fill — put the
two channels on the context each fetch runs on, so no read reaches the new
fail-closed errors.
One operator-facing change: msgBlobFillSurfaceFailed's text now names both
rejections its site answers, not only the factory's. A log filter pinned to the
old string needs updating. The message appears in no dashboard or doc in this
repo.
Tests
Source +575 −272, test +1688 −124.
Unit tests cover each named case: two concurrent builds; two image names through
one held builder, each reaching its own URL and its own repository:<image>:pull
scope; a settled auth_status not re-discovered; a url change picked up on the
next fetch; two builders for two repositories reached concurrently on one request
context, each asserting its own host and its own observed-url discovery guard;
a key miss against a source holding one sibling position, which refuses the lazy
single-entry implementation; fail-closed on a bare context, on state alone, on an
empty source, and on a zero-value source; one rejection per moved validation, at
the new constructor rather than at the read; and a statement-count assertion at 2
and 20 positions, proving the source itself reads nothing.
The registry gets an identity test, a three-arm sweep test over the cap, the idle window and the throttle, and an in-flight survivor test.
The S16 remote read suites re-run. Two are edited for the constructor's new
shape: remote_auth_test.go and remote_e2e_test.go adapt call sites only, and
no existing assertion's subject changed.
Diff size
Past the 500-LOC ceiling at 575 source lines, and justified rather than split. The overrun is the two extra values the channel carries, the two shapes it carries them in, the standalone wiring that moves with them, and the registry's eviction. Splitting lands either a registry that cannot be constructed, a context channel with no reader, a build that does not compile, or an unbounded registry in the interim.
| Group | Lines |
|---|---|
New seams (remote_upstream_config.go, remote_flight_registry.go) |
+388 −0 |
Request builder (remote_requestbuilder.go) |
+78 −119 |
Install sites and glue (remote_manifest.go, remote_blob.go, remote_operations.go, remote_revalidate.go) |
+100 −92 |
Composition root (wire_oci_remote.go) |
+9 −61 |
| Tests | +1688 −124 |
Scope notes
remote_revalidate.go (one line), remote_auth_test.go, and
remote_e2e_test.go are in no step's Files list. All three are mechanical
adaptations the constructor's new shape forces.
No database review is owed: the diff adds no migration and no file that dispatches a statement.
No e2e scenario changes, because no externally observable behavior changes. No route reaches the registry until Step 15, which owns the end-to-end sharing test.
Owed elsewhere
These belong to other MRs and are deliberately absent here:
- The plan's Status row, and its "at most a capped number of entries" wording,
which overstates a sweep the same bullet mandates copying from Maven. Both go
to the batched
docs(plans)MR, which is the Status table's single writer. Measured source and test lines for its LOC line: +575 and +1688. - An S13 amendment before Step 14 wires the registry. S13 asks for a durable
credential read inside
BuildRequeston every call; the source is read per fetch from a row already in hand. Nothing regresses today, because the composition root still builds a builder per fetch. S13's own acceptance criterion already records container as non-conformant and names #596. - S16's three gap records, which say the container request builder is not hoistable. S16 delegates the choice of fix, so only its bookkeeping is stale.
- The plan's sentence that the flight-backed constructor "applies the same bounds
and validation as
NewRemoteOperations", which the validator split makes false in its literal reading. The step's tests never encoded that reading.
Related to #291