chore(accounting): the per-namespace reconciliation task (S22 plan: 14/21)

What this step delivers

This step adds the per-namespace reconciliation task for storage accounting, as one asynq handler in internal/accounting. Nothing registers that handler yet. RegisterAsynqHandlers is still empty and nothing constructs the reconciler, so no pass runs in a running service. Step 15 registers the handler and adds the trigger that enqueues it.

What the handler does:

• Saturation policy. A counting semaphore sized by reconciliation_max_in_flight bounds the passes that run at once. On saturation the handler returns ErrReconcileSaturated before it takes a slot, so the job backend re-enqueues the task. The handler never sheds a namespace, and it never holds a worker while it waits for a slot. A refused admission is logged at Warn and names the cap. A real failure is logged at Error. jobs_processed_total carries status="error" for both, so the log line is the only place an operator tells them apart.
• Per-scope ordering. For each scope the pass clears the live and :flushed buffer pair, recomputes the counters from source, then writes them back. Every repository is written back before the final namespace_statistics UPSERT. So last_reconciled_at advances only after a whole pass succeeds.
• A pass budget inside the task deadline. The pass runs on its own context, derived from the task's deadline less reconcilePassUnwindMargin (10 s). asynq stops reading a handler's return value once the task deadline fires, and it classifies the context error itself. internal/jobsasynq reads that value as transient, so the attempt counter never advances and the final-attempt arm is never reached. A namespace too large for the default 30-minute task deadline then re-runs at the floor of asynq's backoff. Spec line 706 forbids that outcome in its "spun on hot" clause. The budget closes it: the pass returns while asynq still reads the return value, and the attempt budget advances. A resumable cursor stays out of scope. The walk still restarts at the first repository on every attempt.
• Drift reporting. Each drifting counter column reaches one of two unit-matched histograms, reconciliation_drift_bytes or reconciliation_drift_rows, under the level and direction labels. The same column also emits one Info line with the identifiers and both values, before the write-back overwrites the persisted counter. A scope that does not drift records nothing and logs nothing. The two levels do not read the persisted value the same way. The namespace level reads namespace_statistics immediately before its write-back. The repository level compares against the counter its enumeration page carried, up to a page of repositories earlier, so a drain inside that window is recorded as drift the column never held.
• No archived task. On its final attempt the handler records the failure and returns success. asynq archives a task whose attempt returns an error with the retry budget spent. An archived task keeps the hash its deterministic id derives from, for 90 days, and that hash locks its namespace out of re-enqueue. Criterion 38 forbids that outcome, because staleness re-selection is the retry loop this design relies on.
• A statement deadline. Every recompute call carries a 5 s deadline, as an unexported constant. It covers four of the eight store calls a pass makes: both repository recomputes and both namespace recomputes. The ADR-007 section below gives the ground for the value, names the four exempt calls, and states what the exemption leaves open.
• Constructor validation. NewNamespaceReconciler panics on each nil dependency, as NewBacklogCollector and datastore.NewRepositoryReconcileStore already do. A panic inside the handler never reaches the final-attempt arm, so a nil field reaching the first pass archives the task. The in-flight cap is floored at one slot, because config.StorageAccountingConfig is exported and a zero cap refuses every pass forever.
• A deleted namespace completes. datastore.ErrParentNamespaceMissing is a terminal outcome its contract asks the caller to match with errors.Is. This pass is its first caller. It now logs once with the namespace id and completes. The parent's delete cascades the statistics row away, so no later pass has anything to correct.
• One gofail seam, between the last repository write-back and the final UPSERT. It is the only way to inject the mid-task failure that criterion 26 asks for.

Spec coverage

The table comes from the test-author hand-back. It maps each acceptance criterion, error case, and security consideration to the test that covers it.

Acceptance criteria

#	Criterion (short)	Tests
AC-1	Repo-scoped increment reaches repositories after a drain	Step 8 (merged).
AC-2	Namespace-scoped increment reaches namespace_statistics	Step 8 (merged).
AC-3	Concurrent increments sum exactly	Steps 5/6 (merged).
AC-4	Re-marked scope, no lost delta; overlap clause	Overlap clause: TestNamespaceReconcile_OverlappingChunksSettleOnTheNextPass. Other clauses: Step 8 (merged).
AC-5	Stale-claim bail re-adds and meters	Step 8 (merged).
AC-6	Chunk failing every attempt re-adds before the terminal error	Step 8 (merged).
AC-7	Redis unavailable at increment; restore half	TestNamespaceReconcile_RestoresACounterASuppressedEmitDropped. Drop half: Step 6 (merged).
AC-8	Clear before scan; write-back equals source; buffer cleared	Whole: TestNamespaceReconcile_WritesTheSourceValueAndClearsEveryScopeBuffer.
AC-9	Soft-delete visibility; no re-inflation on a second pass	Second-pass clause: TestNamespaceReconcile_SecondPassDoesNotReInflateTheLiveCount (hosted OCI, parent tombstone). Per-format seeding: Steps 12/13.
AC-10	Positive hit per version-type table	Steps 12/13.
AC-11	Drift observed before the override; a non-drifting scope records nothing	TestNamespaceReconcile_ObservesDriftBeforeOverridingThePersistedCounter, TestNamespaceReconcile_ANonDriftingScopeRecordsAndLogsNothing.
AC-12	HINCRBY/SADD crash captured without dirty-set membership	TestNamespaceReconcile_CapturesADeltaLeftUnmarkedByACrash.
AC-13	Sliding TTL on the value keys	Step 5 (merged).
AC-14	Migrations apply and roundtrip	Steps 1/2 (merged).
AC-15	Shadow-table triggers	Step 2b (merged).
AC-16	Every namespace has a statistics row by construction	Step 1 (merged).
AC-17	npm publish increment swapped onto the pipeline	Step 17.
AC-18	OCI emits at the real sites	Step 18.
AC-19	OCI emits decrements at the delete handler	Step 18.
AC-20	Maven post-commit emit	Step 19.
AC-21	Repository cascade hard-delete	Gated on #464 (closed). Not this MR.
AC-22	One task per namespace; task half	TestNamespaceReconcile_WritesTheSourceValueAndClearsEveryScopeBuffer (two repositories plus the namespace row in one pass). Fan-out half: Step 15.
AC-23	Missing statistics row created on the first pass (UPSERT)	Statement owned by Step 13; exercised end to end by TestNamespaceReconcile_ANamespaceWithNoStatisticsRowSettlesAtSource.
AC-24	Concurrency never exceeds reconciliation_max_in_flight	TestNamespaceReconcile_CapsConcurrentPassesAtTheConfiguredMaxInFlight.
AC-25	Source-first ordering at every emit site	Steps 17/18/19.
AC-26	Namespace stamped only after every repository; mid-task failure leaves it	TestNamespaceReconcile_StampsTheNamespaceOnlyAfterEveryRepository (positive), TestNamespaceReconcile_AMidTaskFailureLeavesTheNamespaceStampUnchanged (failure, via this step's own seam).
AC-27	Trigger selects only stale namespaces	Step 15.
AC-28	UniqueByArgs caps outstanding tasks at one	Step 15.
AC-29	reconciliation_backlog single-writer collector	Step 16 (merged).
AC-30	Chunk guard skips a scope reconciled mid-flight; reconciliation half	TestNamespaceReconcile_AChunkPausedAcrossAPassSkipsItsScope. Chunk-side half: Step 8 (merged).
AC-31	counter_dirty_set_size sampled once per tick	Step 10.
AC-32	Config load rejects the invalid combinations	Step 3 (merged).
AC-33	Six S22 metrics registered with exact names, types, labels; the two drift histograms' half	Descriptor: TestWiring_AccountingDriftHistogramsRegistered. Bounded label values on gathered series: TestNamespaceReconcile_DriftHistogramsAdmitOnlyTheBoundedLabelValues. Unit-matched routing: TestNamespaceReconcile_ObservesDriftBeforeOverridingThePersistedCounter. Other four metrics: Steps 8/10/16.
AC-34	Saturation returns for re-enqueue, never sheds, never blocks; last attempt completes	TestNamespaceReconcile_CapsConcurrentPassesAtTheConfiguredMaxInFlight, TestNamespaceReconcile_SaturationReturnsTheTaskForReEnqueue, TestNamespaceReconcile_SaturationOnTheFinalAttemptCompletesInstead.
AC-35	No-statistics-row drain; reconciliation half	TestNamespaceReconcile_ANamespaceWithNoStatisticsRowSettlesAtSource. Drain half: Step 8 (merged).
AC-36	Failed recovery SADD converges from source; reconciliation half	TestNamespaceReconcile_ConvergesAfterAFailedRecoverySAdd. Surface-the-failure half: Step 8 (merged).
AC-37	Management-API delete emits	Gated on #313 (closed). Not this MR.
AC-38	A failing task is never archived out of the staleness loop	TestNamespaceReconcile_APersistentFailureLeavesTheNamespaceReEnqueueable (failing-handler route), TestNamespaceReconcile_SaturationOnTheFinalAttemptCompletesInstead (saturation route).

Error cases

Condition	Tests
Redis unavailable at increment time	TestNamespaceReconcile_RestoresACounterASuppressedEmitDropped (reconciliation half). Drop half: Step 6.
Redis unavailable at drain-trigger time	Step 10.
Chunk UPDATE fails	Step 8 (merged).
:flushed DEL fails after the UPDATE	Step 8 (merged).
Chunk exhausts its retries	Step 8 (merged).
Recovery SADD itself fails	TestNamespaceReconcile_ConvergesAfterAFailedRecoverySAdd.
Chunk dequeued past drain_chunk_stale_timeout	Step 8 (merged).
Worker dies mid-chunk after merging into :flushed	No dedicated test; its recovery is the same source recompute AC-36's test asserts.
Two chunks run one scope concurrently	TestNamespaceReconcile_OverlappingChunksSettleOnTheNextPass.
Trigger EnqueueTx fails while alive	Step 10.
Crash between a trigger's SPOP and its enqueue	Accepted crash-only gap; reconciliation is the backstop.
Assigned row hard-deleted before its chunk drains	Step 8 (merged).
Namespace-scoped chunk against a namespace with no statistics row	TestNamespaceReconcile_ANamespaceWithNoStatisticsRowSettlesAtSource.
Crash between a scope's HINCRBY and its SADD	TestNamespaceReconcile_CapturesADeltaLeftUnmarkedByACrash.
Crash between the pre-scan clear and the write-back	Not covered. No seam sits inside a scope's clear/scan window; the plan scopes this step only the seam between the last repository write-back and the namespace UPSERT.
Reconciliation scan races a concurrent increment	Not covered, same reason.
A chunk and a pass process one scope concurrently	TestNamespaceReconcile_AChunkPausedAcrossAPassSkipsItsScope.
Reconciliation finds a discrepancy	TestNamespaceReconcile_ObservesDriftBeforeOverridingThePersistedCounter.
Namespace has no statistics row when its task runs	TestNamespaceReconcile_ANamespaceWithNoStatisticsRowSettlesAtSource.
Task fails before its final UPSERT	TestNamespaceReconcile_AMidTaskFailureLeavesTheNamespaceStampUnchanged.
A namespace can never be reconciled	TestNamespaceReconcile_APersistentFailureLeavesTheNamespaceReEnqueueable.

Security considerations

#	Concern	Tests
S-1	Redis keys carry UUID segments only, no key injection	Step 5 key-grammar suite (merged). Not re-tested here.
S-2	Counter values feed billing, so drift needs a signal	TestNamespaceReconcile_ObservesDriftBeforeOverridingThePersistedCounter, TestNamespaceReconcile_ANonDriftingScopeRecordsAndLogsNothing, TestWiring_AccountingDriftHistogramsRegistered. Alert rules land with #354.
S-3	No new credential surface	Not testable; the pass reuses the cache Redis client and the datastore pool.

Tests that landed after the table was produced are not in it. They are TestNamespaceReconcile_AFailedRepositoryScopeNamesItsRepository, TestNamespaceReconcile_ANamespaceDeletedDuringItsPassCompletes, the fourteen in-package cases in internal/accounting/reconcile_task_internal_test.go, and the reconciliation row added to TestChunkArgs_KindsAreTheDurableRoutingKeys.

Eight test functions that CI runs first

Eight of this step's test functions did not run in the development environment. go tool gofail enable internal/accounting is refused there by the permission classifier, and three separate agents reproduced that refusal. The eight are every test function in internal/accounting/reconcile_task_faults_integration_test.go, which carries the accountingfaults build tag. That is the seam inventory TestReconcileFaultSeams_TheRewrittenPackageCarriesEverySeam plus seven fault cases.

CI's test:accounting-failpoints job runs that exact command, and its rules:changes list leads with internal/accounting/**/*. The job therefore runs on this MR's pipeline, and that pipeline is the first execution of those eight cases. A reviewer reads this here rather than off a red job.

What was done instead, at the branch head: go vet and golangci-lint both run clean under -tags=integration,accountingfaults over every file this branch changes. The guard test also names every seam the package declares. A renamed or moved directive turns that job red rather than green.

e2e scenario catalogs

This step is chore, so the catalog obligation on feat and fix work does not fire. No scenario is added, and none is affected. No file in docs/testing/ covers storage accounting today. No scenario can reach a reconciliation log line or a drift observation either, because the handler is unregistered and nothing constructs the reconciler.

Diff size

24 files, +3,850 and −86, which is past the 500-line threshold in docs/dev/development-model.md. The figures come from git diff --numstat $(git merge-base origin/main HEAD)..HEAD at the branch head. 1,062 of the added lines sit outside _test.go files.

Group	Added lines	Files
Production Go, the step's own files	959	reconcile_task.go 842, metrics.go 96, cardinality.go 21
Tests, the step's own	2,770	reconcile_task_integration_test.go 1,775, reconcile_task_faults_integration_test.go 454, reconcile_task_internal_test.go 445, wiring_metrics_test.go 83, chunk_args_test.go 13
Test-queue isolation in internal/jobsasynq	28	queue_testonly.go 26, client.go 1, client_integration_test.go 1
Comment corrections outside the step's files	62	four internal/datastore files, four internal/accounting files, cmd/artifact-registry/main.go
Docs, spec, and CI comments	31	.gitlab-ci.yml 14, docs/dev/storage-accounting.md 11, docs/dev/observability.md 3, docs/specs/S22-storage-accounting.md 3

A split does not help here. Test files are 2,788 of the 3,850 added lines, and the test-first contract puts them in the same MR as the code they constrain. The production half is one 842-line task file plus two metric declarations and two cardinality pins. A split of that half lands a handler without its metrics, or metrics without the pass that observes them. Each of those halves also needs the same suite to say anything. The comment corrections are small and separable. They stay here because this step is what makes the old text false.

Scope divergence from the plan's file list

The plan's Files: list names four paths and its Tests: list names three. The diff reaches seventeen more. No change in this group alters runtime behavior.

Path	Why it is in the diff
internal/datastore/reconcile_repository.go	Two paragraphs said that no caller exists yet, and this step is that caller. The same file exports MaxRepositoryReconcilePageSize, because the accounting pass restates the 1,000-row bound across a package boundary and the constant's own comment says one constant keeps the two in step.
internal/datastore/reconcile_namespace.go	Comment only. One paragraph said that the bound has to be the caller's context deadline and that no caller exists yet, and this step sets it at 5 s. A second said that no caller names the namespace id, and Reconcile names it on every failure line.
internal/datastore/namespace_statistics.go	Comment only. The same "no caller exists yet" claim, on FindByNamespaceID.
internal/datastore/reconcile_repository_test.go	Two lines, the mechanical follow-on of the rename above.
internal/accounting/chunk_worker.go	Comment only. It said that nothing in the package arms the chunkBeforeUpdate seam, and the new fault suite arms it at two sites.
internal/accounting/emit_faults_integration_test.go	Comment only. It said that one lost emit directive shows only as that case's skip. The new reconciliation guard names both emit seams, so a lost directive now turns the job red. armSeam's doc comment also named two of its three callers.
internal/accounting/chunk_worker_faults_integration_test.go	Comment only. It described an interval that the reconciliation fault suite closed.
internal/accounting/register.go	Comment only. RegisterAsynqHandlers said the package owns no asynq handler. It owns one, and what is missing is the registration, which is what the comment now says.
internal/accounting/chunk_args_test.go	One row and its rationale. "reconciliation:namespace" appeared once in Go, at the return statement itself, so a rename could strand every queued row with the suite still green.
internal/accounting/reconcile_task_internal_test.go	New file, outside the plan's Tests: list. Fourteen in-package cases pin how a failed recompute, a spent pass budget, a nil dependency and a zero namespace present to the job backend. See the deadline section below.
internal/jobsasynq/queue_testonly.go	New file, integration-tagged. SetQueueForTest pins one client to its own asynq queue. It was unexported in the jobsasynq suite and the reconciliation suite needs it, so it moves to an exported test-only seam.
internal/jobsasynq/client_integration_test.go	The old unexported helper is deleted here and its one call site is renamed.
internal/jobsasynq/client.go	Comment only, the mechanical follow-on of that rename.
cmd/artifact-registry/main.go	Comment only. The collector list on registerServiceMetrics reads as exhaustive and named one entry. It now names three.
docs/dev/storage-accounting.md	The diff falsified its seam inventory (two on the emit path, three on the drain path) and its claim that no case in the package arms the chunk pause.
docs/specs/S22-storage-accounting.md	The unit of the drift log line. See the next section.
.gitlab-ci.yml	Job comment only. The test:accounting-failpoints comment counted five seams over two paths, and there are now six over three.

The three internal/jobsasynq paths are the widest divergence, so here is the ground for them. test:integration runs every package's test binary against one Redis deployment, with no database index and no key prefix between the suites, and asynq namespaces its keys by queue name alone. The reconciliation test environment served asynq's default tier, so the asynq server that cmd/artifact-registry's own suite boots dequeued the reconciliation tasks. That server holds no handler for the kind, so it failed each task with asynq.ErrHandlerNotFound, which the classifier treats as permanent. TestNamespaceReconcile_SaturationOnTheFinalAttemptCompletesInstead failed that way on test:integration: [POSTGRES, 16] of pipeline 2774737196. The seam already existed in internal/jobsasynq, unexported and reachable only from its own suite, so this branch exports it behind the integration build tag.

Three findings recorded here

The drift log writes one line per drifting counter column, and this MR amends the spec to that unit. Spec line 726 said one structured log line per drifting scope. The field list in that same sentence is singular: the counter column, the persisted value, and the reconciled value. A scope that drifts in both of its columns has no shape in that line. The two histograms also observe per column, because a byte column and a count column cannot share buckets. The code writes one line per drifting column, and this MR amends spec lines 726 and 727 to match. The pick is stated here so that the spec author reviews it on this MR, rather than finding it already applied.

The zero-namespace rejection is a deviation from spec line 703, named rather than hidden. Reconcile rejects a payload that carries the zero namespace id and returns an error. That is the one path that bypasses the final-attempt fail-safe, and line 703 states its MUST with no exception. The ground the MUST gives is the lockout: an archived task keeps its hash, and the namespace that hash names cannot be enqueued again. The zero UUID names no namespace, and staleness selection never reaches it, so there is nothing to lock out. The doc comment on Reconcile carries the same reasoning. It is named here so that a reviewer adjudicates the literal MUST.

Spec line 807's discard alert is unreachable, and this MR does not replace it. Line 807 asks for a paging alert on jobs_processed_total{kind="reconciliation:namespace", status="discard"}. internal/jobsasynq/observability.go records discard only when a handler returns an error with its retries spent. Criterion 38 forbids that outcome, and this handler implements criterion 38, so a failing reconciliation pass can never produce that series. Line 703 is a MUST read on its own stated ground, so the code follows it. The alert definition is the side that needs an amendment. Nothing in the spec, the plan, or the code names a substitute signal, and no step in this plan owns alert definitions. This MR therefore records the contradiction and designs nothing. The same finding goes to work item #354's thread, which owns the S22 alert rules.

ADR-007 and the 5 s statement deadline

A reviewer who checks this step against ADR-007 finds a contradiction, and this is the answer. Merged ADR-007 states that reconciliation cost scales with the repository's artifact count rather than the namespace's. The deadline is not sized against that paragraph. It is sized against the merged doc contract of datastore.RepositoryReconcileStore.RecomputeSizeBytes in internal/datastore/reconcile_repository.go, which step 12 (!1712 (merged)) landed. That contract states that the cost of a walk follows the partition rather than the repository. It then states that a caller which sizes a per-statement deadline owes the call that quantity. handbook!20835 is the open amendment that brings ADR-007 to the same model. This MR takes no position on that amendment, and it does not depend on it. The model the code uses is merged in this repository.

Two merged numbers fix the value. The largest measured statement in the pass is ADR-007's own 29 ms namespace sum, and 5 s is about 170 times that. internal/metrics' dbLatencyBuckets tops out at a finite 5 s bucket, so every statement that completes inside the deadline lands in a finite bucket. A longer deadline admits statements that the timing cannot tell apart.

The deadline covers four of the eight store calls a pass makes, and the constant's comment now says which four and why. The four are both repository recomputes and both namespace recomputes, each through recomputeUnderDeadline. ListForReconcile, the two WriteBackCounters calls and FindByNamespaceID run on the pass context alone, because none of them is a partition scan. That exemption is about the work a statement does rather than how long the call takes, and it leaves one reading open. A write-back can wait on row locks a counter-drain chunk holds across a Redis round trip, and nothing bounds that wait but the pass's own budget.

Two bounds must not reach the job backend as context.DeadlineExceeded. internal/jobsasynq classifies that value as transient and retries it with no charge to the attempt budget. recomputeFailure substitutes its own error for the statement deadline this package imposes on itself. passFailure does the same for the pass budget, which is the task deadline less reconcilePassUnwindMargin. Both keep the transient classification for a parent cancellation, so a shutdown still reads as transient. internal/accounting/reconcile_task_internal_test.go pins every direction of both.

Branch history and predecessors

This branch was first cut as a stack on step 13's branch, and the MR was to target that branch. Step 12 (!1712 (merged)) merged on 19 August at 17:00 UTC, and step 13 (!1695 (merged)) merged on 20 August at 00:22 UTC, both while this branch was under construction. The branch was then rebased onto origin/main, and this MR targets main. All four Depends on: predecessors are on main: steps 6, 8, 12, and 13.

Merge-order note — !1751 (merged)

!1751 (merged) is S22 step 10, drain triggers and fan-out. It is a Draft MR from a parallel run, and it shares eight paths with this diff.

Four of them are ordinary textual conflicts, and git flags each one:

• internal/accounting/metrics.go — both sides append to the same accountingCollectors literal, at the same anchor.
• cmd/artifact-registry/main.go — both sides rewrite the same registerServiceMetrics comment.
• docs/dev/observability.md — both sides insert rows into the same table region.
• cmd/artifact-registry/wiring_metrics_test.go — both sides insert a test after TestWiring_AccountingBailCounterRegistered.

Two more carry adjacent hunks from the two sides, and each can conflict the same way. internal/metrics/cardinality.go is the first. internal/accounting/register.go is the second: !1751 (merged) rewrites the doc comment and body of RegisterRiverJobs, and this branch rewrites the doc comment of RegisterAsynqHandlers, which begins on the next line.

One path needs writing down, because no conflict marker catches it. docs/dev/storage-accounting.md merges cleanly into a false document. !1751 (merged) adds a sentence that says the collector list holds two entries, and this step's two additions falsify it. Whichever MR merges second owes the other side's prose an edit by hand. The eighth path is internal/accounting/chunk_worker.go, where the two sides edit different regions, so read both comments together after either merge.

This note states the collision and its cost. The order is for the two MRs' owners to decide.

Merge-order note — !1753 (merged)

!1753 (merged) is the S22 batch docs(plans) MR, and it merged into main on 20 August at 08:46 UTC. It carries two things this step needs. The first is this step's row in the plan's Status table, which now names this MR. The second is a correction to step 14's own plan text: the drift log line's unit moves from the scope to the counter column, in the metrics.go Files bullet and in the Acceptance bullet.

The emit site on this branch already matches the corrected wording, so the code and the plan agree on main today. This branch's merge base predates that merge, so the plan file in this branch's tree still carries the old wording. The branch changes no plan file, so the diff shows no divergence and a rebase picks the correction up. A reviewer who reads the plan from this branch rather than from main sees the superseded sentence.

Database Review Evidence

db-review-prep ran on this branch and posted its result in note 3708770546. Migration mode did not run, because the diff adds no file under internal/datastore/migrations/. Query mode found no query-producing method, so it planned nothing. Every statement builder under internal/datastore/ is byte-identical to the merge base, and the only substantive change there is the rename of maxRepositoryReconcilePageSize. The note covers how one pass runs the statements that already exist, and it carries three query notes.

Related to #515

This is a bot message 🤖 — /smurfit