docs(plans): rule the repository-column-writers open sub-decision
What
Rules the one open sub-decision in the repository-column-writers plan, so Step 4 can start.
The plan adds a third, repository-keyed counter family that carries
downloads_count and nothing else. counterbuf.Family.Fields() is two slots
wide, and both Buffer.Increment and Buffer.Merge iterate the whole array,
so a one-counter family would send an empty field name to Redis. The plan
named two routes and left the choice open, which blocked Step 4 and, through
the dependency edge, Step 5.
The ruling
Filter the padding slot on the Go side of those two methods. Both Lua script bodies stay byte-identical, so the two shipped families reach Redis exactly as they do today.
One direction inside that route is ruled here too, because the two options
differ by whether a later mistake is loud or silent. A family's carried
fields are the leading slots of the array and the padding is the trailing
remainder, and the helper the two methods read rejects an array that
breaks that shape instead of truncating at the first hole. Truncating is the
quiet option: Buffer.Merge is handed no delta, so the guard that catches a
delta in an uncarried slot cannot cover it, and a short field list would have
the merge script delete the live hash and return a reply the arity check
accepts, so Merge would report zeroes with a nil error and lose every
buffered delta for that family with nothing logged.
The rejections get a sentinel of their own rather than ErrInvalidID, whose
doc comment closes the list of what returns it
(internal/accounting/counterbuf/keys.go:8-10) and restates that closed list
at :130 and in the package doc.
One value covers both rejections, under a closed-list doc comment of the
shape ErrInvalidID's already carries. Step 4 has two rejections: the
non-dense array the prefix helper refuses, and the non-zero delta in an
uncarried slot Increment refuses. A second value would add a distinction
nothing can branch on, because no rejection crosses the package boundary,
which is the same reachability that keeps the value unexported. Increment's
doc comment names both conditions in one sentence; Merge's names only the
array shape, since it is handed no delta.
The merge script's body is unchanged; the doc comment above it is not.
mergeLiveIntoFlushed documents that ARGV[2..] must list the family's full
field set, because the live hash is deleted unconditionally and a field left
out of the call is dropped silently. Step 4 has Merge pass the carried
prefix, so a reader holding that sentence beside a two-slot Fields()
concludes the padding slot has to be passed, which is the empty field name
this ruling exists to prevent. It becomes "the family's carried fields" and
keeps the silent-drop clause, which is the only place the hazard is written
at the script itself.
The new family does not join families().
internal/accounting/counterbuf/script_integration_test.go reaches both
shipped families through that fixture, which drives all eight
range families() loops in the file. Six of the eight put a non-zero delta
in slot 1, from {3, 500} at :233 to {5, 5000} at :639, measured at
1f74762a3, and that is what the uncarried-slot rejection refuses, so a
one-counter family cannot be looped through them. Its doc comment's "both
counter families" is reworded to the two two-counter families, testEnv.scope
(:87-99) gains a third arm because it t.Fatalfs on an unrecognized
family, and the two standalone integration cases are the positive hit per
enumerated value that guardrail 6 asks for.
Why not the two routes the plan named
Pairing downloads_count with a second repository-level counter is
unavailable, not rejected. repositories carries exactly three bigint
counters, and two of them are RepoScoped already
(internal/datastore/migrations/sql/20260519155301_create_repositories.sql:11-13),
so there is no third to pair with. Moving either of those two across is worse
than unavailable: a family has one apply, and applyRepoDeltasStmt carries
AND r.last_reconciled_at = v.baseline
(internal/datastore/counter_drain.go:736), which the two reconciled
counters need and this family must not have, because reconciliation
recomputes no downloads and leaves the column out of its write-back
(internal/datastore/reconcile_repository.go:1299-1302).
A skip-empty leg in both Lua scripts would work, but Go is the better
place for it. Choosing the fields becomes a pure function over Fields(),
which a unit test pins with no Redis running, where the same branch written
into Lua is observable only by running the script. It also keeps a per-field
conditional out of a script that runs on every increment of every family.
Derived at ef3e55828.
Citation bases
The plan is measured at d5ee57ffdf60 except where a later basis is named at
the claim, and it keeps a register at the top of which facts moved to which
later basis. This MR adds a third basis and registers it, rather than
re-pinning the document.
That leaves writeBackRepositoryCountersStmt's doc comment cited at two
numbers inside Research Findings, which is deliberate:
internal/datastore/reconcile_repository.go:1299-1302 in the route paragraph
derived at ef3e55828, and :1370, :1381 and :1401 in the
permanent-loss paragraph derived at the base pin. All three anchors moved
exactly 80 lines between the two commits, so both are right at their own
basis. The register says so, so a reader does not have to re-derive it.
Who ruled it, and why no outside owner is owed
The plan sent this decision outside itself because the skip-empty leg edits the script bodies S22 owns. On the route taken it does not: no Lua changes, and the edit lands in the package Step 4 already opens.
S22 is also no longer a workstream with a DRI to wait on.
docs/specs/S22-storage-accounting.md carries **Status:** Implemented and
is a landing point for inbound links, and the mechanism now lives in
docs/dev/storage-accounting.md, maintained alongside the code. This MR's
own review is the ratification.
The sub-decision keeps its home as a note on #292, which now carries the ruling under the question it recorded, rather than getting a number of its own.
Sites changed
Every passage the fact lives at moves here:
- The citation-basis register, which gains the
ef3e55828entry and drops "route" from thee590735c4one. - Research Findings, the paragraph that named the two routes.
- The dependency graph, which loses its
Q1node and its edge into Step 4. - Step 4's own sub-decision bullet, which names the files the ruling shapes rather than counting them.
- Step 4's
Files:list:script.gofor the two call sites that read the prefix helper, plus what the rejection owes the emit path;keys.gofor theFields()doc-comment contract, the helper beside it, and the sentinel the helper returns, declared besideErrInvalidID; andscript_internal_test.goandscript_integration_test.go, which are where the new cases can actually land. The prefix helper is unexported andkeys_test.goispackage counterbuf_test, so it cannot reach it. - Step 4's
Tests:list, which gains the prefix case, both rejection cases, and the two integration cases, each naming its file. - Dependencies, the fixed-width passage.
- Open Questions, now
None., with this ruling recorded as its own and the reject-versus-truncate argument beside it. The preamble names each ruling instead of numbering them, because the amendment's own is second in the section rather than third. - The Status table's
Blockedcolumn for Steps 4 and 5, and the prose that explains the column. The column's meaning is narrowed rather than exempted: it names a ruling or an outside answer a step waits on, and an empty cell means none is outstanding, which is narrower than nothing blocking the step.Depends on:edges stay in each step's own entry, so Step 5's empty cell illustrates the rule instead of carving out of it. Row 5's cell stays empty andvia Step 4is not restored.
Conventions captured in AGENTS.md
Three review comments on this MR asked for their instruction to be kept as a
project convention, so AGENTS.md gains three sections. Each ships as its own
commit beside the plan fix it came with, so either half can be reverted alone.
- A Plan Adding an Enumerated Value Rules on the Shared Test Fixtures. A
suite covering every value of an enumerated type reaches those values
through shared fixtures, and a plan step adding a value has to rule on
them. The table-driven enumerator quantifies over the whole set in its doc
comment, so joining reads as the default and its fixture values may be
wrong for the new value; the switch helper
t.Fatalfs on an unrecognized value, so it needs its arm before any new case runs. - A Column's Definition Has To Hold on Every Row Under It. A sentence defining what an empty cell means is a claim about every row, and the row a later sentence explains away is the row that falsifies it. Narrow the definition rather than exempt the row.
- An Unchanged-Artifact Claim Says Which Part of It Is Unchanged. "Both Lua script bodies are unchanged" is true of the bodies and silent about the doc comments above them, and the comment is usually the part that has to move, because it states the precondition the change stops honoring.
Merge order and overlap
git diff --name-only main...HEAD is two files: the plan file and
AGENTS.md.
The plan file. No open MR touches it. !2363 (merged), the Status-table batch fill
for Steps 1, 2 and 3, merged as 1f74762a3; every other open MR was diffed
against its own target and none of them intersects this file. The open-MR
total is deliberately not quoted here, because it moves hourly; the scan is
what the claim rests on.
This branch merged main at 1f74762a3, which carried !2363 (merged), and the table
conflict came out keep-both as planned: rows 1 to 3 carry !2363 (merged)'s MR
references, rows 4 and 5 keep this MR's empty Blocked cells, and the legend
below the table keeps this MR's wording.
If the plan file conflicts again, the same resolution holds and the thing not
to do is take a batch-fill branch's rows 4 and 5 wholesale. A branch that
predates this MR still reads Q1 and via Step 4 there, and restoring them
would put a block back into a plan whose Research Findings, dependency
graph, Step 4 bullet, Dependencies, Open Questions and Status legend all say
the sub-decision is ruled. Nothing in CI reports that.
AGENTS.md, which three open MRs also edit. The file is new to this MR's
diff as of the round that added the three conventions above, so the scan was
re-run against it. This MR appends its sections at line 891, immediately
above ## Labels.
| MR | What it does to AGENTS.md |
Order |
|---|---|---|
| !2020 (merged) | Replaces lines 55-910 with 27, routing the prose conventions into docs/dev/writing-discipline.md |
Merge this MR first, or re-home the three sections |
| !2410 (merged) | Adds three sections at line 844 | Either order |
| !2460 (merged) | Rewrites one passage at line 628 | Either order |
!2020 (merged) is the one that needs an order. Its second hunk deletes the whole
prose-convention cluster this MR appends to, so all three new sections sit
inside the range it removes. Merging this MR first leaves !2020 (merged)'s author one
rebase that carries three more sections into
docs/dev/writing-discipline.md. Merging !2020 (merged) first means these three
sections belong in that module file instead, and this MR is the one that gets
rebased. Either order is fine; what is not fine is merging them out of order
without that rebase, because git resolves it clean and the conventions are
silently dropped or left in a file that no longer holds them. No pipeline
reports this.
!2410 (merged) and !2460 (merged) are co-tenancy rather than conflict: both touch other anchors in the same file, 47 and 263 lines from line 891, so a three-way merge handles them.
Related to #292