feat(accounting): buffer and drain the download-count family part 2/3 (repository-column-writers plan: 4/5)
Part 2 of 3 of the repository download-count family. Adds the Redis half, the emitter method and the chunk worker, on top of part 1's statements.
What this adds
counterbuf.DownloadsScoped, its dirty-set key, thedownloadsfield andDownloadsScope. The family carries one counter where the other two carry a pair, soFamily.Fieldspads the trailing slot andcarriedFieldsreads the leading run, refusing a hole rather than truncating at it: a truncatedMergewould lose every delta past the hole silently.Buffer.Incrementrefuses a non-zero delta opposite a padding slot rather than filtering it.DownloadsScopereusesRepoScope's member soParseRepoMemberreads it back. Only the family differs, and the family is in every key, so the two cannot collide.Emitter.EmitRepoDownloads, taking a named scalar rather than an array so no caller can put a value in the padding slot.DownloadsChunkWorker, which merges each claimed scope and commits the batch through part 1's apply.chunkPlan.drainnow reads the matched-row count back and logs at Warn when a committed batch moved fewer rows than it carried. The comment that authorised discarding it rested on reconciliation drift, and neither drift histogram can produce a reading for this family, so that batch previously had no signal at all: the emits bookrecorded, the dirty set drains, no chunk bails, and a column flat at zero is indistinguishable from a repository nobody downloads from.- How far the Warn reaches is per family, read off
chunkPlan.familyrather than carried as a plan field, so no plan can set the floor the wrong way. Every family reports a batch that moved no row; onlydownloads_scopedreports one that moved some, because a tombstone landing betweenDownloadsBaselinesandApplyDownloadsDeltasleaves a skipped scope's delta merged and its:flushedkey deleted in the same transaction, and no pass recomputesdownloads_count. The rows an apply moved travel beside the batch width undercounter_applied_rows. Neither line is sampled:internal/logging's sampler acts only on records carrying theaccessmessage, so the wider reach is bounded to the one family that cannot recover the loss.
What this part does not reach, and the merge order that closes it
This part registers nothing. No worker and no schedule is bound to a
kind, and no producer calls EmitRepoDownloads, so on this part alone
nothing marks a scope dirty and nothing claims one: the family is inert
rather than half-wired. Part 3 adds the trigger, the registration in
accountingRiverKinds and npm's producer in one change, which is what keeps
a producer from ever running ahead of the drain that serves it.
Merging part 3 before this one does not build. Merging this one alone is safe and does nothing.
Stack
Merge in this order. Each part builds and tests on its own, and the family is inert until part 3. Each part is hard-gated behind its parent, so none can merge early.
| # | MR | What it adds | Reviewable LOC |
|---|---|---|---|
| 1 | !2494 (merged) | The SQL half: the delta type, the two statements, the single-row writer | 1683 |
| 2 | !2495 (merged) | The Redis half, Emitter.EmitRepoDownloads, the chunk worker |
1695 |
| 3 | !2496 (merged) | The drain trigger, the registration, npm's producer, every doc surface | 1338 |
Diff size
Guardrail 18 asks for a split or a justification past 500 reviewable LOC. This step was split three ways and each part is still over it, so the justification stands per part rather than being cleared by the split.
By file group at c24ca324: Go production 10 files +289/-518 = 807, Go tests
11 files +759/-129 = 888. Production is net negative on lines: comment blocks
in internal/accounting were compressed to the caps
scripts/ci/check-comment-caps.sh enforces, which is what editing any line
of them costs, and the reasoning that was not already duplicated moved to
docs/dev/storage-accounting.md and docs/dev/background-jobs.md in part 3
rather than being dropped.
Testing
internal/accounting and internal/accounting/counterbuf unit and
integration suites, including the download family's drain case and its
tombstone case. Both mutations were run and confirmed to redden: booking
staged[1] instead of staged[0] applies +0, and reading RepoBaselines
instead of DownloadsBaselines drops the soft-delete filter.
TestChunkPlan_UnmovedRowsWarning_ReportsAPartialApplyOnlyWhereItIsALoss
covers every arm of the per-family floor, and each arm was confirmed to
redden when dropped. Its cases name a family either side of the floor, so
inverting the family comparison to != and deleting the condition outright
each redden it; both were run. One gap it does not close: the end-to-end
partial apply is reachable only through the chunkBeforeUpdate gofail seam,
so no case drives it. That case is owed to the post-part-3 test(accounting)
sweep rather than dropped.
TestDownloadsChunkFamily_PopulatesEveryAccessor is what holds the four
accessors that fail rather than nil-panic, because no case reaches them.
TestFamilies_AccountsForEveryFamilyConstant and
TestSharedFamilyLists_AccountForEveryFamilyConstant hold families(),
scopeCases() and chunkFamilies() against a catalog of every
counterbuf.Family constant plus an exclusion map carrying each excluded
family's reason, so a later family cannot be skipped from a shared list in
silence. Three failure modes were run in both packages and each reddened: a
family in neither the list nor the exclusions, a family in both, and an
exclusion recorded with an empty reason. The catalogs are hand-written, as
TestDropLogFamilies_ListsEveryFamilyConstant's is, so a constant added to
counterbuf and left out of a catalog is accounted by nothing; that step
stays manual.
No format protocol behaviour changes, so guardrail 12 does not apply, and no e2e scenario is added or affected: the family has no producer until part 3.
Related to #1223 (closed)