Skip to content

fix: make MCP panic logging non-panicking - #66

Open
mukeshbhandarkar wants to merge 1 commit into
intuit:mainfrom
mukeshbhandarkar:fix/mcp-panic-hook-recursion
Open

mukeshbhandarkar wants to merge 1 commit into
intuit:mainfrom
mukeshbhandarkar:fix/mcp-panic-hook-recursion

Conversation

@mukeshbhandarkar

Copy link
Copy Markdown

Summary

Fixes the recursive panic path described in section 1 of #65.

The MCP panic hook currently logs through eprintln!. If stderr is unavailable or closed, eprintln! can panic while the process is already handling a panic. That re-enters the panic hook and can turn a recoverable panic into a hard abort.

This change makes stderr logging best-effort by writing through a locked stderr handle and ignoring write failures.

Changes

  • make mcp_log stderr writes non-panicking
  • make the panic hook's direct stderr write non-panicking
  • add a Linux regression test using /dev/full to simulate stderr write failure

Scope

This PR intentionally addresses only the panic-hook recursion from #65.

It does not address the separate OOM, segfault, worker lifecycle, or core-dump-location observations from that issue.

Validation

Passed locally:

  • cargo fmt --all -- --check
  • git diff --check

The targeted runtime test was attempted locally, but the build was blocked before reaching the MCP test by unrelated native dependency build failures in usearch/simsimd and then lbug.

The regression test is included so CI can exercise the panic path in the repository's supported build environment.

@murari316

Copy link
Copy Markdown
Collaborator

Verified locally (build clean, 123/123 infigraph-mcp tests pass, fmt+clippy clean) — this fix is solid and I was about to merge it, but #57 just merged and both PRs independently added a #[cfg(test)] mod tests block at the exact same location in main.rs, creating a trivial but real textual conflict. Could you rebase onto current main? The conflict is just two independent test modules needing to be combined into one — no logical overlap (this PR's panic-hook stderr test vs #57's ui_enabled_from arg-parsing tests).

Signed-off-by: mukeshbhandarkar <shell00035@gmail.com>
@mukeshbhandarkar
mukeshbhandarkar force-pushed the fix/mcp-panic-hook-recursion branch from 2b2fb0d to fb3226b Compare October 4, 2026 12:45
@mukeshbhandarkar

Copy link
Copy Markdown
Author

Thanks @murari316 — rebased onto current main and resolved the conflict by combining the two test modules, preserving the #57 UI tests alongside the Linux-gated panic-hook regression test.
git diff --check is clean. My local full build is still blocked by the previously noted usearch/simsimd native dependency issue.
The branch should be ready for another look.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants