MB-34367 [SR]: Clear engineSpecific on non-success IO complete

For SyncWrites, ep-engine returns EWOULDBLOCK once the prepare has
been accepted, in the golden path blocking the connection until the
SyncWrite completes at which point notify_io_complete is called with
ENGINE_SUCCESS. This results in memcached re-calling the same method
again, which now returns success (as ep-engine records the fact a
SyncWrite is in progrss via storeEngineSpecific().

If a SyncWrite is aborted then the notify_io_complete() is passed
status:ambiguous; which memcached immediately returns to the client.

However, in the non-success case the previous engineSpecific is *not*
cleared. As a consequence the _next_ SyncWrite call by the same client
will immediately return success.

Fix by clearing the engineSpecific on a non-success status code for
notify_IO_complete.

Change-Id: Ie55f23d44e807e01dc16a861724c12cfeb6fe660
Reviewed-on: http://review.couchbase.org/109912
Tested-by: Build Bot <[email protected]>
Reviewed-by: Jim Walker <[email protected]>
Reviewed-by: Trond Norbye <[email protected]>

Author: daverigby

engines/ep/src/kv_bucket.cc

@@ -2506,6 +2506,13 @@ uint16_t KVBucket::getNumOfVBucketsInState(vbucket_state_t state) const {
 SyncWriteCompleteCallback KVBucket::makeSyncWriteCompleteCB() {
     auto& engine = this->engine;
     return [&engine](const void* cookie, ENGINE_ERROR_CODE status) {
+        if (status != ENGINE_SUCCESS) {
+            // For non-success status codes clear the cookie's engine_specific;
+            // as the operation is now complete. This ensures that any
+            // subsequent call by the same cookie to store() is treated as a new
+            // operation (and not the completion of the previous one).
+            engine.storeEngineSpecific(cookie, nullptr);
+        }
         engine.notifyIOComplete(cookie, status);
     };
 }

engines/ep/tests/module_tests/evp_store_durability_test.cc

@@ -790,6 +790,47 @@ TEST_P(DurabilityBucketTest, TakeoverSendsDurabilityAmbiguous) {
     EXPECT_EQ(ENGINE_SYNC_WRITE_AMBIGUOUS, mockCookie->status);
 }
 
+// Test that if a SyncWrite times out, then a subsequent SyncWrite which
+// _should_ fail does indeed fail.
+// (Regression test for part of MB-34367 - after using notify_IO_complete
+// to report the SyncWrite was timed out with status eambiguous, the outstanding
+// cookie context was not correctly cleared.
+TEST_P(DurabilityBucketTest, MutationAfterTimeoutCorrect) {
+    setVBucketStateAndRunPersistTask(
+            vbid,
+            vbucket_state_active,
+            {{"topology", nlohmann::json::array({{"active", "replica"}})}});
+
+    // Setup: make pending item and store it; then abort it (at VBucket) level.
+    auto key = makeStoredDocKey("key");
+    auto pending = makePendingItem(key, "value");
+    uint64_t cas;
+    ASSERT_EQ(ENGINE_EWOULDBLOCK,
+              engine->store(cookie,
+                            pending.get(),
+                            cas,
+                            OPERATION_SET,
+                            pending->getDurabilityReqs(),
+                            DocumentState::Alive));
+    ASSERT_TRUE(engine->getEngineSpecific(cookie))
+            << "Expected engine specific to be set for cookie after "
+               "EWOULDBLOCK";
+
+    auto& vb = *store->getVBucket(vbid);
+    ASSERT_EQ(ENGINE_SUCCESS,
+              vb.abort(key, {}, vb.lockCollections(key), cookie));
+
+    // Test: Attempt another SyncWrite, which _should_ fail (in this case just
+    // use replace against the same non-existent key).
+    ASSERT_EQ(ENGINE_KEY_ENOENT,
+              engine->store(cookie,
+                            pending.get(),
+                            cas,
+                            OPERATION_REPLACE,
+                            pending->getDurabilityReqs(),
+                            DocumentState::Alive));
+}
+
 // Test cases which run against all persistent storage backends.
 INSTANTIATE_TEST_CASE_P(
         AllBackends,