Handle async prefetch failures safely

[xingbowang]

Summary

FilePrefetchBuffer owns the I/O handles, callback state, and scratch buffers used by asynchronous prefetch reads. Poll and AbortIO are runtime operations whose successful return establishes callback completion; an error does not.

Problem: cancellation results were guarded only by assertions. Release builds could ignore an AbortIO failure and clear or destroy request state that an outstanding callback still referenced. Poll failure likewise destroyed the handle even though completion was not guaranteed. Separately, callback completion status was ignored, including synchronous inline completion, and failure to submit a later buffer could leave earlier requests from the same multi-buffer prefetch active.

Impact: callbacks could access released FilePrefetchBuffer state, resulting in use-after-free, crashes, or memory corruption. Async read errors could be masked and failed or partial buffer state treated as usable. A failed multi-buffer operation could also leave sibling I/O running after the caller had received an error.

Fix: make cancellation and outdated-data helpers return Status and propagate poll, abort, and submission failures without releasing live handles, buffers, or callback state. Poll errors preserve the request for a later poll or abort. The destructor attempts AbortIO, falls back to Poll, and terminates with diagnostics if neither can establish safe completion.

Record callback status only when completion fails, consume it after Poll confirms callback completion, clear failed buffers, and handle default inline ReadAsync completion immediately. When a later multi-buffer submission fails, remove the failed buffer and abort earlier sibling requests. Expected cancellation status is discarded safely, while successful requests retain the allocation-free path. Synchronous fallback, EOF behavior, and public APIs remain unchanged.

Tests: add a controlled async filesystem covering AbortAllIOs and outdated-I/O failure propagation, Poll failure lifetime preservation, destructor fallback, inline and background completion errors, and later-submission cleanup. The complete prefetch_test suite passes normally and with ASSERT_STATUS_CHECKED=1; source and whitespace checks pass as well.

Test plan

• Complete prefetch_test suite.
• Complete prefetch_test suite with ASSERT_STATUS_CHECKED=1.
• Source and whitespace checks.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 184.2s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 578a70a

---

Summary

Solid defensive hardening of async prefetch failure paths. The design is sound: propagating Status from abort/poll/clear operations, handling inline completion, preserving handles on poll failure, and adding destructor fallback logic all address real use-after-free and silent-error risks.

High-severity findings (2):

• [file_prefetch_buffer.cc:PollIfNeeded] After the PR, a Poll failure preserves the handle but does not clear async_read_in_progress_, leaving the buffer in a state where DoesBufferContainData() and similar methods will fire debug assertions on subsequent access.
• [file_prefetch_buffer.cc:destructor] Destructor iterates bufs_ to find buffers with io_handles, then calls AbortAllIOs() which re-iterates bufs_ and checks async_read_in_progress_. But buffers found in the first loop by checking io_handle_ != nullptr may not have async_read_in_progress_ set (e.g., after a prior Poll failure that preserved the handle but cleared the flag). This creates a mismatch where the destructor's bufs_with_io_handles list is non-empty but AbortAllIOs() skips those buffers.

Full review (click to expand)

Findings

🔴 HIGH

H1. Poll failure leaves async_read_in_progress_ set with no cleanup path — file_prefetch_buffer.cc:PollIfNeeded

• Issue: In the new PollIfNeeded, when Poll fails, the code preserves the handle (correct) but returns early without resetting async_read_in_progress_. The buffer remains in bufs_ with async_read_in_progress_ = true. Several BufferInfo methods (DoesBufferContainData(), IsOffsetInBuffer(), IsDataBlockInBuffer()) assert async_read_in_progress_ == false. Any subsequent code path that touches this buffer (e.g., ClearOutdatedData, FreeEmptyBuffers, IsBufferOutdated) will either fire debug assertions or behave incorrectly in release builds.
• Root cause: The old code called DestroyAndClearIOHandle(buf) on Poll failure, which set async_read_in_progress_ = false. The new code correctly preserves the handle but doesn't address the async_read_in_progress_ flag state.
• Suggested fix: This is an inherent tension in the design. If Poll fails and the request is still in-flight, async_read_in_progress_ should remain true (it accurately reflects state). The real fix is to ensure callers of PollIfNeeded that receive an error don't access the buffer through assertion-guarded methods. Verify that all error-return paths after PollIfNeeded avoid touching the buffer's data accessors, or consider adding a dedicated "poisoned" state to BufferInfo.

H2. Destructor's bufs_with_io_handles loop misaligns with AbortAllIOs filter — file_prefetch_buffer.cc:destructor

• Issue: The destructor builds bufs_with_io_handles by checking buf->io_handle_ != nullptr (no async_read_in_progress_ check). After calling AbortAllIOs() (or PollAllIOs()), it calls buf->ClearBuffer() on those buffers. However, AbortAllIOs() only collects handles where buf->async_read_in_progress_ && buf->io_handle_ != nullptr. If a buffer has io_handle_ != nullptr but async_read_in_progress_ == false (possible after the H1 scenario or after inline completion where async_read_in_progress_ was never set), AbortAllIOs() won't abort it, but the destructor will still clear its buffer. This is a partial mismatch rather than a critical bug, since in practice the only way io_handle_ != nullptr with async_read_in_progress_ == false is after a prior cleanup already completed the callback. But the asymmetry is fragile.
• Root cause: Different filtering criteria between the destructor's first loop and AbortAllIOs().
• Suggested fix: Use the same filtering criteria in the destructor's initial loop as AbortAllIOs(), or simply rely on AbortAllIOs() / PollAllIOs() to handle all cleanup (they already iterate bufs_), and clear buffers unconditionally after.

🟡 MEDIUM

M1. CleanupAfterAsyncReadFailure asserts buf == GetLastBuffer() — file_prefetch_buffer.cc:CleanupAfterAsyncReadFailure

• Issue: This assert is correct for all current call sites (the failed buffer is always the last allocated one). But it creates a tight coupling — if a future caller passes a non-last buffer, it silently corrupts state in release builds (the assert only fires in debug). The method name doesn't communicate this restriction.
• Suggested fix: Consider renaming or documenting the precondition more prominently.

🟢 LOW / NIT

L1. <memory> header addition — file_prefetch_buffer.h

• Issue: Adds #include <memory> for std::unique_ptr<IOStatus>. Correct — follows "include what you use."

L2. <cstdio> and <exception> additions — file_prefetch_buffer.cc

• Issue: For std::fprintf and std::terminate in destructor. Both used elsewhere in RocksDB (9 files for std::terminate, 8 for <exception>). Consistent.

L3. Test infrastructure well-designed but local

• Issue: ControlledAsyncFileSystem could potentially be shared for other async IO tests, but local placement is the RocksDB convention.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
Default FS (no io_uring)	YES — inline completion	YES — nullptr io_handle correctly detected	Safe
PosixFS with io_uring	YES — primary target	YES	Safe
ReadOnly DB	YES — prefetch for reads	YES	Safe
Compaction (num_buffers_=1)	YES but limited	YES — sync paths unchanged	Safe
BlobDB	YES — blob_source.cc	YES	Safe
WritePreparedTxnDB	No direct interaction	N/A	Safe

Positive Observations

1. Correct design principle: Recognizes that AbortIO/Poll failure means callbacks may be outstanding — handle/buffer/callback state must not be released. Prevents real use-after-free.
2. Zero-allocation happy path: async_read_error_ as unique_ptr<IOStatus> avoids allocation for successful reads.
3. Comprehensive tests: 8 new tests covering abort failure, poll failure, inline/background completion failure, multi-buffer submission failure, and destructor fallback.
4. Consistent error propagation: All void-to-Status changes propagate correctly through the call chain.
5. Destructor safety: AbortIO -> Poll -> terminate with diagnostics is the correct defensive design.

---

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