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 1 commit into
Conversation
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.
| 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?
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.