fix(retention): keep the newest snapshot, and close the PK/index gaps #9415 left (#9470, #9472, #9473)

[JSONbored]

Summary

Three retention defects from the 2026-07-27 audit. The first is confirmed to have been corrupting data on the production database; the other two are the unbounded-growth class that has already caused two fleet-wide outages.

Closes #9470
Closes #9472
Closes #9473

#9470 — the dedupe kept the stale row and deleted the fresh one

dedupeSignalSnapshots selected its survivor with MAX(rowid). On the self-host Postgres backend, pg-dialect's translateRowid rewrites every rowid to ctid — a physical heap location, not insertion order. Once the age-prune frees pages, a newer row inserted into a reclaimed early page gets a lower ctid than an older row on a later page, so MAX(ctid) selected a stale snapshot and the delete removed the genuinely newest one, permanently.

translateRowid's own doc says it is safe only for bookkeeping "resolved within one statement" and "never for durable application-facing row identity" — deciding which row survives a DELETE is exactly that. The invariant comment claiming rowid "can never tie" was a SQLite-only truth that silently became false when the pg shim was introduced.

Production evidence (2026-07-27, edge-nl-01). Comparing the row ctid picks against the row recency picks, per (signal_type, target_key) with more than one row:

signal_type	keys where ctid picks the wrong row	dedupe-eligible
contributor-evidence-graph	114 / 319	yes
contributor-strategy	112 / 310	yes
contributor-outcome-history	112 / 310	yes
repo-doc-refresh-attempt / config-quality / repo-public-focus-manifest	3 each	yes

~344 keys of dedupe-eligible types. And the dedupe demonstrably runs and deletes at scale — retention.prune audit rows show deduped 8570, 1977, 2031, 16928 on consecutive days, while pruned 0 row(s) (the age-prune deletes nothing, so the dedupe is doing all the deleting).

Survivors are now chosen by (generated_at, id), and the batched delete keys on the real primary key instead of rowid.

#9472 — #9415's five tables prune via full scan, hourly

#9415 added five tables to RETENTION_POLICY but shipped no RETENTION_PK_COLUMN entries and no index migration, while also moving the prune to hourly. So pkColumnFor() fell back to rowid → ctid, making each batched delete's outer IN a full sequential scan, plus a full scan and sort for the inner SELECT (no retention-column index existed). check_summaries (511 MB / 180,820 rows) and pull_request_files (179 MB / 63,091 rows) are the two largest tables in the database. On the synchronous SQLite adapter this also blocks the event loop.

Migration 0196 adds the leading-column indexes, following 0193's pattern.

#9473 — four more unbounded tables in the same class

Verified as written-per-event with no delete path anywhere in src/. Two have a sibling that is already pruned, which is what makes the omission unintentional rather than a decision:

• pull_request_reviews — the sixth GitHub mirror alongside pull_request_files/check_summaries, 30d
• predicted_gate_calibration_ledger — per (login, project, PR, commit); sibling predicted_gate_calls already pruned at 90d
• contributor_gate_history — per (login, project, PR, head_sha), so every push adds a row; every reader is already windowed, so aged rows are pure dead weight, 90d
• decision_replay_inputs — one blob per decision record, whose parent decision_records is pruned at 180d, so these were permanently orphaned; matched at 180d

Two things verification changed

orb_pr_outcomes has a composite primary key, not an id column — as do all four read-side caches (grounding_file_content_cache, ai_review_cache, ai_slop_cache, linked_issue_satisfaction_cache). I had initially mapped orb_pr_outcomes to id from the audit's report and the test suite caught it. Those tables genuinely cannot have a single-column PK mapping.

That exposed the real defect behind #9472: an absent RETENTION_PK_COLUMN entry meant either "composite PK, the rowid fallback is correct" or "someone forgot" — indistinguishable. That ambiguity is how #9415's five shipped unmapped and nothing noticed. RETENTION_COMPOSITE_PK_TABLES now records the deliberate choice explicitly, with a note on the cost of being on that list.

The new drift guard immediately found four pre-existing gaps (the four caches above), which is the point of adding it.

Validation

• npx tsc --noEmit -p tsconfig.json — clean
• npm run db:migrations:check — contiguous 0001..0196, next free 0197
• test/unit/retention.test.ts + test/unit/selfhost-pg-retention.test.ts — 52 passed

New tests:

• Survivor selection (retention: dedupeSignalSnapshots keeps the STALE row on Postgres (MAX(rowid)→MAX(ctid)) — confirmed deleting fresh snapshots in production #9470): newest-by-generated_at survives even when inserted first (under the old MAX(rowid) rule it would have been deleted); ties break deterministically on id; each target_key dedupes independently; a single-row key is untouched. SQLite cannot reproduce ctid page reuse, so these pin the property that makes the bug impossible on either backend rather than simulating the physical layout.
• Drift guard (retention: #9415's five tables prune via full seq scan + sort, hourly — no PK mapping, no indexes, no migration #9472): every policy table is either PK-mapped or explicitly listed as composite-PK; no table claims both; every policy table has an index leading with its retention column somewhere in migrations/; no orphaned PK mappings.

Two existing mock-based tests needed their SQL discriminator narrowed from SELECT DISTINCT to DISTINCT signal_type, because the rewritten stale-row condition now also contains a SELECT DISTINCT target_key subquery. No assertion was weakened.

Not included

#9474 (retention breaking the ledger verifier and the cumulative public counter) is deliberately excluded — it changes consumers rather than the policy, and needs a durable running-total rollup plus a verifier tolerance. It stays open for its own PR.

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[loopover-orb]

Tip

✅ LoopOver review result - approve/merge recommended

_{Review updated: 2026-07-28 00:27:12 UTC}

3 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

• safe to merge

Review summary
This PR fixes a confirmed production data-loss bug where dedupeSignalSnapshots picked survivors by MAX(rowid), which pg-dialect rewrites to MAX(ctid) — a physical heap location that can regress after page reuse from pruning, causing the newest snapshot to be deleted instead of kept. The fix correctly moves survivor selection to (generated_at, id) DESC via a correlated per-key subquery, and separately closes the PK/index gaps #9415 left by adding four new tables to RETENTION_POLICY, RETENTION_PK_COLUMN entries (plus a RETENTION_COMPOSITE_PK_TABLES exemption list) and migration 0196 with matching indexes, backed by a drift-guard test that asserts the three sites (policy/PK-map/migration) stay in sync. The dedupe regression tests are genuine — they deliberately insert rows out of generated_at order against real SQLite so the assertion would actually fail under the old MAX(rowid) logic, not a fabricated scenario.

Nits — 6 non-blocking

• src/db/retention.ts:119-127 — the comment above RETENTION_PK_COLUMN says 'fix(retention): bound the five unpruned tables that filled the hosted D1 #9415 added the five tables below' and 'All five have a single-column id TEXT PRIMARY KEY,' but only four are listed there (the fifth, orb_pr_outcomes, is a composite PK and lives in RETENTION_COMPOSITE_PK_TABLES instead) — the comment is inconsistent with the code beneath it.
• test/unit/retention.test.ts (unchanged existing test) — the test titled 'keeps only the highest-rowid row for latest-only signal types...' still describes selection by rowid even though the underlying logic now selects by (generated_at, id); worth renaming for clarity since it no longer describes what the code does.
• migrations/0196_retention_column_indexes_round_two.sql — I can't verify from the diff alone that 0194/0195 already exist and that 0196 is genuinely the next contiguous migration number; worth double-checking against the full migrations/ directory before merge.
• Update the stale 'highest-rowid' test description noted above to reference the new (generated_at, id) tiebreak so future readers aren't misled.
• Tighten the RETENTION_PK_COLUMN doc comment at src/db/retention.ts:119 to correctly enumerate which of the five fix(retention): bound the five unpruned tables that filled the hosted D1 #9415 tables get a single-column id vs. which is composite-PK-exempt.
• PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Decision drivers

• ✅ Code review — No blockers (1 reviewer)
• ✅ Gate result — Passing (No configured blocker found.)

Context & advisory signals — never blocks the verdict

Signal	Result	Evidence
Linked issue	✅ Linked	#9470, #9472, #9473
Related work	✅ No active overlap found	No same-issue or scoped active PR overlap found.
Change scope	✅ 20/20	Low review scope from cached public metadata (3 linked issues).
Validation posture	✅ 25/25	PR body includes validation/test evidence.
Contributor workload	✅ 10/10	Author activity: 14 registered-repo PR(s), 13 merged, 346 issue(s).
Contributor context	✅ Confirmed Gittensor contributor	JSONbored; Gittensor profile; 14 PR(s), 346 issue(s).
Improvement	✅ Minor	risk: clean · value: minor · LLM: significant

Linked issue satisfaction

Partially addressed
The PR correctly replaces MAX(rowid) with a (generated_at, id) selection and fixes the stale comment, satisfying the core fix and one deliverable, but the issue's other explicit deliverables—auditing all other rowid usages reaching Postgres (including the named orb/relay.ts suspect), strengthening translateRowid's doc with an explicit 'never for MAX/MIN' clause, and a test that runs against real P

Review context

• Author: JSONbored
• Role context: owner (maintainer lane)
• Public audience mode: oss maintainer
• Lane context: Repository is configured for direct PR review.
• Public profile languages: not available
• Official Gittensor activity: 14 PR(s), 346 issue(s).
• PR-specific overlap: none found.

Contributor next steps

• Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.

Signal definitions

• Related work = same linked issue, overlapping active PRs, or title/path similarity.
• Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
• Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
• Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
• Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

• @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
• @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
• A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

_{🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed}

---

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

• Re-run LoopOver review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.55%. Comparing base (6571a1b) to head (49373e1).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9503   +/-   ##
=======================================
  Coverage   89.55%   89.55%           
=======================================
  Files         843      843           
  Lines      110073   110074    +1     
  Branches    26194    26194           
=======================================
+ Hits        98573    98574    +1     
  Misses      10238    10238           
  Partials     1262     1262           

Flag	Coverage Δ
backend	95.26% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/db/retention.ts	100.00% <100.00%> (ø)

[loopover-orb]

LoopOver approves — the gate is satisfied and CI is green.