mirrors/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-07-21 16:18:55 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 7

fix: Windows guard, dedup recovery, profile-safe paths, clear SO_RCVTIMEO

Browse source 
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.
This commit is contained in:
kshitijk4poor 2026-07-20 12:44:10 +05:30 • committed by kshitij
parent 01c02c6f8f
commit 113d9f63b5
4 changed files with 182 additions and 126 deletions
Download patch file Download diff file Expand all files Collapse all files

33
scripts/check_subprocess_stdin.py
View file

@ -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

151
tui_gateway/_stdin_recovery.py Normal file
View file

@ -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

70
tui_gateway/entry.py
View file

@ -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()

54
tui_gateway/slash_worker.py
View file

@ -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()