diff --git a/packages/libsql-client/src/__tests__/client.test.ts b/packages/libsql-client/src/__tests__/client.test.ts index a0d7e2f..754a132 100644 --- a/packages/libsql-client/src/__tests__/client.test.ts +++ b/packages/libsql-client/src/__tests__/client.test.ts @@ -2048,6 +2048,73 @@ describe("timeout option (local files)", () => { withoutTimeout.close(); } }); + + // A COMMIT that fails with SQLITE_BUSY must not leave the database locked: + // once the failed transaction is rolled back and the reader is gone, other + // connections must be able to write again. + for (const [label, failCommit] of [ + [ + "transaction commit()", + async (c: libsql.Client) => { + const txn = await c.transaction("deferred"); + await txn.execute("INSERT INTO t VALUES (1)"); + await expect(txn.commit()).rejects.toBeLibsqlError( + "SQLITE_BUSY", + ); + }, + ], + [ + "batch()", + async (c: libsql.Client) => { + await expect( + c.batch(["INSERT INTO t VALUES (1)"], "deferred"), + ).rejects.toBeLibsqlError("SQLITE_BUSY"); + }, + ], + [ + "migrate()", + async (c: libsql.Client) => { + await expect( + c.migrate([{ sql: "INSERT INTO t VALUES (1)", args: [] }]), + ).rejects.toBeLibsqlError("SQLITE_BUSY"); + }, + ], + ] as const) { + test(`a COMMIT that fails with SQLITE_BUSY in ${label} releases its lock`, async () => { + const setup = createClient({ url: dbUrl }); + await setup.execute("CREATE TABLE t (a)"); + setup.close(); + + const writer = createClient({ url: dbUrl }); + const reader = createClient({ url: dbUrl }); + try { + // A read transaction holds a SHARED lock, so the writer's COMMIT + // cannot take the EXCLUSIVE lock it needs. + const read = await reader.transaction("read"); + await read.execute("SELECT count(*) FROM t"); + + await failCommit(writer); + await read.rollback(); + + // Neither the writer's own pool nor a new connection may be left + // blocked by the failed COMMIT. + await writer.execute("INSERT INTO t VALUES (2)"); + const other = createClient({ url: dbUrl }); + try { + await other.execute("INSERT INTO t VALUES (3)"); + const rs = await other.execute( + "SELECT count(*) AS n FROM t", + ); + expect(rs.rows[0]["n"]).toStrictEqual(2); + } finally { + other.close(); + } + } finally { + writer.close(); + reader.close(); + } + }); + } }); (isFile ? test : test.skip)("raw error codes", async () => { diff --git a/packages/libsql-client/src/sqlite3.ts b/packages/libsql-client/src/sqlite3.ts index 3044da3..9b9b7a7 100644 --- a/packages/libsql-client/src/sqlite3.ts +++ b/packages/libsql-client/src/sqlite3.ts @@ -218,7 +218,7 @@ export class ConnectionPool { // the next borrower would silently inherit it. if (db.open && db.inTransaction) { try { - db.prepare("ROLLBACK").run(); + executeControl(db, "ROLLBACK"); } catch { // the connection is unusable; drop it rather than reuse it closeQuietly(db); @@ -339,7 +339,7 @@ export class Sqlite3Client implements Client { const db = await this.#pool.acquire(); try { this.#checkUsable(db); - executeStmt(db, transactionModeToBegin(mode), this.#intMode); + executeControl(db, transactionModeToBegin(mode)); const resultSets = []; for (let i = 0; i < stmts.length; i++) { try { @@ -374,7 +374,7 @@ export class Sqlite3Client implements Client { throw e; } } - executeStmt(db, "COMMIT", this.#intMode); + executeControl(db, "COMMIT"); return resultSets; } finally { // `release` rolls back anything still open before reuse @@ -388,7 +388,7 @@ export class Sqlite3Client implements Client { try { this.#checkUsable(db); executeStmt(db, "PRAGMA foreign_keys=off", this.#intMode); - executeStmt(db, transactionModeToBegin("deferred"), this.#intMode); + executeControl(db, transactionModeToBegin("deferred")); const resultSets = []; for (let i = 0; i < stmts.length; i++) { try { @@ -417,14 +417,21 @@ export class Sqlite3Client implements Client { throw e; } } - executeStmt(db, "COMMIT", this.#intMode); + executeControl(db, "COMMIT"); return resultSets; } finally { - if (db.inTransaction) { - executeStmt(db, "ROLLBACK", this.#intMode); + try { + if (db.open && db.inTransaction) { + executeControl(db, "ROLLBACK"); + } + if (db.open) { + executeStmt(db, "PRAGMA foreign_keys=on", this.#intMode); + } + } finally { + // always give the connection back; `release` drops it if the + // rollback above failed and it is still inside a transaction + this.#pool.release(db); } - executeStmt(db, "PRAGMA foreign_keys=on", this.#intMode); - this.#pool.release(db); } } @@ -433,7 +440,7 @@ export class Sqlite3Client implements Client { const db = await this.#pool.acquire(true); try { this.#checkUsable(db); - executeStmt(db, transactionModeToBegin(mode), this.#intMode); + executeControl(db, transactionModeToBegin(mode)); } catch (e) { this.#pool.release(db); throw e; @@ -601,7 +608,7 @@ export class Sqlite3Transaction implements Transaction { } try { this.#checkNotClosed(); - executeStmt(db, "ROLLBACK", this.#intMode); + executeControl(db, "ROLLBACK"); } finally { this.#settle(); } @@ -609,7 +616,7 @@ export class Sqlite3Transaction implements Transaction { async commit(): Promise { try { - executeStmt(this.#getDatabase(), "COMMIT", this.#intMode); + executeControl(this.#getDatabase(), "COMMIT"); } finally { this.#settle(); } @@ -624,7 +631,7 @@ export class Sqlite3Transaction implements Transaction { // `client.close()` may have closed this connection already, and // reading `inTransaction` on a closed database aborts the process. if (db.open && db.inTransaction) { - executeStmt(db, "ROLLBACK", this.#intMode); + executeControl(db, "ROLLBACK"); } } finally { this.#settle(); @@ -816,6 +823,22 @@ function executeMultiple(db: Database.Database, sql: string): void { } } +// Runs a transaction-control statement (BEGIN, COMMIT, ROLLBACK). These go +// through `exec` rather than a prepared statement: when a prepared COMMIT +// fails, for example with SQLITE_BUSY because another connection holds a +// SHARED lock, libsql leaves the statement un-reset, and an un-reset statement +// keeps the connection's lock after a successful ROLLBACK and even after +// close(), until garbage collection finalizes it. Every later write from any +// connection then fails with SQLITE_BUSY. `exec` finalizes its statement +// before returning, whether it succeeded or not. +function executeControl(db: Database.Database, sql: string): void { + try { + db.exec(sql); + } catch (e) { + throw mapSqliteError(e); + } +} + function mapSqliteError(e: unknown): unknown { if (e instanceof Database.SqliteError) { const extendedCode = e.code;