From 848129c84fd1499a62ff2c621baff18f5726c705 Mon Sep 17 00:00:00 2001 From: bandzalkin Date: Wed, 21 Oct 2026 20:43:39 +0300 Subject: [PATCH 1/2] fix(codex): let the Codex session exit on stdin EOF before killing its tree close() killed the child's session with SIGKILL before closing stdin, so Codex never saw the EOF it exits on and could do nothing at shutdown. Stdin now closes first and the session gets one second to end on its own; the tree kill remains the fallback. The session id stays reserved while any member lives, so a launcher shim's real CLI child that keeps the room directory is still reaped. --- src/band/integrations/codex/stdio_client.py | 32 ++++++++++++++++++- tests/integrations/codex/test_cleanup.py | 6 ++-- tests/integrations/codex/test_stdio_client.py | 14 ++++++++ 3 files changed, 48 insertions(+), 4 deletions(-) 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..70000a828 100644 --- a/tests/integrations/codex/test_cleanup.py +++ b/tests/integrations/codex/test_cleanup.py @@ -223,8 +223,8 @@ 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.""" + """A launcher gets to exit on stdin EOF, but its child still holding + the workdir is reaped by the tree kill that follows.""" workdir = tmp_path / "room" workdir.mkdir() client = CodexStdioClient( @@ -237,7 +237,7 @@ 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() + 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 From 292c805b46ddc9049c5a2831db325116ec3529a7 Mon Sep 17 00:00:00 2001 From: bandzalkin Date: Thu, 22 Oct 2026 13:31:06 +0300 Subject: [PATCH 2/2] fix: respect platform EOF contracts in Codex cleanup tests --- docs/adapters/codex.md | 3 +++ tests/integrations/codex/test_cleanup.py | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) 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/tests/integrations/codex/test_cleanup.py b/tests/integrations/codex/test_cleanup.py index 70000a828..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 gets to exit on stdin EOF, but its child still holding - the workdir is reaped by the tree kill that follows.""" + """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 (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)