perf(ledger): prune superseded idempotency records - #672
Conversation
The writer idempotency ledger was never pruned. Measured across seven stores it holds 1,149 MB of 3,112 MB total (36%), and 87% of three freshly indexed stores. Pruning by superseded incarnation would have reclaimed far more but is unsound: StoreIncarnationV1 is not monotonic — every non-daemon process derives it from random process-run bytes while the daemon uses a small counter, so a CLI attach can outrank a later daemon one and pruning by it would re-admit duplicate writes. This prunes only rows whose authority epoch trails the checkpoint epoch for that same incarnation, which never crosses a shard or incarnation boundary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b84650119
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the superseded records unreachable, so the prune reads it after persist. | ||
| // The record inserted below sits at the new epoch and is never eligible. | ||
| if checkpoint.supersedes_authority() { | ||
| prune::prune_superseded(transaction, &metadata.shard_id)?; |
There was a problem hiding this comment.
When the first write after an authority transition has hundreds of thousands of superseded rows, this call runs one shard-wide DELETE inside the user mutation's savepoint and sole SQLite writer transaction. At the motivating scale of roughly 257,000 overflow-backed rows, it can monopolize admission and generate substantial journal/WAL pressure; interruption or SQLITE_FULL rolls back the checkpoint advance, so retries repeat the same cleanup and may prevent the new epoch from committing. Move this to bounded background batches rather than coupling all retention work to the first foreground write.
AGENTS.md reference: AGENTS.md:L148-L150
Useful? React with 👍 / 👎.
| SELECT checkpoint.authority_epoch | ||
| FROM td_runtime_writer_checkpoint_v1 AS checkpoint | ||
| WHERE checkpoint.shard_json = td_runtime_writer_idempotency_v1.shard_json | ||
| AND checkpoint.incarnation = td_runtime_writer_idempotency_v1.incarnation |
There was a problem hiding this comment.
When another incarnation on the same shard has an inconsistent checkpoint row, this subquery trusts its raw scalar authority_epoch even though checkpoint::next decoded and validated only the incarnation being advanced. For example, a scalar epoch corrupted to 999 while its watermark and receipt still encode 7 causes an unrelated valid transition to silently delete that incarnation's receipt rows; loading that checkpoint normally would instead return typed LedgerError::Corrupt. Scope the deletion to the already validated incarnation and epoch, or validate every checkpoint consulted before deleting.
AGENTS.md reference: AGENTS.md:L102-L104
Useful? React with 👍 / 👎.
perf(observation) landed an import of tracedecay_runtime_core::background_cpu, but neither the module file nor its declaration was added, so the pushed integration branch does not compile: every branch cut from it fails on an unresolved import. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Retention ran as one shard-wide DELETE inside the user mutation's savepoint and the process's sole SQLite writer transaction. At the measured scale - 257,131 rows across seven stores - that monopolises admission, and an interruption or SQLITE_FULL rolls the checkpoint advance back with it, so every retry re-attempts the same delete and the new epoch may never commit. Each commit now removes at most one bounded batch, so the epoch advance commits regardless of how much backlog remains, and later commits at the standing epoch keep draining it until it converges. The delete also read each incarnation's raw scalar authority_epoch, though checkpoint::next decodes and validates only the incarnation being advanced. A neighbouring row whose scalar is corrupt - 999 while its watermark and receipt still encode 7 - would silently retire that incarnation's live receipts, re-admitting the duplicate writes they exist to stop. The candidate set is now scoped to the single incarnation whose checkpoint this commit validated, at that checkpoint's epoch; a neighbour's records are retired by its own commits. Pruning is still keyed on authority supersession, never on age and never on superseded incarnation: StoreIncarnationV1 is not monotonic, so a non-daemon attach can outrank a later daemon one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both findings verified and fixed in Finding 1 (P1) — unbounded pruning inside a foreground user writeConfirmed. Each commit now removes at most one bounded batch (
On "bounded background batches" specifically: I kept the batches on the writer path rather than introducing a background task. The ledger never opens or commits a connection — the writer supplies the transaction capability — and the process has a sole SQLite writer, so a background prune would contend for that same writer and need its own admission path, while a second writer transaction is not available to be had. Amortized bounded batches deliver the property the finding is actually about (foreground work is bounded; the epoch advance is not hostage to the backlog) without a second writer. Happy to revisit if you want the task boundary regardless. A parallel local change on another branch bounded this the same way; I adopted the identical scheme and constant so the two do not diverge into competing bounding schemes. Finding 2 (P2) — prune subquery trusting an unvalidated checkpointConfirmed, and this is the dangerous one. The correlated subquery read every incarnation's raw scalar Took the conservative fix you named: the candidate set is scoped to the single incarnation whose checkpoint this commit decoded, validated, and persisted, at that checkpoint's validated epoch. No other incarnation's checkpoint row is consulted at all. A neighbour's superseded records are retired by that neighbour's own commits, under its own validated checkpoint. I did not take the "validate every consulted checkpoint" option: it would put a full decode of every checkpoint row on the shard onto every commit's hot path, and it fails open in the case that matters — a corrupt neighbour would abort the user's unrelated write rather than being ignored. The new delete set is a strict subset of the old one, so this cannot delete anything the previous rule kept. RED / GREENRED — new tests against the unmodified rule (the bound constant declared but not enforced by the SQL):
GREEN — same tests after the fix: Whole crate, The bound assertions are on batch counts, not elapsed time: seed What did not change
|
1 participant
Measured problem
The writer idempotency ledger is never pruned. Measured across seven real stores: 1,149 MB of 3,112 MB total (36%), 257,131 rows. Three freshly indexed stores were 87% idempotency ledger.
What the investigation refuted
The original hypothesis was that
shard_json— single-valued across all seven stores, 182 bytes, leading PK column — was causing overflow-page padding. Measured against 5,000 real rows:shard_jsonshard_json→ integerByte-identical. All three proposed fixes save exactly zero. The real cause is that the average row (~1,486 B) exceeds SQLite's 1,002-byte
maxLocalat 4 KiB pages, so every row spills into its own overflow page — 257,131 rows, 257,131 overflow pages, 1:1.The change that would actually work is replacing
original_receipt_json(587 B avg) with acommit_sequenceinteger: measured 5,140,480 bytes, zero overflow, 78% reduction. It is not in this PR — the ledger shards sit atuser_version=0, entirely outside the migration framework, so there is no mechanism to migrate existing stores. That is deliberate scope, not an oversight.What this PR does
Prunes rows whose
authority_epochtrails the checkpoint epoch for that same incarnation, fired when a commit advances the epoch.Pruning by superseded incarnation would have reclaimed ~84% of rows and is unsound:
StoreIncarnationV1is not monotonic — non-daemon processes derive it from random process-run bytes while the daemon uses a small counter, so a CLI attach can outrank a later daemon one. Pruning by it would silently re-admit duplicate writes.Honest caveat: this reclaims zero rows on all seven measured stores, because epochs never advance in those workloads. It bounds growth across authority transitions rather than reclaiming what is there today.
🤖 Generated with Claude Code