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..a32b0e1eede --- /dev/null +++ b/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) => { + 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:` 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(); + }); + }); +});