fix(desktop): skip idle Host restart prompt - #3433
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for keeping this change focused. The exact-head CI is green, but there is one Host-side lifecycle race that the current client snapshot cannot close. I left the final-state requirement inline so the fix can stay small and authoritative.\n\nAI-assisted review disclosure: Codex performed an independent exact-head review; I verified the lifecycle path, current source, and live PR state before posting this comment.
| const decision = await this.#resolveRestartable(result); | ||
| const activity = result.handshake?.activity; | ||
| const decision = | ||
| activity && activity.activeOperations === 0 && activity.residencies.length === 0 |
There was a problem hiding this comment.
[P1] The first handshake is only a snapshot: after it reports idle, an operation, residency, or non-ready state can begin before the takeover handshake. This branch then chooses restart, while the Host-side takeover currently rechecks only accepted transports, so it can still drain and eventually terminate a Host that is no longer truly idle. Please make the Host the single authority here: during takeover, atomically re-evaluate the existing true-idle predicate (ready, no active operations, no residencies, and the required connection state) and return upgrade_required if it changed. A focused regression test where activity starts between discovery and takeover, plus one non-ready-state case, would cover the race without adding another client-side state machine.
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up on our earlier review at this same head — we re-verified the TOCTOU finding rather than assuming it still held, and it does. Adding one piece of evidence and one test gap; no new blocking findings.
The re-verification: the idle decision is made from the discovery handshake snapshot, and the takeoverHostEpoch takeover handshake happens on the next startCandidate. We walked the path and there is no second activity read in between, so the window is real rather than theoretical.
Reviewed at exact head 5d583341. No new P0–P2. One P3, inline.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
| if (result.kind === 'upgrade_required' && result.restartable) { | ||
| const decision = await this.#resolveRestartable(result); | ||
| const activity = result.handshake?.activity; | ||
| const decision = |
There was a problem hiding this comment.
Additional evidence for the existing P1 — the client's idle predicate is narrower than the Host's.
activity carries a connections field, and this predicate does not read it. A Host with activeOperations === 0, residencies.length === 0, and connections > 0 is treated as idle and restarted with no prompt. If any of those connections is mid-reconnect rather than settled, the restart happens silently underneath it.
This is not a separate finding — it falls inside the "required connection state" part of the P1 we already raised — but it is worth naming explicitly, because it is a predicate gap rather than a timing gap and would survive a fix that only made the check atomic.
[P3] The test helper hard-codes connections: 0, which makes that gap invisible.
upgradeRequired sets connections: 0 unconditionally while activeOperations and residencies are both parameterized. So the three new cases (idle / activeOperations > 0 / residencies > 0) can never exercise a non-zero connection count, and a fix that starts consulting connections would have no test to confirm it.
Two cases worth adding:
connections > 0with zero operations and zero residencies — the direct regression anchor for the predicate gap above.activityundefined — the fallback path. Theactivity &&guard means this correctly falls back to prompting, which is the safe direction, but nothing pins it, and it is the behaviour you would most want to keep if the activity payload ever changes shape.
Summary
When Maka restarts immediately after the Desktop client exits, an idle Runtime Host can still be within its idle grace period. The new client currently opens the restart prompt even though the Host has no active work.
This change restarts a generation-aware Host automatically when:
activeOperations === 0residencies.length === 0Hosts with active operations or residencies still use the existing restart prompt.
Verification
npm --workspace @maka/desktop run build:mainnode --test apps/desktop/dist/main/__tests__/runtime-host-desktop-manager.test.jsgit diff --checkAI use
Tool(s) and scope:
Codex assisted with root-cause analysis, implementation, test updates, and verification.
Checklist
Does this PR entail a change in behavior?