Skip to content

fix(desktop): drain untracked Runtime Host before update - #3382

Merged
M4n5ter merged 1 commit into
apache:mainfrom
liugddx:fix/authenticated-runtime-host-update-drain
Aug 21, 2026
Merged

fix(desktop): drain untracked Runtime Host before update#3382
M4n5ter merged 1 commit into
apache:mainfrom
liugddx:fix/authenticated-runtime-host-update-drain

Conversation

@liugddx

@liugddx liugddx commented Aug 21, 2026

Copy link
Copy Markdown
Member

Summary

Prevent a Desktop-managed Runtime Host candidate from becoming an untracked winner while Desktop hands control to the updater.

Desktop now creates one process-scoped candidate launch barrier and uses it for every local Runtime Host candidate connection. Before an ephemeral update, the manager quiesces reconnect, pauses new candidate launches, reads the current Host PID from its authenticated diagnostics connection, and retires every other manager-owned candidate attempt while the current Host still owns the root. It then asks that authenticated current Host to prepare for upgrade and waits for its process to exit before dispatching the installer.

If active tasks block preparation, candidate admission and reconnect resume. The immediate confirmed retry (allowInterruptActiveTasks: false -> true) remains bound to the same current Host.

Fixes #3340

Scope

This is deliberately limited to candidates launched by the current Desktop manager and the current authenticated Host.

It does not perform a post-drain root-wide discovery pass, drain Hosts created by other clients, enumerate processes, terminate by a reacquired PID, introduce a general LaunchLease, or change Runtime Host registration/election semantics. The authenticated PID is used only to exclude the adopted child from manager-owned candidate retirement and to wait for the prepared Host to exit.

Verification

L1, local on exact head 0bd87531:

  • npm run lint (2462 files)
  • npm run format:check (1546 files)
  • npm --workspace @maka/desktop run typecheck (all four Desktop TypeScript configurations)
  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/desktop run build:main
  • git diff --check

L2, local:

  • candidate launch barrier tests: 4 passed, 0 failed
  • affected Desktop candidate/manager/app-update-install/app-update-service tests: 55 passed, 0 failed
  • the manager regression performs the immediate false -> true active-task confirmation retry, verifies the same candidate receives both authorities, and waits for that Host to exit
  • independent local author-gate review: no unresolved P0-P2 findings

The complete runtime-host suite has not produced stable full-green local evidence because an unchanged owned-candidate election startup test is load-sensitive. It is not counted as local passing evidence; the fresh CI run on this exact head is authoritative for the complete suite.

L3, CI:

  • CI run 32469300395 passed in 9m43s, including the complete Runtime Host suite and Desktop e2e
  • Release Windows check 32469300347 passed in 18m06s
  • the packaged automatic-update E2E installed 0.1.11, handed off to the updater, observed the upgraded app relaunch, completed the full packaged smoke, and verified 0.1.11 -> 0.1.12

Review focus

Please independently review this lifecycle/security boundary before merge, especially:

  • all Desktop-local candidate launches using the same manager-owned barrier
  • retirement of committed and pending non-current attempts while the authenticated Host still owns the root
  • active-task and error rollback
  • immediate false -> true confirmation staying on the current Host
  • manager shutdown while a candidate spawn is still pending

M4n5ter re-reviewed and approved exact head 0bd87531 after the root-wide residue path was removed. Fresh CI and Windows L3 are green on that same head.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex inspected #3340/#3348 and the review threads, implemented the narrow lifecycle fix and tests, ran local verification, and used an independent review agent for the author gate.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck, builds, and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes - described under Summary above
  • No

@liugddx
liugddx marked this pull request as draft August 21, 2026 08:08
@liugddx
liugddx force-pushed the fix/authenticated-runtime-host-update-drain branch from c3272be to ed6c5b8 Compare August 21, 2026 08:48
@liugddx
liugddx marked this pull request as ready for review August 21, 2026 09:11

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated lifecycle/security review of exact head ed6c5b8dda90101b94ba451be20680acc29350d2.

I traced the launch barrier through committed-but-pending spawns, pause/retire/resume, manager shutdown, tracked Host exclusion by authenticated diagnostics PID, and the post-exit residue probe. The implementation keeps kill authority on manager-owned child handles (OwnedCandidateAttempt.settle), uses PID only for exact-child exclusion and exit waiting, blocks new local candidate launches during handoff, resumes on active tasks or preparation failures, and excludes service/remote Hosts. The new tests cover late spawn settlement, release-after-close, retirement ordering, rollback paths, authenticated self-drain, and fail-closed discovery. I found no P0-P2 defect.

Required CI and the Windows package/update check are green on this exact head. Merge readiness: not ready yet because repository protection still requires an independent human review; this lifecycle/security boundary should receive that review before merge.

Astro-Han
Astro-Han previously approved these changes Aug 21, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on exact head ed6c5b8. The implementation extends the existing candidate-launch and authenticated Host-drain seams without introducing a parallel execution authority. The remaining inline P2 is a non-blocking lifecycle edge and can be addressed in a follow-up.

AI-assisted review disclosure: Codex performed the code analysis with three independent reviewer agents. The human reviewer reviewed the findings and made the final approval decision for this exact head.

Comment thread apps/desktop/src/main/runtime-host-desktop-manager.ts Outdated

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

[Important] Keep the active-task confirmation connected to the Host it must interrupt

By this point, the tracked Host has already drained and exited. If the separately discovered Host returns active_tasks, resume() only schedules a reconnect and the authenticated connection to that Host is discarded.

The renderer then immediately retries with installUpdate({ allowInterruptActiveTasks: true }), but prepareForUpdate() starts by requiring lifecycle.current and calling quiesce(). If reconnect is still pending, this fails with Runtime Host has no current connection to quiesce; if the residue belongs to a generation the manager cannot adopt, the user's interruption authority may never reach it until its work finishes. The existing test only checks the first active_tasks result and misses this supported two-call flow.

The simplest root fix appears to be removing this second root-wide discovery/drain pass. Every Desktop-local launch now goes through the retained attempt owner, pending attempts are settled before the authenticated current Host drains, reconnect is quiesced, and Desktop is single-instance. A previous Host that owns the root is already the authenticated current Host, while a candidate that fails to acquire ownership exits.

If draining Hosts created by other clients is an intentional product requirement, please preserve the authenticated residue handoff across confirmation—or let the authorized retry reconnect to and drain that residue before requiring a lifecycle current—and add a regression covering the immediate false → true retry with reconnect held pending.

简体中文

[Important] 必须让 active-task 确认流程继续关联到真正需要被中断的 Host

执行到这里时,受跟踪的 Host 已经完成 drain 并退出。如果随后发现的 Host 返回 active_tasksresume() 只会异步安排重连,同时用于认证该 Host 的连接已经被丢弃。

Renderer 接下来会立即使用 installUpdate({ allowInterruptActiveTasks: true }) 重试,但 prepareForUpdate() 一开始就要求存在 lifecycle.current 并调用 quiesce()。如果重连尚未完成,这里会报 Runtime Host has no current connection to quiesce;如果残留 Host 属于 manager 无法采用的其他 generation,用户授予的中断权限甚至可能直到任务自然结束都无法送达。现有测试只验证了第一次返回 active_tasks,没有覆盖真实的连续两次调用。

从根因看,最简单的修复应该是删除第二次 root-wide discovery/drain。现在所有 Desktop 本地 launch 都经过 retained attempt owner;pending attempts 会在当前认证 Host drain 前完成退休;reconnect 已被 quiesce;Desktop 也有 single-instance 保证。此前已经占有 root 的 Host 会成为当前认证 Host,而无法取得 ownership 的 candidate 会直接退出。

如果产品明确要求同时 drain 其他 client 创建的 Host,那么需要跨确认过程保留该认证 residue handoff,或者让授权后的重试先重新连接并 drain residue,再要求 lifecycle current;同时增加一个固定 reconnect 尚未完成、连续执行 false → true 的回归测试。

@liugddx
liugddx force-pushed the fix/authenticated-runtime-host-update-drain branch from ed6c5b8 to 0bd8753 Compare August 21, 2026 09:44
@liugddx

liugddx commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

@M4n5ter Addressed at exact head 0bd8753197274d2f6bddc54bce40d3d18da70df9 by taking the suggested root fix.

The post-drain root-wide discovery/self-drain pass, its boot wiring, helper, and behavior-specific tests are removed. Update preparation now retires manager-owned non-current candidates while the authenticated current Host still owns the root, then drains and waits only for that current Host.

I also replaced the old residue-active-task test with an immediate prepareForUpdate(false) -> prepareForUpdate(true) regression. It asserts that the same current candidate receives [false, true] and that the authorized attempt waits for its PID to exit. Local gates pass: barrier 4/4, affected Desktop tests 55/55, full lint/format, four-config Desktop typecheck, and builds. Fresh CI and Windows L3 are running on the new head.

Please re-review the changes-requested path on this exact head.

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

LGTM. Re-reviewed exact head 0bd87531; the previous finding is resolved by removing the root-wide residue-drain path, and the immediate false → true confirmation regression covers the corrected handoff.

简体中文

LGTM。已复审准确 head 0bd87531;此前的问题已通过删除 root-wide residue-drain 路径解决,新增的连续 false → true 确认回归测试覆盖了修正后的 handoff。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

windows: auto-update silently fails when a Runtime Host process survives the app quit (installer cannot clear it)

4 participants