MB-34393: Subdoc: Correctly handle NOT_STORED race when adding doc

During a subdoc mutation, if the user specified doc_flag::Add, and the
key didn't exist at the start of the command processing, but did exist
at the point we finally store the new value (i.e. another connection
added the key just before us), then the engine update fails with
ENGINE_NOT_STORED (as expected).

However, this status isn't correctly handled by the subdocument state
machine, resulting in an exception being thrown and the user
connection being terminated:

    2019-05-30T16:14:39.675236+01:00 WARNING 47: exception occurred in runloop during packet execution.
    <cut>
    conn_execute: Should leave conn_execute for !EWOULDBLOCK

Issue is in checking the status code from bucket_store in
subdoc_update() - if NOT_STORED and we _don't_ remap to KEY_EEXISTS
(for a retry), return a response to the client.

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

Author: daverigby

daemon/subdocument.cc

@@ -1313,6 +1313,9 @@ static ENGINE_ERROR_CODE subdoc_update(SubdocCmdContext& context,
         if (new_op == OPERATION_ADD &&
             context.mutationSemantics == MutationSemantics::Set) {
             ret = ENGINE_KEY_EEXISTS;
+        } else {
+            // Otherwise ENGINE_NOT_STORED is terminal - return to client.
+            cookie.sendResponse(cb::engine_errc(ret));
         }
         break;
 

tests/testapp/testapp_subdoc.cc

@@ -647,14 +647,14 @@ TEST_P(SubdocTestappTest, SubdocDictUpsert_CasCompressed) {
                              cb::mcbp::ClientOpcode::SubdocDictUpsert);
 }
 
+// Check that if an item is set by another client after the fetch but
+// before the store then we return NotStored.
+// (MB-34393)
 TEST_P(SubdocTestappTest, SubdocAddFlag_BucketStoreCas) {
-    // Check that if an item is set by another client after the fetch but
-    // before the store then we return EEXISTS.
-
     // Setup ewouldblock_engine - first two calls succeed, 3rd (engine->store)
-    // fails. Any further calls should return EEXISTS.
+    // fails with NOT_STORED (key already exists).
     ewouldblock_engine_configure(
-            ENGINE_KEY_EEXISTS,
+            ENGINE_NOT_STORED,
             EWBEngineMode::Sequence,
             0xfffffffc /* <3 MSBytes all-ones>, 0b11,111,100 */);
     EXPECT_SD_ERR(BinprotSubdocCommand(cb::mcbp::ClientOpcode::SubdocDictUpsert,
@@ -664,7 +664,7 @@ TEST_P(SubdocTestappTest, SubdocAddFlag_BucketStoreCas) {
                                        SUBDOC_FLAG_NONE,
                                        mcbp::subdoc::doc_flag::Add,
                                        0),
-                  cb::mcbp::Status::KeyEexists);
+                  cb::mcbp::Status::NotStored);
 }
 
 void SubdocTestappTest::test_subdoc_dict_add_upsert_deep(