From d7c69ae973ccf86a7f2cfe3943decbfe90252d36 Mon Sep 17 00:00:00 2001 From: Arthur Date: Thu, 24 Sep 2026 23:28:09 +0200 Subject: [PATCH] fix(core): stop the host API from starting at login once core is down `stack down core` only unloaded the `dev.famstack.api` LaunchAgent and left its plist in `~/Library/LaunchAgents` with RunAtLoad set. At the next login launchd started the API on port 42001 again while core was reported down. After `stack destroy core` or `stack uninstall` the plist pointed at a wrapper in the deleted data dir, and launchd kept trying to spawn it. core's stop hook now unloads the agent and removes the plist, so down, destroy and uninstall leave nothing for launchd to start. `stack up core` writes it again as before. A plist with the same label that starts another instance's wrapper is left loaded and in place. --- stacklets/core/hooks/on_stop.py | 43 ++++- tests/e2e/remote | 6 - tests/unit/stacklets/test_core_launchagent.py | 170 ++++++++++++++++++ 3 files changed, 207 insertions(+), 12 deletions(-) create mode 100644 tests/unit/stacklets/test_core_launchagent.py diff --git a/stacklets/core/hooks/on_stop.py b/stacklets/core/hooks/on_stop.py index e0b61f0e..22d42079 100644 --- a/stacklets/core/hooks/on_stop.py +++ b/stacklets/core/hooks/on_stop.py @@ -1,14 +1,45 @@ -"""Stop the famstack API socket server.""" +"""Stop the famstack API server and take it out of the login items. +on_start writes the LaunchAgent with RunAtLoad on every up, so the file +only has to exist while core is up. Left in place, launchd would start +the API again at the next login while core reports down, and after a +destroy it would keep respawning a wrapper that no longer exists. +Removing it here covers down, destroy and uninstall, which all run +on_stop. + +Only the agent this instance wrote is touched. Another checkout or data +dir writes the same label with its own wrapper, and that one is left +loaded and in place. +""" + +import plistlib from pathlib import Path +from xml.parsers.expat import ExpatError PLIST_LABEL = "dev.famstack.api" def run(ctx): plist_path = Path.home() / "Library" / "LaunchAgents" / f"{PLIST_LABEL}.plist" - if plist_path.exists(): - try: - ctx.shell(f'launchctl unload "{plist_path}"') - except RuntimeError: - pass + # The wrapper on_start points the agent at. + wrapper = Path(ctx.stack.data) / "core" / "famstack-api" + if not _runs(plist_path, wrapper): + return + + try: + ctx.shell(f'launchctl unload "{plist_path}"') + except RuntimeError: + pass + plist_path.unlink() + ctx.step("famstack API stopped") + + +def _runs(plist_path: Path, wrapper: Path) -> bool: + """True when the agent at plist_path starts this instance's wrapper.""" + try: + with plist_path.open("rb") as f: + plist = plistlib.load(f) + except (OSError, ValueError, ExpatError): + # Missing or unreadable: nothing of ours to remove. + return False + return plist.get("ProgramArguments") == [str(wrapper)] diff --git a/tests/e2e/remote b/tests/e2e/remote index 63357d50..f1fde1bf 100755 --- a/tests/e2e/remote +++ b/tests/e2e/remote @@ -136,12 +136,6 @@ run_stacktests() { collect_logs say "==> stopping the rig" tests/e2e/stacktests down > "$results/down.log" 2>&1 || true - # core's on_start writes a LaunchAgent into the home dir and on_stop - # only unloads it. Remove ours so nothing outside the base dir remains. - local plist="$HOME/Library/LaunchAgents/dev.famstack.api.plist" - if [[ -f "$plist" ]] && grep -q "$base/" "$plist"; then - rm -f "$plist" - fi } # ── install ───────────────────────────────────────────────────────────── diff --git a/tests/unit/stacklets/test_core_launchagent.py b/tests/unit/stacklets/test_core_launchagent.py new file mode 100644 index 00000000..6f2e070e --- /dev/null +++ b/tests/unit/stacklets/test_core_launchagent.py @@ -0,0 +1,170 @@ +"""Core's LaunchAgent exists only while core is up. + +core's on_start writes `dev.famstack.api` into ~/Library/LaunchAgents +with RunAtLoad, so launchd starts the host API at every login. Once +core is down, destroyed or uninstalled, nothing may start it again: a +login after `down` would bring the API back while core reports down, +and one after `destroy` would respawn a wrapper whose data dir is gone. + +The hooks run through the framework's own down and destroy, against a +copy of the core stacklet and a temporary home. launchctl is a stub on +PATH that records its calls; the tests read what is left on disk for +launchd to find at the next login. +""" + +from __future__ import annotations + +import importlib.util +import plistlib +import shutil +import socket +from pathlib import Path + +import pytest + +from stack import Stack +from stack.hooks import build_hook_ctx + +REPO_ROOT = Path(__file__).resolve().parents[3] +CORE = REPO_ROOT / "stacklets" / "core" + +_spec = importlib.util.spec_from_file_location("core_on_start", CORE / "hooks" / "on_start.py") +on_start = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(on_start) + + +# ── Fixtures ────────────────────────────────────────────────────────────── + +@pytest.fixture +def home(tmp_path, monkeypatch): + home = tmp_path / "home" + monkeypatch.setattr(Path, "home", lambda: home) + return home + + +@pytest.fixture +def launchctl(tmp_path, monkeypatch): + """A launchctl on PATH that only records its arguments.""" + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + log = tmp_path / "launchctl.log" + log.touch() + stub = bin_dir / "launchctl" + stub.write_text(f'#!/bin/sh\necho "$*" >> "{log}"\n') + stub.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}:/usr/bin:/bin") + return log + + +@pytest.fixture(autouse=True) +def api_answering(monkeypatch): + """Something listening where on_start probes for the API. + + on_start then takes its "already running" path instead of waiting + five seconds for an API that launchctl, stubbed, never starts. + """ + with socket.socket() as listener: + listener.bind(("127.0.0.1", 0)) + listener.listen() + monkeypatch.setattr(on_start, "API_PORT", listener.getsockname()[1]) + yield + + +@pytest.fixture +def repo(tmp_path): + """A checkout holding a copy of core, so destroy removes nothing real.""" + repo = tmp_path / "repo" + shutil.copytree(CORE, repo / "stacklets" / "core", + ignore=shutil.ignore_patterns(".env", "__pycache__")) + return repo + + +def _instance(tmp_path, repo, name) -> Stack: + instance = tmp_path / name + (instance / ".stack").mkdir(parents=True) + (instance / ".stack" / "secrets.toml").write_text("") + data = tmp_path / f"{name}-data" + (instance / "stack.toml").write_text(f'[core]\ndomain = ""\ndata_dir = "{data}"\n') + return Stack(root=repo, data=data, instance_dir=instance) + + +@pytest.fixture +def stack(tmp_path, repo, home, launchctl): + return _instance(tmp_path, repo, "instance") + + +def _up(stack: Stack): + on_start.run(build_hook_ctx("core", stack=stack)) + + +def _plist(home: Path) -> Path: + return home / "Library" / "LaunchAgents" / "dev.famstack.api.plist" + + +def _calls(launchctl: Path) -> list[str]: + return launchctl.read_text().splitlines() + + +# ── Behaviour ───────────────────────────────────────────────────────────── + +def test_up_leaves_an_agent_that_starts_at_login(stack, home): + _up(stack) + + agent = plistlib.loads(_plist(home).read_bytes()) + assert agent["RunAtLoad"] is True + assert agent["ProgramArguments"] == [str(stack.data / "core" / "famstack-api")] + + +def test_down_unloads_the_agent_and_leaves_nothing_for_the_next_login(stack, home, launchctl): + _up(stack) + + stack.down("core") + + assert f"unload {_plist(home)}" in _calls(launchctl) + assert not _plist(home).exists() + + +def test_up_after_down_brings_the_agent_back(stack, home): + _up(stack) + stack.down("core") + + _up(stack) + + assert plistlib.loads(_plist(home).read_bytes())["RunAtLoad"] is True + + +def test_destroy_leaves_no_agent_behind(stack, home): + """uninstall destroys each stacklet, so this covers it as well.""" + _up(stack) + + stack.destroy("core") + + assert not (stack.data / "core").exists() + assert not _plist(home).exists() + + +def test_repeating_down_and_destroy_changes_nothing(stack, home, launchctl): + _up(stack) + stack.down("core") + launchctl.write_text("") + + assert stack.down("core")["ok"] + assert stack.destroy("core")["ok"] + assert stack.destroy("core")["ok"] + + assert _calls(launchctl) == [] + assert not _plist(home).exists() + + +def test_the_agent_another_instance_wrote_is_left_alone(stack, tmp_path, repo, home, launchctl): + """Same label, other data dir: that API belongs to the other instance.""" + _up(stack) + other = _instance(tmp_path, repo, "other") + _up(other) + written = _plist(home).read_bytes() + + stack.down("core") + stack.destroy("core") + + assert _plist(home).read_bytes() == written + assert _calls(launchctl) == []