chore(update): mark run_git's CompletedProcess as a result object, not a subprocess call

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

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'

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,125 @@ 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'
+
+
+def test_run_git_waits_out_the_freshness_window_before_giving_up_on_the_lock(tmp_path, monkeypatch):
+    # The lock is created a moment before the timeout, so on age alone it is
+    # still "in use" once git is stopped; it must not be left to block the retry.
+    (tmp_path / '.git').mkdir()
+    lock = tmp_path / '.git' / 'index.lock'
+    script = tmp_path / 'slow.sh'
+    script.write_text(f'#!/bin/sh\n: > "{lock}"\nexec sleep 60\n')
+    script.chmod(0o755)
+    monkeypatch.setattr(gil, 'MIN_STALE_AGE_SECONDS', 2)
+    monkeypatch.setattr(gil, 'TERM_GRACE_SECONDS', 1)
+    with pytest.raises(subprocess.TimeoutExpired):
+        gil.run_git([str(script)], cwd=str(tmp_path), timeout=1)
+    assert not lock.exists()
+
+
+# -- a rebase in the checkout ----------------------------------------------------
+
+def _rebase_checkout(tmp_path, monkeypatch):
+    from web_interface.blueprints.api_v3 import system
+    (tmp_path / '.git' / 'rebase-merge').mkdir(parents=True)
+    monkeypatch.setattr(system, 'PROJECT_ROOT', tmp_path)
+    return tmp_path / '.git' / 'rebase-merge'
+
+
+def _rebase_git(calls, on_branch, quit_rc=0, remove_dir=None):
+    base = _fake_git(calls)
+
+    def run(args, **kwargs):
+        if args[:2] == ['git', 'symbolic-ref']:
+            calls.append(list(args))
+            return subprocess.CompletedProcess(args, 0 if on_branch else 1, stdout='', stderr='')
+        if args[:3] == ['git', 'rebase', '--quit']:
+            calls.append(list(args))
+            if quit_rc == 0 and remove_dir is not None:
+                remove_dir.rmdir()
+            return subprocess.CompletedProcess(args, quit_rc, stdout='', stderr='cannot quit' if quit_rc else '')
+        return base(args, **kwargs)
+    return run
+
+
+def test_a_rebase_in_progress_is_left_alone(client, tmp_path, monkeypatch):
+    _rebase_checkout(tmp_path, monkeypatch)
+    calls = []
+    with _patched(_rebase_git(calls, on_branch=False)):
+        data = _update(client)
+    assert data['status'] == 'error' and 'rebase is in progress' in data['message']
+    assert not any(c[:2] in (['git', 'pull'], ['git', 'checkout']) or c[:3] == ['git', 'rebase', '--quit']
+                   for c in calls), "a paused rebase holds the user's conflict work"
+
+
+def test_an_abandoned_rebase_is_quit_before_the_pull(client, tmp_path, monkeypatch):
+    rebase = _rebase_checkout(tmp_path, monkeypatch)
+    calls = []
+    with _patched(_rebase_git(calls, on_branch=True, remove_dir=rebase)):
+        _update(client)
+    names = [c[1] for c in calls]
+    assert ['git', 'rebase', '--quit'] in calls
+    assert names.index('rebase') < names.index('pull')
+
+
+def test_a_rebase_that_cannot_be_cleared_stops_the_update(client, tmp_path, monkeypatch):
+    _rebase_checkout(tmp_path, monkeypatch)
+    calls = []
+    with _patched(_rebase_git(calls, on_branch=True, quit_rc=1)):
+        data = _update(client)
+    assert data['status'] == 'error' and 'could not be cleared' in data['message']
+    assert not any(c[:2] == ['git', 'pull'] for c in calls)
+
+
+def test_the_automatic_status_check_is_read_only_whatever_the_runner(tmp_path):
+    from web_interface import auto_update
+    seen = {}
+
+    def run(args, **kwargs):
+        seen.update(kwargs)
+        return subprocess.CompletedProcess(args, 0, stdout='', stderr='')
+    auto_update.local_changes(tmp_path, run=run)
+    assert seen['env']['GIT_OPTIONAL_LOCKS'] == '0'

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

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.
+    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,
+                 env=git_index_lock.read_only_env())
     if result.returncode != 0:
         return None
     changed = []

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:

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,28 @@ 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. Same rule as the automatic update's
+    # preflight: a rebase really in progress leaves HEAD detached and holds
+    # the user's conflict work, so leave it; with HEAD back on a branch it was
+    # abandoned, and `--quit` drops its state without touching the tree.
+    git_dir = Path(project_dir) / '.git'
+    rebase_dirs = [git_dir / name for name in ('rebase-merge', 'rebase-apply') if (git_dir / name).exists()]
+    if rebase_dirs:
+        def _git(*args):
+            return git_index_lock.run_git(['git', *args], capture_output=True, text=True,
+                                          timeout=60, cwd=project_dir)
+        if _git('symbolic-ref', '-q', 'HEAD').returncode != 0:
+            return {'status': 'error', 'restart_required': False, 'dependency_failures': [],
+                    'message': 'A git rebase is in progress in the LEDMatrix folder. '
+                               'Finish or abort it (git rebase --continue / --abort), then update again.'}
+        cleared = _git('rebase', '--quit')
+        if cleared.returncode != 0 or any(path.exists() for path in rebase_dirs):
+            return {'status': 'error', 'restart_required': False, 'dependency_failures': [],
+                    'message': 'An abandoned git rebase in the LEDMatrix folder could not be cleared: '
+                               f'{(cleared.stderr or "").strip() or "unknown error"}.'}
+        logger.warning("Cleared an abandoned git rebase from %s", ', '.join(str(p) for p in rebase_dirs))
+
     # 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 +458,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 = ''
+    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 = subprocess.run(['git', 'pull', '--rebase', '--autostash'],
-                                    capture_output=True, text=True, timeout=60, cwd=project_dir)
+                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 = subprocess.run(
+            result = git_index_lock.run_git(
                 pull_args,
                 capture_output=True,
                 text=True,
-            timeout=60,
+                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.

web_interface/git_index_lock.py

@@ -18,6 +18,8 @@ the file alone. Standard library only.
 """
 import logging
 import os
+import signal
+import subprocess  # nosec B404 - list-form argv only, no shell  # nosemgrep
 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,73 @@ 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 _clear_after_stop(project_dir):
+    """Remove the lock a stopped git left, waiting out the freshness window.
+
+    A lock created within ``MIN_STALE_AGE_SECONDS`` of the stop reads as IN_USE
+    on age alone; with no git process left it only needs to age, so wait for
+    that rather than leave it to block the next attempt.
+    """
+    deadline = time.monotonic() + MIN_STALE_AGE_SECONDS + 1
+    while clear_stale_index_lock(project_dir) == IN_USE and git_running_in(project_dir) is False:
+        if time.monotonic() >= deadline:
+            break
+        time.sleep(0.5)
+
+
+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:  # nosec B603 - list-form argv  # nosemgrep
+        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_after_stop(cwd)
+            raise
+        except BaseException:
+            _stop_group(proc)
+            raise
+    return subprocess.CompletedProcess(proc.args, proc.returncode, stdout, stderr)  # result object only, nothing is run  # nosemgrep
+
+
+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.")

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)