fix(ensure-deps): stop Bun from seeding an ABI-mismatched native cache - #1195
Closed
jgbriel-io wants to merge 1 commit into
Closed
jgbriel-io wants to merge 1 commit into
jgbriel-io wants to merge 1 commit into
Conversation
The Bun branch added for #543 copied the active better_sqlite3.node to better_sqlite3.abi${process.versions.modules}.node. Bun spoofs that value, while the bytes being copied were produced by whichever Node last ran `npm rebuild` — so the copy is named after an ABI nothing verified it matches. Nothing downstream validates the name. The fast path swaps the cached file into the active slot and, when skipProbe is true, returns without loading it, which installs an ABI-mismatched binding as the live one. On one Linux install this left better_sqlite3.abi127.node and better_sqlite3.abi147.node byte-identical on a host with only Node 22 (ABI 127) present. Under Bun the store runs on bun:sqlite and better-sqlite3 is never dlopened, so the ABI cache is not Bun's to write. The branch is now a bare early return, and /ctx-upgrade reports "not required under Bun" instead of warning about a marker that does not apply — which addresses the spurious warning #543 was opened for without writing an unverifiable file to disk. Refs #543, #1193
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.
Fixes the primary defect in #1193.
The bug
The Bun branch added for #543 seeds the ABI cache:
abiCachePathis named fromprocess.versions.modules. Bun spoofs that value; the bytes atbinaryPathwere produced by whichever Node last rannpm rebuild. So the copy is named after an ABI that nothing verified it matches, and the existing comment states the assumption that turned out to be wrong:The filename is right for the spoofed ABI. The bytes are not.
Nothing downstream checks. The fast path swaps the cached file into the active slot and, when
skipProbeis true, returns on// Trust the cached binary — skip SIGSEGV-prone probewithout ever loading it — so a Node boot at the spoofed ABI installs a mismatched binding as the live one.Observed on an Ubuntu install running context-mode 1.0.169, Node v22.23.2 (ABI 127) and Bun 1.4.2, with no ABI-147 runtime anywhere on the machine:
abi147.nodeis an ABI-127 binary, written 156 ms afterabi127.nodeby the Bun seed.The fix
Under Bun the store runs on
bun:sqliteand better-sqlite3 is never dlopened, so the ABI cache is not Bun's to write and it cannot tell which ABI the file on disk holds. The branch becomes a bare early return.That leaves #543's actual complaint — a spurious
Native addon ABI cache missingwarning from/ctx-upgradeunder Bun — sosrc/cli.tsnow reportsNative addons OK — not required under Bunfor that case. The warning goes away without writing an unverifiable file to disk.I considered two alternatives and rejected both:
NODE_MODULE_VERSIONmarker I could read back (stringsfinds nothing), so this needs a sidecar file recording the building ABI — new on-disk state for a cosmetic warning.skipProbeearly return exist to avoid.Happy to switch to either if you prefer one.
Tests
tests/core/cli.test.ts— theBun ABI cache seeding (#543)block asserted the old behaviour, including the stringproduces the CORRECT filename, so it is replaced rather than extended. Six tests now cover: no cache entry created under Bun, noabi*.nodefile created for any ABI, missing binary and missingnativeDirstill not throwing, an existing entry left untouched, and a source-contract guard that the Bun gate performs no file write at all.Three of the six fail against
nextand pass with the fix:Full suite on this branch:
Ubuntu, kernel 7.0.0-31-generic, Node v22.23.2, npm 10.9.8.