feat(oci): serve cached container remote blob reads (S16 plan: 11c/26)

Why

Step 11c of the S16 container remote plan, the last of the step's three MRs. 11a landed the download-signal seam and 11b the shared response-header writer, the digest gate, and the manifest arm. This one fills the remaining read slot: a kind=2 blob route that resolves a cached row now serves it instead of answering the dispatcher's 501.

What

RemoteBlobHandler.ServeRemoteRead serves an already-cached blob. It runs S12's digest grammar itself before the cache lookup, because the hosted per-action handlers that run it sit below where the dispatcher branches on repository kind. On a hit it honours the ADR-005 delivery mode — 307 under redirect, streamed under proxy — and forces no stream, so the operator's setting decides. It composes 11b's writeRemoteReadHeaders rather than assembling headers of its own, handles Range (206, 416 with Content-Range: bytes */<total>), answers HEAD from BlobInfo without opening anything and never redirects it, and records one download signal per delivered read. A miss still answers the interim 501; Step 15 replaces it.

cmd/artifact-registry/wire_oci.go wires the arm into Step 10.5's WithRemoteBlob slot, and the boot guard asserts the slot is filled by distinguishing the arm's own fill 501 from the dispatcher's slot-unwired one.

The plan changes are in a separate MR

Per CLAUDE.md guardrail 5, step MRs do not edit the plan file. This branch carries none; its Step 11 corrections are in !1762 (merged), alongside 11b's.

Read !1762 (merged)'s Step 11 section, not main's, to review this MR against the plan. The entry on main is the uncorrected one: it says 11c modifies remote_serve.go where this branch creates remote_blob_serve.go, and gives 11c criteria 38 and 39, which 11b owns outright. !1762 (merged) is the corrected version and carries this MR's Files cell, its criteria allocation, and the record of what 11b's two review rounds cost this arm.

The two have no merge-order dependency in either direction and touch disjoint files — !1762 (merged) changes only docs/plans/, this changes nothing there — so they cannot conflict and can land in either order.

Merge order

Clear. 11b (!1707 (merged)) has merged, so this no longer stacks on anything: it is rebased onto main and targets main, and the diff below is this arm's own work rather than 11b's carried along.

The Draft flag was set because the base was about to move. That reason is spent — the base has moved and the branch is on it.

Reviewable size

15 files, +3,711 −68, measured three-dot against the merge base the way GitLab counts it. Past the 500-line ceiling development-model.md sets, so the justification, by file group:

Group Lines
remote_blob_serve_test.go — digest gate, cache-hit serve, delivery mode, failures, signals 1,312
remote_blob_serve_fixtures_test.go — fixtures, fakes, harness 735
remote_blob_serve_range_test.go — Range semantics 319
internal/format/oci/remote_blob_serve.go — the arm 679
Run recipe (driver.sh, SKILL.md) 387
Wiring (wire_oci_remote.go, boot test, wire_oci.go, wire_oci_remote_test.go) 140
Doc comments, the shared resolver's parameter, the goconst hoist, the shared copy-failure emitters both arms call 139

Two thirds is test code. Splitting it across MRs would separate the arm from the suite that exercises it, and the plan's own Ships as three stacked MRs argument already cut Step 11 at the two seams that produce reviewable units — a third cut would put the arm in one MR and its tests in another.

The suite is split across three files rather than one, which is the cut !1707 (merged) was asked to make on the manifest arm and made — expressly so this arm would not grow a two-thousand-line suite. It had grown one (2,183 lines) before this branch was rebased onto that round. Both cuts follow banners the file already carried, and the resulting shape matches the manifest arm's almost line for line (1,219 / 704 / 206 there, 1,312 / 735 / 319 here).

No test body changed and none was added or dropped. The top-level declaration lists before and after the split are identical, as are the 44 subtest row names; both were diffed rather than read.

Test plan

internal/format/oci/remote_blob_serve_test.go and its two siblings cover the arm end to end: the digest gate ahead of the cache lookup (five refusal rows, including the sha512: prefix taking 400 DIGEST_INVALID on this route where the manifest route takes 404), the cache-hit serve under all three delivery-mode values, Range in four RFC 9110 forms, the 416-on-GET against 200-on-HEAD pair through one fixture, Content-Length equality across the verbs asserted against each other rather than a literal, and one download signal per delivered read with none on a 416, a mid-stream failure, a short copy, or any 500.

cmd/artifact-registry/wire_oci_boot_integration_test.go adds the blob slot's boot guard.

No e2e scenario is added or affected. Both docs/testing/e2e/oci.md and docker.md list "Virtual and remote (proxy/cache) repositories" under Out of scope until the capability ships, and it has not: no fill path exists, so a client pulling from a remote repository still gets the interim 501 unless a row was seeded out of band. Container/OCI conformance is unreached for the same reason and has not been run.

driver.sh smoke: 24 passed, 0 failed, re-run from a cold start on the current tree, per guardrail 20. The recipe's previously recorded run predated three changes to the arm, one of them a new refusal path, so it was re-run rather than relied on and the recipe now records what this one covered.

The same boot drove the arm itself through seed-remote, which is what that subcommand exists for. Every answer it produces was checked against a database that had just been created:

Request Answer
GET a cached blob 200, Content-Length: 7296, Docker-Content-Digest, Content-Disposition: attachment, nosniff
HEAD the same 200, same Content-Length and digest, no body
Range: bytes=0-9 206, Content-Range: bytes 0-9/7296, Content-Length: 10
Range: bytes=999999- 416, Content-Range: bytes */7296
uncached digest 501 with Cache-Control: no-store
malformed digest 400 DIGEST_INVALID, detail invalid digest format

For LLM Agents

Detail a reviewer does not need but an agent picking this up does

Branch state

Rebased onto main past 11b's merge with git rebase --onto origin/main e20060fee, which replays only this branch's own commits — 11b merged as squash 8e76e78a8, so replaying from the old base would have re-applied its whole unsquashed series. Backup ref backup/11c-pre-rebase-20260820 holds the pre-rebase tip 550606799, which is what this MR showed before.

Measure this diff three-dot (git diff origin/main...HEAD). Two-dot picks up whatever landed on main after the merge base as spurious deletions — at the time of writing that is 1,657 lines of lifecycle_reap_maven_remote*, which this branch does not touch.

The recurring conflict during that rebase was cmd/artifact-registry/wire_oci.go with an empty "ours" side: 11b's second round moved buildOCIRemoteManifestHandler and the staleness selector into a new wire_oci_remote.go, so every commit here touching the blob builder replayed as "add that whole block back to wire_oci.go". Resolution each time is to take only buildOCIRemoteBlobHandler from the incoming side and put it in wire_oci_remote.go. ~/resolve-wire-oci.py automates it.

Five shapes shared with the manifest arm, across two review rounds

11b's review rounds produced fixes true of both arms, and the branches conflict only on lines both edit — so a shape written separately produces no conflict and nothing reports it. The first round found three, all discharged here:

  • Short copy. streamCachedBlob judges io.Copy's count as well as its error; a clean copy that moved fewer bytes than the row promised is reported undelivered, so the download signal does not fire and hold a truncated payload out of eviction. Logged at Error with fields.WrittenBytes, matching maven.logCopyOutcome, npm.recordCopyOutcome, and the read-back behind errRemoteCacheReadBackLengthMismatch. The manifest arm still logs this at Warn with copied_bytes and owes the raise — that one is 11b's, not fixed here.
  • Exported zero value. NewRemoteBlobHandler's doc says the nil-seam check closes the constructor path and no other, with a matching row in TestNewRemoteBlobHandler_NilSeamPanics.
  • Parent repository id. 11b added repositoryID to remoteReadSubject. The blob arm builds that struct with a keyed literal, which compiled unchanged and left the field at the zero UUID — every blob-read failure logging repository_id=00000000-0000-0000-0000-000000000000, worse than an absent field because it looks populated and matches every tenant. The rebase conflicts on the doc comment above the struct and not on the literal, so it surfaces the harmless half and hides the harmful one. Fixed, and requireRemoteBlobServeSubjectAttrs now takes the parent id as a parameter and requires it — a parameter rather than a package constant because the fixture mints one per test, so a zero value matches none of them.

A second round on 11b found two more, and moved three files. The two shapes:

  • Disagreeing sums on a cache row. serveCachedManifest built Docker-Content-Digest from the row's digest column and opened the payload at blob_sha256 with nothing checking the two hold the same sum, so a well-formed but disagreeing pair advertised one digest and streamed the bytes the other addressed. serveCachedBlob reads the same two columns the same way and had the same hole. It refuses such a row now as the 500 it already sends for a corrupt one, with a disagreeing-pair row in TestRemoteBlobServe_StorageFault_500 that seeds a well-formed digest addressing no seeded object while blob_sha256 still addresses the real one, so nothing else on the route faults and only the equality refuses it. Mutation-checked: without the guard that row serves a 200 carrying the wrong Docker-Content-Digest. The justification is this route's own rather than the manifest arm's borrowed — a layer has one representation, so its OCI digest is the sha256 of the bytes blob_sha256 addresses, where the manifest arm has to argue it from every upstream fetch asking for the identity representation.
  • The lifted repository resolver. 11b moved resolveRepository and streamCachedManifest off *RemoteManifestHandler to package level explicitly so this arm would not copy them. This branch's own (h *RemoteBlobHandler) resolveRepository is deleted and it calls resolveRemoteRepository instead. That left one thing open: the log message inside it read oci remote manifest read: … and would have reported a blob read's failure under the manifest arm's wording. It takes the message from the caller now and each arm passes its own, so an operator filtering by route gets the route they filtered for. A parameter rather than a label on remoteReadSubject, because the failure happens before a subject exists and each arm has exactly one such line.

The three moved files are not shapes but they are what a stale Files list gets wrong: buildOCIRemoteManifestHandler and the staleness selector left wire_oci.go for wire_oci_remote.go, that selector's test left wire_oci_test.go for wire_oci_remote_test.go, and remote_serve_test.go split its fixtures and header-writer cases out. None changes what this arm does; all three change where it does it. That is the half a rebase reports as a conflict and a plan written before the round does not — which is why !1762 (merged) needed a second pass.

Two files outside the arm

internal/format/oci/blob.go and upload.go take two new constants. The gate's five refusal rows push "invalid digest format" to five occurrences and "unsupported digest algorithm" to three, crossing goconst's threshold — and goconst anchors the finding at the production site, so the _test.go$ exclusion does not help. Both reasons are hoisted beside digestInvalidReason, the same move the "reference" detail key took. The package reported 3 issues before this and reports 0 now.

cmd/artifact-registry/wire_oci_remote.go and wire_oci_remote_test.go: the blob arm's builder goes beside the manifest arm's, which 11b's second round moved there, and the blob arm takes its staleness window through the selector the manifest arm already uses rather than reading the config field inline — so remoteManifestStalenessWindow becomes a shared remoteStalenessWindow and its test is renamed. One selector rather than one per arm: the guard is against a call site naming the wrong field, and both arms are covered the moment neither names a field at all. wire_oci.go keeps only the slot append, two lines.

Verification run

Re-run after the rebase onto main and the two round-two commits, not carried over:

go build ./... clean · gofmt -l empty · go vet clean · go test ./internal/format/oci/... ./cmd/artifact-registry/... 2,687 passed across 3 packages · golangci-lint 0 issues on both packages, re-run after the suite split.

Integration-tagged lint (--build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false, per guardrail 7) reports 12 findings across ./internal/format/oci/ ./cmd/artifact-registry/, every one of them on a line this branch does not add — checked against the added line ranges rather than by filename, since insertions shift pre-existing findings down into a diff's own files.

Three assertions mutation-checked, each restored by sha256: blanking Docker-Content-Digest in the shared writer reds the 206 test; deleting repositoryID reproduces the zero UUID in the captured log and reds two tests; and removing the disagreeing-sums guard makes that row serve a 200 with the wrong digest instead of a 500.

Related to #288

Edited by Radamanthus Batnag

Merge request reports

Loading
Loading