From 54e608bcad867eadb189fece0f51e2ea4101d3e6 Mon Sep 17 00:00:00 2001 From: Will Bradley Date: Fri, 10 Jul 2026 00:44:00 -0600 Subject: [PATCH 1/3] fix(code): reap langgraph dev server when startup is cancelled The langgraph dev server starts in a background worker so the TUI paints immediately. `ServerProcess.start()` spawns the subprocess before returning, but the app only records the `ServerProcess` on success. If the user quits (Ctrl+D) before the health check completes, the worker is cancelled mid-`start()`. `start_server_and_get_agent` guarded its cleanup with `except Exception`, which does not catch `asyncio.CancelledError` (a `BaseException`), so `server.stop()` was skipped and the `langgraph dev` subprocess was orphaned. Catch `BaseException` so a cancelled startup also stops the half-started server, then re-raise to preserve cancellation/shutdown semantics. --- .../client/launch/server_manager.py | 2 +- .../tests/unit_tests/test_server_manager.py | 56 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/libs/code/deepagents_code/client/launch/server_manager.py b/libs/code/deepagents_code/client/launch/server_manager.py index 136ddea279..0109cfc620 100644 --- a/libs/code/deepagents_code/client/launch/server_manager.py +++ b/libs/code/deepagents_code/client/launch/server_manager.py @@ -404,7 +404,7 @@ async def start_server_and_get_agent( try: await server.start() await server.wait_for_graph_ready("agent") - except Exception: + except BaseException: server.stop() raise diff --git a/libs/code/tests/unit_tests/test_server_manager.py b/libs/code/tests/unit_tests/test_server_manager.py index 53815588a7..5dcfac3c83 100644 --- a/libs/code/tests/unit_tests/test_server_manager.py +++ b/libs/code/tests/unit_tests/test_server_manager.py @@ -2,6 +2,7 @@ from __future__ import annotations +import asyncio import os from typing import TYPE_CHECKING from unittest.mock import AsyncMock, MagicMock, patch @@ -279,6 +280,61 @@ async def test_stops_server_when_graph_readiness_fails( mock_server.stop.assert_called_once() mock_agent.assert_not_called() + async def test_stops_server_when_start_cancelled( + self, tmp_path: Path, monkeypatch + ) -> None: + """A quit during startup must still reap the half-started server. + + The langgraph subprocess is spawned inside `ServerProcess.start()` + before this function returns, so the caller has not yet stored a + reference to it (`DeepAgentsApp._server_proc` is assigned only on + successful return). When the background startup worker is cancelled + mid-`start()` — e.g. the user presses Ctrl+D before the health check + completes — this `except` clause is the only thing that can stop the + orphaned subprocess. `asyncio.CancelledError` is a `BaseException`, not + an `Exception`, so an `except Exception` guard would leak the process + (see the `wbbradley/process-leak` regression). + """ + project_root = tmp_path / "project" + project_root.mkdir() + monkeypatch.chdir(project_root) + + work_dir = tmp_path / "runtime" + work_dir.mkdir() + + mock_server = MagicMock() + mock_server.start = AsyncMock(side_effect=asyncio.CancelledError) + mock_server.wait_for_graph_ready = AsyncMock() + mock_server.stop = MagicMock() + mock_server.url = "http://127.0.0.1:2024" + + with ( + patch.dict(os.environ, {}, clear=False), + patch( + "deepagents_code.client.launch.server_manager.tempfile.mkdtemp", + return_value=str(work_dir), + ), + patch("deepagents_code.client.launch.server_manager._write_checkpointer"), + patch("deepagents_code.client.launch.server_manager._write_pyproject"), + patch( + "deepagents_code.client.launch.server.ServerProcess", + return_value=mock_server, + ), + patch("deepagents_code.client.remote_client.RemoteAgent") as mock_agent, + pytest.raises(asyncio.CancelledError), + ): + await start_server_and_get_agent( + assistant_id="agent", + mcp_config_path=None, + ) + + mock_server.start.assert_awaited_once() + # The cancellation must propagate: graph readiness is never reached, and + # no client is handed back to a caller that is being torn down. + mock_server.wait_for_graph_ready.assert_not_awaited() + mock_server.stop.assert_called_once() + mock_agent.assert_not_called() + def test_relative_paths_written_verbatim_to_langgraph_json( self, tmp_path: Path ) -> None: From ed4af5adffca7f425d359afa55d46ff864b0b9ec Mon Sep 17 00:00:00 2001 From: Mason Daugherty Date: Fri, 10 Jul 2026 10:55:54 -0400 Subject: [PATCH 2/3] cr --- .../client/launch/server_manager.py | 11 ++- .../tests/unit_tests/test_server_manager.py | 70 ++++++++++++++++--- 2 files changed, 70 insertions(+), 11 deletions(-) diff --git a/libs/code/deepagents_code/client/launch/server_manager.py b/libs/code/deepagents_code/client/launch/server_manager.py index 0109cfc620..0b7c9e8d23 100644 --- a/libs/code/deepagents_code/client/launch/server_manager.py +++ b/libs/code/deepagents_code/client/launch/server_manager.py @@ -405,7 +405,16 @@ async def start_server_and_get_agent( await server.start() await server.wait_for_graph_ready("agent") except BaseException: - server.stop() + # A cancelled/interrupted startup must still reap the subprocess that + # `server.start()` already spawned. Guard the cleanup so an error from + # `stop()` cannot replace the in-flight `BaseException` (e.g. downgrade a + # `CancelledError` to a plain error and break cancellation semantics). + try: + server.stop() + except Exception: + logger.warning( + "Error stopping server during startup cleanup", exc_info=True + ) raise agent = RemoteAgent( diff --git a/libs/code/tests/unit_tests/test_server_manager.py b/libs/code/tests/unit_tests/test_server_manager.py index 5dcfac3c83..06fae5ca98 100644 --- a/libs/code/tests/unit_tests/test_server_manager.py +++ b/libs/code/tests/unit_tests/test_server_manager.py @@ -280,20 +280,24 @@ async def test_stops_server_when_graph_readiness_fails( mock_server.stop.assert_called_once() mock_agent.assert_not_called() - async def test_stops_server_when_start_cancelled( - self, tmp_path: Path, monkeypatch + @pytest.mark.parametrize( + "interrupt", + [asyncio.CancelledError, KeyboardInterrupt, SystemExit], + ) + async def test_stops_server_when_start_interrupted( + self, interrupt: type[BaseException], tmp_path: Path, monkeypatch ) -> None: """A quit during startup must still reap the half-started server. The langgraph subprocess is spawned inside `ServerProcess.start()` before this function returns, so the caller has not yet stored a reference to it (`DeepAgentsApp._server_proc` is assigned only on - successful return). When the background startup worker is cancelled + successful return). When the background startup worker is interrupted mid-`start()` — e.g. the user presses Ctrl+D before the health check completes — this `except` clause is the only thing that can stop the - orphaned subprocess. `asyncio.CancelledError` is a `BaseException`, not - an `Exception`, so an `except Exception` guard would leak the process - (see the `wbbradley/process-leak` regression). + orphaned subprocess. The interrupts covered here are all `BaseException` + subclasses rather than `Exception`, so an `except Exception` guard would + leak the process (regression: PR #4629). """ project_root = tmp_path / "project" project_root.mkdir() @@ -303,7 +307,7 @@ async def test_stops_server_when_start_cancelled( work_dir.mkdir() mock_server = MagicMock() - mock_server.start = AsyncMock(side_effect=asyncio.CancelledError) + mock_server.start = AsyncMock(side_effect=interrupt) mock_server.wait_for_graph_ready = AsyncMock() mock_server.stop = MagicMock() mock_server.url = "http://127.0.0.1:2024" @@ -321,7 +325,7 @@ async def test_stops_server_when_start_cancelled( return_value=mock_server, ), patch("deepagents_code.client.remote_client.RemoteAgent") as mock_agent, - pytest.raises(asyncio.CancelledError), + pytest.raises(interrupt), ): await start_server_and_get_agent( assistant_id="agent", @@ -329,12 +333,58 @@ async def test_stops_server_when_start_cancelled( ) mock_server.start.assert_awaited_once() - # The cancellation must propagate: graph readiness is never reached, and + mock_server.stop.assert_called_once() + # The interrupt must propagate: graph readiness is never reached, and # no client is handed back to a caller that is being torn down. mock_server.wait_for_graph_ready.assert_not_awaited() - mock_server.stop.assert_called_once() mock_agent.assert_not_called() + async def test_start_cleanup_error_does_not_mask_interrupt( + self, tmp_path: Path, monkeypatch + ) -> None: + """A failure inside `stop()` must not replace the in-flight interrupt. + + Cleanup runs while a `BaseException` (here `CancelledError`) is + propagating. If `stop()` itself raises, that error is swallowed and + logged so the original cancellation stays the propagated exception, + preserving cancellation semantics instead of surfacing the teardown + error (regression: PR #4629). + """ + project_root = tmp_path / "project" + project_root.mkdir() + monkeypatch.chdir(project_root) + + work_dir = tmp_path / "runtime" + work_dir.mkdir() + + mock_server = MagicMock() + mock_server.start = AsyncMock(side_effect=asyncio.CancelledError) + mock_server.wait_for_graph_ready = AsyncMock() + mock_server.stop = MagicMock(side_effect=RuntimeError("kill failed")) + mock_server.url = "http://127.0.0.1:2024" + + with ( + patch.dict(os.environ, {}, clear=False), + patch( + "deepagents_code.client.launch.server_manager.tempfile.mkdtemp", + return_value=str(work_dir), + ), + patch("deepagents_code.client.launch.server_manager._write_checkpointer"), + patch("deepagents_code.client.launch.server_manager._write_pyproject"), + patch( + "deepagents_code.client.launch.server.ServerProcess", + return_value=mock_server, + ), + patch("deepagents_code.client.remote_client.RemoteAgent"), + pytest.raises(asyncio.CancelledError), + ): + await start_server_and_get_agent( + assistant_id="agent", + mcp_config_path=None, + ) + + mock_server.stop.assert_called_once() + def test_relative_paths_written_verbatim_to_langgraph_json( self, tmp_path: Path ) -> None: From 3568e1945031f5fe689e4556e7043e87dd7798a6 Mon Sep 17 00:00:00 2001 From: Mason Daugherty Date: Fri, 10 Jul 2026 11:24:30 -0400 Subject: [PATCH 3/3] cr --- .../deepagents_code/client/launch/server.py | 39 +++++- .../client/launch/server_manager.py | 44 +++--- libs/code/tests/unit_tests/test_server.py | 126 +++++++++++++++++- 3 files changed, 184 insertions(+), 25 deletions(-) diff --git a/libs/code/deepagents_code/client/launch/server.py b/libs/code/deepagents_code/client/launch/server.py index dfea201c53..ea9bef8665 100644 --- a/libs/code/deepagents_code/client/launch/server.py +++ b/libs/code/deepagents_code/client/launch/server.py @@ -528,6 +528,7 @@ async def start( stderr=subprocess.STDOUT, ) + started = False try: await wait_for_server_healthy( self.url, @@ -536,9 +537,24 @@ async def start( read_log=self._read_log_file, local=True, ) - except Exception: - self.stop() - raise + started = True + finally: + if not started: + # Reap the subprocess we just spawned if startup did not + # complete — including cancellation (e.g. Ctrl+D / SIGINT before + # the health check returns). A `finally` rather than `except + # Exception` is deliberate: `asyncio.CancelledError` is a + # `BaseException`, so an `except Exception` guard would skip this + # and orphan the process. The inner guard stops a `stop()` error + # from masking the exception already propagating; `stop()` is + # effectively non-raising today, so if it does fire it signals an + # unexpected leak — hence `error`, not `warning`. + try: + self.stop() + except Exception: + logger.exception( + "Error stopping server during startup cleanup", + ) async def wait_for_graph_ready( self, @@ -634,14 +650,29 @@ def _stop_process(self) -> None: self._process.wait(timeout=_SHUTDOWN_TIMEOUT) except subprocess.TimeoutExpired: logger.warning("Server did not stop gracefully, killing") - self._process.kill() + # `kill()` lives in this handler, so a raise here would escape + # the sibling `except OSError` below. Guard it explicitly: + # `ProcessLookupError` just means the process already exited + # (benign — nothing left to reap), while any other `OSError` + # means SIGKILL failed and the process is likely orphaned. try: + self._process.kill() self._process.wait(timeout=2) except subprocess.TimeoutExpired: logger.warning( "Server process pid=%d did not exit after SIGKILL", self._process.pid, ) + except ProcessLookupError: + logger.debug( + "Server process pid=%d already exited before SIGKILL", + self._process.pid, + ) + except OSError: + logger.exception( + "Failed to SIGKILL server process pid=%d; it may be orphaned", + self._process.pid, + ) except OSError: logger.warning("Error stopping server", exc_info=True) diff --git a/libs/code/deepagents_code/client/launch/server_manager.py b/libs/code/deepagents_code/client/launch/server_manager.py index 0b7c9e8d23..cfc79a65c6 100644 --- a/libs/code/deepagents_code/client/launch/server_manager.py +++ b/libs/code/deepagents_code/client/launch/server_manager.py @@ -401,28 +401,34 @@ async def start_server_and_get_agent( owns_config_dir=True, scaffold=_scaffold_workspace, ) + started = False try: await server.start() await server.wait_for_graph_ready("agent") - except BaseException: - # A cancelled/interrupted startup must still reap the subprocess that - # `server.start()` already spawned. Guard the cleanup so an error from - # `stop()` cannot replace the in-flight `BaseException` (e.g. downgrade a - # `CancelledError` to a plain error and break cancellation semantics). - try: - server.stop() - except Exception: - logger.warning( - "Error stopping server during startup cleanup", exc_info=True - ) - raise - - agent = RemoteAgent( - url=server.url, - graph_name="agent", - ) - - return agent, server, None + agent = RemoteAgent( + url=server.url, + graph_name="agent", + ) + started = True + return agent, server, None + finally: + if not started: + # Startup failed or was cancelled before the server was handed off + # to the caller (which records the reference only on success). If + # `start()` itself failed it already reaped its own subprocess, so + # this `stop()` is then an idempotent no-op; this cleanup is the sole + # reaper only when `start()` succeeded but `wait_for_graph_ready()` + # (or `RemoteAgent()`) failed afterward. A `finally` rather than + # `except Exception` is deliberate: `asyncio.CancelledError` is a + # `BaseException`, so an `except Exception` guard would skip cleanup + # and orphan the process. The inner guard stops a `stop()` error + # from masking the exception already propagating. + try: + server.stop() + except Exception: + logger.exception( + "Error stopping server during startup cleanup", + ) # ------------------------------------------------------------------ diff --git a/libs/code/tests/unit_tests/test_server.py b/libs/code/tests/unit_tests/test_server.py index 68dbd3f457..93e9b9eede 100644 --- a/libs/code/tests/unit_tests/test_server.py +++ b/libs/code/tests/unit_tests/test_server.py @@ -2,7 +2,10 @@ from __future__ import annotations +import asyncio +import logging import os +import signal import socket import threading from types import SimpleNamespace @@ -416,9 +419,14 @@ async def test_wait_for_graph_ready_checks_logs_after_transport_error( await server.wait_for_graph_ready("agent") async def test_start_cleans_up_partial_state_on_health_failure( - self, tmp_path: Path + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """Failed startup should stop the process and remove owned resources.""" + # `_stop_process` preserves the log file when debug mode is on, which + # would defeat the `not log_path.exists()` assertion below; pin it off + # so an ambient `DEEPAGENTS_CODE_DEBUG` in the environment can't flake. + monkeypatch.delenv("DEEPAGENTS_CODE_DEBUG", raising=False) + config_dir = tmp_path / "runtime" config_dir.mkdir() (config_dir / "langgraph.json").write_text("{}") @@ -456,7 +464,7 @@ async def test_start_cleans_up_partial_state_on_health_failure( ): await server.start() - process.send_signal.assert_called_once() + process.send_signal.assert_called_once_with(signal.SIGTERM) process.wait.assert_called_once() log_file.close.assert_called_once() assert server._process is None @@ -464,6 +472,120 @@ async def test_start_cleans_up_partial_state_on_health_failure( assert not config_dir.exists() assert not log_path.exists() + async def test_start_cleans_up_partial_state_on_cancellation( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A cancelled startup must reap the subprocess it already spawned. + + `asyncio.CancelledError` is a `BaseException`, not an `Exception`, so + `start()` must clean up in a `finally` — otherwise the `langgraph dev` + subprocess spawned before the health check is orphaned when the caller + is cancelled mid-startup (e.g. Ctrl+D). Regression: PR #4629. + """ + # See sibling health-failure test: pin debug off so the log file is + # unlinked and the `not log_path.exists()` assertion can't flake. + monkeypatch.delenv("DEEPAGENTS_CODE_DEBUG", raising=False) + + config_dir = tmp_path / "runtime" + config_dir.mkdir() + (config_dir / "langgraph.json").write_text("{}") + + log_path = tmp_path / "server.log" + log_path.write_text("booting") + + process = MagicMock() + process.pid = 1234 + process.poll.return_value = None + + log_file = MagicMock() + log_file.name = str(log_path) + + server = ServerProcess(config_dir=config_dir, owns_config_dir=True) + + with ( + patch( + "deepagents_code.client.launch.server._find_free_port", + return_value=12345, + ), + patch( + "deepagents_code.client.launch.server.tempfile.NamedTemporaryFile", + return_value=log_file, + ), + patch( + "deepagents_code.client.launch.server.subprocess.Popen", + return_value=process, + ), + patch( + "deepagents_code.client.launch.server.wait_for_server_healthy", + new=AsyncMock(side_effect=asyncio.CancelledError), + ), + pytest.raises(asyncio.CancelledError), + ): + await server.start() + + process.send_signal.assert_called_once_with(signal.SIGTERM) + process.wait.assert_called_once() + log_file.close.assert_called_once() + assert server._process is None + assert server._log_file is None + assert not config_dir.exists() + assert not log_path.exists() + + async def test_start_cleanup_error_does_not_mask_startup_error( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """A failing `stop()` during cleanup must not mask the startup error. + + `start()`'s `finally` guards `stop()` so that if reaping the subprocess + itself raises, the in-flight startup exception still propagates + unchanged (rather than being replaced by the cleanup error) and the + failure is logged at `error` level. + """ + config_dir = tmp_path / "runtime" + config_dir.mkdir() + (config_dir / "langgraph.json").write_text("{}") + + log_path = tmp_path / "server.log" + log_path.write_text("booting") + + process = MagicMock() + process.pid = 1234 + process.poll.return_value = None + + log_file = MagicMock() + log_file.name = str(log_path) + + server = ServerProcess(config_dir=config_dir, owns_config_dir=True) + + with ( + patch( + "deepagents_code.client.launch.server._find_free_port", + return_value=12345, + ), + patch( + "deepagents_code.client.launch.server.tempfile.NamedTemporaryFile", + return_value=log_file, + ), + patch( + "deepagents_code.client.launch.server.subprocess.Popen", + return_value=process, + ), + patch( + "deepagents_code.client.launch.server.wait_for_server_healthy", + new=AsyncMock(side_effect=RuntimeError("startup boom")), + ), + patch.object( + server, "stop", side_effect=RuntimeError("cleanup boom") + ) as mock_stop, + caplog.at_level(logging.ERROR), + # The original startup error propagates, not the cleanup error. + pytest.raises(RuntimeError, match="startup boom"), + ): + await server.start() + + mock_stop.assert_called_once() + assert "Error stopping server during startup cleanup" in caplog.text + async def test_start_rescaffolds_when_config_missing(self, tmp_path: Path) -> None: """A missing langgraph.json should be rebuilt via the scaffold hook.""" config_dir = tmp_path / "runtime"