tools/terminal_tool.py: drop 30 pure re-export names (lifecycle/config/backends/
sudo/guards/result/interrupt/utils/_DockerEnvironment/is_managed_tool_gateway_ready)
and the noqa-F401 comments on the 25 names the facade itself uses. Sibling modules
(terminal_tool_backends/_result/_sudo/_lifecycle, environments/base, process_registry)
that read removed names through the facade now import from the defining module.
tools/environments/base.py: drop 11 re-exports (base_output/base_session_env/
path_utils) and the BaseEnvironment.stop() compat alias (no in-tree caller; the
lifecycle hasattr(env, 'stop') fallback stays for third-party envs).
tools/environments/docker.py: drop 1 re-export + the re-export comment.
Callers/tests repointed to tools.terminal_tool_{lifecycle,backends,sudo,config,
guards,result}, tools.interrupt, tools.environments.{base_output,base_session_env,
path_utils}.
Phase-2c finding on #101598: the empty-set release closed the pipe and
dropped the Popen without reaping it, so an idle gateway that once
connected a stdio server held one zombie until the next Popen in the
process. wait(timeout=5) after close -- the supervisor exits on EOF
immediately with nothing registered (E2E: ps shows no entry at all).
Also: the replay test asserted against the last line only, which could
not catch the regression it names; assert against the full stream and a
post-unregister replay. Module docstring now states the stdlib-only /
no tools/ import constraint and why _reap's sweep is a deliberate copy.
Contract requested in the #93517 review, now that main has profile-scoped
MCP ownership: two owners each hold a real stdio process group; a scoped
_kill_orphaned_mcp_children(include_active=True, server_name=A) reaps
only A's group, sends 'unregister' only for A, and the per-process
supervisor still holds B.
Addresses @andrexibiza's Blocker 1 on #93517 (reproduced): after a
broken-pipe write dropped the supervisor while groups were still
registered, the no-spawn fast path was keyed on the incoming verb
(`unregister` + no proc => return), so a clean teardown of one server
left the survivors recorded in _supervised_pgids but unsupervised until
the next register. Key the fast path on the supervised set being empty
instead; any later call respawns and replays the survivors.
Regression test: two live groups, write fails, unregister one ->
replacement receives `register` for the survivors. Mutation-checked
against the old verb-keyed guard.
Follow-up to the salvaged #93517. Two gaps found in review:
- After the last unregister the supervisor stayed resident for the life of
the process (~15 MB + a pipe) in any gateway/cron that ever connected a
stdio server; main's per-server watchdog exited with its server. Close
our write end when the supervised set empties: EOF with nothing
registered makes the supervisor exit without reaping, and the next
register already respawns and replays.
- A child that raced and exited before os.getpgid dropped its group from
coverage entirely. The SDK spawns stdio servers as session leaders
(pgid == pid), so fall back to the pid instead; the prune forgets the
group once nothing in it is alive.
Tests: fake-supervisor release/re-spawn sequence, and a real-process EOF
release (supervisor exits 0, unregistered child untouched). Mutation
checked: disabling the release branch fails both.
Review raised a real gap: we reap by pgid, so a registration is only as
meaningful as the group's identity. A group we deliberately keep registered --
an orphan teardown failed to kill, such as the `node` mcp-remote leaves behind
-- can later exit on its own, after which the kernel may hand that pgid to an
unrelated process owned by the same user. An ungraceful Hermes death while the
registration is stale would then signal a stranger. `_is_safe_target` cannot
catch it, because the value is stale rather than invalid.
Prune registrations whose group has no members left on every registration
change, and tell the supervisor to forget them. Signal 0 is a pure existence
question -- it cannot terminate anything -- so this is cheap and safe to run on
the hot path. An ambiguous answer (EPERM: exists but not ours) keeps the
registration, since dropping real coverage is the more expensive mistake.
This narrows the window rather than closing it: a group can still die and its
pgid be recycled between two probes. Closing it completely means proving group
identity at reap time, e.g. stamping children with a boot-unique env marker and
checking a member still carries it. That was judged not worth putting a `ps`
parse into the one process whose job is to stay simple enough to always work,
so the residual is now documented in the module docstring instead, along with
the note that Hermes's existing killpg-based orphan cleanup already carries the
same exposure (upstream #88350).
Also records why supervisor recovery is deliberately two-step: a failed write
drops the handle, and the next call rebuilds coverage from `_supervised_pgids`,
which -- not the pipe -- is the record of what needs reaping.
The protocol tests register synthetic pgids that were never real process
groups, so they now state that precondition through an `all_groups_alive`
fixture instead of depending on pid-space luck.
29 tests pass. Verified the change adds no failures: same selection, my changes
stashed vs applied, 42 failed either way (pre-existing in a locally rebuilt
venv) with passed going 617 -> 619 for the two new tests. Footgun linter clean.
(cherry picked from commit 3a7d219620a8cb57a7fe2c2346ce3f055320e743)
Every stdio MCP server was wrapped in its own CPython watchdog that polled
getppid() every 2s to notice an ungraceful Hermes exit (kill -9, OOM, crash,
force-quit), since macOS has no PR_SET_PDEATHSIG. That is a whole interpreter
per server for a job that does nothing until the moment Hermes dies: 10.1 MB
physical footprint each, measured on macOS/arm64.
Replace the fleet of pollers with a single supervisor per Hermes process
holding the read end of a pipe only Hermes writes to. Death detection becomes
EOF on that pipe -- exact and instant, rather than up to a poll interval late.
Hermes sends `register <pgid>` / `unregister <pgid>` as servers come and go;
on EOF the supervisor killpg's whatever is still registered, which is exactly
the set whose teardown never ran. A clean shutdown unregisters as it goes, so
EOF then finds nothing to kill.
Servers are now spawned unwrapped. The MCP SDK already starts each stdio child
in its own session, so the pgid recorded for killpg is the server's own group
and the existing cleanup paths reach it unchanged. That also deletes the
signal-forwarding layer the wrapper needed: wrapping had put the real server in
a different session from the pgid being tracked, so a graceful killpg would
have hit only the wrapper.
Measured on a 5-gateway host: 10 watchdogs (~98 MB) -> 5 supervisors (~49 MB).
One supervisor costs about what one watchdog did (9.9 vs 10.1 MB), so the win
is (servers_per_process - 1) x ~10 MB, and a process with no stdio servers now
spawns nothing at all.
The supervisor reads length-capped lines rather than iterating the stream: a
writer that never sends a newline would otherwise grow it without bound, which
it must not be vulnerable to when it is the last defense against leaked
servers. Found by feeding it /dev/zero, where it reached 15 GB.
Verified beyond unit coverage: a real stdio MCP server connects and its tools
are discovered on the unwrapped path; with a live Hermes holding a real
connection, kill -9 reaped the server, its grandchild (in the server's group,
the mcp-remote `node` case), and the supervisor exited on its own. The reap
tests were sabotage-checked in both directions -- a no-op reaper fails all
three, while the test pinning that a cleanly unregistered server survives keeps
passing -- and each wiring half fails independently when removed.
(cherry picked from commit a252d4ce7ff1722f687635fdbf0cff79f538c3f1)
Teknium