From 94769c3fbba4b03a9b3205c326bbe7e0248b570a Mon Sep 17 00:00:00 2001 From: Paul Fremantle Date: Thu, 28 May 2026 16:39:23 +0000 Subject: [PATCH 1/2] Record daemon PID in a file so _kill_local_daemon doesn't need pgrep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #9 added _kill_local_daemon as the force-kill path for a wedged boocloud-bridge daemon, using `pgrep -f` to find the PID. That works on hosts with procps installed but fails silently on slim container images (python:3.12-slim, alpine, …) where pgrep isn't available — exactly the environment the Teleclaude agent runs in. Wire _start_daemon to capture proc.pid and write it to a PID file (XDG_RUNTIME_DIR if set, otherwise a per-user file in the system temp dir so concurrent users don't collide). _kill_local_daemon reads the PID file first, validates the process is still alive, and falls back to pgrep for daemons started outside _start_daemon (e.g., the `boocloud daemon` CLI on a developer host with procps). Stale PID files are detected via os.kill(pid, 0) and cleared. Tests cover: PID-file kill happy path; stale PID-file cleanup; pgrep-fallback when no PID file; both-mechanisms-unavailable returns False cleanly; own-PID is never targeted by the pgrep path. Co-Authored-By: Claude Opus 4.7 --- changes/daemon-pid-file.bugfix | 1 + src/boocloud/bridge.py | 109 ++++++++++++++++++++++++++++----- tests/test_bridge.py | 98 +++++++++++++++++++++++++++++ 3 files changed, 194 insertions(+), 14 deletions(-) create mode 100644 changes/daemon-pid-file.bugfix diff --git a/changes/daemon-pid-file.bugfix b/changes/daemon-pid-file.bugfix new file mode 100644 index 0000000..fb64dfe --- /dev/null +++ b/changes/daemon-pid-file.bugfix @@ -0,0 +1 @@ +``_kill_local_daemon`` now records the daemon PID via ``_start_daemon`` writing a PID file (``$XDG_RUNTIME_DIR/boocloud-bridge.pid`` or a per-user temp file) and reads it back to target the exact process. This removes the hard dependency on ``pgrep``, which is missing from slim container images like ``python:3.12-slim``. The ``pgrep`` lookup is still used as a fallback when no PID file exists, so daemons started outside ``_start_daemon`` (e.g., the ``boocloud daemon`` CLI on a host with ``procps`` installed) are still covered. diff --git a/src/boocloud/bridge.py b/src/boocloud/bridge.py index 5a81e8b..8ad6cf6 100644 --- a/src/boocloud/bridge.py +++ b/src/boocloud/bridge.py @@ -424,6 +424,52 @@ def _patch_config_3mf_colors( DAEMON_URL = "http://127.0.0.1:8765" +def _pid_file_path() -> Path: + """Return the path where ``_start_daemon`` records the spawned PID. + + Uses ``$XDG_RUNTIME_DIR`` when available (per-user, auto-cleared on + logout); otherwise a per-user file in the system temp dir so two + users on the same host don't collide. + """ + runtime_dir = os.environ.get("XDG_RUNTIME_DIR") + if runtime_dir: + d = Path(runtime_dir) + if d.is_dir(): + return d / "boocloud-bridge.pid" + return Path(tempfile.gettempdir()) / f"boocloud-bridge-{os.getuid()}.pid" + + +def _write_pid_file(pid: int) -> None: + """Best-effort PID-file write; failures are logged but non-fatal.""" + try: + path = _pid_file_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(f"{pid}\n") + except OSError as e: + log.debug("Failed to write daemon PID file: %s", e) + + +def _read_pid_file() -> int | None: + """Read the recorded daemon PID, or None if the file is missing or invalid.""" + try: + text = _pid_file_path().read_text().strip() + except (FileNotFoundError, OSError): + return None + try: + pid = int(text) + except ValueError: + return None + return pid if pid > 1 else None + + +def _clear_pid_file() -> None: + """Delete the daemon PID file if present; ignore errors.""" + try: + _pid_file_path().unlink() + except (FileNotFoundError, OSError): + pass + + def _daemon_ping() -> bool: """Return True if a bridge daemon is responding on localhost:8765.""" import urllib.request @@ -470,7 +516,13 @@ def _check_daemon_version() -> None: def _start_daemon(token_file: Path, *, verbose: bool = False) -> bool: - """Start the bridge daemon in the background. Returns True if started.""" + """Start the bridge daemon in the background. Returns True if started. + + Records the spawned PID in ``_pid_file_path()`` so ``_kill_local_daemon`` + can target this exact process without depending on ``pgrep``. The Docker + branch skips the PID file (the container is the unit of life and is + managed by ``_start_daemon_docker`` / ``_stop_daemon_docker``). + """ binary = _find_local_bridge() if binary: cmd = [binary] @@ -478,12 +530,13 @@ def _start_daemon(token_file: Path, *, verbose: bool = False) -> bool: cmd.append("-v") cmd.extend(["-c", str(token_file.resolve()), "daemon", "--port", "8765"]) log.debug("Starting daemon: %s", " ".join(cmd)) - subprocess.Popen( + proc = subprocess.Popen( cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, start_new_session=True, ) + _write_pid_file(proc.pid) else: if not _start_daemon_docker(token_file, verbose=verbose): return False @@ -563,13 +616,40 @@ def _kill_local_daemon() -> bool: Used as a fallback when ``POST /shutdown`` fails to bring the daemon down — typical when the daemon is wedged inside libbambu FFI and the - HTTP handler is blocked. Looks up PIDs with ``pgrep`` and sends - ``SIGTERM`` then ``SIGKILL`` after a 1s grace. Returns True if at - least one process was killed. + HTTP handler is blocked. + + PID discovery has two paths: + 1. The PID file written by ``_start_daemon`` (preferred — works on + any image, no extra package required). + 2. ``pgrep -f boocloud-bridge.*\\bdaemon\\b`` (fallback — covers + daemons started outside this Python process, e.g., via + ``boocloud daemon`` CLI). + + Sends ``SIGTERM`` then ``SIGKILL`` after a 1s grace. Returns True + if at least one process was killed, False if no PIDs found / no + suitable mechanism available / permission denied for all targets. """ import signal import time + pids: list[int] = [] + + # 1. PID file (preferred). + recorded = _read_pid_file() + if recorded is not None: + # Sanity check: is the PID still alive? + try: + os.kill(recorded, 0) + pids.append(recorded) + except ProcessLookupError: + # Stale file — daemon already gone. + _clear_pid_file() + except PermissionError: + # Process exists but we can't signal it; record so the caller knows. + pids.append(recorded) + + # 2. pgrep fallback. Only useful when pgrep is installed and there + # might be a daemon we didn't start ourselves. try: result = subprocess.run( ["pgrep", "-f", r"boocloud-bridge.*\bdaemon\b"], @@ -577,17 +657,17 @@ def _kill_local_daemon() -> bool: text=True, timeout=5, ) + for line in result.stdout.split(): + line = line.strip() + if not line.isdigit(): + continue + pid = int(line) + if pid == os.getpid() or pid in pids: + continue + pids.append(pid) except (subprocess.SubprocessError, FileNotFoundError): - log.debug("pgrep unavailable; cannot force-kill daemon", exc_info=True) - return False + log.debug("pgrep unavailable; relying on PID file only", exc_info=True) - pids: list[int] = [] - for line in result.stdout.split(): - line = line.strip() - if line.isdigit(): - pid = int(line) - if pid != os.getpid(): - pids.append(pid) if not pids: return False @@ -611,6 +691,7 @@ def _kill_local_daemon() -> bool: except PermissionError: log.warning("Insufficient permission to SIGKILL daemon pid %d", pid) + _clear_pid_file() return True diff --git a/tests/test_bridge.py b/tests/test_bridge.py index af0535e..3d436c9 100644 --- a/tests/test_bridge.py +++ b/tests/test_bridge.py @@ -6,6 +6,7 @@ import json import os import subprocess +import tempfile import xml.etree.ElementTree as ET import zipfile from unittest.mock import patch @@ -21,6 +22,7 @@ _cloud_print_impl, _ensure_daemon, _find_local_bridge, + _kill_local_daemon, _patch_config_3mf_colors, _run_bridge_local, _shutdown_daemon, @@ -644,6 +646,102 @@ def test_returns_false_when_daemon_refuses_to_die(self): assert _shutdown_daemon() is False +class TestKillLocalDaemon: + """``_kill_local_daemon`` finds the PID via PID-file then pgrep, then kills.""" + + def test_uses_pid_file_when_present(self, tmp_path, monkeypatch): + # Spawn a long-lived dummy process to act as the "daemon" + proc = subprocess.Popen(["sleep", "60"]) + try: + pid_file = tmp_path / "bridge.pid" + pid_file.write_text(f"{proc.pid}\n") + monkeypatch.setattr("boocloud.bridge._pid_file_path", lambda: pid_file) + # No pgrep needed — and we don't want it picking up the test runner + with patch( + "boocloud.bridge.subprocess.run", + side_effect=FileNotFoundError, + ): + assert _kill_local_daemon() is True + proc.wait(timeout=5) + assert not pid_file.exists() # cleaned up after kill + finally: + if proc.poll() is None: + proc.kill() + + def test_stale_pid_file_is_cleared(self, tmp_path, monkeypatch): + # 99999 is almost certainly not a live PID + pid_file = tmp_path / "bridge.pid" + pid_file.write_text("99999\n") + monkeypatch.setattr("boocloud.bridge._pid_file_path", lambda: pid_file) + with patch( + "boocloud.bridge.subprocess.run", + side_effect=FileNotFoundError, + ): + # No pgrep, no live PID — nothing to kill. + assert _kill_local_daemon() is False + # Stale file was removed during the sanity check. + assert not pid_file.exists() + + def test_falls_back_to_pgrep_when_no_pid_file(self, tmp_path, monkeypatch): + proc = subprocess.Popen(["sleep", "60"]) + try: + pid_file = tmp_path / "bridge.pid" + # File deliberately doesn't exist + monkeypatch.setattr("boocloud.bridge._pid_file_path", lambda: pid_file) + + def fake_run(cmd, **kwargs): + if cmd[0] == "pgrep": + return subprocess.CompletedProcess(cmd, 0, f"{proc.pid}\n", "") + raise FileNotFoundError + + with patch("boocloud.bridge.subprocess.run", side_effect=fake_run): + assert _kill_local_daemon() is True + proc.wait(timeout=5) + finally: + if proc.poll() is None: + proc.kill() + + def test_no_pid_file_no_pgrep_returns_false(self, tmp_path, monkeypatch): + pid_file = tmp_path / "bridge.pid" + monkeypatch.setattr("boocloud.bridge._pid_file_path", lambda: pid_file) + with patch( + "boocloud.bridge.subprocess.run", + side_effect=FileNotFoundError, # pgrep not installed + ): + assert _kill_local_daemon() is False + + def test_does_not_target_own_pid(self, tmp_path, monkeypatch): + pid_file = tmp_path / "bridge.pid" + # No PID file written + monkeypatch.setattr("boocloud.bridge._pid_file_path", lambda: pid_file) + + def fake_run(cmd, **kwargs): + if cmd[0] == "pgrep": + return subprocess.CompletedProcess(cmd, 0, f"{os.getpid()}\n", "") + raise FileNotFoundError + + with patch("boocloud.bridge.subprocess.run", side_effect=fake_run): + # Only matched PID is our own → filtered out → nothing to kill. + assert _kill_local_daemon() is False + + +class TestPidFilePath: + def test_uses_xdg_runtime_dir_when_set(self, tmp_path, monkeypatch): + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + from boocloud.bridge import _pid_file_path + + assert _pid_file_path() == tmp_path / "boocloud-bridge.pid" + + def test_falls_back_to_temp_when_xdg_unset(self, monkeypatch): + monkeypatch.delenv("XDG_RUNTIME_DIR", raising=False) + from boocloud.bridge import _pid_file_path + + p = _pid_file_path() + assert p.name == f"boocloud-bridge-{os.getuid()}.pid" + # Should be in a known temp dir, not / or cwd + assert str(p).startswith(tempfile.gettempdir()) + + class TestCloudPrintImpl: def _setup_3mf(self, tmp_path): threemf = tmp_path / "test.gcode.3mf" From a567e2cc33c1bbbd66407ecc8c0c1351d0d5ddb9 Mon Sep 17 00:00:00 2001 From: Paul Fremantle Date: Thu, 28 May 2026 16:44:23 +0000 Subject: [PATCH 2/2] Make daemon kill path Windows-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR adds new tests that exercise _kill_local_daemon end-to-end on a real PID; Windows CI surfaced three issues, two of which were pre-existing portability bugs in the PR #9 code: - `signal.SIGKILL` doesn't exist on Windows (only SIGTERM). - `os.kill(pid, 0)` raises OSError on Windows, not ProcessLookupError. - `os.getuid()` doesn't exist on Windows (used by _pid_file_path). Fixes: - _kill_local_daemon now returns False immediately on Windows. The README documents that Windows always uses Docker (no native daemon to kill), so this isn't a regression — just an honest no-op. - On Unix, SIGKILL is looked up via getattr so the SIGTERM-only loop still works on platforms that lack SIGKILL. - The liveness check (`os.kill(pid, 0)`) catches OSError too, so the parent kill loop doesn't crash if Windows ever reaches it. - _pid_file_path drops the `-{uid}` suffix when os.getuid isn't available, so the helper still works on Windows (PID file write is platform-agnostic even if the kill path isn't). Tests: - TestKillLocalDaemon is skipif Windows (the path is documented as Unix-only and the unit tests rely on `sleep` + Unix process semantics that don't translate). - TestPidFilePath gains test_falls_back_to_temp_without_getuid which monkeypatches getuid off to simulate the Windows path. Co-Authored-By: Claude Opus 4.7 --- src/boocloud/bridge.py | 67 ++++++++++++++++++++++++++++++------------ tests/test_bridge.py | 19 +++++++++++- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/src/boocloud/bridge.py b/src/boocloud/bridge.py index 8ad6cf6..70fd421 100644 --- a/src/boocloud/bridge.py +++ b/src/boocloud/bridge.py @@ -34,6 +34,7 @@ def _xml_ns(root: ET.Element) -> str: DOCKER_IMAGE = "estampo/boocloud-bridge:bambu-02.05.00.00" EXPECTED_API_VERSION = 1 _IS_MACOS = platform.system() == "Darwin" +_IS_WINDOWS = platform.system() == "Windows" # --------------------------------------------------------------------------- # Credentials @@ -429,14 +430,18 @@ def _pid_file_path() -> Path: Uses ``$XDG_RUNTIME_DIR`` when available (per-user, auto-cleared on logout); otherwise a per-user file in the system temp dir so two - users on the same host don't collide. + users on the same host don't collide. Falls back to a non-suffixed + file on platforms without ``os.getuid()`` (Windows), where the + daemon kill path is unused anyway (Windows goes through Docker). """ runtime_dir = os.environ.get("XDG_RUNTIME_DIR") if runtime_dir: d = Path(runtime_dir) if d.is_dir(): return d / "boocloud-bridge.pid" - return Path(tempfile.gettempdir()) / f"boocloud-bridge-{os.getuid()}.pid" + getuid = getattr(os, "getuid", None) + suffix = f"-{getuid()}" if getuid else "" + return Path(tempfile.gettempdir()) / f"boocloud-bridge{suffix}.pid" def _write_pid_file(pid: int) -> None: @@ -628,25 +633,42 @@ def _kill_local_daemon() -> bool: Sends ``SIGTERM`` then ``SIGKILL`` after a 1s grace. Returns True if at least one process was killed, False if no PIDs found / no suitable mechanism available / permission denied for all targets. + + Returns False immediately on Windows: the README documents that + Windows always uses Docker (no native daemon to kill), and + ``signal.SIGKILL`` / ``ProcessLookupError`` semantics differ enough + that the Unix code below is the wrong tool. The Docker container + is reaped by ``_stop_daemon_docker``. """ import signal import time + if _IS_WINDOWS: + log.debug("Windows: no native daemon to kill (Docker path only)") + return False + pids: list[int] = [] + def _is_alive(pid: int) -> bool: + """Best-effort liveness check (signal 0). Treats any OSError other + than a ``no such process`` as "alive, no signal permission".""" + try: + os.kill(pid, 0) + return True + except ProcessLookupError: + return False + except PermissionError: + return True + except OSError: + return True + # 1. PID file (preferred). recorded = _read_pid_file() if recorded is not None: - # Sanity check: is the PID still alive? - try: - os.kill(recorded, 0) + if _is_alive(recorded): pids.append(recorded) - except ProcessLookupError: - # Stale file — daemon already gone. + else: _clear_pid_file() - except PermissionError: - # Process exists but we can't signal it; record so the caller knows. - pids.append(recorded) # 2. pgrep fallback. Only useful when pgrep is installed and there # might be a daemon we didn't start ourselves. @@ -678,18 +700,25 @@ def _kill_local_daemon() -> bool: pass except PermissionError: log.warning("Insufficient permission to SIGTERM daemon pid %d", pid) + except OSError as e: + log.warning("SIGTERM to pid %d failed: %s", pid, e) time.sleep(1.0) - for pid in pids: - try: - os.kill(pid, 0) # still alive? - os.kill(pid, signal.SIGKILL) - log.info("SIGKILL sent to wedged daemon pid %d", pid) - except ProcessLookupError: - pass - except PermissionError: - log.warning("Insufficient permission to SIGKILL daemon pid %d", pid) + sigkill = getattr(signal, "SIGKILL", None) + if sigkill is not None: + for pid in pids: + if not _is_alive(pid): + continue + try: + os.kill(pid, sigkill) + log.info("SIGKILL sent to wedged daemon pid %d", pid) + except ProcessLookupError: + pass + except PermissionError: + log.warning("Insufficient permission to SIGKILL daemon pid %d", pid) + except OSError as e: + log.warning("SIGKILL to pid %d failed: %s", pid, e) _clear_pid_file() return True diff --git a/tests/test_bridge.py b/tests/test_bridge.py index 3d436c9..a487afc 100644 --- a/tests/test_bridge.py +++ b/tests/test_bridge.py @@ -6,6 +6,7 @@ import json import os import subprocess +import sys import tempfile import xml.etree.ElementTree as ET import zipfile @@ -646,6 +647,10 @@ def test_returns_false_when_daemon_refuses_to_die(self): assert _shutdown_daemon() is False +@pytest.mark.skipif( + sys.platform == "win32", + reason="native daemon kill path is Unix-only; Windows uses Docker (see README)", +) class TestKillLocalDaemon: """``_kill_local_daemon`` finds the PID via PID-file then pgrep, then kills.""" @@ -737,10 +742,22 @@ def test_falls_back_to_temp_when_xdg_unset(self, monkeypatch): from boocloud.bridge import _pid_file_path p = _pid_file_path() - assert p.name == f"boocloud-bridge-{os.getuid()}.pid" + getuid = getattr(os, "getuid", None) + expected = f"boocloud-bridge-{getuid()}.pid" if getuid else "boocloud-bridge.pid" + assert p.name == expected # Should be in a known temp dir, not / or cwd assert str(p).startswith(tempfile.gettempdir()) + def test_falls_back_to_temp_without_getuid(self, monkeypatch): + """Windows has no ``os.getuid`` — path drops the per-user suffix.""" + monkeypatch.delenv("XDG_RUNTIME_DIR", raising=False) + monkeypatch.delattr(os, "getuid", raising=False) + from boocloud.bridge import _pid_file_path + + p = _pid_file_path() + assert p.name == "boocloud-bridge.pid" + assert str(p).startswith(tempfile.gettempdir()) + class TestCloudPrintImpl: def _setup_3mf(self, tmp_path):