diff --git a/install.sh b/install.sh index 98e921d..4f8c7d0 100755 --- a/install.sh +++ b/install.sh @@ -32,6 +32,38 @@ else echo "(The rest of this tool is unaffected.)" fi +# Install the web-UI user unit. No root needed: it serves read-only views and +# needs no privilege. Automated rather than documented, because the whole +# reason this unit exists is that a hand-started server outlived its own code. +install_web_user_unit() { + local target_user home_dir dst src + target_user="${SUDO_USER:-$USER}" + home_dir="$(getent passwd "$target_user" | cut -d: -f6)" + if [[ -z $home_dir ]]; then + echo "Warning: cannot resolve home for $target_user; skipping web unit." + return 0 + fi + src="$SCRIPT_DIR/steam-backlog-enforcer-web.service" + dst="$home_dir/.config/systemd/user/steam-backlog-enforcer-web.service" + mkdir -p "$(dirname "$dst")" + sed "s|__REPO_DIR__|$SCRIPT_DIR|g" "$src" > "$dst" + if [[ $EUID -eq 0 ]]; then + chown "$target_user" "$dst" + sudo -u "$target_user" \ + XDG_RUNTIME_DIR="/run/user/$(id -u "$target_user")" \ + systemctl --user daemon-reload + sudo -u "$target_user" \ + XDG_RUNTIME_DIR="/run/user/$(id -u "$target_user")" \ + systemctl --user enable --now steam-backlog-enforcer-web || true + else + systemctl --user daemon-reload + systemctl --user enable --now steam-backlog-enforcer-web || true + fi + echo "Installed and enabled: $dst" +} + +install_web_user_unit + # Install systemd service (system-level, runs as root). # # Non-interactive callers (install_core_system.sh, CI, `vm run`) have no stdin, diff --git a/steam-backlog-enforcer-web.service b/steam-backlog-enforcer-web.service new file mode 100644 index 0000000..47eb6c0 --- /dev/null +++ b/steam-backlog-enforcer-web.service @@ -0,0 +1,26 @@ +[Unit] +Description=Steam Backlog Enforcer web UI (localhost only) +After=network-online.target + +[Service] +Type=simple +WorkingDirectory=__REPO_DIR__ +ExecStart=__REPO_DIR__/run.sh serve +Restart=always +RestartSec=3s +Environment=PYTHONUNBUFFERED=1 + +# Why this unit exists at all: the UI used to be a hand-started `python -c` +# that stayed up for hours after the code beneath it changed, reporting an 8h +# gaming budget while the daemon enforced 6h. _serve_stale now makes the server +# refuse to answer once its own source has moved on -- but refusing is only +# half a fix if nothing brings it back. Restart=always is the other half: the +# server dies on stale code and returns on current code, with no one having to +# remember anything. +# +# Deliberately a *user* unit, not a system one. It reads state and serves +# read-only views; it needs no privilege, and running it as root would give a +# network-facing process rights the daemon's own hardening notes warn about. + +[Install] +WantedBy=default.target diff --git a/steam_backlog_enforcer/_serve_guard.py b/steam_backlog_enforcer/_serve_guard.py index 02162bd..05bb65c 100644 --- a/steam_backlog_enforcer/_serve_guard.py +++ b/steam_backlog_enforcer/_serve_guard.py @@ -20,6 +20,8 @@ import signal import socket import time +from steam_backlog_enforcer._serve_stale import newest_py_after + _PROC = Path("/proc") _NET_TCP = (_PROC / "net" / "tcp", _PROC / "net" / "tcp6") @@ -27,13 +29,12 @@ _NET_TCP = (_PROC / "net" / "tcp", _PROC / "net" / "tcp6") # not a bind, and must not be mistaken for the process owning the port. _LISTEN_STATE = "0A" -# argv fragments that identify one of our own web servers. -_OWN_MODULE = "steam_backlog_enforcer.main" +# argv fragments identifying our own web servers, matched as substrings over +# the joined argv: a `python -c` server never holds "…enforcer.main", so an +# exact match missed it entirely. See _serve_stale for what that cost. +_OWN_PACKAGE = "steam_backlog_enforcer" _OWN_COMMAND = "serve" -# Package tree whose .py mtimes decide whether a running server is stale. -_PACKAGE_ROOT = Path(__file__).resolve().parent - # A wildcard IPv4 bind renders as an all-zero address column. Spelled as hex # rather than _hex_v4("0.0.0.0") so the dotted-quad literal never appears: this # module only ever *matches* that address, it must never bind it. @@ -155,9 +156,14 @@ def read_cmdline(pid: int) -> list[str]: def is_our_server(pid: int) -> bool: - """Check whether *pid* is one of our own ``serve`` processes.""" - argv = read_cmdline(pid) - return _OWN_MODULE in argv and _OWN_COMMAND in argv + """Check whether *pid* is one of our own ``serve`` processes. + + Recognises every launch form -- ``run.sh serve``, ``python -m …main + serve``, and a bare ``python -c``. One we fail to recognise can be neither + replaced when stale nor named accurately when it holds the port. + """ + argv = " ".join(read_cmdline(pid)) + return _OWN_PACKAGE in argv and _OWN_COMMAND in argv def process_started_at(pid: int) -> float | None: @@ -176,19 +182,11 @@ def process_started_at(pid: int) -> float | None: def newest_py_since(started_at: float) -> Path | None: """Return the newest ``.py`` in the package modified after *started_at*. - Returning the path rather than a bool is deliberate: the restart message - names the file, so the decision to kill a server is always auditable. + Delegates to :mod:`_serve_stale`, which owns this scan because the server + itself needs the same answer about its own process. Two copies would be + free to disagree about whether a server is current. """ - newest: Path | None = None - newest_mtime = started_at - for path in _PACKAGE_ROOT.rglob("*.py"): - try: - mtime = path.stat().st_mtime - except OSError: - continue - if mtime > newest_mtime: - newest, newest_mtime = path, mtime - return newest + return newest_py_after(started_at) def _own_pid_chain() -> set[int]: diff --git a/steam_backlog_enforcer/_serve_stale.py b/steam_backlog_enforcer/_serve_stale.py new file mode 100644 index 0000000..faab454 --- /dev/null +++ b/steam_backlog_enforcer/_serve_stale.py @@ -0,0 +1,103 @@ +"""Self-check: is this server process running code that has since changed? + +``_serve_guard`` already answers this about *other* processes, at launch time, +so ``serve`` can replace a stale server before taking the port. That guard has +two holes, both of which were open on 2026-08-29: + +1. It only runs from ``cmd_serve``. A server started as + ``python -c "from ..._web_server import serve; serve(port=8000)"`` never + passes through it. +2. :func:`~steam_backlog_enforcer._serve_guard.is_our_server` matches argv + holding ``steam_backlog_enforcer.main``, which such a process does not have, + so it is not even recognised as ours to replace. + +The result was a UI reporting an 8-hour budget while the daemon enforced six, +which is precisely the divergence the shared ``rules_for`` seam exists to +prevent -- defeated not by a second copy of the logic but by a second copy of +the *process*. + +So the check also lives here, inside the server, where no launch path can skip +it. A server that has been outrun by its own source stops answering rather than +answering wrongly: with a supervisor it comes straight back on fresh code, and +without one it is at least honestly silent. A wrong number is worse than no +number, because only one of the two gets believed. +""" + +from __future__ import annotations + +import logging +from pathlib import Path +import time +from typing import Final + +logger = logging.getLogger(__name__) + +# Package tree whose .py mtimes decide whether this process is out of date. +_PACKAGE_ROOT: Final = Path(__file__).resolve().parent + +# Re-checking on every request would stat the whole package per request. The +# window this leaves is bounded and small: at worst one stale answer in this +# many seconds, versus the unbounded staleness it replaces. +_RECHECK_INTERVAL_SECONDS: Final = 5.0 + +_last_checked: dict[str, float] = {} +_last_result: dict[str, Path | None] = {} + + +def reset_cache() -> None: + """Drop the memoised answer, forcing the next call to re-stat.""" + _last_checked.clear() + _last_result.clear() + + +def newest_py_after(cutoff: float) -> Path | None: + """Return the newest package ``.py`` modified after *cutoff*. + + Args: + cutoff: Epoch seconds to compare mtimes against. + + Returns: + The newest offending path, or ``None`` when the tree is older than + *cutoff*. Returning the path rather than a bool keeps the refusal + auditable: the log names the file that made this process obsolete. + """ + newest: Path | None = None + newest_mtime = cutoff + for path in _PACKAGE_ROOT.rglob("*.py"): + try: + mtime = path.stat().st_mtime + except OSError: + # A file removed mid-scan cannot make us stale; skip it quietly. + continue + if mtime > newest_mtime: + newest, newest_mtime = path, mtime + return newest + + +def outdated_source(started_at: float, *, now: float | None = None) -> Path | None: + """Return the source file that makes this process stale, if any. + + Args: + started_at: Epoch seconds this process began. Any package file newer + than this was not the file this process loaded. + now: Override for the current monotonic time (for testing). + + Returns: + The newest changed path, or ``None`` when the running code is current. + """ + moment = now if now is not None else time.monotonic() + checked = _last_checked.get("at") + if checked is not None and moment - checked < _RECHECK_INTERVAL_SECONDS: + return _last_result.get("path") + + result = newest_py_after(started_at) + _last_checked["at"] = moment + _last_result["path"] = result + if result is not None: + logger.error( + "This server is running outdated code: %s changed after the " + "process started. Refusing to serve rather than report numbers " + "the daemon is not enforcing. Restart it with `./run.sh serve`.", + result, + ) + return result diff --git a/steam_backlog_enforcer/_web_server.py b/steam_backlog_enforcer/_web_server.py index 6c320a6..4fc958f 100644 --- a/steam_backlog_enforcer/_web_server.py +++ b/steam_backlog_enforcer/_web_server.py @@ -17,9 +17,12 @@ import json import logging import mimetypes from pathlib import Path +import threading +import time from urllib.parse import urlsplit from steam_backlog_enforcer._budget_view import build_budget_snapshot +from steam_backlog_enforcer._serve_stale import outdated_source from steam_backlog_enforcer._web_dataset import build_web_dataset, dataset_to_payload from steam_backlog_enforcer.config import State from steam_backlog_enforcer.game_install import _echo @@ -40,6 +43,20 @@ _EXTRA_TEXT_TYPES = frozenset( ) _NOT_BUILT_MSG = b"Frontend not built. Run: cd web && npm install && npm run build" +# Epoch seconds this process began, captured at import. Anything in the +# package newer than this is code we did not load. +_STARTED_AT = time.time() + +# Set once a stale server has begun standing down, so concurrent requests +# do not each spawn a shutdown thread. +_RETIRING = threading.Event() + +_STALE_MSG = ( + b"This server is running outdated code and has stopped answering rather " + b"than report numbers the enforcer is not applying. Restart it: " + b"./run.sh serve" +) + class _Handler(BaseHTTPRequestHandler): """Serve the dataset JSON and the static frontend bundle (read-only).""" @@ -50,6 +67,14 @@ class _Handler(BaseHTTPRequestHandler): def do_GET(self) -> None: """Dispatch a GET to one of the APIs or to a static file.""" + stale = outdated_source(_STARTED_AT) + if stale is not None: + # Fail closed on every route, static included: a page served from a + # fresh bundle that then fetches numbers from stale code is the + # same lie with extra steps. + self._send(HTTPStatus.SERVICE_UNAVAILABLE, _STALE_MSG, "text/plain") + self._retire() + return split = urlsplit(self.path) if split.path == _API_DATASET: self._serve_dataset() @@ -61,6 +86,24 @@ class _Handler(BaseHTTPRequestHandler): else: self._serve_static(split.path) + def _retire(self) -> None: + """Answer this request, then stop the server so it can be replaced. + + Refusing to serve is only half a fix: a process that 503s forever is + honest and useless. Exiting hands the problem to the supervisor, and + ``steam-backlog-enforcer-web.service`` has ``Restart=always``, so the + server comes back on current code with nobody having to remember to + restart it. Started outside systemd it simply exits, which is still + better than serving numbers the enforcer is not applying. + + ``shutdown`` blocks until ``serve_forever`` returns and cannot be + called from the thread handling a request, so it goes to its own. + """ + if _RETIRING.is_set(): + return + _RETIRING.set() + threading.Thread(target=self.server.shutdown, daemon=True).start() + def _serve_budget(self, *, demo: bool) -> None: """Build and send the gaming-budget snapshot as JSON.""" try: diff --git a/steam_backlog_enforcer/tests/test_serve_guard_lifecycle.py b/steam_backlog_enforcer/tests/test_serve_guard_lifecycle.py index dbd6567..c25d33a 100644 --- a/steam_backlog_enforcer/tests/test_serve_guard_lifecycle.py +++ b/steam_backlog_enforcer/tests/test_serve_guard_lifecycle.py @@ -17,7 +17,7 @@ import socket from typing import TYPE_CHECKING from unittest.mock import patch -from steam_backlog_enforcer import _serve_guard +from steam_backlog_enforcer import _serve_guard, _serve_stale from steam_backlog_enforcer.tests._fake_proc import add_pid, make_proc, patch_proc if TYPE_CHECKING: @@ -48,7 +48,7 @@ class TestNewestPySince: def test_no_newer_file_returns_none(self, tmp_path: Path) -> None: (tmp_path / "a.py").write_text("x", encoding="utf-8") os.utime(tmp_path / "a.py", (1000.0, 1000.0)) - with patch.object(_serve_guard, "_PACKAGE_ROOT", tmp_path): + with patch.object(_serve_stale, "_PACKAGE_ROOT", tmp_path): assert _serve_guard.newest_py_since(2000.0) is None def test_newer_file_is_named(self, tmp_path: Path) -> None: @@ -58,19 +58,19 @@ class TestNewestPySince: nested.mkdir() (nested / "b.py").write_text("x", encoding="utf-8") os.utime(nested / "b.py", (3000.0, 3000.0)) - with patch.object(_serve_guard, "_PACKAGE_ROOT", tmp_path): + with patch.object(_serve_stale, "_PACKAGE_ROOT", tmp_path): assert _serve_guard.newest_py_since(2000.0) == nested / "b.py" def test_the_newest_of_several_wins(self, tmp_path: Path) -> None: for name, mtime in (("a.py", 3000.0), ("b.py", 4000.0), ("c.py", 3500.0)): (tmp_path / name).write_text("x", encoding="utf-8") os.utime(tmp_path / name, (mtime, mtime)) - with patch.object(_serve_guard, "_PACKAGE_ROOT", tmp_path): + with patch.object(_serve_stale, "_PACKAGE_ROOT", tmp_path): assert _serve_guard.newest_py_since(2000.0) == tmp_path / "b.py" def test_vanished_file_is_skipped(self, tmp_path: Path) -> None: (tmp_path / "gone.py").symlink_to(tmp_path / "missing.py") - with patch.object(_serve_guard, "_PACKAGE_ROOT", tmp_path): + with patch.object(_serve_stale, "_PACKAGE_ROOT", tmp_path): assert _serve_guard.newest_py_since(1000.0) is None diff --git a/steam_backlog_enforcer/tests/test_serve_stale.py b/steam_backlog_enforcer/tests/test_serve_stale.py new file mode 100644 index 0000000..35c7de9 --- /dev/null +++ b/steam_backlog_enforcer/tests/test_serve_stale.py @@ -0,0 +1,137 @@ +"""Tests for _serve_stale: a server must not outlive its own source. + +On 2026-08-29 a hand-started ``python -c`` server stayed up for hours after the +code beneath it changed, reporting an 8h gaming budget while the daemon +enforced 6h. The launcher guard could not have helped: it only runs from +``cmd_serve``, which that process never went through. So the check lives in the +server, where no launch path can skip it. +""" + +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING +from unittest.mock import patch + +import pytest + +from steam_backlog_enforcer._serve_stale import ( + _RECHECK_INTERVAL_SECONDS, + outdated_source, + reset_cache, +) + +if TYPE_CHECKING: + from collections.abc import Iterator + from pathlib import Path + +_PKG = "steam_backlog_enforcer._serve_stale" + + +@pytest.fixture(autouse=True) +def _clear() -> Iterator[None]: + """Keep the memo from leaking answers between tests. + + Yields: + None, with the cache empty before and after. + """ + reset_cache() + yield + reset_cache() + + +@pytest.fixture +def fake_package(tmp_path: Path) -> Path: + """Point the module at a throwaway package tree. + + Args: + tmp_path: pytest temporary directory. + + Returns: + The fake package root. + """ + root = tmp_path / "pkg" + root.mkdir() + (root / "module.py").write_text("x = 1\n", encoding="utf-8") + return root + + +def _touch(path: Path, mtime: float) -> None: + """Set a file's mtime. + + Args: + path: File to stamp. + mtime: Epoch seconds to set. + """ + path.write_text("x = 2\n", encoding="utf-8") + import os + + os.utime(path, (mtime, mtime)) + + +class TestOutdatedSource: + """The check answers "has my own code moved on since I started".""" + + def test_current_code_is_not_stale(self, fake_package: Path) -> None: + """A tree older than the process start is fine.""" + _touch(fake_package / "module.py", 1000.0) + with patch(f"{_PKG}._PACKAGE_ROOT", fake_package): + assert outdated_source(2000.0, now=0.0) is None + + def test_a_newer_file_is_named(self, fake_package: Path) -> None: + """The offending path is returned, so the refusal is auditable.""" + changed = fake_package / "module.py" + _touch(changed, 3000.0) + with patch(f"{_PKG}._PACKAGE_ROOT", fake_package): + assert outdated_source(2000.0, now=0.0) == changed + + def test_staleness_is_logged_at_error( + self, + fake_package: Path, + caplog: pytest.LogCaptureFixture, + ) -> None: + """Refusing to serve is loud: it needs to reach the journal.""" + _touch(fake_package / "module.py", 3000.0) + with ( + caplog.at_level(logging.ERROR), + patch(f"{_PKG}._PACKAGE_ROOT", fake_package), + ): + outdated_source(2000.0, now=0.0) + assert "outdated code" in caplog.text + assert "run.sh serve" in caplog.text + + def test_a_file_removed_mid_scan_is_skipped(self, fake_package: Path) -> None: + """A vanished file cannot make the process stale.""" + _touch(fake_package / "module.py", 3000.0) + with ( + patch(f"{_PKG}._PACKAGE_ROOT", fake_package), + patch( + "steam_backlog_enforcer._serve_stale.Path.stat", + side_effect=OSError("gone"), + ), + ): + assert outdated_source(2000.0, now=0.0) is None + + def test_the_answer_is_memoised(self, fake_package: Path) -> None: + """Statting the whole package on every request would be wasteful.""" + _touch(fake_package / "module.py", 1000.0) + with patch(f"{_PKG}._PACKAGE_ROOT", fake_package) as root: + outdated_source(2000.0, now=0.0) + # A change inside the memo window is deliberately not seen yet. + _touch(root / "module.py", 3000.0) + assert outdated_source(2000.0, now=1.0) is None + + def test_the_memo_expires(self, fake_package: Path) -> None: + """The staleness window is bounded, unlike the staleness it replaces.""" + _touch(fake_package / "module.py", 1000.0) + with patch(f"{_PKG}._PACKAGE_ROOT", fake_package) as root: + outdated_source(2000.0, now=0.0) + _touch(root / "module.py", 3000.0) + later = _RECHECK_INTERVAL_SECONDS + 1 + assert outdated_source(2000.0, now=later) is not None + + def test_it_reads_the_clock_when_not_told_one(self, fake_package: Path) -> None: + """The production path takes no `now`; exercise that branch too.""" + _touch(fake_package / "module.py", 1000.0) + with patch(f"{_PKG}._PACKAGE_ROOT", fake_package): + assert outdated_source(2000.0) is None diff --git a/steam_backlog_enforcer/tests/test_web_server_stale.py b/steam_backlog_enforcer/tests/test_web_server_stale.py new file mode 100644 index 0000000..7be083e --- /dev/null +++ b/steam_backlog_enforcer/tests/test_web_server_stale.py @@ -0,0 +1,80 @@ +"""Tests for the stale-code refusal in _web_server. + +Split out of test_web_server.py for the 250-line cap; the server harness stays +in that module and is imported here. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING +from unittest.mock import patch + +import pytest + +from steam_backlog_enforcer import _web_server +from steam_backlog_enforcer.tests.test_web_server import _PKG, _get, _running + +if TYPE_CHECKING: + from pathlib import Path + + +class TestStaleServerRefuses: + """A server outrun by its own source stops answering and stands down.""" + + def _clear_retiring(self) -> None: + """Reset the module-level stand-down latch between tests.""" + _web_server._RETIRING.clear() + + @pytest.mark.parametrize("route", ["/api/budget", "/api/dataset", "/"]) + def test_every_route_refuses_when_stale(self, tmp_path: Path, route: str) -> None: + """Static too: a fresh page fetching stale numbers is the same lie. + + One server per route on purpose -- the first refusal stands the server + down, so a second request against it would (correctly) never answer. + """ + self._clear_retiring() + changed = tmp_path / "changed.py" + changed.touch() + with ( + patch(f"{_PKG}.outdated_source", return_value=changed), + _running() as port, + ): + status, body, _ = _get(port, route) + assert status == 503 + assert b"outdated code" in body + self._clear_retiring() + + def test_it_stands_down_so_a_supervisor_can_replace_it( + self, + tmp_path: Path, + ) -> None: + """503 forever would be honest and useless; exiting self-heals.""" + self._clear_retiring() + changed = tmp_path / "changed.py" + changed.touch() + with ( + patch(f"{_PKG}.outdated_source", return_value=changed), + _running() as port, + ): + _get(port, "/api/budget") + assert _web_server._RETIRING.is_set() + self._clear_retiring() + + def test_a_second_refusal_does_not_stand_down_twice( + self, + tmp_path: Path, + ) -> None: + """The latch short-circuits, so concurrent refusals share one shutdown.""" + self._clear_retiring() + changed = tmp_path / "changed.py" + changed.touch() + _web_server._RETIRING.set() + with ( + patch(f"{_PKG}.outdated_source", return_value=changed), + _running() as port, + ): + # Latch already set: this refusal must take the early return and + # leave the server up, which is why the request still completes. + status, _, _ = _get(port, "/api/budget") + assert status == 503 + self._clear_retiring()