refactor(accounting): supply the dirty-set key so suites claim their own

What this fixes

Two main failures in the last day, both reading a counter column as 0 after a clean drain that reported no error: internal/format/maven.TestUploadEmit_SecondVersionOverHeldBlob_MovesCountsNotSizes in job 16060349661 and TestUploadEmit_JarAndPomForOneVersion_MoveCountsByOne in job 16058528113. This branch's own first pipeline met the same failure, on a tree touching nothing near it.

counterbuf's two dirty-set keys are the only keys in the package whose name does not vary: each holds every dirty scope of a family system-wide, which is what S22 specifies and what a deployment wants. A scope's live and staged hashes carry its namespace or repository id, so two suites cannot collide on those.

In CI that fixed name is one key shared by every package of the test:integration job. testutil.StartIsolatedRedisClient isolates the deployment's resolution, not the keyspace — on the env path every caller resolves the job's single redis:6379 — while each suite gets a database of its own. So:

  1. A maven upload emits. MarkDirty publishes its namespace id onto ar:counter:dirty:namespace_scoped.
  2. A tick in internal/accounting's drain-trigger tests SPopNs that key and receives the maven scope, which it cannot tell from its own.
  3. Its chunk merges the maven scope's live hash, finds no namespace_statistics row in its database, and takes resolve's no-row path: delta dropped, staged key deleted.
  4. The maven suite's own drain merges an empty hash. Nothing to apply, no error, columns still 0 — which is why both require.NoError calls in drain pass.

In job 16058528113 the failure lands inside the window of TestDrainTrigger_ScopeDrainedAtTheNormalCadenceKeepsItsHash/namespace_scoped, which ticks every 2s. The same collision crowds that trigger test's own claim out of a batch capped at five, which is issue #818 (closed).

What changed

DirtySets carries an optional segment. Its zero value names DirtySetKeyRepoScoped and DirtySetKeyNamespaceScoped, so production declares nothing and gets today's keys; IsolatedDirtySets builds a pair for one caller, validating the segment the way an identifier is validated so it can add no key segment and form no hash tag.

Family.DirtySetKey and Scope.DirtySetKey are removed rather than left beside it. They took no argument, so any site could build the deployment's key by accident and the mistake would be silent; without them the compiler walks every site and each one names the pair it means.

Three production sites take the pair — Buffer.MarkDirty through WithDirtySets, recoverScopes' re-SADD through Buffer.DirtySets(), and the tick's SCARD/SPOP through DrainTriggerDeps.DirtySets. LiveKey, FlushedKey, Merge, Clear and the staged-key DEL are untouched: their names already carry a per-suite id, so they were never the crossing point.

Each affected suite takes one segment per binary, not per test. The collision is between packages running concurrently, and the cases inside a binary are sequential and clean up their own members, so a per-binary segment reproduces a deployment's conditions. It also has to exist before the first case runs, which a package-level value gets and testutil.UniqueKey — which needs a *testing.T — cannot.

The maven wiring case in cmd/artifact-registry keeps the deployment's pair on purpose: the buffer behind its emitter comes from wireAccounting, which is what that case exercises, so nothing there can hand it an isolated pair. That stays safe because after this change the only writers of the deployment's pair are cases in that one binary, which run sequentially.

Production impact

None. The keys are byte-identical, no configuration knob is added, and config.example.yaml and the configuration reference are untouched. TestDirtySets_ZeroValueNamesTheDeploymentsPair pins the zero value against both constants, for both families and for a scope of each, since that zero value is the whole of what production relies on.

The trade to name: an exported production type gains a field no production caller sets. It exists so these suites can follow the key-namespacing rule in docs/dev/redis-testing.md, which counterbuf's fixed grammar currently makes unfollowable — the same package already follows it for its lease and queue keys.

Verification

  • internal/accounting, internal/accounting/counterbuf, internal/format/maven, internal/format/npm, internal/format/oci and cmd/artifact-registry all pass under -tags=integration against a PostgreSQL matching the CI service's max_locks_per_transaction=1024 and a Redis shared across the six, which is the arrangement that produces the failure.
  • internal/accounting passes under -tags=integration,accountingfaults with gofail enable.
  • After that run, the Redis deployment held only segmented keys — ar:counter:dirty:repo_scoped:maven-emit-<uuid>, ar:counter:dirty:namespace_scoped:maven-emit-<uuid>, ar:counter:dirty:namespace_scoped:npm-wire-<uuid> — and no unsegmented one, which is the isolation this change buys.
  • Untagged unit tests pass for every touched package.
  • golangci-lint run with --build-tags=integration, with --build-tags=integration,accountingfaults, and untagged, each with --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false, reports nothing on any changed file.

Size

458 insertions across 19 files, past the 500-LOC reviewable guidance only if comments are counted. Splitting would not help: the seam is 4 production files (~120 lines with their doc comments), and the rest is one-line call-site wiring in 11 test files plus 4 test cases for the new type and 2 doc paragraphs. A bottom MR carrying the seam alone would fix neither issue, and a stack would put the mechanical half behind review of the half that cannot be exercised without it.

  • Seam and production wiring: counterbuf/keys.go, counterbuf/script.go, chunk_worker.go, drain_trigger.go
  • Test wiring: internal/accounting (6 files), counterbuf (2), internal/format/{maven,npm,oci} (3), cmd/artifact-registry (2)
  • Docs and helper comments: docs/dev/redis-testing.md, docs/dev/storage-accounting.md, internal/testutil/testredis.go

e2e scenarios

No scenario in docs/testing/ is affected: the change alters no request path, no counter value and no production key.

Related issue: #817 (closed) Related issue: #818 (closed)

Merge request reports

Loading
Loading