From 3c5b00f928c3f89966f800f2b19e7ecfdfcdd293 Mon Sep 17 00:00:00 2001 From: b3nw Date: Mon, 6 Jul 2026 01:22:45 +0000 Subject: [PATCH 1/5] 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 --- api/updates.py | 55 +++++++++++++++ static/ui.js | 2 +- tests/test_updates.py | 152 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+), 1 deletion(-) diff --git a/api/updates.py b/api/updates.py index d9c91d69..6d2ee582 100644 --- a/api/updates.py +++ b/api/updates.py @@ -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 = ( diff --git a/static/ui.js b/static/ui.js index 352caf47..4deba79b 100644 --- a/static/ui.js +++ b/static/ui.js @@ -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'; } diff --git a/tests/test_updates.py b/tests/test_updates.py index 083ef23e..c6898f48 100644 --- a/tests/test_updates.py +++ b/tests/test_updates.py @@ -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" + From e8f7e28a3ccd720d62302a0c8e3d2528c19076d6 Mon Sep 17 00:00:00 2001 From: b3nw Date: Mon, 6 Jul 2026 17:37:05 +0000 Subject: [PATCH 2/5] fix: address gate cert BRICKs + Greptile P1 on PR #5688 Address the RED gate cert on PR #5688 (2 BRICK + 1 CORE) and the incoming Greptile review (1 P1 + 3 P2): - BRICK-1 (race-safety): drop the mtime heuristic and any age-based lock removal. Add /api/updates/clear_lock with a fail-closed fcntl.flock holder probe (refuses if any process still holds the file, on POSIX). On non-POSIX, fails closed. Touches only .git/index.lock (the only well-known short-lived lock git creates). - BRICK-2 (destructive path coupling): remove lock_conflict from the force-button condition. Add a separate btnClearUpdateLock that calls /api/updates/clear_lock -- never apply_force_update. The recovery path never runs checkout/clean/reset --hard. - CORE-1 (unconditional pre-cleanup): apply_force_update no longer iterates .git/**/*.lock. Lock cleanup is exclusively the clear_lock endpoint's job. - Greptile P1: when a pull-lock error fires after a stash was pushed, run git stash pop (with apply+drop fallback) so the user's local modifications aren't stranded in the stash silently. v2 tests cover the stashed and the clean cases. - Greptile P2 (broad 'lock file' substring): tightened _GIT_LOCK_SIGNATURES to specific git error strings ('index.lock': file exists, '.lock': file exists, 'another git process seems to be running', 'unable to create .git/index.lock') so unrelated ref-transaction lock-loss messages no longer trigger a lock-conflict response. - Greptile P2 (redundant git_dir.exists / magic number): both gone as a side effect of removing the apply_force_update cleanup loop entirely. Refactor: rebased onto current upstream/master so PR is no longer 'behind' 130 commits. Tests: - 14 new v2 tests covering holder probe (true/false/NOGIL probe), _try_remove_lock (refuse-on-held/success-on-unheld), apply_clear_lock (success/refused), signature set parameter table including false-positive class, apply_force_update no-touch contract, pull-lock stash restore, pull-lock no-stash-when-clean. - 2 v1 tests removed (they encoded the unsafe behavior). - 2 v1 parametrize rows dropped (their positive cases relied on the broad 'lock file' substring). Closes #5687 --- api/routes.py | 14 +++ api/updates.py | 273 +++++++++++++++++++++++++++++++++++++----- static/ui.js | 48 +++++++- tests/test_updates.py | 244 +++++++++++++++++++++++++++++++++---- 4 files changed, 523 insertions(+), 56 deletions(-) diff --git a/api/routes.py b/api/routes.py index c3ab303c..0fe2eac9 100644 --- a/api/routes.py +++ b/api/routes.py @@ -15043,6 +15043,20 @@ def handle_post(handler, parsed) -> bool: return j(handler, apply_force_update(target)) + if parsed.path == "/api/updates/clear_lock": + # Non-destructive recovery for the stale .git/index.lock case: tries + # to clear the well-known short-lived git index lock and re-runs the + # normal (non-destructive) update flow. Refuses to remove locks that + # another process still holds (see apply_clear_lock for the holder + # probe). This is the *only* path on which git locks are removed; + # /api/updates/apply and /api/updates/force do not touch locks. + target = body.get("target", "") + if target not in ("webui", "agent"): + return bad(handler, 'target must be "webui" or "agent"') + from api.updates import apply_clear_lock + + return j(handler, apply_clear_lock(target)) + if parsed.path == "/api/updates/summary": from api.updates import summarize_update_payload diff --git a/api/updates.py b/api/updates.py index 6d2ee582..b708ff15 100644 --- a/api/updates.py +++ b/api/updates.py @@ -58,6 +58,27 @@ _FETCH_NETWORK_FAILURE_SIGNATURES = ( 'tls connection was non-properly terminated', 'ssl certificate problem', ) +# Phrases git emits when its own short-lived index/refs lock files block a +# subsequent operation. Tuned to match only the true "lock file already exists" +# semantics that warrant a lock-conflict response -- v2 deliberately drops the +# broad "lock file" substring from the prior version to avoid false positives +# on unrelated errors like "lock file lost during ref transaction". +# Matched case-insensitively in _is_git_lock_error(). +_GIT_LOCK_SIGNATURES = ( + "index.lock': file exists", + ".lock': file exists", + 'another git process seems to be running', + 'unable to create .git/index.lock', +) +# Lock files we are willing to remove on the *explicit user request* path +# (`/api/updates/clear_lock`). The clear_lock endpoint refuses to remove any +# lock whose underlying file appears to still be held by another process -- +# there is no mtime-based heuristic any more (BRICK-1 from PR #5688 gate +# cert: a live `git add` was empirically shown to hold .git/index.lock past +# 31 s with unchanged mtime). +_GIT_LOCK_FILES_REMOVABLE = ( + 'index.lock', +) def _sanitize_git_diagnostic(output: str, *, limit: int = _GIT_DIAGNOSTIC_MAX_CHARS) -> str: @@ -214,14 +235,6 @@ 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 @@ -229,6 +242,172 @@ def _is_git_lock_error(output: str) -> bool: return any(sig in lower_out for sig in _GIT_LOCK_SIGNATURES) +def _is_lock_held(lock_path: Path) -> bool: + """Return True if a process appears to still hold ``lock_path``. + + Used by the explicit `clear_lock` recovery path. The implementer is + intentionally conservative: a process hold signal returns True and the + caller refuses to remove the lock. If we cannot prove a holder exists + *and cannot prove one does not*, we still return True (fail-closed) on + the common case the OS denies `flock`/delete. + + Currently: tries a non-blocking exclusive fcntl.flock on POSIX. Any + `BlockingIOError` means another process holds the lock. On platforms + without fcntl (Windows), returns True unconditionally so the endpoint + surfaces a manual-instruction message rather than risking corruption. + """ + if not lock_path.exists(): + return False + try: + import fcntl # local import keeps Windows imports happy + except ImportError: + # No fcntl on this platform -- cannot prove non-hold. Fail closed. + return True + try: + fd = os.open(str(lock_path), os.O_RDWR | os.O_NOFOLLOW) + except OSError: + # Cannot open the file (deleted, perm-denied, etc.). Treat as + # held so the caller asks the user to act. + return True + try: + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except (BlockingIOError, OSError): + return True + # We acquired the lock; release immediately and report not-held. + try: + fcntl.flock(fd, fcntl.LOCK_UN) + except OSError: + pass + return False + finally: + try: + os.close(fd) + except OSError: + pass + + +def _try_remove_lock(path: Path, logger_obj: logging.Logger | None = None) -> tuple[bool, str]: + """Best-effort removal of a git lock file. Refuses if a holder is detected. + + Returns ``(removed, diagnostic)``. ``removed`` is True only if the file + was successfully unlinked. ``diagnostic`` is a human-readable reason + string for logging and for the HTTP response message. + """ + if not path.exists(): + return False, 'no-such-file' + if _is_lock_held(path): + if logger_obj is not None: + logger_obj.warning( + 'clear_lock: refusing to remove %s -- appears held', path + ) + return False, 'lock-held-by-another-process' + try: + os.remove(path) + except OSError as exc: + if logger_obj is not None: + logger_obj.warning( + 'clear_lock: os.remove failed for %s: %s', path, exc + ) + return False, f'remove-failed: {exc}' + if logger_obj is not None: + logger_obj.info('clear_lock: removed %s', path) + return True, 'removed' + + +def apply_clear_lock(target: str) -> dict: + """Non-destructively attempt to clear a stale git lock for ``target``. + + Replaces the v1 approach of pre-clearing locks inside ``apply_force_update``. + v1's mtime heuristic was proven unsafe in PR #5688 gate cert (BRICK-1): + a live git add was empirically shown to hold ``.git/index.lock`` past + 31 s with the lock's mtime unchanged, so age is not a proxy for staleness. + + v2 strategy: + - Refuse to remove any lock file that another process currently holds + (probe via non-blocking fcntl.flock; on non-POSIX, fail closed). + - Only touch ``.git/index.lock`` (the only well-known short-lived lock + git creates); other lock files (``refs/**/*.lock``, ``FETCH_HEAD.lock``, + ...) are reported in the response for the operator to handle manually. + - On success, retry ``_apply_update_inner`` once to apply the update. + (No ``checkout`` / ``clean`` / ``reset --hard`` -- safe by design.) + """ + blocker_snapshot = _restart_blocker_snapshot() + if blocker_snapshot.get('restart_blocked'): + return _restart_blocked_response(target, blocker_snapshot) + + if not _apply_lock.acquire(blocking=False): + return {'ok': False, 'message': 'Update already in progress'} + + try: + if target == 'webui': + path = REPO_ROOT + elif target == 'agent': + path = _AGENT_DIR + else: + return {'ok': False, 'message': f'Unknown target: {target}'} + + if path is None or not (path / '.git').exists(): + return {'ok': False, 'message': 'Not a git repository'} + + git_dir = path / '.git' + # Only attempt removal of well-known short-lived lock files. Anything + # outside _GIT_LOCK_FILES_REMOVABLE is returned in the report so the + # operator can inspect it. + removed: list[str] = [] + refused: list[str] = [] + for name in _GIT_LOCK_FILES_REMOVABLE: + ok, diag = _try_remove_lock(git_dir / name) + if ok: + removed.append(name) + elif diag != 'no-such-file': + refused.append(f'{name} ({diag})') + + # Detect other lock files (refs/heads/.lock etc) for the + # diagnostic -- but never delete them here. + other_locks: list[str] = [] + for entry in sorted(git_dir.rglob('*.lock')): + rel = str(entry.relative_to(git_dir)) + if rel not in _GIT_LOCK_FILES_REMOVABLE: + other_locks.append(rel) + + if refused and not removed: + # All attempted removals were refused -- surface a clean + # manual-instruction response. + return { + 'ok': False, + 'message': ( + 'Cannot safely clear the lock: ' + + '; '.join(refused) + + '. A concurrent git process appears to hold the lock. ' + + 'Wait for it to finish, then retry. As a last resort, ' + + 'run: rm -f ' + str(git_dir / _GIT_LOCK_FILES_REMOVABLE[0]) + ), + 'lock_held': True, + 'target': target, + 'refused': refused, + 'other_locks': other_locks, + } + + # Invalidate cache so the next status check is fresh. + with _cache_lock: + _update_cache['checked_at'] = 0 + + # Now retry the normal (non-destructive) update flow once. If this + # succeeds the user gets their update without any checkout/clean/reset. + retry_result = _apply_update_inner(target) + # Annotate with the recovery summary so the UI can show what happened. + retry_result = dict(retry_result) + retry_result['lock_recovery'] = { + 'removed': removed, + 'refused': refused, + 'other_locks': other_locks, + } + return retry_result + finally: + _apply_lock.release() + + def _windows_git_from_registry(): """Best-effort resolve git.exe from the Git-for-Windows registry key. @@ -1378,27 +1557,14 @@ 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}") + # NOTE: v2 of PR #5688 removed the prior stale-lock cleanup loop from + # this entry point. The mtime-based heuristic was empirically proven + # unsafe (a live `git add` was shown to hold .git/index.lock past 31 s + # with unchanged mtime) and unconditional pre-cleanup clobbered locks + # for force-update retries that had nothing to do with a lock error. + # Lock cleanup is now ONLY performed by the explicit + # /api/updates/clear_lock endpoint, where the user has opted in to + # a non-destructive retry. # --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 @@ -1481,6 +1647,41 @@ def apply_update(target): _apply_lock.release() +def _restore_stash_after_pull_failure( + target: str, + path: Path, + pull_out: str, +) -> str: + """Best-effort re-apply of a stash pushed earlier in `_apply_update_inner`. + + Called when `git pull` failed with a lock error and we had already pushed + a stash for the user's local modifications. Without this, the user's + modifications remain in git stash with the working tree clean -- the + wrong user experience because the failure was a lock conflict, not a + stash-apply conflict, and the stash should re-apply cleanly. + + Returns a human-readable note for inclusion in the response message. + """ + _, pop_ok = _run_git(['stash', 'pop'], path) + if pop_ok: + return ('Local modifications were restored from the temporary stash.') + + # `git stash pop` failed -- could be that the working tree changed under + # us. Try apply + drop to keep the change separation explicit. + _, apply_ok = _run_git(['stash', 'apply'], path) + if apply_ok: + _, _ = _run_git(['stash', 'drop'], path) + return ('Local modifications were restored from the temporary stash.') + + detail = (pull_out or '').strip()[:200] + return ( + 'Your local modifications could not be restored automatically ' + f'(stash pop failed after pull error: {detail or "no detail"}). ' + 'They remain safely in `git stash list`; run `git -C ' + + str(path) + ' stash pop` once the lock is cleared.' + ) + + def _apply_update_inner(target): """Inner implementation of apply_update, called under _apply_lock.""" if target == 'webui': @@ -1558,9 +1759,21 @@ def _apply_update_inner(target): pull_out, pull_ok = _run_git(pull_args, path, timeout=30) if not pull_ok: if _is_git_lock_error(pull_out): + # Lock conflict during pull. If a stash was pushed for the local + # modifications, attempt to restore it before returning so the + # user's working tree is not silently left empty with changes + # stranded in the stash (Greptile P1 on PR #5688). + stash_recovery_note = '' + if stashed: + stash_recovery_note = _restore_stash_after_pull_failure( + target, path, pull_out + ) + message = f'Pull failed due to a repository lock: {pull_out.strip()}' + if stash_recovery_note: + message = f'{message} {stash_recovery_note}' return { 'ok': False, - 'message': f'Pull failed due to a repository lock: {pull_out.strip()}', + 'message': message, 'lock_conflict': True, } pull_lower = pull_out.lower() diff --git a/static/ui.js b/static/ui.js index 4deba79b..5363b03b 100644 --- a/static/ui.js +++ b/static/ui.js @@ -9099,11 +9099,55 @@ function _showUpdateError(target,res){ } else { showToast(msg); } - // Show "Force update" button when the error is recoverable by a hard reset - if(forceBtn&&(res.conflict||res.diverged||res.lock_conflict)){ + // Show "Force update" button ONLY for errors recoverable by a destructive + // hard reset. Lock-only failures are routed to a separate non-destructive + // "Clear lock and retry update" button (BRICK-2 fix for PR #5688: a lock + // error should never invoke apply_force_update, which would discard local + // modifications). + if(forceBtn&&(res.conflict||res.diverged)){ forceBtn.dataset.target=target; forceBtn.style.display='inline-block'; } + // Show "Clear lock and retry update" when the only failure was a stale + // git lock. This calls the new non-destructive /api/updates/clear_lock + // endpoint, which probes the lock for a holder and refuses if held. + const clearLockBtn=$('btnClearUpdateLock'); + if(clearLockBtn&&res.lock_conflict){ + clearLockBtn.dataset.target=target; + clearLockBtn.style.display='inline-block'; + } +} +async function applyClearUpdateLock(btn){ + if(window._clearLockInFlight) return; + const target=btn.dataset.target; + if(!target) return; + window._clearLockInFlight=true; + btn.disabled=true; + const originalLabel=btn.textContent; + btn.textContent='Clearing lock\u2026'; + try{ + const res=await api('/api/updates/clear_lock',{method:'POST',body:JSON.stringify({target}),timeoutMs:60000}); + if(res.ok){ + sessionStorage.removeItem('hermes-update-checked'); + sessionStorage.removeItem('hermes-update-dismissed'); + showToast('Lock cleared — update applied\u2026'); + _waitForServerThenReload({}); + } else { + const msg='Could not clear the lock: '+(res.message||'unknown error'); + const errEl=$('updateError'); + if(errEl){errEl.textContent=msg;errEl.style.display='block';} + else showToast(msg); + } + }catch(e){ + const msg='Clear-lock request failed: '+((e&&e.message)||String(e)); + const errEl=$('updateError'); + if(errEl){errEl.textContent=msg;errEl.style.display='block';} + else showToast(msg); + }finally{ + window._clearLockInFlight=false; + btn.disabled=false; + btn.textContent=originalLabel; + } } function _normalizeHealthServerIdentity(rawIdentity){ if(rawIdentity===undefined||rawIdentity===null) return null; diff --git a/tests/test_updates.py b/tests/test_updates.py index c6898f48..5cab3b98 100644 --- a/tests/test_updates.py +++ b/tests/test_updates.py @@ -1,5 +1,6 @@ """Tests for self-update diagnostics (api/updates.py).""" import os +import sys import time from unittest.mock import MagicMock, patch @@ -907,8 +908,6 @@ def test_select_apply_compare_ref_falls_through_when_latest_tag_is_not_ff_reacha "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 @@ -1012,36 +1011,233 @@ def test_apply_update_non_lock_fetch_failure_does_not_include_lock_conflict(tmp_ # ── apply_force_update lock cleanup tests ──────────────────────────────────── +# +# v2 of PR #5688 removed the prior stale-lock cleanup loop from +# apply_force_update entirely (CORE-1 from the gate cert: a force retry +# for conflict/diverged preemptively deleted .git/**/*.lock before any +# lock error was observed). Lock cleanup now lives ONLY in the explicit +# /api/updates/clear_lock endpoint, gated by a holder probe. +# +# The contract under test here is therefore: apply_force_update must NEVER +# touch git lock files. The check is implemented below as +# test_apply_force_update_no_longer_touches_locks in the v2 tests block. -def test_apply_force_update_removes_stale_lock_files(tmp_path): - """apply_force_update removes lock files older than 30 seconds.""" +def test_apply_force_update_no_longer_touches_locks(tmp_path, monkeypatch): + """apply_force_update must not iterate .git/**/*.lock any more (CORE-1 fix).""" (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)) + lock = tmp_path / '.git' / 'index.lock' + lock.write_text('') + old_mtime = time.time() - 999 # ancient mtime -- would have been removed pre-v2 + os.utime(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') + monkeypatch.setattr(updates, '_run_git', + MagicMock(return_value=('', True))) + monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) + monkeypatch.setattr( + updates, '_restart_blocker_snapshot', + lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0} + ) - assert not stale_lock.exists(), "Stale lock file should have been removed" + updates.apply_force_update('webui') + assert lock.exists(), ( + "apply_force_update must not remove locks; that is the clear_lock " + "endpoint's job" + ) -def test_apply_force_update_skips_recent_lock_file(tmp_path): - """apply_force_update does NOT remove lock files younger than 30 seconds.""" +# ── v2 tests for PR #5688 ────────────────────────────────────────────────── + + +@pytest.mark.skipif(sys.platform.startswith('win'), + reason='fcntl-based holder probe is POSIX-only') +def test_is_lock_held_returns_false_when_no_other_holder(tmp_path): + """An unheld .git/index.lock passes the holder probe.""" + lock = tmp_path / 'index.lock' + lock.write_text('') + assert updates._is_lock_held(lock) is False + + +@pytest.mark.skipif(sys.platform.startswith('win'), + reason='fcntl-based holder probe is POSIX-only') +def test_is_lock_held_returns_true_when_other_process_holds(tmp_path): + """If an external process holds the lock via flock, the probe returns True.""" + lock = tmp_path / 'index.lock' + lock.write_text('') + # Simulate a holder by acquiring flock ourselves from this process. + import fcntl + fd = os.open(str(lock), os.O_RDWR) + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + try: + # Now the probe (a different os.open + flock attempt) must fail + # and return True, refusing removal. + assert updates._is_lock_held(lock) is True + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + os.close(fd) + + +def test_try_remove_lock_refuses_when_held(tmp_path, monkeypatch): + """_try_remove_lock must NEVER touch a file if a holder is detected.""" + lock = tmp_path / 'index.lock' + lock.write_text('sentinel') + + monkeypatch.setattr( + updates, '_is_lock_held', lambda p: True + ) + ok, diag = updates._try_remove_lock(lock) + assert ok is False + assert diag == 'lock-held-by-another-process' + assert lock.exists(), "Lock must NOT be removed when a holder is detected" + assert lock.read_text() == 'sentinel', "Lock contents must NOT be modified" + + +def test_try_remove_lock_succeeds_when_unheld(tmp_path, monkeypatch): + ok, diag = updates._try_remove_lock(tmp_path / 'nope') + assert ok is False and diag == 'no-such-file' + + lock = tmp_path / 'index.lock' + lock.write_text('') + monkeypatch.setattr(updates, '_is_lock_held', lambda p: False) + ok, diag = updates._try_remove_lock(lock) + assert ok is True, diag + assert not lock.exists() + + +def test_apply_clear_lock_removes_unheld_lock_and_runs_normal_update(tmp_path, + monkeypatch): + """clear_lock removes an unheld index.lock and re-runs the normal update path.""" (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)) + lock = tmp_path / '.git' / 'index.lock' + lock.write_text('') + monkeypatch.setattr(updates, '_is_lock_held', lambda p: False) - 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') + fake_git_calls = [] - assert recent_lock.exists(), "Recent lock file should NOT have been removed" + def fake_git(args, cwd, timeout=10): + fake_git_calls.append(args) + return '', True + + monkeypatch.setattr(updates, '_run_git', fake_git) + monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) + monkeypatch.setattr( + updates, '_restart_blocker_snapshot', + lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0} + ) + monkeypatch.setattr( + updates, '_select_apply_compare_ref', + lambda path: 'origin/main' + ) + + result = updates.apply_clear_lock('webui') + assert result['ok'] is True, result + assert result.get('lock_recovery', {}).get('removed') == ['index.lock'] + assert not lock.exists() + + +def test_apply_clear_lock_refuses_when_lock_held(tmp_path, monkeypatch): + """If a holder is detected, clear_lock must NOT remove the lock and must + return a manual-instruction response.""" + (tmp_path / '.git').mkdir() + lock = tmp_path / '.git' / 'index.lock' + lock.write_text('do-not-touch') + + # Always appear held. + monkeypatch.setattr(updates, '_is_lock_held', lambda p: True) + monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) + monkeypatch.setattr( + updates, '_restart_blocker_snapshot', + lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0} + ) + + result = updates.apply_clear_lock('webui') + assert result['ok'] is False + assert result.get('lock_held') is True + assert 'Cannot safely clear the lock' in result['message'] + assert lock.exists(), "Lock must NOT be removed when held" + assert lock.read_text() == 'do-not-touch' + + +def test_apply_update_pull_lock_restores_stash(tmp_path, monkeypatch): + """Greptile P1: a pull-lock error after stashing must restore the stash.""" + (tmp_path / '.git').mkdir() + git_calls = [] + + def fake_git(args, cwd, timeout=10): + git_calls.append(args) + if args[0] == 'status': + # Mark the working tree as dirty so the apply path stashes first. + return ' M api/updates.py\n', True + if args[0] == 'stash' and args[1] == 'push': + return 'Saved working files\n', True + if args[0] == 'stash' and args[1] == 'pop': + return '', True + if args[0] == 'pull': + return "fatal: Unable to create '.git/index.lock': File exists.", False + return '', True + + monkeypatch.setattr(updates, '_run_git', fake_git) + monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) + monkeypatch.setattr( + updates, '_select_apply_compare_ref', + lambda path: 'origin/main' + ) + + result = updates._apply_update_inner('webui') + assert result['ok'] is False + assert result.get('lock_conflict') is True + assert 'Local modifications were restored' in result['message'], ( + f"Expected stash-restore note in message, got: {result['message']!r}" + ) + # Confirm the recorded call list included `stash pop`. + assert any(call[0] == 'stash' and call[1] == 'pop' for call in git_calls), ( + f"Expected git stash pop to be called; git_calls={git_calls!r}" + ) + + +@pytest.mark.parametrize('output,expected', [ + # Tightened v2 signatures -- these match. + ("fatal: Unable to create '/app/.git/index.lock': File exists.", True), + ("fatal: Unable to create '.git/index.lock': File exists.", True), + ("fatal: Unable to create '.git/FETCH_HEAD.lock': File exists.", True), + ("another git process seems to be running in this repository", True), + ("fatal: Unable to create .git/index.lock", True), + # Greptile P2 false-positive class: ref transaction / "lock file lost" + # errors that mention "lock file" but are not stale-lock conditions. + ("fatal: lock file lost while flushing ref transaction", False), + # Plain non-lock errors. + ("", False), + (None, False), + ("fatal: could not resolve host", False), + ("fatal: not possible to fast-forward, aborting", False), +]) +def test_v2_is_git_lock_error_signature_set(output, expected): + assert updates._is_git_lock_error(output) is expected + + +def test_apply_update_pull_lock_no_stash_when_clean(tmp_path, monkeypatch): + """If the working tree was clean, no stash was pushed and no stash pop is needed.""" + (tmp_path / '.git').mkdir() + git_calls = [] + + def fake_git(args, cwd, timeout=10): + git_calls.append(args) + if args[0] == 'status': + return '', True # clean tree + if args[0] == 'pull': + return "fatal: Unable to create '.git/index.lock': File exists.", False + return '', True + + monkeypatch.setattr(updates, '_run_git', fake_git) + monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) + monkeypatch.setattr( + updates, '_select_apply_compare_ref', + lambda path: 'origin/main' + ) + + result = updates._apply_update_inner('webui') + assert result['ok'] is False + assert result.get('lock_conflict') is True + # No stash pop on a clean pull-lock path. + assert not any(c[0] == 'stash' for c in git_calls) From dcec232dfa40865f926146e7f242e67dd498a378 Mon Sep 17 00:00:00 2001 From: b3nw Date: Mon, 6 Jul 2026 17:46:30 +0000 Subject: [PATCH 3/5] fix: add btnClearUpdateLock markup for PR #5688 v2 frontend path Address Greptile P1 review (discussion r3530941293) on commit e8f7e28a: the v2 frontend recovery path was dead at the DOM level. static/ui.js references $('btnClearUpdateLock') but no matching element existed in static/index.html, so users hitting a lock conflict never saw a clickable affordance -- the error panel was the only visible state. Add the missing button next to btnForceUpdate in the update-banner action row: Style mirrors the neutral update-btn (NOT the red --error used by the destructive Force update) so the recovery affordance doesn't read as a warning. Hidden by default; revealed by the existing _showUpdateError() helper when res.lock_conflict is set. Regression tests in TestClearLockButton (3 tests): - test_clear_lock_button_exists - test_clear_lock_button_hidden_by_default - test_clear_lock_button_calls_applyClearUpdateLock_handler These lock in the contract that the button element must be present and correctly bound; future refactors that rename the id or strip the button will fail loud. Mirrors the existing TestIndexHtmlBanner conventions for btnForceUpdate. Verification: - ./scripts/test.sh tests/test_updates.py -k lock -> 35 passed - ./scripts/test.sh tests/test_update_banner_fixes.py -> 102 passed - ./scripts/test.sh tests/test_api_timeout.py -> 8 passed - ruff_lint --diff upstream/master -> 0 new violations --- static/index.html | 1 + tests/test_update_banner_fixes.py | 33 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/static/index.html b/static/index.html index 3e5acbd2..23bd8271 100644 --- a/static/index.html +++ b/static/index.html @@ -415,6 +415,7 @@ +
diff --git a/tests/test_update_banner_fixes.py b/tests/test_update_banner_fixes.py index 13bbe4f9..b9f7610e 100644 --- a/tests/test_update_banner_fixes.py +++ b/tests/test_update_banner_fixes.py @@ -1805,6 +1805,39 @@ class TestIndexHtmlBanner: ) +class TestClearLockButton: + """PR #5688 follow-up: clear-lock button must exist in index.html and be + hidden by default. Without this, the v2 frontend recovery path is dead -- + $('btnClearUpdateLock') returns null at runtime so the lock-only error + branch in _showUpdateError() never reveals a clickable affordance (P1). + """ + + def test_clear_lock_button_exists(self): + src = read('static/index.html') + assert 'id="btnClearUpdateLock"' in src, ( + "index.html must have #btnClearUpdateLock button (hidden by " + "default) -- PR #5688 v2 frontend path" + ) + + def test_clear_lock_button_hidden_by_default(self): + src = read('static/index.html') + m = re.search(r'id="btnClearUpdateLock"[^>]*>', src) + assert m, "#btnClearUpdateLock not found" + tag = m.group(0) + assert 'display:none' in tag, ( + "#btnClearUpdateLock must be hidden by default (display:none)" + ) + + def test_clear_lock_button_calls_applyClearUpdateLock_handler(self): + src = read('static/index.html') + m = re.search(r'id="btnClearUpdateLock"[^>]*>', src) + assert m, "#btnClearUpdateLock not found" + tag = m.group(0) + assert 'applyClearUpdateLock(this)' in tag, ( + "#btnClearUpdateLock onclick must invoke applyClearUpdateLock" + ) + + # ── Regression: sequential webui+agent update — restart coordination ────────── class TestSequentialUpdateRestartCoordination: From 2ddef230ffdbdcfc0f03cc854dd42a5394725328 Mon Sep 17 00:00:00 2001 From: b3nw Date: Mon, 6 Jul 2026 19:02:16 +0000 Subject: [PATCH 4/5] fix: drop auto-delete; manual-instruction only (PR #5688 round-3) Address round-2 gate cert (issuecomment-4896321817) on sha:dcec232d: 1. BRICK (Codex strace-verified): v2's fcntl-flock holder check on .git/index.lock was wrong. Codex straced git 2.43.0 and proved that git uses O_CREAT|O_EXCL + rename(2), NOT advisory locking, so an fcntl-flock probe returns False for a lock file a live git process is holding open. The 'fail-closed' claim was wrong: auto-delete could still race and corrupt the index. 2. CORE (Codex): if .git/index.lock vanished between _is_lock_held and os.remove, the FileNotFoundError surfaced as a 'remove-failed' diagnostic instead of 'already gone' and prevented the apply retry. Fix: stop deleting lock files from the server entirely. The only thing that removes a lock is the user, on the host, via the manual command surfaced in the response. Once the lock is gone, the user re-clicks Update Now and the normal non-destructive apply path runs. Implementation: api/updates.py: - Removed _is_lock_held (fcntl-flock probe) -- was provably unsafe. - Removed _try_remove_lock (os.remove path) -- was provably unsafe. - Removed _GIT_LOCK_FILES_REMOVABLE (no longer enumerable by name). - Added _inventory_locks(path) -> dict: pure inventory of well-known + other lock files under .git/. Never touches anything. - Rewrote apply_clear_lock to inventory-only + manual-instruction. When .git/index.lock is absent, runs the normal non-destructive apply path; when present, returns ok=False with the exact 'rm -f' command + an explanation of why the server cannot do this safely (O_CREAT|O_EXCL cannot be detected with advisory probes). CORE-1 race evaporates: there is no os.remove to race against. api/routes.py: - Updated the /api/updates/clear_lock comment to reflect v2.2. static/ui.js: - applyClearUpdateLock: when res.lock_held, call new _renderLockManualInstruction(target, res) which surfaces the exact 'rm -f ' command in a copyable code block plus an 'I've removed the lock -- retry update' button that POSTs the same endpoint again; the second call takes the success branch and re-runs the normal apply path. Also lists any other lock files present so the operator can investigate. tests/test_updates.py: - Removed all 4 tests for the deleted v2 helpers (_is_lock_held Returns*, _try_remove_lock*). - Added test_v2_probe_helpers_removed: regression guard that fails loud if either helper is ever reintroduced. - Rewrote test_apply_clear_lock_removes_unheld_lock_and_runs_normal_update and test_apply_clear_lock_refuses_when_lock_held for the v2.2 contract. The 'refuses' test monkeypatches os.remove to record any deletion attempt and asserts the call list is empty -- a strict runtime guard against future regressions to auto-delete. - Added 7 new tests: test_inventory_locks_* (4), test_apply_clear lock_with_no_lock_runs_normal_update, test_apply_clear_lock_with lock_present_returns_manual_instruction, test_apply_clear_lock listing_includes_other_locks, test_apply_clear_lock_rejects unknown_target, test_apply_clear_lock_rejects_not_git_repo. Verification: ./scripts/test.sh tests/test_updates.py -> 73 passed ./scripts/test.sh tests/test_update_banner_fixes.py -> 102 passed ./scripts/test.sh tests/test_api_timeout.py -> 8 passed python3 scripts/ruff_lint.py --diff upstream/master -> 0 new violations Full ./scripts/test.sh -> 12137 passed, 18 failed (all 18 pre-existing flakes in unrelated subsystems; verified not v2.2 regressions) --- api/routes.py | 13 +-- api/updates.py | 230 +++++++++++++++++------------------------- static/ui.js | 81 ++++++++++++++- tests/test_updates.py | 224 ++++++++++++++++++++++++++-------------- 4 files changed, 327 insertions(+), 221 deletions(-) diff --git a/api/routes.py b/api/routes.py index 0fe2eac9..0ce292ca 100644 --- a/api/routes.py +++ b/api/routes.py @@ -15044,12 +15044,13 @@ def handle_post(handler, parsed) -> bool: return j(handler, apply_force_update(target)) if parsed.path == "/api/updates/clear_lock": - # Non-destructive recovery for the stale .git/index.lock case: tries - # to clear the well-known short-lived git index lock and re-runs the - # normal (non-destructive) update flow. Refuses to remove locks that - # another process still holds (see apply_clear_lock for the holder - # probe). This is the *only* path on which git locks are removed; - # /api/updates/apply and /api/updates/force do not touch locks. + # Manual-instruction recovery for the .git/index.lock case. The + # endpoint NEVER removes a lock file from the server -- it returns + # the diagnostic + the exact 'rm' command for the operator, and on + # a re-click with the lock already gone, it re-runs the normal + # non-destructive apply path. See apply_clear_lock for the v2.2 + # design rationale (round-2 gate cert: fcntl-flock cannot detect + # git's O_CREAT|O_EXCL locks, so any auto-delete path races). target = body.get("target", "") if target not in ("webui", "agent"): return bad(handler, 'target must be "webui" or "agent"') diff --git a/api/updates.py b/api/updates.py index b708ff15..51c5aa5e 100644 --- a/api/updates.py +++ b/api/updates.py @@ -70,15 +70,11 @@ _GIT_LOCK_SIGNATURES = ( 'another git process seems to be running', 'unable to create .git/index.lock', ) -# Lock files we are willing to remove on the *explicit user request* path -# (`/api/updates/clear_lock`). The clear_lock endpoint refuses to remove any -# lock whose underlying file appears to still be held by another process -- -# there is no mtime-based heuristic any more (BRICK-1 from PR #5688 gate -# cert: a live `git add` was empirically shown to hold .git/index.lock past -# 31 s with unchanged mtime). -_GIT_LOCK_FILES_REMOVABLE = ( - 'index.lock', -) +# Lock files we previously enumerated for auto-removal in v2. v2.2 no longer +# removes anything on the server, so the enumerable list is no longer needed; +# ``_inventory_locks`` reports whatever ``.git/**/*.lock`` files currently exist +# via plain ``rglob``. + def _sanitize_git_diagnostic(output: str, *, limit: int = _GIT_DIAGNOSTIC_MAX_CHARS) -> str: @@ -242,95 +238,70 @@ def _is_git_lock_error(output: str) -> bool: return any(sig in lower_out for sig in _GIT_LOCK_SIGNATURES) -def _is_lock_held(lock_path: Path) -> bool: - """Return True if a process appears to still hold ``lock_path``. +def _inventory_locks(path: Path) -> dict: + """Return a snapshot of lock files currently present under ``path/.git``. - Used by the explicit `clear_lock` recovery path. The implementer is - intentionally conservative: a process hold signal returns True and the - caller refuses to remove the lock. If we cannot prove a holder exists - *and cannot prove one does not*, we still return True (fail-closed) on - the common case the OS denies `flock`/delete. - - Currently: tries a non-blocking exclusive fcntl.flock on POSIX. Any - `BlockingIOError` means another process holds the lock. On platforms - without fcntl (Windows), returns True unconditionally so the endpoint - surfaces a manual-instruction message rather than risking corruption. + v2.2: replaced v2's `_is_lock_held` + `_try_remove_lock` machinery with + pure inventory. Round-2 cert (gate-fail) proved that `fcntl.flock` + cannot detect a live git lock, because git uses `O_CREAT|O_EXCL` and + `rename(2)`, NOT advisory locking. Any auto-delete path can therefore + race against a running `git add` and corrupt the index. v2.2 stops + deleting locks from the server entirely: the only thing that removes + a lock is the user, on the host, via the manual command surfaced in + the response. Once the lock is gone, the user re-clicks Update Now + and the normal non-destructive apply path runs. """ - if not lock_path.exists(): - return False + git_dir = path / '.git' + out = { + 'well_known_lock_present': False, # ``.git/index.lock`` exists? + 'well_known_lock_path': None, # absolute path of ``.git/index.lock`` + 'other_locks': [], # any other lock files, by relative path + } + if not git_dir.exists(): + return out + well_known = git_dir / 'index.lock' try: - import fcntl # local import keeps Windows imports happy - except ImportError: - # No fcntl on this platform -- cannot prove non-hold. Fail closed. - return True - try: - fd = os.open(str(lock_path), os.O_RDWR | os.O_NOFOLLOW) + out['well_known_lock_present'] = well_known.exists() except OSError: - # Cannot open the file (deleted, perm-denied, etc.). Treat as - # held so the caller asks the user to act. - return True + # Permission problem reading the directory -- treat conservatively. + out['well_known_lock_present'] = True + out['well_known_lock_path'] = str(well_known) + + # Enumerate every other lock file under .git/ for diagnostic reporting. + # We never touch them; this is purely an inventory. try: - try: - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) - except (BlockingIOError, OSError): - return True - # We acquired the lock; release immediately and report not-held. - try: - fcntl.flock(fd, fcntl.LOCK_UN) - except OSError: - pass - return False - finally: - try: - os.close(fd) - except OSError: - pass - - -def _try_remove_lock(path: Path, logger_obj: logging.Logger | None = None) -> tuple[bool, str]: - """Best-effort removal of a git lock file. Refuses if a holder is detected. - - Returns ``(removed, diagnostic)``. ``removed`` is True only if the file - was successfully unlinked. ``diagnostic`` is a human-readable reason - string for logging and for the HTTP response message. - """ - if not path.exists(): - return False, 'no-such-file' - if _is_lock_held(path): - if logger_obj is not None: - logger_obj.warning( - 'clear_lock: refusing to remove %s -- appears held', path - ) - return False, 'lock-held-by-another-process' - try: - os.remove(path) - except OSError as exc: - if logger_obj is not None: - logger_obj.warning( - 'clear_lock: os.remove failed for %s: %s', path, exc - ) - return False, f'remove-failed: {exc}' - if logger_obj is not None: - logger_obj.info('clear_lock: removed %s', path) - return True, 'removed' + for entry in sorted(git_dir.rglob('*.lock')): + try: + rel = str(entry.relative_to(git_dir)) + except ValueError: + continue + if rel == 'index.lock': + continue + out['other_locks'].append(rel) + except OSError: + # rglob can fail on unreadable subtrees; skip quietly. + pass + return out def apply_clear_lock(target: str) -> dict: - """Non-destructively attempt to clear a stale git lock for ``target``. + """Manual-instruction lock recovery for ``target``. - Replaces the v1 approach of pre-clearing locks inside ``apply_force_update``. - v1's mtime heuristic was proven unsafe in PR #5688 gate cert (BRICK-1): - a live git add was empirically shown to hold ``.git/index.lock`` past - 31 s with the lock's mtime unchanged, so age is not a proxy for staleness. + v2.2: NEVER removes a lock file. Strategy: - v2 strategy: - - Refuse to remove any lock file that another process currently holds - (probe via non-blocking fcntl.flock; on non-POSIX, fail closed). - - Only touch ``.git/index.lock`` (the only well-known short-lived lock - git creates); other lock files (``refs/**/*.lock``, ``FETCH_HEAD.lock``, - ...) are reported in the response for the operator to handle manually. - - On success, retry ``_apply_update_inner`` once to apply the update. - (No ``checkout`` / ``clean`` / ``reset --hard`` -- safe by design.) + - If ``.git/index.lock`` is absent: re-run the normal non-destructive + apply path so the user lands on the latest version without ever + touching destructive git operations. + - If ``.git/index.lock`` is present: do NOT touch it -- the server + has no reliable proof that no live git process is still using + it (round-2 cert showed `fcntl.flock` does not detect git's + actual ``O_CREAT|O_EXCL`` locking). Return a response with the + exact manual command the operator can run, plus the inventory of + any other lock files so they can investigate. The frontend then + surfaces a copyable ``rm`` line and a "I've removed the lock -- + try update again" button that re-invokes this endpoint, which + (now that the lock is gone) will take the success branch and + re-run the normal apply. """ blocker_snapshot = _restart_blocker_snapshot() if blocker_snapshot.get('restart_blocked'): @@ -350,60 +321,43 @@ def apply_clear_lock(target: str) -> dict: if path is None or not (path / '.git').exists(): return {'ok': False, 'message': 'Not a git repository'} - git_dir = path / '.git' - # Only attempt removal of well-known short-lived lock files. Anything - # outside _GIT_LOCK_FILES_REMOVABLE is returned in the report so the - # operator can inspect it. - removed: list[str] = [] - refused: list[str] = [] - for name in _GIT_LOCK_FILES_REMOVABLE: - ok, diag = _try_remove_lock(git_dir / name) - if ok: - removed.append(name) - elif diag != 'no-such-file': - refused.append(f'{name} ({diag})') + inv = _inventory_locks(path) + manual_command = f"rm -f {inv['well_known_lock_path']}" - # Detect other lock files (refs/heads/.lock etc) for the - # diagnostic -- but never delete them here. - other_locks: list[str] = [] - for entry in sorted(git_dir.rglob('*.lock')): - rel = str(entry.relative_to(git_dir)) - if rel not in _GIT_LOCK_FILES_REMOVABLE: - other_locks.append(rel) - - if refused and not removed: - # All attempted removals were refused -- surface a clean - # manual-instruction response. - return { - 'ok': False, - 'message': ( - 'Cannot safely clear the lock: ' - + '; '.join(refused) - + '. A concurrent git process appears to hold the lock. ' - + 'Wait for it to finish, then retry. As a last resort, ' - + 'run: rm -f ' + str(git_dir / _GIT_LOCK_FILES_REMOVABLE[0]) - ), - 'lock_held': True, - 'target': target, - 'refused': refused, - 'other_locks': other_locks, + if not inv['well_known_lock_present']: + # Lock is gone. Run the normal non-destructive update flow and + # annotate the response with what we found for the user's + # records. + with _cache_lock: + _update_cache['checked_at'] = 0 + retry_result = _apply_update_inner(target) + retry_result = dict(retry_result) + retry_result['lock_recovery'] = { + 'action': 'no-lock-found', + 'manual_command': manual_command, + 'other_locks': inv['other_locks'], } + return retry_result - # Invalidate cache so the next status check is fresh. - with _cache_lock: - _update_cache['checked_at'] = 0 - - # Now retry the normal (non-destructive) update flow once. If this - # succeeds the user gets their update without any checkout/clean/reset. - retry_result = _apply_update_inner(target) - # Annotate with the recovery summary so the UI can show what happened. - retry_result = dict(retry_result) - retry_result['lock_recovery'] = { - 'removed': removed, - 'refused': refused, - 'other_locks': other_locks, + # Lock is present. The server cannot prove it's safe to delete; + # the only safe path is to ask the operator. + message = ( + 'A git lock file (.git/index.lock) is present. The server does ' + 'not delete locks automatically -- git uses O_CREAT|O_EXCL ' + 'locking, which cannot be detected with advisory probes. To ' + 'recover: confirm no other git process is running against ' + f'this checkout, then run: {manual_command} ' + 'Click "Retry update" once you have removed it.' + ) + return { + 'ok': False, + 'message': message, + 'lock_held': True, + 'target': target, + 'manual_command': manual_command, + 'well_known_lock_path': inv['well_known_lock_path'], + 'other_locks': inv['other_locks'], } - return retry_result finally: _apply_lock.release() diff --git a/static/ui.js b/static/ui.js index 5363b03b..11cbd0ec 100644 --- a/static/ui.js +++ b/static/ui.js @@ -9124,22 +9124,29 @@ async function applyClearUpdateLock(btn){ window._clearLockInFlight=true; btn.disabled=true; const originalLabel=btn.textContent; - btn.textContent='Clearing lock\u2026'; + btn.textContent='Checking lock…'; try{ const res=await api('/api/updates/clear_lock',{method:'POST',body:JSON.stringify({target}),timeoutMs:60000}); if(res.ok){ sessionStorage.removeItem('hermes-update-checked'); sessionStorage.removeItem('hermes-update-dismissed'); - showToast('Lock cleared — update applied\u2026'); + showToast('Update applied — restarting…'); _waitForServerThenReload({}); + } else if(res.lock_held){ + // v2.2: server returns manual-instruction. Show the exact `rm` + // command + a one-click "I've removed it, retry update" affordance + // that POSTs the same endpoint a second time (now that the user + // has presumably removed the lock, the server's success branch + // runs the normal non-destructive apply). + _renderLockManualInstruction(target, res); } else { - const msg='Could not clear the lock: '+(res.message||'unknown error'); + const msg='Could not check the lock: '+(res.message||'unknown error'); const errEl=$('updateError'); if(errEl){errEl.textContent=msg;errEl.style.display='block';} else showToast(msg); } }catch(e){ - const msg='Clear-lock request failed: '+((e&&e.message)||String(e)); + const msg='Lock-check request failed: '+((e&&e.message)||String(e)); const errEl=$('updateError'); if(errEl){errEl.textContent=msg;errEl.style.display='block';} else showToast(msg); @@ -9149,6 +9156,72 @@ async function applyClearUpdateLock(btn){ btn.textContent=originalLabel; } } +function _renderLockManualInstruction(target, res){ + // Replace the inline `updateError` text with a richer block that shows + // the exact manual command and offers a one-click retry button. The + // "retry" handler re-invokes `applyClearUpdateLock`; this time, with + // the lock gone, the server's success branch runs the normal apply. + const cmd = res.manual_command || ('rm -f ' + (res.well_known_lock_path || '.git/index.lock')); + const errEl=$('updateError'); + if(!errEl){ + showToast('Lock present. Run: '+cmd); + return; + } + errEl.style.display='block'; + errEl.innerHTML=''; + const intro=document.createElement('div'); + intro.style.marginBottom='6px'; + intro.textContent='A stale .git/index.lock is present. The server cannot remove it safely — please run this command on the host:'; + errEl.appendChild(intro); + const code=document.createElement('pre'); + code.style.background='rgba(0,0,0,0.05)'; + code.style.padding='6px'; + code.style.margin='4px 0'; + code.style.fontFamily='ui-monospace,monospace'; + code.style.borderRadius='4px'; + code.style.whiteSpace='pre-wrap'; + code.style.wordBreak='break-all'; + code.textContent=cmd; + errEl.appendChild(code); + const actions=document.createElement('div'); + actions.style.display='flex'; + actions.style.gap='8px'; + actions.style.flexWrap='wrap'; + const copyBtn=document.createElement('button'); + copyBtn.type='button'; + copyBtn.className='update-btn'; + copyBtn.textContent='Copy command'; + copyBtn.onclick=async()=>{ + try{ + if(navigator.clipboard&&navigator.clipboard.writeText){ + await navigator.clipboard.writeText(cmd); + copyBtn.textContent='Copied'; + setTimeout(()=>{ copyBtn.textContent='Copy command'; }, 1500); + } else { + copyBtn.textContent='Clipboard unavailable'; + } + } catch(_){ + copyBtn.textContent='Copy failed'; + } + }; + actions.appendChild(copyBtn); + const retryBtn=document.createElement('button'); + retryBtn.type='button'; + retryBtn.className='update-btn update-primary'; + retryBtn.textContent="I've removed the lock — retry update"; + retryBtn.dataset.target=target; + retryBtn.onclick=()=>{ applyClearUpdateLock(retryBtn); }; + actions.appendChild(retryBtn); + errEl.appendChild(actions); + if(Array.isArray(res.other_locks)&&res.other_locks.length){ + const other=document.createElement('div'); + other.style.marginTop='6px'; + other.style.fontSize='11px'; + other.style.opacity='0.85'; + other.textContent='Other lock files also present: '+res.other_locks.join(', '); + errEl.appendChild(other); + } +} function _normalizeHealthServerIdentity(rawIdentity){ if(rawIdentity===undefined||rawIdentity===null) return null; if(typeof rawIdentity==='string'){ diff --git a/tests/test_updates.py b/tests/test_updates.py index 5cab3b98..28b18d4f 100644 --- a/tests/test_updates.py +++ b/tests/test_updates.py @@ -1,6 +1,5 @@ """Tests for self-update diagnostics (api/updates.py).""" import os -import sys import time from unittest.mock import MagicMock, patch @@ -1046,79 +1045,95 @@ def test_apply_force_update_no_longer_touches_locks(tmp_path, monkeypatch): ) -# ── v2 tests for PR #5688 ────────────────────────────────────────────────── +# ── v2 (Round-2) tests for PR #5688 ────────────────────────────────────────── +# +# v2.2 dropped v2's fcntl-flock holder probe + os.remove path entirely. +# Those round-2 functions (`_is_lock_held`, `_try_remove_lock`) are +# removed in v2.2 because they were proven unsafe (Codex strace showed +# git uses O_CREAT|O_EXCL, not advisory locking). The deletion tests +# below assert they no longer exist -- if a future refactor reintroduces +# either, those tests fail loud. The replacements for them are the v2.2 +# inventory + apply_clear_lock tests further below. -@pytest.mark.skipif(sys.platform.startswith('win'), - reason='fcntl-based holder probe is POSIX-only') -def test_is_lock_held_returns_false_when_no_other_holder(tmp_path): - """An unheld .git/index.lock passes the holder probe.""" - lock = tmp_path / 'index.lock' - lock.write_text('') - assert updates._is_lock_held(lock) is False +def test_v2_probe_helpers_removed(): + """v2.2 contract: the round-2 fcntl-flock probe machinery is gone. - -@pytest.mark.skipif(sys.platform.startswith('win'), - reason='fcntl-based holder probe is POSIX-only') -def test_is_lock_held_returns_true_when_other_process_holds(tmp_path): - """If an external process holds the lock via flock, the probe returns True.""" - lock = tmp_path / 'index.lock' - lock.write_text('') - # Simulate a holder by acquiring flock ourselves from this process. - import fcntl - fd = os.open(str(lock), os.O_RDWR) - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) - try: - # Now the probe (a different os.open + flock attempt) must fail - # and return True, refusing removal. - assert updates._is_lock_held(lock) is True - finally: - fcntl.flock(fd, fcntl.LOCK_UN) - os.close(fd) - - -def test_try_remove_lock_refuses_when_held(tmp_path, monkeypatch): - """_try_remove_lock must NEVER touch a file if a holder is detected.""" - lock = tmp_path / 'index.lock' - lock.write_text('sentinel') - - monkeypatch.setattr( - updates, '_is_lock_held', lambda p: True + Round-2 cert proved `flock` cannot detect git's O_CREAT|O_EXCL locks, + so any auto-delete path can race a running git process. This guard + test fails loud if anyone reintroduces either helper. + """ + assert not hasattr(updates, '_is_lock_held'), ( + "_is_lock_held was re-introduced after v2.2 removal -- round-2 cert " + "showed fcntl.flock cannot detect git locks; do not bring it back." + ) + assert not hasattr(updates, '_try_remove_lock'), ( + "_try_remove_lock was re-introduced after v2.2 removal -- auto-delete " + "from the server is unsafe on a brick-risk path; do not bring it back." ) - ok, diag = updates._try_remove_lock(lock) - assert ok is False - assert diag == 'lock-held-by-another-process' - assert lock.exists(), "Lock must NOT be removed when a holder is detected" - assert lock.read_text() == 'sentinel', "Lock contents must NOT be modified" -def test_try_remove_lock_succeeds_when_unheld(tmp_path, monkeypatch): - ok, diag = updates._try_remove_lock(tmp_path / 'nope') - assert ok is False and diag == 'no-such-file' - - lock = tmp_path / 'index.lock' - lock.write_text('') - monkeypatch.setattr(updates, '_is_lock_held', lambda p: False) - ok, diag = updates._try_remove_lock(lock) - assert ok is True, diag - assert not lock.exists() +# ── v2.2 tests for PR #5688 ────────────────────────────────────────────────── +# +# v2.2 dropped v2's fcntl-flock holder probe and os.remove path entirely. +# ``apply_clear_lock`` is now inventory-only and manual-instruction: if +# the lock is gone it re-runs the normal update; if the lock is present +# it returns the exact ``rm`` command the operator must run. These tests +# lock in that contract. -def test_apply_clear_lock_removes_unheld_lock_and_runs_normal_update(tmp_path, - monkeypatch): - """clear_lock removes an unheld index.lock and re-runs the normal update path.""" +def test_inventory_locks_reports_when_index_lock_present(tmp_path): + """Inventory must report well_known_lock_present=True when .git/index.lock + exists, plus its absolute path.""" (tmp_path / '.git').mkdir() lock = tmp_path / '.git' / 'index.lock' - lock.write_text('') - monkeypatch.setattr(updates, '_is_lock_held', lambda p: False) + lock.write_text('stale') + inv = updates._inventory_locks(tmp_path) + assert inv['well_known_lock_present'] is True + assert inv['well_known_lock_path'] == str(lock) + assert inv['other_locks'] == [] - fake_git_calls = [] - def fake_git(args, cwd, timeout=10): - fake_git_calls.append(args) - return '', True +def test_inventory_locks_reports_when_index_lock_absent(tmp_path): + """Inventory must report well_known_lock_present=False when no lock exists.""" + (tmp_path / '.git').mkdir() + inv = updates._inventory_locks(tmp_path) + assert inv['well_known_lock_present'] is False + assert inv['other_locks'] == [] - monkeypatch.setattr(updates, '_run_git', fake_git) + +def test_inventory_locks_lists_other_locks(tmp_path): + """Inventory must enumerate refs/*.lock etc without including index.lock.""" + (tmp_path / '.git').mkdir() + (tmp_path / '.git' / 'index.lock').write_text('') + (tmp_path / '.git' / 'refs' / 'heads').mkdir(parents=True) + (tmp_path / '.git' / 'refs' / 'heads' / 'main.lock').write_text('') + (tmp_path / '.git' / 'FETCH_HEAD.lock').write_text('') + inv = updates._inventory_locks(tmp_path) + assert inv['well_known_lock_present'] is True + assert sorted(inv['other_locks']) == [ + 'FETCH_HEAD.lock', + 'refs/heads/main.lock', + ] + + +def test_inventory_locks_handles_missing_git_dir(tmp_path): + """When ``.git`` does not exist the inventory must still return a valid shape.""" + inv = updates._inventory_locks(tmp_path) + assert inv == { + 'well_known_lock_present': False, + 'well_known_lock_path': None, + 'other_locks': [], + } + + +def test_apply_clear_lock_with_no_lock_runs_normal_update(tmp_path, monkeypatch): + """v2.2: when ``.git/index.lock`` is absent, apply_clear_lock re-runs + the normal non-destructive apply path.""" + (tmp_path / '.git').mkdir() + # No lock file written. + monkeypatch.setattr(updates, '_run_git', + MagicMock(return_value=('', True))) monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) monkeypatch.setattr( updates, '_restart_blocker_snapshot', @@ -1128,22 +1143,34 @@ def test_apply_clear_lock_removes_unheld_lock_and_runs_normal_update(tmp_path, updates, '_select_apply_compare_ref', lambda path: 'origin/main' ) - result = updates.apply_clear_lock('webui') assert result['ok'] is True, result - assert result.get('lock_recovery', {}).get('removed') == ['index.lock'] - assert not lock.exists() + assert result['lock_recovery']['action'] == 'no-lock-found' + assert 'manual_command' in result['lock_recovery'] + assert 'rm -f' in result['lock_recovery']['manual_command'] -def test_apply_clear_lock_refuses_when_lock_held(tmp_path, monkeypatch): - """If a holder is detected, clear_lock must NOT remove the lock and must - return a manual-instruction response.""" +def test_apply_clear_lock_with_lock_present_returns_manual_instruction(tmp_path, monkeypatch): + """v2.2: when a lock is present, apply_clear_lock must NEVER touch the + lock file; it returns ok=False with a manual-instruction response.""" (tmp_path / '.git').mkdir() lock = tmp_path / '.git' / 'index.lock' - lock.write_text('do-not-touch') + lock.write_text('user-removed-this-by-hand') - # Always appear held. - monkeypatch.setattr(updates, '_is_lock_held', lambda p: True) + # Spy: capture whether the server attempted to delete anything. The + # correct v2.2 behavior is NO DELETE attempt whatsoever, regardless + # of any property of the lock file. + delete_attempts = [] + + def forbid_delete(*args, **kwargs): + delete_attempts.append((args, kwargs)) + # Use a sentinel that will NEVER happen; if delete is called the + # test must fail. The cheap way: just record the attempt. + return None + + # Patch os.remove + Path.unlink on the instance/module to record any + # destructive attempt. apply_clear_lock must NOT call them. + monkeypatch.setattr(updates.os, 'remove', forbid_delete) monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) monkeypatch.setattr( updates, '_restart_blocker_snapshot', @@ -1153,11 +1180,62 @@ def test_apply_clear_lock_refuses_when_lock_held(tmp_path, monkeypatch): result = updates.apply_clear_lock('webui') assert result['ok'] is False assert result.get('lock_held') is True - assert 'Cannot safely clear the lock' in result['message'] - assert lock.exists(), "Lock must NOT be removed when held" - assert lock.read_text() == 'do-not-touch' + assert result.get('manual_command', '').startswith('rm -f') + assert result.get('well_known_lock_path') == str(lock) + assert 'O_CREAT|O_EXCL' in result['message'], ( + "message must explain why the server cannot do this automatically" + ) + assert lock.exists(), "Lock must NOT be removed" + assert lock.read_text() == 'user-removed-this-by-hand', ( + "Lock contents must NOT be modified" + ) + assert delete_attempts == [], ( + "v2.2 contract: apply_clear_lock must never attempt os.remove " + "under any circumstances" + ) +def test_apply_clear_lock_listing_includes_other_locks(tmp_path, monkeypatch): + """Inventory of other-lock files must round-trip through the response so + the operator can act on them too.""" + (tmp_path / '.git').mkdir() + (tmp_path / '.git' / 'index.lock').write_text('') + (tmp_path / '.git' / 'refs').mkdir() + (tmp_path / '.git' / 'refs' / 'main.lock').write_text('') + monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) + monkeypatch.setattr( + updates, '_restart_blocker_snapshot', + lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0} + ) + + result = updates.apply_clear_lock('webui') + assert result['ok'] is False + assert result['lock_held'] is True + assert result['other_locks'] == ['refs/main.lock'] + + +def test_apply_clear_lock_rejects_unknown_target(tmp_path, monkeypatch): + monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) + monkeypatch.setattr( + updates, '_restart_blocker_snapshot', + lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0} + ) + result = updates.apply_clear_lock('not-a-target') + assert result['ok'] is False + assert 'Unknown target' in result['message'] + + +def test_apply_clear_lock_rejects_not_git_repo(tmp_path, monkeypatch): + """If REPO_ROOT has no .git, apply_clear_lock must refuse.""" + # tmp_path has no .git + monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path) + monkeypatch.setattr( + updates, '_restart_blocker_snapshot', + lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0} + ) + result = updates.apply_clear_lock('webui') + assert result['ok'] is False + assert 'Not a git repository' in result['message'] def test_apply_update_pull_lock_restores_stash(tmp_path, monkeypatch): """Greptile P1: a pull-lock error after stashing must restore the stash.""" (tmp_path / '.git').mkdir() From 9ae4ec8f097efd6661c71a949788a51202a18639 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 6 Jul 2026 21:46:39 +0000 Subject: [PATCH 5/5] docs(changelog): stamp #5688 self-update lock recovery --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8b8150d..5212775b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ ## [Unreleased] +### Fixed + +- **Self-update recovers when a `.git/index.lock` is blocking it.** If "Update Now" failed because a stale `.git/index.lock` was present (a previous git process left it behind), the WebUI gave no in-UI way out — you were stuck on an un-updatable install. The update banner now detects that specific failure, shows the exact `rm -f …/.git/index.lock` command to run on the host, and offers a "Retry update" button that re-runs the normal update once the lock is gone. The server never deletes anything under `.git` itself (fail-closed — git's lock can't be safely auto-cleared without racing), so this is purely a detect-and-guide recovery. Thanks @b3nw. (#5688, #5687) + ### Internal - **Fixed the last 2 chronic local full-suite test-isolation flakes.** `test_tls_support::test_tls_startup_failure_fallback_to_http` and `test_v050259_sessiondb_fd_leak::test_session_db_close_is_idempotent` failed only in a full local suite run (they skip on CI, where the agent package isn't co-located). Root cause was the same "simulate agent package unavailable" antipattern shipped-fixed for the profile cluster: `test_custom_provider_prefix_collisions` clobbered `sys.modules['agent']` at collection time (never restored), and several helpers emptied real package `__path__` lists in place. Fixed at the source — the collection-time shim is now guarded on `importlib.util.find_spec` (only installed when the real package is genuinely absent), the in-place `__path__` mutations use `monkeypatch.setattr(..., raising=False)`, and `test_issue1574`'s fake-agent helper restores the env/`sys.path` it mutates. Full local suite now 0 failed (was 2); `test_title_aux_routing` unaffected. Test-only.