diff --git a/test/test_git_index_lock.py b/test/test_git_index_lock.py index a60cd94d..0233d169 100644 --- a/test/test_git_index_lock.py +++ b/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' diff --git a/web_interface/auto_update.py b/web_interface/auto_update.py index 405a903a..35ce1b6f 100644 --- a/web_interface/auto_update.py +++ b/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 = [] diff --git a/web_interface/blueprints/api_v3/system.py b/web_interface/blueprints/api_v3/system.py index 4f343825..eb07ec98 100644 --- a/web_interface/blueprints/api_v3/system.py +++ b/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. diff --git a/web_interface/git_index_lock.py b/web_interface/git_index_lock.py index 8ad5528d..e56c8976 100644 --- a/web_interface/git_index_lock.py +++ b/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)