Finish RAII transaction migration in MCP Engine #72

Summary

Finish the transaction-API migration by moving hyperdb-mcp::Engine::execute_in_transaction (and its ~7 closure call sites) off the deprecated raw Connection::begin_transaction / commit / rollback methods and onto the RAII Transaction guard. This is the remaining hold-out from #69 (PR #71).

Background

#69 retired the &self raw transaction trio from hyperdb-api's public surface (deprecated, hidden from rustdoc) and migrated MCP's four async pool-path ingest functions to the RAII guard. The sync path through Engine::execute_in_transaction was deliberately not migrated because it's a structural cascade rather than a mechanical one.

Engine today holds the connection by value and exposes operations as &self methods so its containing Arc<RwLock<Engine>> lock model can support concurrent read-only reuse. The RAII guard requires &mut self on Connection::transaction, so any path that closes over Engine while holding a transaction can't use the guard without one of two structural changes.

The deprecated call sites

hyperdb-mcp/src/engine.rs:725-770 — Engine::execute_in_transaction(&self, f: F) annotated with #[allow(deprecated, reason = "Engine borrows &self; the RAII guard requires &mut. Migration tracked as a follow-up.")]. Body uses:

• self.connection.begin_transaction() — line 727
• self.connection.commit() — line 744
• self.connection.rollback() — line 749 (error path)
• self.connection.rollback() — line 769 (panic recovery path)

8 closure callers across the workspace, each passing |engine| { ... engine.execute_command(...) ... }:

• hyperdb-mcp/src/ingest.rs:652, 789, 883
• hyperdb-mcp/src/ingest_arrow.rs:318, 611
• hyperdb-mcp/src/lakehouse.rs:196
• hyperdb-mcp/src/server.rs:2270
• Test/integration sites in hyperdb-mcp/tests/

Two implementation paths

Path A — Engine::transaction(&mut self) -> Result<EngineTransaction<'_>>

Add an analogue of hyperdb-api::Transaction at the Engine level. The guard delegates the same query/insert/catalog helpers &Engine exposes today, but its existence borrows &mut Engine exclusively (matching Transaction's &mut Connection borrow).

Closure call sites rewrite:

// before
engine.execute_in_transaction(|engine| {
    engine.create_table_in(...)?;
    engine.execute_command(&sql)?;
    Ok(row_count)
})?;

// after
let txn = engine.transaction()?;
txn.create_table_in(...)?;
txn.execute_command(&sql)?;
let row_count = ...;
txn.commit()?;

Pro: mirrors the Connection / Transaction pattern symmetrically; compile-time exclusivity at the engine boundary.
Con: requires every closure callsite to be flattened (no more closure scope). EngineTransaction needs to expose the subset of Engine methods that callers actually use inside transactions — non-trivial surface to design.

Path B — Mutex<Connection> inside Engine

Keep Engine's &self external API, but wrap connection so a &self method can transiently take &mut. execute_in_transaction becomes:

pub fn execute_in_transaction<F, T>(&self, f: F) -> Result<T, McpError>
where F: FnOnce(&Engine) -> Result<T, McpError>
{
    let mut conn = self.connection.lock()?;  // or tokio::Mutex for async paths
    let txn = conn.transaction()?;
    // ... work ...
}

Pro: preserves all existing closure call sites unchanged.
Con: introduces runtime locking where there was none. Every &self method that accesses the connection now competes for the same mutex. Possibly fine (the connection was already serialized by Connection's wire protocol), but it's a real model change. Also, the panic-safety story (std::panic::catch_unwind already in execute_in_transaction) needs to be re-validated under lock-poisoning semantics.

Recommendation

Path A is the conceptually cleaner end state. Path B is a smaller diff. Pick after measuring the surface area EngineTransaction would need to expose — if more than ~6 methods, Path B is probably the pragmatic call.

Acceptance criteria

• Engine::execute_in_transaction no longer calls the deprecated Connection::begin_transaction / commit / rollback. The #[allow(deprecated, reason = "...")] annotation can be removed.
• All 8 closure call sites still work (or are rewritten without behavior change).
• Panic safety preserved — the std::panic::catch_unwind + best-effort rollback story at engine.rs:740-771 continues to work.
• No new Mutex-related deadlock risks; if Path B is chosen, document the lock ordering with respect to HyperMcpServer's outer Arc<RwLock<Engine>> (or whatever wraps Engine).
• After this lands, the deprecated pub fn begin_transaction / commit / rollback on Connection and AsyncConnection have zero in-tree callers (verify with grep). At that point a follow-up PR can delete the deprecated wrappers and the _raw rename, completing the API consolidation.

Related

• Remove the &self begin_transaction/commit/rollback trio (RAII Transaction is the only safe path) #69 / PR chore(api): flatten Error enum and add ergonomic constructors workspace-wide (#70) #71 — the original transaction-API consolidation. This issue is its remaining work.
• MIGRATING-0.3.md — describes the deprecation strategy and references this follow-up.
• docs/TRANSACTIONS.md — same.

Estimate

Path A: ~1 day (designing EngineTransaction API surface + rewriting 8 call sites + tests).
Path B: ~2 hours (wrap connection in Mutex, update execute_in_transaction, re-verify lock interaction).