fix: don't let a failed revision write leave a hole in pad history

src/node/db/Pad.ts

@@ -28,6 +28,9 @@ const hooks = require('../../static/js/pluginfw/hooks');
 import pad_utils from "../../static/js/pad_utils";
 import {SmartOpAssembler} from "../../static/js/SmartOpAssembler";
 import {timesLimit} from "async";
+import log4js from 'log4js';
+
+const logger = log4js.getLogger('pad');
 
 type PadViewSettings = {
   showAuthorColors: boolean;
@@ -295,6 +298,9 @@ class Pad {
         this.head !== -1) {
       return this.head;
     }
+    // Snapshot for the rollback below, taken before this.atext is mutated.
+    const prevHead = this.head;
+    const prevAText: AText = {text: this.atext.text, attribs: this.atext.attribs};
     copyAText(newAText, this.atext);
 
     const newRev = ++this.head;
@@ -303,7 +309,19 @@ class Pad {
     if (authorId !== '') this.pool.putAttrib(['author', authorId]);
 
     const hook = this.head === 0 ? 'padCreate' : 'padUpdate';
-    await Promise.all([
+
+    // The revision record and the pad record (which carries `head`) are two
+    // independent writes. If the revision write fails while the pad record
+    // lands, the pad claims a revision that was never stored -- and because
+    // the next successful append writes head+1 straight over it, the gap is
+    // permanent. Any later pad.check() then trips on the missing revision,
+    // which blocks cleanup/compaction forever. See #8134.
+    //
+    // They stay concurrent (sequencing them would add a write round-trip to
+    // every commit on the editing hot path); instead a failure rolls the
+    // in-memory state back and re-persists the pad record, so the pad never
+    // ends up pointing past its own history.
+    const storageWrites = Promise.all([
       // @ts-ignore
       this.db.set(`pad:${this.id}:revs:${newRev}`, {
         changeset: aChangeset,
@@ -317,6 +335,12 @@ class Pad {
         },
       }),
       this.saveToDatabase(),
+    ]);
+
+    // Kept separate from the storage writes: a throwing padUpdate hook (or a
+    // failed author-index update) must not roll back a revision that was
+    // stored successfully. Started here so it still runs concurrently.
+    const sideEffects = Promise.all([
       authorId && authorManager.addPad(authorId, this.id),
       hooks.aCallAll(hook, {
         pad: this,
@@ -336,9 +360,49 @@ class Pad {
         },
       }),
     ]);
+    // Awaited below. Attach a no-op handler so a rejection while we're
+    // awaiting the storage writes isn't reported as unhandled.
+    sideEffects.catch(() => {});
+
+    try {
+      await storageWrites;
+    } catch (err) {
+      await this._rollbackFailedRevision(newRev, prevHead, prevAText);
+      throw err;
+    }
+
+    await sideEffects;
     return newRev;
   }
 
+  /**
+   * Undoes the in-memory effects of a failed appendRevision and re-persists
+   * the pad record, so `head` never points at a revision that isn't stored.
+   *
+   * The attribute pool is deliberately not rolled back: pool entries are
+   * addressed by position, so removing one would invalidate the attribute
+   * numbers in every changeset already written. A pool author with no
+   * revisions is harmless -- pad.check() derives both sides of its author
+   * comparison from the pool, so they still agree.
+   */
+  private async _rollbackFailedRevision(newRev: number, prevHead: number, prevAText: AText) {
+    this.head = prevHead;
+    copyAText(prevAText, this.atext);
+    try {
+      await this.saveToDatabase();
+    } catch (rollbackErr: any) {
+      // Both writes failed. The pad record may still claim `newRev`, which
+      // is the pre-#8134 behaviour; say so loudly rather than silently
+      // leaving a hole for an admin to find months later via a failed
+      // cleanup run.
+      logger.error(
+          `pad ${this.id}: revision ${newRev} failed to store AND the ` +
+          `rollback of head to ${prevHead} failed. The pad record may claim ` +
+          `a revision that does not exist; run a consistency check on it. ` +
+          `Rollback error: ${rollbackErr.stack || rollbackErr}`);
+    }
+  }
+
   toJSON() {
     const o:Pad = {...this, pool: this.pool.toJsonable()};
     // @ts-ignore

src/tests/backend/specs/appendRevisionAtomicity.ts

@@ -0,0 +1,189 @@
+'use strict';
+
+// Regression coverage for #8134.
+//
+// appendRevision() writes the revision record and the pad record (which
+// carries `head`) as two independent writes. When the revision write failed
+// and the pad record landed, the pad claimed a revision that was never
+// stored -- and the next successful append wrote head+1 straight over it,
+// making the gap permanent. Every later pad.check() then tripped on the
+// missing revision, which blocks cleanup/compaction forever.
+//
+// The reporter on #8134 hit exactly this: revisions 599 and 601 present,
+// 600 absent, cleanup refusing to run.
+
+const assert = require('assert').strict;
+const common = require('../common');
+const padManager = require('../../../node/db/PadManager');
+const db = require('../../../node/db/DB');
+
+describe(__filename, function () {
+  let padId: string;
+
+  before(async function () { await common.init(); });
+
+  beforeEach(async function () {
+    padId = common.randomString();
+    assert(!await padManager.doesPadExist(padId));
+  });
+
+  // Runs `fn` with the write to `failKey` rejecting.
+  const withFailingWrite = async (failKey: string, fn: () => Promise<any>) => {
+    const realSet = db.set;
+    db.set = async (key: string, value: unknown) => {
+      if (key === failKey) throw new Error('simulated backend write failure');
+      return await realSet(key, value);
+    };
+    try {
+      return await fn();
+    } finally {
+      db.set = realSet;
+    }
+  };
+
+  describe('when the revision write fails', function () {
+    it('rejects', async function () {
+      const pad = await padManager.getPad(padId);
+      await pad.appendText('one\n');
+      const doomed = pad.getHeadRevisionNumber() + 1;
+      await withFailingWrite(`pad:${padId}:revs:${doomed}`, async () => {
+        await assert.rejects(pad.appendText('two\n'),
+            /simulated backend write failure/);
+      });
+    });
+
+    it('does not leave head pointing past the stored history', async function () {
+      const pad = await padManager.getPad(padId);
+      await pad.appendText('one\n');
+      const goodHead = pad.getHeadRevisionNumber();
+      const doomed = goodHead + 1;
+
+      await withFailingWrite(`pad:${padId}:revs:${doomed}`, async () => {
+        await assert.rejects(pad.appendText('two\n'));
+      });
+
+      assert.equal(pad.getHeadRevisionNumber(), goodHead,
+          'in-memory head should be rolled back');
+
+      padManager.unloadPad(padId);
+      const padRecord = await db.get(`pad:${padId}`);
+      assert.equal(padRecord.head, goodHead,
+          'persisted head should be rolled back');
+      // `== null`, not `assert.equal(..., null)`: the dirty/rusty driver
+      // yields null for an absent key on Linux but undefined on Windows.
+      // Either way the record is not there.
+      assert.ok(await db.get(`pad:${padId}:revs:${doomed}`) == null,
+          'the failed revision should not exist');
+    });
+
+    it('rolls the in-memory text back too', async function () {
+      const pad = await padManager.getPad(padId);
+      await pad.appendText('one\n');
+      const textBefore = pad.atext.text;
+      const doomed = pad.getHeadRevisionNumber() + 1;
+
+      await withFailingWrite(`pad:${padId}:revs:${doomed}`, async () => {
+        await assert.rejects(pad.appendText('two\n'));
+      });
+
+      assert.equal(pad.atext.text, textBefore,
+          'atext must not keep changes that were never stored');
+      assert.ok(!pad.atext.text.includes('two'));
+    });
+
+    it('leaves the pad consistent for a later append', async function () {
+      const pad = await padManager.getPad(padId);
+      await pad.appendText('one\n');
+      const goodHead = pad.getHeadRevisionNumber();
+      const doomed = goodHead + 1;
+
+      await withFailingWrite(`pad:${padId}:revs:${doomed}`, async () => {
+        await assert.rejects(pad.appendText('two\n'));
+      });
+
+      // The next append reuses the revision number rather than skipping it.
+      await pad.appendText('three\n');
+      assert.equal(pad.getHeadRevisionNumber(), doomed);
+      assert.ok(await db.get(`pad:${padId}:revs:${doomed}`) != null);
+    });
+
+    it('leaves the pad passing check()', async function () {
+      // The whole point: a failed write must not make the pad
+      // permanently uncleanable.
+      const pad = await padManager.getPad(padId);
+      for (let i = 0; i < 3; i++) await pad.appendText(`line ${i}\n`);
+      const doomed = pad.getHeadRevisionNumber() + 1;
+
+      await withFailingWrite(`pad:${padId}:revs:${doomed}`, async () => {
+        await assert.rejects(pad.appendText('doomed\n'));
+      });
+      await pad.appendText('after\n');
+
+      padManager.unloadPad(padId);
+      await (await padManager.getPad(padId)).check();
+    });
+  });
+
+  describe('when the pad record write fails', function () {
+    it('rejects and rolls back without orphaning head', async function () {
+      const pad = await padManager.getPad(padId);
+      await pad.appendText('one\n');
+      const goodHead = pad.getHeadRevisionNumber();
+
+      // The rollback re-saves the pad record, so let only the first
+      // `pad:<id>` write fail.
+      const realSet = db.set;
+      let failed = false;
+      db.set = async (key: string, value: unknown) => {
+        if (key === `pad:${padId}` && !failed) {
+          failed = true;
+          throw new Error('simulated pad record write failure');
+        }
+        return await realSet(key, value);
+      };
+      try {
+        await assert.rejects(pad.appendText('two\n'));
+      } finally {
+        db.set = realSet;
+      }
+
+      assert.equal(pad.getHeadRevisionNumber(), goodHead);
+      padManager.unloadPad(padId);
+      const padRecord = await db.get(`pad:${padId}`);
+      assert.equal(padRecord.head, goodHead);
+      // A stored-but-unreferenced revision record is harmless: check()
+      // only walks 0..head.
+      await (await padManager.getPad(padId)).check();
+    });
+  });
+
+  describe('side effects', function () {
+    it('a throwing padUpdate hook does not roll back a stored revision',
+        async function () {
+          // Hook failures are not storage failures. Rolling back here would
+          // discard a revision that was written successfully.
+          const hooks = require('../../../static/js/pluginfw/hooks');
+          const pad = await padManager.getPad(padId);
+          await pad.appendText('one\n');
+          const goodHead = pad.getHeadRevisionNumber();
+
+          const realACallAll = hooks.aCallAll;
+          hooks.aCallAll = async (hookName: string, ...rest: any[]) => {
+            if (hookName === 'padUpdate') throw new Error('plugin blew up');
+            return await realACallAll(hookName, ...rest);
+          };
+          try {
+            await assert.rejects(pad.appendText('two\n'), /plugin blew up/);
+          } finally {
+            hooks.aCallAll = realACallAll;
+          }
+
+          assert.equal(pad.getHeadRevisionNumber(), goodHead + 1,
+              'the revision was stored, so head must stand');
+          assert.ok(await db.get(`pad:${padId}:revs:${goodHead + 1}`) != null);
+
+          padManager.unloadPad(padId);
+          await (await padManager.getPad(padId)).check();
+        });
+  });
+});