feat: report a damaged pad history instead of asserting

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<number[]> {
+    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.
    */

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')

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');
+    });
+  });
+});

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');
+      });
+});