[jaykorean]

Summary:
Why the change is required:
WriteOptions::disableWAL returns Status::InvalidArgument when DBOptions::recycle_log_file_num > 0 (T185140603), and the crash test forces recycle_log_file_num=0 whenever disable_wal=1. The root cause also loses data without disableWAL: external file ingestion consumes a sequence number without a WAL record, which is the recovery data loss reproduced by the disabled DBWALTest.RecycleMultipleWalsCrash (github PR 12918) and the reason WAL recycling is off in the crash test.

Previous behavior:
After a crash, a recycled WAL usually ends with records of its previous incarnation. In kPointInTimeRecovery the log reader reports such a record through OldLogRecord(). Since it cannot be told apart from a corrupted record, recovery stops replaying there and only continues into the next WAL if that WAL's first WriteBatch has exactly the sequence number recovery expects next (github PR 12403, D54544824). Any sequence numbers consumed between the last WriteBatch of one WAL and the first WriteBatch of the next without a WAL record (an unlogged write, an ingested file) fail that check, so recovery silently drops every later WAL, including acknowledged sync writes, or fails DB::Open with "SST file contains data beyond the point of corruption" when a column family was already flushed past it. PR 12403 therefore rejected disableWAL with recycling, PR 12639 exempted WritePrepared's internal split writes, and ingestion was never covered.

New behavior:

• disableWAL writes are accepted with recycle_log_file_num > 0 for plain DBs and WriteCommitted and optimistic transaction DBs. WritePrepared and WriteUnprepared (seq_per_batch) still return InvalidArgument, exactly as before (with a message that now names them).
• Point-in-time recovery from recycled WALs no longer drops WAL data after sequence numbers consumed outside the WAL at a WAL boundary, whether by disableWAL writes, ingestion, or any other feature.
• Real corruption in a recycled WAL still stops recovery at the same point as before.

Implementation:

• DBImpl tracks next_seq_after_last_wal_record_: the sequence number recovery reaches after replaying the last WriteBatch successfully added to a WAL, i.e. its sequence plus the sequence numbers it consumes. WriteToWAL() and WriteGroupToWAL() take that count (seq_inc in WriteImpl() and ConcurrentWriteGroupToWAL(), total_count in PipelinedWriteImpl(), 0 for the DB::Open dummy entry). A failed WAL write sets it to 0 (unknown).
• Before the first WriteBatch of each WAL (log::Writer::GetLastSeqnoRecorded() == 0), MaybeAddWalSeqnoGapFiller() writes an empty WriteBatch whose sequence number is the expected one, if recycling is on, seq_per_batch_ is false, the expected number is known, and the first WriteBatch skips ahead of it. Recovery's continuity check then compares against the filler instead of the first real WriteBatch. This is the same trick DB::Open already uses (an empty batch at the recovered sequence number) to keep point-in-time recovery working across WritePrepared gaps.
• The fix is entirely on the write side and keyed on what was actually written to the WAL, so it does not depend on auditing every feature that consumes sequence numbers, which was the concern in PR 12918. The recovery code is unchanged.
• The InvalidArgument check now applies only with seq_per_batch_. The crash-test workaround is removed.

Safety, compatibility, and limitations:

• Corruption detection: if corruption truncates WAL N (a record whose header log number is damaged looks like an old record; other damage fails the checksum), recovery ends WAL N below the filler's sequence number and stops, as it did when the next WAL started with a real WriteBatch. Detection power equals the existing continuity check; losing records that consume no sequence numbers (WriteCommitted prepares, empty batches) was never detected and still is not.
• Format: no new record type. The filler is a regular record holding an empty WriteBatch, which every RocksDB version reads, and older versions apply the same continuity check, so they also recover these WALs correctly after a downgrade. Fillers are written only with recycling on and a sequence gap at the start of a WAL, so WALs of existing workloads without such gaps are unchanged byte for byte.
• Safety invariant: a filler must never be below the sequence number recovery reaches after the previous WAL, or CheckSeqnoNotSetBackDuringRecovery() would fail DB::Open. Recovery reproduces sequence plus seq_inc for each WriteBatch without seq_per_batch (keys outside prepare sections, the prepared batch at a commit marker, reserved WBWI counts). Cases where recovery advances less (a WalFilter dropping entries, a commit marker whose prepare is gone) only make the filler larger than needed, which at worst falls back to the old stop-replay behavior. Fillers are skipped when the expected number is unknown, such as after a failed WAL write.
• Observable side effects: a WalFilter or GetUpdatesSince() can see an extra empty WriteBatch, as they already can for the DB::Open dummy entry. GetUpdatesSince() already reported such gaps as "Gap in sequence numbers".
• Performance: per WAL write group, one load and compare of the writer's last recorded sequence number (true once per WAL) plus one store; disableWAL writes are unchanged. See the benchmark below.
• Not covered: WritePrepared and WriteUnprepared keep the restriction. Their own internal unlogged writes (e.g. the commit publish write with two_write_queues) can already leave gaps at WAL boundaries under recycling; that pre-existing issue is unchanged here and needs follow-up. The crash test still defaults recycle_log_file_num to 0; enabling it at random is left to a follow-up after more stress runs.

Requested by: Jay Huh (jewoongh)

Differential Revision: D129451715

[meta-codesync]

@jaykorean has exported this pull request. If you are a Meta employee, you can view the originating Diff in D129451715.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 209.9s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 35db9c1

---

Summary

Well-designed and carefully reasoned fix for a real data-loss bug in WAL recovery with recycled logs. The gap-filler approach is elegant -- it fixes the problem on the write side without touching recovery code, and it's backwards-compatible since older RocksDB versions process the filler identically. The change correctly limits scope by excluding seq_per_batch (WritePrepared/WriteUnprepared) and is well-tested.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Gap filler bytes not counted in wals_total_size_ or WAL_FILE_BYTES stats -- db_impl_write.cc:2387

• Issue: MaybeAddWalSeqnoGapFiller() calls log_writer->AddRecord() directly, bypassing the wals_total_size_.FetchAddRelaxed() and WAL_FILE_BYTES accounting in WriteToWAL() (which only accounts for the main log_entry). The filler record's bytes are untracked.
• Root cause: The filler is written as a side effect inside WriteToWAL, before the main log_entry size accounting at line 2373. The *log_size output and the wals_total_size_ update reflect only the caller's batch, not the filler.
• Impact: wals_total_size_ undercounts total WAL bytes, which could delay WAL-size-triggered flushes (when max_total_wal_size is set). The gap filler is small (a WriteBatch header ~12 bytes + record header ~11 bytes), so real-world impact is negligible. Still, the accounting inconsistency is worth noting.
• Suggested fix: Add the filler's serialized size to *log_size or call wals_total_size_.FetchAddRelaxed() for the filler record. Alternatively, document why this is intentionally ignored.

M2. next_seq_after_last_wal_record_ not reset on MaybeAddUserDefinedTimestampSizeRecord failure -- db_impl_write.cc:2360

• Issue: If MaybeAddUserDefinedTimestampSizeRecord fails (line 2360), WriteToWAL returns early before the next_seq_after_last_wal_record_ update. The field retains its value from the previous successful write. While the WAL write fails and triggers error handling, the stale value could theoretically cause an incorrect filler on a subsequent write if the error is somehow recovered.
• Root cause: The early return at line 2361 predates this change and skips all post-AddRecord logic.
• Impact: Low in practice -- a UDT size record failure cascades to a WAL error that typically puts the DB into an error state. Pre-existing pattern.
• Suggested fix: Consider setting next_seq_after_last_wal_record_ = 0 on the early return path, consistent with the io_s.ok() ? ... : 0 pattern used for the main record.

M3. DB::Open dummy batch sets next_seq_after_last_wal_record_ to recovered_seq (not recovered_seq + 1) -- db_impl_open.cc:2840

• Issue: After DB::Open writes the dummy empty batch at sequence recovered_seq with seq_inc=0, next_seq_after_last_wal_record_ becomes recovered_seq + 0 = recovered_seq. This is the sequence of the dummy batch itself, not the next expected sequence. The first real write after Open will be at sequence >= recovered_seq + 1.
• Root cause: The dummy batch has Count=0, so seq_inc=0 is correct for how recovery treats it (0 sequence numbers consumed). But from the filler's perspective, recovery would advance to recovered_seq (not past it) after replaying the dummy batch.
• Analysis: This actually works correctly because: (1) The first real write after Open goes into the SAME WAL as the dummy batch (the WAL opened by DB::Open). (2) Since the dummy batch's AddRecord sets GetLastSeqnoRecorded() to recovered_seq (non-zero), the GetLastSeqnoRecorded() == 0 check returns false for the first real write in this WAL. (3) The filler is only triggered for the first write in a NEW WAL. By that time, the previous WAL's writes have set next_seq_after_last_wal_record_ correctly. So no bug, but the reasoning is subtle enough to warrant a comment.
• Suggested fix: Add a comment at the seq_inc=0 call site explaining why this is correct.

🟢 LOW / NIT

L1. Consider asserting !seq_per_batch_ in MaybeAddWalSeqnoGapFiller -- db_impl_write.cc:2393

• Issue: The function returns early when seq_per_batch_ is true, but does so silently. An assertion after the early return (or a comment) would catch callers who accidentally use this in a seq_per_batch context after future refactoring removes the guard in WriteImpl.
• Impact: Defensive coding improvement.

L2. Test RecycleWalsWithUnloggedWrites uses EXPECT_EQ for gap filler count -- db_wal_test.cc

• Issue: EXPECT_EQ(gap_fillers.load(), 2) could be ASSERT_EQ since if the count is wrong, the subsequent recovery verification is testing a different scenario than intended.
• Impact: Test quality only.

L3. CorruptRecycledWalRecord hardcodes record header layout offset 7 -- db_wal_test.cc:145

• Issue: The helper writes at record_offset + 7 to overwrite the log number, assuming the log record header layout (4-byte CRC + 2-byte length + 1-byte type = 7 bytes before log number). This is correct for the current recyclable format but fragile.
• Impact: Test-only. Low risk since log format is stable, and a header change would break many things.

L4. VerifyWalFilesToRecycle uses EXPECT_* where ASSERT_* may be more appropriate -- db_wal_test.cc:97

• Issue: If the WAL count assertion fails, subsequent test steps proceed with incorrect assumptions.
• Impact: Test quality only.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES - guarded by seq_per_batch_	N/A - correctly excluded	None
WriteUnpreparedTxnDB	YES - same as above	N/A - correctly excluded	None
ReadOnly DB	NO - no writes	N/A	None
User-defined timestamps	YES - UDT size record before filler	GetLastSeqnoRecorded() not affected by UDT record	Safe
External file ingestion	YES - creates the gap the filler bridges	Correctly addressed	Safe
unordered_write	YES - WriteImplWALOnly -> ConcurrentWriteGroupToWAL	seq_inc correct via kDoAssignOrder	Safe
PipelinedWrite	YES - WriteGroupToWAL -> WriteToWAL	seq_inc = total_count, correct	Safe
two_write_queues (WriteCommitted)	YES - under wal_write_mutex_	Synchronization correct	Safe
manual_wal_flush_	YES - filler under same lock	Correct	Safe
Other WAL recovery modes	YES - filler written regardless	Benign (empty WriteBatch)	Safe
Older RocksDB versions (downgrade)	YES - read as normal empty batch	Continuity check works same way	Safe

DB Mutex Analysis

The changed code does not execute while DBImpl::mutex_ is held, except for the DB::Open path (db_impl_open.cc:2839-2841) where the mutex is held per impl->mutex_.AssertHeld(). The DB::Open path with seq_inc=0 does minimal additional work (the gap filler early-returns because next_seq_after_last_wal_record_ is 0). No change to mutex-held critical section cost.

The main write path (WriteToWAL) runs outside the DB mutex. The gap filler adds one log_writer->AddRecord() call amortized once per WAL file (not per write group), which is negligible.

Positive Observations

• Elegant design: The fix is entirely on the write side, requiring no changes to recovery code. This means older RocksDB versions can correctly recover WALs produced by the new code.
• Correct scoping: Excluding seq_per_batch_ avoids complex interactions and is clearly documented with a follow-up path.
• Good reuse of existing patterns: The gap filler reuses the "empty WriteBatch" trick already used by DB::Open, making it a natural extension.
• Thorough test coverage: Tests cover all four write paths, verify gap filler invocation count, and test that corruption detection still works. The RecycleWalsWithUnloggedWritesDetectCorruption test is particularly well-designed.
• Good test refactoring: Common patterns extracted into reusable helpers (DisableWalTruncationOnClose, VerifyWalFilesToRecycle, PopulateWalsToRecycle).
• Correct synchronization: next_seq_after_last_wal_record_ protected by the same synchronization as WriteToWAL.
• Re-enabling DISABLED_RecycleMultipleWalsCrash: Previously disabled test now passing.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase