Conversation
tadjik1
commented
Mar 16, 2026
|
|
||
| timeOffset = 0; | ||
| const originalNow = performance.now.bind(performance); | ||
| sinon.stub(performance, 'now').callsFake(() => originalNow() + timeOffset); |
Member
Author
There was a problem hiding this comment.
This is how spec suggests to test, similar implementations in:
- python: PatchSessionTimeout(0)
- java: Duration.ofSeconds(120)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the driver’s withTransaction retry-timeout behavior to align with the DRIVERS-3391 spec change: when retrying runs out the overall deadline (CSOT timeoutMS or the legacy 120s cap), it throws a MongoOperationTimeoutError that wraps the last transient error as cause (and propagates error labels onto the timeout error).
Changes:
- Update
ClientSession.withTransactionto throw aMongoOperationTimeoutErroron timeout instead of surfacing the raw transient error. - Add a
makeWithTransactionTimeoutErrorhelper that wraps the last error ascauseand copies error labels. - Extend spec/integration tests to cover the new timeout error behavior (and add
commandFailedEventobservations in the CSOT unified spec test).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/sessions.ts |
Implements timeout wrapping behavior and adds helper to build labeled MongoOperationTimeoutError. |
test/spec/client-side-operations-timeout/convenient-transactions.yml |
Adds commandFailedEvent observation and a new unified test for timeout surfacing after transient retries. |
test/spec/client-side-operations-timeout/convenient-transactions.json |
JSON equivalent updates for the unified spec test additions. |
test/integration/transactions-convenient-api/transactions-convenient-api.prose.test.ts |
Adds prose integration tests that simulate exceeding the legacy 120s retry window and assert a timeout error is returned. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+162
to
+165
| // 3. Assert that the error is a timeout error wrapping the TransientTransactionError. | ||
| expect(result).to.be.instanceOf(MongoOperationTimeoutError); | ||
| expect((result as MongoOperationTimeoutError).cause).to.be.an('error'); | ||
| } |
|
|
||
| // 3. Assert that the error is a timeout error wrapping the commit error. | ||
| expect(result).to.be.instanceOf(MongoOperationTimeoutError); | ||
| expect((result as MongoOperationTimeoutError).cause).to.be.an('error'); |
Comment on lines
+244
to
+246
| // 3. Assert that the error is a timeout error wrapping the TransientTransactionError. | ||
| expect(result).to.be.instanceOf(MongoOperationTimeoutError); | ||
| expect((result as MongoOperationTimeoutError).cause).to.be.an('error'); |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the driver’s withTransaction retry-timeout behavior to match the updated transactions convenient API spec (DRIVERS-3391 / NODE-7430): when retry attempts run past the allowed deadline (CSOT timeoutMS or legacy 120s), the driver should throw a MongoOperationTimeoutError that wraps the last retryable/transient error as the cause, rather than throwing the transient error directly.
Changes:
- Update
ClientSession.withTransactionto throw aMongoOperationTimeoutError(with labels copied from the cause) when retry deadlines are exceeded, and fix legacy 120s deadline logic when CSOT is enabled. - Add/extend unified spec tests for CSOT convenient transactions to cover transient retry exhaustion behavior.
- Add integration prose tests that stub time to validate the legacy 120-second retry timeout behavior for callback + commit retry paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/sessions.ts |
Implements timeout-wrapping behavior for withTransaction retry deadline exhaustion and refines deadline checks for CSOT vs legacy timeout paths. |
test/spec/client-side-operations-timeout/convenient-transactions.yml |
Adds command failed event observation and a new unified spec test covering transient retry exhaustion surfacing as a timeout. |
test/spec/client-side-operations-timeout/convenient-transactions.json |
JSON equivalent updates for the unified spec additions/changes. |
test/integration/transactions-convenient-api/transactions-convenient-api.prose.test.ts |
Adds integration prose tests that validate legacy 120s retry timeout behavior using a stubbed clock. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/sessions.ts
Outdated
Comment on lines
+878
to
+884
| const hasTimedOut = | ||
| !this.timeoutContext?.csotEnabled() && processTimeMS() - startTime >= MAX_TIMEOUT; | ||
|
|
||
| if (!hasTimedOut) { | ||
| /* | ||
| * Note: a maxTimeMS error will have the MaxTimeMSExpired | ||
| * code (50) and can be reported as a top-level error or | ||
| * inside writeConcernError, ex. | ||
| * { ok:0, code: 50, codeName: 'MaxTimeMSExpired' } | ||
| * { ok:1, writeConcernError: { code: 50, codeName: 'MaxTimeMSExpired' } } | ||
| */ | ||
| if ( | ||
| !isMaxTimeMSExpiredError(commitError) && | ||
| commitError.hasErrorLabel(MongoErrorLabel.UnknownTransactionCommitResult) | ||
| ) { | ||
| // 10.i If the `commitTransaction` error includes a "UnknownTransactionCommitResult" label and the error is not | ||
| // MaxTimeMSExpired and the elapsed time of `withTransaction` is less than 120 seconds, jump back to step eight. | ||
| continue retryCommit; | ||
| } | ||
|
|
||
| if (commitError.hasErrorLabel(MongoErrorLabel.TransientTransactionError)) { | ||
| // 10.ii If the commitTransaction error includes a "TransientTransactionError" label | ||
| // and the elapsed time of withTransaction is less than 120 seconds, jump back to step two. | ||
| lastError = commitError; | ||
|
|
||
| continue retryTransaction; | ||
| } | ||
| (this.timeoutContext?.csotEnabled() && this.timeoutContext.remainingTimeMS <= 0) || | ||
| (!this.timeoutContext?.csotEnabled() && processTimeMS() - startTime >= MAX_TIMEOUT); | ||
|
|
||
| if (hasTimedOut) { | ||
| throw makeWithTransactionTimeoutError(commitError); | ||
| } |
Comment on lines
+129
to
+136
| mode: alwaysOn | ||
| data: | ||
| failCommands: ["insert"] | ||
| blockConnection: true | ||
| blockTimeMS: 25 | ||
| errorCode: 24 | ||
| errorLabels: ["TransientTransactionError"] | ||
|
|
Comment on lines
+164
to
+167
| - commandStartedEvent: | ||
| commandName: abortTransaction | ||
| - commandFailedEvent: | ||
| commandName: abortTransaction |
Comment on lines
+226
to
+229
| "mode": "alwaysOn", | ||
| "data": { | ||
| "failCommands": [ | ||
| "insert" |
Comment on lines
+281
to
+289
| { | ||
| "commandStartedEvent": { | ||
| "commandName": "abortTransaction" | ||
| } | ||
| }, | ||
| { | ||
| "commandFailedEvent": { | ||
| "commandName": "abortTransaction" | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary of Changes
Align
withTransactiontimeout behavior with the spec change from DRIVERS-3391: when retry attempts exhaust the timeout (CSOTtimeoutMSor legacy 120s), throw aMongoOperationTimeoutErrorwrapping the last transient error ascause, instead of throwing the raw transient error directly.Notes for Reviewers
The
makeWithTransactionTimeoutErrorhelper mirrors the spec'smakeTimeoutErrorpseudocode function. It creates aMongoOperationTimeoutErrorand copies error labels from the cause, so callers can checkhasErrorLabel('TransientTransactionError')on the timeout error itself.The 120s cap fix is a one-line change (adding
!csotEnabled()guard to the legacy branch ofwillExceedTransactionDeadline), but it's included here since it's in the same code path.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