fix(universaldb): report the last error when transaction retries are exhausted

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/actors

• #5704 fix(universaldb): run postgres transaction reads concurrentl …
• #5705 test(gasoline): reproduce slow workflow pulls on postgres
• #5702 fix(universaldb): chunk postgres commit requests that exceed …
• #5703 fix(universaldb): clear postgres key ranges one statement pe …
• #5706 perf(universaldb): time each drain batch statement to locali …
• #5695 fix(universaldb): report the last error when transaction ret … 👈
• main

Get stack: forklift get 5695
Push local edits: forklift submit
Merge when ready: forklift merge 5695

_{change otvmloun}

[railway-app]

🚅 Deployed to the actors-pr-5695 environment in rivet-frontend

Service	Status	Web	Updated
frontend-cloud	😴 Sleeping (View Logs)	Web	Sep 13, 2026 at 4:36 am UTC
kitchen-sink	😴 Sleeping (View Logs)	Web	Sep 12, 2026 at 2:53 am UTC
website	❌ Build Failed (View Logs)	Web	Sep 11, 2026 at 9:34 pm UTC
frontend-inspector	😴 Sleeping (View Logs)	Web	Sep 11, 2026 at 6:00 pm UTC
mcp-hub	✅ Success (View Logs)	Web	Sep 10, 2026 at 7:47 pm UTC
ladle	✅ Success (View Logs)	Web	Sep 10, 2026 at 7:47 pm UTC

[the-company-company]

🟠 1 medium-severity finding

_{Reviewed commit 487b538.}

[the-company-company]

🟠 Medium · Preserve the last error in the error source chain

The tuple field is formatted into this error, but it is not marked as a thiserror source. Consequently anyhow::Error::chain() stops at MaxRetriesReached and callers cannot inspect or downcast the retry-exhausting TransactionTooOld, NotCommitted, or its contextual cause programmatically; only the rendered alternate display includes it.

Mark the wrapped anyhow::Error with #[source] (or use a named source field) so the reported cause remains part of the standard error chain.

[claude]

Review

Reviewed the diff for fix(universaldb): report the last error when transaction retries are exhausted (head c04064763322d5fde344640bfce035dc1c25c276).

Likely bug: retry-loop refactor changes attempt count and backoff timing (unrelated to the stated fix)

In both engine/packages/universaldb/src/driver/postgres/database.rs and .../rocksdb/database.rs, the retry-budget check moved from before each attempt to after an attempt fails, and attempt += 1 was reordered to happen before the backoff calculation instead of after. Two side effects fall out of that reordering that look unintentional given the PR is only supposed to add error context:

1. Off-by-one increase in total attempts for the database-wide (unset) retry budget. Old code pre-checked attempt >= max_attempts (where max_attempts == max_retries when unset) before running the next attempt, so max_retries was the total attempt count. New code checks attempt >= retry_budget (same max_retries value, no +1) only after an attempt has already run and failed, using the pre-increment attempt. Tracing it through (e.g. max_retries = 2): attempt [SVC-2555] Set up issue templates #1 runs, fails, check 0 >= 2 → false, retry; attempt [SVC-2479] Send cluster events to PostHog #2 runs, fails, check 1 >= 2 → false, retry; attempt [SVC-2504] Fix 5 GB upload limit for local development from Cloudflare #3 runs, fails, check 2 >= 2 → true, return. That's 3 total attempts where the old code produced 2. So every transaction using the default (unset) retry budget — the common case, since most callers never call tx.retry_limit(..) — now runs one extra attempt before giving up. With the default max_retries = 10, a persistently conflicting transaction now takes 11 attempts instead of 10. (The per-transaction retry_limit(N) path is unaffected: it already used a +1 offset in the old code, which happens to cancel out with the new check position, so retry_limit_allows_that_many_retries still passes.)

2. Backoff schedule shifted one step later. Old order was compute backoff (using current attempt) -> sleep -> attempt += 1. New order is attempt += 1 -> compute backoff (using the incremented attempt) -> sleep. Since calculate_tx_retry_backoff is 2^attempt * 10ms, this means the first retry now waits with 2^1 instead of 2^0 as its base (20ms vs 10ms), the second waits 2^2 instead of 2^1, and the backoff cap (2^7) is now reached one retry earlier than before.

Neither behavior is mentioned in the commit message, and neither is covered by an existing test: unset_retry_limit_still_retries (engine/packages/universaldb/tests/retry_limit.rs) only asserts attempts > 1, which doesn't pin the exact count, and the db.txn_retry_limit(1) calls in conflict_parity.rs/depot/tests/*.rs only assert that MaxRetriesReached eventually occurs, not how many attempts it took. If the count/backoff change is intentional (e.g. to make the default budget consistent with the "N retries after the first attempt" semantics documented for retry_limit), it'd be worth calling that out explicitly and adding a test mirroring retry_limit_allows_that_many_retries for the unset/database-wide case (e.g. asserting the exact attempt count under a small txn_retry_limit). If it's not intentional, the retry-budget check can stay where it moved to (it needs error in scope to build MaxRetriesReached), while keeping the backoff computed before attempt += 1, matching the old order, and adjusting the unset-branch budget by one to preserve the old total-attempt count.

Other notes

• The commit.rs changes (attaching .context(...) to each DatabaseError::NotCommitted site) look good and match the "prefer .context() over the anyhow! macro" guidance. They preserve DatabaseError in the anyhow::Error chain, so the existing chain().find_map(downcast_ref::<DatabaseError>()) / is_retryable() checks upstream still work correctly.
• Test updates in depot/tests/*.rs and universaldb/tests/conflict_parity.rs correctly follow MaxRetriesReached becoming a tuple variant.
• Test coverage gap: no test asserts that the new MaxRetriesReached message actually contains the wrapped error's text (e.g. a conflict message surfacing in the final error). Given the PR's whole purpose is surfacing that last error, a small test asserting the returned error's Display/chain contains the original failure reason would directly validate the fix; the existing conflict_parity.rs test only checks the variant via MaxRetriesReached(_), not its payload.
• Minor/pre-existing, not introduced here: DatabaseError::is_retryable uses a _ => false catch-all over the enum's remaining variants, which the repo's "never use _ fall-through on an enum match" rule would flag. Since this exact match arm is being touched to add (_) for MaxRetriesReached, it may be worth enumerating UsedDuringCommit and RetryLimitUnsupported explicitly while it's already being edited.

Nothing else stood out; the rest of the diff (error type change, call-site updates) is mechanical and consistent.

🤖 Generated with Claude Code

[the-company-company]

🟠 1 medium-severity finding

_{Reviewed commit c040647.}

[the-company-company]

🟠 Medium · Preserve the last error in the error source chain

The tuple field is formatted into this error, but it is not marked as a thiserror source. Consequently anyhow::Error::chain() stops at MaxRetriesReached and callers cannot inspect or downcast the retry-exhausting TransactionTooOld, NotCommitted, or its contextual cause programmatically; only the rendered alternate display includes it.

Mark the wrapped anyhow::Error with #[source] (or use a named source field) so the reported cause remains part of the standard error chain.