From ec718ddf60896376f28d1ffcf67a8f052d3eec63 Mon Sep 17 00:00:00 2001 From: Team AppThreat Date: Tue, 15 Sep 2026 01:36:06 +0100 Subject: [PATCH 1/3] fix: address the six issues reported from vdb-node's 9.1 port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Prebuild libc tagging (high, silent wrong-version loads). node-gyp-build resolves libc to 'glibc' on every non-Alpine platform, so a *.glibc.node inside prebuilds/darwin-*/ outranks the untagged binary beside it and a stale build loads in preference to the current one, silently. `pnpm run prebuild` now goes through tools/prebuild.mjs, which forwards --tag-libc on linux and drops it elsewhere; tools/check-prebuilds.mjs (a CI step in ci.yml, electron-asar.yml and the container build) refuses a libc-tagged binary outside linux-* and a binary whose object format contradicts its directory; and the addon now exports NATIVE_INTERFACE_VERSION, which lib/sqlite3-binding.js refuses to load a mismatch of — naming the file and the remedy instead of presenting as missing methods. 2. Rebasing: the test asserted a semantic its own code never exercised, and had the topology backwards. Verified against the SQLite source and empirically: the buffer belongs to the apply performed on *your* database, entries match by primary key, and the change's old.* values are rewritten to the peer's — so the changeset recorded *before* the apply lands upstream with no conflict handling at all. Rewrote the tests around that loop, added one pinning the by-primary-key rule (and why later work needs its own round), and corrected the README section. 3. compat DatabaseSync.close() swallowed SQLITE_BUSY, so a connection with a leaked statement never closed. It now finalizes the statements it prepared (as node:sqlite does, tracked weakly so GC still collects them), reports a close that fails anyway on the connection's 'error' event, and asyncDispose finalizes too. 4. subscribeQueries spans arrive asynchronously and unsubscribe lost them. Fixed rather than only documented: Database::_flushProfile drains the profile queue synchronously, sqlite3.flushQuerySpans() exposes it, and unsubscribe() drains before detaching. 5. Minor items: the post-abort drain (`await db.wait()`) is documented and tested, MIGRATING-TO-V9.md gained a 9.0 → 9.1 section (parameterNames, close()/unfinalized statements, span delivery), and the README documents sqlite3.complete()/compileOptions(). 6. Pool workers opened without OPEN_URI, so the documented `file:…?mode=ro` form failed every worker with SQLITE_CANTOPEN. They now set it for URI filenames; a read-only URI drops the WAL default (and refuses an explicit walMode: true), and an in-memory URI is refused with readers > 0 like ':memory:' already was. Signed-off-by: Team AppThreat --- .github/workflows/ci.yml | 8 + .github/workflows/electron-asar.yml | 2 + MIGRATING-TO-V9.md | 51 +++++++ README.md | 83 ++++++++-- docs/concurrency.md | 18 +++ docs/install.md | 35 ++++- lib/compat.d.ts | 13 +- lib/compat.js | 114 ++++++++++++-- lib/native.d.ts | 35 +++++ lib/pool.d.ts | 13 +- lib/pool.js | 86 ++++++++++- lib/sqlite3-binding.js | 46 ++++++ lib/sqlite3.d.ts | 1 + lib/sqlite3.js | 67 +++++++++ lib/worker.js | 16 +- package.json | 3 +- src/async.h | 9 ++ src/database.cc | 20 +++ src/database.h | 8 + src/node_sqlite3.cc | 14 ++ test/abort.test.js | 29 ++++ test/compat.test.js | 87 +++++++++++ test/diagnostics.test.js | 41 +++++ test/pool.test.js | 68 +++++++++ test/prebuild_layout.test.js | 225 ++++++++++++++++++++++++++++ test/session_rebase.test.js | 168 ++++++++++++++++----- tools/BinaryBuilder.Dockerfile | 2 + tools/check-prebuilds.mjs | 154 +++++++++++++++++++ tools/prebuild.mjs | 103 +++++++++++++ 29 files changed, 1448 insertions(+), 71 deletions(-) create mode 100644 test/prebuild_layout.test.js create mode 100644 tools/check-prebuilds.mjs create mode 100644 tools/prebuild.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d460f22..5b6cdaf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -195,8 +195,16 @@ jobs: echo "CXXFLAGS=${CXXFLAGS:-} -include ../src/gcc-preinclude.h" >> $GITHUB_ENV - name: Build binaries + # tools/prebuild.mjs forwards --tag-libc on linux and drops it + # elsewhere: a libc-tagged binary in a darwin-*/win32-* prebuilds + # directory outranks the untagged one at load time (node-gyp-build + # reads libc as 'glibc' on every non-Alpine platform), so a stale + # copy would be preferred silently. run: pnpm run prebuild --arch ${{ env.TARGET }} --tag-libc + - name: Verify prebuild layout + run: pnpm run check:prebuilds + - name: Print binary info if: contains(matrix.os, 'ubuntu') run: | diff --git a/.github/workflows/electron-asar.yml b/.github/workflows/electron-asar.yml index 2e9daf0..159994a 100644 --- a/.github/workflows/electron-asar.yml +++ b/.github/workflows/electron-asar.yml @@ -48,6 +48,8 @@ jobs: echo "CXXFLAGS=${CXXFLAGS:-} -include ../src/gcc-preinclude.h" >> $GITHUB_ENV - name: Build the shipped prebuild run: pnpm run prebuild --tag-libc + - name: Verify prebuild layout + run: pnpm run check:prebuilds - name: Install xvfb run: sudo apt-get update && sudo apt-get install -y xvfb - name: Packaged-ASAR tests (unpacked + sealed-in-archive) diff --git a/MIGRATING-TO-V9.md b/MIGRATING-TO-V9.md index 22fb08c..3bb9cc0 100644 --- a/MIGRATING-TO-V9.md +++ b/MIGRATING-TO-V9.md @@ -52,6 +52,14 @@ run to completion; its result is dropped. The signal listener is removed when the call settles, so one long-lived signal does not accumulate listeners. +The rejection is delivered when the signal fires, which is before the +interrupted statement has finished unwinding on its worker — so the +connection is not idle yet at that moment and a synchronous method called +there refuses with "database is busy: sync methods require a fully idle +database". `await db.wait()` in the catch block drains the teardown. A +cancellation token's own rejection (`token.cancel()`) arrives after the +unwind and needs no drain. + ## User-defined functions, aggregates, window functions and collations (new) `db.function()`, `db.aggregate()`, `db.collation()`, `db.removeFunction()` @@ -344,6 +352,49 @@ fixes it (the original `node-gyp-build` error is preserved as `err.cause`). node-webkit support and its build instructions were removed. See [docs/electron.md](docs/electron.md). +## Within v9: 9.0 → 9.1 behaviour changes + +Small, but they change what working 9.0 code observes: + +- **`stmt.parameterNames` on a fully positional statement is now + `undefined`.** 9.0 returned an array of `null`s, one per `?` + parameter. A statement with no named parameters at all has no names to + report, so it reports none; a **mixed** statement still carries `null` + at each positional index, so indices stay aligned with + `parameterCount`: + + ```js + db.prepareSync("SELECT * FROM t WHERE a = ? AND b = ?").parameterNames; + // 9.0: [null, null] 9.1: undefined + db.prepareSync("SELECT * FROM t WHERE a = ? AND b = $b").parameterNames; + // [null, '$b'] in both + ``` + + Code that iterated the array unconditionally needs a `?? []`. + +- **`db.close()` refuses while a statement is unfinalized.** This is + unchanged behaviour, called out here because it is the first thing that + bites code ported from `node:sqlite` or `better-sqlite3`, where a + garbage-collected statement is finalized for you: + + ```js + const stmt = db.prepareSync("SELECT 1"); + await db.close(); // SQLITE_BUSY: unable to close due to unfinalized statements + ``` + + Finalize the statement (`await stmt.finalize()`, or `using stmt = ...` + for scope-bound disposal) before closing. The statement cache and the + `*Sync` fast paths manage their own statements, so this only concerns + statements you prepared explicitly. The `/compat` shim's + `DatabaseSync.close()` finalizes the statements it prepared for you, as + `node:sqlite` does — since 9.1, where it previously discarded the + resulting `SQLITE_BUSY` and left the connection open. + +- **Query spans are delivered asynchronously**, and `sqlite3.flushQuerySpans()` + is the new drain point — see the observability section of the README. + 9.0 code that read collected spans immediately after an awaited query + was seeing nothing; `unsubscribe()` now drains before it detaches. + ## Minor notes - `Date` still binds as epoch milliseconds (REAL) and reads back as a diff --git a/README.md b/README.md index 8f31031..614164b 100644 --- a/README.md +++ b/README.md @@ -567,6 +567,26 @@ reaches every statement running on the connection. While a token exists, each query pays one relaxed atomic load per `period` VM instructions (default 1000) — within measurement noise in the benchmark suite. +One timing detail worth knowing: an `AbortSignal` rejection is delivered +as soon as the signal fires, which is *before* the interrupted statement +has finished unwinding on its worker. The connection is therefore not yet +idle at the moment the rejection is observed, so a synchronous method +called right there refuses with "database is busy: sync methods require a +fully idle database". `await db.wait()` (or any awaited query) drains the +teardown: + +```js +try { + await db.all(longRunningSql, { signal }); +} catch (err) { + await db.wait(); // the interrupted statement has finished unwinding + db.allSync("SELECT 1"); // now the sync path is available again +} +``` + +A cancellation token's own rejection (`token.cancel()` with no signal) +arrives with the statement already unwound, so it needs no drain. + A JavaScript callback form exists for progress reporting — `db.progress(10000, () => shouldStop)` calls the callback every 10,000 VM instructions and aborts the statement when it returns truthy — but @@ -637,24 +657,44 @@ const both = sqlite3.concatChangeset(a, b); // a then b ### Rebasing (9.1): the fork-free sync loop Apply with `{ rebase: true }` to harvest a **rebase buffer** — the -record of which conflicting changes were omitted or replaced — then -rebase later local changesets against it before applying them remotely. +record of which conflicting changes this database omitted or replaced — +then rebase the changesets you recorded *before* that apply, so they +land upstream without anyone resolving the same conflicts twice. Together with `session.diff()` (a changeset of the differences between an attached database's table and this one, without recording anything) this is a complete offline-first sync toolkit on stock SQLite; no other JS driver has rebasing (rusqlite is the only binding anywhere): ```js -// one round of sync: apply the server's changes, remember the conflicts -const rebase = await serverDb.applyChangeset(serverChangeset, { - conflict: "omit", +// This database is at S0 and records its own work (S0 → S1): +const session = db.session({ table: "t" }); +await db.run("UPDATE t SET v = ? WHERE id = ?", "mine", 1); +const local = await session.changeset(); +await session.close(); + +// A changeset based on S0 arrives from the peer. Apply it *here*, +// resolving conflicts, and keep the record of those resolutions: +const rebase = await db.applyChangeset(incoming, { + conflict: "omit", // this database's row wins rebase: true, // resolves the rebase buffer (null if no conflicts) }); -// later: rebase the client's new work against those resolutions and push -const rebased = sqlite3.rebaseChangeset(clientChangeset, rebase); -await serverDb.applyChangeset(rebased); + +// Rebasing rewrites the local changeset's old values to the ones the +// peer holds, so pushing it needs no conflict handling at all: +await peer.applyChangeset(sqlite3.rebaseChangeset(local, rebase)); ``` +The matching rule, because it is easy to get backwards: the rebaser +finds a buffer entry **by primary key** and rewrites the change's +`old.*` values to the values the buffer carries (for an omitted remote +UPDATE, the values that remote left in place). It does not check when +the change was recorded — so a changeset recorded *after* the apply is +rewritten just the same, and its old values then describe a state the +peer has already moved past. **One buffer belongs to the changesets +recorded before its apply**; later work gets its own round of the loop. +The direction matters too: the buffer must come from the apply performed +on the database whose changeset you are rebasing. + `session.diff('t', 'other')` records the changes that transform the attached database `other`'s table into this connection's — the "what changed between these two databases" primitive for verification @@ -780,6 +820,10 @@ catch (err) { err.offset; } // 9 // per-statement integer mode const stmt = db.prepareSync(sql, { integerMode: "bigint" }); + +// connection-free namespace helpers +sqlite3.complete("SELECT 1;"); // true — a complete statement (REPL input) +sqlite3.compileOptions(); // ['ENABLE_FTS5', 'ENABLE_SESSION', …] ``` **JavaScript virtual tables** — generator-computed, read-only, working @@ -846,6 +890,22 @@ const unsubscribe = sqlite3.subscribeQueries(({ sql, durationMs }) => { }); ``` +Spans are delivered **asynchronously**: SQLite reports a statement's +timing on the thread that ran it, and the span crosses to the JS thread +through a queue drained on a later event-loop turn — so right after +`await db.all(sql)` the span for that query has usually not arrived yet. +`sqlite3.flushQuerySpans()` delivers everything pending, synchronously, +which is what a test or a shutdown flush wants; `unsubscribe()` drains +first as well, so nothing recorded before it is lost. + +```js +const spans = []; +const stop = sqlite3.subscribeQueries((s) => spans.push(s.sql)); +await db.all("SELECT 1"); +sqlite3.flushQuerySpans(); // spans === ['SELECT 1'] +stop(); +``` + **node:sqlite drop-in** — code written against the built-in module can switch without rewriting: @@ -860,9 +920,10 @@ The shim maps onto the sync fast path (re-entrant UDFs included) and exposes the full async surface through `db.native`. The places a synchronous form cannot exist keep this package's async signatures and are listed at the top of [lib/compat.js](lib/compat.js): sessions, -`serialize`, and `close()`, which starts the close rather than completing -it (`await using`, or `await db.native.close()`, when a caller must know -the file is free — deleting or reopening it on Windows). +`serialize`, and `close()`, which finalizes the statements it prepared +(as `node:sqlite` does) and then *starts* the close rather than +completing it (`await using`, or `await db.native.close()`, when a caller +must know the file is free — deleting or reopening it on Windows). `StatementSync.iterate()` materialises its rows, because the sync path has no mid-cursor suspension. diff --git a/docs/concurrency.md b/docs/concurrency.md index 7184fe4..b5f9faa 100644 --- a/docs/concurrency.md +++ b/docs/concurrency.md @@ -171,6 +171,24 @@ Error's own properties. transaction finishes or rolls back), closes every connection and waits for every worker's exit. `await using pool` works. +**`file:` URI filenames** work in the pool as they do on a single +connection: the workers set `OPEN_URI` when the filename starts with +`file:`, so `mode`, `immutable` and `cache` are honoured (before 9.1 the +URI reached SQLite as a literal path and every worker failed with +`SQLITE_CANTOPEN`). Two consequences worth knowing: + +```js +// Read-only over a shipped database — WAL is a write, so the pool's WAL +// default is dropped for a read-only URI rather than failing the open. +const ro = await sqlite3.pool("file:/data/vdb.sqlite?mode=ro", { readers: 2 }); +// Asking for WAL explicitly on such a URI is refused up front: +await sqlite3.pool("file:/data/vdb.sqlite?mode=ro", { walMode: true }); // TypeError +``` + +An in-memory URI (`file::memory:`, `mode=memory`) is refused with +`readers > 0` for the same reason `:memory:` is — each worker would get +its own empty database — unless the URI carries `cache=shared`. + ## Terminating a worker `worker.terminate()` while a query is in flight used to abort the whole diff --git a/docs/install.md b/docs/install.md index 7783f06..127bd22 100644 --- a/docs/install.md +++ b/docs/install.md @@ -44,6 +44,33 @@ which looks in `prebuilds//` first, then falls back to a `build/Release/` build. (For `--tag-libc` builds the directory stays `linux-` and the libc is carried by the file suffix.) +### The libc tag belongs to linux only + +`node-gyp-build` resolves the running libc as `'glibc'` on **every +non-Alpine platform, macOS and Windows included**, and prefers a +tag-matching file over an untagged one. So a +`prebuilds/darwin-arm64/@appthreat+sqlite3.glibc.node` matches the +platform *and* the libc, outranks the `@appthreat+sqlite3.node` beside it, +and is loaded in preference to it — even when it comes from an entirely +different revision. That failure is silent: the addon loads, and only the +APIs added since are missing. + +Three things prevent it: + +- `pnpm run prebuild` goes through `tools/prebuild.mjs`, which forwards + `--tag-libc` on linux and drops it everywhere else. CI passes the flag + for every target; the platform rule lives in the wrapper so a local + build cannot recreate the trap either. +- `pnpm run check:prebuilds` (a CI step after every prebuild) fails when a + `darwin-*`/`win32-*` directory contains a `*.glibc.node`/`*.musl.node`, + or when a binary's object format contradicts its directory (a Mach-O + file in `linux-x64/`). +- The loader refuses a binding whose `NATIVE_INTERFACE_VERSION` is not the + one `lib/` expects, naming the file it loaded and the + `rm -rf prebuilds build && pnpm run rebuild` remedy. The two constants + live in `src/node_sqlite3.cc` and `lib/sqlite3-binding.js` and are + bumped together whenever `lib/` starts using a new native export. + ## pnpm 10+ and the blocked install script pnpm 10 and later refuse to run a dependency's lifecycle scripts unless the @@ -154,7 +181,10 @@ Error: No native build was found for platform=linux arch=arm64 runtime=node ... `pnpm run rebuild`. 3. **Stale `prebuilds/` while iterating on C++**: `node-gyp-build` prefers `prebuilds/` over `build/`, so your `pnpm run rebuild` output is being - shadowed. Delete `prebuilds/` while iterating. + shadowed. Delete `prebuilds/` while iterating. Since 9.1 a mismatch + between a resolved binary and `lib/` throws at import ("loaded a native + binding that does not match this JavaScript"), naming the file — rather + than presenting as methods missing from the namespace. 4. **Runtime below the Node-API floor** (Node < 22, Electron < 35): the binding loader refuses with an error naming the floors rather than crashing. On Electron the default prebuild needs no rebuild at all; only @@ -171,7 +201,8 @@ This repo is developed with **pnpm >= 11** (pinned exactly in pnpm install # strictDepBuilds is on; frozen form: pnpm install --frozen-lockfile pnpm run rebuild # node-gyp rebuild — always `pnpm run rebuild` pnpm run test -pnpm run prebuild # prebuildify --napi --strip +pnpm run prebuild # tools/prebuild.mjs → prebuildify --napi --strip +pnpm run check:prebuilds # refuses a prebuilds/ layout that loads the wrong file pnpm pack # tarball includes prebuilds/ — smoke-test it in a scratch project ``` diff --git a/lib/compat.d.ts b/lib/compat.d.ts index 85559e3..469e22e 100644 --- a/lib/compat.d.ts +++ b/lib/compat.d.ts @@ -150,8 +150,17 @@ export declare class DatabaseSync { ): Promise; /** Serializes the database (async divergence). */ serialize(): Promise; - /** Closes the connection (asynchronously under the hood). */ + /** + * Closes the connection, finalizing the statements prepared through + * it (as node:sqlite does). The close itself is queued: the + * connection refuses further work at once, the handle is released a + * turn later. A close that still fails is reported on the underlying + * connection's 'error' event. + */ close(): void; - /** `await using` support. */ + /** + * `await using` support: finalizes outstanding statements, then waits + * for the close to complete. + */ [Symbol.asyncDispose](): Promise; } diff --git a/lib/compat.js b/lib/compat.js index eacfadb..c8aa90b 100644 --- a/lib/compat.js +++ b/lib/compat.js @@ -26,10 +26,14 @@ // - serialize() returns the bytes asynchronously. // - StatementSync.close() finalizes asynchronously under the hood; the // statement is unusable immediately after. DatabaseSync.close() is the -// same shape: it starts the close (the connection is unusable at once) -// and the handle is released on the queue — `await db[Symbol. +// same shape: it finalizes the statements prepared through it (as +// node:sqlite does) and starts the close — the connection is unusable +// at once, the handle is released on the queue. Use `await db[Symbol. // asyncDispose]()`, or `await db.native.close()`, when the close must -// have completed (deleting the file, reopening it on Windows). +// have completed (deleting the file, reopening it on Windows). A close +// that fails anyway — a statement prepared directly on `db.native` +// holds the connection — is reported on the connection's 'error' +// event, never discarded. // - StatementSync.iterate() materialises the rows before yielding: the // sync fast path has no mid-cursor suspension. Use // `db.native.iterate()` for a true streaming cursor. @@ -89,6 +93,8 @@ class StatementSync { #returnArrays; /** @type {(mode: 'number' | 'bigint') => import('./sqlite3-binding.js').Statement} */ #reprepare; + /** @type {(stmt: StatementSync) => void} */ + #onClose; /** * @param {import('./sqlite3-binding.js').Statement} stmt the wrapped statement. @@ -96,8 +102,10 @@ class StatementSync { * @param {(mode: 'number' | 'bigint') => import('./sqlite3-binding.js').Statement} [reprepare] * re-prepares the same SQL under another integer mode, for * setReadBigInts (the mode belongs to the prepared statement). + * @param {(stmt: StatementSync) => void} [onClose] tells the owning + * connection this statement no longer needs finalizing at close. */ - constructor(stmt, options, reprepare) { + constructor(stmt, options, reprepare, onClose) { this.#stmt = stmt; this.#readBigInts = options?.readBigInts === true; this.#returnArrays = options?.returnArrays === true; @@ -108,6 +116,11 @@ class StatementSync { 'setReadBigInts() cannot re-prepare this statement', ); }); + this.#onClose = + onClose ?? + (() => { + // Unowned statement: nobody is tracking it for close. + }); } /** @@ -280,6 +293,7 @@ class StatementSync { /* best effort */ }); } + this.#onClose(this); } /** @@ -309,6 +323,21 @@ class DatabaseSync { /** @type {import('./sqlite3-binding.js').Database} */ #db; #allowExtension = false; + // Statements prepared through this connection and not yet closed. + // node:sqlite finalizes outstanding statements when the database + // closes, and code written against it relies on that: this package's + // close() instead fails with SQLITE_BUSY ("unable to close due to + // unfinalized statements") while one is live, so without this the + // connection would simply never close. Weakly held, because a + // statement nobody kept a reference to is finalized by GC on its own + // and pinning it here would be the leak the tracking is meant to + // avoid. + /** @type {Set>} */ + #statements = new Set(); + /** @type {FinalizationRegistry>} */ + #collected = new FinalizationRegistry((ref) => { + this.#statements.delete(ref); + }); /** * Opens a connection. Options map onto this package's opens and @@ -450,7 +479,51 @@ class DatabaseSync { const stmt = prepare( options.readBigInts === true ? 'bigint' : undefined, ); - return new StatementSync(stmt, options, prepare); + /** @type {StatementSync} */ + const wrapper = new StatementSync(stmt, options, prepare, (closed) => + this.#forget(closed), + ); + const ref = new WeakRef(wrapper); + this.#statements.add(ref); + this.#collected.register(wrapper, ref, wrapper); + return wrapper; + } + + /** + * Drops a statement from the close-time finalize list. + * + * @param {StatementSync} stmt the statement that closed itself. + * @returns {void} + */ + #forget(stmt) { + this.#collected.unregister(stmt); + for (const ref of this.#statements) { + const live = ref.deref(); + if (live === undefined || live === stmt) + this.#statements.delete(ref); + } + } + + /** + * Finalizes every statement still open on this connection, so that + * the close below is not refused with SQLITE_BUSY. + * + * @returns {void} + */ + #finalizeStatements() { + for (const ref of this.#statements) { + const stmt = ref.deref(); + if (stmt !== undefined && !stmt.finalized) { + // One statement refusing to finalize must not strand the + // rest — or the close. + try { + stmt.close(); + } catch { + /* keep going; close() reports what it cannot do */ + } + } + } + this.#statements.clear(); } /** @@ -587,29 +660,44 @@ class DatabaseSync { } /** - * Closes the connection. Divergence: the close is queued (this - * package's close is asynchronous), so the connection refuses further - * work at once but the file handle is released a turn later — use - * `await using` / `Symbol.asyncDispose` when a caller must know the - * file is free, e.g. before deleting or reopening it on Windows. + * Closes the connection, finalizing any statement prepared through it + * that is still open — node:sqlite's semantics, and required here: + * this package refuses to close a connection holding an unfinalized + * statement (`SQLITE_BUSY: unable to close due to unfinalized + * statements`). + * + * Divergence: the close is queued (this package's close is + * asynchronous), so the connection refuses further work at once but + * the file handle is released a turn later — use `await using` / + * `Symbol.asyncDispose`, or `await db.native.close()`, when a caller + * must know the file is free, e.g. before deleting or reopening it on + * Windows. A close that still fails (a statement prepared directly on + * `db.native`, say) is reported on the connection's `'error'` event + * rather than discarded; with no listener attached that surfaces as an + * uncaught error, which is the point — the connection stayed open. * * @returns {void} */ close() { if (this.#db.open) { - this.#db.close(function () { - /* sync-shaped: completes on the queue */ + this.#finalizeStatements(); + const db = this.#db; + db.close((err) => { + if (err) db.emit('error', err); }); } } /** - * `await using` support. + * `await using` support: finalizes outstanding statements, then waits + * for the close to complete (unlike {@link DatabaseSync.close}, which + * only starts it). * * @returns {Promise} resolves once closed. */ async [Symbol.asyncDispose]() { if (this.#db.open) { + this.#finalizeStatements(); await this.#db.close(); } } diff --git a/lib/native.d.ts b/lib/native.d.ts index d2f92aa..92c51bc 100644 --- a/lib/native.d.ts +++ b/lib/native.d.ts @@ -295,6 +295,21 @@ export declare class Database extends EventEmitter { */ interrupt(): this; + /** + * Delivers the `'profile'` events already recorded for this + * connection but not yet dispatched, synchronously. A finished + * statement's timing is captured on the thread that ran the statement + * and crosses to JS through a queue drained on a later loop turn, so + * a caller that has just awaited a query has a pending span, not a + * delivered one. `sqlite3.subscribeQueries()` drains through this + * before dropping a subscriber, and `sqlite3.flushQuerySpans()` + * exposes it. + * + * @returns nothing. + * @internal + */ + _flushProfile(): void; + /** * True while an exclusive operation (exec/close/wait/loadExtension) * is running or waiting on the queue. Used by the statement cache in @@ -1542,6 +1557,14 @@ declare const binding: { * `applyChangeset(..., { rebase: true })` — the client-server sync * primitive (sqlite3rebaser_*). * + * The buffer must come from the apply performed on the database whose + * changeset is being rebased, and `changeset` must be one recorded + * *before* that apply: entries are matched by primary key and the + * change's `old.*` values are rewritten to the ones the buffer + * carries, with no check on when the change was recorded. Rebasing + * later work against the same buffer therefore produces old values + * describing a state the peer has already moved past. + * * @param changeset the changeset to rebase. * @param rebase the harvested rebase buffer. * @returns the rebased changeset bytes. @@ -1568,6 +1591,18 @@ declare const binding: { */ compileOptions(): string[]; + /** + * The addon's native-interface revision. `lib/sqlite3-binding.js` + * refuses to load a binary reporting anything other than the number + * it expects, which is what turns "a stale .node was resolved ahead + * of the current build" into an error instead of silently missing + * methods. Not part of the supported surface; the number carries no + * meaning beyond equality. + * + * @since 9.1.0 + */ + readonly NATIVE_INTERFACE_VERSION: number; + /** * Installs the generator the addon uses to compile a row builder for * each result shape, so a row costs one call into JS instead of one diff --git a/lib/pool.d.ts b/lib/pool.d.ts index 0755a8d..f3ebf30 100644 --- a/lib/pool.d.ts +++ b/lib/pool.d.ts @@ -271,7 +271,13 @@ declare class SqlitePool { * docs/concurrency.md). With `readers: 0` a `:memory:` pool is fine: * everything runs on the single writer. * - * @param {string} filename the database file. + * A `file:` URI is opened as a URI (the workers set `OPEN_URI` for it), + * so `mode`, `immutable` and `cache` are honoured. A read-only URI + * (`mode=ro`, `immutable=1`) turns WAL off by itself, since enabling WAL + * is a write: pass `walMode: true` explicitly on such a URI and pool() + * refuses rather than failing every worker at `PRAGMA journal_mode`. + * + * @param {string} filename the database file, or a `file:` URI. * @param {PoolOptions} [options] the pool options. * @returns {Promise} the opened pool. * @throws {TypeError} when the filename is missing or malformed, or an @@ -285,6 +291,11 @@ declare class SqlitePool { * const rows = await pool.read('SELECT * FROM t WHERE a = ?', [1]); * await pool.write('INSERT INTO t (a) VALUES (?)', [2]); * await pool.close(); + * @example + * // A read-only pool over a shipped database, canonical URI form: + * const ro = await sqlite3.pool('file:/data/vdb.sqlite?mode=ro', { + * readers: 2, + * }); */ declare function pool(filename: string, options?: PoolOptions): Promise; export { pool, SqlitePool }; diff --git a/lib/pool.js b/lib/pool.js index 6b524e5..712c351 100644 --- a/lib/pool.js +++ b/lib/pool.js @@ -1154,6 +1154,45 @@ function parsePoolOptions(options) { return { readers, walMode, busyTimeout, integerMode }; } +/** + * Reads the parts of a SQLite URI filename the pool has to act on. + * + * Only the query parameters matter here (the path is the worker's and + * SQLite's business), and only three of them: `mode`, `immutable` and + * `cache`. A non-URI filename reports `uri: false` and nothing else. + * + * @param {string} filename the filename as given to pool(). + * @returns {{ uri: boolean, readonly: boolean, memory: boolean, + * sharedCache: boolean }} what the URI asks for. + * @private + */ +function parsePoolUri(filename) { + if (!/^file:/i.test(filename)) { + return { + uri: false, + readonly: false, + memory: false, + sharedCache: false, + }; + } + const q = filename.indexOf('?'); + const params = new URLSearchParams(q === -1 ? '' : filename.slice(q + 1)); + let path = filename.slice('file:'.length, q === -1 ? undefined : q); + try { + path = decodeURIComponent(path); + } catch { + // Left as written: a malformed escape is SQLite's error to report, + // and the only thing read from the path here is ':memory:'. + } + const mode = params.get('mode'); + return { + uri: true, + readonly: mode === 'ro' || params.get('immutable') === '1', + memory: mode === 'memory' || path.endsWith(':memory:'), + sharedCache: params.get('cache') === 'shared', + }; +} + /** * Creates a worker-thread pool over a database file: one writer * connection plus `options.readers` read-only connections (default 4), @@ -1167,7 +1206,13 @@ function parsePoolOptions(options) { * docs/concurrency.md). With `readers: 0` a `:memory:` pool is fine: * everything runs on the single writer. * - * @param {string} filename the database file. + * A `file:` URI is opened as a URI (the workers set `OPEN_URI` for it), + * so `mode`, `immutable` and `cache` are honoured. A read-only URI + * (`mode=ro`, `immutable=1`) turns WAL off by itself, since enabling WAL + * is a write: pass `walMode: true` explicitly on such a URI and pool() + * refuses rather than failing every worker at `PRAGMA journal_mode`. + * + * @param {string} filename the database file, or a `file:` URI. * @param {PoolOptions} [options] the pool options. * @returns {Promise} the opened pool. * @throws {TypeError} when the filename is missing or malformed, or an @@ -1181,21 +1226,54 @@ function parsePoolOptions(options) { * const rows = await pool.read('SELECT * FROM t WHERE a = ?', [1]); * await pool.write('INSERT INTO t (a) VALUES (?)', [2]); * await pool.close(); + * @example + * // A read-only pool over a shipped database, canonical URI form: + * const ro = await sqlite3.pool('file:/data/vdb.sqlite?mode=ro', { + * readers: 2, + * }); */ async function pool(filename, options) { if (typeof filename !== 'string' || filename.length === 0) { throw new TypeError('pool() requires a non-empty filename string'); } const opts = parsePoolOptions(options); - if (opts.readers > 0 && (filename === ':memory:' || filename === '')) { + const uri = parsePoolUri(filename); + const inMemory = + filename === ':memory:' || + filename === '' || + (uri.memory && !uri.sharedCache); + if (opts.readers > 0 && inMemory) { + const what = + filename === '' + ? "''" + : uri.memory + ? `the in-memory URI '${filename}'` + : "':memory:'"; throw new TypeError( - `pool() cannot open ${filename === '' ? "''" : "':memory:'"} ` + + `pool() cannot open ${what} ` + 'with readers: every pool connection is a separate ' + 'database, and an in-memory one cannot be shared across ' + 'workers — use readers: 0, or move the data with ' + - 'serializeToBytes()/deserializeFromBytes()', + 'serializeToBytes()/deserializeFromBytes()' + + (uri.memory ? ' (or add cache=shared to the URI)' : ''), ); } + if (uri.readonly && opts.walMode) { + // Enabling WAL writes to the file; on a read-only URI the writer + // worker's `PRAGMA journal_mode = WAL` cannot succeed, and the + // pool would fail to open at all. WAL is on by default, so a + // default is quietly dropped and only an explicit request is an + // error — silently ignoring what the caller asked for would be + // the worse half of the trade. + if (/** @type {any} */ (options)?.walMode === true) { + throw new TypeError( + `pool() cannot enable WAL on the read-only URI '${filename}': ` + + 'switching journal mode is a write. Pass walMode: false, ' + + 'or open the file read-write', + ); + } + opts.walMode = false; + } return SqlitePool.create(filename, opts); } diff --git a/lib/sqlite3-binding.js b/lib/sqlite3-binding.js index 5bddd80..daa1d3e 100644 --- a/lib/sqlite3-binding.js +++ b/lib/sqlite3-binding.js @@ -57,6 +57,19 @@ if (!(napiVersion >= NAPI_VERSION_REQUIRED)) { } let binding; +let bindingPath; +try { + // node-gyp-build exposes .path only through its own JS + // implementation; on a runtime where it delegates to require.addon + // the resolver is not reachable, so the path is best-effort context + // for the error message below. + bindingPath = + typeof (/** @type {any} */ (nodeGypBuild).path) === 'function' + ? /** @type {any} */ (nodeGypBuild).path(rootDir) + : undefined; +} catch { + bindingPath = undefined; +} try { binding = nodeGypBuild(rootDir); } catch (err) { @@ -84,6 +97,39 @@ try { throw new Error(hint, { cause: err }); } +// Staleness canary. node-gyp-build picks one binary out of prebuilds/ and +// build/ by a specificity sort, so the file that loads is not always the +// file that was just built: a leftover binary from an older revision +// loads clean and merely lacks whatever lib/ gained since. The sharpest +// case is a libc-tagged binary in a darwin-* or win32-* prebuilds +// directory — node-gyp-build resolves libc to 'glibc' on every non-Alpine +// platform, so `*.glibc.node` matches there and outranks the untagged +// current build. Symptom before this check: methods missing from the +// namespace, no error anywhere, and a 9.0-era binary passing itself off +// as 9.1. Now the mismatch is a load-time error that names the file. +// +// src/node_sqlite3.cc defines NODE_SQLITE3_NATIVE_INTERFACE; the two +// numbers are bumped in the same commit, whenever lib/ starts depending +// on something the addon did not export before. +const NATIVE_INTERFACE_EXPECTED = 1; +const reportedInterface = /** @type {any} */ (binding).NATIVE_INTERFACE_VERSION; +if (reportedInterface !== NATIVE_INTERFACE_EXPECTED) { + const where = bindingPath ? ` Loaded binary: ${bindingPath}.` : ''; + const reported = + reportedInterface === undefined + ? 'no NATIVE_INTERFACE_VERSION at all (a binary predating the check)' + : `native interface ${reportedInterface}`; + throw new Error( + '@appthreat/sqlite3 loaded a native binding that does not match this ' + + `JavaScript: lib/ expects native interface ${NATIVE_INTERFACE_EXPECTED}, ` + + `the binding reports ${reported}. A binary from another revision is being ` + + 'resolved ahead of the current one: delete the stale artifacts and rebuild ' + + '(`rm -rf prebuilds build && pnpm run rebuild`). A libc-tagged binary ' + + '(*.glibc.node, *.musl.node) inside a darwin-* or win32-* prebuilds ' + + `directory is the usual cause — it must not be there at all.${where}`, + ); +} + // The addon object carries everything; the classes are additionally // exported by name so `export { Database } from './sqlite3-binding.js'` // in lib/sqlite3.js is a real ESM re-export (whose declaration emit keeps diff --git a/lib/sqlite3.d.ts b/lib/sqlite3.d.ts index 9f4fde7..1497c32 100644 --- a/lib/sqlite3.d.ts +++ b/lib/sqlite3.d.ts @@ -59,6 +59,7 @@ export type sqlite3 = import('./sqlite3-binding.js').NativeBinding & { duration: bigint; durationMs: number; }) => void) => () => void; + flushQuerySpans: () => void; }; declare const sqlite3: sqlite3; declare const NativeDatabase: typeof import("./native.js").Database & DatabaseConstructor; diff --git a/lib/sqlite3.js b/lib/sqlite3.js index 9256bdf..bcabff4 100644 --- a/lib/sqlite3.js +++ b/lib/sqlite3.js @@ -71,6 +71,7 @@ import { extendTrace } from './trace.js'; * iterdump: (db: import('./sqlite3-binding.js').Database) => AsyncGenerator, * migrate: typeof import('./migrate.js').migrate, * subscribeQueries: (onMessage: (message: { sql: string, database: import('./sqlite3-binding.js').Database, duration: bigint, durationMs: number }) => void) => () => void, + * flushQuerySpans: () => void, * }} sqlite3 */ @@ -4724,6 +4725,31 @@ function armDiagnostics(arm) { } } +/** + * Delivers the spans already recorded but not yet dispatched, to every + * current subscriber, synchronously. + * + * SQLite reports a finished statement's timing on the thread that ran it + * — before that statement's own completion reaches JS — and the timing + * crosses to the JS thread through a queue drained on a later loop turn, + * so it arrives after the query it belongs to has resolved. Draining is + * how a caller reads it at a known moment. + * + * @returns {void} + * @private + */ +function flushQuerySpansNow() { + for (const db of trackedConnections()) { + if (!armedProfileListeners.has(db)) continue; + try { + /** @type {any} */ (db)._flushProfile(); + } catch { + // A connection closing underneath the drain is not a failure + // worth propagating out of a flush. + } + } +} + /** * Subscribes to query spans: every finished statement is published as * `{ sql, database, duration (bigint ns), durationMs }` on the @@ -4733,6 +4759,16 @@ function armDiagnostics(arm) { * the first subscriber and disarmed when the last one goes — nothing runs * while nobody listens. * + * **Delivery is asynchronous.** SQLite reports a statement's timing on + * the thread that executed it, and the span reaches the JS thread through + * a queue drained on a later event-loop turn — so immediately after + * `await db.all(sql)` the span for that query has usually not been + * delivered yet. Await a macrotask (`setImmediate`), or call + * {@link sqlite3.flushQuerySpans} to deliver what is pending right now, + * before asserting on or flushing collected spans. The unsubscribe + * function returned here drains first, so nothing recorded before it is + * lost. + * * Returns the unsubscribe function. Connections opened while a * subscription is active publish their spans too. * @@ -4743,6 +4779,9 @@ function armDiagnostics(arm) { * @example * const unsubscribe = sqlite3.subscribeQueries(({ sql, durationMs }) => * console.log(sql, durationMs.toFixed(3))); + * await db.all('SELECT 1'); + * sqlite3.flushQuerySpans(); // the span for that query is delivered now + * unsubscribe(); */ sqlite3.subscribeQueries = function subscribeQueries(onMessage) { if (typeof onMessage !== 'function') { @@ -4751,6 +4790,10 @@ sqlite3.subscribeQueries = function subscribeQueries(onMessage) { diagnosticsChannelSubscribers.add(onMessage); armDiagnostics(true); return () => { + // Drain before dropping the subscriber: the spans of queries it + // already awaited are sitting in the queue, and disarming would + // otherwise deliver them after it stopped listening. + flushQuerySpansNow(); diagnosticsChannelSubscribers.delete(onMessage); if (diagnosticsChannelSubscribers.size === 0) { armDiagnostics(false); @@ -4758,6 +4801,30 @@ sqlite3.subscribeQueries = function subscribeQueries(onMessage) { }; }; +/** + * Delivers every query span recorded but not yet dispatched, + * synchronously, to the current subscribers and channels. + * + * Spans are inherently asynchronous (see {@link sqlite3.subscribeQueries}): + * this is the drain point for code that must read them at a known moment + * — a test asserting on the span of a query it just awaited, or a + * shutdown path flushing to an APM sink. A no-op when nothing is + * subscribed. + * + * @returns {void} + * @since 9.1.0 + * @example + * const spans = []; + * const unsubscribe = sqlite3.subscribeQueries((s) => spans.push(s.sql)); + * await db.all('SELECT 1'); + * sqlite3.flushQuerySpans(); + * // spans now contains 'SELECT 1' + * unsubscribe(); + */ +sqlite3.flushQuerySpans = function flushQuerySpans() { + flushQuerySpansNow(); +}; + /** * @this {import('./sqlite3-binding.js').Database} * @param {string} type diff --git a/lib/worker.js b/lib/worker.js index a5e5e8c..a442c99 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -138,9 +138,23 @@ async function runQuery(msg) { * @private */ async function open(msg) { - const flags = msg.readOnly + let flags = msg.readOnly ? sqlite3.OPEN_READONLY | sqlite3.OPEN_FULLMUTEX : sqlite3.OPEN_READWRITE | sqlite3.OPEN_CREATE | sqlite3.OPEN_FULLMUTEX; + // SQLite interprets a `file:` filename as a URI only with + // SQLITE_OPEN_URI, and without it treats the whole string as a + // literal path — so `pool('file:/db.sqlite?mode=ro')` used to fail + // every worker with a bare SQLITE_CANTOPEN even though pool() and + // sqlite3.open() both document the URI form. The flag is set for + // exactly the filenames that look like URIs rather than + // unconditionally: with it on, *every* filename becomes URI syntax, + // which would change the meaning of a plain path that happens to + // start with 'file:' (and widens what an untrusted filename can ask + // for). Deliberately the same rule the permission-model check in + // lib/sqlite3.js uses, so both see the same target path. + if (/^file:/i.test(/** @type {string} */ (connectionFilename))) { + flags |= sqlite3.OPEN_URI; + } try { db = await new Promise((resolve, reject) => { const conn = new sqlite3.Database( diff --git a/package.json b/package.json index 835a6e4..79b976b 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,8 @@ "scripts": { "install": "node-gyp-build", "install:frozen": "pnpm install --config.strict-dep-builds=true --frozen-lockfile --package-import-method copy", - "prebuild": "prebuildify --napi --strip", + "prebuild": "node tools/prebuild.mjs", + "check:prebuilds": "node tools/check-prebuilds.mjs", "rebuild": "node-gyp rebuild", "lint": "biome check --write", "lint:check": "biome check", diff --git a/src/async.h b/src/async.h index f36e4cb..79276ee 100644 --- a/src/async.h +++ b/src/async.h @@ -53,6 +53,15 @@ template class Async { uv_close((uv_handle_t*)&watcher, close); } + // Delivers whatever is already queued, now, on the calling thread — + // which must be the JS thread, since the callback enters JS. Used by + // Database::_flushProfile so a caller that has just awaited a query + // can read its span instead of waiting for the loop turn on which the + // uv_async callback happens to run. + void flush() { + listener(&watcher); + } + void add(Item* item) { NODE_SQLITE3_MUTEX_LOCK(&mutex); data.emplace_back(item); diff --git a/src/database.cc b/src/database.cc index 8c398c7..47f6c47 100644 --- a/src/database.cc +++ b/src/database.cc @@ -31,6 +31,7 @@ Napi::Object Database::Init(Napi::Env env, Napi::Object exports) { InstanceMethod("parallelize", &Database::Parallelize, napi_default_method), InstanceMethod("configure", &Database::Configure, napi_default_method), InstanceMethod("interrupt", &Database::Interrupt, napi_default_method), + InstanceMethod("_flushProfile", &Database::FlushProfile, napi_default_method), InstanceMethod("_queueBusy", &Database::QueueBusy, napi_default_method), // User-defined functions (Deliverable 06): internal entry points // wrapped by lib/sqlite3.js, which parses options and flushes the @@ -864,6 +865,25 @@ void Database::RegisterProfileCallback(Baton* b) { db->Process(); } +// _flushProfile(): delivers the profile batons already queued for this +// connection right now, instead of on whichever loop turn the uv_async +// callback runs. SQLITE_TRACE_PROFILE fires on the worker thread as a +// statement finishes, before that statement's own completion reaches JS, +// so a caller that has awaited its query has a span waiting in the queue +// — this is what lets lib/sqlite3.js drain it before dropping a +// subscribeQueries() subscriber rather than losing it. +// +// Safe to call at any time: the queue is swapped out under its mutex, so +// a listener that calls back in sees an empty queue rather than +// re-entering the same items, and a connection with tracing off has no +// queue at all. +Napi::Value Database::FlushProfile(const Napi::CallbackInfo& info) { + if (debug_profile != NULL) { + debug_profile->flush(); + } + return info.Env().Undefined(); +} + void Database::ProfileCallback(Database *db, ProfileInfo* i) { auto info = std::unique_ptr(i); auto env = db->Env(); diff --git a/src/database.h b/src/database.h index 5a2dbec..242e105 100644 --- a/src/database.h +++ b/src/database.h @@ -522,6 +522,14 @@ class Database : public Napi::ObjectWrap { Napi::Value Parallelize(const Napi::CallbackInfo& info); Napi::Value Configure(const Napi::CallbackInfo& info); Napi::Value Interrupt(const Napi::CallbackInfo& info); + // Delivers the profile ('profile' event) batons already queued for + // this connection, synchronously. A finished statement's timing is + // handed over on the worker thread and dispatched to JS on a later + // loop turn, so a caller that has just awaited a query has not seen + // its span yet; lib/sqlite3.js drains through this before dropping a + // subscribeQueries() subscriber, and exposes it as + // sqlite3.flushQuerySpans(). + Napi::Value FlushProfile(const Napi::CallbackInfo& info); /** Current integerMode as a string: 'number' | 'bigint' | 'mixed'. */ Napi::Value IntegerModeGetter(const Napi::CallbackInfo& info); diff --git a/src/node_sqlite3.cc b/src/node_sqlite3.cc index c1a1d47..76acbcc 100644 --- a/src/node_sqlite3.cc +++ b/src/node_sqlite3.cc @@ -13,6 +13,18 @@ using namespace node_sqlite3; +// The native-interface revision. lib/sqlite3-binding.js requires this +// exact number (NATIVE_INTERFACE_EXPECTED there) and throws when the +// binary it loaded reports another one, or none at all. +// +// Bump both, in the same commit, whenever lib/ starts depending on +// something this addon did not export before. That turns "a stale .node +// was resolved ahead of the current build" — which otherwise presents as +// unexplained missing APIs on the namespace, e.g. a 9.0 binary shadowing +// a 9.1 checkout and reporting `rebaseChangeset === undefined` — into a +// load-time error naming the file. +#define NODE_SQLITE3_NATIVE_INTERFACE 1 + namespace { // setRowFactoryGenerator(fn): installs the JS half of the row builder. @@ -92,6 +104,8 @@ Napi::Object RegisterModule(Napi::Env env, Napi::Object exports) { Napi::Function::New(env, Complete)); exports.Set("compileOptions", Napi::Function::New(env, CompileOptions)); + exports.Set("NATIVE_INTERFACE_VERSION", + Napi::Number::New(env, NODE_SQLITE3_NATIVE_INTERFACE)); exports.DefineProperties({ DEFINE_CONSTANT_INTEGER(exports, SQLITE_OPEN_READONLY, OPEN_READONLY) diff --git a/test/abort.test.js b/test/abort.test.js index f5267fc..fc7beea 100644 --- a/test/abort.test.js +++ b/test/abort.test.js @@ -67,6 +67,35 @@ describe('abort', function () { await db.close(); }); + it('a sync method needs one drain after an in-flight abort', { + timeout: 30000, + }, async function () { + // Documented in the README and MIGRATING-TO-V9: an AbortSignal + // rejection is delivered when the signal fires, before the + // interrupted statement has finished unwinding on its worker, so + // the connection is not idle at that instant. `await db.wait()` + // (or any awaited query) drains the teardown. + const db = await sqlite3.open(':memory:'); + const controller = new AbortController(); + const pending = db.all(HEAVY, { signal: controller.signal }); + setTimeout(() => controller.abort('drain me'), 5); + await assert.rejects(pending, (reason) => reason === 'drain me'); + try { + db.allSync('SELECT 1'); + // The abort may have lost its race with the worker (see HEAVY + // above), in which case the connection really is idle and the + // sync call legitimately works. Nothing to assert then. + } catch (err) { + assert.match( + /** @type {Error} */ (err).message, + /sync methods require a fully idle database/, + ); + } + await db.wait(); + assert.deepStrictEqual(db.allSync('SELECT 1 AS x'), [{ x: 1 }]); + await db.close(); + }); + it('the signal option never collides with named parameters', async function () { const db = await sqlite3.open(':memory:'); await db.exec('CREATE TABLE t (a INT)'); diff --git a/test/compat.test.js b/test/compat.test.js index 1b15478..972d4ad 100644 --- a/test/compat.test.js +++ b/test/compat.test.js @@ -1,10 +1,13 @@ import assert from 'node:assert'; +import { rmSync } from 'node:fs'; +import { join } from 'node:path'; import { afterEach, beforeEach, describe, it } from 'node:test'; import { DatabaseSync } from '@appthreat/sqlite3/compat'; import { DatabaseSync as DatabaseSyncRelative } from '../lib/compat.js'; import sqlite3 from '../lib/sqlite3.js'; +import { TMP_DIR } from './support/db.js'; // Phase 5: the node:sqlite compatibility shim. @@ -165,6 +168,90 @@ describe('node:sqlite compat shim', function () { gated.close(); }); + it('close() finalizes leaked statements instead of never closing', async function () { + // node:sqlite finalizes outstanding statements on close, and code + // ported from it leaks prepares freely. This package refuses to + // close while one is unfinalized (SQLITE_BUSY), and the shim used + // to discard that error in an empty callback: nothing threw, + // nothing was emitted, and the connection stayed open with the + // file handle held. + const file = join( + TMP_DIR, + `compat-close-${process.pid}-${Date.now()}.db`, + ); + rmSync(file, { force: true }); + const leaky = new DatabaseSync(file); + leaky.exec('CREATE TABLE t (a)'); + const leaked = leaky.prepare('SELECT a FROM t'); + assert.deepStrictEqual(leaked.all(), []); + assert.ok(!leaked.finalized); + + /** @type {Error[]} */ + const errors = []; + leaky.native.on('error', (err) => errors.push(err)); + leaky.close(); + // The close is queued; wait for it the way the divergence note says. + await leaky.native.wait().catch(() => { + // A wait that fails says nothing about the close; poll below. + }); + for (let i = 0; i < 20 && leaky.isOpen; i++) { + await new Promise((resolve) => setImmediate(resolve)); + } + assert.strictEqual( + leaky.isOpen, + false, + 'the connection must actually close', + ); + assert.deepStrictEqual( + errors.map((e) => e.message), + [], + 'no error should have been needed', + ); + assert.ok(leaked.finalized, 'the leaked statement was finalized'); + rmSync(file, { force: true }); + }); + + it('close() reports a failure it cannot fix instead of swallowing it', async function () { + // A statement prepared directly on db.native is outside the + // shim's bookkeeping, so the close genuinely fails. The point of + // the test is that the failure is visible. + const bare = new DatabaseSync(':memory:'); + bare.exec('CREATE TABLE t (a)'); + const native = bare.native.prepareSync('SELECT a FROM t'); + /** @type {any[]} */ + const errors = []; + bare.native.on('error', (err) => errors.push(err)); + bare.close(); + for (let i = 0; i < 20 && errors.length === 0; i++) { + await new Promise((resolve) => setImmediate(resolve)); + } + assert.strictEqual(errors.length, 1, 'the close error must surface'); + assert.match(errors[0].message, /unfinalized statements/); + assert.strictEqual(errors[0].code, 'SQLITE_BUSY'); + // Clean up: finalize and close for real. + await new Promise((resolve) => native.finalize(() => resolve(null))); + await bare.native.close(); + }); + + it('await using closes a connection with statements still open', async function () { + const file = join( + TMP_DIR, + `compat-dispose-${process.pid}-${Date.now()}.db`, + ); + rmSync(file, { force: true }); + { + await using disposed = new DatabaseSync(file); + disposed.exec('CREATE TABLE t (a)'); + disposed.prepare('SELECT a FROM t').all(); + } + // The file is free once the block exits: reopening and dropping + // the table proves the handle was released. + const reopened = new DatabaseSync(file); + reopened.exec('DROP TABLE t'); + await reopened[Symbol.asyncDispose](); + rmSync(file, { force: true }); + }); + it('dispose support works', function () { { using stmt = db.prepare('SELECT 1'); diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js index 6a4a940..b1845aa 100644 --- a/test/diagnostics.test.js +++ b/test/diagnostics.test.js @@ -165,6 +165,47 @@ describe('diagnostics_channel', function () { } }); + it('flushQuerySpans() delivers the span of a query just awaited', async function () { + // Spans ride a uv_async queue, so `await db.all(...)` returns + // before the span for that query has been dispatched: reading the + // collected spans right there saw nothing at all. The flush is the + // documented drain point. + /** @type {string[]} */ + const spans = []; + const unsubscribe = sqlite3.subscribeQueries((span) => + spans.push(span.sql), + ); + try { + await db.all('SELECT 11 AS v'); + sqlite3.flushQuerySpans(); + assert.deepStrictEqual(spans, ['SELECT 11 AS v']); + // Idempotent: a second flush has nothing left to deliver. + sqlite3.flushQuerySpans(); + assert.deepStrictEqual(spans, ['SELECT 11 AS v']); + } finally { + unsubscribe(); + } + // A no-op with nothing subscribed. + sqlite3.flushQuerySpans(); + }); + + it('unsubscribe delivers pending spans instead of losing them', async function () { + /** @type {string[]} */ + const spans = []; + const unsubscribe = sqlite3.subscribeQueries((span) => + spans.push(span.sql), + ); + await db.all('SELECT 12 AS v'); + // The reported race: unsubscribing immediately after the await + // used to drop the span for the awaited query. + unsubscribe(); + assert.deepStrictEqual(spans, ['SELECT 12 AS v']); + // And nothing arrives afterwards. + await db.all('SELECT 13 AS v'); + await new Promise((resolve) => setTimeout(resolve, 50)); + assert.deepStrictEqual(spans, ['SELECT 12 AS v']); + }); + it('validates the listener', function () { assert.throws(() => sqlite3.subscribeQueries(7), TypeError); }); diff --git a/test/pool.test.js b/test/pool.test.js index ec7762d..715ef16 100644 --- a/test/pool.test.js +++ b/test/pool.test.js @@ -373,6 +373,64 @@ describe('pool', function () { assert.ok(true, 'await using disposed without hanging'); }); + it('opens a file: URI filename in every worker', { + timeout: 30000, + }, async function () { + // Without OPEN_URI in the workers' flags SQLite read the whole + // URI as a literal path and every worker failed with a bare + // SQLITE_CANTOPEN, even though pool() documents the URI form. + file = join(TMP_DIR, `pool-uri-${process.pid}-${Date.now()}.db`); + removeDb(file); + const seed = await sqlite3.pool(file, { readers: 0 }); + await seed.exec('CREATE TABLE t (a INTEGER PRIMARY KEY, b TEXT)'); + await seed.write('INSERT INTO t (b) VALUES (?)', ['uri']); + await seed.close(); + + pool = await sqlite3.pool(`file:${file}`, { readers: 2 }); + assert.strictEqual((await pool.get('SELECT b FROM t')).b, 'uri'); + // A read-write URI is still writable through the writer. + await pool.write('INSERT INTO t (b) VALUES (?)', ['second']); + assert.strictEqual( + (await pool.get('SELECT COUNT(*) AS n FROM t')).n, + 2, + ); + }); + + it('honours a read-only URI and drops the WAL default for it', { + timeout: 30000, + }, async function () { + // The canonical read-only form. WAL is a write, so the default + // must not be applied here — otherwise the writer worker's + // `PRAGMA journal_mode = WAL` fails and the whole pool refuses + // to open. + file = join(TMP_DIR, `pool-uri-ro-${process.pid}-${Date.now()}.db`); + removeDb(file); + const seed = await sqlite3.pool(file, { readers: 0, walMode: false }); + await seed.exec('CREATE TABLE t (a INTEGER PRIMARY KEY, b TEXT)'); + await seed.write('INSERT INTO t (b) VALUES (?)', ['ro']); + await seed.close(); + + pool = await sqlite3.pool(`file:${file}?mode=ro`, { readers: 2 }); + assert.strictEqual((await pool.get('SELECT b FROM t')).b, 'ro'); + await assert.rejects( + pool.write('INSERT INTO t (b) VALUES (?)', ['nope']), + /readonly|SQLITE_READONLY/i, + 'mode=ro must actually be read-only', + ); + }); + + it('refuses walMode: true on a read-only URI instead of failing every worker', { + timeout: 30000, + }, async function () { + await assert.rejects( + sqlite3.pool('file:/tmp/whatever.db?mode=ro', { + readers: 1, + walMode: true, + }), + /cannot enable WAL on the read-only URI/, + ); + }); + it('refuses :memory: and unknown options loudly', { timeout: 30000, }, async function () { @@ -380,6 +438,16 @@ describe('pool', function () { sqlite3.pool(':memory:'), /in-memory one cannot be shared across workers/, ); + // The URI spellings of the same thing: separate memory databases + // per worker is the silent-wrong-answer version of this error. + await assert.rejects( + sqlite3.pool('file::memory:'), + /in-memory one cannot be shared across workers/, + ); + await assert.rejects( + sqlite3.pool('file:anything?mode=memory'), + /in-memory one cannot be shared across workers/, + ); await assert.rejects( sqlite3.pool('/tmp/x.db', { readers: -1 }), /readers.*non-negative integer/, diff --git a/test/prebuild_layout.test.js b/test/prebuild_layout.test.js new file mode 100644 index 0000000..822a1ff --- /dev/null +++ b/test/prebuild_layout.test.js @@ -0,0 +1,225 @@ +import assert from 'node:assert'; +import { spawnSync } from 'node:child_process'; +import { mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; + +import sqlite3 from '../lib/sqlite3.js'; +import { applyLibcTagRule } from '../tools/prebuild.mjs'; + +const here = dirname(fileURLToPath(import.meta.url)); +const root = join(here, '..'); + +// The failure this file guards against loaded silently: a libc-tagged +// binary inside prebuilds/darwin-arm64/ outranks the untagged current +// build (node-gyp-build resolves libc to 'glibc' on every non-Alpine +// platform), so a stale .node was preferred to a fresh one with nothing +// reporting it — 9.1 APIs simply appeared to be missing. Three layers are +// pinned here: the build never emits such a file, CI refuses a directory +// containing one, and the loader refuses a binary that does not match +// lib/. + +const ELF = Buffer.from([0x7f, 0x45, 0x4c, 0x46, 0x02, 0x01, 0x01, 0x00]); +const MACHO64 = Buffer.from([0xcf, 0xfa, 0xed, 0xfe, 0x0c, 0x00, 0x00, 0x01]); +const PE = Buffer.from([0x4d, 0x5a, 0x90, 0x00, 0x03, 0x00, 0x00, 0x00]); + +/** + * Builds a synthetic prebuilds/ tree and runs the checker over it. + * + * @param {string} label a unique directory suffix. + * @param {Record} layout directory name → + * [filename, header bytes] entries. + * @returns {{ status: number | null, out: string }} the checker's result. + * @private + */ +function checkLayout(label, layout) { + const dir = join(root, 'test', 'tmp', `prebuilds-${process.pid}-${label}`); + rmSync(dir, { recursive: true, force: true }); + for (const [platformDir, files] of Object.entries(layout)) { + mkdirSync(join(dir, platformDir), { recursive: true }); + for (const [name, header] of files) { + writeFileSync(join(dir, platformDir, name), header); + } + } + const proc = spawnSync( + process.execPath, + [join(root, 'tools', 'check-prebuilds.mjs'), dir], + { encoding: 'utf8', cwd: root }, + ); + rmSync(dir, { recursive: true, force: true }); + return { status: proc.status, out: proc.stdout + proc.stderr }; +} + +describe('prebuild packaging guards', () => { + it('keeps --tag-libc on linux and drops it on darwin and win32', () => { + const args = ['--arch', 'arm64', '--tag-libc']; + const linux = applyLibcTagRule(args, 'linux'); + assert.deepStrictEqual(linux.forwarded, args); + assert.strictEqual(linux.droppedLibcTag, false); + + for (const platform of ['darwin', 'win32']) { + const other = applyLibcTagRule(args, platform); + assert.deepStrictEqual(other.forwarded, ['--arch', 'arm64']); + assert.strictEqual(other.droppedLibcTag, true); + } + }); + + it('drops the camelCase and =value spellings of the flag too', () => { + // minimist aliases tagLibc → tag-libc, and --tag-libc=musl is the + // same trap under a different filename. + for (const arg of ['--tagLibc', '--tag-libc=musl', '--tagLibc=glibc']) { + const { forwarded, droppedLibcTag } = applyLibcTagRule( + [arg, '--strip'], + 'darwin', + ); + assert.deepStrictEqual(forwarded, ['--strip'], `for ${arg}`); + assert.strictEqual(droppedLibcTag, true, `for ${arg}`); + } + }); + + it('leaves unrelated arguments alone', () => { + const args = ['--arch', 'ia32', '--target', '24.0.0', '--quiet']; + const { forwarded, droppedLibcTag } = applyLibcTagRule(args, 'darwin'); + assert.deepStrictEqual(forwarded, args); + assert.strictEqual(droppedLibcTag, false); + }); + + it('accepts a correctly tagged and formatted prebuilds tree', () => { + const { status, out } = checkLayout('ok', { + 'darwin-arm64': [['@appthreat+sqlite3.node', MACHO64]], + 'linux-x64': [ + ['@appthreat+sqlite3.glibc.node', ELF], + ['@appthreat+sqlite3.musl.node', ELF], + ], + 'win32-x64': [['@appthreat+sqlite3.node', PE]], + }); + assert.strictEqual(status, 0, out); + assert.match(out, /4 binaries/); + }); + + it('rejects a libc-tagged binary in a darwin directory', () => { + const { status, out } = checkLayout('darwin-glibc', { + 'darwin-arm64': [ + ['@appthreat+sqlite3.node', MACHO64], + ['@appthreat+sqlite3.glibc.node', MACHO64], + ], + }); + assert.strictEqual(status, 1, out); + assert.match(out, /libc tag 'glibc' on a darwin binary/); + assert.match(out, /outranks the untagged binary/); + }); + + it('rejects a libc-tagged binary in a win32 directory', () => { + const { status, out } = checkLayout('win32-musl', { + 'win32-x64': [['@appthreat+sqlite3.musl.node', PE]], + }); + assert.strictEqual(status, 1, out); + assert.match(out, /libc tag 'musl' on a win32 binary/); + }); + + it('rejects a binary whose object format contradicts its directory', () => { + // The reported case was a Mach-O file; a cross build putting one + // in linux-x64/ fails only on a user's machine otherwise. + const { status, out } = checkLayout('wrong-format', { + 'linux-x64': [['@appthreat+sqlite3.glibc.node', MACHO64]], + }); + assert.strictEqual(status, 1, out); + assert.match(out, /expected a elf binary for linux, found macho/); + }); + + it('rejects an empty prebuilds tree', () => { + const { status, out } = checkLayout('empty', { 'darwin-arm64': [] }); + assert.strictEqual(status, 1, out); + assert.match(out, /the prebuild produced nothing/); + }); + + it('exposes a native interface version that lib/ and src/ agree on', () => { + // The staleness canary: lib/sqlite3-binding.js refuses a binding + // reporting any other number, which is what turns "a stale .node + // was resolved" into an error instead of missing methods. Both + // constants are bumped in the same commit; this test fails when + // only one of them is. + const reported = /** @type {any} */ (sqlite3).NATIVE_INTERFACE_VERSION; + assert.strictEqual(typeof reported, 'number'); + + const cc = readFileSync(join(root, 'src', 'node_sqlite3.cc'), 'utf8'); + const native = cc.match( + /#define NODE_SQLITE3_NATIVE_INTERFACE\s+(\d+)/, + ); + assert.ok(native, 'src/node_sqlite3.cc must define the marker'); + assert.strictEqual(reported, Number(native[1])); + + const loader = readFileSync( + join(root, 'lib', 'sqlite3-binding.js'), + 'utf8', + ); + const expected = loader.match(/NATIVE_INTERFACE_EXPECTED\s*=\s*(\d+)/); + assert.ok(expected, 'lib/sqlite3-binding.js must pin the marker'); + assert.strictEqual( + reported, + Number(expected[1]), + 'lib/ and src/ disagree on the native interface version — bump both', + ); + }); + + it('refuses a binding whose native interface does not match lib/', () => { + // Simulated by pointing the loader at a stubbed node-gyp-build + // that returns a binding without the marker — exactly what a + // pre-check binary looks like. + const fixture = join( + root, + 'test', + 'tmp', + `stale-binding-${process.pid}`, + ); + rmSync(fixture, { recursive: true, force: true }); + mkdirSync(join(fixture, 'node_modules', 'node-gyp-build'), { + recursive: true, + }); + mkdirSync(join(fixture, 'lib'), { recursive: true }); + writeFileSync( + join(fixture, 'node_modules', 'node-gyp-build', 'package.json'), + JSON.stringify({ + name: 'node-gyp-build', + version: '0.0.0', + main: 'index.js', + }), + ); + writeFileSync( + join(fixture, 'node_modules', 'node-gyp-build', 'index.js'), + // A 9.0-era binding: classes present, marker absent. + `function load () { + return { Database: class {}, Statement: class {}, + Backup: class {}, Session: class {}, Blob: class {} }; + } + load.path = function () { return '/fake/prebuilds/darwin-arm64/@appthreat+sqlite3.glibc.node' }; + module.exports = load;\n`, + ); + writeFileSync( + join(fixture, 'lib', 'sqlite3-binding.js'), + readFileSync(join(root, 'lib', 'sqlite3-binding.js')), + ); + + const proc = spawnSync( + process.execPath, + [ + '--input-type=module', + '-e', + `import('${new URL(`file://${join(fixture, 'lib', 'sqlite3-binding.js')}`).href}') + .then(() => console.log('UNEXPECTED_LOAD')) + .catch((err) => console.log(err.message.replaceAll('\\n', ' ')));`, + ], + { encoding: 'utf8', cwd: fixture }, + ); + const out = proc.stdout + proc.stderr; + rmSync(fixture, { recursive: true, force: true }); + + assert.ok(!out.includes('UNEXPECTED_LOAD'), out); + assert.match(out, /does not match this JavaScript/); + assert.match(out, /no NATIVE_INTERFACE_VERSION at all/); + // The remedy and the offending file are both named. + assert.match(out, /rm -rf prebuilds build/); + assert.match(out, /@appthreat\+sqlite3\.glibc\.node/); + }); +}); diff --git a/test/session_rebase.test.js b/test/session_rebase.test.js index 3d725a1..0b417b5 100644 --- a/test/session_rebase.test.js +++ b/test/session_rebase.test.js @@ -37,6 +37,10 @@ describe('changeset rebasing', function () { } it('harvests a rebase buffer from a conflicting apply', async function () { + // The buffer is produced by whichever site *applies* an incoming + // changeset and resolves conflicts: it records what that site + // decided, so the decisions do not have to be made again + // elsewhere in the network. await local.run("INSERT INTO t VALUES (1, 'local')"); const session = local.session({ table: 't' }); await local.run("UPDATE t SET v = 'local2' WHERE id = 1"); @@ -44,7 +48,7 @@ describe('changeset rebasing', function () { await session.close(); const remote = await remoteWith('server'); - // OMIT: the remote's row wins; the rebase buffer records that. + // OMIT: the applying site's row wins; the rebase buffer records that. const rebase = await remote.applyChangeset(changeset, { conflict: 'omit', rebase: true, @@ -55,50 +59,142 @@ describe('changeset rebasing', function () { await remote.close(); }); - it('rebases a later local changeset against the harvested resolutions', async function () { - // Round one: the client learns its update lost to the server's. - await local.run("INSERT INTO t VALUES (1, 'v1')"); - const session1 = local.session({ table: 't' }); - await local.run("UPDATE t SET v = 'client-edit' WHERE id = 1"); - const round1 = await session1.changeset(); - await session1.close(); + it('rebases the local changeset against an incoming apply, so it lands cleanly upstream', async function () { + // The sync loop the rebaser exists for, in SQLite's own terms: + // this site is at S0, records local work (S0 → S1), then receives + // a changeset based on S0 from a peer and applies it *here*, + // resolving conflicts. Rebasing the local changeset against the + // resolutions makes it apply cleanly at the peer — no second + // conflict to resolve there. + // + // Direction matters and is easy to get backwards: the buffer must + // come from the apply performed on *this* database, and the + // changeset rebased must be the one recorded *before* that apply. + await local.run("INSERT INTO t VALUES (1, 'v0')"); - const remote = await remoteWith('server-edit'); - const rebase = await remote.applyChangeset(round1, { + // The peer: same S0, its own edit, its changeset. + const peer = new sqlite3.Database(':memory:'); + await peer.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v TEXT)'); + await peer.run("INSERT INTO t VALUES (1, 'v0')"); + const peerSession = peer.session({ table: 't' }); + await peer.run("UPDATE t SET v = 'remote-edit' WHERE id = 1"); + const remoteChangeset = await peerSession.changeset(); + await peerSession.close(); + + // This site's own work, recorded before the peer's changeset arrives. + const localSession = local.session({ table: 't' }); + await local.run("UPDATE t SET v = 'local-edit' WHERE id = 1"); + await local.run("INSERT INTO t VALUES (2, 'local-row')"); + const localChangeset = await localSession.changeset(); + await localSession.close(); + + // Apply the peer's changeset here, keeping the local value. + const rebase = await local.applyChangeset(remoteChangeset, { conflict: 'omit', rebase: true, }); - await remote.close(); + assert.ok(rebase instanceof Uint8Array); + assert.strictEqual( + (await local.get('SELECT v FROM t')).v, + 'local-edit', + ); + + // Un-rebased, the local changeset cannot be applied at the peer: + // its old values say 'v0' and the peer holds 'remote-edit'. + await assert.rejects( + peer.applyChangeset(localChangeset), + /SQLITE_ABORT/, + ); + assert.strictEqual( + (await peer.get('SELECT v FROM t')).v, + 'remote-edit', + ); + + // Rebased, the conflicting change's old values are rewritten to + // the values the OMIT left in place at the peer… + const rebased = sqlite3.rebaseChangeset(localChangeset, rebase); + const ops = [...sqlite3.iterateChangeset(rebased)]; + assert.deepStrictEqual( + ops.map((op) => op.op), + ['update', 'insert'], + ); + assert.deepStrictEqual(ops[0].oldRow, [1, 'remote-edit']); + assert.deepStrictEqual(ops[0].newRow, [null, 'local-edit']); - // Round two: the client makes a *new* local change (starting from - // its own state) and rebases it onto the resolved history — the - // textbook sync loop. The rebased changeset applies cleanly to - // the server state. - await local.run("UPDATE t SET v = 'after-rebase' WHERE id = 1"); - const session2 = local.session({ table: 't' }); - await local.run("INSERT INTO t VALUES (2, 'new-row')"); - const round2 = await session2.changeset(); - await session2.close(); - - const rebased = sqlite3.rebaseChangeset(round2, rebase); - assert.ok(rebased instanceof Uint8Array); - assert.ok(rebased.length > 0); - - const converged = await remoteWith('server-edit'); - await converged.applyChangeset(rebased); - // Rebase semantics: the OMIT resolution recorded in round one - // means the client's further edit to that same row is rebased - // away — the server's value stands. The new row (no conflict) - // lands normally. + // …so it applies with no conflict handler at all, and the + // non-conflicting insert rides along. + await peer.applyChangeset(rebased); assert.strictEqual( - (await converged.get('SELECT v FROM t WHERE id = 1')).v, - 'server-edit', + (await peer.get('SELECT v FROM t WHERE id = 1')).v, + 'local-edit', ); assert.strictEqual( - (await converged.get('SELECT v FROM t WHERE id = 2')).v, - 'new-row', + (await peer.get('SELECT v FROM t WHERE id = 2')).v, + 'local-row', + ); + await peer.close(); + }); + + it('rewrites by primary key, so only changesets recorded before the apply may be rebased', async function () { + // The matching rule, pinned because getting it wrong is silent: + // the rebaser finds a buffer entry by *primary key* and rewrites + // the change's old values to the ones the buffer carries. It does + // not check that the change was recorded before the apply — so a + // changeset recorded afterwards is rewritten just the same, and + // its old values then describe a state the peer left behind two + // pushes ago. One buffer belongs to the changesets recorded + // before its apply; later work needs its own round. + await local.run("INSERT INTO t VALUES (1, 'v0')"); + const incoming = new sqlite3.Database(':memory:'); + await incoming.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v TEXT)'); + await incoming.run("INSERT INTO t VALUES (1, 'v0')"); + const incomingSession = incoming.session({ table: 't' }); + await incoming.run("UPDATE t SET v = 'remote-edit' WHERE id = 1"); + const remoteChangeset = await incomingSession.changeset(); + await incomingSession.close(); + await incoming.close(); + + const before = local.session({ table: 't' }); + await local.run("UPDATE t SET v = 'local-edit' WHERE id = 1"); + const recordedBefore = await before.changeset(); + await before.close(); + + const rebase = await local.applyChangeset(remoteChangeset, { + conflict: 'omit', + rebase: true, + }); + + // Recorded after the apply: its own old values are 'local-edit', + // the state this database is actually in. + const after = local.session({ table: 't' }); + await local.run("UPDATE t SET v = 'later-edit' WHERE id = 1"); + const recordedAfter = await after.changeset(); + await after.close(); + assert.deepStrictEqual( + [...sqlite3.iterateChangeset(recordedAfter)][0].oldRow, + [1, 'local-edit'], + ); + + // Rebased against the same buffer, both changesets have their old + // values replaced by 'remote-edit' — correct for the first, + // wrong for the second (a peer that already received the first + // push holds 'local-edit'). + assert.deepStrictEqual( + [ + ...sqlite3.iterateChangeset( + sqlite3.rebaseChangeset(recordedBefore, rebase), + ), + ][0].oldRow, + [1, 'remote-edit'], + ); + assert.deepStrictEqual( + [ + ...sqlite3.iterateChangeset( + sqlite3.rebaseChangeset(recordedAfter, rebase), + ), + ][0].oldRow, + [1, 'remote-edit'], ); - await converged.close(); }); it('resolves undefined (no rebase) when no conflicts occurred', async function () { diff --git a/tools/BinaryBuilder.Dockerfile b/tools/BinaryBuilder.Dockerfile index 3e39ad6..2194b9d 100644 --- a/tools/BinaryBuilder.Dockerfile +++ b/tools/BinaryBuilder.Dockerfile @@ -26,6 +26,8 @@ RUN if case $VARIANT in "alpine"*) true;; *) false;; esac; then \ pnpm run prebuild --tag-libc; \ fi +RUN pnpm run check:prebuilds + RUN if case $VARIANT in "alpine"*) false;; *) true;; esac; then ldd prebuilds/*/*.node; nm prebuilds/*/*.node | grep \"GLIBC_\" | c++filt || true ; fi RUN pnpm run test && ls -l prebuilds diff --git a/tools/check-prebuilds.mjs b/tools/check-prebuilds.mjs new file mode 100644 index 0000000..169e024 --- /dev/null +++ b/tools/check-prebuilds.mjs @@ -0,0 +1,154 @@ +#!/usr/bin/env node +// Asserts that prebuilds/ is laid out the way node-gyp-build resolves it. +// Run after `pnpm run prebuild` (CI does; see .github/workflows/ci.yml) and +// before shipping a tarball. +// +// Two classes of fault, both of which have happened and neither of which +// fails a build on its own: +// +// 1. A libc-tagged binary outside `prebuilds/linux-*/`. node-gyp-build +// resolves libc to 'glibc' on every non-Alpine platform, macOS and +// Windows included, so `darwin-arm64/@appthreat+sqlite3.glibc.node` +// matches the platform and outranks the untagged binary beside it — +// a stale copy is then loaded in preference to the current build, +// silently. tools/prebuild.mjs stops producing these; this check +// catches any other producer. +// 2. A binary whose object format does not match its directory (the +// Mach-O file above sat in a directory whose name says darwin, but +// the reverse — a Mach-O in linux-x64/ — is what a misconfigured +// cross build emits, and it fails only at install time on a user's +// machine). +// +// Usage: node tools/check-prebuilds.mjs [prebuildsDir] + +import { closeSync, openSync, readdirSync, readSync, statSync } from 'node:fs'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const root = join(dirname(fileURLToPath(import.meta.url)), '..'); +const prebuildsDir = process.argv[2] + ? resolve(process.cwd(), process.argv[2]) + : join(root, 'prebuilds'); + +/** Tags node-gyp-build reads as a libc constraint (node-gyp-build.js). */ +const LIBC_TAGS = new Set(['glibc', 'musl']); + +/** + * Identifies an object file from its leading bytes. + * + * @param {string} file the path to inspect. + * @returns {'elf' | 'macho' | 'pe' | 'unknown'} the detected format. + * @private + */ +function objectFormat(file) { + const head = Buffer.alloc(4); + const fd = openSync(file, 'r'); + try { + readSync(fd, head, 0, 4, 0); + } finally { + closeSync(fd); + } + if (head.equals(Buffer.from([0x7f, 0x45, 0x4c, 0x46]))) { + return 'elf'; + } + const magic = head.readUInt32LE(0); + // Mach-O 32/64-bit, both endiannesses, plus the fat/universal header + // (which is big-endian by definition, hence the byte-swapped forms). + if ( + magic === 0xfeedface || + magic === 0xfeedfacf || + magic === 0xcefaedfe || + magic === 0xcffaedfe || + magic === 0xbebafeca || + magic === 0xbfbafeca + ) { + return 'macho'; + } + if (head[0] === 0x4d && head[1] === 0x5a) return 'pe'; + return 'unknown'; +} + +/** @type {Record} */ +const EXPECTED_FORMAT = { + linux: 'elf', + android: 'elf', + darwin: 'macho', + win32: 'pe', +}; + +/** @type {string[]} */ +const problems = []; +let checked = 0; + +let entries; +try { + entries = readdirSync(prebuildsDir, { withFileTypes: true }); +} catch (err) { + console.error( + `check-prebuilds: cannot read ${prebuildsDir} — run \`pnpm run prebuild\` first ` + + `(${/** @type {Error} */ (err).message})`, + ); + process.exit(1); +} + +for (const entry of entries) { + if (!entry.isDirectory()) continue; + const dir = join(prebuildsDir, entry.name); + // prebuildify names directories `-`; the platform is + // everything before the last dash (no supported platform name + // contains one, but the arch never does either way). + const dash = entry.name.lastIndexOf('-'); + const platform = dash === -1 ? entry.name : entry.name.slice(0, dash); + const expected = EXPECTED_FORMAT[platform]; + + for (const file of readdirSync(dir)) { + if (!file.endsWith('.node')) continue; + const path = join(dir, file); + if (!statSync(path).isFile()) continue; + checked++; + + const tags = file.slice(0, -'.node'.length).split('.').slice(1); + const libcTags = tags.filter((t) => LIBC_TAGS.has(t)); + if (libcTags.length > 0 && platform !== 'linux') { + problems.push( + `${entry.name}/${file}: libc tag '${libcTags.join(',')}' on a ` + + `${platform} binary. node-gyp-build resolves libc to 'glibc' on ` + + 'every non-Alpine platform, so this file outranks the untagged ' + + 'binary in the same directory and is loaded in preference to it. ' + + 'Build with tools/prebuild.mjs (which drops --tag-libc off linux) ' + + 'and delete this file.', + ); + } + + const format = objectFormat(path); + if (expected === undefined) { + problems.push( + `${entry.name}/${file}: unknown platform '${platform}' — teach ` + + 'EXPECTED_FORMAT in tools/check-prebuilds.mjs about it.', + ); + } else if (format !== expected) { + problems.push( + `${entry.name}/${file}: expected a ${expected} binary for ` + + `${platform}, found ${format}.`, + ); + } + } +} + +if (checked === 0) { + problems.push( + `no *.node files found under ${prebuildsDir} — the prebuild produced nothing.`, + ); +} + +if (problems.length > 0) { + console.error( + 'check-prebuilds: prebuilds/ layout is not loadable as intended:', + ); + for (const problem of problems) console.error(` - ${problem}`); + process.exit(1); +} + +console.log( + `check-prebuilds: ${checked} binaries under ${prebuildsDir} are correctly tagged and formatted.`, +); diff --git a/tools/prebuild.mjs b/tools/prebuild.mjs new file mode 100644 index 0000000..ce89d3c --- /dev/null +++ b/tools/prebuild.mjs @@ -0,0 +1,103 @@ +#!/usr/bin/env node +// `pnpm run prebuild` — prebuildify with one platform rule enforced: +// **the libc tag belongs to linux builds only**. +// +// Why the wrapper exists. prebuildify resolves the libc tag as +// `PREBUILD_LIBC || (isAlpine() ? 'musl' : 'glibc')`, and node-gyp-build +// resolves the loader's libc the same way — so on macOS and Windows both +// sides say "glibc". A `--tag-libc` build there produces +// `prebuilds/darwin-arm64/@appthreat+sqlite3.glibc.node`, which matches +// the running platform *and* carries a tag, so it wins node-gyp-build's +// specificity sort over the untagged `@appthreat+sqlite3.node` sitting +// next to it. Two binaries in one directory, the tagged one always +// preferred: a stale build named that way is then loaded in preference to +// the current one, silently. That happened (a pre-9.1 Mach-O binary +// shadowing a 9.1 build) and it presented as missing 9.1 APIs rather than +// as a packaging fault. +// +// So: `--tag-libc` is passed through on linux and dropped everywhere +// else, with a line on stderr saying so. CI passes the flag +// unconditionally for every target; the platform rule lives here rather +// than in the workflow matrix so that a local `pnpm run prebuild +// --tag-libc` cannot recreate the trap either. +// +// Every other argument is forwarded untouched. tools/check-prebuilds.mjs +// re-checks the resulting directory, so a regression fails a build even +// when prebuildify is invoked directly. + +import { spawn } from 'node:child_process'; +import { realpathSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import { fileURLToPath } from 'node:url'; + +/** + * Applies the platform rule to a prebuildify argument list. + * + * @param {string[]} argv the arguments after the script name. + * @param {string} platform a `process.platform` value. + * @returns {{ forwarded: string[], droppedLibcTag: boolean }} the + * arguments to pass on, and whether a libc tag was removed. + */ +export function applyLibcTagRule(argv, platform) { + /** @type {string[]} */ + const forwarded = []; + let droppedLibcTag = false; + for (const arg of argv) { + // Both spellings minimist accepts for the flag, bare and in + // `=value` form. A `--tag-libc=musl` cross-tag on a non-linux + // host is the same trap under a different filename, so it goes + // too. + if (/^--(tag-libc|tagLibc)(=|$)/.test(arg)) { + if (platform === 'linux') forwarded.push(arg); + else droppedLibcTag = true; + continue; + } + forwarded.push(arg); + } + return { forwarded, droppedLibcTag }; +} + +/** + * Runs prebuildify with the shipping flags and the filtered arguments. + * + * @returns {void} + * @private + */ +function main() { + const require = createRequire(import.meta.url); + const prebuildifyBin = require.resolve('prebuildify/bin.js'); + const { forwarded, droppedLibcTag } = applyLibcTagRule( + process.argv.slice(2), + process.platform, + ); + if (droppedLibcTag) { + process.stderr.write( + `tools/prebuild.mjs: dropped --tag-libc on ${process.platform} — ` + + 'the libc tag is meaningful only for linux builds, and a tagged ' + + 'binary in a non-linux prebuilds directory shadows the untagged ' + + 'one at load time. See the comment in tools/prebuild.mjs.\n', + ); + } + const child = spawn( + process.execPath, + [prebuildifyBin, '--napi', '--strip', ...forwarded], + { stdio: 'inherit' }, + ); + child.on('exit', (code, signal) => { + if (signal) { + process.kill(process.pid, signal); + return; + } + process.exit(code ?? 1); + }); +} + +// Run only as a script; importing this file (the test does) must not +// start a build. +const invoked = process.argv[1]; +if ( + invoked !== undefined && + realpathSync(invoked) === realpathSync(fileURLToPath(import.meta.url)) +) { + main(); +} From e33f1f3bfcd34d8f3e5b97f27dabd596b0da227c Mon Sep 17 00:00:00 2001 From: Team AppThreat Date: Tue, 15 Sep 2026 02:10:27 +0100 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20round-2=20findings=20=E2=80=94=20vta?= =?UTF-8?q?b=20constraint=20enforcement,=20pool=20URI=20edge=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7. pool() with a read-only URI (`immutable=1`) on a missing file fabricated an empty database: `immutable` is not a `mode`, so SQLite did not stop the writer's OPEN_CREATE, and a typo'd path came back as zero rows from every query with no error anywhere. A URI asserting read-only access now opens *every* connection read-only, writer included, so a missing file fails with SQLITE_CANTOPEN and nothing is created. 8. Hidden-parameter equality constraints were marked omit, promising SQLite the virtual table had applied them — but the value is only delivered to the generator, which may ignore it. A generator writing its own values into a parameter's column therefore defeated the query silently: `WHERE n = 1` returned every row, `WHERE n IN (1,2)` concatenated one unfiltered scan per value, and a join on that column multiplied rows. Nothing is omitted now: SQLite re-checks each constraint against the value xColumn reports, which costs one comparison per row and makes any generator correct — the well-behaved shape is unaffected, since xFilter fills a NULL parameter column with the argument. The rule this implies (a parameter column reports the argument: leave it NULL or echo it) is documented, and two of our own tests that only passed because of the omit were ill-formed under it — one named a single column as both output and parameter, the other put unrelated data in a parameter column. An ignoring generator now yields an endless scan rather than an OOM (non-matching rows are dropped instead of accumulating); a test pins that LIMIT and a cancellation token both stop it. 9. `pool()` could not open an in-memory database under default options — `PRAGMA journal_mode = WAL` reports 'memory', which the writer read as a failure. That broke both documented in-memory forms, including the `cache=shared` URI the readers>0 error recommends. WAL is now dropped for an in-memory pool as it already was for a read-only URI, and an explicit `walMode: true` is refused with a message that says why. 10. Documented that pool queries are invisible to subscribeQueries(): each worker has its own module instance and its own channels. Minor: a throwing vtab generator (and factory) now attaches the thrown value as `err.cause`, as a throwing UDF does; check-prebuilds.mjs accepts a package root as well as a prebuilds directory, and its empty-tree error says what layout it expected. Signed-off-by: Team AppThreat --- README.md | 19 ++++ docs/concurrency.md | 14 ++- lib/pool.d.ts | 19 +++- lib/pool.js | 47 +++++++++- lib/sqlite3.d.ts | 7 ++ lib/sqlite3.js | 10 ++- lib/worker.js | 24 +++-- src/database.h | 7 ++ src/function.cc | 4 + src/vtab.cc | 90 +++++++++++-------- test/pool.test.js | 81 ++++++++++++++++- test/prebuild_layout.test.js | 27 +++++- test/vtab.test.js | 167 +++++++++++++++++++++++++++++++++-- tools/check-prebuilds.mjs | 36 ++++++-- 14 files changed, 481 insertions(+), 71 deletions(-) diff --git a/README.md b/README.md index 614164b..740d138 100644 --- a/README.md +++ b/README.md @@ -851,6 +851,21 @@ generator as `undefined`; one the query constrained is also reported as that column's value, so `SELECT count FROM sequence(5)` works without the generator echoing it. +A parameter is a real (hidden) column, and `sequence(5)` is exactly +`WHERE count = 5` — **SQLite re-checks that predicate against every row +the generator produces**, rather than trusting the generator to have +applied it. So a row either leaves the parameter's column NULL — filled +with the argument, as above — or echoes the argument. A row reporting +anything else in that column contradicts the `WHERE` clause the argument +came from and is filtered out; put unrelated output in its own column. The +generator is still free to pre-filter for speed, and should: a generator +that ignores a constraint it cannot satisfy again produces an endless +scan (correct, but unbounded — `LIMIT`, or a cancellation token on the +async path, is the stop). A row shorter than `columns` pads with NULL, so +a mis-ordered `yield` shows up as NULLs rather than an error, and a +throwing generator fails the query with a message naming the table and the +thrown value attached as `err.cause`. + `db.values(array)` exposes any JS array as a queryable table — the rusqlite `rarray()` ergonomics no JS driver had: `JOIN` against in-memory data instead of building IN-lists. `drop()` the handle when you @@ -906,6 +921,10 @@ sqlite3.flushQuerySpans(); // spans === ['SELECT 1'] stop(); ``` +Spans are per module instance, so `pool()` traffic is invisible here: a +pool's queries run in workers, each with its own copy of this module and +its own channels ([docs/concurrency.md](docs/concurrency.md#the-pool)). + **node:sqlite drop-in** — code written against the built-in module can switch without rewriting: diff --git a/docs/concurrency.md b/docs/concurrency.md index b5f9faa..99f3eb0 100644 --- a/docs/concurrency.md +++ b/docs/concurrency.md @@ -187,7 +187,19 @@ await sqlite3.pool("file:/data/vdb.sqlite?mode=ro", { walMode: true }); // TypeE An in-memory URI (`file::memory:`, `mode=memory`) is refused with `readers > 0` for the same reason `:memory:` is — each worker would get -its own empty database — unless the URI carries `cache=shared`. +its own empty database — unless the URI carries `cache=shared`. WAL is +turned off by itself for an in-memory pool (there is no journal to +switch) and for a read-only URI, so both forms open under the default +options. + +**What the pool does not do: spans.** Each worker loads its own instance +of this module, so `sqlite3.subscribeQueries()` on the main thread sees +nothing from `pool.read()`, `pool.write()` or `pool.exec()` — those spans +are published on the worker's own `diagnostics_channel` (and its +`sqlite.db.query` mirror), where no main-thread subscriber is listening. +`flushQuerySpans()` cannot reach them either. If pool traffic must be +traced, time it at the call site, or use a dedicated worker you control +(the path handoff above) and subscribe inside it. ## Terminating a worker diff --git a/lib/pool.d.ts b/lib/pool.d.ts index f3ebf30..b082d8d 100644 --- a/lib/pool.d.ts +++ b/lib/pool.d.ts @@ -272,10 +272,21 @@ declare class SqlitePool { * everything runs on the single writer. * * A `file:` URI is opened as a URI (the workers set `OPEN_URI` for it), - * so `mode`, `immutable` and `cache` are honoured. A read-only URI - * (`mode=ro`, `immutable=1`) turns WAL off by itself, since enabling WAL - * is a write: pass `walMode: true` explicitly on such a URI and pool() - * refuses rather than failing every worker at `PRAGMA journal_mode`. + * so `mode`, `immutable` and `cache` are honoured. A URI that asserts + * read-only access (`mode=ro`, `immutable=1`) opens **every** connection + * read-only, the writer included: with `OPEN_CREATE` the writer would + * create the database the caller declared immutable, so a missing file + * fails with `SQLITE_CANTOPEN` instead of coming back as an empty one. + * Such a URI, and an in-memory database, also turn the WAL default off by + * themselves (a read-only connection cannot switch journal mode, and an + * in-memory database has no journal); asking for `walMode: true` there is + * refused up front rather than failing every worker at + * `PRAGMA journal_mode`. + * + * Pool queries are **not** visible to {@link sqlite3.subscribeQueries}: + * each worker loads its own instance of this module, so spans are + * published on the worker's channels and never reach a main-thread + * subscriber. See docs/concurrency.md. * * @param {string} filename the database file, or a `file:` URI. * @param {PoolOptions} [options] the pool options. diff --git a/lib/pool.js b/lib/pool.js index 712c351..a0e2138 100644 --- a/lib/pool.js +++ b/lib/pool.js @@ -113,6 +113,16 @@ class SqlitePool { /** @type {boolean} */ #walMode; + // The filename is a URI asserting read-only access (`mode=ro`, + // `immutable=1`). Every connection — the writer included — then opens + // without OPEN_CREATE, so a path that does not exist fails loudly + // instead of being created: `mode=ro` was already refused by SQLite + // itself (a URI mode may only narrow the flags), but `immutable=1` + // is not a mode, so the writer's OPEN_CREATE used to fabricate a + // 0-byte database and every read then returned nothing, silently. + /** @type {boolean} */ + #readOnlyFile; + /** @type {number} */ #busyTimeout; @@ -157,6 +167,7 @@ class SqlitePool { this.#walMode = options.walMode; this.#busyTimeout = options.busyTimeout; this.#integerMode = options.integerMode; + this.#readOnlyFile = parsePoolUri(filename).readonly; } /** @@ -212,6 +223,7 @@ class SqlitePool { kind: 'open', filename: this.#filename, readOnly, + readOnlyFile: this.#readOnlyFile, walMode: this.#walMode, busyTimeout: this.#busyTimeout, integerMode: this.#integerMode, @@ -1207,10 +1219,21 @@ function parsePoolUri(filename) { * everything runs on the single writer. * * A `file:` URI is opened as a URI (the workers set `OPEN_URI` for it), - * so `mode`, `immutable` and `cache` are honoured. A read-only URI - * (`mode=ro`, `immutable=1`) turns WAL off by itself, since enabling WAL - * is a write: pass `walMode: true` explicitly on such a URI and pool() - * refuses rather than failing every worker at `PRAGMA journal_mode`. + * so `mode`, `immutable` and `cache` are honoured. A URI that asserts + * read-only access (`mode=ro`, `immutable=1`) opens **every** connection + * read-only, the writer included: with `OPEN_CREATE` the writer would + * create the database the caller declared immutable, so a missing file + * fails with `SQLITE_CANTOPEN` instead of coming back as an empty one. + * Such a URI, and an in-memory database, also turn the WAL default off by + * themselves (a read-only connection cannot switch journal mode, and an + * in-memory database has no journal); asking for `walMode: true` there is + * refused up front rather than failing every worker at + * `PRAGMA journal_mode`. + * + * Pool queries are **not** visible to {@link sqlite3.subscribeQueries}: + * each worker loads its own instance of this module, so spans are + * published on the worker's channels and never reach a main-thread + * subscriber. See docs/concurrency.md. * * @param {string} filename the database file, or a `file:` URI. * @param {PoolOptions} [options] the pool options. @@ -1274,6 +1297,22 @@ async function pool(filename, options) { } opts.walMode = false; } + if (uri.memory || filename === ':memory:') { + // Same treatment for an in-memory database, which has no journal + // to switch: `PRAGMA journal_mode = WAL` reports 'memory' and the + // writer's check turned that into "could not enable WAL mode", + // failing the two documented in-memory pool forms — `readers: 0` + // and the `cache=shared` URI the readers>0 error message + // recommends — under nothing but the default options. + if (opts.walMode && /** @type {any} */ (options)?.walMode === true) { + throw new TypeError( + `pool() cannot enable WAL on the in-memory database '${filename}': ` + + "an in-memory database has no journal (PRAGMA journal_mode reports 'memory'). " + + 'Pass walMode: false, or use a file', + ); + } + opts.walMode = false; + } return SqlitePool.create(filename, opts); } diff --git a/lib/sqlite3.d.ts b/lib/sqlite3.d.ts index 1497c32..f3f59c3 100644 --- a/lib/sqlite3.d.ts +++ b/lib/sqlite3.d.ts @@ -190,6 +190,13 @@ export type VtabDefinition = { * a subset of `columns` to declare * HIDDEN — the table-valued function's arguments * (`SELECT * FROM name(arg)` passes `arg` to `rows`). + * + * A parameter is a real (hidden) column, and `name(arg)` is the + * predicate `WHERE param = arg`, which SQLite re-checks against every + * row the generator yields — the generator is not trusted to have + * applied it. A row therefore either leaves that column NULL (it is + * filled with the argument) or echoes the argument; a row reporting + * anything else there contradicts the predicate and is filtered out. */ parameters?: string[]; }; diff --git a/lib/sqlite3.js b/lib/sqlite3.js index bcabff4..565b25d 100644 --- a/lib/sqlite3.js +++ b/lib/sqlite3.js @@ -3585,6 +3585,13 @@ function vtabColumnName(spec, who) { * @property {string[]} [parameters] a subset of `columns` to declare * HIDDEN — the table-valued function's arguments * (`SELECT * FROM name(arg)` passes `arg` to `rows`). + * + * A parameter is a real (hidden) column, and `name(arg)` is the + * predicate `WHERE param = arg`, which SQLite re-checks against every + * row the generator yields — the generator is not trusted to have + * applied it. A row therefore either leaves that column NULL (it is + * filled with the argument) or echoes the argument; a row reporting + * anything else there contradicts the predicate and is filtered out. * @since 9.1.0 */ @@ -3615,7 +3622,8 @@ function vtabColumnName(spec, who) { * @since 9.1.0 * @example * db.table('sequence', { - * columns: ['value'], + * columns: ['value', 'count'], + * parameters: ['count'], // HIDDEN: sequence(5) passes 5 to rows() * rows: function* (count) { * for (let i = 0; i < count; i++) yield [i]; * }, diff --git a/lib/worker.js b/lib/worker.js index a442c99..a39085a 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -5,7 +5,8 @@ // writer a serializer rather than a race. // // Message protocol (parent → worker): -// { kind: 'open', readOnly, walMode, busyTimeout, integerMode } +// { kind: 'open', readOnly, readOnlyFile, walMode, busyTimeout, +// integerMode } // { id, kind: 'query', method: 'all'|'get'|'run'|'exec', sql, params, // cancel?: SharedArrayBuffer } // { kind: 'close' } @@ -132,15 +133,24 @@ async function runQuery(msg) { /** * Opens the connection and applies the pool configuration. * - * @param {{ filename: string, readOnly?: boolean, walMode?: boolean, - * busyTimeout?: number, integerMode?: string }} msg the open request. + * @param {{ filename: string, readOnly?: boolean, readOnlyFile?: boolean, + * walMode?: boolean, busyTimeout?: number, integerMode?: string }} msg + * the open request. * @returns {Promise} resolves once 'ready' (or 'openError') is posted. * @private */ async function open(msg) { - let flags = msg.readOnly - ? sqlite3.OPEN_READONLY | sqlite3.OPEN_FULLMUTEX - : sqlite3.OPEN_READWRITE | sqlite3.OPEN_CREATE | sqlite3.OPEN_FULLMUTEX; + // readOnlyFile: the filename is a URI asserting read-only access + // (mode=ro, immutable=1). The writer opens read-only too then — with + // OPEN_CREATE it would *create* a database the caller declared + // immutable, and a missing file would come back as an empty one + // instead of SQLITE_CANTOPEN. + let flags = + msg.readOnly || msg.readOnlyFile + ? sqlite3.OPEN_READONLY | sqlite3.OPEN_FULLMUTEX + : sqlite3.OPEN_READWRITE | + sqlite3.OPEN_CREATE | + sqlite3.OPEN_FULLMUTEX; // SQLite interprets a `file:` filename as a URI only with // SQLITE_OPEN_URI, and without it treats the whole string as a // literal path — so `pool('file:/db.sqlite?mode=ro')` used to fail @@ -188,7 +198,7 @@ async function open(msg) { // WAL is a persistent property of the file, so only the writer // needs to set it; readers pick it up from the file. Setting it // from a read-only connection is refused by SQLite anyway. - if (msg.walMode && !msg.readOnly) { + if (msg.walMode && !msg.readOnly && !msg.readOnlyFile) { const mode = await /** @type {(...args: unknown[]) => any} */ ( connection.get )('PRAGMA journal_mode = WAL'); diff --git a/src/database.h b/src/database.h index 242e105..f0869f1 100644 --- a/src/database.h +++ b/src/database.h @@ -485,6 +485,13 @@ class Database : public Napi::ObjectWrap { Database(const Napi::CallbackInfo& info); + // Records `err` — a value thrown by a user callback on the JS thread — + // as the `cause` the next SQLite error built for this connection will + // carry (AttachPendingJsError consumes it). Public because the virtual + // table machinery reaches it from free functions on the JS thread; the + // user-function path sets the slot directly (it is a friend). + void SetPendingJsError(Napi::Value err); + ~Database() { RemoveCallbacks(); RemoveUserFunctions(); diff --git a/src/function.cc b/src/function.cc index a41f850..3ae5771 100644 --- a/src/function.cc +++ b/src/function.cc @@ -1331,6 +1331,10 @@ int Database::ProgressHandler(void* ctx) { } } +void Database::SetPendingJsError(Napi::Value err) { + pending_js_error = Napi::Persistent(err); +} + void Database::AttachPendingJsError(Napi::Object err) { if (pending_js_error.IsEmpty()) return; err.Set("cause", pending_js_error.Value()); diff --git a/src/vtab.cc b/src/vtab.cc index 6c71dd5..d1fd885 100644 --- a/src/vtab.cc +++ b/src/vtab.cc @@ -120,6 +120,33 @@ void ApplyCellToResult(sqlite3_context* ctx, const Cell& cell) { } } +// Consumes the pending JS exception: appends its `message` to `text` (when +// it has one) and keeps the thrown value on the database as the `cause` of +// the step failure this is about to produce — the same contract a throwing +// user-defined function gets (src/function.cc SetCallError), so +// `err.cause` means the same thing whichever kind of callback threw. +static void CaptureVtabThrow(Napi::Env env, Database* db, std::string* text) { + napi_value pending = NULL; + napi_get_and_clear_last_exception(env, &pending); + if (pending == NULL) return; + Napi::Value err(env, pending); + if (err.IsObject()) { + Napi::Value msg = err.As().Get("message"); + if (!env.IsExceptionPending()) { + if (text != NULL && msg.IsString()) { + *text += ": " + msg.As().Utf8Value(); + } + } + else { + // Reading .message threw (a hostile getter); the value is + // still worth carrying as the cause. + napi_value stray = NULL; + napi_get_and_clear_last_exception(env, &stray); + } + } + db->SetPendingJsError(err); +} + // Converts one yielded JS value into a Cell via the shared bind converter // (strict marshalling: no [object Object], no silent coercion). Returns // false with the call marked errored. @@ -244,16 +271,7 @@ void PullRows(Napi::Env env, Database* db, VtabCall* call, Napi::Object iter) { call->errored = true; call->error = "the rows generator of virtual table '" + module->name + "' threw"; - napi_value pending = NULL; - napi_get_and_clear_last_exception(env, &pending); - Napi::Value err(env, pending); - if (err.IsObject()) { - Napi::Value msg = err.As().Get("message"); - if (!env.IsExceptionPending() && msg.IsString()) { - call->error += ": " + msg.As().Utf8Value(); - } - napi_get_and_clear_last_exception(env, &pending); - } + CaptureVtabThrow(env, db, &call->error); return; } if (!step.IsObject()) { @@ -321,18 +339,9 @@ void ExecuteVtabCallOnJsThread(napi_env nenv, VtabCall* call) { Napi::Value definition = factory.Call(env.Undefined(), argv); if (env.IsExceptionPending()) { call->errored = true; - napi_value pending = NULL; - napi_get_and_clear_last_exception(env, &pending); - Napi::Value err(env, pending); call->error = "the factory of virtual table module '" + module->name + "' threw"; - if (err.IsObject()) { - Napi::Value msg = err.As().Get("message"); - if (!env.IsExceptionPending() && msg.IsString()) { - call->error += ": " + msg.As().Utf8Value(); - } - napi_get_and_clear_last_exception(env, &pending); - } + CaptureVtabThrow(env, db, &call->error); return; } Napi::Value rows = definition.IsObject() @@ -411,19 +420,14 @@ void ExecuteVtabCallOnJsThread(napi_env nenv, VtabCall* call) { Napi::Value iterable_v = rows_fn.Call(env.Undefined(), argv); if (env.IsExceptionPending() || !iterable_v.IsObject()) { call->errored = true; - call->error = "the rows generator of virtual table '" + - module->name + "' did not return an iterable"; - napi_value pending = NULL; - napi_get_and_clear_last_exception(env, &pending); - Napi::Value err(env, pending); - if (err.IsObject()) { - Napi::Value msg = err.As().Get("message"); - if (!env.IsExceptionPending() && msg.IsString()) { - call->error = "the rows generator of virtual table '" + - module->name + "' threw: " + - msg.As().Utf8Value(); - } - napi_get_and_clear_last_exception(env, &pending); + if (env.IsExceptionPending()) { + call->error = "the rows generator of virtual table '" + + module->name + "' threw"; + CaptureVtabThrow(env, db, &call->error); + } + else { + call->error = "the rows generator of virtual table '" + + module->name + "' did not return an iterable"; } return; } @@ -637,11 +641,20 @@ static int BestIndex(sqlite3_vtab* vtab, sqlite3_index_info* info) { // `WHERE b = ?` on a later parameter and any duplicate constraint on // one parameter. // - // A second constraint on an already-supplied parameter is left for - // sqlite to check against the column value (not omitted): omitting it - // would claim `count = 3 AND count = 4` holds. xFilter fills HIDDEN - // columns the generator left NULL with the argument value, so that - // check sees what the caller passed. + // Nothing is omitted. `aConstraintUsage[i].omit = 1` promises sqlite + // the virtual table applied the constraint itself, and sqlite then + // drops it from the WHERE clause entirely — but the constraint value + // is only *delivered* to the generator as an argument, and a generator + // is free to ignore it. A generator that writes its own values into a + // parameter's column therefore defeated the query silently: + // `WHERE n = 1` returned every row it yielded, `WHERE n IN (1,2)` + // concatenated one unfiltered scan per IN value, and a join on that + // column multiplied rows. Leaving omit at 0 costs one comparison per + // row against the column value xColumn reports and makes *any* + // generator correct — including the well-behaved shape, where xFilter + // fills the HIDDEN columns the generator left NULL with the argument + // (ApplyCursorArgs), so the re-check sees exactly what the caller + // passed and passes. std::string mapping; int argv_n = 0; std::vector taken(module->params.size(), false); @@ -652,7 +665,6 @@ static int BestIndex(sqlite3_vtab* vtab, sqlite3_index_info* info) { if (p < 0 || taken[static_cast(p)]) continue; taken[static_cast(p)] = true; info->aConstraintUsage[i].argvIndex = ++argv_n; - info->aConstraintUsage[i].omit = 1; if (!mapping.empty()) mapping += ","; mapping += std::to_string(p); } diff --git a/test/pool.test.js b/test/pool.test.js index 715ef16..bd30c6d 100644 --- a/test/pool.test.js +++ b/test/pool.test.js @@ -3,7 +3,7 @@ // postMessage boundary, cancellation through the shared flag, and // shutdown that leaves no worker behind. import assert from 'node:assert'; -import { rmSync } from 'node:fs'; +import { existsSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { afterEach, describe, it } from 'node:test'; @@ -431,6 +431,85 @@ describe('pool', function () { ); }); + it('refuses a read-only URI whose file does not exist, and creates nothing', { + timeout: 30000, + }, async function () { + // immutable=1 is a read-only assertion, but it is not a `mode`, so + // SQLite did not stop the writer's OPEN_CREATE: the pool created a + // 0-byte database, the readers opened it happily, and every query + // returned nothing — a typo'd path looked like an empty database. + const missing = join( + TMP_DIR, + `pool-missing-${process.pid}-${Date.now()}.db`, + ); + removeDb(missing); + await assert.rejects( + sqlite3.pool(`file:${missing}?immutable=1`, { readers: 1 }), + /SQLITE_CANTOPEN/, + ); + assert.strictEqual( + existsSync(missing), + false, + 'a read-only URI must not create the database', + ); + // mode=ro was already refused by SQLite itself; assert it stays so. + await assert.rejects( + sqlite3.pool(`file:${missing}?mode=ro`, { readers: 1 }), + /SQLITE_CANTOPEN/, + ); + assert.strictEqual(existsSync(missing), false); + }); + + it('reads an existing file through an immutable URI, and refuses writes', { + timeout: 30000, + }, async function () { + file = join(TMP_DIR, `pool-imm-${process.pid}-${Date.now()}.db`); + removeDb(file); + const seed = await sqlite3.pool(file, { readers: 0, walMode: false }); + await seed.exec('CREATE TABLE t (a INTEGER PRIMARY KEY, b TEXT)'); + await seed.write('INSERT INTO t (b) VALUES (?)', ['frozen']); + await seed.close(); + + pool = await sqlite3.pool(`file:${file}?immutable=1`, { readers: 2 }); + assert.strictEqual((await pool.get('SELECT b FROM t')).b, 'frozen'); + await assert.rejects( + pool.write('INSERT INTO t (b) VALUES (?)', ['nope']), + /readonly|SQLITE_READONLY/i, + ); + }); + + it('an in-memory pool works under default options', { + timeout: 30000, + }, async function () { + // `PRAGMA journal_mode = WAL` reports 'memory' for an in-memory + // database, which the writer read as a failure — so both + // documented in-memory forms (readers: 0, and the cache=shared URI + // the readers>0 error recommends) failed under nothing but the + // defaults. + const solo = await sqlite3.pool(':memory:', { readers: 0 }); + await solo.exec('CREATE TABLE t (a)'); + await solo.write('INSERT INTO t VALUES (1)'); + assert.deepStrictEqual(await solo.read('SELECT a FROM t'), [{ a: 1 }]); + await solo.close(); + + const shared = await sqlite3.pool('file::memory:?cache=shared', { + readers: 2, + }); + await shared.exec('CREATE TABLE t (a)'); + await shared.write('INSERT INTO t VALUES (2)'); + // The readers share the writer's database through the shared cache. + assert.deepStrictEqual(await shared.read('SELECT a FROM t'), [ + { a: 2 }, + ]); + await shared.close(); + + // An explicit request is still an error rather than a silent no-op. + await assert.rejects( + sqlite3.pool(':memory:', { readers: 0, walMode: true }), + /cannot enable WAL on the in-memory database/, + ); + }); + it('refuses :memory: and unknown options loudly', { timeout: 30000, }, async function () { diff --git a/test/prebuild_layout.test.js b/test/prebuild_layout.test.js index 822a1ff..9398a60 100644 --- a/test/prebuild_layout.test.js +++ b/test/prebuild_layout.test.js @@ -131,7 +131,32 @@ describe('prebuild packaging guards', () => { it('rejects an empty prebuilds tree', () => { const { status, out } = checkLayout('empty', { 'darwin-arm64': [] }); assert.strictEqual(status, 1, out); - assert.match(out, /the prebuild produced nothing/); + assert.match(out, /nothing to check/); + // The message must say what shape was expected, since the argument + // may as easily have been the wrong directory as an empty build. + assert.match(out, /-/); + }); + + it('accepts a package root as well as a prebuilds directory', () => { + // `node tools/check-prebuilds.mjs ` used to report + // "no *.node files" without hinting that the argument should be + // the prebuilds/ directory; it now finds the nested one. + const pkg = join(root, 'test', 'tmp', `pkgroot-${process.pid}`); + rmSync(pkg, { recursive: true, force: true }); + mkdirSync(join(pkg, 'prebuilds', 'darwin-arm64'), { recursive: true }); + writeFileSync( + join(pkg, 'prebuilds', 'darwin-arm64', '@appthreat+sqlite3.node'), + MACHO64, + ); + const proc = spawnSync( + process.execPath, + [join(root, 'tools', 'check-prebuilds.mjs'), pkg], + { encoding: 'utf8', cwd: root }, + ); + rmSync(pkg, { recursive: true, force: true }); + const out = proc.stdout + proc.stderr; + assert.strictEqual(proc.status, 0, out); + assert.match(out, /1 binaries/); }); it('exposes a native interface version that lib/ and src/ agree on', () => { diff --git a/test/vtab.test.js b/test/vtab.test.js index 21f284f..5aa87ae 100644 --- a/test/vtab.test.js +++ b/test/vtab.test.js @@ -60,11 +60,18 @@ describe('virtual tables', function () { // Regression: xBestIndex used to hand xFilter the constraints in // reverse (argvIndex = size - p), so a two-parameter table // function received its arguments swapped. + // + // The row yields values for the two visible columns only, leaving + // the parameter columns to be filled with the arguments. That is + // the contract now that constraints are re-checked per row (see + // 'a parameter column must report the argument' below): a + // parameter column carrying unrelated data contradicts the WHERE + // clause it came from, and the row is filtered out. db.table('pair', { columns: ['first', 'second', 'n', 'm'], parameters: ['n', 'm'], rows: function* pair(n, m) { - yield [n, m, 'seen']; + yield [n, m]; }, }); assert.deepStrictEqual(await db.all('SELECT * FROM pair(10, 20)'), [ @@ -81,9 +88,13 @@ describe('virtual tables', function () { }); it('runs from the synchronous methods too', function () { + // 'count' is the parameter, 'value' the output. Naming one column + // both — `columns: ['value'], parameters: ['value']` — means + // `sequence(2)` is the constraint `value = 2`, which no generated + // row satisfies. db.table('sequence', { - columns: ['value'], - parameters: ['value'], + columns: ['value', 'count'], + parameters: ['count'], rows: function* seq(count) { for (let i = 0; i < count; i++) yield [i]; }, @@ -94,6 +105,31 @@ describe('virtual tables', function () { ]); }); + it('a parameter column must report the argument', async function () { + // The rule the re-checked constraint implies, pinned so it is a + // decision rather than an accident: a parameter is a real (hidden) + // column, and `t(x)` is `WHERE param = x`. A row either leaves that + // column NULL — the binding fills it with the argument — or echoes + // the argument. A row that reports something else contradicts the + // WHERE clause the argument came from and is filtered out. + db.table('contradicts', { + columns: ['v', 'p'], + parameters: ['p'], + rows: function* (p) { + yield [1, p]; // echoes: kept + yield [2]; // NULL, filled with p: kept + yield [3, 'something else']; // contradicts: filtered + }, + }); + assert.deepStrictEqual( + await db.all('SELECT v, p FROM contradicts(9)'), + [ + { v: 1, p: 9 }, + { v: 2, p: 9 }, + ], + ); + }); + it('joins against real tables', async function () { db.table('ids', { columns: ['n'], @@ -207,18 +243,33 @@ describe('virtual tables', function () { }); it('reports a generator that throws mid-scan and stays usable', async function () { + const boom = new Error('mid-scan failure'); db.table('flaky', { columns: ['v'], rows: function* () { for (let i = 0; i < 5000; i++) { // Past the first batch, so the failure lands in xNext. - if (i === 100) throw new Error('mid-scan failure'); + if (i === 100) throw boom; yield [i]; } }, }); - await assert.rejects(db.all('SELECT v FROM flaky'), /mid-scan failure/); + // The thrown value rides along as `cause`, as it does for a + // throwing user-defined function — it used to be dropped, leaving + // only the message. + await assert.rejects(db.all('SELECT v FROM flaky'), (err) => { + assert.match(err.message, /mid-scan failure/); + assert.strictEqual(err.cause, boom); + return true; + }); assert.strictEqual((await db.get('SELECT 1 AS v')).v, 1); + await assert.throws( + () => db.allSync('SELECT v FROM flaky'), + (err) => { + assert.strictEqual(err.cause, boom, 'sync path too'); + return true; + }, + ); }); it('re-filters a cursor for each row of a correlated subquery', async function () { @@ -313,6 +364,92 @@ describe('virtual tables', function () { ); }); + it('enforces a hidden-parameter constraint even when the generator ignores it', async function () { + // BestIndex used to set aConstraintUsage.omit = 1, promising sqlite + // the table had applied the constraint — so sqlite dropped it from + // the WHERE clause. But the value is only *delivered* to the + // generator, which is free to ignore it: a generator writing its + // own values into the parameter's column silently defeated the + // query. Every assertion here returned unfiltered rows before. + db.table('ignores', { + columns: ['n'], + parameters: ['n'], + rows: function* (_p) { + yield [0]; + yield [1]; + yield [2]; + }, + }); + assert.deepStrictEqual( + await db.all('SELECT n FROM ignores WHERE n = 1'), + [{ n: 1 }], + ); + assert.deepStrictEqual( + db.allSync('SELECT n FROM ignores WHERE n = 1'), + [{ n: 1 }], + ); + // An IN list re-filters the cursor once per value; the rows of each + // scan used to be concatenated unfiltered (six rows here). + assert.deepStrictEqual( + await db.all('SELECT n FROM ignores WHERE n IN (1, 2)'), + [{ n: 1 }, { n: 2 }], + ); + // A join constraint is the same mechanism, and its failure mode is + // silent row multiplication. + await db.exec('CREATE TABLE driver (k, want)'); + await db.run('INSERT INTO driver VALUES (1, 1), (2, 2)'); + assert.deepStrictEqual( + await db.all( + 'SELECT d.k, i.n FROM driver d JOIN ignores i ON i.n = d.want ORDER BY d.k', + ), + [ + { k: 1, n: 1 }, + { k: 2, n: 2 }, + ], + ); + // The control: the same table without the parameter declaration + // always filtered correctly. + db.table('plain', { + columns: ['n'], + rows: function* () { + yield [0]; + yield [1]; + yield [2]; + }, + }); + assert.deepStrictEqual( + await db.all('SELECT n FROM plain WHERE n = 1'), + [{ n: 1 }], + ); + }); + + it('an unbounded generator that ignores its parameter stays interruptible', { + timeout: 30000, + }, async function () { + // With the constraint enforced, the non-matching rows are dropped + // instead of accumulating in the result — the shape that used to + // exhaust the heap. The scan itself cannot end (only the generator + // knows it will never match again), so the escape hatches are the + // ordinary ones: LIMIT, or cancellation on the async path. + db.table('endless', { + columns: ['n'], + parameters: ['n'], + rows: function* (_p) { + let i = 0; + while (true) yield [i++]; + }, + }); + assert.deepStrictEqual( + db.allSync('SELECT n FROM endless WHERE n = 3 LIMIT 1'), + [{ n: 3 }], + ); + const token = db.cancellationToken(); + const pending = db.all('SELECT n FROM endless WHERE n = 3'); + setTimeout(() => token.cancel(), 200); + await assert.rejects(pending, /SQLITE_INTERRUPT/); + await db.wait(); + }); + it('reports a hidden parameter the generator did not yield', async function () { db.table('echo', { columns: ['value', 'n'], @@ -371,20 +508,34 @@ describe('virtual tables', function () { }); it('propagates generator throws as query errors', async function () { + const boom = new Error('generator exploded'); db.table('broken', { columns: ['v'], // Throwing before the first yield is the case under test: a // generator function that throws on its first next(). rows: function* () { - if (Date.now() > 0) throw new Error('generator exploded'); + if (Date.now() > 0) throw boom; yield 0; }, }); await assert.rejects( db.all('SELECT v FROM broken'), (err) => - /generator exploded/.test(err.message) || - /broken/.test(err.message), + (/generator exploded/.test(err.message) || + /broken/.test(err.message)) && + err.cause === boom, + ); + // A plain function (not a generator) that throws when invoked + // reports through the same channel. + db.table('brokenFn', { + columns: ['v'], + rows: () => { + throw boom; + }, + }); + await assert.rejects( + db.all('SELECT v FROM brokenFn'), + (err) => err.cause === boom, ); // The connection survives. assert.strictEqual((await db.get('SELECT 1 AS v')).v, 1); diff --git a/tools/check-prebuilds.mjs b/tools/check-prebuilds.mjs index 169e024..e83e9b0 100644 --- a/tools/check-prebuilds.mjs +++ b/tools/check-prebuilds.mjs @@ -19,16 +19,39 @@ // cross build emits, and it fails only at install time on a user's // machine). // -// Usage: node tools/check-prebuilds.mjs [prebuildsDir] +// Usage: node tools/check-prebuilds.mjs [dir] +// +// `dir` is the prebuilds directory itself, or a package root containing +// one; it defaults to this repo's prebuilds/. import { closeSync, openSync, readdirSync, readSync, statSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; const root = join(dirname(fileURLToPath(import.meta.url)), '..'); -const prebuildsDir = process.argv[2] - ? resolve(process.cwd(), process.argv[2]) - : join(root, 'prebuilds'); + +/** + * Resolves the directory to scan: the argument as given, or its + * `prebuilds/` subdirectory when the argument is a package root (which is + * what most callers reach for first). + * + * @param {string | undefined} arg the command-line argument. + * @returns {string} the directory to scan. + * @private + */ +function resolvePrebuildsDir(arg) { + if (arg === undefined) return join(root, 'prebuilds'); + const given = resolve(process.cwd(), arg); + try { + const nested = join(given, 'prebuilds'); + if (statSync(nested).isDirectory()) return nested; + } catch { + // No prebuilds/ inside it: the argument is the directory itself. + } + return given; +} + +const prebuildsDir = resolvePrebuildsDir(process.argv[2]); /** Tags node-gyp-build reads as a libc constraint (node-gyp-build.js). */ const LIBC_TAGS = new Set(['glibc', 'musl']); @@ -137,7 +160,10 @@ for (const entry of entries) { if (checked === 0) { problems.push( - `no *.node files found under ${prebuildsDir} — the prebuild produced nothing.`, + `no *.node files found under ${prebuildsDir} — nothing to check. ` + + 'Expected -/ subdirectories holding the addon ' + + '(pass either a prebuilds/ directory or the package root that ' + + 'contains one, and run `pnpm run prebuild` first).', ); } From 0216d5eb6a1875dfc494b23d64a472e855e0e13e Mon Sep 17 00:00:00 2001 From: Team AppThreat Date: Tue, 15 Sep 2026 02:46:11 +0100 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20round-3=20findings=20=E2=80=94=20par?= =?UTF-8?q?ameter-column=20affinity,=20shared-cache=20lock=20waits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 11. A hidden parameter was declared `"n" HIDDEN`, and sqlite parses a vtab declaration as ordinary CREATE TABLE text: the affinity is computed from the whole type string ("HIDDEN" names no affinity keyword, so it falls through to NUMERIC) and the token is only stripped afterwards. Parameter columns therefore carried NUMERIC affinity while every other column had none — on that column the text '2' equalled the integer 2 and sorted below '10', so a table-valued function keyed on a version or a hash compared its parameter numerically. They are declared `BLOB HIDDEN` now, which names the no-affinity rule explicitly and leaves the HIDDEN token where sqlite looks for it. Consequence worth documenting, and now tested: with no affinity to coerce it back, an echoed parameter must be the value as received — `String(n)` of an integer argument no longer satisfies the re-checked constraint. 12. Shared-cache pools surfaced raw SQLITE_LOCKED_SHAREDCACHE to a read that collided with the writer's open transaction, and the busy timeout could not help: sqlite never calls the busy handler for a shared-cache table lock. Such a read is now retried within the connection's busyTimeout budget, so it returns the committed data instead of losing the race; `busyTimeout: 0` still fails fast, a cancelled read stops retrying, and writes/exec are deliberately not retried (re-running a script or an already-landed statement is not safe). The locking model and all three consequences are documented next to the pool. Signed-off-by: Team AppThreat --- README.md | 9 ++-- docs/concurrency.md | 24 +++++++++++ lib/pool.d.ts | 7 +++ lib/pool.js | 7 +++ lib/sqlite3.d.ts | 6 ++- lib/sqlite3.js | 6 ++- lib/worker.js | 103 ++++++++++++++++++++++++++++++++++++-------- src/vtab.cc | 15 ++++++- test/pool.test.js | 67 ++++++++++++++++++++++++++++ test/vtab.test.js | 60 ++++++++++++++++++++++++++ 10 files changed, 278 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 740d138..8543a4a 100644 --- a/README.md +++ b/README.md @@ -855,9 +855,12 @@ A parameter is a real (hidden) column, and `sequence(5)` is exactly `WHERE count = 5` — **SQLite re-checks that predicate against every row the generator produces**, rather than trusting the generator to have applied it. So a row either leaves the parameter's column NULL — filled -with the argument, as above — or echoes the argument. A row reporting -anything else in that column contradicts the `WHERE` clause the argument -came from and is filtered out; put unrelated output in its own column. The +with the argument, as above — or echoes the argument **as it was +received**: parameter columns carry no affinity (like every other column +here), so an echoed `String(5)` is the text `'5'`, which does not equal +the integer `5`, and the row is filtered out. A row reporting anything +else in that column contradicts the `WHERE` clause the argument came from +and is filtered out too; put unrelated output in its own column. The generator is still free to pre-filter for speed, and should: a generator that ignores a constraint it cannot satisfy again produces an endless scan (correct, but unbounded — `LIMIT`, or a cancellation token on the diff --git a/docs/concurrency.md b/docs/concurrency.md index 99f3eb0..e81e848 100644 --- a/docs/concurrency.md +++ b/docs/concurrency.md @@ -192,6 +192,30 @@ turned off by itself for an in-memory pool (there is no journal to switch) and for a read-only URI, so both forms open under the default options. +**`cache=shared` locks per table, not per file.** It is the only way pool +workers can share an in-memory database, and its concurrency model is not +WAL's: while the writer's transaction is open, a reader touching the same +table gets `SQLITE_LOCKED_SHAREDCACHE` — and the busy timeout does not +cover it, because SQLite never calls the busy handler for a shared-cache +table lock (`sqlite3_unlock_notify` is the mechanism there, and it needs a +compile-time option this build does not carry). The pool therefore +**retries a read** that hits a table lock, spending the connection's +`busyTimeout` budget on it, so a read dispatched mid-transaction returns +the committed data instead of failing. Three consequences: + +- `busyTimeout: 0` keeps the fail-fast behaviour — the read rejects with + `SQLITE_LOCKED_SHAREDCACHE` immediately. +- A read outlives its normal latency when it collides with a long + transaction; it is waiting, not working. Cancelling it (`{ signal }`) + stops the retries. +- Writes and `exec` are **not** retried: re-running a script, or a + statement that may already have landed, is not safe. A shared-cache pool + under concurrent writes should keep transactions short. + +For a shared database that needs real reader/writer concurrency, use a +file in WAL mode — that is what the pool is shaped for. `cache=shared` is +for sharing an in-memory database across the pool's workers at all. + **What the pool does not do: spans.** Each worker loads its own instance of this module, so `sqlite3.subscribeQueries()` on the main thread sees nothing from `pool.read()`, `pool.write()` or `pool.exec()` — those spans diff --git a/lib/pool.d.ts b/lib/pool.d.ts index b082d8d..d918c09 100644 --- a/lib/pool.d.ts +++ b/lib/pool.d.ts @@ -288,6 +288,13 @@ declare class SqlitePool { * published on the worker's channels and never reach a main-thread * subscriber. See docs/concurrency.md. * + * With `cache=shared` (the only way the workers can share an in-memory + * database) SQLite locks per table, and a read colliding with the + * writer's open transaction fails with `SQLITE_LOCKED_SHAREDCACHE` + * outside the busy handler's reach — such a **read** is retried within + * the `busyTimeout` budget (`busyTimeout: 0` fails fast); writes and + * `exec` are not, since re-running them is not safe. + * * @param {string} filename the database file, or a `file:` URI. * @param {PoolOptions} [options] the pool options. * @returns {Promise} the opened pool. diff --git a/lib/pool.js b/lib/pool.js index a0e2138..6fa4944 100644 --- a/lib/pool.js +++ b/lib/pool.js @@ -1235,6 +1235,13 @@ function parsePoolUri(filename) { * published on the worker's channels and never reach a main-thread * subscriber. See docs/concurrency.md. * + * With `cache=shared` (the only way the workers can share an in-memory + * database) SQLite locks per table, and a read colliding with the + * writer's open transaction fails with `SQLITE_LOCKED_SHAREDCACHE` + * outside the busy handler's reach — such a **read** is retried within + * the `busyTimeout` budget (`busyTimeout: 0` fails fast); writes and + * `exec` are not, since re-running them is not safe. + * * @param {string} filename the database file, or a `file:` URI. * @param {PoolOptions} [options] the pool options. * @returns {Promise} the opened pool. diff --git a/lib/sqlite3.d.ts b/lib/sqlite3.d.ts index f3f59c3..467c4c5 100644 --- a/lib/sqlite3.d.ts +++ b/lib/sqlite3.d.ts @@ -195,8 +195,10 @@ export type VtabDefinition = { * predicate `WHERE param = arg`, which SQLite re-checks against every * row the generator yields — the generator is not trusted to have * applied it. A row therefore either leaves that column NULL (it is - * filled with the argument) or echoes the argument; a row reporting - * anything else there contradicts the predicate and is filtered out. + * filled with the argument) or echoes the argument *as received*: the + * column has no affinity, so an echoed `String(5)` is text and no + * longer equals the integer `5`. A row reporting anything else there + * contradicts the predicate and is filtered out. */ parameters?: string[]; }; diff --git a/lib/sqlite3.js b/lib/sqlite3.js index 565b25d..68bbec0 100644 --- a/lib/sqlite3.js +++ b/lib/sqlite3.js @@ -3590,8 +3590,10 @@ function vtabColumnName(spec, who) { * predicate `WHERE param = arg`, which SQLite re-checks against every * row the generator yields — the generator is not trusted to have * applied it. A row therefore either leaves that column NULL (it is - * filled with the argument) or echoes the argument; a row reporting - * anything else there contradicts the predicate and is filtered out. + * filled with the argument) or echoes the argument *as received*: the + * column has no affinity, so an echoed `String(5)` is text and no + * longer equals the integer `5`. A row reporting anything else there + * contradicts the predicate and is filtered out. * @since 9.1.0 */ diff --git a/lib/worker.js b/lib/worker.js index a39085a..575ecb4 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -46,6 +46,12 @@ let db = null; // default as db.cancellationToken(). const CANCEL_PERIOD = 1000; +// How long a read may keep retrying a shared-cache table lock: the +// connection's busy timeout, since that is the waiting budget the caller +// asked for (SQLite's own busy handler never fires for SQLITE_LOCKED — +// see runWithLockRetry). Replaced by the 'open' message's value. +let lockRetryBudget = 5000; + /** * Packs a query failure for the trip back: message, stack and the * SQLite diagnostics the driver attaches, none of which survive @@ -100,24 +106,7 @@ async function runQuery(msg) { installed = true; } try { - let value; - if (msg.method === 'exec') { - await connection.exec(msg.sql); - } else if (msg.method === 'run') { - value = await /** @type {(...args: unknown[]) => any} */ ( - connection.run - )(msg.sql, msg.params); - } else { - // get is all + rows[0], deliberately: an all() runs its - // statement to completion, so it can never leave a cursor - // mid-row holding the connection's WAL read snapshot open - // (a get() that returned a row does, until something resets - // it — stale reads for every later query on the reader). - const rows = await /** @type {(...args: unknown[]) => any} */ ( - connection.all - )(msg.sql, msg.params); - value = msg.method === 'get' ? rows[0] : rows; - } + const value = await runWithLockRetry(connection, msg); port.postMessage({ id: msg.id, kind: 'result', value }); } catch (err) { port.postMessage({ @@ -130,6 +119,83 @@ async function runQuery(msg) { } } +/** + * Runs one query, retrying a **read** that lost a shared-cache table lock. + * + * In shared-cache mode (`file:…?cache=shared`, the only way pool workers + * can share an in-memory database) a reader that starts while the writer + * holds a table lock fails with `SQLITE_LOCKED_SHAREDCACHE` — and the busy + * timeout does not apply, because SQLite never calls the busy handler for + * a shared-cache table lock: `sqlite3_unlock_notify` is the mechanism + * there, and it needs a compile-time option this build does not carry. The + * lock is held only for the writer's transaction, so the useful behaviour + * is to wait and try again, which is what the caller's busy timeout budget + * means. + * + * Only `all`/`get` are retried. Re-running an `exec` script, or a `run` + * that may have been the second statement of a transaction, could repeat + * work that already landed; a read repeats nothing. + * + * @param {import('./sqlite3-binding.js').Database} connection the connection. + * @param {{ method: 'all' | 'get' | 'run' | 'exec', sql: string, + * params?: unknown, cancel?: SharedArrayBuffer }} msg the request. + * @returns {Promise} the query's value. + * @private + */ +async function runWithLockRetry(connection, msg) { + const deadline = Date.now() + lockRetryBudget; + let delay = 1; + for (;;) { + try { + return await runOnce(connection, msg); + } catch (err) { + const locked = + /** @type {any} */ (err)?.primaryCode === 'SQLITE_LOCKED'; + const readOnlyQuery = msg.method === 'all' || msg.method === 'get'; + if (!locked || !readOnlyQuery || Date.now() >= deadline) throw err; + // A cancellation asked for this query to stop, not to wait. + if ( + msg.cancel instanceof SharedArrayBuffer && + Atomics.load(new Int32Array(msg.cancel), 0) !== 0 + ) { + throw err; + } + await new Promise((resolve) => setTimeout(resolve, delay)); + if (delay < 50) delay *= 2; + } + } +} + +/** + * One attempt at the request's query. + * + * @param {import('./sqlite3-binding.js').Database} connection the connection. + * @param {{ method: 'all' | 'get' | 'run' | 'exec', sql: string, + * params?: unknown }} msg the request. + * @returns {Promise} the query's value. + * @private + */ +async function runOnce(connection, msg) { + if (msg.method === 'exec') { + await connection.exec(msg.sql); + return undefined; + } + if (msg.method === 'run') { + return await /** @type {(...args: unknown[]) => any} */ ( + connection.run + )(msg.sql, msg.params); + } + // get is all + rows[0], deliberately: an all() runs its statement to + // completion, so it can never leave a cursor mid-row holding the + // connection's WAL read snapshot open (a get() that returned a row + // does, until something resets it — stale reads for every later query + // on the reader). + const rows = await /** @type {(...args: unknown[]) => any} */ ( + connection.all + )(msg.sql, msg.params); + return msg.method === 'get' ? rows[0] : rows; +} + /** * Opens the connection and applies the pool configuration. * @@ -189,6 +255,7 @@ async function open(msg) { /** @type {(...args: unknown[]) => unknown} */ ( /** @type {unknown} */ (connection.configure) )('busyTimeout', msg.busyTimeout); + lockRetryBudget = msg.busyTimeout; } if (msg.integerMode !== undefined) { /** @type {(...args: unknown[]) => unknown} */ ( diff --git a/src/vtab.cc b/src/vtab.cc index d1fd885..b97351c 100644 --- a/src/vtab.cc +++ b/src/vtab.cc @@ -506,6 +506,19 @@ struct VtabOps { // The parameters are a subset of the columns (the better-sqlite3 // contract): `parameters: ['n']` marks the column `n` HIDDEN, which is // what turns the module into a table-valued function. +// +// Parameters are declared `BLOB HIDDEN`, not bare `HIDDEN`. sqlite parses +// a vtab declaration as ordinary CREATE TABLE text and only *afterwards* +// strips the `hidden` token out of the type it recorded (see +// sqlite3VtabCallConnect) — the affinity was already computed from the +// full type string, and a type naming none of INT/CHAR/CLOB/TEXT/BLOB/ +// REAL/FLOA/DOUB gets NUMERIC affinity. So `"n" HIDDEN` gave parameter +// columns NUMERIC affinity while every other column had none: on that +// column text '2' compared equal to the integer 2 and '2' sorted below +// '10', so a table-valued function over a text domain (a version, a +// hash) silently compared its parameter numerically. `BLOB` names the +// no-affinity rule explicitly and leaves the HIDDEN token where sqlite +// looks for it (last, space-separated). static std::string BuildDeclareSql(const VtabModule* module) { std::string sql = "CREATE TABLE x("; for (size_t i = 0; i < module->columns.size(); i++) { @@ -513,7 +526,7 @@ static std::string BuildDeclareSql(const VtabModule* module) { sql += "\"" + module->columns[i] + "\""; for (const auto& param : module->params) { if (param == module->columns[i]) { - sql += " HIDDEN"; + sql += " BLOB HIDDEN"; break; } } diff --git a/test/pool.test.js b/test/pool.test.js index bd30c6d..6296675 100644 --- a/test/pool.test.js +++ b/test/pool.test.js @@ -510,6 +510,73 @@ describe('pool', function () { ); }); + it('waits out a shared-cache table lock instead of failing the read', { + timeout: 30000, + }, async function () { + // Shared cache is the only way pool workers can share an + // in-memory database, and it locks per table rather than per + // file: a read dispatched while the writer's transaction is open + // fails with SQLITE_LOCKED_SHAREDCACHE, and the busy timeout does + // not cover it (SQLite never calls the busy handler for a + // shared-cache table lock). The reader now retries within that + // same budget, so it sees the committed data instead of losing + // the race. + const shared = await sqlite3.pool('file::memory:?cache=shared', { + readers: 2, + }); + try { + await shared.exec('CREATE TABLE t (a)'); + await shared.write('INSERT INTO t VALUES (1)'); + const tx = shared.transaction(async (t) => { + await t.write('INSERT INTO t VALUES (2)'); + await new Promise((resolve) => setTimeout(resolve, 150)); + return 'committed'; + }); + // Dispatched while the transaction holds the table lock. + await new Promise((resolve) => setTimeout(resolve, 40)); + const reads = [ + shared.read('SELECT count(*) AS n FROM t'), + shared.get('SELECT count(*) AS n FROM t'), + ]; + assert.strictEqual(await tx, 'committed'); + const [all, one] = await Promise.all(reads); + assert.deepStrictEqual(all, [{ n: 2 }]); + assert.deepStrictEqual(one, { n: 2 }); + } finally { + await shared.close(); + } + }); + + it('surfaces the lock when the busy timeout budget is zero', { + timeout: 30000, + }, async function () { + // The retry spends the caller's busy-timeout budget, so + // busyTimeout: 0 keeps the old fail-fast behaviour — the escape + // hatch for a caller that would rather see the contention. + const shared = await sqlite3.pool('file::memory:?cache=shared', { + readers: 1, + busyTimeout: 0, + }); + try { + await shared.exec('CREATE TABLE t (a)'); + const tx = shared.transaction(async (t) => { + await t.write('INSERT INTO t VALUES (1)'); + await new Promise((resolve) => setTimeout(resolve, 120)); + }); + await new Promise((resolve) => setTimeout(resolve, 30)); + await assert.rejects( + shared.read('SELECT count(*) AS n FROM t'), + (err) => { + assert.strictEqual(err.primaryCode, 'SQLITE_LOCKED'); + return true; + }, + ); + await tx; + } finally { + await shared.close(); + } + }); + it('refuses :memory: and unknown options loudly', { timeout: 30000, }, async function () { diff --git a/test/vtab.test.js b/test/vtab.test.js index 5aa87ae..568874d 100644 --- a/test/vtab.test.js +++ b/test/vtab.test.js @@ -450,6 +450,66 @@ describe('virtual tables', function () { await db.wait(); }); + it('gives a parameter column no affinity, like every other column', async function () { + // sqlite parses a vtab declaration as CREATE TABLE text and only + // then strips the `hidden` token from the recorded type, so a bare + // `"p" HIDDEN` had already been given NUMERIC affinity (a type + // naming no affinity keyword falls through to it). On that column + // the text '2' compared equal to the integer 2 and sorted below + // '10' — a table-valued function over versions or hashes compared + // its parameter numerically. The declaration says BLOB HIDDEN now. + db.table('probe', { + columns: ['p', 'q'], + parameters: ['p'], + rows: function* (p) { + yield [p, p]; + }, + }); + await db.exec("CREATE TABLE plain (a); INSERT INTO plain VALUES ('2')"); + const affinity = await db.get( + "SELECT p = 2 AS pEq, p < '10' AS pLt, q = 2 AS qEq FROM probe('2')", + ); + const control = await db.get( + "SELECT a = 2 AS eq, a < '10' AS lt FROM plain", + ); + assert.deepStrictEqual(affinity, { pEq: 0, pLt: 0, qEq: 0 }); + assert.deepStrictEqual(control, { eq: 0, lt: 0 }); + // Text ordering is the ordinary one on both columns. + db.table('versions', { + columns: ['v', 'name'], + parameters: ['name'], + rows: function* (name) { + yield [name, name]; + }, + }); + assert.deepStrictEqual( + await db.all("SELECT v FROM versions('1.10') WHERE v > '1.9'"), + [], + "'1.10' must not sort above '1.9' numerically", + ); + }); + + it('requires an echoed parameter to keep the argument\u2019s type', async function () { + // The flip side of the affinity fix: with no affinity on the + // column, an echo must be the value as received. Stringifying an + // integer argument no longer compares equal, so the row is + // filtered — pinned because it used to "work" through NUMERIC + // affinity coercing it back. + db.table('echoes', { + columns: ['v', 'n'], + parameters: ['n'], + rows: function* (n) { + yield [1, n]; // as received: kept + yield [2, String(n)]; // stringified: no longer equal + yield [3]; // NULL, filled with n: kept + }, + }); + assert.deepStrictEqual(await db.all('SELECT v FROM echoes(5)'), [ + { v: 1 }, + { v: 3 }, + ]); + }); + it('reports a hidden parameter the generator did not yield', async function () { db.table('echo', { columns: ['value', 'n'],