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:

  1. The citation-basis register, which gains the ef3e55828 entry and drops "route" from the e590735c4 one.
  2. Research Findings, the paragraph that named the two routes.
  3. The dependency graph, which loses its Q1 node and its edge into Step 4.
  4. Step 4's own sub-decision bullet, which names the files the ruling shapes rather than counting them.
  5. Step 4's Files: list: script.go for the two call sites that read the prefix helper, plus what the rejection owes the emit path; keys.go for the Fields() doc-comment contract, the helper beside it, and the sentinel the helper returns, declared beside ErrInvalidID; and script_internal_test.go and script_integration_test.go, which are where the new cases can actually land. The prefix helper is unexported and keys_test.go is package counterbuf_test, so it cannot reach it.
  6. Step 4's Tests: list, which gains the prefix case, both rejection cases, and the two integration cases, each naming its file.
  7. Dependencies, the fixed-width passage.
  8. 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.
  9. The Status table's Blocked column 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 and via Step 4 is 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.

  1. 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.
  2. 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.
  3. 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

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading