node:sqlite: implement the module and pass the Node v26.3.0 test suite#32498
node:sqlite: implement the module and pass the Node v26.3.0 test suite#32498cirospaciari wants to merge 67 commits into
Conversation
- Add node:sqlite module wrapping bun:sqlite with DatabaseSync/StatementSync - Add process.versions.sqlite (SQLITE_VERSION from bundled sqlite3) - Add skipIfSQLiteMissing/hasSQLite to test common (matches upstream Node) - Fix heap-use-after-free in Bun__closeAllSQLiteDatabasesForTermination: the handle was closed but not nulled, so the GC finalizer running during VM teardown (BUN_DESTRUCT_VM_ON_EXIT=1) would close it again - Enable Node tests: test-sqlite-data-types, test-sqlite-database-sync-dispose, test-sqlite-typed-array-and-data-view Not yet supported (throw ENOTIMPLEMENTED): DatabaseSync.function/aggregate, createSession/applyChangeset, backup().
Replace the JS wrapper with a native C++ implementation that shares the bundled sqlite3 with bun:sqlite (JSSQLStatement.cpp) but exposes Node.js's DatabaseSync/StatementSync API and error semantics directly. - JSDatabaseSync / JSStatementSync as JSDestructibleObject classes - Node-compatible parameter binding: sqlite3_clear_bindings on each call, bare named parameter mapping via sqlite3_bind_parameter_name, allowUnknownNamedParameters/allowBareNamedParameters flags - Uses sqlite3_changes64() (matches Node; unblocks test-sqlite-transactions) - sqlite3_db_config for DQS/FKEY/ENABLE_LOAD_EXTENSION, sqlite3_busy_timeout - ERR_SQLITE_ERROR with errcode/errstr, ERR_INVALID_STATE, etc. - Registered as a native module (no src/js layer) Now passing (4 upstream tests, up from 3): test-sqlite-data-types test-sqlite-database-sync-dispose test-sqlite-transactions test-sqlite-typed-array-and-data-view Still not implemented (throw ERR_METHOD_NOT_IMPLEMENTED): function()/aggregate() (UDFs), createSession()/applyChangeset(), backup()
…directly On macOS, bun:sqlite lazy-loads the system libsqlite3 via dlopen (LAZY_LOAD_SQLITE=1), and sqlite3.c was not compiled at all. NodeSqlite.cpp calls sqlite3_* directly, so the darwin-cpp build failed with unresolved symbols. node:sqlite now uses the bundled amalgamation on every platform (matching Node.js, which always bundles its own SQLite). bun:sqlite's macOS lazy-load path is unchanged.
NodeSqlite.h now forward-declares struct sqlite3 / sqlite3_stmt instead of pulling in sqlite3_local.h, so ZigGlobalObject.cpp and InternalModuleRegistry.cpp (which also see the system sqlite3.h on macOS via JSSQLStatement.h) do not hit header-guard collisions. NodeSqlite.cpp includes sqlite3_local.h directly. Also: drop the unused openImmediately parameter from JSDatabaseSync::create(), and update the remaining node:sqlite probes in issue/25707.test.ts to node:quic.
…d sqlite3 node:sqlite requires the sqlite3 amalgamation to be linked in (Node.js does the same). Previously macOS only dlopen'd the system libsqlite3 for bun:sqlite and did not compile sqlite3.c at all; now it is compiled on every platform. Linux/Windows already bundled it, so they are unchanged (+0-48 KB noise).
- prepare(''): throw ERR_INVALID_STATE 'contains no statements' when
sqlite3_prepare_v2 returns SQLITE_OK with a null stmt (empty /
whitespace / comment-only SQL), instead of returning a zombie
StatementSync that later throws 'statement has been finalized'.
- options.timeout: reject Infinity, non-finite, and out-of-int32-range
values instead of silently wrapping through ToInt32 (matches V8's
IsInt32() check in Node).
- DatabaseSync path: only accept Uint8Array (and Buffer subclass), not
arbitrary TypedArray/DataView, matching Node and the error message.
[skip size check]
…te, backup) - DatabaseSync.prototype.function(): scalar UDFs via sqlite3_create_function_v2, with useBigIntArguments/varargs/deterministic/directOnly options. JS exceptions thrown inside the callback propagate to the caller unwrapped. - DatabaseSync.prototype.aggregate(): custom aggregates and window functions via sqlite3_create_window_function. Per-invocation accumulator state lives in sqlite3_aggregate_context as a placement-new'd Strong<> so it survives GC across window steps; torn down in xFinal. - Session class + createSession()/applyChangeset(): full session-extension support with onConflict/filter callbacks. Sessions are tracked on the database by handle (not JS pointer) so close() can sweep them regardless of GC ordering. - StatementSync.prototype.iterate(): now returns a lazy StatementSyncIterator whose prototype chain ends at %IteratorPrototype%, with next()/return() that step/reset the underlying statement. A reset-generation counter detects the statement being reused under a live iterator. - backup(): copies page-by-page via sqlite3_backup_*, invoking the progress callback between batches and returning a Promise resolved with the total page count. - prepare(sql, options): per-statement readBigInts/returnArrays/ allowBareNamedParameters/allowUnknownNamedParameters, falling back to database-level defaults set on the constructor. - columns(): origin table/column/database metadata now populated (SQLITE_ENABLE_COLUMN_METADATA). Build: enable SQLITE_ENABLE_SESSION/PREUPDATE_HOOK/DBSTAT_VTAB/GEOPOLY/RBU in the bundled sqlite3.c to match Node.js's feature set. Tests: extend node-sqlite.test.ts with coverage for every new surface, and check in upstream Node's test-sqlite-custom-functions.js (30/30) and test-sqlite-statement-sync-columns.js (7/7).
…guard invalid-UTF8 paths; track origin connection on statements
- Bun__sqlite3_version(): moved from JSSQLStatement.cpp to NodeSqlite.cpp so
macOS's LAZY_LOAD_SQLITE build (which sees the system sqlite3.h in that
file) doesn't report Apple's SDK version for process.versions.sqlite.
- validateDatabasePath(): reject Uint8Array paths whose bytes aren't valid
UTF-8 instead of silently opening a private temporary database via
sqlite3_open_v2("").
- JSStatementSync: remember the sqlite3* the statement was prepared on so
isFinalized() can tell a live statement apart from one belonging to a
close()d-then-open()d connection, matching Node's 'statement has been
finalized' instead of the confusing 'errcode: 0 / not an error'.
…h/bun into farm/ed7cafd7/node-sqlite
… busy-spinning The synchronous step loop previously did an immediate 'continue' on SQLITE_BUSY/SQLITE_LOCKED, which would spin the JS thread at 100% CPU forever if the destination file was held by another process. Now sleep 25ms between retries and reject after the source database's busy timeout (min 5s).
…h/bun into farm/ed7cafd7/node-sqlite
…idateExceptionChecks sqlite invokes xFunc/xStep/xFinal/xFilter/xConflict repeatedly within a single sqlite3_step()/sqlite3changeset_apply() call. A ThrowScope's destructor always simulateThrow()s to signal its caller, so the *next* callback invocation's ThrowScope constructor would find m_needExceptionCheck already set and assert under BUN_JSC_validateExceptionChecks=1. Use TopExceptionScope in the callbacks instead: its destructor verifies but does not simulate, so as long as each callback clears the flag via scope.exception() after every potentially-throwing inner call, the chain is satisfied. The outer host function's own ThrowScope then observes the final state via CHECK_UDF_EXCEPTION (which expands to RETURN_IF_EXCEPTION) after sqlite returns. sqliteValueToJS now takes TopExceptionScope&; its single throw path opens a released transient ThrowScope whose simulated throw is consumed by the caller's next scope.exception() check. Also: make the backup() unwritable-path test portable to Windows by targeting a non-existent subdirectory of a real tempDir instead of a rooted POSIX path.
…n rm on Windows The prepared statement created to verify the backup isn't GC'd before dst.close(), so sqlite3_close_v2 zombies the connection and keeps the file handle open. On Windows that blocks tempDir's Symbol.dispose rm with EBUSY. Bun.gc(true) after close finalizes the stmt and releases the zombie. (The aarch64-musl-verify-baseline failure in the same build is an unrelated rustup download timeout.)
closeInternal() frees every tracked sqlite3_session* but can't null the pointer on each JSNodeSqliteSession wrapper (it doesn't track the JS objects, by design). After db.close()+db.open(), a stale session passed both guards (db->isOpen() true again, m_session non-null but dangling) and sqlite3session_changeset/_delete() hit freed memory. Track the originating sqlite3* on the wrapper (same as JSStatementSync) and reject with 'database is not open' when it no longer matches. Also: drop the unused test/js/node/test/sqlite/next-db.js fixture and the explicit 30_000 test timeouts (per test/CLAUDE.md).
…t stale handles The m_originDb check compared the raw sqlite3* pointer, but after close()+open() the allocator frequently recycles the same address for the new connection (seen on Windows release builds), so the stale-handle guard on JSStatementSync/JSNodeSqliteSession passed and dereferenced freed memory. Bump a generation counter on every successful open() and compare that instead — immune to pointer reuse.
for-of's IteratorClose invokes return() on break/early-return with a
normal completion; throwing there turns 'for (r of stmt.iterate()) {
db.close(); break; }' into an unexpected ERR_INVALID_STATE. Match Node
(and this file's own [Symbol.dispose]() convention): skip the reset
and report {done: true} when the statement is already gone.
…s INTEGER
- jsStatementSyncIteratorNext: hoist the done() short-circuit above the
isFinalized() guard so an already-exhausted iterator keeps returning
{done:true} after db.close() instead of throwing (same reorder as the
return() fix).
- bindValue: route isInt32() values through sqlite3_bind_int so
typeof(?) on a bare bound parameter yields 'integer' (not 'real') and
expandedSQL renders 42 instead of 42.0, matching Node.
Same fix as bindValue() in 9e04f81 — a UDF returning 42 should give typeof(udf()) === 'integer', not 'real'.
…failure
sqlite3_create_function_v2 / sqlite3_create_window_function invoke
xDestroy(p) on the failure path (name >255 bytes, nArg out of range,
SQLITE_BUSY) — the header says so explicitly and createFunctionApi's
nRef==0 branch confirms it. The manual 'delete udf' / 'delete agg' on
r != SQLITE_OK therefore freed the same object a second time (heap
double-free reachable from JS via db.function('a'.repeat(256), ()=>0)).
SQLite owns cleanup once xDestroy is passed; drop the manual delete.
Also add 'node:sqlite' to module.builtinModules (NodeModuleModule.cpp).
…e wrapper's m_db
A named-parameter accessor getter (or UDF callback) can call db.close()
between the REQUIRE_STMT guard and the changes64 read; the wrapper's
m_db is then null and sqlite3_changes64(nullptr) is a raw db->nChange
deref (no SQLITE_ENABLE_API_ARMOR in this build). sqlite3_db_handle()
reads the statement's own back-pointer, which survives zombification —
this is what Node's StatementSync::Run does. Test: stmt.run({ get a() {
db.close(); return 1; } }).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/test.ts`:
- Around line 71-72: The reset() and restoreAll() methods in the test tracker
are currently empty implementations that do nothing, which means spy state is
never actually cleaned up even though these calls appear to succeed. Implement
these methods to actually reset and restore the tracker's internal spy state
respectively. Since the tracker instance returned at lines 162-166 is shared,
ensure these cleanup methods properly clear all tracked spies and restore their
original behavior instead of being no-op methods.
- Around line 51-52: The fn method accepts original and impl parameters without
validating that they are callable functions, causing errors to surface later at
the $apply method instead of immediately. Add validation guards at the beginning
of the fn method to check that both original and impl parameters are functions,
and throw an appropriate error if either is not callable. This should occur
before creating the MockFunctionContext instance to catch invalid inputs
upfront.
In `@src/jsc/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 283-287: Replace the sqlite3_close(db->db) call with
sqlite3_close_v2(db->db) to properly defer cleanup in garbage-collected
environments. The sqlite3_close_v2() function always succeeds (no return-code
check needed) and marks unreferenceable connections as zombie connections for
cleanup during VM teardown, matching the pattern already used in
VersionSqlite3::release(). This ensures that when db->db is nulled on the
following line, the finalizer's cleanup path can still defer proper resource
release.
In `@src/jsc/bindings/sqlite/NodeSqlite.h`:
- Around line 162-173: The BusyScope struct can be accidentally copied or moved,
which would cause multiple instances to exist and each would decrement
m_busyDepth in their destructors, leading to counter underflow and bypassing
re-entrancy protection. Make BusyScope non-copyable and non-movable by
explicitly deleting the copy constructor, copy assignment operator, move
constructor, and move assignment operator. Add these deleted member function
declarations to the BusyScope struct definition.
In `@test/js/node/test/parallel/test-sqlite-session.js`:
- Line 607: The test `concurrent applyChangeset with workers` has an explicit
timeout of 120 seconds which violates the no-timeout rule. Remove the `{
timeout: 120_000 }` configuration from this test. Instead of extending the
timeout, reduce the test complexity by either decreasing the number of
iterations from 10 to 3-5, or splitting the test into separate test cases with
fewer workers per test to keep execution within the fast test budget without
requiring explicit timeouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fa6b59e-be07-4812-aace-2c53082f8f1b
📒 Files selected for processing (46)
scripts/build/deps/sqlite.tsscripts/build/unified.tssrc/js/node/test.tssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.hsrc/jsc/bindings/isBuiltinModule.cppsrc/jsc/bindings/sqlite/JSSQLStatement.cppsrc/jsc/bindings/sqlite/NodeSqlite.cppsrc/jsc/bindings/sqlite/NodeSqlite.hsrc/jsc/bindings/sqlite/sqlite3.csrc/jsc/bindings/sqlite/sqlite3_local.hsrc/jsc/bindings/webcore/DOMClientIsoSubspaces.hsrc/jsc/bindings/webcore/DOMIsoSubspaces.hsrc/jsc/modules/NodeModuleModule.cppsrc/jsc/modules/NodeSqliteModule.hsrc/jsc/modules/_NativeModule.hsrc/resolve_builtins/HardcodedModule.rssrc/resolve_builtins/HardcodedModule.zigtest/js/node/module/node-module-module.test.jstest/js/node/process/process.test.jstest/js/node/sqlite/node-sqlite.test.tstest/js/node/test/common/index.jstest/js/node/test/common/index.mjstest/js/node/test/parallel/test-sqlite-aggregate-function.mjstest/js/node/test/parallel/test-sqlite-authz.jstest/js/node/test/parallel/test-sqlite-backup.mjstest/js/node/test/parallel/test-sqlite-config.jstest/js/node/test/parallel/test-sqlite-custom-functions.jstest/js/node/test/parallel/test-sqlite-data-types.jstest/js/node/test/parallel/test-sqlite-database-sync.jstest/js/node/test/parallel/test-sqlite-limits.jstest/js/node/test/parallel/test-sqlite-named-parameters.jstest/js/node/test/parallel/test-sqlite-serialize.jstest/js/node/test/parallel/test-sqlite-session.jstest/js/node/test/parallel/test-sqlite-statement-sync-columns.jstest/js/node/test/parallel/test-sqlite-statement-sync.jstest/js/node/test/parallel/test-sqlite-template-tag.jstest/js/node/test/parallel/test-sqlite-timeout.jstest/js/node/test/parallel/test-sqlite-transactions.jstest/js/node/test/parallel/test-sqlite-typed-array-and-data-view.jstest/js/node/test/parallel/test-sqlite.jstest/js/node/test/sqlite/next-db.jstest/js/node/test/sqlite/worker.jstest/regression/issue/25707.test.ts
… it in ErrorCode.rs The error list in ErrorCode.ts is index-aligned with the checked-in Rust mirror (src/jsc/ErrorCode.rs); new codes must only be appended. ERR_SQLITE_ERROR had been inserted mid-list, shifting every later discriminant by one, so errors thrown from Rust (fs validation, dns, parse-args, blocklist, ...) surfaced with the wrong constructor and code on every platform. Move the entry to the end and add the matching constant, ERR_ alias, CODE_STR entry, and COUNT bump to the Rust mirror.
…ermination path - Delete BusyScope's copy/move operations so an accidental copy can't double-decrement the busy depth and bypass the re-entrant close() guard. - Bun__closeAllSQLiteDatabasesForTermination: switch to sqlite3_close_v2. Plain sqlite3_close returns SQLITE_BUSY when unfinalized statements are still alive and leaves the connection open, which would leak it once the pointer is nulled; close_v2 defers the actual free until the remaining statements are finalized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/ErrorCode.rs`:
- Around line 706-710: The error code index alignment between Rust and
TypeScript is broken. In the Rust ErrorCode.rs file, the SQLITE_ERROR constant
is defined at index 324 with a total COUNT of 325 entries, but the TypeScript
errors array only contains 321 entries with SQLITE_ERROR at index 320, and is
additionally missing 51 error codes that exist in Rust's CODE_STR. Update the
TypeScript errors array to include all 325 error code entries in the exact same
order as they appear in Rust's CODE_STR constant, ensuring that SQLITE_ERROR
ends up at index 324 to maintain the index-alignment invariant required by the
C++ memory-safety constraint referenced in the code comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 350fc0c6-eff9-4d06-bc58-2d58738e9b44
📒 Files selected for processing (4)
src/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/sqlite/JSSQLStatement.cppsrc/jsc/bindings/sqlite/NodeSqlite.h
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a large feature PR adding an entire new built-in module (node:sqlite) with ~4k lines of new native C++ JSC bindings, GC/IsoSubspace wiring, a bundled SQLite version bump, and module-resolver changes across Rust/Zig/C++ — it warrants a human review.
Extended reasoning...
Overview
This PR implements the node:sqlite built-in module end-to-end and brings it to parity with Node v26.3.0's test suite. It touches 47 files, including:
- New native bindings:
NodeSqlite.h(758 lines) declaring six newJSDestructibleObjectcell types (JSDatabaseSync,JSStatementSync,JSStatementSyncIterator,JSNodeSqliteSession,JSNodeSqliteLimits,JSNodeSqliteTagStore) plus their prototypes/constructors, andNodeSqlite.cpp(not shown in the diff header but referenced as a new no-unify TU) implementing them. - GC/VM wiring: six new
LazyClassStructuremembers inZigGlobalObject.h, sixinitLaterblocks infinishCreation, and matchingIsoSubspace/GCClient::IsoSubspaceslots inDOMIsoSubspaces.h/DOMClientIsoSubspaces.h. - Module registration:
HardcodedModule.rs,HardcodedModule.zig,_NativeModule.h,NodeModuleModule.cpp,isBuiltinModule.cpp,NodeSqliteModule.h. - Bundled SQLite bump 3.53.0 → 3.53.2: large amalgamation diff in
sqlite3.c/sqlite3_local.h(upstream-generated), plus six new compile-timeSQLITE_ENABLE_*flags and making the sqlite dep unconditionally built. bun:sqliteteardown fix:Bun__closeAllSQLiteDatabasesForTerminationnow usessqlite3_close_v2and nulls the handle.node:testshim: a minimalMockTracker/MockFunctionContextso vendored Node tests usingt.mock.fn()work.- Error-code table append:
ERR_SQLITE_ERRORadded at the end ofErrorCode.tsand mirrored inErrorCode.rs(index 324, COUNT bumped to 325). - Tests: 19 vendored upstream
test-sqlite-*files, a 1100-line Bun-native test, common/index.{js,mjs} helpers, and a regression-fixture swap fromnode:sqlite→node:quic.
Security risks
The implementation surface is memory-safety-sensitive: raw sqlite3* / sqlite3_stmt* / sqlite3_session* ownership across GC-managed wrappers, re-entrancy guards (BusyScope) protecting against close() from inside UDF/authorizer/option-getter callbacks, ABA-style open-generation tracking, and buffer-detach defenses around applyChangeset/deserialize. loadExtension is gated on allowExtension per Node semantics. The header design is careful and well-commented (e.g., sessions freed before sqlite3_close_v2, deleteTrackedSessions() invariant, structure-cache invalidation on schema change), and the test suite explicitly exercises several UAF/double-free scenarios under ASAN. I did not spot a concrete vulnerability, but the .cpp implementation is large and not fully visible in the diff, and any of these invariants being subtly wrong is a heap-UAF in production.
The SQLite 3.53.2 bump is an upstream patch release and includes the session-extension hardening the PR description calls out; the amalgamation diff is mechanical.
Level of scrutiny
High. This is a brand-new public API surface backed by thousands of lines of new native code that manages C resource lifetimes against a concurrent GC, wires new IsoSubspaces, and changes the module-resolution tables that every require() consults. It also incorporates another open PR (#29821) by merge. None of this is mechanical or pattern-following; it's exactly the kind of change a maintainer should read.
Other factors
- The bug-hunting system found nothing.
- My earlier inline nit (
require('../common/index.mjs')intest-sqlite-config.js) was answered — the file is byte-identical to upstream Node v26.3.0, intentionally kept verbatim. - The author already addressed two CodeRabbit findings in commit 4481f87 (
BusyScopemade non-copyable/non-movable; termination path switched tosqlite3_close_v2). The remaining CodeRabbit comments (mock.fn input validation, mock.reset/restoreAll no-ops, and the explicit per-test timeout on the worker-based session test) are minor and the author can decide whether to act on them. - Test coverage is extensive (319 upstream subtests passing per the description, plus a dedicated Bun suite covering the GC/re-entrancy edge cases), which raises confidence but doesn't substitute for a human pass over the native lifetime management and the GC visit/subspace plumbing.
…eck] Bundling sqlite3.c on macOS (previously dlopen-only there) grows the darwin binaries by about 1.8 MB. node:sqlite needs the bundled build: Apple's system libsqlite3 lacks the session extension and percentile() and disables extension loading. Node.js bundles SQLite for the same reason. The [skip size check] tag marks the increase as intentional so the binary-size step records it without failing the build.
|
CI status after the error-code alignment fix (build #63406): 282/286 jobs passed. Notes on the remaining 4 failed jobs — none are sqlite failures:
|
….test.ts [skip size check] - NodeSqlite.h: the kNodeSqliteLimitCount comment described a static_assert against SQLITE_N_LIMIT-1 that doesn't exist (that macro is internal to sqlite3.c); describe the actual assert, which pins the SQLITE_LIMIT_* id range. - node-sqlite.test.ts: import builtinModules at module scope instead of an inline require, and stop asserting stderr is exactly empty in the VM teardown regression test (ASAN/debug builds emit benign noise; the invariant is no ASAN report and exit code 0).
|
Build #63424 (latest commit): 281/286 jobs passed; the binary-size step now soft-fails as an intentional, documented increase. The 4 remaining job failures are all pre-existing or flaky and unrelated to this PR — puppeteer's Chrome download failing on the darwin-26 runner (next-pages tests), the pre-existing test-tls-client-destroy-soon on darwin-14, the cross-branch test-net-connect-memleak GC flake on Alpine, and a grpc-js flake. I've retried those 4 jobs to get a clean run. |
… sessions on failed deserialize [skip size check] Three lifetime fixes from review: - function()/aggregate() callbacks are now rooted in a GC-traced vector on the DatabaseSync cell (visited under the cell lock) instead of JSC::Strong roots inside the sqlite-owned contexts, so a callback closure that captures the database no longer pins the connection forever — same approach as the existing authorizer field. The contexts hold raw pointers and are destroyed by xDestroy before the cell goes away. - Sessions now share a refcounted record between the DatabaseSync and the JS wrapper. A wrapper swept without close() flags the record (its destructor must not call into SQLite or touch the database cell), and the database frees the orphaned native session at its next entry point or on close, so dropped sessions stop recording writes instead of leaking until close(). - deserialize() frees tracked sessions only after sqlite3_deserialize() succeeds. On failure the connection is unchanged, so existing sessions keep their recorded history (statements are still invalidated first, matching Node). Session staleness is now derived from the shared record rather than the open generation. Adds GC-lifetime regression tests for all three.
… [skip size check] Re-registering a function/aggregate name made the previous callback stay rooted in m_registeredCallbacks until close(). The registration contexts now remember their slot(s); xDestroy clears them under the cell lock and new registrations reuse cleared slots, matching Node where the superseded callback is released by xDestroy. Also add the missing Bun.gc(true) after db.close() in the file:-URL test so the zombie connection can't hold the temp file open on Windows.
…te, not from xDestroy [skip size check] With unfinalized statements the connection is zombified and xDestroy runs from the last sqlite3_finalize() — possibly after the JSDatabaseSync cell was swept, or after the database was reopened and the slot reused — so xDestroy must never touch the cell. Revert it to a plain delete and release the superseded roots in function()/aggregate() instead: the cell keeps a small (name, SQL arg count) → slots table, the registration path clears the replaced registration's slots after sqlite3_create_*function succeeds, and new values reuse the cleared slots. Adds a regression test for the deferred-teardown case (close with an outstanding statement, reopen, re-register, then finalize the old statement via GC).
…eleasing superseded roots [skip size check] sqlite3_create_function_v2 replaces registrations case-insensitively, so releaseSupersededRegistration() must compare names with equalIgnoringASCIICase or a differently-cased re-registration keeps the old callback rooted until close(). Also reword the three comments that still described the reverted xDestroy-clears-slot mechanism, and cover the mixed-case re-registration in the test.
…[skip size check] staticSqlite now only controls whether bun:sqlite dlopens the system library on macOS; the bundled sqlite3.c is compiled on every platform for node:sqlite.
…ip size check] A workload that prepares once and then only runs statements never hits a DatabaseSync-level entry point, so orphaned sessions kept recording until close(). BUSY_SCOPE_STMT now gives the deferred sweep a chance (a flag check on the fast path) before marking the connection busy, and the header comment describes the actual set of sweep points.
… check] Statement iterators and the tag-store entry points construct BusyScope directly rather than through BUSY_SCOPE_STMT, so they still skipped the deferred orphan sweep. Move the sweep into BusyScope's constructor (every connection entry point takes one, and the existing depth/flag guard keeps re-entrant calls and the no-orphan fast path free) and drop the per-site calls it replaces.
|
Build #63467 (latest commit, all review feedback applied): 281/286 jobs passed. The binary-size step soft-fails as the documented intentional increase, and the 4 hard-failed jobs are the same recurring flaky/pre-existing tests already triaged earlier in this PR — test-net-connect-memleak.js on the two Alpine shards (GC-timing flake also seen on unrelated branches), test-tls-client-destroy-soon.js on darwin-14 (pre-existing, also fails on main), and napi.test.ts on Windows (known flaky suite; this PR doesn't touch N-API). I've retried those 4 jobs to get a clean run. |
…ct changesets detached by option getters [skip size check] - StatementSyncIterator.return() now checks the reset generation like next() does, so an invalidated iterator's IteratorClose can no longer rewind a newer iterator's cursor on the same statement. - applyChangeset() rejects the input if an options getter detached it (same guard deserialize() already has) instead of silently applying an empty changeset and reporting success. Both come with regression tests.
There was a problem hiding this comment.
No further issues found after the latest fixes — all prior findings look addressed. Deferring to a human reviewer given the scope (~4.5k lines of new GC-sensitive JSC bindings in NodeSqlite.{cpp,h}) and the macOS binary-size trade-off you flagged.
Extended reasoning...
Overview
This PR implements the node:sqlite built-in module end-to-end: ~3.7k lines of new C++ in NodeSqlite.cpp plus an 821-line header defining six JSC cell types (JSDatabaseSync, JSStatementSync, iterator, session, limits, tag-store), lazy class-structure / IsoSubspace wiring in ZigGlobalObject, module registration across the Rust/Zig/C++ resolver tables, a bundled SQLite 3.53.0→3.53.2 amalgamation bump, six new compile-time feature defines, an always-on sqlite build (previously macOS dlopen-only), a minimal node:test mock.fn() shim, process.versions.sqlite, a new ERR_SQLITE_ERROR code, and ~6k lines of vendored Node v26.3.0 tests plus ~1.3k lines of Bun-authored tests.
Security risks
The implementation handles native resource lifetimes (sqlite3*, sqlite3_stmt*, sqlite3_session*) across JSC GC boundaries with re-entrant JS callbacks (UDFs, authorizers, filter/conflict handlers, option getters). Over the review cycle I flagged and the author fixed four memory-safety issues in this area: a Strong<>-rooted UDF cycle leak, an orphaned-session unbounded-growth leak, a deferred-xDestroy use-after-free on zombified connections, and a stale-iterator return() that silently rewound a live cursor. Buffer-detachment via hostile option getters is now guarded in both deserialize() and applyChangeset(). Extension loading is gated behind allowExtension; the authorizer hook is exposed. These are exactly the surfaces a human should re-read in NodeSqlite.cpp — the diff omits that file's body due to size.
Level of scrutiny
High. This is a large new native module with subtle GC/finalizer ordering invariants, and the review history demonstrates that getting those invariants right took several iterations. Separately, the author explicitly raised a policy question for reviewers: bundling sqlite3.c on macOS grows the darwin binaries by ~1.8 MB (documented and [skip size check]-tagged), and they noted they're "happy to revisit if reviewers prefer a different trade-off."
Other factors
All 15+ inline findings I raised across eight review rounds have been applied and resolved; the latest revision (commits through 4d19528) produced no new findings from the bug-hunting system. CI on the most recent build is green except for documented pre-existing/flaky failures unrelated to this change. Test coverage is extensive (319 vendored Node subtests passing plus Bun-authored regression tests for each fixed lifetime bug). The remaining work is human sign-off on the implementation design, the lifetime-management approach in NodeSqlite.cpp, and the binary-size trade-off.
|
Final-state build #63476: 282/286 jobs passed; binary-size soft-fails as the documented intentional increase, and the only 3 hard-failed jobs are the recurring flakes already triaged (terminal.test.ts PTY timeout on darwin — also failing on 10+ unrelated branches — and test-net-connect-memleak.js on the two Alpine shards). Retried those 3 jobs to get the build green. Automated review has signed off ("no further issues"); ready for human review. |
|
Retry outcome on build #63476: the three flaky jobs failed again on the same tests, so the build stays at 282/286 with those two flake classes. For the record on why they're not from this PR: test-net-connect-memleak.js asserts a dropped socket is collected after one gc() — with conservative stack scanning that's sensitive to binary layout, it isn't exercising any sqlite code (node:sqlite isn't loaded there), it passed on the same Alpine shards on build #63456 of this PR, and it also fails on unrelated branches (e.g. #63365, #63344). The terminal.test.ts PTY timeout on darwin appears on 10+ recent unrelated branches. Happy to dig further if a maintainer suspects either is real. |
3 participants
What
Adds the
node:sqlitemodule and brings the full Node v26.3.0test-sqlite-*suite into the repo, passing.This builds on #29821 (merged into this branch unchanged): native
DatabaseSync,StatementSync,backup(), sessions/changesets, custom scalar/aggregate/window functions, the authorizer, andprocess.versions.sqlite, all implemented in C++ against the bundled sqlite3 amalgamation. On top of that, this PR adds what was still missing to actually ship it on current main and to match Node v26.3.0:src/resolve_builtins/HardcodedModule.rs): the module resolver no longer reads the Zig tables, sonode:sqliteis registered there as a prefix-only builtin (same treatment asnode:test). This is the line that makesrequire("node:sqlite")resolve.scripts/update-sqlite-amalgamation.sh 3530200 2026). 3.53.2 contains the session-extension hardening (SQLite check-ine807d4e379) that makessqlite3changeset_apply()returnSQLITE_CORRUPTon malformed UPDATE changesets instead of dereferencing a NULLsqlite3_value*. Node v26.3.0 ships 3.53.1 plus a cherry-pick of the same check-in, andtest-sqlite-session.jsexercises it.SQLITE_ENABLE_PERCENTILEadded to the sqlite build defines, matching Node's compile-time feature set, so thepercentile()/median()SQL functions exist (asserted by upstreamtest-sqlite.js).test-sqlite-database-sync.js,test-sqlite-backup.mjs, andtest-sqlite-session.jsrefreshed to the upstream copies (using-based cleanup, the backup keep-alive GC test, the malformed-changeset test);test-sqlite-database-sync-dispose.jsremoved because upstream merged that suite intotest-sqlite-database-sync.js; thepercentile is enabledskip removed.common/index.jsnow treats--experimental-sqlite/--no-experimental-sqliteas no-ops under Bun instead of re-spawning the test, so upstream// Flags:headers stay verbatim.Test results (debug build, Linux x64)
test/js/node/test/parallel/test-sqlite-*.{js,mjs}files pass when run the way CI runs them (bun test --config=bunfig.node-test.toml): 319 subtests pass, 0 fail. Node v26.3.0 has no sqlite tests undertest/sequential/.online-timing in the busy-timeout test, the error shape forrequire("sqlite")without thenode:prefix, the--no-experimental-sqliteflag, and one upstream-conditional skip in the backup test.test/js/node/sqlite/node-sqlite.test.ts(37 tests),test/js/bun/sqlite/,test/js/sql/sqlite-sql.test.ts,test/js/node/process/process.test.js,test/js/node/module/node-module-module.test.js, andtest/regression/issue/25707.test.tspass with the bumped SQLite.Not vendored on purpose:
test-permission-sqlite-load-extension.js(requires Node's--permissionmodel) andtest-webstorage-without-sqlite.js(covers Node's webstorage global, notnode:sqlite).Binary size
Bundling sqlite3.c on macOS (previously dlopen-only there) grows the darwin binaries by ~1.8 MB; Linux/Windows grow ~200 KB from the added feature defines. This is required for node:sqlite parity (Apple's system libsqlite3 lacks the session extension and percentile() and disables extension loading) and matches what Node.js does by bundling SQLite.
Test plan
bun test --config=bunfig.node-test.toml ./test/js/node/test/parallel/test-sqlite-session.jspasses (covers the malformed-changeset fix from the SQLite bump)bun test test/js/node/sqlite/node-sqlite.test.tsandbun test test/js/bun/sqlite/pass (regression coverage for the bundled SQLite update)Fixes #31402
Fixes #20412
(Not marking #24255 as fixed: this PR only adds the minimal
mock.fn()tracker the sqlite tests need, not the fullnode:testMockTracker API.)