refactor(mcp)!: drive Engine transactions with the RAII guard, and flatten client::Error - #261
Merged
StefanSteiner merged 2 commits intoSep 6, 2026
Conversation
StefanSteiner
force-pushed
the
refactor/raii-transactions-and-flat-client-error
branch
from
September 6, 2026 08:03
d78a9fb to
5b7c5a2
Compare
…uard `Engine::execute_in_transaction` drove the session with the unguarded `begin_transaction_unguarded` / `commit_unguarded` / `rollback_unguarded` trio and discharged the pairing obligation by hand: a `catch_unwind` around the closure plus a three-arm match that rolled back before resuming an unwind. That works, but every exit path has to be named, and the closure received `&Engine` — so transactional code could reach the raw connection and issue statements around the transaction it was supposed to be inside. Hold `hyperdb_api::Transaction` instead. The helper takes `&mut self` (the guard borrows the connection exclusively) and hands the closure an `EngineTransaction` view exposing the three operations the call sites actually use: `execute_command`, `create_table_in`, and `connection()` for `ArrowInserter`. Commit and the error-path rollback stay explicit, so the `tracing::warn!` on a failed rollback is preserved; the panic path is now the guard's `Drop`, which makes `catch_unwind` redundant and covers exit paths a hand-written match cannot enumerate. Two call sites held a `ScopedSearchPath` across the transaction. That guard borrows `&Engine` for its whole lifetime and cannot coexist with the transaction's exclusive borrow of the same connection, so add `Engine::with_search_path(alias, f)` — the closure form, which sequences the set/restore around `f` rather than holding a borrow across it. It restores on the `Ok`, `Err`, and panic paths, matching the guard's `Drop`. `scoped_search_path` is unchanged and still preferred where `&Engine` suffices. The `&mut Engine` requirement propagates to the seven ingest entry points and to `merge_via_temp_table`, which now passes the engine into its `replace_load` closure instead of having callers capture it alongside the `&mut` borrow. `HyperMcpServer::with_engine` hands out `&mut Engine`, which costs nothing: the engine already lives behind an exclusive `Arc<Mutex<Option<Engine>>>`, so no caller was ever sharing it. `create_table_in`'s validation and identifier quoting move into a shared `create_table_statements` builder so the `&Engine` and transaction-scoped paths cannot drift. Tests: `execute_in_transaction_commit_outlives_its_transaction` forces the committed row to survive a *later* transaction's rollback, so replacing the explicit commit with a bare drop fails it; and `execute_in_transaction_never_leaks_an_open_transaction` walks commit -> error -> panic -> commit on one engine, catching any path that leaves a `BEGIN` open. Four tests cover `with_search_path` restoring on success, error, panic, and the `None` no-routing case. Refs tableau#72 BREAKING CHANGE: `hyperdb-mcp` is published to crates.io, and this reshapes its library surface: `Engine::execute_in_transaction` takes `&mut self` and yields `EngineTransaction` rather than `&Engine`, the seven ingest entry points and `merge_via_temp_table` take `&mut Engine`, and `Engine::with_search_path` is new. The `missing_docs` allow reason on `src/lib.rs` claimed the crate is not published; corrected in passing. What the guard does *not* cover: the panic still propagates, exactly as the previous `resume_unwind` did, so `with_engine`'s `MutexGuard` is dropped mid-unwind and poisons the engine mutex. `ensure_engine` then fails every later tool call with "Lock poisoned" and nothing calls `clear_poison()`. That is pre-existing, not a regression, but the docs asserted otherwise, so `DEVELOPMENT.md` now scopes the claim to the SQL session and flags mutex recovery as an open design question. Also corrects a false claim this change introduced: `MIGRATING-0.3.md` and `docs/TRANSACTIONS.md` said nothing outside `hyperdb-api`'s tests and examples calls the unguarded methods. `KvStore::{pop, set_batch, set_batch_if_absent}` and the three `AsyncKvStore` equivalents do. They are not migrable by the same trick — they hold `&Connection`, so the guard's `&mut self` would have to be threaded out through `Connection::kv_store()` — and the async three cannot be fixed by any guard, since Rust has no async `Drop`. Both documents now name them as holdouts and describe the residual panic and cancellation windows. `Transaction::drop` now logs its implicit rollback (`warn!` on failure, `debug!` on success), restoring the observability the removed `catch_unwind` provided on the panic path.
…RORS shape Applies to `hyperdb_api_core::client::Error` the shape `hyperdb_api::Error` already took in tableau#70: a flat enum with one variant per failure mode, matched directly, per the Microsoft Pragmatic Rust Guidelines (M-ERRORS-CANONICAL-STRUCTS, M-ERRORS-AVOID-WRAPPING-AND-AS-DYN). Was a struct carrying `kind: ErrorKind` plus a `Box<dyn StdError + Send + Sync>` cause channel, which forced a two-level match and type-erased the cause. No effect on `hyperdb-api`'s public API, though `hyperdb-api-core` is itself published, so this is a breaking change to a published crate. `client::Error` and `client::ErrorKind` are not re-exported from `hyperdb-api` — `hyperdb-api/src/lib.rs` says so explicitly at the `Notice` re-export — and the only in-tree consumer outside `hyperdb-api-core` is the `From<client::Error> for hyperdb_api::Error` impl. `hyperdb-api-core` is published for dependency resolution only and its README and CHANGELOG both document it as internal with no semver promise. Information preserved: - `Connection`, `Cancelled`, and `Closed` carry `{ message, sqlstate }`; `Query` carries `{ message, sqlstate, detail, hint }`. Those are exactly the variants that could hold a SQLSTATE before, so `sqlstate()`, `detail()`, `hint()`, and `message()` return what they always did on those variants — which matters because the public mapping reads `sqlstate` for all four. - Narrower on the four single-string variants, though: `Authentication`, `FeatureNotSupported`, `Timeout`, and `Other` fold any `detail` into the message and discard `hint` and `sqlstate`, where `new_with_details` stored all three on any kind. Nothing observes the loss — the public mapping already discarded both on the arms these feed — so this is a narrowing of what is stored, not of what any caller can read. - `Error::with_cause` had zero call sites, so removing the `Box<dyn>` channel drops no information that was ever populated. The only other producer, `Error::io`, stored the same text as both `message` and `cause` and `Display` printed both — an I/O failure rendered as "refused: refused". That duplication is now gone. - gRPC is the one place that picks a variant at runtime from a wire code, so `grpc/error.rs` keeps a private `Variant` discriminator with a `build` method. Variants without a `detail` field fold it into the message rather than dropping it, matching what the old `Display` rendered. Constructors: every variant has a snake_case one taking `impl Into<String>` (`config`, `conversion`, and `cancelled` are new). `Error::new`, `with_cause`, and `new_with_details` are gone, along with `ErrorKind` and `kind()`. `closed()` and `timeout()` now take a message instead of supplying a canned one, and `io(io::Error)` became `from_io` so `io` could be the message-taking constructor like its peers. The public mapping is now a variant-to-variant match with no wildcard arm. `client::Error` is deliberately *not* `#[non_exhaustive]`: the attribute would buy forward-compatible matching that an internal type with a single in-workspace consumer has no use for, at the cost of the compile-time exhaustiveness check on the one match that must give every variant a public meaning. A variant added upstream should break that build rather than silently degrade to `Error::Internal`. Refs tableau#75 BREAKING CHANGE: `hyperdb-api-core` is published to crates.io, so although `client::Error` is internal and never re-exported, this breaks that crate's API: `client::ErrorKind` is deleted; `Error` goes from struct to enum; `Error::new`, `with_cause`, `new_with_details`, and `kind()` are removed; and `Error::io` is resignatured (the message-taking constructor now owns that name, and the `io::Error` one is `from_io`). Both removals fail loudly — `io::Error` does not implement `Into<String>`, and a missing `ErrorKind` is a hard resolution error — so no downstream can silently misbehave. `Display` text changes for I/O-origin connection errors, which do reach the public `hyperdb_api::Error`: `"refused: refused"` becomes `"refused"`. Intended, but string-matching on the message will notice.
StefanSteiner
force-pushed
the
refactor/raii-transactions-and-flat-client-error
branch
from
September 6, 2026 08:08
5b7c5a2 to
1897bb8
Compare
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
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.
Closes #72
Closes #75
Two related refactors:
hyperdb-mcp'sEnginenow drives transactions with the RAIITransactionguard, andhyperdb_api_core::client::Erroris flattened to one variant per failure mode.Both commits carry
!.mainalready pins the next release to1.0.0-rc.2with aRelease-As:footer, so the markers do not push the workspace to 2.0.0.#72 — RAII transactions in
EngineThe issue's premise was stale. It described the code as calling the deprecated
Connection::begin_transaction/commit/rollbackwrappers. Those were deleted in 1.0.0, soEnginewas already on the sanctioned, undeprecated*_unguardedAPI rather than on deprecated calls. The real work is adopting the guard.Engine::execute_in_transactionnow takes&mut self, holds ahyperdb_api::Transaction, and hands the closure anEngineTransactionview instead of&Engine. That is the substantive win: transactional code can no longer reach the raw connection and issue statements around the transaction it is supposed to be inside.EngineTransactionexposes only what the call sites actually use —execute_command,create_table_in, andconnection()forArrowInserter.Scope of the change:
EngineTransactionview.&mut Enginesignature cascade through seven ingest entry points,merge_via_temp_table, andHyperMcpServer::with_engine. This costs nothing: the engine already lives behind an exclusiveArc<Mutex<Option<Engine>>>, so no caller was ever sharing it.Engine::with_search_path(alias, f)was needed. The previousScopedSearchPathborrows&Enginefor its whole lifetime and runs SQL on drop, which cannot coexist with the transaction's exclusive borrow of the same connection. The closure form sequences set/restore aroundfrather than holding a borrow across it, and restores on theOk,Err, and panic paths.scoped_search_pathis unchanged and still preferred where&Enginesuffices.create_table_in's validation and identifier quoting move into a sharedcreate_table_statementsbuilder so the&Engineand transaction-scoped paths cannot drift.Worth flagging: the design the issue itself sketched would have deadlocked. It holds the connection mutex while calling
f(self), and the closure'sexecute_commandthen re-locks a non-reentrantstd::sync::Mutex.Removing the old
catch_unwindis behaviour-preserving. The guard'sDropperforms the same best-effort rollback and the unwind continues untouched.Transaction::dropnow also logs that rollback (warn!on failure,debug!on success), restoring the observability thecatch_unwindprovided on the panic path.An asymmetry worth stating plainly
Sync
Transaction::dropissues a best-effort rollback when the transaction was not committed. AsyncAsyncTransaction::dropcannot roll back at all — Rust has no async drop. It only emits a warning, and the transaction is left open until the next command on that connection. The guard is therefore not an equivalent safety net on the async side.Six unguarded call sites deliberately remain
KvStore::{pop, set_batch, set_batch_if_absent}and the threeAsyncKvStoreequivalents still call the*_unguardedtrio. They are not oversights, andMIGRATING-0.3.mdanddocs/TRANSACTIONS.mdnow document them as holdouts.KvStore<'conn>holds a shared&'conn Connection, whileConnection::transaction()takes&mut self. Adopting the guard would mean threading&mutout through the publicConnection::kv_store()— which breaks published API and would prevent holding two stores on one connection. This is exactly the casehyperdb-api/CHANGELOG.mdalready documents: the*_unguardedmethods exist precisely for "a helper that holds&self". The async three have the further problem above, which no guard can fix.#75 — flatten
client::Errorhyperdb_api_core::client::Errorbecomes a flatthiserrorenum with one variant per failure mode, mirroring what #70 did for the public error type. It was a struct carryingkind: ErrorKindplus aBox<dyn StdError + Send + Sync>cause channel, which forced a two-level match and type-erased the cause.#[non_exhaustive]was deliberately not used. The crate documents the type as forever-internal with no semver promise, so forward-compatibility there would buy nothing, and it would cost the compile-time exhaustiveness check on the one match that must give every variant a public meaning. A variant added upstream should break that build rather than silently landing inError::Internal.The
Box<dyn>cause channel is dropped.with_causehad zero call sites, so nothing that was ever populated is lost — and removing it fixed a real display bug:Error::iostored the same text as both message and cause, so an I/O failure rendered as"refused: refused".Behaviour changes to call out for consumers
Displaytext for I/O-origin connection errors changes:"refused: refused"becomes"refused". This does reach the publichyperdb_api::Error, so anything string-matching on the message will notice.detailinto the message for the non-Queryvariants, and dropshint/sqlstateon those four. This reaches nobody today, since the public mapping already discarded them there.From adversarial review
Three corrections that came out of review rather than the original work:
hyperdb-mcp/DEVELOPMENT.mdoverstated the guard. A panic still poisonswith_engine'sstd::sync::Mutex;ensure_enginethen returnsLock poisonedfor every later tool call, and nothing callsclear_poison(), so the engine is wedged for the process lifetime regardless. That is pre-existing rather than a regression. The doc is now scoped to the SQL session, and mutex-poison recovery inensure_engineis flagged as an open design question rather than changed silently — a reasonable follow-up, deliberately out of scope here.missing_docsallow reason athyperdb-mcp/src/lib.rs:6claimed the crate is not published to crates.io. It is, at1.0.0-rc.1. Corrected in passing.