fix: the hand-off scripts read an unopenable checkout lock as held
marker.sh::checkout_lock_held and marker.ps1::Test-CheckoutLockHeld answered "not held" when the lock file exists but cannot be opened (root-owned after `sudo hermes update`, or held open without sharing on Windows), while the Desktop's own probes (checkout_held / WINDOWS_CHECKOUT_LOCK_PS) answer "held". The script helper's reclaim then deleted a dead marker the Desktop's probe keeps. Both scripts now fail closed on every open/probe error except a missing file: reclaim needs a provably free checkout. Parity tests: the POSIX probe with perl, python3 and flock(1), plus an end-to-end reclaim; the Windows reclaim against a lock file held with no sharing (Windows CI lane).
This commit is contained in:
@@ -387,15 +387,18 @@ function Test-CheckoutLockHeld {
|
||||
# could not bind also locks one of the 16 lease bytes just past it (R5b): a leased
|
||||
# child outlives a killed owner and keeps the checkout busy. Probe all 1+16 bytes
|
||||
# in one LockFile (it fails if ANY of them is held) and give them straight back.
|
||||
# A lock file that exists but cannot be opened or probed (denied, opened without
|
||||
# sharing) counts as held, like marker.sh and the Desktop's own probe: reclaim
|
||||
# needs a provably free checkout.
|
||||
$path = Get-CheckoutLockPath
|
||||
if (-not $path -or -not [System.IO.File]::Exists($path)) { return $false }
|
||||
$fs = $null
|
||||
try {
|
||||
$fs = [System.IO.File]::Open($path, [System.IO.FileMode]::Open, [System.IO.FileAccess]::Read,
|
||||
([System.IO.FileShare]::ReadWrite -bor [System.IO.FileShare]::Delete))
|
||||
} catch { return $false }
|
||||
} catch [System.IO.FileNotFoundException], [System.IO.DirectoryNotFoundException] { return $false } catch { return $true }
|
||||
try {
|
||||
try { $fs.Lock(1048576, 17) } catch [System.IO.IOException] { return $true }
|
||||
try { $fs.Lock(1048576, 17) } catch { return $true }
|
||||
try { $fs.Unlock(1048576, 17) } catch {}
|
||||
return $false
|
||||
} finally {
|
||||
|
||||
@@ -531,7 +531,9 @@ checkout_lock_held() { # 0 iff some process holds the checkout kernel lock right
|
||||
# take and the drop are two consecutive syscalls in one process that opened
|
||||
# the file itself -- no process exit, wait or shell work in between. flock(1)
|
||||
# can only take it on a shell fd, so there the hold spans flock(1)'s exit and
|
||||
# our close. No tool: held (fail closed, the release waits).
|
||||
# our close. No tool, or a lock file that exists but cannot be opened (root-
|
||||
# owned after `sudo hermes update`): held -- fail closed like the Desktop's
|
||||
# own probe (checkout_held): reclaim needs a provably free checkout.
|
||||
local path rc tool="$MARKER_LOCK_TOOL_FORCED"
|
||||
path="$(checkout_lock_path)"
|
||||
[ -f "$path" ] || return 1
|
||||
@@ -545,7 +547,7 @@ checkout_lock_held() { # 0 iff some process holds the checkout kernel lock right
|
||||
case "$tool" in
|
||||
perl)
|
||||
perl -MFcntl=:flock -e '
|
||||
open(my $f, "<", $ARGV[0]) or exit 2;
|
||||
open(my $f, "<", $ARGV[0]) or exit($!{ENOENT} ? 3 : 2);
|
||||
flock($f, LOCK_EX | LOCK_NB) or exit 1;
|
||||
flock($f, LOCK_UN); exit 0' "$path"; rc=$? ;;
|
||||
python3)
|
||||
@@ -553,6 +555,8 @@ checkout_lock_held() { # 0 iff some process holds the checkout kernel lock right
|
||||
import fcntl, os, sys
|
||||
try:
|
||||
fd = os.open(sys.argv[1], os.O_RDONLY)
|
||||
except FileNotFoundError:
|
||||
sys.exit(3)
|
||||
except OSError:
|
||||
sys.exit(2)
|
||||
try:
|
||||
@@ -561,12 +565,12 @@ except OSError:
|
||||
sys.exit(1)
|
||||
fcntl.flock(fd, fcntl.LOCK_UN)' "$path"; rc=$? ;;
|
||||
flock)
|
||||
{ exec 8<"$path"; } 2>/dev/null || return 1
|
||||
{ exec 8<"$path"; } 2>/dev/null || { [ -e "$path" ]; return $?; }
|
||||
flock -x -n 8; rc=$?
|
||||
exec 8<&- ;; # closing our probe fd releases it when we did get it
|
||||
*) rc=1 ;;
|
||||
esac
|
||||
[ "$rc" -eq 1 ]
|
||||
[ "$rc" -ne 0 ] && [ "$rc" -ne 3 ] # 3: it vanished since the -f look
|
||||
}
|
||||
|
||||
# ── helper ops for the Desktop (it never mutates the marker itself) ──────────
|
||||
|
||||
@@ -337,6 +337,32 @@ def test_reclaim_without_a_marker_reports_held_while_the_checkout_lock_is_held(t
|
||||
assert _helper(tmp_path, home, install, "reclaim") == "absent"
|
||||
|
||||
|
||||
@pytest.mark.skipif(os.geteuid() == 0, reason="root opens a mode-000 file")
|
||||
@pytest.mark.parametrize("tool", ["perl", "python3", "flock"])
|
||||
def test_an_unreadable_checkout_lock_counts_as_held(tmp_path, tool):
|
||||
"""kshitij P2: a lock file that exists but cannot be opened (root-owned after `sudo hermes
|
||||
update`) read `not held`, while the Desktop's own probe (checkout_held) reads `held` -- so the
|
||||
local reclaim deleted a dead marker the remote helper kept. Fail closed, every probe tool."""
|
||||
if shutil.which(tool) is None:
|
||||
pytest.skip(f"{tool} is not installed")
|
||||
home, install = _install(tmp_path, legacy=True)
|
||||
lock = install / ".hermes-update.lock"
|
||||
probe = (f"log() {{ :; }}; MARKER=/dev/null INSTALL_ROOT={shlex.quote(str(install))} MARKER_LOCK_TOOL={tool}; "
|
||||
f". {shlex.quote(str(MARKER_SH))}; checkout_lock_held && echo held || echo free")
|
||||
def held() -> str:
|
||||
return subprocess.run(["bash", "-c", probe], capture_output=True, text=True, timeout=30).stdout.strip()
|
||||
assert held() == "free" # absent
|
||||
lock.touch(); lock.chmod(0)
|
||||
try:
|
||||
assert held() == "held"
|
||||
if tool == "perl": # end to end: the Desktop's reclaim keeps the dead marker
|
||||
(home / ".hermes-update-in-progress").write_text(f"999999\n{int(time.time())}\nct:1.000\n", encoding="utf-8")
|
||||
assert _helper(tmp_path, home, install, "reclaim") == "held"
|
||||
finally:
|
||||
lock.chmod(0o644)
|
||||
assert held() == "free"
|
||||
|
||||
|
||||
def test_release_waits_for_the_survivor_then_removes_the_marker(tmp_path):
|
||||
home, install = _install(tmp_path, legacy=True)
|
||||
completion = tmp_path / "release-completion"
|
||||
|
||||
@@ -224,6 +224,31 @@ def test_marker_op_reclaim_without_a_marker_reports_held_while_the_checkout_lock
|
||||
assert _op(tmp_path, '-MarkerOp', 'reclaim')[:2] == (0, 'absent\n')
|
||||
|
||||
|
||||
@pytest.mark.platforms('windows')
|
||||
def test_marker_op_reclaim_reports_held_while_the_checkout_lock_cannot_be_opened(tmp_path: Path) -> None:
|
||||
"""kshitij P2 (parity with marker.sh and the Desktop's probe): a lock file that exists but
|
||||
cannot be opened -- here another process holds it with no sharing -- read `not held`, so the
|
||||
reclaim deleted a dead marker the Desktop's own probe kept. It counts as held."""
|
||||
import ctypes
|
||||
|
||||
install = tmp_path / 'hermes-agent'
|
||||
install.mkdir()
|
||||
lock = install / '.hermes-update.lock'
|
||||
lock.touch()
|
||||
dead = f'{_dead_pid()}\n{int(time.time())}\nct:5.000\n'.encode()
|
||||
(tmp_path / MARKER).write_bytes(dead)
|
||||
k32 = ctypes.WinDLL('kernel32', use_last_error=True)
|
||||
k32.CreateFileW.restype = ctypes.c_void_p
|
||||
handle = k32.CreateFileW(str(lock), 0x80000000, 0, None, 3, 0x80, None) # GENERIC_READ, no sharing
|
||||
assert handle not in (None, ctypes.c_void_p(-1).value), ctypes.get_last_error()
|
||||
try:
|
||||
assert _op(tmp_path, '-MarkerOp', 'reclaim')[:2] == (0, 'held\n')
|
||||
assert (tmp_path / MARKER).read_bytes() == dead
|
||||
finally:
|
||||
k32.CloseHandle(ctypes.c_void_p(handle))
|
||||
assert _op(tmp_path, '-MarkerOp', 'reclaim')[:2] == (0, 'reclaimed\n')
|
||||
|
||||
|
||||
@pytest.mark.platforms('windows')
|
||||
@pytest.mark.parametrize('lease', [1, 16])
|
||||
def test_marker_op_reclaim_reports_held_while_a_leased_child_outlives_its_owner(tmp_path: Path, lease: int) -> None:
|
||||
|
||||
Reference in New Issue
Block a user