feat(datastore): add the container remote cache fill and freshness bump

Why

S16 caches what a container remote fetches, so a second pull for the same manifest or blob is served from the database instead of the upstream. The read half — the path-keyed Lookup and its freshness verdict — is already on main. This MR adds the write half it has nothing to read yet: the cache fill, and the freshness bump a 304 revalidation needs.

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

Step 8 ships as two stacked MRs, split at the package seam for review quality. This is the internal/datastore half; the internal/format/oci wrapper that classifies the payload and applies the by-digest comparison is the stacked follow-up. The plan's Step 8 entry names "Step 8's MR spans two packages" as an accepted smell — splitting removes it rather than taking it.

What (the non-obvious parts)

• UpsertCacheFill writes the container_remote_images parent, the container_remote_manifests or container_remote_blobs row the kind selects, and the container_remote_tags row on a by-tag fill, all in one transaction. A failure anywhere leaves none of them, so no request can observe an image row from a fill that did not commit.

• What serializes concurrent fills of one image is the image upsert. Its ON CONFLICT ... DO UPDATE writes a new heap tuple and holds that row's lock to end of transaction, so two replicas filling the same image block there before either mints an attachment. The lock is broader than one digest — every manifest, blob, and tag under one image serializes on that row — which is what keeps a concurrent re-fill from leaving a second attachment behind.

• A second, redundant lock on the image row, and why it is here. The fill also takes SELECT ... FOR UPDATE on its container_remote_images row after the upsert that created it. That locks nothing the upsert has not already locked, so no behavioral test can fail if the call is deleted — the method's doc comment says so outright rather than leaving a reviewer to find it. It is here because MavenRemoteCacheStore.lockParentForFill is, for the reason that method's own doc gives: the serialization above rests on the parent being resolved by an upsert inside the transaction, so a later change that resolves the image outside it removes the conflict lock and leaves the attachment mint unserialized, with nothing failing. The two differ on the missing-row arm — Maven swallows qrm.ErrNoRows because it locks a parent it did not create; this one locks the row the preceding upsert returned an id for, so no rows is unexplained and reaches the caller.

• The tombstoned-parent decision this step owed the plan: refuse the fill. Both writes gate on the parent repositories row, through the same predicate the read composes into its joins, but they carry it differently: the fill runs the gate as its own statement ahead of every INSERT, while the bump has one statement and carries it inside its WHERE as a correlated EXISTS. A row written under a tombstoned parent is unreadable the moment it commits, so accepting one cannot make even the request that wrote it succeed; it would only mint an image row and an attachment holding cached bytes against a namespace the operator deleted.

• The bump's gate reaches the manifest, not just the image. containerRemoteCacheTagStmt joins container_remote_manifests and requires that the manifest be live and hang off the same image as the tag, so the bump's correlated EXISTS carries that join too. container_remote_tags holds its image and its manifest as two independent composite foreign keys, each keyed on (id, namespace_id) alone, so nothing in the schema requires the two to agree. A gate stopping at the image would report success on a tag whose manifest is soft-deleted or hangs off another image in the same namespace — extending the freshness window on an entry no read can serve, so no re-fetch would ever replace it.

• BumpUpstreamCheckedAt returns remote.ErrCacheEntryNotFound when the UPDATE matches no row, not nil. Retention can delete a row between a Lookup that saw it and the bump, and reporting success would let a caller treat a vanished entry as revalidated and serve a blob reference with no cache row behind it. The verdict reads off rows-affected rather than a pre-read, since the row can vanish between the two.

• A digest-keyed path names no tag row, so the bump answers the same sentinel rather than inventing a second failure a caller could not act on differently.

• An unstorable upstream ETag degrades to NULL rather than failing the fill. check_container_remote_tags_upstream_etag_no_ctl refuses every C0 control and DEL, while the shared capture-time gate remote.cacheableETag refuses only CR, LF, and NUL — so an interior tab, 0x01, 0x1f, or 0x7f reaches this INSERT. Without a guard that aborts the fill on a check_violation after the manifest payload is committed to object storage: a 500, a blob left unreferenced, and a tag that can never be cached because every retry repeats it. The migration that created the table names this fill as the guard's owner. Degrading rather than refusing is the point — refusing would only trade the SQLSTATE for a Go sentinel and strand the blob just the same. mavenRemoteStorableEtag takes the same decision for the same reason; the guard lives datastore-side rather than format-side so every caller reaching the statement is covered.

• A zero freshness stamp is refused on both writes. It is the one value here with no database backstop: the column is NOT NULL DEFAULT now() with no CHECK, and both statements that write it name the column, so a zero time.Time binds 0001-01-01 and commits. The tag then reads stale forever and every request revalidates upstream. The by-tag fill's UpstreamCheckedAt and BumpUpstreamCheckedAt's checkedAt argument both carry the guard, on each of their two halves, because a caller composing a qrm.DB-accepting half into its own transaction never passes through the exported method. A stamp in the future is accepted — replica clock skew makes one legitimate.

• The write half refuses a negative cache_validity_hours. The read half's lookupKey already did, on every route including the digest ones, because a negative window says the repository row the store was built over is wrong whatever that read needs from it. Neither write reads the window, so this is a guard on the store snapshot rather than on a statement, and no live snapshot reaches it — FindRemoteRepository's column carries CHECK (>= 0). What it closes is the two halves disagreeing about what a valid store looks like.

• The store now holds a BlobStorageAttachmentStore. blob_storage_attachments has no unique key on (namespace_id, sha256), so every fill mints a fresh row and the superseded one is unreferenced in the same transaction once the upsert re-points. An attachment nothing references leaves its blob with a non-zero count and is invisible to gc:reconcile-scan; the deferred data-reconciliation service is what collects that class.

• The media type arrives as a plain string. Deciding it means reading the committed payload back out of object storage and parsing its JSON, and neither belongs in a seam whose job is SQL; ADR-023's no-reverse-dependency rule also forbids this package from importing internal/format.

• Nine new statements run through the named-query timer. main's TestEveryStatementIsInstrumented refuses a bare database/sql execution verb outside queries.go, so query_names.go gains nine catalog names — 279 declared names to 288. Naming follows the sibling remote caches rather than a new scheme: <table>_insert_upsert and <table>_select_existing_attachment are what maven_remote_files and npm_remote_files already use, and the bump takes Maven's _update_upstream_checked_at spelling because the bump is the remote.CacheStore method and Maven is the other implementation of that interface.

• Comments describing the format wrapper are in future tense, because the wrapper does not exist on main until the stacked follow-up lands.

Reviewable size

4,737 reviewable LOC, so 9.5× the ceiling. development-model.md fixes the gate and its exclusions (vendored, generated, binary) but names no arithmetic; the harness check that implements it sums a --numstat total, so added and removed, and Markdown alongside Go. That is the count below. By file group:

Group	LOC	Files
Production Go	1,327	container_remote_cache_write.go 1,222, container_remote_cache.go 73, query_names.go 21, internal/format/maven/remote_store.go 11
Tests	3,151	container_remote_cache_write_integration_test.go 2,302, container_remote_cache_write_test.go 652, container_remote_cache_write_explain_integration_test.go 197
Docs	259	plan 255, spec 4

The plan's Est. cell reports the same work under a different rule — added .go lines with blank and comment-only lines dropped, which comes to 600 production and 1,718 tests here — and says which count it is, so the two reconcile. Every figure in this section and in that cell was re-measured at d64f3cc13 against merge base 88babb6aa.

Reconciling 4,767 with 4,737. The plan quotes 4,767 on this same gate rule. 4,737 is what the tree gives: git diff --numstat 88babb6aa..d64f3cc13 totals 4,682 added and 55 removed, and 4,767 reproduces at no commit on this branch against that base. The plan's figure is 30 high, and the two derived from it move with it — the by-tag split lands near 3,600 rather than 3,700. Correcting the plan is a separate commit on this branch.

The package-seam split does not justify this size, and it was not meant to. Step 8 ships as two MRs because the plan named "spans two packages" as an accepted smell and splitting at that seam removes it. That bought coherence. It bought no reviewability: the two MRs together come to more added lines than the single branch they replaced, and this half is 4,737 either way. Read the split as scoping each MR to one package's context, not as a size mitigation.

Where the size is. container_remote_cache_write_integration_test.go alone is 2,302 lines, half of this MR. Each subtest seeds its own namespace, repository chain, image, and cache row rather than sharing a fixture, and the gate coverage runs that seeding once per soft-delete level, per scoping predicate, and per parent-format value. This plan's cache-read step recorded the same cost for its sibling suite, where a ~350 estimate came in at ~3,400.

Why it cannot be deferred. This is a SQL-only change and the untagged tests execute no statement — they cover the argument guards, the ETag storability rule, and the wrap arms a fake handle can reach. The integration suite is the only thing that proves the nine statements against a real database, so splitting it out would ship the write with nothing exercising it.

Two further splits, measured and rejected. The larger one is the whole by-tag route: everything here that exists because a fill can be reached by a mutable tag rather than by an immutable digest. That is the container_remote_tags write and its statement, the ETag storability helpers, BumpUpstreamCheckedAt and its statement, and their tests — about 1,100 lines across the tag write, the _ByTag and _BumpUpstreamCheckedAt integration subtests, the rejected-tag-row case, the tag read helper, the untagged ETag tests, and the bump's EXPLAIN test. Pulling it out leaves this MR near 3,600.

It is the split most likely to be asked about, so here is why it was not taken. 3,600 and 4,700 are the same review problem — both are many times the ceiling and neither fits in one sitting. Against that it costs a third MR, a third review cycle, a three-deep stack whose bottom two must both merge before the oci half, and churn rather than a clean lift: the manifest upsert would use Exec in the first MR and change to Query with RETURNING in the second, and the shared test fixture, its seed helper, and the suite header would each be written twice.

The smaller seam is BumpUpstreamCheckedAt alone — a separate remote.CacheStore method with its own statement and its own rows-affected sentinel contract. About 540 lines, leaving this MR near 4,200. Rejected for the same reason with less to show for it.

One more thing worth knowing before you read the diff. Roughly 1,200 of these lines came from two rounds of pre-push review rather than from the original implementation: the EXPLAIN suite, the untagged guard file, the ETag storability guard and its tests, the zero-UpstreamCheckedAt guard, the image lock, the wrap-arm and conflict-path coverage, and a substantial amount of comment prose correcting claims that were wrong. The MR is more correct than it was and harder to read than it was; both are true.

Test plan

go test -count=1 ./internal/datastore/
go test -count=1 -tags=integration ./internal/datastore/
golangci-lint run ./internal/datastore/
golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 ./internal/datastore/

The untagged run covers the argument guards, the ETag storability rule and its statement wiring, and seven of the eleven error-wrap arms across the two writes, none of which previously ran in the ordinary unit job. Of the other four, two are reached from the integration suite by a real constraint firing — attachments.Create's and the tag upsert's — and two are uncovered, each for its own reason: the superseded-attachment Delete wrap sits inside upsertCacheFill past the parent gate, where no fake handle reaches it and no constraint fires (BlobStorageAttachmentStore.Delete checks no rows-affected, so a missing row is a silent no-op), and the bump's res.RowsAffected() wrap needs a sql.Result that fails on RowsAffected, which no fake in this package supplies. The integration suite's header carries the same accounting.

The integration-tagged lint run reports 3,807 findings across the package; filtered to this MR's files it is 193 — 191 contextcheck in the new integration suite, which is the same package-wide pattern every sibling suite shares, and two maintidx. Every other file this MR touches, including the untagged and EXPLAIN test files and internal/format/maven/remote_store.go, draws zero. Every //nolint token in this MR was measured rather than inherited: removing them produces 10 ireturn, 2 dupl, and 1 thelper finding, so each token suppresses something it was written for.

The two maintidx findings are on TestContainerRemoteCacheStore_BumpUpstreamCheckedAt (Maintainability Index 9 against a threshold of 20), where the gate subtests are what carry it past the line, and on TestContainerRemoteCacheStore_UpsertCacheFill_Manifest (19), which the conflict-path assertions tipped over. Both are left unsuppressed to match the package: ten other functions here already trip maintidx between 9 and 19, none of them with a //nolint, two of them at 19, and the sibling read suite's TestContainerRemoteCacheStore_Lookup_Tag sits at 18.

Clearing the bump's means splitting the function, not trimming it — and that is settled on this branch rather than open on the MR. The obvious trim, extracting the repeated refusal tail into a helper, is done: the tail was repeated five times and is one helper now. Re-measured at d64f3cc13, the function still reads Maintainability Index 9 at Halstead volume 17,074, so the duplication was not what carried it past the line. The 16-to-17 figure quoted here earlier predated the gate coverage this round added and does not survive that re-run.

No e2e scenario is added or affected: this MR is a datastore write with no HTTP surface, and nothing calls it until Steps 13-15 wire the serve paths, so docs/testing/ has nothing to exercise yet.

Related to #288