From 2ddef230ffdbdcfc0f03cc854dd42a5394725328 Mon Sep 17 00:00:00 2001 From: b3nw Date: Mon, 6 Jul 2026 19:02:16 +0000 Subject: [PATCH] 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()