feat(oci): fill a by-digest manifest miss from the upstream (S16 plan: 14/26)

Radamanthus Batnagrequested to merge
rbatnag/container-remote-step-14-2 into main

Reviewable size

22 files, +3,939 −256, measured three-dot against the merge base with main. Well past the 500-line ceiling in development-model.md, so guardrail 17 wants the split written out:

Group Lines
Production Go +1,236 −184 — remote_manifest.go 574/7, wire_oci_remote.go 343/18, remote_serve.go 158/33, remote_relay.go 52/23, remote_operations.go 25/19, remote_errors.go 19/15, manifest_read.go 14/0, wire_oci.go 12/36, remote_cache_store.go 9/8, remote_download.go 8/8, wire.go 7/6, remote_list.go 6/3, remote_auth.go 6/5, remote_requestbuilder.go 3/3
Tests +2,628 −58
Run recipe (.claude/skills/) +75 −14

Why not split further. Two thirds of the diff is coverage. The production half is two pieces that cannot be reviewed apart: the fill itself, and the composition root that builds it. A fill with no wiring is unreachable and its refusal-for-an-unwired-boot untestable; wiring with no fill is a factory returning nothing. Step 14 is already split four ways for exactly this reason — it measured 1,883 reviewable lines as a single MR before revalidation was written — and the cut runs along which reference kind fills, not along the fill's own seams. Cutting the composition root out would leave a third part whose only content is a constructor.

Seven of the 14 production files are comment-only — remote_download.go, remote_errors.go, remote_operations.go, remote_cache_store.go, remote_requestbuilder.go, remote_auth.go, and remote_list.go. This MR is the first production caller for several surfaces that shipped with "nothing calls this yet" written on them, and guardrail 21 wants those corrected in the change that falsifies them.

Why

Step 14-2 of the S16 container remote plan, the second of the step's four MRs. 14-1 landed the refusal surface and settled the retry-hint rule with nothing fetching behind it. This part lands the fetch: after it, a by-digest manifest miss fills from the upstream. A tag miss and a stale tag stay on the interim 501 until 14-3 and 14-4.

What

The by-digest cache-miss fill. Accept is forwarded verbatim, one field line per element. The body is drained to io.Discard rather than to the client, because the cache fill commits at end-of-body and the digest comparison, the payload classification and the body cap all report from that final Read — so sinking the bytes keeps every refusal ahead of the first byte of response, where an OCI error envelope is still writable. A caller that streamed to the client would be committed to a 200 by the time any of them reported.

The answer comes from the committed row, never from the upstream response. After the commit the arm re-reads through LookupRow and hands off to the same answerFromCache a cache hit uses. That is what makes content negotiation, header discipline and the download signal identical on a hit and a cold fill, and it is why an upstream Content-Type that contradicts the payload's own mediaType loses: the served type is the payload's, classified from the bytes.

No validator is sent. An immutable reference names the bytes it wants, so there is nothing for a revalidation to establish, and hasCache is passed false rather than consulted — a read that reached the fill found no row at all.

Composition root. ociRemoteWiring and ociRemoteWiringFrom, the buildOCIRemoteFill factory with a named refusal per non-positive bound, a refusal sentinel for an upstream-less boot, and ociRemoteManifestBodyBounds, which resolves the fill's bound to the minimum of container.manifest_max_payload and body_size_cap_metadata. The wiring struct is threaded from wireServices down through wireOCI and wireOCIWithStore.

headResponseWriter.Unwrap. A HEAD is served through that wrapper, so http.NewResponseController stops at it and every arming fails with http.ErrNotSupported — silently, since a failed arm is logged and serves on. HEAD is what a container client issues before it pulls, so this is the method that mattered. GET alone cannot see the bug: it reaches the recorder directly.

499 for a client that left. context.Canceled is not fallback-eligible and classifies as 500 INTERNAL, which is right as a classification and wrong as a response — an interrupted docker pull is routine and would otherwise file an ERROR and move the 5xx rate for every one of them. 499 is stamped for the access log and no envelope is written, because there is nobody left to read one.

The guard covers the cached serve's three storage legs as well as the fill's own failure arms: the HEAD's sizing call, and the stream's OpenBlob and content.Reader(). Those run after the commit, by which point the fill has already spent up to its whole configured upstream budget, so a client that gave up in the meantime is the likely caller rather than the edge one. Each check runs on the failure rather than ahead of the call, because the case that matters is a cancellation landing during the object-store round trip. The two malformed-column checks that precede them keep their unconditional ERROR: a corrupt row is worth reporting whether or not the client stayed. The warm read shares the code and gets the same treatment; remote_blob_serve.go's equivalents are untouched, since the blob arm has no fill yet.

Two refusal log lines no longer render error_message: null. MapFetchOutcome mints a 503 off the upstream's status alone for a 405, a 407, a 426, an undecoded 200 and a 3xx, and relays a 429, 502 or 504 the same way, passing no error with any of them. logging.md requires one type per field name across every entry, so both writers omit the field instead of flipping its type. writeInternalFault already drew that distinction. And the failed write-deadline arm now carries the four subject identifiers: it is the only record that a fill ran unarmed, because OCI has no counterpart to npm's remote_response_deadline_arm_failures_total counter.

Worth a reviewer's attention

A digest mismatch refuses the row, not the staged upload. The spec's Digest verification section promises "the staged upload is discarded and no row is written". Only the second half holds: internal/remote/fetch.go commits the CAS object before UpsertCacheEntry is offered the row, so bytes that hash to the wrong digest do land in the object store, and nothing in the tree reclaims them today: the gc:reconcile-scan sweep ADR-025 designs is Proposed and no gc:* implementation is merged. Nothing reads them back — no row points at them and the next request re-fetches — so the refusal is correct as an answer. This MR is the first production caller to reach the gap, so commitUpstreamManifest now says so at the site and points at #433, which tracks putting an expected digest on the commit itself. Inherited, not introduced here, and out of scope for this MR.

A cold fill moves the manifest across the object store three times. The staging write during the drain, classifyCommittedManifest's read-back to resolve media_type and length, and the serve's OpenBlob. The read-back sits inside the commit on the request critical path, so a stalled object store can refuse a fill whose CAS object already landed, and the client's retry re-fetches from the upstream. It is bounded — one time per cache entry, and a manifest is a few KB — and the drain does hold both values it re-reads, so capturing them there would remove the middle pass. Recorded here rather than changed because the blob arm inherits this pipeline shape and should inherit a measured cost with it.

Two accepted internal/remote gaps compose on this path, and it is the first OCI caller to reach either. The fill composes remote.Fetcher directly, so resolution.max_concurrent_fills_per_repo is not enforced at all — that semaphore lives inside remote.SingleFlight, which Maven and npm both compose and this arm deliberately does not (#752). Separately, the fill's terminal Session.Commit runs on a context with no deadline at all (#396). Together: a stalled object store parks fills with no per-repository ceiling, each holding its pooled connection, and the response write deadline this MR arms gives no cover because the block is on a read rather than a socket write. Both halves are documented and tracked; what is new is that they now sit on one request path. remote_operations.go carries the #752 pointer, and per the once-per-obligation rule it is not repeated elsewhere in the tree.

The fill admits manifests up to 4 MB; ADR-004 documents 250 KB. ociRemoteManifestBodyBounds resolves to min(container.manifest_max_payload, body_size_cap_metadata), and manifest_max_payload defaults to 4 MB in internal/config/container.go — matching the hosted ceiling in internal/format/oci/manifest.go, both merged with S12. ADR-004's limits table says 250 KB. Nothing in docs/specs/ or docs/dev/ reconciles the two. This MR is the first place that value bounds a remote upstream fetch, which is why it is flagged here; the remediation is a handbook ADR-004 amendment or a config correction, neither of which belongs on this branch. It is tracked as gap 4 of #279, "Manifest payload limit value mismatch".

Plan changes are in a separate MR

Per guardrail 5, step MRs do not edit the plan file. This branch carries none.

Review this against the Step 14 entry in !1831, not main's. main's entry still costs Step 14 as one MR and has no per-MR table, so it gives no Files list and no criteria allocation for this part.

Two annotations in that entry's Files bullet are behind the code and want a follow-up on !1831 rather than a change here:

  • wire.go and wire_oci.go are annotated "Modify in 14-1" only, but the same entry's Est. bullet puts the wireServices → wireOCI → wireOCIWithStore signature hunks with 14-2's composition-root work, which is what the code does. The ociRemoteWiring struct cannot reach wireOCIWithStore otherwise.
  • remote_serve.go is annotated "Modify in 14-1" only, but the entry's own narrative ("after 14-2 a by-digest miss fills") requires the changes here.

The miss path's download signal also ships here rather than in 14-3, because answerFromCache is shared by the hit and the fill and cannot be reached one without the other.

Merge order

Targets main. The parent step !1869 (merged) has merged and this branch carries it, so the earlier stacking note no longer applies.

!1808 (merged) has merged, and this branch does not carry it yet. It added FetchOptions.ExpectedDigest and carried it into storage.Session.Commit, which already compares and already rolls the fill back on a mismatch. The relation is removed: it was only ever for ordering, so that the seam reached main ahead of the slice that wires it, and it has.

This MR passes no expected digest, and will not once main is merged down: RemoteOperations.FetchManifest takes no options, and ExpectedDigest appears nowhere in internal/format/oci. So the orphan window is the same on either side of that merge. A by-digest fill whose upstream serves bytes hashing to some other digest commits the CAS object, refuses the cache row through the cache store's own comparison, and leaves the object for the reconciliation scan.

Closing it needs the by-digest fills wired to pass the digest, plus an isRefusedUpstreamContent arm for remote.ErrUpstreamDigestMismatch — without that arm, passing the digest alone would move the refusal from 503 to 500, which is not what Error Cases asks for. S16's Follow-ups assigns both to whichever slice wires the container fetch helpers, and names no work item; commitUpstreamManifest's comment points at that section rather than at #433, whose own description now disclaims the container-side half.

Open MRs overlapping these files:

Whichever lands second rebases; no pipeline reports it.

Guardrails with nothing to add

  • Conformance tests. The OCI conformance suite runs against hosted repositories and cannot reach a kind=2 route, so it cannot cover this behavior.
  • e2e scenario catalogs. docs/testing/e2e/oci.md puts virtual and remote repositories out of scope until the capability ships, so no scenario is added or affected.
  • Configuration reference. No schema, loader, or config.example.yaml change. ociRemoteManifestBodyBounds is a Go-side selector over fields that are already documented.
  • Bruno / OpenAPI. No operation added, renamed, or removed.

Run recipe

.claude/skills/run-artifact-registry/SKILL.md gains #### Driving the by-digest manifest fill, and driver.sh's header comment narrows its "no fill path yet" claim to the blob arm, which is now the only arm that has none. driver.sh smoke deliberately asserts none of the new rows — they need live Docker Hub — and the section says so.

Related to #288

Edited by Radamanthus Batnag

Merge request reports