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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user