feat: report a damaged pad history instead of asserting - #8145
Open
JohnMcLear wants to merge 1 commit into
Open
Conversation
deleteRevisions() went straight into pad.check(), which replays the
whole history and dies on
AssertionError: The expression evaluated to a falsy value:
assert(timestamp != null)
That is an assertion about a null timestamp, when what an operator needs
to hear is which revision is missing and what to do about it. The
reporter on #8134 had to bisect their database by hand to find it.
Add Pad.findMissingRevisions(), a cheap scan of 0..head for revisions
that are absent or carry no meta.timestamp -- it reads one sub-field per
revision and replays nothing -- and run it before check() so the clear
error wins. Cleanup now fails with:
Pad eu-demopad is missing revision(s) 6. Its history cannot be
replayed, so revisions cannot be cleaned up. The pad's current text is
unaffected. Rebuild the history with a full compaction (compactPad
with no keepRevisions) to make the pad cleanable again.
The admin UI already renders err.toString(), so this reaches the
operator with no UI change.
Full compaction is deliberately NOT gated on the same check: it does not
replay history, it rebuilds from the current text, so it is the recovery
path the message points at.
Refs #8134
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoReport missing pad revisions during cleanup instead of assertion failure
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| }); | ||
|
|
||
| it('still cleans up a healthy pad', async function () { | ||
| const pad = await padWithHoles([]); |
| const padId = common.randomString(); | ||
| const pad = await padManager.getPad(padId); | ||
| for (let i = 0; i < 12; i++) await pad.appendText(`line ${i}\n`); | ||
| const staleHead = pad.getHeadRevisionNumber(); |
Code Review by Qodo
1. deleteRevisions() throws on gaps
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+53
to
+61
| const missing = await pad.findMissingRevisions(); | ||
| if (missing.length > 0) { | ||
| throw new Error( | ||
| `Pad ${padId} is missing revision(s) ${missing.join(', ')}` + | ||
| `${missing.length >= 20 ? ' (and possibly more)' : ''}. ` + | ||
| 'Its history cannot be replayed, so revisions cannot be cleaned up. ' + | ||
| "The pad's current text is unaffected. Rebuild the history with a " + | ||
| 'full compaction (compactPad with no keepRevisions) to make the pad ' + | ||
| 'cleanable again.'); |
There was a problem hiding this comment.
1. deleterevisions() throws on gaps 📎 Requirement gap ☼ Reliability
deleteRevisions() now throws an Error when missing revisions are detected, which aborts cleanup/keepRevisions instead of completing via a defined non-failing path. This conflicts with the requirement that cleanup/keepRevisions must not fail/abort when revision records are missing.
Agent Prompt
## Issue description
`deleteRevisions()` throws when `pad.findMissingRevisions()` finds gaps, causing cleanup/keepRevisions to abort rather than completing via a defined recovery behavior.
## Issue Context
The compliance requirement for #8134 expects cleanup/keepRevisions to handle missing revision records without failing/aborting (e.g., by logging a controlled warning and skipping, or automatically taking a defined recovery path).
## Fix Focus Areas
- src/node/utils/Cleanup.ts[53-61]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+51
to
+54
| console.log(` head before=${headBefore}; after: head=${state.head} ` + | ||
| `missing=[${state.missing}] threw=${threw}`); | ||
| assert.ok(state.missing.length > 0, | ||
| 'expected deleteRevisions to leave holes'); |
There was a problem hiding this comment.
2. Probe test committed 🐞 Bug ⚙ Maintainability
src/tests/backend/specs/holeProbe.ts is explicitly labeled a “Probe”, prints to stdout, and asserts that deleteRevisions() leaves revision holes, which makes CI noisy and encodes an undesirable failure mode as a permanent test expectation.
Agent Prompt
### Issue description
`src/tests/backend/specs/holeProbe.ts` is committed as a normal backend test but is clearly intended as exploratory diagnostics: it uses `console.log()` and asserts that cleanup corruption (holes) occurs. This will add noise to CI output and makes the test suite assert an undesirable state as “expected”.
### Issue Context
This file is new in this PR and not referenced as part of the intended feature-level test coverage (the real regression coverage is in `cleanupMissingRevisions.ts`).
### Fix Focus Areas
- src/tests/backend/specs/holeProbe.ts[1-85]
### What to change
- Preferred: delete `holeProbe.ts` from the normal specs suite.
- Alternative: change it to `describe.skip(...)` (or move it to a non-default/diagnostic test location) and remove `console.log()`.
- If you want to keep coverage, rewrite as a focused regression test that asserts the *desired* behavior (e.g., cleanup is atomic / does not leave holes) rather than asserting that corruption is produced.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+938
to
+941
| const revs = Stream.range(0, this.getHeadRevisionNumber() + 1) | ||
| .map(async (r: number) => [r, await this.getRevisionDate(r)]) | ||
| .batch(100).buffer(99); | ||
| for await (const [r, timestamp] of revs) { |
There was a problem hiding this comment.
3. Prefetch abort risks rejections 🐞 Bug ☼ Reliability
Pad.findMissingRevisions() uses Stream.batch(100).buffer(99) and then breaks once the missing-revision limit is hit, which can start extra DB reads beyond what is needed and (per Stream’s documented abort semantics) can surface unhandled Promise rejections for read-but-unyielded operations if any prefetched DB read fails after early termination.
Agent Prompt
### Issue description
`Pad.findMissingRevisions()` prefetches revision timestamp reads using `Stream.batch(100).buffer(99)` but may terminate iteration early via `break` once `limit` gaps are found. Because `Stream` explicitly documents that early-aborted iteration will “un-suppress” rejections for read-but-unyielded Promises, any prefetched DB read that rejects after the early break can become an unhandled rejection. It also means the method can issue more DB reads than necessary even when the limit is reached quickly.
### Issue Context
- `Stream.batch()` reads an entire batch before yielding, and both `batch()` and `buffer()` manage suppression/un-suppression of rejections on early abort.
- The method’s limit is meant to stop work once enough gaps are found.
### Fix Focus Areas
- src/node/db/Pad.ts[936-949]
- src/node/utils/Stream.ts[25-53]
- src/node/utils/Stream.ts[57-72]
- src/node/utils/Stream.ts[96-102]
### What to change (one good option)
- Replace the Stream-based pipeline with explicit bounded batching that always awaits/observes all started reads:
- Loop `r` from 0..head in chunks (e.g., 100).
- For each chunk, `await Promise.allSettled(chunk.map(getRevisionDate))` (or `Promise.all` if you want to fail fast).
- Collect missing revisions from the settled results.
- Stop once `missing.length >= limit`.
This preserves concurrency but guarantees there are no “started but never observed” Promises when the function returns early.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+56
to
+58
| `Pad ${padId} is missing revision(s) ${missing.join(', ')}` + | ||
| `${missing.length >= 20 ? ' (and possibly more)' : ''}. ` + | ||
| 'Its history cannot be replayed, so revisions cannot be cleaned up. ' + |
There was a problem hiding this comment.
4. Hardcoded scan limit 🐞 Bug ⚙ Maintainability
Cleanup.deleteRevisions() hard-codes 20 when deciding whether to append “(and possibly more)”, which is coupled to Pad.findMissingRevisions()’s current default limit and can become inaccurate if the default or call site limit changes.
Agent Prompt
### Issue description
The cleanup error message uses a hard-coded `20` (`missing.length >= 20`) to decide whether to append “(and possibly more)”, but the scan limit is defined by `Pad.findMissingRevisions(limit = 20)`. If the default changes or cleanup later passes a different limit, the message becomes misleading.
### Issue Context
`deleteRevisions()` currently calls `pad.findMissingRevisions()` with no argument, so it matches today, but this is brittle.
### Fix Focus Areas
- src/node/utils/Cleanup.ts[53-61]
- src/node/db/Pad.ts[936-947]
### What to change
- Define `const limit = 20;` in `deleteRevisions()`, call `pad.findMissingRevisions(limit)`, and use `missing.length >= limit` in the message; or
- Return `{missing, truncated}` from `findMissingRevisions()` and use `truncated` to decide whether to append the suffix.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #8134. First of two: this makes a damaged pad fail gracefully; the follow-up stops cleanup creating damage.
Problem
deleteRevisions()goes straight intopad.check(), which replays the entire history and dies on:That's an assertion about a null timestamp. What the operator needs to hear is which revision is missing and what to do about it. The reporter on #8134 had to bisect their database by hand to find it.
Change
Pad.findMissingRevisions(limit = 20)scans0..headfor revisions that are absent or carry nometa.timestamp. It reads one sub-field per revision and replays nothing, so it's cheap next tocheck(). It runs beforecheck()so the clear error wins.What the admin UI now shows (it already renders
err.toString(), so no UI change):Three things an operator actually wants: which revision, that their content is safe, and the way out.
Full compaction is deliberately not gated on the same check — it doesn't replay history, it rebuilds from the current text, so it's the recovery path the message points at. There's a test asserting it still works on a damaged pad.
Tests
src/tests/backend/specs/cleanupMissingRevisions.ts— 11 cases coveringfindMissingRevisions(healthy pad, one gap, several gaps in ascending order, the limit, nothing beyond head) anddeleteRevisionson a damaged pad (names the revision, no raw assertion text, says the text is safe and names the recovery, lists multiple gaps, leaves the damaged pad untouched, still cleans healthy pads), plus full compaction still working on a damaged pad.Full backend suite: 1634 passing, 0 failing.
tsc --noEmitclean.Note
The
deleteAllRevisionstest compares text trimmed rather than byte-exact, because ondevelopcopyPadWithoutHistorystill appends a newline per copy (#8139, fixed by #8140). Keeping this PR independent of that one rather than stacking them.🤖 Generated with Claude Code