Skip to content

Commit 4291616

Browse files
iceteaSAwoodsonl
iceteaSA
authored and
woodsonl
committed
fix(core): retry sqlite lock timeouts only outside transactions
#47567's retryLocked wraps every LockTimeoutError, but a held DEFERRED WAL transaction whose read snapshot has been invalidated by another connection's commit returns SQLITE_BUSY_SNAPSHOT on the next write — retrying the same statement will keep failing the same way until a rollback + restart of the whole transaction, so the 30 s budget only delays the inevitable. Only retry in autocommit; inside an explicit transaction surface the error immediately so the caller can decide. Bun's bun:sqlite Database exposes inTransaction; node:sqlite's DatabaseSync exposes isTransaction. Each driver captures its native handle in the make closure and consults the right property in the retry's `while` predicate.
1 parent 57f1bda commit 4291616

4 files changed

Lines changed: 98 additions & 15 deletions

File tree

‎packages/core/src/database/sqlite.bun.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ const make = (options: Config) =>
5353
? Statement.defaultTransforms(options.transformResultNames).array
5454
: undefined
5555

56+
const retryIfAutocommit = retryLocked(() => native.inTransaction)
5657
const run = (query: string, params: ReadonlyArray<unknown> = []) =>
5758
Effect.withFiber<Array<Record<string, unknown>>, SqlError>((fiber) => {
5859
const statement = native.query(query)
@@ -67,7 +68,7 @@ const make = (options: Config) =>
6768
}),
6869
)
6970
}
70-
}).pipe(retryLocked)
71+
}).pipe(retryIfAutocommit)
7172

7273
const runValues = (query: string, params: ReadonlyArray<unknown> = []) =>
7374
Effect.withFiber<Array<unknown[]>, SqlError>((fiber) => {
@@ -83,7 +84,7 @@ const make = (options: Config) =>
8384
}),
8485
)
8586
}
86-
}).pipe(retryLocked)
87+
}).pipe(retryIfAutocommit)
8788

8889
const connection = identity<SqliteConnection>({
8990
execute(query, params, transformRows) {

‎packages/core/src/database/sqlite.node.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ const make = (options: Config) =>
5353
? Statement.defaultTransforms(options.transformResultNames).array
5454
: undefined
5555

56+
const retryIfAutocommit = retryLocked(() => native.isTransaction)
5657
const run = (query: string, params: ReadonlyArray<unknown> = []) =>
5758
Effect.withFiber<Array<Record<string, unknown>>, SqlError>((fiber) => {
5859
const statement = native.prepare(query)
@@ -66,7 +67,7 @@ const make = (options: Config) =>
6667
}),
6768
)
6869
}
69-
}).pipe(retryLocked)
70+
}).pipe(retryIfAutocommit)
7071

7172
const runValues = (query: string, params: ReadonlyArray<unknown> = []) =>
7273
Effect.withFiber<ReadonlyArray<ReadonlyArray<unknown>>, SqlError>((fiber) => {
@@ -84,7 +85,7 @@ const make = (options: Config) =>
8485
}),
8586
)
8687
}
87-
}).pipe(retryLocked)
88+
}).pipe(retryIfAutocommit)
8889

8990
const connection = identity<SqliteConnection>({
9091
execute(query, params, transformRows) {

‎packages/core/src/database/sqlite.ts‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,19 @@ export class Drizzle extends Context.Service<Drizzle, DrizzleClient>()("@opencod
1313
// statement itself is fine — retrying with an async sleep lets the competing
1414
// process finish so the next attempt can acquire the lock. Without this, one
1515
// long write in a concurrent process kills another process's prompt.
16-
export const retryLocked = <A, E>(effect: Effect.Effect<A, E, never>) =>
17-
effect.pipe(
18-
Effect.retry({
19-
while: (error) => error instanceof SqlError && error.reason._tag === "LockTimeoutError",
20-
schedule: Schedule.exponential(50, 2).pipe(
21-
Schedule.either(Schedule.spaced(250)),
22-
Schedule.jittered,
23-
Schedule.while((meta) => meta.elapsed < 30_000),
24-
),
25-
}),
26-
)
16+
// Only autocommit statements are retried. Inside a transaction the same error repeats
17+
// until the transaction is rolled back and restarted (e.g. SQLITE_BUSY_SNAPSHOT on a
18+
// deferred transaction whose read snapshot went stale), so it is surfaced at once.
19+
export const retryLocked =
20+
(inTransaction: () => boolean) =>
21+
<A, E>(effect: Effect.Effect<A, E, never>) =>
22+
effect.pipe(
23+
Effect.retry({
24+
while: (error) => error instanceof SqlError && error.reason._tag === "LockTimeoutError" && !inTransaction(),
25+
schedule: Schedule.exponential(50, 2).pipe(
26+
Schedule.either(Schedule.spaced(250)),
27+
Schedule.jittered,
28+
Schedule.while((meta) => meta.elapsed < 30_000),
29+
),
30+
}),
31+
)

‎packages/core/test/sqlite-lock-retry.test.ts‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,3 +44,79 @@ test("retries statements while another process holds the write lock", async () =
4444
await rm(dir, { recursive: true, force: true }).catch(() => {})
4545
}
4646
})
47+
48+
// Regression: a DEFERRED WAL transaction that holds a read snapshot cannot be
49+
// upgraded to a write after another connection commits — the snapshot is
50+
// stale and the upgrade fails with SQLITE_BUSY_SNAPSHOT. #47567's retry
51+
// wrapper, applied indiscriminately, would loop this error for the full
52+
// 30 s budget; only a rollback + restart of the whole transaction helps.
53+
// Inside an explicit transaction, statements must fail fast.
54+
test("fails fast on BUSY_SNAPSHOT inside a deferred transaction", async () => {
55+
const dir = await mkdtemp(join(tmpdir(), "sqlite-snapshot-"))
56+
const filename = join(dir, "snap.db")
57+
58+
try {
59+
// Set up schema and one row, enable WAL, all via the layer.
60+
await Effect.runPromise(
61+
Effect.gen(function* () {
62+
const db = yield* EffectDrizzleSqlite.makeWithDefaults()
63+
const native = (yield* Sqlite.Native) as Database
64+
native.run("create table t (id integer primary key, v text)")
65+
native.run("insert into t values (1, 'before')")
66+
native.run("PRAGMA journal_mode = WAL")
67+
native.run("PRAGMA busy_timeout = 0")
68+
}).pipe(Effect.provide(sqliteLayer({ filename })), Effect.scoped),
69+
)
70+
71+
// Inside a deferred transaction: read, then have another connection write
72+
// and commit (invalidating the read snapshot), then attempt a write.
73+
// The write must surface a LockTimeoutError immediately, not loop the
74+
// 30 s retry budget.
75+
const start = Date.now()
76+
let error: unknown
77+
try {
78+
await Effect.runPromise(
79+
Effect.gen(function* () {
80+
const db = yield* EffectDrizzleSqlite.makeWithDefaults()
81+
yield* db.transaction(
82+
(tx) =>
83+
Effect.gen(function* () {
84+
yield* tx.get(sql`select v from t where id = 1`)
85+
// A separate connection writes and commits while we hold the
86+
// deferred transaction's read snapshot.
87+
yield* Effect.sync(() => {
88+
const other = new Database(filename)
89+
other.run("update t set v = 'after' where id = 1")
90+
other.close()
91+
})
92+
// This write must fail with BUSY_SNAPSHOT and surface fast —
93+
// the retry helper must NOT keep trying the same stale snapshot.
94+
yield* tx.run(sql`update t set v = 'after2' where id = 1`)
95+
}),
96+
{ behavior: "deferred" },
97+
)
98+
}).pipe(Effect.provide(sqliteLayer({ filename })), Effect.scoped),
99+
)
100+
} catch (e) {
101+
error = e
102+
}
103+
const elapsed = Date.now() - start
104+
105+
expect(error).toBeDefined()
106+
// drizzle wraps the raw SqlError inside EffectDrizzleQueryError.cause.
107+
// Effect wraps the SqlError in a Cause. Walk the failure chain to confirm
108+
// the surface error is a LockTimeoutError wrapping SQLITE_BUSY_SNAPSHOT.
109+
const reasons =
110+
(
111+
error as {
112+
cause?: { reasons?: ReadonlyArray<{ error?: { reason?: { _tag?: string; cause?: { code?: string } } } }> }
113+
}
114+
).cause?.reasons ?? []
115+
const inner = reasons[0]?.error
116+
expect(inner?.reason?._tag).toBe("LockTimeoutError")
117+
expect((inner?.reason?.cause as { code?: string } | undefined)?.code).toBe("SQLITE_BUSY_SNAPSHOT")
118+
expect(elapsed).toBeLessThan(2000)
119+
} finally {
120+
await rm(dir, { recursive: true, force: true }).catch(() => {})
121+
}
122+
})

0 commit comments

Comments
 (0)