DRIVERS-3436 - Refine withTransaction timeout error wrapping semantics and label propagation in spec and prose tests#1920
DRIVERS-3436 - Refine withTransaction timeout error wrapping semantics and label propagation in spec and prose tests#1920
Conversation
b16260a to
56ae0ad
Compare
56ae0ad to
3398035
Compare
|
|
||
| 3. Otherwise, propagate the `commitTransaction` error (see Note 1 below) to the caller of `withTransaction` and | ||
| return immediately. | ||
| 3. Otherwise, propagate the `commitTransaction` error (see |
There was a problem hiding this comment.
Revert to the previous behaviour see #1920 (comment)
|
|
||
| 4. Otherwise, propagate the callback's error (see Note 1 below) to the caller of `withTransaction` and return | ||
| immediately. | ||
| 4. Otherwise, propagate the callback's error (see |
| * {ok:1, writeConcernError: {code: 50, codeName: "MaxTimeMSExpired"}} | ||
| */ | ||
| lastError = error; | ||
| if (Date.now() - startTime >= timeout) { |
There was a problem hiding this comment.
This was removed because in sections 7.4 & 10.3 of the [sequence-of-actions|https://github.com/mongodb/specifications/blob/master/source/transactions-convenient-api/transactions-convenient-api.md#sequence-of-actions] the errors are not retriable, and they are mainly the reason for returning immediately from withTransaction. So there's no need to wrap them in a TimeoutError (if Timeout is encountered)
There was a problem hiding this comment.
However, in section 10.1.1 (UnknownTransactionCommitResult encountered during commit) The error is retriable if the timeout is not exeeded. So the wrapping into a timeout error needs to happen if we are throwing
There was a problem hiding this comment.
Pull request overview
This PR updates the Convenient Transactions API specification and associated prose tests to clarify how withTransaction should propagate errors when the retry timeout is exceeded, including timeout error wrapping semantics and error label propagation.
Changes:
- Adds a dedicated “timeout error propagation mechanism” section and updates step text to reference it.
- Updates wording across the spec and tests from “raise” to “propagate” and removes “Note 1” references.
- Adjusts pseudo-code related to timeout wrapping behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| source/transactions-convenient-api/transactions-convenient-api.md | Clarifies timeout wrapping/label propagation semantics; updates step wording; modifies pseudo-code and adds a new timeout propagation section. |
| source/transactions-convenient-api/tests/README.md | Updates prose test expectations to reference the timeout propagation mechanism and adds an assertion about label preservation. |
Comments suppressed due to low confidence (1)
source/transactions-convenient-api/transactions-convenient-api.md:269
- In the pseudo-code, the commit retry loop no longer checks whether the overall
withTransactiontimeout has been exceeded before retryingcommitTransactionagain onUnknownTransactionCommitResult. As written, this loop can continue retrying indefinitely and never raise the timeout-wrapping error required by the spec (see Sequence of Actions step 10.1). Reintroduce an elapsed-time check beforecontinue retryCommitand throwmakeTimeoutError(lastError)when the timeout is exceeded.
lastError = error;
if (!isMaxTimeMSExpiredError(error) &&
error.hasErrorLabel("UnknownTransactionCommitResult")) {
continue retryCommit;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| `withTransaction` should propagate the error as described in the | ||
| [propagation mechanism](../transactions-convenient-api.md#timeout-error-propagation-mechanism) to its caller. | ||
| - If committing raises an error with the `UnknownTransactionCommitResult` label, and the retry timeout has been | ||
| exceeded, `withTransaction` should propagate the error to its caller. |
There was a problem hiding this comment.
Is this point also supposed to call out the propagation mechanism?
There was a problem hiding this comment.
Good catch 👍 I fixed the prose and the spec.
TL;DR UnknownTransactionCommitResult is retriable in the commit loop if we don't exceed the timeout, so it makes sense to wrap it into a Timeout error if we exceed the timeout and want to throw and return (as described in section 10.1.1)
DRIVERS-3436
Updates the named prose test to be consistent with the first two cases.
Please complete the following before merging:
clusters). Spruce patch