fix: make observation a property of the database, not the handle (#53) - #62
Merged
Merged
Conversation
jjhafer
commented
Jul 31, 2026
jjhafer
commented
Jul 31, 2026
jjhafer
force-pushed
the
53-observation-per-db
branch
from
August 5, 2026 18:16
bce26b7 to
8ea4d00
Compare
velocitysystems
requested changes
Aug 6, 2026
jjhafer
force-pushed
the
53-observation-per-db
branch
from
August 6, 2026 17:54
8ea4d00 to
1202481
Compare
…vermine#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 silvermine#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) <noreply@anthropic.com>
jjhafer
force-pushed
the
53-observation-per-db
branch
from
August 6, 2026 20:33
1202481 to
0f8a191
Compare
velocitysystems
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Observation lived in a field on DatabaseWrapper, but every clone of a wrapper shares one underlying database and one physical write connection. A clone therefore kept its own empty observer, silently took the unobserved writer, and its committed writes reached no subscriber. This defeated the plugin's primary Rust entry point: connect() hands back a clone, so a consumer that enabled observation and subscribed was observing only its own copy.
Observation now hangs off the shared database, so every clone and every independent connect() to the same path observes through one broker. Whether two handles share observation is no longer something a caller can get wrong.
Auditing the bypass surface turned up two further ways a committed write could go unannounced. Writes through an attached database never consulted the observer at all, including from JavaScript, and including when every attachment was read-only. Changes are now routed to the broker of the database that owns the affected table. Separately, a writer dropped mid-transaction discarded its hooks before the pool's defensive rollback could fire, leaving the abandoned events to be published by the next transaction's commit as though they had been kept.
parking_lot becomes a direct dependency of sqlx-sqlite-conn-mgr, which owns the new observation slot. It was already compiled for that crate via both sqlx and tokio, so this adds nothing to the build graph or the lockfile. std::sync is unsuitable here 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. 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.