Repository navigation
cold-store-reaper: prune_backups apply is not crash-consistent — a mid-run trash failure rolls back every ledger row, but the Drive trashes stay, and the trashed files are invisible to any retry #2550
Description
Summary
ColdStore.prune_backups(apply=True) performs the Google Drive trash calls inside the local SQLite transaction (with conn:), interleaved with the transfer_ledger inserts. Drive is not transactional: when any one trash PATCH fails part-way through a prune run, the files already trashed stay trashed, but the SQLite context manager rolls back — so every ledger row for the run, including the rows for backups that really were trashed, is discarded. Worse, the loss is unrecoverable: list_backups only lists untrashed files, so the already-trashed backups are invisible to every later prune plan and their ledger rows can never be written by a retry.
Evidence
pkgs/cold-store-reaper/cold_store.py (main 28a70c2b; file sha256 d3fe48c5…):
:1124-1143— the apply block:conn = self.db(); with conn:wraps the whole multi-file loop; each iteration doeshttp_request("PATCH", …/files/{id}, {"trashed": True})(Drive, external, immediate) and thenself.ledger_add(conn, "trash", f["size"], "backup-prune", f["name"])(SQLite, uncommitted until the block exits). Any non-200 raisesColdStoreError(:1137-1140), thewith conn:block rolls back, and all ledger inserts of the run vanish while the Drive trashes persist.ledger_additself is a plain insert on the caller's connection (:450-458) — it commits only when the outer block commits.list_backupsasks Drive for untrashed files only (the test harness documents the same contract: trashed files are invisible to the listing), so a retry's plan contains only the not-yet-trashed remainder.
Reproduced this run (laptop probe, package's own FakeDrive harness)
Probe: ~/git/fleet/probe-2026-10-29-laneA-pruneledger/probe.py on the laptop, against pkgs/cold-store-reaper/tests/test_cold_store.py's TestPruneBackups seed series (5 prune candidates), with cold_store.http_request wrapped to return HTTP 500 on the 2nd trash PATCH:
PATCH attempts: 2
raised: ColdStoreError: trash failed for backup d1-20260331T010000Z-export.sql (HTTP 500)
fake Drive trashed_calls: 1
files actually trashed in Drive: 1
ledger backup-prune rows after failure: 0
trashed file visible to re-run plan: [False]
re-run prune count: 4
So one backup is really trashed, zero ledger rows survive, and the re-run plan (4 remaining candidates) never sees the trashed file again — its ledger row is permanently unwritable.
Why it matters
- The
transfer_ledgeris the audit/accounting record for retention: cold-store-reaper: project-budget enforcement in put-backup never counts backup bytes — cumulative backups can cross the 3 TiB budget unrefused (PR #988) #1028's backup-bytes accounting decrements counted backup bytes frombackup-prunetrash rows (test_backup_prune_ledger_row_decrements_backup_bytes). A missing row means the store permanently over-counts the pruned backup's bytes — budget/retention decisions are made from numbers that say a deleted backup still exists. - The operator sees only
trash failed for backup … (HTTP 500)for the second file; nothing reports that the first file was trashed without a ledger record. The divergence is silent. - Partial failure is the normal case for a multi-file network operation (token expiry, quota, one bad file id), not an exotic one: any prune run with ≥2 candidates that fails after the first success hits this.
Acceptance criteria
- A prune run that fails part-way leaves the ledger consistent with Drive: every backup actually trashed has exactly one
backup-pruneledger row, and no ledger row exists for a backup that was not trashed. (Shape is the fixer's choice — e.g. commit per file after its successful trash, or record-then-trash with reconciliation — as long as the invariant holds under the probe below.) - A retry after a partial failure completes the remaining prunes and does not double-ledger or re-trash the already-trashed files.
- Regression test in
pkgs/cold-store-reaper/tests/test_cold_store.py: using the FakeDrive harness, fail the 2nd trash PATCH of a multi-candidate apply (the existing tests cover only the dry-run plan and the all-success apply:test_prune_dry_run_plan,test_prune_apply_trashes_and_ledgers); assert (a) the error still surfaces, (b) the first file's ledger row is present, (c) a re-run plan/apply finishes the rest with exactly one ledger row per trashed backup in total. - Existing cold-store-reaper tests still pass.
Activity
dezren39 commentedon Oct 30, 2026
agent: new-issue-creator-code-a
state: filed
Found via: filed this run — verified with a laptop probe (~/git/fleet/probe-2026-10-29-laneA-pruneledger/probe.py) reusing the package's FakeDrive harness: HTTP 500 on the 2nd trash PATCH left 1 file trashed in Drive, 0 backup-prune ledger rows, and the trashed file invisible to the re-run plan. Evidence file: hidden_files/issue-code-pruneledger-body.md (goal workspace). Not self-solved: the commit-boundary shape (per-file commit vs record-then-reconcile) is triage's pin.
dezren39 commentedon Nov 1, 2026
🤖 Agent note · picked up
dezren39 commentedon Nov 1, 2026
🤖 Agent note · done (triage)
Verdict: CONFIRMED at current main
70c206250(2026-11-01), exactly as filed.Evidence (fresh, this run):
pkgs/cold-store-reaper/cold_store.pyprune_backupsapply block (~:1177): ONEwith conn:transaction wraps the Drive trash PATCHes for every db and candidate, interleaved withledger_addinserts into the same uncommitted transaction. A non-200 raisesColdStoreErrorinside the block → full rollback.list_backups(cold_store.py:1029+) queries Drive withtrashed = false— a trashed file can never reappear in a later plan.http_requestwrapped to return HTTP 500 on the 2nd PATCH):ColdStoreErrorraised; Drivetrashed_calls= 1 (the trash persisted);transfer_ledgerbackup-prune rows = 0 (all rolled back); re-run dry plan prune count = 4, the trashed file invisible — its ledger row is permanently unwritable, and the cold-store-reaper: project-budget enforcement in put-backup never counts backup bytes — cumulative backups can cross the 3 TiB budget unrefused (PR #988) #1028 backup-bytes accounting loses that decrement forever.with conn:per operation (lines 789, 904, 930, 982, 1019) — prune_backups is the outlier.Commit boundary pinned — per-file commit (PATCH → immediately commit that file's ledger row, then next file):
ColdStoreErrornaming the failing file.Regression tests named in the fix packet: mid-series 500 (1 trashed ⇔ 1 ledger row, remaining 3 still planned), retry completes the set with no duplicate row for the first file, existing apply/dry-run tests unchanged.
dezren39 commentedon Nov 1, 2026
agent-note:v1
state: claimed
task: 5365 (repo-health-code-pilot, lane D)
stage: pickup
did: claimed small_fix; revalidated #2550 OPEN, no competing PR; prune_backups apply block verified on fresh main — one
with conn:wraps all PATCH+ledger_add across every db, exactly as triage 3786 pinned.resume: restructure to per-file commit on a laptop worktree, regression tests, PR, merge when verified.
Merge pull request #3451 from developing-today/queue/5365-2550-prune-…
dezren39 commentedon Nov 1, 2026
agent-note:v1
state: done
task: 5365 (repo-health-code-pilot, lane D)
stage: fixed_merged
did: PR #3451 merged (merge commit e94657a); this issue auto-closed. prune_backups apply is now crash-consistent: per-file commit (trash PATCH, then that file's ledger row in its own transaction). Full test_cold_store suite 89 OK on the laptop worktree; Codex review completed with no findings.
resume: none — terminal.