Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I copied the new test into the base checkout. The lock-holder case fails there with database is locked and passes with this change, so the main #47566 path is covered: the durable event writes' BEGIN IMMEDIATE runs outside a transaction and gets retried. The packages/core typecheck and the database and bus tests pass.
Public API: unchanged (internal to the core SQLite drivers).
One thing should be fixed before merging. The retry check reads the transaction state of the shared connection, not of the fiber that owns the statement. Between attempts the statement doesn't hold the client semaphore, so it can run inside another fiber's transaction. I reproduced a retried autocommit insert that reports success but is rolled back with another fiber's failed transaction. Details are inline.
| <A, E>(effect: Effect.Effect<A, E, never>) => | ||
| effect.pipe( | ||
| Effect.retry({ | ||
| while: (error) => isSqlError(error) && error.reason._tag === "LockTimeoutError" && !inTransaction(), |
There was a problem hiding this comment.
inTransaction() reads the state of the shared native connection, not of the fiber that issued the statement. acquirer lets go of the semaphore right after handing out the connection, so while this statement sleeps between attempts, another fiber can take transactionAcquirer and run BEGIN on the same connection. The next attempt then runs inside that fiber's transaction.
I checked this with a throwaway test. Another connection holds BEGIN IMMEDIATE, fiber A forks insert … (busy_timeout 0), and 20 ms later fiber B runs a deferred db.transaction that reads, sleeps and fails:
- If the lock is freed at 30 ms, A's insert reports Success, but it ran inside B's transaction and was rolled back with it. The table ends up empty.
- If the lock is freed at 100 ms, A gives up with
LockTimeoutError, because this check now sees B's transaction.
One fix is to retry at the client level, so each attempt goes through acquirer (the semaphore) again. Another is to decide "in a transaction" from the fiber's own transaction context instead of the shared connection.
|
Pre-landing gate run on head db-lock-retry-v2 against dev (contributor-side; leaving review/merge to maintainers). Verdict: ready. No blocking findings. Verified: retry semantics against effect 4.0.1 internals and both runtimes (bun inTransaction and node isTransaction checked live); LockTimeoutError classification covers BUSY/LOCKED plus extended codes (517 BUSY_SNAPSHOT, 262 LOCKED_SHAREDCACHE) with the in-transaction variants correctly excluded; WAL-mode BUSY occurs at lock acquisition, so a retried statement cannot double-apply a non-idempotent write; loop bounded, no recursion; bun and node implementations behaviorally identical. Tests: new sqlite-lock-retry tests 2/2 pass; existing database suite 64/64 pass across 6 files; typecheck clean. Notes for the thread:
|
github-actions
Bot
added
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
When multiple opencode processes share one database, statements that cannot grab the SQLite write lock within busy_timeout fail with database is locked and kill the running prompt. The statement only needs the lock, so retry autocommit statements classified as LockTimeoutError (SQLITE_BUSY/SQLITE_LOCKED) with jittered exponential backoff (50 ms doubling to a 250 ms cap, 30 s total) in the bun and node sqlite drivers. The async sleep between attempts lets the competing process finish its write. Only autocommit statements are retried. Inside a transaction the same error repeats until the transaction is rolled back and restarted (SQLITE_BUSY_SNAPSHOT on a deferred transaction whose read snapshot went stale), so it surfaces at once for the caller to handle. Ports #47567 to the V2 tree. The in-transaction fail-fast and its test originate from iceteaSA's work on that PR.
…etry paths
Review of the V2 port found three gaps:
- node:sqlite reports SQLITE_BUSY as { code: "ERR_SQLITE_ERROR", errcode: 5 }
with no errno and no SQLITE_* string code, which is all
classifySqliteError reads, so the lock timeout filed as UnknownError and
the retry never fired on the node driver. node's errcode is now copied
onto errno before classification, which the classifier maps to
LockTimeoutError (verified red against the old code, green after).
- The retry policy is now named constants in one place, with
Schedule.upTo replacing the hand-elapsed check.
- Tests cover the node retry path both ways (retry and the in-transaction
fail-fast case), the runValues wiring, and a non-lock error surfacing
fast, so dropping the pipe or widening the predicate breaks the suite.
A retry that only repeats the statement sleeps without holding the
semaphore, so a transaction another fiber opens while it sleeps swallows
the next attempt: its writes roll back with that transaction, or the
in-transaction check, which reads the shared connection, sees the
foreign transaction and gives up. Plain attempts now re-enter the
semaphore before executing. The running transaction's own statements
fail fast on BUSY_SNAPSHOT, and its BEGIN/COMMIT/ROLLBACK retry in
place while it holds the semaphore. The gate lives in makeClient, so
the drivers no longer carry a retryLocked pipe keyed on shared
connection state.
Also fixes the failures tsgo reports on the previous head:
Schedule.upTo takes an options object, the node busy-error guard
indexed an un-narrowed {}, the test file had a trailing comma inside an
as cast plus an unused import, and DatabaseSync was used as a type.
aae3d3c to
a121b9d
Compare
|
The |
Issue for this PR
Closes #47566
Type of change
What does this PR do?
Port of #47567 to the dev line: statements that fail with SQLITE_BUSY after busy_timeout retry with jittered backoff instead of killing an unrelated prompt. Inside a transaction the error still surfaces at once (BUSY_SNAPSHOT repeats until rollback), and a BEGIN that loses the race still retries, since the connection is not in a transaction until BEGIN succeeds.
The review on the first port caught a hole: an attempt that only repeats the statement sleeps without holding the client semaphore, so a transaction another fiber opens while it sleeps swallows the next attempt. Its writes roll back with that transaction, or the in-transaction check, which reads the shared connection, sees the foreign transaction and gives up. Retry now re-enters the semaphore per attempt, so it waits out transactions that opened meanwhile. The running transaction's own statements fail fast, and its BEGIN/COMMIT/ROLLBACK retry in place while it holds the semaphore.
The port also fixed node:sqlite reporting busy as
ERR_SQLITE_ERRORwith no errno, which the classifier reads, so lock timeouts filed as UnknownError and the retry never fired on that driver.How did you verify your code works?
In packages/core:
bun run typecheckpasses andbun test test/sqlite-lock-retry.test.tsruns 7 tests, all green. The new one fails on the previous head: a plain statement retrying while another fiber opens a transaction on the same client must wait that transaction out instead of joining it or giving up.Screenshots / recordings
Not a UI change.
Checklist