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. diff --git a/api/routes.py b/api/routes.py index 589f2afc..a22ad08e 100644 --- a/api/routes.py +++ b/api/routes.py @@ -15074,6 +15074,21 @@ def handle_post(handler, parsed) -> bool: return j(handler, apply_force_update(target)) + if parsed.path == "/api/updates/clear_lock": + # 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"') + 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 d9c91d69..51c5aa5e 100644 --- a/api/updates.py +++ b/api/updates.py @@ -58,6 +58,23 @@ _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 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: @@ -214,6 +231,137 @@ def _run_git(args, cwd, timeout=10): return f'git failed to start: {exc}', False +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 _inventory_locks(path: Path) -> dict: + """Return a snapshot of lock files currently present under ``path/.git``. + + 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. + """ + 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: + out['well_known_lock_present'] = well_known.exists() + except OSError: + # 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: + 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: + """Manual-instruction lock recovery for ``target``. + + v2.2: NEVER removes a lock file. Strategy: + + - 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'): + 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'} + + inv = _inventory_locks(path) + manual_command = f"rm -f {inv['well_known_lock_path']}" + + 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 + + # 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'], + } + finally: + _apply_lock.release() + + def _windows_git_from_registry(): """Best-effort resolve git.exe from the Git-for-Windows registry key. @@ -1363,6 +1511,15 @@ def apply_force_update(target: str) -> dict: if path is None or not (path / '.git').exists(): return {'ok': False, 'message': 'Not a git repository'} + # 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 # existing tag". See #2756. @@ -1444,6 +1601,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': @@ -1460,6 +1652,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 +1674,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 +1712,24 @@ 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): + # 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': message, + '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/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/static/ui.js b/static/ui.js index 6eb81982..653d1aa1 100644 --- a/static/ui.js +++ b/static/ui.js @@ -9254,11 +9254,128 @@ function _showUpdateError(target,res){ } else { showToast(msg); } - // Show "Force update" button when the error is recoverable by a hard reset + // 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='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('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 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='Lock-check 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 _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; 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: diff --git a/tests/test_updates.py b/tests/test_updates.py index 083ef23e..28b18d4f 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,425 @@ 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.", +]) +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 ──────────────────────────────────── +# +# 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_no_longer_touches_locks(tmp_path, monkeypatch): + """apply_force_update must not iterate .git/**/*.lock any more (CORE-1 fix).""" + (tmp_path / '.git').mkdir() + 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)) + + 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} + ) + + updates.apply_force_update('webui') + assert lock.exists(), ( + "apply_force_update must not remove locks; that is the clear_lock " + "endpoint's job" + ) + + +# ── 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. + + +def test_v2_probe_helpers_removed(): + """v2.2 contract: the round-2 fcntl-flock probe machinery is gone. + + 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." + ) + + +# ── 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_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('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'] == [] + + +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'] == [] + + +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', + 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['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_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('user-removed-this-by-hand') + + # 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', + 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 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() + 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) +