feat: report a damaged pad history instead of asserting

deleteRevisions() went straight into pad.check(), which replays the
whole history and dies on

  AssertionError: The expression evaluated to a falsy value:
    assert(timestamp != null)

That is an assertion about a null timestamp, when what an operator needs
to hear is which revision is missing and what to do about it. The
reporter on #8134 had to bisect their database by hand to find it.

Add Pad.findMissingRevisions(), a cheap scan of 0..head for revisions
that are absent or carry no meta.timestamp -- it reads one sub-field per
revision and replays nothing -- and run it before check() so the clear
error wins. Cleanup now fails with:

  Pad eu-demopad is missing revision(s) 6. 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.

The admin UI already renders err.toString(), so this reaches the
operator with no UI change.

Full compaction is deliberately NOT gated on the same check: it does not
replay history, it rebuilds from the current text, so it is the recovery
path the message points at.

Refs #8134

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: JohnMcLear

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