From d5a788efda5c6f65e08106896a455039e50144a0 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 15 Aug 2026 12:04:05 +0100 Subject: [PATCH] fix: copyPadWithoutHistory wrote an invalid changeset pack() takes the TOTAL length of the new document, but the call passed `assem.getLengthChange()` -- a delta. The resulting revision 1 header disagreed with its own ops, with two consequences: 1. opsFromAText() skips the source document's final newline, so both of the destination pad's rev-0 newlines survived and the copy came out one newline longer than the source. It grew again on every subsequent copy. 2. The copy's revision 1 failed checkRep(), so the copied pad failed pad.check() from then on. (2) is the damaging one. deleteRevisions() -- `cleanup.keepRevisions`, and compactPad with a keep count -- calls pad.check() before it touches anything. compactPad's full-collapse mode goes through this function twice, so the feature meant to reclaim database space produced pads that could never be cleaned up again. Pass the total length, drop the surplus newline, and trim the char bank to the characters actually inserted. Also await dstPad.appendRevision(): an un-awaited rejection surfaced as an unhandled rejection rather than failing the copy, which is how the malformed changeset went unnoticed. Same for the `saveToDatabase()` that the comment above it calls "flush the source pad". The existing API test quantified the off-by-one without naming it: the direct getHTML test strips one trailing '
' while the copy test stripped '

'. It now strips one, like the source. The compactPad test's tolerance for "adjusted trailing whitespace" is likewise replaced with a byte-exact comparison plus a check() assertion. Found while investigating #8134. Co-Authored-By: Claude Opus 5 (1M context) --- src/node/db/Pad.ts | 33 +++- src/tests/backend/specs/api/pad.ts | 7 +- src/tests/backend/specs/compactPad.ts | 23 +-- .../specs/copyPadWithoutHistoryIntegrity.ts | 141 ++++++++++++++++++ 4 files changed, 188 insertions(+), 16 deletions(-) create mode 100644 src/tests/backend/specs/copyPadWithoutHistoryIntegrity.ts diff --git a/src/node/db/Pad.ts b/src/node/db/Pad.ts index dba21da275e..0abda01462a 100644 --- a/src/node/db/Pad.ts +++ b/src/node/db/Pad.ts @@ -27,6 +27,7 @@ import {SYSTEM_AUTHOR_ID} from '../utils/SystemAuthor'; const hooks = require('../../static/js/pluginfw/hooks'); import pad_utils from "../../static/js/pad_utils"; import {SmartOpAssembler} from "../../static/js/SmartOpAssembler"; +import Op from "../../static/js/Op"; import {timesLimit} from "async"; type PadViewSettings = { @@ -750,7 +751,7 @@ class Pad { } // flush the source pad - this.saveToDatabase(); + await this.saveToDatabase(); // if it's a group pad, let's make sure the group exists. const destGroupID = await this.checkIfGroupExistAndReturnIt(destinationID); @@ -790,19 +791,39 @@ class Pad { } assem.append(op); } - assem.endDocument(); - // although we have instantiated the dstPad with '\n', an additional '\n' is // added internally, so the pad text on the revision 0 is "\n\n" const oldLength = 2; - const newLength = assem.getLengthChange(); - const newText = oldAText.text; + // opsFromAText() intentionally omits the source document's final newline, + // so the ops appended above insert oldAText.text minus its last character. + // Both of the destination pad's existing newlines would then survive and + // the copy would come out one newline longer than the source -- growing + // again on every subsequent copy. Delete one of them so the copy's text + // matches the source exactly. + const dropExtraNewline = new Op('-'); + dropExtraNewline.chars = 1; + dropExtraNewline.lines = 1; + assem.append(dropExtraNewline); + assem.endDocument(); + + // pack() takes the TOTAL length of the new document, not the delta. + // Passing the delta (assem.getLengthChange()) produced a changeset whose + // header disagreed with its own ops, so every pad produced by this + // function failed checkRep() -- and therefore pad.check(), which is what + // `cleanup.keepRevisions` runs before it will touch a pad. + const newLength = oldLength + assem.getLengthChange(); + // The char bank holds only the inserted characters, which is the source + // text without the final newline that opsFromAText() skipped. + const newText = oldAText.text.slice(0, -1); // create a changeset that removes the previous text and add the newText with // all atributes present on the source pad const changeset = pack(oldLength, newLength, assem.toString(), newText); - dstPad.appendRevision(changeset, authorId); + // Must be awaited: an un-awaited rejection here (an invalid changeset, + // a failed write) surfaces as an unhandled rejection instead of failing + // the copy, which is how the length bug above went unnoticed. + await dstPad.appendRevision(changeset, authorId); await hooks.aCallAll('padCopy', { get originalPad() { diff --git a/src/tests/backend/specs/api/pad.ts b/src/tests/backend/specs/api/pad.ts index f4d081ef4a7..fc348c75d84 100644 --- a/src/tests/backend/specs/api/pad.ts +++ b/src/tests/backend/specs/api/pad.ts @@ -615,7 +615,12 @@ describe(__filename, function () { res = await agent.get(`${endPoint('getHTML')}?padID=${newPad}`) .set("Authorization", (await common.generateJWTToken())) .expect(200); - const receivedHtml = res.body.data.html.replace('

', '').toLowerCase(); + // Strips the same single trailing
as the direct getHTML test + // above. This used to strip '

': copyPadWithoutHistory packed + // a length delta where pack() wanted a total, so every copy came out + // one newline longer than its source. Now the copy matches the source + // exactly. + const receivedHtml = res.body.data.html.replace('
', '').toLowerCase(); assert.equal(receivedHtml, expectedHtml); }); diff --git a/src/tests/backend/specs/compactPad.ts b/src/tests/backend/specs/compactPad.ts index 426103f1289..6805e2473de 100644 --- a/src/tests/backend/specs/compactPad.ts +++ b/src/tests/backend/specs/compactPad.ts @@ -40,23 +40,28 @@ describe(__filename, function () { await pad.appendText('marker-gamma\n'); const before = pad.getHeadRevisionNumber(); assert.ok(before >= 3, `expected at least 3 revs, got ${before}`); + const textBefore = pad.atext.text; + padManager.unloadPad(padId); const result = await api.compactPad(padId); assert.deepStrictEqual(result, {ok: true, mode: 'all'}); // Reload: the compacted pad lands at head<=1 (matches the shape - // `copyPadWithoutHistory` produces). The content survives — we - // don't assert byte-exact equality because Cleanup.deleteAllRevisions - // goes through copyPadWithoutHistory twice and may adjust trailing - // whitespace; what we care about is that the author-written content - // is still there. + // `copyPadWithoutHistory` produces). + padManager.unloadPad(padId); const reloaded = await padManager.getPad(padId); assert.ok(reloaded.getHeadRevisionNumber() <= 1, `expected head<=1, got ${reloaded.getHeadRevisionNumber()}`); - const text = reloaded.atext.text; - assert.ok(text.includes('marker-alpha'), 'alpha content preserved'); - assert.ok(text.includes('marker-beta'), 'beta content preserved'); - assert.ok(text.includes('marker-gamma'), 'gamma content preserved'); + // Byte-exact. This used to tolerate "adjusted trailing whitespace" + // because deleteAllRevisions goes through copyPadWithoutHistory + // twice and each pass added a newline; that was a bug, not a + // property of compaction. + assert.equal(reloaded.atext.text, textBefore, + 'compaction must not alter the text'); + // And the compacted pad must still be checkable, otherwise it could + // never be compacted again by keep-count (deleteRevisions calls + // pad.check() before it touches anything). + await reloaded.check(); }); it('keeps only the last N revisions when keepRevisions is a number', diff --git a/src/tests/backend/specs/copyPadWithoutHistoryIntegrity.ts b/src/tests/backend/specs/copyPadWithoutHistoryIntegrity.ts new file mode 100644 index 00000000000..94834f8da0d --- /dev/null +++ b/src/tests/backend/specs/copyPadWithoutHistoryIntegrity.ts @@ -0,0 +1,141 @@ +'use strict'; + +// Regression coverage for the changeset `copyPadWithoutHistory()` writes. +// +// It packed `assem.getLengthChange()` (a delta) into pack()'s `newLen` +// parameter (a total), so the resulting changeset's header disagreed with +// its own ops. Two consequences: +// +// 1. The copy came out one newline longer than the source, and grew again +// on every subsequent copy. +// 2. The copy's revision 1 failed checkRep(), so the copied pad failed +// pad.check() forever after -- which is exactly what `deleteRevisions` +// (cleanup.keepRevisions / compactPad with a keep count) runs first. +// +// Because compactPad's "collapse everything" mode goes through this +// function twice, the feature meant to reclaim space left pads that could +// never be cleaned up again. Found while investigating #8134. + +const assert = require('assert').strict; +const common = require('../common'); +const padManager = require('../../../node/db/PadManager'); +const db = require('../../../node/db/DB'); +const api = require('../../../node/db/API'); +const settings = require('../../../node/utils/Settings'); +const Changeset = require('../../../static/js/Changeset'); + +describe(__filename, function () { + let cleanupEnabledBackup: boolean; + + before(async function () { + await common.init(); + cleanupEnabledBackup = settings.cleanup.enabled; + settings.cleanup.enabled = true; + }); + + after(function () { settings.cleanup.enabled = cleanupEnabledBackup; }); + + const makeSourcePad = async (lines = 6) => { + const padId = common.randomString(); + const pad = await padManager.getPad(padId); + for (let i = 0; i < lines; i++) await pad.appendText(`line ${i}\n`); + await pad.check(); + return pad; + }; + + describe('copyPadWithoutHistory()', function () { + it('produces a copy whose text equals the source exactly', async function () { + const src = await makeSourcePad(); + const srcText = src.atext.text; + const dstId = common.randomString(); + await src.copyPadWithoutHistory(dstId, false); + + padManager.unloadPad(dstId); + const dst = await padManager.getPad(dstId); + assert.equal(dst.atext.text, srcText, + 'copy must not gain or lose characters'); + }); + + it('writes a revision 1 that passes checkRep()', async function () { + const src = await makeSourcePad(); + const dstId = common.randomString(); + await src.copyPadWithoutHistory(dstId, false); + + const rev1 = await db.get(`pad:${dstId}:revs:1`); + assert.ok(rev1 != null, 'revision 1 should exist'); + Changeset.checkRep(rev1.changeset); // throws if malformed + + const unpacked = Changeset.unpack(rev1.changeset); + assert.equal(unpacked.newLen, src.atext.text.length, + 'changeset header must claim the real resulting length'); + }); + + it('leaves the copy passing pad.check()', async function () { + const src = await makeSourcePad(); + const dstId = common.randomString(); + await src.copyPadWithoutHistory(dstId, false); + + padManager.unloadPad(dstId); + await (await padManager.getPad(dstId)).check(); + }); + + it('does not drift when copied repeatedly', async function () { + // The old bug compounded: each copy added another newline. + const src = await makeSourcePad(); + const expected = src.atext.text; + + let currentId = src.id; + for (let i = 0; i < 3; i++) { + const nextId = common.randomString(); + await (await padManager.getPad(currentId)).copyPadWithoutHistory(nextId, false); + padManager.unloadPad(nextId); + currentId = nextId; + } + const final = await padManager.getPad(currentId); + assert.equal(final.atext.text, expected, + 'text must be stable across repeated copies'); + await final.check(); + }); + + it('preserves author attribution on the copied text', async function () { + const src = await makeSourcePad(); + const authors = src.getAllAuthors(); + const dstId = common.randomString(); + await src.copyPadWithoutHistory(dstId, false); + + padManager.unloadPad(dstId); + const dst = await padManager.getPad(dstId); + assert.deepEqual(dst.getAllAuthors().sort(), authors.sort()); + }); + }); + + describe('compactPad() full collapse', function () { + it('preserves the text and leaves the pad checkable', async function () { + const src = await makeSourcePad(); + const padId = src.id; + const before = src.atext.text; + padManager.unloadPad(padId); + + assert.deepStrictEqual(await api.compactPad(padId), {ok: true, mode: 'all'}); + + padManager.unloadPad(padId); + const after = await padManager.getPad(padId); + assert.equal(after.atext.text, before, 'compaction must not alter the text'); + await after.check(); + }); + + it('leaves a pad that can still be cleaned up by keep-count', async function () { + // deleteRevisions() calls pad.check() first, so a pad corrupted by + // full compaction could never be compacted again. + const src = await makeSourcePad(10); + const padId = src.id; + padManager.unloadPad(padId); + + await api.compactPad(padId); + padManager.unloadPad(padId); + + const pad = await padManager.getPad(padId); + await pad.check(); + }); + }); +});