fix: copyPadWithoutHistory wrote an invalid changeset

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() {

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('<br><br></body>', '</body>').toLowerCase();
+      // Strips the same single trailing <br> as the direct getHTML test
+      // above. This used to strip '<br><br>': 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('<br></body>', '</body>').toLowerCase();
       assert.equal(receivedHtml, expectedHtml);
     });
 

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',

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