fix(desktop): clear registered Runtime Host residue before update handoff - #3348
fix(desktop): clear registered Runtime Host residue before update handoff#3348liugddx wants to merge 4 commits into
Conversation
…doff The update drain covered only the tracked connection: installUpdate -> prepareForUpdate -> prepareHostUpgrade drains the host the manager is connected to, but an election winner it never adopted - a late winner, or a survivor from a previous app generation - keeps running from the installation directory the updater is about to replace. Windows has no cross-process SIGTERM, the NSIS upgrade quits when it cannot clear such a process, and the update silently never applies (apache#3340; run 32382283646 captured the orphan's command line, the untouched files and registration, and the retained pre-upgrade backup). After the tracked drain, read the root-scoped host registration (the authoritative live-host record election winners write) through a boot-injected capability closure; a live registered ephemeral host that is not the drained pid is terminated outright - its image is about to be replaced and its committed state is crash-safe by the platform's recovery evidence - and awaited. A residue that survives termination rejects, surfacing install_failed instead of a doomed installer handoff. Normal-quit host survival (session persistence) is untouched: the sweep runs only on the update path. Unit tests pin the sweep: orphan terminated and awaited, drained-pid and service-mode and dead and unregistered residue left alone, an unkillable residue rejects, an unreadable registration degrades to the installer's own handling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Run 32392391944 failed at the packaged renderer smoke with the 30-second CDP deadline and no app stderr - the exact flake family apache#3327 fixes; this branch is based on main, which predates those harness repairs.
jackwener
left a comment
There was a problem hiding this comment.
Automated Codex review — blocking finding on exact head 27adb845a288a080a49a156ad7898ee321991f3b.
P1: prove process identity before terminating the registered PID. In RuntimeHostDesktopManagerImpl.#drainRegisteredHostResidue, the registration may be stale after a Host crash. The current guards prove only that the old registration said ephemeral and that some process now occupies its PID; process.kill(pid) then terminates that process. If Windows has reused the PID before the update, this kills an unrelated same-user process. The PR description acknowledges exactly this execution path, so it is not merely theoretical.
The smallest sound fix is to authenticate the live endpoint immediately before termination using the registration's expected hostEpoch and root identity (the existing connection handshake already validates those values), and fail safe when identity cannot be proved. Add a regression case where a stale registration names a live reused PID and assert that killHost is not called. A liveness probe alone cannot close this gap.
Required conclusions:
- Optimal for the actual problem: no; the post-drain sweep addresses the orphan, but its destructive identity boundary is incomplete.
- Production code to delete: none identified.
- Tests to delete/replace: none; add the stale-registration/PID-reuse case above.
- Deeper refactor: no; reuse the existing authenticated Host identity check at this boundary.
- Ready to merge: no.
- Residual risk/gaps: unrelated-process termination during update; independent human review is also still required.
This affects process safety and update behavior, so it is a protected-area change requiring independent human review under CONTRIBUTING.md after the blocker is fixed.
Codex review blocker on apache#3348: the sweep proved only that the old registration said ephemeral and that some process now occupies its pid; after a host crash leaves a stale registration and Windows reuses the pid, that terminates an unrelated same-user process. The sweep now requires a positive identity proof before the kill, through the mechanism the review pointed at: a boot-injected probe connects through the published control plane only (connectExistingRuntimeHost - no filesystem writes), the existing handshake validates root identity, composition and Host Epoch, and the probe additionally requires the reported epoch to equal the one in the registration the sweep read. A dead endpoint, refused handshake, moved epoch, or probe error all refuse termination - a reused pid cannot answer on the dead host's control plane, so the stale-registration case degrades to the installer's own app-running handling. The default without wiring refuses everything. Regressions: reused-pid stale registration is never killed nor awaited; a probe failure refuses termination without failing the update; the kill path pins verify-then-kill-then-await ordering; the guard-polarity cases inject a proving identity so each stays protected by its own specific guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
@jackwener Addressed at head After implementing the suggested authenticated handshake, I re-reviewed the destructive boundary and found that So this revision removes the untracked registration/PID residue sweep entirely. Update preparation now drains only the Host authenticated by the manager's existing connection and waits for that tracked PID to exit; any untracked process residue remains fail-closed at the installer's app-running gate. The sweep-only boot wiring, injectable kill/liveness surface, and its tests were removed as well (net Local verification:
This avoids terminating any process from stale registration evidence without introducing the broader process-handle/protocol architecture deferred from this slice. Independent human security review is still required before merge. |
Summary
Fixes the update-drain gap behind #3340: a Runtime Host election winner the desktop manager never adopted — a late winner, or a survivor from a previous app generation — keeps running from the installation directory
quitAndInstallis about to replace. Windows delivers no cross-process SIGTERM, electron-builder's NSIS upgrade quits when it cannot clear such a process, and the update silently never applies. CI run 32382283646 captured the orphan's full command line (execution-candidate-main.js --generation 0.1.11, alive 106s after creation, well past its parent's quit), untouched files and registration, and the retained pre-upgrade backup.Mechanism
The existing tracked drain is kept as-is:
installUpdate→prepareForUpdate→host.upgrade.preparewire verb → kernel drain →waitForHostExit(pid). After it, the manager now reads the root-scoped host registration (registration.json, the authoritative live-host record election winners write) through a boot-injected capability closure (resolveExistingStorageRootControlDirectory+readHostRegistration, newly exported from@maka/runtime-host/client), and:lifecycleMode, could be a deployment-owned service host — destructive polarity requires the explicit label), dead, unreadable, or absent leaves the sweep inert, with the installer's own app-running handling as the remaining line;ESRCHon the kill is tolerated (the idle ephemeral residue can self-terminate between the liveness probe and the kill);EPERMstays fatal;install_failedinstead of handing off to an installer that cannot succeed.Normal-quit host survival (session persistence) is untouched: the sweep runs only on the update path.
Stated boundaries and accepted residual risk
expectedHostEpochprobe before the kill) is noted as a follow-up hardening; the trigger requires a crashed registered host, pid reuse, and an update in the same window, and no lane can deterministically exercise it. Flagged by the local review gate and accepted here rather than silently.prepareForUpdatethrows before the sweep (pre-existing behavior); a winner elected after the registration read is inherently uncovered (millisecond window, installer handling remains).Verification
@maka/runtime-host+@maka/desktopbuild and typecheck clean; biome clean;runtime-host-desktop-managersuite 19 pass / 0 fail, including new pins: kill-before-await ordering for the orphan, inert sweep for service/unlabeled/dead/unregistered registrations (each case constructed so removing its specific guard turns it red), unkillable residue → rejection, unreadable registration → degrade to installer handling. An independent review gate ran before push; its three P2s (ESRCH tolerance, destructive polarity on unlabeled registrations, red-signal coverage for the service guard) are fixed in this commit, and its pid-reuse finding is the accepted risk above.release-windows-checkautoupdate gate exercises the real handoff; the orphan race it removes was hitting roughly one in three recent runs.Closes #3340. Relates to #3279 (a stale endpoint owner is a plausible shared root) and #2142 Phase 4 (process-drain evidence).