fix: keep err.stack in sync when pad.check() adds context

[JohnMcLear]

Problem

pad.check() carefully prefixes failures with (pad <id> revision <n>) so an admin knows which record is bad — but it only assigns to err.message. err.stack is rendered from the message when the error is constructed, so the stack keeps showing the original, context-free text.

Both places that report a failed check log err.stack:

• Cleanup.checkTodos() — logger.error(\Error in pad ${padId}: ${err.stack || err}`)`
• the admin cleanupPadRevisions socket handler — same shape

…so the revision number never reaches the log. The reporter on #8134 had to bisect their database by hand to work out which revision was missing:

[ERROR] adminSettings - Error in pad eu-<id>: AssertionError [ERR_ASSERTION]:
  The expression evaluated to a falsy value:

  assert(timestamp != null)

    at Pad.check (/opt/etherpad-lite/src/node/db/Pad.ts:969:9)
    at async deleteRevisions (/opt/etherpad-lite/src/node/utils/Cleanup.ts:48:3)

Nothing there says revision 600, even though check() knew.

Fix

Fix it at the source rather than at each log site: an addErrorContext() helper that prefixes the message and rewrites the matching text in err.stack. All three context-adding sites in Pad.check() (revision load, revision replay, chat message) now go through it, so every reporter of a check() failure benefits — including plugins that log err.stack themselves.

The helper replaces only the first occurrence (the stack's header line) and no-ops on an empty message, which would otherwise match at offset 0 and corrupt the stack.

Tests

src/tests/backend/specs/padCheckErrorContext.ts builds a pad in exactly the shape from #8134 — head pointing past a revision whose pad:<id>:revs:<n> record is absent — and asserts the context reaches err.stack, isn't duplicated, and that the original assertion text and stack frames survive. Also covers the chat-message path and the deleteRevisions() entry point from the issue report.

Verified red→green: 4 of the 7 fail without the Pad.ts change, all 7 pass with it. Full backend suite: 1628 passing, 0 failing.

Scope

Diagnostics only — this does not repair pads with a missing revision, and does not address how the hole forms. Both are tracked separately off #8134.

Refs #8134

🤖 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 Pad.check context so err.stack includes pad/revision details

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Keep err.stack consistent when pad.check() prefixes failure context.
• Centralize context injection via addErrorContext() for revisions and chat messages.
• Add regression tests for missing revision/chat records and deleteRevisions() path.

Diagram

graph TD
  T["Test: padCheckErrorContext"] --> P["Pad.check()"] --> A["addErrorContext()"] --> E("Error message+stack") --> L["Cleanup/admin logging"]
  P --> D[("DB: pad:* records")]

  subgraph Legend
    direction LR
    _t["Test"] ~~~ _f["Function"] ~~~ _e("Error") ~~~ _d[("Database")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Patch each logging site to prefer err.message (or add explicit context fields)

• ➕ Avoids mutating Error.stack, which some tooling assumes is immutable
• ➕ Keeps Pad.check() behavior closer to default Node.js Error semantics
• ➖ Misses other callers (including plugins) that log err.stack directly
• ➖ Duplicates logic across multiple reporting paths and can regress easily

2. Wrap original error in a new Error (e.g., new Error(context, {cause: err}))

• ➕ Avoids string replacement inside stack text
• ➕ Preserves original error intact as the cause
• ➖ Many loggers still print the wrapper stack (without original frames) unless they render cause chains
• ➖ Changes error shape/stack in ways that can reduce diagnostic usefulness unless carefully formatted

Recommendation: The chosen approach (central helper that prefixes err.message and rewrites the stack header once) is the most practical for Etherpad’s logging reality: downstream code logs err.stack, and this fix ensures the pad/revision context reliably shows up everywhere without touching each call site. The helper’s guardrails (only first occurrence, no-op on empty message) keep the risk of stack corruption low, and the added regression tests cover the critical failure shapes.

Files changed (2) +133 / -6

Bug fix (1) +24 / -6

Pad.tsAdd addErrorContext() and use it when Pad.check() prefixes failures +24/-6

Add addErrorContext() and use it when Pad.check() prefixes failures

• Introduces an addErrorContext() helper that prepends contextual information to an error message while also updating the corresponding header line in err.stack. Updates all Pad.check() context-prefix sites (revision load, revision replay, chat message validation) to throw the context-synchronized error so logs that print err.stack include pad/revision identifiers.

src/node/db/Pad.ts

Tests (1) +109 / -0

padCheckErrorContext.tsRegression tests ensuring Pad.check() context appears in err.stack +109/-0

Regression tests ensuring Pad.check() context appears in err.stack

• Adds a backend spec that reproduces the #8134 missing-revision scenario by deleting a specific pad:<id>:revs:<n> key and asserting the pad/revision context is present in both err.message and err.stack. Verifies the context is not duplicated, original assertion text and stack frames remain, covers the deleteRevisions() entry point, and also tests the chat-message missing-record path.

src/tests/backend/specs/padCheckErrorContext.ts

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

Code Review by Qodo

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

1. Cleanup assumes contiguous revisions 📎 Requirement gap ☼ Reliability

Description

deleteRevisions() computes a contiguous revision range to load and delete, but it does not
tolerate missing revision numbers, so gaps can cause it to throw and abort rather than reliably
retaining the requested keepRevisions. Because it also calls pad.check() and propagates its
assertion failures when revision metadata is missing, the cleanup cannot complete in a controlled
manner as required.

Code

src/node/db/Pad.ts[R998-1000]

          } catch (err:any) {
-            err.message = `(pad ${this.id} revision ${r}) ${err.message}`;
-            throw err;
+            throw addErrorContext(err, `(pad ${this.id} revision ${r})`);
          }

Evidence

PR Compliance IDs 1 and 2 require cleanup to (a) not abort when revision records are missing and (b)
still retain a specified number of revisions even if there are database gaps. The current cleanup
code iterates revisions using a computed contiguous index (rev = i + cleanupUntilRevision) and
attempts to load each revision via pad.getRevision(rev), so any missing pad:<id>:revs:<n> entry
causes an exception and the added test shows that a gap makes deleteRevisions() throw instead of
completing retention. Additionally, the cleanup path invokes await pad.check(), and pad.check()
contains an assertion (assert(timestamp != null)), so when expected revision timestamps are absent
the assertion error propagates and the cleanup flow aborts rather than handling the missing revision
record in a controlled way.

Cleanup revisions should support retaining a specified number of revisions despite database gaps
src/node/utils/Cleanup.ts[52-69]
src/tests/backend/specs/padCheckErrorContext.ts[83-92]
src/node/db/Pad.ts[998-1000]
src/node/utils/Cleanup.ts[43-50]
src/node/db/Pad.ts[1006-1011]

Agent prompt

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

## Issue description
Cleanup revision deletion/retention is not gap-tolerant: `deleteRevisions()` iterates over a computed contiguous revision range and can throw when a revision record is missing, and the admin cleanup flow also calls `pad.check()` which asserts on missing timestamps, causing the cleanup operation to abort. Update the cleanup logic so it can complete in a controlled manner and still enforce (or clearly define) `keepRevisions` even when intermediate revision entries are missing.

## Issue Context
Compliance requirements (PR Compliance IDs 1 and 2) state that cleanup must not abort due to missing `pad:<id>:revs:<n>` records and must support retaining `X` revisions despite discontinuities in revision numbering. The current behavior fails these requirements because gaps lead to thrown errors during `pad.getRevision(rev)` iteration and/or assertion failures from `pad.check()`.

## Fix Focus Areas
- src/node/utils/Cleanup.ts[43-69]
- src/node/utils/Cleanup.ts[52-69]
- src/node/utils/Cleanup.ts[65-68]
- src/node/db/Pad.ts[998-1000]
- src/node/db/Pad.ts[1004-1012]
- src/tests/backend/specs/padCheckErrorContext.ts[83-92]

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

---

2. Stack context can duplicate 🐞 Bug ◔ Observability

Description

addErrorContext() always replaces oldMessage inside err.stack even if err.stack is generated after
err.message is updated (lazy stack formatting), which can cause the context prefix to be duplicated
in the stack header. This risks corrupting/duplicating the most important diagnostic line for errors
thrown during pad.check() (including DB read failures).

Code

src/node/db/Pad.ts[R69-71]

+  if (oldMessage && typeof err.stack === 'string' && err.stack.includes(oldMessage)) {
+    err.stack = err.stack.replace(oldMessage, err.message);
+  }

Evidence

The replacement condition checks for oldMessage, but oldMessage is always a substring of the
newly prefixed err.message. If the stack is rendered after updating err.message, err.stack
will already contain the new message yet still satisfy includes(oldMessage), causing replace()
to inject the prefix a second time. Pad.check() uses this helper to wrap errors from DB-backed
revision reads, so non-assertion errors are also subject to this behavior.

src/node/db/Pad.ts[54-73]
src/node/db/Pad.ts[986-1000]
src/node/db/Pad.ts[385-398]

Agent prompt

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

### Issue description
`addErrorContext()` sets `err.message` and then conditionally does `err.stack.replace(oldMessage, err.message)` based on `err.stack.includes(oldMessage)`. If `err.stack` is lazily rendered (common), the first access inside `addErrorContext()` can already include the *new* prefixed message, which still contains `oldMessage` as a substring, so the replace duplicates the prefix.

### Issue Context
This helper is used in `Pad.check()` to add pad/revision/chat context to errors coming from multiple sources (assertions and DB reads). We want to update the stack header **only when the stack still reflects the old message**.

### Fix Focus Areas
- src/node/db/Pad.ts[54-72]

Suggested approach:
- After updating `err.message`, only run the `replace(oldMessage, err.message)` when the stack does **not** already contain the new message, for example:
 - `if (oldMessage && typeof err.stack === 'string' && err.stack.includes(oldMessage) && !err.stack.includes(err.message)) { ... }`
- (Optional) Consider capturing the current `err.stack` into a local before modifying `err.stack` to avoid multiple getter evaluations.

ⓘ 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

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. Cleanup assumes contiguous revisions 📎 Requirement gap ☼ Reliability

deleteRevisions() computes a contiguous revision range to load and delete, but it does not
tolerate missing revision numbers, so gaps can cause it to throw and abort rather than reliably
retaining the requested keepRevisions. Because it also calls pad.check() and propagates its
assertion failures when revision metadata is missing, the cleanup cannot complete in a controlled
manner as required.

Agent Prompt

## Issue description
Cleanup revision deletion/retention is not gap-tolerant: `deleteRevisions()` iterates over a computed contiguous revision range and can throw when a revision record is missing, and the admin cleanup flow also calls `pad.check()` which asserts on missing timestamps, causing the cleanup operation to abort. Update the cleanup logic so it can complete in a controlled manner and still enforce (or clearly define) `keepRevisions` even when intermediate revision entries are missing.

## Issue Context
Compliance requirements (PR Compliance IDs 1 and 2) state that cleanup must not abort due to missing `pad:<id>:revs:<n>` records and must support retaining `X` revisions despite discontinuities in revision numbering. The current behavior fails these requirements because gaps lead to thrown errors during `pad.getRevision(rev)` iteration and/or assertion failures from `pad.check()`.

## Fix Focus Areas
- src/node/utils/Cleanup.ts[43-69]
- src/node/utils/Cleanup.ts[52-69]
- src/node/utils/Cleanup.ts[65-68]
- src/node/db/Pad.ts[998-1000]
- src/node/db/Pad.ts[1004-1012]
- src/tests/backend/specs/padCheckErrorContext.ts[83-92]

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

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

2. Stack context can duplicate 🐞 Bug ◔ Observability

addErrorContext() always replaces oldMessage inside err.stack even if err.stack is generated after
err.message is updated (lazy stack formatting), which can cause the context prefix to be duplicated
in the stack header. This risks corrupting/duplicating the most important diagnostic line for errors
thrown during pad.check() (including DB read failures).

Agent Prompt

### Issue description
`addErrorContext()` sets `err.message` and then conditionally does `err.stack.replace(oldMessage, err.message)` based on `err.stack.includes(oldMessage)`. If `err.stack` is lazily rendered (common), the first access inside `addErrorContext()` can already include the *new* prefixed message, which still contains `oldMessage` as a substring, so the replace duplicates the prefix.

### Issue Context
This helper is used in `Pad.check()` to add pad/revision/chat context to errors coming from multiple sources (assertions and DB reads). We want to update the stack header **only when the stack still reflects the old message**.

### Fix Focus Areas
- src/node/db/Pad.ts[54-72]

Suggested approach:
- After updating `err.message`, only run the `replace(oldMessage, err.message)` when the stack does **not** already contain the new message, for example:
  - `if (oldMessage && typeof err.stack === 'string' && err.stack.includes(oldMessage) && !err.stack.includes(err.message)) { ... }`
- (Optional) Consider capturing the current `err.stack` into a local before modifying `err.stack` to avoid multiple getter evaluations.

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