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