Daemon PID file: remove hard dependency on pgrep - #12
Merged
Conversation
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 <noreply@anthropic.com>
3 tasks
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 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #9 added
_kill_local_daemonas the force-kill path for a wedgedboocloud-bridgedaemon, usingpgrep -fto find the PID. That works on hosts withprocpsinstalled but fails silently on slim container images (e.g.,python:3.12-slim, alpine, …) wherepgrepisn't available — exactly the environment the Teleclaude agent container runs in.This PR makes the force-kill path work without
pgrepby recording the PID at spawn time.Changes
src/boocloud/bridge.py_pid_file_path()— returns$XDG_RUNTIME_DIR/boocloud-bridge.pidwhen set (per-user, auto-cleared on logout), otherwise a per-user file in the system temp dir (/tmp/boocloud-bridge-{uid}.pid) so concurrent users on the same host don't collide._write_pid_file/_read_pid_file/_clear_pid_filehelpers — best-effort I/O, all failures logged at DEBUG and non-fatal._start_daemoncapturesproc.pidfromsubprocess.Popenand writes the PID file before the readiness-poll loop._kill_local_daemonnow:pgrep -f boocloud-bridge.*\bdaemon\bfor daemons started outside_start_daemon(e.g.,boocloud daemonCLI on a developer host withprocps).pgrepand silently no-op'd.Why a file, not an in-process variable
The Python
boocloudpackage is invoked as separate processes (CLI, MCP server, sometimes both). State has to outlive the invocation that spawned the daemon.Test plan
pytest tests/test_bridge.py::TestKillLocalDaemon tests/test_bridge.py::TestPidFilePath— 7 passed.pytest tests/test_bridge.py— 60 passed, 1 pre-existing failure unrelated to this PR (TestFindLocalBridge::test_returns_none_when_not_founddoesn't patch/usr/local/bin/boocloud-bridgeand fails on dev hosts where the binary is installed system-wide).ruff check+ruff format --checkclean.pgrep, force-restart the daemon, confirm a fresh process appears.🤖 Generated with Claude Code