Merge remote-tracking branch 'couchbase/alice'

* couchbase/alice:
  MB-33852: Fix NOT_STORED being returned from arithmetic op

Change-Id: Idf8748cb826961851dd045c7da7d8600d0531429

Author: daverigby

daemon/protocol/mcbp/arithmetic_context.cc

@@ -122,6 +122,13 @@ ENGINE_ERROR_CODE ArithmeticCommandContext::storeNewItem() {
     } else if (ret == ENGINE_KEY_EEXISTS && cas == 0) {
         state = State::Reset;
         ret = ENGINE_SUCCESS;
+    } else if(ret == ENGINE_NOT_STORED) {
+        // hit race condition, item with our key was created between our call
+        // to bucket_store() and when we checked to see if it existed, by
+        // calling bucket_get() so just re-try by resetting our state to the
+        // start again.
+        state = State::Reset;
+        ret = ENGINE_SUCCESS;
     }
 
     return ret;

protocol/connection/client_connection.h

@@ -529,6 +529,15 @@ class MemcachedConnection {
             uint32_t value = 0,
             const std::string& key = "");
 
+    /**
+     * Disable the ewouldblock engine entirely.
+     */
+    void disableEwouldBlockEngine() {
+        // We disable the engine by telling it to inject the given error
+        // the next 0 times
+        configureEwouldBlockEngine(EWBEngineMode::Next_N, ENGINE_SUCCESS, 0);
+    }
+
     /**
      * Identify ourself to the server.
      *

tests/testapp/testapp_arithmetic.cc

@@ -224,6 +224,83 @@ TEST_P(ArithmeticTest, TestIllegalDatatype) {
     }
 }
 
+/*
+ * Unit test to test the fix for MB-33813
+ * This test checks that we do not return NOT_STORED back to the client
+ * if bucket_store returns it to ArithmeticCommandContext::storeNewItem().
+ * Instead ArithmeticCommandContext::storeNewItem() should reset the
+ * ArithmeticCommandContext state machine and try again.
+ *
+ * To check that ENGINE_NOT_STORED/NOT_STORED is not returned we use
+ * eWouldBlockEngine to return ENGINE_NOT_STORED on the 3rd engine request using
+ * the binary sequence 0b100.
+ *
+ * This works as the process we expect to see by the ArithmeticCommandContext
+ * state machine is as follows:
+ *  start: GetItem
+ *      calls bucket_get() should return ENGINE_SUCCESS
+ *  -> CreateNewItem
+ *      calls bucket_allocate_ex should return ENGINE_SUCCESS
+ *  -> StoreNewItem
+ *      calls bucket_store return ENGINE_NOT_STORED
+ *      ENGINE_NOT_STORED returned so reset and try again.
+ *  -> Reset
+ *  -> GetItem
+ *  .... Do the work to increment the value again
+ *  -> Done
+ */
+TEST_P(ArithmeticTest, MB33813) {
+    auto& connection = getConnection();
+    std::string key(name + "_inc");
+
+    // Make the 3rd request send to the engine return ENGINE_NOT_STORED
+    // In this case this will be the store that happens as a result of
+    // a call to ArithmeticCommandContext::storeNewItem()
+    // We also set all the higher bits after the 6th. So that if the sequence
+    // of engine accesses change this test will fail.
+    connection.configureEwouldBlockEngine(
+            EWBEngineMode::Sequence,
+            ENGINE_NOT_STORED,
+            /* 0b11111111111111111111111111000100 */ 0xffffffc4);
+
+    EXPECT_EQ(1, connection.increment(key, 0, 1));
+
+    connection.disableEwouldBlockEngine();
+
+    Vbid vb(0);
+    Document doc = connection.get(key, vb);
+    EXPECT_EQ("1", doc.value);
+
+    EXPECT_EQ(2, connection.increment(key, 1));
+
+    doc = connection.get(key, vb);
+    EXPECT_EQ("2", doc.value);
+
+    // Sanity check do the same thing but with a decrement
+    key = name + "_dec";
+    // Make the 3rd request send to the engine return ENGINE_NOT_STORED
+    // In this case this will be the store that happens as a result of
+    // a call to ArithmeticCommandContext::storeNewItem()
+    // We also set all the higher bits after the 6th. So that if the sequence
+    // of engine accesses change this test will fail.
+    connection.configureEwouldBlockEngine(
+            EWBEngineMode::Sequence,
+            ENGINE_NOT_STORED,
+            /* 0b11111111111111111111111111000100 */ 0xffffffc4);
+
+    EXPECT_EQ(2, connection.decrement(key, 0, 2));
+
+    connection.disableEwouldBlockEngine();
+
+    doc = connection.get(key, vb);
+    EXPECT_EQ("2", doc.value);
+
+    EXPECT_EQ(1, connection.decrement(key, 1));
+
+    doc = connection.get(key, vb);
+    EXPECT_EQ("1", doc.value);
+}
+
 static void test_stored_doc(MemcachedConnection& conn,
                             const std::string& key,
                             const std::string& content,