From b92758d37c2668bca0dc55b8cdfe0bc5d24baee8 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sat, 3 Oct 2026 22:01:11 -0700 Subject: [PATCH] fix(server): sqlite transactions wait for the write lock instead of failing Writable connections now begin transactions with BEGIN IMMEDIATE. With a deferred BEGIN, another process (the t3 project CLI) could commit between a transaction's first read and its first write, and the write then failed at once with SQLITE_BUSY_SNAPSHOT, which busy_timeout cannot wait out. Read-only connections keep the deferred BEGIN. node:sqlite reports the result code as errcode while classifySqliteError reads errno, so every busy, locked and constraint failure surfaced as an unknown error. The code is now copied across before classifying. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/shared/src/nodeSqliteClient.test.ts | 80 ++++++++++++++++++++ packages/shared/src/nodeSqliteClient.ts | 58 ++++++++------ 2 files changed, 114 insertions(+), 24 deletions(-) diff --git a/packages/shared/src/nodeSqliteClient.test.ts b/packages/shared/src/nodeSqliteClient.test.ts index bbdaa4624901..4d08effdab56 100644 --- a/packages/shared/src/nodeSqliteClient.test.ts +++ b/packages/shared/src/nodeSqliteClient.test.ts @@ -61,8 +61,88 @@ layer("NodeSqliteClient", (it) => { assert.equal(error.reason.operation, "prepare"); }), ); + + it.effect("classifies constraint failures by their SQLite result code", () => + Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + yield* sql`CREATE TABLE constrained(name TEXT NOT NULL UNIQUE)`; + yield* sql`INSERT INTO constrained VALUES ('taken')`; + + const duplicate = yield* sql`INSERT INTO constrained VALUES ('taken')`.pipe(Effect.flip); + assert(duplicate.reason._tag === "UniqueViolation"); + assert.equal(duplicate.reason.constraint, "constrained.name"); + + const missing = yield* sql`INSERT INTO constrained VALUES (NULL)`.pipe(Effect.flip); + assert.equal(missing.reason._tag, "ConstraintError"); + }), + ); }); +const makeTempDatabase = Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const directory = yield* fs.makeTempDirectoryScoped({ prefix: "t3-sqlite-transaction-" }); + const filename = path.join(directory, "state.sqlite"); + // node:sqlite connections fail a busy statement at once unless given a timeout. + const other = yield* Effect.acquireRelease( + Effect.sync(() => new NodeSqlite.DatabaseSync(filename)), + (database) => Effect.sync(() => database.close()), + ); + yield* Effect.sync(() => { + other.exec(` + PRAGMA journal_mode = WAL; + CREATE TABLE counters(id INTEGER PRIMARY KEY, value INTEGER NOT NULL); + INSERT INTO counters VALUES (1, 0); + `); + }); + return { filename, other }; +}); + +it.effect("keeps another connection from committing between a transaction's read and write", () => + Effect.gen(function* () { + const { filename, other } = yield* makeTempDatabase; + yield* Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + yield* sql.withTransaction( + Effect.gen(function* () { + const [row] = yield* sql<{ readonly value: number }>` + SELECT value FROM counters WHERE id = 1 + `; + // With a deferred BEGIN this commit lands and the write below fails + // with SQLITE_BUSY_SNAPSHOT, which no busy timeout can wait out. + yield* Effect.sync(() => + assert.throws( + () => other.exec("UPDATE counters SET value = value + 1 WHERE id = 1"), + /database is locked/, + ), + ); + yield* sql`UPDATE counters SET value = ${(row?.value ?? 0) + 10} WHERE id = 1`; + }), + ); + yield* Effect.sync(() => other.exec("UPDATE counters SET value = value + 1 WHERE id = 1")); + assert.deepEqual(yield* sql`SELECT value FROM counters WHERE id = 1`.values, [[11]]); + }).pipe(Effect.provide(SqliteClient.layer({ filename }))); + }).pipe(Effect.provide(NodeServices.layer)), +); + +it.effect("reports a transaction blocked by another writer as a lock timeout", () => + Effect.gen(function* () { + const { filename, other } = yield* makeTempDatabase; + yield* Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + yield* sql`PRAGMA busy_timeout = 0`; + const read = sql.withTransaction(sql`SELECT value FROM counters WHERE id = 1`.values); + + yield* Effect.sync(() => other.exec("BEGIN IMMEDIATE")); + const error = yield* read.pipe(Effect.flip); + assert.equal(error.reason._tag, "LockTimeoutError"); + + yield* Effect.sync(() => other.exec("COMMIT")); + assert.deepEqual(yield* read, [[0]]); + }).pipe(Effect.provide(SqliteClient.layer({ filename }))); + }).pipe(Effect.provide(NodeServices.layer)), +); + it.effect("returns a typed failure when the database cannot be opened", () => Effect.gen(function* () { const error = yield* Effect.flip( diff --git a/packages/shared/src/nodeSqliteClient.ts b/packages/shared/src/nodeSqliteClient.ts index 047de403ed8c..6f2c415e7f3b 100644 --- a/packages/shared/src/nodeSqliteClient.ts +++ b/packages/shared/src/nodeSqliteClient.ts @@ -13,6 +13,7 @@ import * as Exit from "effect/Exit"; import * as Fiber from "effect/Fiber"; import { identity } from "effect/Function"; import * as Layer from "effect/Layer"; +import * as Predicate from "effect/Predicate"; import * as Schema from "effect/Schema"; import * as Scope from "effect/Scope"; import * as Semaphore from "effect/Semaphore"; @@ -82,6 +83,22 @@ const checkNodeSqliteCompat = () => { return Effect.void; }; +/** + * `node:sqlite` reports the SQLite result code as `errcode`, while + * `classifySqliteError` reads `errno`. Copy it across so busy, locked and + * constraint failures get their own reasons instead of `UnknownError`. + */ +const classifyError = (cause: unknown, message: string, operation: string) => { + if ( + Predicate.hasProperty(cause, "errcode") && + typeof cause.errcode === "number" && + !Predicate.hasProperty(cause, "errno") + ) { + Object.assign(cause, { errno: cause.errcode }); + } + return classifySqliteError(cause, { message, operation }); +}; + const make = Effect.fn("makeWithDatabase")(function* ( options: SqliteClientConfig, ): Effect.fn.Return { @@ -102,10 +119,7 @@ const make = Effect.fn("makeWithDatabase")(function* ( }), catch: (cause) => new SqlError({ - reason: classifySqliteError(cause, { - message: "Failed to open database", - operation: "open", - }), + reason: classifyError(cause, "Failed to open database", "open"), }), }); yield* Scope.addFinalizer( @@ -114,10 +128,7 @@ const make = Effect.fn("makeWithDatabase")(function* ( try: () => db.close(), catch: (cause) => new SqlError({ - reason: classifySqliteError(cause, { - message: "Failed to close database", - operation: "close", - }), + reason: classifyError(cause, "Failed to close database", "close"), }), }).pipe(Effect.orDie), ); @@ -138,10 +149,7 @@ const make = Effect.fn("makeWithDatabase")(function* ( try: () => db.prepare(sql), catch: (cause) => new SqlError({ - reason: classifySqliteError(cause, { - message: "Failed to prepare statement", - operation: "prepare", - }), + reason: classifyError(cause, "Failed to prepare statement", "prepare"), }), }); @@ -168,10 +176,7 @@ const make = Effect.fn("makeWithDatabase")(function* ( } catch (cause) { return Effect.fail( new SqlError({ - reason: classifySqliteError(cause, { - message: "Failed to execute statement", - operation: "execute", - }), + reason: classifyError(cause, "Failed to execute statement", "execute"), }), ); } @@ -201,10 +206,7 @@ const make = Effect.fn("makeWithDatabase")(function* ( }, catch: (cause) => new SqlError({ - reason: classifySqliteError(cause, { - message: "Failed to execute statement", - operation: "execute", - }), + reason: classifyError(cause, "Failed to execute statement", "execute"), }), }), (statement) => @@ -216,10 +218,11 @@ const make = Effect.fn("makeWithDatabase")(function* ( }, catch: (cause) => new SqlError({ - reason: classifySqliteError(cause, { - message: "Failed to reset statement result mode", - operation: "resetResultMode", - }), + reason: classifyError( + cause, + "Failed to reset statement result mode", + "resetResultMode", + ), }), }).pipe(Effect.orDie), ); @@ -273,6 +276,13 @@ const make = Effect.fn("makeWithDatabase")(function* ( acquirer, compiler, transactionAcquirer, + // A deferred BEGIN only takes the write lock at the first write. If another + // process commits after this transaction's first read, that write fails at + // once with SQLITE_BUSY_SNAPSHOT, which busy_timeout cannot wait out. Taking + // the lock up front makes it wait instead, at the cost of serializing + // read-only transactions behind other processes' writers. Read-only + // connections cannot write, so they keep the deferred BEGIN. + beginTransaction: options.readonly === true ? "BEGIN" : "BEGIN IMMEDIATE", spanAttributes: [ ...(options.spanAttributes ? Object.entries(options.spanAttributes) : []), [ATTR_DB_SYSTEM_NAME, "sqlite"],