v0.11 item 3, after review: seven defects #203 merged with

[arpanghoshal]

What happened

#203 merged before its independent review landed. The review found seven defects, each demonstrated against the merged code with a script, and main carries all seven right now. This is those fixes, plus the tests that fail without them.

The one that matters most is the first.

The seven

1. The checkpoint was launderable. §4.6 says a checkpoint that is itself anchored supersedes the anchors below it, so that pruning and anchoring do not cancel. verify_anchors built the set of anchored checkpoints from the union of what the provider returned and what the store's own anchors table held:

anchored_checkpoints = {anchor.seq for anchor in held if anchor.kind == CHECKPOINT} | {
    anchor.seq for anchor in cached.values() if anchor.kind == CHECKPOINT
}

An attacker who erases a prefix, writes a checkpoint row to explain it, and writes one row into the local anchors table beside it gets superseded instead of anchor_broken. The hash in that row is never read, so sha256:not-a-hash-at-all works. The whole of what §4.6 is supposed to buy is that the prune leaves a record outside the store, and it bought nothing.

Supersession now comes from the provider alone, and the anchor's (seq, hash) must be the pair the checkpoint claims:

anchored_checkpoints = {
    anchor.seq
    for anchor in held
    if anchor.kind == CHECKPOINT
    and checkpoint is not None
    and anchor.seq == checkpoint[0]
    and anchor.hash == checkpoint[1]
}

T547b passed through all of this, because it writes the checkpoint row and no anchors row beside it. T550 and T559 are the tests that fail without the fix.

2. The SQLite prune dropped its lock at the first write. pruning() opened BEGIN IMMEDIATE, and then every put_anchor and put_checkpoint went through with connection: and committed it. The prune held the lock for one statement. Postgres had the guard and SQLite, the default backend, did not, because the defect was found on Postgres and fixed only where it was found. A failed prune left [('missing', 4), ('link_broken', 1)] on a chain that was intact when it started. T551.

3. A hold placed during a prune was ignored and its receipts deleted: put_hold on Postgres took no row lock, so it landed between the prune's validation and its delete. T558b.

4. --through above the head was decided by receipt_chain. One UPDATE receipt_chain SET seq = 99 turned --through 8 into a full-chain delete that both readers called clean. The bound now comes from the receipts themselves, which is the row §2.1 assumes is rewritten. T552.

5. Rule 2's simulation was not the store. It dropped UnreadableReceipt rows and re-derived the head, so a store with one unparseable row refused every prune. T553.

6. The checkpoint could assert a (seq, hash) pair that never existed and then anchor it: the seq came from --through rather than from the boundary receipt. T554.

7. A refused prune and a successful one left byte-identical receipts, and --older-than was in neither. The prune now writes a request staged proposed and an outcome staged completed or refused. T555.

Mutation

22 of 22 caught, against a git archive HEAD copy with PYTHONDONTWRITEBYTECODE=1.

The last one is worth naming. R1 deletes rule 2's comparison outright, if caused: to if False:, and every test in the file still passed. That is a real finding about the code, not only the test: once #6 makes the checkpoint name a pair that exists and #5 makes the simulation the store, no store state reaches that branch any more. Every construction is caught earlier, by the head bound, the forward-only checkpoint, or the missing-hash refusal. T556 looked like it covered it and did not, because its assertion is an or the earlier refusal satisfies.

The comparison stays as a backstop and T556c keeps it honest, feeding it a simulation that reports one break more than the store does and asserting the refusal names the seq, deletes nothing, and leaves no checkpoint.

Gate

All checks passed!            (ruff, ruff format, mypy --strict: 52 files)
4565 passed, 6 warnings in 160.40s

Postgres tests ran against a real server. T549 races two prunes in separate OS processes.

Pairing

Docs are v0.11/3a-retention-review, which stacks on the still-open ctrlrun-docs#34.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 27 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d6d350c1-fad7-428b-b661-9bc4d405e5de

📥 Commits

Reviewing files that changed from the base of the PR and between a1c9633 and 735f98b.

📒 Files selected for processing (6)

• src/ctrlrun/anchor.py
• src/ctrlrun/cli/main.py
• src/ctrlrun/postgres.py
• src/ctrlrun/retention.py
• src/ctrlrun/state.py
• tests/test_retention.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}