From c9e014fdeaa62f5df428d8bf502acfffd3c20704 Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Tue, 7 Jul 2026 15:20:07 +0000 Subject: [PATCH] fix(shell): reap zombie processes on all subprocess exit paths The previous fix (dbcc7cb5) only added os.waitpid() to _kill_process(), covering the timeout/cancel path of one-shot exec. Zombies continued to accumulate because several other exit paths never reaped children: - _ExecSession.kill(): sent SIGKILL + process.wait(5s) but had no os.waitpid() fallback if the wait timed out - ExecTool.execute() generic exception handler: leaked the subprocess if communicate() raised an unexpected error - Normal completion paths: relied entirely on asyncio's child-watcher, which can miss exits inside Docker containers (pidfd/SIGCHLD gaps) Changes: - Extract _reap_pid() helper for consistent, safe os.waitpid(WNOHANG) - Add _reap_pid() fallback to _ExecSession.kill() via try/finally - Add _reap_pid() safety-net after normal process exit in both ExecTool.execute() and _ExecSession.poll() - Kill + reap subprocess in the generic except Exception handler - Add periodic zombie reaper background task (every 30s) in the gateway as a last line of defense --- nanobot/agent/tools/exec_session.py | 14 ++++++++++-- nanobot/agent/tools/shell.py | 34 ++++++++++++++++++++++++----- nanobot/cli/commands.py | 29 ++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/nanobot/agent/tools/exec_session.py b/nanobot/agent/tools/exec_session.py index 6a38d30b..f67f35bb 100644 --- a/nanobot/agent/tools/exec_session.py +++ b/nanobot/agent/tools/exec_session.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import os import time import uuid from contextlib import suppress @@ -148,6 +149,9 @@ class _ExecSession: asyncio.gather(self._stdout_task, self._stderr_task), timeout=2.0, ) + # Safety-net reap after normal exit. + from nanobot.agent.tools.shell import _reap_pid + _reap_pid(self.process.pid) elif yield_time_ms > 0: await self._wait_for_buffered_output() @@ -171,8 +175,14 @@ class _ExecSession: if self.process.returncode is not None: return self.process.kill() - with suppress(asyncio.TimeoutError): - await asyncio.wait_for(self.process.wait(), timeout=5.0) + try: + with suppress(asyncio.TimeoutError): + await asyncio.wait_for(self.process.wait(), timeout=5.0) + finally: + # Safety-net waitpid — prevent zombie if asyncio's child watcher + # did not reap the process (common in containers). + from nanobot.agent.tools.shell import _reap_pid + _reap_pid(self.process.pid) async def _wait_for_buffered_output(self) -> None: deadline = time.monotonic() + OUTPUT_DRAIN_GRACE_S diff --git a/nanobot/agent/tools/shell.py b/nanobot/agent/tools/shell.py index 6c495b71..81f0f2c4 100644 --- a/nanobot/agent/tools/shell.py +++ b/nanobot/agent/tools/shell.py @@ -41,6 +41,24 @@ from nanobot.security.workspace_policy import is_path_within _IS_WINDOWS = sys.platform == "win32" +def _reap_pid(pid: int) -> None: + """Best-effort ``waitpid`` to reap a child and prevent zombies. + + Call this after killing or after normal completion of any subprocess + as a safety net — asyncio's child-watcher *should* have reaped it, + but in containers / edge-cases it sometimes doesn't. + """ + if _IS_WINDOWS: + return + try: + os.waitpid(pid, os.WNOHANG) + except (ProcessLookupError, ChildProcessError): + # Already reaped, or not our child — both are fine. + pass + except OSError as exc: + logger.debug("_reap_pid({}): {}", pid, exc) + + # Policy note appended to recoverable workspace-boundary guard errors. _WORKSPACE_BOUNDARY_NOTE = ( "\n\nNote: this is a hard policy boundary, not a transient failure. " @@ -283,6 +301,7 @@ class ExecTool(Tool): if yield_time_ms is not None: return await self._execute_session(prepared, yield_time_ms, max_output_chars) + process: asyncio.subprocess.Process | None = None try: process = await self._spawn( prepared.command, @@ -304,6 +323,11 @@ class ExecTool(Tool): await self._kill_process(process) raise + # Safety-net reap: asyncio *should* have reaped the child via + # communicate(), but in containers the child-watcher sometimes + # misses it, leaving a zombie. + _reap_pid(process.pid) + output_parts = [] if stdout: @@ -330,6 +354,10 @@ class ExecTool(Tool): return result except Exception as e: + # Kill and reap the child if it was spawned but an unexpected + # error prevented communicate() from completing. + if process is not None: + await self._kill_process(process) return ToolResult.error(f"Error executing command: {str(e)}") async def _execute_session( @@ -604,11 +632,7 @@ class ExecTool(Tool): with suppress(asyncio.TimeoutError): await asyncio.wait_for(process.wait(), timeout=5.0) finally: - if not _IS_WINDOWS: - try: - os.waitpid(process.pid, os.WNOHANG) - except (ProcessLookupError, ChildProcessError) as e: - logger.debug("Process already reaped or not found: {}", e) + _reap_pid(process.pid) def _build_env(self) -> dict[str, str]: """Build a minimal environment for subprocess execution. diff --git a/nanobot/cli/commands.py b/nanobot/cli/commands.py index 3013a8f9..846ae36a 100644 --- a/nanobot/cli/commands.py +++ b/nanobot/cli/commands.py @@ -201,6 +201,7 @@ app = typer.Typer( ) console = Console() +_IS_WINDOWS = sys.platform == "win32" EXIT_COMMANDS = {"exit", "quit", "/exit", "/quit", ":q"} _REASONING_SENTENCE_ENDINGS = (".", "!", "?", "。", "！", "？") _REASONING_FLUSH_CHARS = 60 @@ -1695,6 +1696,29 @@ def _run_gateway( else: console.print("[yellow]✗[/yellow] Heartbeat: disabled") + async def _zombie_reaper() -> None: + """Periodically reap zombie child processes. + + asyncio's child-watcher *should* reap all children, but inside + Docker containers the pidfd / SIGCHLD mechanism can miss exits. + This task runs every 30 s and calls ``os.waitpid(-1, WNOHANG)`` + in a loop to collect any zombies that slipped through. + """ + _INTERVAL = 30 + while True: + await asyncio.sleep(_INTERVAL) + reaped = 0 + while True: + try: + pid, _ = os.waitpid(-1, os.WNOHANG) + if pid == 0: + break # no more zombie children + reaped += 1 + except ChildProcessError: + break # no child processes at all + if reaped: + logger.info("Zombie reaper: reaped {} defunct child process(es)", reaped) + async def _health_server(host: str, health_port: int): """Lightweight HTTP health endpoint on the gateway port.""" import json as _json @@ -1820,6 +1844,11 @@ def _run_gateway( name="nanobot-local-triggers", ), ] + if not _IS_WINDOWS: + tasks.append(asyncio.create_task( + _zombie_reaper(), + name="nanobot-zombie-reaper", + )) if health_server_enabled: tasks.append(asyncio.create_task( _health_server(config.gateway.host, port),