Skip to content

Commit 3dcf15e

Browse files
antiguruclaude
andauthored
Mark idempotent FoundationDB transactions as retryable (#37062)
### Motivation FoundationDB transactions can fail with a `commit_unknown_result` error, indicating the transaction may or may not have committed. Currently, all transactions in the timestamp oracle use `TransactOption::default()`, which causes these errors to propagate as failures rather than being retried. Several transactions in the timestamp oracle are idempotent and safe to retry on this error: - Read-only transactions (they have no side effects) - Transactions that use `GREATEST()` to monotonically advance values (retries only move the value further forward) This change improves reliability by allowing safe retries instead of failures. Closes PER-25. ### Description Updated four transaction sites in `src/timestamp-oracle/src/foundationdb_oracle.rs` to use `TransactOption::idempotent()` instead of `TransactOption::default()`: 1. **`max_read_ts()`** — Pure read transaction, safe to retry 2. **`write_ts()` in `TimestampOracle::next()`** — Uses `max(write_ts + 1, proposed_next_ts)`, so retries only advance the timestamp further while preserving monotonicity 3. **`peek_write_ts()`** — Pure read transaction, safe to retry 4. **`read_ts()`** — Pure read transaction, safe to retry 5. **`apply_write()`** — Uses `GREATEST(current, write_ts)` to bump values, making it idempotent; retries cannot cause regression Each change includes a comment explaining why the transaction is safe to retry on `commit_unknown_result` errors. ### Verification No new tests added — this change improves error handling for existing transaction patterns. The idempotency properties are documented in the code comments and follow from the transaction logic (read-only operations and monotonic value updates via `GREATEST()`). https://claude.ai/code/session_012DArBrNjtGVVBSdTomhsuF Co-authored-by: Claude <noreply@anthropic.com>
1 parent e0f4198 commit 3dcf15e

1 file changed

Lines changed: 20 additions & 5 deletions

File tree

src/timestamp-oracle/src/foundationdb_oracle.rs

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,10 @@ impl<N: Sync> FdbTimestampOracle<N> {
234234
.transact_boxed(
235235
&(),
236236
|trx, ()| self.max_rs_trx(trx).boxed(),
237-
TransactOption::default(),
237+
// This transaction is a pure read, so it is safe to retry on a
238+
// `commit_unknown_result` ("transaction may or may not have
239+
// committed") error.
240+
TransactOption::idempotent(),
238241
)
239242
.await?;
240243
Ok(max_ts)
@@ -482,7 +485,12 @@ where
482485
.transact_boxed(
483486
&proposed_next_ts,
484487
|trx, proposed_next_ts| self.write_ts_trx(trx, **proposed_next_ts).boxed(),
485-
TransactOption::default(),
488+
// Safe to retry on a `commit_unknown_result` error: the
489+
// transaction re-reads `write_ts` and hands out
490+
// `max(write_ts + 1, proposed_next_ts)`, so a retry only
491+
// ever advances the timestamp further, preserving
492+
// monotonicity (it may skip a timestamp, which is fine).
493+
TransactOption::idempotent(),
486494
)
487495
.await
488496
.map_err(anyhow::Error::from)
@@ -515,7 +523,8 @@ where
515523
.transact_boxed(
516524
&(),
517525
|trx, ()| self.peek_write_ts_trx(trx).boxed(),
518-
TransactOption::default(),
526+
// Pure read, safe to retry on `commit_unknown_result`.
527+
TransactOption::idempotent(),
519528
)
520529
.await
521530
.map_err(anyhow::Error::from)?
@@ -544,7 +553,8 @@ where
544553
.transact_boxed(
545554
&(),
546555
|trx, ()| self.read_ts_trx(trx).boxed(),
547-
TransactOption::default(),
556+
// Pure read, safe to retry on `commit_unknown_result`.
557+
TransactOption::idempotent(),
548558
)
549559
.await
550560
.map_err(anyhow::Error::from)?
@@ -576,7 +586,12 @@ where
576586
.transact_boxed(
577587
&write_ts,
578588
|trx, write_ts| self.apply_write_trx(trx, **write_ts).boxed(),
579-
TransactOption::default(),
589+
// `apply_write_trx` only bumps `read_ts`/`write_ts` to
590+
// `GREATEST(current, write_ts)`, so it is idempotent and
591+
// safe to retry on a `commit_unknown_result` ("transaction
592+
// may or may not have committed") error rather than
593+
// panicking. This is the failure observed in PER-25.
594+
TransactOption::idempotent(),
580595
)
581596
.await
582597
.map_err(anyhow::Error::from)

0 commit comments

Comments
 (0)