From 193f40c02974fddde44d2f83eaf0b3356631d1ea Mon Sep 17 00:00:00 2001 From: StuBehan Date: Tue, 8 Sep 2026 12:18:16 +0100 Subject: [PATCH 1/2] fix: wait for the daemon to exit before stop reports success --- stackvox/daemon.py | 47 ++++++++++++++++++++++++----- tests/test_daemon.py | 72 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 8 deletions(-) diff --git a/stackvox/daemon.py b/stackvox/daemon.py index 61be30e..5d7dbbf 100644 --- a/stackvox/daemon.py +++ b/stackvox/daemon.py @@ -26,6 +26,7 @@ import socketserver import sys import threading +import time from typing import Any import sounddevice as sd @@ -42,6 +43,10 @@ WORKER_POLL_SECONDS = 0.5 CLIENT_TIMEOUT_SECONDS = 1.0 PING_TIMEOUT_SECONDS = 0.5 +# How long `stop` waits for the daemon process to actually exit after it has +# acknowledged the request, and how often it re-checks. +STOP_TIMEOUT_SECONDS = 5.0 +STOP_POLL_SECONDS = 0.05 RECV_BYTES = 1024 @@ -273,14 +278,19 @@ def _pid_alive(pid: int) -> bool: return False -def is_running() -> bool: +def _read_pid() -> int | None: + """The pid recorded in the pid file, or None if absent or unparseable.""" if not PID_PATH.exists(): - return False + return None try: - pid = int(PID_PATH.read_text().strip()) - except ValueError: - return False - return _pid_alive(pid) + return int(PID_PATH.read_text().strip()) + except (ValueError, OSError): + return None + + +def is_running() -> bool: + pid = _read_pid() + return pid is not None and _pid_alive(pid) def _check_for_update_async() -> None: @@ -359,8 +369,29 @@ def say( return send(req) -def stop() -> tuple[bool, str]: - return send({"command": "stop"}) +def stop(timeout: float = STOP_TIMEOUT_SECONDS) -> tuple[bool, str]: + """Shut the daemon down and wait until the process has actually gone. + + The daemon acknowledges the request and only then unwinds `serve_forever` + and removes its pid file, so returning on the ack alone made + `stackvox stop && stackvox serve` race: `serve` still saw a live pid, refused + to start, and the shutdown then completed, leaving nothing running at all. + Reporting success before the process has exited makes that sequence, the one + `status` recommends, silently useless. + """ + pid = _read_pid() + ok, resp = send({"command": "stop"}) + if not ok or pid is None: + # Nothing acknowledged, or no pid to watch: the caller's own + # `is_running` check is as good as it gets. + return ok, resp + deadline = time.monotonic() + timeout + while True: + if not _pid_alive(pid): + return True, resp + if time.monotonic() >= deadline: + return False, f"daemon acknowledged stop but pid {pid} is still alive after {timeout:g}s" + time.sleep(STOP_POLL_SECONDS) def cancel() -> tuple[bool, str]: diff --git a/tests/test_daemon.py b/tests/test_daemon.py index d7c5eb9..e7c0673 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -78,6 +78,9 @@ def test_say_without_overrides_passes_only_text(self, mocker): assert send.call_args.args[0] == {"text": "hi"} def test_stop_sends_command_stop(self, mocker): + # _read_pid is mocked out so the wait-for-exit loop can't reach a real + # daemon's pid file and block on a live process. + mocker.patch.object(daemon, "_read_pid", return_value=None) send = mocker.patch.object(daemon, "send", return_value=(True, "ok")) daemon.stop() assert send.call_args.args[0] == {"command": "stop"} @@ -136,3 +139,72 @@ def test_disabled_when_coreaudio_unavailable(self, mocker, caplog): with caplog.at_level(logging.DEBUG, logger="stackvox.daemon"): daemon._start_device_watcher() # must not raise assert any("CoreAudio unavailable" in r.message for r in caplog.records) + + +class TestStopWaitsForExit: + """`stop` reports success only once the process has actually gone. + + The daemon acknowledges the request before unwinding, so returning on the + ack made `stackvox stop && stackvox serve` race: serve saw a live pid and + refused, then the shutdown finished, leaving nothing running. + """ + + def test_returns_send_failure_unchanged(self, mocker): + mocker.patch.object(daemon, "_read_pid", return_value=4242) + mocker.patch.object(daemon, "send", return_value=(False, "daemon not running")) + + actual = daemon.stop() + + assert actual == (False, "daemon not running") + + def test_does_not_wait_when_no_pid_is_recorded(self, mocker): + mocker.patch.object(daemon, "_read_pid", return_value=None) + mocker.patch.object(daemon, "send", return_value=(True, "ok")) + alive = mocker.patch.object(daemon, "_pid_alive") + + actual = daemon.stop() + + assert actual == (True, "ok") + alive.assert_not_called() + + def test_waits_until_the_process_has_exited(self, mocker): + mocker.patch.object(daemon, "_read_pid", return_value=4242) + mocker.patch.object(daemon, "send", return_value=(True, "ok")) + alive = mocker.patch.object(daemon, "_pid_alive", side_effect=[True, True, False]) + sleep = mocker.patch.object(daemon.time, "sleep") + + actual = daemon.stop() + + assert actual == (True, "ok") + assert alive.call_count == 3 + assert sleep.call_count == 2 + + def test_fails_when_the_process_outlives_the_timeout(self, mocker): + mocker.patch.object(daemon, "_read_pid", return_value=4242) + mocker.patch.object(daemon, "send", return_value=(True, "ok")) + mocker.patch.object(daemon, "_pid_alive", return_value=True) + mocker.patch.object(daemon.time, "sleep") + + ok, resp = daemon.stop(timeout=0.0) + + assert ok is False + assert "still alive" in resp + assert "4242" in resp + + +class TestReadPid: + def test_returns_none_when_missing(self, mocker, tmp_path): + mocker.patch.object(daemon, "PID_PATH", tmp_path / "missing.pid") + assert daemon._read_pid() is None + + def test_returns_none_when_unparseable(self, mocker, tmp_path): + pid = tmp_path / "garbage.pid" + pid.write_text("not-a-number") + mocker.patch.object(daemon, "PID_PATH", pid) + assert daemon._read_pid() is None + + def test_returns_the_recorded_pid(self, mocker, tmp_path): + pid = tmp_path / "live.pid" + pid.write_text("4242\n") + mocker.patch.object(daemon, "PID_PATH", pid) + assert daemon._read_pid() == 4242 From fdcb4da5067cf323c46d95e76756e562f6d5b681 Mon Sep 17 00:00:00 2001 From: StuBehan Date: Tue, 8 Sep 2026 12:18:23 +0100 Subject: [PATCH 2/2] fix: read version from the source tree, not stale dist metadata --- stackvox/cli.py | 17 ++++++++++++---- stackvox/updates.py | 40 +++++++++++++++++++++++++++++++++++++- tests/test_cli.py | 21 ++++++++++++++++++++ tests/test_updates.py | 45 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 5 deletions(-) diff --git a/stackvox/cli.py b/stackvox/cli.py index 7475096..b08ff91 100644 --- a/stackvox/cli.py +++ b/stackvox/cli.py @@ -432,12 +432,21 @@ def _cmd_status(_: argparse.Namespace) -> int: rc = 0 # The running daemon can lag the installed package if it wasn't restarted # after an upgrade — surface that skew instead of trusting it blindly. + # Only a daemon that is genuinely BEHIND wants restarting; a newer one + # means the client just run is the older install (a checkout alongside a + # global one), and restarting would downgrade what's serving. got, running_version = daemon.version() if got and running_version != installed: - print( - f"daemon running {running_version}, but {installed} is installed " - f"— restart the daemon (stackvox stop; stackvox serve) to pick it up" - ) + if updates._is_newer(installed, running_version): + print( + f"daemon running {running_version}, but {installed} is installed " + f"— restart the daemon (stackvox stop; stackvox serve) to pick it up" + ) + else: + print( + f"daemon running {running_version} is newer than the {installed} client " + f"you just ran; no restart needed" + ) else: print("stopped") rc = 1 diff --git a/stackvox/updates.py b/stackvox/updates.py index caf5c0a..5a8fecf 100644 --- a/stackvox/updates.py +++ b/stackvox/updates.py @@ -18,6 +18,7 @@ import json import logging import os +import sys import urllib.error import urllib.request from dataclasses import dataclass @@ -26,14 +27,51 @@ from importlib.metadata import version as _pkg_version from pathlib import Path +if sys.version_info >= (3, 11): + import tomllib +else: # pragma: no cover - covered by 3.10 CI + import tomli as tomllib + from stackvox.paths import cache_dir logger = logging.getLogger(__name__) +def _source_tree_version(pyproject: Path | None = None) -> str | None: + """Our version from `pyproject.toml`, when running from a source checkout. + + An editable install records its version in dist metadata at install time, so + the moment a release bumps `pyproject.toml` that metadata goes stale. The + developer then gets a phantom "update available: 0.9.0 -> 0.11.0" against + their own tree, and `status` reads the running daemon as newer than the + "installed" package and tells them to restart it for no reason. + + Returns None when there's no such file (a normal pip/pipx install) or when it + isn't ours, so the metadata path stays authoritative for real installs. + """ + path = pyproject or Path(__file__).resolve().parent.parent / "pyproject.toml" + try: + with path.open("rb") as handle: + data = tomllib.load(handle) + except (OSError, tomllib.TOMLDecodeError): + return None + project = data.get("project") + if not isinstance(project, dict) or project.get("name") != "stackvox": + return None + version = project.get("version") + return version if isinstance(version, str) else None + + def _current_version() -> str: """Read our own installed version. Late-bound so importing this module - early in the package init chain doesn't trip a circular import.""" + early in the package init chain doesn't trip a circular import. + + A source checkout wins over dist metadata, which goes stale on an editable + install after every release. + """ + from_source = _source_tree_version() + if from_source is not None: + return from_source try: return _pkg_version("stackvox") except PackageNotFoundError: diff --git a/tests/test_cli.py b/tests/test_cli.py index d0c8db8..f19c03a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -292,6 +292,27 @@ def test_running_flags_daemon_version_skew(self, mocker, capsys): assert "0.5.0" in out and "0.8.0" in out # the skew is surfaced assert "restart" in out.lower() + def test_running_does_not_advise_restart_when_the_daemon_is_ahead(self, mocker, capsys): + """A daemon NEWER than the client means the client is the older install, + so restarting would downgrade what's serving.""" + mocker.patch.object(cli.daemon, "is_running", return_value=True) + pid_path = mocker.MagicMock() + pid_path.read_text.return_value = "1\n" + mocker.patch.object(cli.daemon, "PID_PATH", pid_path) + mocker.patch.object(cli.daemon, "SOCKET_PATH", "/tmp/x.sock") + mocker.patch.object(cli.daemon, "version", return_value=(True, "0.11.0")) + mocker.patch.object(cli.updates, "_current_version", return_value="0.9.0") + mocker.patch.object(cli.updates, "check_for_update", return_value=None) + + rc = cli._cmd_status(_ns()) + + assert rc == 0 + out = capsys.readouterr().out + assert "0.11.0" in out and "0.9.0" in out + assert "stackvox stop" not in out # the restart advice itself + assert "newer" in out.lower() + assert "no restart needed" in out + def test_stopped_returns_one(self, mocker, capsys): mocker.patch.object(cli.daemon, "is_running", return_value=False) rc = cli._cmd_status(_ns()) diff --git a/tests/test_updates.py b/tests/test_updates.py index efdf14e..43cb497 100644 --- a/tests/test_updates.py +++ b/tests/test_updates.py @@ -218,3 +218,48 @@ def test_includes_versions_and_upgrade_command(self): assert "0.3.1" in msg assert "0.4.0" in msg assert "pipx upgrade stackvox" in msg + + +class TestSourceTreeVersion: + """An editable install's dist metadata goes stale the moment a release bumps + pyproject, so a source checkout has to win over it.""" + + def test_reads_the_version_from_our_own_pyproject(self, tmp_path): + path = tmp_path / "pyproject.toml" + path.write_text('[project]\nname = "stackvox"\nversion = "1.2.3"\n') + assert updates._source_tree_version(path) == "1.2.3" + + def test_ignores_a_pyproject_that_is_not_ours(self, tmp_path): + path = tmp_path / "pyproject.toml" + path.write_text('[project]\nname = "something-else"\nversion = "1.2.3"\n') + assert updates._source_tree_version(path) is None + + def test_returns_none_when_there_is_no_pyproject(self, tmp_path): + assert updates._source_tree_version(tmp_path / "nope.toml") is None + + def test_returns_none_when_the_file_is_malformed(self, tmp_path): + path = tmp_path / "pyproject.toml" + path.write_text("not valid toml {{{") + assert updates._source_tree_version(path) is None + + def test_returns_none_when_the_version_is_absent(self, tmp_path): + path = tmp_path / "pyproject.toml" + path.write_text('[project]\nname = "stackvox"\n') + assert updates._source_tree_version(path) is None + + +class TestCurrentVersion: + def test_source_tree_beats_stale_dist_metadata(self, mocker): + mocker.patch.object(updates, "_source_tree_version", return_value="0.11.0") + mocker.patch.object(updates, "_pkg_version", return_value="0.9.0") + assert updates._current_version() == "0.11.0" + + def test_falls_back_to_dist_metadata_for_a_real_install(self, mocker): + mocker.patch.object(updates, "_source_tree_version", return_value=None) + mocker.patch.object(updates, "_pkg_version", return_value="0.11.0") + assert updates._current_version() == "0.11.0" + + def test_reports_unknown_when_not_installed_at_all(self, mocker): + mocker.patch.object(updates, "_source_tree_version", return_value=None) + mocker.patch.object(updates, "_pkg_version", side_effect=updates.PackageNotFoundError) + assert updates._current_version() == "0.0.0+unknown"