Fix SQLite client locking defaults - #7162
Conversation
🦋 Changeset detectedLatest commit: e4eda0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one rough edge worth documenting.
Reviewed changes — reviewed PR #7162, which hard-codes a 5-second SQLite busy timeout and BEGIN IMMEDIATE transaction start into the Node and Bun SQLite clients, adds client-level regression tests for both defaults, and ships a patch changeset.
- busy timeout default —
PRAGMA busy_timeout = 5000runs at connection setup in both clients, before WAL mode is enabled. - immediate transactions — both clients now pass
beginTransaction: "BEGIN IMMEDIATE"to the sharedSqlClient.make, sowithTransactionacquires the write lock at transaction start, avoiding deferred-snapshot upgrade failures. - regression tests — "uses a 5 second busy timeout" and "starts transactions immediately" in each client; the latter contends two clients on one file using a 1ms busy timeout to fail fast.
- changeset — patch bump for both packages.
Validated on this checkout: the full @effect/sql-sqlite-node suite (59 tests) passes, and the "starts transactions immediately" test fails when BEGIN IMMEDIATE is reverted, confirming it genuinely pins the new behavior. I also probed node:sqlite directly: BEGIN IMMEDIATE succeeds on read-only connections (no readonly regression) and contended lock acquisition honors busy_timeout.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-reviewed #7162 against the delta of the prior Pullfrog review (3a59c29 → 701d95f, "Address SQLite client review"):
- configurable
busyTimeout— bothSqliteClientConfigs gain abusyTimeout?: Duration.Inputoption, applied at connection setup viaPRAGMA busy_timeout = ${Duration.toMillis(...)}; defaults stay at 5 seconds.Duration.toMillisacceptsDuration.Input, so string/number durations type-check and run correctly. - documented trade-offs — module and
make()JSDoc in both clients now state that busy waits block the event loop (sync drivers) and thatBEGIN IMMEDIATEmakes read-only transactions serialize behind other writers; the prior review's request to call this out explicitly is addressed, and the concurrency consequence now has an escape hatch. - regression tests — the "uses a 5 second busy timeout" tests now also pin the override (
busyTimeout: "1 second"→[{ timeout: 1000 }], which would fail if the knob weren't wired), a new node "supports transactions on readonly clients" test guards the readonly +BEGIN IMMEDIATEinteraction, and the bun readonly test gained awithTransactionread. - changeset — description expanded to reflect the configurable timeout and the event-loop/serialization trade-offs.
Validated on this checkout: the full @effect/sql-sqlite-node Client suite (8 tests) passes, including the new override and readonly-transaction tests. The prior feedback thread is resolved.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-reviewed #7162 against the delta since the prior approved Pullfrog review (701d95f → e4eda0b, "Handle infinite SQLite busy timeouts"):
- clamped busy timeout — both Node and Bun clients now clamp
busyTimeoutviaMath.min(MAX_BUSY_TIMEOUT, Math.max(0, Math.round(Duration.toMillis(...)))), mappingDuration.infinity, negative, and oversized durations to SQLite's signed-32-bit maximum2_147_483_647. - JSDoc — both
SqliteClientConfigdocs note thatDuration.infinityis clamped to SQLite's maximum timeout. - regression tests — the node and bun "uses a 5 second busy timeout" tests now also assert
busyTimeout: Duration.infinity→[{ timeout: 2_147_483_647 }].
Validated on this checkout: the @effect/sql-sqlite-node Client suite (8 tests) passes. I also probed node:sqlite directly: without the clamp, PRAGMA busy_timeout = Infinity silently parses to 0 (no waiting at all), while 2147483647 round-trips — so the new test genuinely pins the clamp and the change fixes a real silent-misconfiguration edge case for Duration.infinity.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1 participant
Summary
BEGIN IMMEDIATEto avoid snapshot upgrade failuresWhy
Concurrent workflow runner and control-plane clients can contend for the same SQLite store. A zero busy timeout fails ordinary lock contention immediately, while deferred transactions can fail when upgrading a read snapshot to a writer. Waiting for locks and acquiring the write lock at transaction start avoids both failure modes.
Because both SQLite drivers are synchronous, busy waits block the event loop. The configurable timeout lets applications choose a shorter wait, while the 5-second default preserves safe out-of-the-box behavior. Immediate transactions on writable connections serialize behind other writers even when they only read; clients opened with
readonly: trueare unaffected.Validation
pnpm vitest run --project @effect/sql-sqlite-node(60 tests)bun node_modules/vitest/vitest.mjs run --project @effect/sql-sqlite-bun(4 tests)@effect/sql-sqlite-nodeand@effect/sql-sqlite-bunpnpm lintpnpm changeset statusCloses EFF-577
Closes #6179