mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-30 09:49:20 +00:00
fix(update): stop a timed-out git from leaving .git/index.lock behind (#802)
* fix(update): stop a timed-out git from leaving .git/index.lock behind subprocess.run's timeout SIGKILLs only the direct git child, so the lock is left behind and git's helpers are orphaned. On a slow SD card the same pull then times out on every retry and recreates the lock, so removing it by hand does not help. - run_git starts git in its own session, stops the group with SIGTERM before SIGKILL, and clears the lock if nothing owns it. Update Code's pull, checkout, reset and stash go through it, with a 600s pull timeout. - Read-only calls (describe --dirty, status, the update-check fetch) set GIT_OPTIONAL_LOCKS=0 so a timed-out poll cannot take the lock. - A timed-out pull returns a clear error; a rebase left by an interrupted update is aborted before pulling. - The manual-fix hint says to stop the services and kill running git. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * 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> * 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> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
Chuck
co-authored by
Claude Sonnet 5.5
Claude Sonnet 5.5
parent
fa51ebab5f
commit
237d611b89
+128
-3
@@ -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'
|
||||
|
||||
Reference in New Issue
Block a user