Repository navigation
fix(fp_mcp): redirect Fortran WRITE(6,...) to stderr to protect JSON-RPC channel - #232
Conversation
…RPC channel
fp_mcp was the only MCP server without the fd-isolation guard that
eq_mcp/server.py and tr_mcp/server.py have carried since the
`fix(mcp): redirect Fortran WRITE(6,...) to stderr` patch. Fortran
WRITE(6,...) targets OS fd 1, which for an MCP stdio server IS the
JSON-RPC write pipe, so every Fortran diagnostic line lands in the
protocol stream.
FP is by far the worst offender of the three: FPWRTPRF dumps the whole
radial profile table to unit 6 on every time step, so the damage grows
linearly with NTMAX.
Measured client-side on the fp_iter01 fixture (NRMAX=40, NPMAX=50,
NTHMAX=50, NSAMAX=1, NSMAX=3) at NTMAX=2, counting
"Failed to parse JSONRPC message from server" records emitted by
mcp/client/stdio/__init__.py:157:
before this commit : 211 parse errors per run
after this commit : 0 parse errors per run
(~61 junk lines per extra time step, i.e. ~6000 at NTMAX=100. An
earlier measurement put NTMAX=1 at 150.) Runs still reported
status: ok before the fix — the client logs each unparseable line and
continues — so this was silent protocol corruption, not a hard failure.
Fix is the same two-step dance as eq_mcp/tr_mcp, applied at module load
before any mcp/logging import touches sys.stdout:
1. dup fd 1 (the JSON-RPC pipe) to a fresh fd, then dup2(2, 1) so
Fortran WRITE(6,...) goes to the subprocess stderr;
2. rebuild sys.stdout around the saved fd so the MCP stdio transport
still writes to the real pipe.
--print-tools / --help / --version keep the untouched stdout via the
same _ONESHOT_FLAGS guard the other two servers use.
fd 0 is deliberately NOT redirected — the Fortran library uses stdin
internally and redirecting it raised crash rates in the eq/tr work.
Side benefit confirmed while verifying: Fortran diagnostics are now
readable in the per-attempt stderr log instead of being shredded into
the protocol stream. That is what made the EQ->FP eq_load trace
("# OLD FILE (eq.bin) IS ASSIGNED FOR INPUT.", "XX FROPEN (EQ): FILE
NOT FOUND", "XX FPMESH:EQLOAD:IERR=7") visible at all.
Verified: 45 passed, 55 subtests passed; --print-tools still prints to
the terminal; 7 live fp_mcp runs at 0 parse errors each.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Heads-up on how this guard is installed. It redirects fd 1 when |
|
Yes, please make that change: move the guard into an |
The guard redirected fd 1 and replaced sys.stdout when fp_mcp.server was imported, so pytest, a notebook or an embedding application that imported the module lost its own stdout (what k-yoshimi#227 item 1 objects to for eq_mcp and tr_mcp). It is now `_install_fd_isolation()`, idempotent, called once by main() after the --help and --print-tools paths and after the check for the MCP SDK, immediately before the stdio server starts. This is the form k-yoshimi#226 gives eq_mcp and tr_mcp. The list of one-shot flags that skipped the import-time redirect is gone with it. It held --version, an option main() does not have: main() starts the stdio server for it, unguarded until now and guarded from here on. tests/test_import_isolation.py, each case in a fresh interpreter: an import leaves fd 1 and sys.stdout alone; main() installs the isolation before the server's first write; a second call keeps sys.stdout on the pipe; --help, --print-tools and a missing SDK install nothing. Against the previous commit all five fail. A running stdio server behaves as before. One real session (initialize, init, run with the library's defaults; mcp 1.27.1), stdout lines that are not JSON-RPC of all stdout lines: 63 of 66 without the guard (NTMAX=1; 83 of 86 at NTMAX=2), 0 of 3 with the guard at import (bb52868), 0 of 3 with it installed from main(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed
One behaviour to know: the old skip list held A running stdio server behaves as before. One real session (
|
… did nothing Answers the review of 0214201 on k-yoshimi#257. Two descriptor layouts defeated the isolation while it reported itself installed. Reproduced on the three servers with a write through sys.stdout (JSON-RPC) and one to fd 1 (Fortran): launched with its own stderr the pipe gets JSON-RPC only stderr merged into stdout the pipe gets both fd 2 closed the pipe gets both With fd 2 closed, dup(1) returns 2 and dup2(2, 1) copies the pipe back onto fd 1; with 2>&1, fd 1 and fd 2 are the same file and dup2 changes nothing. The helper is now one module, python/_fd_isolation.py, beside _runtime_mode.py: every server already puts <repo>/python on sys.path for its library wrapper, so the module is found wherever the wrapper is (also for a server package copied out of the tree and run with PYTHONPATH=<repo>/python; python/mcp-servers was on no server's path, and is not put there). The three copies are gone. install(): - opens fd 2 on the null device when it is closed, makes it inheritable, and gives it to sys.stderr, which Python left at None (print(file=sys.stderr) and a traceback would go to sys.stdout, the pipe). A repair that fails half-way is taken back, so that the next call does not find fd 2 open and skip the rest; a sys.stderr of None beside an open fd 2 is given fd 2 as well; - when fd 1 and fd 2 are the same file (st_dev and st_ino of fstat), points fd 1 at the null device instead of fd 2 and writes one warning line to stderr, best effort. That includes one terminal on both: a host that runs the server on a pty parses it, and a server started by hand shows the warning instead of the library's output. An inode number of 0 is no identity (Windows gives it for a pipe and a console; two TCP sockets gave it on macOS): such descriptors are taken for different files, so there a merged stream is not recognised and fd 1 goes to fd 2 as before; - keeps one flag for the process, so that a second server's main() in the same process does not dup the redirected fd 1 and take stderr for the pipe; - saves the pipe on a descriptor above 2, also when 0 is free; - changes nothing until everything is prepared. The one call that changes fd 1 is a dup2; a failure up to and including it leaves fd 1, sys.stdout and the flag as they were and closes what was opened for the redirect (a repair of fd 2 that was completed stays), and nothing after it can fail (the saved descriptor is wrapped with closefd=False, so one owner closes it); - flushes the old sys.stdout first and keeps a reference to it: a stream that owns fd 1 would close the redirected descriptor when collected. In a real stdio session (initialize, init, run) of tr_mcp with stderr merged into stdout, the stream now carries the three JSON-RPC answers, the warning and four lines of the SDK's own log; before this pull request it carried 13 lines that were not JSON-RPC, nine of them Fortran output. With its own stderr and with fd 2 closed: three lines, all JSON-RPC, for each of the three servers. Tests, 38 cases against 19. python/mcp-servers/fd_isolation_cases.py holds the checks every server has to pass, and each server's tests/test_fd_isolation.py runs them for itself (7 each), so a run of one server's directory includes its share; python/tests/test_fd_isolation.py has the 17 of the module itself, three of which check that each server's file calls every shared case. The importing modules register the cases module for assertion rewriting, so that a failure shows the values. New since 0214201: stderr merged into stdout and fd 2 closed, through each server's main(); two servers' main() in one process; a failure at the stream's construction and at the redirect; a stream that owns fd 1; descriptors without an identity; the repair of fd 2 for Python and for a child, a repair that fails (with fd 2 closed before, and with fd 2 open), and a Python without sys.stderr beside an open fd 2; the null device closed again after the redirect; the saved pipe with fd 0 closed; text buffered before the installation; a warning that cannot be written. The stub server writes through sys.stdout.buffer, the attribute the SDK wraps, and to fd 2; the child's PYTHONPATH is prepended to, not replaced. Measured by mutation of the module, each with the number of cases that then fail: no flag (2), fd 2 not repaired (8), fd 2 repaired with the pipe (6), merged stream not recognised (5), identity 0 accepted (1), old stream not kept (1), sys.stderr not repaired (4), fd 2 left close-on-exec (1), old stdout not flushed (1), the pipe saved on the lowest free descriptor (1), no cleanup after a failure (2), the warning not best effort (1), a failed repair of fd 2 not undone (1), sys.stderr built only on a descriptor the call opened (2), the repaired sys.stderr strict and block-buffered (1), the null device left open (1), the undo closing an fd 2 it did not open (1), the repaired sys.stderr owning fd 2 (1). One detail has no case: the new sys.stdout not owning its descriptor (closefd=False). The cases also pass with PYTHONDEVMODE=1, PYTHONWARNINGS=default and PYTHONUNBUFFERED=1 set. Comments and docstrings no longer say that the SDK writes through sys.stdout's line buffering. Not done here: fp_mcp (k-yoshimi#232, which installs from main() since cd51cb4), ti_mcp, wr_mcp and wrx_mcp would each adopt the module with an import and a call; the last three have no isolation today. Nothing was run on Windows or Linux. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-RPC pipe (k-yoshimi#262) The three servers had the layout of eq_mcp / tr_mcp / tot_mcp (k-yoshimi#257) and the same exposure: the library's WRITE(6,...) went to fd 1, the pipe to the MCP client. Each main() now calls _fd_isolation.install() (the module k-yoshimi#257 added, python/_fd_isolation.py) immediately before the stdio server is built; importing a server module leaves fd 1 and sys.stdout of the importing process alone, and --help / --print-tools / the missing-SDK path return before anything is redirected. Each server gets tests/test_fd_isolation.py running the shared checks of python/mcp-servers/fd_isolation_cases.py, and the meta-test in python/tests/test_fd_isolation.py now lists all six servers, so a case a server's file forgets to call fails there. fp_mcp is left to k-yoshimi#232, which the author is rebasing onto the shared module. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TA2KahTs3qLMZL6yjSsWwT
develop gained, among others, k-yoshimi#257 (python/_fd_isolation.py, the fd isolation shared by the stdio servers) and k-yoshimi#263 (ti_mcp, wr_mcp and wrx_mcp adopt it). No conflict. The next commit moves fp_mcp onto the shared module. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
As said on k-yoshimi#257: with the shared module in develop, fp_mcp drops its own copy of the isolation and does what the other six servers do since k-yoshimi#257 and k-yoshimi#263. main() calls _fd_isolation.install() immediately before the stdio server is built (after the --help / --print-tools / missing-SDK returns); the module is imported after the library wrapper, whose directory it shares. fp_mcp/server.py now differs from develop by that import, that call and the docstring section, which keeps the FP measurement (FPWRTPRF writes the radial profile table at preparation and every NTG2STEP steps, every step by default). So fp_mcp also gets what the module handles beyond the plain redirect: fd 2 closed, stdout and stderr the same file, one flag per process. Tests: fp_mcp/tests/test_fd_isolation.py runs the shared checks (python/mcp-servers/fd_isolation_cases.py), and python/tests/test_fd_isolation.py lists fp_mcp among the servers whose test file must call every shared case. fp_mcp/tests/test_import_isolation.py is removed: its five cases (import leaves fd 1 and sys.stdout alone, main() installs before the server starts, installing twice, --help and --print-tools, the missing SDK) are among the shared ones or the module's own. README Q8 says where unit 6 output goes, as k-yoshimi#264 did for the other servers; a CHANGELOG entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
As said on #257: |
|
Post-merge review of One process note for next time: on this repository the merge is done on the maintainer side after the two reviews, even when CI is green and the change is the one the review asked for. Please leave the merge to us. |
No line conflict. What the merge changes for this branch without one: - k-yoshimi#233 put a scalars object into fp_mcp's state schema, zero after a set until the next run; set_param_str opens that window too, which the schema text did not say yet (next commit). - k-yoshimi#256 brought eq_mcp's save tool, which this branch's docs still called future, after k-yoshimi#226 (fixed in a later commit). - k-yoshimi#232 and k-yoshimi#257 moved fp_mcp's fd isolation into python/_fd_isolation; this branch's server.py keeps it. - k-yoshimi#261 added the layout queries fp_state_abi_version and fp_state_size to libfpapi.so; fplib refuses a library without them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An Added entry, as k-yoshimi#232 has one for fp_mcp's fd isolation: the tool, what it calls and needs, the scalars window it opens, and that tot_mcp lists fp:KNAMEQ as run_pipeline only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
develop now carries the pieces split out of this branch (k-yoshimi#256 eq_save, k-yoshimi#257 the MCP fd isolation, k-yoshimi#265 the mcp pin) and k-yoshimi#232, k-yoshimi#233, k-yoshimi#240, k-yoshimi#242, k-yoshimi#243, k-yoshimi#248, k-yoshimi#249, k-yoshimi#251, k-yoshimi#254, k-yoshimi#255, k-yoshimi#258, k-yoshimi#259, k-yoshimi#261, k-yoshimi#263, k-yoshimi#264. 43 files conflicted; each was resolved as follows. - Taken from develop (36 files): every file where this branch only changed the mcp pin to "mcp>=0.9,<2" (develop has ">=1.2,<2", k-yoshimi#265): the seven pyproject.toml, six READMEs, the fp, ti, tot, wr and wrx servers, the Sphinx MCP pages, the trlib presentation and its builder, the 2026-04-18 MCP plan; and the eq_save pieces now in develop in their reviewed form: eq/eq_api.f90, eq/eq_api.h, python/eqlib/tests/test_save.py, eq_mcp/tests/test_save_tool.py; and tr_mcp/server.py, whose changes here were this branch's own fd isolation (develop's _fd_isolation replaces it) and the os._exit(0) at the end of main(). - The os._exit(0) that ended main() of tr_mcp and eq_mcp on this branch (to skip the Python teardown, cited in k-yoshimi#227 item 5) is gone with them: develop's main() returns 0, as approved in k-yoshimi#257. The abort it avoided was a double DEALLOCATE, which this branch fixed in tr/trcomm.f90 (DEALLOCATE_TRCOMM returns when nothing is allocated); that fix stays, merged without a conflict. - eq/eqfile.f90: develop's (EQSAVE without an argument, EQSAVE_IERR doing the work, and its kept EQDSKR error), plus this branch's MODELG=25 in EQ_READ's EQDSK branch (k-yoshimi#228-2). eq/eqmenu.f90 merged without a conflict but kept this branch's "CALL EQSAVE(IERR)"; it is develop's "CALL EQSAVE" again, as EQSAVE takes no argument. - python/eqlib/_ffi.py, eqlib.py, tests/test_eqlib.py: develop's, plus this branch's get_psi_rz (the eq_common_get_psi_rz_ prototype, the Eq.get_psi_rz method, its closed-handle assertion). - eq_mcp/server.py: develop's (the save tool of k-yoshimi#256, the shared fd isolation), plus this branch's get_psi_rz handler and tool and its --print-tools entry (13 tools). eq_mcp/tests/test_server.py: this branch's (13 tools with save and get_psi_rz), which contains develop's change. - python/mcp-servers/eq_mcp/tests/test_import_isolation.py (this branch only, no conflict) is removed: it pinned this branch's own isolation helper (_install_fd_isolation, _FD_ISOLATION_INSTALLED in eq_mcp and tr_mcp), which develop's python/_fd_isolation.py replaces; develop's tests/test_fd_isolation.py of each of the seven servers checks the same contract (an import leaves fd 1 and sys.stdout alone). - .github/workflows/python-tests.yml: develop's mcp pin; this branch's numpy install (the get_psi_rz tests import numpy) and ../task symlink step kept. - CHANGELOG.md did not conflict. The three k-yoshimi#228 entries this branch held back in d8b6e4d are added to [Unreleased] Fixed, after its first entry, where no other open pull request inserts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
fp_mcpgets the fd isolation that the other six stdio servers have ondevelopsince #257 and #263:main()calls_fd_isolation.install()(the shared module,python/_fd_isolation.py) immediately before the stdio server is built.Fortran
WRITE(6,...)targets OS fd 1, which for an MCP stdio server is the JSON-RPC write pipe, so every Fortran diagnostic line lands in the protocol stream.Why FP is the worst case
FPWRTPRFwrites the whole radial profile table to unit 6 once at preparation and then everyNTG2STEPsteps (every step by default), so the damage grows withNTMAX. One real stdio session (initialize,init,run, library defaults), counting the stdout lines that are not JSON-RPC:NTMAX=1NTMAX=2develop'sfp_mcpClient-side, on the
fp_iter01fixture (NRMAX=40, NPMAX=50, NTHMAX=50), the MCP client logged 211Failed to parse JSONRPC message from serverrecords per run atNTMAX=2(150 atNTMAX=1), and 0 with the isolation atNTMAX=2(measured in August, with the earlier form of this change). Runs still reportedstatus: ok: the client logs each unparseable line and goes on, so this was silent protocol corruption.Change
fp_mcp/server.py:import _fd_isolationafter thefplibimport, and_fd_isolation.install()inmain()after the--help/--print-tools/ missing-SDK returns. It now differs fromdevelopby that import, that call and a docstring section, liketi_mcp,wr_mcpandwrx_mcpin fix(mcp): ti_mcp, wr_mcp and wrx_mcp keep Fortran output off the JSON-RPC pipe (#262) #263. Importingfp_mcp.server(pytest, a notebook,tot_mcp) leaves fd 1 andsys.stdoutalone (PR #226 follow-up: fix MCP stdio isolation, TR lifecycle, and EQ save error reporting #227 item 1).fp_mcpalso handles what the plain redirect did not: fd 2 closed, stdout and stderr the same file, one flag per process. Measured atNTMAX=2: with stderr merged into stdout, the stream carries the three answers, the module's warning line and four lines of the SDK's own log; with fd 2 closed, the three answers only.fp_mcp/tests/test_fd_isolation.pyruns the shared checks (python/mcp-servers/fd_isolation_cases.py), andpython/tests/test_fd_isolation.pylistsfp_mcpamong the servers whose test file must call every one of them. This branch's earlierfp_mcp/tests/test_import_isolation.pyis removed: its five cases are among the shared ones or the module's own.Verification
python/with CI's flags, after a clean build: 971 passed, 38 skipped (macOS arm64, gfortran 15).Note for reviewers
Independent of #231 (the
set_param_strtool offp_mcp): the two touch different regions ofserver.pyand either can merge first. Python-only; no.sorebuild.🤖 Generated with Claude Code