fix: permanent fd redirect for stdout suppression (issue #42)#45
Closed
brianlball wants to merge 1 commit intodevelopfrom
Closed
fix: permanent fd redirect for stdout suppression (issue #42)#45brianlball wants to merge 1 commit intodevelopfrom
brianlball wants to merge 1 commit intodevelopfrom
Conversation
Redirect C-level stdout (fd 1) to stderr once at startup, give Python sys.stdout a private fd to the real MCP client pipe. Catches ALL C-level pollution (SWIG GC, Polyhedron geometry, future unknowns) with zero races and no per-callsite wrappers. Fixes concurrent tool timeout (issue #42) and Polyhedron stdout leak on complex models (test_complex_model_stdout_purity). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Closing — will apply fix directly to optimize branch and merge optimize→develop instead. |
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
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.
Fixes #42
Problem
Two related bugs corrupt the MCP JSON-RPC stream on stdout:
Concurrent tool timeout (issue MCP error -32001: Request timed out on all tools — stdout suppression race condition #42): Global FastMCP middleware held
os.dup2()on fd 1 for the entire tool call. Concurrent worker threads interleaved their fd redirects, sending JSON-RPC responses to stderr — clients received nothing →-32001timeout.Polyhedron stdout leak: OpenStudio's C++ geometry engine writes
[utilities.Polyhedron]diagnostics directly to fd 1. These aren't SWIG GC warnings — they fire during read-only queries likeget_building_infoon complex models. Any per-callsite suppression approach misses them.Root Cause
Both bugs stem from the same design flaw: toggling fd 1 back and forth between stdout and stderr on a per-call basis. This is inherently racy under concurrency and inherently incomplete against unknown C-level callsites.
Fix
Redirect fd 1 to stderr once at startup, then give Python
sys.stdouta private fd to the real MCP client pipe.After this:
Files Changed (4)
stdout_suppression.pyredirect_c_stdout_to_stderr(). Keepsuppress_openstudio_warnings()as no-op for backward compat.server.pyredirect_c_stdout_to_stderr()beforemcp.run()tests/test_concurrent_tools.py.github/workflows/ci.ymlTest Results
Supersedes PR #43
PR #43 (from anchapin's fork) used per-callsite targeted suppression with RLock. That approach fixes the race but misses C-level stdout from unexpected callsites (Polyhedron geometry warnings). This PR uses a permanent redirect that catches everything.
🤖 Generated with Claude Code