[archang19]

Summary:
X-link: #15458

Bug: with WAL recycling on (recycle_log_file_num > 0), RocksDB can lose writes that were already acknowledged, including sync writes.

Why: FindObsoleteFiles() puts an old WAL A on the recycle list and only closes A's writer later, after releasing the mutexes. If a memtable switch happens in that window, A is renamed and reused as the new WAL C, so A's still-open writer and C point at the same file. When the old writer finally closes, PosixWritableFile::Close() truncates the file to A's old size and wipes out what was written to C. The next reopen recovers from the truncated WAL, and those writes are gone. In the test, C went from 65,589 bytes to 32.

With background_close_inactive_wals = true, two more paths (FindObsoleteFiles() and MarkLogsSynced()) take A out of logs_ before its writer is closed, so the same reuse can happen.

Fix: never reuse a WAL whose writer may still be open.

• SwitchMemtable() skips recycling a WAL that is still in logs_ (IsWalInLogs()) and creates a fresh WAL that time.
• With recycling on, FindObsoleteFiles() and MarkLogsSynced() close a WAL's writer before removing it from logs_.

Reviewed By: xingbowang

Differential Revision: D126882379

[meta-codesync]

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

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 383.5s.

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit b8fce73

---

Summary

This PR correctly diagnoses and fixes a real data-loss race condition where WAL recycling can truncate acknowledged sync writes. The fix is well-scoped, the approach is sound, and the locking is correct. The test exercises the key race windows for both background_close_inactive_wals configurations.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Stalled recycling when IsWalInLogs persistently returns true — db_impl_write.cc:3298

• Issue: When IsWalInLogs returns true, recycling is skipped and a fresh WAL is created, but the skipped WAL stays at the front of wal_recycle_files_. If SwitchMemtableImpl is called rapidly (e.g., multiple column families flushing) before FindObsoleteFiles runs and closes the old writer, every call skips recycling and creates a fresh WAL while the recycle queue is effectively stalled. This is a transient condition — FindObsoleteFiles will eventually close the writer and clear the blockage — but it could cause a brief burst of fresh WAL file allocations rather than reuse.
• Root cause: The fix is intentionally conservative: it skips recycling rather than waiting. This is the right trade-off (waiting would hold mutex_ longer), but the temporary fallback behavior means recycling provides no benefit during the race window.
• Suggested fix: This is acceptable behavior — the race window is brief and creating fresh WAL files is the standard non-recycling path. No code change needed, but a comment explaining the transient fallback would aid future readers.

M2. IsWalInLogs declared in public section of DBImpl — db_impl.h:1085

• Issue: The new IsWalInLogs() method is declared in the public: section of DBImpl (between MinLogNumberToRecycle() at line 1084 and MinObsoleteSstNumberToKeep() at line 1092). While DBImpl is an internal class and neighboring methods like MinLogNumberToRecycle() and FindObsoleteFiles() are also public, IsWalInLogs is purely an internal implementation detail called only from SwitchMemtableImpl. It would be better placed in the private: section near other WAL helper functions (e.g., near MarkLogsSynced at line 3202).
• Root cause: Following the placement pattern of neighboring functions.
• Suggested fix: Move declaration to the private section.

🟢 LOW / NIT

L1. Comment update in MarkLogsSynced could be clearer — db_impl.cc:2667-2668

• Issue: The updated comment reads "background_close_inactive_wals without WAL recycling" but doesn't explain why recycling changes the behavior. A reader unfamiliar with the bug might wonder why recycling matters for the close-before-reclaim decision.
• Suggested fix: Add a brief note like "with WAL recycling, closing must precede removal from logs_ to prevent the old writer from truncating a recycled file."

L2. Linear scan in IsWalInLogs — db_impl_files.cc:124-130

• Issue: IsWalInLogs iterates over the entire logs_ deque. While logs_ is typically very small (current WAL + a few pending close), this is called under both mutex_ and wal_write_mutex_ on the SwitchMemtable path.
• Root cause: logs_ is a deque with no index by WAL number.
• Suggested fix: Not needed — logs_ rarely exceeds 2-3 entries and this is on a warm path (memtable switch, not per-write). The overhead is negligible compared to other work in SwitchMemtableImpl.

L3. REQUIRES: wal_write_mutex_ not held comment could clarify mutex_ expectation — db_impl.h:1088

• Issue: The comment says "REQUIRES: wal_write_mutex_ not held" (correct — it acquires it internally) but doesn't mention that mutex_ may or may not be held. While this is fine (the lock ordering permits mutex_ → wal_write_mutex_), documenting this would be helpful.
• Suggested fix: Consider adding "May be called with or without mutex_ held."

Cross-Component Analysis

Context	Affected?	Analysis
two_write_queues	Yes	SwitchMemtableImpl acquires/releases wal_write_mutex_ for wal_empty_ (lines 3286-3292), then IsWalInLogs acquires it again. These are independent lock regions with no state dependency — safe.
background_close_inactive_wals=true	Yes (primary bug path)	Both MarkLogsSynced and FindObsoleteFiles fixes apply. The MarkLogsSynced fix prevents opportunistic removal from logs_ with open writer. FindObsoleteFiles fix ensures writer is closed before removal.
background_close_inactive_wals=false	Yes (secondary bug path)	FindObsoleteFiles already closed writer before removal (original behavior unchanged). SwitchMemtableImpl IsWalInLogs check adds defense-in-depth.
ReadOnly/Secondary DB	No	No WAL writing or recycling.
Recovery	No	wal_recycle_files_ is empty during recovery.
Concurrent flushes	Safe	Serialized by mutex_.

TOCTOU Analysis

The IsWalInLogs() check at line 3298 runs under mutex_. Between this check and CreateWAL() at line 3356, mutex_ is released (line 3335). Two scenarios:

1. IsWalInLogs returns true → recycling skipped: Safe. recycle_log_number = 0, fresh WAL created.
2. IsWalInLogs returns false → recycling proceeds: The WAL's writer is closed and removed from logs_. Between the check and the actual rename in CreateWAL, could the writer be re-opened? No — WAL numbers are monotonically increasing and never re-enter logs_. Could the file be deleted? No — it's in wal_recycle_files_ which protects it from purge. Safe.

DB Mutex Impact

• Before: SwitchMemtableImpl did not acquire wal_write_mutex_ at line 3298.
• After: One additional wal_write_mutex_ acquire/release + linear scan of logs_ (typically 1-3 entries).
• Assessment: Negligible. The function already acquires wal_write_mutex_ at lines 3286 and 3338. The added work is O(1) in practice and occurs only when recycling is enabled and the recycle queue is non-empty.

Lock Ordering Verification

db_impl.h:3561-3562: "if needed to acquire both wal_write_mutex_ and mutex_, the order should be first mutex_ and then wal_write_mutex_." IsWalInLogs is called with mutex_ held and acquires wal_write_mutex_ internally — correct order.

Positive Observations

• The fix is minimal and well-targeted — three small changes that close the race from both directions (prevent premature removal from logs_, and check logs_ before recycling).
• Defense-in-depth: the IsWalInLogs check in SwitchMemtableImpl is a safety net even if the FindObsoleteFiles and MarkLogsSynced changes handle the common paths.
• The test uses sync points and std::promise/std::future for deterministic synchronization — no sleeps.
• The test covers 4 combinations of background_close_inactive_wals × sync_old_wal_before_flush with SCOPED_TRACE.
• The release note is clear, concise, and accurately describes the user impact.

---

ℹ️ 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

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit f8920da

---

Summary

This PR correctly identifies and fixes a real data-loss race condition in WAL recycling. The fix is well-targeted: three coordinated changes ensure a WAL's writer is closed before the WAL can be recycled. The approach is sound, the locking is correct, and the test coverage is good. No high-severity issues found.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. IsWalInLogs acquires wal_write_mutex_ while mutex_ is held, adding contention to the memtable switch critical section — db_impl_write.cc:3313

• Issue: SwitchMemtableImpl holds mutex_ (asserted at line 3278) when calling IsWalInLogs (line 3313). IsWalInLogs acquires wal_write_mutex_ and iterates logs_. This adds wal_write_mutex_ acquisition time to the mutex_-held critical section of memtable switch. While the lock ordering is correct (mutex_ before wal_write_mutex_, per db_impl.h:3675-3676), this could increase mutex_ hold time under wal_write_mutex_ contention from concurrent write threads.
• Root cause: The check could not be moved outside mutex_ because wal_recycle_files_ (read at line 3313) is protected by mutex_.
• Severity justification: MEDIUM rather than HIGH because: (1) logs_ is typically 1-3 entries, so the iteration is O(1) in practice; (2) SwitchMemtableImpl is a warm path (per memtable switch, not per write); (3) the existing code already briefly acquires wal_write_mutex_ in the same function (lines 3300-3307 for two_write_queues_). The marginal impact is small.
• Suggested fix: Consider combining the IsWalInLogs check with the existing wal_write_mutex_ acquisition at lines 3300-3307. However, this optimization is not critical and the current approach is simpler and more readable.

M2. WAL error recovery path removes entries from logs_ without closing writer first — db_impl.cc:645-646

• Issue: The WAL write error recovery path at db_impl.cc:645 calls ReleaseWriter() and logs_.erase() without first closing the writer's file. This means the writer's fd stays open until BackgroundCallPurge deletes the writer object, at which point ~WritableFileWriter → PosixWritableFile::Close() → ftruncate() would run.
• Mitigating factors: This path only executes during WAL error recovery (context.IsWALWriteErrorRecovery()). The failed WALs being removed here are recent (active when failure occurred) and would not be in wal_recycle_files_ (they haven't been through FindObsoleteFiles yet). Additionally, after this path, FindObsoleteFiles is called at line 677, which would handle any recycling decisions with proper closing. So the race described in the PR summary cannot occur through this path.
• Suggested fix: No fix needed for the current bug, but a defensive comment documenting why this path is safe would improve maintainability.

🟢 LOW / NIT

L1. IsWalInLogs uses linear scan — db_impl_files.cc:125-130

• Issue: IsWalInLogs linearly scans logs_ to check membership. While logs_ is small in practice (1-3 entries), a std::find_if or checking only the front entries would be slightly more idiomatic.
• Suggested fix: Not worth changing given the small size of logs_. The code is clear as-is.

L2. No mutex_.AssertHeld() in IsWalInLogs — db_impl_files.cc:124

• Issue: IsWalInLogs is called from SwitchMemtableImpl which holds mutex_. The function's REQUIRES comment says wal_write_mutex_ not held, but doesn't document whether mutex_ is expected. Since wal_write_mutex_ alone suffices for reading logs_, the function is correct without mutex_, but adding documentation about the expected caller context would help.
• Suggested fix: Consider adding a comment that callers typically hold mutex_ (needed to read wal_recycle_files_), but the function itself only requires wal_write_mutex_ (which it acquires internally).

L3. Test uses std::promise/std::future for synchronization — db_wal_test.cc:1305-1315

• Issue: The new test uses std::promise/std::future/std::shared_future for inter-thread synchronization. While this works, RocksDB tests typically use SyncPoint for thread coordination. The test does use SyncPoint for the pause points, but uses std::promise for the old_wal_synced signal.
• Mitigating factors: The std::promise usage is for coordinating the test's own thread with the flush thread at BackgroundCallFlush:start, which happens before the sync points are reached. Using SyncPoint for this coordination might be more complex. The approach is reasonable.

L4. Existing tests modified to add TEST_WaitForBackgroundWork() — db_wal_test.cc:1265,1940

• Issue: Two existing tests (FullPurgePreservesLogPendingReuse and TrackAndVerifyWALsRecycleWAL) were modified to add TEST_WaitForBackgroundWork() after Flush(). This is because the fix now delays WAL recycling until the writer is closed, which happens asynchronously. The tests need to wait for the writer close before proceeding.
• Assessment: These modifications are correct and necessary given the fix. They address latent assumptions in the existing tests about when WALs become recyclable.

Cross-Component Analysis

Execution Context Table

Context	Code executes?	Assumptions hold?	Action needed?
Normal (single write queue)	YES	YES — mutex_ held when checking wal_recycle_files_ and calling IsWalInLogs	None
two_write_queues_	YES	YES — wal_write_mutex_ briefly held/released at 3300-3307, not held when IsWalInLogs called at 3313	None
background_close_inactive_wals=true + recycling	YES	YES — both MarkLogsSynced and FindObsoleteFiles changes prevent premature removal from logs_	None
background_close_inactive_wals=false + recycling	YES	YES — FindObsoleteFiles already closed writers before this fix; the new condition preserves this behavior	None
recycle_log_file_num=0 (no recycling)	Code partially executes	YES — IsWalInLogs never called (outer condition at 3312 checks recycle_log_file_num); MarkLogsSynced/FindObsoleteFiles changes are no-ops when recycling is off	None
WAL error recovery	Not for recycling	Safe — failed WALs are recent, not in wal_recycle_files_	None
ReadOnly/Secondary DB	NO	N/A	None
2PC	YES	YES — same WAL lifecycle	None

Assumption Stress Test

Claim: "A WAL still in logs_ means its writer may still be open"

• Precondition 1: WAL is in logs_ → writer object exists (via ReleaseWriter contract)
• Precondition 2: Writer's file may still be open (file() != nullptr)
• Counterexample: WAL is in logs_ but writer was already closed via FindObsoleteFiles's close path, followed by PublishIfClosed() → file() returns nullptr. In this case, IsWalInLogs returns true but the writer is already closed. This is conservative but safe — we skip recycling unnecessarily, creating a fresh WAL. The next FindObsoleteFiles removes it from logs_, and subsequent recycling attempts succeed.
• Verdict: The claim is intentionally conservative. No bug.

Claim: "Skipping recycling when IsWalInLogs returns true prevents data loss"

• Precondition: The WAL's writer close (with ftruncate) must happen BEFORE recycling
• The fix ensures either: (a) IsWalInLogs returns true → skip recycling → safe, or (b) IsWalInLogs returns false → WAL already removed from logs_ → its writer already freed → Close/ftruncate already happened → safe to recycle
• TOCTOU analysis: Between IsWalInLogs returning false (line 3313, mutex_ held) and CreateWAL (line 3371, mutex_ released at 3350): Could the WAL's writer somehow not have been closed? No — if WAL is not in logs_, its writer was transferred to wals_to_free_. For the FindObsoleteFiles path, Close({}) is called explicitly at line 454 BEFORE removing from logs_. For the MarkLogsSynced path with the fix, the WAL stays in logs_ until FindObsoleteFiles handles it. So by the time IsWalInLogs returns false, Close has already been called.
• Verdict: The TOCTOU is safe.

Positive Observations

1. Root cause analysis is excellent. The PR description clearly identifies the race window and the mechanism (ftruncate in PosixWritableFile::Close).
2. Three-pronged fix is comprehensive. The fix addresses all three paths where a WAL could leave logs_ with an open writer: SwitchMemtableImpl (defense at consumption), FindObsoleteFiles (close before removal), and MarkLogsSynced (prevent premature removal).
3. Test covers all relevant dimensions. The new test iterates over pause points, background_close_inactive_wals settings, and sync-before-flush scenarios. It verifies data survives reopen.
4. Existing test fixes are correct. The TEST_WaitForBackgroundWork() additions address latent timing assumptions exposed by the fix.
5. Release note is appropriately concise and describes the user-facing impact.
6. Lock ordering is correct throughout all changes.

---

ℹ️ 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

[meta-codesync]

This pull request has been merged in 02815ee.