feat(accounting): backlog and missing-statistics collectors (S22 plan: 16/21)

What this step delivers

Two single-writer gauges for storage-accounting reconciliation, gated by one Redis lease. Both are label-free, and at most one process in the fleet emits them.

• reconciliation_backlog counts the namespace_statistics rows whose last_reconciled_at is older than storage_accounting.reconciliation_interval. The collector recomputes it on every scrape. A covering index exists for that predicate, and the planner chooses it only while the stale rows are a small fraction of the table; before a deployment's first reconciliation pass every row qualifies and the count is a sequential scan instead.
• namespaces_missing_statistics counts the namespaces that hold no namespace_statistics row. It is a NOT EXISTS anti-join, and no index serves it. The collector therefore caches its own result. It re-issues the count only when the cached value is older than storage_accounting.reconciliation_orphan_sweep_interval.

The same interval also floors a failed count, which is this step's one behavior change beyond the two new gauges. When CountNamespacesMissingStatistics returns an error, the collector does not issue that count again until one reconciliation_orphan_sweep_interval passes from the failure. The floor holds from boot, rather than only after the first success. So a failed first count delays the family's first appearance by up to one interval. What it costs is recovery latency: after the database answers again, the gauge keeps its stale value until the floor expires.

The floor adds no new constant. internal/config/storageaccounting.go names two consumers of this interval: the orphan sweep, and this collector's cache refresh. The >= 30m configuration floor exists to bound what those two cost. A failed count is a third use of the same value. It bounds the same unindexed anti-join, on the one path that had no bound at all.

The parts:

• internal/accounting/lease.go holds the Redis lease that elects the writer. It acquires with jittered retry, refreshes at half the lease duration, and releases the key at shutdown.
• internal/accounting/backlog_collector.go reads the lease once per scrape, before any database work. A process without the lease exposes no series at all, so it costs nothing per scrape.
• internal/datastore/reconcile_backlog.go carries the two counts, under the query names namespace_statistics_select_stale_count and namespaces_select_count_missing_statistics.
• cmd/artifact-registry/wire_accounting_metrics.go registers the lease as a component and the collector on the prefixed registry, from wireServices. When the cache-purpose Redis client or the database pool is absent, it registers nothing and boot still succeeds.

A local run exercised two instances of the service against one Redis and one PostgreSQL. Exactly one instance emitted the two families, and the other emitted neither.

Spec coverage

Spec: docs/specs/S22-storage-accounting.md

Step 16 owns acceptance criterion 29 and the two gauges' share of criterion 33's metric-registration half. Every other row names the step that owns it.

Acceptance criteria

#	Criterion	Tests
AC-1	Repo-scoped increment reaches repositories after a drain tick	Steps 7-8 (chunk workers). Not tested in this MR.
AC-2	Namespace-scoped increment reaches namespace_statistics	Steps 7-8. Not tested in this MR.
AC-3	Concurrent increments to one scope sum exactly	Steps 5-6. Covered by TestEmitCounters_ConcurrentEmitsSumExactly.
AC-4	Re-increment during a claimed batch loses no delta	Steps 8, 14. Not tested in this MR.
AC-5	Stale-claim admission check bails and re-adds	Step 8. Not tested in this MR.
AC-6	Chunk failing every attempt re-adds before the terminal error	Step 8. Not tested in this MR.
AC-7	Redis unavailable at increment time does not fail the operation	Steps 6, 14. Covered by internal/accounting/emit_faults_integration_test.go.
AC-8	Reconciliation clears buffered state before the scan	Step 14. Not tested in this MR.
AC-9	Soft-delete visibility per format in the recomputes	Steps 11-13. Not tested in this MR.
AC-10	Positive hit per version-type table and (format, kind)	Steps 11-13. Not tested in this MR.
AC-11	Drift recorded on the unit-matched histogram before write-back	Step 14. Not tested in this MR.
AC-12	Crash between HINCRBY and SADD still captured	Steps 6, 14. Not tested in this MR.
AC-13	Sliding TTL refreshed on every write; dirty sets never expire	Step 5. Not tested in this MR.
AC-14	Migrations apply cleanly and roundtrip through jet	Steps 1, 2a, 2b. Not tested in this MR.
AC-15	Shadow-table triggers keep the shadow consistent	Step 2b. Not tested in this MR.
AC-16	Every namespace has a zeroed statistics row by construction	Step 1. Relied on as a fixture here: dropStatisticsRow requires the trigger to have created the row it removes.
AC-17	npm publish and unpublish emit sites	Step 17. Not tested in this MR.
AC-18	OCI increments at blob finalize, mount, and manifest PUT	Step 18. Not tested in this MR.
AC-19	OCI decrements at the delete handler	Step 18. Not tested in this MR.
AC-20	Maven upload emits from a post-commit site	Step 19. Not tested in this MR.
AC-21	Repository cascade hard-delete emits through S20-A's purger	Integration contract verified in #464 (closed) / S20-A. Not tested in this MR.
AC-22	One asynq task per namespace candidate, never per repository	Steps 14-15. Not tested in this MR.
AC-23	A namespace with no statistics row gets one on its first pass	Step 14. Not tested in this MR.
AC-24	In-flight reconciliation tasks never exceed the cap	Step 14. Not tested in this MR.
AC-25	Source-first ordering at every emit site	Steps 17-19. Not tested in this MR.
AC-26	last_reconciled_at stamped only after every repository	Step 14. Not tested in this MR.
AC-27	Trigger selects stale namespaces; the sweep reaches the rest	Step 15 owns both walks. The criterion's closing clause — namespaces_missing_statistics reports what the sweep walks and reads zero once the row exists — is covered by TestBacklogCollector_MissingStatisticsIsCachedBetweenSweeps, TestBacklogCollector_MissingStatisticsRefreshesAfterTheSweepInterval, and the zero-value assertion in TestBacklogCollector_LeaseHolderReportsTheStaleCount.
AC-28	UniqueByArgs caps a namespace at one outstanding task	Step 15. Not tested in this MR.
AC-29	Single-writer reconciliation_backlog: holder value, non-holder silence, exactly one of two	TestBacklogCollector_LeaseHolderReportsTheStaleCount (holder value equals the seeded stale count, and returns to zero after a full hand-stamped catch-up), TestBacklogCollector_TwoInstancesExactlyOneEmits (non-holder exposes no sample; exactly one of two emits), TestBacklogCollector_LosingTheLeaseStopsTheSamples (the gate is holding the lease now, not having held it), TestIntegration_BacklogCollectorReachesTheAssembledRegistry (the collector reaches the registry /-/metrics is served from).
AC-30	Chunk reconciled mid-flight skips its scope	Step 8. Not tested in this MR.
AC-31	counter_dirty_set_size sampled once per tick before SPOP	Step 10. Not tested in this MR.
AC-32	Config load rejects each invalid configuration	Steps 3, 3b. Covered by internal/config's storage-accounting suite.
AC-33	Six metrics registered with exact names, types, and label sets	This MR carries the two gauges' share: TestBacklogCollector_ExposesTwoLabelFreeGauges (both names, gauge type, empty label set on descriptor and gathered family) and TestIntegration_BacklogCollectorReachesTheAssembledRegistry (both prefixed families reach the composition root's registry). The bounded-label-value clause is vacuous for both — neither carries a label. counter_dirty_set_size, reconciliation_drift_bytes, reconciliation_drift_rows, and counter_drain_chunk_bailed_total belong to Steps 8, 10, and 14. The three paging alerts land with #354, which has no alerting surface to wire against here.
AC-34	Reconciliation saturation policy re-enqueues, never sheds	Step 14. Not tested in this MR.
AC-35	Namespace-scoped chunk with no statistics row drops and deletes	Step 8. Not tested in this MR.
AC-36	A failing recovery SADD does not lose the delta	Steps 8, 14. Not tested in this MR.
AC-37	Management-API deletes emit once their transaction commits	Integration contract verified in #313 (closed). Not tested in this MR.
AC-38	A persistently failing task leaves its namespace re-enqueueable	Step 14. Not tested in this MR.

Error cases

#	Condition	Tests
E-1	Redis unavailable at increment time	Steps 6, 14. Not tested in this MR.
E-2	Redis unavailable at drain-trigger time	Step 10. Not tested in this MR.
E-3	Chunk's Postgres UPDATE fails	Step 8. Not tested in this MR.
E-4	Chunk's :flushed DEL fails after the UPDATE succeeded	Step 8. Not tested in this MR.
E-5	Chunk exhausts all retry attempts	Step 8. Not tested in this MR.
E-6	Recovery SADD itself fails	Steps 8, 14. Not tested in this MR.
E-7	Chunk dequeued past drain_chunk_stale_timeout	Step 8. Not tested in this MR.
E-8	Worker dies mid-chunk after merging into :flushed	Step 8. Not tested in this MR.
E-9	Two chunks run one scope concurrently	Steps 8, 14. Not tested in this MR.
E-10	Trigger's EnqueueTx fails while the process is alive	Step 10. Not tested in this MR.
E-11	Crash between a trigger's SPOP and its EnqueueTx	Step 10. Not tested in this MR.
E-12	Assigned row hard-deleted before its chunk drains	Step 7. Not tested in this MR.
E-13	Namespace-scoped chunk drains a namespace with no statistics row	Step 8. Not tested in this MR.
E-14	Crash between HINCRBY and SADD	Steps 6, 14. Not tested in this MR.
E-15	Crash between reconciliation's clear and its SET	Step 14. Not tested in this MR.
E-16	Reconciliation scan races a concurrent increment	Step 14. Not tested in this MR.
E-17	A chunk and a reconciliation process one scope concurrently	Steps 8, 14. Not tested in this MR.
E-18	Reconciliation finds a discrepancy	Step 14. Not tested in this MR.
E-19	Namespace has no statistics row when its task runs	Steps 14-15 own the UPSERT and the sweep that reaches it. The state itself is this MR's fixture: dropStatisticsRow produces it and the gauge counts it.
E-20	Task fails before its final UPSERT	Steps 14-15. Not tested in this MR.
E-21	A namespace can never be reconciled	Steps 14-15 own the retry cadence. The "stays counted in reconciliation_backlog" clause is what TestBacklogCollector_LeaseHolderReportsTheStaleCount asserts: a namespace whose last_reconciled_at does not advance stays in the count.

Security considerations

#	Concern	Tests
S-1	Redis keys carry only internal UUIDs, so no key injection or slot relocation	Steps 5-6 own the counter key grammar. This MR adds one Redis key, the lease key, which carries no user-derived segment: production supplies a constant and the suite supplies testutil.UniqueKey.
S-2	Counter values feed billing, so drift has financial impact	The gauge that signals reconciliation falling behind is what this MR delivers: TestBacklogCollector_LeaseHolderReportsTheStaleCount pins that it counts exactly the overdue namespaces and returns to zero on catch-up. The drift histograms belong to Step 14; the paging rules land with #354.
S-3	No new credential surface: the existing cache client and datastore pool are reused	TestIntegration_BacklogCollectorReachesTheAssembledRegistry runs the collector through the composed app, which builds no client of its own; newBacklogEnv builds the suite's client through redisclient.NewCacheClient, the same cache-purpose constructor the composition root wires.

The bsm/redislock dependency

github.com/bsm/redislock v0.10.0 is new in go.mod, and docs/dev/go-libraries.md carries its entry. Acceptance criterion 29 requires exactly one emitter fleet-wide for the two gauges. River's leader elector is unexported, so the collector elects its own writer over a Redis lease. ADR-006 names distributed locking over Redis as an intended use of the Redis tier, and names go-redis as the client. redislock sits directly on that client: redislock.New takes a redis.Scripter, which the cache-purpose UniversalClient already satisfies. It opens no pool and adds no operational dependency. Its refresh and release compare the lock's stored token before they act. The license is Apache-2.0, which is not the MPL case that triggered River's legal review. go mod tidy ran, and internal/accounting/lease.go is the only importer.

e2e scenario catalogs

No e2e scenario is added or affected. docs/testing/e2e/ catalogs the Docker, Maven, npm, and OCI protocol scenarios. This step adds two Prometheus gauges behind a Redis lease. It adds no route, no format behavior, and no protocol surface for a scenario to exercise.

Reviewable LOC

The diff is 24 files, 2431 insertions and 53 deletions, so guardrail 18 applies. The split by file group, measured on git diff origin/main...HEAD, with HEAD at a6299e83:

Group	Insertions
Production Go	844
Tests	1523
Docs, CI, and dependency files	64

Tests are about 63% of the insertions.

A split does not help here. Criterion 29 is a property of three parts together: the collector, the lease, and the wire file. The two-instance test needs all three, and the composition-root test needs the wire file. A collector without its lease has no criterion left to satisfy, and a lease without a consumer has no test that can fail. The branch is 43 commits at ff743e25, rebased onto main. The first five split the change by concern: the Go change, the developer docs, the CI comment, the run recipe, and the plan corrections. The other 38 each carry one review fix, over one to four files each. A commit-by-commit review keeps each part small.

Merge order

!1650 (merged) and this merge request each add a package-wide TestMain to internal/accounting, and git merge raises nothing about it. This branch's sits in internal/accounting/backlog_collector_integration_test.go, and !1650 (merged)'s sits in a new file of its own. Go permits one TestMain per test binary, so whichever merges second leaves that package's tests unable to compile. The two definitions sit in different files, so both sides apply cleanly and the failure appears at go test rather than at the merge. A rebase alone therefore does not resolve it: the second branch has to delete its own TestMain and keep the other. The order this argues for is this merge request first, with !1650 (merged) dropping its TestMain when it rebases.

!1650 (merged) and this branch also rewrite the same text in four shared files, and each of those conflicts in either order.

• .gitlab-ci.yml, in both the test:integration comment and the test:accounting-failpoints "Both backends" comment. Both branches reach the same conclusion — internal/accounting's integration suite needs Redis and PostgreSQL together — and credit a different cause. After both land, the surviving comment names the TestMain that survived.
• internal/accounting/metrics.go, in the accountingCollectors comment. Both rewrite the claim that appending to the list is the whole of what a new collector needs.
• internal/accounting/doc.go, at the same insertion point in the package doc.
• docs/dev/storage-accounting.md, at the sentence about that same registration list.

!1650 (merged) also fills Status row 8 of the S22 plan where this branch fills row 16. It adds a metric-catalog row above the two this branch appends. Both are separate positions, so a clean merge is expected on each.

!1535 (merged) and this merge request both append one row at the end of the metric catalog table in docs/dev/observability.md. Both append after the same last row on main, gitlab_artifact_registry_crypto_fallback_unwraps_total, against an identical pre-image. A rebase conflict there is expected, not merely possible. The rows are independent: !1535 (merged) adds a different metric, and no open merge request mentions either gauge name. The one that merges second carries the conflict, and the resolution keeps both rows. Neither merge request blocks the other.

internal/datastore/query_names.go needs two resolutions in the same // namespaces. block. main deleted queryNamespacesUpdateFillOrganizationID and re-aligned that block, and this step re-aligns it to insert queryNamespacesSelectCountMissingStatistics. A rebase onto main resolves that one, keeping the deletion and the new constant. !1651 (merged) inserts its own namespaces page constant into the same block, which is the second resolution, in either merge order. That one keeps both constants.

The other open merge requests checked against this diff are clean. Four edit docs/dev/observability.md in other parts of the table or the file. Seven edit .gitlab-ci.yml away from both comments this step touches. Three change go.mod or go.sum, which resolves by taking both sides and running go mod tidy. No open merge request creates or edits any of the nine files this step adds.

Notes for the reviewer

The lease releases on the first refresh failure. Spec S27 prescribes three consecutive failures before a release, so a reviewer who knows S27 can read this as drift. S22 governs this step. Its ### Observability section binds only the single-writer property, and states that either mechanism is conformant while at most one pod emits the series. A release at the first failure can only shrink the set of emitters. The cost is an absent series until some pod re-acquires the key, which the metric's contract already admits for a non-holder. The reason is on the hold doc comment in internal/accounting/lease.go.

This merge request corrects six claims in the plan.

• The plan stated that a panic inside the Prometheus gather goroutine is unrecovered. On the pinned client_golang v1.24.1 the registry recovers it into a gather error, so the process survives and the whole /-/metrics response is lost instead.
• The plan stated that criterion 33 covers five metrics, and credited Step 16 with reconciliation_backlog alone in its Acceptance line. The spec enumerates six and names namespaces_missing_statistics, which this step registers and asserts at both levels.
• The plan described cmd/artifact-registry/wire_accounting_metrics.go as collector registration only. The file also builds the lease, registers it as a non-critical component, and owns the lease key and its TTL.
• The plan's cardinality-table sentence quantified over Steps 8, 10, 14, and 16. Step 16 carries no such edit: AuditCardinality keys both halves off a descriptor's declared label names, and both of this step's descriptors are label-free.
• The plan's placement exception claimed this step asserts "the same property" in its own test. That test reads the assembled gatherer rather than the recorded collector set, and the plan now names the divergence. The collector-set requirement exists for unobserved label vectors, and both of this step's metrics are label-free const metrics.
• The plan stated that the (last_reconciled_at, namespace_id) index answers the backlog count without a heap fetch. Step 1's migration records that the unbounded count takes a sequential scan while stale rows are not a small fraction of the table. That is the state a deployment is in before its first reconciliation pass. The plan now separates the index covering that count from the planner choosing it.

Ten files carry a change the plan's step 16 does not name. Derived from git diff --name-only origin/main...HEAD against that step's Files and Tests entries.

• .gitlab-ci.yml: the test:integration: comment stated that the internal/accounting integration suite needs one Redis and no database. This step makes that false, because the collector leases on Redis and counts through PostgreSQL. The test:accounting-failpoints comment is the second site: its PostgreSQL was anticipatory, and this step's TestMain makes it a precondition. No job behavior changed, and scripts/ci/check-integration-test-wiring.sh passes.
• .claude/skills/run-artifact-registry/SKILL.md: guardrail 21. The two gauges need a reachable Redis, which the driver does not start, and an absent gauge there is the documented non-holder behavior. Guardrail 21 is the rule that applies here: it requires this recipe to change in the same merge request as the change to how the service boots. So the edit does not ride along as unrelated work, and the dedicated-merge-request rule for skill changes in docs/dev/conventions.md is not engaged. That rule also asks a skill merge request for benchmark results, and none is owed here.
• internal/datastore/container_tag_test.go: the comment on nilContextName listed the files that reference the constant. That list was already out of date on main, where 38 files use it, and this step adds a 39th. The project's docs rule prefers a grep over an enumeration, so the comment now points at the grep.
• internal/datastore/query_names.go: the two new query names. No step in this plan names this file, including merged Step 6, whose merge request also edited it.
• internal/accounting/metrics.go: the accountingCollectors comment said that appending to the list is the whole of what a new collector needs. This step's collector cannot go through the list. It needs a store over the database pool and a started lease, and RegisterMetrics' call site holds neither. The comment now says which collectors the list can carry, and names the wire file that registers this one.
• internal/accounting/doc.go: the package doc described the emit pipeline alone, and this step adds two exported types beside it. It now names BacklogCollector and Lease, and its Redis-key sentence covers the lease key as well.
• docs/dev/storage-accounting.md: the same claim as metrics.go, in the operator-facing document. It now names wireAccountingMetrics as the other route onto the scraped registry.
• docs/dev/configuration-reference.md: the reconciliation_orphan_sweep_interval entry named two uses of the interval, the orphan sweep and this collector's cache refresh. The retry floor is a third use of the same value, so the entry now names it. The twice over count stays, because it counts consumers and the floor is a second gate inside one of them.
• internal/config/storageaccounting.go: the same gap in the sentence that explains the 30m floor. It now says the collector waits the interval out again after a count that failed. Editing this file is what makes the configuration-reference edit above mandatory rather than discretionary.
• docs/specs/S22-storage-accounting.md: three claims rested on the refresh model this step changed, and the spec was the last surface still on the old one. In the namespaces_missing_statistics row, the accuracy sentence asserted the one-interval bound unconditionally, the refresh clause named only the cache-age gate, and the Type cell named only the sweep's cadence. The orphan-sweep interval's cost paragraph named two roles where this step gives the interval a third, which 710a788a9 had already carried into the two sibling sites. The twice over count stays there too, for the same reason it stays in the configuration reference.

Four more paths are outside those two lists, and the group above leaves them out on purpose. internal/accounting/backlog_collector_test.go, internal/accounting/lease_test.go and internal/datastore/reconcile_backlog_test.go are the unit suites of three files the Files list names. The plan file itself carries the corrections above.

A lease hand-off can move namespaces_missing_statistics backwards, and a failing count can freeze it. The cache is per process, and its expiry is keyed on the age of the cached read rather than on lease state. So a new holder can emit a fresh value, and a pod that regains the lease can then emit its older one. Across a hand-off the gap is one reconciliation_orphan_sweep_interval, which the spec gives as the accuracy of this gauge.

That bound covers a count that succeeds. The failing count is the second path: when CountNamespacesMissingStatistics fails after at least one success, the collector re-emits the cached value and leaves the cache timestamp where it was. The collector does not re-issue the count until one reconciliation_orphan_sweep_interval passes from the failure. A transient error therefore costs up to one interval, not one scrape. While the count keeps failing, the exported number ages with the outage, and nothing bounds how stale it gets.

Prometheus cannot see this, because a re-emitted sample looks live. The cached value is almost always zero, and the spec reads a non-zero value as a broken schema invariant. So a cached zero reports the invariant as intact for the length of the outage, and the warning log is the only live signal. This merge request keeps that emit behavior, and it qualifies the spec's bound to match. What it changes is how often a failed count runs. The missingStatistics doc comment, the metric-catalog row, and the spec's own row all state the exception. Whether a failing read must also change the emit behavior travels to #354, the work item that will wire an alert against this gauge.

Each count carries a countTimeout of 5 s, and the worst case still spends 10 s. One context used to cover both counts, and Collect issues the backlog count first, so a slow backlog count left the orphan count whatever remained. A deadline the database had not earned then floored that count for a whole reconciliation_orphan_sweep_interval, because missingStatistics floors every failure the same way. Each count now takes its own budget where its query is issued, so a stalled count reaches only its own gauge. The two counts still run in sequence, so the worst case spends both budgets. The registry gathers every collector before the handler writes any part of the body, so that spend is a floor on the whole /-/metrics response. Prometheus's own default scrape timeout is the same 10 s. Measured against a paused database, under one shared budget, one scrape answered 200 after 10.001950 s and carried 201 families, which a Prometheus at that default then drops. Splitting the budget bounds which gauge a stall reaches, and it does not lower that sum. No value is derived from the deployment's scrape timeout, because nothing in this repository states one. .runway/ declares the metrics port and opts a ServiceMonitor in, so Runway's chart and collector choose the timing. Whether the total belongs below the deployment's scrape timeout travels to #354, in a note of its own.

Caveats

• ./scripts/adr-freshness.sh exits 1: the local mirror lags the handbook, and it lags on ADR-007, ADR-020, ADR-021 and ADR-022. No part of that delta touches a surface this diff has. The two ADR-007 paragraphs this step rests on came from a direct read of the handbook and are byte-identical to the mirror, so the text this step was written against is the current text. The mirror syncs on its own schedule, so a later run of the script reports a different set.
• A second class of ADRs lives outside this repository. docs/adr/README.md, under ## Mirror freshness and internal ADRs, states where that class is read. It was not reachable with the token available to this run.

Related to #515

This is a bot message 🤖 — /smurfit