Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-5720): on pre-4.4 sharded servers, the node driver uses error.writeConcern.code to determine retryability #4155

Merged
merged 24 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
6fd82f8
sync spec tests - no other changes needed
aditi-khare-mongoDB Jun 17, 2024
242c12d
remove stray only
aditi-khare-mongoDB Jun 17, 2024
a8f67d6
sharded <4.4 support
aditi-khare-mongoDB Jun 18, 2024
41e8587
remove typo
aditi-khare-mongoDB Jun 18, 2024
96b5c95
bugs fixed?
aditi-khare-mongoDB Jun 18, 2024
dcef868
remove stray only
aditi-khare-mongoDB Jun 18, 2024
f468ae4
skip outdated tests
aditi-khare-mongoDB Jun 20, 2024
e205a77
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jun 20, 2024
b22466e
skip tests
aditi-khare-mongoDB Jun 28, 2024
9eb063e
remove stray only
aditi-khare-mongoDB Jun 28, 2024
231fb80
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jun 28, 2024
e7037e9
skip tests with comment
aditi-khare-mongoDB Jul 9, 2024
440bac7
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jul 9, 2024
ad5b150
remove stray only
aditi-khare-mongoDB Jul 9, 2024
60ad9ac
fix logic
aditi-khare-mongoDB Jul 10, 2024
a7b760d
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jul 10, 2024
985dc03
changes to consider top-level code, doesnt pass tests
aditi-khare-mongoDB Jul 11, 2024
ac247a7
delete extraneous folder
aditi-khare-mongoDB Jul 11, 2024
10578d2
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jul 19, 2024
021e118
lint fix
aditi-khare-mongoDB Jul 19, 2024
fca53fe
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jul 29, 2024
7a1964b
post node-6276 update
aditi-khare-mongoDB Jul 29, 2024
a0bdcf4
post node-6276 update 2
aditi-khare-mongoDB Jul 29, 2024
5f8020e
requested changes
aditi-khare-mongoDB Jul 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/cmap/connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
MIN_SUPPORTED_SERVER_VERSION,
MIN_SUPPORTED_WIRE_VERSION
} from './wire_protocol/constants';
import { isSharded } from './wire_protocol/shared';

/** @public */
export type Stream = Socket | TLSSocket;
Expand Down Expand Up @@ -164,7 +165,7 @@ export async function performInitialHandshake(
} catch (error) {
if (error instanceof MongoError) {
error.addErrorLabel(MongoErrorLabel.HandshakeError);
if (needsRetryableWriteLabel(error, response.maxWireVersion)) {
if (needsRetryableWriteLabel(error, response.maxWireVersion, isSharded(conn))) {
error.addErrorLabel(MongoErrorLabel.RetryableWriteError);
}
}
Expand Down
10 changes: 8 additions & 2 deletions src/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1217,7 +1217,11 @@ const RETRYABLE_READ_ERROR_CODES = new Set<number>([
// see: https://github.com/mongodb/specifications/blob/master/source/retryable-writes/retryable-writes.rst#terms
const RETRYABLE_WRITE_ERROR_CODES = RETRYABLE_READ_ERROR_CODES;

export function needsRetryableWriteLabel(error: Error, maxWireVersion: number): boolean {
export function needsRetryableWriteLabel(
error: Error,
maxWireVersion: number,
isSharded: boolean
): boolean {
// pre-4.4 server, then the driver adds an error label for every valid case
// execute operation will only inspect the label, code/message logic is handled here
if (error instanceof MongoNetworkError) {
Expand All @@ -1237,7 +1241,9 @@ export function needsRetryableWriteLabel(error: Error, maxWireVersion: number):
}

if (error instanceof MongoWriteConcernError) {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
return RETRYABLE_WRITE_ERROR_CODES.has(error.result?.code ?? error.code ?? 0);
return isSharded && maxWireVersion < 9
? false
: RETRYABLE_WRITE_ERROR_CODES.has(error.result?.code ?? error.code ?? 0);
}

if (error instanceof MongoError && typeof error.code === 'number') {
Expand Down
3 changes: 2 additions & 1 deletion src/sdam/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '../cmap/connection_pool';
import { PoolClearedError } from '../cmap/errors';
import { type MongoDBResponseConstructor } from '../cmap/wire_protocol/responses';
import { isSharded } from '../cmap/wire_protocol/shared';
import {
APM_EVENTS,
CLOSED,
Expand Down Expand Up @@ -453,7 +454,7 @@ export class Server extends TypedEventEmitter<ServerEvents> {
} else {
if (
(isRetryableWritesEnabled(this.topology) || isTransactionCommand(cmd)) &&
needsRetryableWriteLabel(error, maxWireVersion(this)) &&
needsRetryableWriteLabel(error, maxWireVersion(this), isSharded(this)) &&
!inActiveTransaction(session, cmd)
) {
error.addErrorLabel(MongoErrorLabel.RetryableWriteError);
Expand Down
35 changes: 26 additions & 9 deletions test/integration/retryable-writes/retryable_writes.spec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,22 @@ describe('Legacy Retryable Writes Specs', function () {

const retryableWrites = loadSpecTests('retryable-writes', 'legacy');

const LEGACY_SKIP_TESTS_4_4_SHARDED = [
'BulkWrite succeeds after WriteConcernError ShutdownInProgress',
'DeleteOne succeeds after WriteConcernError ShutdownInProgress',
'FindOneAndDelete succeeds after WriteConcernError ShutdownInProgress',
'FindOneAndReplace succeeds after WriteConcernError ShutdownInProgress',
'FindOneAndUpdate succeeds after WriteConcernError ShutdownInProgress',
'InsertMany succeeds after WriteConcernError ShutdownInProgress',
'InsertOne succeeds after WriteConcernError InterruptedAtShutdown',
'InsertOne succeeds after WriteConcernError InterruptedDueToReplStateChange',
'InsertOne succeeds after WriteConcernError PrimarySteppedDown',
'InsertOne succeeds after WriteConcernError ShutdownInProgress',
'InsertOne fails after multiple retryable writeConcernErrors',
'ReplaceOne succeeds after WriteConcernError ShutdownInProgress',
'UpdateOne succeeds after WriteConcernError ShutdownInProgress'
];

for (const suite of retryableWrites) {
describe(suite.name, function () {
beforeEach(async function () {
Expand Down Expand Up @@ -63,16 +79,17 @@ describe('Legacy Retryable Writes Specs', function () {
await ctx.client.close();
ctx = {}; // reset context
});

for (const spec of suite.tests) {
// Step 2: Run the test
const mochaTest = it(spec.description, async () => await executeScenarioTest(spec, ctx));

// A pattern we don't need to repeat for unified tests
// In order to give the beforeEach hook access to the
// spec test so it can be responsible for skipping it
// and executeScenarioSetup
mochaTest.spec = spec;
if (!LEGACY_SKIP_TESTS_4_4_SHARDED.includes(spec.description)) {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
// Step 2: Run the test
const mochaTest = it(spec.description, async () => await executeScenarioTest(spec, ctx));

// A pattern we don't need to repeat for unified tests
// In order to give the beforeEach hook access to the
// spec test so it can be responsible for skipping it
// and executeScenarioSetup
mochaTest.spec = spec;
}
}
});
}
Expand Down
Loading