fix(replay): recover from corrupt transaction-log frames instead of aborting startup - #1140
Merged
Merged
Conversation
…borting startup
A torn or corrupt transaction-log frame (a torn write at crash time, or a
flipped length byte in an unflushed/corrupt log) makes rocksdb-js 1.4.x's
reader throw a bounded RangeError ("declared length N overruns the log" /
"truncated entry header") out of its query iterator's .next(). That throw
escaped uncaught through RocksTransactionLogStore.getRange()'s consumers —
startup replay (replayLogs.ts) and replication broadcast (transactionBroadcast.ts)
— aborting the boot with an uncaughtException, then a fatal upgrade-abort on the
next boot.
rocksdb-js is correct to surface corruption loudly (#612); the consumer decides
policy. Wrap each log.query() iterator in getRange with endIteratorOnCorruptFrame
(new pure, callback-based helper in replayLogsGuards.ts): catch the framing
RangeError, log once via harperLogger.warn, and treat the corrupt frame as
end-of-log — torn-write semantics, since once framing is lost the next entry
cannot be located. Entries before the corruption have already been replayed;
non-RangeError failures propagate unchanged.
Also de-flakes the replay-stress suite's byte-flip test (#1136), the top
Integration Tests flake: it replaces nondeterministic random byte flips plus a
20s wall-clock replay budget and an rss-growth heuristic with a deterministic
corruption — corrupt the last well-framed entry's length prefix (the unflushed
tail replay actually reads) — and a behavioral recovery assertion (server comes
back, tables queryable), guarded by corrupted>0 so it can't pass vacuously.
Closes #1135
Closes #1136
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to handle framing-level corruption in transaction logs gracefully. Instead of allowing a RangeError (thrown by rocksdb-js when a log entry's declared length overruns the log) to bubble up and abort startup, the new endIteratorOnCorruptFrame wrapper catches the error, logs a warning, and terminates the iterator cleanly. This is supported by updated integration and unit tests. Feedback on the changes highlights that the wrapped iterator returned by endIteratorOnCorruptFrame does not delegate the optional return and throw methods of the underlying iterator, which could lead to resource leaks if the iterator is exited early. A code suggestion is provided to forward these methods.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. |
Both PR review bots flagged that the corrupt-frame iterator wrapper dropped the underlying iterator's optional return()/throw(), so early termination (a for-of break or an outer .return()) would skip the source iterator's cleanup. The current rocksdb-js query iterator implements neither, so nothing leaks today, but the wrapper should stay a faithful proxy — delegate both when present (guarded by typeof), and latch stopped on return(). Adds unit tests for delegation and for the no-synthesis-when-absent case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both review bots (gemini-code-assist, claude) suggested the identical shape: always define return()/throw() on the wrapper, delegate to the source when it implements them, otherwise fall back to the protocol defaults (return -> done, throw -> rethrow), marking stopped first so a later next() can't re-enter. Adopt that verbatim so the applied fix matches the inline suggestions exactly. Functionally equivalent to the prior conditional form for the current rocksdb-js query iterator (which implements neither), and still forwards cleanup if a future iterator adds it. Unit test updated for the always-defined defaults. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
heskew
marked this pull request as ready for review
June 4, 2026 19:44
heskew
marked this pull request as draft
June 4, 2026 19:52
De-duplicate the #1135/RangeError/end-of-log rationale that was repeated across the guard JSDoc, its inline comments, the stress-test helper/body, and the unit-test header. State each point once in its canonical spot; drop rocksdb-internals detail that isn't this module's concern. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
heskew
marked this pull request as ready for review
June 4, 2026 20:18
kriszyp
approved these changes
Jun 8, 2026
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
2 participants
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.
rocksdb-js's txnlog reader throws a bounded
RangeErroron a torn/corrupt frame (its intentional #612 hardening). That throw escaped uncaught throughRocksTransactionLogStore.getRange()'s consumers — startup replay and replication broadcast — aborting the boot.endIteratorOnCorruptFramenow catches it at that shared chokepoint and treats the frame as end-of-log (torn-write semantics). Thereplay-stresstest (#1136) is now deterministic (corrupt the last entry's length prefix) with a behavioral assertion, replacing random byte-flips + a wall-clock budget.Closes #1135, #1136.
For review: ending iteration drops any entries after the corrupt frame — unreadable anyway once framing is lost, and strictly better than the boot-aborting crash. @kriszyp, does this consumer-side "corrupt frame ⇒ end-of-log" policy match your intent for the #612 reader hardening?
CI: shard 2/4 (replay-stress) is green on all runtimes; the red shard 1/4 (
4.x-upgrade) and 4/4-Bun (csv_data_load) are pre-existing/unrelated — 1/4 is red onmaintoo.🤖 Generated with Claude Code