feat(npm): remote cache fills emit their storage-accounting deltas
What
An npm 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, CacheStore.UpsertCacheEntry wrote npm_remote_versions,
npm_remote_files and npm_remote_metadata_files 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 npm_remote_versions row |
repositories.size_bytes |
the incoming blob when the repository did not already reference it, less the digest the write displaced |
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 npm arm of #834 (closed). The Container arm is a separate MR, and the two touch disjoint files.
Process note
The operator approved landing this without a plan MR, an acknowledged deviation from the
plan-before-code guardrail. No plan under docs/plans/ declares #834 (closed)'s npm arm, and
docs/plans/2026-08-04-s22-storage-accounting.md's Status table has no remote-cache-fill
step. The research that would have gone into that plan is in this description and in the
doc comments the diff adds, which is the shape !1901 (merged) used for the Maven arm. Because no
plan declares the work the title carries no (<plan token> plan: <step>/<total>) marker;
adding a plan would make it required.
Approach
Ported from the merged Maven arm, !1901 (merged), whose
internal/format/maven/remote_fill_emit.go this mirrors so the three arms read
alike. Three differences are forced rather than chosen:
-
The facts assemble in the format package. Maven has one datastore method owning the whole fill; npm composes four store seams, so no datastore method sees it.
npmremote.CacheStorecombines them. -
npmremotedeclares its ownCounterEmitter.internal/format/npmimportsnpmremote, so the reverse import is a cycle.cmd/artifact-registry/wire_npm.gocarries the_ npmremote.CounterEmitter = (*accounting.Emitter)(nil)anchor. -
npmRemoteRepoStillReferencesBlobStmtis reused, not twinned. The eviction arm already wrote it, it filterssoft_deleted_atat none of its three levels, and that is what makes it answer the same numberrecomputeNpmRemoteFilesSizeStmtsums. Its runner widens from*sql.Txtoqrm.DB, because ADR-023's raw-SQL ban stops a format package importingdatabase/sql. -
The read-arm provider takes the emitter too, and nothing on that arm fills.
RemoteOperationsProviderandnpmremote.FlightRegistryDepsbuild the samenpmremote.CacheStore, so one emitter is handed to both construction sites rather than letting the two shapes diverge. Only the flight registry's seam reachesUpsertCacheEntry, which is whyNewRemoteOperationsProvider's doc says its copy reaches no fill.
Displacement is settled after the repoint, on Maven's terms:
UpsertNpmRemoteMetadataFileForBlob repoints a packument row when upstream metadata
changes, so a packument refresh would otherwise climb size_bytes forever while the
recompute did not follow.
Testing
TestNpmRemoteCacheFill_ReconciliationParity is the done-when arm: it runs
RecomputeArtifactsCount, RecomputeSizeBytes and RecomputeComponentsCount before
and after a five-fill sequence and asserts each moved by what the facts sum to, with
require.NotZero guards so it cannot pass vacuously.
TestNpmRemoteFileStore_RemoteRepositoryHoldsBlob covers all three levels at which the
probe and the recompute could disagree. The version and package legs were each verified
against an injected AND ... soft_deleted_at IS NULL: either predicate could otherwise
have been added with the suite still green.
TestBuildNpmDispatcher_ThreadsCounterEmitterIntoTheRemoteFill is the wiring arm.
buildNpmRemoteOperations carries the emitter to the fill's seam on two lines that are
optional and nil-inert by design, and NewFlightRegistry validates neither, so dropping
either boots green and emits nothing forever with no metric, no log and no error. It drives
one tarball fill through the built dispatcher and reads the repository scope's live hash out
of Redis; it was verified to fail with either line removed. It is also the only case that
composes npmremote over the four real datastore stores, which is what makes the repository
scope name the parent repositories.id rather than the npm_remote_repositories surrogate.
TestNpmRemoteFileStore_RemoteRepositoryHoldsBlobStmt_PrunesEveryArm covers the plan.
npmRemoteRepoStillReferencesBlobStmt was worker-only, once per marked row; the probe puts
it on the cache-fill path twice per fill and three times on a digest-changing refill, while
the parent row is held FOR UPDATE. Its tarball arm prunes through join-derived equivalence
rather than a predicate per table, which is the pruning that can be lost with the SQL
unchanged.
namespace_statistics.deduplicated_size_bytes has no parity assertion. Session.Commit
decides it and no store in this package sees it, matching
TestMavenRemoteCacheStore_UpsertCacheEntry_ReconciliationParity.
Diff size
2750 insertions across 43 files, past guardrail 13's 500 reviewable LOC. A split would not help: production is 516 added and 274 removed, and the rest is tests plus 45 lines of docs.
| Group | Added | Removed | Files |
|---|---|---|---|
internal/format/npm/npmremote prod |
264 | 26 | 3 |
internal/datastore prod |
203 | 208 | 8 |
internal/format/npm prod |
34 | 34 | 3 |
cmd/artifact-registry prod |
15 | 6 | 2 |
| tests | 2189 | 182 | 23 |
| docs | 45 | 7 | 4 |
Roughly 180 of the production lines are mechanical return-shape plumbing (error to
(*DisplacedBlob, error), (uuid.UUID, error) to (uuid.UUID, bool, error)), and most
of the 208 datastore removals are comment-cap compression rather than deleted logic. Of
the test lines, the bulk is the two new suites; the roughly 25 UpsertNpmRemoteVersion
call-site updates are one line each.
e2e scenarios
None added or affected, per guardrail 12. The npm remote scenarios in
docs/testing/e2e/npm.md assert these counters only through the reconciliation pass,
which this MR leaves answering the same numbers. Closing the fast-path gap is the point.
Records this corrects
Two claims elsewhere in the tree said no merged path raises repositories.size_bytes
for an npm remote cache fill, which this MR falsifies:
docs/dev/storage-accounting.md in two places, and a comment in
lifecycle_reap_npm_remote_integration_test.go.
That same paragraph named work item #681 (closed) as tracking ContainerReaper and
NpmRemoteVersionReaper. #681 (closed) is closed and its scope names neither arm, so it now
names #942 (closed), which is their actual reap-side successor.
docs/dev/database-query-patterns.md gains #### The remote-cache twins. The two
remote repoint helpers cited an ordering paragraph this MR's comment-cap compression
removed from the exported upserts, and that page is already where the sibling
repointMetadataAttachment points for the same reason.
Three more records read against the code this branch writes, and are corrected here. That
new section said repointMetadataAttachment has no step 0 and that the missing lock is
what carries the cross-replica serialization. It has one, and the same page says so at
:58 and :66; the real distinction is that its step 0 locks an npm_packages row and
checks a rebuild fence the twins have no token for. Both twins and that section also said
"steps 0 to 4" over bodies this branch numbers to 5, and the section said they run no
charge or credit probe and that the byte accounting settles post-commit: step 5 is the
credit-side read, and both membership probes run inside the fill's transaction, which is
what makes a failed accounting read fail the fill rather than drop a delta.
npm.NewRemoteOperationsProvider's doc said a nil emitter leaves fills inert, which its
own emitter field comment contradicts: that emitter reaches the read-arm store, which
never fills. docs/dev/observability.md's npm row described the series as write-path-only
where !1901 (merged) updated the Maven row to name both arms, so it now names the fill as the second
producer of column="size_bytes" and records that the series carries no site label.
docs/dev/storage-accounting.md's reap-versus-fill lock inversion now also records that
the fill holds the parent row through its accounting reads.
colRepositorySizeBytes's doc enumerated what a shed retires at three of the five emit
sites, and this MR adds the fifth. The block is 17 lines under an unexported cap of 1, so
the enumeration moves to internal/format/npm/metrics.md, which carries no cap, and comes
back complete: one table row per emit site, packument_rebuild included, which the Go
comment never covered. The comment keeps the one claim a reader needs at the call site,
that a rate under the label is never a byte total.
displacedRemoteShadowSizeStmt's doc named MavenRemoteCacheStore.displacedRemoteShadowSize
as its only consumer. This MR adds npmRemoteDisplacedBlobSize as a second one, which
query_names.go already acknowledged ("the statement builder they share") and the builder
itself did not.
docs/dev/observability.md's column cell opened with a garden-path clause, "plus the
storage-accounting emits its read-path remote cache fill dispatches", where "emits" reads
as a verb before it resolves as a noun. It also claimed the fill is the higher-volume
producer, with nothing behind the comparison. Both are rewritten, and the cell now points
at internal/format/npm/metrics.md for the per-site table.
Follow-ups this does not do
NpmRemoteVersionReaperstill reportsSizeBytes == 0. This MR discharges its precondition, so a filled-then-reaped repository reads high onrepositories.size_bytesuntil the next reconciliation pass. #942 (closed) tracks the reap side anddocs/dev/storage-accounting.mdis updated to say so.counter_emit_dispatch_internal_test.go's header still counts three emitting sites and four properties, where there are now five of each. Same trade as the bullet above: the header is an 18-line run under a test-file cap of 2, and touching it at all subjects it to that cap, so correcting two numbers means deleting sixteen lines of load-bearing rationale. Measured: the caps gate accepts the file untouched and rejects any edit to that block.//nolint:thelperis still dead innpmremote/health_sweep_test.goandtarget_test.go. This branch does not touch those files; #682 owns the repo-wide trim. The nine dead directives incache_store_test.go, which this branch does touch, are removed here.
Merge order
!2163 (merged) gains a conflict with this branch in docs/dev/storage-accounting.md that it does
not have against main: both edit the Maven remote cache fill row of
### Where each format emits, !2163 (merged) for the container eviction and this branch to stop
that row denying the npm fill. Whichever lands second carries the npm correction. No
pipeline reports this.
!1973 (merged) and !1916 (merged) have merged, and this branch merged main in at 47df694b, so neither is
outstanding. !2040 (merged) no longer conflicts with this branch. !1011 (closed) conflicts with this branch
over the same 29 files it conflicts with main over, so that one is theirs rather than
this branch's merge order.
Measured at 47df694b with git merge-tree --write-tree --name-only.
Related to #834 (closed)