diff --git a/test/test_auto_update.py b/test/test_auto_update.py index 840b00f6..2464d33e 100644 --- a/test/test_auto_update.py +++ b/test/test_auto_update.py @@ -823,6 +823,7 @@ def test_core_update_names_the_requirement_files_that_failed(): pkg.api_v3.plugin_store_manager = None failed_install = subprocess.CompletedProcess([], 1, stdout='', stderr='boom') with patch.object(pkg.subprocess, 'run', run), \ + patch.object(system.git_index_lock, 'run_git', run), \ patch.object(system, '_pip_install_requirements', return_value=failed_install): result = system.perform_core_update() assert result['status'] == 'success' diff --git a/test/test_git_index_lock.py b/test/test_git_index_lock.py index d49d337b..a60cd94d 100644 --- a/test/test_git_index_lock.py +++ b/test/test_git_index_lock.py @@ -11,6 +11,7 @@ import subprocess import sys import time from pathlib import Path +import contextlib from unittest.mock import patch import pytest @@ -27,6 +28,13 @@ needs_git = pytest.mark.skipif(shutil.which('git') is None, reason='git not inst OLD = 1_000_000.0 +@contextlib.contextmanager +def _patched(fake): + """Fake git for both the plain subprocess calls and the timeout-safe runner.""" + with patch.object(mod.subprocess, 'run', fake), patch.object(mod.git_index_lock, 'run_git', fake): + yield + + def _checkout(tmp_path, lock_age=None): repo = tmp_path / 'LEDMatrix' (repo / '.git').mkdir(parents=True) @@ -173,7 +181,7 @@ def test_update_code_clears_the_lock_before_running_git(client): return gil.REMOVED calls = [] with patch.object(gil, 'clear_stale_index_lock', clear), \ - patch.object(mod.subprocess, 'run', _fake_git(calls)): + _patched(_fake_git(calls)): data = _update(client) assert seen == [str(mod.PROJECT_ROOT)] assert data['status'] == 'success' @@ -182,7 +190,7 @@ def test_update_code_clears_the_lock_before_running_git(client): def test_update_code_waits_for_a_git_that_is_still_running(client): calls = [] with patch.object(gil, 'clear_stale_index_lock', lambda d: gil.IN_USE), \ - patch.object(mod.subprocess, 'run', _fake_git(calls)): + _patched(_fake_git(calls)): data = _update(client) assert data['status'] == 'error' assert 'Another git command is running' in data['message'] @@ -193,8 +201,45 @@ def test_a_lock_it_could_not_clear_gets_the_manual_fix(client): err = "fatal: Unable to create '/home/ledpi/LEDMatrix/.git/index.lock': File exists.\n" calls = [] with patch.object(gil, 'clear_stale_index_lock', lambda d: gil.UNKNOWN), \ - patch.object(mod.subprocess, 'run', _fake_git(calls, pull_rc=128, pull_err=err)): + _patched(_fake_git(calls, pull_rc=128, pull_err=err)): data = _update(client) assert data['status'] == 'error' assert data['message'].startswith('Update failed: fatal: Unable to create') assert 'rm -f ' in data['message'] and 'index.lock' in data['message'] + + +# -- run_git: a timed-out git must not leave a lock or an orphaned helper ------ + +def _lock_holder_script(tmp_path, lock): + """A stand-in for a slow git: takes the lock, spawns a helper, then hangs.""" + script = tmp_path / 'slow.sh' + script.write_text(f'#!/bin/sh\n: > "{lock}"\nsleep 60 &\necho $! > "{tmp_path}/helper.pid"\nwait\n') + script.chmod(0o755) + return script + + +def test_run_git_returns_a_completed_process(tmp_path): + result = gil.run_git([sys.executable, '-c', 'print("hi")'], cwd=str(tmp_path), timeout=30) + assert (result.returncode, result.stdout.strip()) == (0, 'hi') + + +@pytest.mark.skipif(not os.path.isdir('/proc'), reason='needs /proc') +def test_run_git_timeout_stops_the_group_and_clears_the_lock(tmp_path, monkeypatch): + (tmp_path / '.git').mkdir() + lock = tmp_path / '.git' / 'index.lock' + script = _lock_holder_script(tmp_path, lock) + monkeypatch.setattr(gil, 'MIN_STALE_AGE_SECONDS', 0) + monkeypatch.setattr(gil, 'TERM_GRACE_SECONDS', 1) + # The scan only sees processes named git, so the stand-in must not match; + # the lock is then provably unowned once the group is gone. + with pytest.raises(subprocess.TimeoutExpired): + gil.run_git([str(script)], cwd=str(tmp_path), timeout=1) + assert not lock.exists() + helper = int((tmp_path / 'helper.pid').read_text()) + time.sleep(0.2) + with pytest.raises(ProcessLookupError): + os.kill(helper, 0) + + +def test_read_only_env_disables_optional_locks(): + assert gil.read_only_env()['GIT_OPTIONAL_LOCKS'] == '0' diff --git a/test/test_update_prompts_restart.py b/test/test_update_prompts_restart.py index b4924c29..f535532b 100644 --- a/test/test_update_prompts_restart.py +++ b/test/test_update_prompts_restart.py @@ -9,6 +9,7 @@ restart is owed, and the UI raises the existing restart-pending banner. import subprocess import sys from pathlib import Path +import contextlib from unittest.mock import patch import pytest @@ -20,6 +21,13 @@ from web_interface.blueprints import api_v3 as mod # noqa: E402 from web_interface.blueprints.api_v3 import api_v3 # noqa: E402 +@contextlib.contextmanager +def _patched(fake): + """Fake git for both the plain subprocess calls and the timeout-safe runner.""" + with patch.object(mod.subprocess, 'run', fake), patch.object(mod.git_index_lock, 'run_git', fake): + yield + + @pytest.fixture def client(): app = Flask(__name__) @@ -62,7 +70,7 @@ def _pull(client): class TestRestartIsRequestedWhenCodeChanged: def test_a_pull_that_moved_head_asks_for_a_restart(self, client): - with patch.object(mod.subprocess, 'run', _git(['aaa111', 'bbb222'])): + with _patched(_git(['aaa111', 'bbb222'])): data = _pull(client) assert data['status'] == 'success' assert data['restart_required'] is True, ( @@ -70,15 +78,14 @@ class TestRestartIsRequestedWhenCodeChanged: "nothing told the user to restart") def test_already_up_to_date_does_not(self, client): - with patch.object(mod.subprocess, 'run', - _git(['aaa111', 'aaa111'], pull_out='Already up to date.\n')): + with _patched(_git(['aaa111', 'aaa111'], pull_out='Already up to date.\n')): data = _pull(client) assert data['status'] == 'success' assert data['restart_required'] is False, ( "prompting after a no-op update trains users to ignore the prompt") def test_a_failed_pull_does_not(self, client): - with patch.object(mod.subprocess, 'run', _git(['aaa111'], pull_rc=1)): + with _patched(_git(['aaa111'], pull_rc=1)): data = _pull(client) assert data['status'] == 'error' assert data['restart_required'] is False @@ -100,19 +107,18 @@ class TestUnitsAreRefreshedWithTheCode: return calls def test_an_update_that_moved_head_refreshes_the_units(self, client, refresh): - with patch.object(mod.subprocess, 'run', _git(['aaa111', 'bbb222'])): + with _patched(_git(['aaa111', 'bbb222'])): data = _pull(client) assert refresh == [True] assert data['unit_refresh']['status'] == 'refreshed' assert 'Service settings updated' in data['message'] def test_nothing_new_touches_no_units(self, client, refresh): - with patch.object(mod.subprocess, 'run', - _git(['aaa111', 'aaa111'], pull_out='Already up to date.\n')): + with _patched(_git(['aaa111', 'aaa111'], pull_out='Already up to date.\n')): data = _pull(client) assert refresh == [] and data['unit_refresh'] is None def test_a_failed_pull_touches_no_units(self, client, refresh): - with patch.object(mod.subprocess, 'run', _git(['aaa111'], pull_rc=1)): + with _patched(_git(['aaa111'], pull_rc=1)): data = _pull(client) assert refresh == [] and data['unit_refresh'] is None diff --git a/web_interface/auto_update.py b/web_interface/auto_update.py index 51d4e702..405a903a 100644 --- a/web_interface/auto_update.py +++ b/web_interface/auto_update.py @@ -302,10 +302,12 @@ def local_changes(project_dir, run=None, timeout=30): Returns the changed paths, or None when git could not tell. Raises what ``run`` raises (e.g. ``subprocess.TimeoutExpired``). """ - run = run or subprocess.run + # A status that times out must not leave index.lock: don't refresh the index. + extra = {} if run else {'env': git_index_lock.read_only_env()} + run = run or git_index_lock.run_git result = run(['git', '-c', 'core.fileMode=false', 'status', '--porcelain', '--untracked-files=no', '-z'], - cwd=str(project_dir), capture_output=True, text=True, timeout=timeout) + cwd=str(project_dir), capture_output=True, text=True, timeout=timeout, **extra) if result.returncode != 0: return None changed = [] diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index d98fe958..8998da2d 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -35,6 +35,7 @@ from typing import Dict, Any, Optional, Tuple, Type from urllib.parse import urlparse, urlunparse logger = logging.getLogger(__name__) # Import new infrastructure +from web_interface import git_index_lock from src.web_interface.api_helpers import (success_response, error_response, exception_error_response, validate_request_json) from src.web_interface.errors import ErrorCode @@ -558,7 +559,8 @@ def get_git_version(project_dir=None): capture_output=True, text=True, timeout=5, - cwd=str(project_dir) + cwd=str(project_dir), + env=git_index_lock.read_only_env() ) if result.returncode == 0: diff --git a/web_interface/blueprints/api_v3/system.py b/web_interface/blueprints/api_v3/system.py index b5c6198e..4f343825 100644 --- a/web_interface/blueprints/api_v3/system.py +++ b/web_interface/blueprints/api_v3/system.py @@ -14,6 +14,7 @@ from web_interface.blueprints.api_v3 import ( ) import threading +from web_interface import git_index_lock from web_interface.cache import get_cached, set_cached from web_interface.system_metrics import collect_system_metrics, format_uptime import web_interface.blueprints.api_v3 as _pkg @@ -163,6 +164,7 @@ def check_for_update(): ['git', 'fetch', '--quiet', '--tags', '--force', update_channel.REMOTE, update_channel.BETA_BRANCH], capture_output=True, timeout=10, cwd=cwd, + env=git_index_lock.read_only_env(), ) if fetch_result.returncode != 0: stderr = fetch_result.stderr.decode(errors='replace').strip() @@ -332,7 +334,7 @@ def channel_status(project_dir=None, fetch=True): def _perform_core_update_locked(stash_local_changes=True): project_dir = str(PROJECT_ROOT) - from web_interface import update_channel, git_index_lock + from web_interface import update_channel # A git killed mid-command (a timed-out pull, a power cut) leaves # .git/index.lock behind, and every later update then fails with @@ -343,6 +345,14 @@ def _perform_core_update_locked(stash_local_changes=True): 'message': 'Another git command is running in the LEDMatrix folder; ' 'try again in a minute.'} + # A pull killed mid-rebase leaves .git/rebase-merge behind, and every + # later `pull --rebase` then fails. Nothing is running (checked above). + git_dir = Path(project_dir) / '.git' + if any((git_dir / name).exists() for name in ('rebase-merge', 'rebase-apply')): + logger.warning("aborting a rebase left by an interrupted update") + git_index_lock.run_git(['git', 'rebase', '--abort'], capture_output=True, text=True, + timeout=60, cwd=project_dir) + # Which code to move to: the newest release (stable) or main (beta). # See web_interface/update_channel.py; it never picks an older commit. channel, fetch_error = channel_status(project_dir) @@ -434,25 +444,33 @@ def _perform_core_update_locked(stash_local_changes=True): # Stable checks out the release tag, carrying edits across the way the # pull's --autostash does. channel_note = '' - if action == update_channel.ACTION_CHECKOUT_TAG: - result, autostash_note = update_channel.checkout_release(project_dir, channel.newest_release) - channel_note = f"Now on release {channel.newest_release} (stable channel). {autostash_note}".strip() - elif action == update_channel.ACTION_SWITCH_TO_BETA: - result, autostash_note = update_channel.checkout_beta_branch(project_dir) - if result.returncode == 0: - result = subprocess.run(['git', 'pull', '--rebase', '--autostash'], - capture_output=True, text=True, timeout=60, cwd=project_dir) - channel_note = f"Now following {update_channel.BETA_BRANCH} (beta channel). {autostash_note}".strip() - else: - result = subprocess.run( - pull_args, - capture_output=True, - text=True, - timeout=60, - cwd=project_dir - ) - if channel.waiting and channel.newest_release: - channel_note = channel.message + try: + if action == update_channel.ACTION_CHECKOUT_TAG: + result, autostash_note = update_channel.checkout_release(project_dir, channel.newest_release) + channel_note = f"Now on release {channel.newest_release} (stable channel). {autostash_note}".strip() + elif action == update_channel.ACTION_SWITCH_TO_BETA: + result, autostash_note = update_channel.checkout_beta_branch(project_dir) + if result.returncode == 0: + result = git_index_lock.run_git(['git', 'pull', '--rebase', '--autostash'], + capture_output=True, text=True, + timeout=git_index_lock.MUTATING_TIMEOUT_SECONDS, cwd=project_dir) + channel_note = f"Now following {update_channel.BETA_BRANCH} (beta channel). {autostash_note}".strip() + else: + result = git_index_lock.run_git( + pull_args, + capture_output=True, + text=True, + timeout=git_index_lock.MUTATING_TIMEOUT_SECONDS, + cwd=project_dir + ) + if channel.waiting and channel.newest_release: + channel_note = channel.message + except subprocess.TimeoutExpired: + # run_git has stopped git and removed the lock it left. + logger.warning("git timed out during Update Code") + return {'status': 'error', 'restart_required': False, 'dependency_failures': [], + 'message': 'Update failed: git took too long (a slow network or SD card). ' + 'The partial update was cleaned up; try again.'} # Give the branch tracking information so the next pull is a plain # `git pull` — otherwise every update repeats the fallback. diff --git a/web_interface/git_index_lock.py b/web_interface/git_index_lock.py index 8ccaa05e..8ad5528d 100644 --- a/web_interface/git_index_lock.py +++ b/web_interface/git_index_lock.py @@ -18,6 +18,8 @@ the file alone. Standard library only. """ import logging import os +import signal +import subprocess import time from pathlib import Path @@ -33,6 +35,15 @@ REMOVED = 'removed' # it was stale and has been removed IN_USE = 'in_use' # a git process is running here (or the lock is brand new) UNKNOWN = 'unknown' # could not tell whether git is running; left alone +#: How long a timed-out git gets to exit after SIGTERM (which lets it remove +#: its own lock) before the whole process group is SIGKILLed. +TERM_GRACE_SECONDS = 5 + +#: Timeout for commands that rewrite the checkout (pull, checkout, reset, +#: stash). A Pi on an SD card can take minutes; killing git is what leaves the +#: lock behind, so this is generous. +MUTATING_TIMEOUT_SECONDS = 600 + _PROC = Path('/proc') @@ -137,7 +148,59 @@ def clear_stale_index_lock(project_dir, proc_root=_PROC, now=None): return REMOVED +def _stop_group(proc): + """SIGTERM the process group, then SIGKILL whatever is left of it.""" + try: + os.killpg(proc.pid, signal.SIGTERM) + except (ProcessLookupError, PermissionError): + pass + try: + proc.wait(timeout=TERM_GRACE_SECONDS) + except subprocess.TimeoutExpired: + pass + # Also reaps git's children (git-remote-https, rebase helpers) that + # outlive a killed parent. + try: + os.killpg(proc.pid, signal.SIGKILL) + except (ProcessLookupError, PermissionError): + pass + + +def run_git(cmd, *, cwd=None, timeout=None, capture_output=True, text=True, **kwargs): + """``subprocess.run`` for git, without leaving ``.git/index.lock`` behind. + + ``subprocess.run`` SIGKILLs only the direct child when ``timeout`` expires, + which skips git's cleanup and orphans its helpers. This starts git in its + own session, stops the whole group with SIGTERM before SIGKILL, removes a + lock nothing owns any more, then raises ``TimeoutExpired`` as + ``subprocess.run`` would. + """ + if capture_output: + kwargs['stdout'] = kwargs['stderr'] = subprocess.PIPE + with subprocess.Popen(cmd, cwd=cwd, text=text, start_new_session=True, **kwargs) as proc: + try: + stdout, stderr = proc.communicate(timeout=timeout) + except subprocess.TimeoutExpired as exc: + _stop_group(proc) + exc.stdout, exc.stderr = proc.communicate() + if cwd is not None: + clear_stale_index_lock(cwd) + raise + except BaseException: + _stop_group(proc) + raise + return subprocess.CompletedProcess(proc.args, proc.returncode, stdout, stderr) + + +def read_only_env(): + """Environment for git calls that only read: ``GIT_OPTIONAL_LOCKS=0`` + stops ``status`` and ``describe --dirty`` refreshing (and so locking) the + index, so a poll that times out cannot leave a lock.""" + return {**os.environ, 'GIT_OPTIONAL_LOCKS': '0'} + + def manual_fix_hint(project_dir): """The sentence shown when a lock blocks the update and was not removed.""" - return (f"If no other git command is running, remove the stale lock with " - f"rm -f {Path(project_dir) / '.git' / 'index.lock'} and try again.") + lock = Path(project_dir) / '.git' / 'index.lock' + return (f"If it keeps coming back, stop the LEDMatrix services, kill any running git " + f"(ps aux | grep git), then remove it with rm -f {lock} and try again.") diff --git a/web_interface/update_channel.py b/web_interface/update_channel.py index f7fdb3f2..597bed1a 100644 --- a/web_interface/update_channel.py +++ b/web_interface/update_channel.py @@ -24,6 +24,8 @@ import re import subprocess # nosec B404 - list-form argv only, no shell # nosemgrep from pathlib import Path +from web_interface import git_index_lock + PROJECT_ROOT = Path(__file__).resolve().parent.parent CHANNELS = ('stable', 'beta') DEFAULT_CHANNEL = 'stable' @@ -105,7 +107,7 @@ class ChannelStatus(dict): def _git(project_dir, run, *args, timeout=30): - run = run or subprocess.run + run = run or git_index_lock.run_git return run(['git', *args], cwd=str(project_dir), capture_output=True, text=True, timeout=timeout)