From dc576abb135da587247fe08be985710d4d628323 Mon Sep 17 00:00:00 2001 From: Mark Wotton Date: Sat, 19 Sep 2026 06:38:08 +0000 Subject: [PATCH 1/2] fix(runtime): wait for the shared session DB lock instead of failing busy Concurrent zcode processes share ~/.zcode/cli/db/db.sqlite. The vendored runtime opens it with node:sqlite, whose default busy timeout is 0, so write contention at a turn boundary surfaces as an immediate SQLITE_BUSY ("database is locked") that fatally fails the turn (refs #162). Inject `pragma busy_timeout = 10000` at the SqliteSessionStore connection site (the only DatabaseSync construction in the bundle) through the existing runtime patch framework, so writers wait for the lock instead of dying. The migrations' own WAL-mode retry budget is unchanged. Follow-up for the upstream runtime: treat SQLITE_BUSY at the persistence layer as retryable with backoff, since a busy timeout only bounds the wait. Co-Authored-By: Claude Code --- scripts/check-runtime.ts | 4 +++ scripts/sync-runtime.ts | 37 +++++++++++++++++++++ test/sync-runtime.test.ts | 68 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/scripts/check-runtime.ts b/scripts/check-runtime.ts index 01b566a..1f2194d 100755 --- a/scripts/check-runtime.ts +++ b/scripts/check-runtime.ts @@ -13,12 +13,14 @@ import { hasRuntimeCliHelpContract, hasRuntimeHttpNoContentGuard, hasRuntimeNetworkRetryGuard, + hasRuntimeSqliteBusyTimeout, hasRuntimeStreamEofFinishGuard, patchRuntimeGoalFailurePause, patchRuntimeHttpNoContent, patchRuntimeLoginModelDefaults, patchRuntimeNetworkRetryClassification, patchRuntimeOfficialMcpAvailability, + patchRuntimeSqliteBusyTimeout, patchRuntimeStreamEofFinishGuard, parseRuntimePatchReports, runtimePatchPlan, @@ -77,6 +79,8 @@ if (patchRuntimeLoginModelDefaults(runtimeSource) !== runtimeSource || !hasRuntimeHttpNoContentGuard(runtimeSource) || patchRuntimeNetworkRetryClassification(runtimeSource) !== runtimeSource || !hasRuntimeNetworkRetryGuard(runtimeSource) + || patchRuntimeSqliteBusyTimeout(runtimeSource) !== runtimeSource + || !hasRuntimeSqliteBusyTimeout(runtimeSource) || patchRuntimeStreamEofFinishGuard(runtimeSource) !== runtimeSource || !hasRuntimeStreamEofFinishGuard(runtimeSource) || (patchEnabled("cli-help-contract") && !hasRuntimeCliHelpContract(runtimeSource)) diff --git a/scripts/sync-runtime.ts b/scripts/sync-runtime.ts index 951e09b..05a2969 100755 --- a/scripts/sync-runtime.ts +++ b/scripts/sync-runtime.ts @@ -1059,6 +1059,37 @@ export function patchRuntimeHttpNoContent(runtime: string): string { return changed ? patched : runtime; } +export const sqliteBusyTimeoutMs = 10_000; + +const runtimeSqliteBusyTimeoutPragma = `pragma busy_timeout = ${sqliteBusyTimeoutMs}`; + +/** Detect the busy-timeout pragma on the shared session-DB connection. */ +export function hasRuntimeSqliteBusyTimeout(runtime: string): boolean { + return runtime.includes(runtimeSqliteBusyTimeoutPragma); +} + +/** + * Concurrent zcode processes share ~/.zcode/cli/db/db.sqlite. The runtime opens + * it with node:sqlite, whose default busy timeout is 0: lock contention between + * writers surfaces as an immediate SQLITE_BUSY ("database is locked") that + * kills the turn instead of waiting. Set an explicit busy_timeout (longer than + * the open-site `timeout`, which only newer Node runtimes honor) so writers + * wait for the lock instead of failing the turn. + */ +export function patchRuntimeSqliteBusyTimeout(runtime: string): string { + if (hasRuntimeSqliteBusyTimeout(runtime)) return runtime; + // The session store is the only DatabaseSync construction site in the + // bundle; migrations and every session write reuse this connection. + const connectionPattern = /this\.db=new ([A-Za-z_$][\w$]*)\.DatabaseSync\(this\.dbPath(?:,\{([^{}]*)\})?\)/u; + if (!connectionPattern.test(runtime)) { + throw new Error("ZCode runtime is incompatible with the SQLite busy-timeout patch (session DB connection anchor missing)."); + } + return runtime.replace( + connectionPattern, + (_match, sqlite: string, options: string | undefined) => `this.db=new ${sqlite}.DatabaseSync(this.dbPath${options === undefined ? "" : `,{${options}}`}),this.db.exec("${runtimeSqliteBusyTimeoutPragma}")` + ); +} + function escapeRegExpName(value: string): string { return value.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"); } @@ -1530,6 +1561,12 @@ export const runtimePatchPlan: readonly RuntimePatchDefinition[] = [ apply: patchRuntimeStreamEofFinishGuard, verify: hasRuntimeStreamEofFinishGuard }, + { + id: "sqlite-busy-timeout", + requirement: "required", + apply: patchRuntimeSqliteBusyTimeout, + verify: hasRuntimeSqliteBusyTimeout + }, { id: "oauth-http-errors", requirement: "optional", diff --git a/test/sync-runtime.test.ts b/test/sync-runtime.test.ts index f0ec480..5374076 100644 --- a/test/sync-runtime.test.ts +++ b/test/sync-runtime.test.ts @@ -12,6 +12,7 @@ import { hasRuntimeCliHelpContract, hasRuntimeHttpNoContentGuard, hasRuntimeNetworkRetryGuard, + hasRuntimeSqliteBusyTimeout, hasRuntimeStreamEofFinishGuard, installRuntimeProviderConfig, manifestUrl, @@ -26,6 +27,7 @@ import { patchRuntimeLoginModelDefaults, patchRuntimeNetworkRetryClassification, patchRuntimeOAuthHttpErrors, + patchRuntimeSqliteBusyTimeout, patchRuntimeStreamEofFinishGuard, patchRuntimeTerminalToolProjection, patchRuntimeTuiBridge, @@ -36,6 +38,7 @@ import { selectRuntimeLock, serviceManifestUrl, serviceReleasePlatform, + sqliteBusyTimeoutMs, supportsMultiMessageFileRewind, writeRuntimeCompatibilityFailure } from "../scripts/sync-runtime.ts"; @@ -299,6 +302,71 @@ describe("runtime synchronization", () => { expect(hasRuntimeHttpNoContentGuard("runtime without the HTTP wrapper")).toBe(false); }); + test("waits for the shared session DB lock instead of failing with SQLITE_BUSY", async () => { + const runtime = [ + 'class Store{constructor(t={}){this.dbPath=t.dbPath??"~/.zcode/cli/db/db.sqlite";let r=t.startupLockTimeoutMs??5e3;', + 'try{this.db=new FNt.DatabaseSync(this.dbPath,{timeout:r})}catch(n){throw new Error(`Failed to open SQLite session database at ${this.dbPath}`)}', + 'this.db.exec("pragma foreign_keys = on")}}' + ].join(""); + const patched = patchRuntimeSqliteBusyTimeout(runtime); + expect(hasRuntimeSqliteBusyTimeout(runtime)).toBe(false); + expect(hasRuntimeSqliteBusyTimeout(patched)).toBe(true); + const opened: { options: unknown; path: string }[] = []; + const statements: string[] = []; + const FNt = { + DatabaseSync: function DatabaseSync(this: { exec: (sql: string) => void }, path: string, options: unknown) { + opened.push({ options, path }); + this.exec = (sql: string) => statements.push(sql); + } + }; + const Store = new Function("FNt", `${patched};return Store;`)(FNt) as new (options?: { + dbPath?: string; + startupLockTimeoutMs?: number; + }) => { db: { exec: (sql: string) => void } }; + new Store({ startupLockTimeoutMs: 2500 }); + // The pragma must land on the connection right after open, before any + // other statement, regardless of the open-site timeout option. + expect(opened).toEqual([{ options: { timeout: 2500 }, path: "~/.zcode/cli/db/db.sqlite" }]); + expect(statements).toEqual([`pragma busy_timeout = ${sqliteBusyTimeoutMs}`, "pragma foreign_keys = on"]); + expect(patchRuntimeSqliteBusyTimeout(patched)).toBe(patched); + expect(() => patchRuntimeSqliteBusyTimeout("incompatible runtime")).toThrow(/connection anchor/); + + // Without a busy timeout, node:sqlite's default is 0: a second writer on a + // WAL database fails immediately with ERR_SQLITE_ERROR ("database is + // locked"), the fatal turn.failed cause from the field reports. With the + // pragma the same open blocks for the configured window instead. + const { DatabaseSync } = require("node:sqlite") as { + DatabaseSync: new (path: string, options?: { timeout?: number }) => { + exec: (sql: string) => void; + close: () => void; + }; + }; + const dbPath = join(tmpdir(), `zcode-busy-${process.pid}-${Date.now()}.sqlite`); + const holder = new DatabaseSync(dbPath); + holder.exec("create table t(x)"); + holder.exec("pragma journal_mode = wal"); + holder.exec("begin immediate"); + try { + const immediate = new DatabaseSync(dbPath); + let startedAt = Date.now(); + expect(() => immediate.exec("begin immediate")).toThrow("database is locked"); + expect(Date.now() - startedAt).toBeLessThan(500); + immediate.close(); + + const waiting = new DatabaseSync(dbPath); + waiting.exec("pragma busy_timeout = 250"); + startedAt = Date.now(); + expect(() => waiting.exec("begin immediate")).toThrow("database is locked"); + // SQLITE_BUSY only surfaces once the full busy window has elapsed. + expect(Date.now() - startedAt).toBeGreaterThan(150); + waiting.close(); + } finally { + holder.exec("rollback"); + holder.close(); + await rm(dbPath, { force: true }); + } + }); + test("classifies wrapped transport failures without retrying in place after output", () => { const runtime = [ "var yt2={ModelRequestFailed:'model_request_failed',ProviderNotConfigured:'provider_not_configured'},", From fb307d203b3229d65a2fcf4f4fc67a7e9259b56f Mon Sep 17 00:00:00 2001 From: Kingsword Date: Sat, 19 Sep 2026 20:26:52 +0800 Subject: [PATCH 2/2] fix(runtime): preserve SQLite write timeout after startup --- .github/workflows/ci.yml | 69 ++++++++++- .github/workflows/prepare-release.yml | 2 +- .github/workflows/publish.yml | 2 +- bun.lock | 4 +- docs/SQLITE_CONCURRENCY.md | 55 +++++++++ package.json | 7 +- scripts/sync-runtime.ts | 37 +++--- test/fixtures/sqlite-session-store.cjs | 78 ++++++++++++ test/node/sqlite-session-store.test.cjs | 150 ++++++++++++++++++++++++ test/release-package.test.ts | 25 ++++ test/sync-runtime.test.ts | 95 +++++++-------- 11 files changed, 451 insertions(+), 73 deletions(-) create mode 100644 docs/SQLITE_CONCURRENCY.md create mode 100644 test/fixtures/sqlite-session-store.cjs create mode 100644 test/node/sqlite-session-store.test.cjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cb68bed..bd1c5b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,7 +30,7 @@ jobs: - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 with: - bun-version: 1.3.12 + bun-version: 1.4.1 - name: Install extraction tools run: sudo apt-get update && sudo apt-get install -y p7zip-full @@ -59,3 +59,70 @@ jobs: npm pkg fix --dry-run --json git diff --check git diff --exit-code -- package.json zcode-runtime.lock.json + + # Transfer the exact validated package, preserving executable bits in + # its tarball. Matrix jobs must not extract a different upstream runtime. + - name: Upload runtime under test + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: node-runtime-under-test + path: | + .release/*.tgz + .release/release.json + include-hidden-files: true + if-no-files-found: error + retention-days: 3 + + node-runtime: + needs: validate + name: Node ${{ matrix.node }} / ${{ matrix.os }} + runs-on: ${{ matrix.os }} + timeout-minutes: 20 + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest] + node: ["22.19.0", "24", "26"] + include: + - os: macos-latest + node: "24" + steps: + - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 + with: + persist-credentials: false + + - uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 # v6 + with: + node-version: ${{ matrix.node }} + package-manager-cache: false + + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.4.1 + + - name: Install test dependencies + run: bun install --frozen-lockfile + + - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 + with: + name: node-runtime-under-test + path: .release + + - name: Restore validated runtime and launcher + shell: bash + run: | + RUNTIME_TARBALL=$(node -p "JSON.parse(require('fs').readFileSync('.release/release.json', 'utf8')).tarball") + tar -xzf "$RUNTIME_TARBALL" --strip-components=1 + node --version + bun --version + + - name: Verify Node runtime and packaged installation + run: | + bun scripts/check-runtime.ts + bun scripts/smoke-package.ts + + - name: Run runtime integration tests + run: bun run test:runtime + + - name: Run real Node SQLite concurrency tests + run: bun run test:node diff --git a/.github/workflows/prepare-release.yml b/.github/workflows/prepare-release.yml index 64c8a70..a6d9aa1 100644 --- a/.github/workflows/prepare-release.yml +++ b/.github/workflows/prepare-release.yml @@ -41,7 +41,7 @@ jobs: - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 with: - bun-version: 1.3.12 + bun-version: 1.4.1 - name: Install extraction tools run: sudo apt-get update && sudo apt-get install -y p7zip-full diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 664a920..0995440 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -67,7 +67,7 @@ jobs: - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 with: - bun-version: 1.3.12 + bun-version: 1.4.1 - name: Install extraction tools run: sudo apt-get update && sudo apt-get install -y p7zip-full diff --git a/bun.lock b/bun.lock index 5a427e6..5195ee4 100644 --- a/bun.lock +++ b/bun.lock @@ -11,7 +11,7 @@ "devDependencies": { "@xterm/headless": "6.0.0", "beautiful-mermaid": "^1.1.3", - "bun-types": "^1.3.14", + "bun-types": "1.4.1", "cli-highlight": "^2.1.11", "diff": "^9.0.0", "just-bash": "^3.4.2", @@ -228,7 +228,7 @@ "buffer": ["buffer@5.7.1", "", { "dependencies": { "base64-js": "^1.3.1", "ieee754": "^1.1.13" } }, "sha512-EHcyIPBQ4BSGlvjB16k5KgAJ27CIsHY/2JBmCRReo48y9rQ3MaUzWX3KVlBa4U7MyX02HdVj0K7C3WaB3ju7FQ=="], - "bun-types": ["bun-types@1.3.14", "", { "dependencies": { "@types/node": "*" } }, "sha512-4N0ig0fEomHt5R0KCFWjovxow98rIoRwKolrYdCcknNwMekCXRnWEUvgu5soYV8QXtVsrUD8B95MBOZGPvr6KQ=="], + "bun-types": ["bun-types@1.4.1", "", { "dependencies": { "@types/node": "*" } }, "sha512-loKuVrAFZKfEv+JvWkHRS9GW5IqLuLRjVXN9p+vZvBN86O5hf/pBZQ5hSoyipsrMmWObZBDvWnlmKvjKTM0PdA=="], "cac": ["cac@7.0.0", "", {}, "sha512-tixWYgm5ZoOD+3g6UTea91eow5z6AAHaho3g0V9CNSNb45gM8SmflpAc+GRd1InC4AqN/07Unrgp56Y94N9hJQ=="], diff --git a/docs/SQLITE_CONCURRENCY.md b/docs/SQLITE_CONCURRENCY.md new file mode 100644 index 0000000..d927151 --- /dev/null +++ b/docs/SQLITE_CONCURRENCY.md @@ -0,0 +1,55 @@ +# SQLite contention regression (#162 / #163) + +The CLI's shared session database uses SQLite WAL. Readers can overlap a writer, +but different CLI processes still serialize writes to this file. After successful +store initialization, each connection now waits up to 10 seconds for a write lock. +Both the synchronous constructor and asynchronous `openStartup()` path apply this +setting **after** migrations finish. Startup migration lock budgets, short busy +waits, backoff, rollback, and failure cleanup are unchanged. + +This is a bounded contention mitigation, not unlimited concurrency or a retry of +an entire agent turn. A lock held beyond the budget can still fail. The change +does not migrate historical data or alter the database schema. Do not delete the +database or its WAL/SHM files to work around a lock while processes are using it. + +## Reproduce and verify + +Use Bun 1.4.1 for building and Node >=22.19.0 for the CLI and storage tests: + +```sh +bun install --frozen-lockfile +bun run sync:locked +bun run test:node +``` + +The Node tests use the actual vendored `SqliteSessionStore`, temporary databases, +and independent Node processes. They do not use account credentials or send model +requests. They cover: + +- the effective `PRAGMA busy_timeout` after sync open, async startup, and reopen; +- a native session write blocked by another process for 6.5 seconds, longer than + the previous 5-second timeout, then succeeding exactly once; +- a deliberately shortened timeout that reports `SQLITE_BUSY`, writes no session, + and permits a later write after the lock is released; +- four independent processes migrating the same fresh database and persisting + distinct sessions without duplicate migrations or integrity errors; +- killing a writer with uncommitted changes, then reopening without those changes + and successfully writing another session. + +The hold-and-release test needs a separate process: `DatabaseSync` blocks the +calling event loop, so a timer in that same process cannot release the lock. +Each test creates its own database and holder rather than reusing the remainder +of a lock window from a previous measurement. + +CI builds and packs once, then tests that exact artifact on Node 22.19.0, 24, and +26 on Linux, plus Node 24 on macOS. Bun's `node:sqlite` compatibility implementation +is not used as a substitute for Node's SQLite driver. + +## Follow-up boundary + +If real workloads still exceed the wait budget, investigate long transactions and +add bounded retries only at persistence boundaries that can be safely rolled back +and replayed. Never retry a whole agent turn and repeat completed external tools. +Per-session databases or a shared writer service are separate architectural changes +requiring discovery, lifecycle, and migration design. Updating to Desktop 3.14.0 +also remains separate from this fix's pinned-runtime validation. diff --git a/package.json b/package.json index e75634a..b2e1dd8 100644 --- a/package.json +++ b/package.json @@ -58,10 +58,11 @@ "check:tui": "bun scripts/smoke-tui.ts && bun scripts/smoke-tui-features.ts && bun scripts/smoke-tui-clear.ts && bun scripts/smoke-tui-session-title.ts && bun scripts/smoke-tui-pressure.ts && bun scripts/smoke-tui-widths.ts && bun scripts/smoke-tui-fullscreen.ts && bun scripts/smoke-tui-fullscreen-switch.ts && bun scripts/smoke-tui-fullscreen-layout.ts", "check:tui-scenarios": "bun run test:tui", "test": "bun run test:unit", - "test:all": "bun run test:unit && bun run test:tui && bun run test:runtime", + "test:all": "bun run test:unit && bun run test:tui && bun run test:runtime && bun run test:node", "test:unit": "bun test test/*.test.ts", "test:fast": "bun run test:unit", "test:runtime": "bun test test/runtime/*.test.ts", + "test:node": "node --test test/node/*.test.cjs", "test:tui": "bun run test:tui:component && bun run test:tui:e2e", "test:tui:component": "bun test test/tui/scenario-http.test.ts test/tui/scenario-runtime.test.ts test/tui/scenario-shell.test.ts test/tui/scenario-workspace.test.ts test/tui/terminal-screen.test.ts", "test:tui:e2e": "bun test test/tui/allowlisted-shell.test.ts test/tui/http-mock.test.ts test/tui/model-resume.test.ts test/tui/permission-request-queue.test.ts test/tui/run-scenario.test.ts test/tui/session-rename.test.ts test/tui/terminal-session.test.ts test/tui/write-and-diff.test.ts", @@ -79,7 +80,7 @@ "engines": { "node": ">=22.19.0" }, - "packageManager": "bun@1.3.12", + "packageManager": "bun@1.4.1", "publishConfig": { "access": "public", "provenance": true @@ -91,7 +92,7 @@ "devDependencies": { "@xterm/headless": "6.0.0", "beautiful-mermaid": "^1.1.3", - "bun-types": "^1.3.14", + "bun-types": "1.4.1", "cli-highlight": "^2.1.11", "diff": "^9.0.0", "just-bash": "^3.4.2", diff --git a/scripts/sync-runtime.ts b/scripts/sync-runtime.ts index 05a2969..459de0f 100755 --- a/scripts/sync-runtime.ts +++ b/scripts/sync-runtime.ts @@ -1063,31 +1063,38 @@ export const sqliteBusyTimeoutMs = 10_000; const runtimeSqliteBusyTimeoutPragma = `pragma busy_timeout = ${sqliteBusyTimeoutMs}`; -/** Detect the busy-timeout pragma on the shared session-DB connection. */ +/** Verify both successful store-open paths, not an unrelated pragma string. */ export function hasRuntimeSqliteBusyTimeout(runtime: string): boolean { - return runtime.includes(runtimeSqliteBusyTimeoutPragma); + const pragma = escapeRegExpName(runtimeSqliteBusyTimeoutPragma); + const sync = new RegExp(`try\\{[A-Za-z_$][\\w$]*!==[A-Za-z_$][\\w$]*&&\\([A-Za-z_$][\\w$]*\\(this\\.db,this\\.dbPath,[A-Za-z_$][\\w$]*\\),this\\.db\\.exec\\("${pragma}"\\)\\)\\}catch`, "gu"); + const startup = new RegExp(`try\\{return await [A-Za-z_$][\\w$]*\\(([A-Za-z_$][\\w$]*)\\.db,\\1\\.dbPath,[A-Za-z_$][\\w$]*\\),\\1\\.db\\.exec\\("${pragma}"\\),\\1\\}catch`, "gu"); + return countRegExpMatches(runtime, sync) === 1 && countRegExpMatches(runtime, startup) === 1; } /** * Concurrent zcode processes share ~/.zcode/cli/db/db.sqlite. The runtime opens - * it with node:sqlite, whose default busy timeout is 0: lock contention between - * writers surfaces as an immediate SQLITE_BUSY ("database is locked") that - * kills the turn instead of waiting. Set an explicit busy_timeout (longer than - * the open-site `timeout`, which only newer Node runtimes honor) so writers - * wait for the lock instead of failing the turn. + * it with a 5s timeout. Async startup temporarily uses a short timeout for + * migration retries and resets it in finally. Apply the steady-state timeout + * AFTER either migration path succeeds, preserving startup's lock budget and + * cleanup. This bounds ordinary write contention; it is not a transaction or + * whole-turn retry, and cannot guarantee success under sustained contention. */ export function patchRuntimeSqliteBusyTimeout(runtime: string): string { if (hasRuntimeSqliteBusyTimeout(runtime)) return runtime; - // The session store is the only DatabaseSync construction site in the - // bundle; migrations and every session write reuse this connection. - const connectionPattern = /this\.db=new ([A-Za-z_$][\w$]*)\.DatabaseSync\(this\.dbPath(?:,\{([^{}]*)\})?\)/u; - if (!connectionPattern.test(runtime)) { - throw new Error("ZCode runtime is incompatible with the SQLite busy-timeout patch (session DB connection anchor missing)."); + const sync = /try\{([A-Za-z_$][\w$]*)!==([A-Za-z_$][\w$]*)&&([A-Za-z_$][\w$]*)\(this\.db,this\.dbPath,([A-Za-z_$][\w$]*)\)\}catch/gu; + const startup = /try\{return await ([A-Za-z_$][\w$]*)\(([A-Za-z_$][\w$]*)\.db,\2\.dbPath,([A-Za-z_$][\w$]*)\),\2\}catch/gu; + if (countRegExpMatches(runtime, sync) !== 1 || countRegExpMatches(runtime, startup) !== 1) { + throw new Error("ZCode runtime is incompatible with the SQLite busy-timeout patch (store migration anchors missing or ambiguous)."); } - return runtime.replace( - connectionPattern, - (_match, sqlite: string, options: string | undefined) => `this.db=new ${sqlite}.DatabaseSync(this.dbPath${options === undefined ? "" : `,{${options}}`}),this.db.exec("${runtimeSqliteBusyTimeoutPragma}")` + const patched = runtime.replace( + sync, + (_match, mode: string, deferred: string, migrate: string, timeout: string) => `try{${mode}!==${deferred}&&(${migrate}(this.db,this.dbPath,${timeout}),this.db.exec("${runtimeSqliteBusyTimeoutPragma}"))}catch` + ).replace( + startup, + (_match, migrate: string, store: string, options: string) => `try{return await ${migrate}(${store}.db,${store}.dbPath,${options}),${store}.db.exec("${runtimeSqliteBusyTimeoutPragma}"),${store}}catch` ); + if (!hasRuntimeSqliteBusyTimeout(patched)) throw new Error("SQLite busy-timeout patch failed postcondition verification."); + return patched; } function escapeRegExpName(value: string): string { diff --git a/test/fixtures/sqlite-session-store.cjs b/test/fixtures/sqlite-session-store.cjs new file mode 100644 index 0000000..95584fe --- /dev/null +++ b/test/fixtures/sqlite-session-store.cjs @@ -0,0 +1,78 @@ +// Test-only access to the native store. No diagnostic API is added to the bundle. +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const Module = require("node:module"); +const path = require("node:path"); + +function loadSessionStore() { + assert.equal(process.versions.bun, undefined, "SQLite runtime tests must execute in real Node.js"); + const file = path.resolve(process.env.ZCODE_TEST_RUNTIME || path.join(__dirname, "../../vendor/zcode.cjs")); + let source = fs.readFileSync(file, "utf8"); + const store = /([A-Za-z_$][\w$]*)=class(?: [A-Za-z_$][\w$]*)?\{static\{[A-Za-z_$][\w$]*\(this,"SqliteSessionStore"\)/u.exec(source); + assert.ok(store, "Missing native SqliteSessionStore"); + const init = [...source.slice(0, store.index).matchAll(/([A-Za-z_$][\w$]*)=[A-Za-z_$][\w$]*\(\(\)=>\{/gu)].at(-1)?.[1]; + const main = /async function [A-Za-z_$][\w$]*\(\)\{let [A-Za-z_$][\w$]*=process\.argv\.slice\(2\);/u.exec(source); + assert.ok(init && main, "Missing native store test entry"); + source = source.replace(main[0], `${main[0]}${init}();module.exports=${store[1]};return;`); + const runtime = new Module(file, module); + runtime.filename = file; + runtime.paths = Module._nodeModulePaths(path.dirname(file)); + runtime._compile(source, file); + assert.equal(typeof runtime.exports.openStartup, "function"); + return runtime.exports; +} + +function sessionInput(directory, id) { + return { id, projectID: "sqlite-contention-test", directory, slug: id, title: id, version: "test" }; +} + +async function worker(mode, options) { + assert.equal(process.versions.bun, undefined); + if (mode === "hold") { + const { DatabaseSync } = require("node:sqlite"); + const db = new DatabaseSync(options.dbPath); + db.exec("BEGIN IMMEDIATE"); + if (options.changeTitleFor) db.prepare("UPDATE session SET title = ? WHERE id = ?").run("uncommitted title", options.changeTitleFor); + let finished = false; + let releaseTimer; + const finish = (commit) => { + if (finished) return; + finished = true; + clearTimeout(releaseTimer); + clearTimeout(watchdog); + try { db.exec(commit ? "COMMIT" : "ROLLBACK"); } + finally { db.close(); if (process.connected) process.disconnect(); } + }; + const watchdog = setTimeout(() => { process.exitCode = 1; finish(false); }, 25_000); + process.on("disconnect", () => finish(false)); + process.on("message", message => { + if (message === "release") finish(true); + if (message === "wait" && !releaseTimer) releaseTimer = setTimeout(() => finish(true), options.holdMs); + }); + process.send({ type: "ready" }); + return; + } + assert.equal(mode, "startup"); + await new Promise(resolve => { + process.once("message", resolve); + process.send({ type: "ready" }); + }); + const Store = loadSessionStore(); + const store = await Store.openStartup({ dbPath: options.dbPath }); + try { + assert.equal(store.db.prepare("PRAGMA busy_timeout").get().timeout, 10_000); + await store.createSession(sessionInput(path.dirname(options.dbPath), options.id)); + } finally { + store.close(); + } + process.disconnect(); +} + +module.exports = { loadSessionStore, sessionInput }; +if (require.main === module) { + worker(process.argv[2], JSON.parse(process.argv[3])).catch(error => { + console.error(error); + process.exitCode = 1; + if (process.connected) process.disconnect(); + }); +} diff --git a/test/node/sqlite-session-store.test.cjs b/test/node/sqlite-session-store.test.cjs new file mode 100644 index 0000000..79ea6fb --- /dev/null +++ b/test/node/sqlite-session-store.test.cjs @@ -0,0 +1,150 @@ +const assert = require("node:assert/strict"); +const { fork } = require("node:child_process"); +const { mkdtemp, rm } = require("node:fs/promises"); +const { tmpdir } = require("node:os"); +const path = require("node:path"); +const { test } = require("node:test"); +const { loadSessionStore, sessionInput } = require("../fixtures/sqlite-session-store.cjs"); + +assert.equal(process.versions.bun, undefined, "Run with npm run test:node / node --test, not bun test"); +const Store = loadSessionStore(); + +async function withDirectory(run) { + const directory = await mkdtemp(path.join(tmpdir(), "zcode-sqlite-runtime-")); + try { await run(directory, path.join(directory, "sessions.sqlite")); } + finally { await rm(directory, { recursive: true, force: true }); } +} + +async function startWorker(mode, options) { + const child = fork(path.join(__dirname, "../fixtures/sqlite-session-store.cjs"), [mode, JSON.stringify(options)], { + execArgv: [], stdio: ["ignore", "ignore", "pipe", "ipc"], timeout: 20_000 + }); + let stderr = ""; + child.stderr.on("data", chunk => { stderr += chunk; }); + const exited = new Promise(resolve => child.once("close", (code, signal) => resolve({ code, signal }))); + try { + await new Promise((resolve, reject) => { + child.once("error", reject); + child.once("message", message => message?.type === "ready" ? resolve() : reject(new Error("Unexpected worker message"))); + child.once("close", () => reject(new Error(`Worker exited before ready: ${stderr}`))); + }); + } catch (error) { + child.kill(); + await exited; + throw error; + } + return { + send(message) { if (child.connected) child.send(message); }, + async wait() { + const result = await exited; + assert.equal(result.code, 0, `Worker failed (${result.signal}): ${stderr}`); + }, + async stop(signal = "SIGTERM") { if (child.connected) child.kill(signal); await exited; } + }; +} + +test("real runtime keeps the write timeout after sync open, async startup and reopen", { timeout: 15_000 }, async t => { + await withDirectory(async (_directory, dbPath) => { + const sync = new Store({ dbPath, startupLockTimeoutMs: 750 }); + try { assert.equal(sync.db.prepare("PRAGMA busy_timeout").get().timeout, 10_000); } + finally { sync.close(); } + for (let attempt = 0; attempt < 2; attempt++) { + const store = await Store.openStartup({ dbPath }); + try { + assert.equal(store.db.prepare("PRAGMA busy_timeout").get().timeout, 10_000); + assert.equal(store.db.prepare("PRAGMA journal_mode").get().journal_mode, "wal"); + t.diagnostic(`Node ${process.version}; SQLite ${store.db.prepare("SELECT sqlite_version() AS version").get().version}; busy_timeout=10000`); + } finally { store.close(); } + } + }); +}); + +test("a native session write survives a lock held beyond the old five-second window", { timeout: 20_000 }, async () => { + await withDirectory(async (directory, dbPath) => { + const store = await Store.openStartup({ dbPath }); + let holder; + try { + assert.equal(store.db.prepare("PRAGMA busy_timeout").get().timeout, 10_000); + holder = await startWorker("hold", { dbPath, holdMs: 6500 }); + // The independent process releases the lock even while DatabaseSync blocks this event loop. + holder.send("wait"); + const startedAt = performance.now(); + await store.createSession(sessionInput(directory, "waited-session")); + assert.ok(performance.now() - startedAt >= 6000, "The write must overlap the held lock"); + await holder.wait(); + assert.equal(store.db.prepare("SELECT COUNT(*) AS count FROM session WHERE id = ?").get("waited-session").count, 1); + } finally { + await holder?.stop(); + store.close(); + } + }); +}); + +test("lock timeout is bounded, writes nothing, and the same connection works after release", { timeout: 15_000 }, async () => { + await withDirectory(async (directory, dbPath) => { + const store = await Store.openStartup({ dbPath }); + let holder; + try { + // Use a short test budget; the production budget is asserted separately above. + store.db.exec("PRAGMA busy_timeout = 250"); + holder = await startWorker("hold", { dbPath }); + const startedAt = performance.now(); + await assert.rejects(store.createSession(sessionInput(directory, "bounded-session")), error => { + assert.equal(error.code, "ERR_SQLITE_ERROR"); + assert.equal(error.errcode & 255, 5); // SQLITE_BUSY, not every SQLite error. + return true; + }); + assert.ok(performance.now() - startedAt >= 150); + assert.equal(store.db.prepare("SELECT COUNT(*) AS count FROM session").get().count, 0); + holder.send("release"); + await holder.wait(); + await store.createSession(sessionInput(directory, "bounded-session")); + assert.equal(store.db.prepare("SELECT COUNT(*) AS count FROM session").get().count, 1); + } finally { + await holder?.stop(); + store.close(); + } + }); +}); + +test("independent processes migrate one fresh database and retain every session write", { timeout: 25_000 }, async () => { + await withDirectory(async (_directory, dbPath) => { + const workers = []; + try { + for (let index = 0; index < 4; index++) workers.push(await startWorker("startup", { dbPath, id: `concurrent-${index}` })); + for (const worker of workers) worker.send("go"); + await Promise.all(workers.map(worker => worker.wait())); + const store = await Store.openStartup({ dbPath }); + try { + assert.equal(store.db.prepare("SELECT COUNT(*) AS count FROM session").get().count, 4); + const migrations = store.db.prepare("SELECT id, checksum FROM schema_migration").all(); + assert.ok(migrations.length > 0); + assert.equal(new Set(migrations.map(row => row.id)).size, migrations.length); + assert.ok(migrations.every(row => typeof row.checksum === "string" && row.checksum.length > 0)); + assert.equal(store.db.prepare("PRAGMA integrity_check").get().integrity_check, "ok"); + } finally { store.close(); } + } finally { await Promise.all(workers.map(worker => worker.stop())); } + }); +}); + +test("a killed writer releases its lock and uncommitted changes do not survive reopen", { timeout: 15_000 }, async () => { + await withDirectory(async (directory, dbPath) => { + let store = await Store.openStartup({ dbPath }); + let holder; + try { + await store.createSession(sessionInput(directory, "committed-session")); + holder = await startWorker("hold", { dbPath, changeTitleFor: "committed-session" }); + await holder.stop("SIGKILL"); + store.close(); + store = undefined; + store = await Store.openStartup({ dbPath }); + assert.equal(store.db.prepare("SELECT title FROM session WHERE id = ?").get("committed-session").title, "committed-session"); + await store.createSession(sessionInput(directory, "after-crash")); + assert.equal(store.db.prepare("SELECT COUNT(*) AS count FROM session").get().count, 2); + assert.equal(store.db.prepare("PRAGMA integrity_check").get().integrity_check, "ok"); + } finally { + await holder?.stop(); + store?.close(); + } + }); +}); diff --git a/test/release-package.test.ts b/test/release-package.test.ts index b746e61..5a0d2cb 100644 --- a/test/release-package.test.ts +++ b/test/release-package.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, test } from "bun:test"; +import { parse } from "yaml"; import { validatePackageTree } from "../scripts/check-package.ts"; import { runtimePatchPlan } from "../scripts/sync-runtime.ts"; @@ -81,6 +82,8 @@ describe("release package", () => { expect(packageJson.scripts["test:all"]).toContain("test:unit"); expect(packageJson.scripts["test:all"]).toContain("test:tui"); expect(packageJson.scripts["test:all"]).toContain("test:runtime"); + expect(packageJson.scripts["test:all"]).toContain("test:node"); + expect(packageJson.scripts["test:node"]).toBe("node --test test/node/*.test.cjs"); expect(packageJson.bin.zcode).toBe("bin/zcode.js"); expect(packageJson.engines).toEqual({ node: ">=22.19.0" }); expect(packageJson.dependencies.zigpty).toBeUndefined(); @@ -95,6 +98,28 @@ describe("release package", () => { expect(packageJson.keywords).toEqual(expect.arrayContaining(["cli", "node", "terminal", "tui", "zcode"])); }); + test("pins one Bun toolchain and tests the validated artifact with real Node versions", async () => { + const packageJson = await Bun.file(new URL("../package.json", import.meta.url)).json(); + expect(packageJson.packageManager).toBe("bun@1.4.1"); + expect(packageJson.devDependencies["bun-types"]).toBe("1.4.1"); + for (const name of ["ci", "prepare-release", "publish"]) { + const workflow = parse(await Bun.file(new URL(`../.github/workflows/${name}.yml`, import.meta.url)).text()); + for (const job of Object.values(workflow.jobs) as { steps?: { uses?: string; with?: Record }[] }[]) { + for (const step of job.steps ?? []) { + if (step.uses?.startsWith("oven-sh/setup-bun@")) expect(step.with?.["bun-version"]).toBe("1.4.1"); + } + } + if (name === "ci") { + const matrix = workflow.jobs["node-runtime"]; + expect(matrix.needs).toBe("validate"); + expect(matrix.strategy.matrix.node).toEqual(["22.19.0", "24", "26"]); + expect(matrix.strategy.matrix.include).toContainEqual({ os: "macos-latest", node: "24" }); + expect(matrix.steps.some((step: { run?: string }) => step.run === "bun run test:node")).toBe(true); + expect(matrix.steps.some((step: { uses?: string }) => step.uses?.startsWith("actions/download-artifact@"))).toBe(true); + } + } + }); + test("syncs the runtime before running runtime-backed integration tests", async () => { const source = await Bun.file(new URL("../scripts/build-release.ts", import.meta.url)).text(); const syncStep = source.indexOf('await run(["run", latest ? "sync" : "sync:locked"]);'); diff --git a/test/sync-runtime.test.ts b/test/sync-runtime.test.ts index 5374076..eb28636 100644 --- a/test/sync-runtime.test.ts +++ b/test/sync-runtime.test.ts @@ -302,69 +302,64 @@ describe("runtime synchronization", () => { expect(hasRuntimeHttpNoContentGuard("runtime without the HTTP wrapper")).toBe(false); }); - test("waits for the shared session DB lock instead of failing with SQLITE_BUSY", async () => { + test("sets the SQLite write timeout after both migration paths without changing startup budgets", async () => { const runtime = [ - 'class Store{constructor(t={}){this.dbPath=t.dbPath??"~/.zcode/cli/db/db.sqlite";let r=t.startupLockTimeoutMs??5e3;', - 'try{this.db=new FNt.DatabaseSync(this.dbPath,{timeout:r})}catch(n){throw new Error(`Failed to open SQLite session database at ${this.dbPath}`)}', - 'this.db.exec("pragma foreign_keys = on")}}' + 'const deferred=Symbol();class Store{constructor(t={},n){this.dbPath=t.dbPath??"fixture.sqlite";let r=t.startupLockTimeoutMs??5e3;', + 'this.db=new sqlite.DatabaseSync(this.dbPath,{timeout:r});', + 'try{n!==deferred&&migrateSync(this.db,this.dbPath,r)}catch(e){this.db.close();throw e}}', + 'static async openStartup(t={},n={}){let o=new Store(t,deferred);try{return await migrateAsync(o.db,o.dbPath,n),o}catch(e){try{o.close()}catch{}throw e}}', + 'close(){this.db.close()}}' ].join(""); const patched = patchRuntimeSqliteBusyTimeout(runtime); expect(hasRuntimeSqliteBusyTimeout(runtime)).toBe(false); expect(hasRuntimeSqliteBusyTimeout(patched)).toBe(true); + expect(hasRuntimeSqliteBusyTimeout(`unrelated "pragma busy_timeout = ${sqliteBusyTimeoutMs}"`)).toBe(false); const opened: { options: unknown; path: string }[] = []; const statements: string[] = []; - const FNt = { - DatabaseSync: function DatabaseSync(this: { exec: (sql: string) => void }, path: string, options: unknown) { + let migrationFailure = false; + type Database = { exec: (sql: string) => void; close: () => void }; + const sqlite = { + DatabaseSync: function DatabaseSync(this: Database, path: string, options: unknown) { opened.push({ options, path }); this.exec = (sql: string) => statements.push(sql); + this.close = () => { statements.push("close"); }; } }; - const Store = new Function("FNt", `${patched};return Store;`)(FNt) as new (options?: { - dbPath?: string; - startupLockTimeoutMs?: number; - }) => { db: { exec: (sql: string) => void } }; + const Store = new Function("sqlite", "migrateSync", "migrateAsync", `${patched};return Store;`)( + sqlite, + (_db: Database, _path: string, timeout: number) => { + statements.push(`sync migration budget: ${timeout}`); + if (migrationFailure) throw new Error("migration failed"); + }, + async (db: Database) => { + db.exec("pragma busy_timeout = 25"); + try { if (migrationFailure) throw new Error("migration failed"); } + finally { db.exec("pragma busy_timeout = 5000"); } + } + ); new Store({ startupLockTimeoutMs: 2500 }); - // The pragma must land on the connection right after open, before any - // other statement, regardless of the open-site timeout option. - expect(opened).toEqual([{ options: { timeout: 2500 }, path: "~/.zcode/cli/db/db.sqlite" }]); - expect(statements).toEqual([`pragma busy_timeout = ${sqliteBusyTimeoutMs}`, "pragma foreign_keys = on"]); + expect(opened).toEqual([{ options: { timeout: 2500 }, path: "fixture.sqlite" }]); + expect(statements).toEqual(["sync migration budget: 2500", `pragma busy_timeout = ${sqliteBusyTimeoutMs}`]); + statements.length = 0; + await Store.openStartup(); + expect(statements).toEqual(["pragma busy_timeout = 25", "pragma busy_timeout = 5000", `pragma busy_timeout = ${sqliteBusyTimeoutMs}`]); + + // Failed migration paths must close the connection, not hand it to callers. + migrationFailure = true; + statements.length = 0; + await expect(Store.openStartup()).rejects.toThrow("migration failed"); + expect(statements).toEqual(["pragma busy_timeout = 25", "pragma busy_timeout = 5000", "close"]); + statements.length = 0; + expect(() => new Store()).toThrow("migration failed"); + expect(statements).toEqual(["sync migration budget: 5000", "close"]); expect(patchRuntimeSqliteBusyTimeout(patched)).toBe(patched); - expect(() => patchRuntimeSqliteBusyTimeout("incompatible runtime")).toThrow(/connection anchor/); - - // Without a busy timeout, node:sqlite's default is 0: a second writer on a - // WAL database fails immediately with ERR_SQLITE_ERROR ("database is - // locked"), the fatal turn.failed cause from the field reports. With the - // pragma the same open blocks for the configured window instead. - const { DatabaseSync } = require("node:sqlite") as { - DatabaseSync: new (path: string, options?: { timeout?: number }) => { - exec: (sql: string) => void; - close: () => void; - }; - }; - const dbPath = join(tmpdir(), `zcode-busy-${process.pid}-${Date.now()}.sqlite`); - const holder = new DatabaseSync(dbPath); - holder.exec("create table t(x)"); - holder.exec("pragma journal_mode = wal"); - holder.exec("begin immediate"); - try { - const immediate = new DatabaseSync(dbPath); - let startedAt = Date.now(); - expect(() => immediate.exec("begin immediate")).toThrow("database is locked"); - expect(Date.now() - startedAt).toBeLessThan(500); - immediate.close(); - - const waiting = new DatabaseSync(dbPath); - waiting.exec("pragma busy_timeout = 250"); - startedAt = Date.now(); - expect(() => waiting.exec("begin immediate")).toThrow("database is locked"); - // SQLITE_BUSY only surfaces once the full busy window has elapsed. - expect(Date.now() - startedAt).toBeGreaterThan(150); - waiting.close(); - } finally { - holder.exec("rollback"); - holder.close(); - await rm(dbPath, { force: true }); - } + expect(() => patchRuntimeSqliteBusyTimeout("incompatible runtime")).toThrow(/migration anchors/); + expect(() => patchRuntimeSqliteBusyTimeout(runtime.replace("await migrateAsync", "await changed"))).not.toThrow(); + expect(() => patchRuntimeSqliteBusyTimeout(runtime.replace("o.dbPath,n", "o.dbPath"))).toThrow(/migration anchors/); + expect(() => patchRuntimeSqliteBusyTimeout(runtime + runtime)).toThrow(/ambiguous/); + const partial = patched.replace(`,o.db.exec("pragma busy_timeout = ${sqliteBusyTimeoutMs}")`, ""); + expect(hasRuntimeSqliteBusyTimeout(partial)).toBe(false); + expect(() => patchRuntimeSqliteBusyTimeout(partial)).toThrow(/migration anchors/); }); test("classifies wrapped transport failures without retrying in place after output", () => {