fix(worker-shell): bundle sqlite worker runtime - #144
agent-think[bot] wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: dff14a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 3 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| async terminate() { | ||
| this.#terminated = true; | ||
| return 0; |
There was a problem hiding this comment.
🔴 SQLite timeouts cannot stop queries
A CPU-bound query prevents terminate() from running until executeQuery finishes. SQLite limits and shell cancellation cannot stop it, blocking every command in the Dynamic Worker.
Learn more
The original SQLite implementation runs executeQuery in a Node worker thread. Its controller enforces maxSqliteTimeoutMs by terminating that thread. The inline adapter runs the same synchronous sql.js WebAssembly work on the Dynamic Worker's event loop. A timer, abort RPC, or terminate() call cannot execute while that work occupies the isolate. The configured query timeout therefore only takes effect after the query has already returned.
Example: A recursive query that runs for minutes starts with a five-second SQLite timeout. The five-second timer cannot run while WebAssembly executes. The shell isolate remains unavailable until the query naturally finishes instead of returning after five seconds.
Recommended fix: Execute SQLite in a separately terminable Worker-compatible isolate, or add an interruption mechanism inside SQLite that the runtime can trigger independently of the blocked event loop. Do not report successful termination unless the computation has actually stopped.
Was this helpful? React with 👍 or 👎 to provide feedback.
commit: |
just-bash keys its sqlite3 database locks on `fsIdentity ?? fs`, then on the canonical database path. ShellWorker.exec builds a fresh Bash and WorkspaceFsAdapter for every execution, and adaptSqliteCommand wraps that in a fresh Proxy, so each execution landed in its own lock bucket and two concurrent writers to the same database never contended. Because writeback replaces the whole database image, the later writer silently discarded the earlier one's rows rather than failing. Pin one module-level identity shared by every sqlite3 command in the isolate and let the canonical path keep distinguishing databases, so separate databases still run concurrently.
| source = replaceExactlyOnce( | ||
| source, | ||
| "export{$e as a,_e as b,Fe as c};", | ||
| "const __sqliteCommand=__adaptSqliteCommand(_e);export{$e as a,__sqliteCommand as b,Fe as c};", | ||
| "sqlite command export", | ||
| ); | ||
| source = | ||
| `import { adaptSqliteCommand as __adaptSqliteCommand } from ${JSON.stringify( | ||
| resolve(here, "sqlite-command-adapter.mjs"), | ||
| )};\n` + source; |
There was a problem hiding this comment.
🔍 SQLite adaptation tracks minified output
The plugin rewrites exact minified just-bash symbols while accepting caret upgrades. Routine dependency updates can break package preparation despite unchanged SQLite behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // WorkerDefenseInDepth belongs around a dedicated thread. Running it | ||
| // in the shell's isolate would harden the shell itself after a query. | ||
| source = replaceExactlyOnce( | ||
| source, | ||
| " activateDefense();\n", | ||
| "", | ||
| "worker defense activation", | ||
| ); |
There was a problem hiding this comment.
🔍 SQLite loses worker defense layer
Removing activateDefense() leaves the Dynamic Worker as SQLite's only isolation boundary. Review whether that boundary matches the upstream worker threat model.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #106.
Summary
sql-wasm.wasmas a Dynamic Worker Loader WebAssembly module and adapt sql.js to instantiate itnode:worker_threads.WorkerWorkerShellBackend@cloudflare/computer/shell/sqliteVerification
npm run test:worker-backend --workspace @cloudflare/computernpm exec vitest run --workspace packages/computer -- src/backends/worker-shell/shell-modules.test.ts src/backends/worker-shell/script/partition.test.tsnpm run typecheck --workspace @cloudflare/computernpm run build --workspace @cloudflare/computernpm pack --ignore-scripts --workspace @cloudflare/computer(published artifact contains the SQLite query worker and Wasm module)The generated core group is byte-for-byte unchanged with and without this fix: 1,882,415 generated-file bytes, 1,793,181 module-source bytes, 179 modules, and a 623,950-byte
shell.js. Projects that do not import the SQLite subpath therefore do not take a bundle-size increase.