GODRIVER-2651 Break NoWritesPerformed-Only Error Sequence (#1135)

Co-authored-by: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
Co-authored-by: Kevin Albertson <kevin.albertson@mongodb.com>
Co-authored-by: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>

mongo/integration/retryable_writes_prose_test.go

@@ -282,7 +282,7 @@ func TestRetryableWritesProse(t *testing.T) {
 
 			require.True(mt, secondFailPointConfigured)
 
-			// Assert that the "NotWritablePrimary" error is returned.
+			// Assert that the "ShutdownInProgress" error is returned.
 			require.True(mt, err.(mongo.WriteException).HasErrorCode(int(shutdownInProgressErrorCode)))
 		})
 }

testdata/retryable-writes/unified/insertOne-noWritesPerformedError.yml

@@ -0,0 +1,54 @@
+description: "retryable-writes insertOne noWritesPerformedErrors"
+
+schemaVersion: "1.0"
+
+runOnRequirements:
+  - minServerVersion: "6.0"
+    topologies: [ replicaset ]
+
+createEntities:
+  - client:
+      id: &client0 client0
+      useMultipleMongoses: false
+      observeEvents: [ commandFailedEvent ]
+  - database:
+      id: &database0 database0
+      client: *client0
+      databaseName: &databaseName retryable-writes-tests
+  - collection:
+      id: &collection0 collection0
+      database: *database0
+      collectionName: &collectionName no-writes-performed-collection
+
+tests:
+  - description: "InsertOne fails after NoWritesPerformed error"
+    operations:
+      - name: failPoint
+        object: testRunner
+        arguments:
+          client: *client0
+          failPoint:
+            configureFailPoint: failCommand
+            mode:
+              times: 2
+            data:
+              failCommands:
+                - insert
+              errorCode: 64
+              errorLabels:
+                - NoWritesPerformed
+                - RetryableWriteError
+      - name: insertOne
+        object: *collection0
+        arguments:
+          document:
+            x: 1
+        expectError:
+          errorCode: 64
+          errorLabelsContain:
+            - NoWritesPerformed
+            - RetryableWriteError
+    outcome:
+      - collectionName: *collectionName
+        databaseName: *databaseName
+        documents: []

testdata/retryable-writes/unified/insertOne-noWritesPerformedErrors.json

@@ -0,0 +1,90 @@
+{
+  "description": "retryable-writes insertOne noWritesPerformedErrors",
+  "schemaVersion": "1.0",
+  "runOnRequirements": [
+    {
+      "minServerVersion": "6.0",
+      "topologies": [
+        "replicaset"
+      ]
+    }
+  ],
+  "createEntities": [
+    {
+      "client": {
+        "id": "client0",
+        "useMultipleMongoses": false,
+        "observeEvents": [
+          "commandFailedEvent"
+        ]
+      }
+    },
+    {
+      "database": {
+        "id": "database0",
+        "client": "client0",
+        "databaseName": "retryable-writes-tests"
+      }
+    },
+    {
+      "collection": {
+        "id": "collection0",
+        "database": "database0",
+        "collectionName": "no-writes-performed-collection"
+      }
+    }
+  ],
+  "tests": [
+    {
+      "description": "InsertOne fails after NoWritesPerformed error",
+      "operations": [
+        {
+          "name": "failPoint",
+          "object": "testRunner",
+          "arguments": {
+            "client": "client0",
+            "failPoint": {
+              "configureFailPoint": "failCommand",
+              "mode": {
+                "times": 2
+              },
+              "data": {
+                "failCommands": [
+                  "insert"
+                ],
+                "errorCode": 64,
+                "errorLabels": [
+                  "NoWritesPerformed",
+                  "RetryableWriteError"
+                ]
+              }
+            }
+          }
+        },
+        {
+          "name": "insertOne",
+          "object": "collection0",
+          "arguments": {
+            "document": {
+              "x": 1
+            }
+          },
+          "expectError": {
+            "errorCode": 64,
+            "errorLabelsContain": [
+              "NoWritesPerformed",
+              "RetryableWriteError"
+            ]
+          }
+        }
+      ],
+      "outcome": [
+        {
+          "collectionName": "no-writes-performed-collection",
+          "databaseName": "retryable-writes-tests",
+          "documents": []
+        }
+      ]
+    }
+  ]
+}

x/mongo/driver/operation.go

@@ -59,8 +59,9 @@ type RetryablePoolError interface {
 	Retryable() bool
 }
 
-// LabeledError is an error that can have error labels added to it.
-type LabeledError interface {
+// labeledError is an error that can have error labels added to it.
+type labeledError interface {
+	error
 	HasErrorLabel(string) bool
 }
 
@@ -398,9 +399,19 @@ func (op Operation) Execute(ctx context.Context) error {
 		// Set the previous indefinite error to be returned in any case where a retryable write error does not have a
 		// NoWritesPerfomed label (the definite case).
 		switch err := err.(type) {
-		case LabeledError:
+		case labeledError:
+			// If the "prevIndefiniteErr" is nil, then the current error is the first error encountered
+			// during the retry attempt cycle. We must persist the first error in the case where all
+			// following errors are labeled "NoWritesPerformed", which would otherwise raise nil as the
+			// error.
+			if prevIndefiniteErr == nil {
+				prevIndefiniteErr = err
+			}
+
+			// If the error is not labeled NoWritesPerformed and is retryable, then set the previous
+			// indefinite error to be the current error.
 			if !err.HasErrorLabel(NoWritesPerformed) && err.HasErrorLabel(RetryableWriteError) {
-				prevIndefiniteErr = err.(error)
+				prevIndefiniteErr = err
 			}
 		}
 
@@ -595,6 +606,13 @@ func (op Operation) Execute(ctx context.Context) error {
 		finishedInfo.cmdErr = err
 		op.publishFinishedEvent(ctx, finishedInfo)
 
+		// prevIndefiniteErrorIsSet is "true" if the "err" variable has been set to the "prevIndefiniteErr" in
+		// a case in the switch statement below.
+		var prevIndefiniteErrIsSet bool
+
+		// TODO(GODRIVER-2579): When refactoring the "Execute" method, consider creating a separate method for the
+		// error handling logic below. This will remove the necessity of the "checkError" goto label.
+	checkError:
 		var perr error
 		switch tt := err.(type) {
 		case WriteCommandError:
@@ -627,9 +645,13 @@ func (op Operation) Execute(ctx context.Context) error {
 			}
 
 			// If the error is no longer retryable and has the NoWritesPerformed label, then we should
-			// return the previous indefinite error.
-			if tt.HasErrorLabel(NoWritesPerformed) {
-				return prevIndefiniteErr
+			// set the error to the "previous indefinite error" unless the current error is already the
+			// "previous indefinite error". After reseting, repeat the error check.
+			if tt.HasErrorLabel(NoWritesPerformed) && !prevIndefiniteErrIsSet {
+				err = prevIndefiniteErr
+				prevIndefiniteErrIsSet = true
+
+				goto checkError
 			}
 
 			// If the operation isn't being retried, process the response
@@ -720,9 +742,13 @@ func (op Operation) Execute(ctx context.Context) error {
 			}
 
 			// If the error is no longer retryable and has the NoWritesPerformed label, then we should
-			// return the previous indefinite error.
-			if tt.HasErrorLabel(NoWritesPerformed) {
-				return prevIndefiniteErr
+			// set the error to the "previous indefinite error" unless the current error is already the
+			// "previous indefinite error". After reseting, repeat the error check.
+			if tt.HasErrorLabel(NoWritesPerformed) && !prevIndefiniteErrIsSet {
+				err = prevIndefiniteErr
+				prevIndefiniteErrIsSet = true
+
+				goto checkError
 			}
 
 			// If the operation isn't being retried, process the response