refactor: support closing all connections to a db in Rust#51
refactor: support closing all connections to a db in Rust#51onehumandev wants to merge 2 commits into
Conversation
| crate::resolve::resolve_database_path(path, app) | ||
| } | ||
|
|
||
| /// Close all database connections to a specific path. |
There was a problem hiding this comment.
The doc reads "Close all database connections to a specific path," but the function removes a single instance keyed by db and closes its wrapper/pool — and it aborts subscriptions while leaving in-flight transactions untouched. Given the motivating use case (close everything before deleting/recreating the db file), that gap matters: if a regular or interruptible transaction is in flight it still holds a checked-out connection, so wrapper.close().await will block until that transaction completes/rolls back (or its timeout fires), and the file lock can outlive this call. close_all/shutdown handle this via cleanup_all_transactions, but this path doesn't.
Would it be worth either (a) also clearing the transaction state for db here so the "safe to do file ops" guarantee actually holds, or (b) documenting that callers must ensure no transactions are open before relying on this? Either way, tightening the wording — it closes one named instance, not literally "all connections to a path," and reads close to close_all — would help readers.
Comment added by AI model Claude
| /// Returns `false` if the database was not loaded (nothing to close). | ||
| /// Any active subscriptions for this database are aborted before closing. | ||
| pub async fn close_database<R: Runtime>(app: &tauri::AppHandle<R>, db: &str) -> Result<bool> { | ||
| let active_subs = app.state::<ActiveSubscriptions>(); |
There was a problem hiding this comment.
Now that this logic is a public API callable by downstream Rust code, app.state::<T>() will panic if the plugin's state isn't managed (e.g. called before the plugin is initialized), whereas the old State<'_, T> command extractor surfaced a clean error. The happy path is safe since build() always manages these, and this matches the app.state() convention used elsewhere in this file — so this is a nit, not a blocker. What do you think about either switching to try_state() and returning an Err, or adding a short # Panics note to the doc so external callers know the precondition?
Comment added by AI model Claude
| /// Returns `true` if the database was loaded and successfully closed. | ||
| /// Returns `false` if the database was not loaded (nothing to close). | ||
| /// Any active subscriptions for this database are aborted before closing. | ||
| pub async fn close_database<R: Runtime>(app: &tauri::AppHandle<R>, db: &str) -> Result<bool> { |
There was a problem hiding this comment.
close_database looks up the raw db string as the map key with no resolution. Once #50 lands (keys become canonical absolute paths), a Rust caller passing a logically-equal-but-differently-spelled path (trailing slash, symlink, non-canonical form) gets Ok(false) — a silent no-op — and the subsequent file delete races an open connection, which is exactly the failure this function exists to prevent.
Would it make sense to resolve the path through crate::resolve::resolve_database_path (as load() does) before the lookup?
There was a problem hiding this comment.
Indeed; I will wait to re-request approval on this and to merge it until after that MR goes in to make sure this doesn't get lost in the shuffle.
onehumandev
force-pushed
the
onehumandev/close_invocation
branch
from
97a0955 to
d075d71
Compare
Replace path-as-identifier IPC with explicit key registration so the frontend and Rust callers open databases by stable keys (e.g. "MAIN") instead of filesystem paths. Paths are resolved once in Rust during plugin setup, validated, and cached; later opens only look up the key. Problems solved: - Security: untrusted frontend can no longer supply arbitrary paths over IPC; unknown keys fail with PATH_NOT_REGISTERED. - Cross-language identity: avoids TS/Rust path string mismatches (separators, symlinks, canonicalization) on every load. - Runtime path discovery: `on_setup` + `SetupRegistrar` register paths from `app.path()` / platform resolvers once, without repeat JNI or resolver work on each open. - Consistent open path: `load`, `Connection::connect`, and migrations share `connect_to_database` and the same `DbInstances` cache, along with assurance that migrations have completed before connecting to the database. - Safer registration: `validate_database_path` rejects relative paths, traversal, null bytes, and canonicalizes file paths at startup (fail-fast INVALID_PATH / PATH_TRAVERSAL). API changes: - `add_migrations(path, migrator)` → `register_database(key, path, migrator?)` (returns Result; adds `Builder::on_setup` / `SetupRegistrar`) - IPC/command args: `db` → `dbKey`; attached `databasePath` → `databaseKey` - `MigrationEvent`: adds `dbKey`, `dbPath` is now absolute PathBuf - `TransactionToken`: `dbPath` → `dbKey` - `Connection` trait on `AppHandle` for Rust-side opens by key Also: expose `canonicalize_database_path` from conn-mgr, tighten `is_memory_database` query-param matching, add toolkit `:memory:` tests, update README/guest-js rustdoc, and add `validate.rs` (replaces load-time path resolution in `resolve.rs`).
Previously, the only way to close the connection to a database was through a command invocation, which is only available through IPC. We need the ability to close any open connections to a db in Rust. For example if we are performing file operations (such as deleting and recreating the db) from the Rust layer, we need to first ensure all connections are closed before taking any further action. This simple refactor breaks the logic to close any open connections into a public function callable from Rust.
onehumandev
force-pushed
the
onehumandev/close_invocation
branch
from
d075d71 to
5aabb86
Compare
4 participants
Previously, the only way to close the connection to a database was through a command invocation, which is only available through IPC.
We need the ability to close any open connections to a db in Rust.
For example if we are performing file operations (such as deleting and recreating the db) from the Rust layer, we need to first ensure all connections are closed before taking any further action.
This simple refactor breaks the logic to close any open connections into a public function callable from Rust.
The logic is unchanged.