Merge pull request #5708 from nesquena/release/stage-5688b
Release: self-update .git/index.lock recovery (#5688)
This commit is contained in:
nesquena-hermes
GitHub
commit
5ecd248e92
7 changed files with 819 additions and 1 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
222
api/updates.py
222
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 = (
|
||||
|
|
|
|||
|
|
@ -415,6 +415,7 @@
|
|||
<button class="update-btn" onclick="dismissUpdate()">Later</button>
|
||||
<button class="update-btn update-primary" id="btnApplyUpdate" onclick="applyUpdates()">Update Now</button>
|
||||
<button class="update-btn" id="btnForceUpdate" style="display:none;background:var(--error,#e05);color:#fff;border-color:var(--error,#e05)" onclick="forceUpdate(this)">Force update</button>
|
||||
<button class="update-btn" id="btnClearUpdateLock" style="display:none" onclick="applyClearUpdateLock(this)">Clear lock and retry update</button>
|
||||
</div>
|
||||
</div>
|
||||
<div class="update-banner" id="staleClientBanner">
|
||||
|
|
|
|||
119
static/ui.js
119
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;
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue