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..8543a4a 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 @@ -807,6 +851,24 @@ 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 **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 +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 @@ -846,6 +908,26 @@ 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(); +``` + +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: @@ -860,9 +942,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..e81e848 100644 --- a/docs/concurrency.md +++ b/docs/concurrency.md @@ -171,6 +171,60 @@ 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`. 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. + +**`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 +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 `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..d918c09 100644 --- a/lib/pool.d.ts +++ b/lib/pool.d.ts @@ -271,7 +271,31 @@ 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 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. + * + * 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. * @throws {TypeError} when the filename is missing or malformed, or an @@ -285,6 +309,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..6fa4944 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, @@ -1154,6 +1166,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 +1218,31 @@ 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 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. + * + * 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. * @throws {TypeError} when the filename is missing or malformed, or an @@ -1181,21 +1256,70 @@ 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; + } + 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-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..467c4c5 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; @@ -189,6 +190,15 @@ 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 *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 9256bdf..68bbec0 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 */ @@ -3584,6 +3585,15 @@ 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 *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 */ @@ -3614,7 +3624,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]; * }, @@ -4724,6 +4735,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 +4769,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 +4789,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 +4800,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 +4811,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..575ecb4 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' } @@ -45,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 @@ -99,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({ @@ -129,18 +119,118 @@ 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. * - * @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) { - const 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 + // 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( @@ -165,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} */ ( @@ -174,7 +265,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/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..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(); @@ -522,6 +529,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/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/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/src/vtab.cc b/src/vtab.cc index 6c71dd5..b97351c 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; } @@ -502,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++) { @@ -509,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; } } @@ -637,11 +654,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 +678,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/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..6296675 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'; @@ -373,6 +373,210 @@ 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 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('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 () { @@ -380,6 +584,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..9398a60 --- /dev/null +++ b/test/prebuild_layout.test.js @@ -0,0 +1,250 @@ +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, /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', () => { + // 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/test/vtab.test.js b/test/vtab.test.js index 21f284f..568874d 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,152 @@ 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('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'], @@ -371,20 +568,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/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..e83e9b0 --- /dev/null +++ b/tools/check-prebuilds.mjs @@ -0,0 +1,180 @@ +#!/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 [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)), '..'); + +/** + * 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']); + +/** + * 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} — 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).', + ); +} + +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(); +}