From b802f677b500a954dcb060bfb0daeedb20b19e67 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 3 Oct 2026 23:23:52 -0700 Subject: [PATCH] fix: windows hand-off keeps the marker while the checkout lock is held (R6) Release waits (bounded) while any process still holds the checkout lock's byte (hermes_cli/update_lock.py takes it with msvcrt.locking at 1 MiB), and -MarkerOp reclaim answers `held` instead of freeing a dead marker in that state, so a Desktop gate never opens over a surviving completion. posix.sh withdraw now keeps a dead adopter's claim (`foreign`), like marker.ps1. (cherry picked from commit 8a5525d26f036a3c4b5176f11ee4a590c746e2db) --- scripts/desktop-update/marker-claim.ps1 | 15 +++++- scripts/desktop-update/marker.ps1 | 48 +++++++++++++++++++ scripts/desktop-update/marker.sh | 7 ++- .../test_desktop_update_windows_marker_ops.py | 36 ++++++++++++++ 4 files changed, 103 insertions(+), 3 deletions(-) diff --git a/scripts/desktop-update/marker-claim.ps1 b/scripts/desktop-update/marker-claim.ps1 index 053bad878d..ca55d2f036 100644 --- a/scripts/desktop-update/marker-claim.ps1 +++ b/scripts/desktop-update/marker-claim.ps1 @@ -188,6 +188,18 @@ function Invoke-MarkerRelease { # Corpus 'release' under the lock: owner deletes, or hands the claim over # to a live delegate; anything that is not ours is kept. if ($NoMarkerCleanup -or $script:MarkerClaim -notin @('claimed', 'adopted')) { return } + # R6: never while a survivor of the update still holds the checkout lock -- + # the marker would read free while that process still mutates the checkout. + $waited = 0 + while (Test-CheckoutLockHeld) { + if ($waited -eq 0) { Write-HandoffLog "a process still holds the checkout update lock; keeping the update marker until it exits" } + if ($waited -ge $script:MarkerReleaseWaitSeconds) { + Write-HandoffLog "checkout update lock still held after $($waited)s; leaving the update marker" + return + } + Start-Sleep -Seconds 1 + $waited++ + } $lock = Open-MarkerLock if ($null -eq $lock) { Write-HandoffLog "update marker lock stayed busy; leaving the marker to identity-checking readers"; return } try { @@ -236,7 +248,7 @@ function Update-MarkerHeartbeat { function Invoke-MarkerOp([string]$Op, [int]$Desktop, [string]$Run) { # SPEC 6 helper ops (Electron's only way to mutate). Returns the verdict - # line: absent | reclaimed | live | busy | withdrawn | taken | foreign. + # line: absent | reclaimed | held | live | busy | withdrawn | taken | foreign. $lock = Open-MarkerLock if ($null -eq $lock) { return 'busy' } try { @@ -248,6 +260,7 @@ function Invoke-MarkerOp([string]$Op, [int]$Desktop, [string]$Run) { $j = $read.Judgement if ($Op -eq 'reclaim') { if ($j.Verdict -in @('live', 'ours')) { return "live $($j.Owner)" } + if (Test-CheckoutLockHeld) { return 'held' } # R6: a survivor still mutates the checkout if (-not (Remove-MarkerLocked)) { return 'busy' } Write-HandoffLog "marker-op reclaim: removed a $($j.Verdict) update marker" return 'reclaimed' diff --git a/scripts/desktop-update/marker.ps1 b/scripts/desktop-update/marker.ps1 index 9640aadc43..2a8dd24afd 100644 --- a/scripts/desktop-update/marker.ps1 +++ b/scripts/desktop-update/marker.ps1 @@ -29,6 +29,7 @@ $script:MarkerCtTolerance = 2.0 $script:MarkerOwnCtEpsilon = 0.005 $script:MarkerLockTimeoutMs = 10000 $script:MarkerHeartbeatSeconds = 300 +$script:MarkerReleaseWaitSeconds = 7200 $script:ProcessCtCache = @{} $script:MarkerClaim = "none" # claimed | adopted | refused $script:MarkerBlocker = 0 @@ -330,3 +331,50 @@ function Read-MarkerLocked($Ctx) { } . (Join-Path $PSScriptRoot 'marker-claim.ps1') + +function Get-CheckoutLockPath { + # hermes_cli/update_lock.py::checkout_lock_path: /hermes-update.lock, + # else /.hermes-update.lock. + $root = Get-Variable -Name InstallRoot -ValueOnly -ErrorAction SilentlyContinue + if (-not $root) { return $null } + $dot = Join-Path $root '.git' + $gitDir = $null + if ([System.IO.Directory]::Exists($dot)) { + $gitDir = $dot + } elseif ([System.IO.File]::Exists($dot)) { + $first = (([System.IO.File]::ReadAllText($dot)).TrimStart([char]0xFEFF) -split "`n")[0].Trim() + if ($first -like 'gitdir:*') { + $gitDir = $first.Substring(7).Trim() + if (-not [System.IO.Path]::IsPathRooted($gitDir)) { $gitDir = Join-Path $root $gitDir } + } + } + if (-not $gitDir) { return (Join-Path $root '.hermes-update.lock') } + $commonFile = Join-Path $gitDir 'commondir' + if ([System.IO.File]::Exists($commonFile)) { + $common = (([System.IO.File]::ReadAllText($commonFile)).TrimStart([char]0xFEFF) -split "`n")[0].Trim() + if ($common) { + $gitDir = if ([System.IO.Path]::IsPathRooted($common)) { $common } else { Join-Path $gitDir $common } + } + } + return (Join-Path $gitDir 'hermes-update.lock') +} + +function Test-CheckoutLockHeld { + # R6: does some process hold the checkout lock right now? Python takes it with + # msvcrt.locking on one byte at offset 1 MiB (LockFile); probe the same byte and + # give it straight back. + $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 } + try { + try { $fs.Lock(1048576, 1) } catch [System.IO.IOException] { return $true } + try { $fs.Unlock(1048576, 1) } catch {} + return $false + } finally { + $fs.Dispose() + } +} diff --git a/scripts/desktop-update/marker.sh b/scripts/desktop-update/marker.sh index 1b9db05623..ab712808cf 100644 --- a/scripts/desktop-update/marker.sh +++ b/scripts/desktop-update/marker.sh @@ -479,7 +479,7 @@ marker_op_withdraw_locked() { marker_judge "$SEEN" if [ -z "$M_PID" ] || [ "$M_RUN" != "$HANDOFF_RUN" ]; then echo foreign; return 0; fi if [ "$M_PID" != "$DESKTOP_PID" ] && [ "$J_OWNER_STATE" -eq 2 ]; then echo "taken $M_PID"; return 0; fi - if [ "$M_PID" = "$DESKTOP_PID" ] || [ "$J_VERDICT" = dead ]; then + if [ "$M_PID" = "$DESKTOP_PID" ] && [ "$J_OWNER_STATE" -eq 2 ] && [ -n "$M_CT" ]; then rm -f "$MARKER" 2>/dev/null log "withdrew the Desktop's hand-off marker (run $HANDOFF_RUN)" echo withdrawn; return 0 @@ -487,7 +487,10 @@ marker_op_withdraw_locked() { echo foreign } -marker_op() { # reclaim | withdraw -> one verdict line on stdout +marker_op() { # reclaim | withdraw -> one verdict line on stdout (same words as marker.ps1): + # reclaim absent | live | busy | held (dead, but the checkout lock is held) | reclaimed + # withdraw absent | taken (a live adopter carries the run) | withdrawn (this Desktop's + # own bridge, removed) | foreign (anything else, kept -- a dead adopter included) local rc case "$1" in reclaim) marker_locked marker_op_reclaim_locked ;; diff --git a/tests/scripts/desktop_update/test_desktop_update_windows_marker_ops.py b/tests/scripts/desktop_update/test_desktop_update_windows_marker_ops.py index 2faab7ba24..af885575dd 100644 --- a/tests/scripts/desktop_update/test_desktop_update_windows_marker_ops.py +++ b/tests/scripts/desktop_update/test_desktop_update_windows_marker_ops.py @@ -145,3 +145,39 @@ def test_heartbeat_refreshes_line_2_only_for_our_own_claim(tmp_path: Path, sleep foreign = f'{sleeper.pid}\n{started}\nct:{dct}\n' _harness(tmp_path, foreign, 'heartbeat') assert (tmp_path / MARKER).read_bytes().decode() == foreign + + +_CHECKOUT_HOLDER = """ +import msvcrt, os, sys, time +from pathlib import Path +fd = os.open(sys.argv[1], os.O_RDWR | os.O_CREAT | os.O_BINARY, 0o644) +os.lseek(fd, 1 << 20, os.SEEK_SET) +msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) # exactly hermes_cli/update_lock.py::_try_lock +Path(sys.argv[2] + '.ready').write_text('1', encoding='utf-8') +while not Path(sys.argv[2]).exists(): + time.sleep(0.05) +""" + + +@pytest.mark.platforms('windows') +def test_marker_op_reclaim_reports_held_while_a_survivor_holds_the_checkout_lock(tmp_path: Path) -> None: + """R6: the update died (dead marker) but a process it started still holds the checkout lock: + the Desktop's reclaim must not free the marker; it answers `held` until that process exits.""" + install = tmp_path / 'hermes-agent' + install.mkdir() + dead = f'{_dead_pid()}\n{int(time.time())}\nct:5.000\n'.encode() + (tmp_path / MARKER).write_bytes(dead) + release = tmp_path / 'release-holder' + holder = subprocess.Popen([sys.executable, '-c', _CHECKOUT_HOLDER, str(install / '.hermes-update.lock'), str(release)]) + try: + deadline = time.monotonic() + 30 + while not Path(str(release) + '.ready').exists(): + assert time.monotonic() < deadline and holder.poll() is None + time.sleep(0.05) + assert _op(tmp_path, '-MarkerOp', 'reclaim')[:2] == (0, 'held\n') + assert (tmp_path / MARKER).read_bytes() == dead + finally: + release.touch() + holder.wait(timeout=30) + assert _op(tmp_path, '-MarkerOp', 'reclaim')[:2] == (0, 'reclaimed\n') + assert not (tmp_path / MARKER).exists()