ci: Node 22/24 test workflow; better-sqlite3 13 to stop the Node 24.19 ObjectWrap abort - #9
Merged
Merged
Conversation
…o stop the Node 24.19 ObjectWrap abort GKS had no CI. .github/workflows/test.yml runs `npm ci` + `npm test` (vitest contract+integration, node:test security) on ubuntu-latest with Node 22 and 24, on pull_request and push to main. Actions are pinned to commit SHAs (checkout v7.0.1, setup-node v7.0.0 with npm cache); no secrets; the external MSP suite skips without MSP_REPO_ROOT. SQLite-backed tests aborted the process on Node 24.19.0 with `node::RemoveEnvironmentCleanupHook ... Assertion failed: (env) != nullptr` under `Statement::~Statement`. Root cause is upstream, not GKS's statement handling: Node 24.19.0 backported "src: add cleanup hooks to node::ObjectWrap" (nodejs/node#63642) into the header-only node_object_wrap.h without the global cleanup-hook registry (1723773d), so ~ObjectWrap() calls RemoveEnvironmentCleanupHook(Isolate::GetCurrent()) and aborts whenever a wrapped object is collected with no entered context (nodejs/node#65446; v24 backport of the registry still open in nodejs/node#65943; 24.20.0 and 24.21.0 are unchanged). better-sqlite3 11.10.0 has no node-v137 prebuild, so it was compiled from source against the 24.19.0 headers and inherited the new destructor; any Statement GC could abort, whether or not its Database was closed. better-sqlite3 13 is a node-addon-api rewrite (Napi::ObjectWrap, napi_wrap) with prebuilt binaries shipped in the package; its binary imports neither cleanup-hook symbol. Node floor raised to >=22 to match it. Local (Windows, Node 24.19.0): before, pipeline-genesisrag17 aborts (worker exited unexpectedly) and the security file fails 0/1; after, npm test = vitest 19 files passed | 2 skipped, 156 tests passed | 2 skipped; node:test security 10/10 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Summary
Two infrastructure fixes, no pipeline logic touched (
packages/gks-core/src/pipeline.mjsis untouched; PR #7 owns it)..github/workflows/test.ymlrunsnpm ci+npm test(vitest contract + integration, then the node:test security suite) onubuntu-latest, Node 22 and 24 matrix, onpull_requestandpushtomain.actions/checkoutv7.0.1 andactions/setup-nodev7.0.0 (npm cache) are pinned to commit SHAs;permissions: contents: read; no secrets. The external MSP integration suite skips withoutMSP_REPO_ROOT, as intended.better-sqlite3^11.10.0→^13.0.3; rootengines.node>=20→>=22(13's floor).Root cause of the abort
node::RemoveEnvironmentCleanupHook ... Assertion failed: (env) != nullptrunderStatement::~Statementis an upstream Node.js regression. It is not caused by how GKS caches or closes statements:node::ObjectWrap(nodejs/node#63642) into the header-onlynode_object_wrap.h: the constructor adds a cleanup hook, and~ObjectWrap()callsRemoveEnvironmentCleanupHook(Isolate::GetCurrent(), …). It did not backport the global addon cleanup-hook registry (1723773d), which is what makes removal safe without a liveEnvironment. So when V8 collects a wrapped object with no entered context (allocation-driven GC, weak callback),Environment::GetCurrentis null and the CHECK aborts. Tracked as nodejs/node#65446. The v24 backport of the registry is still open (nodejs/node#65943). I checked the tags: v24.20.0 and v24.21.0 still have the new header and noCleanupHookThunkinsrc/api/hooks.cc, so upgrading Node does not fix it yet.better-sqlite311.10.0 publishes nonode-v137prebuild, sonpm cion Node 24 builds it from source against those headers. AnyStatementcollected by GC can then abort the process, including mid-run with theDatabasestill open. The "statement outlivesdb.close()" hypothesis does not hold: GC of any statement can trigger it.RemoveEnvironmentCleanupHook. The 12.11.1 prebuild (built before 24.19) imports onlyAddEnvironmentCleanupHook. The 13.0.3 prebuild imports neither and usesnapi_wrap.better-sqlite313.0.0 is a node-addon-api rewrite (Statement : Napi::ObjectWrap<Statement>) that ships its prebuilt binaries inside the package (release notes), so it is structurally outside the broken header. For reference, the upstream report WiseLibs/better-sqlite3#1515 was closed as not planned, pointing at the Node regression.Verification (this machine: Windows 11, Node v24.19.0)
pipeline-genesisrag17.test.mjs(vitest)Assertion failed: (env) != nullptr, worker exited unexpectedlytests 1 / pass 0 / fail 1Full
npm testafter a cleannpm ci: vitest 19 files passed | 2 skipped (21), 156 tests passed | 2 skipped (158); node:test security 10 pass, 0 fail. The 2 skips are the external MSP tests (msp-provider-compatibility,msp-service-chain), which skip becauseMSP_REPO_ROOTis unset.Docs
README.md0.2.2b → 0.2.4b: toolchain note (Node>=22, the workflow, why 11.x/12.x must not return). The version is 0.2.4b because open PR feat(pipeline): produce ontology_v2, gate {ontology_v1, ontology_v2} (ADR-075 Phase 2, step 2) #7 takes 0.2.3b. The frontmatter lines will conflict textually with feat(pipeline): produce ontology_v2, gate {ontology_v1, ontology_v2} (ADR-075 Phase 2, step 2) #7; whichever PR merges second keeps both CHANGELOG rows.docs/RUNBOOK-GKS-LOCAL.md0.2.1b → 0.2.2b: replaced the "rebuildbetter-sqlite3against 24.18.x headers" step with the reason it is no longer needed, and named the CI workflow.No
DPS-KI-*stage is advanced by this PR. It is test and CI infrastructure only.🤖 Generated with Claude Code