fix(txnlog): scan recovery framing from the file, not a whole-image buffer - #792
Conversation
…uffer The recovery walk only needs 13-byte entry headers. Slurping payloads was leftover from the validation-oriented scanner. I/O failure now fails open rather than being classified as a torn tail. Co-authored-by: Cursor <cursoragent@cursor.com>
A 13-byte pread per entry regresses dense-log cold opens; a 64 KiB refill at every header would pull large payloads. Nearby sequential headers share a window, and a jump past it reads exactly one header. Co-authored-by: Cursor <cursoragent@cursor.com>
The first header is a 13-byte read, so a 13-byte nearby slack never engaged the window for real payload sizes. Treat a gap up to the window size as sequential so typical entries amortize without slurping megabyte payloads. Co-authored-by: Cursor <cursoragent@cursor.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the transaction log recovery mechanism to perform positional reads instead of loading the entire log file into memory. It introduces a ScanReader with a 64 KiB sliding window to optimize sequential header reads while skipping large payloads, and updates the Windows file pointer logic to support files larger than 2 GB. Comprehensive unit tests have been added to verify the new on-demand reading behavior and error handling. There are no review comments, and I have no additional feedback to provide.
The old name implied a whole-file slurp. The helper only fills n bytes at an offset, retrying short reads. Co-authored-by: Cursor <cursoragent@cursor.com>
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Cool, I think I had looked at doing this, but glad you are actually carrying it out.
🤖 Reviewed with Codex
| RecoveryScan scan = scanTransactionLogForRecovery(buffer.data(), fileSize); | ||
| // Publish the complete-transaction boundary from this same scan so the store can | ||
| // seed its committed watermark without re-reading the file. | ||
| this->lastCompleteTransactionEnd.store(scan.lastCompleteTransactionEnd, std::memory_order_relaxed); |
There was a problem hiding this comment.
When an interrupted transaction begins in a newly rotated file, this file has no local flagged boundary, so the whole uncommitted prefix is retained. load() seeds the watermark from an older file, but the next successful commit advances it to nextLogPosition; committed queries then traverse and expose the retained aborted entries before the new transaction. Please repair the complete rotation-spanning tail, or fail/quarantine the store when it cannot be repaired safely. A regression should reopen this scenario, commit one new transaction, and verify that only the previously completed transaction plus the new one are visible.
(src/binding/transaction_log/transaction_log_file.cpp:324 is not part of this PR's diff — anchored to the nearest line this PR's diff can hold)
| RecoveryScan scan = scanTransactionLogForRecovery(buffer.data(), fileSize); | ||
| // Publish the complete-transaction boundary from this same scan so the store can | ||
| // seed its committed watermark without re-reading the file. | ||
| this->lastCompleteTransactionEnd.store(scan.lastCompleteTransactionEnd, std::memory_order_relaxed); |
There was a problem hiding this comment.
At this point recovery has identified a trailing batch as uncommitted, but an eraseTail() failure only logs and allows the store to open. A later commit advances the monotonic watermark past these known-aborted entries and exposes them as committed. Please propagate repair failure (including truncation durability failure) so the store cannot accept writes until the tail is safely removed, and add an injected-failure test.
(src/binding/transaction_log/transaction_log_file.cpp:349 is not part of this PR's diff — anchored to the nearest line this PR's diff can hold)
| // Intact frames after the break are mid-file corruption; truncating would | ||
| // discard them. A torn tail has nothing valid behind it. | ||
| if (validFramingResumes(source, pos + 1)) { | ||
| return scan(RecoveryScan::Kind::MidFileCorruption, pos); |
There was a problem hiding this comment.
validFramingResumes() proves that intact frames exist after this break, but returning here leaves lastCompleteTransactionEnd at the last boundary before the corruption. If txn.state is behind, committed transactions after the break remain outside the recovered watermark, so the reader never reaches the CorruptFrameError/resync path intended to preserve them. Please return the resync offset and continue scanning transaction flags after it while retaining the first corruption location; cover a break followed by flagged commits with an older txn.state.
|
Thanks — dug into all three Codex threads. Each targets behavior that's identical in the base branch (#723,
Suggest tracking all three against #723, where the repair-policy and watermark/resync design lives. #3 is the only one whose code sits in a file this PR edits — happy to take it on as a follow-up in this stack if you'd prefer it here. Unrelated CI note: Generated by Barber AI 🤖 |
af5149f
into
kris/txnlog-committed-position-recovery
2 participants
Stacked on #723. Open-time recovery now walks v1 framing with positional header reads instead of allocating a whole-file buffer, and
findLastCompleteTransactionEnd()is gone. An I/O failure throwsDBExceptionrather than classifying as a torn tail.For the human reviewer
A read error during active-file recovery fails database open rather than logging and skipping recovery as before. Alternative: catch at
TransactionLogStore::load()and degrade. Reversible with one catch. "No" means we can append past a torn tail we never classified.Positional header walk with a 64 KiB nearby window rather than mapping the file, reading fixed chunks, or reusing
MemoryMap. Peak memory is O(1); a change of mind is contained toScanReader. Mapping was rejected because recovery must run before mappings are handed to readers (Windows truncate).Validation still slurps a whole image (
validateTransactionLogImage) while recovery streams. Alternative: convert validation too.backups.verify()keeps the O(file) profile; reversible now that the callback exists.scanTransactionLogForRecovery(TransactionLogFile&)is public and takesfileMutex; in-lock callers usescanRecoveryLocked(). Alternative: tests lock and callscanRecoveryLocked()themselves. The mutex is not recursive.readFullyFromFileretriesEINTRunboundedly. Alternative: a bound. A signal storm could spin underfileMutex.Unresolved leftovers that do not need a ruling: corruption-resync still does a positional read per plausible chain hop (cold path only); the Windows
SetFilePointerExchange has no >2GiB test on Windows (open()would map 2GiB); there is no Vitest that an unreadable active log rejectsopen().Verification
Native GoogleTest: 125 tests, 122 passed (3 MADV_COLD skipped on macOS), including
TypicalPayloadsAmortizesReads, header-only large-payload walks, throw-not-truncate on I/O failure, file/buffer parity, and a POSIX sparse header at0x80000000. Vitesttest/transaction-log.test.ts+test/transaction-log-crash-recovery.test.ts: 82 passed, including SIGKILL crash recovery. Deep review of the recovery-scan diff at1ada20b2: 0 findings.Review coverage
Authored by Cursor Grok 4.6. Cross-model review: gpt-5.6-sol (codex) ✓, gemini via agy (default model) ✓, Harper domain adjudication (claude-opus-5) ✓ on
1ada20b2(the delta at797e416fpruned domain). cursor-grok disabled (authoring family). cursor-composer disabled (an earlier round refused a diff that changesAGENTS.md). Receipt @ 797e416.Complexity: complicated
Human-Review-Need: 3 @ 797e416