fix(mcp): a panicking tool call poisons the engine mutex and wedges the server for the process lifetime #266

Summary

with_engine holds a std::sync::MutexGuard across the closure it invokes, so a panic propagating out of a tool call drops that guard mid-unwind and poisons the engine mutex. ensure_engine maps a poisoned lock to InternalError "Lock poisoned" — and does so for every subsequent tool call, because clear_poison() appears nowhere in the crate. One panic in one tool call bricks the MCP server until the process is restarted.

// hyperdb-mcp/src/server.rs:1627-1641
let (result, daemon_health_port, connection_lost) = {
    let mut guard = self.ensure_engine()?;
    // ...
    let engine = guard.as_mut().expect("ensure_engine guarantees Some");
    let daemon_health_port = engine.daemon_health_port();
    // ...
    self.ensure_catalog_ready(engine);
    let result = f(engine);

f(engine) runs with guard live, so an unwind through it poisons self.engine. Every path back into the engine then fails:

// hyperdb-mcp/src/server.rs:1423-1427
fn ensure_engine(&self) -> Result<std::sync::MutexGuard<'_, Option<Engine>>, McpError> {
    let guard = self
        .engine
        .lock()
        .map_err(|_| McpError::new(ErrorCode::InternalError, "Lock poisoned"))?;

The same mapping repeats at :1436, :1443, and :1476, and there is no recovery anywhere: grepping the workspace for clear_poison matches only prose in hyperdb-mcp/DEVELOPMENT.md, never Rust source.

What a user experiences

The first tool call after the panicking one returns InternalError: Lock poisoned, and so does every call after that — query, execute, load_file, all of them. From the client's side the server is still up, still connected, and still answering, but has become uniformly and permanently broken with a message that says nothing about what happened. The only remedy is restarting the MCP server, which for an editor-hosted server means restarting the host.

status is a partial exception: it takes try_lock and degrades rather than erroring (hyperdb-mcp/src/server.rs:3845), so it reports a degraded engine instead of Lock poisoned. Informative, but it can't restore service.

Why this is separate from the transaction work in #261

#261 added the RAII transaction guard, and that does close the SQL-level hole: a panic inside Engine::execute_in_transaction now rolls back as the unwind passes through, so the session isn't left with an open transaction. The failure described here survives that fix — it sits one layer up, at the server level. #261's own documentation says so:

It does not keep a panicking tool call from wedging the server, and neither did the catch_unwind it replaced — both re-raise the panic once the rollback is done. with_engine holds a std::sync::MutexGuard across the closure, so the unwind poisons Arc<Mutex<Option<Engine>>>; ensure_engine then returns InternalError "Lock poisoned" for every subsequent tool call, and nothing calls clear_poison(). The engine is unusable for the process lifetime.

— hyperdb-mcp/DEVELOPMENT.md:116-117

That note goes on to call recovery "a live design question, deliberately out of scope for issue #72." It was raised in review and deferred on purpose rather than changed silently; this issue is that deferred question, filed so it's tracked rather than resting in a development doc.

It also records the testing gap: the two panic tests (execute_in_transaction_rolls_back_on_panic and execute_in_transaction_never_leaks_an_open_transaction) drive TestEngine directly and never cross with_engine, so nothing in the suite can currently observe the poisoning.

Fix direction

ensure_engine already knows how to recover from an unusable engine. with_engine drops it out of the mutex on ErrorCode::ConnectionLost so the next call transparently re-spawns hyperd:

// hyperdb-mcp/src/server.rs:1647-1663
if connection_lost {
    tracing::warn!(
        // ...
    );
    *guard = None;
    // Reset so the next call re-bootstraps the catalog
    // against the fresh engine.
    if let Ok(mut ready) = self.catalog_ready.lock() {
        *ready = false;
    }
}

Treating poisoning the same way fits that existing design: on a poisoned lock, take the engine out (clear_poison() plus *guard = None, or poisoned.into_inner()), reset catalog_ready, and let the normal initialization path build a fresh one. The engine is already an Option<Engine> precisely so it can be absent and rebuilt, so "poisoned" is representable as "no engine yet" without new state.

Two things to settle rather than assume. First, whether the state behind the mutex is genuinely safe to resume from after an arbitrary panic — dropping and rebuilding sidesteps that question, which argues for it over into_inner(). Second, whether the attachment replay already wired into ensure_engine at :1469 is enough to restore the session's view of attached databases across the rebuild. A regression test would need to panic through with_engine itself rather than through TestEngine.

Provenance

Surfaced during review of #261 and explicitly deferred there. Related: #263, the async pooled-transaction leak from the same review — also an unwind-safety gap, but at the connection-pool layer rather than this one.