From 0f8a1910d61177f408ccba1a183cdae96803e6bd Mon Sep 17 00:00:00 2001 From: Jordan Hafer <42755763+jjhafer@users.noreply.github.com> Date: Fri, 31 Jul 2026 11:41:56 -0400 Subject: [PATCH] fix: make observation a property of the database, not the handle (#53) Observation was something a caller could hold wrongly without ever being told. It lived in a field on DatabaseWrapper, but every clone of a wrapper shares one underlying database and one physical write connection, so a clone kept its own empty observer and silently took the unobserved writer. The plugin's primary Rust entry point handed back exactly such a clone, which meant a consumer could enable observation, subscribe, commit writes, and receive nothing - with every call along the way reporting success. The intent is to make that mistake unrepresentable rather than merely documented. Observation now belongs to the database itself, so whether two handles share it is no longer something a caller can get wrong: the answer is always yes, for every clone and every independent connect() to the same path. The slot holds only the broker, not a whole observable handle, because a handle carries a reference back to the database owning the slot - a cycle that would keep the database alive for the life of the process. Auditing what else could swallow a committed write drove the rest of the change, and the recurring theme was observation depending on the wrong object. Writes into attached databases bypassed the observer entirely. Once routed, they were still gated on the database a write was issued through rather than the one that owns the affected table, so an observed database could learn nothing about its own rows when another connection wrote them. Routing now follows ownership, and neither side of an attachment has to be observed for the other to work. The remaining fixes are about not stranding what callers cannot release themselves. An attached alias is bound to a pooled connection that nothing else will free - not the guards' Drop impls, not the pool's rollback hook - and the write pool holds a single connection, so one leaked alias wedges every later attach against that database. Every path that can fail while holding one now releases it, and aliases SQLite reserves or cannot disambiguate are refused before anything is attached rather than discovered midway through. Schema warming likewise no longer runs while holding the single write permit, since the work it does needs the read pool and the two could wait on each other until sqlx's acquire timeout broke the tie. Where a guarantee could not be made absolute, it is recorded rather than implied. The README's caveats and the relevant rustdoc now state what remains: readOnly is a locking mode and not an enforced restriction, temp tables and caller-written savepoints are not tracked, schema-dependent fields can arrive unresolved, the per-webview reference count does not cover a Rust caller, and a writer outliving an observation cycle keeps publishing to the broker it started with. That last one is pinned by an ignored test, so the invariant is executable rather than prose, and the accumulated observed-table set stays bounded only per call. parking_lot becomes a direct dependency of sqlx-sqlite-conn-mgr, which owns the observation slot. It was already compiled for that crate via sqlx and tokio, so the build graph and lockfile are unaffected. std::sync is unsuitable because the slot builds the broker while holding its own write lock, where a panic would poison the lock and leave the database permanently unobservable. Fixes issue #53. BREAKING CHANGE: TableChange and the JavaScript change payload gain a schema field reporting the schema a write occurred under. TableChange and hooks::PreUpdateEvent are also now non_exhaustive, so downstream struct literals and exhaustive destructuring patterns need updating. AttachedSpec::schema_name is capped at 64 bytes and additionally rejects main and temp, compared case-insensitively, as well as two specs sharing one alias; Error gains DuplicateSchemaName in sqlx-sqlite-conn-mgr and BrokerAliasCollision in sqlx-sqlite-observer, and neither enum is non_exhaustive, so exhaustive matches need new arms. Error::InvalidSchemaName's message text now lists every rule it enforces. Which code these reach the frontend as depends on whether observation is active: CONNECTION_ERROR when it is not, OBSERVER_ERROR when it is. subscribe() now rejects more than 100 tables in one call with INVALID_CONFIG, matching observe(); an empty tables list is still valid and still means no filter. DatabaseWrapper::observable() returns an owned handle rather than a borrow, since a borrow cannot escape the slot's lock; enable_observation() and disable_observation() take &self; close() and remove() take self; and ObservableWriteGuard::into_inner() returns an UnobservedWriter, as the guard may now hold either writer kind. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 44 + Cargo.lock | 1 + README.md | 109 +- crates/sqlx-sqlite-conn-mgr/Cargo.toml | 1 + crates/sqlx-sqlite-conn-mgr/src/attached.rs | 474 ++++++- crates/sqlx-sqlite-conn-mgr/src/database.rs | 26 + crates/sqlx-sqlite-conn-mgr/src/error.rs | 13 +- crates/sqlx-sqlite-conn-mgr/src/lib.rs | 4 +- .../sqlx-sqlite-conn-mgr/src/observer_slot.rs | 402 ++++++ crates/sqlx-sqlite-observer/src/broker.rs | 1 + crates/sqlx-sqlite-observer/src/change.rs | 21 + crates/sqlx-sqlite-observer/src/conn_mgr.rs | 446 ++++++- crates/sqlx-sqlite-observer/src/connection.rs | 8 +- crates/sqlx-sqlite-observer/src/error.rs | 18 + crates/sqlx-sqlite-observer/src/hooks.rs | 168 ++- crates/sqlx-sqlite-observer/src/lib.rs | 5 +- .../tests/conn_mgr_tests.rs | 1136 ++++++++++++++++- crates/sqlx-sqlite-toolkit/src/builders.rs | 127 +- crates/sqlx-sqlite-toolkit/src/lib.rs | 2 +- .../sqlx-sqlite-toolkit/src/transactions.rs | 30 +- crates/sqlx-sqlite-toolkit/src/wrapper.rs | 459 ++++++- .../tests/attached_detach_tests.rs | 343 +++++ .../tests/attached_observation_tests.rs | 311 +++++ .../tests/observation_tests.rs | 471 ++++++- guest-js/index.test.ts | 7 + guest-js/index.ts | 15 + src/commands.rs | 52 +- src/lib.rs | 56 + src/subscriptions.rs | 8 + 29 files changed, 4534 insertions(+), 224 deletions(-) create mode 100644 crates/sqlx-sqlite-conn-mgr/src/observer_slot.rs create mode 100644 crates/sqlx-sqlite-toolkit/tests/attached_detach_tests.rs create mode 100644 crates/sqlx-sqlite-toolkit/tests/attached_observation_tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d361a74..d45f962 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,12 @@ Databases must be registered on the Rust side with a stable key before they can - **`TransactionToken`**: **`dbPath`** → **`dbKey`**. - **`observe()` no longer resets observation.** It previously aborted every subscription for the database and rebuilt the observer; it is now additive and reference-counted (#54, see Fixed below). An app relying on that implicit reset now accumulates one **live** subscription per call, eventually failing with `TOO_MANY_SUBSCRIPTIONS`. **Migration:** `unsubscribe()` the previous subscription explicitly. - **`subscribe()` now requires the calling window to have called `observe()` itself**, failing with `OBSERVATION_NOT_ENABLED` otherwise. Previously a window could piggyback on another window's registration, then have its subscription silently aborted when that window released it. **Migration:** every window that subscribes must call `observe()` first. +- **`subscribe()` now rejects more than 100 tables in a single call** with `INVALID_CONFIG`, mirroring the cap `observe()` has always applied. `subscribe()` merges its `tables` into the same shared broker `observe()` does, so it grows the database's observed set, but it was previously unbounded. An empty `tables` is still valid and still means "no filter" — only the upper bound is new. **Migration:** split a subscription over more than 100 tables into several, or subscribe with no filter and discriminate on the client. Note this bounds a *single call*, not the accumulated set for a database, which remains unbounded (#56). - **`observe()` now rejects a conflicting `channelCapacity`/`captureValues`** with `OBSERVATION_CONFIG_CONFLICT` instead of silently ignoring them. Both are fixed by the first window to observe a database, since the broadcast channel behind them cannot be resized without dropping subscribers. Omitting either field inherits the active value; only an explicit request for a *different* value is rejected. +- **`TableChange` gains a `schema` field** (#53): `"main"` for the primary database, or the `.attach()` alias otherwise. Present on every change notification; code that destructures or type-checks the full shape of a `TableChange` object needs to account for it. +- **An attached database's `schemaName` is now capped at 64 characters**, failing above that. Aliases were already restricted to `[A-Za-z0-9_]` with no leading digit; only the length limit is new. It exists because the alias is now reported on every change notification (see the `schema` field above), so an unbounded one becomes per-event payload for every subscriber of the attached database. +- **`schemaName` values of `main`/`temp` (case-insensitively, so `MAIN`/`Temp` too) are now rejected**, since SQLite reserves both for its own schemas and `ATTACH ... AS "main"` fails outright. **Two attachments in the same call sharing one `schemaName`, compared case-insensitively, are now also rejected** (`"x"` and `"X"` collide at `ATTACH` even though they're different strings). Both previously surfaced as a raw SQLite `ATTACH` error and, worse, could strand every alias the same call had already attached (see Fixed below). No call site anywhere in this workspace used either pattern, so this is not expected to affect existing callers in practice. +- **Which error code an attached-database rejection reports depends on whether observation is active** — `CONNECTION_ERROR` when it is not, `OBSERVER_ERROR` when it is, because the observed acquisition path wraps the underlying connection-manager error. This applies to every `schemaName` rejection above (shape, the 64-character cap, the reserved names, and duplicate aliases). It is pre-existing behavior rather than new, but the entries above are the first to depend on it, and since observation is now database-wide (#53) it is reachable far more often than before: observation enabled by *any* window, or by an attached database alone, is enough. **Migration:** a frontend discriminating on the code for these failures must accept both. #### Rust API @@ -33,6 +38,18 @@ Databases must be registered on the Rust side with a stable key before they can - Invalid registration paths fail at startup (`INVALID_PATH`, `PATH_TRAVERSAL`); unregistered keys fail at open time (`PATH_NOT_REGISTERED`). - **`DatabaseWrapper::enable_observation()` no longer tears down the existing broker** (#54, see Fixed below); it reuses one additively. Callers who re-called it to shed subscribers, or to change `channel_capacity`/`capture_values` on a live database, must now call `disable_observation()` first. It stays infallible and only logs a conflict — and that log is compiled out in release, so read back `broker().channel_capacity()` / `.capture_values()` to confirm. - Added **`Error::ObservationConfigConflict`** (code `OBSERVATION_CONFIG_CONFLICT`). `Error` is not `#[non_exhaustive]`, so exhaustive matches on it need a new arm. +- **Observation is now database-wide rather than per-handle** (#53, see Fixed below), which changes several `sqlx-sqlite-toolkit`/`sqlx-sqlite-observer` signatures: + - **`DatabaseWrapper::enable_observation()` and `disable_observation()` now take `&self`** instead of `&mut self`. Existing call sites on a `let mut` binding keep compiling but will warn on `unused_mut`. + - **`DatabaseWrapper::observable()` now returns an owned `ObservableSqliteDatabase`** instead of `Option<&ObservableSqliteDatabase>`. `ObservableSqliteDatabase::clone()` only bumps two `Arc` refcounts, so this is not a deep copy, but a `let broker = db.observable().unwrap().broker();` pattern that borrows from the return value in one statement and uses it in a later one now fails to compile (E0716) — bind the `ObservableSqliteDatabase` to a variable first. + - **`DatabaseWrapper::close()` and `remove()` now take `self`** instead of `mut self`. Source-compatible for existing callers. + - **`ObservableWriteGuard::into_inner()` now returns the new `sqlx_sqlite_observer::UnobservedWriter` enum** (`Regular(WriteGuard)` or `Attached(AttachedWriteGuard)`) instead of unconditionally returning `WriteGuard`. Had no callers anywhere in this workspace prior to this release. + - **`sqlx_sqlite_observer::hooks::register_hooks()` now takes a schema-alias → broker `HashMap`** instead of a single `Arc`, to support routing attached-database writes to their owning database's broker. +- **`sqlx_sqlite_observer::TableChange` and `sqlx_sqlite_observer::hooks::PreUpdateEvent` each gain a `pub schema: String` field** (#53) — `"main"` for the primary database, the `ATTACH` alias otherwise. See `TableChange::schema`'s own docs for why it is provenance metadata rather than a stable identifier. **Both types are now also `#[non_exhaustive]`**, so downstream struct literals and exhaustive destructuring patterns need updating (read fields, or add `..` to patterns). Only this crate constructs either type, so in practice this affects code that pattern-matches their full shape. Deliberately unlike `Error` above, which stays exhaustive: these two are expected to gain fields again, and `#[non_exhaustive]` makes the next addition a non-event rather than another major bump. +- **`AttachedSpec::schema_name` is now capped at 64 bytes**, failing with `sqlx_sqlite_conn_mgr::Error::InvalidSchemaName` above that (which reaches the frontend as `CONNECTION_ERROR`, or `OBSERVER_ERROR` when observation is active — see the Frontend API note above). Shape was already validated (`[A-Za-z0-9_]`, no leading digit) but length was not, and the alias is now copied into every change notification for the owning database and serialized to each of its subscribers, once per changed row. +- **`AttachedSpec::schema_name` values of `main`/`temp` (case-insensitively) are now rejected with `Error::InvalidSchemaName`**, and two specs sharing an alias - compared case-insensitively - now fail with the new `Error::DuplicateSchemaName` instead of a raw SQLite `ATTACH` error (see Fixed below for why this also closes a stranding vector). +- **`Error::InvalidSchemaName`'s message now lists every rule it enforces** — non-empty, `[A-Za-z0-9_]` only, no leading digit, at most 64 bytes, and not the reserved `main`/`temp`. It previously named only the character-shape rules, so a rejected `main` (or an over-long alias) produced an error whose every stated condition the input satisfied. The variant is unchanged; only the message text is, which matters for anything matching on the string rather than the variant. +- Added **`sqlx_sqlite_conn_mgr::Error::DuplicateSchemaName`** (surfaces to the frontend the same way `InvalidSchemaName` does — `CONNECTION_ERROR`, or `OBSERVER_ERROR` when observation is active). `Error` is not `#[non_exhaustive]`, so exhaustive matches on it need a new arm. +- Added **`sqlx_sqlite_observer::Error::BrokerAliasCollision`** (feature-gated on `conn-mgr`). Like `sqlx_sqlite_conn_mgr::Error`, the observer's `Error` is not `#[non_exhaustive]`, so exhaustive matches need a new arm. It guards the alias → broker map against being built with a colliding key; `validate_attached_specs()` now rejects every input that could reach it, so it is defence-in-depth against a future regression in `ATTACH` ordering rather than a reachable error today. ### Added @@ -40,6 +57,16 @@ Databases must be registered on the Rust side with a stable key before they can - Parent directory auto-creation during registration validation for file paths. - CI check that committed `api-iife.js` matches a fresh Rollup build. +#### Rust API + +- **`sqlx_sqlite_toolkit::AttachedWriterGuard`** and **`DatabaseWrapper::acquire_writer_with_attached()`** — the attached-writer acquisition path that routes through the observer. +- **`sqlx_sqlite_conn_mgr::ObserverSlot`** (with `SqliteDatabase::observer_slot()`) — the database-scoped observation slot underlying the database-wide sharing described above. Populated only through `get_or_init()` / `get_or_init_with()`, both of which reuse rather than replace, so the slot cannot come to hold two different concrete types from safe code. `get_or_init_with()` additionally runs a caller-supplied merge callback under the same write lock that decided to reuse. +- **`sqlx_sqlite_observer::ObservableSqliteDatabase::acquire_writer_with_attached()`** and **`ObservableWriteGuard::detach_all()`**. +- **`sqlx_sqlite_observer::ObservableSqliteDatabase::from_broker()`** — rebuilds a handle from a database plus a broker. The observer slot stores `Arc` (a leaf type with no reference back to the database) rather than a whole handle, so the handle is rebuilt on demand at each read site. +- **`sqlx_sqlite_observer::UnobservedWriter`** — what `ObservableWriteGuard::into_inner()` now returns (see Breaking Changes). +- **`sqlx_sqlite_observer::acquire_writer_with_attached_brokers()`** — the attached-writer acquisition that `ObservableSqliteDatabase::acquire_writer_with_attached()` now delegates to, taking main's broker as an `Option` so the observed side can be an attached database alone (see Fixed). A free function rather than a method because `ObservableSqliteDatabase` is precisely what cannot be constructed when the main database is unobserved. +- **`sqlx_sqlite_conn_mgr::validate_attached_specs()`** — validates a batch of `AttachedSpec`s (shape, length, and the `main`/`temp` reservation for each alias, plus a case-insensitive cross-spec duplicate-alias check) before any lock is acquired or `ATTACH` issued. Both `acquire_reader_with_attached()` and `acquire_writer_with_attached()` call it first; it's also available to callers - like the observer crate's alias-to-broker routing - that derive per-alias state ahead of acquisition and need to validate before building that state from input SQLite would later reject anyway. + ### Changed #### `close` aborts active transactions before closing @@ -67,6 +94,23 @@ Transaction cleanup failures propagate as errors rather than being logged and ig - Closing a window without calling `unobserve()` no longer leaks its registration; it is released when the window is destroyed. - **Known limitation:** the 100-observed-table limit still bounds only a single `observe()` request, not the accumulated set for a database - see the README's Resource Limits section. Because the destructive teardown was the only incremental reset of that set, a nonexistent observed table now costs schema round trips on every writer acquisition indefinitely (#56). - **Known limitation:** observation is reference-counted per *webview*, not per caller. Two modules in the same window share one registration, so whichever calls `unobserve()` first tears down observation - and subscriptions - for both. A window needs a single owner of the `observe()`/`unobserve()` pair (#57). +- **Observation was attached to a handle, not a database (#53).** `DatabaseWrapper` is `Clone` and every clone shares one underlying database and one physical write connection, but observation lived in a per-clone field. A sibling clone - including what `app.connect(key)` returns to Rust callers - silently took the unobserved bypass path, so its committed writes fired no hooks and reached no subscriber. Observation now lives on the shared database itself, so every clone and every independent `connect()`/`load()` of the same file observes through the same broker. `:memory:` databases remain independently observed, since each one is its own database with nothing else able to open it. + - **Writes into attached databases are observed too, and are delivered to the *owning* database's subscribers.** A write to `other.users` (through `.attach()`) now notifies whichever database `other` points at, not the database the write was issued through - and this applies to *every* attach-capable operation (`execute`, `executeTransaction`, `beginInterruptibleTransaction`, and their Rust equivalents). Previously every one of those write paths bypassed the observer entirely, including when nothing was actually attached in read-write mode, because the main database's own writer went through the same unobserved path. Only `readWrite` attachments with their own observation enabled produce notifications; a `readOnly` attachment, or a `readWrite` one with no observer of its own, has its changes silently dropped rather than misrouted to the wrong subscribers. + - **A writer dropped mid-transaction without an explicit commit or rollback no longer leaks its buffered changes into the next transaction's commit** as phantom notifications for writes that were never kept. This was already possible before database-wide sharing, but became materially more likely once one broker serves every writer to a file, since any single careless caller could now poison every other consumer's stream. + - `acquire_regular_writer()` and migrations remain intentionally unobserved. Calling `acquire_regular_writer()` while observation is enabled now logs a `tracing::warn!` (compiled out in release builds) as a development-time aid. + - **Known limitation:** `readOnly` describes which locks are taken, not an enforced restriction. Attached databases are attached as a plain quoted path, so SQLite is never asked to reject writes to them; a write through a `readOnly` attachment lands *and* goes unobserved. Enforcing it (via a `file:...?mode=ro` URI) is a behavior change in `ATTACH` construction and is left as follow-up work. + - **Known limitation:** writes to `CREATE TEMP TABLE` objects never notify — `temp` has no owning database to route to and cannot be an attachment alias. Previously such a write could produce a *false* notification for an observed table of the same name, so this is a net improvement, but it is a silent drop either way. + - **Known limitation:** `SAVEPOINT`/`ROLLBACK TO` is not tracked. SQLite fires no rollback hook for `ROLLBACK TO` and the change buffer has no savepoint awareness, so rows undone by a `ROLLBACK TO` are still published when the outer transaction commits. This pre-dates the change. It requires savepoint SQL the caller wrote, inside a statement list passed to `executeTransaction()`/`beginInterruptibleTransaction()` (or their Rust equivalents) — consecutive `execute()` calls don't share a transaction, so a savepoint can't survive between them. + - **Attached-database observation no longer requires the main database to be observed too.** The whole observable acquisition path was gated on the *main* database's broker, so a write through an unobserved database into an observed `readWrite` attachment registered no hooks at all and reached that attachment's own subscribers not at all — silently, with every call reporting success. This was the same class of defect as #53 itself: observation depending on the wrong object. The observable path is now taken when *any* participating database is observed — main or a `readWrite` attachment — and `main` is simply omitted from the alias → broker map when the main database has no broker of its own, so its changes are dropped rather than misattributed. Nothing had covered this configuration, because the attached-observation tests enabled observation on both databases. + - **Known limitation:** a writer holds the broker it bound at acquisition for its whole lifetime, so a `disable_observation()` + `enable_observation()` cycle *during* an open transaction leaves that transaction publishing to the previous broker. Subscribers created before the cycle still receive its commit (the hook context keeps that broker alive); subscribers created after the cycle do not, while `is_observing()`, `subscribe()`, and the commit itself all report success. Reachable as: the last window `unobserve()`s and then `observe()`s again while another caller's interruptible transaction is still open. Pinned by an `#[ignore]`d test rather than only prose; the fix (reviving the slot's previous broker, gated on a lease held for the hooks' lifetime) is follow-up work. + - **Known limitation:** the plugin's `observe()`/`unobserve()` reference count is keyed on webview labels, so a Rust caller that enabled observation directly on a `DatabaseWrapper` is outside it. The last window releasing tears observation down database-wide and silently ends that caller's subscription; conversely a Rust `disable_observation()` breaks every window's live subscription while leaving their registrations non-zero. A plugin-level API for Rust observers is follow-up work. +- **`ObservableSqliteDatabase::acquire_writer()` no longer holds the single write permit while waiting on a read-pool connection.** It warmed each observed table's cached schema info *after* taking the permit, and that warm-up awaits the read pool (six connections by default). Six tasks each holding a read connection and then waiting for the writer would deadlock against it until sqlx's 30-second acquire timeout fired, failing up to seven of them with `PoolTimedOut`. Reachable from an ordinary query-then-write pattern, and on every acquisition — not just the first — whenever an observed table doesn't exist in the schema, since that result is never cached. The attached-writer path already warmed first; both now do. **The trade this makes:** a table that joins the observed set after warming — via `subscribe()`, `subscribe_stream()`, or a later `observe()`, including *mid-transaction*, since the preupdate hook checks the observed set live when a change fires rather than snapshotting it at hook registration — has its changes published with an empty `primary_key`, and a meaningless `rowid` if the table is `WITHOUT ROWID`, converging on the next writer acquisition. Re-checking after the permit is acquired would not close that window (it ends at end-of-transaction, not at permit acquisition) and would reintroduce the deadlock, because an observed name that doesn't resolve in the schema is never cached and so would take the read pool under the permit on every acquisition. +- **An attached-database query whose statement fails now still detaches.** `execute`, `fetch_all`, `fetch_one`, `fetch_page` and both transaction builders returned early on a statement error, or on a failed `BEGIN IMMEDIATE`, without issuing `DETACH`. (A failing `COMMIT` or `ROLLBACK` still strands the alias — a narrower case, since releasing it there means rolling back first, and the error to report becomes a judgement call. Tracked separately. This was not, however, the only remaining stranding vector - acquiring the attached connection or writer in the first place could strand an alias before a single query ever ran, see the next entry.) Neither the attached guards' `Drop` nor the pool's `after_release` hook detaches — `after_release` runs `ROLLBACK` only — so the alias stayed bound to the pooled connection. Since the write pool holds a single connection, every later attach of that alias then failed with "database ... is already in use" until the connection went genuinely idle (30 seconds by default), which a retry loop prevents indefinitely. +- **`acquire_reader_with_attached()` and `acquire_writer_with_attached()` no longer strand an alias when a later spec in the same call is rejected or fails to `ATTACH`.** Three related gaps in the same invariant as the entry above: + - **`main` and `temp` are now rejected as schema aliases**, case-insensitively (`MAIN`/`Temp` too), since SQLite reserves both for its own schemas and `ATTACH ... AS "main"` fails outright - and because the alias namespace is itself case-insensitive, `"MAIN"` fails the same way (see Breaking Changes). + - **Two specs sharing one alias are now rejected as `Error::DuplicateSchemaName`**, compared case-insensitively - `"x"` and `"X"` collide at `ATTACH` even though they compare unequal as plain strings, and previously passed the existing (path-keyed) duplicate check undetected (see Breaking Changes). + - **A rejected or failed spec now unwinds every alias already attached earlier in the same call**, issuing `DETACH` for each before returning the original error. Previously the attach loop returned on the first error without detaching anything already attached, and the reader path made this worse by validating each schema name *inside* the loop, so a single invalid or colliding alias could strand every alias attached before it in the same call. This is reachable with entirely valid, non-colliding input too: attaching more aliases than SQLite's default attach limit (`SQLITE_LIMIT_ATTACHED`, 10) fails on the one that exceeds it, and without this fix stranded the rest - disabling every future attached query against that connection, not just the alias involved, until it went genuinely idle. +- **A single `subscribe()` call can no longer grow a database's observed set without bound** (see Breaking Changes). `subscribe()` merges its `tables` into the shared broker exactly as `observe()` does, but only `observe()` applied the 100-table cap. Since #53 makes that broker shared across every handle to the file, one unbounded call's unresolvable names cost a schema round trip on *every* writer acquisition, for every caller, indefinitely — roughly 130µs per name, and never cached. Reachable from untrusted or buggy frontend input, which is what the README's Resource Limits section exists to bound. The accumulated set across calls is still unbounded (#56). - Finished subscriptions now remove their own tracking entry when their forwarding loop ends, so entries left over from a torn-down broker no longer count against the 100-subscriptions-per-database limit. **Known limitation:** this does not cover a reloaded or destroyed webview, where delivery from Rust still succeeds and the forwarding task keeps running (#58). Call `unsubscribe()` (or `unobserve()`) before navigating away or closing a window. - `remove()` no longer deletes the database files outside the registry write lock, where a concurrent `load()` could connect to the database being torn down and then have its files unlinked underneath it - leaving the frontend writing to unlinked inodes on Unix, or failing `remove()` with the pools already closed on Windows. - Regular transaction cleanup no longer uses string-prefix matching on database keys, which could abort transactions belonging to a different registered database when keys contain `:` (for example `:memory:` or `a` vs `a:b`). diff --git a/Cargo.lock b/Cargo.lock index e0e9c3b..2e329dd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3615,6 +3615,7 @@ dependencies = [ name = "sqlx-sqlite-conn-mgr" version = "0.9.0" dependencies = [ + "parking_lot", "serde", "sqlx", "tempfile", diff --git a/README.md b/README.md index 319eba8..1b63923 100644 --- a/README.md +++ b/README.md @@ -577,12 +577,16 @@ await db.executeTransaction([ * Read-write attachments acquire write locks on all involved databases * Attachments are connection-scoped and don't persist across queries * Main database is always accessible without a schema prefix + * A `readWrite` write into an attached database is observed too, and is + delivered to _that database's own_ subscribers, not the database the + write was issued through — see [Change Notifications](#change-notifications) ### Change Notifications Subscribe to real-time change notifications when rows are inserted, updated, or deleted. Changes are only published after transactions commit — you never see -partial or rolled-back data. +partial data, or data undone by a `ROLLBACK` of the transaction that wrote it. +(Savepoints you write yourself are the one exception — see the caveats below.) ```typescript // 1. Enable observation for specific tables @@ -651,12 +655,90 @@ await db.observe(['users'], { dropping subscribers. Omitting either field inherits the active value; an explicit _different_ value fails with `OBSERVATION_CONFIG_CONFLICT`. To change them, every window must `unobserve()` first + * **Observation is database-wide, not per-connection.** The broker belongs to + the underlying database file, keyed by its canonical path — not to any one + way of opening it. Every window that loads the same database key shares it, + and so does Rust code that opens the same file directly through + `sqlx-sqlite-toolkit`, outside this plugin entirely (a different + registration key for the same path is still the same broker). `:memory:` + databases are the one exception — each one is its own private database that + nothing else can open, so it's always independently observed + * **The reference count only covers webview windows.** Rust code that calls + `DatabaseWrapper::enable_observation()` directly is not registered + anywhere, so it sits outside the count: when the last window that called + `observe()` releases (explicitly, or by being destroyed), observation is + torn down for the whole file and the Rust consumer's subscription silently + ends without it having called anything. The reverse also holds — a Rust + `disable_observation()` breaks every window's live subscription while + leaving their registrations non-zero, so a later `observe()` builds a fresh + broker the old subscriptions are not bound to. Until there's a + plugin-level API for Rust observers, a Rust consumer that must not be torn + down needs its own database file (per the bullet above, a distinct key + alone won't do it) or must re-enable observation after a teardown * Multiple subscriptions can be active on the same database, each filtering by different tables * `lagged` events indicate the broadcast channel filled up before the subscriber could read — increase `channelCapacity` + * Each `TableChange`'s `schema` field reports where the change occurred — + `"main"` for the primary database, or the alias used in `.attach()` + otherwise (see [Cross-Database Queries](#cross-database-queries)). Treat + it as provenance metadata, not a stable identifier: an alias is chosen by + whoever attached the database, so the same physical database can appear + under different aliases depending on who's asking — it's only guaranteed + consistent for the change that reported it + * **`primaryKey`/`rowid` can arrive unresolved.** Both are looked up from a + per-table schema cache that's warmed lazily — on a writer's _next_ + acquisition after a table joins the observed set, not synchronously when + `observe()`/`subscribe()` returns. Until that next acquisition warms it, + a change notification for that table carries an empty `primaryKey`, and + a meaningless `rowid` if the table is `WITHOUT ROWID`. This is a general + property, not a narrow race, and it happens two ways: observing a table + before it exists in the schema at all (the likelier case in practice — + e.g. observing ahead of the migration that creates it), or adding a table + to the observed set while a write against it is already in flight, whose + acquisition already committed to whichever tables were warmed when it + started. Either way, it converges by the following writer acquisition. + Note also that the observed set is checked live when a change fires + rather than snapshotted at transaction start, so a `subscribe()` that + lands mid-transaction can still receive that same transaction's changes + — with unresolved schema info if it lands in one of the two cases above + * A write into an attached database only produces a notification if that + database is attached in `readWrite` mode **and** has its own observation + enabled — a `readOnly` attachment, or a `readWrite` one with no observer + of its own, has its changes silently dropped rather than misrouted to the + wrong subscribers. `readOnly` describes which locks are taken, not an + enforced restriction: SQLite is never asked to reject writes through such + an attachment, so one lands _and_ goes unobserved + * Writes to `CREATE TEMP TABLE` objects never notify — `temp` has no owning + database to route to and cannot be an attachment alias. This includes an + unqualified write that resolves to a temp table shadowing an observed one + * `SAVEPOINT`/`ROLLBACK TO` is not tracked. SQLite fires no rollback hook for + `ROLLBACK TO`, and the change buffer has no savepoint awareness, so rows + undone by a `ROLLBACK TO` are still published when the outer transaction + commits. The library never issues savepoint SQL itself, so this only + applies if you write it — and only inside a statement list passed to + `executeTransaction()` or `beginInterruptibleTransaction()`, since + consecutive `execute()` calls do not share a transaction (the write + connection is released and rolled back between them, taking any savepoint + with it) * Column values (`oldValues`, `newValues`) are typed as `ColumnValue` — a tagged union of `null`, `integer`, `real`, `text`, or `blob` (base64-encoded) + * Migrations never produce change notifications — schema changes aren't row + changes and have no `TableChange` representation. (Rust code using + `sqlx-sqlite-toolkit::DatabaseWrapper` directly has an additional, + JavaScript-unreachable bypass: `acquire_regular_writer()` opts a specific + writer out of observation entirely.) + * Observation only sees writes made through connections this library + manages, in this process. SQLite's preupdate hook is registered per + connection, so writes from another process — or any other tool touching + the same file — are invisible. This is a limit of how SQLite's hooks + work, not a bug, and there's no file-wide guarantee to fall back on + * Rust code using `sqlx-sqlite-observer` directly, without going through + `sqlx-sqlite-toolkit::DatabaseWrapper`, isn't automatically discoverable + as an attached-database's broker — registering a database so others can + find it that way is `DatabaseWrapper::enable_observation()`'s job. + Attached-database routing therefore requires the toolkit on the attached + side too, not just the side issuing the write ### Error Handling @@ -765,7 +847,8 @@ interface CustomConfig { interface AttachedDatabaseSpec { databaseKey: string; // Registration key of a database already loaded via load() - schemaName: string; // Schema name for accessing tables (e.g., 'orders') + schemaName: string; // Schema name for accessing tables (e.g., 'orders'). + // [A-Za-z0-9_] only, no leading digit, max 64 chars mode: 'readOnly' | 'readWrite'; } @@ -802,6 +885,7 @@ type ColumnValue = | { type: 'blob'; value: string }; // base64-encoded interface TableChange { + schema: string; // "main" or an attached alias - provenance only, not a stable id table: string; operation?: ChangeOperation; rowid?: number; @@ -1137,13 +1221,20 @@ untrusted or buggy frontend code: default (5 minutes) are automatically rolled back on the next access attempt (configurable via `Builder::transaction_timeout()`) * **Observer channel capacity**: Capped at 10,000 (default 256) - * **Observed tables**: Maximum 100 tables per single `observe()` call — **not** - a bound on the accumulated set for a database. `observe()` merges its tables - into the existing broker, `subscribe()` also adds tables with no per-call - limit, and nothing removes an individual table (the set is cleared only on a - full teardown), so the total is currently unbounded. An observed table that - does not exist also costs schema round trips on _every_ writer acquisition, - indefinitely, while that database's write connection is held (#56) + * **Observed tables**: Maximum 100 tables per single `observe()` or + `subscribe()` call — **not** a bound on the accumulated set for a + database. Both commands merge their tables into the same underlying + broker, and nothing ever removes an individual table from it (the set is + cleared only on a full teardown), so the accumulated total is currently + unbounded (#56). An observed table that does not exist also costs a + schema round trip on _every_ writer acquisition, indefinitely — that + round trip is paid before the write permit is acquired, not while it's + held, so an unresolvable name delays a write rather than extending how + long the connection is held once acquired. (Earlier versions of this + plugin warmed the schema cache after acquiring the write permit, which + paid this cost while the connection was held; that ordering was inverted + to avoid a deadlock between a saturated read pool and a pending writer - + see `ObservableSqliteDatabase::acquire_writer` in `sqlx-sqlite-observer`.) * **Subscriptions**: Maximum 100 active subscriptions per database ### Unbounded Result Sets diff --git a/crates/sqlx-sqlite-conn-mgr/Cargo.toml b/crates/sqlx-sqlite-conn-mgr/Cargo.toml index e74606d..81deaec 100644 --- a/crates/sqlx-sqlite-conn-mgr/Cargo.toml +++ b/crates/sqlx-sqlite-conn-mgr/Cargo.toml @@ -18,6 +18,7 @@ thiserror = "2.0.17" tokio = { version = "1.49.0", features = ["full"] } tracing = { version = "0.1.44", default-features = false, features = ["std", "release_max_level_off"] } serde = { version = "1.0.228", features = ["derive"] } +parking_lot = "0.12.3" [dev-dependencies] tempfile = "3.24.0" diff --git a/crates/sqlx-sqlite-conn-mgr/src/attached.rs b/crates/sqlx-sqlite-conn-mgr/src/attached.rs index e03629f..42d6db7 100644 --- a/crates/sqlx-sqlite-conn-mgr/src/attached.rs +++ b/crates/sqlx-sqlite-conn-mgr/src/attached.rs @@ -9,6 +9,7 @@ use sqlx::pool::PoolConnection; use sqlx::sqlite::SqliteConnection; use std::ops::{Deref, DerefMut}; use std::sync::Arc; +use tracing::error; /// Specification for attaching a database to a connection #[derive(Clone)] @@ -174,12 +175,23 @@ impl Drop for AttachedWriteGuard { } } +/// Longest accepted schema alias, in bytes. +/// +/// SQLite imposes no limit of its own (a 100k-character alias attaches happily), +/// but since observation started reporting the alias as `TableChange::schema` it +/// is copied into every change notification and serialized to every subscriber, +/// once per changed row. 64 is the conventional identifier ceiling and +/// comfortably above every alias this workspace uses. +const MAX_SCHEMA_NAME_LEN: usize = 64; + /// Validates that a schema name is a valid SQLite identifier /// /// A valid schema name: /// - Must not be empty /// - Must contain only ASCII alphanumeric characters and underscores /// - Must not start with a digit +/// - Must be at most [`MAX_SCHEMA_NAME_LEN`] bytes long +/// - Must not be `main` or `temp`, compared case-insensitively /// /// This prevents SQL injection by ensuring the schema name can only be used /// as an identifier and cannot: @@ -187,10 +199,76 @@ impl Drop for AttachedWriteGuard { /// - Start comments (--) /// - Break out of string context (') /// - Execute any SQL operations +/// +/// The `main`/`temp` rule is separate from injection-safety: SQLite owns both names +/// for its own schemas, so `ATTACH ... AS "main"` simply fails - and because SQLite's +/// schema namespace is case-insensitive, so does `ATTACH ... AS "MAIN"` or `"Temp"`. +/// Left unrejected here, that failure would happen inside the attach loop instead of +/// before it, stranding whatever alias was already attached earlier in the same call. fn is_valid_schema_name(name: &str) -> bool { !name.is_empty() + && name.len() <= MAX_SCHEMA_NAME_LEN && name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') && !name.chars().next().unwrap().is_ascii_digit() + && !name.eq_ignore_ascii_case("main") + && !name.eq_ignore_ascii_case("temp") +} + +/// Validates a batch of attached-database specs before any lock is acquired or any +/// `ATTACH` is issued. +/// +/// Exists so a caller that derives per-alias state ahead of acquisition - the observer +/// crate builds a schema-alias-to-broker map before it attaches anything - can validate +/// first and never build that state from input SQLite will later reject anyway. +/// +/// Checks, for every spec: +/// - the schema name itself, via [`is_valid_schema_name`] (shape, length, and the +/// `main`/`temp` reservation) +/// +/// and across the whole batch: +/// - that no two specs share an alias, compared via [`str::to_ascii_lowercase`] rather +/// than `==`, because SQLite's schema namespace is case-insensitive: `"x"` and `"X"` +/// collide at `ATTACH` even though they compare unequal as plain strings. +/// +/// Does not check for duplicate *database paths* - `acquire_reader_with_attached` and +/// `acquire_writer_with_attached` each do that themselves, since attaching the same +/// path is only a hard error for the writer (it would deadlock acquiring that +/// database's writer twice) and both need the paths sorted first anyway. +pub fn validate_attached_specs(specs: &[AttachedSpec]) -> Result<()> { + use std::collections::HashSet; + let mut seen_aliases = HashSet::new(); + for spec in specs { + if !is_valid_schema_name(&spec.schema_name) { + return Err(Error::InvalidSchemaName(spec.schema_name.clone())); + } + if !seen_aliases.insert(spec.schema_name.to_ascii_lowercase()) { + return Err(Error::DuplicateSchemaName(spec.schema_name.clone())); + } + } + Ok(()) +} + +/// Detaches a single already-attached alias while unwinding a partial `ATTACH` +/// failure - some earlier spec in the same call attached successfully before a later +/// one failed, and that earlier alias is still live on the connection or writer this +/// call is about to hand back with an error. +/// +/// Mirrors the error-precedence idiom in +/// `sqlx_sqlite_toolkit::builders::detach_after`: `original` is always what the caller +/// gets back. A failure here would only replace "why the attach failed" with "why the +/// cleanup of an attach you never got to use also failed", so it's logged instead of +/// propagated. +async fn detach_unwind(conn: &mut SqliteConnection, schema_name: &str, original: &Error) { + let detach_sql = format!("DETACH DATABASE \"{}\"", schema_name); + if let Err(detach_err) = sqlx::query(sqlx::AssertSqlSafe(detach_sql)) + .execute(conn) + .await + { + error!( + "failed to detach '{}' while unwinding from an earlier error ({}): {}", + schema_name, original, detach_err + ); + } } /// Acquire a read connection with attached database(s) @@ -210,6 +288,8 @@ fn is_valid_schema_name(name: &str) -> bool { /// # Errors /// /// Returns an error if: +/// - A schema name is invalid, reserved (`main`/`temp`), or shared by two specs +/// (case-insensitively) - see [`validate_attached_specs`] /// - The main database is closed /// - Cannot acquire a read connection /// - Attempting to attach read-write to a read connection @@ -218,6 +298,11 @@ pub async fn acquire_reader_with_attached( main_db: &SqliteDatabase, mut specs: Vec, ) -> Result { + // Validate every alias up front, before acquiring or attaching anything. Doing it + // here rather than inline in the loop below is what stops a bad spec later in the + // list from stranding a good one attached earlier in the same call. + validate_attached_specs(&specs)?; + // Acquire read connection from main database let mut conn = main_db.read_pool()?.acquire().await?; @@ -239,17 +324,19 @@ pub async fn acquire_reader_with_attached( } } - let mut schema_names = Vec::new(); + let mut schema_names: Vec = Vec::new(); for spec in specs { - // Validate schema name to prevent SQL injection - if !is_valid_schema_name(&spec.schema_name) { - return Err(Error::InvalidSchemaName(spec.schema_name.clone())); - } - - // Read connections can only attach as read-only + // Read connections can only attach as read-only. Schema names are already + // validated above, but earlier specs in this loop may already be attached to + // `conn` - unwind those before returning, or they'd strand on the connection + // this call is about to hand back with an error. if spec.mode == AttachedMode::ReadWrite { - return Err(Error::CannotAttachReadWriteToReader); + let original = Error::CannotAttachReadWriteToReader; + for attached in &schema_names { + detach_unwind(&mut conn, attached, &original).await; + } + return Err(original); } // Execute ATTACH DATABASE @@ -260,9 +347,16 @@ pub async fn acquire_reader_with_attached( "ATTACH DATABASE '{}' AS \"{}\"", escaped_path, spec.schema_name ); - sqlx::query(sqlx::AssertSqlSafe(attach_sql)) + if let Err(err) = sqlx::query(sqlx::AssertSqlSafe(attach_sql)) .execute(&mut *conn) - .await?; + .await + { + let original: Error = err.into(); + for attached in &schema_names { + detach_unwind(&mut conn, attached, &original).await; + } + return Err(original); + } schema_names.push(spec.schema_name); } @@ -290,6 +384,8 @@ pub async fn acquire_reader_with_attached( /// # Errors /// /// Returns an error if: +/// - A schema name is invalid, reserved (`main`/`temp`), or shared by two specs +/// (case-insensitively) - see [`validate_attached_specs`] /// - The main database is closed /// - Cannot acquire the main writer /// - Cannot acquire an attached database's writer (for read-write mode) @@ -298,12 +394,8 @@ pub async fn acquire_writer_with_attached( main_db: &SqliteDatabase, specs: Vec, ) -> Result { - // Validate schema names first - for spec in &specs { - if !is_valid_schema_name(&spec.schema_name) { - return Err(Error::InvalidSchemaName(spec.schema_name.clone())); - } - } + // Validate every alias first, before any lock is acquired or anything attached. + validate_attached_specs(&specs)?; // CRITICAL: To prevent deadlocks, we must acquire locks in a consistent global order. // Example deadlock without global ordering: @@ -353,7 +445,7 @@ pub async fn acquire_writer_with_attached( let held_writers = all_writers; // Execute ATTACH commands - let mut schema_names = Vec::new(); + let mut schema_names: Vec = Vec::new(); for spec in specs { let path = spec.database.path_str(); @@ -362,9 +454,19 @@ pub async fn acquire_writer_with_attached( "ATTACH DATABASE '{}' AS \"{}\"", escaped_path, spec.schema_name ); - sqlx::query(sqlx::AssertSqlSafe(attach_sql)) + if let Err(err) = sqlx::query(sqlx::AssertSqlSafe(attach_sql)) .execute(&mut *writer) - .await?; + .await + { + // Earlier specs in this loop may already be attached to `writer` - unwind + // those before returning the original error, or they'd strand on the write + // pool's single connection until it goes genuinely idle. + let original: Error = err.into(); + for attached in &schema_names { + detach_unwind(&mut writer, attached, &original).await; + } + return Err(original); + } schema_names.push(spec.schema_name); } @@ -504,6 +606,76 @@ mod tests { )); } + /// `test_attach_readwrite_to_reader_fails` above passes a single `ReadWrite` spec, + /// so `schema_names` is still empty when `CannotAttachReadWriteToReader` fires and + /// the unwind loop in `acquire_reader_with_attached` never runs its body. This test + /// reaches it: a `ReadOnly` spec attaches successfully first, then a `ReadWrite` + /// spec is rejected, and the unwind loop must detach the already-attached alias + /// before returning. + #[tokio::test] + async fn test_reader_unwinds_attached_alias_on_readwrite_rejection() { + let temp_dir = TempDir::new().unwrap(); + + // Pin to a single read connection so the connection this test's failed attempt + // touches is deterministically the one reused by the follow-up acquisition + // below - see test_reader_unwinds_partial_attach_on_limit_error for the same + // reasoning (otherwise which pooled connection gets reused, and therefore + // whether a stranded alias would even be visible, is a coin flip). + let main_db = SqliteDatabase::connect( + temp_dir.path().join("main.db"), + Some(crate::SqliteDatabaseConfig { + max_read_connections: 1, + ..Default::default() + }), + ) + .await + .unwrap(); + + let ro_db = create_test_db("a_ro.db", &temp_dir).await; + let rw_db = create_test_db("z_rw.db", &temp_dir).await; + + // Load-bearing filenames: `acquire_reader_with_attached` sorts specs by + // database *path* before the loop runs, regardless of the order given here. + // "a_ro.db" must sort before "z_rw.db" so the `ReadOnly` spec is attached + // first, making `schema_names` non-empty by the time the `ReadWrite` spec is + // rejected. Renaming these files without preserving that ordering would make + // this test pass vacuously, exercising the same empty-unwind-loop path as + // `test_attach_readwrite_to_reader_fails`. + let specs = vec![ + AttachedSpec { + database: ro_db, + schema_name: "a_ro".to_string(), + mode: AttachedMode::ReadOnly, + }, + AttachedSpec { + database: rw_db, + schema_name: "z_rw".to_string(), + mode: AttachedMode::ReadWrite, + }, + ]; + + let result = acquire_reader_with_attached(&main_db, specs).await; + assert!(matches!( + result.unwrap_err(), + Error::CannotAttachReadWriteToReader + )); + + // Load-bearing: without unwinding "a_ro" before returning, it would still be + // live on the pool's single reused connection, and a freshly acquired reader + // would see it alongside "main" instead of "main" alone. + let mut conn = main_db.read_pool().unwrap().acquire().await.unwrap(); + let rows = sqlx::query("PRAGMA database_list") + .fetch_all(&mut *conn) + .await + .unwrap(); + let names: Vec = rows.iter().map(|row| row.get::("name")).collect(); + assert_eq!( + names, + vec!["main".to_string()], + "attached alias 'a_ro' should have been unwound, leaving only 'main'" + ); + } + #[tokio::test] async fn test_attach_multiple_databases() { let temp_dir = TempDir::new().unwrap(); @@ -754,6 +926,7 @@ mod tests { let other_db = create_test_db("other.db", &temp_dir).await; // Test various invalid schema names + let too_long = "a".repeat(MAX_SCHEMA_NAME_LEN + 1); let invalid_names = vec![ "", // Empty "123invalid", // Starts with digit @@ -762,6 +935,7 @@ mod tests { "schema;DROP TABLE users", // SQL injection attempt "schema'--", // SQL injection attempt "schema/*comment*/", // Contains special chars + &too_long, // Longer than MAX_SCHEMA_NAME_LEN ]; for invalid_name in invalid_names { @@ -780,6 +954,28 @@ mod tests { } } + /// Pins the accepting side of the length cap - the rejecting side is covered + /// above. An alias exactly at the limit must still attach, or the cap is + /// off-by-one. + #[tokio::test] + async fn test_schema_name_at_max_length_accepted() { + let temp_dir = TempDir::new().unwrap(); + let main_db = create_test_db("main.db", &temp_dir).await; + let other_db = create_test_db("other.db", &temp_dir).await; + + let at_limit = "a".repeat(MAX_SCHEMA_NAME_LEN); + let specs = vec![AttachedSpec { + database: other_db.clone(), + schema_name: at_limit.clone(), + mode: AttachedMode::ReadOnly, + }]; + + let conn = acquire_reader_with_attached(&main_db, specs) + .await + .expect("an alias exactly at the length limit should be accepted"); + drop(conn); + } + #[tokio::test] async fn test_duplicate_attached_database_rejected() { let temp_dir = TempDir::new().unwrap(); @@ -855,4 +1051,244 @@ mod tests { "Should attach database with single quote in path" ); } + + /// Asserts that nothing besides `main` is attached on a freshly acquired writer - + /// i.e. that no earlier failed attach left an alias stranded on the write pool's + /// single pooled connection. + async fn assert_only_main_attached(main_db: &SqliteDatabase) { + let mut writer = main_db.acquire_writer().await.unwrap(); + let rows = sqlx::query("PRAGMA database_list") + .fetch_all(&mut *writer) + .await + .unwrap(); + let names: Vec = rows.iter().map(|row| row.get::("name")).collect(); + assert_eq!( + names, + vec!["main".to_string()], + "no alias should remain attached on a fresh writer" + ); + } + + #[tokio::test] + async fn test_reserved_schema_names_rejected() { + let temp_dir = TempDir::new().unwrap(); + let main_db = create_test_db("main.db", &temp_dir).await; + let other_db = create_test_db("other.db", &temp_dir).await; + + // SQLite's schema namespace is case-insensitive, so "main"/"temp" must be + // rejected regardless of case, and on both the reader and writer paths. + for reserved in ["main", "temp", "MAIN", "Temp"] { + let specs = vec![AttachedSpec { + database: other_db.clone(), + schema_name: reserved.to_string(), + mode: AttachedMode::ReadOnly, + }]; + let result = acquire_reader_with_attached(&main_db, specs).await; + assert!( + matches!(result, Err(Error::InvalidSchemaName(_))), + "reader should reject reserved alias '{}', got {:?}", + reserved, + result.err() + ); + + let specs = vec![AttachedSpec { + database: other_db.clone(), + schema_name: reserved.to_string(), + mode: AttachedMode::ReadOnly, + }]; + let result = acquire_writer_with_attached(&main_db, specs).await; + assert!( + matches!(result, Err(Error::InvalidSchemaName(_))), + "writer should reject reserved alias '{}', got {:?}", + reserved, + result.err() + ); + } + + // Load-bearing: every rejection above happens in `validate_attached_specs`, + // before any lock is acquired or any `ATTACH` is issued, so nothing should be + // left attached and a normal alias should still attach cleanly afterward. + assert_only_main_attached(&main_db).await; + let specs = vec![AttachedSpec { + database: other_db.clone(), + schema_name: "other".to_string(), + mode: AttachedMode::ReadOnly, + }]; + assert!( + acquire_writer_with_attached(&main_db, specs).await.is_ok(), + "a normal alias should still attach after the reserved-name rejections above" + ); + } + + #[tokio::test] + async fn test_duplicate_schema_name_different_paths_rejected() { + let temp_dir = TempDir::new().unwrap(); + let main_db = create_test_db("main.db", &temp_dir).await; + let db_b = create_test_db("b.db", &temp_dir).await; + let db_c = create_test_db("c.db", &temp_dir).await; + + // Two different files sharing one alias pass the (path-keyed) duplicate-path + // check but must still be rejected before any `ATTACH` runs. + let specs = vec![ + AttachedSpec { + database: db_b.clone(), + schema_name: "x".to_string(), + mode: AttachedMode::ReadOnly, + }, + AttachedSpec { + database: db_c.clone(), + schema_name: "x".to_string(), + mode: AttachedMode::ReadOnly, + }, + ]; + let result = acquire_writer_with_attached(&main_db, specs).await; + assert!( + matches!(result, Err(Error::DuplicateSchemaName(_))), + "two different databases sharing alias 'x' should be rejected, got {:?}", + result.err() + ); + + // Load-bearing: the rejection above must not have attached either database, so + // the alias 'x' is still free to use afterward. + assert_only_main_attached(&main_db).await; + let specs = vec![AttachedSpec { + database: db_b.clone(), + schema_name: "x".to_string(), + mode: AttachedMode::ReadOnly, + }]; + assert!( + acquire_writer_with_attached(&main_db, specs).await.is_ok(), + "alias 'x' should attach cleanly after the rejected duplicate-alias pair" + ); + } + + #[tokio::test] + async fn test_duplicate_schema_name_case_insensitive_rejected() { + let temp_dir = TempDir::new().unwrap(); + let main_db = create_test_db("main.db", &temp_dir).await; + let db_x = create_test_db("x.db", &temp_dir).await; + let db_upper_x = create_test_db("upper_x.db", &temp_dir).await; + + // "x" and "X" compare unequal as plain strings but collide in SQLite's + // case-insensitive schema namespace - a plain `HashSet` would miss this. + let specs = vec![ + AttachedSpec { + database: db_x.clone(), + schema_name: "x".to_string(), + mode: AttachedMode::ReadOnly, + }, + AttachedSpec { + database: db_upper_x.clone(), + schema_name: "X".to_string(), + mode: AttachedMode::ReadOnly, + }, + ]; + let result = acquire_writer_with_attached(&main_db, specs).await; + assert!( + matches!(result, Err(Error::DuplicateSchemaName(_))), + "the 'x'/'X' pair should be rejected as a case-insensitive duplicate, got {:?}", + result.err() + ); + + assert_only_main_attached(&main_db).await; + let specs = vec![AttachedSpec { + database: db_x.clone(), + schema_name: "x".to_string(), + mode: AttachedMode::ReadOnly, + }]; + assert!( + acquire_writer_with_attached(&main_db, specs).await.is_ok(), + "alias 'x' should attach cleanly after the rejected 'x'/'X' pair" + ); + } + + /// Builds `count` distinct single-table databases with distinct schema aliases, + /// suitable for pushing past SQLite's default `SQLITE_LIMIT_ATTACHED` (10). + async fn build_many_attach_specs( + count: usize, + temp_dir: &TempDir, + ) -> Vec { + let mut specs = Vec::with_capacity(count); + for i in 0..count { + let db = create_test_db(&format!("many{i}.db"), temp_dir).await; + specs.push(AttachedSpec { + database: db, + schema_name: format!("many{i}"), + mode: AttachedMode::ReadOnly, + }); + } + specs + } + + #[tokio::test] + async fn test_writer_unwinds_partial_attach_on_limit_error() { + let temp_dir = TempDir::new().unwrap(); + let main_db = create_test_db("main.db", &temp_dir).await; + + // 11 distinct valid aliases exceeds SQLite's default attach limit of 10, so the + // 11th `ATTACH` fails after the first 10 already succeeded on this writer. + let specs = build_many_attach_specs(11, &temp_dir).await; + let result = acquire_writer_with_attached(&main_db, specs).await; + assert!( + result.is_err(), + "attaching 11 databases should exceed SQLite's default attach limit" + ); + + // Load-bearing: without unwinding the 10 aliases that attached before the 11th + // failed, they would still be live on the write pool's single connection, and + // this attach of a brand-new, previously-unused alias would fail too. + let unused_db = create_test_db("unused.db", &temp_dir).await; + let specs = vec![AttachedSpec { + database: unused_db, + schema_name: "unused".to_string(), + mode: AttachedMode::ReadOnly, + }]; + let follow_up = acquire_writer_with_attached(&main_db, specs).await; + assert!( + follow_up.is_ok(), + "a later attach of an unused alias must succeed once the failed attempt has \ + unwound: {:?}", + follow_up.err() + ); + } + + #[tokio::test] + async fn test_reader_unwinds_partial_attach_on_limit_error() { + let temp_dir = TempDir::new().unwrap(); + + // At the default of 6 read connections, whether the wedged connection is the one + // reused by the follow-up attach is a coin flip (measured 5/12 and 6/12 in + // practice). Pinning the pool to a single connection makes reuse - and therefore + // this test - deterministic. + let main_db = SqliteDatabase::connect( + temp_dir.path().join("main.db"), + Some(crate::SqliteDatabaseConfig { + max_read_connections: 1, + ..Default::default() + }), + ) + .await + .unwrap(); + + let specs = build_many_attach_specs(11, &temp_dir).await; + let result = acquire_reader_with_attached(&main_db, specs).await; + assert!( + result.is_err(), + "attaching 11 databases should exceed SQLite's default attach limit" + ); + + let unused_db = create_test_db("unused.db", &temp_dir).await; + let specs = vec![AttachedSpec { + database: unused_db, + schema_name: "unused".to_string(), + mode: AttachedMode::ReadOnly, + }]; + let follow_up = acquire_reader_with_attached(&main_db, specs).await; + assert!( + follow_up.is_ok(), + "a later attach of an unused alias must succeed once the failed attempt has \ + unwound: {:?}", + follow_up.err() + ); + } } diff --git a/crates/sqlx-sqlite-conn-mgr/src/database.rs b/crates/sqlx-sqlite-conn-mgr/src/database.rs index a48a86b..642c127 100644 --- a/crates/sqlx-sqlite-conn-mgr/src/database.rs +++ b/crates/sqlx-sqlite-conn-mgr/src/database.rs @@ -3,6 +3,7 @@ use crate::Result; use crate::config::SqliteDatabaseConfig; use crate::error::Error; +use crate::observer_slot::ObserverSlot; use crate::registry::{get_or_open_database, is_memory_database, uncache_database}; use crate::write_guard::WriteGuard; use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; @@ -64,6 +65,15 @@ pub struct SqliteDatabase { /// Path to database file (used for cleanup and registry lookups) path: PathBuf, + + /// Type-erased slot for a higher layer's observation state. + /// + /// Living here - rather than in a side registry or on the wrapper that calls + /// `connect()` - means every handle that resolves to this same `SqliteDatabase` + /// (clones of a wrapper, or independent `connect()` calls to the same path) + /// shares one observation state. See [`ObserverSlot`] for the type-erasure + /// mechanics. + observer_slot: ObserverSlot, } impl SqliteDatabase { @@ -74,6 +84,14 @@ impl SqliteDatabase { self.path.to_string_lossy().to_string() } + /// Get this database's observation slot. + /// + /// Opaque to this crate - see [`ObserverSlot`] for what it's for and how a + /// higher layer is expected to use it. + pub fn observer_slot(&self) -> &ObserverSlot { + &self.observer_slot + } + /// Connect to a SQLite database /// /// If the database is already connected, returns the existing connection. @@ -220,6 +238,7 @@ impl SqliteDatabase { wal_initialized: AtomicBool::new(false), closed: AtomicBool::new(false), path: path.clone(), + observer_slot: ObserverSlot::default(), }) }) .await @@ -370,6 +389,13 @@ impl SqliteDatabase { // Mark as closed self.closed.store(true, Ordering::SeqCst); + // Hygiene only: the slot holds a leaf value with no reference back to this + // database, so the field would drop on its own once this `Arc` does. + // Clearing here just releases it - and, transitively, any subscribers - + // promptly rather than whenever the last strong reference happens to go + // away. + self.observer_slot.clear(); + // Remove from registry if let Err(e) = uncache_database(&self.path).await { error!("Failed to remove database from cache: {}", e); diff --git a/crates/sqlx-sqlite-conn-mgr/src/error.rs b/crates/sqlx-sqlite-conn-mgr/src/error.rs index debdeb8..3a11fbd 100644 --- a/crates/sqlx-sqlite-conn-mgr/src/error.rs +++ b/crates/sqlx-sqlite-conn-mgr/src/error.rs @@ -26,9 +26,11 @@ pub enum Error { #[error("Cannot attach database as read-write to a read-only connection")] CannotAttachReadWriteToReader, - /// Invalid schema name provided for attached database + /// Invalid schema name provided for attached database. See + /// `attached::is_valid_schema_name` for the authoritative rule set this message + /// must stay in sync with. #[error( - "Invalid schema name '{0}': must contain only alphanumeric characters and underscores, and cannot start with a digit" + "Invalid schema name '{0}': must be non-empty, contain only alphanumeric characters and underscores, not start with a digit, be at most 64 bytes long, and not be the reserved name 'main' or 'temp' (case-insensitive)" )] InvalidSchemaName(String), @@ -37,4 +39,11 @@ pub enum Error { "Database '{0}' appears multiple times in attached database list (would cause deadlock)" )] DuplicateAttachedDatabase(String), + + /// Two attached-database specs used the same schema alias. Compared + /// case-insensitively, matching SQLite's own schema namespace - a spec named `"x"` + /// and one named `"X"` collide at `ATTACH` even though they compare unequal as + /// plain strings. + #[error("Schema name '{0}' is used by more than one attached database")] + DuplicateSchemaName(String), } diff --git a/crates/sqlx-sqlite-conn-mgr/src/lib.rs b/crates/sqlx-sqlite-conn-mgr/src/lib.rs index d3bc2b3..37a3d1a 100644 --- a/crates/sqlx-sqlite-conn-mgr/src/lib.rs +++ b/crates/sqlx-sqlite-conn-mgr/src/lib.rs @@ -64,17 +64,19 @@ mod attached; mod config; mod database; mod error; +mod observer_slot; mod registry; mod write_guard; // Re-export public types pub use attached::{ AttachedMode, AttachedReadConnection, AttachedSpec, AttachedWriteGuard, - acquire_reader_with_attached, acquire_writer_with_attached, + acquire_reader_with_attached, acquire_writer_with_attached, validate_attached_specs, }; pub use config::SqliteDatabaseConfig; pub use database::SqliteDatabase; pub use error::Error; +pub use observer_slot::ObserverSlot; pub use write_guard::WriteGuard; // Re-export sqlx migrate types for convenience diff --git a/crates/sqlx-sqlite-conn-mgr/src/observer_slot.rs b/crates/sqlx-sqlite-conn-mgr/src/observer_slot.rs new file mode 100644 index 0000000..4e27607 --- /dev/null +++ b/crates/sqlx-sqlite-conn-mgr/src/observer_slot.rs @@ -0,0 +1,402 @@ +//! Opaque, database-scoped slot for a higher layer's observation state. +//! +//! `sqlx-sqlite-conn-mgr` is the lowest crate in the observation stack and cannot +//! name the observer crate's types without an upward dependency, so this slot is +//! type-erased over `Arc`. Higher layers (currently +//! `sqlx-sqlite-toolkit`'s `DatabaseWrapper`, which stores an +//! `Arc`) put a value here and downcast on read. +//! +//! Hanging the slot off [`SqliteDatabase`](crate::SqliteDatabase) rather than a +//! side registry means every handle that shares the same `Arc` - +//! clones of a wrapper and independent `connect()` calls to the same path alike - +//! shares one observation state (see issue #53). `:memory:` databases are excluded +//! from the path registry (`registry.rs`) and therefore never share a +//! `SqliteDatabase`, so they get independent observation for free with no special +//! casing here. + +use std::any::Any; +use std::sync::Arc; + +use parking_lot::RwLock; +use tracing::warn; + +/// Database-scoped slot holding at most one type-erased value. +/// +/// `sqlx-sqlite-conn-mgr` never interprets the contents - it only stores and +/// hands back the `Arc` a higher layer gave it. The only way to *populate* the +/// slot is [`get_or_init`](Self::get_or_init) or +/// [`get_or_init_with`](Self::get_or_init_with), both of which reuse whatever is +/// already there instead of overwriting it, so nothing can replace a stored +/// value with one of another type ([`clear`](Self::clear) can only empty the +/// slot). A caller can still populate two different `T`s across a clear, which +/// remains a programming error rather than a supported use case; see +/// [`get`](Self::get) for the never-panicking behavior when it happens. +#[derive(Default)] +pub struct ObserverSlot(RwLock>>); + +impl ObserverSlot { + /// Empties the slot. + pub fn clear(&self) { + *self.0.write() = None; + } + + /// Returns whether the slot currently holds a value, regardless of its type. + pub fn is_set(&self) -> bool { + self.0.read().is_some() + } + + /// Returns the slot's value downcast to `T`, or `None` if the slot is empty. + /// + /// Clones the `Arc` out and drops the internal lock guard before returning, + /// so no guard is ever observable to the caller or held across an `.await`. + /// + /// If the slot holds a value that is not a `T` - a programming error, since + /// this slot is meant to hold one concrete type for its whole lifetime - + /// this returns `None` rather than a wrong-typed value, after a + /// `tracing::warn!`. It never panics, in debug or release. + pub fn get(&self) -> Option> { + let value = self.0.read().clone()?; + match value.downcast::() { + Ok(typed) => Some(typed), + Err(_) => { + warn!( + "ObserverSlot::get() requested a type that does not match the value \ + already stored in the slot; returning None instead of a wrong-typed \ + value. This indicates a programming error - the slot should only ever \ + hold one concrete type." + ); + None + } + } + } + + /// Atomically returns the existing value downcast to `T`, or creates one via + /// `init` and stores it, if the slot is empty. The returned `bool` is `true` + /// when `init` ran (a new value was created), `false` when an existing value + /// was reused. + /// + /// The whole check-and-create happens under the slot's write lock, so two + /// concurrent callers can never both observe an empty slot and each build and + /// store their own value - the second caller always sees the first's value + /// instead of silently overwriting it (and, with it, any subscribers already + /// registered against the value it replaced). + /// + /// `init` runs synchronously while the write lock is held. It must not touch + /// this slot (no reentrancy - the lock is not reentrant), block, or panic, + /// since any of the three stalls or fails every other reader/writer of this + /// database's observation state. A panic is not merely theoretical: the + /// toolkit's `enable_observation()` builds an `ObservationBroker` in `init`, + /// and that constructor asserts a non-zero channel capacity, so a direct + /// Rust caller passing `ObserverConfig::with_channel_capacity(0)` unwinds + /// out of an otherwise infallible call. The slot survives it intact: this is + /// a `parking_lot::RwLock`, which does not poison, so the lock is released + /// on unwind and the slot is simply left empty - as if the call never + /// happened - rather than permanently unusable. + /// + /// Same downcast-mismatch behavior as [`get`](Self::get): if the slot already + /// holds a value of some other type, this returns `None` rather than a + /// wrong-typed value or a second, competing value of type `T`. + /// + /// A thin wrapper around [`get_or_init_with`](Self::get_or_init_with), which + /// takes a callback for the reuse case as well. + pub fn get_or_init(&self, init: F) -> Option<(Arc, bool)> + where + T: Any + Send + Sync, + F: FnOnce() -> Arc, + { + self.get_or_init_with(init, |_| {}) + } + + /// Same as [`get_or_init`](Self::get_or_init), but runs `on_existing` when an + /// existing value is reused, still under the write lock that decided "reuse, + /// don't create". + /// + /// That closes a window a caller can't close itself: acting on the existing + /// value after this returns (merging new entries into a broker already stored + /// here, say) leaves room for a concurrent [`clear`](Self::clear) in between, + /// so the follow-up mutates a value the slot no longer holds. + /// + /// `on_existing` carries the same restrictions as `init` - no reentrancy, no + /// blocking, no panicking. Exactly one of the two runs per call. + pub fn get_or_init_with(&self, init: F, on_existing: G) -> Option<(Arc, bool)> + where + T: Any + Send + Sync, + F: FnOnce() -> Arc, + G: FnOnce(&Arc), + { + let mut guard = self.0.write(); + + if let Some(existing) = guard.as_ref() { + return match Arc::clone(existing).downcast::() { + Ok(typed) => { + on_existing(&typed); + Some((typed, false)) + } + Err(_) => { + warn!( + "ObserverSlot::get_or_init_with() requested a type that does not \ + match the value already stored in the slot; returning None instead \ + of a wrong-typed value. This indicates a programming error - the \ + slot should only ever hold one concrete type. (Reached through \ + get_or_init() if that is what the caller used.)" + ); + None + } + }; + } + + let created = init(); + *guard = Some(Arc::clone(&created) as Arc); + Some((created, true)) + } +} + +// `dyn Any` doesn't implement `Debug`, so this can't be derived - `SqliteDatabase` +// derives `Debug` and needs this field to cooperate. Only report whether the slot +// is occupied, since the contents are opaque to this crate anyway. +impl std::fmt::Debug for ObserverSlot { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ObserverSlot") + .field("is_set", &self.is_set()) + .finish() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn empty_slot_reports_unset_and_none() { + let slot = ObserverSlot::default(); + assert!(!slot.is_set()); + assert!(slot.get::().is_none()); + } + + #[test] + fn get_or_init_then_get_round_trips() { + let slot = ObserverSlot::default(); + slot.get_or_init(|| Arc::new(42_u32)); + assert!(slot.is_set()); + assert_eq!(*slot.get::().unwrap(), 42); + } + + #[test] + fn clear_empties_the_slot() { + let slot = ObserverSlot::default(); + slot.get_or_init(|| Arc::new(42_u32)); + slot.clear(); + assert!(!slot.is_set()); + assert!(slot.get::().is_none()); + } + + #[test] + fn get_with_wrong_type_returns_none_not_a_wrong_typed_value() { + let slot = ObserverSlot::default(); + slot.get_or_init(|| Arc::new(42_u32)); + assert!(slot.get::().is_none()); + // The original value is untouched by a mismatched read. + assert_eq!(*slot.get::().unwrap(), 42); + } + + #[test] + fn get_or_init_creates_on_first_call_and_reuses_on_second() { + let slot = ObserverSlot::default(); + + let (first, created) = slot.get_or_init(|| Arc::new(1_u32)).unwrap(); + assert!(created); + assert_eq!(*first, 1); + + let (second, created) = slot.get_or_init(|| Arc::new(999_u32)).unwrap(); + assert!(!created, "second call should reuse the existing value"); + assert_eq!(*second, 1, "second call must not replace the first value"); + assert!(Arc::ptr_eq(&first, &second)); + } + + #[test] + fn get_or_init_with_wrong_type_returns_none() { + let slot = ObserverSlot::default(); + slot.get_or_init(|| Arc::new(42_u32)); + assert!(slot.get_or_init(|| Arc::new(String::from("x"))).is_none()); + } + + #[test] + fn get_or_init_with_runs_on_existing_only_on_reuse() { + let slot = ObserverSlot::default(); + + let (first, created) = slot + .get_or_init_with( + || Arc::new(1_u32), + |_| panic!("on_existing must not run when init runs"), + ) + .unwrap(); + assert!(created); + assert_eq!(*first, 1); + + let mut seen: Option = None; + let (second, created) = slot + .get_or_init_with( + || panic!("init must not run when the slot is already populated"), + |existing| seen = Some(**existing), + ) + .unwrap(); + assert!(!created); + assert_eq!(*second, 1); + assert_eq!( + seen, + Some(1), + "on_existing should observe the existing value" + ); + } + + /// Mirrors `get_or_init_holds_the_lock_for_the_whole_init_closure` below, but + /// for `on_existing` - which running under the write lock is the whole reason + /// `get_or_init_with` exists. + #[test] + fn get_or_init_with_holds_the_lock_for_the_whole_on_existing_closure() { + use std::sync::{Barrier, mpsc}; + use std::thread; + use std::time::Duration; + + let slot = Arc::new(ObserverSlot::default()); + slot.get_or_init(|| Arc::new(1_u32)); + + let entered_barrier = Arc::new(Barrier::new(2)); + let release_barrier = Arc::new(Barrier::new(2)); + + let a_slot = Arc::clone(&slot); + let a_entered = Arc::clone(&entered_barrier); + let a_release = Arc::clone(&release_barrier); + let a_handle = thread::spawn(move || { + a_slot + .get_or_init_with::( + || panic!("slot is already seeded; init must not run"), + |_existing| { + // Signals the main thread that we're now inside + // `on_existing` - i.e. the write lock is held - then blocks + // until told to finish. + a_entered.wait(); + a_release.wait(); + }, + ) + .expect("get_or_init_with should return Some for the seeded slot") + }); + + // Blocks until thread A is confirmed to be inside its on_existing + // closure, holding the write lock. + entered_barrier.wait(); + + let (b_done_tx, b_done_rx) = mpsc::channel(); + let b_slot = Arc::clone(&slot); + let b_handle = thread::spawn(move || { + // clear() also takes the write lock, so it should block on A too. + b_slot.clear(); + let _ = b_done_tx.send(()); + }); + + // Absence of a message within this timeout is the proof that B is still + // blocked, exactly as in the init-closure test below. + let still_blocked = b_done_rx.recv_timeout(Duration::from_millis(50)); + assert!( + still_blocked.is_err(), + "thread B's clear() must still be blocked on the write lock while \ + thread A's on_existing closure is running" + ); + + // Lets thread A's on_existing closure finish, releasing the write lock. + release_barrier.wait(); + + a_handle.join().expect("thread A should not panic"); + b_done_rx + .recv_timeout(Duration::from_secs(1)) + .expect("thread B should complete once the write lock is released"); + b_handle.join().expect("thread B should not panic"); + + assert!( + !slot.is_set(), + "clear() must have run (and emptied the slot) only after \ + on_existing finished" + ); + } + + /// Deterministic version of the race + /// `concurrent_enable_observation_converges_on_one_broker` (in + /// `sqlx-sqlite-toolkit`'s `observation_tests.rs`) demonstrates + /// probabilistically under a multi-thread tokio runtime. Proves the write + /// lock is held for the *entire* `init` closure, not just the + /// check-and-store around it, by making a second thread's `get_or_init()` + /// call observably block until the first thread's closure returns. + #[test] + fn get_or_init_holds_the_lock_for_the_whole_init_closure() { + use std::sync::{Barrier, mpsc}; + use std::thread; + use std::time::Duration; + + let slot = Arc::new(ObserverSlot::default()); + let entered_barrier = Arc::new(Barrier::new(2)); + let release_barrier = Arc::new(Barrier::new(2)); + + let a_slot = Arc::clone(&slot); + let a_entered = Arc::clone(&entered_barrier); + let a_release = Arc::clone(&release_barrier); + let a_handle = thread::spawn(move || { + a_slot + .get_or_init(|| { + // Signals the main thread that we're now inside `init` - i.e. + // the write lock is held - then blocks until told to finish. + a_entered.wait(); + a_release.wait(); + Arc::new(1_u32) + }) + .expect("get_or_init should return Some for a freshly-typed slot") + }); + + // Blocks until thread A is confirmed to be inside its init closure, + // holding the write lock. + entered_barrier.wait(); + + let (b_done_tx, b_done_rx) = mpsc::channel(); + let b_slot = Arc::clone(&slot); + let b_handle = thread::spawn(move || { + let result = b_slot.get_or_init(|| Arc::new(999_u32)); + let _ = b_done_tx.send(result); + }); + + // Thread B's get_or_init() call must not be able to complete - or even + // decide whether to create or reuse - while A's init closure is still + // running under the write lock. A non-atomic implementation (drop the + // lock before calling init, or check-then-set without holding it + // throughout) would let B race ahead here instead of blocking, so the + // absence of a message within this timeout is the proof, not merely a + // wait: a several-orders-of-magnitude-longer stall than an uncontended + // get_or_init needs is only possible if B is genuinely blocked on the + // lock A is still holding. + let still_blocked = b_done_rx.recv_timeout(Duration::from_millis(50)); + assert!( + still_blocked.is_err(), + "thread B's get_or_init() must still be blocked on the write lock \ + while thread A's init closure is running" + ); + + // Lets thread A's init closure finish, releasing the write lock. + release_barrier.wait(); + + let (a_value, a_created) = a_handle.join().expect("thread A should not panic"); + assert!(a_created, "thread A should have created the value"); + + let (b_value, b_created) = b_done_rx + .recv_timeout(Duration::from_secs(1)) + .expect("thread B should complete once the write lock is released") + .expect("get_or_init should return Some for a freshly-typed slot"); + b_handle.join().expect("thread B should not panic"); + + assert!( + !b_created, + "thread B must reuse the value thread A created, not build its own" + ); + assert!( + Arc::ptr_eq(&a_value, &b_value), + "both threads must converge on the exact same Arc" + ); + } +} diff --git a/crates/sqlx-sqlite-observer/src/broker.rs b/crates/sqlx-sqlite-observer/src/broker.rs index 33c3556..fdcf696 100644 --- a/crates/sqlx-sqlite-observer/src/broker.rs +++ b/crates/sqlx-sqlite-observer/src/broker.rs @@ -274,6 +274,7 @@ impl ObservationBroker { }; Ok(TableChange { + schema: event.schema, table: event.table, operation: Some(event.operation), rowid, diff --git a/crates/sqlx-sqlite-observer/src/change.rs b/crates/sqlx-sqlite-observer/src/change.rs index 03020ec..53015ef 100644 --- a/crates/sqlx-sqlite-observer/src/change.rs +++ b/crates/sqlx-sqlite-observer/src/change.rs @@ -129,8 +129,29 @@ pub enum TableChangeEvent { /// Contains the table name, operation type, affected rowid, and the /// old/new column values (when available). Changes are only sent after /// the transaction commits successfully. +// `#[non_exhaustive]` so future fields are additive rather than a major bump for +// downstream struct literals and exhaustive destructuring - this struct already +// grew `schema` once. `broker.rs` is the only place that builds one, and +// construction stays legal inside this crate. #[derive(Debug, Clone)] +#[non_exhaustive] pub struct TableChange { + /// The schema this change occurred under: `"main"` for the primary database, + /// or the caller-chosen `ATTACH ... AS ` name otherwise. + /// + /// This is provenance metadata - "which schema name did the write use on + /// the connection that made it" - not a stable identifier. `schema_name` on + /// an attached-database spec is caller-supplied and validated only for + /// identifier shape, so the same physical database can legitimately be + /// attached under different aliases by different call sites. A subscriber + /// on the *owning* database (the one whose broker actually published this + /// change - see `ObservableSqliteDatabase::acquire_writer_with_attached`) + /// may see an alias chosen by some other, unrelated database's caller, and + /// that alias means nothing from the owning database's own perspective, + /// where the table is simply `main.`. It is only guaranteed + /// consistent for the lifetime of the write guard that produced this + /// change - do not treat it as a lookup key or cache it across calls. + pub schema: String, pub table: String, pub operation: Option, /// The SQLite internal rowid. This is `None` for WITHOUT ROWID tables diff --git a/crates/sqlx-sqlite-observer/src/conn_mgr.rs b/crates/sqlx-sqlite-observer/src/conn_mgr.rs index c140608..d9f4f78 100644 --- a/crates/sqlx-sqlite-observer/src/conn_mgr.rs +++ b/crates/sqlx-sqlite-observer/src/conn_mgr.rs @@ -41,13 +41,16 @@ //! } //! ``` +use std::collections::HashMap; use std::ops::{Deref, DerefMut}; use std::sync::Arc; use libsqlite3_sys::sqlite3; use sqlx::sqlite::SqliteConnection; use sqlx::{Pool, Sqlite}; -use sqlx_sqlite_conn_mgr::{SqliteDatabase, WriteGuard}; +use sqlx_sqlite_conn_mgr::{ + AttachedMode, AttachedSpec, AttachedWriteGuard, SqliteDatabase, WriteGuard, +}; use tokio::sync::broadcast; use tracing::{debug, trace, warn}; @@ -86,6 +89,22 @@ impl ObservableSqliteDatabase { Self { db, broker } } + /// Rebuilds an observable handle from a broker already stored in `db`'s + /// [`ObserverSlot`](sqlx_sqlite_conn_mgr::ObserverSlot). + /// + /// The slot holds `Arc`, not `Arc` - storing `Self` + /// there would put `db`'s own `Arc` field back into the slot + /// of the very database it came from, a strong reference cycle that keeps + /// the database alive forever (it's what the registry's `Weak` reference is + /// meant to prevent). Every internal read site that finds a broker already + /// in the slot uses this constructor to hand callers back the same + /// `ObservableSqliteDatabase` API without recreating that cycle. Unlike + /// [`new`](Self::new), this never applies an [`ObserverConfig`] - the + /// broker already carries whatever configuration it was created with. + pub fn from_broker(db: Arc, broker: Arc) -> Self { + Self { db, broker } + } + /// Subscribe to change notifications. /// /// Returns a broadcast receiver that will receive `TableChange` events @@ -144,7 +163,59 @@ impl ObservableSqliteDatabase { /// /// On first acquisition for each table, queries the schema to determine /// primary key columns and WITHOUT ROWID status. + /// + /// Nothing is attached on this path, so SQLite only ever reports the + /// `"main"` schema for writes made through it - see + /// [`acquire_writer_with_attached`](Self::acquire_writer_with_attached) for + /// the multi-schema case. + /// + /// Warming happens before the writer is acquired, so the single write permit + /// is never held while awaiting a read-pool connection. + /// + /// **Known limitation: the broker this guard's hooks bind to is fixed for + /// the guard's whole lifetime.** It is `self.broker`, snapshotted when + /// `Self` was built. If a `disable_observation()` + `enable_observation()` + /// cycle runs on the same database while this guard's transaction is still + /// open, the observer slot ends up holding a new broker while these hooks + /// stay bound to the old one. The commit still reaches subscribers that + /// existed before the cycle (the hook context's `Arc` keeps that broker + /// alive), but a subscriber created after it subscribes against the new + /// broker and never sees this commit. Nothing reports a failure: + /// `is_observing()`, the new `subscribe()`, and the commit all succeed. The + /// reachable trigger is an `unobserve()`/`observe()` pair running while + /// another caller's interruptible transaction is open. Fixing it means + /// keeping the previous broker reachable for as long as a writer is bound + /// to it; deferred to a follow-up issue (not yet filed). pub async fn acquire_writer(&self) -> Result { + // Warm before taking the write permit, never after. `ensure_table_info()` + // awaits a *read*-pool connection, so warming under the permit lets a full + // read pool and a pending writer wait on each other: tasks holding all six + // default read connections and then wanting the writer can't get it, and + // this task can't get a reader, until sqlx's acquire timeout breaks it. A + // query-then-write pattern reaches this, and on every acquisition, since an + // observed table missing from the schema is never cached. + // + // The trade: a table added to the observed set (via `subscribe`/ + // `subscribe_stream`) after this warm-up goes unwarmed for this + // transaction - empty `primary_key`, meaningless `rowid` if `WITHOUT + // ROWID`. That window is wider than the wait for the permit: the + // preupdate hook reads the observed set live at fire time, not from a + // snapshot taken here or in `register_hooks`, so a `subscribe()` landing + // mid-transaction still delivers a change with an empty `primary_key`. + // The window closes at end-of-transaction, not at permit acquisition. + // + // Do not "shrink" it by re-checking `ensure_table_info()` after the + // permit is acquired. That re-check is only cheap when its work list is + // empty, and the list never empties for an observed name that doesn't + // resolve in the schema: `query_table_info` returns `Ok(None)` and the + // warn-only branch never calls `set_table_info`, so the name stays + // queued forever. The re-check would then await a read-pool connection + // *while holding the write permit* on every acquisition for such a + // database, reintroducing this exact deadlock (verified: it deadlocks). + // A `try_acquire` variant avoids the deadlock but leaves the window open + // anyway, per the paragraph above, so it buys nothing. + self.ensure_table_info().await?; + let writer = self .db .acquire_writer() @@ -152,18 +223,80 @@ impl ObservableSqliteDatabase { .map_err(crate::error::Error::ConnMgr)?; let mut observable = ObservableWriteGuard { - writer: Some(writer), + writer: Some(InnerWriter::Regular(writer)), hooks_registered: false, raw_db: None, + brokers: HashMap::new(), }; - // Query table info for any observed tables that don't have it yet - self.ensure_table_info().await?; - - observable.register_hooks(Arc::clone(&self.broker)).await?; + let mut brokers = HashMap::with_capacity(1); + brokers.insert("main".to_string(), Arc::clone(&self.broker)); + observable.register_hooks(brokers).await?; Ok(observable) } + /// Acquire an observable write guard with one or more databases attached. + /// + /// Each change is published to the broker of the database that **owns** + /// the affected table: a write to `other.users` (where `other` is some + /// attached database's schema alias) notifies that database's own + /// subscribers, while a write to `main.users` notifies this database's - + /// provided this database's own observation is enabled at all; see below. + /// + /// Only attached databases in [`AttachedMode::ReadWrite`] that themselves + /// have observation enabled contribute a broker: + /// - **`ReadOnly` attachments are skipped because their write permit isn't + /// held here - not because they can't be written through.** `ReadOnly` + /// describes which locks are taken, not an enforced restriction: databases + /// are attached as a plain quoted path, so SQLite is never asked to reject + /// writes to them, and such a write lands *and* goes unobserved. Enforcing + /// it would mean a `file:...?mode=ro` URI - a behavior change in + /// `sqlx-sqlite-conn-mgr`'s `ATTACH` construction, left as follow-up work. + /// The skip matters for more than tidiness: every broker in the hook map + /// must belong to a database whose write permit *this guard* holds for + /// its whole lifetime - that invariant is what makes the commit/rollback + /// fan-out safe, since it guarantees no other, independent writer can be + /// committing or rolling back that same connection concurrently with + /// this guard's hooks. Adding a `ReadOnly` attachment's broker to the map + /// would violate that and risks corrupting an unrelated connection's + /// buffer. Do not "fix" the skip by removing it. + /// - A `ReadWrite` attachment with no observation enabled has nowhere for + /// its changes to go. It's left out of the broker map rather than routed + /// to `self`'s broker, which is what makes the preupdate callback drop + /// those changes instead of misattributing them to this database. + /// + /// **This database's own observation is independent of the attachments'.** + /// Reaching this method means `self.broker` exists, so `"main"` always gets + /// a map entry and this database's own changes are always buffered and + /// published to it - to no effect if nothing ever subscribed, but the + /// per-row buffering and `TableInfo` warming still happen. An attachment's + /// observation lives on *its* database's own slot, discovered below, and + /// neither side implies the other. For the case this type cannot express - + /// this database unobserved, an attached `ReadWrite` database observed on + /// its own - see [`acquire_writer_with_attached_brokers`], which takes the + /// main broker as an `Option` precisely because `Self` cannot be built + /// without one. + /// + /// Each participating database's `TableInfo` cache is warmed from its own + /// read pool, not just `self`'s. An attached table's `TableInfo` is only + /// ever populated by that database's own writer acquisitions, which may + /// never have happened before it is attached here; without this, its + /// changes would carry an empty `primary_key` and a meaningless `rowid` + /// for a `WITHOUT ROWID` table. + /// + /// Warming happens before the writer is acquired, so the single write + /// permit is not held while waiting on a read-pool connection - see + /// [`acquire_writer`](Self::acquire_writer)'s body for the deadlock that + /// ordering avoids. + pub async fn acquire_writer_with_attached( + &self, + specs: Vec, + ) -> Result { + // `self.broker` always exists, so `Some` always. Only the free function's + // other caller passes `None`; see its doc. + acquire_writer_with_attached_brokers(&self.db, Some(Arc::clone(&self.broker)), specs).await + } + /// Ensures TableInfo is set for all observed tables. /// /// Uses the read pool to query schema information, respecting conn-mgr's @@ -219,6 +352,162 @@ impl ObservableSqliteDatabase { } } +/// Acquire an observable write guard with one or more databases attached, without +/// requiring `main_db`'s own observation to be enabled. +/// +/// A free function rather than a method because `ObservableSqliteDatabase` +/// cannot be constructed without a broker, which is exactly the case this +/// exists for: `main_db` unobserved, with only an attached `ReadWrite` +/// database observed on its own. The method form seeded `"main"` +/// unconditionally, so that combination could not be represented at all and +/// an attached database's subscribers silently received nothing. +/// +/// `main_broker` is added to the broker map - and `main_db` to the set whose +/// `TableInfo` is warmed - only when `Some`. With `None`, `main_db`'s writes +/// still happen but are neither buffered nor published, exactly as on a plain +/// [`acquire_writer`](ObservableSqliteDatabase::acquire_writer) against an +/// unobserved database. `ObservableSqliteDatabase::acquire_writer_with_attached` +/// always passes `Some`; `sqlx_sqlite_toolkit::DatabaseWrapper::acquire_writer_with_attached` +/// is what passes `None`, reading the slot directly rather than through a +/// handle it may not be able to build. +/// +/// The `ReadOnly`-skip and unobserved-`ReadWrite`-drop rules are unchanged from +/// [`acquire_writer_with_attached`]; only `main_db`'s own treatment differs. +/// +/// # Preconditions +/// +/// When `main_broker` is `Some`, it must be `main_db`'s own broker - what +/// `main_db.observer_slot().get::()` returned when the caller +/// read it - not another database's and not one left over from an earlier +/// observation cycle. Passing another database's broker would publish +/// `main_db`'s changes to that database's subscribers, the misattribution the +/// `ReadOnly`-skip and `ReadWrite`-drop rules exist to prevent, and would break +/// the write-permit invariant, since nothing here acquires a writer on whatever +/// database that broker belongs to. +/// +/// Deliberately unchecked: the slot is legitimately mutable between the +/// caller's read and this call, so "does the slot hold this exact `Arc` now" +/// has no stable answer to assert on. The precondition is on what the caller +/// read, not on what the slot holds now. +/// +/// [`acquire_writer_with_attached`]: ObservableSqliteDatabase::acquire_writer_with_attached +pub async fn acquire_writer_with_attached_brokers( + main_db: &Arc, + main_broker: Option>, + specs: Vec, +) -> Result { + // Validate before the broker map is built, so "validation precedes map + // construction" is a property of this control flow rather than an accident of + // the ATTACH further down happening to reject the same input later. + // `validate_attached_specs` rejects `main`/`temp` (case-insensitively) and + // any two specs sharing an alias. conn-mgr re-validates internally - it has + // to, being callable directly - and that second pass is an idempotent no-op + // here, not a redundant check to delete from either side. + sqlx_sqlite_conn_mgr::validate_attached_specs(&specs).map_err(crate::error::Error::ConnMgr)?; + + // Build the broker map, and collect every observable whose `TableInfo` + // cache needs warming, before `specs` is consumed by the conn-mgr call + // below. `AttachedSpec` only needs to be read here, not cloned - the + // whole `Vec` is handed off afterward. + let mut brokers: HashMap> = HashMap::new(); + let mut participating: Vec = Vec::new(); + + if let Some(broker) = main_broker { + brokers.insert("main".to_string(), Arc::clone(&broker)); + participating.push(ObservableSqliteDatabase::from_broker( + Arc::clone(main_db), + broker, + )); + } + + for spec in &specs { + // See `acquire_writer_with_attached`'s doc for why this skip is + // load-bearing rather than a mere filter: nothing asks SQLite to + // enforce ReadOnly on an attached schema, so this is about which write + // permits this guard actually holds, not about which writes are + // possible. + if spec.mode != AttachedMode::ReadWrite { + continue; + } + + if let Some(broker) = spec.database.observer_slot().get::() { + // A handle is needed for both halves here - the broker for the map, + // and something to call `ensure_table_info()` on, which reads the + // attached database's own read pool - so it's rebuilt from the + // broker rather than read out whole. See `from_broker`'s doc. + let observable = + ObservableSqliteDatabase::from_broker(Arc::clone(&spec.database), broker); + + // Fail loud rather than let `insert` silently overwrite. The + // validation above already rules this out, so it should never fire; + // it's an independent guard so that a regression there, or in the + // ATTACH ordering, breaks here instead of resurfacing as one broker's + // changes misattributed to another's subscribers. + if brokers + .insert(spec.schema_name.clone(), Arc::clone(&observable.broker)) + .is_some() + { + return Err(crate::error::Error::BrokerAliasCollision( + spec.schema_name.clone(), + )); + } + participating.push(observable); + } + } + + // Populates each participating database's own TableInfo cache - see + // `acquire_writer_with_attached`'s doc for why an empty primary_key, not a + // wrong-column decode, is what's actually at stake here. + for observable in &participating { + observable.ensure_table_info().await?; + } + + let writer = sqlx_sqlite_conn_mgr::acquire_writer_with_attached(main_db, specs) + .await + .map_err(crate::error::Error::ConnMgr)?; + + let mut observable = ObservableWriteGuard { + writer: Some(InnerWriter::Attached(writer)), + hooks_registered: false, + raw_db: None, + brokers: HashMap::new(), + }; + + // `brokers` can be empty here even though a caller's own gate found at least + // one side observed: `DatabaseWrapper::acquire_writer_with_attached` reads + // `main_db`'s slot (and each `ReadWrite` spec's) to decide whether to take + // this path at all, but that read and this one are not atomic with each + // other, so observation disabled on every side in between leaves nothing in + // the map. Skipping registration in that case is behaviorally identical to + // registering hooks against an empty map - there is nothing to publish to + // either way - but avoids paying for `lock_handle()` and FFI hook + // registration for nothing, and avoids newly requiring + // `SQLITE_ENABLE_PREUPDATE_HOOK` on a call that, had the race not happened, + // would have taken the plain, unobserved path instead. `hooks_registered` + // stays `false` (its constructed default above), so `Drop` correctly does + // no cleanup. + if !brokers.is_empty() + && let Err(err) = observable.register_hooks(brokers).await + { + // register_hooks failed before touching any of observable's state + // (see its own body), so the writer is untouched and still has the + // ATTACH(es) live on it. Detach before propagating: AttachedWriteGuard's + // own Drop deliberately doesn't detach (see its doc), and the write + // pool is max_connections(1), so leaving the alias attached here + // would strand it on the pooled connection - every later acquisition + // that reuses the same alias would then fail at ATTACH with + // "database is already in use", permanently. + if let Err(detach_err) = observable.detach_all().await { + warn!( + "failed to detach after register_hooks failed ({err}); the \ + write connection may be stuck with a stale ATTACH: {detach_err}" + ); + } + return Err(err); + } + Ok(observable) +} + impl Clone for ObservableSqliteDatabase { fn clone(&self) -> Self { Self { @@ -228,19 +517,73 @@ impl Clone for ObservableSqliteDatabase { } } +/// Either kind of writer an `ObservableWriteGuard` may wrap. +/// +/// Both `WriteGuard` and `AttachedWriteGuard` already `Deref`/`DerefMut` to +/// `SqliteConnection`, so giving this enum the same impls (matching each +/// variant to its inner guard) lets `ObservableWriteGuard` stay agnostic to +/// which one it holds everywhere except construction. +enum InnerWriter { + Regular(WriteGuard), + Attached(AttachedWriteGuard), +} + +impl Deref for InnerWriter { + type Target = SqliteConnection; + + fn deref(&self) -> &Self::Target { + match self { + InnerWriter::Regular(w) => w, + InnerWriter::Attached(w) => w, + } + } +} + +impl DerefMut for InnerWriter { + fn deref_mut(&mut self) -> &mut Self::Target { + match self { + InnerWriter::Regular(w) => w, + InnerWriter::Attached(w) => w, + } + } +} + +/// The plain (non-observing) guard handed back by +/// [`ObservableWriteGuard::into_inner`]. +/// +/// Which variant comes back mirrors how the guard was acquired - +/// `Regular` from [`ObservableSqliteDatabase::acquire_writer`], `Attached` +/// from [`ObservableSqliteDatabase::acquire_writer_with_attached`]. +/// +/// `#[must_use]` like the guard it came out of, so `guard.into_inner();` as a +/// bare statement still warns: every hazard the inner guards warn about survives +/// the unwrapping, including a stranded `ATTACH` for the `Attached` variant. +#[must_use = "if unused, the write guard and locks are immediately dropped"] +pub enum UnobservedWriter { + Regular(WriteGuard), + Attached(AttachedWriteGuard), +} + /// RAII guard for observable write access to the database. /// -/// This guard wraps a `WriteGuard` from `sqlx-sqlite-conn-mgr` and adds -/// change tracking via SQLite hooks. Changes are published to subscribers -/// when transactions commit. +/// Wraps either a `WriteGuard` or an `AttachedWriteGuard` from +/// `sqlx-sqlite-conn-mgr` and adds change tracking via SQLite hooks. Changes +/// are published to subscribers when transactions commit. #[must_use = "if unused, the write lock is immediately released"] pub struct ObservableWriteGuard { - writer: Option, + writer: Option, hooks_registered: bool, /// Raw sqlite3 pointer, cached during register_hooks so we can /// call unregister_hooks synchronously in Drop without needing /// the async lock_handle. raw_db: Option<*mut sqlite3>, + /// Brokers hooks were registered with, keyed by schema alias. Retained + /// (rather than discarded once `hooks::register_hooks` has its own copy) + /// so `Drop` can discard each one's buffered-but-uncommitted events if + /// this guard is dropped without an explicit commit or rollback - see + /// `Drop`'s impl for why that's safe to do unconditionally. Empty until + /// `register_hooks` populates it. + brokers: HashMap>, } // SAFETY: The raw_db pointer is only used for hook registration/unregistration @@ -249,12 +592,11 @@ pub struct ObservableWriteGuard { unsafe impl Send for ObservableWriteGuard {} impl ObservableWriteGuard { - fn writer_mut(&mut self) -> &mut WriteGuard { - self.writer.as_mut().expect("writer already taken") - } - /// Registers SQLite observation hooks on this writer. - async fn register_hooks(&mut self, broker: Arc) -> Result<()> { + async fn register_hooks( + &mut self, + brokers: HashMap>, + ) -> Result<()> { if self.hooks_registered { return Ok(()); } @@ -272,22 +614,46 @@ impl ObservableWriteGuard { let db: *mut sqlite3 = handle.as_raw_handle().as_ptr(); unsafe { - hooks::register_hooks(db, broker)?; + hooks::register_hooks(db, brokers.clone())?; } // Cache the raw pointer so Drop can call unregister_hooks synchronously. - // SAFETY: The pointer remains valid for the lifetime of the WriteGuard, + // SAFETY: The pointer remains valid for the lifetime of the writer, // which we own via self.writer. self.raw_db = Some(db); self.hooks_registered = true; + self.brokers = brokers; Ok(()) } + /// Discards every broker's buffered-but-uncommitted events. + /// + /// Safe to call unconditionally, regardless of whether a commit or + /// rollback already ran: `on_commit` drains the buffer via `mem::take` + /// before publishing, and an explicit `ROLLBACK`'s own rollback_hook + /// already clears it too, so calling this afterward always finds nothing + /// left to discard. The only case where it does something is the one it + /// exists for: hooks torn down - by [`Drop`](Self), [`into_inner`], or + /// [`detach_all`] - with no commit or rollback ever having run, which + /// would otherwise let this transaction's buffered events resurface as + /// phantom changes on the *next* transaction's commit. + /// + /// [`into_inner`]: Self::into_inner + /// [`detach_all`]: Self::detach_all + fn flush_all_brokers(&self) { + for broker in self.brokers.values() { + broker.on_rollback(); + } + } + /// Consumes this wrapper and returns the underlying write guard. /// /// Hooks are unregistered before returning the guard, so it can be - /// safely used without observation. - pub fn into_inner(mut self) -> WriteGuard { + /// safely used without observation. Also flushes every broker's buffer + /// (see [`flush_all_brokers`](Self::flush_all_brokers)) - safe to call + /// whether or not a commit/rollback already ran, and necessary if this is + /// called mid-transaction, with no commit or rollback yet sent. + pub fn into_inner(mut self) -> UnobservedWriter { // Unregister hooks before returning the writer to prevent // use-after-free if the broker is dropped before the connection is reused. if self.hooks_registered @@ -296,11 +662,41 @@ impl ObservableWriteGuard { unsafe { crate::hooks::unregister_hooks(db); } - trace!("Hooks unregistered before returning inner WriteGuard"); + trace!("Hooks unregistered before returning inner writer"); + self.flush_all_brokers(); + } + self.hooks_registered = false; + self.raw_db = None; + match self.writer.take().expect("writer already taken") { + InnerWriter::Regular(w) => UnobservedWriter::Regular(w), + InnerWriter::Attached(w) => UnobservedWriter::Attached(w), + } + } + + /// Unregisters hooks and detaches any databases attached to this writer. + /// + /// If this guard wraps a plain (non-attached) writer, there is nothing to + /// detach - this reduces to hook unregistration, safe to call regardless + /// of which kind of writer this guard wraps. Also flushes every broker's + /// buffer (see [`flush_all_brokers`](Self::flush_all_brokers)) - safe to + /// call whether or not a commit/rollback already ran. + pub async fn detach_all(mut self) -> Result<()> { + if self.hooks_registered + && let Some(db) = self.raw_db + { + unsafe { + crate::hooks::unregister_hooks(db); + } + trace!("Hooks unregistered before detach_all"); + self.flush_all_brokers(); } self.hooks_registered = false; self.raw_db = None; - self.writer.take().expect("writer already taken") + + match self.writer.take().expect("writer already taken") { + InnerWriter::Regular(_) => Ok(()), + InnerWriter::Attached(w) => w.detach_all().await.map_err(crate::error::Error::ConnMgr), + } } } @@ -310,12 +706,14 @@ impl Drop for ObservableWriteGuard { && let Some(db) = self.raw_db { // SAFETY: db was obtained from lock_handle during register_hooks and - // remains valid because we still own the WriteGuard (self.writer). - // The writer has not been taken (into_inner clears hooks_registered). + // remains valid because we still own the writer (self.writer). The + // writer has not been taken (into_inner/detach_all clear + // hooks_registered before taking it). unsafe { hooks::unregister_hooks(db); } trace!("ObservableWriteGuard dropped, hooks unregistered"); + self.flush_all_brokers(); } } } @@ -330,6 +728,6 @@ impl Deref for ObservableWriteGuard { impl DerefMut for ObservableWriteGuard { fn deref_mut(&mut self) -> &mut Self::Target { - self.writer_mut() + self.writer.as_mut().expect("writer already taken") } } diff --git a/crates/sqlx-sqlite-observer/src/connection.rs b/crates/sqlx-sqlite-observer/src/connection.rs index 2cba242..5fbe085 100644 --- a/crates/sqlx-sqlite-observer/src/connection.rs +++ b/crates/sqlx-sqlite-observer/src/connection.rs @@ -84,8 +84,14 @@ impl ObservableConnection { let db: *mut sqlite3 = handle.as_raw_handle().as_ptr(); + // This crate's plain (non-conn-mgr) connection path never attaches + // other databases, so there is only ever one schema to route: SQLite + // always reports "main" for the primary database. + let mut brokers = std::collections::HashMap::with_capacity(1); + brokers.insert("main".to_string(), Arc::clone(&self.broker)); + unsafe { - hooks::register_hooks(db, Arc::clone(&self.broker))?; + hooks::register_hooks(db, brokers)?; } // Cache the raw pointer so Drop can call unregister_hooks synchronously. diff --git a/crates/sqlx-sqlite-observer/src/error.rs b/crates/sqlx-sqlite-observer/src/error.rs index e06cb5e..010209a 100644 --- a/crates/sqlx-sqlite-observer/src/error.rs +++ b/crates/sqlx-sqlite-observer/src/error.rs @@ -33,4 +33,22 @@ pub enum Error { expected: usize, actual: usize, }, + + /// An attached-database spec's schema alias collided with an entry already in + /// the broker map - either `main`'s own alias, or another spec's alias, that + /// was about to be silently overwritten by `HashMap::insert`. + /// + /// This should never actually surface: `validate_attached_specs` runs first, + /// case-insensitively, and already rejects a `main`-aliased spec and any two + /// specs sharing an alias before the broker map is even created. This variant + /// exists as a second, independent guard - so that if the ATTACH ordering, or + /// the validation call itself, ever regresses, the failure is this clear error + /// instead of one broker's changes being silently misattributed to another's + /// subscribers, or an opaque "database main is already in use" surfacing three + /// steps later, one layer away, out of `ATTACH` itself. + #[cfg(feature = "conn-mgr")] + #[error( + "attached schema alias '{0}' collides with an existing entry in the observation broker map" + )] + BrokerAliasCollision(String), } diff --git a/crates/sqlx-sqlite-observer/src/hooks.rs b/crates/sqlx-sqlite-observer/src/hooks.rs index 55b2d9b..bb41cfe 100644 --- a/crates/sqlx-sqlite-observer/src/hooks.rs +++ b/crates/sqlx-sqlite-observer/src/hooks.rs @@ -9,8 +9,10 @@ //! Use [`is_preupdate_hook_enabled()`] to check at runtime whether the linked //! SQLite library supports this feature. +use std::collections::HashMap; use std::ffi::{CStr, CString, c_char, c_int, c_void}; -use std::panic::catch_unwind; +use std::io::Write; +use std::panic::{AssertUnwindSafe, catch_unwind}; use std::ptr; use std::sync::Arc; @@ -82,8 +84,17 @@ impl SqliteValue { } /// Raw change event captured by the preupdate hook before commit decision. +/// +/// `#[non_exhaustive]` for the same reason as +/// [`TableChange`](crate::change::TableChange); only `preupdate_callback` builds +/// one. #[derive(Debug, Clone)] +#[non_exhaustive] pub struct PreUpdateEvent { + /// The schema this change occurred under (`"main"` or an `ATTACH` alias). + /// See [`TableChange::schema`](crate::change::TableChange::schema) for what + /// this is - and is not - safe to rely on. + pub schema: String, pub table: String, pub operation: ChangeOperation, pub old_rowid: i64, @@ -94,10 +105,14 @@ pub struct PreUpdateEvent { /// Context data passed to SQLite hook callbacks. /// -/// Stored as user_data pointer in SQLite hooks. The Arc ensures the broker -/// stays alive as long as hooks are registered. +/// Stored as user_data pointer in SQLite hooks. Keyed by schema alias +/// (`"main"` for the primary database, the `ATTACH` alias otherwise) rather +/// than holding a single broker, so that one connection with attached +/// databases can route each change to the broker of whichever database +/// actually owns the affected table. The `Arc`s ensure each broker stays +/// alive as long as hooks are registered. struct HookContext { - broker: Arc, + brokers: HashMap>, } /// Checks if the linked SQLite library was compiled with `SQLITE_ENABLE_PREUPDATE_HOOK`. @@ -131,13 +146,22 @@ pub fn is_preupdate_hook_enabled() -> bool { /// - Must be called from the same thread that owns the connection, or /// the connection must be in serialized threading mode /// +/// # Arguments +/// +/// * `db` - the connection to register hooks on +/// * `brokers` - schema alias -> broker map. `"main"` covers the primary +/// database; any other key routes changes made under that `ATTACH` alias to +/// the corresponding broker. A schema with no entry here has its changes +/// silently dropped by the preupdate callback rather than misrouted to some +/// other schema's broker - see [`preupdate_callback`]. +/// /// # Errors /// /// Returns an error if preupdate hooks are not supported by the linked SQLite /// library, or if the hooks cannot be registered. pub unsafe fn register_hooks( db: *mut sqlite3, - broker: Arc, + brokers: HashMap>, ) -> crate::Result<()> { // Check at runtime if preupdate hook is supported if !is_preupdate_hook_enabled() { @@ -153,7 +177,7 @@ pub unsafe fn register_hooks( // Heap-allocate the context so it outlives this function. SQLite's C API // requires a raw pointer to pass user data to callbacks. - let context = Box::new(HookContext { broker }); + let context = Box::new(HookContext { brokers }); // Transfer ownership out of Rust's memory management. // // NOTE: This pointer is shared across all three hooks and is intentionally @@ -206,7 +230,9 @@ pub unsafe fn unregister_hooks(db: *mut sqlite3) { /// Preupdate hook callback - captures changes before they're committed. /// /// Called by SQLite for INSERT, UPDATE, and DELETE operations. Captures old/new -/// row values and buffers them in the broker until commit or rollback. +/// row values and buffers them in the broker of whichever database owns the +/// affected table (selected by schema - see [`register_hooks`]) until commit +/// or rollback. /// /// Note: `user_data` is SQLite's C API term for callback context (our HookContext), /// unrelated to our app's user data. @@ -214,29 +240,59 @@ unsafe extern "C" fn preupdate_callback( user_data: *mut c_void, db: *mut sqlite3, op: c_int, - _database: *const c_char, + database: *const c_char, table: *const c_char, old_rowid: i64, new_rowid: i64, ) { - if user_data.is_null() || table.is_null() { + if user_data.is_null() || database.is_null() || table.is_null() { return; } - // Catch any panics to prevent unwinding across the FFI boundary (which is UB). + // Catch any panics to prevent unwinding across the FFI boundary, which aborts + // the process (a guarantee since Rust 1.81, UB before it; MSRV here is 1.94). let result = catch_unwind(|| { // SAFETY: user_data is a valid HookContext pointer created in register_hooks // and remains valid until unregister_hooks is called. let context = unsafe { &*(user_data as *const HookContext) }; + // SAFETY: database is a non-null C string provided by SQLite, valid for + // this callback. SQLite reports "main" for the primary schema and the + // caller-chosen ATTACH alias otherwise. Kept as `&str` through both + // gates below (broker lookup, then observed-table check) rather than + // allocated right away - `HashMap::get`/`HashSet::contains` both take + // `&str`, so an unobserved schema or table costs no allocation on this + // per-row FFI hot path. Only a change that clears both gates has its + // strings turned into owned `String`s, for the `PreUpdateEvent` below. + let schema_name = match unsafe { CStr::from_ptr(database) }.to_str() { + Ok(s) => s, + Err(_) => return, + }; + // SAFETY: table is a non-null C string provided by SQLite, valid for this callback. let table_name = match unsafe { CStr::from_ptr(table) }.to_str() { - Ok(s) => s.to_string(), + Ok(s) => s, Err(_) => return, }; + // No broker is registered for this schema - a `temp` table, an attached + // database with no observation enabled, or a `ReadOnly` attachment whose + // broker was deliberately left out of the map (see + // `ObservableSqliteDatabase::acquire_writer_with_attached`'s doc). Either + // way, drop the change rather than publish it under some other schema's + // broker. `trace!` rather than `warn!` because the `temp` case is routine + // and a warning would cry wolf. + let Some(broker) = context.brokers.get(schema_name) else { + trace!( + schema = %schema_name, + table = %table_name, + "Dropping change for a schema with no broker in the hook map" + ); + return; + }; + // Check if this table is being observed - if !context.broker.is_table_observed(&table_name) { + if !broker.is_table_observed(table_name) { return; } @@ -247,7 +303,7 @@ unsafe extern "C" fn preupdate_callback( _ => return, }; - trace!(table = %table_name, ?operation, old_rowid, new_rowid, "Preupdate hook fired"); + trace!(schema = %schema_name, table = %table_name, ?operation, old_rowid, new_rowid, "Preupdate hook fired"); // SAFETY: db is a valid sqlite3 pointer provided by SQLite for this callback. let column_count = unsafe { sqlite3_preupdate_count(db) }; @@ -294,7 +350,8 @@ unsafe extern "C" fn preupdate_callback( }; let event = PreUpdateEvent { - table: table_name, + schema: schema_name.to_string(), + table: table_name.to_string(), operation, old_rowid, new_rowid, @@ -302,13 +359,18 @@ unsafe extern "C" fn preupdate_callback( new_values, }; - context.broker.on_preupdate(event); + broker.on_preupdate(event); }); if result.is_err() { - // Cannot use tracing here since it may have been the source of the panic. - // The best we can do is silently absorb it to prevent UB. - eprintln!("sqlx-sqlite-observer: panic in preupdate_callback (absorbed to prevent UB)"); + // Cannot use tracing here since it may have been the source of the panic, + // nor eprintln!, which panics on a write failure - and that unwind would + // abort the process. Absorbing it is the best available outcome. The other + // two callbacks below report their own panics the same way. + let _ = writeln!( + std::io::stderr(), + "sqlx-sqlite-observer: panic in preupdate_callback (absorbed to keep the process alive)" + ); } } @@ -317,6 +379,12 @@ unsafe extern "C" fn preupdate_callback( /// Called by SQLite when a transaction is about to commit. Returning 0 allows /// the commit to proceed; returning non-zero would cause a rollback. /// +/// `sqlite3_commit_hook`'s callback takes no schema argument and fires exactly +/// once per transaction regardless of how many schemas (main plus any attached +/// databases) were touched, so every broker in the map must be flushed here - +/// a single-broker flush would strand an attached database's buffered changes +/// with no commit of its own to release them on. +/// /// Note: `user_data` is SQLite's C API term for callback context (our HookContext), /// unrelated to application-level user data. unsafe extern "C" fn commit_callback(user_data: *mut c_void) -> c_int { @@ -324,16 +392,28 @@ unsafe extern "C" fn commit_callback(user_data: *mut c_void) -> c_int { return 0; } - // Catch any panics to prevent unwinding across the FFI boundary (which is UB). - let result = catch_unwind(|| { - // SAFETY: user_data is a valid HookContext pointer created in register_hooks. - let context = unsafe { &*(user_data as *const HookContext) }; - trace!("Commit hook fired - flushing changes"); - context.broker.on_commit(); - }); - - if result.is_err() { - eprintln!("sqlx-sqlite-observer: panic in commit_callback (absorbed to prevent UB)"); + // SAFETY: user_data is a valid HookContext pointer created in register_hooks. + let context = unsafe { &*(user_data as *const HookContext) }; + // Wrapped because it's the one other thing in this fn that can panic, and an + // unwind out of an extern "C" fn aborts the process. Nothing to report if it + // does fail - the panic would be coming from the logger itself. + let _ = catch_unwind(|| trace!("Commit hook fired - flushing changes")); + + // catch_unwind wraps each broker's flush individually rather than the + // whole loop: a panic partway through must not skip flushing the brokers + // that come after it. With a single broker this distinction was moot: the + // fan-out is what makes it matter, since brokers left unflushed here have + // their buffered events resurface as phantom changes on the connection's + // *next* commit rather than this one. + for broker in context.brokers.values() { + if catch_unwind(AssertUnwindSafe(|| broker.on_commit())).is_err() { + // Reported via writeln! to stderr rather than tracing - see + // preupdate_callback's equivalent for why. + let _ = writeln!( + std::io::stderr(), + "sqlx-sqlite-observer: panic in commit_callback (absorbed to keep the process alive)" + ); + } } 0 // Allow commit to proceed @@ -341,7 +421,9 @@ unsafe extern "C" fn commit_callback(user_data: *mut c_void) -> c_int { /// Rollback hook callback - discards buffered changes. /// -/// Called by SQLite when a transaction is rolled back. +/// Called by SQLite when a transaction is rolled back. Fans out to every +/// broker in the map for the same reason [`commit_callback`] does - one +/// rollback hook fires for the whole transaction, not per schema. /// /// Note: `user_data` is SQLite's C API term for callback context (our HookContext), /// unrelated to application-level user data. @@ -350,16 +432,24 @@ unsafe extern "C" fn rollback_callback(user_data: *mut c_void) { return; } - // Catch any panics to prevent unwinding across the FFI boundary (which is UB). - let result = catch_unwind(|| { - // SAFETY: user_data is a valid HookContext pointer created in register_hooks. - let context = unsafe { &*(user_data as *const HookContext) }; - trace!("Rollback hook fired - discarding changes"); - context.broker.on_rollback(); - }); - - if result.is_err() { - eprintln!("sqlx-sqlite-observer: panic in rollback_callback (absorbed to prevent UB)"); + // SAFETY: user_data is a valid HookContext pointer created in register_hooks. + let context = unsafe { &*(user_data as *const HookContext) }; + // Wrapped for the same reason as commit_callback's - see its comment. + let _ = catch_unwind(|| trace!("Rollback hook fired - discarding changes")); + + // catch_unwind wraps each broker's discard individually - see + // commit_callback's comment for why the fan-out makes this matter: a + // panic partway through must not leave the remaining brokers' buffers + // undiscarded. + for broker in context.brokers.values() { + if catch_unwind(AssertUnwindSafe(|| broker.on_rollback())).is_err() { + // Reported via writeln! to stderr rather than tracing - see + // preupdate_callback's equivalent for why. + let _ = writeln!( + std::io::stderr(), + "sqlx-sqlite-observer: panic in rollback_callback (absorbed to keep the process alive)" + ); + } } } diff --git a/crates/sqlx-sqlite-observer/src/lib.rs b/crates/sqlx-sqlite-observer/src/lib.rs index c21bf30..2fdbd2f 100644 --- a/crates/sqlx-sqlite-observer/src/lib.rs +++ b/crates/sqlx-sqlite-observer/src/lib.rs @@ -144,6 +144,9 @@ pub use observer::SqliteObserver; pub use stream::{TableChangeStream, TableChangeStreamExt}; #[cfg(feature = "conn-mgr")] -pub use conn_mgr::{ObservableSqliteDatabase, ObservableWriteGuard}; +pub use conn_mgr::{ + ObservableSqliteDatabase, ObservableWriteGuard, UnobservedWriter, + acquire_writer_with_attached_brokers, +}; pub type Result = std::result::Result; diff --git a/crates/sqlx-sqlite-observer/tests/conn_mgr_tests.rs b/crates/sqlx-sqlite-observer/tests/conn_mgr_tests.rs index 0951376..3d7ea7e 100644 --- a/crates/sqlx-sqlite-observer/tests/conn_mgr_tests.rs +++ b/crates/sqlx-sqlite-observer/tests/conn_mgr_tests.rs @@ -1,16 +1,21 @@ //! Integration tests for conn-mgr feature (sqlx-sqlite-conn-mgr integration). //! //! Tests verify the same behaviors as integration_tests.rs but using -//! `ObservableSqliteDatabase` instead of `SqliteObserver`. +//! `ObservableSqliteDatabase` instead of `SqliteObserver`. Also covers issue +//! #53's attached-database routing: writes into an attached database publish +//! to the broker of whichever database *owns* the affected table, not +//! necessarily the database the write was issued through. //! //! Run with: cargo test --features conn-mgr #![cfg(feature = "conn-mgr")] +use std::sync::Arc; +use std::time::Duration; + use futures::StreamExt; -use sqlx_sqlite_conn_mgr::SqliteDatabase; +use sqlx_sqlite_conn_mgr::{AttachedMode, AttachedSpec, SqliteDatabase, SqliteDatabaseConfig}; use sqlx_sqlite_observer::{ChangeOperation, ObservableSqliteDatabase, ObserverConfig}; -use std::time::Duration; use tokio::time::timeout; struct TestDb { @@ -341,3 +346,1128 @@ async fn test_stream_receives_notifications() { } } } + +// ============================================================================ +// Attached-database routing (issue #53) +// ============================================================================ + +/// Creates a file-backed `SqliteDatabase` with the given DDL already applied. +/// +/// A real file (not `:memory:`) is required here because `ATTACH DATABASE` +/// needs a path the attaching connection can open - attaching `:memory:` +/// creates a brand new, unrelated anonymous database, not a second handle +/// onto the caller's existing one. +async fn create_attachable_db( + temp_file: &tempfile::NamedTempFile, + create_table_sql: &str, +) -> Arc { + let db = SqliteDatabase::connect(temp_file.path().to_str().unwrap(), None) + .await + .unwrap(); + let mut writer = db.acquire_writer().await.unwrap(); + sqlx::query(sqlx::AssertSqlSafe(create_table_sql.to_string())) + .execute(&mut *writer) + .await + .unwrap(); + drop(writer); + db +} + +/// Registers `observable`'s broker in `db`'s observer slot, exactly as +/// `sqlx_sqlite_toolkit::DatabaseWrapper::enable_observation` does. +/// +/// `acquire_writer_with_attached` discovers an attached database's broker by +/// reading this slot (it has no other way to reach one it wasn't directly +/// handed), so a test exercising that discovery has to set it up the same way +/// production code does rather than passing the broker in some more direct +/// way that wouldn't exercise the real lookup path. +fn register_as_observed(db: &Arc, observable: &ObservableSqliteDatabase) { + let broker = Arc::clone(observable.broker()); + db.observer_slot() + .get_or_init(|| broker) + .expect("slot must not already hold a value of some other type"); +} + +#[tokio::test] +async fn attached_write_notifies_owning_database() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a, ObserverConfig::new().with_tables(["users"])); + let observable_b = + ObservableSqliteDatabase::new(db_b.clone(), ObserverConfig::new().with_tables(["users"])); + register_as_observed(&db_b, &observable_b); + + let mut rx_a = observable_a.subscribe(["users"]); + let mut rx_b = observable_b.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: db_b, + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }]; + + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO other.users (name) VALUES ('Bob')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("COMMIT").execute(&mut *writer).await.unwrap(); + writer.detach_all().await.unwrap(); + + let change = timeout(Duration::from_millis(200), rx_b.recv()) + .await + .expect("should not time out") + .expect("B's subscriber should receive the change"); + assert_eq!(change.table, "users"); + assert_eq!(change.schema, "other"); + + let a_result = timeout(Duration::from_millis(100), rx_a.recv()).await; + assert!( + a_result.is_err(), + "A's subscriber must not see a write that landed in B's table" + ); +} + +#[tokio::test] +async fn attached_write_to_unobserved_database_is_dropped() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + // db_b is deliberately never wrapped in an ObservableSqliteDatabase or + // registered in its observer slot - it has no broker of its own. + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a, ObserverConfig::new().with_tables(["users"])); + let mut rx_a = observable_a.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: db_b, + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }]; + + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO other.users (name) VALUES ('Ghost')") + .execute(&mut *writer) + .await + .unwrap(); + // Must not panic even though there's no broker for "other" to publish to. + sqlx::query("COMMIT").execute(&mut *writer).await.unwrap(); + writer.detach_all().await.unwrap(); + + let result = timeout(Duration::from_millis(100), rx_a.recv()).await; + assert!( + result.is_err(), + "a write into an unobserved attached database must not surface anywhere, \ + including on the attaching database's own broker" + ); +} + +/// Pins today's `ReadOnly` contract, which the test above does not cover (it +/// exercises `ReadWrite` + unobserved): the write *succeeds*, since nothing asks +/// SQLite to refuse it, and the deliberate skip in +/// `acquire_writer_with_attached` keeps `other`'s broker out of the hook map so +/// its own subscriber never hears about it. +/// +/// Asserting both halves means a future change that enforces `ReadOnly` has to +/// update this test deliberately rather than by accident. +#[tokio::test] +async fn readonly_attachment_write_lands_and_is_not_observed() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + // Observed, unlike the unobserved-attachment test: the point here is that + // the skip - not a missing broker - is what suppresses the notification. + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a, ObserverConfig::new().with_tables(["users"])); + let observable_b = + ObservableSqliteDatabase::new(db_b.clone(), ObserverConfig::new().with_tables(["users"])); + register_as_observed(&db_b, &observable_b); + + let mut rx_a = observable_a.subscribe(["users"]); + let mut rx_b = observable_b.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: Arc::clone(&db_b), + schema_name: "other".to_string(), + mode: AttachedMode::ReadOnly, + }]; + + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO other.users (name) VALUES ('Written anyway')") + .execute(&mut *writer) + .await + .expect("SQLite is not asked to enforce ReadOnly, so this write succeeds"); + sqlx::query("COMMIT").execute(&mut *writer).await.unwrap(); + writer.detach_all().await.unwrap(); + + // The row really is there - this is the part that makes the silence matter. + let mut reader = db_b.read_pool().unwrap().acquire().await.unwrap(); + let count: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM users") + .fetch_one(&mut *reader) + .await + .unwrap(); + assert_eq!(count, 1, "the ReadOnly attachment was written through"); + + let b_result = timeout(Duration::from_millis(100), rx_b.recv()).await; + assert!( + b_result.is_err(), + "a write through a ReadOnly attachment is not observed, even though the \ + attached database has observation enabled" + ); + let a_result = timeout(Duration::from_millis(100), rx_a.recv()).await; + assert!( + a_result.is_err(), + "and it must not be misrouted to the attaching database's broker either" + ); +} + +#[tokio::test] +async fn mixed_transaction_publishes_to_both_brokers() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a.clone(), ObserverConfig::new().with_tables(["users"])); + let observable_b = + ObservableSqliteDatabase::new(db_b.clone(), ObserverConfig::new().with_tables(["users"])); + register_as_observed(&db_b, &observable_b); + + let mut rx_a = observable_a.subscribe(["users"]); + let mut rx_b = observable_b.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: db_b, + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }]; + + // sqlite3_commit_hook's callback takes no schema argument and fires exactly + // once for the whole transaction, regardless of how many schemas it + // touched - this is the test that proves the commit hook fans out to every + // broker instead of only the one for whichever schema happened to be + // touched last. + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO main.users (name) VALUES ('MainRow')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("INSERT INTO other.users (name) VALUES ('OtherRow')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("COMMIT").execute(&mut *writer).await.unwrap(); + writer.detach_all().await.unwrap(); + + let change_a = timeout(Duration::from_millis(200), rx_a.recv()) + .await + .expect("should not time out") + .expect("A's subscriber should receive its own change"); + assert_eq!(change_a.schema, "main"); + assert!( + change_a + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "MainRow") + ); + + let change_b = timeout(Duration::from_millis(200), rx_b.recv()) + .await + .expect("should not time out") + .expect("B's subscriber should receive its own change"); + assert_eq!(change_b.schema, "other"); + assert!( + change_b + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "OtherRow") + ); + + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not also receive B's change" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not also receive A's change" + ); +} + +#[tokio::test] +async fn mixed_transaction_rollback_discards_both_brokers_buffers() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a.clone(), ObserverConfig::new().with_tables(["users"])); + let observable_b = + ObservableSqliteDatabase::new(db_b.clone(), ObserverConfig::new().with_tables(["users"])); + register_as_observed(&db_b, &observable_b); + + let mut rx_a = observable_a.subscribe(["users"]); + let mut rx_b = observable_b.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: db_b, + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }]; + + // Mirrors mixed_transaction_publishes_to_both_brokers, but rolls back + // instead of committing. rollback_callback fans out to every broker in + // the hook map exactly the way commit_callback does - discarding only + // "main"'s buffer would leave B's buffer holding this rolled-back INSERT, + // ready to resurface as a phantom notification on B's *next* real commit. + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO main.users (name) VALUES ('RolledBackMain')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("INSERT INTO other.users (name) VALUES ('RolledBackOther')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("ROLLBACK").execute(&mut *writer).await.unwrap(); + writer.detach_all().await.unwrap(); + + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not receive a notification for a rolled-back change" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not receive a notification for a rolled-back change" + ); + + // A subsequent, unrelated commit on each database - through its own plain + // writer, not another attached transaction, so this doesn't also depend on + // the ATTACH alias above having been cleanly released - must publish only + // its own change, not the rolled-back row resurfacing alongside it. + let mut writer_a = observable_a.acquire_writer().await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('RealMain')") + .execute(&mut *writer_a) + .await + .unwrap(); + drop(writer_a); + + let change_a = timeout(Duration::from_millis(200), rx_a.recv()) + .await + .expect("should not time out") + .expect("A's subscriber should receive its own change"); + assert!( + change_a + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "RealMain"), + "A must publish only the real change, not the rolled-back one" + ); + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not receive a second, phantom notification" + ); + + let mut writer_b = observable_b.acquire_writer().await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('RealOther')") + .execute(&mut *writer_b) + .await + .unwrap(); + drop(writer_b); + + let change_b = timeout(Duration::from_millis(200), rx_b.recv()) + .await + .expect("should not time out") + .expect("B's subscriber should receive its own change"); + assert!( + change_b + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "RealOther"), + "B must publish only the real change, not the rolled-back one" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not receive a second, phantom notification" + ); +} + +#[tokio::test] +async fn abandoned_attached_transaction_does_not_leak_into_next_commit() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a.clone(), ObserverConfig::new().with_tables(["users"])); + let observable_b = + ObservableSqliteDatabase::new(db_b.clone(), ObserverConfig::new().with_tables(["users"])); + register_as_observed(&db_b, &observable_b); + + let mut rx_a = observable_a.subscribe(["users"]); + let mut rx_b = observable_b.subscribe(["users"]); + + // Abandon a transaction mid-flight on both schemas: BEGIN plus writes to + // each, then drop the writer directly - no COMMIT, no ROLLBACK, no + // detach_all(). This is what ObservableWriteGuard::drop's broker fan-out + // must clean up for every broker in the hook map, not just "main". (The + // stale ATTACH this leaves on the pooled connection is a separate, + // pre-existing conn-mgr contract - AttachedWriteGuard::drop deliberately + // doesn't detach either, see its own doc - so the follow-up commits below + // use each database's own plain writer rather than reusing this alias.) + { + let specs = vec![AttachedSpec { + database: db_b.clone(), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }]; + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO main.users (name) VALUES ('AbandonedMain')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("INSERT INTO other.users (name) VALUES ('AbandonedOther')") + .execute(&mut *writer) + .await + .unwrap(); + // Dropped here with no COMMIT, ROLLBACK, or detach_all() ever sent. + } + + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not receive a notification for an abandoned, uncommitted change" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not receive a notification for an abandoned, uncommitted change" + ); + + // A subsequent, unrelated commit on each database must publish only its + // own change, not the abandoned row resurfacing alongside it. + let mut writer_a = observable_a.acquire_writer().await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('RealMain')") + .execute(&mut *writer_a) + .await + .unwrap(); + drop(writer_a); + + let change_a = timeout(Duration::from_millis(200), rx_a.recv()) + .await + .expect("should not time out") + .expect("A's subscriber should receive its own change"); + assert!( + change_a + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "RealMain"), + "A must publish only the real change, not the abandoned one" + ); + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not receive a second, phantom notification" + ); + + let mut writer_b = observable_b.acquire_writer().await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('RealOther')") + .execute(&mut *writer_b) + .await + .unwrap(); + drop(writer_b); + + let change_b = timeout(Duration::from_millis(200), rx_b.recv()) + .await + .expect("should not time out") + .expect("B's subscriber should receive its own change"); + assert!( + change_b + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "RealOther"), + "B must publish only the real change, not the abandoned one" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not receive a second, phantom notification" + ); +} + +#[tokio::test] +async fn detach_all_discards_buffered_events_for_every_broker() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a.clone(), ObserverConfig::new().with_tables(["users"])); + let observable_b = + ObservableSqliteDatabase::new(db_b.clone(), ObserverConfig::new().with_tables(["users"])); + register_as_observed(&db_b, &observable_b); + + let mut rx_a = observable_a.subscribe(["users"]); + let mut rx_b = observable_b.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: db_b.clone(), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }]; + + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO main.users (name) VALUES ('AbandonedMain')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("INSERT INTO other.users (name) VALUES ('AbandonedOther')") + .execute(&mut *writer) + .await + .unwrap(); + + // detach_all() calls flush_all_brokers() *before* it attempts the DETACH - + // and that flush is the only cleanup that happens here. The transaction is + // still open (no COMMIT/ROLLBACK was ever sent), so SQLite refuses to + // detach "other" out from under it and this deterministically returns + // Err(ConnMgr(Sqlx("database other is locked"))). That failure is exactly + // what this test is pinning around - the point is the flush that already + // ran, not whether the DETACH itself succeeded - so the result is + // deliberately discarded. Do NOT "fix" this into an `.unwrap()`; it is + // supposed to fail. + let _ = writer.detach_all().await; + + // This alone would also pass under the bug: nothing has committed yet, so + // there's nothing to leak *yet*. The real assertion is below, after a real + // commit. + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not receive a notification for a never-committed change" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not receive a notification for a never-committed change" + ); + + // Follow-up commits go through each database's own plain writer, not a + // re-attach: the failed DETACH above left the "other" alias stranded on + // A's pooled connection (same reasoning as + // abandoned_attached_transaction_does_not_leak_into_next_commit above). + let mut writer_a = observable_a.acquire_writer().await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('RealMain')") + .execute(&mut *writer_a) + .await + .unwrap(); + drop(writer_a); + + // The load-bearing check: A's first (and only) notification must carry the + // real value, not a phantom replay of the discarded buffer. + let change_a = timeout(Duration::from_millis(200), rx_a.recv()) + .await + .expect("should not time out") + .expect("A's subscriber should receive its own change"); + assert!( + change_a + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "RealMain"), + "A's first notification must be the real change, not the buffered one" + ); + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not receive a second, phantom notification" + ); + + let mut writer_b = observable_b.acquire_writer().await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('RealOther')") + .execute(&mut *writer_b) + .await + .unwrap(); + drop(writer_b); + + let change_b = timeout(Duration::from_millis(200), rx_b.recv()) + .await + .expect("should not time out") + .expect("B's subscriber should receive its own change"); + assert!( + change_b + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "RealOther"), + "B's first notification must be the real change, not the buffered one" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not receive a second, phantom notification" + ); +} + +#[tokio::test] +async fn into_inner_discards_buffered_events_for_every_broker() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a.clone(), ObserverConfig::new().with_tables(["users"])); + let observable_b = + ObservableSqliteDatabase::new(db_b.clone(), ObserverConfig::new().with_tables(["users"])); + register_as_observed(&db_b, &observable_b); + + let mut rx_a = observable_a.subscribe(["users"]); + let mut rx_b = observable_b.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: db_b.clone(), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }]; + + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO main.users (name) VALUES ('AbandonedMain')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("INSERT INTO other.users (name) VALUES ('AbandonedOther')") + .execute(&mut *writer) + .await + .unwrap(); + + // into_inner() unregisters the hooks and flushes both brokers' buffers + // before handing back the plain (attached) writer - dropping that writer + // here doesn't detach (AttachedWriteGuard::drop deliberately can't run an + // async DETACH), so the "other" alias is left stranded exactly as it is + // in the detach_all test above. Same reason the follow-up commits below + // go through each database's own plain writer instead of reusing it. + let unobserved = writer.into_inner(); + drop(unobserved); + + // This alone would also pass under the bug, for the same reason as in the + // detach_all test: nothing has committed yet. + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not receive a notification for a never-committed change" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not receive a notification for a never-committed change" + ); + + let mut writer_a = observable_a.acquire_writer().await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('RealMain')") + .execute(&mut *writer_a) + .await + .unwrap(); + drop(writer_a); + + // The load-bearing check: A's first (and only) notification must carry the + // real value, not a phantom replay of the discarded buffer. + let change_a = timeout(Duration::from_millis(200), rx_a.recv()) + .await + .expect("should not time out") + .expect("A's subscriber should receive its own change"); + assert!( + change_a + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "RealMain"), + "A's first notification must be the real change, not the buffered one" + ); + assert!( + timeout(Duration::from_millis(100), rx_a.recv()) + .await + .is_err(), + "A must not receive a second, phantom notification" + ); + + let mut writer_b = observable_b.acquire_writer().await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('RealOther')") + .execute(&mut *writer_b) + .await + .unwrap(); + drop(writer_b); + + let change_b = timeout(Duration::from_millis(200), rx_b.recv()) + .await + .expect("should not time out") + .expect("B's subscriber should receive its own change"); + assert!( + change_b + .new_values + .expect("capture_values defaults to true") + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "RealOther"), + "B's first notification must be the real change, not the buffered one" + ); + assert!( + timeout(Duration::from_millis(100), rx_b.recv()) + .await + .is_err(), + "B must not receive a second, phantom notification" + ); +} + +#[tokio::test] +async fn main_write_reports_main_schema() { + let test_db = setup_test_db().await; + let config = ObserverConfig::new().with_tables(["users"]); + let observable = ObservableSqliteDatabase::new(test_db.db.clone(), config); + + let mut rx = observable.subscribe(["users"]); + let mut writer = observable.acquire_writer().await.unwrap(); + + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO users (name) VALUES ('Alice')") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("COMMIT").execute(&mut *writer).await.unwrap(); + + let change = timeout(Duration::from_millis(100), rx.recv()) + .await + .expect("should not time out") + .expect("should receive a change"); + assert_eq!(change.schema, "main"); +} + +#[tokio::test] +async fn attached_primary_key_uses_owning_schema() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + // A's `id` is column 0. + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + // B's `id` is column 1 - a different position than A's, so decoding B's + // write with A's (or no) TableInfo would extract the wrong column, or none + // at all, as the primary key. + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (label TEXT, id INTEGER PRIMARY KEY)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a, ObserverConfig::new().with_tables(["users"])); + let observable_b = + ObservableSqliteDatabase::new(db_b.clone(), ObserverConfig::new().with_tables(["users"])); + register_as_observed(&db_b, &observable_b); + + let mut rx_b = observable_b.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: db_b, + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }]; + + let mut writer = observable_a + .acquire_writer_with_attached(specs) + .await + .unwrap(); + sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); + sqlx::query("INSERT INTO other.users (label, id) VALUES ('widget', 42)") + .execute(&mut *writer) + .await + .unwrap(); + sqlx::query("COMMIT").execute(&mut *writer).await.unwrap(); + writer.detach_all().await.unwrap(); + + let change = timeout(Duration::from_millis(200), rx_b.recv()) + .await + .expect("should not time out") + .expect("B's subscriber should receive the change"); + assert_eq!(change.schema, "other"); + assert_eq!( + change.primary_key.len(), + 1, + "B's own TableInfo must have been queried - an empty primary_key means \ + ensure_table_info was skipped for the attached database" + ); + assert_eq!( + change.primary_key[0].as_integer(), + Some(42), + "must extract B's own id column (position 1), not whatever column A's \ + schema would put at the same index" + ); +} + +/// Table info must be warmed before the single write permit is taken - see +/// `ObservableSqliteDatabase::acquire_writer`'s body for why. +/// +/// The two orderings are indistinguishable without a third party, so the probe +/// here is an independent writer. With the read pool pinned to one connection +/// and this test holding it, the warming task can't proceed: under the correct +/// ordering it hasn't taken the write permit yet and the probe gets it +/// immediately, while under the reverse ordering it sits on the permit awaiting a +/// reader nobody will release and the probe blocks until sqlx's acquire timeout. +/// +/// **Does not catch a post-permit re-check regression.** If a second +/// `ensure_table_info()` call were added after the write permit is acquired (see +/// `acquire_writer`'s body for why that was investigated and rejected), this +/// test would still pass: the pre-permit warm above blocks on the held reader +/// first, so the permit is never taken and the rejected re-check is never +/// reached. That regression needs a dedicated test with a table that never +/// resolves in the schema, not this one. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn table_info_warming_does_not_hold_the_write_permit() { + let temp = tempfile::NamedTempFile::new().unwrap(); + let db = SqliteDatabase::connect( + temp.path().to_str().unwrap(), + Some(SqliteDatabaseConfig { + max_read_connections: 1, + ..Default::default() + }), + ) + .await + .unwrap(); + + let mut writer = db.acquire_writer().await.unwrap(); + sqlx::query("CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)") + .execute(&mut *writer) + .await + .unwrap(); + drop(writer); + + // Fresh observable: "users" is observed but its TableInfo has never been + // queried, so the next acquire_writer() has to warm it. + let observable = ObservableSqliteDatabase::new( + Arc::clone(&db), + ObserverConfig::new().with_tables(["users"]), + ); + + // Take the only read connection and keep it. + let held_reader = db.read_pool().unwrap().acquire().await.unwrap(); + + let warming = tokio::spawn({ + let observable = observable.clone(); + async move { observable.acquire_writer().await.map(|_| ()) } + }); + + // Let the warming task reach its await on the read pool. + tokio::time::sleep(Duration::from_millis(100)).await; + + let probe = timeout(Duration::from_millis(500), db.acquire_writer()).await; + assert!( + probe.is_ok(), + "an unrelated writer must still be able to take the write permit while \ + another task is warming table info; the permit is being held across a \ + read-pool await" + ); + drop(probe); + + // Release the reader so the warming task can finish, and confirm it does. + drop(held_reader); + timeout(Duration::from_secs(5), warming) + .await + .expect("warming task should finish once a read connection frees up") + .expect("warming task should not panic") + .expect("warming task should acquire its writer"); +} + +// ============================================================================ +// Broker-map collision guards (thread 10) - `acquire_writer_with_attached` +// used to seed the broker map with a blind `HashMap::insert`, so a spec +// aliased "main" would silently replace this database's own broker in the +// map, and two specs sharing an alias would silently collide. Neither was +// exploitable in practice only because `sqlx_sqlite_conn_mgr::validate_attached_specs` +// rejects both cases anyway, one layer away, after the map was already built - +// these tests pin that the rejection now happens up front, in this crate, as +// its own clear error, rather than relying on that other layer alone. +// ============================================================================ + +/// A `main`-aliased spec must be rejected before the broker map is built, and +/// as `validate_attached_specs`'s own `InvalidSchemaName` - not as some opaque +/// failure surfacing later out of `ATTACH` itself (which would report "database +/// main is already in use" once conn-mgr's own, independent validation pass +/// runs). The load-bearing half of this test is the assertion below: this +/// database's own broker must still work normally afterward - the rejection +/// must not have registered any hooks against a partially-built map. +#[tokio::test] +async fn acquire_writer_with_attached_rejects_main_aliased_spec_and_leaves_own_broker_intact() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a, ObserverConfig::new().with_tables(["users"])); + let mut rx_a = observable_a.subscribe(["users"]); + + let specs = vec![AttachedSpec { + database: db_b, + schema_name: "main".to_string(), + mode: AttachedMode::ReadWrite, + }]; + + let result = observable_a.acquire_writer_with_attached(specs).await; + assert!( + matches!( + result, + Err(sqlx_sqlite_observer::Error::ConnMgr( + sqlx_sqlite_conn_mgr::Error::InvalidSchemaName(_) + )) + ), + "a main-aliased spec should surface as conn-mgr's own InvalidSchemaName, \ + not an opaque ATTACH failure three steps later, got {:?}", + result.err() + ); + + // This database's own broker must still be reachable and working: a plain + // (non-attached) writer must still register hooks and publish normally. + let mut writer = observable_a + .acquire_writer() + .await + .expect("acquire writer after rejected spec"); + sqlx::query("INSERT INTO users (name) VALUES ('StillWorks')") + .execute(&mut *writer) + .await + .unwrap(); + drop(writer); + + let change = timeout(Duration::from_millis(200), rx_a.recv()) + .await + .expect("should not time out") + .expect("main's own broker must still be reachable after the rejected spec"); + assert_eq!(change.table, "users"); +} + +/// Same guard, for two specs sharing one alias rather than one spec aliased +/// `main`. Compared case-insensitively by `validate_attached_specs`, matching +/// SQLite's own schema namespace - this test uses an exact match rather than +/// an `"x"`/`"X"` pair since the case-insensitive comparison itself is already +/// pinned in `sqlx-sqlite-conn-mgr`'s own tests; this one is about this +/// crate's broker map staying untouched afterward. +#[tokio::test] +async fn acquire_writer_with_attached_rejects_duplicate_alias_and_leaves_own_broker_intact() { + let temp_a = tempfile::NamedTempFile::new().unwrap(); + let temp_b = tempfile::NamedTempFile::new().unwrap(); + let temp_c = tempfile::NamedTempFile::new().unwrap(); + let db_a = create_attachable_db( + &temp_a, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_b = create_attachable_db( + &temp_b, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + let db_c = create_attachable_db( + &temp_c, + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)", + ) + .await; + + let observable_a = + ObservableSqliteDatabase::new(db_a, ObserverConfig::new().with_tables(["users"])); + let mut rx_a = observable_a.subscribe(["users"]); + + let specs = vec![ + AttachedSpec { + database: db_b, + schema_name: "dup".to_string(), + mode: AttachedMode::ReadWrite, + }, + AttachedSpec { + database: db_c, + schema_name: "dup".to_string(), + mode: AttachedMode::ReadWrite, + }, + ]; + + let result = observable_a.acquire_writer_with_attached(specs).await; + assert!( + matches!( + result, + Err(sqlx_sqlite_observer::Error::ConnMgr( + sqlx_sqlite_conn_mgr::Error::DuplicateSchemaName(_) + )) + ), + "two specs sharing an alias should surface as conn-mgr's own \ + DuplicateSchemaName, not a collision inside this crate's own broker map, \ + got {:?}", + result.err() + ); + + let mut writer = observable_a + .acquire_writer() + .await + .expect("acquire writer after rejected specs"); + sqlx::query("INSERT INTO users (name) VALUES ('StillWorksToo')") + .execute(&mut *writer) + .await + .unwrap(); + drop(writer); + + let change = timeout(Duration::from_millis(200), rx_a.recv()) + .await + .expect("should not time out") + .expect("main's own broker must still be reachable after the rejected specs"); + assert_eq!(change.table, "users"); +} diff --git a/crates/sqlx-sqlite-toolkit/src/builders.rs b/crates/sqlx-sqlite-toolkit/src/builders.rs index 94d8582..ae08195 100644 --- a/crates/sqlx-sqlite-toolkit/src/builders.rs +++ b/crates/sqlx-sqlite-toolkit/src/builders.rs @@ -12,6 +12,46 @@ use crate::Error; use crate::pagination::{KeysetColumn, KeysetPage, build_paginated_query}; use crate::wrapper::{DatabaseWrapper, WriteQueryResult, bind_value}; +/// Runs `detach` and returns `result`, whichever way `result` went. +/// +/// Every attached-database query below has to detach on its error paths, not just +/// on success, because nothing else will: the attached guards' `Drop` impls can't +/// (an async `DETACH` from a synchronous `Drop`), and the pools' `after_release` +/// hook runs `ROLLBACK` but never `DETACH`. So a bare `?` between acquiring the +/// connection and detaching strands the alias on a *pooled* connection, and the +/// next acquisition that reuses both fails at `ATTACH` with "database ... is +/// already in use". For writes that is effectively permanent: the write pool is +/// `max_connections(1)`, and only genuine idleness (default 30s) retires the +/// connection. +/// +/// A detach failure never masks an error the query already produced - it's logged +/// instead, as in the transaction paths. On the success path there is no earlier +/// error to preserve, so a detach failure becomes the returned error and the rows +/// are dropped: the connection's alias state is then unknown, which is worse to +/// hand back as success than to report. +async fn detach_after(result: Result, detach: F) -> Result +where + F: FnOnce() -> Fut, + Fut: Future>, + E: std::fmt::Display, + Error: From, +{ + match result { + Ok(value) => { + detach().await?; + Ok(value) + } + Err(original) => { + if let Err(detach_err) = detach().await { + tracing::error!( + "detach_all failed while unwinding from an earlier error ({original}): {detach_err}" + ); + } + Err(original) + } + } +} + /// Builder for SELECT queries returning multiple rows pub struct FetchAllBuilder { db: Arc, @@ -56,16 +96,18 @@ impl FetchAllBuilder { let mut conn = sqlx_sqlite_conn_mgr::acquire_reader_with_attached(&self.db, self.attached).await?; - let mut q = sqlx::query(sqlx::AssertSqlSafe(self.query)); - for value in self.values { - q = bind_value(q, value); + let (query, values) = (self.query, self.values); + let result = async { + let mut q = sqlx::query(sqlx::AssertSqlSafe(query)); + for value in values { + q = bind_value(q, value); + } + let rows = sqlx::Executor::fetch_all(&mut *conn, q).await?; + decode_rows(rows) } - let rows = sqlx::Executor::fetch_all(&mut *conn, q).await?; - let result = decode_rows(rows)?; + .await; - // Explicit cleanup - conn.detach_all().await?; - Ok(result) + detach_after(result, || conn.detach_all()).await } } } @@ -122,15 +164,19 @@ impl FetchOneBuilder { let mut conn = sqlx_sqlite_conn_mgr::acquire_reader_with_attached(&self.db, self.attached).await?; - let mut q = sqlx::query(sqlx::AssertSqlSafe(self.query)); - for value in self.values { - q = bind_value(q, value); + let (query, values) = (self.query, self.values); + let result = async { + let mut q = sqlx::query(sqlx::AssertSqlSafe(query)); + for value in values { + q = bind_value(q, value); + } + sqlx::Executor::fetch_all(&mut *conn, q) + .await + .map_err(Error::from) } - let rows = sqlx::Executor::fetch_all(&mut *conn, q).await?; + .await; - // Explicit cleanup - conn.detach_all().await?; - rows + detach_after(result, || conn.detach_all()).await? }; // Validate row count @@ -268,15 +314,18 @@ impl FetchPageBuilder { let mut conn = sqlx_sqlite_conn_mgr::acquire_reader_with_attached(&self.db, self.attached).await?; - let mut q = sqlx::query(sqlx::AssertSqlSafe(sql)); - for value in all_values { - q = bind_value(q, value); + let result = async { + let mut q = sqlx::query(sqlx::AssertSqlSafe(sql)); + for value in all_values { + q = bind_value(q, value); + } + sqlx::Executor::fetch_all(&mut *conn, q) + .await + .map_err(Error::from) } - let rows = sqlx::Executor::fetch_all(&mut *conn, q).await?; + .await; - // Explicit cleanup - conn.detach_all().await?; - rows + detach_after(result, || conn.detach_all()).await? }; // Decode rows @@ -376,23 +425,27 @@ impl ExecuteBuilder { }) } else { // With attached database(s) - acquire writer with attached database(s) - let mut conn = - sqlx_sqlite_conn_mgr::acquire_writer_with_attached(self.db.inner(), self.attached) - .await?; - - let mut q = sqlx::query(sqlx::AssertSqlSafe(self.query)); - for value in self.values { - q = bind_value(q, value); + // (routes through the observer when in use, same as the non-attached + // branch above - this is also the main database's own writer, so + // observation applies even if nothing ends up attached in ReadWrite + // mode) + let mut conn = self.db.acquire_writer_with_attached(self.attached).await?; + + let (query, values) = (self.query, self.values); + let result = async { + let mut q = sqlx::query(sqlx::AssertSqlSafe(query)); + for value in values { + q = bind_value(q, value); + } + let result = sqlx::Executor::execute(&mut *conn, q).await?; + Ok(WriteQueryResult { + rows_affected: result.rows_affected(), + last_insert_id: result.last_insert_rowid(), + }) } - let result = sqlx::Executor::execute(&mut *conn, q).await?; - let write_result = WriteQueryResult { - rows_affected: result.rows_affected(), - last_insert_id: result.last_insert_rowid(), - }; + .await; - // Explicit cleanup - conn.detach_all().await?; - Ok(write_result) + detach_after(result, || conn.detach_all()).await } } } diff --git a/crates/sqlx-sqlite-toolkit/src/lib.rs b/crates/sqlx-sqlite-toolkit/src/lib.rs index 627801c..6746ded 100644 --- a/crates/sqlx-sqlite-toolkit/src/lib.rs +++ b/crates/sqlx-sqlite-toolkit/src/lib.rs @@ -49,7 +49,7 @@ pub use transactions::{ Statement, TransactionWriter, cleanup_all_transactions, cleanup_transactions_for_db, }; pub use wrapper::{ - DatabaseWrapper, InterruptibleTransaction, InterruptibleTransactionBuilder, + AttachedWriterGuard, DatabaseWrapper, InterruptibleTransaction, InterruptibleTransactionBuilder, TransactionExecutionBuilder, WriteQueryResult, WriterGuard, bind_value, }; diff --git a/crates/sqlx-sqlite-toolkit/src/transactions.rs b/crates/sqlx-sqlite-toolkit/src/transactions.rs index 587cc1d..a8a9469 100644 --- a/crates/sqlx-sqlite-toolkit/src/transactions.rs +++ b/crates/sqlx-sqlite-toolkit/src/transactions.rs @@ -16,7 +16,7 @@ use tracing::{debug, warn}; #[cfg(feature = "observer")] use sqlx_sqlite_observer::ObservableWriteGuard; -use crate::wrapper::WriterGuard; +use crate::wrapper::{AttachedWriterGuard, WriterGuard}; use crate::{Error, Result, WriteQueryResult}; /// Wrapper around WriteGuard, ObservableWriteGuard, or AttachedWriteGuard @@ -24,6 +24,11 @@ use crate::{Error, Result, WriteQueryResult}; pub enum TransactionWriter { Regular(WriteGuard), Attached(AttachedWriteGuard), + /// An observable writer, attached or not. One variant rather than two + /// because `ObservableWriteGuard` already knows which kind of writer it wraps + /// and handles both in `detach_all()`. Two variants of the same type would + /// duplicate that distinction with nothing but the `From` impl below keeping + /// them aligned - and a mis-map there strands the `ATTACH` alias. #[cfg(feature = "observer")] Observable(ObservableWriteGuard), } @@ -75,8 +80,17 @@ impl TransactionWriter { /// Detach all attached databases if this is an attached writer pub async fn detach_if_attached(self) -> Result<()> { - if let Self::Attached(w) = self { - w.detach_all().await?; + match self { + Self::Attached(w) => w.detach_all().await?, + // Called unconditionally, deliberately. For a non-attached inner writer + // `detach_all()` skips the DETACH but still unregisters the hooks and + // discards the guard's buffered events - work `Drop` would otherwise do + // at an unspecified point. Do not "optimize" this into a no-op arm for a + // writer that looks unattached; only the guard knows, and getting it + // wrong strands the alias on the single write connection permanently. + #[cfg(feature = "observer")] + Self::Observable(w) => w.detach_all().await?, + Self::Regular(_) => {} } Ok(()) } @@ -92,6 +106,16 @@ impl From for TransactionWriter { } } +impl From for TransactionWriter { + fn from(guard: AttachedWriterGuard) -> Self { + match guard { + AttachedWriterGuard::Regular(w) => TransactionWriter::Attached(w), + #[cfg(feature = "observer")] + AttachedWriterGuard::Observable(w) => TransactionWriter::Observable(w), + } + } +} + /// Active transaction state holding the writer and metadata #[must_use = "if unused, the transaction is immediately rolled back"] pub struct ActiveInterruptibleTransaction { diff --git a/crates/sqlx-sqlite-toolkit/src/wrapper.rs b/crates/sqlx-sqlite-toolkit/src/wrapper.rs index aea9b4d..dfaad0c 100644 --- a/crates/sqlx-sqlite-toolkit/src/wrapper.rs +++ b/crates/sqlx-sqlite-toolkit/src/wrapper.rs @@ -9,7 +9,9 @@ use sqlx_sqlite_conn_mgr::{SqliteDatabase, SqliteDatabaseConfig, WriteGuard}; use tracing::warn; #[cfg(feature = "observer")] -use sqlx_sqlite_observer::{ObservableSqliteDatabase, ObservableWriteGuard, ObserverConfig}; +use sqlx_sqlite_observer::{ + ObservableSqliteDatabase, ObservableWriteGuard, ObservationBroker, ObserverConfig, +}; use crate::Error; @@ -58,6 +60,68 @@ impl DerefMut for WriterGuard { } } +/// Unified attached-writer guard that routes through the observer when enabled. +/// +/// Mirrors [`WriterGuard`]'s Regular/Observable split, but for the +/// attached-database acquisition path (see +/// [`DatabaseWrapper::acquire_writer_with_attached`]). +/// +/// Derefs to `SqliteConnection` so it can be used with `sqlx::query().execute()`. +/// +/// **Important**: call [`detach_all`](Self::detach_all) before dropping. Neither +/// inner guard detaches in `Drop`, so a dropped guard leaves the alias on the +/// pooled write connection until that connection is eventually closed - and the +/// write pool holds a single connection, so every later attach of that alias +/// fails until then. `#[must_use]` catches the discarded-guard case but cannot +/// carry this hazard in its message, which is why it's stated here. +#[must_use = "if unused, the write guard and locks are immediately dropped"] +pub enum AttachedWriterGuard { + /// Plain attached writer from the connection manager - unobserved. + Regular(sqlx_sqlite_conn_mgr::AttachedWriteGuard), + /// Attached writer that tracks changes via SQLite hooks, routed per-schema + /// to whichever database owns each affected table. + #[cfg(feature = "observer")] + Observable(ObservableWriteGuard), +} + +impl Deref for AttachedWriterGuard { + type Target = SqliteConnection; + + fn deref(&self) -> &Self::Target { + match self { + AttachedWriterGuard::Regular(w) => w, + #[cfg(feature = "observer")] + AttachedWriterGuard::Observable(w) => w, + } + } +} + +impl DerefMut for AttachedWriterGuard { + fn deref_mut(&mut self) -> &mut Self::Target { + match self { + AttachedWriterGuard::Regular(w) => &mut *w, + #[cfg(feature = "observer")] + AttachedWriterGuard::Observable(w) => &mut *w, + } + } +} + +impl AttachedWriterGuard { + /// Detach all attached databases from this writer. + /// + /// See `sqlx_sqlite_conn_mgr::AttachedWriteGuard::detach_all` / + /// `ObservableWriteGuard::detach_all` for what this does on each side; + /// both are safe to call only after an explicit commit or rollback has + /// already run. + pub async fn detach_all(self) -> Result<(), Error> { + match self { + AttachedWriterGuard::Regular(w) => Ok(w.detach_all().await?), + #[cfg(feature = "observer")] + AttachedWriterGuard::Observable(w) => Ok(w.detach_all().await?), + } + } +} + /// Wrapper around SqliteDatabase that provides a high-level API for database operations. /// /// This struct is the main entry point for interacting with SQLite databases through @@ -65,12 +129,15 @@ impl DerefMut for WriterGuard { /// builder-pattern APIs for queries, transactions, and write operations. /// /// When the `observer` feature is enabled, the wrapper can also manage an -/// `ObservableSqliteDatabase` for change notification support. +/// `ObservableSqliteDatabase` for change notification support. Observation state +/// itself lives on the shared `SqliteDatabase` (see [`enable_observation`'s +/// doc](Self::enable_observation)), not on this struct, so `#[derive(Clone)]` +/// cloning only `inner` is exactly what makes every clone of a `DatabaseWrapper`, +/// and every independent `connect()` call to the same path, observe through the +/// same broker. #[derive(Clone)] pub struct DatabaseWrapper { inner: Arc, - #[cfg(feature = "observer")] - observer: Option, } impl DatabaseWrapper { @@ -91,11 +158,32 @@ impl DatabaseWrapper { /// /// When observation is enabled, returns an observable writer that tracks /// changes via SQLite hooks. Otherwise, returns a regular writer. + /// + /// **Known limitation:** the broker read from the slot here is snapshotted + /// into the returned guard for its whole lifetime, so a + /// [`disable_observation`] + [`enable_observation`] cycle during an open + /// transaction leaves it publishing to the previous broker - silently, as + /// far as any status check is concerned. See + /// `sqlx_sqlite_observer::ObservableSqliteDatabase::acquire_writer`'s doc + /// for the mechanics and [`disable_observation`] for the reachable trigger. + /// + /// [`disable_observation`]: Self::disable_observation + /// [`enable_observation`]: Self::enable_observation pub async fn acquire_writer(&self) -> Result { #[cfg(feature = "observer")] - if let Some(ref observable) = self.observer { - let writer = observable.acquire_writer().await.map_err(Error::Observer)?; - return Ok(WriterGuard::Observable(writer)); + { + // Read the broker out of the slot and drop the guard immediately - + // `get()` already does this internally - before doing anything async, + // so no lock guard is ever held across an `.await` here. The slot + // holds `Arc`, not `Arc` + // (see `ObservableSqliteDatabase::from_broker`'s doc), so the handle + // is rebuilt here rather than read out directly. + let broker = self.inner.observer_slot().get::(); + if let Some(broker) = broker { + let observable = ObservableSqliteDatabase::from_broker(Arc::clone(&self.inner), broker); + let writer = observable.acquire_writer().await.map_err(Error::Observer)?; + return Ok(WriterGuard::Observable(writer)); + } } Ok(WriterGuard::Regular(self.inner.acquire_writer().await?)) @@ -106,10 +194,110 @@ impl DatabaseWrapper { /// This always bypasses the observer, even when observation is enabled. /// Useful when you need a writer for operations that should not trigger /// change notifications (e.g., internal bookkeeping). + /// + /// **This is an intentional, documented bypass, not a hole to close.** + /// Callers who reach for this method are opting out of observation for this + /// one writer; every other writer obtained via [`acquire_writer`] on this + /// same (now database-wide) observation state still gets tracked normally. + /// When observation is enabled, calling this logs a `tracing::warn!` as a + /// development-time aid - it compiles out entirely in release builds, since + /// this workspace pins `tracing` with `release_max_level_off`, so don't rely + /// on it surfacing in a shipped app. Reads never need an equivalent bypass or + /// warning: [`fetch_all`](Self::fetch_all), [`fetch_one`](Self::fetch_one), + /// and [`fetch_page`](Self::fetch_page) all go through the read pool, which + /// is opened `read_only(true)` and therefore can never write, observed or not. + /// + /// [`acquire_writer`]: Self::acquire_writer pub async fn acquire_regular_writer(&self) -> Result { + // Checked via the slot directly rather than `is_observing()`, which goes + // through `observable()` and builds a whole handle only to drop it - the + // `warn!` compiles out in release but the condition does not. + #[cfg(feature = "observer")] + if self + .inner + .observer_slot() + .get::() + .is_some() + { + warn!( + "acquire_regular_writer() called while observation is enabled on this \ + database; writes through this guard will not be tracked or published \ + to subscribers. This is intentional if you meant to bypass \ + observation - otherwise use acquire_writer() instead." + ); + } + Ok(self.inner.acquire_writer().await?) } + /// Acquire a writer guard with one or more databases attached. + /// + /// When observation is enabled, routes through the observer so that writes + /// into attached databases are tracked too - each change publishes to the + /// broker of whichever database *owns* the affected table, not necessarily + /// this one. See + /// `sqlx_sqlite_observer::acquire_writer_with_attached_brokers` + /// for the exact routing rule. When observation is not enabled, this falls + /// back to the plain conn-mgr attached-writer acquisition, identical to + /// calling `sqlx_sqlite_conn_mgr::acquire_writer_with_attached` directly. + /// + /// **Observation is checked on both sides, not just this database.** The + /// observable path is taken when this database is observed *or* any + /// `ReadWrite` spec's database is, since an attachment observed on its own + /// still needs hooks registered for its subscribers to hear writes made + /// through this call. Only when neither side is observed does this fall back + /// to the plain conn-mgr call, which keeps an entirely unobserved caller + /// from paying `lock_handle()` and FFI hook registration for nothing, and + /// from newly requiring `SQLITE_ENABLE_PREUPDATE_HOOK` on a build that never + /// asked for observation. [`acquire_writer_with_attached_brokers`] takes a + /// free function's `Option` broker rather than being a method because this + /// call has no `Self` to invoke when its own observation is off. + /// + /// This gate and that function's own map build are not atomic: it re-reads + /// every slot. If observation is disabled on all sides in between, the + /// observable path is taken with an empty map, which it handles by skipping + /// hook registration - so the race is inert rather than contradicting the + /// rationale above. + /// + /// [`acquire_writer_with_attached_brokers`]: sqlx_sqlite_observer::acquire_writer_with_attached_brokers + pub async fn acquire_writer_with_attached( + &self, + specs: Vec, + ) -> Result { + #[cfg(feature = "observer")] + { + // Same immediate clone-and-drop, and same slot-holds-a-broker + // rebuild, as acquire_writer() - see its comments. + let main_broker = self.inner.observer_slot().get::(); + + // Whether any ReadWrite spec's own database is observed, independent of + // this database's `main_broker` above - see this method's doc for why + // either side alone is enough to take the observable path. + let any_readwrite_attachment_observed = specs.iter().any(|spec| { + spec.mode == sqlx_sqlite_conn_mgr::AttachedMode::ReadWrite + && spec + .database + .observer_slot() + .get::() + .is_some() + }); + + if main_broker.is_some() || any_readwrite_attachment_observed { + let guard = sqlx_sqlite_observer::acquire_writer_with_attached_brokers( + &self.inner, + main_broker, + specs, + ) + .await?; + return Ok(AttachedWriterGuard::Observable(guard)); + } + } + + Ok(AttachedWriterGuard::Regular( + sqlx_sqlite_conn_mgr::acquire_writer_with_attached(&self.inner, specs).await?, + )) + } + /// Begin an interruptible transaction that can be paused and resumed. /// /// Returns a builder that allows attaching databases before executing the transaction. @@ -163,11 +351,7 @@ impl DatabaseWrapper { ) -> Result { let db = SqliteDatabase::connect(abs_path, custom_config).await?; - Ok(Self { - inner: db, - #[cfg(feature = "observer")] - observer: None, - }) + Ok(Self { inner: db }) } /// Create a builder for write queries (INSERT/UPDATE/DELETE). @@ -350,6 +534,11 @@ impl DatabaseWrapper { /// /// Runs all pending migrations from the provided migrator. /// SQLx tracks applied migrations, so this is safe to call multiple times. + /// + /// Migrations run through `self.inner` directly, never through the observer, + /// even when observation is enabled - schema changes are not row changes and + /// have no `TableChange` representation, so there's nothing for a subscriber + /// to receive here regardless. pub async fn run_migrations( &self, migrator: &sqlx_sqlite_conn_mgr::Migrator, @@ -362,8 +551,10 @@ impl DatabaseWrapper { /// /// Checkpoints the WAL and closes all connection pools. /// If observation is enabled, it is disabled first to unregister SQLite hooks - /// and allow the write connection to close cleanly. - pub async fn close(mut self) -> Result<(), Error> { + /// and allow the write connection to close cleanly - which, per + /// [`disable_observation`](Self::disable_observation), affects every handle + /// to this database. + pub async fn close(self) -> Result<(), Error> { #[cfg(feature = "observer")] self.disable_observation(); @@ -375,8 +566,9 @@ impl DatabaseWrapper { /// /// Removes the main database file, WAL, and SHM files. /// If observation is enabled, it is disabled first to unregister SQLite hooks - /// and allow the write connection to close cleanly. - pub async fn remove(mut self) -> Result<(), Error> { + /// and allow the write connection to close cleanly. Same database-wide caveat + /// as [`close`](Self::close). + pub async fn remove(self) -> Result<(), Error> { #[cfg(feature = "observer")] self.disable_observation(); @@ -389,12 +581,35 @@ impl DatabaseWrapper { /// After calling this, write operations will be tracked and subscribers /// can receive change notifications. /// - /// **Additive, not destructive:** if observation is already enabled, the existing - /// broker is reused rather than replaced. The requested tables are unioned into - /// its observed-table set, and any subscribers created before this call keep - /// receiving notifications uninterrupted — this is what allows independent callers - /// (e.g. multiple windows observing the same database) to call `enable_observation` - /// without tearing down each other's subscriptions. + /// **Database-wide, not per-handle (issue #53):** observation state lives on + /// the shared `SqliteDatabase` behind `self.inner`, not on this + /// `DatabaseWrapper` value. Every clone of this wrapper and every independent + /// `DatabaseWrapper::connect()` call that resolves to the same underlying file + /// observes through the same broker - there is no such thing as "my own" + /// observation separate from anyone else's handle to this database. `:memory:` + /// databases are the one exception: each `connect()` call gets its own + /// `SqliteDatabase` (they're excluded from the path registry), so they're + /// independently observed by construction, not because of anything special + /// here. + /// + /// **Additive, not destructive (issue #54):** if observation is already + /// enabled - by this handle, a clone of it, or a completely independent + /// connection to the same database - the existing broker is reused rather + /// than replaced. The requested tables are unioned into its observed-table + /// set, and any subscribers created before this call keep receiving + /// notifications uninterrupted. This is what allows independent callers (e.g. + /// multiple windows observing the same database) to call `enable_observation` + /// without tearing down each other's subscriptions. The check for an existing + /// broker, the creation of a new one, and - on the reuse path - the merge of + /// the requested tables into the existing broker's observed set all happen + /// under the database's observer slot's single write lock, so two callers + /// racing to be first can't each build their own broker and have one + /// silently overwrite (and orphan the subscribers of) the other, and a + /// concurrent [`disable_observation`](Self::disable_observation) can't land + /// in the middle of the merge and have this call's tables register against + /// a broker the slot no longer points to. The lock is released before this + /// returns, though, so a `disable_observation()` immediately afterward still + /// tears down what this call just set up. /// /// `config.channel_capacity` and `config.capture_values` can only take effect on /// the *first* call that enables observation for this database. Both are baked @@ -413,69 +628,158 @@ impl DatabaseWrapper { /// is to read them back afterward via `broker().channel_capacity()` / /// `.capture_values()`. Call [`disable_observation`](Self::disable_observation) /// first if you need to change these values, accepting that existing - /// subscribers will be dropped. + /// subscribers will be dropped - and, per the database-wide note above, dropped + /// for every handle to this database, not just this one. /// /// Requires the `observer` feature. #[cfg(feature = "observer")] - pub fn enable_observation(&mut self, config: ObserverConfig) { - if let Some(existing) = &self.observer { - let broker = existing.broker(); - - if config.channel_capacity != broker.channel_capacity() - || config.capture_values != broker.capture_values() - { - warn!( - requested_channel_capacity = config.channel_capacity, - active_channel_capacity = broker.channel_capacity(), - requested_capture_values = config.capture_values, - active_capture_values = broker.capture_values(), - "enable_observation() called with different channel_capacity/capture_values \ - while observation is already active; keeping the original values since \ - recreating the broadcast channel would drop existing subscribers. Only the \ - requested tables were merged in." - ); - } + pub fn enable_observation(&self, config: ObserverConfig) { + let requested_channel_capacity = config.channel_capacity; + let requested_capture_values = config.capture_values; + let requested_tables = config.tables.clone(); + let inner = Arc::clone(&self.inner); - if !config.tables.is_empty() { - broker.observe_tables(config.tables.iter().map(String::as_str)); - } + // `get_or_init_with` rather than `get_or_init` so the merge below runs under + // the same write lock that decided "reuse, don't create" - see its doc. + // Doing the merge after that lock released would leave a window for a + // concurrent `disable_observation()` to clear the slot, orphaning this + // call's `observe_tables()` on a broker nothing points to. + let result = self.inner.observer_slot().get_or_init_with( + || { + // `ObservableSqliteDatabase::new` stays the single place an + // `ObserverConfig` becomes a broker, but only the broker goes in the + // slot - see `ObservableSqliteDatabase::from_broker`'s doc for why + // storing the whole handle would form a reference cycle. + let observable = ObservableSqliteDatabase::new(inner, config); + Arc::clone(observable.broker()) + }, + |broker| { + // Merge path: a broker already existed (this handle's own prior + // call, a clone's, or an entirely independent connection's) and + // get_or_init_with left it in place rather than replacing it. + if requested_channel_capacity != broker.channel_capacity() + || requested_capture_values != broker.capture_values() + { + warn!( + requested_channel_capacity = requested_channel_capacity, + active_channel_capacity = broker.channel_capacity(), + requested_capture_values = requested_capture_values, + active_capture_values = broker.capture_values(), + "enable_observation() called with different channel_capacity/capture_values \ + while observation is already active; keeping the original values since \ + recreating the broadcast channel would drop existing subscribers. Only the \ + requested tables were merged in." + ); + } - return; - } + if !requested_tables.is_empty() { + broker.observe_tables(requested_tables.iter().map(String::as_str)); + } + }, + ); - self.observer = Some(ObservableSqliteDatabase::new( - Arc::clone(&self.inner), - config, - )); + if result.is_none() { + // The slot holds a value of some other type - a programming error + // elsewhere in this process, since this method is the slot's only + // writer. Nothing safe to do here but warn and leave observation + // exactly as it was; see `ObserverSlot::get_or_init`'s doc for why + // this can't happen from repeated calls to this method alone. + warn!( + "enable_observation: observer slot for this database holds a value \ + of an unexpected type; leaving observation state untouched" + ); + } } /// Disable observation on this database. /// - /// Drops the observable wrapper and stops tracking changes. + /// Clears the database's observer slot and stops tracking changes. /// Existing subscribers will stop receiving notifications. /// + /// **Affects every handle to this database (issue #53).** Observation is + /// database-wide (see [`enable_observation`](Self::enable_observation)), so + /// this tears it down for clones and independent `connect()` callers alike, + /// including ones this call has no way to know about. Nothing here counts + /// how many callers still want observation; a caller that needs that must + /// coordinate above this crate. + /// + /// **The coordination that exists above this crate does not cover you.** The + /// `tauri-plugin-sqlite` layer reference-counts observation per *webview + /// label* (issue #54), and a Rust caller holding a `DatabaseWrapper` registers + /// nothing there. So the plugin's `unobserve()` (or a window being destroyed) + /// can drive that count to zero and call this method on the database you are + /// observing, ending your subscription without you having called anything. + /// Symmetrically, calling this yourself leaves those registrations non-zero + /// while the slot is empty: the plugin's `subscribe()` then fails with + /// `OBSERVATION_NOT_ENABLED`, and the next `observe()` builds a fresh broker + /// that other windows' existing subscriptions are not bound to. A Rust + /// consumer that must not be torn down needs its own database file - the + /// broker is keyed by canonical path, so registering the same file under a + /// different plugin key still shares it. + /// + /// **Known limitation: this clears the slot, but never reaches a writer + /// that already has a broker bound.** Calling this and then + /// [`enable_observation`](Self::enable_observation) while such a writer's + /// transaction is open leaves it publishing to the broker it bound at + /// acquisition, so a subscriber created after the cycle misses that commit + /// while pre-existing ones still receive it - with `is_observing()`, the new + /// `subscribe()`, and the commit all reporting success. The reachable + /// trigger: the last window's `unobserve()` runs, then a new caller's + /// `observe()`, while another caller's interruptible transaction is still + /// open. See + /// `sqlx_sqlite_observer::ObservableSqliteDatabase::acquire_writer`'s doc + /// for the mechanics and the deferred fix. + /// /// Requires the `observer` feature. #[cfg(feature = "observer")] - pub fn disable_observation(&mut self) { - self.observer = None; + pub fn disable_observation(&self) { + self.inner.observer_slot().clear(); } - /// Get a reference to the observable database, if observation is enabled. + /// Get an owned handle to the observable database, if observation is enabled. /// /// Returns `None` if observation has not been enabled via `enable_observation()`. /// + /// Returns an owned `ObservableSqliteDatabase` rather than a reference, since a + /// reference into the observer slot can't escape the slot's internal lock + /// guard. The slot itself only holds the broker (see + /// `ObservableSqliteDatabase::from_broker`'s doc for why), so the handle is + /// rebuilt from `self.inner` plus that broker - two refcount bumps, not a deep + /// copy - semantically identical to holding a reference for as long as you need + /// one. + /// /// Requires the `observer` feature. #[cfg(feature = "observer")] - pub fn observable(&self) -> Option<&ObservableSqliteDatabase> { - self.observer.as_ref() + pub fn observable(&self) -> Option { + self + .inner + .observer_slot() + .get::() + .map(|broker| ObservableSqliteDatabase::from_broker(Arc::clone(&self.inner), broker)) } /// Returns true if observation is currently enabled on this database. /// + /// Deliberately defined in terms of [`observable`](Self::observable) rather + /// than the slot's own `is_set()` (which only checks that *something* is + /// there, not that it downcasts to the `ObservationBroker` this layer + /// stores). On the slot type-mismatch case documented on + /// `ObserverSlot::get`, `is_set()` would report `true` while `observable()` + /// returns `None` - a predicate built on `is_set()` would then claim + /// observation is on while every acquisition path silently took its + /// unobserved branch. Defining it this way keeps "is observing" and + /// "`observable()` returns `Some`" in agreement, which is what makes this + /// usable as an external invariant: the `tauri-plugin-sqlite` layer's + /// lock-order tests assert it against its own observer registrations. The + /// acquisition paths in this file - [`acquire_writer`](Self::acquire_writer) + /// and [`acquire_regular_writer`](Self::acquire_regular_writer) - read the + /// slot directly instead, since they need the broker itself (or just a + /// boolean) without building a handle only to drop it. + /// /// Requires the `observer` feature. #[cfg(feature = "observer")] pub fn is_observing(&self) -> bool { - self.observer.is_some() + self.observable().is_some() } } @@ -513,14 +817,23 @@ impl InterruptibleTransactionBuilder { let guard = self.db.acquire_writer().await?; TransactionWriter::from(guard) } else { - let guard = - sqlx_sqlite_conn_mgr::acquire_writer_with_attached(self.db.inner(), self.attached) - .await?; - TransactionWriter::Attached(guard) + let guard = self.db.acquire_writer_with_attached(self.attached).await?; + TransactionWriter::from(guard) }; - // Begin transaction - writer.begin_immediate().await?; + // Begin transaction. A failure here (a busy database, say) is the one early + // return not covered by `ActiveInterruptibleTransaction`'s Drop, since the + // writer hasn't been handed over yet - so detach explicitly, or the alias + // strands on the pooled write connection (see `builders::detach_after`). + if let Err(err) = writer.begin_immediate().await { + if let Err(detach_err) = writer.detach_if_attached().await { + tracing::error!( + "detach_all failed after BEGIN IMMEDIATE failed: {}", + detach_err + ); + } + return Err(err); + } // Create active transaction and execute initial statements let mut active_tx = ActiveInterruptibleTransaction::new( @@ -618,14 +931,22 @@ impl TransactionExecutionBuilder { let guard = self.db.acquire_writer().await?; TransactionWriter::from(guard) } else { - let guard = - sqlx_sqlite_conn_mgr::acquire_writer_with_attached(self.db.inner(), self.attached) - .await?; - TransactionWriter::Attached(guard) + let guard = self.db.acquire_writer_with_attached(self.attached).await?; + TransactionWriter::from(guard) }; - // Begin transaction - writer.begin_immediate().await?; + // Begin transaction. Same reasoning as the commit/rollback arms below: this + // early return has to detach too, or the alias strands on the single write + // connection. + if let Err(err) = writer.begin_immediate().await { + if let Err(detach_err) = writer.detach_if_attached().await { + tracing::error!( + "detach_all failed after BEGIN IMMEDIATE failed: {}", + detach_err + ); + } + return Err(err); + } // Execute all statements let exec_result = async { diff --git a/crates/sqlx-sqlite-toolkit/tests/attached_detach_tests.rs b/crates/sqlx-sqlite-toolkit/tests/attached_detach_tests.rs new file mode 100644 index 0000000..9e06e0e --- /dev/null +++ b/crates/sqlx-sqlite-toolkit/tests/attached_detach_tests.rs @@ -0,0 +1,343 @@ +//! An attached-database query that fails must still release its `ATTACH` alias. +//! +//! Nothing else will do it, and a stranded alias wedges every later attach of +//! that name on the same pooled connection - see `builders::detach_after`'s doc +//! for the mechanism. Each test below strands the alias if the fix regresses, +//! then proves it didn't by reusing the same alias immediately. + +use std::sync::Arc; + +use sqlx::ConnectOptions; +use sqlx_sqlite_conn_mgr::{AttachedMode, AttachedSpec, SqliteDatabaseConfig}; +use sqlx_sqlite_toolkit::{DatabaseWrapper, KeysetColumn}; +use tempfile::TempDir; + +/// Returns (main, other, tempdir). `main` has `users`, `other` has `logs`. +/// +/// `main`'s read pool is pinned to a single connection: the write pool is already +/// `max_connections(1)`, so a stranded write alias is always hit again on the next +/// attempt, but with the default read pool of 6 a stranded *read* alias is usually +/// dodged by landing on a different connection - which would make the read tests +/// pass whether or not the detach happened. +async fn two_databases() -> (DatabaseWrapper, DatabaseWrapper, TempDir) { + let temp = TempDir::new().expect("temp dir"); + + let single_reader = SqliteDatabaseConfig { + max_read_connections: 1, + ..Default::default() + }; + + let main = DatabaseWrapper::connect(&temp.path().join("main.db"), Some(single_reader)) + .await + .expect("connect main"); + let other = DatabaseWrapper::connect(&temp.path().join("other.db"), None) + .await + .expect("connect other"); + + main + .execute( + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)".into(), + vec![], + ) + .await + .expect("create users"); + other + .execute( + "CREATE TABLE logs (id INTEGER PRIMARY KEY, msg TEXT)".into(), + vec![], + ) + .await + .expect("create logs"); + + (main, other, temp) +} + +#[tokio::test] +async fn failed_attached_write_still_releases_the_alias() { + let (main, other, _temp) = two_databases().await; + + let make_spec = || { + vec![AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }] + }; + + // ATTACH succeeds, the statement itself fails. This is the shape that + // stranded the alias: the error return skipped the detach. + let err = main + .execute( + "INSERT INTO other.no_such_table (msg) VALUES ('x')".into(), + vec![], + ) + .attach(make_spec()) + .await + .expect_err("write to a nonexistent table should fail"); + assert!( + err.to_string().contains("no such table"), + "expected the statement's own error, got: {err}" + ); + + // Same alias, same (single) write connection, valid statement. + main + .execute( + "INSERT INTO other.logs (msg) VALUES ('after failure')".into(), + vec![], + ) + .attach(make_spec()) + .await + .expect("the alias must be reusable after a failed attached write"); +} + +#[tokio::test] +async fn failed_attached_read_still_releases_the_alias() { + let (main, other, _temp) = two_databases().await; + + let make_spec = || { + vec![AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadOnly, + }] + }; + + let err = main + .fetch_all("SELECT * FROM other.no_such_table".into(), vec![]) + .attach(make_spec()) + .await + .expect_err("read from a nonexistent table should fail"); + assert!( + err.to_string().contains("no such table"), + "expected the statement's own error, got: {err}" + ); + + main + .fetch_all("SELECT * FROM other.logs".into(), vec![]) + .attach(make_spec()) + .await + .expect("the alias must be reusable after a failed attached read"); +} + +/// `fetch_one` and `fetch_page` route through the same helper as the two above, +/// but "shares a helper" is exactly the kind of assumption that stops being true +/// during a refactor, and each builder wires the guarded region up itself. +#[tokio::test] +async fn failed_attached_fetch_one_and_fetch_page_still_release_the_alias() { + let (main, other, _temp) = two_databases().await; + + let make_spec = || { + vec![AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadOnly, + }] + }; + + main + .fetch_one("SELECT * FROM other.no_such_table".into(), vec![]) + .attach(make_spec()) + .await + .expect_err("fetch_one on a nonexistent table should fail"); + main + .fetch_one("SELECT * FROM other.logs LIMIT 1".into(), vec![]) + .attach(make_spec()) + .await + .expect("the alias must be reusable after a failed attached fetch_one"); + + let keyset = vec![KeysetColumn::asc("id")]; + main + .fetch_page( + "SELECT * FROM other.no_such_table".into(), + vec![], + keyset.clone(), + 10, + ) + .attach(make_spec()) + .await + .expect_err("fetch_page on a nonexistent table should fail"); + main + .fetch_page("SELECT * FROM other.logs".into(), vec![], keyset, 10) + .attach(make_spec()) + .await + .expect("the alias must be reusable after a failed attached fetch_page"); +} + +/// The transaction and interruptible-transaction builders detach on their own +/// error paths too, but "shares the alias-release contract with the builders +/// above" is exactly the kind of assumption a refactor can quietly break - +/// each of these three tests below pins one specific `detach_if_attached()` +/// call site by name. +#[tokio::test] +async fn failed_attached_transaction_statement_still_releases_the_alias() { + let (main, other, _temp) = two_databases().await; + + let make_spec = || { + vec![AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }] + }; + + // The transaction's first statement succeeds; its second fails on a + // nonexistent table, so the whole transaction rolls back - including the + // first statement's insert. Pins `detach_if_attached()` in the `Err(e)` + // arm of `TransactionExecutionBuilder::execute`: without it, the alias + // strands on the single write connection and the reuse below fails at + // ATTACH instead of exercising anything interesting. + let err = main + .execute_transaction(vec![ + ("INSERT INTO other.logs (msg) VALUES ('ok')", vec![]), + ("INSERT INTO other.no_such_table (msg) VALUES ('x')", vec![]), + ]) + .attach(make_spec()) + .execute() + .await + .expect_err("the second statement's nonexistent table should fail the whole transaction"); + assert!( + err.to_string().contains("no such table"), + "expected the statement's own error, got: {err}" + ); + + // Same alias, same (single) write connection, valid statement. + main + .execute( + "INSERT INTO other.logs (msg) VALUES ('after failure')".into(), + vec![], + ) + .attach(make_spec()) + .await + .expect("the alias must be reusable after a failed attached transaction"); + + // Only the post-failure row is present - proving the rollback actually + // happened, not just that the alias survived. + let rows = other + .fetch_all("SELECT msg FROM logs".into(), vec![]) + .await + .expect("logs should be readable"); + assert_eq!(rows.len(), 1, "the rolled-back 'ok' row must not be present"); + assert_eq!( + rows[0]["msg"].as_str(), + Some("after failure"), + "only the row inserted after the failure should remain" + ); +} + +#[tokio::test] +async fn failed_attached_interruptible_statement_still_releases_the_alias() { + let (main, other, _temp) = two_databases().await; + + let make_spec = || { + vec![AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }] + }; + + // `InterruptibleTransaction` deliberately doesn't derive `Debug` (it holds + // a live write guard), so `expect_err` won't compile here - match on the + // `Result` directly instead. + match main + .begin_interruptible_transaction() + .attach(make_spec()) + .execute(vec![( + "INSERT INTO other.no_such_table (msg) VALUES ('x')", + vec![], + )]) + .await + { + Ok(_) => panic!("expected the initial statement's nonexistent table to fail"), + Err(err) => assert!( + err.to_string().contains("no such table"), + "expected the statement's own error, got: {err}" + ), + } + + // Unlike the transaction builder above, this doesn't pin code in + // `InterruptibleTransactionBuilder::execute` itself: the failing initial + // statement fails inside `continue_with`, which returns via `?` before the + // builder ever detaches, so it's `ActiveInterruptibleTransaction`'s own + // `Drop` impl that rolls back and detaches - in a task spawned onto the + // runtime. That task holds the single write permit until its `DETACH` + // finishes, so this reuse attempt genuinely blocks on it for a moment + // (measured 1-2ms) rather than racing it - deterministic, just not + // instant. + main + .execute( + "INSERT INTO other.logs (msg) VALUES ('after failure')".into(), + vec![], + ) + .attach(make_spec()) + .await + .expect("the alias must be reusable after a failed interruptible transaction"); +} + +#[tokio::test] +async fn failed_begin_immediate_still_releases_the_alias() { + let (main, other, temp) = two_databases().await; + + let make_spec = || { + vec![AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }] + }; + + // A raw connection, opened outside `SqliteDatabase`'s registry, is required + // to hog the lock: `SqliteDatabase::connect` returns the same registry-shared + // instance for a given path, so a second `DatabaseWrapper::connect("main.db")` + // would share main's single-connection write pool rather than contend with + // it. This locks "main" itself - the busy database this test is named for - + // not the attached "other". + let mut hog = sqlx::sqlite::SqliteConnectOptions::new() + .filename(temp.path().join("main.db")) + .connect() + .await + .expect("open a raw connection to main.db"); + sqlx::query("BEGIN IMMEDIATE") + .execute(&mut hog) + .await + .expect("lock main via the raw connection"); + sqlx::query("INSERT INTO users (name) VALUES ('hog')") + .execute(&mut hog) + .await + .expect("write on the raw connection"); + + // The pooled writer's own BEGIN IMMEDIATE now contends with the raw + // connection's still-open write transaction on the same file and + // deterministically ends in SQLITE_BUSY - but only after sqlx exhausts its + // default 5-second busy_timeout retrying internally, which + // `SqliteDatabaseConfig` exposes no knob to shorten. That wait, not + // anything wrong with the test, is why this test takes about 5 seconds. + // Pins `detach_if_attached()` in the `if let Err(err) = writer.begin_immediate()` + // arm of `TransactionExecutionBuilder::execute`. + let err = main + .execute_transaction(vec![("INSERT INTO main.users (name) VALUES ('a')", vec![])]) + .attach(make_spec()) + .execute() + .await + .expect_err("BEGIN IMMEDIATE should fail while the raw connection holds the write lock"); + assert!( + err.to_string().contains("database is locked"), + "expected a busy/locked error, got: {err}" + ); + + // Release the hog's lock and reuse the alias. + sqlx::query("ROLLBACK") + .execute(&mut hog) + .await + .expect("release the raw connection's lock"); + drop(hog); + + main + .execute( + "INSERT INTO other.logs (msg) VALUES ('after failure')".into(), + vec![], + ) + .attach(make_spec()) + .await + .expect("the alias must be reusable after a failed BEGIN IMMEDIATE"); +} diff --git a/crates/sqlx-sqlite-toolkit/tests/attached_observation_tests.rs b/crates/sqlx-sqlite-toolkit/tests/attached_observation_tests.rs new file mode 100644 index 0000000..ce8e196 --- /dev/null +++ b/crates/sqlx-sqlite-toolkit/tests/attached_observation_tests.rs @@ -0,0 +1,311 @@ +//! End-to-end coverage of observed writes into an attached database, through +//! the public toolkit API rather than one layer down at +//! `ObservableSqliteDatabase::acquire_writer_with_attached`. +//! +//! Two things are pinned here that nothing else pins: +//! +//! 1. **Routing.** A change made under an `ATTACH` alias reaches the *owning* +//! database's subscribers carrying that alias in `TableChange::schema`, +//! while the same transaction's `main` write reaches this database's own +//! subscribers as `"main"`. +//! 2. **Alias release.** The attached alias is detached when the transaction +//! finishes (see `builders::detach_after`'s doc for what stranding it costs). +//! The second transaction below is the assertion for that; without it, a +//! `detach_all()` call silently becoming a no-op passes the whole suite. + +#![cfg(feature = "observer")] + +use std::sync::Arc; +use std::time::Duration; + +use sqlx_sqlite_conn_mgr::{AttachedMode, AttachedSpec}; +use sqlx_sqlite_observer::ObserverConfig; +use sqlx_sqlite_toolkit::DatabaseWrapper; +use tempfile::TempDir; +use tokio::time::timeout; + +/// How long to wait for a published change before calling it lost. Generous +/// because the publish happens on the commit hook, not the caller's task. +const RECV_TIMEOUT: Duration = Duration::from_millis(500); + +#[tokio::test] +async fn observed_attached_transaction_routes_by_owner_and_releases_the_alias() { + let temp = TempDir::new().unwrap(); + + let main = DatabaseWrapper::connect(&temp.path().join("main.db"), None) + .await + .expect("connect main"); + let other = DatabaseWrapper::connect(&temp.path().join("other.db"), None) + .await + .expect("connect other"); + + main + .execute( + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)".into(), + vec![], + ) + .await + .expect("create users"); + other + .execute( + "CREATE TABLE logs (id INTEGER PRIMARY KEY, msg TEXT)".into(), + vec![], + ) + .await + .expect("create logs"); + + // Both databases must observe: a ReadWrite attachment only contributes a + // broker to the hook map if it has observation enabled of its own, so + // without this the write to `other.logs` lands unobserved. + main.enable_observation(ObserverConfig::new().with_tables(["users"])); + other.enable_observation(ObserverConfig::new().with_tables(["logs"])); + + let mut rx_main = main.observable().unwrap().subscribe(["users"]); + let mut rx_other = other.observable().unwrap().subscribe(["logs"]); + + // ReadWrite, not ReadOnly: `acquire_writer_with_attached` deliberately leaves a + // ReadOnly attachment's broker out of the map, so a ReadOnly spec here would + // make the `other` write unobserved and fail against correct code. + let make_spec = || AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }; + + main + .execute_transaction(vec![ + ("INSERT INTO users (name) VALUES ('Zed')", vec![]), + ("INSERT INTO other.logs (msg) VALUES ('hello')", vec![]), + ]) + .attach(vec![make_spec()]) + .execute() + .await + .expect("attached observed transaction should commit"); + + let main_change = timeout(RECV_TIMEOUT, rx_main.recv()) + .await + .expect("main's own change should be published") + .expect("main subscriber should still be live"); + assert_eq!(main_change.schema, "main"); + assert_eq!(main_change.table, "users"); + + let other_change = timeout(RECV_TIMEOUT, rx_other.recv()) + .await + .expect("the attached database's change should reach its own subscribers") + .expect("other subscriber should still be live"); + assert_eq!(other_change.schema, "other"); + assert_eq!(other_change.table, "logs"); + + // Reuses the same alias on the same (single) write connection. Fails with + // "database other is already in use" if the first transaction didn't + // detach. + main + .execute_transaction(vec![("INSERT INTO users (name) VALUES ('Yan')", vec![])]) + .attach(vec![make_spec()]) + .execute() + .await + .expect("second attached transaction must be able to reuse the alias"); +} + +/// Pins the defect this file's suite otherwise missed: every test above enables +/// observation on *both* main and the attached database, so nothing exercised +/// the case where main's own observation is off but an attached `ReadWrite` +/// database's own observation is on. Before the fix, +/// `DatabaseWrapper::acquire_writer_with_attached` only took the observable +/// path when *main's* broker existed, so an unobserved main sent this straight +/// to `AttachedWriterGuard::Regular` - no hooks registered at all - and +/// `other`'s own subscribers silently got nothing, even though the README +/// promises routing based only on the *attached* database's own state. +/// +/// Verified failing against the pre-fix code (a `TempDir`-based reproduction of +/// this exact scenario against the code at commit `8ea4d00`, run in an isolated +/// worktree): `rx_other.recv()` timed out with `Elapsed(())`. +#[tokio::test] +async fn main_unobserved_attached_observed_still_notifies_attached_subscriber() { + let temp = TempDir::new().unwrap(); + + let main = DatabaseWrapper::connect(&temp.path().join("main.db"), None) + .await + .expect("connect main"); + let other = DatabaseWrapper::connect(&temp.path().join("other.db"), None) + .await + .expect("connect other"); + + main + .execute( + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)".into(), + vec![], + ) + .await + .expect("create users"); + other + .execute( + "CREATE TABLE logs (id INTEGER PRIMARY KEY, msg TEXT)".into(), + vec![], + ) + .await + .expect("create logs"); + + // main is deliberately left unobserved - this is the one variable this test + // changes relative to `observed_attached_transaction_routes_by_owner_and_releases_the_alias` + // above. + other.enable_observation(ObserverConfig::new().with_tables(["logs"])); + + let mut rx_other = other.observable().unwrap().subscribe(["logs"]); + + let spec = AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }; + + main + .execute_transaction(vec![( + "INSERT INTO other.logs (msg) VALUES ('hello')", + vec![], + )]) + .attach(vec![spec]) + .execute() + .await + .expect("an attached transaction through an unobserved main should still commit"); + + let change = timeout(RECV_TIMEOUT, rx_other.recv()) + .await + .expect("other's subscriber should receive the change even though main is unobserved") + .expect("other subscriber should still be live"); + assert_eq!(change.schema, "other"); + assert_eq!(change.table, "logs"); +} + +/// The reverse-direction guard the test above doesn't cover, and which is what +/// catches an over-broad fix to the defect above: it would be a mistake to fix +/// "main unobserved, attached observed" by routing every write to whichever +/// broker exists, since that could misattribute an attached write to main's +/// broker whenever the table names happen to collide. Here, main's *own* +/// observed-table set deliberately includes "logs" - the exact name of the +/// table being written through the attached alias - and `other` is left +/// unobserved. A correct fix still drops the change (per +/// `acquire_writer_with_attached`'s documented "nowhere for its changes to go" +/// rule for an unobserved `ReadWrite` attachment); an over-broad fix that +/// matched by table name alone would incorrectly deliver it to main's "logs" +/// subscriber. +#[tokio::test] +async fn main_observed_with_colliding_table_name_does_not_receive_unobserved_attached_write() { + let temp = TempDir::new().unwrap(); + + let main = DatabaseWrapper::connect(&temp.path().join("main.db"), None) + .await + .expect("connect main"); + let other = DatabaseWrapper::connect(&temp.path().join("other.db"), None) + .await + .expect("connect other"); + + // Both databases happen to have a "logs" table - main's own is unrelated to + // the attached one, but shares the name on purpose. + main + .execute( + "CREATE TABLE logs (id INTEGER PRIMARY KEY, msg TEXT)".into(), + vec![], + ) + .await + .expect("create main.logs"); + other + .execute( + "CREATE TABLE logs (id INTEGER PRIMARY KEY, msg TEXT)".into(), + vec![], + ) + .await + .expect("create other.logs"); + + // main observes "logs" - the same name as the table written through the + // attached alias below. other is left unobserved entirely. + main.enable_observation(ObserverConfig::new().with_tables(["logs"])); + let mut rx_main = main.observable().unwrap().subscribe(["logs"]); + + let spec = AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }; + + main + .execute_transaction(vec![( + "INSERT INTO other.logs (msg) VALUES ('should not surface anywhere')", + vec![], + )]) + .attach(vec![spec]) + .execute() + .await + .expect("an attached transaction into an unobserved database should still commit"); + + let result = timeout(Duration::from_millis(200), rx_main.recv()).await; + assert!( + result.is_err(), + "a write into an unobserved attached database's 'logs' table must not be \ + misattributed to main's own 'logs' subscriber just because the names \ + collide - it must be dropped entirely, not misrouted" + ); +} + +/// The alias-release guard from the first test in this file, replayed against +/// this file's new "main unobserved" scenario: a regression that stops +/// detaching after a main-unobserved, attached-observed transaction should +/// fail the *second* transaction's `ATTACH` with "database other is already in +/// use", not silently pass because nothing ever reused the alias. +#[tokio::test] +async fn main_unobserved_attached_observed_alias_is_still_released() { + let temp = TempDir::new().unwrap(); + + let main = DatabaseWrapper::connect(&temp.path().join("main.db"), None) + .await + .expect("connect main"); + let other = DatabaseWrapper::connect(&temp.path().join("other.db"), None) + .await + .expect("connect other"); + + main + .execute( + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)".into(), + vec![], + ) + .await + .expect("create users"); + other + .execute( + "CREATE TABLE logs (id INTEGER PRIMARY KEY, msg TEXT)".into(), + vec![], + ) + .await + .expect("create logs"); + + other.enable_observation(ObserverConfig::new().with_tables(["logs"])); + + let make_spec = || AttachedSpec { + database: Arc::clone(other.inner()), + schema_name: "other".to_string(), + mode: AttachedMode::ReadWrite, + }; + + main + .execute_transaction(vec![( + "INSERT INTO other.logs (msg) VALUES ('first')", + vec![], + )]) + .attach(vec![make_spec()]) + .execute() + .await + .expect("first attached transaction should commit"); + + // Reuses the same alias on the same (single) write connection. Fails with + // "database other is already in use" if the first transaction's detach + // regressed on this (main-unobserved) path. + main + .execute_transaction(vec![( + "INSERT INTO other.logs (msg) VALUES ('second')", + vec![], + )]) + .attach(vec![make_spec()]) + .execute() + .await + .expect("second attached transaction must be able to reuse the alias"); +} diff --git a/crates/sqlx-sqlite-toolkit/tests/observation_tests.rs b/crates/sqlx-sqlite-toolkit/tests/observation_tests.rs index b264b59..1bf9743 100644 --- a/crates/sqlx-sqlite-toolkit/tests/observation_tests.rs +++ b/crates/sqlx-sqlite-toolkit/tests/observation_tests.rs @@ -1,11 +1,22 @@ -//! Regression tests for `DatabaseWrapper::enable_observation()`. +//! Regression tests for `DatabaseWrapper`'s observation API. //! -//! These specifically cover issue #54: re-calling `enable_observation()` (surfaced -//! to Tauri callers as `observe()`) must not destroy the existing broadcast broker, -//! or every subscriber created before the re-call silently stops receiving events. +//! Covers issue #54: re-calling `enable_observation()` (surfaced to Tauri callers +//! as `observe()`) must not destroy the existing broadcast broker, or every +//! subscriber created before the re-call silently stops receiving events. +//! +//! Also covers issue #53: observation is a property of the underlying database, +//! not of any one `DatabaseWrapper` value. A clone of a wrapper, and a completely +//! independent `DatabaseWrapper::connect()` call to the same path, must observe +//! through the exact same broker as the handle that enabled it - and `:memory:` +//! databases, which never share a `SqliteDatabase` with anything, must stay +//! independent of each other precisely because of that. Also covers #53's +//! abandoned-transaction buffer leak: a writer dropped mid-transaction without +//! an explicit commit or rollback must not have its buffered changes resurface +//! on the next transaction's commit. #![cfg(feature = "observer")] +use std::sync::Arc; use std::time::Duration; use sqlx_sqlite_observer::ObserverConfig; @@ -40,7 +51,7 @@ async fn create_test_db() -> (DatabaseWrapper, TempDir) { #[tokio::test] async fn test_first_enable_observation_applies_requested_config() { - let (mut wrapper, _temp) = create_test_db().await; + let (wrapper, _temp) = create_test_db().await; wrapper.enable_observation( ObserverConfig::new() @@ -49,10 +60,19 @@ async fn test_first_enable_observation_applies_requested_config() { .with_capture_values(false), ); - let broker = wrapper.observable().unwrap().broker(); + let observable = wrapper.observable().unwrap(); + let broker = observable.broker(); assert_eq!(broker.channel_capacity(), 8); assert!(!broker.capture_values()); assert!(wrapper.is_observing()); + + // The requested tables are part of "applies requested config" too, and this is + // the only test that pins them on the *create* path - every other one either + // subscribes (which registers the table itself) or goes through the merge path. + // Sorted because `observed_tables()` collects from a `HashSet`. + let mut observed = observable.observed_tables(); + observed.sort(); + assert_eq!(observed, vec!["users".to_string()]); } /// This is the exact regression scenario from issue #54: a subscriber created @@ -61,7 +81,7 @@ async fn test_first_enable_observation_applies_requested_config() { /// seeing its `broadcast::Receiver` closed because the broker was replaced. #[tokio::test] async fn test_reenable_observation_preserves_existing_subscriber_across_new_tables() { - let (mut wrapper, _temp) = create_test_db().await; + let (wrapper, _temp) = create_test_db().await; wrapper.enable_observation(ObserverConfig::new().with_tables(["users"])); @@ -100,7 +120,7 @@ async fn test_reenable_observation_preserves_existing_subscriber_across_new_tabl /// `tauri-plugin-sqlite` crate, not here. #[tokio::test] async fn test_reenable_observation_ignores_conflicting_config() { - let (mut wrapper, _temp) = create_test_db().await; + let (wrapper, _temp) = create_test_db().await; wrapper.enable_observation( ObserverConfig::new() @@ -116,7 +136,8 @@ async fn test_reenable_observation_ignores_conflicting_config() { .with_capture_values(true), ); - let broker = wrapper.observable().unwrap().broker(); + let observable = wrapper.observable().unwrap(); + let broker = observable.broker(); assert_eq!( broker.channel_capacity(), 4, @@ -145,3 +166,435 @@ async fn test_reenable_observation_ignores_conflicting_config() { "capture_values=true from the second call should have been ignored" ); } + +/// The issue's exact scenario: a clone of a wrapper shares the same underlying +/// `SqliteDatabase` as the original, so enabling and subscribing through the +/// clone must see writes made through the original. +#[tokio::test] +async fn observation_is_shared_across_clones() { + let (original, _temp) = create_test_db().await; + let clone = original.clone(); + + clone.enable_observation(ObserverConfig::new().with_tables(["users"])); + let mut rx = clone.observable().unwrap().subscribe(["users"]); + + original + .execute("INSERT INTO users (name) VALUES ('Alice')".into(), vec![]) + .await + .expect("insert into users via original"); + + let change = timeout(Duration::from_millis(200), rx.recv()) + .await + .expect("should not time out") + .expect("should receive a change"); + assert_eq!(change.table, "users"); +} + +/// What per-clone sharing alone would not fix: two entirely separate +/// `DatabaseWrapper::connect()` calls to the same path resolve to the same +/// underlying `SqliteDatabase` (the path registry in `sqlx-sqlite-conn-mgr` +/// guarantees this), so they must share observation state too, even though +/// neither is a clone of the other. +#[tokio::test] +async fn observation_is_shared_across_independent_connects() { + let temp_dir = TempDir::new().expect("Failed to create temp directory"); + let db_path = temp_dir.path().join("test.db"); + + let a = DatabaseWrapper::connect(&db_path, None) + .await + .expect("connect handle A"); + a.execute( + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)".into(), + vec![], + ) + .await + .expect("create users table"); + + let b = DatabaseWrapper::connect(&db_path, None) + .await + .expect("connect handle B"); + + a.enable_observation(ObserverConfig::new().with_tables(["users"])); + let mut rx = a.observable().unwrap().subscribe(["users"]); + + b.execute("INSERT INTO users (name) VALUES ('Bob')".into(), vec![]) + .await + .expect("insert into users via handle B"); + + let change = timeout(Duration::from_millis(200), rx.recv()) + .await + .expect("should not time out") + .expect("should receive a change"); + assert_eq!(change.table, "users"); +} + +/// `:memory:` databases are deliberately excluded from the path registry (they +/// all share the literal path `":memory:"`, so registry sharing would be wrong), +/// which means two separate connects never resolve to the same `SqliteDatabase` +/// and therefore never share an observer slot either. Enabling observation on +/// one must not make the other report as observing. +#[tokio::test] +async fn memory_databases_do_not_share_observation() { + let db1 = DatabaseWrapper::connect(std::path::Path::new(":memory:"), None) + .await + .expect("connect first :memory: database"); + let db2 = DatabaseWrapper::connect(std::path::Path::new(":memory:"), None) + .await + .expect("connect second :memory: database"); + + db1.enable_observation(ObserverConfig::new().with_tables(["users"])); + + assert!(db1.is_observing()); + assert!( + !db2.is_observing(), + ":memory: databases must not share observation state" + ); +} + +/// The inverse of `observation_is_shared_across_independent_connects`: +/// `disable_observation()` clears the shared slot, so calling it from one +/// independently-connected handle must stop observation for every handle to the +/// same database - including the one that originally enabled it. This is the +/// widened blast radius documented on `disable_observation`'s rustdoc. +#[tokio::test] +async fn disable_observation_affects_all_handles() { + let temp_dir = TempDir::new().expect("Failed to create temp directory"); + let db_path = temp_dir.path().join("test.db"); + + let a = DatabaseWrapper::connect(&db_path, None) + .await + .expect("connect handle A"); + a.execute( + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)".into(), + vec![], + ) + .await + .expect("create users table"); + + let b = DatabaseWrapper::connect(&db_path, None) + .await + .expect("connect handle B"); + + a.enable_observation(ObserverConfig::new().with_tables(["users"])); + let mut rx = a.observable().unwrap().subscribe(["users"]); + + // Disable via the independently-connected handle, not the one that enabled it. + b.disable_observation(); + + assert!(!a.is_observing(), "handle A must see the disable too"); + assert!(!b.is_observing()); + + a.execute("INSERT INTO users (name) VALUES ('Carol')".into(), vec![]) + .await + .expect("insert into users after disable_observation()"); + + // Disabling drops the broker (no other Arc keeps it alive once the slot is + // cleared), which closes the broadcast channel out from under `rx` - so the + // no-notification outcome can surface either as a timeout or as an + // immediate `RecvError` on the now-closed channel. Either is a pass; only an + // actual change arriving is a failure. + match timeout(Duration::from_millis(100), rx.recv()).await { + Err(_) => {} // timed out waiting - no notification arrived + Ok(Err(_)) => {} // channel closed/lagged - no notification arrived + Ok(Ok(change)) => panic!( + "no notification should arrive after disable_observation() from any handle, got {change:?}" + ), + } +} + +/// The intentional bypass survives the refactor: `acquire_regular_writer()` +/// must still skip observation entirely, even now that observation is +/// database-wide rather than per-handle. +#[tokio::test] +async fn regular_writer_bypasses_observation() { + let (wrapper, _temp) = create_test_db().await; + + wrapper.enable_observation(ObserverConfig::new().with_tables(["users"])); + let mut rx = wrapper.observable().unwrap().subscribe(["users"]); + + { + let mut writer = wrapper + .acquire_regular_writer() + .await + .expect("acquire regular writer"); + sqlx::query("INSERT INTO users (name) VALUES ('Dave')") + .execute(&mut *writer) + .await + .expect("insert via regular writer"); + } + + // Asserting behavior, not log output: this workspace has no log-capture + // harness, so the only observable evidence of the bypass is the absence of a + // notification for a write that unquestionably happened. + let outcome = timeout(Duration::from_millis(100), rx.recv()).await; + assert!( + outcome.is_err(), + "acquire_regular_writer() must not publish any change notification" + ); + + // Confirm the write actually landed - the point is that it bypassed + // observation, not that it silently failed. + let rows = wrapper + .fetch_all("SELECT name FROM users WHERE name = 'Dave'".into(), vec![]) + .execute() + .await + .expect("select after regular writer insert"); + assert_eq!(rows.len(), 1); +} + +/// The race this fix closes: many independent handles to the same database +/// calling `enable_observation()` concurrently must converge on a single +/// broker, not each build their own and let the last one silently win - which +/// would orphan whichever subscriber was registered against a broker that got +/// replaced. `ObserverSlot::get_or_init` makes the check-and-create atomic +/// under the slot's own lock specifically to rule this out. +/// +/// Requires a multi-thread runtime: `enable_observation()` has no `.await` in +/// it, so on the default current-thread runtime, cooperative scheduling can +/// never preempt one task's call mid-body to let another one interleave - +/// every task would run its entire `enable_observation()` to completion +/// before the next one starts, and this test would pass even against a naive +/// (non-atomic) get-then-set implementation. See +/// `sqlx_sqlite_conn_mgr::observer_slot`'s tests for a deterministic version +/// of this same race, using real OS threads and a `Barrier`. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn concurrent_enable_observation_converges_on_one_broker() { + let temp_dir = TempDir::new().expect("Failed to create temp directory"); + let db_path = temp_dir.path().join("test.db"); + + let seed = DatabaseWrapper::connect(&db_path, None) + .await + .expect("connect seed handle"); + seed + .execute( + "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)".into(), + vec![], + ) + .await + .expect("create users table"); + + // Each task connects independently (not a clone) and races to be the first + // to enable observation. A naive get()-then-set() implementation would let + // several of these each observe an empty slot and build their own broker. + let mut tasks = Vec::new(); + for _ in 0..16 { + let db_path = db_path.clone(); + tasks.push(tokio::spawn(async move { + let wrapper = DatabaseWrapper::connect(&db_path, None) + .await + .expect("connect racing handle"); + wrapper.enable_observation(ObserverConfig::new().with_tables(["users"])); + wrapper + .observable() + .expect("observation should be enabled after enable_observation()") + .broker() + .clone() + })); + } + + let mut brokers = Vec::new(); + for task in tasks { + brokers.push(task.await.expect("racing task should not panic")); + } + + let first_ptr = std::sync::Arc::as_ptr(&brokers[0]); + for broker in &brokers[1..] { + assert!( + std::ptr::eq(std::sync::Arc::as_ptr(broker), first_ptr), + "every concurrent enable_observation() call must converge on the same broker" + ); + } + + // A subscriber registered against the very first broker instance handed back + // must still receive notifications once the race settles - if some later + // caller had silently replaced the broker instead of reusing it, this + // receiver would be listening to a broker no writer publishes through + // anymore. + let mut rx = brokers[0].subscribe(); + seed + .execute("INSERT INTO users (name) VALUES ('Eve')".into(), vec![]) + .await + .expect("insert into users after the race settles"); + + let change = timeout(Duration::from_millis(200), rx.recv()) + .await + .expect("should not time out") + .expect("should receive a change"); + assert_eq!(change.table, "users"); +} + +/// A writer dropped mid-transaction - BEGIN plus a write, then dropped with no +/// COMMIT or ROLLBACK ever sent - must not have its buffered change resurface +/// on the *next* transaction's commit. Without `ObservableWriteGuard::drop` +/// discarding the buffer, the abandoned INSERT below would still be sitting in +/// the broker's buffer when the real transaction commits, and `on_commit`'s +/// `mem::take` would publish it right alongside (or instead of) the real change. +/// +/// Uses `acquire_writer()` directly (not `execute_transaction()` or +/// `begin_interruptible_transaction()`) because both of those already have +/// their own higher-level auto-rollback-on-drop safety nets that issue a real +/// `ROLLBACK` before the writer itself drops - which would exercise that +/// existing mechanism, not the gap this test is for: a writer dropped with +/// hooks still registered and no commit or rollback statement ever sent. +#[tokio::test] +async fn abandoned_transaction_does_not_leak_into_next_commit() { + let (wrapper, _temp) = create_test_db().await; + + wrapper.enable_observation(ObserverConfig::new().with_tables(["users"])); + let mut rx = wrapper.observable().unwrap().subscribe(["users"]); + + { + let mut writer = wrapper.acquire_writer().await.expect("acquire writer"); + sqlx::query("BEGIN") + .execute(&mut *writer) + .await + .expect("begin"); + sqlx::query("INSERT INTO users (name) VALUES ('Abandoned')") + .execute(&mut *writer) + .await + .expect("insert (never committed)"); + // Dropped here with no COMMIT or ROLLBACK ever sent. + } + + wrapper + .execute("INSERT INTO users (name) VALUES ('Real')".into(), vec![]) + .await + .expect("insert into users"); + + let change = timeout(Duration::from_millis(200), rx.recv()) + .await + .expect("should not time out") + .expect("should receive a change"); + + assert_eq!(change.table, "users"); + let new_values = change.new_values.expect("capture_values defaults to true"); + assert!( + new_values + .iter() + .filter_map(|v| v.as_text()) + .any(|s| s == "Real"), + "expected the committed row's own values, got {new_values:?} - a leaked \ + abandoned-transaction event would carry 'Abandoned' instead" + ); + + // No second notification - specifically not the abandoned transaction's + // insert straggling in as a phantom change alongside the real one. + let no_more = timeout(Duration::from_millis(100), rx.recv()).await; + assert!( + no_more.is_err(), + "the abandoned transaction's buffered insert must not surface as a \ + second notification" + ); +} + +/// Regression test for a strong reference cycle the initial database-wide +/// observation implementation introduced: the observer slot used to hold +/// `Arc`, whose own `db` field is an `Arc` back to the +/// very `SqliteDatabase` that owns the slot. That kept the database alive forever +/// once observed, even after every external handle was dropped without calling +/// `close()` - defeating the registry's `Weak` reference and the free-on-drop +/// contract documented on `SqliteDatabase::close`. +/// +/// The conn-mgr path registry isn't reachable from this crate to assert against +/// its `Weak` directly, so this asserts the same thing one layer down: +/// downgrading the wrapper's own `Arc` and confirming it does not +/// upgrade once the last strong reference is dropped. While the slot still stored +/// the whole observable rather than just its broker, this `upgrade()` succeeded. +#[tokio::test] +async fn dropping_wrapper_without_close_frees_database_even_when_observed() { + let (wrapper, _temp) = create_test_db().await; + + wrapper.enable_observation(ObserverConfig::new().with_tables(["users"])); + assert!(wrapper.is_observing()); + + let weak = Arc::downgrade(wrapper.inner_for_testing()); + + // No explicit close() - the whole point is what happens on a bare drop. + drop(wrapper); + + assert!( + weak.upgrade().is_none(), + "the SqliteDatabase must be freed once the last external handle drops, \ + even with observation enabled; a successful upgrade here means the \ + observer slot still holds a strong reference back to this database" + ); +} + +/// Pins the known, deliberately deferred broker-identity limitation: a writer +/// binds its hooks to whichever broker was in the slot at acquisition and keeps +/// them for its whole lifetime, so a `disable_observation()` + +/// `enable_observation()` cycle during its open transaction leaves it publishing +/// to the pre-cycle broker. Subscribers created before the cycle still receive +/// the commit; one created after it does not. See +/// `sqlx_sqlite_observer::ObservableSqliteDatabase::acquire_writer`'s doc for the +/// mechanics and the deferred fix. +#[tokio::test] +#[ignore = "pins a known, deliberately deferred limitation - see \ + sqlx_sqlite_observer::ObservableSqliteDatabase::acquire_writer's doc. This test \ + asserts today's buggy behavior (after_result.is_err()), so un-ignoring alone would \ + turn it red once the fix lands: invert the assertion to expect a successful recv() \ + first, then remove #[ignore]. Follow-up issue not yet filed"] +async fn disable_enable_cycle_during_open_writer_strands_new_subscribers_on_new_broker() { + let (wrapper, _temp) = create_test_db().await; + + wrapper.enable_observation(ObserverConfig::new().with_tables(["users"])); + + // Created *before* the cycle - expected to still receive the commit below, + // since its Arc keeps the pre-cycle broker alive regardless of what the slot + // points to afterward. + let mut rx_before = wrapper.observable().unwrap().subscribe(["users"]); + + // Stands in for "some other caller's interruptible transaction is still + // open": acquire a writer and begin, but don't commit yet. Its hooks bind to + // whatever broker is in the slot right now - the pre-cycle one. + let mut writer = wrapper.acquire_writer().await.expect("acquire writer"); + sqlx::query("BEGIN") + .execute(&mut *writer) + .await + .expect("begin"); + sqlx::query("INSERT INTO users (name) VALUES ('MidCycle')") + .execute(&mut *writer) + .await + .expect("insert while writer is open"); + + // The disable/enable cycle, while `writer`'s transaction is still open - the + // plugin-level equivalent of `unobserve()` followed by `observe()`. + wrapper.disable_observation(); + wrapper.enable_observation(ObserverConfig::new().with_tables(["users"])); + assert!( + wrapper.is_observing(), + "is_observing() reports success throughout - that's exactly the problem: \ + nothing observable here signals the broker-identity mismatch this test \ + exists to pin" + ); + + // Created *after* the cycle - subscribes against the new broker the slot now + // holds, which `writer`'s already-registered hooks are not bound to. + let mut rx_after = wrapper.observable().unwrap().subscribe(["users"]); + + sqlx::query("COMMIT") + .execute(&mut *writer) + .await + .expect("commit"); + drop(writer); + + let change = timeout(Duration::from_millis(200), rx_before.recv()) + .await + .expect("should not time out") + .expect("pre-cycle subscriber should still receive the commit"); + assert_eq!(change.table, "users"); + + // The bug: a subscriber created after the cycle never sees a commit from a + // writer whose hooks were already bound to the old broker before the cycle + // ran. Once the deferred fix lands, this should become a successful + // `rx_after.recv()` carrying the "MidCycle" row instead of a timeout - at + // which point remove the #[ignore] above. + let after_result = timeout(Duration::from_millis(200), rx_after.recv()).await; + assert!( + after_result.is_err(), + "a subscriber created after the disable/enable cycle unexpectedly received \ + the in-flight writer's commit - if this is failing, the deferred fix may \ + already be in place, in which case update this test and remove the \ + #[ignore] above rather than leaving it pinned to the old (buggy) behavior" + ); +} diff --git a/guest-js/index.test.ts b/guest-js/index.test.ts index bef3c47..c3c4e3b 100644 --- a/guest-js/index.test.ts +++ b/guest-js/index.test.ts @@ -785,6 +785,7 @@ describe('Observer types', () => { it('TableChange structure', () => { const change: TableChange = { + schema: 'main', table: 'users', operation: 'insert', rowid: 1, @@ -805,6 +806,7 @@ describe('Observer types', () => { it('TableChange without rowid', () => { const change: TableChange = { + schema: 'main', table: 'kv_store', operation: 'update', primaryKey: [ { type: 'text', value: 'my-key' } ], @@ -818,6 +820,11 @@ describe('Observer types', () => { const event: TableChangeEvent = { event: 'change', data: { + // Not 'main' here on purpose - schema is whatever alias the + // write happened under, not always the primary database, so a + // fixture that only ever uses 'main' would document the field as + // if it were a constant. + schema: 'archive', table: 'users', operation: 'delete', primaryKey: [ { type: 'integer', value: 5 } ], diff --git a/guest-js/index.ts b/guest-js/index.ts index 733d204..3c4f5ca 100644 --- a/guest-js/index.ts +++ b/guest-js/index.ts @@ -34,6 +34,11 @@ export interface AttachedDatabaseSpec { /** * Schema name to use for the attached database in queries * (e.g., "orders" to query as "SELECT * FROM orders.table_name") + * + * Must be a plain identifier - letters, digits and underscores only, not + * starting with a digit - and at most 64 characters. Anything else fails with + * `CONNECTION_ERROR`. The name is also reported as `schema` on change + * notifications for this database. */ schemaName: string; @@ -361,6 +366,16 @@ export type ColumnValue = */ export interface TableChange { + /** + * The schema this change occurred under: `"main"` for the primary + * database, or an attached database's alias otherwise. + * + * This is provenance metadata, not a stable identifier - the alias is + * chosen by whoever attached the database and isn't guaranteed to be + * consistent across calls. Don't use it as a lookup key. + */ + schema: string; + /** Name of the table that was changed */ table: string; diff --git a/src/commands.rs b/src/commands.rs index 817d439..d1be503 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -102,6 +102,19 @@ use crate::{ }; use crate::{close_all_loaded_databases, close_database, connect_to_database}; +/// Upper bound on the number of table names accepted by a *single* `observe()` +/// or `subscribe()` call. Both commands ultimately call +/// `broker.observe_tables()` on the database's shared observation broker (one +/// instance per file, shared across every window's handle to it - see #53), so +/// an unbounded request from either command grows the same observed set and pays +/// the same unresolvable-name cost documented on `observe()` below and in the +/// Resource Limits section of the README. +/// +/// This bounds a single call's request only, not the accumulated set of +/// tables observed on a database overall across many calls - that remains +/// unbounded for now and is tracked as issue #56. +const MAX_OBSERVED_TABLES: usize = 100; + /// Token representing an active interruptible transaction #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -595,13 +608,14 @@ pub async fn begin_interruptible_transaction( // Generate unique transaction ID let transaction_id = Uuid::new_v4().to_string(); - // Acquire appropriate writer based on whether databases are attached + // Acquire appropriate writer based on whether databases are attached. + // `acquire_writer_with_attached` routes through the observer when it's + // enabled, so writes into attached databases are tracked here too - not + // just direct writes to this database. let mut writer = if let Some(specs) = attached { let resolved_specs = resolve_attached_specs(specs, &instances)?; - let guard = - sqlx_sqlite_conn_mgr::acquire_writer_with_attached(wrapper.inner(), resolved_specs) - .await?; - TransactionWriter::Attached(guard) + let guard = wrapper.acquire_writer_with_attached(resolved_specs).await?; + TransactionWriter::from(guard) } else { TransactionWriter::from(wrapper.acquire_writer().await?) }; @@ -754,7 +768,6 @@ pub async fn observe( tables: Vec, config: Option, ) -> Result<()> { - const MAX_OBSERVED_TABLES: usize = 100; const MAX_CHANNEL_CAPACITY: usize = 10_000; if tables.is_empty() || tables.len() > MAX_OBSERVED_TABLES { @@ -876,6 +889,15 @@ pub async fn observe( /// labels persist across a reload and registrations aren't cleared on one, so /// this proves "this webview label called `observe()` at some point", not /// "this specific page load did". +/// +/// `tables` is optional filtering, not a fresh observation request, but it is +/// still forwarded to the shared broker's `observe_tables()` (see `observe()` +/// above), so it is bounded by the same `MAX_OBSERVED_TABLES` for the same +/// reason: an unresolvable name added here costs a schema round trip on every +/// writer's `acquire_writer()` for this database, indefinitely, just as it +/// would if added via `observe()`. As with `observe()`, this bounds a single +/// `subscribe()` call's request only, not the accumulated set of tables +/// observed on a database overall - see issue #56. #[tauri::command] pub async fn subscribe( db_instances: State<'_, DbInstances>, @@ -893,6 +915,17 @@ pub async fn subscribe( return Err(Error::TooManySubscriptions(MAX_SUBSCRIPTIONS_PER_DATABASE)); } + // Unlike observe(), an empty `tables` is valid here - it means "no filter, + // receive every change already being observed" (see + // `ObservableSqliteDatabase::subscribe_stream`), so only the upper bound + // applies. + if tables.len() > MAX_OBSERVED_TABLES { + return Err(Error::InvalidConfig(format!( + "tables count must be at most {MAX_OBSERVED_TABLES}, got {}", + tables.len() + ))); + } + let instances = db_instances.inner.read().await; let wrapper = instances @@ -1003,6 +1036,13 @@ pub async fn unsubscribe( /// Calling this from a window that never called `observe()` for `db_key` is a /// no-op (beyond validating that `db_key` itself is loaded) - it does not tear /// down observation that other windows are legitimately still using. +/// +/// **The teardown is database-wide, and the reference count only covers +/// webviews.** Since observation became a property of the database rather than of +/// a handle (#53), the `disable_observation()` this performs on the last release +/// also silences any Rust consumer observing the same file directly - such a +/// caller registers nothing here. See `DatabaseWrapper::disable_observation`'s +/// doc, and the README's Change Notifications caveats. #[tauri::command] pub async fn unobserve( db_instances: State<'_, DbInstances>, diff --git a/src/lib.rs b/src/lib.rs index 5529624..59a04cb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2390,6 +2390,62 @@ mod tests { }); } + /// `subscribe()` forwards `tables` into the same shared broker's + /// `observe_tables()` that `observe()` does (see `MAX_OBSERVED_TABLES`'s doc + /// comment in `src/commands.rs`), so it must be bounded the same way - + /// otherwise a single `subscribe()` call could grow the observed set past + /// the limit `observe()` enforces. Unlike `observe()`, an empty `tables` is + /// valid on `subscribe()` (it means "no filter"), so only the upper bound + /// is exercised here. + #[test] + fn test_subscribe_rejects_too_many_tables() { + let temp_dir = tempfile::tempdir().unwrap(); + let db_path = validate::validate_database_path(temp_dir.path().join("main.db")).unwrap(); + let key = "MAIN".to_string(); + + tauri::async_runtime::block_on(async { + let (app, _) = + tokio::task::spawn_blocking(move || init_app_with_registered_db_at_path(&key, db_path)) + .await + .expect("plugin init task should succeed"); + + load_and_create_test_table(&app, "MAIN").await; + + let webview = tauri::WebviewWindowBuilder::new(&app, "window-a", Default::default()) + .build() + .expect("webview window should build"); + + commands::observe( + app.state::(), + app.state::(), + webview.as_ref().clone(), + "MAIN".to_string(), + vec!["test".to_string()], + None, + ) + .await + .expect("observe should succeed"); + + // One past MAX_OBSERVED_TABLES (100). + let too_many_tables: Vec = (0..101).map(|i| format!("table_{i}")).collect(); + let channel = tauri::ipc::Channel::new(|_body| Ok(())); + + let err = commands::subscribe( + app.state::(), + app.state::(), + app.state::(), + webview.as_ref().clone(), + "MAIN".to_string(), + too_many_tables, + channel, + ) + .await + .expect_err("subscribe should reject a request over MAX_OBSERVED_TABLES"); + + assert!(matches!(err, Error::InvalidConfig(_))); + }); + } + /// Refcount teardown boundary: the broker stays live while at least one /// window is registered as an observer, and is only torn down once the last /// registered window releases via `unobserve()`. A non-final `unobserve()` diff --git a/src/subscriptions.rs b/src/subscriptions.rs index ec3b100..d09a6fc 100644 --- a/src/subscriptions.rs +++ b/src/subscriptions.rs @@ -103,6 +103,13 @@ impl From<&ColumnValue> for ColumnValuePayload { #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] pub struct TableChangeData { + /// The schema this change occurred under: `"main"` for the primary + /// database, or the caller-chosen `ATTACH ... AS ` name otherwise. + /// Provenance metadata, not a stable identifier - see + /// `sqlx_sqlite_observer::TableChange::schema` for the full explanation of + /// why the same physical database can report different aliases depending + /// on who attached it. + pub schema: String, pub table: String, pub operation: Option, pub rowid: Option, @@ -133,6 +140,7 @@ pub fn event_to_payload(event: TableChangeEvent) -> TableChangePayload { /// Convert an observer `TableChange` to serializable data. fn change_to_data(change: &TableChange) -> TableChangeData { TableChangeData { + schema: change.schema.clone(), table: change.table.clone(), operation: change.operation.map(|op| match op { ChangeOperation::Insert => "insert".to_string(),