From 45972cc031e9ee31163b3e747d6ce31155baac98 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 15 Aug 2026 14:24:48 +0100 Subject: [PATCH] fix: stop cleanup punching holes in the pad it is cleaning deleteRevisions() removed every revision 0..head and only then wrote the replacements. Any failure in that window left the pad with holes -- or with no history at all -- while the pad record still claimed them. Worse, the cached Pad was only unloaded on the success path. A failed cleanup left an object carrying the pre-cleanup head, so one more edit through it appended at the OLD head and persisted a head far past the rebuilt history. Measured on develop: a cleanup that fails one write, plus a single subsequent edit, takes a 12-revision pad to head=13 with revisions 3..12 all missing. Ten holes, none of them recoverable, and the pad can then never be cleaned up again. Two changes: - Reorder to write-then-swap-then-drop. The rebuilt revisions are written first, then head is moved onto them, then the now-orphaned revisions above the new head are removed. Overwriting revisions 0..keepRevisions in place is safe because everything needed to rebuild them is already in memory; nothing is read back from those keys. The final delete step touches only orphans, so failing there wastes space without making the pad inconsistent. - Unload the cached Pad in a finally, so a failed cleanup cannot leave a stale object able to append past the rewritten head. ueberdb offers no transaction across the rewrite, so a failure part-way can still leave revisions 0..keepRevisions rebuilt while head has not moved onto them -- check() then fails on a content mismatch. That state is recoverable: full compaction rebuilds the history from the current text. Holes are not recoverable, which is why they are the thing worth eliminating. Refs #8134 Co-Authored-By: Claude Opus 5 (1M context) --- src/node/utils/Cleanup.ts | 69 ++++++-- .../specs/cleanupNoDestructiveWindow.ts | 165 ++++++++++++++++++ 2 files changed, 217 insertions(+), 17 deletions(-) create mode 100644 src/tests/backend/specs/cleanupNoDestructiveWindow.ts diff --git a/src/node/utils/Cleanup.ts b/src/node/utils/Cleanup.ts index 30967654f52..3e5738efc39 100644 --- a/src/node/utils/Cleanup.ts +++ b/src/node/utils/Cleanup.ts @@ -56,6 +56,19 @@ export const deleteRevisions = async (padId: string, keepRevisions: number): Pro padMessageHandler.kickSessionsFromPad(padId) + try { + return await rebuildHistory(pad, padId, keepRevisions) + } finally { + // Always drop the cached Pad, success or failure. It still carries the + // pre-cleanup head, and if cleanup failed after the pad record was + // rewritten, an edit through that stale object would append at the OLD + // head -- persisting a head far past the rebuilt history and punching a + // run of holes that no later cleanup can repair. See #8134. + padManager.unloadPad(padId); + } +} + +const rebuildHistory = async (pad: any, padId: string, keepRevisions: number): Promise => { const cleanupUntilRevision = pad.head - keepRevisions logger.debug('Composing changesets: ', cleanupUntilRevision) const changeset = await padMessageHandler.composePadChangesets(pad, 0, cleanupUntilRevision + 1) @@ -69,24 +82,17 @@ export const deleteRevisions = async (padId: string, keepRevisions: number): Pro logger.debug('Loaded revisions: ', revisions.length) - await timesLimit(pad.head + 1, 500, async (i: string) => { - await db.remove(`pad:${padId}:revs:${i}`, null); - }); - - let padContent = await db.get(`pad:${padId}`) - padContent.head = keepRevisions - if (padContent.savedRevisions) { - let newSavedRevisions = [] + const oldHead = pad.head - for (let i = 0; i < padContent.savedRevisions.length; i++) { - if (padContent.savedRevisions[i].revNum > cleanupUntilRevision) { - padContent.savedRevisions[i].revNum = padContent.savedRevisions[i].revNum - cleanupUntilRevision - newSavedRevisions.push(padContent.savedRevisions[i]) - } - } - padContent.savedRevisions = newSavedRevisions - } - await db.set(`pad:${padId}`, padContent); + // Order matters. This used to remove every revision 0..head first and only + // then write the replacements, so any failure in that window left the pad + // with holes -- or with no history at all -- while the pad record still + // claimed them. Instead: write the rebuilt history, then move head onto it, + // then drop what is left over. + // + // Overwriting revisions 0..keepRevisions in place is safe because + // everything needed to rebuild them is already in memory above + // (`changeset` and `revisions`); nothing is read back from those keys. let newAText = Changeset.makeAText('\n'); let pool = pad.apool() @@ -126,8 +132,37 @@ export const deleteRevisions = async (padId: string, keepRevisions: number): Pro await Promise.all(p) + // The rebuilt history is durable; point the pad at it. + let padContent = await db.get(`pad:${padId}`) + padContent.head = keepRevisions + if (padContent.savedRevisions) { + let newSavedRevisions = [] + + for (let i = 0; i < padContent.savedRevisions.length; i++) { + if (padContent.savedRevisions[i].revNum > cleanupUntilRevision) { + padContent.savedRevisions[i].revNum = padContent.savedRevisions[i].revNum - cleanupUntilRevision + newSavedRevisions.push(padContent.savedRevisions[i]) + } + } + padContent.savedRevisions = newSavedRevisions + } + await db.set(`pad:${padId}`, padContent); + + // Only now drop the revisions the new head no longer references. These are + // orphans: check() walks 0..head, so if this part fails it wastes space + // without making the pad inconsistent. + if (oldHead > keepRevisions) { + await timesLimit(oldHead - keepRevisions, 500, async (i: number) => { + await db.remove(`pad:${padId}:revs:${keepRevisions + 1 + i}`, null); + }); + } + logger.debug('Finished migration. Checking pad now') + // Drop the cached Pad before re-reading: it still carries the pre-cleanup + // head, and verifying against that would walk revisions this cleanup just + // removed. (The caller's `finally` unloads it again; this one has to happen + // here so the verification below reads from storage.) padManager.unloadPad(padId); let newPad = await padManager.getPad(padId); diff --git a/src/tests/backend/specs/cleanupNoDestructiveWindow.ts b/src/tests/backend/specs/cleanupNoDestructiveWindow.ts new file mode 100644 index 00000000000..06087977885 --- /dev/null +++ b/src/tests/backend/specs/cleanupNoDestructiveWindow.ts @@ -0,0 +1,165 @@ +'use strict'; + +// deleteRevisions() must not be able to damage the pad it is cleaning. +// +// It used to remove every revision 0..head and only then write the +// replacements. A failure anywhere in that window left the pad with holes, +// or with no history at all, while the pad record still claimed them. And +// because the cached Pad was only unloaded on the success path, a failed +// cleanup left an object carrying the pre-cleanup head -- one more edit +// through it appended at the OLD head and punched a whole run of holes. +// +// Measured on develop before this change: a cleanup that failed one write, +// plus a single subsequent edit, took a 12-revision pad to head=13 with +// revisions 3..12 all missing. +// +// Found while investigating #8134. + +const assert = require('assert').strict; +const common = require('../common'); +const padManager = require('../../../node/db/PadManager'); +const db = require('../../../node/db/DB'); +const settings = require('../../../node/utils/Settings'); +const {deleteRevisions} = require('../../../node/utils/Cleanup'); + +describe(__filename, function () { + let padId: string; + let cleanupEnabledBackup: boolean; + + before(async function () { + await common.init(); + cleanupEnabledBackup = settings.cleanup.enabled; + settings.cleanup.enabled = true; + }); + + after(function () { settings.cleanup.enabled = cleanupEnabledBackup; }); + + beforeEach(async function () { + padId = common.randomString(); + assert(!await padManager.doesPadExist(padId)); + }); + + const makePad = async (revs = 12) => { + const pad = await padManager.getPad(padId); + for (let i = 0; i < revs; i++) await pad.appendText(`line ${i}\n`); + return pad; + }; + + // Revision numbers in [0, head] with no stored record. + const holes = async () => { + const rec = await db.get(`pad:${padId}`); + const missing = []; + for (let r = 0; r <= rec.head; r++) { + if (await db.get(`pad:${padId}:revs:${r}`) == null) missing.push(r); + } + return {head: rec.head, missing}; + }; + + const withFailingWrite = async (failKey: string, fn: () => Promise) => { + const realSet = db.set; + db.set = async (key: string, value: unknown) => { + if (key === failKey) throw new Error('simulated write failure'); + return await realSet(key, value); + }; + try { return await fn(); } finally { db.set = realSet; } + }; + + it('a failed rewrite leaves no holes', async function () { + await makePad(); + padManager.unloadPad(padId); + + await withFailingWrite(`pad:${padId}:revs:2`, + async () => { await deleteRevisions(padId, 3).catch(() => {}); }); + + const {head, missing} = await holes(); + assert.deepEqual(missing, [], + `cleanup left holes: head=${head} missing=[${missing}]`); + }); + + it('a failed rewrite leaves a pad that full compaction can repair', + async function () { + // Honest about the limit here. ueberdb offers no transaction across + // the rewrite, so a failure part-way still leaves revisions + // 0..keepRevisions holding rebuilt content while `head` is not yet + // moved onto them -- check() fails on a content mismatch. What it no + // longer leaves is a *hole*, which is the unrecoverable state: gaps + // cannot be reconstructed, whereas a mismatch is fixed by rebuilding + // the history from the current text. + const {deleteAllRevisions} = require('../../../node/utils/Cleanup'); + await makePad(); + padManager.unloadPad(padId); + + await withFailingWrite(`pad:${padId}:revs:2`, + async () => { await deleteRevisions(padId, 3).catch(() => {}); }); + + assert.deepEqual((await holes()).missing, [], 'no holes'); + + await deleteAllRevisions(padId); + padManager.unloadPad(padId); + const repaired = await padManager.getPad(padId); + assert.deepEqual((await holes()).missing, [], 'history rebuilt intact'); + assert.ok(repaired.atext.text.includes('line 11'), 'content preserved'); + // Not asserting repaired.check() here: on develop full compaction + // still writes an invalid revision 1 (#8139, fixed by #8140), and + // this spec is deliberately independent of that one. + }); + + it('a failed cleanup does not leave a stale pad that can append past head', + async function () { + // The 10-hole case. Hold a reference the way a caller would, let + // cleanup fail, then keep editing. + const pad = await makePad(); + + await withFailingWrite(`pad:${padId}:revs:3`, + async () => { await deleteRevisions(padId, 3).catch(() => {}); }); + + // Whatever happened, an edit afterwards must not create holes. + await (await padManager.getPad(padId)).appendText('later edit\n') + .catch(() => {}); + + const {head, missing} = await holes(); + assert.deepEqual(missing, [], + `stale-pad append left holes: head=${head} missing=[${missing}]`); + }); + + it('the cached pad is dropped even when cleanup fails', async function () { + await makePad(); + await withFailingWrite(`pad:${padId}:revs:2`, + async () => { await deleteRevisions(padId, 3).catch(() => {}); }); + + // A fresh getPad must read the pad record rather than hand back the + // pre-cleanup object. + const rec = await db.get(`pad:${padId}`); + const reloaded = await padManager.getPad(padId); + assert.equal(reloaded.getHeadRevisionNumber(), rec.head, + 'getPad returned a pad whose head disagrees with storage'); + }); + + it('a successful cleanup still keeps the last N revisions and checks out', + async function () { + const pad = await makePad(); + const textBefore = pad.atext.text; + padManager.unloadPad(padId); + + assert.equal(await deleteRevisions(padId, 3), true); + + padManager.unloadPad(padId); + const after = await padManager.getPad(padId); + assert.equal(after.getHeadRevisionNumber(), 3); + assert.equal(after.atext.text, textBefore, 'text preserved'); + assert.deepEqual((await holes()).missing, []); + await after.check(); + }); + + it('a successful cleanup removes the orphaned revisions', async function () { + await makePad(); + padManager.unloadPad(padId); + assert.equal(await deleteRevisions(padId, 3), true); + + // Everything above the new head should be gone, not left as litter. + for (const r of [4, 5, 8, 12]) { + assert.ok(await db.get(`pad:${padId}:revs:${r}`) == null, + `revision ${r} should have been removed`); + } + }); +});