From e8f7e28a3ccd720d62302a0c8e3d2528c19076d6 Mon Sep 17 00:00:00 2001 From: b3nw Date: Mon, 6 Jul 2026 17:37:05 +0000 Subject: [PATCH] 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)