fix(worker-shell): bundle sqlite worker runtime - #144
Conversation
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| execute(args, context) { | ||
| return command.execute(args, { | ||
| ...context, | ||
| fs: filesystemWithStableIdentity(context.fs), | ||
| }); |
There was a problem hiding this comment.
🔴 Concurrent SQLite writes lose updates
adaptSqliteCommand preserves each Bash instance's fsIdentity, so concurrent executions acquire separate lock maps. They can read the same snapshot and overwrite one another's changes.
Learn more
just-bash stores SQLite locks in a WeakMap keyed first by context.fsIdentity, then by database identity. ShellWorker.exec creates a fresh Bash and filesystem adapter for every execution. Spreading context preserves that fresh identity even though this wrapper replaces context.fs. Two concurrent executions therefore never see each other's lock for the same database. Both can read the old bytes, execute independently, and write complete replacement database images.
Example: Executions A and B open /workspace/data.db together. Both read a database containing one row. A inserts alice, B inserts bob, and both export their image. If B writes last, alice disappears.
Recommended fix: Give every SQLite command in the Dynamic Worker a shared fsIdentity object while retaining canonical database paths as the second-level keys. Add a concurrent integration test that forces two executions to overlap and verifies both committed changes remain.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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.
| if (stat.identity !== undefined || (stat.dev !== undefined && stat.ino !== undefined)) { | ||
| return stat; | ||
| } | ||
| return { ...stat, identity: await target.realpath(path) }; |
There was a problem hiding this comment.
🔴 Symlinked databases bypass locks
filesystemWithStableIdentity uses an unresolved symlink path because realpath only normalizes paths. Concurrent writes through the symlink and target use different locks and can overwrite changes.
Learn more
SQLite adds the filesystem's stable identity to its database lock keys. This wrapper synthesizes that identity from realpath, assuming aliases converge to one canonical path. WorkspaceFsAdapter.realpath validates the path with stat but returns only normalizePath(path). A symlink and its target therefore remain distinct identities even when they reference the same file. Concurrent commands can then read and replace the same database without sharing a lock.
Example: /workspace/current.db is a symlink to /workspace/data.db. One command inserts through current.db while another inserts through data.db. Each obtains a different lock, and the later full-database write can erase the earlier insert.
Recommended fix: Resolve symlinks to their final canonical target before synthesizing identity, including relative and chained links. Add a concurrency test that opens one database through both its target path and a symlink.
Was this helpful? React with 👍 or 👎 to provide feedback.
commit: |
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.