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:
- A maven upload emits.
MarkDirtypublishes its namespace id ontoar:counter:dirty:namespace_scoped. - A tick in
internal/accounting's drain-trigger testsSPopNs that key and receives the maven scope, which it cannot tell from its own. - Its chunk merges the maven scope's live hash, finds no
namespace_statisticsrow in its database, and takesresolve's no-row path: delta dropped, staged key deleted. - The maven suite's own drain merges an empty hash. Nothing to apply, no error,
columns still 0 — which is why both
require.NoErrorcalls indrainpass.
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/ociandcmd/artifact-registryall pass under-tags=integrationagainst a PostgreSQL matching the CI service'smax_locks_per_transaction=1024and a Redis shared across the six, which is the arrangement that produces the failure.internal/accountingpasses under-tags=integration,accountingfaultswithgofail 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 runwith--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)