DRIVERS-1934: withTransaction API retries too frequently #1851

sleepyStick · 2025-10-17T23:04:58Z

Please complete the following before merging:

Is the relevant DRIVERS ticket in the PR title?

Update changelog.
Test changes in at least one language driver.
Test these changes against all server versions and topologies (including standalone, replica set, and sharded
clusters).

sleepyStick · 2025-10-17T23:06:30Z

source/transactions-convenient-api/transactions-convenient-api.md

 callbacks.

-A previous design had no limits for retrying commits or entire transactions. The callback is always able indicate that
+A previous design had no limits for retrying commits or entire transactions. The callback is always able to indicate that


Maybe I read it wrong, or maybe its a typo? Not sure.

sleepyStick · 2025-10-17T23:08:32Z

source/transactions-convenient-api/transactions-convenient-api.md

        [abortTransaction](../transactions/transactions.md#aborttransaction) on the session.
    2. If the callback's error includes a "TransientTransactionError" label and the elapsed time of `withTransaction` is
-        less than 120 seconds, jump back to step two.
+        less than 120 seconds, sleep for `jitter * min(BACKOFF_INITIAL * (1.25**retry), BACKOFF_MAX)` where: 


I know python uses ** as the exponential operator. I don't believe that is a standard across other programming languages. Is it clear that its exponent? Is there a different preferred symbol for exponent? (I know math commonly uses ^ but it typically also means bitwise XOR in code so I felt like that could be confusing.)

** seems fine to me, but I might also be biased because that's what Node does 🙂. But I don't think people will have trouble understanding what this means.

ShaneHarvey · 2025-10-20T20:47:35Z

source/transactions-convenient-api/tests/README.md

+### Retry Backoff is Enforced
+
+Drivers should test that retries within `withTransaction` do not occur immediately. Ideally, set BACKOFF_INITIAL 500ms
+and configure a failpoint that forces one retry. Ensure that the operation took more than 500ms so succeed.


This test would not work as described because jitter is non-deterministic.

An alternative test would be to use a failpoint to fail the transaction X times and then assert the overall time is larger than some threshold.

Yeah, I realized that when implementing it. I set the fail point to fail 3 times and that seems to be consistently working (and without jitter failing 3 times would still cause the success to happen within 500ms)

update: it was consistent locally but super flakey on atlas. I modified the with transaction code to append backoff values to make this easier to test and the test is now more stable.

source/transactions-convenient-api/transactions-convenient-api.md

sleepyStick · 2025-10-23T17:30:03Z

source/transactions-convenient-api/tests/README.md


+### Retry Backoff is Enforced
+
+Drivers should test that retries within `withTransaction` do not occur immediately. Configure a fail point that forces 3


3 here was a bit of an arbitrary number. The most important part of this value is that its greater than 1
3 just felt like a small enough to be a quick test but big enough to conclude backoff is consistently happening.
If folks have more opinions on this number, I'm not attached to 3.

I'm not sure the _transaction_retry_backoffs concept is viable for testing here since:

many languages don't have a way to implement it without making the attribute public

It's an unbounded list that can grow forever.

there's no prior art for something like this in the driver as far as I know.

Instead I'd suggest my test from earlier where we fail the transaction X times and assert the run time is greater than some threshold T. X should be large enough to reduce false positives where the test fails due to jitter resulting in a small delay for every retry.

We can calculate T by recording the command failed+succeeded events, summing their duration, and adding a fixed constant.

Makes sense, it took me a bit but i figured out what X and T are for python -- I don't know if they'll be the same for the other languages tho? Should the test description leave it as X and T? or should I put in the numbers that I have and see what others have to say about it?

An alternative I'm considering in Node is to configure the random number generator to be deterministic for testing purposes. Because this would make the tests both simpler to reason about and deterministic. ex: make random() always return 1, then we can make assertions on the timing of retries deterministically

Ohhh! I like that idea! Would you want that to replace the current proposed test idea, or is this in addition to the previously defined test?
I'm imaging this as a second test where we fail the transactions a handful of times and calculate the backoff times assuming random() always returns 1 and ensure that the transaction takes longer than the sum of the backoff times.

The main reason to keep the first test (where we don't configure random) is to just ensure that jitter is applied? If so, then should that first test be modified to assert that the transaction succeeds in < T where T is the sum of the maximum backoff times?

Then together we're effectively checking a minimum and maximum threshold on the backoff? Did that make sense?

It depends exactly what we think we need to test here, I think. If the goal is only to test that retry backoff is enforced, I think your current implementation works.

Although I'd be worried about flakiness, because anything that relies on random values is inherently non-determinstic and just due to the distribution of the delays it seems like flakes might be likely:

assuming jitter = 1, the max delay for the test is 3064ms

the last 4 retries account for almost 1750ms of delay in the test (330.8ms, 413.5ms, 500ms, 500ms)

So, short delays in the last few retries could make make the test run a lot shorter than expected. A determinsitic jitter would solve this problem. That, and the test is a bit simpler to reason about imo (I spent a bit of time trying to calculate the probability that this test fails assuming random() returns an equal distribution of values in [0,1], which goes away if we random() is deterministic).

I like the idea of keeping both tests if we do want to make sure jitter is applied, but a determinsitic alternative would be to make random() return a non-1 value (like .5) and assert that the test takes between [total sleep time with jitter = .5, total sleep time with jitter = 1 (or some other value)].

I also know this is feasible in Node, I'm not sure how feasible configuring the random() function would be in other drivers.

I think theoretically the test described by shane works but I agree that flakiness is likely an issue. I tried to account for that with the "optionally change initial_backoff to a higher value" but I don't know how feasible that is across various drivers (hence the optional)

As you've stated, the effect of jitter on the later attempt have a higher impact than the earlier ones, but I generally hope that over time the avg of random should be ~0.5 which sanity checked my initial 1.25 second timeout (discovered through guess and check for better or worse) with random jitter -- but clearly we've been observing that it's not consistent enough across languages

Noting my journey in trying to get values that were consistent in python and seeing that they don't carry over to Node seems to imply that if this test were to actually be implemented, each driver may have to find their own values of X and T? which feels silly imo?

All of that is to say, I think at this point i'm convinced a deterministic approach is the way to go for tests. Just to be clear, you are suggesting that the two test be:

random always returns 1 and assert that the test takes more than total sleep time (with jitter = 1)

random always returns non-1 value, a and assert that the test takes between [total sleep time with jitter = b, total sleep time with jitter = c (or some other value)].
does a = b or can it be such that b <= a? obviously c > a, correct?
how do we want to decide on a, b, and c? There is a part of me that worries if the time difference between total sleep time with c vs total sleep time with a isn't big enough, the rest of the driver operations could total test time > total sleep time with jitter = c? idk i think i'm rambling now.

ShaneHarvey · 2025-10-24T17:48:13Z

source/transactions-convenient-api/tests/README.md


+### Retry Backoff is Enforced
+
+Drivers should test that retries within `withTransaction` do not occur immediately. Configure a fail point that forces 3


I'm not sure the _transaction_retry_backoffs concept is viable for testing here since:

many languages don't have a way to implement it without making the attribute public

It's an unbounded list that can grow forever.

there's no prior art for something like this in the driver as far as I know.

Instead I'd suggest my test from earlier where we fail the transaction X times and assert the run time is greater than some threshold T. X should be large enough to reduce false positives where the test fails due to jitter resulting in a small delay for every retry.

We can calculate T by recording the command failed+succeeded events, summing their duration, and adding a fixed constant.

source/transactions-convenient-api/transactions-convenient-api.md

baileympearson · 2025-10-29T18:44:50Z

source/transactions-convenient-api/transactions-convenient-api.md

    1. If the ClientSession is in the "starting transaction" or "transaction in progress" state, invoke
        [abortTransaction](../transactions/transactions.md#aborttransaction) on the session.
+
    2. If the callback's error includes a "TransientTransactionError" label and the elapsed time of `withTransaction` is
-        less than 120 seconds, jump back to step two.
+        less than 120 seconds, calculate the backoff value to be
+        `jitter * min(BACKOFF_INITIAL * (1.25**retry), BACKOFF_MAX)` where:
+
+        1. jitter is a random float between \[0, 1)
+        2. retry is one less than the number of times Step 2 has been executed since Step 1 was executed
+        3. BACKOFF_INITIAL is 1ms
+        4. BACKOFF_MAX is 500ms
+
+        If the time elapsed thus far plus the backoff value would not exceed 120 seconds, then sleep for the backoff
+        value and jump back to step two, otherwise, raise last known error.
+


I'm not sure why the spec doesn't mention CSOT. We should be factoring timeoutMS into this algorithm, I think it would look something like:

calculate the backoffMS value

if timeoutMS is set and remainingTimeMS < backoffMS: raise last known error

if timeoutMS is not set and elapsed time + backoff > 120, raise last known error

sleep for backoffMS and return to step 2

add exponential backoff to withTransactions API

25759c4

sleepyStick commented Oct 17, 2025

View reviewed changes

run pre-commit

bdcd2ef

ShaneHarvey requested changes Oct 20, 2025

View reviewed changes

sleepyStick added 2 commits October 20, 2025 16:07

add design rational for backoff

48890a2

fix prose test

71ba1ba

sleepyStick commented Oct 23, 2025

View reviewed changes

sleepyStick requested a review from ShaneHarvey October 23, 2025 17:30

ShaneHarvey requested changes Oct 24, 2025

View reviewed changes

baileympearson requested changes Oct 27, 2025

View reviewed changes

source/transactions-convenient-api/transactions-convenient-api.md Outdated Show resolved Hide resolved

sleepyStick added 2 commits October 27, 2025 15:07

fix pseudocode

b602606

fix test

057fbbf

baileympearson reviewed Oct 29, 2025

View reviewed changes

sleepyStick added 2 commits October 29, 2025 15:38

account for CSOT / timeoutMS in algorithm

42e4d94

add more details to tests

c11aef8


		### Retry Backoff is Enforced

		Drivers should test that retries within `withTransaction` do not occur immediately. Configure a fail point that forces 3

DRIVERS-1934: withTransaction API retries too frequently #1851

Are you sure you want to change the base?

DRIVERS-1934: withTransaction API retries too frequently #1851

Uh oh!

Conversation

sleepyStick commented Oct 17, 2025 • edited by ShaneHarvey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

sleepyStick Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sleepyStick commented Oct 17, 2025 •

edited by ShaneHarvey

Loading

sleepyStick Oct 23, 2025 •

edited

Loading