SQLite: Fix spurious `BEGIN TRANSACTION should have failed when alrea…

…dy in a transaction?`

Bug introduced in #2866. See comments for explanation.

src/workerd/api/sql-test.js

@@ -1294,6 +1294,40 @@ export class DurableObjectExample extends DurableObject {
   }
 
   async alarm() {}
+
+  async testMultiStatement() {
+    // Performing this PRAGMA will cause sqlite to invalidate prepared statements and re-compile
+    // them the next time they are executed. (Probably, many other pragmas would have the same
+    // effect, but this is the one that we observed causing issues.)
+    //
+    // In particular, the prepared statement ActorSqlite::beginTxn, which is simply
+    // `BEGIN TRANSACTION`, will be invalidated and recomplied on the next invocation.
+    //
+    // When we perform our multi-statement exec below, the first line will invoke the
+    // `ActorSqlite::onWrite` callback, which will invoke `beginTxn`. Because `BEGIN TRANSACTION`
+    // must be recompiled, the SQLite authorizer callback will be invoked to check if it is
+    // authorized. But we use the authorizer callback to detect when SQLite has parsed a statement
+    // as a transaction statement. At one point, we had a bug where we incorrectly thought that
+    // the authorizer was being called on behalf of the statement we were trying to parse and
+    // execute, namely, `CREATE TABLE items...`. We therefore incorrectly made note that this
+    // statement was beginning a transaction. This led the transaction state tracking to become
+    // all wrong!
+    //
+    // This only turned out to be an issue when performing a multi-statement exec(), because in
+    // this case all statements except the last are executed inside the parse loop, which is why
+    // we misinterpreted the authorizer callback.
+    this.state.storage.sql.exec("PRAGMA case_sensitive_like = TRUE");
+
+    let cursor = this.state.storage.sql.exec(`
+      CREATE TABLE items(i INTEGER, s TEXT);
+      CREATE INDEX itemsIdx ON items(s);
+      INSERT INTO items VALUES (123, "abc");
+      INSERT INTO items VALUES (456, "def");
+      SELECT i FROM items WHERE s = "abc";
+    `);
+
+    assert.deepEqual([...cursor], [{i: 123}]);
+  }
 }
 
 export default {
@@ -1380,6 +1414,13 @@ export let testRollbackKvInit = {
   },
 };
 
+export let testMultiStatement = {
+  async test(ctrl, env, ctx) {
+    let stub = env.ns.get(env.ns.idFromName('multi-statement-test'));
+    await stub.testMultiStatement();
+  },
+};
+
 const INSERT_36_ROWS = ['a', 'b', 'c', 'd', 'e', 'f']
   .map(
     (prefix) =>

src/workerd/util/sqlite.c++

@@ -613,9 +613,11 @@ SqliteDatabase::StatementAndEffect SqliteDatabase::prepareSql(
           KJ_IF_SOME(cb, onWriteCallback) {
             if (!sqlite3_stmt_readonly(result)) {
               // The callback is allowed to invoke queries of its own, so we have to un-set the
-              // regulator while we call it.
+              // regulator and parse context while we call it.
               currentRegulator = kj::none;
               KJ_DEFER(currentRegulator = regulator);
+              currentParseContext = kj::none;
+              KJ_DEFER(currentParseContext = parseContext);
               cb();
             }
           }