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.CacheStore combines them.

  • npmremote declares its own CounterEmitter. internal/format/npm imports npmremote, so the reverse import is a cycle. cmd/artifact-registry/wire_npm.go carries the _ npmremote.CounterEmitter = (*accounting.Emitter)(nil) anchor.

  • npmRemoteRepoStillReferencesBlobStmt is reused, not twinned. The eviction arm already wrote it, it filters soft_deleted_at at none of its three levels, and that is what makes it answer the same number recomputeNpmRemoteFilesSizeStmt sums. Its runner widens from *sql.Tx to qrm.DB, because ADR-023's raw-SQL ban stops a format package importing database/sql.

  • The read-arm provider takes the emitter too, and nothing on that arm fills. RemoteOperationsProvider and npmremote.FlightRegistryDeps build the same npmremote.CacheStore, so one emitter is handed to both construction sites rather than letting the two shapes diverge. Only the flight registry's seam reaches UpsertCacheEntry, which is why NewRemoteOperationsProvider'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

  • NpmRemoteVersionReaper still reports SizeBytes == 0. This MR discharges its precondition, so a filled-then-reaped repository reads high on repositories.size_bytes until the next reconciliation pass. #942 (closed) tracks the reap side and docs/dev/storage-accounting.md is 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:thelper is still dead in npmremote/health_sweep_test.go and target_test.go. This branch does not touch those files; #682 owns the repo-wide trim. The nine dead directives in cache_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)

Edited by Sylvia Shen

Merge request reports

Loading
Loading