diff --git a/src/node/db/Pad.ts b/src/node/db/Pad.ts index dba21da275e..dd0f18b75a0 100644 --- a/src/node/db/Pad.ts +++ b/src/node/db/Pad.ts @@ -916,6 +916,39 @@ class Pad { return this.savedRevisions; } + /** + * Scans `0..head` for revisions that are absent or unusable. + * + * `check()` already trips over these, but only as + * `assert(timestamp != null)` part-way through replaying the history -- + * an assertion about a null timestamp, when what the operator needs to + * hear is "revision 600 is missing". This reports the gaps directly so + * callers can say something actionable instead. See #8134. + * + * Cheap relative to check(): it reads one sub-field per revision and + * replays nothing. + * + * @param limit Stop after this many gaps. A pad damaged by a failed + * cleanup can be missing hundreds of revisions and the operator does + * not need them all enumerated. + * @returns Ascending revision numbers with no usable stored record. + */ + async findMissingRevisions(limit = 20): Promise { + const missing: number[] = []; + const revs = Stream.range(0, this.getHeadRevisionNumber() + 1) + .map(async (r: number) => [r, await this.getRevisionDate(r)]) + .batch(100).buffer(99); + for await (const [r, timestamp] of revs) { + // A record that exists but carries no meta.timestamp is just as + // unreplayable as one that is absent, and fails check() identically. + if (timestamp == null) { + missing.push(r); + if (missing.length >= limit) break; + } + } + return missing; + } + /** * Asserts that all pad data is consistent. Throws if inconsistent. */ diff --git a/src/node/utils/Cleanup.ts b/src/node/utils/Cleanup.ts index 30967654f52..17e1b7682bc 100644 --- a/src/node/utils/Cleanup.ts +++ b/src/node/utils/Cleanup.ts @@ -45,6 +45,22 @@ export const deleteRevisions = async (padId: string, keepRevisions: number): Pro logger.debug('Start cleanup revisions', padId) let pad = await padManager.getPad(padId); + + // Report a damaged history as a damaged history. check() detects it too, + // but only as `assert(timestamp != null)` part-way through replaying the + // revisions, which tells an operator nothing about which record is bad or + // what to do about it. See #8134. + const missing = await pad.findMissingRevisions(); + if (missing.length > 0) { + throw new Error( + `Pad ${padId} is missing revision(s) ${missing.join(', ')}` + + `${missing.length >= 20 ? ' (and possibly more)' : ''}. ` + + 'Its history cannot be replayed, so revisions cannot be cleaned up. ' + + "The pad's current text is unaffected. Rebuild the history with a " + + 'full compaction (compactPad with no keepRevisions) to make the pad ' + + 'cleanable again.'); + } + await pad.check() logger.debug('Initial pad is valid') diff --git a/src/tests/backend/specs/cleanupMissingRevisions.ts b/src/tests/backend/specs/cleanupMissingRevisions.ts new file mode 100644 index 00000000000..bb4c4e455a5 --- /dev/null +++ b/src/tests/backend/specs/cleanupMissingRevisions.ts @@ -0,0 +1,150 @@ +'use strict'; + +// Cleanup should report a damaged history as a damaged history. +// +// Before this, deleteRevisions() went straight into pad.check(), which +// replays the whole history and dies on `assert(timestamp != null)` -- +// an assertion about a null timestamp, when what the operator needs to +// hear is "revision 600 is missing, here is what to do about it". The +// reporter on #8134 had to bisect their database by hand. + +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, deleteAllRevisions} = 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 padWithHoles = async (holes: number[], revs = 12) => { + const pad = await padManager.getPad(padId); + for (let i = 0; i < revs; i++) await pad.appendText(`line ${i}\n`); + for (const h of holes) await db.remove(`pad:${padId}:revs:${h}`, null); + padManager.unloadPad(padId); + return await padManager.getPad(padId); + }; + + describe('Pad.findMissingRevisions()', function () { + it('returns [] for a healthy pad', async function () { + const pad = await padWithHoles([]); + assert.deepEqual(await pad.findMissingRevisions(), []); + }); + + it('finds a single gap', async function () { + const pad = await padWithHoles([3]); + assert.deepEqual(await pad.findMissingRevisions(), [3]); + }); + + it('finds several gaps, in ascending order', async function () { + const pad = await padWithHoles([7, 2, 5]); + assert.deepEqual(await pad.findMissingRevisions(), [2, 5, 7]); + }); + + it('honours the limit', async function () { + const pad = await padWithHoles([2, 3, 4, 5, 6]); + const found = await pad.findMissingRevisions(2); + assert.equal(found.length, 2); + assert.deepEqual(found, [2, 3]); + }); + + it('does not report revisions beyond head', async function () { + const pad = await padWithHoles([]); + const head = pad.getHeadRevisionNumber(); + await db.remove(`pad:${padId}:revs:${head + 5}`, null); // no-op + assert.deepEqual(await pad.findMissingRevisions(), []); + }); + }); + + describe('deleteRevisions() on a damaged pad', function () { + it('names the missing revision instead of asserting', async function () { + await padWithHoles([3]); + padManager.unloadPad(padId); + const err: any = await deleteRevisions(padId, 2).then(() => null, (e: any) => e); + assert.ok(err != null, 'expected deleteRevisions to throw'); + assert.match(err.message, /missing revision\(s\) 3\b/); + assert.match(err.message, new RegExp(padId)); + // Not a bare assertion failure any more. + assert.ok(!/timestamp != null/.test(err.message), + `still surfacing the raw assertion:\n${err.message}`); + }); + + it('tells the operator their text is safe and how to recover', + async function () { + await padWithHoles([3]); + padManager.unloadPad(padId); + const err: any = + await deleteRevisions(padId, 2).then(() => null, (e: any) => e); + assert.match(err.message, /current text is unaffected/i); + assert.match(err.message, /compactPad/); + }); + + it('lists multiple gaps', async function () { + await padWithHoles([3, 6]); + padManager.unloadPad(padId); + const err: any = await deleteRevisions(padId, 2).then(() => null, (e: any) => e); + assert.match(err.message, /missing revision\(s\) 3, 6/); + }); + + it('leaves the damaged pad untouched', async function () { + // The whole point of failing before the destructive phase. + const pad = await padWithHoles([3]); + const headBefore = pad.getHeadRevisionNumber(); + const textBefore = pad.atext.text; + padManager.unloadPad(padId); + + await deleteRevisions(padId, 2).catch(() => {}); + + padManager.unloadPad(padId); + const after = await padManager.getPad(padId); + assert.equal(after.getHeadRevisionNumber(), headBefore); + assert.equal(after.atext.text, textBefore); + }); + + it('still cleans up a healthy pad', async function () { + const pad = await padWithHoles([]); + padManager.unloadPad(padId); + assert.equal(await deleteRevisions(padId, 3), true); + padManager.unloadPad(padId); + await (await padManager.getPad(padId)).check(); + }); + }); + + describe('full compaction is still allowed', function () { + it('deleteAllRevisions works on a damaged pad', async function () { + // This is the recovery path the error message points at, so it must + // not be gated behind the same check. + const pad = await padWithHoles([3]); + const textBefore = pad.atext.text; + padManager.unloadPad(padId); + + await deleteAllRevisions(padId); + + padManager.unloadPad(padId); + const after = await padManager.getPad(padId); + // Compared trimmed: on develop, copyPadWithoutHistory still appends a + // newline per copy (issue #8139, fixed by #8140), and this spec is + // deliberately independent of that one. What matters here is that the + // recovery path runs at all on a pad the keep-count path refuses. + assert.equal(after.atext.text.trimEnd(), textBefore.trimEnd(), + 'author-written text preserved'); + assert.deepEqual(await after.findMissingRevisions(), [], + 'history rebuilt without gaps'); + }); + }); +}); diff --git a/src/tests/backend/specs/holeProbe.ts b/src/tests/backend/specs/holeProbe.ts new file mode 100644 index 00000000000..456ef749c32 --- /dev/null +++ b/src/tests/backend/specs/holeProbe.ts @@ -0,0 +1,85 @@ +'use strict'; + +// Probe: are there hole-forming paths other than appendRevision (#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'); + +const missingRevs = async (padId: string) => { + 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}; +}; + +describe(__filename, function () { + let backup: boolean; + before(async function () { + await common.init(); + backup = settings.cleanup.enabled; + settings.cleanup.enabled = true; + }); + after(function () { settings.cleanup.enabled = backup; }); + + it('MECHANISM 2: a failed write during deleteRevisions leaves holes', + async function () { + const padId = common.randomString(); + const pad = await padManager.getPad(padId); + for (let i = 0; i < 12; i++) await pad.appendText(`line ${i}\n`); + const headBefore = pad.getHeadRevisionNumber(); + + // deleteRevisions removes every revision, then rewrites the kept + // ones. Fail one of the rewrites. + const realSet = db.set; + db.set = async (key: string, value: unknown) => { + if (key === `pad:${padId}:revs:2`) throw new Error('boom'); + return await realSet(key, value); + }; + let threw = false; + try { + await deleteRevisions(padId, 3); + } catch { threw = true; } finally { db.set = realSet; } + + padManager.unloadPad(padId); + const state = await missingRevs(padId); + console.log(` head before=${headBefore}; after: head=${state.head} ` + + `missing=[${state.missing}] threw=${threw}`); + assert.ok(state.missing.length > 0, + 'expected deleteRevisions to leave holes'); + }); + + it('MECHANISM 3: a stale in-memory pad appends past the rewritten head', + async function () { + const padId = common.randomString(); + const pad = await padManager.getPad(padId); + for (let i = 0; i < 12; i++) await pad.appendText(`line ${i}\n`); + const staleHead = pad.getHeadRevisionNumber(); + + // Fail late, after the pad record has been rewritten to the new head. + const realSet = db.set; + db.set = async (key: string, value: unknown) => { + if (key === `pad:${padId}:revs:3`) throw new Error('boom'); + return await realSet(key, value); + }; + try { await deleteRevisions(padId, 3); } catch { /* expected */ } + finally { db.set = realSet; } + + const recAfter = await db.get(`pad:${padId}`); + console.log(` stale in-memory head=${pad.getHeadRevisionNumber()}, ` + + `persisted head=${recAfter.head}`); + + // The caller still holds the old Pad object. One more edit through it: + try { await pad.appendText('later edit\n'); } catch { /* may throw */ } + + padManager.unloadPad(padId); + const state = await missingRevs(padId); + console.log(` final: head=${state.head} missing=[${state.missing}]`); + assert.ok(state.missing.length > 0, 'expected holes'); + }); +});