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;
There was a problem hiding this comment.
I don't see real difference here between 2 ternary conditions essentially:
const deadline = this.timeoutContext?.csotEnabled()
? processTimeMS() + this.timeoutContext.remainingTimeMS
: processTimeMS() + MAX_TIMEOUT;
and
const remainingTimeMS = this.timeoutContext?.csotEnabled()
? this.timeoutContext.remainingTimeMS
: MAX_TIMEOUT;
const deadline = processTimeMS() + remainingTimeMS;
I don't have strong opinion here as well, do you think the 2nd is simpler or easier to to read?
There was a problem hiding this comment.
Introducing remainingTimeMS clarifies that that's what is changing wrt CSOT ("depending on CSOT, we'll change how much time remains in this operation"). And we also end up calling processTimeMS() only once.
dariakp
left a comment
dariakp
left a comment
There was a problem hiding this comment.
@PavelSafronov @tadjik1 Can we address copilot's comments? They are on spec tests, but if we think they are valid, it's always an option to submit an improvement to the spec.
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