block GetCreationTimeOfOldestFile on async file open completion (#14723)

[joshkang97]

Summary:

Context

GetCreationTimeOfOldestFile() assumed max_open_files = -1 meant every live SST had a pinned table reader. That is not true with open_files_async: recovery intentionally skips loading table files, DB::Open() returns, and BGWorkAsyncFileOpen() pins readers later. Any caller — e.g. fb_rocksdb's daily report at FbRocksDb.cpp:1025 — invoking the API in the window between DB::Open returning and the background opener completing trips a debug assert. Reported in https://fb.workplace.com/groups/rocksdb/permalink/31668956482726231/.

Removing the assert alone is insufficient. For legacy DBs whose manifest does not carry file_creation_time, FileMetaData::TryGetFileCreationTime() falls back to the pinned reader; with no reader, it returns kUnknownFileCreationTime and the function silently returns 0 (the "info unavailable" sentinel). The caller cannot distinguish "no info" from "raced with async open."

Changes

• Remove the invalid debug assert in Version::GetCreationTimeOfOldestFile.
• Add a private helper DBImpl::WaitForAsyncFileOpen() that blocks on bg_cv_ while bg_async_file_open_state_ == kScheduled. The synchronization machinery (bg_async_file_open_state_ + bg_cv_) already exists — the destructor wait loop in db_impl.cc:674-687 uses the same pattern. The helper is a no-op when open_files_async = false, and bails on shutting_down_ so DB::Close() is not blocked by an in-flight caller.
• Call WaitForAsyncFileOpen() at the top of DBImpl::GetCreationTimeOfOldestFile() (inside the max_open_files == -1 branch).
• Document the blocking behavior in include/rocksdb/db.h.
• Replace the regression test with one that uses a DBImpl::WaitForAsyncFileOpen::BeforeWait sync point: spawn a thread that calls GetCreationTimeOfOldestFile, deterministically confirm it blocks inside the wait, release async open, confirm the caller wakes with the real value.

Potential Followups (not included here)

• Apply the same wait to GetLiveFilesMetaData and GetColumnFamilyMetaData — both zero out oldest_ancester_time / file_creation_time in the SST metadata they return during the async-open window (db/version_set.cc:7877-7878, 2090-2091, 2172-2173).
• Address compaction-picker effects: TTL/periodic file selection (db/version_set.cc:4039, 4092-4094), bottommost over-marking (db/version_set.cc:4700-4701), FIFO TTL/temperature pickers (db/compaction/compaction_picker_fifo.cc:105-107, 167-170, 401-411), and tiered-compaction output time inheritance (db/compaction/compaction.cc:981, 1000).
• Harden FbRocksDb.cpp:1025 to check status.ok() instead of status.code() != kNotSupported.

Differential Revision: D104285992

[meta-codesync]

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

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 388.6s.

[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 d094290

---

Summary

This PR correctly addresses the debug assert crash and race in GetCreationTimeOfOldestFile() with open_files_async = true. The core synchronization pattern (WaitForAsyncFileOpen) is sound and follows existing RocksDB patterns. However, the PR is missing the documented API header change and should add error handling for failed async opening.

High-severity findings (2):

• [include/rocksdb/db.h:1871] PR description promises to document blocking behavior in the public header, but the diff contains no changes to include/rocksdb/db.h.
• [db/db_impl/db_impl.cc:~7408] After WaitForAsyncFileOpen() returns, there is no check for background errors from a failed async open. If BGWorkAsyncFileOpen fails (I/O error, corruption), the state still transitions to kComplete, the wait returns, the retry still gets ctime=0, and the caller cannot distinguish a race from a genuine failure.

Full review (click to expand)

Findings

🔴 HIGH

H1. Missing API documentation update — include/rocksdb/db.h:1871

• Issue: The PR description explicitly states "Document the blocking behavior in include/rocksdb/db.h", but the diff includes no changes to the public header. Users calling GetCreationTimeOfOldestFile() have no way to know it may now block when open_files_async = true.
• Root cause: Documentation change was omitted from the diff.
• Suggested fix: Add a note to the doc comment on GetCreationTimeOfOldestFile() in include/rocksdb/db.h explaining that when open_files_async = true, the API may block until background SST file loading completes for legacy DBs whose manifest lacks file_creation_time.

H2. No error check after waiting for async file open — db/db_impl/db_impl.cc:~7408

• Issue: BGWorkAsyncFileOpen can fail (I/O error, corruption) but its deleter always sets bg_async_file_open_state_ = kComplete and signals bg_cv_ (db/db_impl/db_impl_open.cc:2859). After failure, WaitForAsyncFileOpen() returns as if everything succeeded, the retry of version->GetCreationTimeOfOldestFile(&ctime) still returns 0 (readers are still null), and the function silently returns 0. The caller cannot distinguish "raced with async open" from "async open failed."
• Root cause: WaitForAsyncFileOpen() does not check error_handler_.GetBGError() after the wait loop completes.
• Suggested fix: After the wait loop in WaitForAsyncFileOpen(), check for a background error. Propagate it as a Status return value so GetCreationTimeOfOldestFile() can return an error status instead of silently returning 0. Alternatively, check error_handler_.GetBGError() in the call_once lambda after the wait and skip the retry if a BG error is set.

🟡 MEDIUM

M1. Comment typo in db_impl.h — db/db_impl/db_impl.h:1858

• Issue: The doc comment reads "Block the until any in-flight async file open work" — missing a noun after "the" (should be "Block the caller until..." or "Block until...").
• Suggested fix: // Block until any in-flight async file open work has completed.

M2. Shutdown produces silent 0 return — db/db_impl/db_impl.cc:~7408

• Issue: When WaitForAsyncFileOpen() returns early due to shutting_down_, the retry inside call_once still executes version->GetCreationTimeOfOldestFile(&ctime), which may return 0 (readers still null if async open didn't complete). The function returns Status::OK() with creation_time = 0, indistinguishable from a legitimate "unknown creation time" result.
• Suggested fix: After WaitForAsyncFileOpen() returns, check shutting_down_ and either skip the retry or return an appropriate error status.

M3. WaitForAsyncFileOpen() visibility — db/db_impl/db_impl.h:1860

• Issue: The method is declared in the protected section alongside BGWorkAsyncFileOpen. Since it's only called from GetCreationTimeOfOldestFile() (a DBImpl method), private would be more appropriate unless subclasses need to override or call it.
• Suggested fix: Move to private section unless there's a planned need for subclass access.

🟢 LOW / NIT

L1. std::once_flag is unnecessary — db/db_impl/db_impl.cc:7399

• Issue: std::once_flag waited_for_async_open with std::call_once is used to ensure the wait happens only once across the CF loop. However, a simple bool waited = false would suffice since GetCreationTimeOfOldestFile runs on a single thread per call (the once_flag is stack-local and not shared). std::call_once adds synchronization overhead that isn't needed for a local-only flag.
• Suggested fix: Replace with bool waited = false; if (!waited) { waited = true; WaitForAsyncFileOpen(); ... } for clarity.

L2. Sync point in version_set.cc on every call — db/version_set.cc:2248

• Issue: The TEST_SYNC_POINT_CALLBACK("Version::GetCreationTimeOfOldestFile::FileCreationTime", &file_creation_time) executes on every file in every call. In non-test builds this is a no-op (compiled out), so there is no production impact. However, in debug/test builds it adds overhead to every iteration.
• Suggested fix: Acceptable as-is since sync points compile to no-ops in release. No change needed.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (inherits DBImpl)	YES	None
ReadOnly DB	YES (supports open_files_async)	YES	None — WaitForAsyncFileOpen works the same
SecondaryInstance	NO (no open_files_async support)	N/A	None
FIFO compaction	Not directly — FIFO reads creation_time via table properties, not this API	N/A	Pre-existing separate issue (mentioned in PR follow-ups)
Concurrent callers	YES	YES — each caller gets its own stack-local once_flag; WaitForAsyncFileOpen is thread-safe (acquires mutex_)	None
Shutdown during wait	YES	Partially — wait exits but retry still runs	See M2

Debated findings resolved:

• Multi-CF retry logic (initially flagged as HIGH by 5+ agents): After debate, determined to be a false positive. WaitForAsyncFileOpen() completes ALL CFs' file opening globally. When the loop proceeds to CF2, its first version->GetCreationTimeOfOldestFile(&ctime) call sees the already-pinned readers and returns real values. The call_once only needs to retry the CF that triggered the wait. If async opening failed for some CFs, returning 0 is correct (creation time is genuinely unknown).

• SuperVersion lifetime during wait (initially flagged as HIGH): After verification, determined to be safe. GetAndRefSuperVersion() increments the ref count, preventing the Version and its FileMetaData from being freed during the wait. The version pointer remains valid.

• Test 1 sync point logic (initially flagged as HIGH): After verification, determined to be correct. For modern DBs, TryGetFileCreationTime() returns the manifest value directly (not 0), so the ctime == 0 condition is false and WaitForAsyncFileOpen() is never called. The sync point is never reached, waited stays false, and the test correctly validates this behavior.

• Thread safety of concurrent callers (initially flagged as HIGH): After analysis, determined to be safe. Each call to GetCreationTimeOfOldestFile() creates its own stack-local std::once_flag. WaitForAsyncFileOpen() acquires mutex_ properly and the wait loop correctly handles spurious wakeups by re-checking both bg_async_file_open_state_ and shutting_down_.

Positive Observations

• The fix correctly identifies that modern DBs (manifest carries file_creation_time) never need the wait — the optimization to only wait when ctime == 0 is well-designed.
• WaitForAsyncFileOpen() follows the established pattern from the destructor wait loop (db_impl.cc:774-783), ensuring consistency.
• The shutting_down_ check prevents the wait from blocking DB::Close().
• Test 2 uses a well-designed sync point dependency chain to deterministically test the blocking behavior with a simulated legacy DB.
• The release note is clear and at the right level for external consumption.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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 98b3995

---

Summary

Sound bugfix that correctly addresses the GetCreationTimeOfOldestFile() / open_files_async race. The implementation uses established RocksDB synchronization patterns (mutex + bg_cv_ wait, std::call_once), properly handles shutdown, and avoids unnecessary blocking for modern DBs. No correctness issues found.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Consider checking background error after wait — db/db_impl/db_impl.cc:7404

• Issue: If BGWorkAsyncFileOpen fails (e.g., SST corruption), it sets a background error via error_handler_.SetBGError() and breaks out of its loop. The deleter still transitions state to kComplete, so WaitForAsyncFileOpen() returns successfully. The re-query then returns ctime == 0 because pinned readers were never loaded. The caller receives creation_time = 0 with Status::OK() — indistinguishable from "legitimately unknown."
• Root cause: WaitForAsyncFileOpen() doesn't check or propagate the background error state. This is a pre-existing limitation (before this PR, the API also returned 0 in this scenario — and additionally crashed on the debug assert), but this PR has an opportunity to improve it.
• Suggested fix: After WaitForAsyncFileOpen() returns, optionally check error_handler_.GetBGError() and return it if non-OK. Alternatively, accept as-is and defer to a follow-up, since the caller can independently check DB::GetBGError().

M2. SuperVersion held during potentially long wait — db/db_impl/db_impl.cc:7404

• Issue: GetAndRefSuperVersion(cfd) pins the SuperVersion before entering WaitForAsyncFileOpen(). During the wait (which could be long for remote filesystems), the reference prevents SuperVersion cleanup. This is safe (ref-counted) but holds memory unnecessarily.
• Suggested fix: Release the SuperVersion before waiting and re-acquire afterward:

  if (ctime == 0 && immutable_db_options_.open_files_async) {
    std::call_once(waited_for_async_open, [&]() {
      ReturnAndCleanupSuperVersion(cfd, sv);
      WaitForAsyncFileOpen();
      sv = GetAndRefSuperVersion(cfd);
      version = sv->current;
      version->GetCreationTimeOfOldestFile(&ctime);
    });
  }

  This also has the benefit of picking up the latest version after async open completes, though in practice FileMetaData sharing makes this equivalent.

🟢 LOW / NIT

L1. Documentation note about shutdown incompleteness — include/rocksdb/db.h:1874

• Issue: The added doc note says "It is possible to have an incomplete result if the DB is closed while files are still being opened in the background." This is accurate but could be more precise: after shutdown, call_once won't retry, so the API returns 0 for the remainder of the CDF loop. Consider noting that callers should check DB::GetBGError() or shutting_down status if they need to distinguish shutdown from legitimately unknown.

L2. Test 1 sync point semantics — db/db_test.cc:8228

• Issue: GetCreationTimeOfOldestFileSkipsWaitForModernDB checks that waited (set by the BeforeWait sync point callback) is false. This works correctly because modern DBs return ctime > 0, so the ctime == 0 condition is never met, and WaitForAsyncFileOpen() is never called. However, if async open hasn't completed by the time the API runs, the open_files_async check alone would enter WaitForAsyncFileOpen — the test relies on the fact that ctime != 0 prevents entry. A comment in the test clarifying this dependency would improve readability.

L3. std::once_flag could be replaced with a simple bool — db/db_impl/db_impl.cc:7399

• Issue: Since once_flag is stack-local (not shared between threads), a plain bool waited = false with if (!waited) { waited = true; ... } would be equivalent and slightly more readable. std::call_once is typically used for thread-safe one-time initialization, which isn't needed here since the flag isn't shared.

Cross-Component Analysis

Context	Safe?	Notes
Shutdown (CancelAllBackgroundWork)	Yes	shutting_down_ set under mutex_ with bg_cv_.SignalAll() — wait always terminates
Destructor wait loop	Yes	Both use same bg_cv_; no priority inversion
DBImplReadOnly / DBImplSecondary	Yes	MarkAsyncFileOpenNotNeeded() sets state to kComplete; wait is no-op
WritePreparedTxnDB	Yes	Inherits DBImpl; no impact on creation time queries
Multiple concurrent callers	Yes	Each gets own stack-local once_flag; all wait on same bg_cv_ (standard pattern)
BGWorkAsyncFileOpen failure	Safe but imprecise	State transitions to kComplete regardless; API returns 0 (see M1)

Positive Observations

• Correct synchronization: Reuses established mutex_ + bg_cv_ pattern from destructor wait loop (db_impl.cc:774-783) — proven reliable.
• Lazy evaluation: Modern DBs never block; only legacy DBs without manifest file_creation_time trigger the wait. Good performance preservation.
• FileMetaData sharing insight: Correctly leverages the fact that FileMetaData objects are shared between versions, so readers pinned by BGWorkAsyncFileOpen are visible through any version referencing the same metadata.
• Test design: Tests use deterministic sync point dependencies rather than sleep-based timing, following RocksDB best practices.
• Surgical scope: Minimal change footprint — only touches the affected code paths without architectural disruption.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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 e07ccc3.