From 44176239eecfbb2eef8813e3065884190968ea31 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 15 Aug 2026 12:08:05 +0100 Subject: [PATCH 1/2] fix: don't let a failed revision write leave a hole in pad history appendRevision() writes the revision record and the pad record (which carries `head`) as two independent writes in one Promise.all. When the revision write failed and the pad record landed, the pad claimed a revision that was never stored. The next successful append then wrote head+1 straight over it, so the gap became permanent, and every later pad.check() tripped on the missing revision -- which blocks cleanup and compaction forever, with no way back. That is what #8134 reports: revisions 599 and 601 present, 600 absent, "Cleanup revisions" refusing to run. Revision 600 is a key revision (`Math.floor(rev/100)*100`), so it embeds the whole attribute pool and atext -- by far the largest record written, and the one most likely to exceed max_allowed_packet or time out. Keep the two writes concurrent (sequencing them would add a write round-trip to every commit on the editing hot path) and instead roll the in-memory head and atext back on failure, then re-persist the pad record, so the pad never points past its own history. If the rollback write also fails we log loudly rather than leaving a silent hole for an admin to discover months later via a failed cleanup run. Hook and author-index calls move out of the storage Promise.all so a throwing padUpdate hook cannot roll back a revision that was stored successfully. They still run concurrently with the writes. 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. Fixes #8134 Co-Authored-By: Claude Opus 5 (1M context) --- src/node/db/Pad.ts | 66 ++++++- .../backend/specs/appendRevisionAtomicity.ts | 186 ++++++++++++++++++ 2 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 src/tests/backend/specs/appendRevisionAtomicity.ts diff --git a/src/node/db/Pad.ts b/src/node/db/Pad.ts index dba21da275e..204f1fa2493 100644 --- a/src/node/db/Pad.ts +++ b/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 diff --git a/src/tests/backend/specs/appendRevisionAtomicity.ts b/src/tests/backend/specs/appendRevisionAtomicity.ts new file mode 100644 index 00000000000..395f8ec2793 --- /dev/null +++ b/src/tests/backend/specs/appendRevisionAtomicity.ts @@ -0,0 +1,186 @@ +'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) => { + 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'); + assert.equal(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.notEqual(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:` 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.notEqual(await db.get(`pad:${padId}:revs:${goodHead + 1}`), null); + + padManager.unloadPad(padId); + await (await padManager.getPad(padId)).check(); + }); + }); +}); From 5b62d08ec8694c81dc731ff02c6b1e4b209863fc Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 15 Aug 2026 14:17:45 +0100 Subject: [PATCH 2/2] test: don't assume which nullish value an absent key returns The Windows backend-test jobs failed on AssertionError: the failed revision should not exist + actual: undefined - expected: null `assert.strict.equal(rec, null)` distinguishes null from undefined, and the storage driver yields null for an absent key on Linux but undefined on Windows. The assertion, not the behaviour, was platform-specific -- the revision is absent either way, and the code under test compares with `== null` throughout. Assert nullish-ness instead. Co-Authored-By: Claude Opus 5 (1M context) --- src/tests/backend/specs/appendRevisionAtomicity.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/tests/backend/specs/appendRevisionAtomicity.ts b/src/tests/backend/specs/appendRevisionAtomicity.ts index 395f8ec2793..a32b0e1eede 100644 --- a/src/tests/backend/specs/appendRevisionAtomicity.ts +++ b/src/tests/backend/specs/appendRevisionAtomicity.ts @@ -69,7 +69,10 @@ describe(__filename, function () { const padRecord = await db.get(`pad:${padId}`); assert.equal(padRecord.head, goodHead, 'persisted head should be rolled back'); - assert.equal(await db.get(`pad:${padId}:revs:${doomed}`), null, + // `== 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'); }); @@ -101,7 +104,7 @@ describe(__filename, function () { // The next append reuses the revision number rather than skipping it. await pad.appendText('three\n'); assert.equal(pad.getHeadRevisionNumber(), doomed); - assert.notEqual(await db.get(`pad:${padId}:revs:${doomed}`), null); + assert.ok(await db.get(`pad:${padId}:revs:${doomed}`) != null); }); it('leaves the pad passing check()', async function () { @@ -177,7 +180,7 @@ describe(__filename, function () { assert.equal(pad.getHeadRevisionNumber(), goodHead + 1, 'the revision was stored, so head must stand'); - assert.notEqual(await db.get(`pad:${padId}:revs:${goodHead + 1}`), null); + assert.ok(await db.get(`pad:${padId}:revs:${goodHead + 1}`) != null); padManager.unloadPad(padId); await (await padManager.getPad(padId)).check();