JAVA-5950 Update Transactions Convenient API with exponential backoff on retries

[nhachicha]

Relevant specification changes:

• DRIVERS-1934: withTransaction API retries too frequently (#1851)
• DRIVERS-1934: clarify drivers back off before all transaction retries (#1868) - the change has not yet been addressed in this PR, because it was introduced after creation of the PR.
• DRIVERS-3364: Fix Retry Backoff is Enforced prose test. - the change has not yet been addressed in this PR, because it was introduced after creation of the PR.

JAVA-5950, JAVA-6046

[Copilot]

Pull request overview

This PR implements exponential backoff with jitter for transaction retries in MongoDB's withTransaction convenience API. The implementation adds a configurable backoff mechanism that applies delays between retry attempts when transient transaction errors occur, following the MongoDB specification with a growth factor of 1.5 for transactions.

Key Changes

• Introduces ExponentialBackoff utility class with factory methods for transaction retries (5ms base, 500ms max, 1.5x growth) and command retries (100ms base, 10s max, 2.0x growth)
• Integrates backoff logic into ClientSessionImpl.withTransaction() to delay between retry attempts
• Adjusts test configuration to verify backoff behavior with multiple retry attempts

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
driver-core/src/main/com/mongodb/internal/ExponentialBackoff.java	New utility class implementing exponential backoff with jitter using ThreadLocalRandom
driver-sync/src/main/com/mongodb/client/internal/ClientSessionImpl.java	Adds backoff delay before transaction retries and uses CSOT timeout when available
driver-core/src/test/unit/com/mongodb/internal/ExponentialBackoffTest.java	Comprehensive unit tests validating backoff calculations, growth factors, and maximum caps
driver-sync/src/test/functional/com/mongodb/client/WithTransactionProseTest.java	New functional test verifying exponential backoff behavior and adjusted existing test configuration

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The test verifies the retry count but does not validate that exponential backoff delays are actually applied. Consider measuring elapsed time and asserting minimum expected delays to ensure backoff is functioning correctly. For example, with 3 retries at delays of ~5ms, ~7.5ms, and ~11.25ms, the total elapsed time should be at least the sum of minimum expected delays.

[nhachicha]

ExponentialBackoffTest covers these unit tests already.

[dariakp]

@nhachicha Please take note of mongodb/specifications#1868

[stIncMale]

• I haven't reviewed ExponentialBackoffTest, because it depends on ExponentialBackoff, where I left many suggestions.
• I haven't reviewed ClientSessionImpl, because it has to implement the new specification change DRIVERS-1934: clarify drivers back off before all transaction retries (#1868).
• The last reviewed commit is 90ec4d5.

[stIncMale]

Within this PR, I failed to find any benefits of expressing the backoff computation as behavior of an object (an instance of ExponentialBackoff), rather than just a static method; regardless of the approach, the ClientSessionImpl.withTransaction method has to maintain one new local variable: either the lazily initialized transactionBackoff, or the transactionAttempt (that's how it is named in the spec, but we are free to use a different name). Given this, I propose to go with the more straightforward approach of expressing the backoff computation as a static method¹, rather than as an object behavior.

If in the future we observe that this is not enough and the "object" approach is needed, we'll be able to change the approach. But we will have a clear reason for that.

Also, storing all the constants in each instance of ExponentialBackoff as instance fields is unnecessary. If we have to use the "object" approach in the future, we better implement it in such a way that does not duplicate constants as instance fields in each object instance (an interface / abstract class with two implementations is one way to achieve that).

P.S. Some other review comments I left are written within the current "object" approach (as opposed to he proposed "method" approach). If the suggestion in this comment is applied, those comments should not be automatically discarded, but rather each should be examined and adopted, if applicable, to the "method" approach.

---

¹ If in the future we need another static method for command retries, we will be able to move the computational logic in a private static method, and call that method from two public methods, passing the suitable constants as method arguments.

[nhachicha]

I agree with your points (especially the memory overhead of the constants and the state management), however, the current approach also has the following benefit:

• Encapsulation: Retry count management is internal - caller can't forget to increment
• Configuration bundling: forTransactionRetry() vs forCommandRetry() clearly separates concerns
• Type safety: Can't accidentally mix transaction/command constants

I pushed a "middle-ground" stateless solution based on Enum, which only defines convenient transaction retries 👍

[stIncMale]

The new code is better, but the above description of benefits the current enum approach provides is flawed. The current enum approach is actually conceptually equivalent to the approach with the static method I proposed, but achieves the same in a more complex way (from the source code standpoint), with a potentially less efficient memory organization, and probably allows fewer JIT/javac optimizations¹.

• Encapsulation: Retry count management is internal - caller can't forget to increment
  • I like encapsulation, but the current enum approach cannot handle counting internally, and does not do that: retryCount is passed to the instance method ExponentialBackoff.calculateDelayBeforeNextRetryMs.
  • Note also that the way the overload retry policy is formulated at the moment (still work in progress), does not even allow us to encapsulate retry counting in ExponentialBackoff, because generally speaking backoff does not need to be computed/applied to each retry attempt, but the retry attempt index used to compute the backoff is the index of the retry attempt in the sequence of all retry attempts, regardless of what retry policy an attempt was executed under. That is, computing the backoff and counting retry attempts have to be done independently of each other. But if ExponentialBackoff were to encapsulate the counting, then they could only have been done together.
• Configuration bundling: forTransactionRetry() vs forCommandRetry() clearly separates concerns
  • With the current enum approach, we'll have ExponentialBackoff.TRANSACTION.calculateDelayBeforeNextRetryMs(retryCount) vs ExponentialBackoff.COMMAND.calculateDelayBeforeNextRetryMs(retryCount), as opposed to what you wrote. With the static method approach, we'll have ExponentialBackoff.forTransaction(retryCount) vs ExponentialBackoff.forCommand(retryCount) - conceptually the same exact thing.
• Type safety: Can't accidentally mix transaction/command constants
  • Enum constants all have the same type: ExponentialBackoff.TRANSACTION and ExponentialBackoff.COMMAND are both of the ExponentialBackoff type. Therefore, the enum approach addresses type-safety to the same extent as the static method approach - it does not.
  • Of course, an "object" approach where different instances of ExponentialBackoff have different compile-time types still possible, just not with ExponentialBackoff being enum. But it still won't improve type-safety simply because the code that uses it won't be able to benefit from those different compile-time types due to its trivial nature.

Thus, the current enum approach gives us only "configuration bundling", which is achievable to the same extent with the static method approach without involving any objects and enum classes.

---

¹

• I think performance-related considerations are practically inconsequential here, and the code complexity aspect totally dominates the performance aspect. However, when there is conceptual equivalence of two approaches, considering other aspects, including inconsequential ones to make a choice is still not necessarily unreasonable. I am using the performance aspect to add a tiny bit to my argument.
• enum constants are references to objects on heap, and accessing their state, even when it's final (baseMs, maxMs, growth) likely allows fewer JIT/javac optimizations than accessing constants from the run-time constant pool (which is where the numeric literals from a static method end up at), because final fields are modifiable, while constants in the pool are not.

[stIncMale]

Let's use InterruptionUtil.interruptAndCreateMongoInterruptedException.

[stIncMale]

Let's extract this code to a private static method. The withTransaction method is already too long, we should not make it longer without good reason.

[stIncMale]

[just a comment on a code that wasn't changed in this PR]

I have just noticed this ClientSessionClock - it uses non-monotonic clock. Horrendous.

[dariakp]

@stIncMale @nhachicha Flagging one more relevant spec test adjustment here: mongodb/specifications#1876

[stIncMale]

@dariakp Thank you for the heads up, I updated the PR description.

[stIncMale]

This is a partial review, where I reviewed only ExponentialBackoff.java.

The last reviewed commit is 36ecbf9.

[stIncMale]

1. This method is called only from a test that tests this method. This makes the method effectively dead. If the method is introduced in anticipation to become useful in the future, then we should introduce it when and in the PR where it is going to be actually used.
2. Just for information: if the method were not dead, I would have suggest that the implementation of the calculateDelayBeforeNextRetryMs method should be done by calling calculateDelayBeforeNextRetryMs, to reuse the code instead of duplicating it. But I am not suggesting that because of item 1 above.

[stIncMale]

• The tag format is TODO-<ticket ID>. This way we can find all such tags by searching the codebase for "TODO-", and not have them mixed them with the (useless) TODO tags that were introduced to the code before we started using the new approach.
• Comments / error messages / etc. with such tags is a mechanism we resort to if the description of a ticket is not enough to conveniently specify all the information. In this particular case, the description of a ticket is very much enough, as we can just say in it something like "Use InternalMongoClientSettings to get rid of the ExponentialBackoff.testJitterSupplier global state". So we should probably not resort to leaving a comment with the TODO-<ticket ID> tag.
• When we resort to this mechanism, we should also leave a note in the ticket description that addressing comments with the TODO-<ticket ID> tag is in scope of the ticket. See https://jira.mongodb.org/browse/JAVA-6005, https://jira.mongodb.org/browse/JAVA-6059 as examples (I updated the latter, as well as some other tickets, because they were missing the note). Such notes are important because they draw attention of the assignee to the tagged comments. Without a note, the assignee is more likely to not even realize there are relevant tagged comments.

https://jira.mongodb.org/browse/JAVA-6060 is about introducing InternalMongoClientSettings and getting rid of InternalStreamConnection.setRecordEverything, but is not about getting rid of ExponentialBackoff.testJitterSupplier. Therefore, we should

• create another ticket that will use InternalMongoClientSettings to get rid of the ExponentialBackoff.testJitterSupplier global state;
• link it to https://jira.mongodb.org/browse/JAVA-6060 via the "depends on" link.

[stIncMale]

The new code is better, but the above description of benefits the current enum approach provides is flawed. The current enum approach is actually conceptually equivalent to the approach with the static method I proposed, but achieves the same in a more complex way (from the source code standpoint), with a potentially less efficient memory organization, and probably allows fewer JIT/javac optimizations¹.

• Encapsulation: Retry count management is internal - caller can't forget to increment
  • I like encapsulation, but the current enum approach cannot handle counting internally, and does not do that: retryCount is passed to the instance method ExponentialBackoff.calculateDelayBeforeNextRetryMs.
  • Note also that the way the overload retry policy is formulated at the moment (still work in progress), does not even allow us to encapsulate retry counting in ExponentialBackoff, because generally speaking backoff does not need to be computed/applied to each retry attempt, but the retry attempt index used to compute the backoff is the index of the retry attempt in the sequence of all retry attempts, regardless of what retry policy an attempt was executed under. That is, computing the backoff and counting retry attempts have to be done independently of each other. But if ExponentialBackoff were to encapsulate the counting, then they could only have been done together.
• Configuration bundling: forTransactionRetry() vs forCommandRetry() clearly separates concerns
  • With the current enum approach, we'll have ExponentialBackoff.TRANSACTION.calculateDelayBeforeNextRetryMs(retryCount) vs ExponentialBackoff.COMMAND.calculateDelayBeforeNextRetryMs(retryCount), as opposed to what you wrote. With the static method approach, we'll have ExponentialBackoff.forTransaction(retryCount) vs ExponentialBackoff.forCommand(retryCount) - conceptually the same exact thing.
• Type safety: Can't accidentally mix transaction/command constants
  • Enum constants all have the same type: ExponentialBackoff.TRANSACTION and ExponentialBackoff.COMMAND are both of the ExponentialBackoff type. Therefore, the enum approach addresses type-safety to the same extent as the static method approach - it does not.
  • Of course, an "object" approach where different instances of ExponentialBackoff have different compile-time types still possible, just not with ExponentialBackoff being enum. But it still won't improve type-safety simply because the code that uses it won't be able to benefit from those different compile-time types due to its trivial nature.

Thus, the current enum approach gives us only "configuration bundling", which is achievable to the same extent with the static method approach without involving any objects and enum classes.

---

¹

• I think performance-related considerations are practically inconsequential here, and the code complexity aspect totally dominates the performance aspect. However, when there is conceptual equivalence of two approaches, considering other aspects, including inconsequential ones to make a choice is still not necessarily unreasonable. I am using the performance aspect to add a tiny bit to my argument.
• enum constants are references to objects on heap, and accessing their state, even when it's final (baseMs, maxMs, growth) likely allows fewer JIT/javac optimizations than accessing constants from the run-time constant pool (which is where the numeric literals from a static method end up at), because final fields are modifiable, while constants in the pool are not.

[stIncMale]

The class name uses the term "backoff", but the method uses the term "delay" (both in its name and in the documentation comment). Let's not use two different terms to refer to the same thing.

I am guessing the above is an indirect result of you deciding to call backoff the intermediate result when computing the backoff. Given that this intermediate result does not serve any purpose, we don't have to make it a thing and name it, we can just write the whole formula like return Math.round(jitter * Math.min(baseMs * Math.pow(growth, retryCount), maxMs)).

[stIncMale]

We don't use this declaration style in the Java driver codebase. Let's declare each instance field separately.

[stIncMale]

This method accepts retryCount despite:

• both the specification and ClientSessionImpl.withTransaction operating 0-based number of a transaction attempt that it is called transactionAttempt;
  • As a result, ClientSessionImpl has to pass transactionAttempt - 1 when calling calculateDelayBeforeNextRetryMs to compute the backoff for the attempt with number transactionAttempt.
• RetryState.attempt() returning 0-based attempt number.

Let's change this method so that it also operates 0-based attempt number by accepting attempt documented as the 0-based number of the transaction attempt for which backoff is to be calculated.