feat(oci): the virtual resolution facade and its error mapping (S32 plan: 11/19)

Summary

Step 11 of the S32 container-virtual plan: the resolution facade, and the mapping from S13's resolution taxonomy onto the OCI envelope.

Three new files:

  • internal/format/oci/virtual_resolve.go — one call that takes a namespace, a virtual repository, and a target, and answers a resolved read or a mapped error, plus the remote.FetchOptions a virtual read runs its winner's Fetch under (the size-cap class per reference shape, and the expected digest read off the assembled path rather than derived a second time).
  • internal/format/oci/virtual_errors.go — the mapping. One arm per resolution verdict, wiring sentinel, propagated sentinel, and addressing fault, and the four Retry-After classes.
  • internal/format/oci/virtual_errors.md — the sidecar arguing the three points where this mapping departs from npm's and from remote_errors.md, and the two that look like departures and are not.

The resolution deadline the step originally carried is not here: #1139 (closed) landed resolve_timeout, the remote.ResolutionConfig field, the sentinel, the configuration mapping, and the budget mechanism in internal/virtual/resolve.go before this step opened. This MR consumes all six and writes none of them.

Three files sit outside the plan's Files entry:

  • internal/format/oci/remote_errors.go (+8/-2), in two hunks. remoteFillShedRetryAfterSeconds() int is extracted from the existing remoteFillShedRetryAfter() string, which now calls it, so the virtual arm can stamp the header off an int. The second hunk replaces the literal "1" in mapRemoteFetchFailure's coalescing arm with the new named constant. That arm is a live standalone-remote serving path; the change is value-preserving (strconv.Itoa(1) == "1"), and remote_errors_test.go already pins UpstreamRetryAfter as the literal "1" from the other side.
  • internal/format/oci/remote_relay.go (+3): the remoteCoalescingRetryAfterSeconds constant that hunk names.
  • internal/format/oci/virtual_assembly_test.go (+6/-1): one test function, and the plan books both the file and the acceptance criterion it covers to Step 10b. TestVirtualUpstreamAssembly_DeniedAndHealthSkippedPositionsAcquireNoFlight asserted nothing about the health gate: the fixture's default doer answers 404, so an ungated unhealthy remote at position 1 would be probed, miss, and leave the hosted hit at position 2 winning regardless, and a probe builds no flight either way. Verified by mutation rather than by reading: writing remote.HealthUnknown for every row in virtualHealthStatuses left that test green, and with cvaProbeHit the same mutation fails it at winner fetch at position 1. virtualHealthStatuses and VirtualHealthStatusReader are untouched. The criterion was not unguarded in the meantime, since this MR's own TestVirtualResolution_AnAllUnhealthyListIsNeverProbed already fails under the same mutation.

Governing ADRs

  • ADR-004 (data and application limits): container.manifest_max_payload is applied to every manifest-class fetch, and a non-positive value is refused at construction rather than at first read. A relayed 429 always carries Retry-After. Conforms.
  • ADR-008 (content-addressable storage): ExpectedDigest is taken from the assembled path's last segment, already canonical sha256:<hex>. A tag-kind read carries none, which is correct: nothing is content-addressed at that point. Conforms.
  • ADR-007 / ADR-001: every resolution carries NamespaceID beside ContainerVirtualRepositoryID, so the read stays inside the namespace the composite foreign key enforces. Conforms.
  • ADR-009 (API design): adds no route, method, or client-facing URL, and introduces no OCI error code. It maps onto codes this package already declares. Conforms.
  • ADR-021 (authorization): no authorization decision here; authorization runs before kind is read. The 404 on remote.ErrAllUpstreamsDenied is an upstream allow/deny rule verdict, not an authorization one. The 500 arm composes no envelope field from the error, so no upstream host, URL, or response header reaches a client.

Mirror freshness re-measured on 2026-09-23 with scripts/adr-freshness.sh: STALE on one unsynced commit, which adds pipeline_id to ADR-020's gitlab.job claim and touches 020_authentication_flow.md alone. This change makes no authentication decision and reads no token claim, so it is unaffected. One internal ADR never mirrors locally and is not checked here.

Reviewable size

3070 reviewable LOC, past the 500 development-model.md asks a justification for. The split by file group:

Group LOC
Non-test Go 563 (virtual_errors.go 362, virtual_resolve.go 188, remote_errors.go 10, remote_relay.go 3)
Tests 2433 (virtual_errors_test.go 1524, virtual_resolve_test.go 653, virtual_resolve_internal_test.go 249, virtual_assembly_test.go 7)
Prose 74 (virtual_errors.md)

Splitting would not help, because the seam a split would cut is the one thing this step exists to hold. The taxonomy and its mapping are one artifact: virtual_errors.go answers one arm per value internal/virtual and internal/remote can return, and the acceptance criterion is that every such value has a named arm. Landing half the arms means landing a mapping whose fall-through silently answers 500 for the other half, which is the exact failure the per-value discipline exists to prevent. The suite is four fifths of the diff and is derived from that taxonomy rather than written alongside it: virtualFallbackExclusionSentinels walks the AST of remote.FallbackEligible and diffs its return false arms against the table, so the rows are not a list a reviewer has to audit by eye. virtual_resolve.go is the one call the mapping hangs off, at 188 lines.

Testing

Test-first: test(oci) lands the suite and two panic skeletons, feat(oci) the implementation.

  • The propagated sentinel set is derived, not transcribed. virtualFallbackExclusionSentinels walks the AST of remote.FallbackEligible in internal/remote/errors.go and diffs its return false arms against the table, so a sentinel added to that predicate fails this test instead of shipping a 500. The two arms the predicate cannot see, remote.ErrFillAbandoned and virtual.ErrZeroValueCoordinate, are named against the sentinels themselves.
  • Every no-winner branch is asserted by name, not counted: NoUpstreams and AllUpstreamsDenied → 404, AllUpstreamsUnhealthy and UpstreamUnavailable → 503, AllUpstreamsIneligible → 500 INTERNAL.
  • A malformed tag is driven through NewVirtualCoordinate, not injected. ErrVirtualTarget answers the route's own 404 MANIFEST_UNKNOWN per S32's Error Cases row, and the test builds the coordinate rather than handing the mapping the sentinel, so it pins the chain and not just the arm. ErrVirtualHostedReference stays on 500, and the comment says why.
  • ErrResolverCancelled and the budget expiry are two arms, not one, with a test per cause: a departed caller gets 499 and no envelope, a budget expiry gets 503 with Retry-After: 1 and a parseable body. The expiry keeps its own log record, since resolve_timeout is the one condition here an operator sets.
  • The four Retry-After classes are asserted separately: the health sweep (300), the coalescing class (1), the deterministic floor (5), and the jittered fill-shed window.
  • Every content-refusal sentinel runs bare and wrapped in remote.ErrCacheFill, the shape production produces. Without the wrapped row, a reorder letting the ErrCacheFill 500 claim those refusals first turns their 503s into 500s with the suite still green.
  • Every 500 the mapping owns is recorded as named. virtualServerError covers this package's own 500-class values alongside the ones it receives, so a half-set credential, an auth status outside the taxonomy, a failed discovery write, a manifest past the read-back ceiling, and a facade that bypassed its constructor no longer record as reasons the taxonomy does not name. The same split applies at the disconnect guard: the 499 is unchanged and only the record moves.
  • The digest precondition is asserted here rather than left to remote's panics. Every digest-addressed read's ExpectedDigest validates, names the canonical algorithm, and equals its path's last segment, which is SingleFlight.Fetch's own rule, so a future change to the assembled path's format fails in this package. Every other shape names no digest, with the reason an empty value is correct recorded against a later attempt to salvage a partial one.
  • The winner's non-2xx is a second table, one row per status mapUpstreamStatus names by value, plus an unnamed 3xx and an undefined status. A relay decision no response can carry fails closed to this arm's own 500, which is the obligation RemoteFetchOutcome's doc assigns to the renderer.
  • Three positional fixtures: a failing higher position with a 404 at a lower one answers 404; a pull whose only cached copy sits at a denied position does not serve it; and a data error at a higher position withholds the 404 a lower one established, with a control proving the same list answers 404 once the defective row is repaired.
  • The redaction suites cannot pass vacuously. Both assert an absence, so a fixture that stopped embedding the credential would pass them whether or not any arm redacts. One case fails if that fixture ever goes inert.

The five resolution-deadline cases and the configuration round-trip are #1139 (closed)'s and already on main, in internal/virtual/resolve_budget_test.go and internal/config/virtualrepositories_resolvetimeout_test.go.

Local: go build ./... and go test -race ./internal/format/oci/... ./internal/virtual/... pass, golangci-lint run ./internal/format/oci/... reports 0 issues, and scripts/ci/check-comment-caps.sh is clean. golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/format/oci/... reports 8 issues, all in virtual_assembly_integration_test.go and all pre-existing on main, which this branch does not otherwise touch.

No e2e scenario is added: nothing routes to this facade until Step 12's serve surface and Step 14's composition root. No docs/user/** change either, for the same reason — no customer-visible surface is reachable yet.

Deferred

  • Widening RemoteOperationsDeps.ManifestMaxPayload's "both constructors" sentence to cover NewVirtualResolution, which takes the same pair. The sentence sits inside a 22-line comment block that predates the comment caps, and touching it charges the whole block, so it goes to the wiring step that makes the claim wrong rather than being rewritten here. Booked on Step 14 by !2904 (merged).
  • The virtual repository's identity on the mapping's own records. logVirtualMappedFailure emits error_message, plus upstream_status on the winner refusal, and writeVirtualReadError takes no attrs, so an operator asking which virtual repository is 503-ing joins to the access log on correlation_id. Step 17's oci_virtual_resolve is the natural home, but its acceptance names winner_repository_id rather than the virtual repository being read, so this is not closed as written and Step 17 should pick it up. Adding the attrs parameter here would be a seam with no caller until Step 12 mounts the route. Booked on Step 17, as namespace_id and repository_id on the error mapping's records, by !2904 (merged).
  • The cache-fallback half of a failed token exchange. The new *UpstreamAuthError arm answers the statuses S16 fixes, but it maps on hasCache false, so a status that produced no token never serves a cached copy the way the standalone arm can. Whether it should is a question about remote.FallbackEligible in the foundation rather than about this mapping, so it is not closed here. Tracked in #1431.
  • The relayed Retry-After clause of S32's WinnerFetchFailed row. remote.WinnerFetchFailedError carries only Status, so an upstream's own hint cannot reach this relay and the deterministic floor goes out instead. Tracked in #687.

Related to #291 (closed)

🤖 Generated with Claude Code

Edited by Sylvia Shen

Merge request reports

Loading
Loading