fix: copyPadWithoutHistory wrote an invalid changeset

[JohnMcLear]

Fixes #8139. Found while investigating #8134.

Problem

copyPadWithoutHistory() packed a length delta into pack()'s newLen parameter, which wants a total:

const oldLength = 2;
const newLength = assem.getLengthChange();   // delta, not total
const changeset = pack(oldLength, newLength, assem.toString(), oldAText.text);

The resulting revision 1 header disagrees with its own ops. Two consequences:

1. Copies grow a newline. opsFromAText() skips the source document's final newline, so both of the destination pad's rev-0 newlines survive alongside the inserted text. A 242-char source produced a 243-char copy, growing again on every subsequent copy.
2. The copy fails pad.check() forever. Revision 1 fails checkRep() with claimed length does not match actual length.

(2) is the damaging one: deleteRevisions() — cleanup.keepRevisions from the admin UI, and compactPad with a keep count — calls pad.check() before touching anything, and compactPad's full-collapse mode routes through this function twice. The feature that exists to reclaim database space left pads that could never be cleaned up again.

Fix

• Pass oldLength + assem.getLengthChange() as the total new length.
• Append a 1-char/1-line - op to drop the surplus newline so the copy matches the source exactly.
• Trim the char bank to the characters actually inserted (oldAText.text.slice(0, -1)), since opsFromAText() omits the final newline.
• await dstPad.appendRevision(...). Un-awaited, a rejection became an unhandled rejection instead of failing the copy — which is how the malformed changeset survived this long. Same for the saveToDatabase() whose own comment calls it "flush the source pad".

Tests

New src/tests/backend/specs/copyPadWithoutHistoryIntegrity.ts: text equality with the source, checkRep() on revision 1, pad.check() on the copy, stability across three chained copies, author attribution, and both compactPad full-collapse properties (text preserved, and the pad remains cleanable afterwards). 6 of its 7 cases fail without the Pad.ts change.

Two existing tests encoded the bug rather than catching it, and are corrected here:

• tests/backend/specs/api/pad.ts stripped '<br></body>' when checking a pad's own HTML but '<br><br></body>' when checking a copy of it — the off-by-one, quantified. Now strips one, like the source.
• tests/backend/specs/compactPad.ts declined to "assert byte-exact equality because Cleanup.deleteAllRevisions goes through copyPadWithoutHistory twice and may adjust trailing whitespace", and never called check() afterwards. Now byte-exact, plus a check() assertion.

Full backend suite: 1628 passing, 0 failing.

Note on existing data

This stops new pads being corrupted; it does not repair pads already copied or compacted by the old code. Those still fail pad.check() and so still refuse keep-count cleanup. Worth a follow-up if we want a repair path — happy to take that on separately.

🤖 Generated with Claude Code

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix copyPadWithoutHistory changeset length, trailing newline drift, and async writes

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Fix copyPadWithoutHistory() to pack a total new length (not a delta) and generate a valid
 changeset.
• Prevent copies from gaining an extra newline by explicitly deleting the surplus newline.
• Harden copy/compaction integrity via new regression tests and stricter existing assertions.

Diagram

graph TD
  A["API/Cleanup (compactPad)"] --> B["Pad.copyPadWithoutHistory"] --> C["Ops + SmartOpAssembler"] --> D["Changeset.pack"] --> E["dstPad.appendRevision"] --> F[("ueberDB / pad revs")]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Rebuild destination from full AText snapshot

• ➕ Avoids manual newline bookkeeping by constructing a canonical AText and deriving changeset from that snapshot.
• ➕ Potentially simpler invariants: destination text length always derives from AText length.
• ➖ Still needs a correct changeset to persist rev1; requires careful integration with existing revision/pool expectations.
• ➖ May be more invasive than a focused fix and risk subtle attribute/author replay differences.

2. Initialize destination pad with a single newline (instead of internal "\n\n")

• ➕ Could remove the need for an explicit '-/1char/1line' op in the copy path.
• ➕ Makes copy semantics more intuitive (less special casing of rev-0).
• ➖ Likely affects broader pad initialization assumptions and may have compatibility impact across code paths.
• ➖ Higher blast radius than correcting the changeset header + trimming the char bank.

3. Add a repair/rehydration tool for already-corrupted pads

• ➕ Provides an operational path to fix historical data so keep-count cleanup and compaction work again.
• ➕ Can be run as a targeted maintenance job rather than requiring manual intervention.
• ➖ Additional scope and operational risk; needs careful detection and auditing to avoid data loss.
• ➖ Not necessary to stop new corruption; better as a follow-up PR.

Recommendation: Keep the PR’s surgical approach (correct total newLen, trim char bank, delete the surplus newline, and await async writes). It fixes the root correctness issue with minimal behavioral surface area, and the added integrity tests meaningfully reduce regression risk. Consider a separate follow-up for repairing already-corrupted pads.

Files changed (4) +188 / -16

Bug fix (1) +27 / -6

Pad.tsFix changeset packing, newline drift, and async durability in copyPadWithoutHistory() +27/-6

Fix changeset packing, newline drift, and async durability in copyPadWithoutHistory()

• Corrects copyPadWithoutHistory() to pass pack() the total new document length (oldLength + delta) instead of the delta. Adds an explicit delete op to remove the extra newline that otherwise accumulates due to opsFromAText() omitting the final newline, and trims the char bank accordingly. Also awaits saveToDatabase() and dstPad.appendRevision() to ensure failures surface as copy errors rather than unhandled rejections.

src/node/db/Pad.ts

Tests (3) +161 / -10

pad.tsAlign copyPadWithoutHistory HTML assertion with fixed newline behavior +6/-1

Align copyPadWithoutHistory HTML assertion with fixed newline behavior

• Updates the API test to strip a single trailing <br> (matching the direct getHTML test) now that pad copies no longer gain an extra newline. Adds inline commentary documenting the historical bug and why the expectation changed.

src/tests/backend/specs/api/pad.ts

compactPad.tsMake compactPad full-collapse test byte-exact and assert pad.check() +14/-9

Make compactPad full-collapse test byte-exact and assert pad.check()

• Strengthens the full-compaction test to require byte-exact text preservation instead of tolerating trailing whitespace drift caused by repeated copyPadWithoutHistory(). Adds an explicit post-compaction pad.check() to ensure the compacted pad remains valid and cleanable in future keep-count runs.

src/tests/backend/specs/compactPad.ts

copyPadWithoutHistoryIntegrity.tsAdd regression tests for copyPadWithoutHistory changeset integrity and compaction safety +141/-0

Add regression tests for copyPadWithoutHistory changeset integrity and compaction safety

• Introduces a focused backend spec verifying copied text equality, Changeset.checkRep() for revision 1, pad.check() on copies, stability across chained copies, and author attribution preservation. Also covers compactPad full-collapse invariants (text preserved and pad remains checkable/cleanable).

src/tests/backend/specs/copyPadWithoutHistoryIntegrity.ts

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

1. No cleanup on failure 🐞 Bug ☼ Reliability

Description

If dstPad.appendRevision() rejects, copyPadWithoutHistory() throws after creating the destination
pad and writing author/group metadata, leaving a partially-created destination pad behind. A retry
with force=false will then fail early with “destinationID already exists”.

Code

src/node/db/Pad.ts[R823-826]

+    // 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);

Evidence

The destination pad and related metadata are created/updated before the awaited appendRevision().
If appendRevision() throws, the function exits early with those side effects still present; on
retry, the existing-pad check will throw unless force=true.

src/node/db/Pad.ts[754-767]
src/node/db/Pad.ts[769-827]
src/node/db/Pad.ts[717-737]
src/node/db/Pad.ts[286-340]
src/node/db/PadManager.ts[109-151]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`copyPadWithoutHistory()` now correctly `await`s `dstPad.appendRevision(...)`, which means failures (db write, hook error, invariant error) will reject the copy. However, the function has already created/persisted the destination pad and updated related metadata (authors + group pad list). If `appendRevision()` fails, there is no rollback, so an incomplete destination pad can remain and a subsequent retry with `force=false` will fail with `destinationID already exists`.

### Issue Context
The destination pad is created via `padManager.getPad(...)` (which initializes and persists rev0). Then author/group side effects are applied before the awaited `appendRevision()`.

### Fix Focus Areas
- src/node/db/Pad.ts[746-843]
- src/node/db/Pad.ts[717-737]

### Suggested fix
- Wrap the destination-pad creation + revision append in a `try { ... } catch (err) { ... }`.
- In the `catch` block, best-effort remove the destination pad that was just created (for example `await dstPad.remove()`), then rethrow the original error.
- Consider moving `copyAuthorInfoToDestinationPad()` and the `group:${destGroupID}.pads` update to *after* the revision append succeeds, to reduce rollback work.
- Add a regression test that forces `appendRevision()` to throw (stub/hook) and asserts the destination pad does not exist afterward (or is retryable without `force=true`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context

✅ Tickets: 🎫 "Cleanup revisions" fails due to missing revision 🎫 copyPadWithoutHistory writes an invalid changeset: copies grow a newline and fail pad.check() forever

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-free-for-open-source-projects]

1. No cleanup on failure 🐞 Bug ☼ Reliability

If dstPad.appendRevision() rejects, copyPadWithoutHistory() throws after creating the destination
pad and writing author/group metadata, leaving a partially-created destination pad behind. A retry
with force=false will then fail early with “destinationID already exists”.

Agent Prompt

### Issue description
`copyPadWithoutHistory()` now correctly `await`s `dstPad.appendRevision(...)`, which means failures (db write, hook error, invariant error) will reject the copy. However, the function has already created/persisted the destination pad and updated related metadata (authors + group pad list). If `appendRevision()` fails, there is no rollback, so an incomplete destination pad can remain and a subsequent retry with `force=false` will fail with `destinationID already exists`.

### Issue Context
The destination pad is created via `padManager.getPad(...)` (which initializes and persists rev0). Then author/group side effects are applied before the awaited `appendRevision()`.

### Fix Focus Areas
- src/node/db/Pad.ts[746-843]
- src/node/db/Pad.ts[717-737]

### Suggested fix
- Wrap the destination-pad creation + revision append in a `try { ... } catch (err) { ... }`.
- In the `catch` block, best-effort remove the destination pad that was just created (for example `await dstPad.remove()`), then rethrow the original error.
- Consider moving `copyAuthorInfoToDestinationPad()` and the `group:${destGroupID}.pads` update to *after* the revision append succeeds, to reduce rollback work.
- Add a regression test that forces `appendRevision()` to throw (stub/hook) and asserts the destination pad does not exist afterward (or is retryable without `force=true`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools