fix: recover from panic/cancellation-induced mutex poisoning and leaked transactions - #280
Merged
StefanSteiner merged 3 commits intoSep 6, 2026
Conversation
…bricking the server `with_engine` holds a `std::sync::MutexGuard<Option<Engine>>` across the tool closure it invokes. When a tool handler panics, the guard drops mid-unwind and poisons the mutex per std's default behavior. `ensure_engine` had no recovery path for poisoning (unlike its existing `ConnectionLost` recovery), so every subsequent `.lock()` returned `Err`, and every future tool call failed with `InternalError "Lock poisoned"` until the process restarted. `ensure_engine`'s engine-lock call sites now go through `lock_engine_recovering_poison`, which mirrors the shape of the existing `ConnectionLost` recovery: on a poisoned lock, it discards whatever `Engine` the poisoned guard held (a panic may have caught it mid-mutation, so its invariants can't be trusted), clears the poison flag, and falls through to the normal single-flight rebuild path. The next tool call gets a fresh engine instead of a possibly-corrupt one. Added a regression test that drives the defect through `HyperMcpServer::with_engine` itself (not `Engine` directly via `TestEngine`, which is why the crate's existing panic-path tests never caught this) — proven red against the pre-fix code with captured "Lock poisoned" output, green after the fix. Closes tableau#266
`AsyncTransaction::drop` cannot issue an async `ROLLBACK` — Rust has no async `Drop` — so it only warns when dropped without an explicit `commit()`/`rollback()`, exactly what happens when a task holding the guard panics or is cancelled. The transaction stays open on the connection when it returns to the pool. Confirmed empirically against the real engine (not merely asserted) that this is exploitable: `SELECT 1`, the previous default recycle probe (`RecycleStrategy::SelectOne`), succeeds even inside an open transaction, so it never detected the leak and the next borrower silently inherited a connection mid-transaction. Also confirmed a nested `BEGIN` does not error either (Postgres-style warn-and-continue), so "probe by issuing another BEGIN" is not a viable detection strategy — and that `ROLLBACK` is both a harmless no-op when nothing is open and a genuine, durable undo when something is (verified via a real INSERT that a second connection could not see afterward). `SelectOne` now issues an unconditional `ROLLBACK` instead of `SELECT 1` — same one-round-trip cost, since it doubles as the liveness probe a dead connection fails identically. `Ping`, `None` and `Custom` are unchanged (documented as not discharging a leaked transaction); the sync pool is unaffected because `Transaction`'s `Drop` rolls back synchronously and therefore cannot leak this way. Added a regression test using a real panicking `tokio::spawn` task (mirroring the join-error handling already present in `ingest.rs`/`ingest_arrow.rs`) that leaves a transaction open on a `max_size(1)` pool, then proves the next checkout does not observe the leaked row — proven red against the pre-fix `SELECT 1` probe with captured output (the leaked row survived), green after the fix. Closes tableau#263
…e pool fix's mechanism Review follow-ups on the tableau#266 / tableau#263 resilience work. The poison recovery stopped one line short of the brick it was written to prevent. `ensure_engine` still mapped a poisoned `engine_initialization` to `InternalError "Lock poisoned"`, so a panic in the construction critical section — `Engine::new*`, attachment replay, the `debug_assert!` — wedged the server exactly as before. The engine-mutex recovery made that *more* reachable, not less: it empties the slot, so every later call must construct and therefore must pass through that lock. It is a `Mutex<()>` held purely for mutual exclusion, so recovering is unconditionally safe. The recovery is now a shared free function in `engine.rs`, plus a `try_lock` sibling, rather than a private method on the server. Every site that locks the co-owned `Mutex<Option<Engine>>` goes through one of them: - `build_watcher_pool`, which returned a permanent "Engine lock poisoned" where an unattended ingest may see no tool call for hours. It now observes the empty slot and returns its existing transient "not initialized" error. - the watcher's `_table_catalog` upsert, whose `if let Ok(guard)` silently skipped bookkeeping for every subsequent file. - `status`, which conflated poisoned with contended and so answered `engine_busy: true` — "retry later" — permanently, with nothing in flight. Not in the review; found while auditing the remaining lock sites. Recovery destroys the ephemeral primary, which was invisible to the client. Behavior is kept — an `Engine` a panic caught mid-mutation can't be trusted — but the doc comment, the `warn!` and the changelog now say so. On the pool side the tests asserted the outcome without pinning the mechanism. `max_size(1)` does not guarantee connection reuse: deadpool replaces on any `RecycleError`, and a fresh session cannot see another session's uncommitted row, so `count == 0` passed either way. Comparing `async_session_id` across the leak proves the same physical connection was recycled, and is the suite's only assertion that `ROLLBACK` succeeds on an idle connection — the premise the one-round-trip cost argument rests on. A cancelled-task variant backs the changelog's "panicked or cancelled"; against the old `SELECT 1` probe it fails with the row surviving, so cancellation genuinely leaks rather than being covered by luck. Also drops a false claim predating the PR: `RecycleStrategy::None` said the pool still applies a passive `AsyncConnection::is_alive` check. The async manager never calls it — `None` is a genuine no-op. Only the sync pool checks. Most misleading for the caller combining `None` with `AsyncTransaction`. Tests: mcp lib 133 passed; api pool_tests 16 passed; watcher_tests 9 passed; fmt, clippy (-D warnings), rustdoc (-D warnings) and markdownlint clean. Each new test proven red against the reverted fix. Refs tableau#266 Refs tableau#263
StefanSteiner
force-pushed
the
fix/engine-pool-panic-resilience
branch
from
September 6, 2026 21:24
3cd7a38 to
22c6529
Compare
This was referenced Sep 6, 2026
Merged
docs: design exploration for promoting the shared hyperd daemon to a first-class API capability
#291
Closed
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
1 participant
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.
Summary
Two related resilience defects, same failure shape in adjacent layers: a panic or cancellation leaves shared state unusable, and nothing detects it.
#266 — a panicking tool call wedges the MCP server for the process lifetime
HyperMcpServer::with_engineholds astd::sync::MutexGuard<Option<Engine>>across the tool closure it invokes. A panic propagating out of that closure drops the guard mid-unwind and poisons the mutex.ensure_enginehad a recovery path forConnectionLostbut none for poisoning, so every subsequent tool call failed withInternalError "Lock poisoned"until the process restarted.Fix:
ensure_engine's engine-lock call sites now go through a newlock_engine_recovering_poisonhelper that mirrors the existingConnectionLostrecovery shape: on a poisoned lock it discards whateverEnginevalue the poisoned guard held (a panic may have caught it mid-mutation, so the invariants can't be trusted — rebuilding from scratch is the safe default), clears the poison flag, and falls through to the normal single-flight rebuild path. The next tool call gets a fresh engine instead of a possibly-corrupt one.This is a different layer from the existing RAII transaction guard (which prevents the SQL-level wedge of an open transaction) — this fixes the server-level wedge of a poisoned
Mutexthat outlives it.A regression test drives this through
HyperMcpServer::with_engineitself (notEnginedirectly viaTestEngine, which is why the crate's two existing panic-path tests never caught this — they never crosswith_engine). Proven red against the pre-fix code (captured "Lock poisoned" panic on the second call), green after the fix.#263 — a panicked or cancelled async task returns a pooled connection with
BEGINopenAsyncTransaction::dropcannot issue an asyncROLLBACK— Rust has no asyncDrop— so it only warns when dropped without an explicitcommit()/rollback(), exactly what happens when a task holding the guard panics or is cancelled. The transaction stays open on the connection when it returns to the pool.Verified the reported premise empirically against the real engine (it was previously unconfirmed):
SELECT 1, the prior default recycle probe (RecycleStrategy::SelectOne), does succeed even inside an open transaction, so it never detects the leak. Also confirmed a nestedBEGINdoes not error either (Postgres-style warn-and-continue), ruling out "probe via a second BEGIN" as a detection strategy — and confirmedROLLBACKis both a harmless no-op when nothing is open and a genuine, durable undo when something is (verified via a realINSERTinvisible from a second connection after rollback).Fix:
RecycleStrategy::SelectOnenow issues an unconditionalROLLBACKinstead ofSELECT 1— same one-round-trip cost, since it doubles as the liveness probe (a dead connection failsROLLBACKexactly as it would failSELECT 1).Ping,None, andCustomare left unchanged and documented as not discharging a leaked transaction. The sync pool is unaffected becauseTransaction'sDroprolls back synchronously and therefore cannot leak this way.A regression test uses a real panicking
tokio::spawntask (mirroring the join-error handling already present iningest.rs/ingest_arrow.rs) that leaves a transaction open on amax_size(1)pool, then proves the next checkout does not observe the leaked row. Proven red against the pre-fixSELECT 1probe (captured: the leaked row survived,left: 1, right: 0), green after the fix.Verification
cargo fmt --all -- --check: cleancargo clippy -p hyperdb-mcp --all-targets --all-features -- -D warnings: cleancargo clippy -p hyperdb-api --all-targets --all-features -- -D warnings: cleancargo test -p hyperdb-mcp: 611 passed, 0 failed, exit 0cargo test -p hyperdb-api: 630 passed, 0 failed, exit 0hyperdb-mcp/CHANGELOG.mdandhyperdb-api/CHANGELOG.mdupdated under## [Unreleased](merged into existing### Fixedheadings, no MD024 duplicates)npx markdownlint-cli2on both changed changelogs: 0 issuesCloses #266
Closes #263