fix(kernel-store): move to better-sqlite3 13, which survives teardown on Node 24.20 - #1045
Open
sirtimid wants to merge 1 commit into
Open
fix(kernel-store): move to better-sqlite3 13, which survives teardown on Node 24.20#1045sirtimid wants to merge 1 commit into
sirtimid wants to merge 1 commit into
Conversation
… on Node 24.20
On Node 24.20.0, `better-sqlite3` 12.x aborts the process while it is shutting
down:
node::RemoveEnvironmentCleanupHook(Isolate*, CleanupHook, void*)
at ../src/api/hooks.cc:142
Assertion failed: (env) != nullptr
Statement::~Statement() [better_sqlite3.node]
The destructor runs after the environment is gone and asserts on the way out.
Everything it kills has already passed, so it reads as a worker dying rather
than as a test failure, and vitest reports only that the fork exited.
Not a missing `close`. The suites that hit it close their databases; a plain
script, a vitest worker holding an open database, and one holding an open
database plus a live worker thread are all clean on 24.20.0. It takes the whole
kernel to provoke, and the same code on 24.18.0 is clean, which places it
between the two Node releases rather than in this repository.
The tests never ran into it because CI floats on `24.x` and had not run since
2026-09-03, when the runner still resolved that to 24.19.0.
13.x is a major, but it moves none of the surface this driver uses -- `prepare`,
`run`, `get`, `all`, `iterate`, `pluck`, `transaction`, `exec`, `close` -- and
its Node floor is the one this package already sets. Verified on 24.20.0: the
unit suite, the integration suite, and the `kernel-node-runtime` e2e all run
without the abort, and kernel-store passes on 22.23.2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failure
On Node 24.20.0,
better-sqlite312.x aborts the process during shutdown:The destructor runs after the environment is gone and asserts on the way out. Everything it kills has already passed, so it surfaces as
Worker exited unexpectedlyrather than as a test failure anyone can read.Nothing in this repository changed. CI's matrix floats on
24.x, and had not run since 2026-09-03, when the runner still resolved that to 24.19.0. It now resolves to 24.20.0.It is not a missing
closeThe suites that hit this close their databases correctly. I narrowed it on 24.20.0:
close()Same code, same test, only the Node patch differs — which places it between the two Node releases rather than in our teardown.
The fix
13.x is a major, but it moves none of the surface this driver uses —
prepare,run,get,all,iterate,pluck,transaction,exec,close— and itsengines.nodefloor is>=22, the same one this package already declares.Verified on Node 24.20.0 with the bump:
kernel-node-runtimee2e: went from 9 failures plus the abort to running normallyOne caveat, so nobody is surprised
remote-comms.test.ts > handles remote intentional disconnect without reconnectingfails locally for me, and it does so on 24.18.0, 24.19.0 and 24.20.0 alike, with and without this bump. It is flaky in my environment rather than related to either the Node bump or this change — I initially misread it as a second Node regression, then reproduced the failure on 24.18.0 twice in a row. Worth watching in CI, but it is not what this PR is about.Why not pin Node instead
Pinning the matrix to a known-good patch would go green too, and hide this until the next time someone bumps Node. The floating
24.xis a separate reproducibility question worth having on its own terms; this is the actual defect.Needs #1043 to land before CI here can run at all.
🤖 Generated with Claude Code
Note
Medium Risk
Major native-addon upgrade affects all Node SQLite storage paths and install/rebuild behavior, though application code is untouched and the Node engine floor is unchanged.
Overview
Bumps
better-sqlite3from^12.4.1to^13.0.3in@metamask/kernel-store(plus lockfile). There are no changes to the Node SQLite driver in this repo.This addresses a Node 24.20 shutdown bug where 12.x can abort during
Statementteardown (RemoveEnvironmentCleanupHookafter the environment is gone), which shows up as Vitest workers dying after tests pass. The changelog marks the bump as breaking for semver because 13.x is a major, though the package already requires Node ≥22 and the driver APIs in use are unchanged.Lockfile churn drops
bindings/prebuild-installfrom thebetter-sqlite3resolution path and alignsnode-addon-apiwith 13.x’s native build setup.Reviewed by Cursor Bugbot for commit adfc6c4. Bugbot is set up for automated code reviews on this repo. Configure here.