From b2678712f5bda9d31d66c5ef2dcf5b7db1dcf53d Mon Sep 17 00:00:00 2001
From: Teknium <127238744+teknium1@users.noreply.github.com>
Date: Mon, 5 Oct 2026 22:37:59 -0700
Subject: [PATCH] fix: an old Desktop never reads a dead owner while the
 Windows hand-off dies

Same gap as the POSIX hand-off: the -MarkerOp custody watcher took line 1
over only after windows.ps1 exited (WaitForExit, then the marker lock), so
an old packaged Desktop reading in between (lines 1 and 2 only) deleted the
marker while the resumed `hermes update` or a completion survivor still
held the checkout.

Start-MarkerCustodian now names the watcher on line 1 under the marker lock
right after starting it, before any update work, and the hand-off judges
its claim as that watcher from then on ($script:MarkerOwner in
New-MarkerContext): delegate publication, heartbeat and release keep
working. Invoke-MarkerRelease stops the watcher only after the release, so
line 1 stays live until then. Invoke-MarkerCustody keeps a claim that
already names it (judged by the creation time the hand-off recorded) and
falls back to the old takeover only when the handover failed.

Tests (Windows CI lane): the kill cell holds the marker lock across
taskkill /F and runs the shipped legacy reader at once; the pre-publication
kill cell and the A7 race expect the custodian on line 1.
---
 scripts/desktop-update/marker-claim.ps1       | 88 +++++++++++++------
 scripts/desktop-update/marker.ps1             |  8 +-
 .../test_desktop_update_windows_marker.py     | 17 +++-
 ...test_desktop_update_windows_marker_lock.py | 17 ++--
 .../desktop_update/windows_handoff_support.py |  8 ++
 5 files changed, 97 insertions(+), 41 deletions(-)

diff --git a/scripts/desktop-update/marker-claim.ps1 b/scripts/desktop-update/marker-claim.ps1
index c694952632..3ee01affb0 100644
--- a/scripts/desktop-update/marker-claim.ps1
+++ b/scripts/desktop-update/marker-claim.ps1
@@ -235,9 +235,10 @@ function Invoke-MarkerRelease {
         Start-Sleep -Seconds 1
         $waited++
     }
-    Stop-MarkerCustodian
+    # The custodian (line 1) is stopped only once the release is done: until
+    # then an old Desktop must keep reading a live owner.
     $lock = Open-MarkerLock
-    if ($null -eq $lock) { Write-HandoffLog "update marker lock stayed busy; leaving the marker to identity-checking readers"; return }
+    if ($null -eq $lock) { Stop-MarkerCustodian; Write-HandoffLog "update marker lock stayed busy; leaving the marker to identity-checking readers"; return }
     try {
         if (-not [System.IO.File]::Exists($MarkerPath)) { return }
         $info = ConvertFrom-MarkerText (Read-MarkerText)
@@ -258,6 +259,7 @@ function Invoke-MarkerRelease {
         Write-HandoffLog "could not release the update marker: $($_.Exception.Message)"
     } finally {
         $lock.Dispose()
+        Stop-MarkerCustodian
     }
 }
 
@@ -283,12 +285,16 @@ function Update-MarkerHeartbeat {
 }
 
 function Start-MarkerCustodian {
-    # An old packaged Desktop judges the marker by lines 1 and 2 alone, and the
-    # heartbeat above runs in this process: killed outright (taskkill /F runs
-    # no finally) it would leave line 1 naming a dead pid while the resumed
-    # `hermes update` or a completion survivor still mutates the checkout. A
-    # hidden watcher process takes the claim over then (Invoke-MarkerCustody);
-    # Invoke-MarkerRelease stops it once our own release is decided.
+    # An old packaged Desktop judges the marker by lines 1 and 2 alone (never
+    # the delegate or the marker lock), and we can be killed outright
+    # (taskkill /F runs no finally) while the resumed `hermes update` or a
+    # completion survivor still mutates the checkout. A takeover after our
+    # death would leave an instant in which line 1 names a dead pid. So, before
+    # any update work starts, a hidden watcher process that outlives us is
+    # named on line 1 and the claim becomes "ours" as that watcher
+    # ($script:MarkerOwner). If we die it keeps the marker until that work is
+    # gone (Invoke-MarkerCustody); Invoke-MarkerRelease stops it once our own
+    # release is done.
     if ($script:MarkerClaim -notin @('claimed', 'adopted')) { return }
     $exe = [Diagnostics.Process]::GetCurrentProcess().MainModule.FileName
     # A trailing backslash would escape the closing quote (CommandLineToArgvW).
@@ -298,6 +304,23 @@ function Start-MarkerCustodian {
         $script:MarkerCustodian = Start-Process -FilePath $exe -ArgumentList $arguments -WindowStyle Hidden -PassThru -ErrorAction Stop
     } catch {
         Write-HandoffLog "could not start the update marker custodian: $($_.Exception.Message)"
+        return
+    }
+    $custodian = $script:MarkerCustodian.Id
+    $probe = Get-LiveProcessCt $custodian -Fresh
+    $lock = if ($probe.Alive -and $null -ne $probe.Ct) { Open-MarkerLock } else { $null }
+    if ($null -eq $lock) { Write-HandoffLog "could not name the update marker custodian; it takes over only if this hand-off dies"; return }
+    try {
+        $ctx = New-MarkerContext
+        $read = Read-MarkerLocked $ctx
+        $info = $read.Info
+        if ($null -ne $info -and $read.Judgement.OwnerState -eq 'ours' -and
+            (Set-MarkerBodyLocked (Format-MarkerBody $custodian $ctx.Now (Format-Ct $probe.Ct) (Get-MarkerDelegateLine $info) $info.Runs))) {
+            $script:MarkerOwner = @{ Pid = $custodian; Ct = $probe.Ct }
+            Write-HandoffLog "update marker names its custodian pid $custodian (hand-off pid $PID)"
+        }
+    } finally {
+        $lock.Dispose()
     }
 }
 
@@ -308,32 +331,39 @@ function Stop-MarkerCustodian {
 }
 
 function Invoke-MarkerCustody([int]$Of) {
-    # -MarkerOp custody: once hand-off pid $Of is gone, if its claim is still
-    # published while its delegate runs or the checkout lock is held, name this
-    # process on line 1 and keep line 2 young until both end (bounded like the
-    # R6 wait), then release. A claim the hand-off released or handed over is
-    # no longer its own: nothing to keep. Only the hand-off ever wrote a
-    # delegate, so an unlocked look finding neither one nor a held checkout
-    # lock is final -- no need to wait out a busy marker lock.
+    # -MarkerOp custody: once hand-off pid $Of is gone, keep its claim -- which
+    # names this process on line 1 since before the update work started
+    # (Start-MarkerCustodian) -- and line 2 young while its delegate runs or the
+    # checkout lock is held (bounded like the R6 wait), then release. Had that
+    # handover failed, take the claim over now. A claim the hand-off released
+    # or handed on is no longer its own: nothing to keep. Only the hand-off
+    # ever wrote a delegate, so an unlocked look is enough to decide.
     $watched = Get-Process -Id $Of -ErrorAction SilentlyContinue
     if ($watched) { $watched.WaitForExit() }
-    $peek = ConvertFrom-MarkerText (Read-MarkerText)
-    if ($null -eq $peek -or $peek.Pid -ne $Of -or ($peek.DelegatePid -le 0 -and -not (Test-CheckoutLockHeld))) { return }
-    $lock = Open-MarkerLock
-    if ($null -eq $lock) { return }
-    try {
-        $ctx = New-MarkerContext
-        $read = Read-MarkerLocked $ctx
-        $info = $read.Info
-        if ($null -eq $info -or $info.Pid -ne $Of -or (Test-ProcessIdentityLive $Of $info.Ct)) { return }
-        $delegate = if ($read.Judgement.DelegateState -eq 'live') { Get-MarkerDelegateLine $info } else { $null }
+    $info = ConvertFrom-MarkerText (Read-MarkerText)
+    if ($null -eq $info -or $info.Pid -notin @($Of, $PID)) { return }
+    $delegate = Get-MarkerDelegateLine $info
+    # Line 1 naming our own live pid is us: judge it by the creation time the
+    # hand-off recorded for us, not one re-derived another way.
+    if ($info.Pid -eq $PID) { $script:MarkerOwner = @{ Pid = $PID; Ct = $info.Ct } }
+    if ($info.Pid -eq $Of) {
         if (-not $delegate -and -not (Test-CheckoutLockHeld)) { return }
-        if (-not (Set-MarkerBodyLocked (Format-MarkerBody $PID $ctx.Now (Format-Ct $ctx.OwnCt) $delegate $info.Runs))) { return }
-    } finally {
-        $lock.Dispose()
+        $lock = Open-MarkerLock
+        if ($null -eq $lock) { return }
+        try {
+            $ctx = New-MarkerContext
+            $read = Read-MarkerLocked $ctx
+            $info = $read.Info
+            if ($null -eq $info -or $info.Pid -ne $Of -or (Test-ProcessIdentityLive $Of $info.Ct)) { return }
+            $delegate = if ($read.Judgement.DelegateState -eq 'live') { Get-MarkerDelegateLine $info } else { $null }
+            if (-not $delegate -and -not (Test-CheckoutLockHeld)) { return }
+            if (-not (Set-MarkerBodyLocked (Format-MarkerBody $PID $ctx.Now (Format-Ct $ctx.OwnCt) $delegate $info.Runs))) { return }
+        } finally {
+            $lock.Dispose()
+        }
     }
     $script:MarkerClaim = 'claimed'
-    Write-HandoffLog "update hand-off pid $Of died while its update still holds the checkout; pid $PID keeps the update marker"
+    Write-HandoffLog "update hand-off pid $Of is gone; pid $PID keeps the update marker while its update holds the checkout"
     for ($waited = 0; ($delegate -and (Test-ProcessIdentityLive $info.DelegatePid $info.DelegateCt)) -or (Test-CheckoutLockHeld); $waited++) {
         if ($waited -ge $script:MarkerReleaseWaitSeconds) { return }
         Update-MarkerHeartbeat
diff --git a/scripts/desktop-update/marker.ps1 b/scripts/desktop-update/marker.ps1
index 0db9e255a2..2747831749 100644
--- a/scripts/desktop-update/marker.ps1
+++ b/scripts/desktop-update/marker.ps1
@@ -36,6 +36,7 @@ $script:MarkerBlocker = 0
 $script:MarkerOwnCtText = $null
 $script:MarkerLastHeartbeat = $null
 $script:MarkerCustodian = $null     # the watcher process Start-MarkerCustodian started
+$script:MarkerOwner = $null         # @{ Pid; Ct } of that watcher once line 1 names it instead of us
 $script:MarkerReleaseWaited = $false   # the R6 wait ran (windows.ps1 re-stamps the result after it)
 $script:StartedAt = $null
 $script:MarkerRunPattern = '\A[A-Za-z0-9._-]{1,128}\z'
@@ -198,9 +199,10 @@ function Get-MarkerDelegateLine($Info) {
 function New-MarkerContext {
     # Who "we" are and how a pid is probed; the corpus test injects its own.
     # Fresh creation times: a judgement here decides a claim, a delegate or a
-    # hand-over, which must never ride a reused pid's cached identity.
-    $own = Get-LiveProcessCt $PID
-    return @{ OwnPid = [int64]$PID; OwnCt = $own.Ct; Now = (Get-UnixNow); Probe = { param($p) Get-LiveProcessCt $p -Fresh } }
+    # hand-over, which must never ride a reused pid's cached identity. Once line
+    # 1 names our custodian (Start-MarkerCustodian), the claim is "ours" as that.
+    $own = if ($script:MarkerOwner) { $script:MarkerOwner } else { @{ Pid = $PID; Ct = (Get-LiveProcessCt $PID).Ct } }
+    return @{ OwnPid = [int64]$own.Pid; OwnCt = $own.Ct; Now = (Get-UnixNow); Probe = { param($p) Get-LiveProcessCt $p -Fresh } }
 }
 
 function Get-MarkerIdentityState([int64]$ProcessId, $RecordedCt, [int64]$StartedAt, $Ctx) {
diff --git a/tests/scripts/desktop_update/test_desktop_update_windows_marker.py b/tests/scripts/desktop_update/test_desktop_update_windows_marker.py
index f123129f7e..e58a8c39eb 100644
--- a/tests/scripts/desktop_update/test_desktop_update_windows_marker.py
+++ b/tests/scripts/desktop_update/test_desktop_update_windows_marker.py
@@ -17,6 +17,7 @@ import pytest
 
 from tests.installation_launcher_fixture import publish_fixture_launcher
 from tests.scripts.desktop_update.legacy_desktop_reader import legacy_read
+from tests.scripts.desktop_update.windows_handoff_support import _HeldLock
 
 ROOT = Path(__file__).resolve().parent.parent.parent.parent
 SCRIPT = ROOT / 'scripts/desktop-update/windows.ps1'
@@ -305,8 +306,10 @@ def test_script_killed_right_after_spawning_the_update_leaves_a_live_marker(tmp_
     """C1 rule 6, written by the script itself: windows.ps1 is killed (taskkill /F, no /T) while
     its `hermes update` child is only starting up and has not taken the update lock. The marker
     must still read LIVE through the child named on line 4 -- also to an old packaged Desktop,
-    which judges line 1 alone, so the hand-off's custodian takes line 1 -- and be released once
-    that child is gone."""
+    which judges line 1 alone, so line 1 names the hand-off's custodian before the update starts
+    and the first old-reader read right after the kill (the marker lock held, so nothing can
+    take over yet) already sees a live owner (review 5423056011) -- and be released once that
+    child is gone."""
     install = tmp_path / 'checkout'
     publish_fixture_launcher(install, HOLD_CLI)
     home = tmp_path / 'home'; home.mkdir()
@@ -329,8 +332,14 @@ def test_script_killed_right_after_spawning_the_update_leaves_a_live_marker(tmp_
         settle = time.monotonic() + 3
         while time.monotonic() < settle and not any(line.startswith('delegate:') for line in _marker_lines(marker)):
             time.sleep(0.01)
-        subprocess.run(['taskkill', '/F', '/PID', str(script.pid)], capture_output=True, check=True)
-        script.wait(timeout=30)
+        a7 = _HeldLock(home)   # no marker mutation (a takeover included) until we let go
+        try:
+            subprocess.run(['taskkill', '/F', '/PID', str(script.pid)], capture_output=True, check=True)
+            script.wait(timeout=30)
+            seen = legacy_read(home)
+            assert seen['live'] is not None and seen['kept'], ('hand-off just died', seen)
+        finally:
+            a7.release()
         child = int(child_pid_file.read_text(encoding='utf-8-sig'))
         deadline = time.monotonic() + 60
         while (lines := _marker_lines(marker))[:1] in ([], [str(script.pid)]):   # the custodian takes over
diff --git a/tests/scripts/desktop_update/test_desktop_update_windows_marker_lock.py b/tests/scripts/desktop_update/test_desktop_update_windows_marker_lock.py
index 362946b810..4e41051941 100644
--- a/tests/scripts/desktop_update/test_desktop_update_windows_marker_lock.py
+++ b/tests/scripts/desktop_update/test_desktop_update_windows_marker_lock.py
@@ -23,6 +23,7 @@ from tests.scripts.desktop_update.windows_handoff_support import (
     _finish,
     _op,
     _HeldLock,
+    _custodian,
 )
 
 
@@ -81,7 +82,7 @@ def test_a7_concurrent_claimants_over_a_dead_marker_yield_one_owner(tmp_path: Pa
         refused = [c for c in claimants if c.poll() == 2]
         running = [c for c in claimants if c.poll() is None]
         assert len(running) == 1 and len(refused) == 3, [c.returncode for c in claimants]
-        assert marker.read_bytes().decode().split('\n')[0] == str(running[0].pid)
+        assert marker.read_bytes().decode().split('\n')[0] == _custodian(home, running[0].pid)
     finally:
         hold.touch()
         for c in claimants:
@@ -113,7 +114,8 @@ exit 1
 def test_script_killed_before_publishing_the_delegate_runs_no_update(tmp_path: Path) -> None:
     """Kill cell at the pre-publication boundary: windows.ps1 has created its `hermes update`
     child but has not published it as the marker delegate (the marker lock is held elsewhere).
-    Killed there, no update instruction may have run and the marker must read DEAD."""
+    Killed there, no update instruction may have run, and the custodian line 1 names releases
+    the marker (nothing holds the checkout)."""
     install = tmp_path / 'checkout'
     publish_fixture_launcher(install, HOLD_CLI)
     home = tmp_path / 'home'; home.mkdir()
@@ -126,7 +128,8 @@ def test_script_killed_before_publishing_the_delegate_runs_no_update(tmp_path: P
     lock = None
     try:
         deadline = time.monotonic() + 60
-        while not (marker.exists() and marker.read_bytes().split(b'\n')[0] == str(script.pid).encode()):
+        while not (marker.exists() and _custodian(home, script.pid)
+                   and marker.read_bytes().split(b'\n')[0] == _custodian(home, script.pid).encode()):
             assert time.monotonic() < deadline and script.poll() is None, 'script never claimed'
             time.sleep(0.02)
         lock = _HeldLock(home)   # after the claim, before the delegate publication
@@ -141,7 +144,7 @@ def test_script_killed_before_publishing_the_delegate_runs_no_update(tmp_path: P
         assert not ran.exists(), 'the update child ran before it was published as the delegate'
         assert not _alive(child_pid), 'the never-resumed update child outlived its script'
         lines = marker.read_bytes().decode().split('\n')
-        assert lines[0] == str(script.pid), lines
+        assert lines[0] == _custodian(home, script.pid), lines
         assert not any(line.startswith('delegate:') for line in lines), lines
     finally:
         if lock:
@@ -150,4 +153,8 @@ def test_script_killed_before_publishing_the_delegate_runs_no_update(tmp_path: P
         if script.poll() is None:
             subprocess.run(['taskkill', '/T', '/F', '/PID', str(script.pid)], capture_output=True)
             script.wait()
-    assert _op(home, '-MarkerOp', 'reclaim')[:2] == (0, 'reclaimed\n')
+    deadline = time.monotonic() + 60   # the custodian releases the marker once the lock is free
+    while (verdict := _op(home, '-MarkerOp', 'reclaim')[1]).startswith('live '):
+        assert time.monotonic() < deadline
+        time.sleep(0.2)
+    assert verdict in ('absent\n', 'reclaimed\n')
diff --git a/tests/scripts/desktop_update/windows_handoff_support.py b/tests/scripts/desktop_update/windows_handoff_support.py
index 3daa02f31e..3b731ff584 100644
--- a/tests/scripts/desktop_update/windows_handoff_support.py
+++ b/tests/scripts/desktop_update/windows_handoff_support.py
@@ -3,6 +3,7 @@ from __future__ import annotations
 
 import os
 from pathlib import Path
+import re
 import subprocess
 import sys
 import time
@@ -91,6 +92,13 @@ def _log(home: Path) -> str:
     return path.read_text(encoding='utf-8-sig') if path.exists() else ''
 
 
+def _custodian(home: Path, handoff: int) -> str:
+    """The pid windows.ps1 names on line 1 before any update work starts: its custodian, which
+    outlives the hand-off so an old Desktop never reads a dead owner ('' until it is named)."""
+    found = re.search(rf'names its custodian pid (\d+) \(hand-off pid {handoff}\)', _log(home))
+    return found.group(1) if found else ''
+
+
 class _HeldLock:
     """Hold the A7 sidecar lock from another process (this one): an open handle
     conflicts with the script's FileShare.None open."""