Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the driver’s withTransaction retry-timeout behavior to align with the transactions convenient API spec changes (DRIVERS-3391), ensuring that when the retry deadline is exceeded under CSOT (timeoutMS), the driver surfaces a timeout error rather than the last transient error directly.
Changes:
- Update
ClientSession.withTransactionretry loop to enforce a computed retry deadline and throw aMongoOperationTimeoutError(CSOT) that wraps the last retry-triggering error ascause. - Add/extend CSOT unified spec coverage for
withTransactiontimeout behavior, including retry scenarios with transient transaction errors. - Add integration prose tests to validate legacy (non-CSOT) retry-timeout enforcement via mocked time progression.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| test/spec/client-side-operations-timeout/convenient-transactions.yml | Adds CSOT unified spec coverage for withTransaction timeout and introduces additional event assertions. |
| test/spec/client-side-operations-timeout/convenient-transactions.json | JSON equivalent of the above spec updates. |
| test/integration/transactions-convenient-api/transactions-convenient-api.prose.test.ts | Adds prose tests to validate the legacy 120s retry timeout behavior using a stubbed clock. |
| src/sessions.ts | Implements deadline-based retry timeout checks and introduces makeTimeoutError helper for CSOT wrapping behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - commandStartedEvent: | ||
| commandName: abortTransaction | ||
| - commandFailedEvent: | ||
| commandName: abortTransaction |
| expectError: | ||
| isError: true | ||
| expectError: | ||
| isTimeoutError: true |
| { | ||
| "commandFailedEvent": { | ||
| "commandName": "abortTransaction" | ||
| } | ||
| }, |
| { | ||
| "commandFailedEvent": { | ||
| "commandName": "abortTransaction" | ||
| } | ||
| } |
| - commandStartedEvent: | ||
| commandName: abortTransaction | ||
| - commandFailedEvent: | ||
| commandName: abortTransaction |
PavelSafronov
left a comment
PavelSafronov
left a comment
There was a problem hiding this comment.
Overall changes look good, just have some nit comments, but nothing blocking.
| // triggered retry. With CSOT this is a MongoOperationTimeoutError; without CSOT the raw error | ||
| // is propagated directly. See makeTimeoutError() below. | ||
| const csotEnabled = !!this.timeoutContext?.csotEnabled(); | ||
| const deadline = this.timeoutContext?.csotEnabled() |
There was a problem hiding this comment.
Nit: simplify this a bit by introducing a timeout variable
const remainingTimeMS = this.timeoutContext?.csotEnabled() ? this.timeoutContext.remainingTimeMS : MAX_TIMEOUT';
const deadline = processTimeMS() + remainingTimeMS;
| return result; | ||
| } | ||
| // 5. (cont.) and whether the callback reported an error | ||
| // 7. If the callback reported an error: |
There was a problem hiding this comment.
Nit: move this down, to match the similar pattern we have on line 864, where the comment is inside the catch-block.
| ? this.timeoutContext.start | ||
| : processTimeMS(); | ||
| // 1. Define the following: | ||
| // 1.1 Record the current monotonic time, which will be used to enforce the timeout before later retry attempts. |
There was a problem hiding this comment.
Clarify this, that while we are not storing the monotonic time, we are calculating the equivalent.
Description
Summary of Changes
Align withTransaction timeout behavior with the spec change from DRIVERS-3391: when retry attempts exhaust the timeout, enforce a deadline-based check. With CSOT (timeoutMS), throw a MongoOperationTimeoutError wrapping the last transient error as cause; without CSOT (legacy 120s), propagate the last error directly.
Spec: https://github.com/mongodb/specifications/blob/master/source/transactions-convenient-api/transactions-convenient-api.md#sequence-of-actions
Notes for Reviewers
The
makeTimeoutErrorhelper mirrors the spec'smakeTimeoutErrorpseudocode function. It also copies error labels from the cause, so callers can checkhasErrorLabel('TransientTransactionError')on the timeout error itself.What is the motivation for this change?
NODE-7430 / DRIVERS-3391
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript