fix(mcp): panicked or cancelled async ingest returns a pooled connection with BEGIN still open

[StefanSteiner]

Summary

AsyncTransaction cannot roll back when it is dropped — Rust has no async Drop — so a task that dies between conn.transaction() and its commit()/rollback() leaves BEGIN open on the connection. That connection is then returned to its deadpool pool, where the default recycle probe is a SELECT 1 that succeeds perfectly well inside an open transaction. Nothing detects the leak, and the next borrower of that connection inherits it mid-transaction.

The Drop impl is explicit that it has no way out:

// hyperdb-api/src/async_transaction.rs:190-204
impl Drop for AsyncTransaction<'_> {
    fn drop(&mut self) {
        if !self.completed {
            // CRITICAL: Rust does not support async Drop, so we CANNOT issue a
            // ROLLBACK here (unlike the sync Transaction which can and does).
            // The transaction remains open on the server until the next command
            // on the connection, at which point Hyper handles it implicitly.
            // Users MUST explicitly call commit() or rollback().
            tracing::warn!(
                "AsyncTransaction dropped without explicit commit/rollback — \
                 transaction state is undefined until the next command on this connection"
            );
        }
    }
}

A tracing::warn! is the whole mitigation. The sync Transaction can and does issue the ROLLBACK from Drop; the async twin structurally cannot.

Why the pool doesn't catch it

ConnectionManager::recycle is the one thing guaranteed to run before a connection is handed back out, and its health probe defaults to SelectOne:

// hyperdb-api/src/pool.rs:539-544
match &self.config.recycle {
    RecycleStrategy::SelectOne => {
        conn.execute_command("SELECT 1")
            .await
            .map_err(RecycleError::Backend)?;
    }

RecycleStrategy::SelectOne is the PoolConfig default (hyperdb-api/src/pool.rs:292), and neither MCP pool overrides it:

// hyperdb-mcp/src/server.rs:2330-2332 (load_files)
PoolConfig::new(endpoint, workspace)
    .create_mode(CreateMode::DoNotCreate)
    .max_size(concurrency),

// hyperdb-mcp/src/watcher.rs:146-148 (directory watcher)
let cfg = PoolConfig::new(endpoint, workspace)
    .create_mode(CreateMode::DoNotCreate)
    .max_size(concurrency);

SELECT 1 is a legal statement inside an open transaction and returns a row, so the probe passes and deadpool concludes the connection is healthy. Nothing in the checkout path inspects transaction state. (This part is reasoning from the source plus standard transaction semantics, not an engine experiment.)

Reachable call sites

Four async ingest paths open a transaction on an Object<AsyncConnection> checked out of one of those pools:

• hyperdb-mcp/src/ingest.rs:973 — async CSV file COPY
• hyperdb-mcp/src/ingest.rs:1121 — async JSON INSERT loop
• hyperdb-mcp/src/ingest_arrow.rs:473 — async Parquet/Arrow CREATE TABLE AS SELECT
• hyperdb-mcp/src/ingest_arrow.rs:773 — async Arrow IPC via AsyncArrowInserter

The error paths are all safe. Each of the four runs its work in an inner async block and matches on the result, with an explicit rollback() before returning:

// hyperdb-mcp/src/ingest.rs:986-997
let row_count = match inner {
    Ok(n) => {
        txn.commit().await.map_err(McpError::from)?;
        n
    }
    Err(e) => {
        if let Err(rb) = txn.rollback().await {
            tracing::warn!("rollback after error failed: {}", rb);
        }
        return Err(e);
    }
};

So the exposure is panic and cancellation only, not error returns.

Panic is not hypothetical — load_files already anticipates one and keeps going:

// hyperdb-mcp/src/server.rs:2504-2512
while let Some(joined) = set.join_next().await {
    match joined {
        Ok((idx, outcome)) => collected[idx] = Some(outcome),
        Err(e) => {
            // A task panicked — surface it as an error on a
            // synthetic slot so the caller sees something.
            tracing::warn!("load_files task join error: {e}");
        }
    }
}

tokio catches the panic at the task boundary, the Object<AsyncConnection> drops and returns to the pool with BEGIN open, and the remaining entries in the batch keep drawing from that same pool.

Cancellation is available wherever these futures can be dropped at an await point: the JoinSet at hyperdb-mcp/src/server.rs:2364 aborts any still-running tasks if it is dropped before being drained, and the watcher's per-file ingests are detached tokio::spawns (hyperdb-mcp/src/watcher.rs:602) dropped at runtime shutdown.

Blast radius differs by caller

• load_files builds a fresh pool per call (hyperdb-mcp/src/server.rs:2329). A leaked BEGIN can only be inherited by the remaining entries of that same batch; the pool is dropped when the call returns.
• The directory watcher builds its pool once (hyperdb-mcp/src/watcher.rs:444) and holds it for the life of the watch, rebuilding only when an ingest hits a connection-lost error (hyperdb-mcp/src/watcher.rs:818-830). A BEGIN leaked there persists indefinitely and is re-inherited every time that connection is checked out again.

What a user experiences: after one panicked ingest, later ingests on the affected connection run inside a transaction they did not open. Their commit/rollback boundaries no longer line up with their own work — a later rollback() can discard rows written by an earlier call, and an earlier commit() can publish rows the current call hasn't finished writing. In the load_files case that's confined to one batch; in the watcher case it is a persistent, silent mis-attribution for the life of the watch.

Proposed direction

The sync fix (roll back from Drop) isn't available here, and wrapping every call site in catch_unwind plus a cancellation-safe scope guard has to be re-done correctly for every future call site. recycle is the only place left that always runs before a connection is reused — and, unlike a drop guard, it is directly testable. Two shapes worth weighing:

• Issue an unconditional ROLLBACK in ConnectionManager::recycle before the probe. Cheap, harmless outside a transaction, and discharges the obligation at the point of reuse.
• Have the probe assert an idle session rather than mere liveness, and evict the connection when it comes back non-idle. Slightly more code, but it converts a silent leak into an observable eviction.

Either way the invariant becomes "a connection leaving the pool has no open transaction," which a test can assert by panicking inside a pooled ingest and then checking the next checkout is clean.

Provenance

Surfaced during review of #261 (refactor(mcp)!: drive Engine transactions with the RAII guard). #261 closed the equivalent hole on the sync engine path — its RAII guard rolls back as the unwind passes through — but the async pooled paths above don't route through Engine::execute_in_transaction and were outside its scope. A companion issue on MCP engine mutex poisoning, the other unwind-safety gap from that same review, is filed alongside this one.