fix: stop cleanup punching holes in the pad it is cleaning

[JohnMcLear]

Refs #8134. Second of two: #8145 makes a damaged pad fail gracefully; this stops cleanup creating the damage.

Problem

While investigating #8134 I went looking for hole-forming paths other than appendRevision, and found two — both inside the cleanup feature itself.

1. deleteRevisions deletes the whole history before rewriting it.

await timesLimit(pad.head + 1, 500, async (i) => {
  await db.remove(`pad:${padId}:revs:${i}`, null);     // every revision, gone
});
// ...only now are the replacements written

Any failure in that window leaves the pad with holes, or with no history at all, while the pad record still claims them.

2. The cached Pad is only unloaded on the success path. A failed cleanup leaves an object carrying the pre-cleanup head. One more edit through it appends at the OLD head and persists a head far past the rebuilt history.

Measured on develop, failing a single write and then making one edit:

a failed rewrite leaves no holes:            head=3   missing=[2]
stale-pad append left holes:                 head=13  missing=[3,4,5,6,7,8,9,10,11,12]

Ten holes from one failed cleanup plus one keystroke — none recoverable, and the pad can then never be cleaned up again. The feature meant to reclaim space is itself a way into the state #8134 reports.

Fix

Write-then-swap-then-drop. Write the rebuilt revisions first, then move head onto them, then remove the revisions the new head no longer references. Overwriting revisions 0..keepRevisions in place is safe because everything needed to rebuild them is already in memory (changeset and revisions) — nothing is read back from those keys. The final delete touches only orphans, so failing there wastes space without making the pad inconsistent.

Unload the cached Pad in a finally, so a failed cleanup can't leave a stale object able to append past the rewritten head.

What this does not fix

ueberdb offers no transaction across the rewrite. A failure part-way can still leave revisions 0..keepRevisions rebuilt while head hasn't moved onto them, and check() then fails on a content mismatch.

That state is recoverable — full compaction rebuilds the history from the current text. Holes are not recoverable: a missing revision can't be reconstructed, because every later revision is a delta that assumes its result. That asymmetry is why holes are the thing worth eliminating, and the test says so explicitly rather than pretending the window is gone.

Tests

src/tests/backend/specs/cleanupNoDestructiveWindow.ts — 6 cases: a failed rewrite leaves no holes; a failed rewrite leaves a pad full compaction can repair; a failed cleanup can't leave a stale pad that appends past head; the cached pad is dropped even on failure; and two success-path cases (keeps the last N revisions and checks out; orphans actually removed).

4 of the 6 fail on develop, including both hole assertions above. All 6 pass with the fix. Full backend suite: 1627 passing, 0 failing. tsc --noEmit clean.

Note

The repair test asserts no-holes and content rather than check(), because on develop full compaction still writes an invalid revision 1 (#8139, fixed by #8140). Keeping this PR independent of that one rather than stacking.

🤖 Generated with Claude Code

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix cleanup to avoid creating revision holes and unload stale cached pads

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Reorder cleanup to write rebuilt revisions before updating head and deleting orphans
• Always unload cached Pad objects on cleanup success or failure to prevent stale-head appends
• Add regression tests covering failure windows, stale cache behavior, and orphan deletion

Diagram

graph TD
  A["Cleanup.deleteRevisions()"] --> B["Kick sessions"] --> C["Rebuild history"] --> D[("DB: padId revs 0..N")]
  D --> E[("DB: pad record (head)")] --> F["Delete orphan revs"] --> G["Unload/reload Pad + check()"]
  C -. "finally" .-> H["PadManager.unloadPad()"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Write rebuilt revisions under a temporary keyspace then swap pointer

• ➕ Avoids in-place overwrites, reducing content-mismatch risk if rewrite fails mid-way
• ➕ Makes it easier to retry/rollback by keeping old revisions intact until swap
• ➖ Requires an atomic rename/swap or transaction support that ueberdb typically lacks
• ➖ More keys and cleanup complexity; still needs careful failure handling

2. Always fall back to full compaction (deleteAllRevisions) on any cleanup failure

• ➕ Simplifies consistency model: rebuild from current text is recoverable
• ➕ Avoids partially rewritten history states entirely
• ➖ More expensive; discards revision history granularity during repair
• ➖ Does not solve the core hole-creation bug by itself; just mitigates aftermath

Recommendation: The PR’s write-then-swap-then-drop ordering is the right durability tradeoff under ueberdb’s non-transactional constraints: it eliminates unrecoverable holes (the worst failure mode) while keeping space reclamation best-effort. The added unconditional cache unload in a finally is essential to prevent stale-head appends; alternatives either rely on missing atomic primitives or increase operational cost.

Files changed (2) +217 / -17

Bug fix (1) +52 / -17

Cleanup.tsMake revision cleanup non-destructive under failures and unload cached pads +52/-17

Make revision cleanup non-destructive under failures and unload cached pads

• Refactors deleteRevisions() to rebuild and persist new revisions first, then update the pad head/savedRevisions, and only then delete orphaned revisions above the new head. Adds a try/finally to always unload the cached Pad to prevent stale-head appends after a failed cleanup, and unloads before verification reload so check() reads from storage.

src/node/utils/Cleanup.ts

Tests (1) +165 / -0

cleanupNoDestructiveWindow.tsAdd regression tests for cleanup failure windows and stale-pad behavior +165/-0

Add regression tests for cleanup failure windows and stale-pad behavior

• Introduces a new backend spec that simulates write failures during cleanup to assert no revision holes are created and that stale cached pads are dropped even on failure. Also verifies success-path behavior (keeps last N revisions, passes check(), and deletes orphaned revisions).

src/tests/backend/specs/cleanupNoDestructiveWindow.ts

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

1. Orphan deletion non-retryable 🐞 Bug ☼ Reliability

Description

If orphan revision deletion fails, rebuildHistory() throws after already committing pad:<id>.head to
keepRevisions, so callers see a cleanup failure even though the compacted history is already live.
Subsequent cleanup attempts can short-circuit on pad.head <= keepRevisions, leaving orphaned
revisions behind with no normal retry path.

Code

src/node/utils/Cleanup.ts[R154-157]

+  if (oldHead > keepRevisions) {
+    await timesLimit(oldHead - keepRevisions, 500, async (i: number) => {
+      await db.remove(`pad:${padId}:revs:${keepRevisions + 1 + i}`, null);
+    });

Evidence

The code commits padContent.head = keepRevisions and persists it before attempting to delete
orphan revisions; any exception from db.remove() will reject the whole operation after the head is
already moved. Because deleteRevisions() short-circuits when pad.head <= keepRevisions, a later
retry can skip the orphan deletion phase, and the admin handler will present the thrown exception as
a cleanup failure.

src/node/utils/Cleanup.ts[135-158]
src/node/utils/Cleanup.ts[52-55]
src/node/hooks/express/adminsettings.ts[369-399]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rebuildHistory()` updates the pad record (`pad:<id>.head`) before deleting orphaned revision keys. If the orphan-deletion phase fails, the function rejects and the admin/API surface an error even though the pad now points at the rebuilt history. Because `deleteRevisions()` returns early when `pad.head <= keepRevisions`, a subsequent retry can become a no-op, leaving orphaned revisions permanently.

### Issue Context
This PR intentionally makes orphan deletion safe (wasted space vs. inconsistency). That safety goal is undermined by treating orphan deletion failure as fatal and by having no retry path once `head` has been moved.

### Fix Focus Areas
- src/node/utils/Cleanup.ts[151-158]
- src/node/utils/Cleanup.ts[52-55]

### Suggested change
- Wrap the orphan-deletion block in `try/catch`.
- On failure, log a warning/error with padId and continue to unload + re-read + `newPad.check()`.
- Return `true` if the rebuilt history was written and the pad checks out (even if orphan deletion failed), so the API/admin UI doesn’t report failure for a successful compaction.
- (Optional) If you need to surface partial success, add structured logging/metrics for “orphan cleanup failed” rather than throwing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context

✅ Tickets: 🎫 "Cleanup revisions" fails due to missing revision 🎫 copyPadWithoutHistory writes an invalid changeset: copies grow a newline and fail pad.check() forever

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-free-for-open-source-projects]

1. Orphan deletion non-retryable 🐞 Bug ☼ Reliability

If orphan revision deletion fails, rebuildHistory() throws after already committing pad:<id>.head to
keepRevisions, so callers see a cleanup failure even though the compacted history is already live.
Subsequent cleanup attempts can short-circuit on pad.head <= keepRevisions, leaving orphaned
revisions behind with no normal retry path.

Agent Prompt

### Issue description
`rebuildHistory()` updates the pad record (`pad:<id>.head`) before deleting orphaned revision keys. If the orphan-deletion phase fails, the function rejects and the admin/API surface an error even though the pad now points at the rebuilt history. Because `deleteRevisions()` returns early when `pad.head <= keepRevisions`, a subsequent retry can become a no-op, leaving orphaned revisions permanently.

### Issue Context
This PR intentionally makes orphan deletion safe (wasted space vs. inconsistency). That safety goal is undermined by treating orphan deletion failure as fatal and by having no retry path once `head` has been moved.

### Fix Focus Areas
- src/node/utils/Cleanup.ts[151-158]
- src/node/utils/Cleanup.ts[52-55]

### Suggested change
- Wrap the orphan-deletion block in `try/catch`.
- On failure, log a warning/error with padId and continue to unload + re-read + `newPad.check()`.
- Return `true` if the rebuilt history was written and the pad checks out (even if orphan deletion failed), so the API/admin UI doesn’t report failure for a successful compaction.
- (Optional) If you need to surface partial success, add structured logging/metrics for “orphan cleanup failed” rather than throwing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools