From 113d9f63b5a1b254839f5f566e339dfd53ada566 Mon Sep 17 00:00:00 2001 From: kshitijk4poor Date: Mon, 20 Jul 2026 12:44:10 +0530 Subject: [PATCH] fix: Windows guard, dedup recovery, profile-safe paths, clear SO_RCVTIMEO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up fixes for salvaged PR #67686 (issue #67639): 1. Windows regression: bare 'import fcntl' at module level in entry.py and slash_worker.py would crash on Windows (fcntl is POSIX-only). entry.py explicitly aims to 'import cleanly on Windows'. Extracted all fcntl/socket logic to tui_gateway/_stdin_recovery.py with try/except import guards. 2. fd leak: socket.fromfd() dups fd 0; s.detach() returned the fd number without closing it, leaking one fd per recovery call. Changed to s.close() (safe — fromfd duped the fd, closing won't close stdin). 3. Code duplication: ~80 lines of recovery loop + diagnostic were copy-pasted between entry.py and slash_worker.py. Extracted to shared tui_gateway/_stdin_recovery.py (handle_spurious_eof + diagnose_stdin_state). 4. Profile-unsafe path: scanner used Path.home() / '.hermes' / 'plugins' but canonical plugin discovery uses get_hermes_home() / 'plugins'. With HERMES_HOME pointing elsewhere, scanner missed the actual plugin dir. Also gated project plugins behind HERMES_ENABLE_PROJECT_PLUGINS to match hermes_cli/plugins.py. 5. SO_RCVTIMEO not cleared: recovery only called os.set_blocking(0, True) but a child that set SO_RCVTIMEO would cause the next readline to time out and loop. Now clears SO_RCVTIMEO alongside O_NONBLOCK. --- scripts/check_subprocess_stdin.py | 33 ++++--- tui_gateway/_stdin_recovery.py | 151 ++++++++++++++++++++++++++++++ tui_gateway/entry.py | 70 +------------- tui_gateway/slash_worker.py | 54 +---------- 4 files changed, 182 insertions(+), 126 deletions(-) create mode 100644 tui_gateway/_stdin_recovery.py diff --git a/scripts/check_subprocess_stdin.py b/scripts/check_subprocess_stdin.py index 9ee42e1d4908..eca28b814ee5 100644 --- a/scripts/check_subprocess_stdin.py +++ b/scripts/check_subprocess_stdin.py @@ -38,13 +38,15 @@ TUI_CONTEXT_DIRS = [ ] # User plugin roots — scanned at runtime if they exist. Plugins load from -# ~/.hermes/plugins/ and ./.hermes/plugins/ (hermes_cli/plugins.py:10-12), -# but the guard only checked the bundled plugins/ dir, missing user-installed -# code that spawns subprocesses (gap reported in #67639). -USER_PLUGIN_DIRS = [ - Path.home() / ".hermes" / "plugins", - Path.cwd() / ".hermes" / "plugins", -] +# ``get_hermes_home() / "plugins"`` (user) and ``./.hermes/plugins/`` (project, +# gated behind ``HERMES_ENABLE_PROJECT_PLUGINS``) — see +# ``hermes_cli/plugins.py:10-12``. The guard only checked the bundled +# ``plugins/`` dir, missing user-installed code that spawns subprocesses +# (gap reported in #67639). +# +# Import is deferred to ``main()`` (after ``os.chdir(repo_root)``) because +# this script runs as a standalone subprocess — ``hermes_constants`` isn't +# on ``sys.path`` until the repo root is added. # subprocess and os APIs that inherit stdin by default when called without # an explicit stdin= argument. The original regex only covered run/Popen @@ -155,6 +157,11 @@ def main() -> int: repo_root = Path(__file__).resolve().parent.parent os.chdir(repo_root) + # Add repo root to sys.path so we can import hermes_constants (this script + # runs as a standalone subprocess, not as a module). + sys.path.insert(0, str(repo_root)) + from hermes_constants import get_hermes_home + all_violations = [] for tui_dir in TUI_CONTEXT_DIRS: @@ -178,11 +185,15 @@ def main() -> int: violations = find_subprocess_calls(content, rel) all_violations.extend(violations) - # Scan user plugin directories (Gap 1: guard missed ~/.hermes/plugins/ - # and ./.hermes/plugins/, where user-installed plugins like ori/hooks.py - # can spawn subprocesses with inherited stdin — #67639). + # Scan user plugin directories (Gap 1: guard missed user-installed + # plugins in get_hermes_home()/plugins/ and project plugins in + # ./.hermes/plugins/, where code like ori/hooks.py can spawn + # subprocesses with inherited stdin — #67639). + plugin_roots: list[Path] = [get_hermes_home() / "plugins"] + if os.environ.get("HERMES_ENABLE_PROJECT_PLUGINS"): + plugin_roots.append(Path.cwd() / ".hermes" / "plugins") seen_roots: set[Path] = set() - for plugin_root in USER_PLUGIN_DIRS: + for plugin_root in plugin_roots: resolved = plugin_root.resolve() if resolved in seen_roots or not resolved.is_dir(): continue diff --git a/tui_gateway/_stdin_recovery.py b/tui_gateway/_stdin_recovery.py new file mode 100644 index 000000000000..80c77aeb0dca --- /dev/null +++ b/tui_gateway/_stdin_recovery.py @@ -0,0 +1,151 @@ +"""Shared spurious stdin-EOF recovery for the TUI gateway entry point and slash worker. + +When a child process inherits fd 0 (stdin) and sets ``O_NONBLOCK``, the flag +lands on the **shared open file description** — not just the child's descriptor. +The gateway's next ``read()`` returns ``EAGAIN``, which CPython's buffered +``TextIOWrapper`` converts to ``b''`` (apparent EOF), killing the gateway. + +This module provides: +- :func:`diagnose_stdin_state` — forensic diagnostic (``O_NONBLOCK`` / ``SO_RCVTIMEO``) +- :func:`handle_spurious_eof` — check whether an empty ``readline()`` is a genuine + peer-close or a spurious EOF, and recover if spurious. + +The recovery is **POSIX-only** (``fcntl``). On Windows, ``O_NONBLOCK`` on a +shared file description is not a concern, so the guard simply reports a +genuine EOF and lets the caller exit. +""" + +from __future__ import annotations + +import os +import time + +try: + import fcntl as _fcntl + _HAS_FCNTL = True +except ImportError: + _fcntl = None # type: ignore[assignment] + _HAS_FCNTL = False + +try: + import socket as _socket + _HAS_SOCKET = True +except ImportError: + _socket = None # type: ignore[assignment] + _HAS_SOCKET = False + +import struct + + +# Rate-limit: at most this many spurious-EOF recoveries per 60-second window. +# A child aggressively flipping ``O_NONBLOCK`` on the shared fd would otherwise +# create a tight busy-loop burning CPU. Exceeding the cap exits the process — +# the parent (TUI / gateway) respawns it with fresh state, which is safer than +# fighting forever. +MAX_RECOVERIES_PER_MINUTE = 10 + + +def diagnose_stdin_state() -> str: + """Return a diagnostic string about stdin's current state. + + Used for crash-log forensics when stdin iteration falls through. + Distinguishes genuine peer-close (flag clear) from spurious EOF + caused by a child setting ``O_NONBLOCK`` on the shared file description. + """ + parts: list[str] = [] + if _HAS_FCNTL and _fcntl is not None: + try: + flags = _fcntl.fcntl(0, _fcntl.F_GETFL) + parts.append(f"O_NONBLOCK={'1' if flags & os.O_NONBLOCK else '0'}") + except Exception as e: + parts.append(f"F_GETFL error: {e}") + else: + parts.append("O_NONBLOCK=n/a (no fcntl)") + # ``SO_RCVTIMEO`` is a socket option (not a file-status flag), equally + # shared on the open file description. A child setting it via + # ``setsockopt`` launders into the same spurious-EOF path with + # ``O_NONBLOCK`` clear, so we report it alongside the flag. + if _HAS_SOCKET and _socket is not None: + try: + s = _socket.fromfd(0, _socket.AF_UNIX, _socket.SOCK_STREAM) + try: + tv = s.getsockopt(_socket.SOL_SOCKET, _socket.SO_RCVTIMEO) + parts.append(f"SO_RCVTIMEO={tv!r}") + finally: + # ``fromfd`` duped the fd; ``close`` releases the dup without + # touching the original fd 0. + s.close() + except Exception: + pass + return ", ".join(parts) if parts else "unknown" + + +def handle_spurious_eof( + recovery_times: list[float], + log_fn: object, +) -> bool: + """Check whether an empty ``readline()`` is spurious; recover if so. + + Returns ``True`` if the caller should ``continue`` the read loop + (spurious EOF was recovered), ``False`` if it should ``break`` (genuine + peer-close or rate limit exceeded). + + ``log_fn`` is called with a diagnostic string — ``_log_exit`` in + ``entry.py``, ``print(file=sys.stderr)`` in ``slash_worker.py``. + """ + # Without ``fcntl`` (Windows) we can't check the flag, and the + # ``O_NONBLOCK`` shared-description issue is POSIX-specific anyway — + # treat it as a genuine EOF. + if not (_HAS_FCNTL and _fcntl is not None): + log_fn("stdin EOF (peer closed)") # type: ignore[operator] + return False + + try: + flags = _fcntl.fcntl(0, _fcntl.F_GETFL) + is_nonblock = bool(flags & os.O_NONBLOCK) + except Exception: + is_nonblock = False + + if not is_nonblock: + # Genuine peer-close — no subprocess flag tampering detected. + log_fn("stdin EOF (peer closed)") # type: ignore[operator] + return False + + # Spurious EOF: a child set ``O_NONBLOCK`` (and/or ``SO_RCVTIMEO``) on + # the shared file description, laundered into ``b''`` / ``EAGAIN`` by + # CPython's buffered layer. Restore blocking mode and resume. + now = time.time() + recovery_times.append(now) + recovery_times[:] = [t for t in recovery_times if t > now - 60] + if len(recovery_times) > MAX_RECOVERIES_PER_MINUTE: + log_fn( # type: ignore[operator] + f"stdin spurious-EOF recovery rate exceeded " + f"({len(recovery_times)}/min, cap {MAX_RECOVERIES_PER_MINUTE})" + ) + return False + + diag = diagnose_stdin_state() + log_fn(f"stdin spurious EOF (subprocess O_NONBLOCK flip), recovering: {diag}") # type: ignore[operator] + + # Clear ``O_NONBLOCK`` on the shared file description. + os.set_blocking(0, True) + + # Also clear ``SO_RCVTIMEO`` if it was set by a child on the shared + # description. A non-zero timeout would cause the next ``readline()`` + # to time out and return ``''`` again, looping until the rate limiter + # kicks in. Clearing it restores fully blocking semantics. + if _HAS_SOCKET and _socket is not None: + try: + s = _socket.fromfd(0, _socket.AF_UNIX, _socket.SOCK_STREAM) + try: + # Zero timeval: tv_sec=0, tv_usec=0 (struct timeval on most platforms) + s.setsockopt(_socket.SOL_SOCKET, _socket.SO_RCVTIMEO, struct.pack("ll", 0, 0)) + finally: + s.close() + except Exception: + pass + + # ``_io.TextIOWrapper.readline`` returns an empty string on ``EAGAIN`` + # but does NOT stick EOF; after restoring blocking, the next call will + # block until data arrives or the peer truly closes. + return True diff --git a/tui_gateway/entry.py b/tui_gateway/entry.py index 743bc144c7b6..be755c461b5c 100644 --- a/tui_gateway/entry.py +++ b/tui_gateway/entry.py @@ -10,14 +10,14 @@ import hermes_bootstrap hermes_bootstrap.harden_import_path() -import fcntl import json import logging import signal -import socket import time import traceback +from tui_gateway._stdin_recovery import handle_spurious_eof + from tui_gateway import server from tui_gateway.server import _CRASH_LOG, dispatch, resolve_skin, write_json from tui_gateway.transport import TeeTransport @@ -294,36 +294,6 @@ def join_mcp_discovery(timeout: float | None = None) -> bool: # Spurious stdin-EOF recovery tracker (shared open-file-description O_NONBLOCK flip). _recovery_times: list[float] = [] -_MAX_RECOVERIES_PER_MINUTE = 10 - - -def _diagnose_stdin_state() -> str: - """Return a diagnostic string about stdin's current state. - - Used for crash-log forensics when stdin iteration falls through. - Distinguishes genuine peer-close (flag clear) from spurious EOF - caused by a child setting O_NONBLOCK on the shared file description. - """ - parts: list[str] = [] - try: - flags = fcntl.fcntl(0, fcntl.F_GETFL) - parts.append(f"O_NONBLOCK={'1' if flags & os.O_NONBLOCK else '0'}") - except Exception as e: - parts.append(f"F_GETFL error: {e}") - # SO_RCVTIMEO is a socket option (not a file-status flag), equally shared - # on the open file description. A child setting it via setsockopt launders - # into the same spurious-EOF path with O_NONBLOCK clear, so we report it - # alongside the flag. - try: - s = socket.fromfd(0, socket.AF_UNIX, socket.SOCK_STREAM) - try: - tv = s.getsockopt(socket.SOL_SOCKET, socket.SO_RCVTIMEO) - parts.append(f"SO_RCVTIMEO={tv}") - finally: - s.detach() - except Exception: - pass - return ", ".join(parts) if parts else "unknown" def main(): @@ -393,40 +363,10 @@ def main(): while True: raw = sys.stdin.readline() if not raw: - # Stdin iteration fell through — check if spurious (O_NONBLOCK flip - # by a child on the shared open file description) or genuine EOF. - try: - flags = fcntl.fcntl(0, fcntl.F_GETFL) - is_nonblock = bool(flags & os.O_NONBLOCK) - except Exception: - is_nonblock = False - - if not is_nonblock: - # Genuine peer-close — no subprocess flag tampering detected. - _log_exit("stdin EOF (peer closed)") + # Stdin fell through — check if spurious (O_NONBLOCK flip by a + # child on the shared open file description) or genuine EOF. + if not handle_spurious_eof(_recovery_times, _log_exit): break - - # Spurious EOF: a child set O_NONBLOCK (or SO_RCVTIMEO) on the - # shared file description, laundered into b''/EAGAIN by CPython's - # buffered layer. Restore blocking mode and resume. - now = time.time() - _recovery_times.append(now) - _recovery_times[:] = [t for t in _recovery_times if t > now - 60] - if len(_recovery_times) > _MAX_RECOVERIES_PER_MINUTE: - _log_exit( - f"stdin spurious-EOF recovery rate exceeded " - f"({len(_recovery_times)}/min, cap {_MAX_RECOVERIES_PER_MINUTE})" - ) - break - - diag = _diagnose_stdin_state() - _log_exit( - f"stdin spurious EOF (subprocess O_NONBLOCK flip), recovering: {diag}" - ) - os.set_blocking(0, True) - # _io.TextIOWrapper readline returns empty string on EAGAIN but - # does NOT stick EOF; after restoring blocking, the next call - # will block until data arrives or the peer truly closes. continue line = raw.strip() diff --git a/tui_gateway/slash_worker.py b/tui_gateway/slash_worker.py index 372addfbc8d9..5b75dbd16565 100644 --- a/tui_gateway/slash_worker.py +++ b/tui_gateway/slash_worker.py @@ -18,17 +18,16 @@ hermes_bootstrap.harden_import_path() import argparse import contextlib -import fcntl import io import json import os -import socket import sys import threading import time import cli as cli_mod from cli import HermesCLI +from tui_gateway._stdin_recovery import handle_spurious_eof from rich.console import Console # Env-overridable so the integration test can drive sub-second timing. @@ -146,60 +145,15 @@ def main(): # issue as the gateway entry point — any child inheriting fd 0 can flip # the flag and launder EAGAIN into an apparent EOF). _sw_recovery_times: list[float] = [] - _SW_MAX_RECOVERIES_PER_MINUTE = 10 - def _sw_diagnose_stdin() -> str: - parts: list[str] = [] - try: - flags = fcntl.fcntl(0, fcntl.F_GETFL) - parts.append(f"O_NONBLOCK={'1' if flags & os.O_NONBLOCK else '0'}") - except Exception as e: - parts.append(f"F_GETFL error: {e}") - try: - s = socket.fromfd(0, socket.AF_UNIX, socket.SOCK_STREAM) - try: - tv = s.getsockopt(socket.SOL_SOCKET, socket.SO_RCVTIMEO) - parts.append(f"SO_RCVTIMEO={tv}") - finally: - s.detach() - except Exception: - pass - return ", ".join(parts) if parts else "unknown" + def _sw_log(reason: str) -> None: + print(f"[slash-worker] {reason}", file=sys.stderr, flush=True) while True: raw = sys.stdin.readline() if not raw: - try: - flags = fcntl.fcntl(0, fcntl.F_GETFL) - is_nonblock = bool(flags & os.O_NONBLOCK) - except Exception: - is_nonblock = False - - if not is_nonblock: - # Genuine peer-close + if not handle_spurious_eof(_sw_recovery_times, _sw_log): break - - # Spurious EOF — recover - now = time.time() - _sw_recovery_times.append(now) - _sw_recovery_times[:] = [t for t in _sw_recovery_times if t > now - 60] - if len(_sw_recovery_times) > _SW_MAX_RECOVERIES_PER_MINUTE: - print( - f"[slash-worker] stdin spurious-EOF recovery rate exceeded " - f"({len(_sw_recovery_times)}/min)", - file=sys.stderr, - flush=True, - ) - break - - diag = _sw_diagnose_stdin() - print( - f"[slash-worker] stdin spurious EOF (subprocess O_NONBLOCK flip), " - f"recovering: {diag}", - file=sys.stderr, - flush=True, - ) - os.set_blocking(0, True) continue line = raw.strip()