From a8999428a15cc7d24fd613b87e2b794f4ce906c8 Mon Sep 17 00:00:00 2001 From: Guilherme Costa Date: Wed, 15 Jul 2026 14:53:21 +0100 Subject: [PATCH] updater(fixes): Self-heal hardening and previous commit fix --- BlocksScreen/lib/utils/blocks_progressbar.py | 3 +- tests/updater/test_executor_unit.py | 156 +++++++++++++++++++ tests/updater/test_service_unit.py | 2 +- tests/widgets/test_progress_bar_unit.py | 7 +- updater/executor.py | 82 +++++++++- updater/hooks/Spoolman.sh | 7 + updater/service.py | 6 +- 7 files changed, 251 insertions(+), 12 deletions(-) diff --git a/BlocksScreen/lib/utils/blocks_progressbar.py b/BlocksScreen/lib/utils/blocks_progressbar.py index bc08f573..56494700 100644 --- a/BlocksScreen/lib/utils/blocks_progressbar.py +++ b/BlocksScreen/lib/utils/blocks_progressbar.py @@ -1,5 +1,6 @@ import typing -from PyQt6 import QtWidgets, QtGui, QtCore + +from PyQt6 import QtCore, QtGui, QtWidgets class CustomProgressBar(QtWidgets.QProgressBar): diff --git a/tests/updater/test_executor_unit.py b/tests/updater/test_executor_unit.py index f4618ead..af1d3fc7 100644 --- a/tests/updater/test_executor_unit.py +++ b/tests/updater/test_executor_unit.py @@ -3,13 +3,18 @@ from __future__ import annotations import asyncio +import os +import time from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch import pytest from updater.executor import ( + _clear_stale_git_index_lock, + _is_head_readable, _make_clean_env, + _repair_corrupt_head, _remove_broken_loose_ref, _run, apt_update, @@ -910,6 +915,11 @@ async def test_repair_deletes_empty_objects_then_fetches(self, tmp_path): new_callable=AsyncMock, return_value=False, ), + patch( + "updater.executor._repair_corrupt_head", + new_callable=AsyncMock, + return_value=True, + ), ): ok, _msg = await git_repair(tmp_path) assert ok is True @@ -963,6 +973,11 @@ async def test_repair_quarantines_non_empty_corrupt_then_succeeds(self, tmp_path new_callable=AsyncMock, return_value=1, ) as mock_quarantine, + patch( + "updater.executor._repair_corrupt_head", + new_callable=AsyncMock, + return_value=True, + ), ): ok, msg = await git_repair(tmp_path) assert ok is True @@ -1125,3 +1140,144 @@ def test_permanent(self, err): ) def test_transient(self, err): assert classify_apt_error(err) == "transient" + + +class TestStaleIndexLockHandling: + @pytest.mark.asyncio + async def test_clear_stale_git_index_lock_removes_old_lock(self, tmp_path): + + lock_path = tmp_path / ".git" / "index.lock" + lock_path.parent.mkdir(parents=True) + lock_path.write_text("") + lock_path.touch() + + st = lock_path.stat() + os.utime(lock_path, (st.st_atime, time.time() - 30)) + assert _clear_stale_git_index_lock(tmp_path) is True + assert not lock_path.exists() + + @pytest.mark.asyncio + async def test_clear_stale_git_index_lock_preserves_fresh_lock(self, tmp_path): + lock_path = tmp_path / ".git" / "index.lock" + lock_path.parent.mkdir(parents=True) + lock_path.write_text("") + assert _clear_stale_git_index_lock(tmp_path) is False + assert lock_path.exists() + + @pytest.mark.asyncio + async def test_clear_stale_git_index_lock_no_lock(self, tmp_path): + + assert _clear_stale_git_index_lock(tmp_path) is True + + @pytest.mark.asyncio + async def test_git_reset_to_hash_clears_lock_on_index_lock_error(self): + fail_proc = _make_proc(1, b"", b"error: index.lock blocked git") + ok_proc = _make_proc(0, b"abc123\n", b"") + procs = [fail_proc, ok_proc] + idx = [0] + + async def mock_exec(*_args, **_kwargs): + result = procs[idx[0]] + idx[0] = min(idx[0] + 1, len(procs) - 1) + return result + + with ( + patch( + "updater.executor._clear_stale_git_index_lock", return_value=True + ) as mock_clear, + patch( + "asyncio.create_subprocess_exec", + new_callable=AsyncMock, + side_effect=mock_exec, + ), + ): + ok, _msg = await git_reset_to_hash(Path("/x"), "abc123") + assert ok is True + mock_clear.assert_called_once_with(Path("/x")) + + +class TestCorruptHeadHandling: + @pytest.mark.asyncio + async def test_is_head_readable_true_when_valid(self): + proc = _make_proc(0, b"abc123\n", b"") + with patch( + "asyncio.create_subprocess_exec", new_callable=AsyncMock, return_value=proc + ): + assert await _is_head_readable(Path("/x")) is True + + @pytest.mark.asyncio + async def test_is_head_readable_false_when_corrupt(self): + proc = _make_proc(1, b"", b"fatal: Not a git repository") + with patch( + "asyncio.create_subprocess_exec", new_callable=AsyncMock, return_value=proc + ): + assert await _is_head_readable(Path("/x")) is False + + @pytest.mark.asyncio + async def test_repair_corrupt_head_succeeds_when_head_already_readable(self): + proc = _make_proc(0, b"", b"") + with patch( + "asyncio.create_subprocess_exec", new_callable=AsyncMock, return_value=proc + ): + ok = await _repair_corrupt_head(Path("/x"), "main") + assert ok is True + + @pytest.mark.asyncio + async def test_repair_corrupt_head_fixes_by_symbolic_ref(self): + fail_proc = _make_proc(1, b"", b"fatal: corrupt HEAD") + ok_proc = _make_proc(0, b"", b"") + procs = [fail_proc, ok_proc] + idx = [0] + + async def mock_exec(*_args, **_kwargs): + result = procs[idx[0]] + idx[0] = min(idx[0] + 1, len(procs) - 1) + return result + + with patch( + "asyncio.create_subprocess_exec", + new_callable=AsyncMock, + side_effect=mock_exec, + ): + ok = await _repair_corrupt_head(Path("/x"), "main") + assert ok is True + + @pytest.mark.asyncio + async def test_repair_corrupt_head_uses_custom_branch(self): + fail_proc = _make_proc(1, b"", b"fatal: corrupt HEAD") + ok_proc = _make_proc(0, b"", b"") + procs = [fail_proc, ok_proc] + idx = [0] + + async def mock_exec(*_args, **_kwargs): + result = procs[idx[0]] + idx[0] = min(idx[0] + 1, len(procs) - 1) + return result + + with patch( + "asyncio.create_subprocess_exec", + new_callable=AsyncMock, + side_effect=mock_exec, + ) as mock_exec_obj: + ok = await _repair_corrupt_head(Path("/x"), "dev") + assert ok is True + calls = mock_exec_obj.call_args_list + assert any("refs/heads/dev" in str(call) for call in calls) + + +class TestCorruptionSignatures: + @pytest.mark.asyncio + async def test_has_corruption_detects_unable_to_unpack(self): + proc = _make_proc(1, b"", b"error: unable to unpack objects\n") + with patch( + "asyncio.create_subprocess_exec", new_callable=AsyncMock, return_value=proc + ): + assert await git_has_corruption(Path("/x")) is True + + @pytest.mark.asyncio + async def test_has_corruption_detects_inflate_error(self): + proc = _make_proc(1, b"", b"error: inflate: data stream error\n") + with patch( + "asyncio.create_subprocess_exec", new_callable=AsyncMock, return_value=proc + ): + assert await git_has_corruption(Path("/x")) is True diff --git a/tests/updater/test_service_unit.py b/tests/updater/test_service_unit.py index 543c0036..980cdf4c 100644 --- a/tests/updater/test_service_unit.py +++ b/tests/updater/test_service_unit.py @@ -1604,7 +1604,7 @@ async def test_unreadable_head_triggers_repair(self, tmp_path): svc = UpdateService() svc._components = [comp] await svc.reconcile() - mock_repair.assert_called_once_with(comp.path) + mock_repair.assert_called_once_with(comp.path, "main") @pytest.mark.asyncio async def test_repair_failure_falls_back_to_prev_hash_reset(self, tmp_path): diff --git a/tests/widgets/test_progress_bar_unit.py b/tests/widgets/test_progress_bar_unit.py index 4a919b76..becd6d5b 100644 --- a/tests/widgets/test_progress_bar_unit.py +++ b/tests/widgets/test_progress_bar_unit.py @@ -2,13 +2,13 @@ import pytest -from lib.utils.blocks_progressbar import CustomProgressBar +from BlocksScreen.lib.utils.blocks_progressbar import CustomProgressBar @pytest.fixture() def bar(qtbot): """Create a CustomProgressBar registered with qtbot.""" - w = CustomProgressBar() + w = CustomProgressBar(parent=None) qtbot.addWidget(w) return w @@ -26,7 +26,8 @@ def test_full(self, bar): def test_clamps_above_one(self, bar): bar.set_progress(1.4) - assert bar.progress_value == 100 + progress = bar.progress_value + assert progress == 100 def test_clamps_below_zero(self, bar): bar.set_progress(-0.2) diff --git a/updater/executor.py b/updater/executor.py index dbe921cd..437da324 100644 --- a/updater/executor.py +++ b/updater/executor.py @@ -41,6 +41,38 @@ _PACKAGE_NAME_RE = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9+\-.]*$") _HOOKS_DIR = Path(__file__).parent / "hooks" +_STALE_LOCK_AGE_THRESHOLD_S = 10.0 + + +def _clear_stale_git_index_lock(path: Path) -> bool: + """Remove .git/index.lock only if it is stale (older than threshold). + + A stale lock is left by a SIGKILL'd or interrupted git operation. Only + remove if mtime indicates lock is NOT held by a live git process (age >= + threshold). At runtime, a fresh lock may indicate a concurrent legitimate + git op; yanking it would corrupt that op. Returns True if lock was + removed or did not exist; False if lock exists but is too fresh to safely + remove. + """ + lock_path = path / ".git" / "index.lock" + if not lock_path.exists(): + return True + try: + age_s = time.time() - lock_path.stat().st_mtime + if age_s >= _STALE_LOCK_AGE_THRESHOLD_S: + logger.warning("clearing stale index.lock (age %.1fs) from %s", age_s, path) + lock_path.unlink() + return True + logger.debug( + "index.lock is fresh (age %.1fs < %.1fs threshold), not removing to avoid " + "corrupting concurrent git op", + age_s, + _STALE_LOCK_AGE_THRESHOLD_S, + ) + return False + except OSError as exc: + logger.warning("could not age-check index.lock at %s: %s", lock_path, exc) + return False def _kill_proc_group(proc, sig): @@ -400,6 +432,7 @@ async def git_reset_to_hash(path: Path | None, prev_hash: str = "") -> tuple[boo 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 reset fails with an index.lock error, clears it and retries once. """ if not path: return (False, "path error") @@ -407,7 +440,12 @@ async def git_reset_to_hash(path: Path | None, prev_hash: str = "") -> tuple[boo 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=60.0) + ok, err = await _run([GIT, "reset", "--hard", prev_hash], cwd=path, timeout=60.0) + if ok: + return (ok, err) + if "index.lock" in err and _clear_stale_git_index_lock(path): + return await _run([GIT, "reset", "--hard", prev_hash], cwd=path, timeout=60.0) + return (ok, err) # Narrow corruption signatures: "not a git repository" etc. must not match. @@ -419,6 +457,8 @@ async def git_reset_to_hash(path: Path | None, prev_hash: str = "") -> tuple[boo "missing blob", "missing tree", "missing commit", + "unable to unpack", + "inflate: data stream error", ) # Quarantine dir inside .git/objects so it never appears as untracked. @@ -509,25 +549,32 @@ async def _quarantine_corrupt_objects(path: Path) -> int: return moved -async def git_repair(path: Path) -> tuple[bool, str]: +async def git_repair(path: Path, branch: str = "main") -> tuple[bool, str]: """Prune 0-byte loose objects, re-fetch, and re-verify. Mirrors start.sh recovery. If empty-object pruning plus a fetch does not clear the corruption, escalate to quarantining non-empty corrupt loose objects (`fsck --full`) and re-fetch once more. Working tree is untouched (delete/move + fetch only), so tracked-but- - modified files survive. Returns (ok, message). + modified files survive. If fetch fails with index.lock error, clears lock and + retries. Branch parameter is used to repair corrupt HEAD (fallback to "main"). + Returns (ok, message). """ + _clear_stale_git_index_lock(path) objects = path / ".git" / "objects" if not objects.is_dir(): return (False, "no .git/objects directory") removed = _prune_empty_loose_objects(objects) logger.warning("git_repair: removed %d empty object(s) from %s", removed, path) ok, out = await git_fetch(path, prune_remotes=False) + if not ok and "index.lock" in out: + _clear_stale_git_index_lock(path) + ok, out = await git_fetch(path, prune_remotes=False) if not ok: return (False, f"fetch after cleanup failed: {out}") if not await git_has_corruption(path): + if not await _repair_corrupt_head(path, branch): + return (False, "repaired objects but HEAD still unreadable") return (True, f"repaired ({removed} empty objects removed)") - # 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)") @@ -536,6 +583,8 @@ async def git_repair(path: Path) -> tuple[bool, str]: return (False, f"fetch after quarantine failed: {out}") if await git_has_corruption(path): return (False, "still corrupt after quarantine + fetch") + if not await _repair_corrupt_head(path, branch): + return (False, "repaired objects but HEAD still unreadable") return ( True, f"repaired ({removed} empty + {quarantined} corrupt object(s) removed)", @@ -550,6 +599,31 @@ async def git_get_hash(path: Path | None) -> str: return output.strip() if ok else "" +async def _is_head_readable(path: Path) -> bool: + """Return True if HEAD is readable and points to a valid commit.""" + ok, _out = await _run([GIT, "rev-parse", "HEAD"], cwd=path, timeout=10.0) + return ok + + +async def _repair_corrupt_head(path: Path, branch: str = "main") -> bool: + """Try to repair a corrupt HEAD by rewriting it to a valid symbolic ref. + + Returns True if HEAD was repaired or is already readable. + """ + if await _is_head_readable(path): + return True + ok, _msg = await _run( + [GIT, "symbolic-ref", "HEAD", f"refs/heads/{branch}"], + cwd=path, + timeout=10.0, + ) + if ok: + logger.warning("repair_corrupt_head: rewrote HEAD for %s", path) + return True + logger.warning("repair_corrupt_head: failed to rewrite HEAD for %s", path) + return False + + async def git_ref_hash(path: Path | None, ref: str) -> str: """Resolve an arbitrary ref (e.g. origin/main) to its commit hash, or empty.""" if path is None: diff --git a/updater/hooks/Spoolman.sh b/updater/hooks/Spoolman.sh index 8b242ec7..6dd7b552 100755 --- a/updater/hooks/Spoolman.sh +++ b/updater/hooks/Spoolman.sh @@ -31,4 +31,11 @@ fi if [ ! -f ".env" ] && [ -f ".env.example" ]; then cp .env.example .env fi + +if ! systemctl is-active --quiet Spoolman.service 2>/dev/null; then + echo "[hook:Spoolman] enabling and starting Spoolman.service" + sudo systemctl enable --now Spoolman.service 2>/dev/null || { + echo "[hook:Spoolman] WARN: could not enable/start Spoolman.service - continuing" + } +fi echo "[hook:Spoolman] done" diff --git a/updater/service.py b/updater/service.py index 4cb73ea8..77a1671f 100644 --- a/updater/service.py +++ b/updater/service.py @@ -1045,12 +1045,12 @@ async def _reconcile_locked(self) -> None: if await git_get_hash(c.path) != "": continue self._log.warning("reconcile: %s HEAD unreadable - repairing", c.name) - ok, msg = await git_repair(c.path) - if ok: + _branch = c.branch or "main" + ok, msg = await git_repair(c.path, _branch) + if ok and await git_get_hash(c.path) != "": self._history("boot_repair", c.name, detail=msg[:80]) continue comp_state = (await asyncio.to_thread(self._read_state)).get(c.name, {}) - # a power cut can corrupt state.json too: entry may be a non-dict prev = ( comp_state.get("prev_hash") if isinstance(comp_state, dict)