From cfd7e1885c33ce3249f30088334a91b87ee9a82d Mon Sep 17 00:00:00 2001 From: yangshu Date: Sat, 27 Jun 2026 13:29:19 -0400 Subject: [PATCH] fix(daemon): stop the remote browser on SIGTERM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The daemon only caught SIGINT (KeyboardInterrupt); SIGTERM terminated the process before the finally block, so stop_remote() never ran and the remote browser leaked (and kept billing). admin.py escalates to os.kill(pid, SIGTERM) when the "shutdown" IPC can't be delivered, and container runtimes send SIGTERM on stop, so this path is hit in practice. Register an asyncio SIGTERM handler that sets the existing stop Event — the same graceful path as the "shutdown" command — so main() unwinds cleanly and the finally block runs stop_remote(). Registered before start() (the CDP handshake can take seconds) with the stop Event moved to __init__. POSIX-only: add_signal_handler raises NotImplementedError on Windows, where SIGTERM can't be caught anyway. Adds a unit test asserting main() registers the handler and that the stop Event drives a graceful return. Co-Authored-By: Claude Opus 4.8 --- src/browser_harness/daemon.py | 15 ++++++++--- tests/unit/test_daemon.py | 47 +++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/browser_harness/daemon.py b/src/browser_harness/daemon.py index a4d7e00e..f6c99763 100644 --- a/src/browser_harness/daemon.py +++ b/src/browser_harness/daemon.py @@ -1,5 +1,5 @@ """CDP WS holder + IPC relay (Unix socket on POSIX, TCP loopback on Windows). One daemon per BU_NAME.""" -import asyncio, json, os, socket, sys, time, urllib.error, urllib.request +import asyncio, json, os, signal, socket, sys, time, urllib.error, urllib.request from urllib.parse import urlparse from collections import deque from pathlib import Path @@ -193,7 +193,7 @@ def __init__(self): self.target_id = None self.events = deque(maxlen=BUF) self.dialog = None - self.stop = None # asyncio.Event, set inside start() + self.stop = asyncio.Event() # created here so a SIGTERM during start() is honored async def attach_first_page(self): """Attach to a real page (or any page). Sets self.session. Returns attached target or None.""" @@ -237,7 +237,6 @@ async def enable_one(d): await asyncio.gather(*(enable_one(d) for d in ("Page", "DOM", "Runtime", "Network"))) async def start(self): - self.stop = asyncio.Event() url = get_ws_url() log(f"connecting to {url}") self.cdp = CDPClient(url) @@ -398,6 +397,16 @@ async def handler(reader, writer): async def main(): d = Daemon() + # Register SIGTERM BEFORE start(): get_ws_url()/the CDP handshake can take many seconds, + # and a signal during that window must still trigger cleanup (d.stop exists from __init__). + # admin.py escalates to os.kill(pid, SIGTERM) when the "shutdown" IPC can't be delivered, + # and container runtimes send SIGTERM on stop; without this the finally block's stop_remote() + # is skipped and the remote browser leaks. POSIX-only: add_signal_handler raises + # NotImplementedError on Windows, where SIGTERM can't be caught anyway. + try: + asyncio.get_running_loop().add_signal_handler(signal.SIGTERM, d.stop.set) + except (NotImplementedError, AttributeError): + pass await d.start() await serve(d) diff --git a/tests/unit/test_daemon.py b/tests/unit/test_daemon.py index 90c5bc85..83cbe1bb 100644 --- a/tests/unit/test_daemon.py +++ b/tests/unit/test_daemon.py @@ -293,3 +293,50 @@ def test_current_tab_meta_returns_not_attached_when_no_target_id(): assert result == {"error": "not_attached"} # No CDP call should have been issued. assert d.cdp.calls == [] + + +def test_main_registers_sigterm_handler_that_triggers_graceful_stop(): + """main() must register a SIGTERM handler that sets the stop Event. + + admin.py escalates to os.kill(pid, SIGTERM) when the "shutdown" IPC can't be + delivered, and container runtimes send SIGTERM on stop. Without this, the + process is terminated before the finally block runs, so stop_remote() never + fires and a remote browser leaks. We verify the handler is registered (via + the public loop.remove_signal_handler, which returns True iff one was set) + and that setting the stop Event lets main() return — without delivering a + real signal, which on regression would kill the test runner. + """ + import signal + import sys + + if sys.platform == "win32": + return # add_signal_handler and a catchable SIGTERM are POSIX-only + + orig_start, orig_serve = daemon.Daemon.start, daemon.serve + captured = {} + + async def fake_start(self): + captured["d"] = self # d.stop already exists from __init__ + + async def fake_serve(d): + await d.stop.wait() + + daemon.Daemon.start = fake_start + daemon.serve = fake_serve + try: + async def run(): + loop = asyncio.get_running_loop() + task = asyncio.ensure_future(daemon.main()) + await asyncio.sleep(0.05) # let main() register the handler and enter serve() + try: + # public API: True iff main() registered a SIGTERM handler (also removes it) + assert loop.remove_signal_handler(signal.SIGTERM) is True + captured["d"].stop.set() # what the SIGTERM handler does + await asyncio.wait_for(task, timeout=2) # main() must return, not hang + finally: + if not task.done(): + task.cancel() + asyncio.run(run()) + finally: + daemon.Daemon.start = orig_start + daemon.serve = orig_serve