Skip to content

Commit 2b30b8d

Browse files
tadjik1tadjik1
committed
fix(NODE-7430): throw timeout error when withTransaction retries exceed deadline
1 parent 275afa5 commit 2b30b8d

File tree

4 files changed

+402
-46
lines changed

4 files changed

+402
-46
lines changed

‎src/sessions.ts‎

Lines changed: 65 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
MongoErrorLabel,
1818
MongoExpiredSessionError,
1919
MongoInvalidArgumentError,
20+
MongoOperationTimeoutError,
2021
MongoRuntimeError,
2122
MongoServerError,
2223
MongoTransactionError,
@@ -777,14 +778,15 @@ export class ClientSession
777778
const willExceedTransactionDeadline =
778779
(this.timeoutContext?.csotEnabled() &&
779780
backoffMS > this.timeoutContext.remainingTimeMS) ||
780-
processTimeMS() + backoffMS > startTime + MAX_TIMEOUT;
781+
(!this.timeoutContext?.csotEnabled() &&
782+
processTimeMS() + backoffMS > startTime + MAX_TIMEOUT);
781783

782784
if (willExceedTransactionDeadline) {
783-
throw (
785+
throw makeWithTransactionTimeoutError(
784786
lastError ??
785-
new MongoRuntimeError(
786-
`Transaction retry did not record an error: should never occur. Please file a bug.`
787-
)
787+
new MongoRuntimeError(
788+
`Transaction retry did not record an error: should never occur. Please file a bug.`
789+
)
788790
);
789791
}
790792

@@ -827,6 +829,8 @@ export class ClientSession
827829
throw fnError;
828830
}
829831

832+
lastError = fnError;
833+
830834
if (
831835
this.transaction.state === TxnState.STARTING_TRANSACTION ||
832836
this.transaction.state === TxnState.TRANSACTION_IN_PROGRESS
@@ -836,14 +840,18 @@ export class ClientSession
836840
await this.abortTransaction();
837841
}
838842

839-
if (
840-
fnError.hasErrorLabel(MongoErrorLabel.TransientTransactionError) &&
841-
(this.timeoutContext?.csotEnabled() || processTimeMS() - startTime < MAX_TIMEOUT)
842-
) {
843-
// 7.ii If the callback's error includes a "TransientTransactionError" label and the elapsed time of `withTransaction`
844-
// is less than 120 seconds, jump back to step two.
845-
lastError = fnError;
846-
continue retryTransaction;
843+
if (fnError.hasErrorLabel(MongoErrorLabel.TransientTransactionError)) {
844+
if (
845+
this.timeoutContext?.csotEnabled() ||
846+
processTimeMS() - startTime < MAX_TIMEOUT
847+
) {
848+
// 7.ii If the callback's error includes a "TransientTransactionError" label and the elapsed time of `withTransaction`
849+
// is less than TIMEOUT_MS, jump back to step two.
850+
continue retryTransaction;
851+
} else {
852+
// 7.ii (cont.) If timeout has been exceeded, raise a timeout error wrapping the transient error.
853+
throw makeWithTransactionTimeoutError(fnError);
854+
}
847855
}
848856

849857
// 7.iii If the callback's error includes a "UnknownTransactionCommitResult" label, the callback must have manually committed a transaction,
@@ -865,37 +873,39 @@ export class ClientSession
865873
committed = true;
866874
// 10. If commitTransaction reported an error:
867875
} catch (commitError) {
868-
// If CSOT is enabled, we repeatedly retry until timeoutMS expires. This is enforced by providing a
869-
// timeoutContext to each async API, which know how to cancel themselves (i.e., the next retry will
870-
// abort the withTransaction call).
871-
// If CSOT is not enabled, do we still have time remaining or have we timed out?
876+
lastError = commitError;
877+
878+
// Check if the withTransaction timeout has been exceeded.
879+
// With CSOT: check remaining time from the timeout context.
880+
// Without CSOT: check if we've exceeded the 120-second timeout.
872881
const hasTimedOut =
873-
!this.timeoutContext?.csotEnabled() && processTimeMS() - startTime >= MAX_TIMEOUT;
874-
875-
if (!hasTimedOut) {
876-
/*
877-
* Note: a maxTimeMS error will have the MaxTimeMSExpired
878-
* code (50) and can be reported as a top-level error or
879-
* inside writeConcernError, ex.
880-
* { ok:0, code: 50, codeName: 'MaxTimeMSExpired' }
881-
* { ok:1, writeConcernError: { code: 50, codeName: 'MaxTimeMSExpired' } }
882-
*/
883-
if (
884-
!isMaxTimeMSExpiredError(commitError) &&
885-
commitError.hasErrorLabel(MongoErrorLabel.UnknownTransactionCommitResult)
886-
) {
887-
// 10.i If the `commitTransaction` error includes a "UnknownTransactionCommitResult" label and the error is not
888-
// MaxTimeMSExpired and the elapsed time of `withTransaction` is less than 120 seconds, jump back to step eight.
889-
continue retryCommit;
890-
}
891-
892-
if (commitError.hasErrorLabel(MongoErrorLabel.TransientTransactionError)) {
893-
// 10.ii If the commitTransaction error includes a "TransientTransactionError" label
894-
// and the elapsed time of withTransaction is less than 120 seconds, jump back to step two.
895-
lastError = commitError;
896-
897-
continue retryTransaction;
898-
}
882+
(this.timeoutContext?.csotEnabled() && this.timeoutContext.remainingTimeMS <= 0) ||
883+
(!this.timeoutContext?.csotEnabled() && processTimeMS() - startTime >= MAX_TIMEOUT);
884+
885+
if (hasTimedOut) {
886+
throw makeWithTransactionTimeoutError(commitError);
887+
}
888+
889+
/*
890+
* Note: a maxTimeMS error will have the MaxTimeMSExpired
891+
* code (50) and can be reported as a top-level error or
892+
* inside writeConcernError, ex.
893+
* { ok:0, code: 50, codeName: 'MaxTimeMSExpired' }
894+
* { ok:1, writeConcernError: { code: 50, codeName: 'MaxTimeMSExpired' } }
895+
*/
896+
if (
897+
!isMaxTimeMSExpiredError(commitError) &&
898+
commitError.hasErrorLabel(MongoErrorLabel.UnknownTransactionCommitResult)
899+
) {
900+
// 10.i If the `commitTransaction` error includes a "UnknownTransactionCommitResult" label and the error is not
901+
// MaxTimeMSExpired and the elapsed time of `withTransaction` is less than TIMEOUT_MS, jump back to step eight.
902+
continue retryCommit;
903+
}
904+
905+
if (commitError.hasErrorLabel(MongoErrorLabel.TransientTransactionError)) {
906+
// 10.ii If the commitTransaction error includes a "TransientTransactionError" label
907+
// and the elapsed time of withTransaction is less than TIMEOUT_MS, jump back to step two.
908+
continue retryTransaction;
899909
}
900910

901911
// 10.iii Otherwise, propagate the commitTransaction error to the caller of withTransaction and return immediately.
@@ -912,6 +922,18 @@ export class ClientSession
912922
}
913923
}
914924

925+
function makeWithTransactionTimeoutError(cause: Error): MongoOperationTimeoutError {
926+
const timeoutError = new MongoOperationTimeoutError('Timed out during withTransaction', {
927+
cause
928+
});
929+
if (cause instanceof MongoError) {
930+
for (const label of cause.errorLabels) {
931+
timeoutError.addErrorLabel(label);
932+
}
933+
}
934+
return timeoutError;
935+
}
936+
915937
const NON_DETERMINISTIC_WRITE_CONCERN_ERRORS = new Set([
916938
'CannotSatisfyWriteConcern',
917939
'UnknownReplWriteConcern',

‎test/integration/transactions-convenient-api/transactions-convenient-api.prose.test.ts‎

Lines changed: 166 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,18 @@ import { expect } from 'chai';
22
import { test } from 'mocha';
33
import * as sinon from 'sinon';
44

5-
import { type ClientSession, type Collection, type MongoClient } from '../../mongodb';
6-
import { configureFailPoint, type FailCommandFailPoint, measureDuration } from '../../tools/utils';
5+
import {
6+
type ClientSession,
7+
type Collection,
8+
type MongoClient,
9+
MongoOperationTimeoutError
10+
} from '../../mongodb';
11+
import {
12+
clearFailPoint,
13+
configureFailPoint,
14+
type FailCommandFailPoint,
15+
measureDuration
16+
} from '../../tools/utils';
717

818
const failCommand: FailCommandFailPoint = {
919
configureFailPoint: 'failCommand',
@@ -85,3 +95,157 @@ describe('Retry Backoff is Enforced', function () {
8595
}
8696
);
8797
});
98+
99+
describe('Retry Timeout is Enforced', function () {
100+
// Drivers should test that withTransaction enforces a non-configurable timeout before retrying
101+
// both commits and entire transactions.
102+
//
103+
// Note: We use CSOT's timeoutMS to enforce a short timeout instead of blocking for the full
104+
// 120-second retry timeout, as recommended by the spec: "This might be done by internally
105+
// modifying the timeout value used by withTransaction with some private API or using a mock timer."
106+
//
107+
// The error SHOULD be propagated as a timeout error if the language allows to expose the
108+
// underlying error as a cause of a timeout error.
109+
110+
let client: MongoClient;
111+
let collection: Collection;
112+
113+
beforeEach(async function () {
114+
client = this.configuration.newClient({ timeoutMS: 100 });
115+
collection = client.db('foo').collection('bar');
116+
});
117+
118+
afterEach(async function () {
119+
await clearFailPoint(this.configuration);
120+
await client?.close();
121+
});
122+
123+
// Case 1: If the callback raises an error with the TransientTransactionError label and the retry
124+
// timeout has been exceeded, withTransaction should propagate the error (see Note 1) to its caller.
125+
test(
126+
'callback TransientTransactionError propagated as timeout error when retry timeout exceeded',
127+
{
128+
requires: {
129+
mongodb: '>=4.4',
130+
topology: '!single'
131+
}
132+
},
133+
async function () {
134+
// 1. Configure a failpoint that always fails insert with TransientTransactionError
135+
// and blocks for 25ms to consume timeout budget.
136+
await configureFailPoint(this.configuration, {
137+
configureFailPoint: 'failCommand',
138+
mode: 'alwaysOn',
139+
data: {
140+
failCommands: ['insert'],
141+
blockConnection: true,
142+
blockTimeMS: 25,
143+
errorCode: 24,
144+
errorLabels: ['TransientTransactionError']
145+
}
146+
});
147+
148+
// 2. Run withTransaction with a callback that performs an insert.
149+
// The insert will always fail with TransientTransactionError, triggering retries
150+
// until the timeout (timeoutMS: 100) is exceeded.
151+
const { result } = await measureDuration(() => {
152+
return client.withSession(async s => {
153+
await s.withTransaction(async session => {
154+
await collection.insertOne({}, { session });
155+
});
156+
});
157+
});
158+
159+
// 3. Assert that the error is a timeout error wrapping the TransientTransactionError.
160+
expect(result).to.be.instanceOf(MongoOperationTimeoutError);
161+
expect((result as MongoOperationTimeoutError).cause).to.be.an('error');
162+
}
163+
);
164+
165+
// Case 2: If committing raises an error with the UnknownTransactionCommitResult label, and the
166+
// retry timeout has been exceeded, withTransaction should propagate the error (see Note 1) to
167+
// its caller.
168+
test(
169+
'commit UnknownTransactionCommitResult propagated as timeout error when retry timeout exceeded',
170+
{
171+
requires: {
172+
mongodb: '>=4.4',
173+
topology: '!single'
174+
}
175+
},
176+
async function () {
177+
// 1. Configure a failpoint that always fails commitTransaction with
178+
// UnknownTransactionCommitResult and blocks for 25ms to consume timeout budget.
179+
await configureFailPoint(this.configuration, {
180+
configureFailPoint: 'failCommand',
181+
mode: 'alwaysOn',
182+
data: {
183+
failCommands: ['commitTransaction'],
184+
blockConnection: true,
185+
blockTimeMS: 25,
186+
errorCode: 64,
187+
errorLabels: ['UnknownTransactionCommitResult']
188+
}
189+
});
190+
191+
// 2. Run withTransaction with a callback that performs an insert (succeeds).
192+
// The commit will always fail with UnknownTransactionCommitResult, triggering commit
193+
// retries until the timeout (timeoutMS: 100) is exceeded.
194+
const { result } = await measureDuration(() => {
195+
return client.withSession(async s => {
196+
await s.withTransaction(async session => {
197+
await collection.insertOne({}, { session });
198+
});
199+
});
200+
});
201+
202+
// 3. Assert that the error is a timeout error.
203+
expect(result).to.be.instanceOf(MongoOperationTimeoutError);
204+
}
205+
);
206+
207+
// Case 3: If committing raises an error with the TransientTransactionError label and the retry
208+
// timeout has been exceeded, withTransaction should propagate the error (see Note 1) to its
209+
// caller. This case may occur if the commit was internally retried against a new primary after a
210+
// failover and the second primary returned a NoSuchTransaction error response.
211+
test(
212+
'commit TransientTransactionError propagated as timeout error when retry timeout exceeded',
213+
{
214+
requires: {
215+
mongodb: '>=4.4',
216+
topology: '!single'
217+
}
218+
},
219+
async function () {
220+
// 1. Configure a failpoint that always fails commitTransaction with
221+
// TransientTransactionError (errorCode 251 = NoSuchTransaction) and blocks for 25ms
222+
// to consume timeout budget.
223+
await configureFailPoint(this.configuration, {
224+
configureFailPoint: 'failCommand',
225+
mode: 'alwaysOn',
226+
data: {
227+
failCommands: ['commitTransaction'],
228+
blockConnection: true,
229+
blockTimeMS: 25,
230+
errorCode: 251,
231+
errorLabels: ['TransientTransactionError']
232+
}
233+
});
234+
235+
// 2. Run withTransaction with a callback that performs an insert (succeeds).
236+
// The commit will always fail with TransientTransactionError, triggering full
237+
// transaction retries until the timeout (timeoutMS: 100) is exceeded.
238+
const { result } = await measureDuration(() => {
239+
return client.withSession(async s => {
240+
await s.withTransaction(async session => {
241+
await collection.insertOne({}, { session });
242+
});
243+
});
244+
});
245+
246+
// 3. Assert that the error is a timeout error wrapping the TransientTransactionError.
247+
expect(result).to.be.instanceOf(MongoOperationTimeoutError);
248+
expect((result as MongoOperationTimeoutError).cause).to.be.an('error');
249+
}
250+
);
251+
});

0 commit comments

Comments
 (0)