feat(oci): coalesce and cap container remote fills, compose virtual reads (S32 plan: 14/19)
This is S32 Step 14, the composition root. It builds the container virtual resolution at boot and puts the standalone container remote fill behind the shared per-repository flight registry. Standalone kind=2 container reads now coalesce, stop at the per-repository fill cap, and can answer 503 once the fill wait expires. Both virtual read routes still answer their interim 501: Steps 15 and 16 swap the handlers in.
Merge order. This MR targets main, and main is merged into this branch at f613aa170 (a merge, not a rebase, because Step 15 is stacked on this branch). Three open MRs overlap it:
- !2947 (merged) (Maven virtual composition root) adds the same
remoteAuditLogSink.RecordUpstreamExclusion. The method is byte-identical to !2947 (merged)'s atbd1ea55e5, so the text conflict inremote_audit_sink.gohas a single answer. Whichever MR merges second collapses the S30 and S32 rows indocs/dev/observability.mdinto one, resolves.claude/skills/run-artifact-registry/SKILL.md, and settles the union placement ofremote.UpstreamExclusionAuditSinkand theauditSinkWithoutExclusiontest. - !2856 (merged) conflicts in
.claude/skills/run-artifact-registry/SKILL.md. Once its container upstream association routes land,seed_container_virtual's comment that no route writes the upstream list goes stale, so the MR that lands second updates it. - !2944 conflicts in
docs/specs/S16-container-remote.md.
Each of the three merges cleanly into main, so whichever lands second resolves the conflict.
What changes
Composition (the plan's Step 14):
wire_oci_virtual.gobuilds one flight registry and one upstream authenticator per boot, shared by the standalone and virtual paths. It also builds the resolver over the production audit sink, and the upstream assembly over the sharedupstreamhttpDoer.VirtualPosition.RemoteRepositorycarries a bound remote position's row, andociVirtualWiring.ReadCachesbuilds a winner's read cache from it, the two surfaces Steps 15 and 16 fillvirtualServeRead.rowsfrom.- A bad resolution knob fails the boot instead of silently serving the interim
501. remoteAuditLogSink.RecordUpstreamExclusionrecords each excluded virtual position asvirtual repository upstream exclusion, withposition_index(the 0-based list index, never the storedposition) andresolution_phase.
Behavior that changes on existing kind=2 container reads, because they now fetch through the shared flight:
- Concurrent cold reads of one path coalesce onto one upstream
GET, and fills cap atmax_concurrent_fills_per_repoper pod. - A read queued past the cap, or behind a slow leader, answers
503withRetry-After: 1oncesingle_flight_wait_timeout(default5m) expires. - A follower whose leader leaves re-issues its fetch once. A follower that has written nothing when it learns of the loss mid-stream answers
503withRetry-After: 1instead of500. A follower already streaming the layer gets a truncated response. - The response write deadline is armed before the fetch, with the wait term, and re-armed once the fetch returns. The second arm is
remote.FillTailBudget,75sat the defaults, so a client that stops reading holds its connection for the tail, not for5m45s. - Coalesced followers book no upstream-response or fill samples.
oci_remote_proxyrecords carrycoalesced. - A new counter,
oci_remote_fill_wait_expiries_total{fill_wait="slot"|"follower"}, shows fill-slot saturation. - A follower that gets the leader's committed blob (
remote.FetchResult.CommittedRead) closes it unread and answers from the row, as a cache hit does. A plainGETfollower in redirect mode therefore answers307, and the manifest arm no longer reads the committed manifest twice.
Plan and spec amendments
- The plan's Step 14 entry does not list the committed-read skip or the injected mint constructors (
ociRemoteMints). These came from a branch review. - The exclusion audit record keeps the plan's
virtual repository upstream exclusionmessage. Its keys,position_indexandresolution_phase, come from neither the spec nor the plan, and !2947 (merged) writes the same ones, so one audit query covers container and Maven virtual. remote.SingleFlight.SetBeforeFollowerWaitmoves fromexport_test.gointo production code, andoci.FlightRegistryDeps.BeforeFollowerWaitinstalls it. Both are documented as test-only, so the leader-loss integration tests can park followers on a signal instead of a sleep.- S13, S15 and S16 are restated where they still described the container fill as uncoalesced: the #752 (closed) gates on S16's coalescing criteria, the request builder called unhoistable, the follower-model widening,
coalescedas not emitted, and Container answering500on an abandoned fill. S16 §Metrics gainsoci_remote_fill_wait_expiries_total. - S16's Follow-ups bullet for #596 (closed) is dropped on purpose. #596 (closed)'s work, a request builder that holds only its authenticator and position, landed with Step 8 (
internal/format/oci/remote_requestbuilder.gois identical onmain), so the issue can be closed. - The composition builder is named
buildOCIVirtualSlots, not the plan'sbuildOciVirtualSlots: Go initialism casing only. - The probe/resolve ordering check stays in
virtual.NewResolver, so the boot, not config load, refuses a bad pair. This follows the placement fromc0f0e5d4b.
Size
3,889 changed lines against main, over the 500-line guideline (guardrail 18). By file group:
| Group | Lines |
|---|---|
| Go tests | 2,479 |
| Go source | 924 |
Run recipe (driver.sh, SKILL.md) |
220 |
| Docs and config | 214 |
| Specs | 52 |
Splitting would not help review. The composition and the behavior it changes on the standalone arms have to land together, or the flight exists with nothing in front of it. Tests are about two thirds of the diff, most of it integration cases against a real database.
Test evidence
go testforcmd/artifact-registry,internal/format/oci/...,internal/remote/...,internal/virtual/...,internal/metrics/...andinternal/config/...: pass, andcmd/artifact-registryalso passes under-race.go test -tags=integration ./cmd/artifact-registry/ -run 'TestWireOCI|TestBuildOCI|TestRemoteAuditLogSink': pass. The abandoned-leader and canceled-leader cases fail against the pre-fix code, and the two leader-loss cases passed three runs in a row once parked on follower joins.golangci-lint, untagged and with--build-tags=integration: clean on every file in the diff.scripts/ci/check-comment-caps.sh: OK.driver.sh smokefrom a fresh run dir after merging main atf613aa170: 80 passed, 0 failed, including the six virtual-route interim-501checks.- Conformance:
.conformance-ocidoes not exercisekind=2orkind=1reads. The integration cases above cover the coalescing behavior instead. - The
run-artifact-registryedits are here because guardrail 19 and the plan's Files entry require them in this MR.skill-benchmarkresults have not been run yet.
Scenario and user docs
docs/testing/e2e/oci.mdgainse2e.oci.remote.blob-single-flightande2e.oci.remote.blob-fill-cap-wait, because composing the flight changes standalone remote behavior.docs/user/troubleshooting.mdnames the fill-wait503as a second cause of a503from a remote repository, and says a pull already receiving a layer gets a truncated response when the shared fetch's client disconnects.
Related to #291 (closed)
Closes #752 (closed)