feat(oci): container remote fill emits its storage-accounting deltas

What

A container remote cache fill now records the four storage-accounting deltas it owes, post-commit, so a reconciliation pass over a freshly filled repository moves no counter.

Before this, ContainerRemoteCacheStore.UpsertCacheFill wrote container_remote_images, container_remote_manifests, container_remote_blobs and container_remote_tags rows plus their blobs and referenced internal/accounting nowhere, so all four counters read low until the next reconciliation pass.

Counter Moves by
repositories.artifacts_count 1 when the fill's own INSERT produced the container_remote_manifests row
repositories.size_bytes the incoming blob when the repository did not already reference it
namespace_statistics.components_count the same 1 as artifacts_count
namespace_statistics.deduplicated_size_bytes the incoming blob when it was new to the namespace

This is the Container arm of #834 (closed), and its MR 2. !2123 (merged) was MR 1, the npm arm, and has merged. #834 (closed)'s "Done when" asks for both fill paths, so this closes it.

Process note

The operator approved landing this without a plan MR, an acknowledged deviation from the plan-before-code guardrail. #834 (closed) records that route for both arms: the Maven arm took it under !1901 (merged)'s own ## Process note, and !2123 (merged) followed. The research that would have gone into that plan is in this description and in the doc comments the diff adds. Because no plan under docs/plans/ declares this work, the title carries no (<plan token> plan: <step>/<total>) marker, which is why !2123 (merged) carries none either.

Approach

Ported from the merged Maven arm, !1901 (merged), which #834 (closed) names as the reference. The shape fits because oci.RemoteCacheStore.UpsertCacheEntry delegates the whole write to one datastore seam, ContainerRemoteCacheStore.UpsertCacheFill, so the facts assemble in the datastore and come back in a result struct rather than being composed in the format package the way npm's four store seams forced.

Three differences from Maven are forced rather than chosen, and #834 (closed)'s ### MR 2: the Container arm section settles all three:

  • Two facts, not three: this arm owes no displacement credit. Both content upserts conflict on (namespace_id, container_remote_image_id, digest) and set blob_sha256 from that same digest, and existingContainerRemoteAttachment looks the prior row up by that digest too, so a conflict re-points the coordinate at bytes it already named. container_remote_tags does repoint, and it appears in neither the count walk nor the size walk. So size_bytes takes a charge here and never a credit, and porting Maven's Displaced term is porting dead code.
  • Both counts key on the container_remote_manifests insert. recomputeContainerRemoteManifestsStmt counts it, and it is the second term of recomputeNamespaceComponentsCountStmt. A layer fill writes no such row, so it moves both byte totals and neither count.
  • The probe carries no soft_deleted_at predicate at either level, matching recomputeContainerRemoteBlobsSizeStmt, which filters at none of its three. The hosted containerRepoStillReferencesBlobStmt could not be reused: it walks container_manifests and container_blobs.

upsertContainerRemoteManifestStmt gains the candidate id as a parameter so the RETURNING id decides whether this fill's own INSERT produced the row, mirroring upsertMavenRemoteVersionStmt and upsertNpmRemoteVersionStmt. The probe runs before the content upsert, on the fill's own transaction handle, so a failed accounting read fails the fill rather than committing rows with a silently dropped delta.

That placement is an availability trade-off, and it is route-visible. A cold miss gains a failure mode it did not have: a GET of an uncached blob or manifest that would previously have served now fails when that one SELECT EXISTS fails, so a database blip on the read path becomes a failed pull rather than a counter reading low until reconciliation. On the streamed blob arm the refusal arrives at end-of-body as a withheld terminating chunk; HEAD, a ranged GET and the manifest arm route through their unfilled-miss answers instead. The merged Maven arm states the same policy for the same reason (internal/datastore/maven_remote_cache.go: "Like every accounting read on a fill, a probe error fails the transaction rather than dropping the credit silently"), so this is house policy rather than a divergence, and PostgreSQL aborts at the first failed statement anyway, so the read cannot be softened where it stands.

internal/format/oci already declares CounterEmitter, CounterSink, emitSite and the shed-or-spawn dispatch in the same package as remote_cache_store.go, so this arm needs neither npm's re-declared interface nor its injected dispatch. One CounterSink serves the hosted sites and the fill alike, which keeps datastore.ContainerBlobFootprintStore to a single construction; wire_oci.go hoists its declaration above the store gate because buildOCIRemoteSlots runs after that gate closes and needs the sink there. Splitting the sink would not raise the in-flight cap: NewCounterSink hands every sink it builds the package-level counterEmitSem, which TestNewCounterSink_SharesTheProcessWideCap pins by channel identity.

RepositoryFootprint.BlobFootprint is deliberately not the probe. Its doc scopes it to the hosted populations, and its callers read it post-commit and accept the transient miscount two racing operations over one digest produce.

Testing

TestBuildOCIRemoteFill_CarriesTheCounterSink covers the one wiring hop that fails silently, and was verified to fail with the assignment removed. Every other hop is a positional argument, so dropping it stops the compiler.

It does not cover oci.NewRemoteCacheStore(..., f.counters), where passing nil compiles and that test still passes. TestNewRemoteCacheStore_CarriesTheCounterSink covers the constructor's half, and TestWireOCIWithStore_ThreadsCounterSinkIntoTheRemoteCacheFill closes the hop end to end: it drives one committed layer fill through wireOCIWithStore over remotetest.FakeUpstreamDoer and reads both scopes' live Redis hashes. TestRemoteCacheStore_UpsertCacheEntry_EmitsTheCommittedFacts covers the store's own half of that chain.

TestCommittedRemoteFillDeltas enumerates every combination of the two facts and the two CacheEntry fields, and was verified by mutation to fail 7 of 7 when the charge predicate is inverted.

TestContainerRemoteCacheFill_ReconciliationParity is #834 (closed)'s done-when arm. It runs RecomputeArtifactsCount, RecomputeSizeBytes and RecomputeComponentsCount around a fill sequence and asserts each moved by what the facts sum to, with require.NotZero guards so it cannot pass vacuously. The third recompute is the npm arm's, kept per guardrail 6: the namespace walk counts the same container_remote_manifests table under no soft-delete predicate where the repository walk carries two, so it is a distinct assertion rather than a restatement.

namespace_statistics.deduplicated_size_bytes has no parity assertion, matching both landed arms: Session.Commit decides CacheEntry.Deduplicated and no store here sees it, so it is pinned at the emit instead.

TestContainerRemoteCacheStore_RemoteRepositoryHoldsBlob covers both arms, both tombstone levels on each arm, and the sibling-repository scope. TestContainerRemoteCacheStore_RemoteRepositoryHoldsBlobStmt_PrunesBothArms covers the query plan: the probe moved onto the request path, and the npm twin carries the same assertion at npm_remote_explain_integration_test.go:797. mavenRemoteRepoStillReferencesBlobStmt has no such test.

Its EXPLAIN output shows what that test pins and what it does not. All three tables prune to a single partition (container_remote_manifests_p45, container_remote_blobs_p45, container_remote_images_p45), which is the property the statement can lose with its SQL unchanged. Access inside each partition is a seq scan at fixture row counts, because the partitions hold single-digit rows there.

Database Review Evidence

Query mode triggers and migration mode does not. internal/datastore/repo_blob_references.go is changed and adds the statement that repoStillReferencesBlob runs through db.QueryContext, which is one of the three calls docs/dev/database-migrations.md names as activating query mode. No file under internal/datastore/migrations/sql/ is changed.

All of query mode's summary fields are present. Plan, index choice, partition pruning and timings are in ### The probe's cost at a shared base layer; planner-versus-actual rows and buffer hit / read are in ### Planner-versus-actual rows and buffers after it, from a re-measurement at this head rather than the hand-taken run the first table quotes. Neither run is rebuilt by a committed harness: both seed the same throwaway script, which is why each table says which run it is.

The probe's cost at a shared base layer

The pruning assertion cannot see the join, and the join is what a shared base layer loads. Measured on PostgreSQL 17.10 against a seeded fixture: one namespace, two remote repositories of 800 cached images each, one manifest and eight layers per image, one base-layer digest carried by all 1600 of them, and 5000 background namespaces so the pruned partition is not the only populated one.

Case Plan Execution
The repository holds the digest Memoize over the images primary key, short-circuits on the first match 1.1 ms
Charge, digest carried by all 800 images of the sibling repository same, 800 memoized primary-key lookups 2.8 ms to 14.6 ms across runs
Charge, digest no row anywhere carries nothing to join 0.2 ms

The plan is the good one because this service sends no prepared statement. labkit's pgx query mode is QueryExecModeSimpleProtocol, which PgBouncer transaction pooling requires, so every execution reaches the planner with its literal values and is planned against them. That matters here, because the same statement executed as a server-side prepared statement gets a generic plan from its sixth execution on, and that plan cannot know blob_sha256 matches every cached image's base layer: it materializes both sides and cross-joins them, discarding 640,000 rows in 21.8 ms at this fixture and 160,000 in 14.6 ms at half the scale, quadratic in (rows of the namespace carrying the digest) times (images of the probing repository). None of that is reachable while the query mode stays as it is. Three rewrites were measured against the generic plan and none of them helped, because the estimates are the cause rather than the join's spelling, so the query mode is the only thing standing between this statement and the quadratic term.

What the probe does pay on every execution is a plan over three 64-partition tables, which is the per-execution planning cost this schema imposes on every read in the chain and which TestContainerRemoteCacheStore_ChainWidth already describes for the sibling reads. In this fixture that ranged from 0.6 ms to 54 ms run to run without converging, the same non-convergence that test records, so no figure from it is quoted as a production number.

containerRepoStillReferencesBlobStmt is the same join over container_manifests and container_blobs, with the same index situation, so the five hosted emit sites share whatever this statement's characteristics are; nothing here is particular to the remote arm.

Planner-versus-actual rows and buffers

Re-measured at 0a04f36f4 on PostgreSQL 17.10 against a rebuild of the same fixture script, so the timings here are this run's rather than the table above's. The buffer pool is warm: shared read is 0 in every case because the fixture's working set is resident, and a cold pool reads it from disk instead. The fixture's namespace hashes to _p51 in this rebuild rather than the _p45 above; the pruning property is the same one.

Case Node the cost sits on Est. rows Actual rows Buffers hit / read Execution
The repository holds the digest Nested Loop over the blobs branch 1590 1 7 / 0 0.05 ms to 0.27 ms
Charge, digest carried by all 800 images of the sibling repository Seq Scan on container_remote_blobs_p51, then Memoize per row 795, then 1 per loop 800, then 0 across 800 loops 2649 / 0 2.5 ms to 5.1 ms
Charge, digest no row anywhere carries Index Scan on container_remote_blobs_p51_namespace_id_blob_sha256_idx 1 0 4 / 0 0.06 ms to 0.17 ms

The first row's 1590-to-1 gap is EXISTS stopping at the first match rather than a mis-estimate. The second row's is the mis-estimate, and it is the one the prepared-statement argument turns on: the seq scan estimates 795 rows and gets 800, and the join above it estimates 795 and gets 0, because the planner cannot know that container_remote_repository_id excludes every one of the sibling repository's rows. Under a generic plan that same error is what materializes both sides. Memoize reports Hits: 0 Misses: 800 there, so it pays for itself nowhere in this shape, each row carrying a distinct container_remote_image_id.

Two figures the table above does not carry. Planning touches 1629 shared buffers per execution in all three cases, which is the three-64-partition planning cost stated above, measured rather than asserted. And access inside the blobs partition is a cost-based seq scan in both hit cases at this scale, discarding 11,274 rows and spending 247 of the 2649 buffers, with the namespace_id, blob_sha256 index chosen only where the digest is absent. The single-digit-row seq scan TestContainerRemoteCacheStore_RemoteRepositoryHoldsBlobStmt_PrunesBothArms sees has a different cause.

Diff size

1784 reviewable LOC across 30 files, past guardrail 15's 500. A split would not help: production is 248 added and 93 removed, and the rest is tests plus docs.

Group Added Removed Files
internal/format/oci prod 99 38 4
internal/datastore prod 132 48 4
cmd/artifact-registry prod 14 4 2
internal/remote prod 3 3 1
tests 1161 136 13
docs and the run recipe 105 41 6

Roughly 90 of the production lines are mechanical return-shape plumbing (error to (ContainerRemoteUpsertResult, error)), and the roughly 50 UpsertCacheFill call-site updates in the tests are one line each and change no assertion. The internal/format/oci removals are mostly one comment block: counterEmitTimeout's 25-line census compressed to its one-line cap, with the census moved into docs/dev/storage-accounting.md.

Measured at dddbff074, where the two maintidx findings on container_remote_cache_write_integration_test.go:707 and :1766 are pre-existing. Rerun with git diff origin/main...dddbff074 --numstat. Reviewable LOC is added plus removed.

e2e scenarios

docs/testing/e2e/oci.md gains e2e.oci.remote.fill-storage-counters, per guardrail 12. !2123 (merged) stated no scenario was affected because npm.md asserts these counters only through the reconciliation pass; that reasoning does not transfer, because no OCI remote row asserted a counter at all and the fill is a new emitter on a read path. The row states the blob and manifest arms together, and the section preamble is corrected: it claimed every row was the blob arm's and that the manifest arm had none. Its Interface is `HTTP` / API: the scenario is driven by raw OCI blob and manifest GETs and only the counter read-back is the management API, which is the order the catalog's other combined values use.

Records this corrects

Two are in #834 (closed) itself. Its ## How this splits says the two arms "touch disjoint files, and share no new code"; both clauses fail for internal/datastore/repo_blob_references.go and internal/datastore/query_names.go, which each arm adds to and whose = alignment !2123 (merged) reflowed. The claim running the other way also holds and #834 (closed) does not make it: this arm does not need !2123 (merged)'s qrm.DB widening, because upsertCacheFill and the RunInTx above it already take one.

docs/dev/storage-accounting.md had four claims that this arm emits nothing, in the emit-site table, the known-gaps table, the repositories.size_bytes narrative and the Maven-versus-container comparison. All four are corrected and the emit-site table gains this arm's row. ### Per-format reality's Container and OCI bullet gains the increment sentence its npm and Maven siblings carry. That bullet described only deletes, so it was a gap rather than a stale claim.

Merge order

Re-scanned at 9ac446fcc against the 90 open MRs, then re-checked after main moved twice during the round: each candidate's files diffed against its own target, intersected with this branch's, and every intersecting one run through git merge-tree --write-tree against this branch and against main. Every candidate resolved a refs/merge-requests/<iid>/head ref, so none went unchecked. 80 do not overlap at all. No pipeline reports any of what follows.

This branch owns the order against one. !2270 (merged), prozlach/issue-1018-single-target-remote-delete-deltas, conflicts with this branch in docs/dev/storage-accounting.md and merges cleanly into main, so whichever of the two lands second resolves by hand. Worth knowing alongside it: Step 20b of the S20-a plan is also scheduled to rewrite the docs/dev/storage-accounting.md container-remote passages this MR edits.

Five overlap and conflict against their own base as well, so the rebase they each need is what settles it rather than any order with this branch: !2052, !2217 (merged) (the shallowest overlapping-and-conflicting member of the !2217 (merged) to !2218 (merged) chain), !2218 (merged), !2268 (merged) (in docs/specs/S17-rest-management-api.md, which this branch does not touch) and !2297 (merged). !2272 (merged) joined them mid-round: it conflicted only with this branch until !2232 (merged) landed, and it now conflicts with main in docs/dev/storage-accounting.md too.

!2302 (merged) overlaps and merges cleanly.

Four entries moved during the round, which is the coordination this section exists to record. !2279 (merged) merged as 87cbaa13a before the first re-scan. !2232 (merged) merged as 01bbae06a and !2162 (merged) as d54b2d6d0 after it, and !2232 (merged) is the collision this section predicted: it landed second, so the merge commit above resolves it. !2162 (merged) needed no resolution here. Neither !2270 (merged) nor !2162 (merged) conflicts in internal/format/oci/emit_dispatch.go any more, because the first merge took main's side of that file.

Review round

Four commits on top of ed6eaa332 answer a review of this branch. Nothing in them changes what the fill computes or emits; the delta table above still holds.

Three comments and doc sentences were wrong, and the sixth emit site is what made each one wrong, so none could be left standing and disclosed:

  • The single-sink rationale claimed splitting the sink would admit "64 each". NewCounterSink assigns the package-level counterEmitSem to every sink, and TestNewCounterSink_SharesTheProcessWideCap already pinned that. Replaced with the reason that holds.
  • docs/dev/storage-accounting.md claimed the fill "emits once its status and its body have both gone out". No arm does: a streamed GET commits the fill on the tee read that saw end-of-body, so the emit precedes the write of that read's own chunk, and the three drained shapes emit ahead of the status. The section now names the real hazard of an inline emit there, a stall inside the window armBlobFillResponseDeadline arms truncating the body.
  • counterEmitTimeout's block claimed to cover "all five membership reads". The fill's probe is a sixth counter-emit read outside that bound. The block is a cross-site census over the comment-caps ratchet, so it compressed to its one-line cap and the census, the slot costs and the 15s ceiling moved into docs/dev/storage-accounting.md.

Two subtests the merged npm suite carries had no container twin, and each guarded a predicate that could be added back with the suite green (guardrail 6):

  • The probe's parent-tombstone case was covered on the manifests arm only. The two arms join container_remote_images separately, so adding AND cri.soft_deleted_at IS NULL to the blobs arm's join left every container case passing. Verified by mutation: that change now fails the new subtest and only it. Without the guard, a cold pull of a layer under an evicted image would charge size_bytes for bytes recomputeContainerRemoteBlobsSizeStmt already counts.
  • The facts suite had no refill-over-a-tombstone case, the only shape where a real fill reports both facts true. The unit table covered that pair as arithmetic; nothing proved the store emits it.

ContainerRemoteCacheStore.RemoteRepositoryHoldsBlob is now unexported. It had no caller outside internal/datastore and never compared its id arguments against the receiver's own snapshot, so an out-of-package caller could probe an arbitrary pair through a store scoped elsewhere. MavenRemoteCacheStore.remoteRepositoryHoldsBlob is the shape it now matches, id parameters included; npm exports its equivalent only because internal/format/npm/npmremote declares it as a cross-package seam, and this arm delegates the whole write to the datastore.

Smaller corrections: the run recipe's Container/OCI emit-site list gained the fill, rawSQLTimedFunctions' repoStillReferencesBlob note gained the container caller, the parity test's doc named two recomputes where it asserts three, and the wiring test's comment said the hosted case "builds no remote arm" when what it builds none of is the fill.

Review round 2, and the merge

Seven commits answer a second review, and the branch was then rebased onto main. Nothing in them changes what the fill computes or emits; the delta table above still holds.

Five of the six were corrections the previous round left uncommitted in a working tree, so the pipeline that built 665250a79 never saw them:

  • ContainerRemoteUpsertResult's doc said "its zero value moves neither scope". committedRemoteFillDeltas reads !RepositoryHeldBlob as the charge condition, so the zero value charges the incoming blob to both scopes, which the table case "a first layer fill moves both byte totals and neither count" already asserted. The polarity is right and matches MavenRemoteUpsertResult.RepoHeldBlob; only the doc was wrong, and it is the doc a reader consults before deciding whether the emit may move above the error check. remoteCacheFakeBacking.facts carried the same claim.
  • docs/specs/S20-a-lifecycle-closed-beta.md still said "no merged path raises the column for an npm remote cache fill", which !2123 (merged) falsified.
  • The run recipe's npm emit-site list was missing that same fill.
  • All five degrade the same way had lost its referent once this arm's paragraph landed between the list and the sentence.

The sixth closes a test gap and two more comment defects:

  • TestRemoteCacheStore_UpsertCacheEntry_EmitsNothingOnAFailedFill. The emit sits below UpsertCacheFill's error check and its comment says that is what keeps a rolled-back fill from charging, but nothing held it there: every refusal case in the file builds its store through a helper that passes a nil sink, so a stray emit was unobservable. Verified by mutation, and it is the zero-value reading above that makes the mutation charge rather than emit zeroes.
  • Three comments claimed a nil counter sink is what a storeless boot leaves behind. No boot leaves one: buildOCIRemoteSlots builds ociRemoteFill inside the same store != nil gate wireOCIWithStore assigns the sink in. The nil is the DB-less stubs' shape. No panic was added, because both landed arms take a nil sink deliberately (internal/format/maven/remote_store.go documents "counters may be nil", internal/format/npm/npmremote/cache_store.go says the same) and oci.NewRemoteCacheStore runs once per remote-repository resolution, so panicking there would cost every remote read its response over a best-effort counter.
  • The document's reason for the fill's hop drawing no boot assertion was inverted. "A nil sink there is a legal composition" cannot follow from "a boot that links no BlobStore builds no cached arm": if no arm exists without a store then the sink is non-nil wherever a fill exists, which would make an assertion unconditional rather than illegal. The reason that holds is the one the same document gives about the mount, that the sink arrives as a constructor argument and assertOCIOptionsWired reads handler fields. Recorded alongside it: a nil there is silent past boot too, because dispatch returns at its own nil guard before meterCounterEmit.
  • The new e2e row gated the wrong namespace counter. components_count moves with the container_remote_manifests insert whatever CacheEntry.Deduplicated says; only deduplicated_size_bytes is gated on first attach. The blob-arm sentence also omitted the namespace dedup bytes a layer fill does move.

The seventh covers a subtest the mirrored Maven suite carries and this arm's table did not. TestRemoteCacheStore_EmitCommittedFillCounters_ScopeSplit's three rows all carried bytes, so nothing here called a scope with a count and a zero byte delta; Maven's a count-only movement skips neither scope at internal/format/maven/remote_fill_emit_test.go:374 is the case, and guardrail 6 makes the mirrored suite's subtests the checklist. {NewManifest: true, RepositoryHeldBlob: true} over a deduplicated entry is the one combination that reaches it, and a real fill does: a manifest re-cached over a tombstone whose digest the repository still holds and the namespace already has. Verified by mutation, and stated precisely: narrowing emitNamespace's guard to deltaDedupSize == 0 fails this row and leaves the table's other three green, while the hosted manifest push and delete suites catch the same mutation, so the seam was not unguarded package-wide. What had no case was this arm's own table.

main moved twice during the round and the branch took a merge each time, then was rebased onto main at the end, so the history is linear and the merge commits are gone. What those merges had to settle is recorded here, because the rebase leaves no commit carrying it.

The first merge took four conflicts, two in production files:

  • wire_oci.go: feat(authz)! replaced wireOCIWithStore's iamClient/glazClient with one authzWiring and deleted the line this branch's var counters hoist sat under. Took main's call and kept the hoist above it.
  • emit_dispatch.go: both sides moved the header census out, to different homes. main's sidecar internal/format/oci/emit_dispatch.md wins, because it is the pattern this package already uses and its own rule is to state no list of its own. The per-site read census stays in docs/dev/storage-accounting.md, which is the list that grew a sixth member here. Three pointers still promised that counterEmitTimeout's doc comment enumerates those reads, which this branch is what deletes: two in docs/dev/storage-accounting.md and one in emit_dispatch.md, none in a conflicted hunk, so nothing would have flagged them. All three now name the section carrying the census. The header keeps its divider form, because main's plain one-liner plus this branch's one-line counterEmitTimeout doc is two lines against a cap of one; the sidecar's cap arithmetic is corrected to match.
  • Known gaps: kept main's retried-fill row from !2279 (merged), since the window holds on every remote arm, and rewrote its container clause, which recorded that this arm emits nothing. Dropped main's The container remote cache fill emits nothing row, which is the one this MR retires. Also re-pointed That row is the one place the population is counted, which main's new in-flight caps section falsified, and put the per-site inline-emit census in the conditional mood, since the shipped code never makes that wait the client's.
  • docs/testing/e2e/oci.md: main added the manifest arm's read rows, so Every row below is the blob arm's no longer held. Took main's preamble and kept the one clause it has no reason to carry.

It also fixed two files git merged cleanly that then did not compile: internal/format/oci/remote_e2e_test.go, main's new proxy harness, called NewRemoteCacheStore with four arguments and implemented UpsertCacheFill returning a bare error; and this branch's own cmd/artifact-registry/wire_oci_accounting_integration_test.go, which passed emitter, nil, nil to the new wireOCIWithStore signature. The harness now passes a nil sink and reports both facts off the pre-write state it already holds, rather than a zero value that would read as a charge.

The second merge took one conflict, in docs/dev/storage-accounting.md, where each side was right about a different sentence. !2232 (merged) landed second, which is the collision ## Merge order predicted, so its npm sentence stands: it credits the cache rows an npm whole-package delete removes, and this branch still said that arm misses them. The container sentence is this branch's, because main's reads "Once a caller turns the container remote figure into a counter delta" and this MR is that caller.

The rebase then discarded every one of those resolutions that lived in a merge commit rather than in a replayed commit, which cost four files and broke two compiles. fix(oci): carry the rebase's dropped conflict resolutions brings them back, and the check is by content rather than by inspection: the rebased branch's tree is identical to the merged branch's, git diff b2523e85e 9ac446fcc empty. Because a rebase runs no pre-commit hook on a replayed commit, the gate was re-run by hand afterwards: build, go vet with and without -tags=integration, every unit suite under internal/ and cmd/, scripts/ci/check-comment-caps.sh, and golangci-lint with --build-tags=integration. That last one was run on two of the changed Go packages rather than all of them, and reported clean; internal/datastore was not among the two and did report, which round 3 below covers.

The pipeline for the pre-rebase head is green: 65 jobs, 62 success and 2 manual, with go_unittests, golangci_lint, lint:comment-caps, all twelve test:integration:datastore shards across Postgres 16, 17 and 18, test:integration and test:integration:migrations on all three, and all four conformance:* jobs passing. editorconfig_check failed on two earlier heads with ec: not found and exit 127 from the mstruebing/editorconfig-checker image, which was breaking roughly twenty MRs in this project at the time; it has since been fixed upstream and now passes.

Review round 3, and the third merge

Nine non-blocking findings, one commit each, plus a merge with main. Nothing in them changes what the fill computes or emits; the delta table above still holds.

Three land in docs/dev/storage-accounting.md:

  • a3ec4a4a8 gives the Container remote grid the cache fill that fails after committing the blob row the npm and Maven grids carry. The absence was correct while the container cache-fill row read GAP on all four columns; flipping that row to emits is what puts this arm inside the same internal/remote window, so the missing row read as "this arm is not subject to it".
  • 7d70c7de7 re-derives the non-zero-delta census over the whole set rather than over the delta this branch adds: CacheStore.emitCommittedFillCounters in internal/format/npm/npmremote/remote_fill_emit.go was missing, making it eight functions across seven files, and the guard now lives in CounterSink.emitRepoByID rather than in emitRepo, which this branch left a plain forwarder.
  • 4c37f0dde drops "no site needs one without the other", which the line this branch added three above it contradicts. CounterSink's own doc was corrected for the fill's case and the prose was not.

d1a5368ba takes NpmPackageReaper out of the S20-a bullet's second group and drops the headline count to one. The bullet kept it there because an npm packument write "raises the column no more than hosted OCI does", which storage-accounting.md and emitRebuildCounters in internal/format/npm/packument_cache.go both contradict. The clause predates this branch; what is new is that this branch re-derived the count on top of it. With ContainerRemoteManifestReaper's own departure that group is empty, so the bullet now names it where it is first used instead of counting two sides.

6feb77e21 covers emitSiteRemoteCacheFill, which had none: the fill takes no membership read, so it never reaches the dropped-delta record TestCounterSink_MembershipReadFailureRecordNamesItsSite tables over the five reading sites, and substituting emitSiteBlobFinalize at the call left every suite green. The new case drives a panicking emitter through emitCommittedFillCounters over a capturing slog.Handler and asserts emit_site=remote_cache_fill, which is Maven's TestFillCounters_DispatchPanicLogNamesTheArm. Verified against that mutation, which now fails with actual blob_finalize.

aaae956ea restates TestBuildOCIRemoteFill_AcceptsNoSink's doc. It read "A boot that linked no BlobStore builds no cached arm, so the sink is nil there", and buildOCIRemoteSlots builds ociRemoteFill inside if store != nil, so a storeless boot assembles no fill at all. 14b94d22b removed that claim from three files and did not reach this one.

e19cc9325 unbundles two var declarations in container_remote_fill_accounting_integration_test.go. golangci-lint with --build-tags=integration reported two wsl findings there, and .golangci.yaml sets no run.build-tags, so CI never compiles the file. Re-measured at e19cc9325 with golangci-lint 2.13.1 and guardrail 7's flags: none on any file this branch touches, in any of the four changed Go packages. internal/datastore also reports contextcheck at package scale and internal/remote one paralleltest, both pre-existing and on files this branch does not touch.

dddbff074 retires the while it emits no deltas guard on the container arm from the Help string of gitlab_artifact_registry_remote_cache_fill_post_blob_commit_failures_total and from docs/dev/observability.md's copy of it, using the Known gaps row's own wording. Neither file was otherwise in this diff, which is why nothing flagged them. An operator reading the Help at /-/metrics would otherwise discount a container increment carrying a real deduplicated_size_bytes loss.

The third merge took the two conflicts ## Merge order predicted, both in documents and both on this fill's own claims. In docs/dev/storage-accounting.md, main rewrote the npm and Maven single-delete eviction rows from GAP to emits and this branch rewrote the container fill's emit-site row, so both changes are kept; main's "Nothing raises repositories.size_bytes for a container remote cache fill" is the claim this branch falsifies, so this branch's replacement stands, retargeted at ContainerReaper, which !2272 (merged) moved out of the group the old sentence compared against. In docs/specs/S20-a-lifecycle-closed-beta.md, !2272 (merged) moved ContainerReaper out of the first group and this branch moves ContainerRemoteManifestReaper out of the second, and both moves are kept. !2272 (merged) landed first, so resolving the collision is this branch's, and d1a5368ba is where the two counts are made to agree.

Follow-ups this does not do

  • ContainerRemoteManifestReaper already reports the bytes it frees, so unlike the npm and Maven arms this one needs no reap-side follow-up for size_bytes. #728 still owns the missing purge level for the remote parent tables.
  • The Maven and npm rows are still missing from the emit-site table in docs/dev/storage-accounting.md, which this MR adds the container row to. That is a docs/dev fix rather than a spec MR: docs/specs/S22-storage-accounting.md was reduced to a landing page by 0d825f90c and its own text retires the numbered acceptance criteria, so there is no criterion 25 and no spec-side site table to amend. !1901 (merged) and !2123 (merged) both landed without one.

Closes rather than Related to, and the reasoning is checkable rather than implied: #834 (closed)'s "Done when" asks that both fill paths emit their four deltas and that a reconciliation pass move no counter, "for npm and for Container alike". The npm half's evidence is !2123 (merged), which has merged; the Container half's is this MR's TestContainerRemoteCacheFill_ReconciliationParity and the delta table above. Nothing the issue asks for is left once this lands.

Closes #834 (closed)

Edited by Sylvia Shen

Merge request reports

Loading
Loading