GetUpdatesSince: Fix assert when polling an empty DB

[evanj]

TransactionLogIterator supports polling the end of the WAL e.g. db_log_iter_test.cc test TransactionLogIteratorStallAtLastRecord. To poll, we need to call Next() on an iterator that is at the end (iter->Valid() == false). However, if the iterator is created when there are no WAL files (e.g. an empty DB), the call to Next() hits an assert in NextImpl:

assert(current_log_reader_);

To fix this, make an iterator without files return a TryAgain status code instead. This should not affect any existing applications, since if they are doing this, they will crash.

transaction_log_impl.cc:

• TransactionLogIteratorImpl::NextImpl: Check for no files and return TryAgain.
• db.h: GetUpdatesSince doc comment: Reference the documentation on TransactionLogIterator.
• transaction_log.h: Clarify how to use TransactionLogIterator.

db_log_iter_test:

• OpenTransactionLogIter: Remove EXPECT_TRUE(iter->Valid()): this is not true if the database is empty.
• TransactionLogIteratorPollNoWALFiles: Test polling the iterator in two cases: the database is empty, and all WAL files were purged. These previously triggered an assertion in Next().

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 36.8s.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 294124c

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 294124c

---

Summary

Clean, minimal fix for a real crash (assertion failure when polling an empty-DB TransactionLogIterator). The approach is correct: returning TryAgain is consistent with the existing polling protocol and all callers handle it properly. Documentation improvements are accurate and valuable.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Removing EXPECT_TRUE(iter->Valid()) from OpenTransactionLogIter weakens existing tests -- db/db_log_iter_test.cc:31

• Issue: The EXPECT_TRUE(iter->Valid()) assertion was removed from the shared helper OpenTransactionLogIter. While necessary for the new empty-DB test, it removes a validity check exercised by 10+ existing call sites that all expect the iterator to be valid. All those tests write data before calling the helper, so the assertion should always pass for them.
• Root cause: The helper now serves two roles: empty-DB testing (Valid()==false) and normal testing (Valid()==true).
• Suggested fix: Add an optional expect_valid parameter (default true) to the helper, or add ASSERT_TRUE(iter->Valid()) at each existing call site, or create a separate helper for the empty-DB case.

M2. TryAgainStatus() uses unnecessary static local -- db/transaction_log_impl.cc:176-179

• Issue: static const char* const msg = "..." inside TryAgainStatus() is safe but unnecessary -- string literals already have static storage duration. Passing the literal directly to Status::TryAgain() is simpler and more consistent with the rest of the codebase.
• Suggested fix: Either inline the literal (return Status::TryAgain("Create a new iterator to fetch the new tail.");) or use a file-level constexpr const char* if sharing between the two call sites is desired.

🟢 LOW / NIT

L1. Next() polling docs could mention TryAgain is terminal -- include/rocksdb/transaction_log.h

• Issue: The Next() method comment says it can poll when status() == OK, but doesn't mention that after TryAgain, the iterator is permanently stuck. The class-level comment covers this, but repeating at the method level would help.

L2. Comment capitalization -- db/transaction_log_impl.cc:183

• Issue: New comment starts lowercase ("if the iterator has no files..."). Most multi-line comments in this file use sentence case.

Cross-Component Analysis

All callers (JNI, db_repl_stress, db_stress, tests) are safe. The fix only triggers on a previously-crashing code path, so no existing behavior changes for working callers. WritePrepared/WriteUnprepared transactions don't use this API (returns NotSupported). ReadOnly DB is safe.

Assumption stress test: All 5 key invariant claims verified correct. files_ is never modified after construction, started_ is provably false in the guarded branch, internal callers skip the new check, and the size_t underflow at line 207 is unreachable with the fix in place.

Positive Observations

• Minimal, focused fix (6 lines of logic) for a real crash
• Semantically correct status code (TryAgain matches existing polling protocol)
• Thorough test covering both empty-DB and post-purge scenarios
• Valuable documentation upgrade for the previously underdocumented polling protocol

---

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