Stream Python cell execution through FIFO-backed stdin handling#132
Draft
t-kalinowski wants to merge 17 commits into
Draft
Stream Python cell execution through FIFO-backed stdin handling#132t-kalinowski wants to merge 17 commits into
t-kalinowski wants to merge 17 commits into
Conversation
Addresses review findings: [P1] /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session/windows_stdin.rs:155-164: When a Windows cell reaches input()/pdb with no queued line, the empty active_request path emits one prompt, then this branch runs with exec_state still WaitingInput and immediately continues without waiting on the condvar. That spins and repeatedly emits input_wait until another message arrives, so ordinary stdin prompts on Windows can flood the protocol and hang the worker. [P2] /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:242-245: When a Unix Python session is idle and a background thread mutates a matplotlib figure after the previous request, new cell inputs take this path instead of unix_stdin::begin_input_batch, so the old record_background_plots() call is skipped. The next non-empty no-op cell can then emit that stale background plot as part of the new request after plot_reset_pending is set. [P2] /Users/tomasz/.codex/worktrees/847e/mcp-repl/python/embedded.py:114-118: For cells whose last statement parses as an expression but fails to compile in eval mode, such as `x = 1\nawait foo()` or `x = 1\nyield 2`, this executes the setup statements before the SyntaxError is raised. A normal Python cell rejects the whole input before side effects, so a failed request can leave variables or files mutated even though the cell did not compile.
[P2] Protect cell runner dependencies from user globals — /Users/tomasz/.codex/worktrees/847e/mcp-repl/python/embedded.py:109-110. Because cells execute in the same globals() namespace that this helper reads from, ordinary user assignments can break future execution before recovery code can run. For example, after a user runs ast = 1 or shadows globals/compile, the next repl call fails inside _mcp_repl_run_cell at ast.parse instead of executing the submitted cell, effectively requiring a reset. [P2] Clear partial stdin batches when cells finish — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:531-534. When a Unix cell consumes only part of a stdin batch, such as os.read(0, 5) with the follow-up input bravo, the auto-added newline remains queued. This cleanup only clears fully consumed batches, so the next idle cell hits input_batch arrived while input is active and ends the session instead of running normally.
Finding 1: [P2] Preserve future imports across Python cells — /Users/tomasz/.codex/worktrees/847e/mcp-repl/python/embedded.py:121-126. When a user runs `from __future__ import annotations` in one call and then defines annotated functions or classes in a later call, the future flag is lost because each cell is parsed and compiled with fresh compiler state instead of the stateful `InteractiveInterpreter` compiler used before. This regresses normal REPL semantics; for example, a later `def f(x: list[int]): ...` produces evaluated annotations instead of strings. Finding 2: [P2] Hide cell runner frames from tracebacks — /Users/tomasz/.codex/worktrees/847e/mcp-repl/python/embedded.py:131-135. When any user cell raises, the exception propagates out of `_mcp_repl_run_cell` to Rust's `PyErr_Print`, so replies include the helper frame (`File "<string>", line ..., in _mcp_repl_run_cell`) before the user's `<mcp-repl>` frame. This regresses traceback clarity for common errors like `1/0`; trim the helper frame or print from the user traceback only.
[P1] Avoid the 3.11-only codeop keyword — /Users/tomasz/.codex/worktrees/847e/mcp-repl/python/embedded.py:147-147: On Python 3.10, the default python3 on ubuntu-22.04 in CI, codeop.Compile.__call__ does not accept incomplete_input; Python 3.9 also lacks the PyCF_ALLOW_INCOMPLETE_INPUT constant used above. Because every submitted cell reaches this call or identical calls below, the backend either fails startup or reports TypeError instead of executing cells.
Finding 1: [P2] Preserve sys.last_* when reporting cell errors — /Users/tomasz/.codex/worktrees/847e/mcp-repl/python/embedded.py:211-212. When a cell raises an exception, the code calls sys.excepthook directly, but the old CPython interactive path used PyErr_PrintEx(1) and populated sys.last_type, sys.last_value, sys.last_traceback/sys.last_exc first. After any failed cell, a follow-up like import sys; print(sys.last_value) now sees no last exception, which is a user-visible REPL regression. Finding 2: [P2] Report raw-stdin waits before blocking on Windows — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session/windows_stdin.rs:332-334. For a Windows cell that reaches os.read(0, n) before any follow-up stdin batch exists, guard.active_request is None, so the wait-state handling waits on the condvar without emitting input_wait or switching exec_state to WaitingInput. The first request times out as busy, and the user's next non-empty input is routed as a new cell instead of satisfying the raw read; the raw-stdin wait path needs the same wait-state transition before blocking when no active request exists.
Keep real Python input prompts visible while completed top-level cells return without an idle prompt. Treat Python worker readiness as prompt-free at startup, convert embedded SystemExit into session_end, and detach old readers during session-end respawn.
[P2] Delay session_end until Python finalizers finish — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:264-264: When a cell calls `sys.exit()` and an `atexit` handler or finalizer writes to stdout/stderr, `finish_session_end()` now runs before `Py_FinalizeEx`, so that cleanup `print` is emitted as `output_text` after `session_end`. The server treats any sideband message after `session_end` as a protocol error, so normal cleanup output returns `worker protocol error: worker sideband message after session_end` instead of a clean session end. Move the session-end boundary until after finalization output is drained, or prevent finalization-time writes from using sideband.
[P1] Restore the GIL only after releasing the queue mutex — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:472-475. When sys.stdin/raw stdin is waiting and a background Python thread runs a plot hook or another _mcp_repl callback, that thread can hold the GIL while blocking on SessionState. After a notification this path reacquires the mutex from Condvar::wait and then restores the GIL before releasing the mutex, creating a lock inversion that can hang the session. Release the mutex before dropping PythonThreadsAllowed, then reacquire it if needed. [P2] Consume idle interrupts in the cell loop — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:367-375. After Python reports prompt-free ready, a bare Ctrl-C while idle only sets interrupt_requested and wakes this loop. Because the loop ignores that flag, the server waits for an input_wait that never arrives, returns a timeout, and leaves the interrupt pending for the next stdin read or cell. Handle and clear idle interrupts here, or make the interrupt wait accept the existing ready state. [P2] Clear Python-side stdin buffers when a cell ends — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:406. When a cell reads only part of a stdin follow-up, such as sys.stdin.read(2) or readline(size), McpInputStream may prefetch the rest of that line into its Python-side _buffer. This cleanup only clears the Rust queue, so the next idle cell can consume those leftover bytes as stdin without a new input wait, breaking the new cell/stdin boundary. Clear the persistent Python input stream buffers at cell completion too.
Finding 1: [P2] Keep background stdin waits active after cell return — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:420-424: When a cell starts a background Python thread that calls `input()`/`sys.stdin.readline()` and the top-level cell returns before the answer is sent, the code unconditionally clears the active read consumer and emits `ready`. The next non-empty tool input is still consumed by that background read, but no cell loop remains to emit another `ready`, so the answer request stays busy until timeout and the session rejects later input as busy. Response: Preserved active Python stdin consumers across cell completion and emitted `ready` when a detached consumer answers after the cell loop is idle. Added a public MCP regression test for a background `input()` answer after cell return.
Finding 1: [P2] Complete detached raw stdin reads — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:716-718: When a background thread is blocked in `os.read(0, ...)` after the main cell has returned, the follow-up payload is consumed with `detached_request: true`, but this branch ignores that flag and never emits `ready` via `complete_detached_read_request()`. The tool call that supplied the bytes then waits until timeout even though Python consumed the input; line-based detached reads already complete the turn. Finding 2: [P2] Terminate workers that do not exit after session_end — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/worker_supervisor.rs:1385-1387: If a custom worker emits `session_end` before its process has actually exited, this path detaches readers and moves the process into a reaper that only calls `wait()`. Because the server-side IPC/stdin handles remain owned by that moved `WorkerProcess` and there is no bounded shutdown or kill, a worker waiting for those handles to close can live forever while a new worker is spawned, leaking the old process and session cleanup.
[P2] Wait for fresh readiness after input-wait interrupts — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/worker_process/interrupt.rs:203-208: When interrupting a Python cell paused in input()/stdin, the IPC state is still ready_for_input from the earlier input_wait. The new wait accepts that cached Ready immediately, so the interrupt reply can finish before the worker processes the interrupt and emits its post-interrupt readiness; an immediate follow-up input can then be marked complete by the interrupted cell's readiness instead of the follow-up cell. Clear or ignore pre-interrupt readiness in this path and wait for a fresh readiness event. [P2] Split the Windows stdin fixture too — /Users/tomasz/.codex/worktrees/847e/mcp-repl/tests/python_backend.rs:1475-1476: This split updates only the cross-platform PTY test; the Windows-only python_windows_pty_bridges_stdin_surfaces below still sends alpha, beta, and gamma in the same initial payload after the source. Under the new cell contract those lines are compiled as source instead of queued as stdin, so on Windows that test stops at win> and fails instead of producing WIN_STDIN_VALUES.
Finding 1: [P2] Avoid deleting the respawned worker temp dir — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/worker_supervisor.rs:1382-1389. When a worker sends `session_end` but is still alive, this branch moves the whole `WorkerProcess` into a background reaper and returns before respawning. `shutdown_graceful()` later runs `cleanup_session_tmpdir()` on the same stable session temp dir path that `WorkerSupervisor::spawn()` resets and reuses for the new worker, so the old reaper can remove the new worker's TMPDIR after it has started. Finish or detach the old cleanup before respawn, or allocate a new temp dir per respawn. Finding 2: [P2] Clear detached stdin leftovers before reporting ready — /Users/tomasz/.codex/worktrees/847e/mcp-repl/src/python_session.rs:745-752. When a detached/background stdin read is satisfied with more bytes than that read consumes, for example answering `one\ntwo` to a background `input()`, `complete_detached_read_request()` marks the worker ready but leaves queued stdin bytes untouched. The next idle cell can then call `input()` or `sys.stdin` and consume the stale `two` without reporting a prompt, unlike foreground cells where `finish_cell_request()` clears leftovers. Clear the queued stdin tail when completing a detached read or keep the detached request active until it is drained.
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
input(),sys.stdin, and help/pager flows.