Keep prepared transactions rollbackable after commit write failure

[xingbowang]

Summary

• Restore prepared transactions to PREPARED state when writing the commit marker fails, so callers can still roll them back.
• Preserve PREPARED state when rollback of a prepared transaction hits a retryable write error, allowing rollback to be retried after DB::Resume().
• Update db_stress to clean up prepared transactions after failed commits and report detailed rollback cleanup failure diagnostics.
• Add a WritePrepared regression test covering retryable commit write failure, retryable rollback write failure, successful rollback retry, and DB reopen.

Test Plan

CI

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 311.2s.

[meta-codesync]

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D106202437.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit ac31737

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: 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 ac31737

---

Summary

Well-designed fix that correctly restores transaction state to PREPARED after failed commit/rollback, enabling retry. The core state machine changes are sound. One pre-existing issue in WriteUnprepared becomes more relevant with this change.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Pre-existing unprep_seqs_.clear() in WriteUnprepared rollback becomes actionable — write_unprepared_txn.cc:822

• Issue: WriteUnpreparedTxn::RollbackInternal() clears unprep_seqs_ at line 822 regardless of whether the second WriteImpl (two_write_queues path) succeeded. Before this PR, the state was stuck at AWAITING_ROLLBACK so retry was impossible. With this PR restoring state to PREPARED, retry becomes possible but unprep_seqs_ is empty, making the retry attempt incorrect.
• Root cause: Pre-existing unconditional cleanup in the two-write-queue error path of WriteUnprepared rollback.
• Suggested fix: Move unprep_seqs_.clear() and related cleanup inside the if (s.ok()) block at line 816, or file a follow-up issue. This is outside the scope of this PR but worth noting.

M2. Missing GetState() assertions in test — write_prepared_transaction_test.cc:2363

• Issue: The new test RollbackPreparedAfterCommitWriteFailure does not verify transaction state after each failure. Adding ASSERT_EQ(Transaction::PREPARED, txn->GetState()) after commit failure and after rollback failure would directly validate the core invariant this PR establishes.
• Root cause: Test validates the operations succeed/fail but doesn't assert the state machine transitions.
• Suggested fix: Add state assertions:

  ASSERT_NOK(commit_s);
  ASSERT_EQ(Transaction::PREPARED, txn->GetState());  // NEW
  ...
  ASSERT_NOK(rollback_s);
  ASSERT_EQ(Transaction::PREPARED, txn->GetState());  // NEW

M3. No data visibility verification after rollback — write_prepared_transaction_test.cc:2363

• Issue: The test does not verify that "key" is not readable after successful rollback and before ReOpen. A read check would confirm the rollback actually took effect at the data level, not just the state level.
• Root cause: Test focuses on operational success/failure but not data correctness.
• Suggested fix: Add after successful rollback:

  ASSERT_OK(txn->Rollback());
  {
    ReadOptions ro;
    PinnableSlice val;
    ASSERT_TRUE(db->Get(ro, db->DefaultColumnFamily(), "key", &val).IsNotFound());
  }

🟢 LOW / NIT

L1. API consistency: CommitBatch() does not restore state on failure — pessimistic_transaction.cc:573-578

• Issue: CommitBatch() sets txn_state_.store(AWAITING_COMMIT) at line 574 but does not restore to a usable state on failure. This is inconsistent with the new Commit() behavior. However, CommitBatch() is for non-prepared transactions (STARTED state) where rollback semantics differ.
• Suggested fix: Consider aligning in a follow-up PR, or document the difference.

L2. commit_without_prepare path doesn't restore state — pessimistic_transaction.cc:712-731

• Issue: The commit_without_prepare path calls Clear() before checking status (line 727), then only sets COMMITTED if ok (line 729). On failure, state remains AWAITING_COMMIT and transaction is cleared. This is pre-existing and different from the prepared path, but inconsistent.
• Suggested fix: Consider aligning in a follow-up PR.

L3. Test only covers WritePrepared, not WriteCommitted — write_prepared_transaction_test.cc

• Issue: The new test is parameterized under WritePreparedTransactionTest only. A similar test for WriteCommitted transactions would improve coverage of the state machine fix in PessimisticTransaction::Commit() and Rollback().
• Suggested fix: Add a corresponding test in TransactionTest (which covers WriteCommitted).

L4. Retry loop uses clock_->SleepForMicroseconds() — db_stress_test_base.cc:998

• Issue: CLAUDE.md guidelines discourage sleep in tests due to flakiness. However, this is in db_stress (not unit tests) and the sleep is for error recovery (waiting for Resume to take effect), which is acceptable.
• Suggested fix: No change needed; the context is appropriate.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES	YES — CommitInternal single-write path; no partial commit concern	Safe
WriteUnpreparedTxnDB	YES	MOSTLY — CommitInternal single-write; RollbackInternal two-write path has pre-existing unprep_seqs_ issue (M1)	Follow-up for M1
WriteCommittedTxnDB	YES	YES — CommitInternal/RollbackInternal are simple single writes	Safe
ReadOnly DB	NO	N/A	N/A
Concurrent observers (GetState)	Possible	YES — AWAITING_COMMIT/AWAITING_ROLLBACK were already transient; no code branches on them	Safe
Transaction expiration	NO — expiration_time_ cleared during Prepare	N/A	N/A

Assumption stress-test results:

The claim "CommitInternal two-write path could partially commit" was investigated and found to be unreachable for valid inputs. In both WritePrepared and WriteUnprepared CommitInternal, the two-write path requires includes_data=true, which requires a non-empty commit-time batch without for_recovery=true. But if the batch is non-empty and for_recovery is false, the code returns InvalidArgument before reaching the write path. Therefore do_one_write is always true for valid CommitInternal calls, and partial commit is not a concern.

For RollbackInternal, the two-write path IS reachable (controlled only by two_write_queues option). However, if the first write fails, RollbackInternal returns early before any cleanup, and retry is safe. The M1 finding applies only when the first write succeeds and the second fails.

Positive Observations

• The core fix is minimal and well-targeted: two txn_state_.store(PREPARED) calls in error paths.
• The db_stress retry logic includes good diagnostics (txn name, status strings, retry counts).
• The new test covers the full cycle: commit failure -> resume -> rollback failure -> resume -> rollback success -> reopen.
• The fix correctly does NOT call Clear() on commit failure, preserving locks and write batch for subsequent rollback.
• The serialization reviewer confirmed WAL consistency is maintained in all failure scenarios.

---

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