fix(txn): keep a connection usable after a refused BEGIN

src/include/transaction/transaction_context.h

@@ -20,7 +20,8 @@ namespace transaction {
  * rollback the transaction by calling commit() or rollback().
  *
  * AUTO_COMMIT is the default mode when a Connection is created. If an application calls
- * begin[ReadOnly/Write]Transaction at any point, the mode switches to MANUAL. This creates
+ * begin[ReadOnly/Write]Transaction at any point and the begin succeeds, the mode switches to
+ * MANUAL; a refused begin leaves the connection in AUTO_COMMIT. This creates
  * an "active transaction" in the connection. When a connection is in MANUAL mode and the
  * active transaction is rolled back or committed, then the active transaction is removed (so
  * the connection no longer has an active transaction), and the mode automatically switches
@@ -56,10 +57,14 @@ class LBUG_API TransactionContext {
 
 private:
     void beginTransactionInternal(TransactionType transactionType);
+    void beginManualTransaction(TransactionType transactionType);
 
 private:
     std::mutex mtx;
     main::ClientContext& clientContext;
+    // MANUAL implies activeTransaction != nullptr: beginManualTransaction sets the mode only after
+    // the begin succeeded and clearTransaction resets both. Only beginManualTransaction takes mtx;
+    // the invariant relies on a connection being used by one thread at a time.
     TransactionMode mode;
     Transaction* activeTransaction;
 };

src/transaction/transaction_context.cpp

@@ -17,15 +17,11 @@ TransactionContext::TransactionContext(main::ClientContext& clientContext)
 TransactionContext::~TransactionContext() = default;
 
 void TransactionContext::beginReadTransaction() {
-    std::unique_lock lck{mtx};
-    mode = TransactionMode::MANUAL;
-    beginTransactionInternal(TransactionType::READ_ONLY);
+    beginManualTransaction(TransactionType::READ_ONLY);
 }
 
 void TransactionContext::beginWriteTransaction() {
-    std::unique_lock lck{mtx};
-    mode = TransactionMode::MANUAL;
-    beginTransactionInternal(TransactionType::WRITE);
+    beginManualTransaction(TransactionType::WRITE);
 }
 
 void TransactionContext::beginAutoTransaction(bool readOnlyStatement) {
@@ -40,9 +36,7 @@ void TransactionContext::beginAutoTransaction(bool readOnlyStatement) {
 }
 
 void TransactionContext::beginRecoveryTransaction() {
-    std::unique_lock lck{mtx};
-    mode = TransactionMode::MANUAL;
-    beginTransactionInternal(TransactionType::RECOVERY);
+    beginManualTransaction(TransactionType::RECOVERY);
 }
 
 void TransactionContext::validateManualTransaction(bool readOnlyStatement) const {
@@ -87,6 +81,13 @@ TransactionContext* TransactionContext::Get(const main::ClientContext& context)
     return context.transactionContext.get();
 }
 
+void TransactionContext::beginManualTransaction(TransactionType transactionType) {
+    std::unique_lock lck{mtx};
+    beginTransactionInternal(transactionType);
+    // Only once the begin succeeded: a refused BEGIN leaves the connection in auto-commit mode.
+    mode = TransactionMode::MANUAL;
+}
+
 void TransactionContext::beginTransactionInternal(TransactionType transactionType) {
     DASSERT(!activeTransaction);
     activeTransaction = clientContext.getDatabase()->getTransactionManager()->beginTransaction(

test/test_files/transaction/concurrency/ddl_serial_execution.test

@@ -17,6 +17,38 @@ Cannot start a new write transaction in the system. Only one write transaction a
 0|a|INT32|NULL|True
 1|b|INT32|NULL|False
 
+-CASE RefusedBeginKeepsConnectionUsable
+-STATEMENT CREATE NODE TABLE t(a INT, PRIMARY KEY(a));
+---- ok
+-STATEMENT BEGIN TRANSACTION;
+---- ok
+-STATEMENT CREATE (:t {a: 1});
+---- ok
+-CREATE_CONNECTION conn2
+-STATEMENT [conn2] BEGIN TRANSACTION;
+---- error
+Cannot start a new write transaction in the system. Only one write transaction at a time is allowed in the system.
+-STATEMENT [conn2] ROLLBACK;
+---- error
+No active transaction for ROLLBACK.
+-STATEMENT [conn2] MATCH (n:t) RETURN count(*);
+---- 1
+0
+-STATEMENT [conn2] CREATE (:t {a: 3});
+---- error
+Cannot start a new write transaction in the system. Only one write transaction at a time is allowed in the system.
+-STATEMENT COMMIT;
+---- ok
+-STATEMENT [conn2] BEGIN TRANSACTION;
+---- ok
+-STATEMENT [conn2] CREATE (:t {a: 2});
+---- ok
+-STATEMENT [conn2] COMMIT;
+---- ok
+-STATEMENT MATCH (n:t) RETURN count(*);
+---- 1
+2
+
 # Test CALL debug_enable_multi_writes=true
 -CASE EnableMultiWrites
 -STATEMENT CALL debug_enable_multi_writes=true;

test/transaction/transaction_test.cpp

@@ -3,6 +3,7 @@
 #include "api_test/api_test.h"
 #include "api_test/private_api_test.h"
 #include "common/exception/runtime.h"
+#include "common/exception/transaction_manager.h"
 #include "graph_test/private_graph_test.h"
 #include "storage/buffer_manager/buffer_manager.h"
 #include "storage/buffer_manager/memory_manager.h"
@@ -46,6 +47,51 @@ TEST_F(PrivateApiTest, TransactionModes) {
     ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn));
 }
 
+TEST_F(PrivateApiTest, RefusedBeginKeepsAutoMode) {
+    auto conn2 = std::make_unique<lbug::main::Connection>(database.get());
+    ASSERT_TRUE(conn->query("BEGIN TRANSACTION;")->isSuccess());
+    ASSERT_FALSE(conn2->query("BEGIN TRANSACTION;")->isSuccess());
+    ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2));
+    EXPECT_THROW(TransactionContext::Get(*getClientContext(*conn2))->beginRecoveryTransaction(),
+        TransactionManagerException);
+    ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2));
+    ASSERT_TRUE(conn2->query("RETURN 1;")->isSuccess());
+    ASSERT_TRUE(conn->query("COMMIT;")->isSuccess());
+}
+
+TEST_F(PrivateApiTest, RefusedPreparedAndMultiStatementBeginKeepAutoMode) {
+    auto conn2 = std::make_unique<lbug::main::Connection>(database.get());
+    ASSERT_TRUE(conn->query("BEGIN TRANSACTION;")->isSuccess());
+    auto prepared = conn2->prepare("BEGIN TRANSACTION;");
+    ASSERT_TRUE(prepared->isSuccess()) << prepared->getErrorMessage();
+    ASSERT_FALSE(conn2->execute(prepared.get())->isSuccess());
+    ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2));
+    ASSERT_FALSE(conn2->query("BEGIN TRANSACTION; RETURN 1;")->isSuccess());
+    ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2));
+    ASSERT_TRUE(conn2->query("RETURN 1;")->isSuccess());
+    ASSERT_TRUE(conn->query("COMMIT;")->isSuccess());
+}
+
+TEST_F(PrivateApiTest, BeginOnPoisonedWALKeepsAutoMode) {
+    lbug::storage::WAL::Get(*getClientContext(*conn))->poison("test");
+    auto result = conn->query("BEGIN TRANSACTION;");
+    ASSERT_FALSE(result->isSuccess());
+    EXPECT_NE(result->getErrorMessage().find("panic state"), std::string::npos)
+        << result->getErrorMessage();
+    ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn));
+    ASSERT_TRUE(conn->query("RETURN 1;")->isSuccess());
+}
+
+TEST_F(PrivateApiTest, ReadOnlyBeginBesideWriterSwitchesToManual) {
+    auto conn2 = std::make_unique<lbug::main::Connection>(database.get());
+    ASSERT_TRUE(conn->query("BEGIN TRANSACTION;")->isSuccess());
+    ASSERT_TRUE(conn2->query("BEGIN TRANSACTION READ ONLY;")->isSuccess());
+    ASSERT_EQ(TransactionMode::MANUAL, getTransactionMode(*conn2));
+    ASSERT_TRUE(conn2->query("COMMIT;")->isSuccess());
+    ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2));
+    ASSERT_TRUE(conn->query("COMMIT;")->isSuccess());
+}
+
 TEST_F(PrivateApiTest, MultipleCallsFromSameTransaction) {
     conn->query("BEGIN TRANSACTION READ ONLY;");
     auto activeTransactionID = getActiveTransactionID(*conn);