diff --git a/docs/adapters/codex.md b/docs/adapters/codex.md index 41d6e5ce8..5f12b25ab 100644 --- a/docs/adapters/codex.md +++ b/docs/adapters/codex.md @@ -35,6 +35,9 @@ For existing integrations, follow the [SDK 4.x to 5.x migration](../acp.md#upgra close errors and close timeouts retain the claim for retry. A timed-out or cancelled caller does not abandon subprocess cleanup; retry room cleanup to finish releasing ownership. Shutdown attempts every room and reports failures. + On POSIX, close sends stdin EOF and gives the session a bounded chance to + exit before reaping remaining descendants. Windows shutdown does not promise + session-visible stdin EOF. On Windows, the client owns the helper and its process-tree job from startup through cleanup. It starts the interpreter image directly so a virtual-environment launcher cannot hide another helper process. Cleanup confirms the helper has diff --git a/src/band/integrations/codex/stdio_client.py b/src/band/integrations/codex/stdio_client.py index c87961e5a..f5530b87a 100644 --- a/src/band/integrations/codex/stdio_client.py +++ b/src/band/integrations/codex/stdio_client.py @@ -30,6 +30,9 @@ # Long enough for the child watcher to record an exit, short enough that a # held stdio pipe cannot stall room cleanup. _EXIT_OBSERVE_S = 2.0 +# How long a closed stdin gets to end the whole Codex session before it is killed. +_SESSION_EXIT_S = 1.0 +_SESSION_POLL_S = 0.02 _STDOUT_LIMIT = 16 * 1024 * 1024 HELPER_PATH = Path(__file__).resolve().with_name("winmain.py") @@ -112,6 +115,30 @@ async def kill_process_tree(process: asyncio.subprocess.Process) -> None: return +async def _await_session_exit( + process: asyncio.subprocess.Process, timeout: float +) -> bool: + """True once the child and every other process in its session are gone. + + The session id stays reserved while any member lives, so probing the + group is exact; the launcher alone exiting is not enough, since its + real CLI child would keep the room's working directory. + """ + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout + while True: + try: + os.killpg(process.pid, 0) + except ProcessLookupError: + return True + except PermissionError: + pass + remaining = deadline - loop.time() + if remaining <= 0: + return False + await asyncio.sleep(min(_SESSION_POLL_S, remaining)) + + class CodexStdioClient(BaseJsonRpcClient): """Bidirectional JSON-RPC client with a single stdout read loop.""" @@ -258,8 +285,11 @@ async def _close_process(self) -> None: await self._windows_process.close(_EXIT_OBSERVE_S) self._windows_process = None elif (process := self._proc) is not None: - await kill_process_tree(process) + # Codex and its launcher exit on their own once stdin closes; the + # tree kill is the fallback for a session that does not. await close_stdin(process) + if not await _await_session_exit(process, _SESSION_EXIT_S): + await kill_process_tree(process) if not await await_returncode(process, _EXIT_OBSERVE_S): logger.warning( "Codex process %s still alive after tree kill", process.pid diff --git a/tests/integrations/codex/test_cleanup.py b/tests/integrations/codex/test_cleanup.py index b382d7644..6765799cb 100644 --- a/tests/integrations/codex/test_cleanup.py +++ b/tests/integrations/codex/test_cleanup.py @@ -223,8 +223,7 @@ async def test_forced_close_stops_what_a_launcher_shim_spawned( async def test_close_reaps_a_worker_when_its_launcher_exits_on_stdin_eof( self, tmp_path: Path ) -> None: - """A launcher that exits on stdin EOF would otherwise leave its child - holding the workdir.""" + """POSIX launchers observe stdin EOF; every platform reaps their worker.""" workdir = tmp_path / "room" workdir.mkdir() client = CodexStdioClient( @@ -237,7 +236,8 @@ async def test_close_reaps_a_worker_when_its_launcher_exits_on_stdin_eof( await asyncio.wait_for(wait_for_heartbeat(heartbeat), timeout=CLOSE_BOUND_S) await asyncio.wait_for(client.close(), timeout=CLOSE_BOUND_S) await assert_heartbeat_stopped(heartbeat) - assert not (workdir / STDIN_EOF_MARKER).exists() + if sys.platform != "win32": + assert (workdir / STDIN_EOF_MARKER).exists() shutil.rmtree(workdir) finally: await asyncio.wait_for(client.close(), timeout=CLOSE_BOUND_S) diff --git a/tests/integrations/codex/test_stdio_client.py b/tests/integrations/codex/test_stdio_client.py index ff255cc58..332f425b1 100644 --- a/tests/integrations/codex/test_stdio_client.py +++ b/tests/integrations/codex/test_stdio_client.py @@ -408,3 +408,17 @@ async def test_stdio_client_eof_fails_pending_and_enqueues_disconnect( event = await client.recv_event(timeout_s=1.0) assert event.method == "transport/closed" await client.close() + + +@pytest.mark.asyncio +@pytest.mark.skipif(os.name == "nt", reason="Windows closes through the job object") +async def test_close_lets_codex_exit_on_stdin_eof_before_killing_its_tree( + fake_codex_server_script: Path, +) -> None: + """A harness that exits once its stdin closes ends on its own terms; the + tree kill is the fallback for one that does not.""" + client = await _build_client(fake_codex_server_script, scenario="basic") + process = client._proc + assert process is not None + await client.close() + assert process.returncode == 0