Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ async fn unbounded_compaction_read_ages_out_bounded_survives() -> Result<()> {
let aged_out = err.chain().any(|cause| {
matches!(
cause.downcast_ref::<DatabaseError>(),
Some(DatabaseError::TransactionTooOld | DatabaseError::MaxRetriesReached)
Some(DatabaseError::TransactionTooOld | DatabaseError::MaxRetriesReached(_))
)
});
assert!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ async fn hot_input_read_stays_bounded_at_byte_scale() -> Result<()> {
let aged_out = err.chain().any(|cause| {
matches!(
cause.downcast_ref::<DatabaseError>(),
Some(DatabaseError::TransactionTooOld | DatabaseError::MaxRetriesReached)
Some(DatabaseError::TransactionTooOld | DatabaseError::MaxRetriesReached(_))
)
});
assert!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ async fn get_pages_ages_out_unbounded_bounded_survives() -> Result<()> {
let aged_out = err.chain().any(|cause| {
matches!(
cause.downcast_ref::<DatabaseError>(),
Some(DatabaseError::TransactionTooOld | DatabaseError::MaxRetriesReached)
Some(DatabaseError::TransactionTooOld | DatabaseError::MaxRetriesReached(_))
)
});
assert!(
Expand Down
29 changes: 23 additions & 6 deletions engine/packages/universaldb/src/driver/postgres/commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,20 @@ async fn submit_local(

if commit_tx.send(job).await.is_err() {
// The leader drain loop is gone (driver shutting down). Retryable.
return Err(DatabaseError::NotCommitted.into());
return Err(
anyhow::Error::from(DatabaseError::NotCommitted).context("leader drain loop is gone")
);
}

match response_rx.await {
Ok(CommitOutcome::Committed { .. }) => Ok(()),
Ok(CommitOutcome::Conflict) => Err(DatabaseError::NotCommitted.into()),
// The leader resolved this commit as a loser. A cold-window rejection during leader recovery
// arrives as the same outcome, so the leader's batch log is what separates the two.
Ok(CommitOutcome::Conflict) => Err(anyhow::Error::from(DatabaseError::NotCommitted)
.context("leader resolved the commit as a conflict")),
// The leader dropped the job without responding; it was not applied.
Err(_) => Err(DatabaseError::NotCommitted.into()),
Err(_) => Err(anyhow::Error::from(DatabaseError::NotCommitted)
.context("leader dropped the commit without responding")),
}
}

Expand Down Expand Up @@ -128,7 +134,9 @@ async fn submit_nats(
return Ok(());
}
Ok(CommitOutcome::Conflict) => {
return Err(DatabaseError::NotCommitted.into());
// As in the single-node path, a cold-window rejection is reported as a conflict.
return Err(anyhow::Error::from(DatabaseError::NotCommitted)
.context("leader resolved the commit as a conflict"));
}
Err(err) => {
tracing::warn!(?err, client_seq, "malformed udb commit reply; resending");
Expand Down Expand Up @@ -161,7 +169,11 @@ async fn submit_nats(
wait_ms = submit_start.elapsed().as_millis() as u64,
"udb commit exhausted resend attempts; treating as not committed"
);
Err(DatabaseError::NotCommitted.into())
Err(
anyhow::Error::from(DatabaseError::NotCommitted).context(format!(
"exhausted {MAX_SUBMIT_ATTEMPTS} commit resend attempts without a determinate reply"
)),
)
}

/// Wait for a known leader, returning a retryable error if none is elected in time.
Expand All @@ -172,7 +184,12 @@ async fn wait_for_leader(shared: &Arc<PostgresShared>) -> Result<LeaseInfo> {
return Ok(lease);
}
if Instant::now() >= deadline {
return Err(DatabaseError::NotCommitted.into());
return Err(
anyhow::Error::from(DatabaseError::NotCommitted).context(format!(
"no leader elected within {}s",
LEADER_WAIT_TIMEOUT.as_secs()
)),
);
}
tokio::time::sleep(LEADER_POLL_INTERVAL).await;
}
Expand Down
38 changes: 20 additions & 18 deletions engine/packages/universaldb/src/driver/postgres/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,18 +339,6 @@ impl DatabaseDriver for PostgresDatabaseDriver {

let mut attempt = 0;
loop {
// Re-read every iteration. The first attempt always runs, because nothing has called
// `retry_limit` yet; from then on the closure's limit wins over the database-wide one.
let limit = retry_limit.load(Ordering::SeqCst);
let max_attempts = if limit == RETRY_LIMIT_UNSET {
max_retries
} else {
limit.saturating_add(1)
};
if attempt >= max_attempts {
break;
}

let tx = Transaction::new(Arc::new(PostgresTransactionDriver::with_retry_limit(
self.shared.clone(),
retry_limit.clone(),
Expand Down Expand Up @@ -380,17 +368,31 @@ impl DatabaseDriver for PostgresDatabaseDriver {
maybe_committed = MaybeCommitted(true);
}

// Re-read every iteration. Nothing has called `retry_limit` before the first
// attempt; from then on the closure's limit wins over the database-wide one.
// The check runs after an attempt failed, so both values bound retries rather
// than total attempts.
let limit = retry_limit.load(Ordering::SeqCst);
let retry_budget = if limit == RETRY_LIMIT_UNSET {
max_retries
} else {
limit
};
if attempt >= retry_budget {
return Err(DatabaseError::MaxRetriesReached(error).into());
}

attempt += 1;

let backoff_ms = calculate_tx_retry_backoff(attempt as usize);
tokio::time::sleep(tokio::time::Duration::from_millis(backoff_ms)).await;
attempt += 1;
continue;
} else {
return Err(error);
}
} else {
return Err(error);
}

return Err(error);
}

Err(DatabaseError::MaxRetriesReached.into())
})
}

Expand Down
38 changes: 20 additions & 18 deletions engine/packages/universaldb/src/driver/rocksdb/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,6 @@ impl DatabaseDriver for RocksDbDatabaseDriver {

let mut attempt = 0;
loop {
// Re-read every iteration. The first attempt always runs, because nothing has called
// `retry_limit` yet; from then on the closure's limit wins over the database-wide one.
let limit = retry_limit.load(Ordering::SeqCst);
let max_attempts = if limit == RETRY_LIMIT_UNSET {
max_retries
} else {
limit.saturating_add(1)
};
if attempt >= max_attempts {
break;
}

let tx = Transaction::new(Arc::new(RocksDbTransactionDriver::with_retry_limit(
self.db.clone(),
self.txn_conflict_tracker.clone(),
Expand Down Expand Up @@ -121,17 +109,31 @@ impl DatabaseDriver for RocksDbDatabaseDriver {
maybe_committed = MaybeCommitted(true);
}

// Re-read every iteration. Nothing has called `retry_limit` before the first
// attempt; from then on the closure's limit wins over the database-wide one.
// The check runs after an attempt failed, so both values bound retries rather
// than total attempts.
let limit = retry_limit.load(Ordering::SeqCst);
let retry_budget = if limit == RETRY_LIMIT_UNSET {
max_retries
} else {
limit
};
if attempt >= retry_budget {
return Err(DatabaseError::MaxRetriesReached(error).into());
}

attempt += 1;

let backoff_ms = calculate_tx_retry_backoff(attempt as usize);
tokio::time::sleep(tokio::time::Duration::from_millis(backoff_ms)).await;
attempt += 1;
continue;
} else {
return Err(error);
}
} else {
return Err(error);
}

return Err(error);
}

Err(DatabaseError::MaxRetriesReached.into())
})
}

Expand Down
8 changes: 5 additions & 3 deletions engine/packages/universaldb/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ pub enum DatabaseError {
#[error("transaction is too old to perform reads or be committed")]
TransactionTooOld,

#[error("max number of transaction retries reached")]
MaxRetriesReached,
// Stores the last error. The alternate format prints the whole context chain, so the cause the
// context names is reported alongside the underlying variant.
#[error("max number of transaction retries reached, last error: {0:#}")]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

MaxRetriesReached(anyhow::Error),

#[error("operation issued while a commit was outstanding")]
UsedDuringCommit,
Expand All @@ -22,7 +24,7 @@ impl DatabaseError {
use DatabaseError::*;

match self {
NotCommitted | TransactionTooOld | MaxRetriesReached => true,
NotCommitted | TransactionTooOld | MaxRetriesReached(_) => true,
_ => false,
}
}
Expand Down
2 changes: 1 addition & 1 deletion engine/packages/universaldb/tests/conflict_parity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ async fn writer_conflicts_when_a_key_it_read_was_written(db: Database) {
assert!(
err.chain().any(|x| matches!(
x.downcast_ref::<universaldb::error::DatabaseError>(),
Some(universaldb::error::DatabaseError::MaxRetriesReached)
Some(universaldb::error::DatabaseError::MaxRetriesReached(_))
)),
"expected the conflict to exhaust retries, got {err:?}"
);
Expand Down
Loading