Flexible retry backoff policies #182

MattWhelan · 2025-06-09T23:17:08Z

Extends the existing retry limit mechanism to allow for delays between retries. Includes implementations for fixed delays and exponential backoff.

cockroach-teamcity · 2025-06-09T23:17:16Z

This change is

dikshant · 2025-06-10T20:58:05Z

We haven’t implemented this because cockroachdb-go uses save point based retries and because of savepoint-based retries, a transaction can continue holding locks in between retries. So that means with exponential backoff, you'll be holding locks for longer, thus putting the app at even greater risk of contention.

If you could show us that this is not the case with backoffs, we will consider adding this! Thanks!

MattWhelan · 2025-06-10T22:13:21Z

We haven’t implemented this because cockroachdb-go uses save point based retries and because of savepoint-based retries, a transaction can continue holding locks in between retries. So that means with exponential backoff, you'll be holding locks for longer, thus putting the app at even greater risk of contention.

If you could show us that this is not the case with backoffs, we will consider adding this! Thanks!

I just pushed 9dda97b. Could you take a look at that? I'm not sure what all the implications are of separating the database transaction lifecycle from the Tx object, but aside from that, I think it's sound, and avoids the problem with holding locks during backoff.

rafiss · 2025-07-03T21:26:07Z

crdb/retry.go

+}
+
+// Vargo converts a go-retry style Delay provider into a RetryPolicy
+func Vargo(fn func() VargoBackoff) RetryPolicy {


I'm wondering if we could think of a different name to go with here. Users of cockroach-go who don't already know about the sethvargo/go-retry library may be confused to see this.

I'm open to suggestions. I was trying to build an explicit integration without adding a transitive dependency.

rafiss · 2025-07-03T21:36:55Z

crdb/common.go

+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)
+			}
+			if restartErr := tx.Exec(ctx, "BEGIN"); restartErr != nil {


This seems like it could have issues. Some libraries may not like it if we use the transaction after rolling it back. For example, I see this in the pgx code where it returns an error if you try to use a transaction that was already rolled back: https://github.com/jackc/pgx/blob/ecc9203ef42fbba50507e773901b5aead75288ef/tx.go#L205-L224

It returns an error if you use a transaction object that has the closed flag set. Since we're not doing that, it ought to be ok, I would expect.

Extends the existing retry limit mechanism to allow for delays between retries. Includes implementations for fixed delays and exponential backoff.

rafiss · 2025-10-21T15:27:41Z

crdb/tx.go

 // WithMaxRetries configures context so that ExecuteTx retries tx specified
 // number of times when encountering retryable errors.
-// Setting retries to 0 will retry indefinitely.
+// Setting retries to 0 will not retry: the transaction will be tried only once.


I think we should try to avoid breaking the API here. This changes the behavior of WithMaxRetries(ctx, 0) from "retry indefinitely" to "no retries," which would affect any existing code relying on this.

Maybe we could add a separate WithNoRetries() function, and change something about the internal representation so that the outward facing API remains the same.

Agree. Done.

rafiss · 2025-10-21T15:30:55Z

crdb/common.go

+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)
+			}
+			if restartErr := tx.Exec(ctx, "BEGIN"); restartErr != nil {


This still doesn't seem valid to me -- if we reuse the same tx object here, it will not succeed when sending BEGIN right after a ROLLBACK. (see https://github.com/jackc/pgx/blob/ecc9203ef42fbba50507e773901b5aead75288ef/tx.go#L205-L224)

My understanding is that we not calling tx.Rollback, but rather tx.Exec(ctx, "ROLLBACK"). Please check the comments in the code, and let me know if that makes sense.

rafiss · 2025-10-21T15:32:28Z

crdb/common.go

-			return newTxnRestartError(rollbackErr, err)
+		// We have a retryable error. Check the retry policy.
+		delay, retryErr := retryFunc(err)
+		if delay > 0 && retryErr == nil {


I don't follow this. Why are we doing a retry if retryErr is nil?

I added documentation on the RetryFunc function definition to clarify the semantics. Implementation of RetryFunc may return errors (typically MaxRetriesExceededError) to stop the retry process.

rafiss · 2025-10-21T15:35:04Z

crdb/common.go

+		if delay > 0 && retryErr == nil {
+			// We don't want to hold locks while waiting for a backoff, so restart the entire transaction
+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)


This is the wrong error to return here -- as per this line, when this error is displayed, it will say that there was an issue with ROLLBACK TO SAVEPOINT, which is not what this line is doing:

cockroach-go/crdb/error.go

Line 70 in c787987

const msgPattern = "restarting txn failed. ROLLBACK TO SAVEPOINT " +

(This is also the wrong error to return on lines 98 and 101.)

Done. Passing the operation to newTxnRestartError.

rafiss · 2025-10-21T15:37:11Z

crdb/retry_test.go

+		}
+	}
+}
+


I think we should also have tests for edge cases like zero BaseDelay, negative RetryLimit, and MaxDelay < BaseDelay.

rafiss · 2025-10-21T15:38:07Z

crdb/common.go

-		if rollbackErr := tx.Exec(ctx, "ROLLBACK TO SAVEPOINT cockroach_restart"); rollbackErr != nil {
-			return newTxnRestartError(rollbackErr, err)
+		// We have a retryable error. Check the retry policy.
+		delay, retryErr := retryFunc(err)


before we enter the retry logic, we should check for context cancellation:

if err := ctx.Err(); err != nil { return err }

rafiss · 2025-10-21T15:39:23Z

crdb/retry.go

+	NewRetry() RetryFunc
+}
+
+type LimitBackoffRetryPolicy struct {


This is public-facing so should have a comment explaining what it is and how to use it.

rafiss · 2025-10-21T15:39:57Z

crdb/retry.go

+}
+
+// ExpBackoffRetryPolicy implements RetryPolicy using an exponential backoff with optional
+// saturation.


I don't follow what "saturation" means here. Can we write this comment to explain more how to use this?

Done, reworded comment.

rafiss · 2025-10-21T15:42:02Z

crdb/retry_test.go

+	actualDelays := make([]time.Duration, 0, len(expectedDelays))
+	rf := policy.NewRetry()
+	for {
+		delay, err := rf(nil)


We're always passing a nil error here -- can we add tests that check the behavior with a non-nil error?

Document LimitBackoffRetryPolicy, ExpBackoffRetryPolicy, and Vargo adapter with detailed examples. Preserve backward compatibility by making WithMaxRetries(ctx, 0) mean unlimited retries (original behavior). Add WithNoRetries() for disabling retries and introduce sentinel constants. Enhance RetryFunc documentation to clarify return value semantics and add additional test cases.

sravotto

Thanks for the review. Please take another look (will squash the commits once we are done).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @sean-)

sravotto · 2025-10-23T18:06:34Z

crdb/common.go

-		if rollbackErr := tx.Exec(ctx, "ROLLBACK TO SAVEPOINT cockroach_restart"); rollbackErr != nil {
-			return newTxnRestartError(rollbackErr, err)
+		// We have a retryable error. Check the retry policy.
+		delay, retryErr := retryFunc(err)


sravotto · 2025-10-23T18:06:34Z

crdb/common.go

-			return newTxnRestartError(rollbackErr, err)
+		// We have a retryable error. Check the retry policy.
+		delay, retryErr := retryFunc(err)
+		if delay > 0 && retryErr == nil {


I added documentation on the RetryFunc function definition to clarify the semantics. Implementation of RetryFunc may return errors (typically MaxRetriesExceededError) to stop the retry process.

sravotto · 2025-10-23T18:06:35Z

crdb/common.go

+		if delay > 0 && retryErr == nil {
+			// We don't want to hold locks while waiting for a backoff, so restart the entire transaction
+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)


Done. Passing the operation to newTxnRestartError.

sravotto · 2025-10-23T18:06:35Z

crdb/common.go

+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)
+			}
+			if restartErr := tx.Exec(ctx, "BEGIN"); restartErr != nil {


My understanding is that we not calling tx.Rollback, but rather tx.Exec(ctx, "ROLLBACK"). Please check the comments in the code, and let me know if that makes sense.

sravotto · 2025-10-23T18:06:36Z

crdb/retry.go

+	NewRetry() RetryFunc
+}
+
+type LimitBackoffRetryPolicy struct {


sravotto · 2025-10-23T18:06:36Z

crdb/retry.go

+}
+
+// ExpBackoffRetryPolicy implements RetryPolicy using an exponential backoff with optional
+// saturation.


Done, reworded comment.

sravotto · 2025-10-23T18:06:37Z

crdb/retry_test.go

+	actualDelays := make([]time.Duration, 0, len(expectedDelays))
+	rf := policy.NewRetry()
+	for {
+		delay, err := rf(nil)


sravotto · 2025-10-23T18:06:37Z

crdb/retry_test.go

+		}
+	}
+}
+


sravotto · 2025-10-23T18:06:38Z

crdb/tx.go

 // WithMaxRetries configures context so that ExecuteTx retries tx specified
 // number of times when encountering retryable errors.
-// Setting retries to 0 will retry indefinitely.
+// Setting retries to 0 will not retry: the transaction will be tried only once.


Agree. Done.

MattWhelan force-pushed the backoff branch 2 times, most recently from 3034fdf to d7039d1 Compare June 10, 2025 15:32

MattWhelan requested a review from sean- June 10, 2025 16:01

MattWhelan force-pushed the backoff branch from d7039d1 to 4f09d74 Compare June 10, 2025 16:33

MattWhelan requested a review from rafiss June 23, 2025 20:05

rafiss reviewed Jul 3, 2025

View reviewed changes

Matt Whelan added 2 commits October 21, 2025 14:33

Flexible retry backoff policies

0a27be5

Extends the existing retry limit mechanism to allow for delays between retries. Includes implementations for fixed delays and exponential backoff.

Avoid SAVEPOINT with backoff

d408129

sravotto force-pushed the backoff branch from 79348da to d408129 Compare October 21, 2025 12:33

rafiss requested changes Oct 21, 2025

View reviewed changes

sravotto force-pushed the backoff branch from 782ff75 to 93bb226 Compare October 23, 2025 17:16

sravotto reviewed Oct 23, 2025

View reviewed changes

Flexible retry backoff policies #182

Are you sure you want to change the base?

Flexible retry backoff policies #182

Uh oh!

Conversation

MattWhelan commented Jun 9, 2025

Uh oh!

cockroach-teamcity commented Jun 9, 2025

Uh oh!

dikshant commented Jun 10, 2025

Uh oh!

MattWhelan commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sravotto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees