From cda321e3184baf91a2b295591b539516a1dd3dfd Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Mon, 6 Apr 2026 16:01:35 +0200 Subject: [PATCH] Fix manual shutdown triggered by GUI tool/dashboard not cleaning everything up. Subprocesses were not cleaned up, in particular. * Rename SerenaAgent.shutdown to on_shutdown, because it is really a shutdown handler; it did not perform a shutdown. * Add new method shutdown which is now called from both sites (and which performs the full shutdown) * Manage the process instance used for the dashboard viewer and shut it down explicitly. * Initialise all variables used during shutdown immeditaly (avoiding errors when shutdown is triggered during init) --- CHANGELOG.md | 1 + scripts/demo_progressive_tool_shortening.py | 4 +- src/serena/agent.py | 46 +++++++++++++++------ src/serena/dashboard.py | 14 +------ src/serena/gui_log_viewer.py | 10 ++--- src/serena/mcp.py | 2 +- test/serena/config/test_serena_config.py | 12 +++--- test/serena/test_dashboard.py | 8 +--- test/serena/test_serena_agent.py | 2 +- 9 files changed, 51 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44e37a72..9601b47c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Status of the `main` branch. Changes prior to the next official version change w * Fixes: - Fix reactivation of the same project restarting language servers #1280 - Fix git commit id in version + - Fix manual shutdown triggered by GUI tool/dashboard not cleaning everything up. # 1.0.0 diff --git a/scripts/demo_progressive_tool_shortening.py b/scripts/demo_progressive_tool_shortening.py index f54d1946..3786021f 100644 --- a/scripts/demo_progressive_tool_shortening.py +++ b/scripts/demo_progressive_tool_shortening.py @@ -178,7 +178,7 @@ if __name__ == "__main__": run_lsp_tools(lsp_agent) run_backend_independent_tools(lsp_agent) finally: - lsp_agent.shutdown() + lsp_agent.on_shutdown() # JetBrains backend (requires a running IDE) try: @@ -186,6 +186,6 @@ if __name__ == "__main__": try: run_jb_tools(jb_agent) finally: - jb_agent.shutdown() + jb_agent.on_shutdown() except Exception as e: print(f"\nJetBrains backend not available, skipping: {e}") diff --git a/src/serena/agent.py b/src/serena/agent.py index 4461d682..31267135 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -6,6 +6,7 @@ import json import multiprocessing import os import platform +import signal import subprocess import sys from collections.abc import Callable, Iterator, Sequence @@ -269,6 +270,10 @@ class SerenaAgent: :param memory_log_handler: a MemoryLogHandler instance from which to read log messages; if None, a new one will be created if necessary. """ + self._active_project: Project | None = None + self._gui_log_viewer: Optional["GuiLogViewer"] = None + self._dashboard_viewer_process: multiprocessing.Process | None = None + self.version = serena_version() # obtain serena configuration using the decoupled factory function @@ -277,9 +282,6 @@ class SerenaAgent: # propagate configuration to other components self.serena_config.propagate_settings() - # project-specific instances, which will be initialized upon project activation - self._active_project: Project | None = None - # determine registered project to be activated (if any) registered_project_to_activate: RegisteredProject | None = ( self.serena_config.get_registered_project(project, autoregister=True) if project is not None else None @@ -302,7 +304,6 @@ class SerenaAgent: return memory_log_handler # open GUI log window if enabled - self._gui_log_viewer: Optional["GuiLogViewer"] = None if self.serena_config.gui_log_window: log.info("Opening GUI window") if platform.system() == "Darwin": @@ -312,7 +313,12 @@ class SerenaAgent: # which uv used as a base, unfortunately) from serena.gui_log_viewer import GuiLogViewer - self._gui_log_viewer = GuiLogViewer("dashboard", title="Serena Logs", memory_log_handler=get_memory_log_handler()) + self._gui_log_viewer = GuiLogViewer( + "dashboard", + title="Serena Logs", + memory_log_handler=get_memory_log_handler(), + shutdown_handler=lambda: self.shutdown(), + ) self._gui_log_viewer.start() else: log.debug("GUI window is disabled") @@ -579,8 +585,10 @@ class SerenaAgent: url = self.get_dashboard_url() assert url is not None if SerenaDashboardViewer.is_current_platform_supported(): - process = multiprocessing.Process(target=self._start_dashboard_viewer_process_function, args=(url, minimized), daemon=True) - process.start() + self._dashboard_viewer_process = multiprocessing.Process( + target=self._start_dashboard_viewer_process_function, args=(url, minimized), daemon=True + ) + self._dashboard_viewer_process.start() else: log.info("Not starting Serena dashboard viewer because the current platform does not support it; using browser-based fallback") if not minimized: @@ -952,23 +960,35 @@ class SerenaAgent: ToolRegistry().print_tool_overview(self._active_tools.tools) def __del__(self) -> None: - self.shutdown() + self.on_shutdown() - def shutdown(self, timeout: float = 2.0) -> None: + def on_shutdown(self, timeout: float = 2.0) -> None: """ - Shuts down the agent, freeing resources and stopping background tasks. + Shutdown handler of the agent, freeing resources and stopping background tasks. """ - # guard against __del__ being called on a partially constructed instance - if not hasattr(self, "_active_project"): - return log.info("SerenaAgent is shutting down ...") if self._active_project is not None: + log.info(f"Shutting down active project '{self._active_project.project_name}' ...") self._active_project.shutdown(timeout=timeout) self._active_project = None if self._gui_log_viewer: log.info("Stopping the GUI log window ...") self._gui_log_viewer.stop() self._gui_log_viewer = None + if self._dashboard_viewer_process: + log.info("Stopping the dashboard viewer process ...") + self._dashboard_viewer_process.terminate() + self._dashboard_viewer_process = None + + def shutdown(self) -> None: + """ + Triggers a hard shutdown of the agent, freeing resources and signalling the process to terminate + """ + # perform clean-up right away, because kill does not result in normal deletion of the object + self.on_shutdown() + + # signal process termination + os.kill(os.getpid(), signal.SIGTERM) def get_tool_by_name(self, tool_name: str) -> Tool: tool_class = ToolRegistry().get_tool_class_by_name(tool_name) diff --git a/src/serena/dashboard.py b/src/serena/dashboard.py index 76a32b65..11a9739d 100644 --- a/src/serena/dashboard.py +++ b/src/serena/dashboard.py @@ -2,7 +2,6 @@ import os import socket import sys import threading -from collections.abc import Callable from pathlib import Path from typing import TYPE_CHECKING, Any, Self @@ -136,13 +135,11 @@ class SerenaDashboardAPI: memory_log_handler: MemoryLogHandler, tool_names: list[str], agent: "SerenaAgent", - shutdown_callback: Callable[[], None] | None = None, tool_usage_stats: ToolUsageStats | None = None, ) -> None: self._memory_log_handler = memory_log_handler self._tool_names = tool_names self._agent = agent - self._shutdown_callback = shutdown_callback self._app = Flask(__name__) self._tool_usage_stats = tool_usage_stats self._setup_routes() @@ -214,7 +211,7 @@ class SerenaDashboardAPI: @self._app.route("/shutdown", methods=["PUT"]) def shutdown() -> dict[str, str]: - self._shutdown() + self._agent.shutdown() return {"status": "shutting down"} @self._app.route("/get_available_languages", methods=["GET"]) @@ -541,15 +538,6 @@ class SerenaDashboardAPI: serena_version=self._agent.version, ) - def _shutdown(self) -> None: - log.info("Shutting down Serena") - if self._shutdown_callback: - self._shutdown_callback() - else: - # noinspection PyProtectedMember - # noinspection PyUnresolvedReferences - os._exit(0) - def _get_available_languages(self) -> ResponseAvailableLanguages: from solidlsp.ls_config import Language diff --git a/src/serena/gui_log_viewer.py b/src/serena/gui_log_viewer.py index 4ee16a86..57d3ac08 100644 --- a/src/serena/gui_log_viewer.py +++ b/src/serena/gui_log_viewer.py @@ -1,11 +1,11 @@ # mypy: ignore-errors import logging -import os import queue import sys import threading import tkinter as tk import traceback +from collections.abc import Callable from enum import Enum, auto from pathlib import Path from typing import Literal @@ -38,6 +38,7 @@ class GuiLogViewer: memory_log_handler: MemoryLogHandler | None = None, width=800, height=600, + shutdown_handler: Callable[[], None] | None = None, ): """ :param mode: the mode; if "dashboard", run a dashboard with logs and some control options; if "error", run @@ -57,6 +58,7 @@ class GuiLogViewer: self.log_thread = None self.menubar: tk.Menu | None = None self.tool_names = [] # List to store tool names for highlighting + self.shutdown_handler = shutdown_handler # Define colors for different log levels self.log_colors = { @@ -320,10 +322,8 @@ class GuiLogViewer: self.running = False def _shutdown_server(self) -> None: - log.info("Shutting down Serena") - # noinspection PyUnresolvedReferences - # noinspection PyProtectedMember - os._exit(0) + if self.shutdown_handler is not None: + self.shutdown_handler() class GuiLogViewerHandler(logging.Handler): diff --git a/src/serena/mcp.py b/src/serena/mcp.py index f9374030..75325668 100644 --- a/src/serena/mcp.py +++ b/src/serena/mcp.py @@ -346,7 +346,7 @@ class SerenaMCPFactory: finally: log.info("MCP server shutting down") if self.agent is not None: - self.agent.shutdown() + self.agent.on_shutdown() def _get_initial_instructions(self) -> str: assert self.agent is not None diff --git a/test/serena/config/test_serena_config.py b/test/serena/config/test_serena_config.py index b3632b5c..3f2e7ac3 100644 --- a/test/serena/config/test_serena_config.py +++ b/test/serena/config/test_serena_config.py @@ -236,7 +236,7 @@ class TestEffectiveLanguageBackend: try: assert agent.get_language_backend().is_lsp() finally: - agent.shutdown(timeout=5) + agent.on_shutdown(timeout=5) def test_project_overrides_global_backend(self): """When startup project has language_backend set, it overrides the global.""" @@ -247,7 +247,7 @@ class TestEffectiveLanguageBackend: try: assert agent.get_language_backend().is_jetbrains() finally: - agent.shutdown(timeout=5) + agent.on_shutdown(timeout=5) def test_no_project_uses_global_backend(self): """When no startup project is provided, effective backend is the global one.""" @@ -261,7 +261,7 @@ class TestEffectiveLanguageBackend: try: assert agent.get_language_backend() == LanguageBackend.LSP finally: - agent.shutdown(timeout=5) + agent.on_shutdown(timeout=5) def test_activate_project_rejects_backend_mismatch(self): """Post-init activation of a project with mismatched backend raises ValueError.""" @@ -285,7 +285,7 @@ class TestEffectiveLanguageBackend: with pytest.raises(ValueError, match="Cannot activate project"): agent.activate_project_from_path_or_name("jb_proj") finally: - agent.shutdown(timeout=5) + agent.on_shutdown(timeout=5) def test_activate_project_allows_matching_backend(self): """Post-init activation of a project with matching backend succeeds.""" @@ -308,7 +308,7 @@ class TestEffectiveLanguageBackend: # Should not raise agent.activate_project_from_path_or_name("lsp_proj2") finally: - agent.shutdown(timeout=5) + agent.on_shutdown(timeout=5) def test_activate_project_allows_none_backend(self): """Post-init activation of a project with no backend override succeeds.""" @@ -331,7 +331,7 @@ class TestEffectiveLanguageBackend: # Should not raise — None means "inherit session backend" agent.activate_project_from_path_or_name("proj2") finally: - agent.shutdown(timeout=5) + agent.on_shutdown(timeout=5) class TestGetConfiguredProjectSerenaFolder: diff --git a/test/serena/test_dashboard.py b/test/serena/test_dashboard.py index 8de6e8eb..37e68e01 100644 --- a/test/serena/test_dashboard.py +++ b/test/serena/test_dashboard.py @@ -29,13 +29,7 @@ def _make_dashboard(project_languages: list[Language] | None) -> SerenaDashboard if project_languages is not None: project = SimpleNamespace(project_config=SimpleNamespace(languages=project_languages)) agent = _DummyAgent(project) - return SerenaDashboardAPI( - memory_log_handler=_DummyMemoryLogHandler(), - tool_names=[], - agent=agent, - shutdown_callback=None, - tool_usage_stats=None, - ) + return SerenaDashboardAPI(memory_log_handler=_DummyMemoryLogHandler(), tool_names=[], agent=agent, tool_usage_stats=None) def test_available_languages_include_experimental_when_no_active_project(): diff --git a/test/serena/test_serena_agent.py b/test/serena/test_serena_agent.py index 51a0227c..c2c1e61e 100644 --- a/test/serena/test_serena_agent.py +++ b/test/serena/test_serena_agent.py @@ -110,7 +110,7 @@ def serena_agent(request: pytest.FixtureRequest, serena_config) -> Iterator[Sere yield agent # explicitly shut down to free resources - agent.shutdown(timeout=5) + agent.on_shutdown(timeout=5) class TestSerenaAgent: