chore(accounting): counter-drain chunk workers (S22 plan: 8/21)

What this step delivers

This step adds the two River worker kinds that commit one claimed batch of buffered counter movements, one kind per counter family.

Each chunk runs the stale-claim admission check first, against the fixed tick time in its own payload. A chunk inside drain_chunk_stale_timeout reads a baseline for every claimed scope and merges each scope's delta out of its live hash. It then applies one guarded batched UPDATE, deletes the staged :flushed keys, and commits. A chunk past the timeout does no merge and no UPDATE. It counts the bail, re-adds its scope IDs to the family's dirty set, and returns success only when that re-add succeeds. When the re-add fails, the chunk returns that failure, so River retries the claim. A drain that fails its last attempt re-adds its scope IDs before it returns the terminal error.

New surface:

• internal/accounting/chunk_args.go: RepoChunkArgs, NamespaceChunkArgs, RepoChunkScope, and the two durable kinds counter_drain_chunk_repo and counter_drain_chunk_namespace.
• internal/accounting/chunk_worker.go: RepoChunkWorker, NamespaceChunkWorker, ChunkWorkerDeps, and the consumer-side CounterDrainStore interface.
• internal/accounting/metrics.go: counter_drain_chunk_bailed_total, with its counter_family label budgeted and pinned in internal/metrics/cardinality.go.
• internal/accounting/counterbuf/keys.go: ParseRepoMember, the inverse of the member RepoScope joins.

A repository scope encodes on the wire as its dirty-set member, <namespace_uuid>:<repository_uuid>, rather than as an object with one field per identifier. The object form costs 110 bytes per scope, against the 76 bytes internal/config/storageaccounting.go models. A full chunk of 850 scopes, the largest drain_chunk_size that layer admits, measured 94,410 bytes against a 65,536-byte cap. The member form measures 64,660 bytes at that same size, and MarshalText and UnmarshalText delegate to counterbuf, so the member grammar stays stated once. Both families' sizes are pinned exactly, against the argument types rather than against a model of them.

docs/dev/observability.md gains the catalog row for the bail counter. This MR also corrects docs/dev/storage-accounting.md and two .gitlab-ci.yml comments, each of which this change makes false.

Runtime behavior

Neither worker kind is registered with River. Step 9 owns the registration, so no running service can dispatch either kind and no chunk body executes.

The runtime pass demonstrated this rather than asserted it. The booted service logs its own registered set, mgmtapi:bulk-delete-maven and mgmtapi:bulk-delete-npm beside the remote:health-sweep periodic, and neither chunk kind is in it. That pass then inserted two rows, in the new encoded shape, into the live river_job table. River claimed both within a second and discarded both unexecuted, with job kind is not registered in the client's Workers bundle.

counter_drain_chunk_bailed_total reaches the scraped registry at boot and emits no series. An unobserved CounterVec contributes nothing to a gather, and the only increment is a bail. GET /-/metrics served 1254 gitlab_artifact_registry_ series before and after that probe, and this counter was absent from both. The endpoint cannot tell "registered and unobserved" from "never registered", so TestWiring_AccountingBailCounterRegistered is what separates them.

driver.sh smoke passed 22 of 22 from zero state, with no error line and no composition: line in the boot log.

Spec coverage

Spec: docs/specs/S22-storage-accounting.md Plan: docs/plans/2026-08-04-s22-storage-accounting.md, Step 8

The rows below were written with the tests, then updated for the assertions later review passes added. Every test named exists on this branch.

Acceptance criteria

#	Criterion	Tests
AC-4	Re-marked scope, sequential-reclaim clause only (re-claiming chunk runs after the first chunk finishes)	TestChunkWorker_SequentialReclaimCountsEachDeltaOnce
AC-5	Dequeue lag past drain_chunk_stale_timeout re-adds, counts the bail, issues no merge and no UPDATE, returns success after the re-add lands, and returns the failed SADD as an error when it does not; a chunk inside the timeout drains	TestChunkWorker_BailsOnAChunkPastTheStaleTimeout, TestChunkWorker_DrainsAChunkDispatchedInsideTheStaleTimeout, TestChunkWorker_RecoverySAddFailureSurfacesOnTheBailPath (the error return, and the pin that the bail counter moves even when the re-add fails)
AC-6	A chunk failing every attempt re-adds its scope IDs before the terminal error	TestChunkWorker_TerminalAttemptReAddsItsScopesBeforeFailing
AC-33	Metric-registration half for counter_drain_chunk_bailed_total (name, type, label names; bounded counter_family values)	TestWiring_AccountingBailCounterRegistered (cmd/artifact-registry), TestChunkWorker_BailCounterAdmitsOnlyTheTwoFamilies, TestRegisterMetrics_DuplicateRegistrationReturnsAnError, TestRegisterMetrics_RegistersOntoEachRegistryIndependently
AC-35	Guard-skip half: a no-baseline scope drops its delta, still has :flushed deleted, and no row is inserted	TestChunkWorker_NoBaselineScopeDropsItsDeltaAndStillDeletesFlushed, TestChunkWorker_NoBaselineNamespaceIsNeverInserted
AC-36	Failing-recovery half: a recovery SADD that fails surfaces rather than losing the scopes silently	TestChunkWorker_RecoverySAddFailureSurfacesOnTheTerminalPath, TestChunkWorker_RecoverySAddFailureSurfacesOnTheBailPath

Every other acceptance criterion is another step's, per the plan's per-step Acceptance sections. AC-36's converges-after-reconciliation clause and AC-4's overlap clause both need a reconciliation pass and are Step 14's.

Error cases

Condition	Tests
Chunk job's :flushed DEL fails after the UPDATE succeeded	TestChunkWorker_FlushedDeleteFailureRollsBackTheUpdate
Chunk job exhausts all retry attempts	TestChunkWorker_TerminalAttemptReAddsItsScopesBeforeFailing
Recovery SADD itself fails (bail or terminal path)	TestChunkWorker_RecoverySAddFailureSurfacesOnTheBailPath, TestChunkWorker_RecoverySAddFailureSurfacesOnTheTerminalPath
Chunk dequeued later than drain_chunk_stale_timeout	TestChunkWorker_BailsOnAChunkPastTheStaleTimeout
Assigned repository or namespace row hard-deleted before its chunk drains	TestChunkWorker_NoBaselineScopeDropsItsDeltaAndStillDeletesFlushed (repo scoped)
Namespace-scoped chunk drains a namespace with no namespace_statistics row	TestChunkWorker_NoBaselineNamespaceIsNeverInserted, TestChunkWorker_NoBaselineScopeDropsItsDeltaAndStillDeletesFlushed (namespace scoped)
A full chunk at the largest admitted drain_chunk_size reaches the job payload cap	TestChunkArgs_AFullChunkFitsOnePayload (both families, cap and exact size), TestRepoChunkScope_EncodesAsItsDirtySetMember
A payload carries a member no repository chunk scope could have written	TestRepoChunkScope_RejectsAMemberItDidNotWrite, TestParseRepoMember_RejectsAMemberRepoScopeCouldNotHaveBuilt, TestParseRepoMember_RoundTripsRepoScope
Chunk job's Postgres UPDATE fails (deadlock, transient error)	River-owned retry policy. Not tested in this MR; the store's own failure modes are internal/datastore/counter_drain_integration_test.go's.
Two chunks run the same scope concurrently	Step 14. Needs the baseline-read-to-UPDATE seam this step lands plus a reconciliation pass.
A drain chunk job and a reconciliation process the same scope concurrently	Step 14, same seam.
Worker dies mid-chunk after merging into :flushed	Not reachable from a handler-level test: it is a process death, and its recovery is reconciliation's. Step 14.
Every remaining Error Cases row	Emit path (Step 6), drain trigger (Step 10), or reconciliation (Steps 14-15).

Security considerations

#	Concern	Tests
S-1	Redis keys carry only internal UUIDs, so no key injection	internal/accounting/counterbuf/keys_test.go — Step 5's cases, plus this MR's TestParseRepoMember_RoundTripsRepoScope and TestParseRepoMember_RejectsAMemberRepoScopeCouldNotHaveBuilt. This step builds no key of its own. It reads the member grammar back through counterbuf.ParseRepoMember, which rejects a member outside that grammar with counterbuf.ErrInvalidID, and TestRepoChunkScope_RejectsAMemberItDidNotWrite pins that the rejection lands in the argument decode, before a drain has merged anything.
S-2	Counter values feed billing, so drift has financial impact	The discrepancy histograms and the reconciliation schedule, Steps 14-16. Not reachable here.
S-3	No new credential surface	Structural: ChunkWorkerDeps takes the existing cache-purpose Redis client and the datastore pool, and adds no credential of its own.

e2e scenario catalogs

No docs/testing/ scenario is added, and none is affected. The runtime pass checked this on the merits rather than on the step type.

docs/testing/ names S22 accounting in exactly two places, docs/testing/e2e/maven.md:133 and docs/testing/e2e/npm.md:127. Both are forward references that say freed space is not asserted in that scenario, and this MR touches neither. No scenario asserts a counter value, a drain, or a job kind. Nothing this MR adds is reachable end to end, because no kind is registered.

Two of the changes folded into the squashed chore(accounting) commit are behavior changes rather than new code: the scope's wire encoding, and the bail counter's Help string. Neither is reachable from an e2e scenario, for the same reason.

Reviewable size

Production code is 968 insertions and 23 deletions across eight files, excluding tests, docs/, and .gitlab-ci.yml. That is past the 500-line threshold in docs/dev/development-model.md, so this MR carries a justification instead of a split.

File	Insertions / deletions
internal/accounting/chunk_worker.go	637 / 0
internal/accounting/chunk_args.go	151 / 0
internal/accounting/metrics.go	53 / 8
internal/accounting/doc.go	44 / 0
internal/accounting/counterbuf/keys.go	29 / 0
internal/metrics/cardinality.go	24 / 0
internal/config/storageaccounting.go	20 / 8
cmd/artifact-registry/main.go	10 / 7

chunk_worker.go alone is 637 of the 968, so a split cuts that one file rather than separates two concerns. The admission check, the baseline read, the per-scope merge, the batched guarded UPDATE, the delete-and-commit envelope, and the recovery are one code path. Each part is reachable only through the part before it. A part shipped on its own is a worker that no test can drive end to end, which moves review cost rather than lowers it. The other seven production files are 331 insertions together, and no one of them passes 151.

The rest of the diff, for scale: tests 1652 / 6 across eight files, docs/ 98 / 32 across four files, and .gitlab-ci.yml 29 / 13.

internal/config/storageaccounting.go changes only comments. No constant, default, bound, or validation moves, so no documented behaviour or constraint changes and docs/dev/configuration-reference.md is unchanged.

Merge order

Every trial below ran read-only, with git merge-tree against the candidate head, rather than off the hunks. The trials were re-run after this branch moved onto a newer origin/main. Each pairwise result is reported beside the same trial against origin/main alone, because a branch that is itself behind main conflicts in the pairwise trial for that reason rather than because of an overlap with this one.

• This branch merges clean onto origin/main, and is rebased onto its current head rather than trailing it. No revision is named here on purpose: main moves several times an hour on this project and this branch is rebased onto it each time, so any revision written down goes stale before a reviewer reads it. The branch's own diff is unchanged by those rebases.
• It merges clean against !1551 (merged), !1620 (merged), and !1554.
• It conflicts with !1535 (merged) and !1542 (merged), both in cmd/artifact-registry/main.go. Each inserts a new RegisterMetrics block into registerServiceMetrics, at the point where this branch rewrites the storage-accounting comment. Whichever of the three merges second resolves a two-line conflict. No order is required between them, because the resolution is the same in every order. Both branches now conflict with origin/main alone in the same file set their pairwise trial reports, so the trial isolates no overlap of this branch's and the main.go statement above rests on the hunks.
• It conflicts with !1598, in docs/dev/observability.md alone. !1598 conflicts with origin/main alone in that same one file, so the pairwise result carries that branch's own distance from main and isolates no overlap of this branch's.
• It conflicts with draft !1645 (merged), and not only in .gitlab-ci.yml. The clearest overlap is there: both rewrite the same test:integration sentence for different causes, this branch crediting the package-wide TestMain in internal/accounting/chunk_worker_integration_test.go and !1645 (merged) crediting the backlog collector's lease and count. The overlap reaches beyond that one file, and the set moves while !1645 (merged) is a draft, so it is given as surfaces rather than as a count: both branches touch internal/accounting, the S22 plan, docs/dev/storage-accounting.md, and the failpoints job's configuration. !1645 (merged) also conflicts with origin/main alone, in internal/datastore/query_names.go, and that one file is its own rather than an overlap with this branch. The edit here is phrased with a since … clause, so a rebase of !1645 (merged) can extend it rather than replace it. Whichever merges second reconciles that one hunk by hand, and the combined text names both causes. The job placement is correct in either order, because test:integration runs PostgreSQL.

This step widens no predicate ahead of the code that serves it. Step 9 registers the two kinds. Until it lands, River discards a payload of either kind with an unregistered-kind error.

Accepted rule departure

The staged-key Redis deletes run inside the counter UPDATE's own transaction, against "Keep transactions short" in docs/dev/database-query-patterns.md.

The departure is structural rather than convenient. The rollback the envelope exists for is reachable only while a failing delete can fail the update it stages. A delete moved outside the transaction removes that guarantee instead of relocating it, and the spec requires this shape at its #### 6. Each chunk job's work section.

It is named in three places now: deleteFlushed's doc comment, the plan's Step 8 Files entry as an Accepted code smell: block, and here.

The spec assigns both this hold time and the lock footprint to drain_chunk_size, so the hold is a specified property of the design rather than an omission, and this MR adds no bound of its own on top of it. What drain_chunk_size does not size is the slow-Redis tail, and that tail has a ceiling: go-redis' library defaults give a dial, a write, and a read 5s each and retry a failed command three times, so one attempt runs to about 15s and the four to about 60s, which is where River's default job timeout cancels the job and the transaction rolls back with it. redis.cache.read_timeout and redis.cache.max_retries are the operator's existing instrument for shortening that tail; both move every user of the cache-purpose client together, which is why this MR adds no constant of its own, no Timeout() override, and no re-sizing. The plan gained a second such block for this MR's other accepted smell, which is that the MR spans internal/accounting/counterbuf, the package Step 5 owns.

Spec and plan amendments in this MR

This MR amends two merged documents, each in a commit of its own.

docs/specs/S22-storage-accounting.md, in the commit docs(specs): qualify every chunk-bail success claim in S22. This amendment is what makes the MR's behavior conformant, and it is not a drive-by. Six places stated the chunk bail's success return without qualification. Read as a return-value contract, that text is false on the failed-re-add path. There the handler returns the failed SADD, so River retries the claim rather than drops it. The commit qualifies all six. It rewrites the step 7 paragraph rather than qualifies it. Its second half also inferred that a bail consumes no attempt toward MaxAttempts, and a returned error does consume one. The same commit reconciles two statements about the acknowledgment-lost rollback, which described one sub-case as both a lost delta and not data loss.

docs/plans/2026-08-04-s22-storage-accounting.md, in the commit docs(plans): record S22 step 8 obligations and its file list. It carries four additions:

• the two accepted departures Step 8's own text did not name,
• the two dependency guards Step 9 must apply to the value it assembles,
• the guard-skip visibility question, carried to Step 10 where the fast path first runs,
• the four files no step's Files or Tests list named,
• the Status table row for this step, filled with this MR's own reference.

It also corrects Step 8's Tests entry, which claimed a shortened retry backoff the suite never uses. Neither RetryPolicy nor NextRetry appears in any Go file here. The fault suite stubs the attempt counter instead, which is the substitute the spec's step 7 admits.

Two Go comments and one plan sentence cited ADR-023 for the rule that an interface is declared at its consumer. A measurement over the whole file found no such rule in ADR-023. All three now cite docs/dev/go-style.md's "Define interfaces where they are used, not where they are implemented". The ADR check for this MR read ADR-007 from the handbook, because the local docs/adr/ mirror is stale and its freshness script exits 1. No ADR sets a job payload cap, and the code this MR adds names no ADR.

Decisions a reviewer does not have to re-derive

Review settled six escalations, and one metric-policy question with them.

Question	Answer
A full repository chunk was 44% over the payload cap the configuration layer asserts against	Encode a scope as its dirty-set member, and pin a full chunk's marshaled size against jobs.MaxPayloadBytes in this package's own test. internal/config is untouched and no published operator bound moves.
A bail whose re-add fails: success, or the error?	The error, which is what the plan's acceptance required and the code already did. The spec is amended at the six sites that read the other way.
Neither worker constructor guards its dependencies	No guard in this step. The plan's Step 9 entry now binds the wiring tier to nil checks on the four collaborators and to a value check that rejects a non-positive DrainChunkStaleTimeout. A nil check cannot see a zero duration, and a zero one sends every chunk down the bail path, where each job reports success and nothing drains.
The delete-then-commit window is indistinguishable from the recoverable rollback	No log line and no metric here. deleteFlushed's doc names both routes and records that the delete-failure wrap does not prove the safe half. The plan records Step 14's drift histograms as the operator-visible signal.
The guarded UPDATE's matched-row count is discarded, so "skips occasionally, as designed" and "matches nothing, ever" look identical	No new signal here. The comment names the reading the discard leaves open, and the plan carries the question to Step 10, where the fast path first runs.
recoverScopes runs on the job's own context, where the emit path detaches from its caller's	It stays on the job context. The posture is documented, including that the returned error is what buys a retry on a live context, and that a detach was considered and declined over the Redis teardown race at shutdown.
A seventh S22 metric for either of those two windows	No. This step adds counter_drain_chunk_bailed_total only, and the spec's six-metric enumeration stands.

One gate the first pipeline closes

The accountingfaults suite did not run locally against a real gofail rewrite. The worktree safety hook refuses go tool gofail enable internal/accounting, in every phrasing two agents tried, and the pre-existing emit seam guard fails the same way on main. An equivalent run went through a rendered scratch copy instead, and all four fault cases passed on both families. test:accounting-failpoints performs the real rewrite, so the first pipeline on this MR is what closes the gate.

Related to #515

This is a bot message 🤖 — /smurfit