Skip to content

fix(storage): move better-sqlite3 to 13 and await the runtime's real exit - #7

Merged
Freshair129 merged 3 commits into
mainfrom
fix/better-sqlite3-13-wal-windows
Sep 13, 2026
Merged

Freshair129 merged 3 commits into
mainfrom
fix/better-sqlite3-13-wal-windows

Conversation

@Freshair129

@Freshair129 Freshair129 commented Sep 12, 2026

Copy link
Copy Markdown
Owner

MSP was stuck between two failures on Windows / Node v24.19.0, blocking a clean GenesisRAG17 four-process acceptance run. Both were reproduced in an isolated worktree before anything changed.

Mode 1 — the pinned 11.10.0 aborts the runtime (exit 134)

better-sqlite3 11.10.0 publishes no node-v137 prebuild (prebuild-install warn install No prebuilt binaries found (target=24.19.0 ...)), so npm ci compiles it from source against the running Node's headers. Node 24.19.0 backported the node::ObjectWrap cleanup hooks into the header-only node_object_wrap.h without the global registry that makes removal safe with no live Environment, so any Statement finalization can abort the process:

node::RemoveEnvironmentCleanupHook ... Assertion failed: (env) != nullptr
  Statement::scalar deleting destructor

Seen as MSP process exited with code 134, nondeterministically — 2, 3 and 4 of 7 across three consecutive runs of memory-decay-tick.test.mjs alone. Upstream and not fixable here (nodejs/node#65446; the v24 registry backport is still open in nodejs/node#65943, and 24.20/24.21 are unchanged). Same finding and same remedy as Genesis-Knowledge-System#9.

Mode 2 — why 12+ raises SQLITE_IOERR_TRUNCATE

Every failure on 12.x/13.x landed on open() in packages/msp-storage/src/db/connection.mjs, called from the test process, with SqliteError: disk I/O error, extended code SQLITE_IOERR_TRUNCATE.

It is not the journal mode, the busy timeout, or any open option — a bare SELECT and a read-only open fail the same way, and the case in the original hypothesis (test process reading while the spawned runtime holds the database) is the one that always works. The trigger is a single window: opening the database after child.kill() but before the child has actually exited.

Second connection opens… 11.10.0 (SQLite 3.49.2) 12.11.1 (3.53.2) 13.0.3 (3.53.4)
while the runtime is alive pass pass pass
after child.kill(), same synchronous turn pass 49/60 fail 42/60 fail
after the child's exit event pass 0/60 fail 0/60 fail

child.kill() only asks the OS to terminate the process. Until the kernel finishes tearing it down, the dying process still has the WAL index (<db>-shm) memory-mapped. A connection opened in that window takes the WAL dead-man-switch lock, concludes it is the first connection, and truncates -shm to zero to force a WAL-index rebuild — which Windows refuses while a user-mapped section is open. A plain ftruncate on -shm from Node in the same window fails identically (UNKNOWN, errno -4094, i.e. ERROR_USER_MAPPED_FILE) while -wal truncates fine, which is how the file involved was identified. SQLite 3.53.x surfaces that refusal; 3.49.2 did not, so the suite had been resting on the older library tolerating a race that was always present.

So the break is between 11 and 12 as expected, but it is the bundled SQLite version, not the Node-API rewrite.

The change

  • createMspStdioCaller's close() returns a promise resolving on the child's real exit. Backward compatible: kill() is still issued synchronously, the promise only ever resolves (never rejects), and callers ignoring the return value behave exactly as before. Published as @freshair129/msp-client-js 0.2.1 (additive on top of main's 0.2.0).
  • Every test that reads a vault database from the test process, or deletes the directory holding it, awaits it. The three hand-rolled JSON-RPC harnesses await their own child.kill() the same way, each guarded against an already-exited child so a self-inflicted death cannot hang node --test.
  • The try { rmSync(...) } catch {} "best-effort (Windows file-lock race on child process exit)" swallows and the setTimeout(100) + maxRetries: 5 teardowns were removed rather than kept. With the exit awaited, cleanup must succeed first try, so those blocks now fail loudly if this regresses — which is exactly how the two hand-rolled harnesses were found. Where a mid-test close() sits inside a try, the finally now also closes, so a failing vault-isolation assertion still reports its own message instead of an EPERM from cleanup (verified by deliberately breaking one).
  • tests/contract/transport-close-lifecycle.test.mjs pins the contract: reopen the same WAL database immediately after an awaited close(), ten times, no retry, no sleep. Reverting close() to fire-and-forget fails all three of its cases.

What was deliberately not changed

packages/msp-storage/src/db/connection.mjs is untouched — journal_mode = WAL, foreign_keys = ON and busy_timeout = 5000 all stay exactly as they were. A retry around open() was considered and rejected: SQLite's busy handler does not cover SQLITE_IOERR, so retrying there would swallow a real error class to hide a race reachable only inside one synchronous turn after killing the process that owns the database. A supervisor restart cannot reach it — spawning a replacement costs many event-loop turns — which is why tests/integration/gks-provider-bridge.test.mjs's close-then-restart case passed throughout.

Node floor raised to >=22 to match better-sqlite3 13, which ships per-platform N-API prebuilds inside the package, so npm ci now needs neither a compiler nor an install script. msp-client-js keeps its own >=20 floor: it is published standalone and has no native dependency.

Before / after, measured against this PR's base ef2b2d1

Windows 11, Node v24.19.0, npm 11.17.0.

Suite main as-is (11.10.0) main + the version bump alone this PR
vitest contract + integration 2 failed / 193 passed (195) 6 failed / 189 passed (195) 198 passed (198), 26 files
node --test security 4 failed / 41 passed (45) 1 failed / 44 passed (45) 45 passed (45)

The main as-is column varies run to run — the abort is nondeterministic; that is one sample. The middle column's security count looks better than the left only because main's rmSync swallows still hide the EPERM half of the symptom; the one remaining failure is the SqliteError itself.

The vitest total moves 195 → 198 because the new contract test adds three cases. No existing test was added, removed, or had an assertion changed.

Green on a clean npm ci tree and stable across nine full-suite runs before the merge, plus the merged run.

Merge note

main advanced past this branch's original base while it was in review. The merge conflicts were CHANGELOG/frontmatter collisions only; both sides' rows are kept, with this branch's renumbered above main's (README 0.2.2b, docs/NOTES.md 0.1.7b, docs/MIGRATION.md 0.1.4b). main's new tests/security/pipeline-vault-scoping.security.mjs arrived carrying the same close() + setTimeout(100) + maxRetries: 5 workaround this branch removes everywhere else, so it now awaits close() like the rest of the suite.

Second merge note (base e3f07b9)

main then gained #8, #9 and #10. Two things from that merge a reviewer should see:

Merged tree on Windows 11 / Node v24.19.0: vitest 27 files / 205 tests passed, security 45/45, pack:client packs 0.2.1.

Review

  • RKOI (architecture): APPROVED, 0 critical. Confirmed no vault-isolation proof weakened (git diff -U0 -- tests/ has exactly one assertion-shaped hit, and it is a comment), no guarded package boundary crossed, GKS bridge fail-closed untouched, close() backward compatible. Its four warnings — finally-guaranteed shutdown in four security tests, already-exited guards on the three hand-rolled awaits, the client version bump, and a line-ending-only touch — are all fixed in this branch.
  • GHOST (QA): VERIFIED. 3x npm test green with no flake; judged the new test non-tautological; found no remaining kill-then-reopen gap in tests/.

Docs updated per the repo's frontmatter/CHANGELOG conventions: README.md 0.2.2b, docs/NOTES.md 0.1.7b (full root-cause record), docs/MIGRATION.md 0.1.4b (consumer-facing close() contract), plus CLAUDE.md.

🤖 Generated with Claude Code

Freshair129 and others added 3 commits September 12, 2026 12:11
…exit

Node 24.19.0 made the pinned better-sqlite3 ^11.10.0 unrunnable, and the
upgrade that fixes it exposed a race MSP's tests had always had. Both were
reproduced on Windows 11 / Node v24.19.0 before anything changed.

Mode 1 -- 11.10.0 publishes no node-v137 prebuild, so npm ci compiles it from
source against the running Node's headers. Node 24.19.0 backported the
node::ObjectWrap cleanup hooks into the header-only node_object_wrap.h without
the global registry that makes removal safe with no live Environment, so any
Statement finalization can abort the process:

  node::RemoveEnvironmentCleanupHook ... Assertion failed: (env) != nullptr

Seen as "MSP process exited with code 134", nondeterministically -- 2, 3 and 4
of 7 in three consecutive runs of memory-decay-tick alone. Upstream, not
fixable here (nodejs/node#65446; the v24 registry backport is still open in
nodejs/node#65943, and 24.20/24.21 are unchanged). Same finding and same remedy
as Genesis-Knowledge-System#9.

Mode 2 -- on 12.x/13.x every failure landed on open() in msp-storage's
connection.mjs, called from the *test* process, with SqliteError: disk I/O
error, extended code SQLITE_IOERR_TRUNCATE. It is not the journal mode, the
busy timeout, or any open option: a bare SELECT and a read-only open fail the
same way, and opening against a *live* runtime always succeeds. The trigger is
one window -- opening the database after child.kill() but before the child has
actually exited:

  second connection opens   | 11.10.0 (3.49.2) | 12.11.1 (3.53.2) | 13.0.3 (3.53.4)
  while runtime alive       | pass             | pass             | pass
  after kill(), same turn   | pass             | 49/60 fail       | 42/60 fail
  after the exit event      | pass             | 0/60             | 0/60

child.kill() only asks the OS to terminate the process. Until the kernel
finishes tearing it down, the dying process still has the WAL index (<db>-shm)
memory-mapped. A connection opened in that window takes the WAL dead-man-switch
lock, concludes it is the first connection, and truncates -shm to zero to force
a WAL-index rebuild -- which Windows refuses while a user-mapped section is
open. A plain ftruncate on -shm from Node in the same window fails identically
(UNKNOWN, errno -4094, ERROR_USER_MAPPED_FILE) while -wal truncates fine, which
is how the file involved was identified. SQLite 3.53.x surfaces that refusal;
3.49.2 did not, so the suite had been resting on the older library tolerating a
race that was always present.

createMspStdioCaller's close() therefore returns a promise that resolves on the
child's real exit. It is backward compatible: kill() is still issued
synchronously, the promise only ever resolves, and callers that ignore the
return value behave exactly as before. Published as msp-client-js 0.1.1.

Every test that reads a vault database from the test process, or deletes the
directory holding it, awaits it. The three hand-rolled JSON-RPC harnesses
await their own child.kill() the same way, each guarded against a child that
already exited so a self-inflicted death cannot hang node --test.

The "best-effort cleanup (Windows file-lock race on child process exit)"
rmSync swallows and api-009-conformance's setTimeout(100) + maxRetries: 5 were
removed rather than kept: with the exit awaited, cleanup must succeed first
try, so those blocks now fail loudly if this regresses -- which is exactly how
the two hand-rolled harnesses were found. Where a mid-test close() sits inside
a try, the finally now also closes, so a failing vault-isolation assertion
still reports its own message instead of an EPERM from the cleanup.

packages/msp-storage/src/db/connection.mjs is deliberately unchanged: WAL,
foreign_keys = ON and busy_timeout = 5000 all stay. A retry around open() was
considered and rejected -- SQLite's busy handler does not cover SQLITE_IOERR,
so retrying there would swallow a real error class to hide a race reachable
only inside one synchronous turn after killing the owning process. A supervisor
restart cannot reach it (spawning a replacement costs many event-loop turns),
which is why the GKS bridge's close-then-restart case passed throughout.

Node floor raised to >=22 to match better-sqlite3 13, which ships per-platform
N-API prebuilds inside the package -- npm ci now needs neither a compiler nor
an install script. msp-client-js keeps its own >=20 floor: it is published
standalone and has no native dependency.

Before/after on this machine (Windows 11, Node v24.19.0, npm 11.17.0):

  suite                | 11.10.0        | 13.0.3, no code change | 13.0.3 + this
  vitest contract+int  | 7 failed /180  | 6 failed /180          | 183 passed
  node --test security | 2 failed /33   | 13 failed /33          | 33 passed

The 11.10.0 row varies run to run; the abort is nondeterministic. The vitest
total moves 180 -> 183 because transport-close-lifecycle.test.mjs is new: it
reopens the same WAL database immediately after an awaited close(), ten times,
with no retry and no sleep. Reverting close() to fire-and-forget fails all
three of its cases.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Doc conflicts were CHANGELOG/frontmatter collisions only; both sides' rows are
kept and this branch's renumbered above main's: README 0.1.2b -> 0.2.2b,
docs/NOTES.md 0.1.6b -> 0.1.7b, docs/MIGRATION.md 0.1.3b -> 0.1.4b. Main's
"npm install" -> "npm ci" change in the verification block is kept, with the
Toolchain section following it.

main's new tests/security/pipeline-vault-scoping.security.mjs arrived with the
same workaround this branch removes everywhere else -- close() followed by a
100ms sleep and rmSync with maxRetries: 5 -- so it now awaits close() and
deletes its temp directory once, like the rest of the suite.

Merged suite on Windows 11 / Node v24.19.0: vitest 26 files / 198 tests passed,
node --test security 45/45 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main gained PR #8 (GKS prefix case), #9 (CI test workflow) and #10 (client
spawns MSP with an allowlisted environment, client 0.1.0 -> 0.2.0, breaking).

Conflict: packages/msp-client-js/package.json version. main's 0.2.0 is the
breaking env allowlist; this branch's awaitable close() is additive, so the
combined client is 0.2.1. docs/MIGRATION.md now says close() ships in 0.2.1.
The transport auto-merged with both changes intact: buildMspChildEnv() feeds
spawn(), and close() still resolves on the child's real exit.

.github/workflows/test.yml (new on main) tested Node 20 and 22, and its
comment asked for Node 24 to be added "in the same change that fixes" the
better-sqlite3 11.10.0 teardown abort. This is that change, so the matrix is
now 22 and 24; 20 is dropped because better-sqlite3 13 and this branch's
engines field both require >=22.

tests/integration/msp-client-env-allowlist.test.mjs (new on main) closes its
caller without awaiting. Its fixture opens no database or temp directory, so
it cannot hit the WAL race, but it now awaits close() like every other caller.

Merged suite on Windows 11 / Node v24.19.0: vitest 27 files / 205 tests
passed, node --test security 45/45 passed, pack:client packs 0.2.1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Freshair129
Freshair129 merged commit 5ad2dfa into main Sep 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant