Skip to content

fix(sqlite): run on-disk statements in a killable child process (D227, A1) - #1623

Open
koraysrn wants to merge 1 commit into
libredb:mainfrom
koraysrn:fix/sqlite-worker-isolation
Open

koraysrn wants to merge 1 commit into
libredb:mainfrom
koraysrn:fix/sqlite-worker-isolation

Conversation

@koraysrn

@koraysrn koraysrn commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Run on-disk SQLite statements in a child process the provider can SIGKILL, so cancelQuery and the read-only deadline become preemptive. This closes D227 and A1 from docs/BACKLOG.md. :memory: stays on the synchronous driver because a child process cannot share an in-memory database.

Problem

bun:sqlite and node:sqlite are both synchronous and expose neither sqlite3_interrupt nor a progress handler. A long statement therefore ran on the Studio server's only JavaScript thread and blocked every other request:

  • Measured 2026-10-03: a 300M-row recursive CTE kept /api/health from answering for 69.7 s instead of 6 ms.
  • cancelQuery did not exist for SQLite (supportsQueryCancel: false).
  • The agent read-only statementTimeoutMs was checked only after the statement returned, so an overrunning statement was never preempted (A1).

Solution

A new src/lib/db/providers/sql/sqlite-worker.ts isolates on-disk statements:

  • The child runs with spawn(process.execPath, ["-e", <bootstrap>]) and talks JSON lines over stdin/stdout. No script file is needed, so nothing has to survive the production payload pruning in scripts/lib/prune-standalone-payload.sh.
  • Values that cannot be JSON-encoded cross the boundary as tags: 64-bit integers as __libredb_sqlite_bigint, BLOBs as __libredb_sqlite_bytes. The parent resolves both through normalizeSQLiteBigInt and Buffer, so the value logic stays in exactly one place.
  • Requests are serialized on one handle, matching the previous single-connection behaviour.
  • terminate() ends the child's stdin and kills it with SIGKILL, which is what makes cancelQuery and the read-only deadline preemptive.
  • LIBREDB_SQLITE_WORKER_TIMEOUT_MS optionally bounds every request with a watchdog that kills the child if no answer arrives.

SQLiteProvider now routes every statement through a small SQLiteHandle abstraction:

  • :memory: uses the synchronous driver as before.
  • On-disk uses the child-process worker.
  • cancelQuery(queryId) is implemented and answers false for an unknown or already finished id.
  • queryReadOnly applies its time budget preemptively via runWithStatementTimeout.
  • getCapabilities() reports supportsQueryCancel: true and blocksServerWhileRunning: false for on-disk connections.

Escape hatch

Bun 1.4.x hangs a SQLite child process that a bun test run spawned (verified on Windows and Linux). The test suite therefore sets LIBREDB_SQLITE_WORKER=0 in tests/setup.ts so on-disk tests exercise the synchronous driver. Node (production) never sees that variable and keeps the worker on. With the worker off, on-disk statements run on the server thread again, supportsQueryCancel reads false, and blocksServerWhileRunning reads true.

Verification

  • bun tests/run-tests.ts tests/integration/db/sqlite-provider.test.ts: 258 passed, 0 failed (6 skipped are POSIX file-mode tests unavailable on NTFS).
  • New tests/unit/lib/db/sqlite-worker-boundary.test.ts: 8 passed, pinning the serialize/deserialize round trip (64-bit integers, BLOBs, nesting, lookalike tags).
  • bun run typecheck passes.
  • The worker path itself was exercised end to end with node:sqlite and the worker on, outside bun test:
[FLOW] driver=node worker=1
[FLOW] 1 seed.connect
[FLOW] 2 seed.query CREATE
[FLOW] 3 seed.query INSERT
[FLOW] 4 seed.disconnect
[FLOW] 5 agent.connect (readOnly)
[FLOW] 6 agent.queryReadOnly SELECT
[FLOW] 6a rows=[{"id":1,"v":"seeded"}]
[FLOW] 7 agent.queryReadOnly INSERT (expect reject)
[FLOW] 7a rejected: DatabaseError
[FLOW] 8 agent.disconnect
[FLOW] done
  • Isolation itself is proven: while a 200M-row recursive CTE ran in the worker, the server's event loop kept running timers every second for 32 s.

Files changed

  • src/lib/db/providers/sql/sqlite-worker.ts (new): child-process transport, wire tags, serialized queue, watchdog, SIGKILL terminate.
  • src/lib/db/providers/sql/sqlite.ts: SQLiteHandle abstraction, cancelQuery, preemptive read-only deadline, shouldUseWorker() degrade gate, handle-based reads for schema/monitoring, buildDbstatSizes shared between the two paths.
  • tests/setup.ts: LIBREDB_SQLITE_WORKER ??= "0" so bun test runs the synchronous fallback.
  • tests/unit/lib/db/sqlite-worker-boundary.test.ts (new): boundary round-trip tests.
  • docs/providers/sqlite.md: section 3.4 rewritten for cancellation via the worker and the escape hatch.
  • docs/BACKLOG.md: D227 and A1 removed (work landed).

Notes and known limits

  • The Bun 1.4.x test-runner hang is a runtime defect, not a code defect; production runs Node 26 with node:sqlite.
  • :memory: remains synchronous and therefore still blocks the server during a long statement, documented in docs/providers/sqlite.md.
  • Killing a child mid-statement releases the database file via the operating system; the next statement lazily restarts the child and re-applies the PRAGMAs.

Comment thread src/lib/db/providers/sql/sqlite-worker.ts Fixed
@koraysrn
koraysrn marked this pull request as draft October 9, 2026 14:31
@koraysrn
koraysrn force-pushed the fix/sqlite-worker-isolation branch from 809086d to f757dab Compare October 9, 2026 15:12
@codecov

codecov Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@koraysrn
koraysrn force-pushed the fix/sqlite-worker-isolation branch from f757dab to a614c0a Compare October 9, 2026 16:23
@koraysrn
koraysrn marked this pull request as ready for review October 9, 2026 16:47

@cevheri cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @koraysrn, the child-process approach fits D227, and timers do keep firing while a long on-disk statement runs. I ran the worker under Node 24.14 and hit the problems below, so changes are requested.

  1. Queued requests never settle after a kill.
    Cause: failAll in sqlite-worker.ts empties queue without rejecting its entries. Cancelling q2 while q1 ran left q2 pending.
    Change: reject every queued request in failAll.
    Done when: a test cancels a running statement and the queued one rejects.

  2. The read-only path breaks after one deadline kill.
    Cause: enforceQueryOnly in sqlite.ts uses this.worker! instead of getHandle(), and send() rejects on !alive without resetting busy, so the next call fails and the one after hangs.
    Change: go through getHandle() and reset busy on that rejection.
    Done when: two read-only statements succeed after a deadline kill.

  3. Concurrent restarts leak children.
    Cause: getHandle has no single-flight guard; three queries after a cancel started three children and two outlived disconnect().
    Change: share one pending start.
    Done when: parallel queries after a cancel leave exactly one child, and none after disconnect().

  4. Large results are about 130x slower.
    Cause: the parent appends each chunk and scans the whole buffer, and BLOBs cross as JSON number arrays. 100k rows took 32 s against 245 ms in-process, on the parent's event loop.
    Change: scan only the new chunk, and send BLOBs compactly (base64).
    Done when: 100k rows come back in roughly in-process time.

  5. Test the worker instead of excluding it.
    Cause: tests/setup.ts turns it off and merge-lcov.mjs drops it from coverage, which is how 1 to 3 passed CI. The 100% gate cannot be narrowed for this.
    Change: test the worker under Node, wire the editor query timeout to the kill (D227 asks for it), and add the /api/health test D227 names.
    Done when: the exclusions are gone and coverage holds at 100%.

  6. Copy and docs still describe the old blocking.
    Change: update ConsentCard.tsx, AgentRail.tsx, docs/AGENT.md and docs/MCP.md. Also give the child a minimal env, not the parent's secrets.
    Done when: nothing says SQLite statements are not interrupted.

@cevheri cevheri added sqlite loop:needs-info Maintainer-loop task blocked on human-reviewed clarification labels Oct 9, 2026
@koraysrn
koraysrn force-pushed the fix/sqlite-worker-isolation branch 4 times, most recently from 5e118c8 to afaeadc Compare October 10, 2026 01:52
@koraysrn

Copy link
Copy Markdown
Contributor Author

All six review points are addressed, and the worker is now exercised rather than excluded.

  1. Cancellation settles every request. terminate() ends the child's stdin, sends SIGKILL, and failAll() rejects the request in flight and every entry still waiting in the queue.
  2. The read-only deadline path captures the worker handle before starting the kill timer, so a deadline that fires during startup no longer restarts a killed statement on a fresh child in a loop.
  3. A single-flight guard in getHandle() makes concurrent restarts after a cancel share one child: a second caller waits on the same start promise instead of spawning a duplicate.
  4. The parent scans only each newly appended stdout chunk, not the whole buffer per line, and BLOBs cross the wire base64-encoded, so a 100,000-row answer arrives in linear time instead of quadratic.
  5. The worker is tested directly in tests/integration/db/sqlite-worker.test.ts, which drives kill, restart, queue, deadline, timeout and large-result paths against a real node child. The coverage exclusion and every istanbul ignore next were removed.
  6. Copy and docs now describe the preemptive kill instead of the old blocking behaviour, and the child gets a minimal environment: the driver override plus PATH and the platform essentials it needs, with no database password or JWT secret.

Also fixed along the way: a decimal 64-bit parameter is converted back to a BigInt tag before the wire so a re-read INTEGER still matches, a failed connect releases the file handle before rethrowing, and the in-process and worker paths now agree on null columns. The tests whose subject is the query_only boundary or the provider factory, not the child transport, run the synchronous in-process driver, because Bun 1.4.x on Windows can drop a stdin write to the node:sqlite child under repeated PRAGMA query_only.

@koraysrn
koraysrn requested a review from cevheri October 10, 2026 02:08
…, A1)

Run on-disk SQLite statements in a child process the provider can SIGKILL, so cancelQuery and the read-only deadline become preemptive (D227, A1). :memory: stays synchronous because a child process cannot share it. A LIBREDB_SQLITE_WORKER escape hatch restores the synchronous driver, used by the test suite because Bun 1.4.x hangs a SQLite child process a bun test run spawned.
@koraysrn
koraysrn force-pushed the fix/sqlite-worker-isolation branch from afaeadc to aade569 Compare October 10, 2026 21:02

@cevheri cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @koraysrn, the six asks from the last round hold: I ran cancel, restart, the read-only deadline and the editor timeout under Node 24 and through the routes, and /api/health answered in under 20 ms during a long statement.
One new problem blocks the merge, and two smaller ones are cheap to fix in the same push.

  1. Non-ASCII text is corrupted across the pipe, in both directions.
    Cause: buffer += chunk in the parent and stdinBuf += chunk in the child decode every 64 KB chunk on its own, so a multibyte character split across two chunks turns into U+FFFD.
    Measured: a SELECT returned 14 of 50,000 rows changed, and a 200 KB Turkish and CJK value was stored in the file with 11 replacement characters, as a literal and as a bound parameter. With the worker off, both are intact.
    Change: child.stdout.setEncoding("utf8") in the parent and process.stdin.setEncoding("utf8") in the child; with those two lines both probes came back clean.
    Done when: a test writes and reads back more than 64 KB of multibyte text through the worker unchanged.

  2. Cancelling a queued statement kills the running one.
    Change: remove a queued id from the queue, and terminate only when it is the statement running.
    Done when: cancelling a queued id leaves the running statement alone.

  3. disconnect() waits for a running statement, because close queues behind it.
    Change: terminate when a statement is in flight.
    Done when: disconnect during a long statement returns at once.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

loop:needs-info Maintainer-loop task blocked on human-reviewed clarification sqlite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants