fix(update): address review of the index.lock fix

- The automatic update's status check now sets GIT_OPTIONAL_LOCKS=0
  whichever runner it is given, not only the default one.
- Update Code no longer runs `git rebase --abort` on any rebase dir: a
  rebase in progress (detached HEAD) holds the user's conflict work and
  blocks the update; an abandoned one is dropped with `rebase --quit`,
  as the automatic update does, and a failure to clear it stops the update.
- run_git waits out the lock-freshness window after stopping a timed-out
  git, so a lock created just before the timeout is not left behind.
- Mark the Popen/subprocess use with the repo's nosec comments.

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

test/test_git_index_lock.py

@@ -243,3 +243,83 @@ def test_run_git_timeout_stops_the_group_and_clears_the_lock(tmp_path, monkeypat
 
 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'

web_interface/auto_update.py

@@ -303,11 +303,11 @@ def local_changes(project_dir, run=None, timeout=30):
     ``run`` raises (e.g. ``subprocess.TimeoutExpired``).
     """
     # 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, **extra)
+                 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/system.py

@@ -346,12 +346,26 @@ def _perform_core_update_locked(stash_local_changes=True):
                            '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).
+    # 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'
-    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)
+    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.

web_interface/git_index_lock.py

@@ -19,7 +19,7 @@ the file alone. Standard library only.
 import logging
 import os
 import signal
-import subprocess
+import subprocess  # nosec B404 - list-form argv only, no shell  # nosemgrep
 import time
 from pathlib import Path
 
@@ -166,6 +166,20 @@ def _stop_group(proc):
         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.
 
@@ -177,14 +191,14 @@ def run_git(cmd, *, cwd=None, timeout=None, capture_output=True, text=True, **kw
     """
     if capture_output:
         kwargs['stdout'] = kwargs['stderr'] = subprocess.PIPE
-    with subprocess.Popen(cmd, cwd=cwd, text=text, start_new_session=True, **kwargs) as proc:
+    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_stale_index_lock(cwd)
+                _clear_after_stop(cwd)
             raise
         except BaseException:
             _stop_group(proc)