From 8b91164920b912a8a61f3e4cc4eac3388363ecc9 Mon Sep 17 00:00:00 2001 From: Aura - jc <67582323+Catafal@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:40:50 +0200 Subject: [PATCH 1/6] =?UTF-8?q?feat:=20watcher=5Fprocesses=20=E2=80=94=20e?= =?UTF-8?q?xtract=20spawn=5Fwatcher=20+=20ps-based=20supervised=20scan=20(?= =?UTF-8?q?#11)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the detached watcher Popen from task_service into a reusable runtimes/watcher_processes.spawn_watcher; task_service delegates to it with no launch-path behavior change. Add list_supervised_artifacts() which derives the supervised set from the OS process table (ps scan for 'internal session-watch '), the ground truth that lets the daemon dedupe against watch_on_launch and survive restarts. Closes #11 Co-Authored-By: Claude Fable 5 --- src/bach/runtimes/watcher_processes.py | 207 ++++++++++++++++ src/bach/services/task_service.py | 62 +---- tests/unit/test_task_service.py | 7 +- tests/unit/test_watcher_processes.py | 322 +++++++++++++++++++++++++ 4 files changed, 540 insertions(+), 58 deletions(-) create mode 100644 src/bach/runtimes/watcher_processes.py create mode 100644 tests/unit/test_watcher_processes.py diff --git a/src/bach/runtimes/watcher_processes.py b/src/bach/runtimes/watcher_processes.py new file mode 100644 index 0000000..8967b0e --- /dev/null +++ b/src/bach/runtimes/watcher_processes.py @@ -0,0 +1,207 @@ +"""Process-table adapter for Bach session-watcher supervision. + +This module owns two things: + 1. spawn_watcher — detached Popen of `bach internal session-watch `. + Extracted from TaskService._spawn_session_watcher so it can be reused + outside the service layer (e.g. the AFK runner, future supervision daemon). + + 2. list_supervised_artifacts — process-table scan that returns the set of + artifact Paths currently being watched by a live session-watch process. + Uses an injectable process_probe so tests stay fully hermetic. + +Design mirrors session_discovery.py: + - process_probe is injected; default_process_probe() is the real impl. + - Pure parse helper (parse_supervised_artifacts) is separate so it can be + unit-tested with fake input without touching any OS state. + - warn-not-fail on every exception: caller's primary job must not be blocked. + +Session-watch process command signature (produced by spawn_watcher): + -m bach internal session-watch + +The marker string "internal session-watch" uniquely identifies these processes +in the process table — no other bach subcommand uses that exact phrase. +""" + +import logging +import subprocess +import sys +from pathlib import Path + +from bach.config.paths import bach_home +from bach.runtimes.session_discovery import ProcessProbe, default_process_probe + +logger = logging.getLogger(__name__) + +# The unique substring that identifies a live session-watch process in `ps` output. +# Anchored to the internal subcommand so unrelated Python processes don't match. +_SESSION_WATCH_MARKER = "internal session-watch" + + +# --------------------------------------------------------------------------- +# Pure parse helper — no I/O, fully unit-testable +# --------------------------------------------------------------------------- + + +def parse_supervised_artifacts( + processes: list[tuple[int, str]], +) -> set[Path]: + """Extract watched artifact Paths from a process-table snapshot. + + Scans (pid, command_line) pairs for lines containing the session-watch + marker and extracts the artifact path that follows it. Returns a set + so duplicate artifacts (same path, different PIDs) are deduplicated. + + Ignores: + - Lines without the session-watch marker. + - Lines where the marker appears but no argument follows it. + - Lines with an empty command string. + + Args: + processes: list of (pid, command_line) as returned by a ProcessProbe. + + Returns: + Set of artifact Paths extracted from matching process command lines. + """ + artifacts: set[Path] = set() + + for _pid, cmdline in processes: + if not cmdline: + continue + + marker_idx = cmdline.find(_SESSION_WATCH_MARKER) + if marker_idx == -1: + # Not a session-watch process — skip it. + continue + + # Everything after the marker + one space is the artifact path argument. + # e.g. "python -m bach internal session-watch /path/to/task.md" + # marker ends at idx+len(marker), then one space, then the path. + after_marker = cmdline[marker_idx + len(_SESSION_WATCH_MARKER):] + artifact_str = after_marker.strip() + + if not artifact_str: + # Marker present but no argument — malformed or incomplete line. + logger.debug( + "event=watcher_parse_no_arg cmdline=%r", + cmdline, + ) + continue + + # The path occupies the rest of the command line after the marker. + # In Popen, the path is a distinct argv element, so it appears as-is + # (no shell quoting) in ps output. We take everything as the path. + artifacts.add(Path(artifact_str)) + + return artifacts + + +# --------------------------------------------------------------------------- +# Supervision scan — injectable probe +# --------------------------------------------------------------------------- + + +def list_supervised_artifacts( + *, + process_probe: ProcessProbe = default_process_probe, +) -> set[Path]: + """Return the set of artifact Paths currently watched by a live session-watch. + + Calls process_probe() once to get a process-table snapshot, then delegates + to parse_supervised_artifacts for the pure extraction logic. + + The default probe (default_process_probe) uses `ps -axo pid,command`. + Inject a fake probe in tests to keep tests hermetic and fast. + + Args: + process_probe: callable returning [(pid, cmdline)]. Defaults to the + real ps-based probe from session_discovery. + + Returns: + Set of artifact Paths with at least one live watcher process. + Returns an empty set when the probe fails — never raises. + """ + try: + processes = process_probe() + except Exception as exc: # noqa: BLE001 — probe failure must not crash caller + logger.warning( + "event=watcher_probe_failed reason=%s", + type(exc).__name__, + ) + return set() + + return parse_supervised_artifacts(processes) + + +# --------------------------------------------------------------------------- +# Detached watcher spawn +# --------------------------------------------------------------------------- + + +def spawn_watcher(artifact: Path) -> tuple[bool, Path | None]: + """Spawn `bach internal session-watch ` as a detached child. + + This is the single canonical place for spawning a watcher process. + Extracted from TaskService._spawn_session_watcher so any caller + (service layer, AFK runner, supervision daemon) can use it without + duplicating the log-redirect logic. + + Uses the current Python interpreter (-m bach) so the child inherits + the same venv and PATH without requiring `bach` to be on PATH. + + stdout/stderr are redirected (append mode) to: + bach_home() / "logs" / "watcher-.log" + + This is the ONLY trace of the detached process. DEVNULL would make + watcher crashes completely invisible — always use a real file. + + close_fds=True detaches the child from the parent's open file table + so it outlives this process cleanly. + + Best-effort / warn-not-fail: a spawn exception logs a warning but + NEVER blocks the caller. The caller decides whether to notify the user. + + Args: + artifact: Path to the task artifact that the watcher should observe. + + Returns: + (True, log_path) on successful spawn. + (False, None) on any exception. + """ + # Use the artifact stem for the log name so it's easy to correlate. + # Fallback to the full name if the stem is somehow empty. + stem = artifact.stem if artifact.stem else artifact.name + logs_dir = bach_home() / "logs" + logs_dir.mkdir(parents=True, exist_ok=True) # create on demand; idempotent + log_path = logs_dir / f"watcher-{stem}.log" + + try: + # Re-use the current interpreter so the watcher inherits the same + # venv environment without requiring `bach` to be on PATH. + cmd = [sys.executable, "-m", "bach", "internal", "session-watch", str(artifact)] + + # Append mode: successive watcher restarts accumulate in one file + # rather than overwriting previous diagnostic output. + log_fh = log_path.open("a", encoding="utf-8") + + subprocess.Popen( + cmd, + stdin=subprocess.DEVNULL, + stdout=log_fh, + stderr=log_fh, + close_fds=True, + ) + + logger.info( + "event=session_watcher_spawned artifact=%s log=%s", + artifact, + log_path, + ) + return True, log_path + + except Exception as exc: # noqa: BLE001 — warn-not-fail, never block caller + logger.warning( + "event=session_watcher_spawn_failed artifact=%s reason=%s", + artifact, + type(exc).__name__, + ) + return False, None diff --git a/src/bach/services/task_service.py b/src/bach/services/task_service.py index 04d27eb..9bcc5b6 100644 --- a/src/bach/services/task_service.py +++ b/src/bach/services/task_service.py @@ -14,19 +14,17 @@ """ import logging -import subprocess -import sys from collections.abc import Callable from pathlib import Path import typer -from bach.config.paths import bach_home from bach.config.projects import ProjectRegistry from bach.config.settings import load_config from bach.domain.models import AgentRuntime, Project, SessionMode, TaskArtifact from bach.runtimes.commands import build_launch_command, build_resume_command from bach.runtimes.iterm import launch_in_iterm +from bach.runtimes.watcher_processes import spawn_watcher from bach.services.session_tracker import record_session_start from bach.storage.artifacts import TaskArtifactStore @@ -234,60 +232,14 @@ def launch_task( def _spawn_session_watcher(self, artifact: Path) -> tuple[bool, Path | None]: """Spawn `bach internal session-watch ` as a detached child. - Uses the same Python interpreter so the watcher inherits the same - environment (venv, PATH, etc.) without requiring `bach` to be on PATH. - subprocess.Popen with close_fds=True + redirected stdout/stderr - detaches the child from the parent terminal. We do NOT wait for it. + Thin delegation to runtimes.watcher_processes.spawn_watcher, which owns + the detach logic, log-file routing, and warn-not-fail error handling. + This method exists so callers in this service (launch_task) and tests + can patch it at the service level when needed. - stdout and stderr are redirected to an append-mode log file at: - bach_home() / "logs" / "watcher-.log" - This is the ONLY trace of a detached process — DEVNULL would make - watcher crashes completely invisible and unrecoverable to diagnose. - - Best-effort: a spawn failure logs a warning but NEVER blocks the - primary launch (warn-not-fail). The user has their iTerm tab; the - watcher is bonus. - - Returns (True, log_path) on successful spawn, (False, None) on - failure. The caller decides whether to print a confirmation line — - no output happens here so the output channel stays with launch_task. + Returns (True, log_path) on success, (False, None) on failure. """ - # Build the log file path: bach_home/logs/watcher-.log - # Using the artifact stem makes the log easy to correlate to the task. - # Fallback to a runtime timestamp if the artifact path is unusually short. - stem = artifact.stem if artifact.stem else artifact.name - logs_dir = bach_home() / "logs" - logs_dir.mkdir(parents=True, exist_ok=True) # create on demand - log_path = logs_dir / f"watcher-{stem}.log" - - try: - # Construct the same invocation as `bach internal session-watch` - # by re-using the current interpreter and module entry point. - cmd = [sys.executable, "-m", "bach", "internal", "session-watch", str(artifact)] - # Open in append mode so successive watchers for the same artifact - # (e.g. re-launch after crash) accumulate in one file rather than - # overwriting the previous run's output. - log_fh = log_path.open("a", encoding="utf-8") - subprocess.Popen( - cmd, - stdin=subprocess.DEVNULL, - stdout=log_fh, - stderr=log_fh, - close_fds=True, - ) - logger.info( - "event=session_watcher_spawned artifact=%s log=%s", - artifact, - log_path, - ) - return True, log_path - except Exception as exc: # noqa: BLE001 — warn-not-fail, never block launch - logger.warning( - "event=session_watcher_spawn_failed artifact=%s reason=%s", - artifact, - type(exc).__name__, - ) - return False, None + return spawn_watcher(artifact) def validate_task(self, artifact: Path) -> list[str]: """Return a list of validation errors for the artifact, empty if OK.""" diff --git a/tests/unit/test_task_service.py b/tests/unit/test_task_service.py index 362ab3d..32558bc 100644 --- a/tests/unit/test_task_service.py +++ b/tests/unit/test_task_service.py @@ -185,11 +185,12 @@ class _FakePopen: def __init__(self, cmd: Any, **kwargs: Any) -> None: captured_kwargs.update(kwargs) - # Patch bach_home so the log dir lands in tmp_path, not the real ~/.bach. + # Patch the new module that owns the implementation. + # _spawn_session_watcher now delegates to runtimes.watcher_processes.spawn_watcher. fake_bach_home = tmp_path / "bach_home" with ( - patch("bach.services.task_service.bach_home", return_value=fake_bach_home), - patch("bach.services.task_service.subprocess.Popen", _FakePopen), + patch("bach.runtimes.watcher_processes.bach_home", return_value=fake_bach_home), + patch("bach.runtimes.watcher_processes.subprocess.Popen", _FakePopen), ): ok, log_path = service._spawn_session_watcher(task.artifact_path) diff --git a/tests/unit/test_watcher_processes.py b/tests/unit/test_watcher_processes.py new file mode 100644 index 0000000..bd66d63 --- /dev/null +++ b/tests/unit/test_watcher_processes.py @@ -0,0 +1,322 @@ +"""Tests for runtimes/watcher_processes.py. + +Strategy: + - Inject process_probe as a callable returning fake ps output. + - Use a fake Popen to verify spawn_watcher without touching real subprocesses. + - Patch bach_home so log dirs land in tmp_path, never ~/.bach. + - Test parse_supervised_artifacts separately (pure function, no I/O). + - Never touch real ~/.bach, real processes, or the real filesystem beyond tmp_path. +""" + +import subprocess +from pathlib import Path +from typing import Any +from unittest.mock import patch + +from bach.runtimes.watcher_processes import ( + list_supervised_artifacts, + parse_supervised_artifacts, + spawn_watcher, +) + +# --------------------------------------------------------------------------- +# parse_supervised_artifacts — pure unit tests with fake ps output +# --------------------------------------------------------------------------- + + +def test_parse_returns_empty_for_empty_input() -> None: + """No ps lines → empty set.""" + result = parse_supervised_artifacts([]) + assert result == set() + + +def test_parse_extracts_artifact_from_matching_line(tmp_path: Path) -> None: + """A line with 'internal session-watch ' → artifact Path in result.""" + artifact = tmp_path / "task-001.md" + # Simulate: python -m bach internal session-watch /some/path/task-001.md + lines = [(1234, f"python -m bach internal session-watch {artifact}")] + result = parse_supervised_artifacts(lines) + assert artifact in result + + +def test_parse_extracts_multiple_artifacts(tmp_path: Path) -> None: + """Multiple matching lines → multiple artifacts in result.""" + a1 = tmp_path / "task-001.md" + a2 = tmp_path / "task-002.md" + lines = [ + (1001, f"python -m bach internal session-watch {a1}"), + (1002, f"python -m bach internal session-watch {a2}"), + ] + result = parse_supervised_artifacts(lines) + assert a1 in result + assert a2 in result + assert len(result) == 2 + + +def test_parse_ignores_unrelated_processes(tmp_path: Path) -> None: + """Lines that don't contain 'internal session-watch' are ignored.""" + artifact = tmp_path / "task-001.md" + lines = [ + (1001, f"python -m bach internal session-watch {artifact}"), + (1002, "python -m bach task add"), + (1003, "/usr/bin/python some_other_script.py"), + (1004, "claude --session-id abc123"), + ] + result = parse_supervised_artifacts(lines) + assert len(result) == 1 + assert artifact in result + + +def test_parse_ignores_short_lines_no_artifact_arg() -> None: + """A line matching the pattern but with no artifact argument is ignored.""" + # 'internal session-watch' present but nothing after it + lines = [(999, "python -m bach internal session-watch")] + result = parse_supervised_artifacts(lines) + assert result == set() + + +def test_parse_ignores_malformed_pid_entries() -> None: + """Lines where pid cannot be parsed (or cmd is empty) are skipped.""" + # Simulate a line with a purely empty command — shouldn't crash + lines: list[tuple[int, str]] = [(0, "")] + result = parse_supervised_artifacts(lines) + assert result == set() + + +def test_parse_deduplicates_same_artifact_watched_twice(tmp_path: Path) -> None: + """If the same artifact appears from two processes, it's one set entry.""" + artifact = tmp_path / "task-001.md" + lines = [ + (1001, f"python -m bach internal session-watch {artifact}"), + (1002, f"/usr/bin/python3 -m bach internal session-watch {artifact}"), + ] + result = parse_supervised_artifacts(lines) + assert len(result) == 1 + assert artifact in result + + +def test_parse_handles_artifact_path_with_spaces(tmp_path: Path) -> None: + """Artifact paths containing spaces should be captured correctly. + + Note: the parsing splits on the last space-separated token after + 'session-watch ', so the caller must handle quoted paths by contract + (the Popen in spawn_watcher passes the path as a distinct argv element, + so the actual path in ps output has no shell quoting). + """ + # This is the path as it appears in ps output (no quoting — it's argv, not shell) + artifact = tmp_path / "my task-001.md" + lines = [(1001, f"python -m bach internal session-watch {artifact}")] + result = parse_supervised_artifacts(lines) + # Should recover the artifact path (path with spaces after the keyword) + assert artifact in result + + +# --------------------------------------------------------------------------- +# list_supervised_artifacts — injected probe tests +# --------------------------------------------------------------------------- + + +def test_list_supervised_artifacts_returns_set(tmp_path: Path) -> None: + """list_supervised_artifacts returns a set (not a list).""" + result = list_supervised_artifacts(process_probe=lambda: []) + assert isinstance(result, set) + + +def test_list_supervised_artifacts_uses_injected_probe(tmp_path: Path) -> None: + """list_supervised_artifacts passes probe output through parse_supervised_artifacts.""" + artifact = tmp_path / "task-010.md" + probe_output = [(5555, f"python -m bach internal session-watch {artifact}")] + result = list_supervised_artifacts(process_probe=lambda: probe_output) + assert artifact in result + + +def test_list_supervised_artifacts_empty_probe_returns_empty() -> None: + """When probe returns nothing, result is empty.""" + result = list_supervised_artifacts(process_probe=lambda: []) + assert result == set() + + +def test_list_supervised_artifacts_filters_unrelated(tmp_path: Path) -> None: + """Processes unrelated to session-watch do not appear in the result.""" + artifact = tmp_path / "task-100.md" + probe_output = [ + (1, "launchd"), + (100, "python -m bach task launch something.md"), + (200, f"python -m bach internal session-watch {artifact}"), + ] + result = list_supervised_artifacts(process_probe=lambda: probe_output) + assert len(result) == 1 + assert artifact in result + + +# --------------------------------------------------------------------------- +# spawn_watcher — delegation and log redirect tests +# --------------------------------------------------------------------------- + + +def test_spawn_watcher_returns_true_and_log_path_on_success(tmp_path: Path) -> None: + """spawn_watcher returns (True, Path) when Popen succeeds.""" + + class _FakePopen: + def __init__(self, cmd: Any, **kwargs: Any) -> None: + pass + + fake_bach_home = tmp_path / "bach_home" + with ( + patch("bach.runtimes.watcher_processes.bach_home", return_value=fake_bach_home), + patch("bach.runtimes.watcher_processes.subprocess.Popen", _FakePopen), + ): + artifact = tmp_path / "task-999.md" + ok, log_path = spawn_watcher(artifact) + + assert ok is True + assert log_path is not None + assert log_path.name.startswith("watcher-") + assert log_path.suffix == ".log" + + +def test_spawn_watcher_returns_false_on_popen_exception(tmp_path: Path) -> None: + """spawn_watcher returns (False, None) when Popen raises — never propagates.""" + fake_bach_home = tmp_path / "bach_home" + + def _exploding_popen(cmd: Any, **kwargs: Any) -> None: + raise OSError("no such executable") + + with ( + patch("bach.runtimes.watcher_processes.bach_home", return_value=fake_bach_home), + patch("bach.runtimes.watcher_processes.subprocess.Popen", _exploding_popen), + ): + artifact = tmp_path / "task-999.md" + ok, log_path = spawn_watcher(artifact) + + assert ok is False + assert log_path is None + + +def test_spawn_watcher_log_path_is_under_bach_home_logs(tmp_path: Path) -> None: + """Log file must be under bach_home()/logs/.""" + captured_kwargs: dict[str, Any] = {} + fake_bach_home = tmp_path / "bach_home" + + class _FakePopen: + def __init__(self, cmd: Any, **kwargs: Any) -> None: + captured_kwargs.update(kwargs) + + with ( + patch("bach.runtimes.watcher_processes.bach_home", return_value=fake_bach_home), + patch("bach.runtimes.watcher_processes.subprocess.Popen", _FakePopen), + ): + artifact = tmp_path / "task-stem-check.md" + ok, log_path = spawn_watcher(artifact) + + assert ok is True + assert log_path is not None + assert str(log_path).startswith(str(fake_bach_home / "logs")) + + +def test_spawn_watcher_stdout_stderr_are_not_devnull(tmp_path: Path) -> None: + """stdout and stderr must be real file handles, never DEVNULL.""" + captured_kwargs: dict[str, Any] = {} + fake_bach_home = tmp_path / "bach_home" + + class _FakePopen: + def __init__(self, cmd: Any, **kwargs: Any) -> None: + captured_kwargs.update(kwargs) + + with ( + patch("bach.runtimes.watcher_processes.bach_home", return_value=fake_bach_home), + patch("bach.runtimes.watcher_processes.subprocess.Popen", _FakePopen), + ): + artifact = tmp_path / "task-999.md" + spawn_watcher(artifact) + + assert captured_kwargs.get("stdout") is not subprocess.DEVNULL + assert captured_kwargs.get("stderr") is not subprocess.DEVNULL + # Both should share one file handle (append mode log file). + assert captured_kwargs.get("stdout") is captured_kwargs.get("stderr") + + +def test_spawn_watcher_uses_artifact_stem_in_log_name(tmp_path: Path) -> None: + """Log filename must embed the artifact stem for easy correlation.""" + fake_bach_home = tmp_path / "bach_home" + + class _FakePopen: + def __init__(self, cmd: Any, **kwargs: Any) -> None: + pass + + artifact = tmp_path / "task-my-special-task.md" + with ( + patch("bach.runtimes.watcher_processes.bach_home", return_value=fake_bach_home), + patch("bach.runtimes.watcher_processes.subprocess.Popen", _FakePopen), + ): + ok, log_path = spawn_watcher(artifact) + + assert ok is True + assert log_path is not None + assert "task-my-special-task" in log_path.name + + +def test_spawn_watcher_close_fds_true(tmp_path: Path) -> None: + """close_fds must be True so the child detaches from the parent's file table.""" + captured_kwargs: dict[str, Any] = {} + fake_bach_home = tmp_path / "bach_home" + + class _FakePopen: + def __init__(self, cmd: Any, **kwargs: Any) -> None: + captured_kwargs.update(kwargs) + + with ( + patch("bach.runtimes.watcher_processes.bach_home", return_value=fake_bach_home), + patch("bach.runtimes.watcher_processes.subprocess.Popen", _FakePopen), + ): + artifact = tmp_path / "task-999.md" + spawn_watcher(artifact) + + assert captured_kwargs.get("close_fds") is True + + +# --------------------------------------------------------------------------- +# task_service delegation — _spawn_session_watcher → spawn_watcher +# --------------------------------------------------------------------------- + + +def test_task_service_spawn_delegates_to_spawn_watcher(tmp_path: Path) -> None: + """TaskService._spawn_session_watcher must delegate to spawn_watcher. + + After the extraction, the method body is a thin call — patching + spawn_watcher in the watcher_processes module must be the way to + intercept the invocation. + """ + from bach.config.projects import ProjectRegistry + from bach.domain.models import AgentRuntime, Project, SessionMode + from bach.services.task_service import TaskService + + project_path = tmp_path / "proj" + (project_path / ".bach" / "runs").mkdir(parents=True) + registry = ProjectRegistry(tmp_path / "registry.yaml") + data = registry.load() + data.projects["proj"] = Project(key="proj", name="Proj", path=project_path) + registry.save(data) + + service = TaskService(registry) + task = service.add_task( + description="test delegation", + project_key="proj", + runtime=AgentRuntime.claude_code, + mode=SessionMode.grill_me, + ) + + calls: list[Path] = [] + + def _fake_spawn_watcher(artifact: Path) -> tuple[bool, Path | None]: + calls.append(artifact) + return True, tmp_path / "logs" / "watcher-test.log" + + with patch("bach.runtimes.watcher_processes.spawn_watcher", _fake_spawn_watcher): + # Also patch the import in task_service since it will import spawn_watcher + with patch("bach.services.task_service.spawn_watcher", _fake_spawn_watcher): + ok, log_path = service._spawn_session_watcher(task.artifact_path) + + assert ok is True + assert len(calls) == 1 + assert calls[0] == task.artifact_path From 5c2db80a3ff87494a5ed31d37ef96aa090acdb7e Mon Sep 17 00:00:00 2001 From: Aura - jc <67582323+Catafal@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:49:20 +0200 Subject: [PATCH 2/6] =?UTF-8?q?feat:=20daemon=5Fsupervisor=20=E2=80=94=20r?= =?UTF-8?q?econcile=20engine=20+=20bach=20daemon=20run=20(#12)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure reconcile(desired, supervised, backoff, now) -> SpawnPlan upholds the duplicate-watcher invariant (never spawn an already-supervised artifact) with bounded exponential crash backoff. run_supervisor is a single-threaded synchronous loop with every side-effect injected as a seam. list_supervisable_artifacts filters the session/task join to live, task-linked, non-terminal artifacts. New daemon_reconcile_seconds config knob. bach daemon run wires the seams with SIGTERM/SIGINT clean shutdown. Closes #12 Co-Authored-By: Claude Fable 5 --- src/bach/cli/app.py | 3 + src/bach/cli/daemon_cmd.py | 166 ++++++++++++ src/bach/config/settings.py | 18 +- src/bach/services/daemon_supervisor.py | 285 ++++++++++++++++++++ src/bach/services/sessions_service.py | 59 +++++ tests/unit/test_config_set.py | 11 +- tests/unit/test_daemon_cmd.py | 71 +++++ tests/unit/test_daemon_config.py | 83 ++++++ tests/unit/test_daemon_sessions_filter.py | 212 +++++++++++++++ tests/unit/test_daemon_supervisor.py | 303 ++++++++++++++++++++++ 10 files changed, 1207 insertions(+), 4 deletions(-) create mode 100644 src/bach/cli/daemon_cmd.py create mode 100644 src/bach/services/daemon_supervisor.py create mode 100644 tests/unit/test_daemon_cmd.py create mode 100644 tests/unit/test_daemon_config.py create mode 100644 tests/unit/test_daemon_sessions_filter.py create mode 100644 tests/unit/test_daemon_supervisor.py diff --git a/src/bach/cli/app.py b/src/bach/cli/app.py index 24576d3..b4b3f4e 100644 --- a/src/bach/cli/app.py +++ b/src/bach/cli/app.py @@ -7,6 +7,7 @@ import typer from rich.console import Console +from bach.cli.daemon_cmd import daemon_app from bach.cli.hooks_cmd import hooks_app from bach.cli.internal_sessions import register_internal_sessions from bach.cli.issue import issue_app @@ -73,6 +74,8 @@ # like issue_app and hooks_app. The old try/except guard was for partial-phase dev. app.add_typer(sessions_app, name="sessions") app.add_typer(hooks_app, name="hooks") +# Phase 17 (issue #12): daemon supervisor sub-app. +app.add_typer(daemon_app, name="daemon") # Internal hook-event + session-watch + session-stop commands. register_internal_sessions(internal_app) # Phase 16 L1: `bach afk run` sub-app. afk_runner.py (L2) provides the logic. diff --git a/src/bach/cli/daemon_cmd.py b/src/bach/cli/daemon_cmd.py new file mode 100644 index 0000000..9b26412 --- /dev/null +++ b/src/bach/cli/daemon_cmd.py @@ -0,0 +1,166 @@ +"""Typer sub-app for `bach daemon ...` (Phase 17, issue #12). + +Subcommands (implemented here): + run — foreground supervisor: keeps one watcher alive per live task session. + +Future subcommands (issues #13 and #14 will add them to this file): + status — print supervisor state + stop — gracefully stop a running daemon + install — install the daemon as a launchd/systemd service + uninstall — remove the service + +Registration: wired in cli/app.py via: + app.add_typer(daemon_app, name="daemon") + +Architecture notes: + - Handler is THIN: just loads config, wires seams, calls run_supervisor. + - All heavy logic lives in services/daemon_supervisor.py (loop + reconcile). + - SIGTERM/SIGINT handler flips a flag; should_stop_fn checks the flag — + the loop exits cleanly at the top of the next cycle. + - enumerate_desired_fn wraps list_supervisable_artifacts, which builds the + row list fresh each cycle using the live session discovery + artifact join. + - No real sleep / real process spawning in tests — all seams are injected. +""" + +import logging +import signal +import threading +import time +from pathlib import Path + +import typer + +from bach.config.paths import bach_home +from bach.config.settings import load_config +from bach.runtimes.watcher_processes import ( + list_supervised_artifacts, + spawn_watcher, +) +from bach.services.daemon_supervisor import run_supervisor + +logger = logging.getLogger(__name__) + +# Typer sub-app mounted at `bach daemon` by cli/app.py. +daemon_app = typer.Typer(help="Background daemon supervisor for session watchers.") + + +# --------------------------------------------------------------------------- +# `bach daemon run` +# --------------------------------------------------------------------------- + + +@daemon_app.command("run") +def daemon_run() -> None: + """Start the foreground daemon supervisor. + + Monitors all live, task-linked sessions and ensures each has an active + watcher process. Exits cleanly on SIGTERM or SIGINT (Ctrl-C). + + This is a FOREGROUND command — it runs until interrupted. For a background + deployment, wrap it in a process supervisor (launchd, systemd, etc.). + Use `daemon-reconcile-seconds` config knob to tune the cycle interval. + + Examples: + bach daemon run # defaults from ~/.bach/config.yaml + bach config set daemon-reconcile-seconds 10 + bach daemon run # faster cycles + """ + cfg = load_config() + + # --------------------------------------------------------------------------- + # Clean shutdown: a flag shared between the SIGTERM/SIGINT handler and the + # should_stop_fn that the supervisor loop checks each cycle. + # --------------------------------------------------------------------------- + _stop_event = threading.Event() + + def _signal_handler(signum: int, frame: object) -> None: + """Set the stop flag; the supervisor loop exits at the next cycle top.""" + logger.info("event=daemon_signal_received signum=%d", signum) + _stop_event.set() + + # Register for both SIGTERM (systemd/launchd stop) and SIGINT (Ctrl-C). + signal.signal(signal.SIGTERM, _signal_handler) + signal.signal(signal.SIGINT, _signal_handler) + + def should_stop() -> bool: + return _stop_event.is_set() + + # --------------------------------------------------------------------------- + # enumerate_desired_fn: builds the desired set each cycle using live + # session discovery joined with project artifacts, then filters to + # supervisable (task-linked, not ended, not terminal status). + # --------------------------------------------------------------------------- + def enumerate_desired() -> set[Path]: + """Enumerate artifacts that need a live watcher this cycle.""" + from datetime import UTC, datetime # noqa: PLC0415 + + from bach.config.projects import ProjectRegistry # noqa: PLC0415 + from bach.runtimes.session_discovery import ( # noqa: PLC0415 + default_process_probe, + discover_sessions, + ) + from bach.runtimes.transcript import read_tail_events # noqa: PLC0415 + from bach.services.sessions_service import ( # noqa: PLC0415 + list_sessions, + list_supervisable_artifacts, + scan_registry_artifact_paths, + ) + + registry = ProjectRegistry.default() + artifact_paths = scan_registry_artifact_paths(registry) + + # Discover live sessions with the same thresholds as `bach sessions list`. + current_cfg = load_config() + discovered_sessions = discover_sessions( + claude_projects_dir=Path.home() / ".claude" / "projects", + codex_home=Path.home() / ".codex", + process_probe=default_process_probe, + now=datetime.now(UTC), + running_threshold_s=current_cfg.liveness_running_seconds, + idle_threshold_s=current_cfg.liveness_idle_minutes * 60, + ) + + # Join sessions with artifacts to get SessionRows. + rows = list_sessions( + sessions_dir=bach_home() / "sessions", + artifacts_dir=bach_home(), + discover_fn=lambda: discovered_sessions, + read_events_fn=read_tail_events, + artifact_paths=artifact_paths, + # Include all non-ended sessions — filter to supervisable below. + max_ended_age_s=0, # exclude ended immediately + ) + + return list_supervisable_artifacts(rows=rows) + + # --------------------------------------------------------------------------- + # spawn_fn: wrap spawn_watcher (which returns (ok, log_path)) into a simple + # callable for the supervisor. Log the outcome; never raise. + # --------------------------------------------------------------------------- + def spawn(artifact: Path) -> None: + """Spawn a detached watcher for the given artifact; log the result.""" + ok, log_path = spawn_watcher(artifact) + if ok: + logger.info( + "event=daemon_watcher_spawned artifact=%s log=%s", + artifact, + log_path, + ) + else: + logger.warning("event=daemon_watcher_spawn_failed artifact=%s", artifact) + + logger.info( + "event=daemon_run_start reconcile_seconds=%d", + cfg.daemon_reconcile_seconds, + ) + + run_supervisor( + enumerate_desired_fn=enumerate_desired, + list_supervised_fn=list_supervised_artifacts, + spawn_fn=spawn, + sleep_fn=time.sleep, + should_stop_fn=should_stop, + reconcile_seconds=cfg.daemon_reconcile_seconds, + ) + + logger.info("event=daemon_run_done") diff --git a/src/bach/config/settings.py b/src/bach/config/settings.py index 8d91ad8..945293c 100644 --- a/src/bach/config/settings.py +++ b/src/bach/config/settings.py @@ -108,6 +108,14 @@ class BachConfig: # Hard time budget (minutes) for the AFK runner. Enforced alongside # afk_max_turns — whichever limit fires first stops the loop. afk_time_budget_minutes: int = 60 + # --------------------------------------------------------------------------- + # Phase 17 — Daemon supervisor (issue #12) + # --------------------------------------------------------------------------- + # Seconds between reconcile cycles in `bach daemon run`. A shorter interval + # means new sessions are picked up faster; a longer interval reduces process + # table scan overhead. 30s is a sensible default — new sessions are adopted + # within one cycle, and one extra scan per 30s is negligible overhead. + daemon_reconcile_seconds: int = 30 def _default_config_path() -> Path: @@ -179,6 +187,9 @@ def load_config(path: Path | None = None) -> BachConfig: afk_time_budget_minutes = _coerce_positive_int( parsed.get("afk_time_budget_minutes"), "afk_time_budget_minutes", default=60 ) + daemon_reconcile_seconds = _coerce_positive_int( + parsed.get("daemon_reconcile_seconds"), "daemon_reconcile_seconds", default=30 + ) config = BachConfig( iterm_layout=layout, concurrency=concurrency, @@ -194,6 +205,7 @@ def load_config(path: Path | None = None) -> BachConfig: liveness_idle_minutes=liveness_idle_minutes, afk_max_turns=afk_max_turns, afk_time_budget_minutes=afk_time_budget_minutes, + daemon_reconcile_seconds=daemon_reconcile_seconds, ) logger.info( @@ -201,7 +213,8 @@ def load_config(path: Path | None = None) -> BachConfig: "runtime_default=%s normalize_model=%s theme=%s user_name=%s " "watch_on_launch=%s observer_moves=%s judge_confidence_threshold=%s " "judge_interval_seconds=%d liveness_running_seconds=%d " - "liveness_idle_minutes=%d afk_max_turns=%d afk_time_budget_minutes=%d", + "liveness_idle_minutes=%d afk_max_turns=%d afk_time_budget_minutes=%d " + "daemon_reconcile_seconds=%d", config_path, config.iterm_layout.value, config.concurrency, @@ -217,6 +230,7 @@ def load_config(path: Path | None = None) -> BachConfig: config.liveness_idle_minutes, config.afk_max_turns, config.afk_time_budget_minutes, + config.daemon_reconcile_seconds, ) return config @@ -517,6 +531,8 @@ def _parse_positive_int(raw: str) -> int: "liveness-idle-minutes": _parse_positive_int, "afk-max-turns": _parse_positive_int, "afk-time-budget-minutes": _parse_positive_int, + # Phase 17 — daemon supervisor cycle interval (issue #12) + "daemon-reconcile-seconds": _parse_positive_int, } # Map CLI-spelling (hyphen) → YAML/dataclass field name (underscore). diff --git a/src/bach/services/daemon_supervisor.py b/src/bach/services/daemon_supervisor.py new file mode 100644 index 0000000..f768a49 --- /dev/null +++ b/src/bach/services/daemon_supervisor.py @@ -0,0 +1,285 @@ +"""Foreground daemon supervisor that keeps one watcher alive per live task session. + +Architecture: + - Single-threaded synchronous reconcile loop. NO threads, NO asyncio. + - Desired set is computed fresh each cycle by enumerate_desired_fn. + - Supervised set is the process-table snapshot from list_supervised_fn. + - Gap = desired - supervised → spawn_fn is called for each. + - Watchers self-terminate when their session ends; ended sessions simply + drop out of desired, so there is NO kill path here. + - Bounded exponential crash backoff prevents respawning a dying watcher + every cycle. Backoff state is in-memory and advisory — losing it on + restart results in at most one extra spawn attempt. + +Public API: + BackoffState — type alias for the per-artifact backoff tracking dict. + SpawnPlan — named result of reconcile(), holding the to_spawn set. + reconcile() — pure function, no I/O; determines what to spawn this cycle. + run_supervisor() — the loop; every side-effecting dep is injected. + +Injected seams in run_supervisor (all keyword-only, all have real defaults +so the CLI wires them in one place and tests inject fakes): + enumerate_desired_fn — returns set[Path] of artifact paths that need a watcher. + list_supervised_fn — returns set[Path] of artifacts with a live watcher. + spawn_fn — called with a Path to start a detached watcher. + sleep_fn — pauses between cycles (injected so tests don't block). + should_stop_fn — returns True when the loop must exit. + reconcile_seconds — sleep duration between cycles. +""" + +import logging +import time +from collections.abc import Callable +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any + +logger = logging.getLogger(__name__) + +# --------------------------------------------------------------------------- +# Backoff constants +# --------------------------------------------------------------------------- + +# Initial cooldown in seconds after a first failed spawn attempt. +_BACKOFF_INITIAL_S: float = 10.0 +# Multiplier applied on each successive failed attempt. +_BACKOFF_MULTIPLIER: float = 2.0 +# Hard cap so backoff never grows beyond this. +_BACKOFF_MAX_S: float = 300.0 # 5 minutes + + +# --------------------------------------------------------------------------- +# Public types +# --------------------------------------------------------------------------- + +# BackoffState maps artifact Path → per-artifact tracking dict. +# Keys in the inner dict: +# "last_spawn_at" — float monotonic clock value of the last spawn attempt. +# "attempt" — int count of consecutive failed attempts (watcher kept dying). +# "cooldown" — float seconds to wait before next spawn attempt. +# Using Any for the inner dict value types to keep mypy simple without a +# nested TypedDict — this is internal state, not part of the public API. +BackoffState = dict[Path, dict[str, Any]] + + +@dataclass(frozen=True) +class SpawnPlan: + """Result of a reconcile() call: the set of artifacts to spawn this cycle. + + Frozen so callers cannot mutate it after reconcile() returns. + `to_spawn` is a plain set — order doesn't matter; all are independent. + """ + + to_spawn: set[Path] = field(default_factory=set) + + +# --------------------------------------------------------------------------- +# Pure reconcile — no I/O, no side effects, fully unit-testable +# --------------------------------------------------------------------------- + + +def reconcile( + desired: set[Path], + supervised: set[Path], + backoff: BackoffState, + now: float, +) -> SpawnPlan: + """Determine which artifacts need a new watcher spawned this cycle. + + Rules (applied in order): + 1. Only artifacts in `desired` are candidates — ended sessions have + already been filtered from `desired` by the caller. + 2. Artifacts already in `supervised` are skipped — the duplicate-watcher + invariant; only one watcher per artifact at a time. + 3. Artifacts in backoff cooldown are skipped until the window expires. + 4. All remaining candidates → to_spawn. + + The backoff dict is READ but NOT mutated here. Mutation happens in + run_supervisor after each spawn attempt so the reconcile step stays pure. + + Args: + desired: artifacts that need a live watcher (not ended, not terminal). + supervised: artifacts with an already-running watcher process. + backoff: per-artifact crash backoff tracking (read-only here). + now: current monotonic clock value for backoff comparison. + + Returns: + SpawnPlan with the set of artifacts to spawn this cycle. + """ + to_spawn: set[Path] = set() + + for artifact in desired: + # Duplicate-watcher invariant: skip if already supervised. + if artifact in supervised: + continue + + # Backoff check: if we have a cooldown record and it hasn't expired, skip. + entry = backoff.get(artifact) + if entry is not None: + last_spawn = entry["last_spawn_at"] + cooldown = entry["cooldown"] + elapsed = now - last_spawn + if elapsed < cooldown: + logger.debug( + "event=daemon_backoff_skip artifact=%s elapsed=%.1fs cooldown=%.1fs", + artifact, + elapsed, + cooldown, + ) + continue + + to_spawn.add(artifact) + + return SpawnPlan(to_spawn=to_spawn) + + +# --------------------------------------------------------------------------- +# Backoff state updater — called after each spawn attempt +# --------------------------------------------------------------------------- + + +def _update_backoff_after_spawn(artifact: Path, backoff: BackoffState, now: float) -> None: + """Record a spawn attempt in the backoff state. + + On the first attempt for an artifact, creates a fresh entry with the + initial cooldown. On each successive attempt, doubles the cooldown + (bounded by _BACKOFF_MAX_S). + + The backoff entry is cleared externally (when the artifact drops from + desired, meaning its session ended naturally) — we do NOT clear it here + because reconcile's next cycle will see `desired` without this artifact + and simply never check its backoff entry. + + Args: + artifact: the artifact path that was just spawned. + backoff: mutable BackoffState dict — updated in place. + now: current monotonic clock value. + """ + entry = backoff.get(artifact) + if entry is None: + # First attempt: create a fresh entry. + backoff[artifact] = { + "last_spawn_at": now, + "attempt": 1, + "cooldown": _BACKOFF_INITIAL_S, + } + else: + # Successive attempt: increase attempt count and double cooldown. + new_attempt = entry["attempt"] + 1 + new_cooldown = min(entry["cooldown"] * _BACKOFF_MULTIPLIER, _BACKOFF_MAX_S) + backoff[artifact] = { + "last_spawn_at": now, + "attempt": new_attempt, + "cooldown": new_cooldown, + } + logger.info( + "event=daemon_backoff_updated artifact=%s attempt=%d cooldown=%.1fs", + artifact, + new_attempt, + new_cooldown, + ) + + +# --------------------------------------------------------------------------- +# Supervisor loop — all side effects injected +# --------------------------------------------------------------------------- + + +def run_supervisor( + *, + enumerate_desired_fn: Callable[[], set[Path]], + list_supervised_fn: Callable[[], set[Path]], + spawn_fn: Callable[[Path], Any], + sleep_fn: Callable[[float], None] = time.sleep, + should_stop_fn: Callable[[], bool], + reconcile_seconds: float, +) -> None: + """Run the foreground supervisor loop. + + Each cycle: + 1. Check should_stop_fn — exit cleanly if True. + 2. Enumerate desired artifacts (task-linked, not ended, not terminal). + 3. List currently supervised artifacts (process table scan). + 4. Reconcile to find the gap (desired - supervised, minus backoff). + 5. Spawn a watcher for each artifact in the gap. + 6. Sleep reconcile_seconds. + 7. Repeat. + + Watchers self-terminate when their sessions end — there is no kill path. + An artifact whose watcher keeps crashing is handled by the backoff state + so we back off exponentially instead of hammering the process table. + + All side-effecting deps are keyword-only args so the CLI wires them once + and tests inject fakes without touching real processes or real time. + + Args: + enumerate_desired_fn: returns set[Path] of artifacts needing a watcher. + list_supervised_fn: returns set[Path] of already-supervised artifacts. + spawn_fn: called with a Path to start a detached watcher. + sleep_fn: sleep between cycles (real: time.sleep, fake in tests). + should_stop_fn: returns True when the loop must exit cleanly. + reconcile_seconds: sleep duration; also logged per cycle for diagnostics. + """ + # In-memory backoff state. Advisory: losing it on restart = at most one + # extra spawn attempt before backoff re-accumulates. + backoff: BackoffState = {} + + logger.info( + "event=daemon_supervisor_start reconcile_seconds=%s", + reconcile_seconds, + ) + + while True: + # Enumerate desired artifacts for this cycle. + try: + desired = enumerate_desired_fn() + except Exception as exc: # noqa: BLE001 — best-effort; log and continue + logger.warning("event=daemon_enumerate_error reason=%s", type(exc).__name__) + desired = set() + + # Get the current supervised set from the process table. + try: + supervised = list_supervised_fn() + except Exception as exc: # noqa: BLE001 + logger.warning("event=daemon_supervised_error reason=%s", type(exc).__name__) + supervised = set() + + # Monotonic clock for backoff comparison. + now = time.monotonic() + plan = reconcile(desired, supervised, backoff, now) + + # Spawn watchers for the gap. + for artifact in plan.to_spawn: + logger.info("event=daemon_spawn artifact=%s", artifact) + try: + spawn_fn(artifact) + # Record the spawn attempt in backoff state so a watcher that + # crashes immediately doesn't get respawned on the very next cycle. + _update_backoff_after_spawn(artifact, backoff, now) + except Exception as exc: # noqa: BLE001 — warn-not-fail + logger.warning( + "event=daemon_spawn_error artifact=%s reason=%s", + artifact, + type(exc).__name__, + ) + _update_backoff_after_spawn(artifact, backoff, now) + + logger.debug( + "event=daemon_cycle desired=%d supervised=%d spawned=%d", + len(desired), + len(supervised), + len(plan.to_spawn), + ) + + # Check stop signal AFTER doing work — ensures at least one full cycle + # runs, and that a stop received during a cycle exits cleanly without + # sleeping again. This matches the test contract: enumerate is always + # called before the first stop check. + if should_stop_fn(): + logger.info("event=daemon_supervisor_stop reason=should_stop") + break + + # Sleep before next cycle. + sleep_fn(reconcile_seconds) + + logger.info("event=daemon_supervisor_done") diff --git a/src/bach/services/sessions_service.py b/src/bach/services/sessions_service.py index a3b41e8..5dde132 100644 --- a/src/bach/services/sessions_service.py +++ b/src/bach/services/sessions_service.py @@ -669,3 +669,62 @@ def close_session( pids[0], ) return CloseResult(session.session_id, pids[0], [], "terminated") + + +# --------------------------------------------------------------------------- +# Daemon supervisor enumeration — Phase 17 (issue #12) +# --------------------------------------------------------------------------- + +# Terminal statuses: tasks in these states do not need a live watcher. +# Matches VALID_STATUSES terminal states in status_service.py. +_TERMINAL_STATUSES: frozenset[str] = frozenset({"done", "carried_forward"}) + + +def list_supervisable_artifacts(rows: list[SessionRow]) -> set[Path]: + """Return artifact Paths that the daemon supervisor should keep watched. + + Filters the given rows to only those that: + 1. Are task-linked (task_id is not None AND artifact_path is not None). + 2. Have a session liveness that is NOT ended — ended sessions are gone. + 3. Have a task status that is NOT in _TERMINAL_STATUSES (done/carried_forward). + + The status is read from the artifact's frontmatter (via _read_artifact_frontmatter) + so the check reflects the latest on-disk state, not a stale row snapshot. + An unreadable artifact is treated as having an unknown (non-terminal) status and + is included rather than silently skipped — the watcher will log the missing + artifact and exit itself, which is the correct behavior. + + Args: + rows: list of SessionRow as produced by list_sessions(). The caller is + responsible for building the row list (e.g. from scan_registry_artifact_paths). + + Returns: + Set of artifact Paths that need a live watcher process. + """ + result: set[Path] = set() + + for row in rows: + # Must be task-linked to be supervisable. + if row.task_id is None or row.artifact_path is None: + continue + + # Ended sessions have no live process — nothing to watch. + if row.session.liveness is SessionLiveness.ended: + continue + + # Check task status against terminal states. + payload = _read_artifact_frontmatter(row.artifact_path) + if payload is not None: + status = str(payload.get("status", "")) + if status in _TERMINAL_STATUSES: + logger.debug( + "event=supervisable_skip_terminal artifact=%s status=%s", + row.artifact_path, + status, + ) + continue + + result.add(row.artifact_path) + + logger.debug("event=list_supervisable_artifacts count=%d", len(result)) + return result diff --git a/tests/unit/test_config_set.py b/tests/unit/test_config_set.py index dc2a707..87dcfe7 100644 --- a/tests/unit/test_config_set.py +++ b/tests/unit/test_config_set.py @@ -35,9 +35,12 @@ # --------------------------------------------------------------------------- -def test_observer_settable_keys_has_eight_entries() -> None: - """The allowlist must contain exactly the 8 observer knobs specified in the DAG.""" - assert len(OBSERVER_SETTABLE_KEYS) == 8 +def test_observer_settable_keys_has_nine_entries() -> None: + """The allowlist must contain exactly 9 observer knobs. + + Phase 17 added daemon-reconcile-seconds to the original 8. + """ + assert len(OBSERVER_SETTABLE_KEYS) == 9 def test_observer_settable_keys_contains_expected_keys() -> None: @@ -51,6 +54,8 @@ def test_observer_settable_keys_contains_expected_keys() -> None: "liveness-idle-minutes", "afk-max-turns", "afk-time-budget-minutes", + # Phase 17 (issue #12) — daemon supervisor cycle interval + "daemon-reconcile-seconds", } assert set(OBSERVER_SETTABLE_KEYS.keys()) == expected diff --git a/tests/unit/test_daemon_cmd.py b/tests/unit/test_daemon_cmd.py new file mode 100644 index 0000000..c0ccf73 --- /dev/null +++ b/tests/unit/test_daemon_cmd.py @@ -0,0 +1,71 @@ +"""Smoke/wiring tests for cli/daemon_cmd.py. + +Tests that: + - `bach daemon run` is registered and callable via Typer. + - The `run` command wires run_supervisor with the correct seams. + - No real processes or real time.sleep are called in these tests. + - Clean SIGTERM handling: the stop event causes a clean exit. +""" + +from pathlib import Path +from unittest.mock import MagicMock, patch + +from typer.testing import CliRunner + +from bach.cli.app import app + +runner = CliRunner() + + +def test_daemon_run_is_registered() -> None: + """daemon run subcommand must appear in --help output.""" + result = runner.invoke(app, ["daemon", "--help"]) + assert result.exit_code == 0 + assert "run" in result.output + + +def test_daemon_run_wires_supervisor_and_exits_cleanly(tmp_path: Path) -> None: + """daemon run invokes run_supervisor; with a patched should_stop that fires + immediately, the command exits with code 0. + + We patch: + - run_supervisor (the core loop) — to verify it is called with the right args. + - list_supervisable_artifacts (the enumerate_desired seam). + - list_supervised_artifacts (the list_supervised seam). + - spawn_watcher (the spawn seam). + - load_config — returns a config pointing at tmp_path so no ~/.bach is touched. + """ + from bach.config.settings import BachConfig + + mock_supervisor = MagicMock() + + with ( + patch("bach.cli.daemon_cmd.run_supervisor", mock_supervisor), + patch("bach.cli.daemon_cmd.load_config", return_value=BachConfig()), + ): + result = runner.invoke(app, ["daemon", "run"]) + + # run_supervisor was called once + assert mock_supervisor.call_count == 1, f"Expected 1 call, got {mock_supervisor.call_count}" + + # Exit code must be 0 (clean) + assert result.exit_code == 0, f"Non-zero exit: {result.output}" + + +def test_daemon_run_passes_reconcile_seconds_from_config(tmp_path: Path) -> None: + """reconcile_seconds kwarg is sourced from cfg.daemon_reconcile_seconds.""" + from bach.config.settings import BachConfig + + custom_cfg = BachConfig(daemon_reconcile_seconds=99) + captured_kwargs: dict[str, object] = {} + + def fake_supervisor(**kwargs): # type: ignore[no-untyped-def] + captured_kwargs.update(kwargs) + + with ( + patch("bach.cli.daemon_cmd.run_supervisor", fake_supervisor), + patch("bach.cli.daemon_cmd.load_config", return_value=custom_cfg), + ): + runner.invoke(app, ["daemon", "run"]) + + assert captured_kwargs.get("reconcile_seconds") == 99 diff --git a/tests/unit/test_daemon_config.py b/tests/unit/test_daemon_config.py new file mode 100644 index 0000000..3a831a2 --- /dev/null +++ b/tests/unit/test_daemon_config.py @@ -0,0 +1,83 @@ +"""Tests for the daemon_reconcile_seconds config knob. + +Verifies: + - BachConfig has daemon_reconcile_seconds with a sensible default. + - load_config reads it from YAML correctly. + - Invalid values fall back to the default. + - set_observer_key("daemon-reconcile-seconds", ...) works via the allowlist. +""" + +from pathlib import Path + +from bach.config.settings import ( + _OBSERVER_KEY_TO_FIELD, + OBSERVER_SETTABLE_KEYS, + BachConfig, + load_config, + set_observer_key, +) + +# Default must be a sensible positive int (>=1). +_EXPECTED_DEFAULT = 30 + + +def test_daemon_reconcile_seconds_has_sensible_default() -> None: + """BachConfig() default is a positive int.""" + cfg = BachConfig() + assert cfg.daemon_reconcile_seconds >= 1 + assert cfg.daemon_reconcile_seconds == _EXPECTED_DEFAULT + + +def test_load_config_reads_daemon_reconcile_seconds(tmp_path: Path) -> None: + """A YAML file with daemon_reconcile_seconds: 60 → config value is 60.""" + config_path = tmp_path / "config.yaml" + config_path.write_text("daemon_reconcile_seconds: 60\n") + + cfg = load_config(path=config_path) + assert cfg.daemon_reconcile_seconds == 60 + + +def test_load_config_invalid_daemon_reconcile_falls_back(tmp_path: Path) -> None: + """Non-positive values fall back to default.""" + config_path = tmp_path / "config.yaml" + config_path.write_text("daemon_reconcile_seconds: 0\n") + + cfg = load_config(path=config_path) + assert cfg.daemon_reconcile_seconds == _EXPECTED_DEFAULT + + +def test_daemon_reconcile_seconds_in_observer_allowlist() -> None: + """daemon-reconcile-seconds must be in the observer settable keys allowlist.""" + assert "daemon-reconcile-seconds" in OBSERVER_SETTABLE_KEYS + + +def test_daemon_reconcile_seconds_field_mapping() -> None: + """The CLI key maps to the correct dataclass field name.""" + assert _OBSERVER_KEY_TO_FIELD["daemon-reconcile-seconds"] == "daemon_reconcile_seconds" + + +def test_set_observer_key_daemon_reconcile_seconds(tmp_path: Path) -> None: + """set_observer_key writes daemon_reconcile_seconds correctly.""" + from unittest.mock import patch + + with patch("bach.config.settings._default_config_path", return_value=tmp_path / "config.yaml"): + path = set_observer_key("daemon-reconcile-seconds", "45") + + cfg = load_config(path=path) + assert cfg.daemon_reconcile_seconds == 45 + + +def test_set_observer_key_rejects_zero() -> None: + """Zero is not a positive int — must raise ValueError.""" + import pytest + + with pytest.raises(ValueError, match="positive"): + set_observer_key("daemon-reconcile-seconds", "0") + + +def test_set_observer_key_rejects_negative() -> None: + """Negative values must raise ValueError.""" + import pytest + + with pytest.raises(ValueError, match="positive"): + set_observer_key("daemon-reconcile-seconds", "-5") diff --git a/tests/unit/test_daemon_sessions_filter.py b/tests/unit/test_daemon_sessions_filter.py new file mode 100644 index 0000000..0c32fbe --- /dev/null +++ b/tests/unit/test_daemon_sessions_filter.py @@ -0,0 +1,212 @@ +"""Tests for list_supervisable_artifacts in sessions_service.py. + +Verifies the filtering logic: + - Includes task-linked rows with non-ended liveness and non-terminal status. + - Excludes rows with no task_id (unlinked). + - Excludes rows whose session liveness is `ended`. + - Excludes rows whose task status is a terminal status (done, carried_forward). +""" + +from datetime import UTC, datetime +from pathlib import Path +from unittest.mock import patch + +from bach.domain.models import ( + AgentRuntime, + DiscoveredSession, + SessionLiveness, +) +from bach.services.sessions_service import SessionRow, list_supervisable_artifacts + + +def _make_session( + session_id: str, + liveness: SessionLiveness = SessionLiveness.running, + project_dir: Path | None = None, +) -> DiscoveredSession: + """Helper to build a DiscoveredSession for tests.""" + return DiscoveredSession( + runtime=AgentRuntime.claude_code, + session_id=session_id, + transcript_path=Path("/fake/transcript.jsonl"), + project_dir=project_dir or Path("/fake/project"), + liveness=liveness, + last_activity=datetime.now(UTC), + started_at=datetime.now(UTC), + ) + + +def _make_row( + session_id: str, + task_id: str | None, + artifact_path: Path | None, + liveness: SessionLiveness = SessionLiveness.running, + status: str = "executing", +) -> SessionRow: + """Helper to build a SessionRow for tests.""" + return SessionRow( + session=_make_session(session_id, liveness=liveness), + task_id=task_id, + artifact_path=artifact_path, + preview=None, + ) + + +# --------------------------------------------------------------------------- +# Helpers to simulate artifact frontmatter reads +# --------------------------------------------------------------------------- + + +def _patch_frontmatter(artifact_path: Path, status: str) -> dict[str, str]: + """Return a fake frontmatter dict for the given artifact path.""" + return {"status": status} + + +# --------------------------------------------------------------------------- +# Tests for list_supervisable_artifacts +# --------------------------------------------------------------------------- + + +def test_supervisable_includes_linked_running_artifact(tmp_path: Path) -> None: + """A task-linked, running, non-terminal row should be included.""" + artifact = tmp_path / "task-001.md" + artifact.write_text("---\nstatus: executing\n---\n") + + row = _make_row( + session_id="abc123", + task_id="t1", + artifact_path=artifact, + liveness=SessionLiveness.running, + status="executing", + ) + + with patch( + "bach.services.sessions_service._read_artifact_frontmatter", + return_value={"status": "executing"}, + ): + result = list_supervisable_artifacts(rows=[row]) + + assert artifact in result + + +def test_supervisable_excludes_unlinked_row(tmp_path: Path) -> None: + """A row with no task_id (no artifact) is excluded.""" + row = _make_row( + session_id="abc123", + task_id=None, + artifact_path=None, + liveness=SessionLiveness.running, + status="executing", + ) + + result = list_supervisable_artifacts(rows=[row]) + assert result == set() + + +def test_supervisable_excludes_ended_session(tmp_path: Path) -> None: + """A row whose session is ended is excluded.""" + artifact = tmp_path / "task-001.md" + artifact.write_text("---\nstatus: executing\n---\n") + + row = _make_row( + session_id="abc123", + task_id="t1", + artifact_path=artifact, + liveness=SessionLiveness.ended, + status="executing", + ) + + result = list_supervisable_artifacts(rows=[row]) + assert result == set() + + +def test_supervisable_excludes_done_task(tmp_path: Path) -> None: + """A task with status=done is terminal and excluded.""" + artifact = tmp_path / "task-001.md" + artifact.write_text("---\nstatus: done\n---\n") + + row = _make_row( + session_id="abc123", + task_id="t1", + artifact_path=artifact, + liveness=SessionLiveness.idle, + status="done", + ) + + with patch( + "bach.services.sessions_service._read_artifact_frontmatter", + return_value={"status": "done"}, + ): + result = list_supervisable_artifacts(rows=[row]) + + assert result == set() + + +def test_supervisable_excludes_carried_forward_task(tmp_path: Path) -> None: + """A task with status=carried_forward is terminal and excluded.""" + artifact = tmp_path / "task-002.md" + artifact.write_text("---\nstatus: carried_forward\n---\n") + + row = _make_row( + session_id="abc456", + task_id="t2", + artifact_path=artifact, + liveness=SessionLiveness.idle, + status="carried_forward", + ) + + with patch( + "bach.services.sessions_service._read_artifact_frontmatter", + return_value={"status": "carried_forward"}, + ): + result = list_supervisable_artifacts(rows=[row]) + + assert result == set() + + +def test_supervisable_includes_idle_non_terminal(tmp_path: Path) -> None: + """Idle but not ended — if task is non-terminal, include it.""" + artifact = tmp_path / "task-003.md" + artifact.write_text("---\nstatus: executing\n---\n") + + row = _make_row( + session_id="abc789", + task_id="t3", + artifact_path=artifact, + liveness=SessionLiveness.idle, + status="executing", + ) + + with patch( + "bach.services.sessions_service._read_artifact_frontmatter", + return_value={"status": "executing"}, + ): + result = list_supervisable_artifacts(rows=[row]) + + assert artifact in result + + +def test_supervisable_returns_set_of_paths(tmp_path: Path) -> None: + """Return type is a set of Paths (not rows, not session IDs).""" + a1 = tmp_path / "task-001.md" + a2 = tmp_path / "task-002.md" + a1.write_text("---\nstatus: executing\n---\n") + a2.write_text("---\nstatus: qa\n---\n") + + rows = [ + _make_row("s1", "t1", a1, SessionLiveness.running, "executing"), + _make_row("s2", "t2", a2, SessionLiveness.running, "qa"), + ] + + frontmatter_map = { + a1: {"status": "executing"}, + a2: {"status": "qa"}, + } + + def fake_read(path: Path) -> dict[str, str]: + return frontmatter_map.get(path, {}) + + with patch("bach.services.sessions_service._read_artifact_frontmatter", side_effect=fake_read): + result = list_supervisable_artifacts(rows=rows) + + assert result == {a1, a2} diff --git a/tests/unit/test_daemon_supervisor.py b/tests/unit/test_daemon_supervisor.py new file mode 100644 index 0000000..357b9fd --- /dev/null +++ b/tests/unit/test_daemon_supervisor.py @@ -0,0 +1,303 @@ +"""Tests for services/daemon_supervisor.py. + +Strategy: + - Test reconcile() as a pure function — no I/O. + - Test run_supervisor with fully injected fakes (no real processes, no real sleep). + - Use a fake clock for backoff time control. + - Verify the duplicate-watcher invariant holds strictly. + - Verify clean loop exit when should_stop_fn fires. +""" + +from pathlib import Path + +import pytest + +from bach.services.daemon_supervisor import ( + BackoffState, + reconcile, + run_supervisor, +) + +# --------------------------------------------------------------------------- +# reconcile — pure function tests +# --------------------------------------------------------------------------- + + +def test_reconcile_spawns_desired_not_supervised(tmp_path: Path) -> None: + """A desired artifact not yet in supervised set → plan includes it.""" + artifact = tmp_path / "task-001.md" + desired = {artifact} + supervised: set[Path] = set() + backoff: BackoffState = {} + now = 1000.0 + + plan = reconcile(desired, supervised, backoff, now) + + assert artifact in plan.to_spawn + + +def test_reconcile_no_spawn_when_already_supervised(tmp_path: Path) -> None: + """Duplicate-watcher invariant: already supervised → NOT spawned again.""" + artifact = tmp_path / "task-001.md" + desired = {artifact} + supervised = {artifact} # already has a live watcher + backoff: BackoffState = {} + now = 1000.0 + + plan = reconcile(desired, supervised, backoff, now) + + assert artifact not in plan.to_spawn + + +def test_reconcile_empty_desired_produces_no_spawn() -> None: + """No desired sessions → nothing to spawn.""" + plan = reconcile(set(), set(), {}, 1000.0) + assert plan.to_spawn == set() + + +def test_reconcile_spawns_only_the_gap(tmp_path: Path) -> None: + """Two desired, one already supervised → only the gap is spawned.""" + a1 = tmp_path / "task-001.md" + a2 = tmp_path / "task-002.md" + desired = {a1, a2} + supervised = {a2} # a2 already has a watcher + backoff: BackoffState = {} + now = 1000.0 + + plan = reconcile(desired, supervised, backoff, now) + + assert a1 in plan.to_spawn + assert a2 not in plan.to_spawn + + +def test_reconcile_respects_backoff(tmp_path: Path) -> None: + """An artifact in backoff cooldown is NOT re-spawned before window expires.""" + artifact = tmp_path / "task-001.md" + desired = {artifact} + supervised: set[Path] = set() + # Mark artifact as having been spawned very recently (t=900, now=1000). + # With a typical backoff window of >=30s, this should still be on cooldown. + backoff: BackoffState = { + artifact: {"last_spawn_at": 990.0, "attempt": 1, "cooldown": 60.0} + } + now = 1000.0 # only 10s elapsed, cooldown is 60s + + plan = reconcile(desired, supervised, backoff, now) + + assert artifact not in plan.to_spawn + + +def test_reconcile_spawns_after_backoff_expires(tmp_path: Path) -> None: + """An artifact whose backoff window has elapsed IS included in the plan.""" + artifact = tmp_path / "task-001.md" + desired = {artifact} + supervised: set[Path] = set() + backoff: BackoffState = { + artifact: {"last_spawn_at": 100.0, "attempt": 1, "cooldown": 60.0} + } + now = 200.0 # 100s elapsed, cooldown is 60s → expired + + plan = reconcile(desired, supervised, backoff, now) + + assert artifact in plan.to_spawn + + +def test_reconcile_no_backoff_on_first_spawn(tmp_path: Path) -> None: + """An artifact with no backoff entry at all is spawned immediately.""" + artifact = tmp_path / "task-001.md" + desired = {artifact} + supervised: set[Path] = set() + backoff: BackoffState = {} # no entry → first attempt, no delay + now = 1000.0 + + plan = reconcile(desired, supervised, backoff, now) + + assert artifact in plan.to_spawn + + +def test_reconcile_supervised_not_in_desired_produces_no_spawn(tmp_path: Path) -> None: + """Supervised but not desired (ended session) → not in to_spawn.""" + artifact = tmp_path / "task-ended.md" + desired: set[Path] = set() + supervised = {artifact} # watcher still alive but session ended + backoff: BackoffState = {} + now = 1000.0 + + plan = reconcile(desired, supervised, backoff, now) + + assert plan.to_spawn == set() + + +# --------------------------------------------------------------------------- +# run_supervisor — injected seams, fake clock, no real processes +# --------------------------------------------------------------------------- + + +def test_run_supervisor_exits_on_first_stop(tmp_path: Path) -> None: + """should_stop_fn returning True on first call → loop exits, no spawn.""" + calls: list[str] = [] + + def enumerate_desired() -> set[Path]: + calls.append("enumerate") + return set() + + def list_supervised() -> set[Path]: + return set() + + def spawn(artifact: Path) -> None: + calls.append(f"spawn:{artifact}") + + def sleep(seconds: float) -> None: + calls.append("sleep") + + stop_calls = [True] # stop immediately + + def should_stop() -> bool: + return stop_calls.pop(0) + + run_supervisor( + enumerate_desired_fn=enumerate_desired, + list_supervised_fn=list_supervised, + spawn_fn=spawn, + sleep_fn=sleep, + should_stop_fn=should_stop, + reconcile_seconds=1, + ) + + # enumerate was called once before stop check, sleep never reached + assert "enumerate" in calls + assert "sleep" not in calls + + +def test_run_supervisor_spawns_new_session(tmp_path: Path) -> None: + """A desired+unsupervised artifact gets spawned in the first cycle.""" + artifact = tmp_path / "task-001.md" + spawned: list[Path] = [] + cycle = [0] # mutable counter for stop logic + + def enumerate_desired() -> set[Path]: + # Only return the artifact on the first cycle + return {artifact} if cycle[0] == 0 else set() + + def list_supervised() -> set[Path]: + return set() + + def spawn(a: Path) -> None: + spawned.append(a) + + def sleep(seconds: float) -> None: + cycle[0] += 1 # advance to next cycle + + def should_stop() -> bool: + return cycle[0] >= 1 # stop after first cycle+sleep + + run_supervisor( + enumerate_desired_fn=enumerate_desired, + list_supervised_fn=list_supervised, + spawn_fn=spawn, + sleep_fn=sleep, + should_stop_fn=should_stop, + reconcile_seconds=1, + ) + + assert artifact in spawned + + +def test_run_supervisor_does_not_spawn_already_supervised(tmp_path: Path) -> None: + """Duplicate-watcher invariant: already supervised → spawn never called.""" + artifact = tmp_path / "task-001.md" + spawned: list[Path] = [] + cycle = [0] + + def enumerate_desired() -> set[Path]: + return {artifact} + + def list_supervised() -> set[Path]: + return {artifact} # already supervised + + def spawn(a: Path) -> None: + spawned.append(a) + + def sleep(seconds: float) -> None: + cycle[0] += 1 + + def should_stop() -> bool: + return cycle[0] >= 1 + + run_supervisor( + enumerate_desired_fn=enumerate_desired, + list_supervised_fn=list_supervised, + spawn_fn=spawn, + sleep_fn=sleep, + should_stop_fn=should_stop, + reconcile_seconds=1, + ) + + assert spawned == [] + + +def test_run_supervisor_stops_when_should_stop_returns_true(tmp_path: Path) -> None: + """Loop exits cleanly after should_stop_fn fires.""" + artifact = tmp_path / "task.md" + cycle = [0] + stop_after = 3 # run for 3 sleep cycles then stop + + def enumerate_desired() -> set[Path]: + return {artifact} + + def list_supervised() -> set[Path]: + return {artifact} # always supervised, so no spawning + + def spawn(a: Path) -> None: + pytest.fail("spawn should not be called when artifact is supervised") + + def sleep(seconds: float) -> None: + cycle[0] += 1 + + def should_stop() -> bool: + return cycle[0] >= stop_after + + run_supervisor( + enumerate_desired_fn=enumerate_desired, + list_supervised_fn=list_supervised, + spawn_fn=spawn, + sleep_fn=sleep, + should_stop_fn=should_stop, + reconcile_seconds=5, + ) + + assert cycle[0] == stop_after + + +def test_run_supervisor_uses_injected_sleep_duration(tmp_path: Path) -> None: + """sleep_fn is called with the configured reconcile_seconds.""" + sleep_durations: list[float] = [] + cycle = [0] + + def enumerate_desired() -> set[Path]: + return set() + + def list_supervised() -> set[Path]: + return set() + + def spawn(a: Path) -> None: + pass + + def sleep(seconds: float) -> None: + sleep_durations.append(seconds) + cycle[0] += 1 + + def should_stop() -> bool: + return cycle[0] >= 2 + + run_supervisor( + enumerate_desired_fn=enumerate_desired, + list_supervised_fn=list_supervised, + spawn_fn=spawn, + sleep_fn=sleep, + should_stop_fn=should_stop, + reconcile_seconds=42, + ) + + assert all(d == 42 for d in sleep_durations) + assert len(sleep_durations) == 2 From fff927a0c09fc7a89d4af20b08513d29a07d921a Mon Sep 17 00:00:00 2001 From: Aura - jc <67582323+Catafal@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:54:34 +0200 Subject: [PATCH 3/6] =?UTF-8?q?feat:=20daemon=5Flifecycle=20=E2=80=94=20pi?= =?UTF-8?q?dfile=20single-instance=20guard=20+=20status/stop=20(#13)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit acquire() writes our pid unless a live daemon is recorded (refuse), reclaiming a stale pidfile when the recorded pid is dead. bach daemon run acquires on start and clears the pidfile on clean shutdown. bach daemon status reports running state + the supervised artifact set (ps truth); bach daemon stop SIGTERMs the recorded pid. All side effects (kill, liveness probe, read) injected for temp-dir + fake testing. Closes #13 Co-Authored-By: Claude Fable 5 --- src/bach/cli/daemon_cmd.py | 164 ++++++++++++-- src/bach/services/daemon_lifecycle.py | 223 +++++++++++++++++++ tests/unit/test_daemon_lifecycle.py | 279 ++++++++++++++++++++++++ tests/unit/test_daemon_lifecycle_cmd.py | 207 ++++++++++++++++++ 4 files changed, 853 insertions(+), 20 deletions(-) create mode 100644 src/bach/services/daemon_lifecycle.py create mode 100644 tests/unit/test_daemon_lifecycle.py create mode 100644 tests/unit/test_daemon_lifecycle_cmd.py diff --git a/src/bach/cli/daemon_cmd.py b/src/bach/cli/daemon_cmd.py index 9b26412..80d3427 100644 --- a/src/bach/cli/daemon_cmd.py +++ b/src/bach/cli/daemon_cmd.py @@ -1,41 +1,51 @@ -"""Typer sub-app for `bach daemon ...` (Phase 17, issue #12). +"""Typer sub-app for `bach daemon ...` (Phase 17, issues #12/#13). Subcommands (implemented here): - run — foreground supervisor: keeps one watcher alive per live task session. + run — foreground supervisor: keeps one watcher alive per live task session. + status — report daemon running state + supervised artifacts. + stop — gracefully stop the running daemon via SIGTERM. -Future subcommands (issues #13 and #14 will add them to this file): - status — print supervisor state - stop — gracefully stop a running daemon - install — install the daemon as a launchd/systemd service +Future subcommands (issue #14): + install — install the daemon as a launchd/systemd service uninstall — remove the service Registration: wired in cli/app.py via: app.add_typer(daemon_app, name="daemon") Architecture notes: - - Handler is THIN: just loads config, wires seams, calls run_supervisor. + - Handler is THIN: just loads config, wires seams, calls service functions. - All heavy logic lives in services/daemon_supervisor.py (loop + reconcile). + - Single-instance guard lives in services/daemon_lifecycle.py. - SIGTERM/SIGINT handler flips a flag; should_stop_fn checks the flag — the loop exits cleanly at the top of the next cycle. - - enumerate_desired_fn wraps list_supervisable_artifacts, which builds the - row list fresh each cycle using the live session discovery + artifact join. - - No real sleep / real process spawning in tests — all seams are injected. + - `run` acquires single-instance ownership on start; clears pidfile on exit. + - `status` and `stop` delegate to daemon_lifecycle; handlers stay thin. + - No real sleep / real process spawning in tests — all seams are injectable. """ import logging +import os import signal import threading import time from pathlib import Path import typer +from rich.console import Console from bach.config.paths import bach_home from bach.config.settings import load_config +from bach.config.themes import THEMES, ThemePalette, get_theme from bach.runtimes.watcher_processes import ( list_supervised_artifacts, spawn_watcher, ) +from bach.services.daemon_lifecycle import ( + DaemonStatus, + acquire, + daemon_status, + stop, +) from bach.services.daemon_supervisor import run_supervisor logger = logging.getLogger(__name__) @@ -43,6 +53,21 @@ # Typer sub-app mounted at `bach daemon` by cli/app.py. daemon_app = typer.Typer(help="Background daemon supervisor for session watchers.") +_console = Console(highlight=False) + + +def _theme() -> ThemePalette: + """Load the active theme palette (same pattern as cli/sessions.py).""" + return get_theme(load_config().theme) or THEMES["default"] + + +def _pidfile_path() -> Path: + """Canonical path for the daemon pidfile under bach_home(). + + Centralised so tests can patch a single name to redirect all pidfile I/O. + """ + return bach_home() / "daemon.pid" + # --------------------------------------------------------------------------- # `bach daemon run` @@ -53,8 +78,12 @@ def daemon_run() -> None: """Start the foreground daemon supervisor. + Acquires single-instance ownership via a pidfile. Refuses to start if a + live daemon is already recorded — use `bach daemon stop` first. + Monitors all live, task-linked sessions and ensures each has an active - watcher process. Exits cleanly on SIGTERM or SIGINT (Ctrl-C). + watcher process. Exits cleanly on SIGTERM or SIGINT (Ctrl-C), clearing + the pidfile on shutdown. This is a FOREGROUND command — it runs until interrupted. For a background deployment, wrap it in a process supervisor (launchd, systemd, etc.). @@ -66,6 +95,26 @@ def daemon_run() -> None: bach daemon run # faster cycles """ cfg = load_config() + t = _theme() + pidfile = _pidfile_path() + + # --------------------------------------------------------------------------- + # Single-instance guard: refuse if a live daemon is already running. + # Inject the real liveness probe (default) — the test layer patches acquire. + # --------------------------------------------------------------------------- + ok, existing_pid = acquire( + pidfile=pidfile, + our_pid=os.getpid(), + ) + if not ok: + _console.print( + f"[{t.error}]error[/{t.error}] " + f"daemon is already running (pid {existing_pid}). " + "Use `bach daemon stop` to stop it first." + ) + raise typer.Exit(code=1) + + logger.info("event=daemon_acquire_ok pid=%d", os.getpid()) # --------------------------------------------------------------------------- # Clean shutdown: a flag shared between the SIGTERM/SIGINT handler and the @@ -154,13 +203,88 @@ def spawn(artifact: Path) -> None: cfg.daemon_reconcile_seconds, ) - run_supervisor( - enumerate_desired_fn=enumerate_desired, - list_supervised_fn=list_supervised_artifacts, - spawn_fn=spawn, - sleep_fn=time.sleep, - should_stop_fn=should_stop, - reconcile_seconds=cfg.daemon_reconcile_seconds, - ) + try: + run_supervisor( + enumerate_desired_fn=enumerate_desired, + list_supervised_fn=list_supervised_artifacts, + spawn_fn=spawn, + sleep_fn=time.sleep, + should_stop_fn=should_stop, + reconcile_seconds=cfg.daemon_reconcile_seconds, + ) + finally: + # Always clear the pidfile on exit so `daemon run` can restart cleanly. + pidfile.unlink(missing_ok=True) + logger.info("event=daemon_run_done") + + +# --------------------------------------------------------------------------- +# `bach daemon status` +# --------------------------------------------------------------------------- + - logger.info("event=daemon_run_done") +@daemon_app.command("status") +def daemon_status_cmd() -> None: + """Report whether the daemon is running and which artifacts it supervises. + + Shows: + - Running state (live pid or 'not running'). + - The set of task artifacts currently being watched. + + Examples: + bach daemon status + """ + t = _theme() + pidfile = _pidfile_path() + + status: DaemonStatus = daemon_status(pidfile=pidfile) + + if status.running: + _console.print(f"[{t.success}]running[/{t.success}] pid {status.pid}") + else: + _console.print(f"[{t.muted}]not running[/{t.muted}]") + + # Report the supervised artifact set regardless of daemon state so the + # user can see orphaned watchers if any are still alive. + supervised = list_supervised_artifacts() + if supervised: + _console.print(f"\n[{t.accent}]supervised artifacts ({len(supervised)}):[/{t.accent}]") + for artifact in sorted(supervised): + _console.print(f" {artifact.name}") + else: + _console.print(f"[{t.muted}]no artifacts currently supervised[/{t.muted}]") + + +# --------------------------------------------------------------------------- +# `bach daemon stop` +# --------------------------------------------------------------------------- + + +@daemon_app.command("stop") +def daemon_stop_cmd() -> None: + """Stop the running daemon by sending SIGTERM to the recorded pid. + + Reads the pidfile, checks liveness, and either: + - Sends SIGTERM to the live daemon (and clears the pidfile). + - Prints an informative message if no daemon is running. + + Safe to call even when no daemon is running — never raises. + + Examples: + bach daemon stop + """ + t = _theme() + pidfile = _pidfile_path() + + status: DaemonStatus = daemon_status(pidfile=pidfile) + + if not status.running: + _console.print(f"[{t.muted}]no daemon running[/{t.muted}]") + return + + # Delegate to daemon_lifecycle.stop for the actual SIGTERM + cleanup. + stop(pidfile=pidfile) + _console.print( + f"[{t.success}]stopped[/{t.success}] daemon (pid {status.pid})" + ) + logger.info("event=daemon_stop_cmd pid=%d", status.pid) diff --git a/src/bach/services/daemon_lifecycle.py b/src/bach/services/daemon_lifecycle.py new file mode 100644 index 0000000..c946b1c --- /dev/null +++ b/src/bach/services/daemon_lifecycle.py @@ -0,0 +1,223 @@ +"""Single-instance guard and lifecycle control for the Bach daemon (issue #13). + +Public API: + DaemonStatus — dataclass returned by daemon_status() + acquire() — write our pid to pidfile; refuses if a live daemon is already running + stop() — SIGTERM the recorded pid; clear the pidfile + daemon_status() — read pidfile + liveness check; return DaemonStatus + +Design: + - All side effects are injectable (liveness_probe, kill_fn, pidfile path). + Tests use tmp_path + fake probes; production code uses real os.kill semantics. + - Liveness probe receives a pid (int) and returns True if live, False if dead. + Default: os.kill(pid, 0) — raises OSError with errno.ESRCH if the pid is gone. + - Stale pidfiles (dead pid) are reclaimed automatically in acquire() so a crashed + daemon never permanently blocks a new one from starting. + - The pidfile path is always passed explicitly; callers typically pass + bach_home() / "daemon.pid" (see daemon_cmd.py). + +Invariants: + - acquire() is the ONLY writer. stop() is the only deleter. + - acquire() must not raise even if the pidfile is corrupted — it reclaims it. + - stop() must not raise even if pidfile is absent or pid is already dead. +""" + +import logging +import os +import signal +from collections.abc import Callable +from dataclasses import dataclass +from pathlib import Path + +logger = logging.getLogger(__name__) + + +# --------------------------------------------------------------------------- +# Domain type +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class DaemonStatus: + """Snapshot of the daemon's recorded state. + + Attributes: + running: True if a live daemon pid is recorded. + pid: The live pid, or None if no daemon is running. + """ + + running: bool + pid: int | None + + +# --------------------------------------------------------------------------- +# Internal helpers +# --------------------------------------------------------------------------- + + +def _default_liveness_probe(pid: int) -> bool: + """Return True if the process with 'pid' is alive. + + Uses os.kill(pid, 0) — signal 0 does not kill; it just checks existence. + Returns False for any OSError (no permission → treat as dead; ESRCH → dead). + Permissions errors (EPERM) mean the process EXISTS but we can't signal it, + so we treat it as live to be conservative (don't reclaim). + """ + try: + os.kill(pid, 0) + return True + except OSError as exc: + import errno # noqa: PLC0415 + + if exc.errno == errno.EPERM: + # Process exists but we lack permission → still alive. + return True + # ESRCH or anything else → process is gone. + return False + + +def _read_pidfile(pidfile: Path) -> int | None: + """Read and parse the pidfile; return pid int or None if absent/corrupt.""" + if not pidfile.exists(): + return None + try: + text = pidfile.read_text().strip() + if not text: + return None + return int(text) + except (ValueError, OSError): + logger.warning("event=daemon_pidfile_corrupt path=%s", pidfile) + return None + + +def _write_pidfile(pidfile: Path, pid: int) -> None: + """Write pid to the pidfile, creating parent directories as needed.""" + pidfile.parent.mkdir(parents=True, exist_ok=True) + pidfile.write_text(str(pid)) + + +def _clear_pidfile(pidfile: Path) -> None: + """Remove the pidfile if it exists; log but do not raise on failure.""" + try: + pidfile.unlink(missing_ok=True) + except OSError as exc: + logger.warning("event=daemon_pidfile_clear_failed path=%s reason=%s", pidfile, exc) + + +# --------------------------------------------------------------------------- +# Public API +# --------------------------------------------------------------------------- + + +def acquire( + *, + pidfile: Path, + our_pid: int, + liveness_probe: Callable[[int], bool] = _default_liveness_probe, +) -> tuple[bool, int]: + """Attempt to acquire single-instance ownership. + + Reads the pidfile (if any), checks liveness of the recorded pid, and either: + - Writes our_pid and returns (True, our_pid) when ownership is acquired. + - Returns (False, live_pid) when a live daemon is already running. + + Stale pidfiles (dead pid or corrupt content) are reclaimed automatically: + the file is overwritten with our_pid. + + Idempotent: if our_pid is already written AND is live, returns (True, our_pid). + + Args: + pidfile: Path to the pid file (e.g. bach_home() / "daemon.pid"). + our_pid: The pid of the calling process (typically os.getpid()). + liveness_probe: Callable(pid) → bool. Defaults to os.kill(pid, 0). + + Returns: + (True, our_pid) — ownership acquired. + (False, live_pid) — refused; live_pid is the existing daemon's pid. + """ + existing_pid = _read_pidfile(pidfile) + + if existing_pid is not None: + if existing_pid == our_pid: + # We already own the pidfile — idempotent acquire. + logger.debug("event=daemon_acquire_idempotent pid=%d", our_pid) + return True, our_pid + + if liveness_probe(existing_pid): + # Another live process holds the pidfile — refuse. + logger.info("event=daemon_acquire_refused existing_pid=%d", existing_pid) + return False, existing_pid + + # Existing pid is dead — stale; reclaim it. + logger.info("event=daemon_stale_pidfile_reclaimed stale_pid=%d", existing_pid) + + # No live daemon: write our pid. + _write_pidfile(pidfile, our_pid) + logger.info("event=daemon_acquire_ok pid=%d", our_pid) + return True, our_pid + + +def stop( + *, + pidfile: Path, + liveness_probe: Callable[[int], bool] = _default_liveness_probe, + kill_fn: Callable[[int, int], None] = lambda pid, sig: os.kill(pid, sig), +) -> None: + """Stop the running daemon by sending SIGTERM to the recorded pid. + + Clears the pidfile whether or not the process was live: + - Live pid → SIGTERM then clear. + - Stale pid → just clear (no kill). + - No pidfile → no-op. + + Never raises. All errors are logged as warnings. + + Args: + pidfile: Path to the pid file. + liveness_probe: Callable(pid) → bool. Defaults to os.kill(pid, 0). + kill_fn: Callable(pid, sig) → None. Defaults to os.kill. + """ + pid = _read_pidfile(pidfile) + + if pid is None: + logger.info("event=daemon_stop_no_pidfile path=%s", pidfile) + return + + if liveness_probe(pid): + # Process is live — send SIGTERM. + try: + kill_fn(pid, signal.SIGTERM) + logger.info("event=daemon_stop_sigterm pid=%d", pid) + except OSError as exc: + logger.warning("event=daemon_stop_kill_failed pid=%d reason=%s", pid, exc) + else: + logger.info("event=daemon_stop_stale_pid pid=%d", pid) + + # Always clear the pidfile after attempting to stop. + _clear_pidfile(pidfile) + + +def daemon_status( + *, + pidfile: Path, + liveness_probe: Callable[[int], bool] = _default_liveness_probe, +) -> DaemonStatus: + """Return the current daemon liveness status. + + Reads the pidfile and checks whether the recorded pid is alive. + Does NOT modify any state — purely read-only. + + Args: + pidfile: Path to the pid file. + liveness_probe: Callable(pid) → bool. Defaults to os.kill(pid, 0). + + Returns: + DaemonStatus(running=True, pid=N) — live daemon found. + DaemonStatus(running=False, pid=None) — no daemon or stale entry. + """ + pid = _read_pidfile(pidfile) + + if pid is not None and liveness_probe(pid): + return DaemonStatus(running=True, pid=pid) + + return DaemonStatus(running=False, pid=None) diff --git a/tests/unit/test_daemon_lifecycle.py b/tests/unit/test_daemon_lifecycle.py new file mode 100644 index 0000000..ce5e593 --- /dev/null +++ b/tests/unit/test_daemon_lifecycle.py @@ -0,0 +1,279 @@ +"""Tests for services/daemon_lifecycle.py — single-instance guard, stop, status. + +Strategy: + - All filesystem ops use tmp_path (never ~/.bach). + - kill_fn and liveness probe are injected fakes — no real processes. + - Tests follow TDD: written against the intended public API before implementation. + +Covered cases: + acquire: + - writes pidfile when no daemon is running → returns (True, our_pid) + - refuses when live daemon is already recorded → returns (False, live_pid) + - reclaims stale pidfile (dead pid) → writes our pid, returns (True, our_pid) + - idempotent: calling acquire again with our own pid → returns (True, our_pid) + + stop: + - SIGTERMs the recorded pid; clears the pidfile + - graceful when pidfile does not exist (no-op) + - graceful when recorded pid is stale (clears pidfile, no kill) + + daemon_status: + - returns (running=True, pid=N) when live daemon recorded + - returns (running=False, pid=None) when no pidfile + - returns (running=False, pid=None) when pidfile is stale +""" + +import os +from collections.abc import Callable +from pathlib import Path + +import pytest + +from bach.services.daemon_lifecycle import ( + DaemonStatus, + acquire, + daemon_status, + stop, +) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_OUR_PID = os.getpid() # real pid of the test process (always live) +_DEAD_PID = 99999999 # astronomically large — almost certainly not running + + +def _live_probe(pid: int) -> bool: + """Fake liveness probe: PID 1..9999 = live, anything else = dead.""" + return 1 <= pid <= 9999 + + +def _never_kill(pid: int, sig: int) -> None: + """Fake kill that raises if ever called (unexpected).""" + pytest.fail(f"kill({pid}, {sig}) was called unexpectedly") + + +def _recording_kill(calls: list[tuple[int, int]]) -> Callable[[int, int], None]: + """Return a kill_fn that records (pid, sig) pairs.""" + def _kill(pid: int, sig: int) -> None: + calls.append((pid, sig)) + return _kill + + +# --------------------------------------------------------------------------- +# acquire +# --------------------------------------------------------------------------- + + +def test_acquire_writes_pidfile_when_no_daemon(tmp_path: Path) -> None: + """First acquire: no pidfile → writes our pid, returns (True, our_pid).""" + pidfile = tmp_path / "daemon.pid" + + ok, recorded_pid = acquire( + pidfile=pidfile, + our_pid=42, + liveness_probe=_live_probe, + ) + + assert ok is True + assert recorded_pid == 42 + assert pidfile.exists() + assert pidfile.read_text().strip() == "42" + + +def test_acquire_refuses_when_live_daemon_running(tmp_path: Path) -> None: + """Live daemon already recorded → returns (False, live_pid), no overwrite.""" + pidfile = tmp_path / "daemon.pid" + live_pid = 1234 # within _live_probe's live range + + pidfile.write_text(str(live_pid)) + + ok, recorded_pid = acquire( + pidfile=pidfile, + our_pid=9999, + liveness_probe=_live_probe, + ) + + assert ok is False + assert recorded_pid == live_pid + # Pidfile must still hold the live pid, not be overwritten. + assert pidfile.read_text().strip() == str(live_pid) + + +def test_acquire_reclaims_stale_pidfile(tmp_path: Path) -> None: + """Pidfile exists but pid is dead → reclaim: write our pid, return (True, our_pid).""" + pidfile = tmp_path / "daemon.pid" + pidfile.write_text(str(_DEAD_PID)) # dead pid + + ok, recorded_pid = acquire( + pidfile=pidfile, + our_pid=42, + liveness_probe=_live_probe, + ) + + assert ok is True + assert recorded_pid == 42 + assert pidfile.read_text().strip() == "42" + + +def test_acquire_idempotent_own_pid(tmp_path: Path) -> None: + """Calling acquire with our own pid already written → treated as stale-or-live. + + If the recorded pid IS us (live), we own it — returns (True, our_pid). + This avoids refusing to 'start' when we already wrote the file. + """ + our_pid = 42 + pidfile = tmp_path / "daemon.pid" + pidfile.write_text(str(our_pid)) + + # Make our own pid appear live in the probe. + def probe_own_live(pid: int) -> bool: + return pid == our_pid + + ok, recorded_pid = acquire( + pidfile=pidfile, + our_pid=our_pid, + liveness_probe=probe_own_live, + ) + + assert ok is True + assert recorded_pid == our_pid + + +def test_acquire_creates_parent_dirs(tmp_path: Path) -> None: + """acquire must create missing parent directories for the pidfile.""" + pidfile = tmp_path / "subdir" / "daemon.pid" + + ok, _ = acquire( + pidfile=pidfile, + our_pid=42, + liveness_probe=_live_probe, + ) + + assert ok is True + assert pidfile.exists() + + +# --------------------------------------------------------------------------- +# stop +# --------------------------------------------------------------------------- + + +def test_stop_sigterms_recorded_pid(tmp_path: Path) -> None: + """stop() sends SIGTERM to the pid in the pidfile and clears it.""" + import signal + + pidfile = tmp_path / "daemon.pid" + live_pid = 1234 + pidfile.write_text(str(live_pid)) + + kill_calls: list[tuple[int, int]] = [] + + stop( + pidfile=pidfile, + liveness_probe=_live_probe, + kill_fn=_recording_kill(kill_calls), + ) + + assert (live_pid, signal.SIGTERM) in kill_calls + # Pidfile must be cleared after stop. + assert not pidfile.exists() or pidfile.read_text().strip() == "" + + +def test_stop_no_pidfile_is_graceful(tmp_path: Path) -> None: + """stop() when no pidfile exists must not raise.""" + pidfile = tmp_path / "daemon.pid" + assert not pidfile.exists() + + stop( + pidfile=pidfile, + liveness_probe=_live_probe, + kill_fn=_never_kill, # kill must NOT be called + ) + # No assertion needed beyond 'no exception raised'. + + +def test_stop_stale_pidfile_no_kill(tmp_path: Path) -> None: + """stop() with a stale pidfile: clears file, does NOT call kill.""" + pidfile = tmp_path / "daemon.pid" + pidfile.write_text(str(_DEAD_PID)) + + stop( + pidfile=pidfile, + liveness_probe=_live_probe, + kill_fn=_never_kill, # kill must NOT be called for dead pid + ) + + # File should be cleaned up. + assert not pidfile.exists() or pidfile.read_text().strip() == "" + + +def test_stop_clears_pidfile_after_kill(tmp_path: Path) -> None: + """After SIGTERM is sent, the pidfile is removed.""" + import signal + + pidfile = tmp_path / "daemon.pid" + pidfile.write_text("1234") + kill_calls: list[tuple[int, int]] = [] + + stop( + pidfile=pidfile, + liveness_probe=_live_probe, + kill_fn=_recording_kill(kill_calls), + ) + + assert len(kill_calls) == 1 + assert kill_calls[0] == (1234, signal.SIGTERM) + assert not pidfile.exists() + + +# --------------------------------------------------------------------------- +# daemon_status +# --------------------------------------------------------------------------- + + +def test_daemon_status_running_live_pid(tmp_path: Path) -> None: + """Live pid recorded → DaemonStatus(running=True, pid=N).""" + pidfile = tmp_path / "daemon.pid" + pidfile.write_text("5678") + + def probe(pid: int) -> bool: + return pid == 5678 + + status = daemon_status(pidfile=pidfile, liveness_probe=probe) + + assert isinstance(status, DaemonStatus) + assert status.running is True + assert status.pid == 5678 + + +def test_daemon_status_no_pidfile(tmp_path: Path) -> None: + """No pidfile → DaemonStatus(running=False, pid=None).""" + pidfile = tmp_path / "daemon.pid" + + status = daemon_status(pidfile=pidfile, liveness_probe=_live_probe) + + assert status.running is False + assert status.pid is None + + +def test_daemon_status_stale_pid(tmp_path: Path) -> None: + """Pidfile with dead pid → DaemonStatus(running=False, pid=None).""" + pidfile = tmp_path / "daemon.pid" + pidfile.write_text(str(_DEAD_PID)) + + status = daemon_status(pidfile=pidfile, liveness_probe=_live_probe) + + assert status.running is False + assert status.pid is None + + +def test_daemon_status_returns_dataclass(tmp_path: Path) -> None: + """Return type must be DaemonStatus with .running and .pid attributes.""" + pidfile = tmp_path / "daemon.pid" + + status = daemon_status(pidfile=pidfile, liveness_probe=_live_probe) + + assert hasattr(status, "running") + assert hasattr(status, "pid") diff --git a/tests/unit/test_daemon_lifecycle_cmd.py b/tests/unit/test_daemon_lifecycle_cmd.py new file mode 100644 index 0000000..9579280 --- /dev/null +++ b/tests/unit/test_daemon_lifecycle_cmd.py @@ -0,0 +1,207 @@ +"""CLI integration tests for daemon status, daemon stop, and run's single-instance guard. + +Tests: + daemon status: + - prints "running" + pid when daemon is live + - prints "not running" when no daemon is running + + daemon stop: + - prints "stopped" when live daemon is found and SIGTERM sent + - prints "no daemon running" when no pidfile + + bach daemon run (guard): + - refuses (non-zero exit) when a live daemon is already recorded + - starts normally (calls run_supervisor) when no prior daemon is running + - clears pidfile on clean shutdown +""" + +from pathlib import Path +from unittest.mock import MagicMock, patch + +from typer.testing import CliRunner + +from bach.cli.app import app +from bach.config.settings import BachConfig + +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# daemon status +# --------------------------------------------------------------------------- + + +def test_daemon_status_reports_running(tmp_path: Path) -> None: + """daemon status must say 'running' with the pid when daemon is live.""" + from bach.services.daemon_lifecycle import DaemonStatus + + live_status = DaemonStatus(running=True, pid=1234) + + with ( + patch("bach.cli.daemon_cmd.daemon_status", return_value=live_status), + patch("bach.cli.daemon_cmd.list_supervised_artifacts", return_value=set()), + patch("bach.cli.daemon_cmd._pidfile_path", return_value=tmp_path / "daemon.pid"), + ): + result = runner.invoke(app, ["daemon", "status"]) + + assert result.exit_code == 0, result.output + assert "1234" in result.output + # Some affirmative word about running state + assert any(w in result.output.lower() for w in ("running", "live", "active")) + + +def test_daemon_status_reports_not_running(tmp_path: Path) -> None: + """daemon status must say 'not running' when no live daemon is found.""" + from bach.services.daemon_lifecycle import DaemonStatus + + dead_status = DaemonStatus(running=False, pid=None) + + with ( + patch("bach.cli.daemon_cmd.daemon_status", return_value=dead_status), + patch("bach.cli.daemon_cmd.list_supervised_artifacts", return_value=set()), + patch("bach.cli.daemon_cmd._pidfile_path", return_value=tmp_path / "daemon.pid"), + ): + result = runner.invoke(app, ["daemon", "status"]) + + assert result.exit_code == 0, result.output + assert any(w in result.output.lower() for w in ("not running", "stopped", "no daemon")) + + +def test_daemon_status_shows_supervised_artifacts(tmp_path: Path) -> None: + """daemon status includes the supervised artifact set.""" + from bach.services.daemon_lifecycle import DaemonStatus + + live_status = DaemonStatus(running=True, pid=5678) + artifact = tmp_path / "task-001.md" + artifact.touch() + + with ( + patch("bach.cli.daemon_cmd.daemon_status", return_value=live_status), + patch( + "bach.cli.daemon_cmd.list_supervised_artifacts", + return_value={artifact}, + ), + patch("bach.cli.daemon_cmd._pidfile_path", return_value=tmp_path / "daemon.pid"), + ): + result = runner.invoke(app, ["daemon", "status"]) + + assert result.exit_code == 0 + assert "task-001" in result.output + + +# --------------------------------------------------------------------------- +# daemon stop +# --------------------------------------------------------------------------- + + +def test_daemon_stop_sends_sigterm_and_reports(tmp_path: Path) -> None: + """daemon stop calls stop() and prints a success message.""" + stop_called = [] + + def _fake_stop(*, pidfile: Path, **kwargs: object) -> None: + stop_called.append(pidfile) + + from bach.services.daemon_lifecycle import DaemonStatus + + live_status = DaemonStatus(running=True, pid=1234) + + with ( + patch("bach.cli.daemon_cmd.daemon_status", return_value=live_status), + patch("bach.cli.daemon_cmd.stop", side_effect=_fake_stop), + patch("bach.cli.daemon_cmd._pidfile_path", return_value=tmp_path / "daemon.pid"), + ): + result = runner.invoke(app, ["daemon", "stop"]) + + assert result.exit_code == 0, result.output + assert len(stop_called) == 1 + # Some word indicating success + assert any(w in result.output.lower() for w in ("stopped", "sent sigterm", "stop")) + + +def test_daemon_stop_no_daemon_graceful(tmp_path: Path) -> None: + """daemon stop with no live daemon: prints informative message, exit 0.""" + from bach.services.daemon_lifecycle import DaemonStatus + + dead_status = DaemonStatus(running=False, pid=None) + + with ( + patch("bach.cli.daemon_cmd.daemon_status", return_value=dead_status), + patch("bach.cli.daemon_cmd._pidfile_path", return_value=tmp_path / "daemon.pid"), + ): + result = runner.invoke(app, ["daemon", "stop"]) + + assert result.exit_code == 0, result.output + assert any(w in result.output.lower() for w in ("no daemon", "not running", "nothing")) + + +# --------------------------------------------------------------------------- +# daemon run — single-instance guard integration +# --------------------------------------------------------------------------- + + +def test_daemon_run_refuses_when_live_daemon(tmp_path: Path) -> None: + """daemon run exits non-zero when a live daemon is already recorded.""" + + # Simulate acquire failing (daemon already running) + def _fake_acquire(*, pidfile: Path, our_pid: int, **kwargs: object) -> tuple[bool, int]: + return False, 9999 + + with ( + patch("bach.cli.daemon_cmd.acquire", side_effect=_fake_acquire), + patch("bach.cli.daemon_cmd._pidfile_path", return_value=tmp_path / "daemon.pid"), + patch("bach.cli.daemon_cmd.load_config", return_value=BachConfig()), + ): + result = runner.invoke(app, ["daemon", "run"]) + + assert result.exit_code != 0, f"Expected non-zero exit, got 0. Output: {result.output}" + assert "9999" in result.output or any( + w in result.output.lower() for w in ("already running", "daemon is", "running") + ) + + +def test_daemon_run_starts_when_no_prior_daemon(tmp_path: Path) -> None: + """daemon run calls run_supervisor when acquire succeeds.""" + mock_supervisor = MagicMock() + + def _fake_acquire(*, pidfile: Path, our_pid: int, **kwargs: object) -> tuple[bool, int]: + # Simulate successful acquire + pidfile.parent.mkdir(parents=True, exist_ok=True) + pidfile.write_text(str(our_pid)) + return True, our_pid + + with ( + patch("bach.cli.daemon_cmd.acquire", side_effect=_fake_acquire), + patch("bach.cli.daemon_cmd._pidfile_path", return_value=tmp_path / "daemon.pid"), + patch("bach.cli.daemon_cmd.run_supervisor", mock_supervisor), + patch("bach.cli.daemon_cmd.load_config", return_value=BachConfig()), + ): + result = runner.invoke(app, ["daemon", "run"]) + + assert result.exit_code == 0, f"Unexpected non-zero exit. Output: {result.output}" + assert mock_supervisor.call_count == 1 + + +def test_daemon_run_clears_pidfile_on_clean_shutdown(tmp_path: Path) -> None: + """After run_supervisor exits, the pidfile must be cleared.""" + pidfile = tmp_path / "daemon.pid" + + def _fake_acquire(*, pidfile: Path, our_pid: int, **kwargs: object) -> tuple[bool, int]: + pidfile.parent.mkdir(parents=True, exist_ok=True) + pidfile.write_text(str(our_pid)) + return True, our_pid + + def _fake_supervisor(**kwargs: object) -> None: + # Supervisor runs and exits immediately. + pass + + with ( + patch("bach.cli.daemon_cmd.acquire", side_effect=_fake_acquire), + patch("bach.cli.daemon_cmd._pidfile_path", return_value=pidfile), + patch("bach.cli.daemon_cmd.run_supervisor", side_effect=_fake_supervisor), + patch("bach.cli.daemon_cmd.load_config", return_value=BachConfig()), + ): + result = runner.invoke(app, ["daemon", "run"]) + + assert result.exit_code == 0 + # Pidfile must be cleaned up after run. + assert not pidfile.exists() or pidfile.read_text().strip() == "" From de5853364971a52b0b4c77eed6391376d842cf87 Mon Sep 17 00:00:00 2001 From: Aura - jc <67582323+Catafal@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:59:09 +0200 Subject: [PATCH 4/6] =?UTF-8?q?feat:=20launchd=5Fservice=20=E2=80=94=20Lau?= =?UTF-8?q?nchAgent=20install/uninstall=20with=20printed=20fallback=20(#14?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit render_launch_agent emits a valid plist (plistlib) for com.bach.daemon invoking 'bach daemon run' with RunAtLoad + KeepAlive so supervision is on at login and self-restarts on crash. install/uninstall are idempotent (unload-before-overwrite) and drive launchctl through an injected runner. Any launchctl failure/unavailability surfaces a manual 'bach daemon run' fallback the CLI prints — honoring the iTerm-fallback non-negotiable. Closes #14 Co-Authored-By: Claude Fable 5 --- src/bach/cli/daemon_cmd.py | 85 +++++++ src/bach/services/launchd_service.py | 273 ++++++++++++++++++++++ tests/unit/test_daemon_launchd_cmd.py | 149 ++++++++++++ tests/unit/test_launchd_service.py | 324 ++++++++++++++++++++++++++ 4 files changed, 831 insertions(+) create mode 100644 src/bach/services/launchd_service.py create mode 100644 tests/unit/test_daemon_launchd_cmd.py create mode 100644 tests/unit/test_launchd_service.py diff --git a/src/bach/cli/daemon_cmd.py b/src/bach/cli/daemon_cmd.py index 80d3427..a606e40 100644 --- a/src/bach/cli/daemon_cmd.py +++ b/src/bach/cli/daemon_cmd.py @@ -47,6 +47,10 @@ stop, ) from bach.services.daemon_supervisor import run_supervisor +from bach.services.launchd_service import ( + install_launch_agent, + uninstall_launch_agent, +) logger = logging.getLogger(__name__) @@ -288,3 +292,84 @@ def daemon_stop_cmd() -> None: f"[{t.success}]stopped[/{t.success}] daemon (pid {status.pid})" ) logger.info("event=daemon_stop_cmd pid=%d", status.pid) + + +# --------------------------------------------------------------------------- +# `bach daemon install` +# --------------------------------------------------------------------------- + + +@daemon_app.command("install") +def daemon_install() -> None: + """Install the Bach daemon as a macOS LaunchAgent. + + Writes a plist to ~/Library/LaunchAgents/ and loads it via launchctl so + the daemon starts on login and self-restarts on crash (RunAtLoad + KeepAlive). + + This is an EXPLICIT install — nothing auto-installs the LaunchAgent. + Run `bach daemon uninstall` to remove it. + + If launchctl is unavailable (non-macOS, sandboxed environment, etc.) the + plist is still written and the manual fallback command is printed so you + always have a path to run the daemon. + + Examples: + bach daemon install + """ + t = _theme() + result = install_launch_agent() + + if not result.installed: + # Unexpected failure writing the plist itself — surface the fallback. + _console.print(f"[{t.error}]error[/{t.error}] failed to write LaunchAgent plist.") + if result.fallback_command: + _console.print( + f"[{t.muted}]Run manually (or under tmux):[/{t.muted}] " + f"{result.fallback_command}" + ) + logger.warning("event=daemon_install_failed") + raise typer.Exit(code=1) + + if result.fallback_command: + # Plist written but launchctl load failed — print fallback prominently. + _console.print( + f"[{t.accent}]installed[/{t.accent}] LaunchAgent plist written. " + "However, launchctl failed to load it automatically." + ) + _console.print( + f"[{t.muted}]Run manually (or under tmux / screen):[/{t.muted}] " + f"{result.fallback_command}" + ) + logger.warning( + "event=daemon_install_launchctl_failed fallback=%r", result.fallback_command + ) + else: + _console.print( + f"[{t.success}]installed[/{t.success}] Bach daemon LaunchAgent loaded successfully. " + "The daemon will start on login and self-restart on crash." + ) + logger.info("event=daemon_install_ok") + + +# --------------------------------------------------------------------------- +# `bach daemon uninstall` +# --------------------------------------------------------------------------- + + +@daemon_app.command("uninstall") +def daemon_uninstall() -> None: + """Remove the Bach daemon LaunchAgent. + + Unloads the agent from launchctl and deletes the plist file from + ~/Library/LaunchAgents/. Idempotent — safe to call when the agent is + not installed. + + Examples: + bach daemon uninstall + """ + t = _theme() + uninstall_launch_agent() + _console.print( + f"[{t.success}]uninstalled[/{t.success}] Bach daemon LaunchAgent removed." + ) + logger.info("event=daemon_uninstall_ok") diff --git a/src/bach/services/launchd_service.py b/src/bach/services/launchd_service.py new file mode 100644 index 0000000..081686c --- /dev/null +++ b/src/bach/services/launchd_service.py @@ -0,0 +1,273 @@ +"""macOS launchd LaunchAgent management for the Bach daemon. + +Provides idempotent install/uninstall of a LaunchAgent plist that keeps +`bach daemon run` alive across logins on macOS. + +Design mirrors hooks_service.py: + - `render_launch_agent` is a PURE function — no I/O, easy to test. + - `install_launch_agent` / `uninstall_launch_agent` accept injected `runner` + and `launch_agents_dir` so tests NEVER call real launchctl or touch + ~/Library/LaunchAgents. + - On launchctl failure, a `fallback_command` is returned (NEVER raise + unrecoverably) — callers print it so the user always has a manual path. + - Idempotent: calling install twice or uninstall on an absent plist is safe. + +Architecture rule: install is EXPLICIT — only triggered by `bach daemon install`. +Nothing auto-installs the LaunchAgent on import or first run. +""" + +import logging +import plistlib +import sys +from collections.abc import Callable +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from bach.config.paths import bach_home + +logger = logging.getLogger(__name__) + +# Stable reverse-DNS label for the LaunchAgent. Never change this — it is +# used as the plist filename, the launchctl service name, and for idempotency. +LAUNCH_AGENT_LABEL = "com.bach.daemon" + +# Type alias: runner takes an argv list and returns (returncode, output). +RunnerFn = Callable[[list[str]], tuple[int, str]] + +# The fallback command users can run manually if launchctl is unavailable. +_FALLBACK_CMD = "bach daemon run" + + +@dataclass(frozen=True) +class LaunchdInstallResult: + """Outcome of an install_launch_agent call. + + Attributes: + installed: True when the plist was written successfully. + fallback_command: Set when launchctl failed; the user should run this + command manually (or under tmux/screen). None on + full success. + """ + + installed: bool + fallback_command: str | None = None + + +def _default_launch_agents_dir() -> Path: + """Return the standard macOS LaunchAgents directory for the current user.""" + return Path.home() / "Library" / "LaunchAgents" + + +def _default_log_dir() -> Path: + """Return the Bach log directory for daemon stdout/stderr.""" + return bach_home() / "logs" + + +def render_launch_agent(*, log_dir: Path) -> str: + """Return a valid plist XML string for the Bach daemon LaunchAgent. + + The plist contains: + Label — LAUNCH_AGENT_LABEL (com.bach.daemon) + ProgramArguments — [sys.executable, "-m", "bach", "daemon", "run"] + (uses the current interpreter so no PATH dependency) + RunAtLoad — True (auto-start on login) + KeepAlive — True (restart on crash) + StandardOutPath — /daemon.stdout.log + StandardErrorPath — /daemon.stderr.log + + Args: + log_dir: Directory where stdout/stderr logs are written. The caller + is responsible for creating it if needed. + + Returns: + A plist XML string suitable for writing to disk. + """ + # Use sys.executable so the correct venv Python is invoked even when + # the launchd environment has no PATH — same rationale as hooks_service. + program_args = [sys.executable, "-m", "bach", "daemon", "run"] + + data: dict[str, Any] = { + "Label": LAUNCH_AGENT_LABEL, + "ProgramArguments": program_args, + "RunAtLoad": True, + "KeepAlive": True, + "StandardOutPath": str(log_dir / "daemon.stdout.log"), + "StandardErrorPath": str(log_dir / "daemon.stderr.log"), + } + + # plistlib.dumps produces valid Apple plist XML — no hand-rolled XML needed. + return plistlib.dumps(data, fmt=plistlib.FMT_XML).decode("utf-8") + + +def install_launch_agent( + *, + launch_agents_dir: Path | None = None, + log_dir: Path | None = None, + runner: RunnerFn | None = None, +) -> LaunchdInstallResult: + """Write the LaunchAgent plist and load it via launchctl. + + Idempotent: if the plist already exists it is overwritten with the current + config, then launchctl unloads the old instance before loading the new one + (prevents duplicate agent registration). + + On launchctl failure (unavailable binary, SIP restriction, etc.) the plist + IS still written to disk and a `fallback_command` is returned — the user + can load it manually or run `bach daemon run` under a process supervisor. + + Args: + launch_agents_dir: Override for ~/Library/LaunchAgents (tests use tmp_path). + log_dir: Override for log directory (defaults to bach_home()/logs). + runner: Injectable launchctl runner: (argv) -> (returncode, output). + Defaults to the real subprocess-based runner. + + Returns: + LaunchdInstallResult with installed=True on plist write success. + fallback_command is set when launchctl load failed. + """ + # Resolve defaults (injectable for tests). + resolved_dir = launch_agents_dir or _default_launch_agents_dir() + resolved_log_dir = log_dir or _default_log_dir() + resolved_runner = runner or _subprocess_runner + + # Ensure the LaunchAgents directory exists. + resolved_dir.mkdir(parents=True, exist_ok=True) + + # Ensure the log directory exists so launchd can open the log files. + resolved_log_dir.mkdir(parents=True, exist_ok=True) + + plist_path = resolved_dir / f"{LAUNCH_AGENT_LABEL}.plist" + + # Render the plist XML from the pure render function. + xml = render_launch_agent(log_dir=resolved_log_dir) + + # Idempotency: unload any existing instance before writing the new plist. + # This prevents launchctl from refusing to load a duplicate label. + # We ignore the return code here — the agent may not be loaded yet. + if plist_path.exists(): + _try_unload(plist_path=plist_path, runner=resolved_runner) + + # Write the plist to disk. + plist_path.write_text(xml, encoding="utf-8") + logger.info("event=launchd_plist_written path=%s", plist_path) + + # Load the agent via launchctl. + fallback: str | None = None + load_rc, load_out = _try_load(plist_path=plist_path, runner=resolved_runner) + if load_rc != 0: + # launchctl failed — surface the fallback so the CLI can print it. + fallback = _FALLBACK_CMD + logger.warning( + "event=launchd_load_failed plist=%s rc=%d output=%r fallback=%r", + plist_path, + load_rc, + load_out, + fallback, + ) + else: + logger.info("event=launchd_load_ok plist=%s", plist_path) + + return LaunchdInstallResult(installed=True, fallback_command=fallback) + + +def uninstall_launch_agent( + *, + launch_agents_dir: Path | None = None, + runner: RunnerFn | None = None, +) -> None: + """Unload and remove the LaunchAgent plist. + + Idempotent: if the plist does not exist, this is a no-op (no error raised). + + Args: + launch_agents_dir: Override for ~/Library/LaunchAgents (tests use tmp_path). + runner: Injectable launchctl runner: (argv) -> (returncode, output). + """ + resolved_dir = launch_agents_dir or _default_launch_agents_dir() + resolved_runner = runner or _subprocess_runner + + plist_path = resolved_dir / f"{LAUNCH_AGENT_LABEL}.plist" + + if not plist_path.exists(): + # Already absent — idempotent no-op. + logger.info("event=launchd_uninstall_noop plist=%s (not found)", plist_path) + return + + # Unload from launchd before removing the file. + # Ignore the return code — the agent may not be loaded (e.g. after a crash). + _try_unload(plist_path=plist_path, runner=resolved_runner) + + # Remove the plist file. + plist_path.unlink(missing_ok=True) + logger.info("event=launchd_plist_removed path=%s", plist_path) + + +def status_launch_agent( + *, + launch_agents_dir: Path | None = None, +) -> dict[str, Any]: + """Return the installation status of the LaunchAgent. + + Currently a best-effort check: we report `installed` based on whether the + plist file is present on disk. Full launchd query (launchctl list) would + require parsing output and is not needed for the issue-14 acceptance criteria. + + Args: + launch_agents_dir: Override for ~/Library/LaunchAgents (tests use tmp_path). + + Returns: + dict with keys: + installed: bool — plist file is present at the expected path. + plist_path: str — the expected plist path (for display). + """ + resolved_dir = launch_agents_dir or _default_launch_agents_dir() + plist_path = resolved_dir / f"{LAUNCH_AGENT_LABEL}.plist" + return { + "installed": plist_path.exists(), + "plist_path": str(plist_path), + } + + +# --------------------------------------------------------------------------- +# Internal helpers — launchctl wrappers +# --------------------------------------------------------------------------- + + +def _subprocess_runner(argv: list[str]) -> tuple[int, str]: + """Default runner: execute argv via subprocess, return (returncode, stdout).""" + import subprocess # noqa: PLC0415 — deferred import; not needed during tests + + try: + proc = subprocess.run( + argv, + capture_output=True, + text=True, + timeout=10, + ) + return proc.returncode, proc.stdout + proc.stderr + except (FileNotFoundError, OSError) as exc: + # launchctl binary not found (non-macOS, sandboxed env, etc.). + logger.warning("event=launchctl_not_found argv=%r reason=%s", argv, exc) + return 1, str(exc) + + +def _try_load(*, plist_path: Path, runner: RunnerFn) -> tuple[int, str]: + """Attempt to load the plist via `launchctl load `. + + Returns (returncode, combined output). Never raises. + """ + argv = ["launchctl", "load", str(plist_path)] + logger.debug("event=launchctl_load argv=%r", argv) + return runner(argv) + + +def _try_unload(*, plist_path: Path, runner: RunnerFn) -> tuple[int, str]: + """Attempt to unload the plist via `launchctl unload `. + + Returns (returncode, combined output). Never raises. Errors are logged + at DEBUG level because the agent may not be loaded yet. + """ + argv = ["launchctl", "unload", str(plist_path)] + logger.debug("event=launchctl_unload argv=%r", argv) + return runner(argv) diff --git a/tests/unit/test_daemon_launchd_cmd.py b/tests/unit/test_daemon_launchd_cmd.py new file mode 100644 index 0000000..314e8c6 --- /dev/null +++ b/tests/unit/test_daemon_launchd_cmd.py @@ -0,0 +1,149 @@ +"""Tests for `bach daemon install` and `bach daemon uninstall` CLI commands. + +Strategy: + - All launchd service calls are patched — no real launchctl or + ~/Library/LaunchAgents touched. + - Tests verify: subcommand registration, success output, fallback output + when launchctl fails, and idempotent-call behaviour. +""" + +from pathlib import Path +from unittest.mock import MagicMock, patch + +from typer.testing import CliRunner + +from bach.cli.app import app +from bach.services.launchd_service import LaunchdInstallResult + +runner = CliRunner() + +# Patch targets inside daemon_cmd module. +_INSTALL_PATCH = "bach.cli.daemon_cmd.install_launch_agent" +_UNINSTALL_PATCH = "bach.cli.daemon_cmd.uninstall_launch_agent" + + +# --------------------------------------------------------------------------- +# Subcommand registration +# --------------------------------------------------------------------------- + + +def test_daemon_install_is_registered() -> None: + """bach daemon install must appear in --help output.""" + result = runner.invoke(app, ["daemon", "--help"]) + assert result.exit_code == 0, result.output + assert "install" in result.output + + +def test_daemon_uninstall_is_registered() -> None: + """bach daemon uninstall must appear in --help output.""" + result = runner.invoke(app, ["daemon", "--help"]) + assert result.exit_code == 0, result.output + assert "uninstall" in result.output + + +# --------------------------------------------------------------------------- +# `bach daemon install` — success path +# --------------------------------------------------------------------------- + + +def test_daemon_install_calls_service(tmp_path: Path) -> None: + """daemon install must call install_launch_agent once.""" + mock_install = MagicMock( + return_value=LaunchdInstallResult(installed=True, fallback_command=None) + ) + with patch(_INSTALL_PATCH, mock_install): + result = runner.invoke(app, ["daemon", "install"]) + + assert mock_install.call_count == 1 + assert result.exit_code == 0, result.output + + +def test_daemon_install_prints_success_message(tmp_path: Path) -> None: + """On success, daemon install must print a success-indicating message.""" + mock_install = MagicMock( + return_value=LaunchdInstallResult(installed=True, fallback_command=None) + ) + with patch(_INSTALL_PATCH, mock_install): + result = runner.invoke(app, ["daemon", "install"]) + + # Output must mention some form of success / installed. + assert any( + word in result.output.lower() + for word in ("installed", "success", "loaded", "install") + ), f"Expected success message, got: {result.output!r}" + + +# --------------------------------------------------------------------------- +# `bach daemon install` — fallback path (launchctl failure) +# --------------------------------------------------------------------------- + + +def test_daemon_install_prints_fallback_on_launchctl_failure(tmp_path: Path) -> None: + """When launchctl fails, daemon install must print the manual fallback command.""" + mock_install = MagicMock( + return_value=LaunchdInstallResult( + installed=True, + fallback_command="bach daemon run", + ) + ) + with patch(_INSTALL_PATCH, mock_install): + result = runner.invoke(app, ["daemon", "install"]) + + # The fallback command must appear in the output. + assert "bach daemon run" in result.output, ( + f"Fallback command not printed. Output: {result.output!r}" + ) + + +def test_daemon_install_exit_code_nonzero_on_install_failure(tmp_path: Path) -> None: + """When install_launch_agent returns installed=False (unexpected error), + daemon install must exit with a non-zero code. + """ + mock_install = MagicMock( + return_value=LaunchdInstallResult(installed=False, fallback_command="bach daemon run") + ) + with patch(_INSTALL_PATCH, mock_install): + result = runner.invoke(app, ["daemon", "install"]) + + # Non-zero exit on failure. + assert result.exit_code != 0, ( + f"Expected non-zero exit on failure, got 0. Output: {result.output!r}" + ) + + +# --------------------------------------------------------------------------- +# `bach daemon uninstall` +# --------------------------------------------------------------------------- + + +def test_daemon_uninstall_calls_service(tmp_path: Path) -> None: + """daemon uninstall must call uninstall_launch_agent once.""" + mock_uninstall = MagicMock(return_value=None) + with patch(_UNINSTALL_PATCH, mock_uninstall): + result = runner.invoke(app, ["daemon", "uninstall"]) + + assert mock_uninstall.call_count == 1 + assert result.exit_code == 0, result.output + + +def test_daemon_uninstall_prints_confirmation(tmp_path: Path) -> None: + """daemon uninstall must print a message confirming the action.""" + mock_uninstall = MagicMock(return_value=None) + with patch(_UNINSTALL_PATCH, mock_uninstall): + result = runner.invoke(app, ["daemon", "uninstall"]) + + assert any( + word in result.output.lower() + for word in ("uninstalled", "removed", "uninstall") + ), f"Expected uninstall message, got: {result.output!r}" + + +def test_daemon_uninstall_is_idempotent_from_cli(tmp_path: Path) -> None: + """Calling daemon uninstall twice must succeed both times (exit 0).""" + mock_uninstall = MagicMock(return_value=None) + with patch(_UNINSTALL_PATCH, mock_uninstall): + r1 = runner.invoke(app, ["daemon", "uninstall"]) + r2 = runner.invoke(app, ["daemon", "uninstall"]) + + assert r1.exit_code == 0, r1.output + assert r2.exit_code == 0, r2.output diff --git a/tests/unit/test_launchd_service.py b/tests/unit/test_launchd_service.py new file mode 100644 index 0000000..1e2b64b --- /dev/null +++ b/tests/unit/test_launchd_service.py @@ -0,0 +1,324 @@ +"""Tests for services/launchd_service.py — macOS launchd LaunchAgent management. + +Strategy (mirrors test_hooks_service.py): + - `render_launch_agent` is a pure function — assert plist content directly. + - `install` / `uninstall` / `get_status` accept an injected `runner` callable + so tests NEVER call real launchctl or write to ~/Library/LaunchAgents. + - Plist-file writes are directed at tmp_path via an injected `launch_agents_dir`. + - Idempotency: install/uninstall twice must not produce errors or duplicate state. + - Fallback: when the launchctl runner returns failure, install must surface the + manual `bach daemon run` fallback command (returned in the result). +""" + +import plistlib +import sys +from pathlib import Path +from typing import Any + +from bach.services.launchd_service import ( + LAUNCH_AGENT_LABEL, + LaunchdInstallResult, + install_launch_agent, + render_launch_agent, + status_launch_agent, + uninstall_launch_agent, +) + +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +# The expected ProgramArguments must start with the current interpreter and end +# with ["daemon", "run"]. We do not hard-code the exact path so tests pass +# regardless of which venv pytest runs from. +_EXPECTED_ARGS_TAIL = ["-m", "bach", "daemon", "run"] + +# A fake runner that always succeeds (exit-code 0, empty output). +_OK_RUNNER = lambda argv: (0, "") # noqa: E731 + +# A fake runner that always fails (exit-code 1). +_FAIL_RUNNER = lambda argv: (1, "service not found") # noqa: E731 + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _parse_plist(xml: str) -> dict[str, Any]: + """Parse plist XML string into a Python dict.""" + result: dict[str, Any] = plistlib.loads(xml.encode()) + return result + + +def _write_plist(launch_agents_dir: Path, label: str, data: dict[str, Any]) -> Path: + """Write a plist file and return its path (used for pre-condition setup).""" + launch_agents_dir.mkdir(parents=True, exist_ok=True) + plist_path = launch_agents_dir / f"{label}.plist" + plist_path.write_bytes(plistlib.dumps(data)) + return plist_path + + +# --------------------------------------------------------------------------- +# render_launch_agent — plist content assertions +# --------------------------------------------------------------------------- + + +def test_render_returns_string() -> None: + """render_launch_agent must return a non-empty string.""" + xml = render_launch_agent(log_dir=Path("/tmp/logs")) + assert isinstance(xml, str) + assert len(xml) > 0 + + +def test_render_plist_is_valid_xml() -> None: + """The rendered output must be valid plist XML parseable by plistlib.""" + xml = render_launch_agent(log_dir=Path("/tmp/logs")) + data = _parse_plist(xml) + assert isinstance(data, dict) + + +def test_render_plist_has_correct_label() -> None: + """The plist Label must equal LAUNCH_AGENT_LABEL constant.""" + xml = render_launch_agent(log_dir=Path("/tmp/logs")) + data = _parse_plist(xml) + assert data["Label"] == LAUNCH_AGENT_LABEL + + +def test_render_plist_has_program_arguments() -> None: + """ProgramArguments must be a list starting with sys.executable.""" + xml = render_launch_agent(log_dir=Path("/tmp/logs")) + data = _parse_plist(xml) + args = data["ProgramArguments"] + assert isinstance(args, list) + assert args[0] == sys.executable, ( + f"First arg must be sys.executable, got: {args[0]!r}" + ) + + +def test_render_plist_program_arguments_invoke_daemon_run() -> None: + """ProgramArguments tail must be ['-m', 'bach', 'daemon', 'run'].""" + xml = render_launch_agent(log_dir=Path("/tmp/logs")) + data = _parse_plist(xml) + args = data["ProgramArguments"] + assert args[-4:] == _EXPECTED_ARGS_TAIL, ( + f"Expected tail {_EXPECTED_ARGS_TAIL!r}, got: {args!r}" + ) + + +def test_render_plist_has_run_at_load_true() -> None: + """RunAtLoad must be True so the agent starts on login.""" + xml = render_launch_agent(log_dir=Path("/tmp/logs")) + data = _parse_plist(xml) + assert data.get("RunAtLoad") is True + + +def test_render_plist_has_keep_alive_true() -> None: + """KeepAlive must be True so launchd restarts the daemon on crash.""" + xml = render_launch_agent(log_dir=Path("/tmp/logs")) + data = _parse_plist(xml) + assert data.get("KeepAlive") is True + + +def test_render_plist_has_stdout_path() -> None: + """StandardOutPath must point under the given log_dir.""" + log_dir = Path("/tmp/bach/logs") + xml = render_launch_agent(log_dir=log_dir) + data = _parse_plist(xml) + stdout = data.get("StandardOutPath", "") + assert str(log_dir) in stdout, ( + f"StandardOutPath {stdout!r} does not reference log_dir {log_dir}" + ) + + +def test_render_plist_has_stderr_path() -> None: + """StandardErrorPath must point under the given log_dir.""" + log_dir = Path("/tmp/bach/logs") + xml = render_launch_agent(log_dir=log_dir) + data = _parse_plist(xml) + stderr = data.get("StandardErrorPath", "") + assert str(log_dir) in stderr, ( + f"StandardErrorPath {stderr!r} does not reference log_dir {log_dir}" + ) + + +# --------------------------------------------------------------------------- +# LAUNCH_AGENT_LABEL constant +# --------------------------------------------------------------------------- + + +def test_label_is_reverse_dns() -> None: + """The label must be a stable reverse-DNS string starting with 'com.'.""" + assert LAUNCH_AGENT_LABEL.startswith("com."), ( + f"Label {LAUNCH_AGENT_LABEL!r} must start with 'com.'" + ) + + +# --------------------------------------------------------------------------- +# install_launch_agent — happy path +# --------------------------------------------------------------------------- + + +def test_install_creates_plist_file(tmp_path: Path) -> None: + """install must write a plist file at launch_agents_dir/