diff --git a/BlocksScreen/lib/moonrakerComm.py b/BlocksScreen/lib/moonrakerComm.py index ba0df898..77a06cee 100644 --- a/BlocksScreen/lib/moonrakerComm.py +++ b/BlocksScreen/lib/moonrakerComm.py @@ -22,7 +22,7 @@ class OneShotTokenError(Exception): """Raised when unable to get oneshot token to connect to a websocket""" def __init__(self, message="Unable to get oneshot token", errors=None) -> None: - super(OneShotTokenError).__init__(message, errors) + super().__init__(message) self.errors = errors self.message = message diff --git a/BlocksScreen/lib/panels/widgets/updatePage.py b/BlocksScreen/lib/panels/widgets/updatePage.py index 183b4294..98b63125 100644 --- a/BlocksScreen/lib/panels/widgets/updatePage.py +++ b/BlocksScreen/lib/panels/widgets/updatePage.py @@ -137,9 +137,7 @@ def resizeEvent(self, a0: QtGui.QResizeEvent | None) -> None: return super().resizeEvent(a0) def _needs_update(self, status: ComponentStatus) -> bool: - # Mirrors the daemon's dirty-set in _run_update_all: an errored git repo - # (e.g. a corrupt repo) is included so the one "Update" button shows and - # the update flow self-heals it. apt errors are not repairable this way. + # Mirrors daemon dirty-set: errored git repos self-heal; apt errors don't. return bool( status.commits_behind or status.packages_upgradable > 0 @@ -315,9 +313,10 @@ def handle_status_ready(self, json_str: str) -> None: except (json.JSONDecodeError, TypeError) as exc: _log.error("handle_status_ready: bad payload '%s'", exc) _log.debug(json_str) + # Keep the last good list but tell the user it may be stale. + self._show_toast("Status update failed - tap refresh to retry") return - # Build per-component so one malformed entry (e.g. a newer daemon adding - # a field this UI build doesn't know) can't blank the whole list. + # Build per-component so one malformed entry can't blank the whole list. self._statuses = {} for name, fields in data.items(): try: @@ -381,6 +380,9 @@ def on_update_all_clicked(self) -> None: self._show_update_confirm() def _show_update_confirm(self) -> None: + # Dialogs parented to the page outlive close(); drop the previous one. + if self._update_confirm_popup is not None: + self._update_confirm_popup.deleteLater() popup = BasePopup(self, floating=True) popup.set_message( "The printer will restart.\n" diff --git a/BlocksScreen/lib/updater_worker.py b/BlocksScreen/lib/updater_worker.py index e12969c3..bea90e6a 100644 --- a/BlocksScreen/lib/updater_worker.py +++ b/BlocksScreen/lib/updater_worker.py @@ -22,10 +22,7 @@ _RECONNECT_DELAYS = (5.0, 15.0, 30.0, 60.0) -# Max seconds of *silence* from a busy daemon before declaring it gone. -# Progress signals refresh the deadline, so long multi-component updates -# (big apt upgrades, several klipper restarts) never trip it as long as -# the daemon keeps reporting steps. +# Max silence from a busy daemon; progress signals refresh the deadline. _BUSY_IDLE_LIMIT = 360.0 @@ -147,16 +144,14 @@ async def _async_initialize(self) -> None: self._listener_tasks.append(task) task.add_done_callback(self._on_listener_done) - # Let each listener enter its async-for and register its D-Bus signal - # subscription before we poll current state (one sleep(0) per task). + # One sleep(0) per task lets each listener register its subscription. for _ in listeners: await asyncio.sleep(0) try: busy = await self._proxy.get_busy() except sdbus.SdBusBaseError as exc: - # Proxy creation is lazy; this first real call is what proves the - # daemon is actually reachable. Treat failure as daemon-down. + # Proxy is lazy; this first call proves the daemon is reachable. _log.warning("get_busy failed on (re)connect: %s - scheduling retry", exc) self.daemon_unavailable.emit() self._schedule_reconnect() diff --git a/scripts/bs-apt-helper.sh b/scripts/bs-apt-helper.sh new file mode 100644 index 00000000..1e0cb6d4 --- /dev/null +++ b/scripts/bs-apt-helper.sh @@ -0,0 +1,53 @@ +#!/bin/bash +# bs-apt-helper: the only apt entry sudoers grants 'blocks'; fixed argvs stop option injection. +set -euo pipefail + +APT_GET=/usr/bin/apt-get +DPKG=/usr/bin/dpkg +LOCK=(-o DPkg::Lock::Timeout=60) +CONF=(-o Dpkg::Options::=--force-confdef -o Dpkg::Options::=--force-confold) +export DEBIAN_FRONTEND=noninteractive NEEDRESTART_MODE=a + +usage() { + echo "usage: bs-apt-helper {update|upgrade |autoremove|fix-broken|dselect-upgrade|set-selections}" >&2 + exit 2 +} + +verb="${1:-}" +[ $# -ge 1 ] && shift +case "$verb" in + update) + [ $# -eq 0 ] || usage + exec "$APT_GET" "${LOCK[@]}" update + ;; + upgrade) + [ $# -ge 1 ] || usage + for p in "$@"; do + # Debian name shape; first char alnum so '-options' can't pass. + [[ "$p" =~ ^[a-zA-Z0-9][a-zA-Z0-9+.-]*$ ]] || { + echo "bs-apt-helper: invalid package name: $p" >&2 + exit 2 + } + done + exec "$APT_GET" "${LOCK[@]}" "${CONF[@]}" install --only-upgrade -y "$@" + ;; + autoremove) + [ $# -eq 0 ] || usage + exec "$APT_GET" "${LOCK[@]}" autoremove -y + ;; + fix-broken) + [ $# -eq 0 ] || usage + exec "$APT_GET" "${LOCK[@]}" "${CONF[@]}" -f install -y + ;; + dselect-upgrade) + [ $# -eq 0 ] || usage + exec "$APT_GET" "${LOCK[@]}" "${CONF[@]}" dselect-upgrade -y + ;; + set-selections) + [ $# -eq 0 ] || usage + exec "$DPKG" --set-selections + ;; + *) + usage + ;; +esac diff --git a/scripts/install-updater.sh b/scripts/install-updater.sh index dfab8721..929134f5 100755 --- a/scripts/install-updater.sh +++ b/scripts/install-updater.sh @@ -15,8 +15,7 @@ flock -x -w 30 200 || { echo_error "Another install-updater.sh is already runnin SCRIPT_PATH=$(dirname -- "$(readlink -f -- "$0")") BS_PATH=$(dirname "$SCRIPT_PATH") -# Service always runs as 'blocks'; derive home from that, not the caller's identity. -# Callers vary (sudo, post-merge hook, root shell) so SUDO_USER/$USER are unreliable. +# Always derive home from 'blocks': callers vary, SUDO_USER/$USER unreliable. _BSENV_USER="blocks" _BSENV_HOME=$(getent passwd "$_BSENV_USER" | cut -d: -f6) BSENV="${BLOCKSSCREEN_VENV:-${_BSENV_HOME}/.BlocksScreen-env}" @@ -30,6 +29,12 @@ echo_info "Installing D-Bus activation file ..." sudo cp "$SCRIPT_PATH/com.blockscreen.Updater.service" /usr/share/dbus-1/system-services/ echo_ok "D-Bus activation file installed" +echo_info "Installing bs-apt-helper (root-owned apt wrapper) ..." +# Atomic + before the daemon (re)start: new updater code has no apt fallback. +sudo install -o root -g root -m 0755 "$SCRIPT_PATH/bs-apt-helper.sh" /usr/local/sbin/.bs-apt-helper.new +sudo mv -Tf /usr/local/sbin/.bs-apt-helper.new /usr/local/sbin/bs-apt-helper +echo_ok "bs-apt-helper installed" + echo_info "Installing BlocksScreen-updater service ..." SERVICE=$(cat "$SCRIPT_PATH/BlocksScreen-updater.service") SERVICE=${SERVICE//BS_DIR/$BS_PATH} @@ -51,18 +56,13 @@ echo_ok "Spoolman service unit installed" echo_info "Installing sudoers rules for updater ..." SUDOERS_FILE="/etc/sudoers.d/blockscreen-updater" SUDOERS_TMP=$(mktemp) -printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/apt-get update\n' >>"$SUDOERS_TMP" -printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/apt-get install --only-upgrade -y *\n' >>"$SUDOERS_TMP" -printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/apt-get install -y --quiet *\n' >>"$SUDOERS_TMP" -printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/apt-get autoremove -y\n' >>"$SUDOERS_TMP" +# The wrapper is the only apt/dpkg grant; kept in sync by tests/scripts/test_sudoers_rules.py. +printf 'blocks ALL=(ALL) NOPASSWD: /usr/local/sbin/bs-apt-helper *\n' >>"$SUDOERS_TMP" printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/systemctl reboot\n' >>"$SUDOERS_TMP" printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/chvt 7\n' >>"$SUDOERS_TMP" printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/chvt 8\n' >>"$SUDOERS_TMP" printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/bash %s/scripts/install-updater.sh\n' "$BS_PATH" >>"$SUDOERS_TMP" -# Derive allowed restart targets from components.yaml instead of wildcard. -# Per service we allow: restart (normal), reset-failed (start-limit recovery, -# replaces the old systemctl-kill fallback), and --no-block restart (used for the -# self UI service so a slow restart never blocks/aborts the update batch). +# Per components.yaml service: restart, reset-failed (start-limit), --no-block restart. _emit_svc_rules() { local _s="$1" printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/systemctl restart %s\n' "$_s" >>"$SUDOERS_TMP" @@ -79,11 +79,9 @@ else _emit_svc_rules "$_svc" done fi -# The daemon restarts itself (--no-block) to adopt new updater code after a -# successful self-update; this service is never in components.yaml. +# Daemon self-restart target; never in components.yaml. _emit_svc_rules BlocksScreen-updater.service -# Spoolman is provisioned on demand: the daemon enables its unit after a clean -# first start (restart rules are auto-derived above from components.yaml). +# Spoolman is provisioned on demand; enable rule needed for its first clean start. printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/systemctl enable Spoolman.service\n' >>"$SUDOERS_TMP" if sudo visudo -cf "$SUDOERS_TMP" >/dev/null 2>&1; then sudo install -m 0440 "$SUDOERS_TMP" "$SUDOERS_FILE" @@ -110,13 +108,10 @@ echo_info "Converting BlocksScreen.service copy to symlink (enables sudo-free ho _BS_SVC_SRC="$BS_PATH/scripts/BlocksScreen.service" _BS_SVC_DEST="/etc/systemd/system/BlocksScreen.service" if [[ ! -f "$_BS_SVC_SRC" ]]; then - # Never remove the running unit when there is no source to replace it with: - # a missing BlocksScreen.service on a no-SSH box is unrecoverable. + # Never remove the running unit without a replacement; the device has no SSH recovery. echo_info "WARN: $_BS_SVC_SRC missing, leaving existing BlocksScreen.service intact" elif [[ "$(readlink -f "$_BS_SVC_DEST" 2>/dev/null)" != "$(readlink -f "$_BS_SVC_SRC")" ]]; then - # Atomic replace: create the symlink under a temp name and rename it over the - # destination, so the unit is never absent. The old rm-then-link left a window - # where a link failure (e.g. missing source) deleted the service outright. + # Atomic replace via temp symlink + rename: the unit is never absent. sudo ln -sfn "$_BS_SVC_SRC" "${_BS_SVC_DEST}.new" sudo mv -Tf "${_BS_SVC_DEST}.new" "$_BS_SVC_DEST" sudo systemctl unmask BlocksScreen.service 2>/dev/null || true @@ -132,11 +127,7 @@ sudo chown -R blocks:blocksscreen "$_BSENV_HOME/.cache/blockscreen" sudo chmod 775 "$_BSENV_HOME/.cache/blockscreen" echo_ok "Apt cache directory ready" -# Re-arm the crash-loop boot counter on a deliberate deploy. A stale count left -# over from a prior unstable period would otherwise let BlocksScreen-start.sh -# roll this fresh install straight back to last_good on the first boot. Reset -# boot_attempts only; last_good_commit stays as the rollback target so crash-loop -# protection of the new code still works. +# Deploy re-arms boot_attempts only; last_good stays as the rollback target. echo_info "Re-arming crash-loop boot counter ..." printf '0\n' | sudo -u "$_BSENV_USER" tee "$_BSENV_HOME/.cache/blockscreen/boot_attempts" >/dev/null 2>&1 || true echo_ok "Boot counter re-armed (boot_attempts=0)" @@ -151,9 +142,18 @@ sudo systemd-tmpfiles --create --prefix /var/log/journal 2>/dev/null || true sudo systemctl restart systemd-journald 2>/dev/null || true echo_ok "Persistent journald enabled" -echo_info "Allowing blocks to operate on repos owned by primary user ..." -sudo git config --system --add safe.directory '*' -echo_ok "Git safe.directory configured" +echo_info "Scoping git safe.directory to component repos ..." +# Scope root git trust to component paths instead of '*'. +sudo git config --system --unset-all safe.directory 2>/dev/null || true +_SAFE_YAML="$BS_PATH/updater/components.yaml" +if [[ -f "$_SAFE_YAML" ]]; then + grep '^\s*path:' "$_SAFE_YAML" | awk '{print $2}' | sort -u | while read -r _p; do + sudo git config --system --add safe.directory "${_p/#\~/$_BSENV_HOME}" + done +fi +git config --system --get-all safe.directory 2>/dev/null | grep -qxF "$BS_PATH" || \ + sudo git config --system --add safe.directory "$BS_PATH" +echo_ok "Git safe.directory scoped" echo_info "Setting git repo permissions for updater ..." COMPONENTS_YAML="$BS_PATH/updater/components.yaml" @@ -206,14 +206,12 @@ git -C "$BS_PATH" config core.hooksPath scripts echo_ok "post-merge hook installed" echo_info "Installing Python requirements ..." -# Keep pip itself current on each update (best-effort: a failed self-upgrade must -# not block the requirements install below). +# Best-effort pip self-update; must not block the requirements install below. "$BSENV/bin/pip" install --quiet --upgrade pip 2>/dev/null || true apt-get install -y --quiet libsystemd-dev python3-dev 2>/dev/null || true # xsetroot is used as belt-and-suspenders cursor hiding alongside the Xorg -nocursor server flag. sudo apt-get install -y --quiet x11-xserver-utils 2>/dev/null || true -# sdbus is pinned in requirements.txt (0.12.0); --no-binary forces a clean source build -# (needs libsystemd-dev). --upgrade-strategy=only-if-needed skips satisfied packages. +# sdbus pinned; --no-binary needs libsystemd-dev; only-if-needed skips satisfied. "$BSENV/bin/pip" install --quiet --only-binary :all: --no-binary sdbus,sdbus-networkmanager \ --upgrade-strategy=only-if-needed \ -r "$BS_PATH/scripts/requirements.txt" || true diff --git a/scripts/post-merge b/scripts/post-merge index 27f3194d..30c9dd27 100755 --- a/scripts/post-merge +++ b/scripts/post-merge @@ -14,8 +14,7 @@ bs_migrate_moonraker_conf "$_BSENV_HOME/printer_data/config/moonraker.conf" post changed=$(git -C "$BS_PATH" diff ORIG_HEAD HEAD --name-only 2>/dev/null || true) -# ── Pre-flight: run all heavy prep before any restart ──────────────────────── -# Everything here is idempotent - safe to re-run on every merge. +# Pre-flight: heavy prep before any restart; everything here is idempotent. # 1. Pre-install pip requirements so the next service start is instant. REQS_HASH=$(md5sum "$BS_PATH/scripts/requirements.txt" | cut -d' ' -f1) @@ -32,10 +31,7 @@ if [ ! -f "$SENTINEL" ] || [ "$(cat "$SENTINEL")" != "$REQS_HASH" ]; then rm -f "$ATTEMPT" 2>/dev/null || true echo "[post-merge] deps installed" else - # An atomic -r failure (e.g. a dep without an aarch64 wheel) must not - # re-thrash on every pull. Retry a few times - covering transient or - # offline failures - then accept the partial venv so the sentinel - # advances and the boot path stops retrying a known-failing set. + # Retry atomic -r failures 3x, then accept the partial venv to stop thrash. _prev=$(cut -d' ' -f1 "$ATTEMPT" 2>/dev/null || true) _cnt=$(cut -d' ' -f2 "$ATTEMPT" 2>/dev/null || true) case "$_cnt" in '' | *[!0-9]*) _cnt=0 ;; esac @@ -60,13 +56,8 @@ fi # 4. Precompute splash cache so the first Qt restart shows the right logo. "$BSENV/bin/python3.11" "$SCRIPT_PATH/bs-splash.py" --precompute 2>/dev/null || true -# ───────────────────────────────────────────────────────────────────────────── -# ── Service restart based on what changed ──────────────────────────────────── -# When run by the updater daemon's own `git pull` (mid-batch, inside its cgroup), -# restarting the daemon here would SIGTERM the cgroup and abort+revert the -# update. In that case record the reason to the sentinel and let the daemon act -# once, after the batch completes. Manual pulls (no env) restart as before. +# Under the daemon's own pull a restart here aborts the batch: defer via sentinel. _record_or() { local _reason="$1"; shift if [ -n "${BS_UPDATER_SELF_UPDATE:-}" ] && [ -n "${BS_UPDATER_RESTART_SENTINEL:-}" ]; then @@ -78,7 +69,7 @@ _record_or() { fi } -if echo "$changed" | grep -qE '^scripts/BlocksScreen-updater\.service$|^scripts/BlocksScreen-bootstrap\.service$|^scripts/com\.blockscreen\.Updater\.conf$|^scripts/install-updater\.sh$'; then +if echo "$changed" | grep -qE '^scripts/BlocksScreen-updater\.service$|^scripts/BlocksScreen-bootstrap\.service$|^scripts/com\.blockscreen\.Updater\.conf$|^scripts/install-updater\.sh$|^scripts/bs-apt-helper\.sh$'; then echo "[post-merge] Updater install files changed - reinstalling updater ..." _record_or install sudo bash "$SCRIPT_PATH/install-updater.sh" elif echo "$changed" | grep -qE '^scripts/bs-splash-holder\.py$'; then diff --git a/tests/amu/test_config_toggler_unit.py b/tests/amu/test_config_toggler_unit.py index ccd269f6..81acf6f7 100644 --- a/tests/amu/test_config_toggler_unit.py +++ b/tests/amu/test_config_toggler_unit.py @@ -1,6 +1,5 @@ """Unit tests for BlocksScreen.devices.amu.config_toggler.""" -import pytest from BlocksScreen.devices.amu.config_toggler import ConfigToggler from tests.amu.conftest import COMMENTED_CFG, UNCOMMENTED_CFG diff --git a/tests/amu/test_manager_unit.py b/tests/amu/test_manager_unit.py index 8e93d58d..fe462544 100644 --- a/tests/amu/test_manager_unit.py +++ b/tests/amu/test_manager_unit.py @@ -157,7 +157,7 @@ def test_from_status_gate_speed_override_defaults_to_empty(self, manager) -> Non assert manager.get_state().gate_speed_override == () def test_from_status_parses_endless_spool_groups(self, manager) -> None: - data = {**_FULL_STATUS, "endless_spool_groups": [0, 1, 3,0]} + data = {**_FULL_STATUS, "endless_spool_groups": [0, 1, 3, 0]} manager.update_mmu_state(data) assert manager.get_state().endless_spool_groups == (0, 1, 3, 0) diff --git a/tests/amu/test_models_unit.py b/tests/amu/test_models_unit.py index cd099257..ee969cc0 100644 --- a/tests/amu/test_models_unit.py +++ b/tests/amu/test_models_unit.py @@ -140,7 +140,7 @@ def _full_status_extended(self, num_gates: int = 2) -> dict: def test_from_status_builds_correctly(self) -> None: state: MMUState = MMUState.from_status(self._full_status()) assert state.num_gates == 2 - assert state.enabled == True + assert state.enabled assert len(state.gates) == 2 assert state.gates[0].material == "PLA" assert state.gates[1].status == GateStatus.EMPTY diff --git a/tests/amu/test_moonrest_unit.py b/tests/amu/test_moonrest_unit.py index a154dc2a..2291aad3 100644 --- a/tests/amu/test_moonrest_unit.py +++ b/tests/amu/test_moonrest_unit.py @@ -1,18 +1,24 @@ """Unit tests for BlocksScreen.lib.moonrest""" + from unittest.mock import patch import pytest from BlocksScreen.lib.moonrest import MoonRest + @pytest.fixture def rest(): """MoonRest instance poiting at localhost""" return MoonRest(host="localhost", port=7125) + class TestGetSpool: def test_return_spool_dict_on_sucess(self, rest) -> None: - spool_data = {"id": 42, "filament": {"name": "PLA", "color_hex": "ff0000"}, - "used_weight": 50.0, "remaining_weight": 200.0 - } + spool_data = { + "id": 42, + "filament": {"name": "PLA", "color_hex": "ff0000"}, + "used_weight": 50.0, + "remaining_weight": 200.0, + } with patch.object(rest, "get_request", return_value={"result": spool_data}): assert rest.get_spool(42) == spool_data @@ -29,6 +35,7 @@ def test_calls_correct_endpoint(self, rest) -> None: rest.get_spool(7) mock_get.assert_called_once_with("server/spoolman/spool/7") + class TestSetSpoolUsedWeight: def test_returns_true_on_sucess(self, rest) -> None: with patch.object(rest, "post_request", return_value={"result": "ok"}): @@ -37,15 +44,10 @@ def test_returns_true_on_sucess(self, rest) -> None: def test_returns_false_on_error(self, rest) -> None: with patch.object(rest, "post_request", return_value={"result": "ok"}): assert rest.set_spool_used_weight(42, 75.5) - + def test_calls_correct_endpoint(self, rest) -> None: with patch.object(rest, "post_request", return_value=None) as mock_post: rest.set_spool_used_weight(5, 100.0) mock_post.assert_called_once_with( - "server/spoolman/spool/5", json={ - "used_weight": 100.0 - } + "server/spoolman/spool/5", json={"used_weight": 100.0} ) - - - diff --git a/tests/lib/test_moonraker_comm_unit.py b/tests/lib/test_moonraker_comm_unit.py new file mode 100644 index 00000000..115a0be0 --- /dev/null +++ b/tests/lib/test_moonraker_comm_unit.py @@ -0,0 +1,19 @@ +"""Unit tests for moonrakerComm exception types.""" + +from BlocksScreen.lib.moonrakerComm import OneShotTokenError + + +class TestOneShotTokenError: + def test_str_carries_message(self): + # A super(OneShotTokenError).__init__ regression left str(exc) empty. + exc = OneShotTokenError("token fetch failed") + assert str(exc) == "token fetch failed" + assert exc.message == "token fetch failed" + + def test_default_message(self): + exc = OneShotTokenError() + assert str(exc) == "Unable to get oneshot token" + + def test_errors_attribute_kept(self): + exc = OneShotTokenError("boom", errors={"code": 401}) + assert exc.errors == {"code": 401} diff --git a/tests/lib/test_repeated_timer_unit.py b/tests/lib/test_repeated_timer_unit.py index 00ceea48..42f1740e 100644 --- a/tests/lib/test_repeated_timer_unit.py +++ b/tests/lib/test_repeated_timer_unit.py @@ -3,8 +3,9 @@ from BlocksScreen.lib.utils.RepeatedTimer import RepeatedTimer + def test_stop_during_callback_does_not_rearm(): - """stopTimer() called while the callback is running must stop the timer. """ + """stopTimer() called while the callback is running must stop the timer.""" in_callback = threading.Event() release = threading.Event() calls = [] @@ -17,13 +18,14 @@ def slow_cb(): rt = RepeatedTimer(0.01, slow_cb) assert in_callback.wait(timeout=2), "callback never ran" - rt.stopTimer() # stop while the callback is still executing + rt.stopTimer() # stop while the callback is still executing release.set() - time.sleep(0.2) # give any erroneous re-arm time to fire + time.sleep(0.2) # give any erroneous re-arm time to fire assert rt.running is False assert len(calls) == 1, f"timer re-armed after stop: {len(calls)} calls" + def test_normal_repeat_still_fires(): calls = [] rt = RepeatedTimer(0.01, lambda: calls.append(1)) diff --git a/tests/lib/test_updater_worker_unit.py b/tests/lib/test_updater_worker_unit.py index db8400de..a4a8efca 100644 --- a/tests/lib/test_updater_worker_unit.py +++ b/tests/lib/test_updater_worker_unit.py @@ -36,7 +36,7 @@ def worker(): class TestTriggerMethods: def test_trigger_status_sibmits_to_loop(self, worker): with patch( - "asyncio.run_coroutine_threadsafe", side_effect=lambda c, l: c.close() + "asyncio.run_coroutine_threadsafe", side_effect=lambda c, loop: c.close() ) as mock_rctf: worker.trigger_status() mock_rctf.assert_called_once() @@ -44,21 +44,21 @@ def test_trigger_status_sibmits_to_loop(self, worker): def test_trigger_update_empty_name_calls_update_all(self, worker): with patch( - "asyncio.run_coroutine_threadsafe", side_effect=lambda c, l: c.close() + "asyncio.run_coroutine_threadsafe", side_effect=lambda c, loop: c.close() ) as mock_rctf: worker.trigger_update("") mock_rctf.assert_called_once() def test_trigger_update_name_calls_update_component(self, worker): with patch( - "asyncio.run_coroutine_threadsafe", side_effect=lambda c, l: c.close() + "asyncio.run_coroutine_threadsafe", side_effect=lambda c, loop: c.close() ) as mock_rctf: worker.trigger_update("klipper") mock_rctf.assert_called_once() def test_trigger_recover_submits_coroutine(self, worker): with patch( - "asyncio.run_coroutine_threadsafe", side_effect=lambda c, l: c.close() + "asyncio.run_coroutine_threadsafe", side_effect=lambda c, loop: c.close() ) as mock_rctf: worker.trigger_recover("klipper", True) mock_rctf.assert_called_once() diff --git a/tests/network/conftest.py b/tests/network/conftest.py index 85e07ec8..2825b801 100644 --- a/tests/network/conftest.py +++ b/tests/network/conftest.py @@ -302,11 +302,15 @@ def set_value(self, val): # Now safe to import the actual network package from BlocksScreen.lib.network.models import ( # noqa: E402 - SIGNAL_EXCELLENT_THRESHOLD, SIGNAL_FAIR_THRESHOLD, SIGNAL_GOOD_THRESHOLD, - SIGNAL_MINIMUM_THRESHOLD, ConnectionPriority, ConnectionResult, - ConnectivityState, HotspotConfig, NetworkInfo, NetworkState, NetworkStatus, - PendingOperation, SavedNetwork, SecurityType, VlanInfo, WifiIconKey, - signal_to_bars) + ConnectionPriority, + ConnectivityState, + NetworkInfo, + NetworkState, + NetworkStatus, + PendingOperation, + SavedNetwork, + SecurityType, +) # Alias so ``from lib.network import ...`` works. sys.modules["lib.network"] = sys.modules["BlocksScreen.lib.network"] diff --git a/tests/network/test_manager_unit.py b/tests/network/test_manager_unit.py index d8a0ad55..9b4daed1 100644 --- a/tests/network/test_manager_unit.py +++ b/tests/network/test_manager_unit.py @@ -12,17 +12,19 @@ """ import asyncio -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import MagicMock, patch import pytest import BlocksScreen.lib.network.manager as manager_mod -from BlocksScreen.lib.network.models import (ConnectionPriority, - ConnectionResult, - ConnectivityState, HotspotConfig, - NetworkInfo, NetworkState, - NetworkStatus, SavedNetwork, - SecurityType) +from BlocksScreen.lib.network.models import ( + ConnectionPriority, + ConnectivityState, + HotspotConfig, + NetworkInfo, + NetworkState, + SavedNetwork, +) def _make_mock_timer(): @@ -110,7 +112,6 @@ def nm(qapp): yield nm_inst - def test_manager_hotspot_cache_seeded_from_worker_config(qapp): """Manager must read saved config from worker synchronously, not use defaults.""" with ( @@ -145,7 +146,6 @@ def test_manager_hotspot_cache_seeded_from_worker_config(qapp): assert nm_inst.hotspot_password == "SavedPass" - class TestFacadeCreation: def test_initial_cached_state(self, nm): assert isinstance(nm._cached_state, NetworkState) @@ -202,7 +202,6 @@ async def coro(): test.send(None) - class TestCacheSlots: def test_on_state_changed(self, nm): state = NetworkState( @@ -305,7 +304,6 @@ def test_on_networks_scanned_duplicate_ssid_last_wins(self, nm): assert nm._network_info_map["Same"].signal_strength == 90 - class TestConvenienceProperties: def test_current_ssid(self, nm): nm._cached_state = NetworkState(current_ssid="MySSID") @@ -363,7 +361,6 @@ def test_get_saved_network_not_found(self, nm): assert nm.get_saved_network("Ghost") is None - class TestPublicAPI: def _count(self, nm): return nm._mock_asyncio.run_coroutine_threadsafe.call_count @@ -475,7 +472,6 @@ def test_schedule_drops_when_shutting_down(self, nm): assert self._count(nm) == b - class TestKeepalive: def test_keepalive_tick_dispatches(self, nm): b = nm._mock_asyncio.run_coroutine_threadsafe.call_count @@ -499,7 +495,6 @@ def test_keepalive_tick_refreshes_state_connectivity_saved(self, nm): assert nm._mock_asyncio.run_coroutine_threadsafe.call_count == b + 3 - class TestWorkerInitialized: def test_sets_worker_ready(self, nm): nm._on_worker_initialized() @@ -541,7 +536,6 @@ def fake_schedule(coro): assert fired_with_ms == [123] - class TestFacadeShutdown: def test_shutdown_sets_flag(self, nm): nm.shutdown() @@ -568,7 +562,9 @@ def test_shutdown_swallows_future_exception(self, nm, caplog): nm._mock_asyncio.run_coroutine_threadsafe.return_value.result.side_effect = ( RuntimeError("async shutdown failed") ) - with caplog.at_level(logging.WARNING, logger="BlocksScreen.lib.network.manager"): + with caplog.at_level( + logging.WARNING, logger="BlocksScreen.lib.network.manager" + ): nm.shutdown() assert nm._shutting_down is True assert "async shutdown failed" in caplog.text @@ -580,7 +576,6 @@ def test_shutdown_skips_coroutine_when_loop_not_running(self, nm): assert nm._shutting_down is True - class TestSignalForwarding: def test_error_occurred_signal_exists(self, nm): """Facade exposes error_occurred as a real pyqtSignal.""" diff --git a/tests/network/test_models_unit.py b/tests/network/test_models_unit.py index 5eb4b451..3d042858 100644 --- a/tests/network/test_models_unit.py +++ b/tests/network/test_models_unit.py @@ -2,20 +2,28 @@ import pytest -from BlocksScreen.lib.network.models import (SIGNAL_EXCELLENT_THRESHOLD, - SIGNAL_FAIR_THRESHOLD, - SIGNAL_GOOD_THRESHOLD, - SIGNAL_MINIMUM_THRESHOLD, - ConnectionPriority, - ConnectionResult, - ConnectivityState, HotspotConfig, - HotspotSecurity, NetworkInfo, - NetworkState, NetworkStatus, - PendingOperation, SavedNetwork, - SecurityType, VlanInfo, - WifiIconKey, - is_connectable_security, - is_hidden_ssid, signal_to_bars) +from BlocksScreen.lib.network.models import ( + SIGNAL_EXCELLENT_THRESHOLD, + SIGNAL_FAIR_THRESHOLD, + SIGNAL_GOOD_THRESHOLD, + SIGNAL_MINIMUM_THRESHOLD, + ConnectionPriority, + ConnectionResult, + ConnectivityState, + HotspotConfig, + HotspotSecurity, + NetworkInfo, + NetworkState, + NetworkStatus, + PendingOperation, + SavedNetwork, + SecurityType, + VlanInfo, + WifiIconKey, + is_connectable_security, + is_hidden_ssid, + signal_to_bars, +) class TestSecurityType: @@ -65,13 +73,12 @@ def test_all_expected_values(self): } def test_is_connectable_security(self): - assert is_connectable_security(SecurityType.WEP.value) == False + assert not is_connectable_security(SecurityType.WEP.value) def test_not_connectable_security(self): assert is_connectable_security("No security") - class TestConnectivityState: def test_all_states_exist(self): assert len(ConnectivityState) == 5 @@ -93,7 +100,6 @@ def test_int_conversion(self): assert ConnectivityState.UNKNOWN + 1 == 1 - class TestConnectionPriority: def test_ordering(self): assert ( @@ -113,7 +119,6 @@ def test_medium_is_default_for_saved_network(self): assert sn.priority == ConnectionPriority.MEDIUM.value - class TestPendingOperation: def test_all_members_exist(self): assert len(PendingOperation) == 9 @@ -180,7 +185,6 @@ def test_ethernet_in_tuple_lookup(self): assert op not in (PendingOperation.WIFI_ON, PendingOperation.WIFI_OFF) - class TestSignalToBars: def test_zero_signal(self): assert signal_to_bars(0) == 0 @@ -226,7 +230,6 @@ def test_thresholds_are_ordered(self): ) - class TestWifiIconKey: def test_ethernet_value(self): assert WifiIconKey.ETHERNET == -1 @@ -314,7 +317,6 @@ def test_is_int_enum(self): assert isinstance(member.value, int) - class TestNetworkInfo: def test_defaults(self): info = NetworkInfo() @@ -409,7 +411,6 @@ def test_hashable(self, make_network_info): assert info in s - class TestSavedNetwork: def test_defaults(self): sn = SavedNetwork() @@ -434,7 +435,6 @@ def test_equality(self, make_saved_network): assert a == b - class TestConnectionResult: def test_success(self): r = ConnectionResult(success=True, message="OK") @@ -467,7 +467,6 @@ def test_all_fields_populated(self): assert r.data["ssid"] == "Net" - class TestNetworkState: def test_defaults(self): s = NetworkState() @@ -528,7 +527,6 @@ def test_full_state_snapshot(self): assert s.signal_strength == 85 - class TestHotspotConfig: def test_defaults(self): c = HotspotConfig() @@ -551,13 +549,11 @@ def test_custom_init(self): assert c.band == "a" - class TestHotspotSecurity: def test_is_valid_returns_false_for_invalid_value(self): assert not HotspotSecurity.is_valid("invalid_value") - class TestIsHiddenSSID: @pytest.mark.parametrize( "ssid", @@ -618,7 +614,6 @@ def test_enterprise_ssid_not_hidden(self): assert is_hidden_ssid("CorporateWPA2") is False - class TestVlanInfo: def test_defaults(self): v = VlanInfo() @@ -648,7 +643,6 @@ def test_frozen(self): v.vlan_id = 99 - class TestIsConnectableSecurity: @pytest.mark.parametrize( "sec", @@ -678,7 +672,6 @@ def test_unknown_string_is_connectable(self): assert is_connectable_security("some-future-type") is True - class TestSignalToBarsThresholds: @pytest.mark.parametrize( "signal,expected_bars", @@ -699,7 +692,6 @@ def test_threshold_boundaries(self, signal, expected_bars): assert signal_to_bars(signal) == expected_bars - class TestWifiIconKeyEdgeCases: def test_from_bars_negative_raises(self): with pytest.raises(ValueError): diff --git a/tests/network/test_network_ui.py b/tests/network/test_network_ui.py index 466ec14a..9fd4e750 100644 --- a/tests/network/test_network_ui.py +++ b/tests/network/test_network_ui.py @@ -26,15 +26,21 @@ from unittest.mock import MagicMock, patch import pytest -from PyQt6 import QtCore, QtGui, QtWidgets - -from BlocksScreen.lib.network.models import (ConnectionPriority, - ConnectionResult, - ConnectivityState, NetworkInfo, - NetworkState, NetworkStatus, - PendingOperation, SavedNetwork, - SecurityType, WifiIconKey, - signal_to_bars) +from PyQt6 import QtGui, QtWidgets + +from BlocksScreen.lib.network.models import ( + ConnectionPriority, + ConnectionResult, + ConnectivityState, + NetworkInfo, + NetworkState, + NetworkStatus, + PendingOperation, + SavedNetwork, + SecurityType, + WifiIconKey, + signal_to_bars, +) from BlocksScreen.lib.panels.networkWindow import NetworkControlWindow # ───────────────────────────────────────────────────────────────────────────── @@ -602,7 +608,6 @@ def test_static_ip_keeps_loading_for_old_ip(self, win): assert w._is_connecting - # ───────────────────────────────────────────────────────────────────────────── # _on_operation_complete # ───────────────────────────────────────────────────────────────────────────── @@ -886,7 +891,6 @@ def test_generic_timeout_shows_error_popup(self, win): assert not w.loadingwidget.isVisible() - # ───────────────────────────────────────────────────────────────────────────── # _on_network_error # ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/network/test_worker_unit.py b/tests/network/test_worker_unit.py index 43fce526..b8a8fa7a 100644 --- a/tests/network/test_worker_unit.py +++ b/tests/network/test_worker_unit.py @@ -13,19 +13,25 @@ """ import asyncio -from unittest.mock import AsyncMock, MagicMock, call, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest from PyQt6.QtCore import QObject from BlocksScreen.lib.network import worker as _worker_mod -from BlocksScreen.lib.network.models import (ConnectionPriority, - ConnectionResult, - ConnectivityState, HotspotConfig, - NetworkInfo, NetworkState, - NetworkStatus, SavedNetwork, - SecurityType) +from BlocksScreen.lib.network.models import ( + ConnectionPriority, + ConnectionResult, + ConnectivityState, + HotspotConfig, + NetworkInfo, + NetworkState, + NetworkStatus, + SavedNetwork, + SecurityType, +) from BlocksScreen.lib.network.worker import NetworkManagerWorker + # Import conftest helpers from tests.network.conftest import AsyncProxyMock, _ProxyFactory, _run @@ -61,7 +67,6 @@ def _make_worker(qapp, *, running=True, with_wifi=True, with_wired=False): w._consecutive_dbus_errors = 0 w._background_tasks = set() - # Signal-reactive architecture: persistent signal proxies w._signal_nm = None w._signal_wifi = None @@ -155,7 +160,6 @@ def _wire(w, *, nm=None, wifi_proxy=None, wired_proxy=None, settings=None): return w - class TestWorkerCreation: def test_initial_state(self, qapp): w = _bare_worker(qapp) @@ -205,7 +209,6 @@ def test_proxy_factories_exist(self, qapp): assert callable(getattr(w, name, None)), f"Missing factory: {name}" - class TestHotspotProperties: def test_hotspot_ssid(self, qapp): w = _bare_worker(qapp) @@ -216,7 +219,6 @@ def test_hotspot_password(self, qapp): assert w.hotspot_password == "123456789" - class TestAsyncInitialize: @pytest.mark.asyncio async def test_no_bus_emits_error(self, qapp): @@ -255,7 +257,6 @@ async def test_happy_path_sets_running(self, qapp): assert len(initialized) == 1 - class TestDetectInterfaces: @pytest.mark.asyncio async def test_exception_is_handled(self, qapp): @@ -308,7 +309,6 @@ async def test_detect_wired_device(self, qapp): assert w._primary_wired_iface == "eth0" - class TestDecodeSSID: def test_bytes_input(self): assert NetworkManagerWorker._decode_ssid(b"TestNetwork") == "TestNetwork" @@ -336,7 +336,6 @@ def test_integer_input(self): assert NetworkManagerWorker._decode_ssid(12345) == "12345" - class TestGetAllApProperties: @pytest.mark.asyncio async def test_success(self, qapp): @@ -362,7 +361,6 @@ async def test_exception_returns_empty_dict(self, qapp): assert result == {} - class TestIsEthernetConnected: @pytest.mark.asyncio async def test_no_wired_path_returns_false(self, qapp): @@ -390,7 +388,6 @@ async def test_exception_returns_false(self, qapp): assert await w._is_ethernet_connected() is False - class TestHasEthernetCarrier: @pytest.mark.asyncio async def test_no_wired_path_returns_false(self, qapp): @@ -412,7 +409,6 @@ async def test_state_10_returns_false(self, qapp): assert await w._has_ethernet_carrier() is False - class TestConnectivityMapping: @pytest.mark.parametrize( "value,expected", @@ -430,7 +426,6 @@ def test_map_connectivity(self, value, expected): assert NetworkManagerWorker._map_connectivity(value) == expected - class TestSecurityTypeDetection: def test_open_network(self): assert ( @@ -489,7 +484,6 @@ def test_iphone_transition_mode(self): assert result == SecurityType.WPA3_SAE - class TestSignalEmission: def test_state_changed_signal(self, qapp): w = _bare_worker(qapp) @@ -548,7 +542,6 @@ def test_reconnect_complete_signal(self, qapp): assert received == [True] - class TestCheckConnectivity: @pytest.mark.asyncio async def test_emits_unknown_when_no_bus(self, qapp): @@ -582,7 +575,6 @@ async def test_emits_unknown_on_error(self, qapp): assert received == [ConnectivityState.UNKNOWN] - class TestGetCurrentSSID: @pytest.mark.asyncio async def test_primary_connection_returns_ssid(self, qapp): @@ -611,7 +603,6 @@ async def test_exception_returns_empty(self, qapp): assert await w._get_current_ssid() == "" - class TestSSIDFromActiveConnection: @pytest.mark.asyncio async def test_returns_ssid_from_wireless_settings(self, qapp): @@ -654,7 +645,6 @@ async def test_exception_returns_empty(self, qapp): assert result == "" - class TestGetSSIDFromAnyActive: @pytest.mark.asyncio async def test_finds_wifi_in_active_connections(self, qapp): @@ -678,7 +668,6 @@ async def test_no_active_returns_empty(self, qapp): assert await w._get_ssid_from_any_active() == "" - class TestGetCurrentIp: @pytest.mark.asyncio async def test_primary_slash_returns_empty(self, qapp): @@ -708,7 +697,6 @@ async def test_exception_returns_empty(self, qapp): assert await w._get_current_ip() == "" - class TestGetIpByInterface: @pytest.mark.asyncio async def test_cached_path_used(self, qapp): @@ -739,13 +727,11 @@ async def test_exception_returns_empty(self, qapp): assert await w._get_ip_by_interface("wlan0") == "" - class TestGetIpOsFallback: def test_empty_interface(self): assert NetworkManagerWorker._get_ip_os_fallback("") == "" - class TestGetCurrentState: @pytest.mark.asyncio async def test_not_running_emits_default_state(self, qapp): @@ -767,7 +753,6 @@ async def test_exception_emits_error(self, qapp): assert len(errors) == 1 - class TestBuildCurrentState: @pytest.mark.asyncio async def test_returns_default_when_no_bus(self, qapp): @@ -899,7 +884,6 @@ async def test_exception_returns_default(self, qapp): assert state.connectivity == ConnectivityState.UNKNOWN - class TestScanNetworks: @pytest.mark.asyncio async def test_scan_no_wifi_path_emits_empty(self, qapp): @@ -1008,7 +992,6 @@ async def test_scan_exception_emits_error_and_empty(self, qapp): assert received == [[]] - class TestParseAp: @pytest.mark.asyncio async def test_empty_props_returns_none(self, qapp): @@ -1092,7 +1075,6 @@ async def test_hidden_ssid_returns_none(self, qapp): assert result is None - class TestBuildSignalMap: @pytest.mark.asyncio async def test_no_wifi_returns_empty(self, qapp): @@ -1116,7 +1098,6 @@ async def mock_props(path): assert result["samenet"] == 80 - class TestSavedNetworkCache: def test_invalidate_marks_dirty(self, qapp): w = _bare_worker(qapp) @@ -1211,7 +1192,6 @@ async def test_get_connection_path_not_found(self, qapp): assert await w._get_connection_path("nope") is None - class TestLoadSavedNetworks: @pytest.mark.asyncio async def test_happy_path_emits_list(self, qapp): @@ -1247,7 +1227,6 @@ async def test_exception_emits_error_and_empty(self, qapp): assert received == [[]] - class TestGetSavedNetworksImpl: @pytest.mark.asyncio async def test_no_bus_returns_empty(self, qapp): @@ -1256,7 +1235,6 @@ async def test_no_bus_returns_empty(self, qapp): assert await w._get_saved_networks_impl() == [] - class TestBuildConnectionProperties: def _call(self, qapp, flags=0, wpa_flags=0, rsn_flags=0, password="pass123"): w = _make_worker(qapp) @@ -1302,7 +1280,6 @@ def test_connection_has_correct_base_structure(self, qapp): assert result["802-11-wireless"]["mode"] == ("s", "infrastructure") - class TestConnectNetwork: @pytest.mark.asyncio async def test_connect_success_emits_result_and_state(self, qapp): @@ -1333,7 +1310,6 @@ async def test_connect_exception_emits_failure(self, qapp): assert results[0].error_code == "connect_failed" - class TestConnectNetworkImpl: @pytest.mark.asyncio async def test_no_bus_returns_error(self, qapp): @@ -1357,7 +1333,6 @@ async def test_not_saved_returns_not_found(self, qapp): assert result.error_code == "not_found" - class TestFindConnectionPathDirect: @pytest.mark.asyncio async def test_found(self, qapp): @@ -1413,7 +1388,6 @@ async def test_exception_returns_none(self, qapp): assert await w._find_connection_path_direct("net") is None - class TestDisconnect: @pytest.mark.asyncio async def test_disconnect_emits_success(self, qapp): @@ -1447,7 +1421,6 @@ async def test_disconnect_emits_failure_on_error(self, qapp): assert "Not active" in received[0].message - class TestDeleteNetwork: @pytest.mark.asyncio async def test_delete_success_emits_result_and_state(self, qapp): @@ -1475,7 +1448,6 @@ async def test_delete_exception_emits_failure(self, qapp): assert results[0].error_code == "delete_failed" - class TestDeleteNetworkImpl: @pytest.mark.asyncio async def test_not_found(self, qapp): @@ -1509,7 +1481,6 @@ async def test_disconnects_if_active(self, qapp): wifi_proxy.disconnect.assert_called_once() - class TestUpdateNetwork: @pytest.mark.asyncio async def test_update_emits_result(self, qapp): @@ -1533,7 +1504,6 @@ async def test_update_exception_emits_failure(self, qapp): assert results[0].error_code == "update_failed" - class TestUpdateNetworkImpl: @pytest.mark.asyncio async def test_not_found(self, qapp): @@ -1629,7 +1599,6 @@ async def test_update_exception_returns_failure(self, qapp): assert result.error_code == "update_failed" - class TestEnsureDbusConnection: @pytest.mark.asyncio async def test_not_running_returns_false(self, qapp): @@ -1654,7 +1623,6 @@ async def test_error_increments_counter(self, qapp): assert w._consecutive_dbus_errors == 1 - class TestShutdown: @pytest.mark.asyncio async def test_shutdown_sets_not_running(self, qapp): @@ -1665,7 +1633,6 @@ async def test_shutdown_sets_not_running(self, qapp): assert w._primary_wired_path == "" - class TestDisconnectEthernet: @pytest.mark.asyncio async def test_no_wired_device_is_noop(self, qapp): @@ -1692,7 +1659,6 @@ async def test_exception_logged_not_raised(self, qapp): await w._async_disconnect_ethernet() # should not raise - class TestAddNetwork: @pytest.mark.asyncio async def test_add_emits_result_and_invalidates(self, qapp): @@ -1716,7 +1682,6 @@ async def test_add_exception_emits_failure(self, qapp): assert received[0].error_code == "add_failed" - class TestAddNetworkImpl: @pytest.mark.asyncio async def test_no_wifi_returns_no_interface(self, qapp): @@ -1765,7 +1730,6 @@ async def test_unsupported_security_returns_error(self, qapp): assert result.error_code == "unsupported_security" - class TestDeleteConnectionsById: @pytest.mark.asyncio async def test_deletes_matching(self, qapp): @@ -1827,7 +1791,6 @@ async def test_case_insensitive_match(self, qapp): assert count == 1 - class TestEnterpriseNetworkHandling: def test_eap_detected_via_rsn_flags(self): result = NetworkManagerWorker._determine_security_type( @@ -1878,7 +1841,6 @@ def test_wep_connection_returns_none(self, qapp): assert result is None - class TestIpToNmUint32: def test_loopback(self): assert NetworkManagerWorker._ip_to_nm_uint32("127.0.0.1") > 0 @@ -1905,7 +1867,6 @@ def test_invalid_raises(self): NetworkManagerWorker._mask_to_prefix("33") - class TestAsyncShutdown: def test_sets_not_running(self, qapp): w = _make(qapp) @@ -1952,7 +1913,6 @@ def test_clears_paths_and_caches(self, qapp): assert w._saved_cache == [] - class TestEnsureSignalProxies: def test_creates_nm_proxy_when_none(self, qapp): w = _make(qapp) @@ -1987,7 +1947,6 @@ def test_idempotent_when_already_set(self, qapp): assert w._signal_settings is sentinel_settings - class TestDebounceState: def test_schedule_creates_handle(self, qapp): w = _make(qapp) @@ -2051,7 +2010,6 @@ def test_fire_scan_rebuild_when_running(self, qapp): loop.create_task.assert_called_once() - class TestFallbackPoll: def test_not_running_returns_immediately(self, qapp): w = _make(qapp, running=False) @@ -2070,7 +2028,6 @@ def test_calls_state_and_connectivity(self, qapp): w._async_load_saved_networks.assert_awaited_once() - class TestEnforceBootMutualExclusion: def test_no_ethernet_returns_early(self, qapp): w = _make(qapp) @@ -2111,7 +2068,6 @@ def test_exception_is_non_fatal(self, qapp): _run(w._enforce_boot_mutual_exclusion()) # must not raise - class TestWaitForWifiRadio: def test_returns_true_when_already_matching(self, qapp): w = _make(qapp) @@ -2128,7 +2084,6 @@ def test_returns_false_on_timeout(self, qapp): assert result is False - class TestSetWifiEnabled: def test_disable_wifi_happy_path(self, qapp): w = _make(qapp) @@ -2193,7 +2148,6 @@ def test_exception_emits_error(self, qapp): assert errors[0][0] == "set_wifi_enabled" - class TestDisconnectEthernetAsync: def test_no_wired_path_noop(self, qapp): w = _make(qapp, wired=False) @@ -2254,7 +2208,6 @@ def test_exception_emits_error_and_state(self, qapp): assert "connect_ethernet" in errors - class TestToggleHotspotOff: def test_disable_cleans_up_profiles(self, qapp): w = _make(qapp) @@ -2316,7 +2269,6 @@ def test_enable_delegates_to_create_and_activate(self, qapp): ) - class TestCreateAndActivateHotspot: def test_happy_path(self, qapp): w = _make(qapp) @@ -2370,7 +2322,6 @@ def test_exception_emits_failure(self, qapp): assert w._is_hotspot_active is False - class TestHotspotActivation: def test_activate_connection_passes_wifi_device_path(self, qapp): """activate_connection must receive the wifi device path, not be called bare.""" @@ -2425,7 +2376,6 @@ def test_interface_name_uses_detected_iface(self, qapp): assert iface_name == "wlan1" - class TestUpdateHotspotConfig: def test_inactive_updates_config_without_reactivating(self, qapp): w = _make(qapp) @@ -2487,7 +2437,6 @@ def test_exception_emits_failure(self, qapp): assert results[0].success is False - class TestUpdateWifiStaticIp: def test_not_found_emits_error(self, qapp): w = _make(qapp) @@ -2568,7 +2517,6 @@ def test_happy_path(self, qapp): assert results[0].success is True - class TestCreateVlan: def test_no_wired_device_emits_error(self, qapp): w = _make(qapp, wired=False) @@ -2620,7 +2568,6 @@ def test_exception_emits_error(self, qapp): assert "delete_vlan" in errors - class TestStartSignalListeners: def test_creates_seven_listener_tasks(self, qapp): w = _make(qapp) @@ -2649,7 +2596,6 @@ async def _test(): loop.close() - class TestAsyncInitializeFull: def test_happy_path_full_init(self, qapp): w = _make(qapp, running=False) @@ -2696,7 +2642,6 @@ def test_exception_emits_error_and_still_fires_initialized(self, qapp): assert len(inits) == 1 - class TestExceptionHandlerBranches: """Exercise exception and early-return branches in low-level scan helpers.""" @@ -2775,7 +2720,6 @@ def test_scan_ap_exception_skips_ap(self, qapp): assert scanned == [] - class TestConnectNetworkImplException: """_connect_network_impl returns a failure result when activate_connection raises.""" @@ -2791,7 +2735,6 @@ def test_activate_connection_exception_returns_failure(self, qapp): assert result.error_code == "connect_failed" - class TestWaitForConnection: """_wait_for_connection covers timeout and consecutive-empty early-exit.""" @@ -2816,7 +2759,6 @@ def test_wait_for_connection_inner_exception_continues(self, qapp): assert result is False - class TestGetSavedNetworksHandlesMalformedEntry: """_get_saved_networks_impl skips entries that raise during parse.""" diff --git a/tests/scripts/test_sudoers_rules.py b/tests/scripts/test_sudoers_rules.py new file mode 100644 index 00000000..5f765506 --- /dev/null +++ b/tests/scripts/test_sudoers_rules.py @@ -0,0 +1,102 @@ +"""Guard: apt privilege now flows only through the root-owned bs-apt-helper. + +sudo matches the FULL argv, so the daemon's apt argvs and the sudoers rules +install-updater.sh emits must agree. The wrapper is the single grant; its own +argument validation is what makes the trailing '*' safe. +""" + +from __future__ import annotations + +import fnmatch +import re +import subprocess +from pathlib import Path + +import pytest + +import updater.executor as executor +from updater.executor import _apt_cmd + +_SCRIPTS = Path(__file__).resolve().parents[2] / "scripts" +_INSTALLER = _SCRIPTS / "install-updater.sh" +_HELPER = _SCRIPTS / "bs-apt-helper.sh" + +_WRAPPER_RULE = "/usr/local/sbin/bs-apt-helper *" + + +def _sudoers_rules() -> list[str]: + """Extract every literal NOPASSWD rule body emitted by install-updater.sh.""" + return re.findall( + r"printf 'blocks ALL=\(ALL\) NOPASSWD: ([^']+?)(?:\\n)?'", + _INSTALLER.read_text(), + ) + + +def _daemon_apt_argvs() -> list[list[str]]: + """The sudo apt argvs the daemon builds (mirrors executor call sites).""" + return [ + _apt_cmd("update"), + _apt_cmd("upgrade", ["vim", "curl"]), + _apt_cmd("autoremove"), + _apt_cmd("fix-broken"), + _apt_cmd("dselect-upgrade"), + ] + + +class TestSudoersRules: + def test_wrapper_rule_emitted(self): + assert _WRAPPER_RULE in _sudoers_rules() + + def test_deployed_path_pins_all_three_sides(self): + # executor constant == sudoers rule command == installer destination. + rule_path = _WRAPPER_RULE.split()[0] + assert str(executor.APT_HELPER) == rule_path + assert f"mv -Tf /usr/local/sbin/.bs-apt-helper.new {rule_path}" in ( + _INSTALLER.read_text() + ) + + def test_no_direct_apt_rules_remain(self): + for rule in _sudoers_rules(): + assert "apt-get" not in rule and "/usr/bin/dpkg" not in rule, ( + f"direct apt/dpkg rule bypasses the wrapper: {rule!r}" + ) + + def test_wrapper_argvs_match_rule(self): + for argv in _daemon_apt_argvs(): + cmd = " ".join(argv[1:]) + assert fnmatch.fnmatch(cmd, _WRAPPER_RULE), f"unmatched argv: {cmd!r}" + + +class TestWrapperValidation: + """The wrapper must reject anything but its fixed verbs and clean names. + + All cases exit before reaching apt, so they run unprivileged. + """ + + def _run(self, *args: str) -> subprocess.CompletedProcess: + return subprocess.run( + ["/bin/bash", str(_HELPER), *args], capture_output=True, timeout=10 + ) + + @pytest.mark.parametrize( + "args", + [ + [], + ["badverb"], + ["upgrade"], # needs at least one package + ["upgrade", "-o"], # option injection + ["upgrade", "--reinstall"], + ["upgrade", "vim;rm"], # shell metachars + ["upgrade", "vim", "-oAPT::Update::Pre-Invoke::=x"], + ["update", "extra"], # fixed verbs take no args + ["autoremove", "extra"], + ["set-selections", "extra"], + ], + ) + def test_rejects(self, args): + assert self._run(*args).returncode == 2 + + def test_valid_package_names_pass_validation(self): + # Passes validation, then fails at apt (no root): anything but exit 2. + res = self._run("upgrade", "libstdc++6", "python3.11-dev") + assert res.returncode != 2 diff --git a/tests/updater/conftest.py b/tests/updater/conftest.py index d35d9381..610c887a 100644 --- a/tests/updater/conftest.py +++ b/tests/updater/conftest.py @@ -67,4 +67,5 @@ def svc(): s._status_pending = False s.busy_changed = MagicMock() s.status_ready = MagicMock() + s.error = MagicMock() return s diff --git a/tests/updater/test_dbus_service_unit.py b/tests/updater/test_dbus_service_unit.py index ec8a97e2..253a39c4 100644 --- a/tests/updater/test_dbus_service_unit.py +++ b/tests/updater/test_dbus_service_unit.py @@ -1,6 +1,6 @@ import asyncio import json -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -107,7 +107,7 @@ async def test_update_component_rolls_back_on_restart_failure(self): from updater.service import UpdateService from updater.models import ComponentConfig from pathlib import Path - from unittest.mock import AsyncMock, patch + from unittest.mock import AsyncMock cfg = ComponentConfig( name="klipper", @@ -366,3 +366,32 @@ async def test_update_all_includes_errored_git_repo(self, svc): called_with = svc._svc.update_all.call_args[0][0] assert "RF50-Klipper" in called_with assert "klipper" not in called_with # clean repo not updated + + +class TestLockHeldSurfacesError: + def _held_lock(self): + lock = MagicMock() + lock.return_value.__enter__.return_value = False + return lock + + @pytest.mark.asyncio + async def test_update_all_lock_held_emits_error(self, svc): + with patch("updater.dbus_service.process_lock", self._held_lock()): + await svc._run_update_all() + svc.error.emit.assert_called_once_with(("updater", "another update is running")) + svc._svc.update_all.assert_not_called() + assert svc._busy is False + + @pytest.mark.asyncio + async def test_update_component_lock_held_emits_error(self, svc): + with patch("updater.dbus_service.process_lock", self._held_lock()): + await svc._run_update_component("klipper") + svc.error.emit.assert_called_once_with(("klipper", "another update is running")) + svc._svc.update_component.assert_not_called() + + @pytest.mark.asyncio + async def test_recover_lock_held_emits_error(self, svc): + with patch("updater.dbus_service.process_lock", self._held_lock()): + await svc._run_recover("klipper", hard=False) + svc.error.emit.assert_called_once_with(("klipper", "another update is running")) + svc._svc.recover.assert_not_called() diff --git a/tests/updater/test_executor_unit.py b/tests/updater/test_executor_unit.py index 0e743ab9..c7ddfd00 100644 --- a/tests/updater/test_executor_unit.py +++ b/tests/updater/test_executor_unit.py @@ -631,6 +631,8 @@ async def test_failure(self): @pytest.mark.asyncio async def test_unattended_hardening_options_present(self): + # The hardening options moved into the root-owned wrapper: assert the + # argv routes through it and the wrapper still carries the options. proc = _make_proc(0, b"", b"") exec_mock = AsyncMock(return_value=proc) with ( @@ -642,11 +644,15 @@ async def test_unattended_hardening_options_present(self): ): await apt_upgrade() argv = [str(a) for a in exec_mock.call_args[0]] - assert "DPkg::Lock::Timeout=60" in argv # wait for the lock, never fail fast - assert "Dpkg::Options::=--force-confold" in argv # no conffile prompt - env = exec_mock.call_args[1]["env"] - assert env["DEBIAN_FRONTEND"] == "noninteractive" - assert env["NEEDRESTART_MODE"] == "a" + assert argv[1] == "/usr/local/sbin/bs-apt-helper" + assert argv[2:] == ["upgrade", "pkg1"] + wrapper = ( + Path(__file__).resolve().parents[2] / "scripts" / "bs-apt-helper.sh" + ).read_text() + assert "DPkg::Lock::Timeout=60" in wrapper # wait for the lock + assert "--force-confold" in wrapper # no conffile prompt + assert "DEBIAN_FRONTEND=noninteractive" in wrapper + assert "NEEDRESTART_MODE=a" in wrapper @pytest.mark.asyncio async def test_list_failure(self): diff --git a/tests/updater/test_service_unit.py b/tests/updater/test_service_unit.py index 1dc5173a..9a95abbb 100644 --- a/tests/updater/test_service_unit.py +++ b/tests/updater/test_service_unit.py @@ -548,7 +548,7 @@ class TestUpdateAll: def _git(self, tmp_path: Path, name: str, order: int) -> ComponentConfig: path = tmp_path / name - path.mkdir() + (path / ".git").mkdir(parents=True) return ComponentConfig(name=name, kind="git", path=path, order=order) @pytest.mark.asyncio @@ -1456,7 +1456,7 @@ class TestPreflightFetch: def _git(self, tmp_path: Path, name: str) -> ComponentConfig: path = tmp_path / name - path.mkdir() + (path / ".git").mkdir(parents=True) return ComponentConfig(name=name, kind="git", path=path) @pytest.mark.asyncio @@ -1699,3 +1699,288 @@ async def test_revert_rejects_invalid_prev_hash(self, tmp_path): await svc._revert_inflight() mock_reset.assert_not_called() assert svc._read_inflight() == {} # marker still cleared + + +class TestPingWhile: + @pytest.mark.asyncio + async def test_returns_result(self): + svc = UpdateService(callback=MagicMock()) + + async def fast(): + return (True, "ok") + + assert await svc._ping_while(fast(), "x", 3, 4) == (True, "ok") + + @pytest.mark.asyncio + async def test_emits_on_step_while_waiting(self): + cb = MagicMock() + svc = UpdateService(callback=cb) + + async def slow(): + await asyncio.sleep(0.05) + return "done" + + assert await svc._ping_while(slow(), "klipper", 3, 4, interval=0.01) == "done" + cb.on_step.assert_called_with("klipper", 3, 4) + + @pytest.mark.asyncio + async def test_cancellation_reaches_inner_task(self): + svc = UpdateService(callback=MagicMock()) + started = asyncio.Event() + cancelled = asyncio.Event() + + async def hang(): + started.set() + try: + await asyncio.sleep(30) + except asyncio.CancelledError: + cancelled.set() + raise + + task = asyncio.create_task(svc._ping_while(hang(), "x", 3, 4, interval=0.01)) + await started.wait() + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + assert cancelled.is_set() + + +class TestHookTimeoutBudget: + @pytest.mark.asyncio + async def test_single_update_passes_hook_timeout(self, tmp_path): + cb = MagicMock() + with ( + patch("pathlib.Path.exists", return_value=True), + patch("updater.service.git_get_hash", return_value="a" * 40), + patch("updater.service.git_fetch", return_value=(True, "")), + patch("updater.service.git_pull", return_value=(True, "")), + patch("updater.service.git_reset_to_hash", return_value=(True, "")), + patch("updater.service.git_get_current_branch", return_value="master"), + patch( + "updater.service.UpdateService._install_dependencies", + return_value=(True, ""), + ), + patch("updater.service.run_hook", return_value=(True, "")) as mock_hook, + patch("updater.service.restart_service", return_value=(True, "")), + patch("updater.service.restart_service_noblock", return_value=(True, "")), + patch("updater.service.wait_for_service_active", return_value=True), + patch( + "updater.service._assert_https_remote", + return_value=(True, "https://github.com/test/repo"), + ), + ): + svc = UpdateService(callback=cb) + svc._state_path = tmp_path / "state.json" + svc._inflight_path = tmp_path / "inflight.json" + svc._history_path = tmp_path / "history.jsonl" + assert await svc.update_component("klipper") is True + assert mock_hook.await_args.kwargs["timeout"] == updater_service.HOOK_TIMEOUT + + @pytest.mark.asyncio + async def test_batch_passes_hook_timeout(self, tmp_path): + cb = MagicMock() + comp = ComponentConfig(name="klipper", kind="git", path=tmp_path) + with ( + patch("updater.service.git_get_hash", return_value="a" * 40), + patch( + "updater.service.UpdateService._stage_component", + return_value=(True, ""), + ), + patch( + "updater.service.UpdateService._install_dependencies", + return_value=(True, ""), + ), + patch("updater.service.run_hook", return_value=(True, "")) as mock_hook, + patch( + "updater.service._assert_https_remote", + return_value=(True, "https://github.com/test/repo"), + ), + ): + svc = UpdateService(callback=cb) + svc._state_path = tmp_path / "state.json" + svc._inflight_path = tmp_path / "inflight.json" + svc._history_path = tmp_path / "history.jsonl" + await svc._run_git_batch([comp]) + assert mock_hook.await_args.kwargs["timeout"] == updater_service.HOOK_TIMEOUT + cb.on_component_done.assert_called_once_with("klipper", True) + + +class TestInflightClearedOnEarlyReturn: + def _svc(self, tmp_path, cb): + svc = UpdateService(callback=cb) + svc._state_path = tmp_path / "state.json" + svc._inflight_path = tmp_path / "inflight.json" + svc._history_path = tmp_path / "history.jsonl" + return svc + + @pytest.mark.asyncio + async def test_single_insecure_remote_clears_marker(self, tmp_path): + cb = MagicMock() + with ( + patch("pathlib.Path.exists", return_value=True), + patch("updater.service.git_get_hash", return_value="a" * 40), + patch( + "updater.service._assert_https_remote", + return_value=(False, "origin remote uses non-https URL"), + ), + ): + svc = self._svc(tmp_path, cb) + assert await svc.update_component("klipper") is False + assert not (tmp_path / "inflight.json").exists() + cb.on_error.assert_called_once_with("klipper", "insecure remote") + + @pytest.mark.asyncio + async def test_single_reset_failure_clears_marker(self, tmp_path): + cb = MagicMock() + with ( + patch("pathlib.Path.exists", return_value=True), + patch("updater.service.git_get_hash", return_value="a" * 40), + patch("updater.service.git_fetch", return_value=(True, "")), + patch("updater.service.git_get_current_branch", return_value="master"), + patch("updater.service.git_reset_to_hash", return_value=(False, "boom")), + patch( + "updater.service._assert_https_remote", + return_value=(True, "https://github.com/test/repo"), + ), + ): + svc = self._svc(tmp_path, cb) + assert await svc.update_component("klipper") is False + assert not (tmp_path / "inflight.json").exists() + cb.on_error.assert_called_once_with("klipper", "reset") + + @pytest.mark.asyncio + async def test_batch_insecure_remote_clears_marker(self, tmp_path): + cb = MagicMock() + comp = ComponentConfig(name="klipper", kind="git", path=tmp_path) + with ( + patch("updater.service.git_get_hash", return_value="a" * 40), + patch( + "updater.service._assert_https_remote", + return_value=(False, "origin remote uses non-https URL"), + ), + ): + svc = self._svc(tmp_path, cb) + await svc._run_git_batch([comp]) + assert not (tmp_path / "inflight.json").exists() + cb.on_error.assert_called_once_with("klipper", "insecure remote") + + +class TestNonRepoComponent: + """A dir without .git (pre-updater tarball install) must not poison updates.""" + + def _comps(self, tmp_path): + repo = tmp_path / "klipper" + (repo / ".git").mkdir(parents=True) + nonrepo = tmp_path / "Spoolman" + nonrepo.mkdir() + return ( + ComponentConfig(name="klipper", kind="git", path=repo), + ComponentConfig(name="Spoolman", kind="git", path=nonrepo), + ) + + @pytest.mark.asyncio + async def test_update_all_excludes_nonrepo_and_continues(self, tmp_path): + cb = MagicMock() + good, bad = self._comps(tmp_path) + with ( + patch("updater.service.git_fetch", return_value=(True, "")), + patch( + "updater.service.UpdateService._run_git_batch", new_callable=AsyncMock + ) as mock_batch, + ): + svc = UpdateService(callback=cb) + svc._components = [good, bad] + await svc.update_all() + mock_batch.assert_awaited_once_with([good]) + cb.on_error.assert_called_once_with( + "Spoolman", "not a git repository - reinstall required" + ) + cb.on_component_done.assert_called_once_with("Spoolman", False) + + @pytest.mark.asyncio + async def test_preflight_skips_nonrepo_fetch(self, tmp_path): + good, bad = self._comps(tmp_path) + fetched: list[str] = [] + + async def fake_fetch(path, **kw): + fetched.append(path.name) + return (True, "") + + with patch("updater.service.git_fetch", side_effect=fake_fetch): + svc = UpdateService(callback=MagicMock()) + assert await svc._preflight_fetch([good, bad], [good, bad]) is True + assert fetched == ["klipper"] + + @pytest.mark.asyncio + async def test_single_update_nonrepo_errors_cleanly(self, tmp_path): + cb = MagicMock() + _, bad = self._comps(tmp_path) + svc = UpdateService(callback=cb) + svc._components = [bad] + assert await svc.update_component("Spoolman") is False + cb.on_error.assert_called_once_with( + "Spoolman", "not a git repository - reinstall required" + ) + + def _installable(self, tmp_path) -> ComponentConfig: + nonrepo = tmp_path / "Spoolman" + nonrepo.mkdir() + (nonrepo / ".env").write_text("keep me") + return ComponentConfig( + name="Spoolman", + kind="git", + path=nonrepo, + url="https://github.com/Donkie/Spoolman", + install_if_missing=True, + ) + + @pytest.mark.asyncio + async def test_update_all_quarantines_installable_and_provisions(self, tmp_path): + cb = MagicMock() + comp = self._installable(tmp_path) + with ( + patch("updater.service.git_fetch", return_value=(True, "")), + patch( + "updater.service.UpdateService._provision_component", + new_callable=AsyncMock, + ) as mock_prov, + ): + svc = UpdateService(callback=cb) + svc._history_path = tmp_path / "history.jsonl" + svc._components = [comp] + await svc.update_all() + mock_prov.assert_awaited_once_with(comp) + cb.on_error.assert_not_called() + assert not comp.path.exists() # moved aside + moved = list(tmp_path.glob("Spoolman.pre-updater-*")) + assert len(moved) == 1 + assert (moved[0] / ".env").read_text() == "keep me" # old dir intact + + @pytest.mark.asyncio + async def test_single_update_quarantines_installable_and_provisions(self, tmp_path): + cb = MagicMock() + comp = self._installable(tmp_path) + with patch( + "updater.service.UpdateService._provision_component", + new_callable=AsyncMock, + return_value=True, + ) as mock_prov: + svc = UpdateService(callback=cb) + svc._history_path = tmp_path / "history.jsonl" + svc._components = [comp] + assert await svc.update_component("Spoolman") is True + mock_prov.assert_awaited_once_with(comp) + assert list(tmp_path.glob("Spoolman.pre-updater-*")) + + @pytest.mark.asyncio + async def test_quarantine_failure_falls_back_to_error(self, tmp_path): + cb = MagicMock() + comp = self._installable(tmp_path) + with patch("updater.service.os.rename", side_effect=OSError("busy")): + svc = UpdateService(callback=cb) + svc._components = [comp] + assert await svc.update_component("Spoolman") is False + cb.on_error.assert_called_once_with( + "Spoolman", "not a git repository - reinstall required" + ) + assert comp.path.exists() # nothing moved diff --git a/tests/util/test_keyboard_page_unit.py b/tests/util/test_keyboard_page_unit.py index 00490070..3ca25291 100644 --- a/tests/util/test_keyboard_page_unit.py +++ b/tests/util/test_keyboard_page_unit.py @@ -31,8 +31,13 @@ sys.modules.pop(_key, None) from BlocksScreen.lib.panels.widgets.keyboardPage import ( # noqa: E402 - _LOWERCASE, _NUM_KEYS, _NUMBERS, _SYMBOLS, _UPPERCASE, - CustomQwertyKeyboard) + _LOWERCASE, + _NUM_KEYS, + _NUMBERS, + _SYMBOLS, + _UPPERCASE, + CustomQwertyKeyboard, +) @pytest.fixture() @@ -218,6 +223,4 @@ def test_delete_button_click(self, keyboard, qtbot): def test_back_button_emits_signal(self, keyboard, qtbot): with qtbot.waitSignal(keyboard.request_back, timeout=1000): - qtbot.mouseClick( - keyboard.numpad_back_btn, QtCore.Qt.MouseButton.LeftButton - ) + qtbot.mouseClick(keyboard.numpad_back_btn, QtCore.Qt.MouseButton.LeftButton) diff --git a/tests/util/test_list_model_unit.py b/tests/util/test_list_model_unit.py index 7bd330de..f3f082a9 100644 --- a/tests/util/test_list_model_unit.py +++ b/tests/util/test_list_model_unit.py @@ -1,7 +1,6 @@ """Unit tests for EntryListModel.reconcile() — locks behaviour before refactoring.""" import pytest -from PyQt6 import QtWidgets from BlocksScreen.lib.utils.list_model import EntryListModel, ListItem diff --git a/tests/widgets/test_update_page_unit.py b/tests/widgets/test_update_page_unit.py index 56fe5ac5..c2aa4ef7 100644 --- a/tests/widgets/test_update_page_unit.py +++ b/tests/widgets/test_update_page_unit.py @@ -485,3 +485,24 @@ def test_error_toast_shows_failure_state(self, page): page.handle_error_occurred("firmware", "checksum mismatch") msg = page._show_toast.call_args[0][0] assert "failed" in msg.lower() + + +class TestBadStatusPayload: + def test_bad_payload_keeps_statuses_and_toasts(self, page): + page._statuses = {"klipper": ComponentStatus(name="klipper")} + page._show_toast = MagicMock() + page.handle_status_ready("{not json") + assert "klipper" in page._statuses + page._show_toast.assert_called_once() + + +class TestConfirmPopupCleanup: + def test_second_confirm_deletes_previous_popup(self, page): + with patch("BlocksScreen.lib.panels.widgets.updatePage.BasePopup") as popup_cls: + first = MagicMock() + second = MagicMock() + popup_cls.side_effect = [first, second] + page._show_update_confirm() + page._show_update_confirm() + first.deleteLater.assert_called_once() + second.deleteLater.assert_not_called() diff --git a/updater/__main__.py b/updater/__main__.py index 1271300e..db5dbeb9 100644 --- a/updater/__main__.py +++ b/updater/__main__.py @@ -10,8 +10,7 @@ from updater.locking import process_lock from updater.service import LoggingCallback, UpdateService -# NOTE: sdbus and updater.dbus_service are imported lazily inside _run_daemon so -# the status/update/recover CLI works on an interpreter without sdbus installed. +# NOTE: sdbus imports are lazy (in _run_daemon) so the CLI works without sdbus. def _sd_notify(msg: str) -> None: diff --git a/updater/dbus_service.py b/updater/dbus_service.py index f76d99fa..c5a8db13 100644 --- a/updater/dbus_service.py +++ b/updater/dbus_service.py @@ -226,6 +226,8 @@ async def _run_update_all(self) -> None: with process_lock() as acquired: if not acquired: _log.warning("update_all: a CLI run holds the lock; skipping") + # Surface the rejection so the UI toasts instead of going silent. + self.error.emit(("updater", "another update is running")) return ran = True await self._update_all_locked() @@ -233,8 +235,7 @@ async def _run_update_all(self) -> None: _log.error("_run_update_all failed: %s", exc, exc_info=True) finally: self._set_busy(busy=False) - # Only run the silent apt pass if we actually held the lock - otherwise a - # CLI run owns the update and is responsible for apt. + # Silent apt pass only if we held the lock; else the CLI run owns apt. if ran: self._spawn( self._svc.background_apt_upgrade(), name="background_apt_upgrade" @@ -249,8 +250,7 @@ async def _update_all_locked(self) -> None: or s.packages_upgradable > 0 or s.has_local_changes or s.needs_install - # Errored git repos (e.g. a corrupt repo) are included so the one - # "Update" button reaches them; the update flow self-heals them. + # Errored git repos included: the update flow self-heals them. or (s.error is not None and s.kind != "apt") } if dirty: @@ -263,6 +263,7 @@ async def _run_update_component(self, name: str) -> None: with process_lock() as acquired: if not acquired: _log.warning("update_component: a CLI run holds the lock; skipping") + self.error.emit((name, "another update is running")) return await self._svc.update_component(name) except Exception as exc: # noqa: BLE001 @@ -275,6 +276,7 @@ async def _run_recover(self, name: str, hard: bool) -> None: # noqa: FBT001 with process_lock() as acquired: if not acquired: _log.warning("recover: a CLI run holds the lock; skipping") + self.error.emit((name, "another update is running")) return await self._svc.recover(name, hard) except Exception as exc: # noqa: BLE001 @@ -304,9 +306,7 @@ async def cancel(self) -> None: cancelled_tasks.append(task) _log.info("cancelled task %r", name) if cancelled_tasks: - # asyncio.wait (unlike a cancelled gather) never re-cancels the - # tasks, so an in-flight rollback is not interrupted a second time. - # Budget covers git reset + service restart + active-wait. + # asyncio.wait never re-cancels: rollback isn't interrupted again. _done, pending = await asyncio.wait(cancelled_tasks, timeout=150.0) if pending: _log.error( diff --git a/updater/executor.py b/updater/executor.py index 747e126a..b04f6971 100644 --- a/updater/executor.py +++ b/updater/executor.py @@ -20,24 +20,18 @@ UPDATER_SERVICE = "BlocksScreen-updater.service" +# Hook budget: a dependency-heavy hook can run minutes; a timeout aborts the batch. +HOOK_TIMEOUT = 600.0 + GIT = "/usr/bin/git" PIP = "/usr/bin/pip3" APT = "/usr/bin/apt" -APT_GET = "/usr/bin/apt-get" APT_MARK = "/usr/bin/apt-mark" SUDO = "/usr/bin/sudo" SYSTEMCTL = "/usr/bin/systemctl" - -# Wait up to 60s for the dpkg lock instead of failing immediately when apt-daily -# or unattended-upgrades is mid-run (the daemon now owns apt on this image). -_APT_LOCK_OPTS = ["-o", "DPkg::Lock::Timeout=60"] -# Keep existing conffiles without prompting (an upgrade must never block on input). -_APT_CONF_OPTS = [ - "-o", - "Dpkg::Options::=--force-confdef", - "-o", - "Dpkg::Options::=--force-confold", -] +DPKG = "/usr/bin/dpkg" +# Root-owned fixed-argv apt wrapper (owns the -o opts); installed pre-restart. +APT_HELPER = Path("/usr/local/sbin/bs-apt-helper") _SERVICE_RE = re.compile(r"^[a-zA-Z0-9@:._-]+\.service$") _GIT_SHA_RE = re.compile(r"^[a-f0-9]{7,40}$") @@ -63,8 +57,7 @@ def _make_clean_env() -> dict[str, str]: "PATH", "HOME", "USER", - # SEC: DBUS_SESSION_BUS_ADDRESS and XDG_RUNTIME_DIR intentionally excluded - - # hook scripts must not make D-Bus calls or access the session runtime dir. + # SEC: session bus + XDG runtime vars excluded; hooks must not use them. "TMPDIR", ): val = os.environ.get(key) @@ -78,8 +71,7 @@ def _make_clean_env() -> dict[str, str]: for key, val in os.environ.items(): if key in safe_sudo: env[key] = val - # Lets hooks (git post-merge, the updater's own) tell they run mid-batch and - # defer any daemon restart to the sentinel instead of aborting the update. + # Tells hooks they run mid-batch: defer daemon restarts to the sentinel. env["BS_UPDATER_SELF_UPDATE"] = "1" with contextlib.suppress(OSError): env["BS_UPDATER_RESTART_SENTINEL"] = str(restart_sentinel_path()) @@ -288,6 +280,11 @@ async def check_git_status( ) +def is_git_repo(path: Path | None) -> bool: + """True if path exists and has a .git entry (a non-repo dir can never fetch).""" + return path is not None and (path / ".git").exists() + + async def git_is_dirty(path: Path) -> bool: ok, output = await _run( [GIT, "status", "--porcelain", "--untracked-files=no"], @@ -394,18 +391,22 @@ async def git_clone( async def git_reset_to_hash(path: Path | None, prev_hash: str = "") -> tuple[bool, str]: - """Hard-reset repo at path directly to prev_hash without fetching.""" + """Hard-reset repo at path directly to prev_hash without fetching. + + This is the rollback/heal primitive (abort, boot revert, recover), so the + timeout is generous: a reset across a large delta on a slow SD card must + not be SIGTERM'd mid-checkout in exactly the path meant to fix things. + """ if not path: return (False, "path error") if prev_hash == "": return (False, "prev_hash does not exist") if not _validate_git_ref(prev_hash): return (False, f"invalid git ref: {prev_hash!r}") - return await _run([GIT, "reset", "--hard", prev_hash], cwd=path, timeout=10.0) + return await _run([GIT, "reset", "--hard", prev_hash], cwd=path, timeout=60.0) -# Object-corruption signatures. Kept narrow so generic failures like -# "fatal: not a git repository" are NOT treated as corruption. +# Narrow corruption signatures: "not a git repository" etc. must not match. _GIT_CORRUPT_SIGNATURES = ( "corrupt", "is empty", @@ -416,11 +417,9 @@ async def git_reset_to_hash(path: Path | None, prev_hash: str = "") -> tuple[boo "missing commit", ) -# Quarantine directory for loose objects fsck reports as corrupt (kept inside -# .git/objects so it never shows up as an untracked working-tree file). +# Quarantine dir inside .git/objects so it never appears as untracked. _QUARANTINE_DIRNAME = "objects-corrupt" -# fsck error lines reference a corrupt object either by its on-disk path -# (.git/objects/ab/<38 hex>) or by its 40-hex SHA; match both. +# fsck names corrupt objects by path (.git/objects/ab/<38hex>) or 40-hex SHA. _GIT_OBJ_PATH_RE = re.compile(r"objects/([0-9a-f]{2})/([0-9a-f]{38})") _GIT_OBJ_SHA_RE = re.compile(r"\b([0-9a-f]{40})\b") @@ -524,8 +523,7 @@ async def git_repair(path: Path) -> tuple[bool, str]: return (False, f"fetch after cleanup failed: {out}") if not await git_has_corruption(path): return (True, f"repaired ({removed} empty objects removed)") - # Empty-object pruning + fetch did not clear it: a non-empty loose object is - # corrupt. Quarantine the bad objects and re-fetch to re-download them. + # A non-empty loose object is corrupt: quarantine and re-fetch it. quarantined = await _quarantine_corrupt_objects(path) if quarantined == 0: return (False, "still corrupt after fetch (no quarantinable objects found)") @@ -611,7 +609,8 @@ async def git_checkout(path: Path | None, branch: str) -> tuple[bool, str]: if current_branch == branch: return (True, "already on branch") - return await _run([GIT, "checkout", branch], cwd=path, timeout=10.0) + # Generous: a big checkout on slow SD can pass 10s; SIGTERM = half-written tree. + return await _run([GIT, "checkout", branch], cwd=path, timeout=60.0) async def check_apt_status( @@ -694,13 +693,14 @@ def _apt_env() -> dict[str, str]: return env -def _apt_get(args: list[str], *, keep_conffiles: bool = False) -> list[str]: - """Build a sudo apt-get argv with the dpkg-lock wait always applied. +def _apt_cmd(verb: str, pkgs: Sequence[str] = ()) -> list[str]: + """Build the sudo apt argv via the root-owned wrapper. - keep_conffiles adds --force-confdef/--force-confold for unpacking operations. + The wrapper is the only apt command sudoers grants; install-updater.sh + deploys it before the daemon restarts onto this code, so it is always + present. It owns the lock/conffile -o options and validates its args. """ - opts = [*_APT_LOCK_OPTS, *(_APT_CONF_OPTS if keep_conffiles else [])] - return [SUDO, APT_GET, *opts, *args] + return [SUDO, str(APT_HELPER), verb, *pkgs] async def _apt_snapshot_packages() -> tuple[bool, Path | None]: @@ -715,10 +715,7 @@ async def _apt_snapshot_packages() -> tuple[bool, Path | None]: ) try: snapshot_path.parent.mkdir(parents=True, mode=0o0700, exist_ok=True) - ok, output = await _run( - ["/usr/bin/dpkg", "--get-selections"], - timeout=15.0, - ) + ok, output = await _run([DPKG, "--get-selections"], timeout=15.0) if not ok: logger.warning("dpkg --get-selections failed: %s", output) return False, None @@ -745,11 +742,7 @@ async def _apt_get_fix_broken() -> tuple[bool, str]: Best-effort rollback after failed upgrade. May not fully restore state, but aims to unbreak the system. Called only on apt_upgrade failure. """ - return await _run( - _apt_get(["-f", "install", "-y"], keep_conffiles=True), - timeout=120.0, - env=_apt_env(), - ) + return await _run(_apt_cmd("fix-broken"), timeout=120.0, env=_apt_env()) async def _apt_restore_packages(snapshot_path: Path) -> tuple[bool, str]: @@ -767,8 +760,8 @@ async def _apt_restore_packages(snapshot_path: Path) -> tuple[bool, str]: return False, str(e) proc = await asyncio.create_subprocess_exec( SUDO, - "/usr/bin/dpkg", - "--set-selections", + str(APT_HELPER), + "set-selections", stdin=asyncio.subprocess.PIPE, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, @@ -793,16 +786,12 @@ async def _apt_restore_packages(snapshot_path: Path) -> tuple[bool, str]: msg = stderr.decode(errors="replace") logger.error("dpkg --set-selections failed: %s", msg) return False, msg - return await _run( - _apt_get(["dselect-upgrade", "-y"], keep_conffiles=True), - timeout=120.0, - env=_apt_env(), - ) + return await _run(_apt_cmd("dselect-upgrade"), timeout=120.0, env=_apt_env()) async def apt_update() -> tuple[bool, str]: """Run apt-get update to refresh package lists.""" - return await _run(_apt_get(["update"]), timeout=120.0, env=_apt_env()) + return await _run(_apt_cmd("update"), timeout=120.0, env=_apt_env()) async def apt_upgrade(exclude: tuple[str, ...] = ()) -> tuple[bool, str]: @@ -820,16 +809,12 @@ async def apt_upgrade(exclude: tuple[str, ...] = ()) -> tuple[bool, str]: if not pkgs: return True, "no packages to upgrade" - return await _run( - _apt_get(["install", "--only-upgrade", "-y", *pkgs], keep_conffiles=True), - timeout=300.0, - env=_apt_env(), - ) + return await _run(_apt_cmd("upgrade", pkgs), timeout=300.0, env=_apt_env()) async def apt_autoremove() -> tuple[bool, str]: """Run apt-get autoremove -y to remove orphaned packages after an upgrade.""" - return await _run(_apt_get(["autoremove", "-y"]), timeout=120.0, env=_apt_env()) + return await _run(_apt_cmd("autoremove"), timeout=120.0, env=_apt_env()) async def run_hook( @@ -841,8 +826,9 @@ async def run_hook( ) -> tuple[bool, str]: """Run the per-component update hook if it exists. - timeout is 60s for update hooks; provisioning passes a larger budget since a - first install may sync a full dependency set. + The 60s default suits tests and trivial hooks; the update and provisioning + flows pass HOOK_TIMEOUT since a hook may sync a full dependency set (e.g. + Spoolman's `uv sync` after its deps changed). """ hook = (_HOOKS_DIR / f"{name}.sh").resolve() # SEC: resolve symlinks try: diff --git a/updater/hooks/BlocksScreen.sh b/updater/hooks/BlocksScreen.sh index 3b488ae2..687010a9 100755 --- a/updater/hooks/BlocksScreen.sh +++ b/updater/hooks/BlocksScreen.sh @@ -1,7 +1,5 @@ #!/bin/bash -# Runs after a successful BlocksScreen git update (via updater daemon). -# Uses only sudo-free mechanisms: symlink + dbus-send (polkit) for service -# file changes, flag file + BlocksScreen-deploy.path for install-updater.sh. +# Post-update hook: sudo-free unit sync + deploy-flag mechanics only. set -euo pipefail if [ -z "${COMPONENT_PATH:-}" ] || [ -z "${PREV_HASH:-}" ] || [ -z "${NEW_HASH:-}" ]; then @@ -9,9 +7,7 @@ if [ -z "${COMPONENT_PATH:-}" ] || [ -z "${PREV_HASH:-}" ] || [ -z "${NEW_HASH:- fi _set_deploy_flag() { - # Under the updater (mid-batch), do NOT trigger install-updater now: record - # to the sentinel so the daemon touches the deploy flag once the batch is - # done. Setting the flag here could fire BlocksScreen-deploy.path mid-batch. + # Mid-batch: record to the sentinel; setting the flag now would fire deploy mid-batch. if [ -n "${BS_UPDATER_SELF_UPDATE:-}" ] && [ -n "${BS_UPDATER_RESTART_SENTINEL:-}" ]; then mkdir -p "$(dirname "$BS_UPDATER_RESTART_SENTINEL")" 2>/dev/null || true echo "install" >>"$BS_UPDATER_RESTART_SENTINEL" @@ -79,16 +75,11 @@ if ! git -C "$COMPONENT_PATH" diff --quiet "$PREV_HASH" "$NEW_HASH" \ echo "[hook:BlocksScreen] daemon-reload done (xorg service)" fi -# --- install-updater.sh changed (checked independently) --- +# --- install files changed (checked independently) --- if ! git -C "$COMPONENT_PATH" diff --quiet "$PREV_HASH" "$NEW_HASH" \ - -- scripts/install-updater.sh 2>/dev/null; then - echo "[hook:BlocksScreen] install-updater.sh changed - setting deploy flag" + -- scripts/install-updater.sh scripts/bs-apt-helper.sh 2>/dev/null; then + echo "[hook:BlocksScreen] install files changed - setting deploy flag" _set_deploy_flag fi -# NOTE: do NOT trigger a daemon restart here when updater/ changes. The deploy -# flag fires BlocksScreen-deploy.path immediately, which runs install-updater.sh -# and restarts BlocksScreen-updater.service while it is still running this very -# update batch, which cancels the batch and reverts the update. The daemon picks -# up new updater/ code on the next reboot; a clean post-batch self-restart is the -# proper fix (see docs/superpowers/specs/2026-06-19-updater-self-update-ordering-design.md). +# NOTE: no daemon restart here on updater/ changes: mid-batch restart cancels+reverts (see 2026-06-19 self-update-ordering spec). diff --git a/updater/locking.py b/updater/locking.py index 3d51901e..c7bd5abb 100644 --- a/updater/locking.py +++ b/updater/locking.py @@ -26,8 +26,7 @@ def _runtime_dir() -> Path: return d except OSError: continue - # Both unavailable (broken home): return the cache path so the caller's open() - # surfaces the error rather than silently falling back to world-writable /tmp. + # Broken home: return the cache path so open() surfaces the error (no /tmp). return cache @@ -58,8 +57,7 @@ def process_lock() -> Iterator[bool]: try: f = open(lock_path(), "w") # noqa: SIM115, PTH123 except OSError: - # Disk full / read-only SD: behave as "could not acquire" so the caller - # degrades gracefully instead of crashing the update operation. + # Disk-full/RO SD: treat as "not acquired" so the caller degrades gracefully. yield False return try: diff --git a/updater/models.py b/updater/models.py index a13f171d..684c1b04 100644 --- a/updater/models.py +++ b/updater/models.py @@ -17,8 +17,7 @@ class ComponentConfig: apt_exclude: tuple[str, ...] = () url: str | None = None install_if_missing: bool = False - # Fire-and-forget restart BlocksScreen on update even when the component's - # own service differs (e.g. klipper config the UI reads at startup). + # Restart BlocksScreen on update even when the component's service differs. restart_ui: bool = False diff --git a/updater/service.py b/updater/service.py index 61dbef7e..2a19cbe8 100644 --- a/updater/service.py +++ b/updater/service.py @@ -15,6 +15,7 @@ from updater.components import load_components from updater.executor import ( _GIT_SHA_RE, + HOOK_TIMEOUT, PIP, UPDATER_SERVICE, _apt_get_fix_broken, @@ -37,6 +38,7 @@ git_pull, git_repair, git_reset_to_hash, + is_git_repo, enable_service, restart_service, restart_service_noblock, @@ -48,31 +50,21 @@ from updater.models import ComponentConfig, ComponentStatus _STATE_PATH = Path.home() / ".cache" / "blockscreen" / "updater_state.json" -# Persistent (SD-backed, not tmpfs) record of the components a batch is actively -# mutating, mapped to the hash to return to. Written before staging and removed -# at any terminal outcome, so a reboot finding it knows an update was cut off -# mid-flight and reverts each repo to its pre-update commit. +# SD-backed map of component name to pre-update hash; if present at boot, revert. _INFLIGHT_PATH = Path.home() / ".cache" / "blockscreen" / "updater_inflight.json" _HISTORY_PATH = Path.home() / ".cache" / "blockscreen" / "update_history.jsonl" # Cap the history so a device running for years cannot fill the SD card. _HISTORY_MAX_BYTES = 1_000_000 _HISTORY_KEEP_LINES = 2000 -# git's empty-tree object: used as the prev_hash when provisioning so a -# diff-based hook (`git diff `) sees every file as newly added and -# performs a full install, instead of no-op'ing on an empty prev_hash. +# git's empty tree as provisioning prev_hash: diff hooks see all files as new. _GIT_EMPTY_TREE = "4b825dc642cb6eb9a060e54bf8d69288fbee4904" -# Services that host the updater's own D-Bus client (the UI). They are restarted -# non-blocking and never waited on, so a self-update cannot tear down or abort -# the in-flight batch; new UI code loads on the queued restart, daemon code on -# the next reboot. +# UI services (our D-Bus client): no-block restart so self-update can't kill the batch. _UI_SERVICE = "BlocksScreen.service" _FIRE_AND_FORGET_SERVICES = frozenset({_UI_SERVICE}) -# Deploy flag watched by BlocksScreen-deploy.path; touching it runs -# install-updater.sh out-of-band (its own cgroup), the cgroup-safe way to apply -# unit/sudoers/install changes after a batch completes. +# Deploy flag for BlocksScreen-deploy.path: runs install-updater.sh in its own cgroup. _DEPLOY_FLAG = Path.home() / ".config" / "blockscreen" / ".run-install-updater" @@ -149,8 +141,7 @@ async def _check_one(c: ComponentConfig) -> None: cache_ttl_seconds=0 if force else 86_400, exclude=c.apt_exclude ) elif c.path is None or not c.path.exists(): - # A missing opted-in component surfaces as needs_install so the - # one Update button reaches it; everything else stays skipped. + # Missing opted-in components surface as needs_install; others skip. if c.install_if_missing and c.url: results[c.name] = ComponentStatus(name=c.name, needs_install=True) return @@ -162,9 +153,7 @@ async def _check_one(c: ComponentConfig) -> None: status = await check_git_status( c.name, c.path, c.branch, c.version, skip_fetch ) - # Record fetch time only after a successful check so a failed - # (offline) fetch is retried on the next poll instead of being - # suppressed for the full TTL. + # Record fetch time only on success so offline fetches retry. if not skip_fetch and status.error is None: async with self._git_lock: self._fetch_times[c.name] = now @@ -220,6 +209,13 @@ async def update_all(self, names: set[str] | None = None) -> None: for c in sorted_components if c.kind == "git" and c.path is not None and c.path.exists() ] + # A dir without .git (tarball install) errors individually, not the batch. + for c in [c for c in batch if not is_git_repo(c.path)]: + batch.remove(c) + if c.install_if_missing and c.url and await self._quarantine_nonrepo(c): + continue # path is now absent: the provision pass below clones fresh + self._log.error("%s: %s is not a git repository - skipping", c.name, c.path) + self._cb_error_done(c.name, "not a git repository - reinstall required") provision = [ c for c in sorted_components @@ -246,17 +242,17 @@ async def _preflight_fetch( its own fetch in the apply phase (recorded fetch time). Repos that need cloning or are corrupt are left to self-heal in their own flow. """ + # Non-repo dirs excluded: their fetch failure is not "offline" (see update_all). targets = [ c for c in sorted_components - if c.kind == "git" and c.path is not None and c.path.exists() + if c.kind == "git" and c.path is not None and is_git_repo(c.path) ] offline: list[str] = [] async with self._git_lock: for c in targets: now = time.monotonic() - # Skip a component the status check just fetched (<30s ago); its - # recorded time still lets the apply phase skip its own fetch. + # Skip if fetched <30s ago; apply phase still skips its own fetch. if now - self._fetch_times.get(c.name, 0.0) < 30: continue ok, err = await git_fetch(c.path) @@ -298,13 +294,14 @@ async def _run_git_batch(self, batch: list[ComponentConfig]) -> None: for b in batch: self._cb_error_done(b.name, "failed to persist rollback point") return - # Mark the batch in-flight so a power cut before commit is reverted - # to these hashes on the next boot (reconcile). + # Mark in-flight so a pre-commit power cut is reverted on next boot. await asyncio.to_thread(self._write_inflight, dict(prev)) for c in batch: ok, reason = await _assert_https_remote(c.path) if not ok: self._log.error("%s: SEC-4 remote check failed: %s", c.name, reason) + # Nothing staged: drop the marker (avoids a spurious boot revert). + await asyncio.to_thread(self._clear_inflight) for b in batch: self._cb_error_done(b.name, "insecure remote") return @@ -346,16 +343,23 @@ async def _run_git_batch(self, batch: list[ComponentConfig]) -> None: prev[c.name][:8], new_hashes[c.name][:8], ) - hook_ok, hook_err = await run_hook( - c.name, c.path, new_hashes[c.name], prev[c.name] + hook_ok, hook_err = await self._ping_while( + run_hook( + c.name, + c.path, + new_hashes[c.name], + prev[c.name], + timeout=HOOK_TIMEOUT, + ), + c.name, + 3, + 4, ) if not hook_ok: self._log.error("%s: hook failed: %s", c.name, hook_err) await self._abort_batch(batch, prev, touched, restarted, "hook") return - # Restart each unique non-self service once and verify it. Mark bounced - # before the restart so an abort re-restarts even a service that failed - # its health check, onto the reverted code. Self/UI services are deferred. + # Restart each unique service once; record it BEFORE so abort re-restarts it. self._log.info("git batch: restart phase") seen: set[str] = set() ui_components: list[ComponentConfig] = [] @@ -376,20 +380,15 @@ async def _run_git_batch(self, batch: list[ComponentConfig]) -> None: if not await self._restart_one(c.service): await self._abort_batch(batch, prev, touched, restarted, "restart") return - # All verified services are up: the batch has succeeded. Record success - # and persist BEFORE the fire-and-forget UI restart, which tears down the - # D-Bus client, so a completed update can never be reverted by it. + # Persist success BEFORE UI restart: a committed update is never reverted. for c in batch: self._history( "update_success", c.name, new_hash=new_hashes[c.name][:12] ) self._cb("on_component_done", c.name, True) - # Past this point the update is durable. The post-success restarts - # below include a self-restart of the daemon, whose SIGTERM surfaces - # as CancelledError here; it must never trigger a revert. + # Durable now; the self-restart's CancelledError must never revert. committed = True - # Durably clear the in-flight marker before any restart: past this - # point the update stands and must never be reverted on reboot. + # Clear marker before any restart: the update stands from here on. await asyncio.to_thread(self._clear_inflight) self._log.info( "git batch complete: %d component(s) updated; queueing UI restart(s)", @@ -554,8 +553,7 @@ async def recover(self, name: str, hard: bool = False) -> bool: if hard and component.service: restart_ok, restart_err = await restart_service(component.service) if restart_ok: - # restart_service can fall back to `systemctl kill` and report - # success without the unit coming back, so confirm it is active. + # restart may lie (kill fallback); confirm the unit is active. restart_ok = await wait_for_service_active( component.service, timeout=90.0 ) @@ -684,9 +682,7 @@ async def _install_dependencies( pip_path = self._component_pip_cache[cache_key] if pip_path == PIP: - # No component venv found. Modern Debian rejects system pip installs - # (PEP 668 externally-managed-environment). Deps are managed by the - # component's own installer (e.g. klipper's klippy-env). + # No venv: PEP 668 blocks system pip; component installer owns deps. self._log.info( "%s: no component venv - skipping dep install", component.name ) @@ -705,6 +701,28 @@ async def _install_dependencies( [pip_path, "install", "-r", str(req), "--quiet"], timeout=120.0 ) + async def _ping_while( + self, coro, name: str, step: int, total: int, interval: float = 60.0 + ): + """Await coro, re-emitting on_step every `interval` seconds. + + A HOOK_TIMEOUT-bounded hook can run silent longer than the client's + 360s busy watchdog; the pings keep it fed. Cancellation propagates to + the inner task so its subprocess is still killed. + """ + task = asyncio.ensure_future(coro) + try: + while True: + done, _ = await asyncio.wait({task}, timeout=interval) + if done: + return task.result() + self._cb("on_step", name, step, total) + finally: + if not task.done(): + task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await task + async def _shielded(self, coro, label: str) -> None: """Run cleanup to completion, surviving repeated cancels of the caller.""" task = asyncio.ensure_future(coro) @@ -756,6 +774,33 @@ def _trim_history(self) -> None: tmp.write_text("\n".join(lines[-_HISTORY_KEEP_LINES:]) + "\n", encoding="utf-8") tmp.replace(self._history_path) + def _quarantine_sync(self, path: Path) -> Path | None: + """Rename a non-repo dir aside (same fs, instant, reversible); None on failure.""" + dest = path.with_name( + f"{path.name}.pre-updater-{time.strftime('%Y%m%d-%H%M%S')}" + ) + try: + os.rename(path, dest) + except OSError as exc: + self._log.error("quarantine of %s failed: %s", path, exc) + return None + return dest + + async def _quarantine_nonrepo(self, component: ComponentConfig) -> bool: + """Move an install_if_missing component's non-repo dir aside for a fresh clone.""" + if component.path is None: + return False + dest = await asyncio.to_thread(self._quarantine_sync, component.path) + if dest is None: + return False + self._history("quarantine_nonrepo", component.name, moved_to=dest.name) + self._log.warning( + "%s: non-repo dir quarantined to %s; provisioning fresh clone", + component.name, + dest, + ) + return True + async def _remove_clone(self, component: ComponentConfig) -> None: """Delete a freshly-cloned tree (the path did not exist before provisioning).""" if component.path is not None: @@ -807,8 +852,17 @@ async def _provision_component(self, component: ComponentConfig) -> bool: return await self._fail_provision(component, "deps") self._cb("on_step", component.name, 3, 4) - hook_ok, hook_err = await run_hook( - component.name, component.path, new_hash, _GIT_EMPTY_TREE, timeout=600.0 + hook_ok, hook_err = await self._ping_while( + run_hook( + component.name, + component.path, + new_hash, + _GIT_EMPTY_TREE, + timeout=HOOK_TIMEOUT, + ), + component.name, + 3, + 4, ) if not hook_ok: self._log.error("%s: provision hook: %s", component.name, hook_err) @@ -827,8 +881,7 @@ async def _provision_component(self, component: ComponentConfig) -> bool: "%s: provisioned service not active", component.name ) return await self._fail_provision(component, "restart_timeout") - # Enable only now it started cleanly (a failed provision must not - # leave a boot-looping unit). Best-effort: it already runs now. + # Enable only after a clean start (no boot-looping failed unit). en_ok, en_err = await enable_service(component.service) if not en_ok: self._log.warning( @@ -864,9 +917,7 @@ async def _stage_component(self, component: ComponentConfig) -> tuple[bool, str] if component.path is None: return (False, "path not found") async with self._git_lock: - # Gate the update-fetch on this component's last *successful* fetch, not - # the last status check: an errored/corrupt repo has no recorded fetch - # time, so it always re-fetches here (and then self-heals). + # Gate on last *successful* fetch: errored repos always re-fetch here. last_fetch = self._fetch_times.pop(component.name, 0.0) elapsed = time.monotonic() - last_fetch if last_fetch else float("inf") @@ -874,8 +925,7 @@ async def _stage_component(self, component: ComponentConfig) -> tuple[bool, str] ok, error = await git_fetch(component.path) if not ok: self._log.error(error) - # fsck --connectivity-only can miss the object corruption that - # broke the fetch, so pass the fetch error as a hint. + # connectivity-only fsck can miss it; pass the fetch error as hint. if not await git_has_corruption(component.path, hint=error): return (False, "network") self._log.warning("%s: corrupt repo, repairing", component.name) @@ -885,8 +935,7 @@ async def _stage_component(self, component: ComponentConfig) -> tuple[bool, str] self._history("repair", component.name, detail=rmsg[:80]) if component.reset_mode == "hard": - # Reset to the remote tracking ref, not HEAD, so local commits that - # diverge from origin are discarded before the pull. + # Reset to the remote ref, not HEAD, discarding diverged local commits. if component.branch: hard_ref = f"origin/{component.branch}" else: @@ -946,6 +995,20 @@ async def _run_git_update(self, component: ComponentConfig) -> bool: if component.install_if_missing and component.url: return await self._provision_component(component) return self._cb_error_done(component.name, "path not found") + # Non-repo dir (e.g. pre-updater tarball install): nothing to update or revert. + if not is_git_repo(component.path): + if ( + component.install_if_missing + and component.url + and await self._quarantine_nonrepo(component) + ): + return await self._provision_component(component) + self._log.error( + "%s: %s is not a git repository", component.name, component.path + ) + return self._cb_error_done( + component.name, "not a git repository - reinstall required" + ) prev_hash = await git_get_hash(component.path) if prev_hash == "": @@ -962,6 +1025,8 @@ async def _run_git_update(self, component: ComponentConfig) -> bool: ok, reason = await _assert_https_remote(component.path) if not ok: self._log.error("SEC-4 remote check failed: %s", reason) + # Nothing staged: drop the marker (avoids a spurious boot revert). + await asyncio.to_thread(self._clear_inflight) return self._cb_error_done(component.name, "insecure remote") self._history("update_start", component.name, prev_hash=prev_hash[:12]) @@ -970,9 +1035,9 @@ async def _run_git_update(self, component: ComponentConfig) -> bool: self._cb("on_step", component.name, 1, 4) stage_ok, stage_reason = await self._stage_component(component) if not stage_ok: - # A failed pre-update reset changed nothing, so there is nothing - # to roll back; every other stage failure leaves a dirty tree. + # Failed pre-update reset changed nothing, so nothing to roll back. if stage_reason == "reset": + await asyncio.to_thread(self._clear_inflight) return self._cb_error_done(component.name, "reset") await self._rollback(component, prev_hash, stage_reason) return False @@ -986,8 +1051,17 @@ async def _run_git_update(self, component: ComponentConfig) -> bool: self._cb("on_step", component.name, 3, 4) new_hash = await git_get_hash(component.path) - hook_ok, hook_err = await run_hook( - component.name, component.path, new_hash, prev_hash + hook_ok, hook_err = await self._ping_while( + run_hook( + component.name, + component.path, + new_hash, + prev_hash, + timeout=HOOK_TIMEOUT, + ), + component.name, + 3, + 4, ) if not hook_ok: self._log.error("hook failed for %s: %s", component.name, hook_err) @@ -1120,8 +1194,7 @@ async def _run_apt_update_locked( return True except asyncio.CancelledError: self._log.warning("%s: apt update cancelled", component.name) - # The cancelled subprocess may have been a SIGKILLed dpkg; repair - # the package database before reporting back. + # May have SIGKILLed dpkg; repair the package db before reporting. await self._shielded( _apt_get_fix_broken(), f"{component.name} apt cancel-repair" ) @@ -1183,8 +1256,7 @@ def _write_state(self, data: dict) -> bool: prefix=".updater_state_", ) as f: f.write(json.dumps(data, indent=2)) - # fsync the file + parent dir so the rollback point survives a - # power cut: rename-without-fsync can otherwise zero the file. + # fsync file+dir so the rollback point survives a power cut. f.flush() os.fsync(f.fileno()) temp_path = Path(f.name)