feat(oci): classify container remote cache fills and gate by digest

Why

A container remote's cache fill has to decide what media type it is storing before any SQL runs, and it has to refuse a fill whose bytes disagree with the digest the request named. Neither can live in internal/datastore: both need an object-store read and a JSON parse, and ADR-023's no-reverse-dependency rule forbids that package from importing internal/format.

This MR adds the internal/format/oci wrapper that does both and delegates the write.

Step 8 of the S16 container remote plan, specified by S16.

Depends on

Nothing open. The internal/datastore cache-fill half of this step has merged, so this MR targets main directly and the merge order it needed is satisfied: it requires ContainerRemoteCacheStore.UpsertCacheFill to exist with its cache-key-taking signature, and the compile-time anchor var _ remoteCacheBacking = (*datastore.ContainerRemoteCacheStore)(nil) fails against either its absence or the earlier signature.

The branch was rebased onto main once the predecessor merged, so the merge commit an earlier revision of this description explained is gone. The rebase had to be checked rather than trusted: the predecessor merged squashed, so its individual commits are not on main, and the merge commit being dropped took with it edits that lived in neither of its parents — the absorption of the predecessor's signature change. A plain replay silently reproduced the earlier UpsertCacheFill(ctx, params) shape, which does not compile against main. Both internal/format/oci files were reconciled against the pre-rebase tree and verified byte-identical to the reviewed state.

What (the non-obvious parts)

• The read-back forces a stream. OpenBlob answers under the namespace's configured delivery mode and hands back a signed URL when that mode is redirect, which a server-side caller cannot follow. That failure is environment-dependent rather than deterministic: a dev or CI instance on proxy mode classifies fine while production on redirect mode fails every manifest fill. A redirect arriving despite the override is refused rather than classified from bytes nobody holds. internal/format/maven/reconciler.go refuses one the same way. Note delivery_mode has no configured default — an operator must set it — so the override is not a hedge against a default.
• Every refusal that holds a stream closes it. OpenBlob issues the object-store GET before it returns, so the stream is live by the time the classification inspects it — the reader is taken and its Close deferred ahead of the size check for that reason, not next to the read it feeds. Ordering the cap first abandons the connection on that arm. remoteCacheFakeBlobs counts its closes so the suite can see it: the sentinel and the absent delegation both stay green when a refusal leaks, which is how it went unnoticed until a Close-recording fake existed. The redirect refusal is the one path out after a successful open that closes nothing, and it is not an exception to the rule: a redirect open issues no GET and hands back no reader, which is why its subtest pins zero closes rather than one. The claim is scoped to the stream on purpose — stated absolutely, 30 lines from its own exception, it invites a later "fix" that closes a nil reader.
• Three refusals name the blob they are about, in the log. Each of them reports a fault in one specific object: a redirect despite the stream override, a byte count disagreeing with the blob row, and a payload that is not manifest JSON at all. None of the three sentinels can carry the coordinates — and on a by-tag fill nothing in the caller's scope recovers the digest either, since the cache key names a tag — so all three log the namespace, the remote repository id, and the digest through one logger bound for that purpose. A namespace holds many remote container repositories, so the namespace alone does not identify the row whose fills are failing. The Maven reconciler's read-back logs its own redirect refusal on the same reasoning, carrying its repository and package ids alongside the namespace. The logger is built per refusal rather than once per fill, so the success path pays no allocation for paths that do not run. The suite asserts all three fields on each line; dropping any one of the three log calls turns its own test red. The refusals the caller maps are deliberately not logged here — the two storage errors travel with their own wrapped cause, and a content verdict on what the upstream chose belongs to the frame that answers the client.
• The read-back is bounded from both directions, and the length guard covers both. The size OpenBlob reports is refused ahead of any read, so an oversize payload costs no read and the error can name the real number; the read itself then runs under io.LimitReader at the same ceiling, because a driver reporting a size it does not honour would otherwise stream past it. A remaining disagreement in either direction is then refused — the guard is a !=, not a <, because a stream running past the row's size while stopping short of the ceiling passes both bounds above and reaches only that comparison, and it writes a row whose size and digest describe different byte strings exactly as a truncated one does. Both rows are in the table; a rewrite to < goes red. A payload of exactly the ceiling is covered too, because both guards are >: either one turned into >= refuses a legal manifest with every other assertion still green, and it would diverge from what a hosted push admits, where readManifestBody's http.MaxBytesReader takes exactly the cap.
• The read-back buffer is sized from the blob row rather than grown. The exact length is validated on the line above, so io.ReadAll's 512-byte start and its doubling were paying for a length already in hand — and its final doubling holds the old buffer alongside the new one, so a payload near the ceiling peaked at roughly twice its own size for the whole read. internal/remote sizes the fill's own staging buffer off the response's declared length for exactly that reason, and this read happens on the same request, so growing here gave back part of what that sizing bought. The size is floored at zero: blob_storage_blobs.size is non-negative by a CHECK constraint, but the blob reader is an interface and make panics on a negative capacity, so the floor keeps a bad implementation on the refusal path instead of taking the process down. A negative reported size reaches the length comparison, which is where a disagreement between a row and its object belongs.
• The ceiling is defaultManifestMaxPayload, which is a gap this MR names rather than closes. A constant stands here because an unbounded read is not an option, not because a fixed bound is the right policy. It agrees with container.manifest_max_payload on a default configuration and diverges above a raised cap, where a manifest between them is fetched, committed, and then refused here on every attempt. The plan's Step 13 entry previously said this read-back would inherit the fetch's bound rather than repeat it; it repeats it, so that entry now assigns Step 13 the job of threading the configured value into this store as well — and names internal/format/oci/remote_cache_store.go in its Files list, since an obligation recorded only in prose is one the step's checklist does not carry. See the Step 13 section below for why that is the resolution rather than an error mapping.
• The detection chain runs first, the unaccepted-type refusal second. A payload naming Docker Schema 1 fails as Schema 1; refusing unaccepted types ahead of the chain would collapse that case into this one. The order is also what makes the refusal reachable: with no header supplied, an unaccepted mediaType yields no tentative type and no conflict, so the chain falls through to structural detection and returns an OCI type the manifest never claimed.
• The refusal is here, not in DetectManifestType, because the chain has a second caller. The hosted push path reaches the same headerless fallthrough — enforceRepositoryContent's doc in manifest_push.go says so outright — so refusing inside the chain would change what a hosted PUT accepts. Whether the hosted path wants the same refusal is S12's call on S12's error vocabulary. That is a live gap on the hosted push path, worth its own issue, and it is not this MR's to close.
• A genuine Docker Schema 1 manifest lands on the ambiguity refusal, not the Schema 1 one. Schema 1 predates mediaType, so a real payload carries none and its layer list is fsLayers. With no header there is nothing left to carry the Schema 1 signal, so the chain reaches structural detection with neither array present and stops on the ambiguity guard ahead of the schema-version gate. The fixture named for that case asserts ambiguity for exactly that reason.
• Four sentinels are exported, because the frame that will map a refused fill lands outside this file. ErrUnacceptedManifestMediaType and ErrCacheFillDigestMismatch were already; this MR adds ErrUnparseableManifestPayload and ErrManifestPayloadTooLarge. The first of the two is a condition an upstream causes — a non-manifest body under a 200, such as an HTML error page from a captive portal — so a caller that cannot tell it from an infrastructure failure files it under its datastore-failure default and pages as if AR's storage were broken, and unexported its only handle was a wrapped *json.SyntaxError. Their doc comments say the mapping frame will land rather than asserting it already sits there: nothing outside this file references any of the four yet, and the remote read arms still answer the interim 501 in remote_stub.go, so a present-tense claim is one a reviewer who greps can falsify.
• S16 gains two Error Cases rows and a corrected outcome definition. One row for a manifest payload that is not a JSON object at all, which the table did not have, plus its own acceptance criterion carrying the top-level-null asymmetry. One row for the three read-back faults that belong to this service rather than the upstream, at 500 INTERNAL on the reasoning the half-set-credential row gives. And the paragraph defining outcome="unsupported_content" said all four refused-payload cases take it and that they differ only in which part of the payload the service will not represent — the new non-object row is explicitly not one of the four, so the definition undercounted the value it defines, and an alert built from it missed the case whose likeliest cause is an upstream serving an HTML error page under a 200. The section also records that no outcome value fits an internal read-back fault, rather than inventing a tenth: the upstream response was a clean 200, so ok reports a success that did not happen, unsupported_content blames content the upstream was entitled to serve, and server_error and transport_error both name upstream conditions in a metric scoped to upstream responses. Naming the gap is how that section already handles the missing tag-list transform outcome.
• The cache key is handed to the fill whole rather than split into coordinates. UpsertCacheFill takes the key and derives the image name, the content table, and the tag name from it, so ContainerRemoteUpsertParams carries none of the three and this store passes CacheEntry.Path straight through. Its own parse keeps only what the format side decides with — a manifests-or-blobs discriminator and the digest a by-digest key names — and still checks the image name before dropping it. One grammar writes the rows, so a caller cannot pass coordinates that disagree with the key they came from.
• Two key shapes this parse admits are refused by the fill rather than written. A blobs reference that is not a digest, and a manifests reference carrying a malformed digest, both get past this parse by design: the serving caller has already held the reference to the sha256 pattern or to the tag grammar, so refusing them here would be dead code. Under a signature that took pre-split coordinates this store decided their rows itself, and both landed as rows — the malformed digest as a tag name, which container_remote_tags.name accepts, carrying a length CHECK and no grammar CHECK. Now the key travels instead, parseContainerRemoteCacheKey rejects both, and neither writes anything. The only cost is on the manifests shape: a read-back and a classification spent ahead of the refusal. TestRemoteCacheStore_ShapesTheParseAdmits asserts what this store does with each and points at the datastore's rejection table for the refusal itself.
• The by-digest comparison runs ahead of the read-back as well as the delegation, so a mismatch writes no content row and no image parent and spends no object-store open. The hex body is compared case-insensitively, so a reference spelling the same bytes in upper case commits rather than missing.
• A blob fill takes neither the read-back nor the classification: container_remote_blobs carries no media_type and no size column, and the bytes are a layer rather than JSON. The zero open count is asserted, not assumed.
• The type is one method short of remote.CacheStore until BumpLastDownloadedAt lands, so it carries no compile-time anchor against that interface and every comment about satisfying it is in the future tense. The anchor it does carry is against the narrower backing surface it composes over. Adding the remote.CacheStore anchor now would not compile.
• The embedding promotes two methods, the write is not one of them, and the promotion is now pinned. The embedded interface is narrowed to remoteCachePromoted — Lookup and BumpUpstreamCheckedAt — so UpsertCacheFill stays off this type's method set, and the gates UpsertCacheEntry applies cannot be stepped around by calling the write on the wrapper. The write is reached through an unexported fill field instead, which is package-private by construction. Embedding is kept for the two reads rather than replaced by explicit delegation, because a hand-written forwarder is where BumpUpstreamCheckedAt's ErrCacheEntryNotFound contract gets flattened — and that was a stated contract with nothing asserting it, since the fake panicked on both promoted methods and the method-set test only checked that the names resolve. It now has a test, and the fake keeps its panic as the default so every fill subtest still fails loudly if a fill reaches either method.
• ETag and UpstreamCheckedAt are passed through unvalidated on purpose. The predecessor degrades an unstorable ETag to NULL — its comment explains that the guard belongs to the write that owns the constraint — and a zero UpstreamCheckedAt is unreachable, since the only production producer sets it from time.Now().
• Nothing here belongs to one request, so one instance serves every fill against the repository it was built for. The wrapper itself reads two fields of the row it holds — the namespace the read-back opens under and the repository id its refusals log — neither of which a repository can change. That is not the whole picture, because Lookup is on its method set and the freshness verdict Lookup promotes comes from the cache_validity_hours the backing store snapshotted, so a shared instance has to be rebuilt when the repository row changes. datastore.ContainerRemoteCacheStore documents that on its own type; it reaches callers through this one, so it is stated on both.

Reviewable size

2,751 reviewable LOC (added + removed, the rule development-model.md applies at the 500 gate), past the 500 that asks an author to justify. By file group: production 863 (remote_cache_store.go), tests 1,758 (remote_cache_store_test.go), docs 130 (111 in the plan's Step 8 and Step 13 entries and its criterion-ownership table, 19 in S16's Error Cases table, acceptance criteria, Observability section, and Follow-ups).

The plan's Est. cell reports the same work under a different rule — added .go lines with blank and comment-only lines dropped — and names which count it is, so the two can be reconciled. This half measures 232 production and 1,045 test lines on that rule; the plan's Step 8 entry carries both halves and the step's total.

Splitting further would not help: the wrapper is a struct, a constructor, and one overridden method, and it cannot land without the suite that pins its two orderings. The suite covers all four accepted media types as positive hits plus two rows where the declared type disagrees with the payload's structure, all four refusals with the reason each carries, six digest-gate cases, the redirect guard and the three fields it logs, the size bound from both directions plus the at-cap boundary, the length disagreement from both directions plus a negative reported size, the read-back faults including a stream that fails partway through, the Close count on the committing path and on every refusal that was handed a stream, the key reaching the fill unchanged, both promoted methods and the sentinel one of them has to keep matchable, and the exported type's zero-value behaviour — all against fakes, with no database and no object store.

Test plan

go test -count=1 -race ./internal/format/oci/
golangci-lint run --max-same-issues=0 --max-issues-per-linter=0 ./internal/format/oci/

Both are clean; the suite is untagged, so it runs in the ordinary unit job. No //nolint directive appears in either file.

Every assertion worth not trusting on sight was mutation-checked, production mutated and restored by checksum. Reverting the reader hoist so the size check precedes content.Reader() fails only RefusesOversizeManifestReadBack/a_reported_size_past_the_ceiling_is_refused_before_the_read, on the Close count. Narrowing the length guard to < fails only RefusesReadBackLengthMismatch/a_stream_that_runs_past_the_reported_size_but_under_the_ceiling. Turning the ceiling guard from > to >= fails only the at-cap subtest. Dropping either of the two new log calls fails only that refusal's own test, on the digest field. Shadowing the promoted methods with forwarders that flatten the error chain fails only the not-found-sentinel subtest, while the method-set test stays green — which is the gap that subtest exists to close. Each discriminates the case it is there for and nothing else.

Dropping the buffer's zero floor is the one mutation that did not fail on the first attempt, and the fixture was wrong rather than the floor: at a reported size of -1 the bytes.MinRead headroom makes the capacity 511, so make never sees a negative and the refusal lands either way. The subtest is now a two-case table naming which case depends on the floor — a small negative the headroom absorbs on its own, and one past the headroom that panics without it.

The assertion that replaced the dropped coordinate checks was measured the same way. Handing the fill a key other than CacheEntry.Path fails 14 subtests across 3 tests — every table that asserts the delegated key. Flipping the manifests discriminator fails 31 across 8, since it moves the read-back onto the blob route and off the manifest one.

No e2e scenario is added or affected: nothing constructs this type until the composition root wires it, so there is no HTTP surface to exercise and docs/testing/ has nothing to add yet.

Known and deliberate, so review need not rediscover them

• classifyRemoteCacheManifest unmarshals the full parsedManifest envelope to read two scalar fields, MediaType and SchemaVersion; detectStructuralKeys supplies the structural signals from a separate token scan. A narrower struct is measurably cheaper — on a 2,000-child index, 1,028,830 B/op and 8,065 allocs against 25 B/op and 1 — but it is not a safe local change and is deliberately not made here. parsedManifest's typed fields are what reject a payload whose unused fields have the wrong shape: {"schemaVersion":2,"layers":"nope"} fails the unmarshal today, while against a two-field struct it would be ignored, detectStructuralKeys would report the layers key present, and the payload would be classified and stored. That would also split what a remote fill accepts from what a hosted push accepts, since the hosted path unmarshals the same envelope. Worth doing for both paths at once, preserving the type validation; not worth doing for one.
• A refused fill re-pays its whole cost on every client retry, and this MR adds one object-store GET to that cost. Classification runs after the storage session has committed, so a deterministic content refusal spends an upstream GET, a staging write and move, and now a read-back, and leaves an unreferenced blob. S16 accepts that for the classified refusals and the object dedups on its content address, so nothing accumulates unboundedly. What is new is that the extra GET is synchronous inside the fill, so it lands on the tail latency of every remote manifest cache miss — worth knowing when cache_fill_duration_seconds gets sized.
• The read-back's object-store GET and its read take no timeout of their own, so once a serving caller is wired up they will both come out of whatever budget bounds UpsertCacheEntry. internal/remote's fill bounds that call with a single timeout, and for the Maven and npm slices it had only cache-row SQL inside it — so whoever tunes that bound will be sizing object-store latency alongside datastore latency. Recorded on classifyCommittedManifest rather than fixed here: the store has no production caller yet, so there is no budget to split.
• PgBlobStore.OpenBlob observes blob_download_bytes and emits the blob_served wide event at Info unconditionally, and no option suppresses it, so this server-side classification read counts bytes nobody downloaded and logs a line claiming a blob was served. Not introduced here and not fixed here: four other non-test internal callers already do the same, including internal/remote/singleflight.go, which is on the remote read path too. It belongs to internal/storage as its own change.

An obligation this MR records for Step 13

ErrManifestPayloadTooLarge is exported so a caller can classify it, and the condition it reports should not survive Step 13.

This read-back's ceiling is oci.defaultManifestMaxPayload. Step 13 threads container.manifest_max_payload into the manifest Fetch as its body bound. The two hold the same value on a default configuration and diverge above a raised cap, and while they diverge a manifest between them is fetched, committed, and then refused here on every attempt — that image never caches, and the refusal blames the upstream for content AR's own configuration accepted. So Step 13 threads the same value into oci.NewRemoteCacheStore too, in the same MR, because equal bounds is the point and splitting them is how they drift apart. With them equal, an oversize manifest is refused at the fetch before any commit, which is a body-cap overrun S16's Error Cases table already covers.

What remains of the sentinel once the bounds are equal is the two disagreeing, which is an internal fault in the same family as a read-back whose byte count disagrees with the blob row. S16's new 500 INTERNAL row covers all three of those faults together, on the reasoning its half-set-credential row already gives: a 503 is retryable to an OCI client while none of these clears on a retry of the same bytes. An earlier revision of this description argued the row should not exist at all while the bounds could still diverge — that was wrong in a way worth naming, because Step 13's acceptance is a table over every row of the Error Cases table, so a sentinel with no row is a sentinel with no acceptance test. The row is written for the post-Step-13 world it will be implemented in; note that the half-set-credential row's first leg ("a read-side defect rather than operator configuration") only fits after the bounds are equalized, which is another reason to remove the divergence rather than settle for describing it.

Keeping the constant as allocation defense in depth was considered and rejected. The hosted push path already buffers to the configured cap — readManifestBody in manifest_push.go wraps the request body in an http.MaxBytesReader at h.maxPayload — so the constant holds the remote path to a bound hosted does not apply to the same artifact type, and S13's body_size_cap_metadata independently bounds what a fetch can commit, so threading the configured value hands nobody an unbounded read.

Adjacent, genuinely open, and not this step's: whether manifest_max_payload should be validated against body_size_cap_metadata at startup, since a manifest cap above the metadata cap is dead on the remote path the way one above blob_max_size is dead on the hosted one. That is config validation on the S13/S16 boundary, not error mapping.

Related to #288