feat(runtime-host): add safe managed Host updates - #3591
Conversation
9031458 to
2d969f3
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head 2d969f36c7c92e14c024092ddaa1f28c33d99bd1 (29 files, +1938/-60). Exact-head test, audit, and package are all completed/success.
No [P0]–[P1]. One [P2] that is about merge sequencing rather than the code itself, plus two [P3] notes.
The lock design holds up
process-lifetime-file-update-lock.ts gets the hard part right: stale-lock recovery does not depend on a PID. Authority lives in an OFD/flock-class lock on the .lease file, which the kernel reclaims when the holder dies — fs-native-extensions uses fcntl(F_OFD_SETLK) on Linux, flock on macOS, and LockFileEx on Windows, all per-open-file-description, so two descriptors in the same process still conflict and the in-process lockGates map is a scheduling optimization rather than a correctness crutch.
The .lock marker is only created while the lease is held, which is what makes a leftover regular-file marker provably stale and safe to take over; legacy directory locks are waited on and never stolen. O_NOFOLLOW plus the dev/ino double-stat closes lease-file substitution.
Verified with a three-process probe: concurrent entry is refused, and after SIGKILL a new process recovers the lock. (The probe's first version reported a false recovery failure — that was a stray retrying process in the probe itself, not product behavior.)
The update path fails closed
The staging → retire → cutover split is coherent. Staging runs inside the lifecycle lock and rolls back without touching the service; active_tasks or a retire failure both roll staging back and exit cleanly; the pre-cutover re-read revalidates version, pid, active, and state inside the lock to close the drift window.
The choice not to auto-roll-back after cutover is the right one — storage compatibility is unknown at that point, so an automatic rollback would be the more dangerous option. Raising update_incomplete with a retry-exact-version instruction is the correct shape, and the retry is idempotent because a stopped service returns kind: 'stopped' and continues.
[P2] Merge order with #3602 — two versions of the same fence
This PR and #3602 both evolve runtime-host-service-manager.ts from the same base, and they carry different implementations of the retirement fence.
Here, acquireRuntimeHostRootRetirementFence is the single-attempt form, and — importantly — the retire path does not use it at all. At :401-411 the service.pid !== null branch prepares retirement and stops without ever acquiring a fence; only the service.state === 'starting' branch (:417) and the update path's config-write window fence anything.
In #3602 that same helper became the polling form with an identity re-check on every iteration, and it is wired into the main retire path immediately after prepare succeeds.
These conflict by construction. If this PR merges first and #3602 is then rebased onto it, the hardening can be silently dropped in the conflict resolution — the result would compile and pass tests while quietly losing the drain proof.
Suggested sequence: land #3602 first, then rebase this PR onto its signature. The extra parameters are optional, so adopting it is compatible and the resolution becomes additive rather than a choice between two versions.
[P3] ×2
- The update action's fence covers only the config-write window. Between release and install/start there is a gap where a manually launched host could take the root. Bounded — the old host is already retired and the config already points at the new deployment.
.leasefiles are never cleaned up, so each target permanently accumulates one small0600file. Hygiene only.
Validation
Ran: the storage lock tests plus the three-process probe described above; CLI service-manager tests 18/18, including the 292 lines of new update tests; and a re-check that the three exact-head hosted checks are genuinely green.
Not run: the SSH terminal and desktop dialog flows on real hardware. CI's unit coverage exercises them, but no one has performed a real-machine update rehearsal — worth a maintainer or dedicated Windows/macOS pass before this is treated as field-verified. The secondary surfaces (runtime-host-ssh-terminal.ts, preload contract) were read line by line: presentation plumbing and frame-type narrowing, no findings.
|
@Astro-Han Thank you for the careful review. I agree with the merge-sequencing finding. #3591 will not merge before #3602. Once #3602 lands, I will rebase this branch onto the resulting
I do not plan to duplicate #3602's implementation here before it lands, because that would create two independently evolving copies of the same authority boundary and make the eventual conflict less safe. I also re-evaluated the two P3 notes:
The newer head AI disclosure: Codex drafted and posted this response under maintainer direction. |
Astro-Han
left a comment
There was a problem hiding this comment.
Incremental review at exact head a9817b49608ab40503d591981b681f75a3386c24. This is a substantial change from the previously reviewed 2d969f36c (+2262/-59, with the deltas landing on the two axes that mattered last time), so nothing below is carried forward — it was re-derived.
No [P0]–[P2]. Two observations recorded, and the previous two [P3] items are unchanged.
The lock's two invariants survive the new supervision layer
withLegacyFileUpdateLockLease (process-lifetime-file-update-lock.ts:24-58) adds a .supervised marker and recoverSupervisedLegacyLock, in service of running retire through the old operator — the parent holds the modern lease and passes the fd to a detached child via stdio.
The important property is preserved: liveness is still decided solely by whether the OFD lease can be acquired. The .supervised marker is evidence, never a lock — a surviving marker proves the directory lock is ownerless only once a later process actually acquires the lease, at which point the directory lock can be removed safely. The crash window is self-consistent: if the operation doesn't complete the marker stays, which is exactly the state that means "the legacy child may still be alive," and the recovery path consumes it. The regular-file and symlink checks mirror the existing code.
The fd-inheritance design is the part that makes this work rather than a hazard: because the lease travels with the open file description, a killed updater leaves the child holding the lock until it finishes, and an exact retry queues on the lease instead of stealing in-progress work.
The update path keeps its failure direction, and closes one real gap
Three substantive changes, all in the safe direction:
- the
already_currentfast path now has to passverifyReady, so a broken-but-current install enters repair instead of being reported as success; - when repair's retire returns
retirement_failed, the no-interrupt case exits conservatively through the framework rather than forcing anything, and the interrupt case goes through the current operator'sstopbefore replacing; runOperatoris now detached with an inherited lease fd, which is what makes the recovery authoritative rather than best-effort.
The cutover's five-way revalidation, the deliberate absence of auto-rollback once cutoverStarted, and the update_incomplete retry contract are all intact. I looked specifically for a new branch that could leave a half-updated Host and did not find one.
Observations (not graded)
- Diagnostic fidelity. In the no-interrupt repair branch, a
retirement_failedis wrapped into anactive_tasksframe (runtime-host-update-command.ts:209). Conservative and safe, but the operator sees "there are active tasks" when the truth is "retirement failed" — a detour during triage. - Staged deployment is not rolled back on one path. At
:216-218, a failed forcedstopemits the error and returns1without callingdeployment.rollback(), while the neighbouring error paths at:243and:254do. What survives is a staged package on disk that the nextprepareDeploymenthandles, so this is hygiene rather than a correctness gap.
Still open from the previous round
Both earlier [P3] items are unchanged: the update action's fence still covers only the config-write window, and .lease files still accumulate. The .supervised marker also survives a crash, but the recovery path consumes it, so it doesn't add to that.
The merge-ordering [P2] from the previous comment is also unchanged — acquireRuntimeHostRootRetirementFence at :1054 is still the single-attempt form and the retire main path at :461-467 still does not fence. That remains a sequencing matter between this PR and #3602, not a code defect here.
CI
audit and package are green at this head; test was still in_progress when this was written, so no gate conclusion follows from it. Please read its terminal state directly rather than inferring it from the other two.
Ran: CLI service-manager tests 18/18 locally, including the 105 changed lines, plus line-by-line verification of the three changed files. Not run: any real-machine Windows update.
Add one exact-package replacement transaction that stages the target, retires the current Host under exact identity fences, and reports success only after replacement readiness. Keep unsafe rollback disabled once cutover may have started, and expose the same operation through Desktop SSH management. Generated-by: Codex
Require the loaded service identity and a ready managed process before reporting an update as current. Recover Runtime Host management locks after process exit, preserve operator failure codes, and wait for Desktop to reconnect to the exact Host with interactive SSH fallback when needed.
Keep legacy operator ownership recoverable across updater interruption and repair an exact but unready service through the same replacement transaction. Require an exact systemd deployment and a fresh Desktop Host generation before reporting success.\n\nGenerated-by: Codex
a9817b4 to
d8c164d
Compare
English#3602 has now merged, and this PR has been rebased onto the resulting Post-rebase validation passed: the full test build, 31 focused storage/CLI/Desktop behavior tests, type-checking for all three affected workspaces, Biome, and Please re-review the current head when convenient. 中文#3602 现已合并,本 PR 已 rebase 到合并后的 rebase 后验证均已通过:完整测试构建、31 个 storage/CLI/Desktop 针对性行为测试、三个受影响 workspace 的 typecheck、Biome,以及 方便时请基于当前 head 重新审查。 |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the rebase — picking up the prepared-generation retirement fence from #3602 closes the merge-ordering concern from the previous round, and I could not find a second P0–P2 at d8c164dabd13a26bf42b288a1d83a69c80f3e24c. One blocker remains.
[P1] A new-protocol operator deadlocks against its own inherited legacy lease
Retiring an active Host goes through withLegacyOperatorLeases before the operator runs (packages/cli/src/runtime-host-update-command.ts:189-201); the forced stop used by repair reuses the same wrapper (:211-215). That wrapper's operation stays pending until runOperator returns, so the parent holds both leases for the child's entire lifetime.
The wrapper (packages/cli/src/runtime-host-service-manager.ts:278-294) opens and tryLocks the lifecycle .lease and the config .lease, then passes the two descriptors to the child as stdio (runtime-host-update-command.ts:421-433). There is no environment variable or fd-number handshake, and the child CLI has no code path that consumes an inherited descriptor — retire and stop both enter the lifecycle lock normally via runtime-host-service-management-command.ts:81-84, which reaches packages/storage/src/process-lifetime-file-update-lock.ts:66-83 and re-opens the same .lease.
A re-open creates a second open file description, so it contends with the still-held parent lock rather than sharing it. This is not platform-specific: fs-native-extensions@1.5.0 uses F_OFD_SETLK on Linux, flock on macOS and LockFileEx on Windows, and in any case the parent is a genuinely separate process holding the lock while it waits, so the child's re-open collides regardless of how inherited handles are treated.
Reproduced with the production helpers compiled from this exact head and a real spawned child — no mocks and no same-process gate. The parent takes withLegacyFileUpdateLockLease and places the descriptor in the child's stdio; the child calls withProcessLifetimeFileUpdateLock(target, operation, 500). Five out of five runs returned File update is locked by another process with the operation never entered.
Why the green suite misses it: packages/cli/src/runtime-host-service-manager.test.ts:1355-1358 replaces withLegacyOperatorLeases with operation([]), which removes the parent-lock/child-reopen coupling entirely. The storage tests cover recovery after a lock holder is SIGKILLed, but not a child that already inherited a legacy lease and then takes the new lock path.
Impact. The first upgrade from an old directory-lock operator can still succeed. But once the installed operator is this version — or any version with this mechanism — the next active-Host update, and the active-but-unready same-version repair, both fail before retire/stop can start. With the child's 60s service-operation timeout inside the operator's 120s budget, the user-visible behaviour is a consistent ~60s stall followed by failure, not an intermittent race.
Direction. The child has to explicitly consume the inherited lease through a verifiable descriptor protocol, or the parent lease must not overlap the new operator's ordinary lock path — for example by negotiating capability and supervising only operators that are genuinely legacy. Please don't infer that from a version string, and note that raising the timeout or adding retries cannot help here, since nothing will ever release the parent lock while it waits.
A regression test needs the real production seam with an actual spawned child; a mocked wrapper will keep passing.
ran: static call-path trace across the update command, service manager and storage lock at this exact head; a real parent/child spawn reproduction using the production helpers (5/5); the rebase delta against the previous head, confirming the lock coupling and the storage lock are byte-identical.
did-not-run: real SSH/systemd or Desktop end-to-end. Hosted audit was success; test and package were still in progress at the time of writing, so treat those as unverified here — the finding above does not depend on them.
Record the operator lock protocol in managed service configuration. Legacy operators retain supervised directory-lock recovery, while process-lifetime operators run under their own lock authority instead of contending with a parent-held lease. Generated-by: Codex
EnglishConfirmed. The parent-held advisory leases and the new operator's ordinary lock path used different open file descriptions, so the operator deterministically waited on a lock that the parent would release only after the operator exited. Fixed at
The regression coverage now verifies that the legacy transition uses supervision exactly once and that later repair and forced-stop paths bypass it. The full CLI suite passes 404/404, the cross-process storage lock tests pass 2/2, and storage/CLI/Desktop type-checking passes. Please re-review the current head when convenient. 中文确认成立。父进程持有的 advisory lease 与新 operator 的普通加锁路径使用不同的 open file description,因此 operator 会确定性地等待一把只有在其退出后父进程才会释放的锁。 已在
回归验证现在会确认 legacy 迁移只使用一次 supervision,之后的 repair 与 forced-stop 路径均绕过它。CLI 全量测试 404/404 通过,跨进程 storage lock 测试 2/2 通过,storage/CLI/Desktop typecheck 通过。 方便时请基于当前 head 重新审查。 |
Astro-Han
left a comment
There was a problem hiding this comment.
The capability-negotiation approach is the right one, and the original P1 is closed on the normal path. There is one new item at a1d0d2c359063ddecde8da185872259d773a4c8e.
The original self-deadlock is closed
Verified rather than assumed. With an old config carrying no marker, legacy supervision runs exactly once. Once replace succeeds and the marker is on disk, subsequent active repair and the forced stop after a failed retire both run the operator directly, so the parent no longer holds two legacy leases while waiting for a child that re-opens them. The new tests pin the wrapper call counts across all three branches.
I also excluded the obvious ways this could have gone wrong. Update itself calls activate() before the marker is written, and the Host has already retired by then; a failed backend.replace stops the replacement, so its ordinary retry cannot produce a marker-new/operator-old pairing. Concurrent setup and update are closed by the deployment and lifecycle locks together, and an unknown marker value fails closed.
[P2] Setup persists the new-protocol marker before the stable launcher is activated
runtime-host-service-manager.ts:365-375,711-715 writes operatorLockProtocol: 'process-lifetime-v1' into the config as part of manageService(install), which installs and starts the new Host. Only afterwards does deployment.activate() atomically rewrite the stable launcher (runtime-host-setup-command.ts:160-189). If activate() throws, or the process exits between the two, the config claims the new protocol while the stable operator still points at the old directory-lock CLI.
The consumer at runtime-host-update-command.ts:191-197 trusts the marker alone, so the next active update runs the old operator with withLegacyOperatorLeases skipped entirely.
Reachable on supported paths: a development replacement between two *.dev-<sha> versions, or a same-version setup, where the current stable operator is still the old CLI.
Reproduced against artifacts built from this exact head, driving the real runRuntimeHostSetupCli and manageRuntimeHostService with an old stable launcher in place and a genuine activate() failure. Result: setupExit=1, the on-disk config already reads process-lifetime-v1 and the launch CLI is already the new one, while the stable launcher still targets the old binary. Running the real update controller afterwards gave directOperatorCalls=1 and legacyLeaseCalls=0 — it does bypass supervision on the strength of an incorrect marker.
Why this is P2 and not P1. The old operator's directory lock still serializes correctly, so ordinary operations complete. What is lost is the process-lifetime supervision this PR exists to provide: if that old operator dies during retire or stop, it leaves a .lock nobody can reclaim, and subsequent management operations stay wedged until someone deletes it by hand. That is a degraded supported recovery path, not the previous guaranteed 60s stall.
Why the current guards don't catch it. Setup's deployment and lifecycle locks only prevent concurrency, not an exception or process exit. Schema validation checks the marker string, not whether the stable launcher can actually honour it. Setup has no rollback of either the config or the launcher when activate fails.
Direction. Bind the marker to the successful activation of the stable launcher rather than to whoever generates the service config. Either publish the marker only after activation succeeds and roll it back on failure, or move the operator capability into the same authority the launcher updates atomically and verify it before use. A regression test should cover: old launcher present, install writes the marker, activate fails or is interrupted, and the next update still takes the legacy supervision path.
To be clear about scope: hand-edited config is trusted local state and is not the basis for this finding.
ran: full call-path verification at this exact head; a real setup/update reproduction using artifacts built from this head, as described above; local CLI dependency build with 404/404 tests passing; independent confirmation that all three hosted checks are terminal success at this commit.
did-not-run: real SSH/systemd or Desktop end-to-end.
Query the stable operator for its process-lifetime lock capability before retirement. This keeps legacy crash supervision only where it is required and removes the persisted marker that could diverge from the launcher during setup. Generated-by: Codex
EnglishConfirmed. Persisting the lock protocol in service config created a real two-file commit gap with the stable launcher, so changing the write order would only move the inconsistent state to the other side of Fixed at
The regression coverage now checks both capability projection and the transition from a legacy supervised update to later direct repair/forced-stop paths. The full test build passes, the CLI suite passes 404/404, the cross-process storage lock tests pass 2/2, and runtime-host/storage/CLI/Desktop type-checking passes. Please re-review the current head when convenient. 中文确认成立。把 lock protocol 持久化到 service config,会在它与 stable launcher 之间形成真实的双文件提交窗口;单纯调整写入顺序,只会把不一致状态移到 已在
回归验证现在同时覆盖 capability projection,以及从 legacy supervised update 切换到后续 direct repair/forced-stop 的路径。完整测试构建通过,CLI suite 404/404 通过,跨进程 storage lock 测试 2/2 通过,runtime-host/storage/CLI/Desktop typecheck 通过。 方便时请基于当前 head 重新审查。 |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at 455007f91434caad91925d9a749e9e5ae92675de. Both earlier findings are closed, and the way the second one was closed is better than what I suggested.
The config marker is gone entirely. operatorLockProtocol — the field, the write, and the validation — has been removed. The updater now asks the installed operator directly, calling join(serviceConfig.managedDeploymentRoot, 'operator') for process-lifetime-lock-v1 before staging (runtime-host-update-command.ts:180-189), and retire/stop use that same launcher (:207-214). So the two-file mismatch that a failed or interrupted activate() used to leave behind — config claiming the new protocol while the stable launcher was still the old CLI — is no longer representable. I had suggested binding the marker to successful activation; removing the persisted claim altogether is the stronger fix.
Three candidates were examined and all three retracted:
The capability report is an env echo rather than a live probe. requestedOperatorCapabilities() (runtime-host-service-management-command.ts:188-195) echoes back one of the two recognized requests instead of probing the lock. The mechanism is real, but at this head only a build containing the new constant and projection branch can return that value, and that same build's service, config and lifecycle paths already use the process-lifetime lock — byte-identical since a1d0d2c35. An older legacy operator only knows access-management-v1, returns empty capabilities for the new value, and keeps legacy supervision. No supported artifact path was found where an operator that does not use the new lock can report the new capability.
A missed report would revive the original self-deadlock — mechanism confirmed, reachability retracted. Using the production updater's spawn with the real legacy lease helper and the real process-lifetime lock: with normal environment the operator reports the capability, retires directly, exits 0; with the environment deliberately stripped from the launcher it falls back to legacy, the parent holds the lease while the child re-opens, and it exits 1 with File update is locked by another process. That failure is real, but production spawn copies process.env and injects the request (:463-473), and the generated stable shell launcher execs verbatim (runtime-host-managed-deployment.ts:278-296), so no ordinary timing loses it. An error frame or a non-status action does not degrade either — it fails closed at :431-444.
The extra status call before staging. It runs before any staging, is read-only, and happens while the updater already holds the deployment lock. A forced status error produced only checking and error frames with the operator's own message preserved — no staging, no retire.
[P3] The new regression tests still mock the seam the bug lived in
The added coverage (runtime-host-service-manager.test.ts:1366-1467) mocks runOperator and the legacy wrapper, and the in-process capability test mocks manage. The parent-lock/child-reopen coupling and the env-propagation path were verified here with external probes against production code, but those probes do not live in the repository, so nothing in the suite would catch a regression in either. This does not block the change; the earlier request for a true-spawn production-seam regression still stands as follow-up.
ran: full call-path verification at this exact head; real subprocess probes through the production updater for both the normal and env-stripped cases; a forced status-error probe; local Biome and diff-check, focused 19/19, CLI 404/404, storage 2/2, CLI build; confirmation that test, package and audit are all terminal success at this commit and that no [P0]–[P2] finding is anchored to this head.
did-not-run: real systemd/launchd, SSH or Desktop end-to-end, and macOS/Windows update rehearsals.
English
Summary
Add a safe manual update transaction for managed Linux Runtime Hosts. The exact target package is staged first, the current Host retires through its stable operator, and the service is switched only under exact service and State Root fences. Success requires the replacement to be ready and report the target version; once cutover may have started, Maka retains both deployments and asks the operator to retry the exact target instead of claiming an unsafe rollback.
Desktop exposes the same operation from each managed SSH Host. It installs the package selected for that Desktop build, shows update progress and exact version results, and requires explicit confirmation before interrupting active work.
Fixes #3579
Verification
npm run lintnpm run format:checknpm run typechecknpm run buildnpx knip --workspace apps/desktopmain; fix(ci): add missing ASF headers to session status files #3589 is the passing fixUI evidence
AI use
Select exactly one:
Tool(s) and scope: Codex assisted with implementation, tests, Linux/Desktop verification, and pull request drafting under the contributor's direction
Checklist
Does this PR entail a change in behavior?
简体中文
摘要
为 Linux managed Runtime Host 增加安全的手动更新事务。目标精确 package 会先完成 staging,当前 Host 再通过稳定 operator 退场;service 切换始终受精确 service identity 与 State Root fence 约束。只有 replacement ready 且报告目标版本时才算成功;cutover 一旦可能开始,Maka 会保留两份 deployment,并要求 operator 重试同一目标版本,而不会虚假声称完成了不安全的 rollback。
Desktop 在每个 managed SSH Host 的管理界面提供同一操作:安装当前 Desktop 构建所选择的 package、展示更新进度与精确版本结果,并在中断 active work 前要求用户明确确认。
修复 #3579
验证
npm run lintnpm run format:checknpm run typechecknpm run buildnpx knip --workspace apps/desktopmain新增的两个文件阻断;fix(ci): add missing ASF headers to session status files #3589 是已通过检查的修复UI 证据
AI 使用
请选择一项:
工具与范围:Codex 在贡献者指导下协助实现、测试、Linux/Desktop 验证与 Pull Request 文案整理
检查清单
本 PR 是否改变行为?