fix(desktop): let the launch-owner guard retire the owned Host on quit - #4756
fix(desktop): let the launch-owner guard retire the owned Host on quit#4756me2seeks wants to merge 1 commit into
Conversation
28f9bda to
f98e93a
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head f98e93ace880bd392608dbc3bad7b00fc3d1c4e3 (OPEN, MERGEABLE). No P0–P2; one P3 below. Gate stays NO-GO on red hosted test, which fails on an untouched spec.
P3 — the PR body understates the protocol epoch bump
The body says protocol epoch 109 → 110, but the exact diff is packages/runtime-host/src/protocol/index.ts:104 going 112 → 113. Please correct the description so reviewers checking compatibility against the stated numbers do not look at the wrong range.
Gate note (not attributed to this change)
Hosted test fails on the untouched apps/desktop/e2e/quote-selection.spec.ts:22,79-81; windows_recovery passes. Until that red is re-run or waived by maintainers, the gate cannot close either way.
What was checked on this head
Exit cleanup, launch-owner ownership, host resource reclamation, concurrent/reentrant and failure-recovery paths. What I could not judge: real Windows quit-path behavior with a live owned host was not reproduced locally.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Luna-Deep-Qronos 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移、以及 exact-head 的 CI 状态。当前 head 是 f98e93a,可合并。P3 是正文写的 epoch 数字跟实际 diff 对不上,改一下描述。另外线上 test 红在没动过的用例上,门禁等维护者处理。
Desktop quit unconditionally released the candidate launch barrier in RuntimeHostDesktopManager.#close(), which detached the launch-owner guard at the exact moment it was the close authority: the guard then ignored the IPC disconnect that would otherwise have closed the Host, so an owned ephemeral Host could survive a full quit. Quit no longer drives retirement. prepareRuntimeHostQuit only probes Host activity to feed the interruption-consent dialog, and the guard closes the Host after process exit. The synchronous retirement drive, the PID polling, and the force-terminate recovery path are removed. The Host reports upgradeBlockingActivity in host.diagnostics.query so the consent question stays answered by the same authority that gates host.upgrade.prepare, and a guard-triggered close now records the retirement reason instead of exiting like a crash. Closes #4730. Generated-by: Maka
f98e93a to
851407f
Compare
|
Rebased onto current main (851407f). The two hosted |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 851407f against main (a5a99a633), +245/−381 over 13 files, CI green at this head. The branch conflicts with main on one file only, packages/runtime-host/src/protocol/index.ts: the epoch constant and the ledger under it. main is at 117 and the head still declares 113, so merging as is would lower the epoch. Rebase, declare 118, move your entry to the top of the ledger; everything else auto-merges.
The fix is at the owner and is a net removal. #close() on main releases the candidate launch barrier unconditionally (runtime-host-desktop-manager.ts:977), which flips the guard to released and closes its lease, so the Desktop's disconnect no longer retires the owned ephemeral Host, which is #4730. Dropping that release lets the guard do its job; the force-quit / retry dialog and forceTerminateOwnedLocalHost were the mechanism that compensated, and they go in the same change with the #3709 discussion linked. I followed guard close → close({ reason: 'retirement' }) → shutdownReason → exit code 0, and the reopen-after-quit election with draining retrying a fresh candidate under the 45s deadline. The previous P3 (body said 109 → 110) is fixed; the earlier red gates were untouched transcript e2e and are green after the rebase.
P3:
- The quit probe reads
#hasUpgradeBlockingActivity()fromhost.diagnostics.query, amode: 'query'call, but the predicate's#activeCommandOperations > 1was calibrated forhost.upgrade.prepare, a command that counts itself. From the query path one in-flight command from another client reads as idle and quit does not ask. Give the predicate aselfCommands: 0 | 1argument and pass 0 from the diagnostics path (host-kernel.ts:696,881). - During a reconnect
target.lifecycle?.currentisundefinedand the probe returnsnot_owned(runtime-host-desktop-manager.ts:773-776), so quit does not ask even if the Host is busy; the oldquiesce()covered that window with a timeout. If that is the accepted trade, say so in the probe's comment. host-status.ts:87says a missing field means unknown, not idle; the only reader treats missing as clear. With the epoch bump a mixed-version handshake cannot happen, so pick one: drop the optionality or fix the comment.forceTerminateRegisteredRuntimeHostand itsWithDependenciesvariant (client/registered-host-termination.ts:85, exported fromclient/index.ts:49) have no caller left outside their own test onceforceTerminateOwnedLocalHostis gone. Delete them with it, or say why they stay.
Evidence boundary: static read; not run on a real Electron quit, so the timing of an owned candidate that is no longer unref'd at release, and the reopen election duration, are unverified.
AI-assisted review: drafted with Maka; I verified the conflicting file, the epoch values and the orphaned export myself.
Closes #4730. Review context: #3709 comment confirming the guard-detach root cause and the corrected scope after #4709.
Summary
RuntimeHostDesktopManager.#close(). Releasing detached the launch-owner guard at the exact moment it was the close authority, so the guard ignored the IPC disconnect that would otherwise have closed the Host — an owned ephemeral Host could survive a full quit.prepareRuntimeHostQuitonly probes Host activity to feed the interruption-consent dialog; the guard closes the owned Host after the Desktop process exits. The synchronous retirement drive,waitForProcessExitPID polling, and the force-terminate recovery path are removed from the quit flow.upgradeBlockingActivityinhost.diagnostics.query(optional field; decoded as unknown when absent) so the consent question stays answered by the same authority that gateshost.upgrade.prepare.Protocol epoch moves 112 → 113 for the additive diagnostics key: older Clients reject the unknown key on exact decode, so the pair refuses at the handshake.
Why
#4730's guaranteed-timeout symptom was neutralized by #4709's observation margin, but the root cause was untouched:
#close()still released the barrier unconditionally. The driven retirement #3706 added becomes a transitional wrapper; this PR deletes it from the quit path while keepingretireOwnedLocalHostfor the Desktop update-install and managed-service setup flows, which genuinely need a driven retirement.Behavior change
Verification
npm run buildnpm --workspace @maka/desktop run typechecknpx biome checkon all changed filesnode --test apps/desktop/dist/main/__tests__/*.test.js(2092 passed)node --test packages/runtime-host/dist/__tests__/*.test.js(1677 passed, 12 platform skips)AI use
Tool(s) and scope: Maka assisted investigation, implementation, tests, and review; the commit includes the required trailer.
Checklist
Does this PR entail a change in behavior?