Clean up sql-kv-put-multiple-transaction autogate

We’ve been using this in production for over a month and have no
intention of reverting the autogate.

Author: justin-mp

src/workerd/io/actor-sqlite-test.c++

@@ -235,13 +235,9 @@ KJ_TEST("check put multiple wraps operations in a transaction") {
   }
 }
 
-KJ_TEST(
-    "check put multiple wraps operations in a transaction with sql-kv-put-multiple-transaction autogate") {
+KJ_TEST("check put multiple wraps operations in a transaction") {
   ActorSqliteTest test;
 
-  // This test should reflect the same behavior we saw without the autogate enabled.
-  util::Autogate::initAutogateNamesForTest({"sql-kv-put-multiple-transaction"_kj});
-
   kj::Vector<ActorCache::KeyValuePair> putKVs;
   putKVs.add(ActorCache::KeyValuePair{kj::str("foo"), kj::heapArray(kj::str("bar").asBytes())});
 
@@ -285,105 +281,10 @@ KJ_TEST(
   }
 }
 
-KJ_TEST("check put multiple wraps operations in a transaction and does not rollback on error") {
-  ActorSqliteTest test;
-
-  if (util::Autogate::isEnabled(util::AutogateKey::SQL_KV_PUT_MULTIPLE_TRANSACTION)) {
-    // We should skip this test as it will expect a different behavior when
-    // SQL_KV_PUT_MULTIPLE_TRANSACTION is set
-    KJ_DBG("Skipping test because SQL_KV_PUT_MULTIPLE_TRANSACTION is enabled.");
-    return;
-  }
-
-  // Let's deinit the autogate. This will enforce the old behavior where putMultiple would commit
-  // some puts, until a single put failed.
-  util::Autogate::deinitAutogate();
-
-  kj::Vector<ActorCache::KeyValuePair> putKVs;
-
-  // Add some regular key-value pairs that we know are supported
-  putKVs.add(ActorCache::KeyValuePair{kj::str("foo"), kj::heapArray(kj::str("bar").asBytes())});
-  putKVs.add(ActorCache::KeyValuePair{kj::str("foo2"), kj::heapArray(kj::str("bar2").asBytes())});
-  putKVs.add(ActorCache::KeyValuePair{kj::str("foo3"), kj::heapArray(kj::str("bar3").asBytes())});
-
-  // Now create a key that's too large. Should fail with  string or blob too big: SQLITE_TOOBIG
-  auto tooLongKey = kj::heapString(2200000);
-  tooLongKey.asArray().fill('a');
-  // Add it to our KV array
-  putKVs.add(
-      ActorCache::KeyValuePair{kj::str(tooLongKey), kj::heapArray(kj::str("bar").asBytes())});
-
-  // NoTxn test
-  {
-    // Check that we're in a NoTxn
-    KJ_ASSERT(!test.actor.isCommitScheduled());
-    try {
-      test.putMultiple(putKVs.releaseAsArray());
-      // We should fail with correct error before reaching here.
-      KJ_UNREACHABLE;
-    } catch (kj::Exception& e) {
-      KJ_ASSERT(
-          e.getDescription() == "expected false; jsg.Error: string or blob too big: SQLITE_TOOBIG");
-    }
-    KJ_ASSERT(KJ_ASSERT_NONNULL(expectSync(test.get("foo"))) == kj::str("bar").asBytes());
-    KJ_ASSERT(KJ_ASSERT_NONNULL(expectSync(test.get("foo2"))) == kj::str("bar2").asBytes());
-    KJ_ASSERT(KJ_ASSERT_NONNULL(expectSync(test.get("foo3"))) == kj::str("bar3").asBytes());
-    // During write, all NoTxn operations are wrapped in an ImplicitTxn. Since some puts succeeded
-    // we need to flush commit.
-    auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
-    commitFulfiller->fulfill();
-  }
-
-  {
-    // Let's clear the db.
-    test.actor.deleteAll({});
-    auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
-    commitFulfiller->fulfill();
-    KJ_ASSERT(expectSync(test.get(kj::str("foo"))) == nullptr);
-    KJ_ASSERT(expectSync(test.get(kj::str("foo2"))) == nullptr);
-    KJ_ASSERT(expectSync(test.get(kj::str("foo3"))) == nullptr);
-  }
-
-  // ExplicitTxn test
-  {
-    KJ_ASSERT(!test.actor.isCommitScheduled());
-    // Similar to the previous putMultiple, but wrapped in a transactionSync (ExplicitTxn)
-    test.putMultipleExplicitTxn(putKVs.releaseAsArray());
-    auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
-    commitFulfiller->fulfill();
-    // This was wrapped in an explicit transaction. We rolled back as expected.
-    KJ_ASSERT(expectSync(test.get(kj::str("foo"))) == nullptr);
-    KJ_ASSERT(expectSync(test.get(kj::str("foo2"))) == nullptr);
-    KJ_ASSERT(expectSync(test.get(kj::str("foo3"))) == nullptr);
-  }
-
-  // ImplicitTxn test
-  {
-    // A single put will create an ImplicitTxn that we can use to wrap our putMultiple into.
-    KJ_ASSERT(!test.actor.isCommitScheduled());
-    test.put("baz", "bat");
-
-    // By now, we should check there's a commit scheduled in a ImplicitTxn.
-    KJ_ASSERT(test.actor.isCommitScheduled());
-    test.putMultiple(putKVs.releaseAsArray());
-
-    auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
-    // The single put succeeded, but the putMultiple did not.
-    KJ_ASSERT(KJ_ASSERT_NONNULL(expectSync(test.get("baz"))) == kj::str("bat").asBytes());
-    KJ_ASSERT(expectSync(test.get(kj::str("foo"))) == nullptr);
-    KJ_ASSERT(expectSync(test.get(kj::str("foo2"))) == nullptr);
-    KJ_ASSERT(expectSync(test.get(kj::str("foo3"))) == nullptr);
-    commitFulfiller->fulfill();
-  }
-}
-
-KJ_TEST(
-    "check put multiple wraps operations in a transaction and rollback on error with sql-kv-put-multiple-transaction autogate") {
+KJ_TEST("check put multiple wraps operations in a transaction and rollback on error") {
   ActorSqliteTest test;
 
-  // With the autogate enabled, we expect that putMultiple is all of nothing, rolling back if a
-  // single put fails.
-  util::Autogate::initAutogateNamesForTest({"sql-kv-put-multiple-transaction"_kj});
+  // We expect that putMultiple is all or nothing, rolling back if a single put fails.
 
   kj::Vector<ActorCache::KeyValuePair> putKVs;
 

src/workerd/io/actor-sqlite.c++

@@ -532,46 +532,39 @@ kj::Maybe<kj::Promise<void>> ActorSqlite::put(Key key, Value value, WriteOptions
 kj::Maybe<kj::Promise<void>> ActorSqlite::put(kj::Array<KeyValuePair> pairs, WriteOptions options) {
   requireNotBroken();
   // TODO(cleanup): Most of this code comes from DurableObjectStorage::transactionSync and could be re-used.
-  if (util::Autogate::isEnabled(util::AutogateKey::SQL_KV_PUT_MULTIPLE_TRANSACTION)) {
-    if (currentTxn.is<NoTxn>()) {
-      // If we are not in a transaction, let's use an ExplicitTxn, which should rollback automatically
-      // if some put fails.
-
-      // TODO(someday) this should really start an ImplicitTxn, which has the advantage of
-      // supporting allowUnconfirmed.
-      disableAllowUnconfirmed(options, "multi put will create an ExplicitTxn");
-
-      auto txn = startTransaction();
-      txn->put(kj::mv(pairs), options);
-      txn->commit();
-    } else {
-      if (currentTxn.is<ExplicitTxn*>()) {
-        disableAllowUnconfirmed(options, "multi put is using an already-existing ExplicitTxn");
-      }
+  if (currentTxn.is<NoTxn>()) {
+    // If we are not in a transaction, let's use an ExplicitTxn, which should rollback automatically
+    // if some put fails.
 
-      // If we are in a transaction, let's just set a SAVEPOINT that we can rollback to if needed.
-      db->run(
-          {.regulator = SqliteDatabase::TRUSTED}, kj::str("SAVEPOINT _cf_put_multiple_savepoint"));
-      for (const auto& pair: pairs) {
-        try {
-          kv.put(pair.key, pair.value, {.allowUnconfirmed = options.allowUnconfirmed});
-        } catch (kj::Exception& e) {
-          // We need to rollback to the putMultiple SAVEPOINT. Do it, and then release the SAVEPOINT.
-          db->run({.regulator = SqliteDatabase::TRUSTED},
-              kj::str("ROLLBACK TO _cf_put_multiple_savepoint"));
-          db->run({.regulator = SqliteDatabase::TRUSTED},
-              kj::str("RELEASE _cf_put_multiple_savepoint"));
-          kj::throwFatalException(kj::mv(e));
-        }
-      }
-      // We're done here. RELEASE the savepoint.
-      db->run(
-          {.regulator = SqliteDatabase::TRUSTED}, kj::str("RELEASE _cf_put_multiple_savepoint"));
-    }
+    // TODO(someday) this should really start an ImplicitTxn, which has the advantage of
+    // supporting allowUnconfirmed.
+    disableAllowUnconfirmed(options, "multi put will create an ExplicitTxn");
+
+    auto txn = startTransaction();
+    txn->put(kj::mv(pairs), options);
+    txn->commit();
   } else {
-    for (auto& pair: pairs) {
-      kv.put(pair.key, pair.value, {.allowUnconfirmed = options.allowUnconfirmed});
+    if (currentTxn.is<ExplicitTxn*>()) {
+      disableAllowUnconfirmed(options, "multi put is using an already-existing ExplicitTxn");
+    }
+
+    // If we are in a transaction, let's just set a SAVEPOINT that we can rollback to if needed.
+    db->run(
+        {.regulator = SqliteDatabase::TRUSTED}, kj::str("SAVEPOINT _cf_put_multiple_savepoint"));
+    for (const auto& pair: pairs) {
+      try {
+        kv.put(pair.key, pair.value, {.allowUnconfirmed = options.allowUnconfirmed});
+      } catch (kj::Exception& e) {
+        // We need to rollback to the putMultiple SAVEPOINT. Do it, and then release the SAVEPOINT.
+        db->run({.regulator = SqliteDatabase::TRUSTED},
+            kj::str("ROLLBACK TO _cf_put_multiple_savepoint"));
+        db->run(
+            {.regulator = SqliteDatabase::TRUSTED}, kj::str("RELEASE _cf_put_multiple_savepoint"));
+        kj::throwFatalException(kj::mv(e));
+      }
     }
+    // We're done here. RELEASE the savepoint.
+    db->run({.regulator = SqliteDatabase::TRUSTED}, kj::str("RELEASE _cf_put_multiple_savepoint"));
   }
   return kj::none;
 }

src/workerd/util/autogate.c++

@@ -25,8 +25,6 @@ kj::StringPtr KJ_STRINGIFY(AutogateKey key) {
       return "streaming-tail-worker"_kj;
     case AutogateKey::TAIL_STREAM_REFACTOR:
       return "tail-stream-refactor"_kj;
-    case AutogateKey::SQL_KV_PUT_MULTIPLE_TRANSACTION:
-      return "sql-kv-put-multiple-transaction"_kj;
     case AutogateKey::NumOfKeys:
       KJ_FAIL_ASSERT("NumOfKeys should not be used in getName");
   }

src/workerd/util/autogate.h

@@ -20,8 +20,6 @@ enum class AutogateKey {
   STREAMING_TAIL_WORKER,
   // Enable refactor used to consolidate the different tail worker stream implementations.
   TAIL_STREAM_REFACTOR,
-  // Enable wrapping DO SQL KV put multiple in a transaction, fully rolling back if some put fails
-  SQL_KV_PUT_MULTIPLE_TRANSACTION,
   NumOfKeys  // Reserved for iteration.
 };