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 setblob_sha256from that same digest, andexistingContainerRemoteAttachmentlooks the prior row up by that digest too, so a conflict re-points the coordinate at bytes it already named.container_remote_tagsdoes repoint, and it appears in neither the count walk nor the size walk. Sosize_bytestakes a charge here and never a credit, and porting Maven'sDisplacedterm is porting dead code. - Both counts key on the
container_remote_manifestsinsert.recomputeContainerRemoteManifestsStmtcounts it, and it is the second term ofrecomputeNamespaceComponentsCountStmt. A layer fill writes no such row, so it moves both byte totals and neither count. - The probe carries no
soft_deleted_atpredicate at either level, matchingrecomputeContainerRemoteBlobsSizeStmt, which filters at none of its three. The hostedcontainerRepoStillReferencesBlobStmtcould not be reused: it walkscontainer_manifestsandcontainer_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".
NewCounterSinkassigns the package-levelcounterEmitSemto every sink, andTestNewCounterSink_SharesTheProcessWideCapalready pinned that. Replaced with the reason that holds. docs/dev/storage-accounting.mdclaimed the fill "emits once its status and its body have both gone out". No arm does: a streamedGETcommits 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 windowarmBlobFillResponseDeadlinearms 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 intodocs/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_imagesseparately, so addingAND cri.soft_deleted_at IS NULLto 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 chargesize_bytesfor bytesrecomputeContainerRemoteBlobsSizeStmtalready 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".committedRemoteFillDeltasreads!RepositoryHeldBlobas 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 matchesMavenRemoteUpsertResult.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.factscarried the same claim.docs/specs/S20-a-lifecycle-closed-beta.mdstill 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 wayhad 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 belowUpsertCacheFill'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:
buildOCIRemoteSlotsbuildsociRemoteFillinside the samestore != nilgatewireOCIWithStoreassigns 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.godocuments "counters may be nil",internal/format/npm/npmremote/cache_store.gosays the same) andoci.NewRemoteCacheStoreruns 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
assertOCIOptionsWiredreads handler fields. Recorded alongside it: a nil there is silent past boot too, becausedispatchreturns at its own nil guard beforemeterCounterEmit. - The new e2e row gated the wrong namespace counter.
components_countmoves with thecontainer_remote_manifestsinsert whateverCacheEntry.Deduplicatedsays; onlydeduplicated_size_bytesis 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)!replacedwireOCIWithStore'siamClient/glazClientwith oneauthzWiringand deleted the line this branch'svar countershoist sat under. Tookmain's call and kept the hoist above it.emit_dispatch.go: both sides moved the header census out, to different homes.main's sidecarinternal/format/oci/emit_dispatch.mdwins, 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 indocs/dev/storage-accounting.md, which is the list that grew a sixth member here. Three pointers still promised thatcounterEmitTimeout's doc comment enumerates those reads, which this branch is what deletes: two indocs/dev/storage-accounting.mdand one inemit_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, becausemain's plain one-liner plus this branch's one-linecounterEmitTimeoutdoc 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. Droppedmain'sThe container remote cache fill emits nothingrow, which is the one this MR retires. Also re-pointedThat row is the one place the population is counted, whichmain'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:mainadded the manifest arm's read rows, soEvery row below is the blob arm'sno longer held. Tookmain'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:
a3ec4a4a8gives the Container remote grid thecache fill that fails after committing the blobrow the npm and Maven grids carry. The absence was correct while the container cache-fill row readGAPon all four columns; flipping that row toemitsis what puts this arm inside the sameinternal/remotewindow, so the missing row read as "this arm is not subject to it".7d70c7de7re-derives the non-zero-delta census over the whole set rather than over the delta this branch adds:CacheStore.emitCommittedFillCountersininternal/format/npm/npmremote/remote_fill_emit.gowas missing, making it eight functions across seven files, and the guard now lives inCounterSink.emitRepoByIDrather than inemitRepo, which this branch left a plain forwarder.4c37f0ddedrops "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
ContainerRemoteManifestReaperalready reports the bytes it frees, so unlike the npm and Maven arms this one needs no reap-side follow-up forsize_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 adocs/devfix rather than a spec MR:docs/specs/S22-storage-accounting.mdwas reduced to a landing page by0d825f90cand 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)