fix: detect and recover from stale git lock files during self-update
- Detect lock file errors (lock_conflict flag) on fetch/status/pull failures in _apply_update_inner (#1) - Expose Force Update button on frontend when lock_conflict is received (#3) - Proactively clean stale lock files (>30s old) in apply_force_update using rglob discovery instead of a hardcoded path list (#2) - Add 10 tests: _is_git_lock_error parametrized unit tests, _apply_update_inner lock path coverage, and apply_force_update stale/recent lock behavior
This commit is contained in:
1 parent
426f12ce51
commit
3c5b00f928
3 files changed
+208
-1
No files matched your search
@@ -214,6 +214,21 @@ def _run_git(args, cwd, timeout=10):
|
||||
return f'git failed to start: {exc}', False
|
||||
|
||||
|
||||
_GIT_LOCK_SIGNATURES = (
|
||||
"index.lock': file exists",
|
||||
"another git process seems to be running",
|
||||
".lock': file exists",
|
||||
"lock file",
|
||||
)
|
||||
|
||||
|
||||
def _is_git_lock_error(output: str) -> bool:
|
||||
if not output:
|
||||
return False
|
||||
lower_out = output.lower()
|
||||
return any(sig in lower_out for sig in _GIT_LOCK_SIGNATURES)
|
||||
|
||||
|
||||
def _windows_git_from_registry():
|
||||
"""Best-effort resolve git.exe from the Git-for-Windows registry key.
|
||||
|
||||
@@ -1363,6 +1378,28 @@ def apply_force_update(target: str) -> dict:
|
||||
if path is None or not (path / '.git').exists():
|
||||
return {'ok': False, 'message': 'Not a git repository'}
|
||||
|
||||
# Clean up stale git lock files that are likely orphaned (older than
|
||||
# 30 seconds). Live locks held by a concurrent git process are recent,
|
||||
# so we skip those to avoid corrupting an active operation.
|
||||
_LOCK_STALE_SECS = 30
|
||||
git_dir = path / '.git'
|
||||
if git_dir.exists():
|
||||
for lock_path in git_dir.rglob('*.lock'):
|
||||
if not lock_path.is_file():
|
||||
continue
|
||||
try:
|
||||
age = time.time() - lock_path.stat().st_mtime
|
||||
except OSError:
|
||||
continue
|
||||
if age < _LOCK_STALE_SECS:
|
||||
logger.warning(f"Skipping recent lock file (age={age:.1f}s): {lock_path}")
|
||||
continue
|
||||
try:
|
||||
os.remove(lock_path)
|
||||
logger.info(f"Removed stale lock file (age={age:.1f}s): {lock_path}")
|
||||
except OSError as e:
|
||||
logger.error(f"Failed to remove stale lock file {lock_path}: {e}")
|
||||
|
||||
# --force so a remote re-tag (e.g. squash-merge that re-points an
|
||||
# existing release tag) doesn't jam the apply path with "would clobber
|
||||
# existing tag". See #2756.
|
||||
@@ -1460,6 +1497,12 @@ def _apply_update_inner(target):
|
||||
# --force so a remote re-tag doesn't block the update path (see #2756).
|
||||
fetch_out, fetch_ok = _run_git(['fetch', 'origin', '--quiet', '--tags', '--force'], path, timeout=15)
|
||||
if not fetch_ok:
|
||||
if _is_git_lock_error(fetch_out):
|
||||
return {
|
||||
'ok': False,
|
||||
'message': f'Fetch failed due to a repository lock: {fetch_out.strip()}',
|
||||
'lock_conflict': True,
|
||||
}
|
||||
return {
|
||||
'ok': False,
|
||||
'message': _apply_fetch_failure_message(
|
||||
@@ -1476,6 +1519,12 @@ def _apply_update_inner(target):
|
||||
['status', '--porcelain', '--untracked-files=no'], path
|
||||
)
|
||||
if not status_ok:
|
||||
if _is_git_lock_error(status_out):
|
||||
return {
|
||||
'ok': False,
|
||||
'message': f'Failed to inspect repo status due to a repository lock: {status_out.strip()}',
|
||||
'lock_conflict': True,
|
||||
}
|
||||
return {'ok': False, 'message': f'Failed to inspect repo status: {status_out[:200]}'}
|
||||
# Fail early on unresolved merge conflicts
|
||||
if any(line[:2] in {'DD', 'AU', 'UD', 'UA', 'DU', 'AA', 'UU'}
|
||||
@@ -1508,6 +1557,12 @@ def _apply_update_inner(target):
|
||||
pull_args.extend(['origin', compare_ref])
|
||||
pull_out, pull_ok = _run_git(pull_args, path, timeout=30)
|
||||
if not pull_ok:
|
||||
if _is_git_lock_error(pull_out):
|
||||
return {
|
||||
'ok': False,
|
||||
'message': f'Pull failed due to a repository lock: {pull_out.strip()}',
|
||||
'lock_conflict': True,
|
||||
}
|
||||
pull_lower = pull_out.lower()
|
||||
detail = pull_out.strip()[:300] if pull_out.strip() else '(no output from git)'
|
||||
untracked_collision = (
|
||||
|
||||
+1
-1
@@ -9100,7 +9100,7 @@ function _showUpdateError(target,res){
|
||||
showToast(msg);
|
||||
}
|
||||
// Show "Force update" button when the error is recoverable by a hard reset
|
||||
if(forceBtn&&(res.conflict||res.diverged)){
|
||||
if(forceBtn&&(res.conflict||res.diverged||res.lock_conflict)){
|
||||
forceBtn.dataset.target=target;
|
||||
forceBtn.style.display='inline-block';
|
||||
}
|
||||
|
||||
@@ -1,6 +1,10 @@
|
||||
"""Tests for self-update diagnostics (api/updates.py)."""
|
||||
import os
|
||||
import time
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
import api.updates as updates
|
||||
|
||||
|
||||
@@ -893,3 +897,151 @@ def test_select_apply_compare_ref_falls_through_when_latest_tag_is_not_ff_reacha
|
||||
|
||||
assert ref == 'origin/main'
|
||||
|
||||
|
||||
# ── _is_git_lock_error unit tests ───────────────────────────────────────────
|
||||
|
||||
|
||||
@pytest.mark.parametrize('output', [
|
||||
"fatal: Unable to create '/app/.git/index.lock': File exists.",
|
||||
"fatal: Unable to create '.git/index.lock': File exists.",
|
||||
"another git process seems to be running in this repository",
|
||||
"fatal: Unable to create '.git/FETCH_HEAD.lock': File exists.",
|
||||
"fatal: Unable to create '.git/refs/heads/main.lock': File exists.",
|
||||
"could not open lock file",
|
||||
"error: something about a lock file here",
|
||||
])
|
||||
def test_is_git_lock_error_detects_lock_error(output):
|
||||
assert updates._is_git_lock_error(output) is True
|
||||
|
||||
|
||||
@pytest.mark.parametrize('output', [
|
||||
'',
|
||||
None,
|
||||
"fatal: cannot lock ref 'refs/tags/v0.51.106': is at 123 but expected 456",
|
||||
"fatal: unable to access 'https://github.com/nesquena/hermes-webui.git/': Could not resolve host",
|
||||
"fatal: Not a git repository",
|
||||
"error: failed to push some refs",
|
||||
])
|
||||
def test_is_git_lock_error_returns_false_for_non_lock(output):
|
||||
"""Non-lock git errors must NOT be classified as lock_conflict."""
|
||||
assert updates._is_git_lock_error(output) is False
|
||||
|
||||
|
||||
# ── _apply_update_inner lock detection tests ────────────────────────────────
|
||||
|
||||
|
||||
_MODULE = 'api.updates'
|
||||
|
||||
|
||||
def _assert_lock_conflict_result(result):
|
||||
assert result['ok'] is False
|
||||
assert result.get('lock_conflict') is True
|
||||
assert 'repository lock' in result['message']
|
||||
|
||||
|
||||
def test_apply_update_fetch_lock_error_returns_lock_conflict(tmp_path):
|
||||
"""Fetch failure caused by .git/index.lock returns lock_conflict: True."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
from api import updates as mod
|
||||
with patch(f'{_MODULE}.REPO_ROOT', tmp_path), \
|
||||
patch(f'{_MODULE}._run_git') as mock_run_git:
|
||||
mock_run_git.side_effect = [
|
||||
("fatal: Unable to create '/app/.git/index.lock': File exists.", False),
|
||||
]
|
||||
result = mod._apply_update_inner('webui')
|
||||
_assert_lock_conflict_result(result)
|
||||
|
||||
|
||||
def test_apply_update_fetch_lock_error_does_not_attempt_pull(tmp_path):
|
||||
"""If fetch fails with a lock error, no further git calls are made."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
from api import updates as mod
|
||||
with patch(f'{_MODULE}.REPO_ROOT', tmp_path), \
|
||||
patch(f'{_MODULE}._run_git') as mock_run_git:
|
||||
mock_run_git.side_effect = [
|
||||
("fatal: Unable to create '.git/index.lock': File exists.", False),
|
||||
]
|
||||
mod._apply_update_inner('webui')
|
||||
assert mock_run_git.call_count == 1
|
||||
|
||||
|
||||
def test_apply_update_status_lock_error_returns_lock_conflict(tmp_path):
|
||||
"""Status failure caused by .git/index.lock returns lock_conflict: True."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
from api import updates as mod
|
||||
with patch(f'{_MODULE}.REPO_ROOT', tmp_path), \
|
||||
patch(f'{_MODULE}._select_apply_compare_ref', return_value='origin/main'), \
|
||||
patch(f'{_MODULE}._run_git') as mock_run_git:
|
||||
mock_run_git.side_effect = [
|
||||
('', True), # fetch succeeds
|
||||
("fatal: Unable to create '.git/index.lock': File exists.", False), # status fails
|
||||
]
|
||||
result = mod._apply_update_inner('webui')
|
||||
_assert_lock_conflict_result(result)
|
||||
|
||||
|
||||
def test_apply_update_pull_lock_error_returns_lock_conflict(tmp_path):
|
||||
"""Pull failure caused by .git/index.lock returns lock_conflict: True."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
from api import updates as mod
|
||||
with patch(f'{_MODULE}.REPO_ROOT', tmp_path), \
|
||||
patch(f'{_MODULE}._select_apply_compare_ref', return_value='origin/main'), \
|
||||
patch(f'{_MODULE}.STREAMS', {}), \
|
||||
patch(f'{_MODULE}._run_git') as mock_run_git:
|
||||
mock_run_git.side_effect = [
|
||||
('', True), # fetch succeeds
|
||||
('', True), # status --porcelain (clean)
|
||||
("fatal: Unable to create '.git/index.lock': File exists.", False), # pull fails
|
||||
]
|
||||
result = mod._apply_update_inner('webui')
|
||||
_assert_lock_conflict_result(result)
|
||||
|
||||
|
||||
def test_apply_update_non_lock_fetch_failure_does_not_include_lock_conflict(tmp_path):
|
||||
"""A non-lock fetch failure does NOT return lock_conflict."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
from api import updates as mod
|
||||
with patch(f'{_MODULE}.REPO_ROOT', tmp_path), \
|
||||
patch(f'{_MODULE}._run_git') as mock_run_git:
|
||||
mock_run_git.side_effect = [
|
||||
("fatal: unable to access 'https://github.com/repo.git/': Could not resolve host", False),
|
||||
]
|
||||
result = mod._apply_update_inner('webui')
|
||||
assert result['ok'] is False
|
||||
assert result.get('lock_conflict') is None
|
||||
|
||||
|
||||
# ── apply_force_update lock cleanup tests ────────────────────────────────────
|
||||
|
||||
|
||||
def test_apply_force_update_removes_stale_lock_files(tmp_path):
|
||||
"""apply_force_update removes lock files older than 30 seconds."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
stale_lock = tmp_path / '.git' / 'index.lock'
|
||||
stale_lock.write_text('')
|
||||
old_mtime = time.time() - 60
|
||||
os.utime(stale_lock, (old_mtime, old_mtime))
|
||||
|
||||
with patch.object(updates, '_run_git', return_value=('', True)), \
|
||||
patch.object(updates, 'REPO_ROOT', tmp_path), \
|
||||
patch.object(updates, '_restart_blocker_snapshot', return_value={'restart_blocked': False, 'active_streams': 0, 'active_runs': 0}):
|
||||
updates.apply_force_update('webui')
|
||||
|
||||
assert not stale_lock.exists(), "Stale lock file should have been removed"
|
||||
|
||||
|
||||
def test_apply_force_update_skips_recent_lock_file(tmp_path):
|
||||
"""apply_force_update does NOT remove lock files younger than 30 seconds."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
recent_lock = tmp_path / '.git' / 'index.lock'
|
||||
recent_lock.write_text('')
|
||||
recent_mtime = time.time() - 5
|
||||
os.utime(recent_lock, (recent_mtime, recent_mtime))
|
||||
|
||||
with patch.object(updates, '_run_git', return_value=('', True)), \
|
||||
patch.object(updates, 'REPO_ROOT', tmp_path), \
|
||||
patch.object(updates, '_restart_blocker_snapshot', return_value={'restart_blocked': False, 'active_streams': 0, 'active_runs': 0}):
|
||||
updates.apply_force_update('webui')
|
||||
|
||||
assert recent_lock.exists(), "Recent lock file should NOT have been removed"
|
||||
|
||||
Reference in new issue
Block a user