fix: don't let a failed revision write leave a hole in pad history

[JohnMcLear]

Fixes #8134.

Problem

appendRevision() writes the revision record and the pad record (which carries head) as two independent writes in one Promise.all, after ++this.head:

const newRev = ++this.head;
await Promise.all([
  this.db.set(`pad:${this.id}:revs:${newRev}`, {...}),
  this.saveToDatabase(),          // persists head = newRev
  ...
]);

If the revision write fails and the pad record lands, the pad claims a revision that was never stored. Nothing rolls back — this.head stays advanced, the rejection just propagates up and disconnects the client — so the next successful append writes head+1 straight over the gap. It is permanent.

Every later pad.check() then trips on the missing revision:

AssertionError: The expression evaluated to a falsy value:
  assert(timestamp != null)
  at Pad.check (src/node/db/Pad.ts:969:9)
  at async deleteRevisions (src/node/utils/Cleanup.ts:48:3)

deleteRevisions() calls check() before touching anything, so the pad can never be cleaned up or compacted again, and there is no supported way back.

That is exactly #8134: revisions 599 and 601 present, 600 absent, "Cleanup revisions" refusing to run. Revision 600 being the casualty fits — it is a key revision (Math.floor(rev/100)*100), so it embeds the entire attribute pool and atext. It is by far the largest record written, and so the one most likely to exceed max_allowed_packet or time out. The reporter runs ep_image_upload, which defaults to storageType: 'base64' and puts full data URIs in the pool.

Fix

The two writes stay concurrent — sequencing them would add a write round-trip to every commit on the editing hot path. Instead, on storage failure:

• roll the in-memory head and atext back to their pre-append values,
• re-persist the pad record so the persisted head matches the stored history,
• and if that write fails too, log loudly, rather than leaving a silent hole for an admin to find months later via a failed cleanup run.

Hook and author-index calls move out of the storage Promise.all so a throwing padUpdate hook cannot roll back a revision that was stored successfully. They still start immediately and run concurrently with the writes.

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.

Tests

src/tests/backend/specs/appendRevisionAtomicity.ts fails the specific write and asserts the outcome:

• revision-write failure: rejects; in-memory and persisted head both roll back; atext rolls back; the next append reuses the revision number rather than skipping it; and the pad still passes check()
• pad-record-write failure: rejects and rolls back, leaving at most a harmless unreferenced revision record (check() only walks 0..head)
• a throwing padUpdate hook does not roll back a stored revision

5 of the 7 fail without the Pad.ts change. Full backend suite: 1628 passing, 0 failing. tsc --noEmit clean.

Scope

This stops new holes forming. It does not repair pads that already have one — including the reporter's. Their revision 600 is genuinely unrecoverable: an identity changeset in its place just moves the failure to 601, because 601's changeset assumes 600's result. The realistic repair is rebuilding the history via full compaction, which needs #8139 fixed first (#8140).

🤖 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 appendRevision() atomicity to prevent missing revisions in pad history

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Roll back pad head/atext if revision storage fails to prevent permanent history gaps
• Separate hooks/author-index side effects so plugin failures don’t undo stored revisions
• Add regression tests covering failed writes, rollback behavior, and hook failure semantics

Diagram

graph TD
A["Pad.appendRevision()"] --> B["Storage writes (rev + pad)"] --> C["Return newRev"]
A --> F["Side effects (hooks + author index)"] --> C
B -."write failure".-> D["Rollback helper"] --> E["Persist pad head"] --> A
G[("DB: pad + rev keys")] --- B
G --- E

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Transactional write (DB transaction / batch / multi-key atomic)

• ➕ True atomicity: pad head and revision record commit or abort together
• ➕ Eliminates need for compensating rollback and related edge cases
• ➖ May not be supported across all Etherpad DB backends/adapters
• ➖ Could add operational complexity and performance overhead

2. Sequence writes (write revision first, then update pad head)

• ➕ Simpler mental model and fewer rollback paths
• ➕ Prevents pad head from ever pointing to a non-existent revision
• ➖ Adds an extra round-trip on the editing hot path
• ➖ Still needs to handle failure modes (e.g., revision written but head update fails)

3. Compare-and-swap head update (conditional set if expected head matches)

• ➕ Detects and prevents inconsistent head updates under concurrency/failure
• ➕ Can reduce risk without full transactions if backend supports CAS
• ➖ Requires backend support and changes in DB adapter API
• ➖ Doesn’t by itself guarantee revision record existence unless coupled with other checks

Recommendation: The PR’s compensating-rollback approach is the best pragmatic fix given performance constraints (keep writes concurrent) and likely lack of cross-backend transactional guarantees. The added isolation of side effects (hooks/author index) is also correct: storage success must not be undone by plugin failures. If a future DB adapter can support atomic multi-key writes or CAS semantics, that would be a worthwhile follow-up to simplify correctness guarantees.

Files changed (2) +251 / -1

Bug fix (1) +65 / -1

Pad.tsRollback head/atext on failed revision persistence; isolate side effects +65/-1

Rollback head/atext on failed revision persistence; isolate side effects

• Snapshots pre-append head and atext, then performs concurrent revision+pad writes with explicit error handling. On storage failure, restores in-memory state and re-saves the pad record to keep persisted 'head' consistent, logging loudly if rollback persistence also fails. Moves hooks/author-index updates out of the storage Promise.all so their failures don’t roll back successfully stored revisions.

src/node/db/Pad.ts

Tests (1) +186 / -0

appendRevisionAtomicity.tsAdd regression tests for appendRevision atomicity and hook failure behavior +186/-0

Add regression tests for appendRevision atomicity and hook failure behavior

• Introduces a new backend spec that simulates DB write failures by temporarily overriding db.set. Verifies head/atext rollback, revision-number reuse after a failed append, consistency with pad.check(), safe behavior when pad record write fails, and that a throwing padUpdate hook does not roll back a stored revision.

src/tests/backend/specs/appendRevisionAtomicity.ts

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

Code Review by Qodo

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

1. Rollback can be overwritten 🐞 Bug ≡ Correctness

Description

If the revision write fails, appendRevision() rolls back head/atext and re-saves the pad record, but
the original in-flight saveToDatabase() from storageWrites can still complete afterwards and
re-persist head=newRev. This can leave the persisted pad record pointing past stored history,
recreating the missing-revision gap despite the rollback.

Code

src/node/db/Pad.ts[R368-371]

+      await storageWrites;
+    } catch (err) {
+      await this._rollbackFailedRevision(newRev, prevHead, prevAText);
+      throw err;

Evidence

storageWrites starts this.saveToDatabase() concurrently with the revision write. If the revision
write fails, _rollbackFailedRevision() writes pad:<id> again, but the original
saveToDatabase() is still in-flight and can complete later, overwriting the rollback result
because both writes target the same key.

src/node/db/Pad.ts[313-338]
src/node/db/Pad.ts[367-375]
src/node/db/Pad.ts[388-393]
src/node/db/Pad.ts[415-419]

Agent prompt

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

## Issue description
`appendRevision()` starts two independent writes concurrently (`revs:<newRev>` and `pad:<id>` via `saveToDatabase()`). On failure it immediately calls `_rollbackFailedRevision()` which writes `pad:<id>` again with the rolled-back `head`. Because the original `saveToDatabase()` promise is not canceled, it can still complete after the rollback write and overwrite the pad record with `head=newRev`, reintroducing the missing-revision hole.

## Issue Context
- The failing path relies on `_rollbackFailedRevision()` to ensure the persisted pad record never claims an unstored revision.
- `Promise.all()` does not cancel the other in-flight promise when one rejects.

## Fix Focus Areas
- Ensure the original pad-record write has *settled* (or is otherwise prevented from writing later) before performing the rollback write.
- Prefer keeping explicit handles to the individual promises (revWrite, padWrite), and in the error path `await`/`allSettled` them as needed before re-saving.
- Add/adjust a regression test that forces the first `pad:<id>` write to resolve *after* the rollback save, and assert the final stored `pad:<id>.head` is rolled back.

### Code pointers
- src/node/db/Pad.ts[313-375]
- src/node/db/Pad.ts[388-404]
- src/node/db/Pad.ts[415-419]
- src/tests/backend/specs/appendRevisionAtomicity.ts[30-122]

ⓘ 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. Rollback can be overwritten 🐞 Bug ≡ Correctness

If the revision write fails, appendRevision() rolls back head/atext and re-saves the pad record, but
the original in-flight saveToDatabase() from storageWrites can still complete afterwards and
re-persist head=newRev. This can leave the persisted pad record pointing past stored history,
recreating the missing-revision gap despite the rollback.

Agent Prompt

## Issue description
`appendRevision()` starts two independent writes concurrently (`revs:<newRev>` and `pad:<id>` via `saveToDatabase()`). On failure it immediately calls `_rollbackFailedRevision()` which writes `pad:<id>` again with the rolled-back `head`. Because the original `saveToDatabase()` promise is not canceled, it can still complete after the rollback write and overwrite the pad record with `head=newRev`, reintroducing the missing-revision hole.

## Issue Context
- The failing path relies on `_rollbackFailedRevision()` to ensure the persisted pad record never claims an unstored revision.
- `Promise.all()` does not cancel the other in-flight promise when one rejects.

## Fix Focus Areas
- Ensure the original pad-record write has *settled* (or is otherwise prevented from writing later) before performing the rollback write.
- Prefer keeping explicit handles to the individual promises (revWrite, padWrite), and in the error path `await`/`allSettled` them as needed before re-saving.
- Add/adjust a regression test that forces the first `pad:<id>` write to resolve *after* the rollback save, and assert the final stored `pad:<id>.head` is rolled back.

### Code pointers
- src/node/db/Pad.ts[313-375]
- src/node/db/Pad.ts[388-404]
- src/node/db/Pad.ts[415-419]
- src/tests/backend/specs/appendRevisionAtomicity.ts[30-122]

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