docs(plans): add the repository column writers plan

What this is

The plan MR for issue #1046. It adds one file, docs/plans/2026-09-03-repository-column-writers.md, and changes no code. Merging it is the approval signal for the five step MRs it describes; no step MR opens before it lands.

What the review round changed

The review left 21 blocking: and 2 escalate: findings. Both rulings are in and every finding is answered in its own thread. The two that change what a reviewer should look at first:

  • Coalescing is taken. The plan had ruled S22's buffered pipeline out because counterbuf.Family.Fields() returns [2]string, so routing downloads_count through it "means widening that array and every path that reads it". That is the cost of a third field on RepoScoped, not the cost of the pipeline. Family is an open string type, so a third family keyed on the repository leaves the array and its readers untouched. Steps 4 and 5 now book buffered increments, and Postgres sees one write-back per drain tick per dirty repository whatever the read rate.
  • One open question, and it blocks Step 4. The third family carries one counter while Fields() is two slots wide, and Buffer.Increment and Buffer.Merge both iterate the whole array, so a family holding downloads_count alone would send an empty field name to Redis with deltas[1]. It needs either a second repository-level counter to pair with or a skip-empty leg in both scripts, and the second edits the shipped script path S22 owns. ## Open Questions carries it.

Every governing ADR was checked, the one the local mirror does not carry included. ./scripts/adr-freshness.sh exits 0, so the mirror is current, and the one internal ADR that script reports but never mirrors locally was read at its source before merge. It governs a policy area no step in this plan touches, so there is no deviation and no handbook amendment is owed.

The defect it plans against

repositories.last_updated_at and repositories.downloads_count each have a production writer on one format.

  • last_updated_at moves on an npm publish and on nothing else. Maven builds the UPDATE and discards it (MavenRepositoryStore.MarkRepoLastUpdated assigns its statement to _), and no container path has a writer at all.
  • downloads_count moves on a Maven archive-class download and on nothing else. npm and container repositories read zero forever.

Both columns ship and both are read: the management API maps each as a sort key and binds each into the keyset cursor. So a Maven repository published a minute ago sorts by its creation date, and a download sort over a mixed namespace orders only its Maven rows.

The three decisions a reviewer should check first

All three are recorded in the plan with their reasoning rather than left implicit.

1. The Maven timestamp write goes post-commit, not inside the upload transaction. The in-transaction site the tree invites is a row-lock upgrade. upsertFileRow already holds FOR SHARE on the repositories row, through ReVerifyRepositoryAlive, whose statement joins maven_repositories to repositories with no OF clause and so locks both rows. An UPDATE repositories later in that same transaction asks for FOR NO KEY UPDATE on a row the transaction already holds at share strength, and two concurrent uploads into one repository then wait on each other: a deadlock, aborted by Postgres, on an upload whose bytes are already in storage. A parallel multi-module publish is the ordinary shape that produces it. Step 2 therefore dispatches from writeCommitOutcome's txErr == nil arm, where the emission decision S10's numbered commit step describes is already made (201 fresh, 200 overwrite, byte-identical carried out by its own sentinel), and pays a sixth bufferedUpdate label plus the re-split of five per-label quotas that forces. ## Approach also names the alternative of strengthening the re-verify's lock and why that is worse.

2. Step 1 reverses the hosted half of the downloads_count assignment, and keeps the remote half where it is. S10 and S12 assign the column to S18; Step 1 moves the hosted writers to this work and answers the reasoning in those rulings, which is about the column's mechanism. S16's two sentences stay: no step here writes the container remote read path, and npm remote is served by a different handler than the one Step 4 touches, so removing S16's gate would leave its criteria asserting downloads that nothing books. Both remote writers stay with #292, named in ## Dependencies, and so does the mechanism decision for those two paths.

npm hosted is the one half with no ruling to reverse: no npm spec defines a downloads_count event at all, so Step 1 defines one.

3. The plan routes the read-path counter through the buffered pipeline, and names the shed it does not remove. This is the decision the review reversed, so it is the one worth reading closely. Steps 4 and 5 stand up a third repository-keyed counter family, its drain worker and its write-back, and the read paths book increments into it. That removes two bounds the earlier revision carried: the per-row Postgres ceiling, where an N-layer pull issued N+1 UPDATEs contending on one row, and the timeout ordering where a writer's 5s worker deadline was shorter than a container manifest push's 10s lock hold. What it does not remove is the dispatch shed: the emit still runs under counterEmitMaxInFlight, so a saturated cap or a process exit can lose an increment before it is staged in Redis, and downloads_count is not among the columns reconciliation recomputes. ADR-007 asks that every increment be preserved; ## Dependencies states what is preserved once staged, what the residual is, and that #632 (closed) owns it.

Measurement basis

Pinned at d5ee57ffdf602e852c14c7cce93fb202c82449ac, which is also this branch's merge base with main, and every file:line and every derived count in the document is a claim at that commit unless a later basis is named at the claim itself. That sentence used to cover file:NN citations alone, which is what let two stale facts through review, so it now covers derived counts, quoted comment bodies and file-existence claims as well.

The facts the review found stale are re-derived at e590735c4 and say so where they appear, rather than by re-pinning the document: a pin whose citations were not all re-derived against it asserts more than was checked. The paragraph carrying the pin, under ## Spec Reference, lists which facts those are and where each one says so.

Worth knowing while reviewing: the issue measured at 7f766481cd1d and the tree moved a long way since, so several of the issue's own citations are stale at this basis. The plan lists them under ## Corrections this plan implements rather than silently substituting corrected numbers, and it also flags the claims the issue makes that the tree contradicts.

Two false claims already in the tree get closed in passing, both named in the steps that touch them: docs/specs/S17-rest-management-api.md:93 still calls MavenRepositoryStore.BumpRepoDownloadCount a no-op stub (it executes) and still says nothing maintains downloads_count, and the sentence at :1645 calls MarkRepoLastUpdated a stub, which Step 2 makes false. That second sentence also names BumpRepoCounters, which exists in no Go file, so Step 2 rewrites it whole rather than amending the half it falsifies. The first is what #975 tracks.

!2268 (merged) is a merge dependency for Steps 2, 3 and 4, for two different reasons. For Steps 2 and 3 it adds a one-hour freshness window to RepositoryStore.MarkRepositoryLastUpdated, the statement every last_updated_at writer in this plan lands on, so a repeat write inside the window books nothing, and their acceptance criteria and testing strategy are written against that. For Step 4 it is a shared seam rather than a behaviour change: !2268 (merged) reworks the same internal/datastore/repositories.go and internal/datastore/query_names.go that step adds a write-back to.

Diff size

1486 insertions in one file, past the 500 reviewable-LOC line in docs/dev/development-model.md. Splitting would not help: it is a single plan document whose sections cross-reference each other, and the guardrails ask for one plan file per initiative committed through one plan MR. The document is the unit of review.

Testing

No code changes, so no tests. The plan's own ## Testing Strategy states the e2e scenario impact for each of the five steps: Steps 2 through 5 are behavior-changing and each amends the scenario in its format's catalog under docs/testing/e2e/, and Step 1 is documentation and affects none. All pre-commit hooks pass on the file, markdownlint, lychee and Vale included.

Related to #1046

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading