Repository navigation
fix(desktop): recover deleted WSL Node bindings - #6114
Conversation
Recover only when the persisted WSL Node file is missing. Reuse login-shell bootstrap and the existing deployment transaction while retaining the exact Host package and refreshing Desktop bindings. Fixes apache#6112 Generated-by: Codex
Record the POSIX-shell recovery tests in the generated Windows test inventory so CI can validate the complete exclusion set. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
At f5b5fc1dd5642cbf21705936c3e770b0902a2568, I found one P2: a late recovery write can recreate a deleted Desktop deployment binding and prevent onboarding that WSL deployment again (apps/desktop/src/main/runtime-host-profile-service.ts:800). The profile-catalog update is conditional, but the subsequent management-binding write is not fenced against removal. A management status request can invoke recovery outside the enable mutation queue. The inline comment describes the reproduced interleaving.
The missing-file recovery itself works in the cases I checked. The preflight distinguishes a genuinely missing path from an existing executable, a non-executable, a dangling symlink, inaccessible parents, and non-directory parents. Host startup/probe failures do not trigger discovery. Eight concurrent repairs against real temporary deployment files converge to one revision increment; stale retries are idempotent, the exact package descriptor stays unchanged, and the operator launcher points to the recovered Node. The default repair entrypoint rejects this non-WSL Linux environment. Bypassing only the recovery callback makes the missing-file regression fail.
The normal build, 180 related Host/CLI/Desktop tests, full Desktop typecheck, repository lint/format checks, and Windows skip-inventory checks pass. I also reproduced the deletion race with the real file-backed catalog, management store, and public profile-service methods, delaying only the management write. Suppressing that late write in a diagnostic control permits removal and re-onboarding; that control is not a complete concurrency fix. No live deployment or credentials were changed, and I did not run native Windows/WSL or a complete repository test suite.
There is no wire/schema change; the compatibility epoch remains 222. The synthetic merge with freshly fetched main 1751c399239b0f6b717e98e2a5e2b875c3dd11ce is conflict-free. Current-head CI reports 12 successful checks and one intentionally skipped Eval check, but those checks do not cover the reproduced race. Please address the P2 before merge.
Keep the short catalog and management binding writes in one queue shared with removal, without reentering the enable lifecycle queue. Cover the late-save removal race, stale retries, and re-onboarding with the file-backed stores. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
The previous P2 about a late recovery save resurrecting a deleted profile's management binding is addressed at 79ccf520d493787facec0f7fa33f83f5bd192bf2. I found no new P0–P2 issue within this fix. This is a narrow follow-up to f5b5fc1d, not a new full review of the PR.
The increment changes only the profile service and its regression test. apps/desktop/src/main/runtime-host-profile-service.ts:782 serializes the catalog read/update and management-binding save together; removal takes that same binding queue at line 1332. The binding queue is separate from the existing lifecycle queue, so recovery during enable does not re-enter the queue already held by enable. Management status still runs outside the lifecycle queue, but its recovery callback now reaches the protected binding write.
I exercised the registered management status handler, the WSL controller and real file-backed catalog/service stores. With the recovery save paused, removal waits; after releasing the save, both stores are empty and onboarding the same deployment under a new profile ID succeeds. Loading the actual previous profile-service implementation reproduces the orphan binding through this same status path. Bypassing only the new binding queue also reproduces it, and makes the newly added regression test fail.
Five additional scenarios passed: the delayed-save ordering, removal before recovery reaches the projection, concurrent/idempotent recovery, a failed save followed by successful removal/re-onboarding, and enable-time recovery without deadlock. The fixture runs the missing-file shell probe locally, but supplies the external WSL repair result and framed Host response; it is not native Windows/WSL validation or a cross-process/crash-atomicity proof.
The 65 related profile/store/management/WSL-controller tests pass, along with the clean workspace build, all four Desktop type checks, lint/format checks for the two changed files, and git diff --check. There are no protocol changes against main 1751c399; epoch 222 remains unchanged. I did not rerun the full repository suites or modify a live deployment.
GitHub currently reports the branch as mergeable but blocked. The Windows recovery workflow passed, while CI run 38764098728 and the Linux x64 addon job are still queued. These pending checks are not a green result for this head.
Courtneybrown11
left a comment
There was a problem hiding this comment.
Revoke all open API keys secrets rotate
chinawch007
left a comment
There was a problem hiding this comment.
Automated review by Codex. This is an AI review and does not replace the independent human review required by CONTRIBUTING.md.
Reviewed the full PR at 79ccf520d493787facec0f7fa33f83f5bd192bf2 against 1751c399239b0f6b717e98e2a5e2b875c3dd11ce, including issue #6112 and the earlier review discussion. Two new P2 issues need attention before merge:
- Missing-Node recovery prevents the existing WSL device-remount repair from running.
- After Node recovery, the manager's stale profile snapshot makes the compatibility handoff reject the valid management binding and omit the explicit Host upgrade action.
Both findings are detailed inline and reproduced against freshly built artifacts from this commit. The remount case fails with a device-only marker mismatch and succeeds after the existing constrained remount repair. The handoff case rejects the old profile snapshot after recovery but offers the upgrade action with the recovered snapshot.
The previously reported delayed-save/removal race is addressed in this head and is not repeated here. Review also covered probe trigger restrictions, cancellation, live-owner rejection, deployment locking, preservation of the exact Host package/configuration, stale retries, and management/profile lifecycle paths.
Validation: root build passed; 168 tests across 13 related Runtime Host, CLI and Desktop test files passed; Desktop typechecks, repository lint/format checks and diff whitespace checks passed. Additional isolated checks passed for an inaccessible parent, cancellation after probe spawn, and rejection of a changed operator module. Both reported failure/control cases were independently rerun.
The complete repository test suite and native Windows/WSL end-to-end validation were not run. Current CI status could not be retrieved. No product code or tests were changed. HEAD was rechecked before submission.
Reuse constrained device-remount repair before resolving the missing-Node deployment. Accept a validated same-deployment operator after Node recovery and use that current binding for compatibility handoff, retaining source package, deployment, and Host-generation fences. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed the increment from 79ccf52 to 1b3f1ca. This is one appended commit affecting four files; the three earlier commits are unchanged, and both merge-bases remain 1751c39. No new verified P0–P2 issue found in this increment.
Prior findings:
- The missing-Node/device-remount P2 is addressed at
packages/cli/src/runtime-host-wsl-node-recovery.ts:70. Deployment resolution now uses the existing constrained remount repair before Node replacement. The real-file regression succeeds with only the device ID changed; inode, Root ID and live-owner rejection cases still pass without rewriting the deployment/operator. - The stale-profile compatibility-handoff P2 is addressed at
apps/desktop/src/main/runtime-host-wsl-handoff.ts:54–65. The persisted binding must retain the profile ID, WSL distribution, Root ID and operator module; status uses its recovered Node command. The explicit upgrade action remains available after recovery. Its fresh complete-binding check, source version/fingerprint, deployment ID and Host epoch/PID fences remain in place. - The earlier late-save/removal P2 remains fixed. I reran the five file-backed management/profile scenarios, including delayed save versus removal, removal before projection, concurrent retries, failed saves and recovery during enable. They pass on this head.
Both new positive regressions fail when only the two production modules are loaded from 79ccf52: remount resolution rejects the device mismatch, and the stale profile loses its upgrade action. With the current modules, the full relevant cases pass.
I also checked the request to revoke/rotate keys. It supplies no file, line or credential location. I scanned all four PR commits, including removed patch lines, commit messages and the complete changed-file versions, for credential keywords, common token formats, Bearer literals and private-key headers. I found no exposed credentials in these commits, so that request has no verifiable supporting evidence in this PR. No secret values are included in this review.
Validation: fresh dependencies and normal root build:test passed; eleven related test files produced 137 passes and three conditional skips, with no failures. All four Desktop typechecks, Biome on the four changed files, the Windows inventory check (135 declarations) and epoch guard passed. No protocol change; epoch remains 222. Exact-head CI reports 12 successful checks and one skipped optional Eval check. A synthetic merge with main 2ddaa74 is conflict-free, but I did not build that merged tree. I did not run native Windows/WSL, full repository suites or any live deployment repair. The outstanding review state still belongs to its reviewer/maintainers; this comment does not dismiss it.
liugddx
left a comment
There was a problem hiding this comment.
AI review — OpenAI Codex, posted on behalf of @liugddx. This is an automated review and does not replace independent human committer review.
REQUEST_CHANGES — two P2 findings. Reviewed all 22 changed files at 1b3f1cab88128f1e07cf0d4244c997d7b575c2ad against 1751c399239b0f6b717e98e2a5e2b875c3dd11ce. The missing-file-only preflight and reuse of the existing deployment transaction keep the repair appropriately scoped. The earlier removal/write ordering, device-remount, and stale-profile handoff findings are addressed. The two remaining issues are detailed inline:
-
P2: a delayed repair result can overwrite a newly re-onboarded deployment generation.
recordRecoveredWslNodematches the State Root/distribution/operator but unconditionally persists the old result's deployment UUID. Using the real file-backed catalog, management store and public profile-service methods, I removed the original profile, onboarded a replacement with deployment UUID ending in002, then delivered the old recovery result ending in001. The replacement profile stayed present, but its management binding changed from002back to001. A diagnostic control that omitted the stale projection retained002. This is reachable when an external uninstall/reinstall retains the State Root and stable operator path while an earlier management-status recovery is pending; management status is not cancelled by profile removal. The resulting binding fails the existing deployment-identity fence, blocking status/configuration and compatibility upgrade. Fence recovery persistence against the current deployment incarnation and coordinate that conditional write with onboarding. -
P2: the new WSL-only CLI fixtures run unconditionally on native Windows and fail before their assertions. Running the new test file on Windows/Node 22.23.1 produced six failures and one passing parser test. Each functional case calls
convergeRuntimeHostManagedOperator, which follows the Windows task-launcher path and fails resolving the fixture's absentversions/registry-…/dist/cli.js. The fixture also constructs a native Windows operator path while the repair validates a POSIX path. Keep the parser test portable; restrict the WSL filesystem fixtures to supported POSIX test hosts and record the Windows skips, or provide fixtures that genuinely model the required platform.
Validation: seven related Host/CLI/Desktop test files produced 121 passes, 5 conditional skips, 0 failures. The additional new CLI recovery file produced 1 pass and 6 failures as described above. The stale-generation reproduction and its diagnostic control used temporary real stores; no live deployment was changed. Core/Storage/Runtime Host and CLI compilation passed. Full Runtime/Desktop typechecking was not clean in the reused local dependency tree (version/missing-dependency errors outside this diff), so I do not report those builds as passed. git diff --check passed. The frozen changed-file contents were verified against all 22 GitHub blob IDs. Native WSL and full repository suites were not run; hosted CI is green at this head.
Design and tests: the repair belongs at the WSL pre-activation boundary and should continue reusing the existing bootstrap, State Root authority and transaction. No replacement subsystem or extra Node cache is warranted. Keep the new boundary tests; extend the removal/re-onboarding test with a distinct deployment UUID, since its current same-deployment fixture cannot detect the first issue. No whole test suite needs deleting.
The smallest correct change is the current recovery path with a deployment-incarnation fence on Desktop persistence and a platform-correct CLI test boundary. Please address both P2s before merge.
M4n5ter
left a comment
There was a problem hiding this comment.
English
I found two correctness issues in the recovery path:
-
Recovery can fail when a deployment has both a managed profile and a CLI-created alias. The recovery loop writes every matching profile into the managed-service store, which rejects a second profile bound to the same deployment. Recovery then fails after the catalog has already been updated. Updating the operator should preserve the existing management enrollment.
-
An interrupted Node repair cannot recover on retry. If the process exits after beginning the configure transaction, the next attempt rejects the non-active deployment record, including the repair’s own recoverable Node-only transition. Repeated reconnects keep failing.
Both were reproduced locally and should be addressed before merging.
I also tried a smaller implementation that reuses runtime-host setup --reuse-existing-environment, keeps the recovery decision in the Desktop WSL controller, and updates the operator through the profile catalog. This removes the dedicated recovery command, public recovery callback chain, additional queue, and management-store writes.
The prototype addresses both issues while retaining missing-only recovery, the installed Host package and configuration, and the refusal to interrupt a running Host. Against the same base, the full diff drops from 22 files, +1119/−22 to 17 files, +655/−28; test additions drop from 608 to 275 lines by reusing existing fixtures.
Related tests: 193 passed, 3 skipped. The native WSL interruption/retry probes also passed. Windows Desktop GUI end-to-end testing remains outstanding.
Patches for comparison:
中文
恢复路径中发现了两个正确性问题:
-
同一部署同时存在 managed profile 和 CLI 创建的别名时,恢复可能失败。 恢复循环会把每个匹配的 profile 写入 managed-service store,而该 store 不允许第二个 profile 绑定同一部署。因此,catalog 已经更新后,恢复仍会报错。更新 operator 应保留原有的管理归属。
-
Node 修复被中断后,重试无法自动恢复。 如果进程在 configure 事务开始后退出,下次尝试会拒绝非 active 的部署记录,其中也包括这次修复自身留下的、可以恢复的 Node-only 事务。反复重连仍然失败。
这两个问题均已在本地复现,建议合并前修复。
我也尝试了一个更小的实现:复用 runtime-host setup --reuse-existing-environment,把恢复判断留在 Desktop WSL controller,并通过 profile catalog 更新 operator。这样可以删除专用恢复命令、公共 recovery 回调链、额外队列和 management-store 写入。
原型修复了上述问题,同时保留关键边界:只恢复缺失的 Node,保留已安装的 Host 包和配置,不打断运行中的 Host。相对同一个 base,完整 diff 从 22 个文件、+1119/−22 缩减为 17 个文件、+655/−28;通过复用现有 fixture,测试新增从 608 行降到 275 行。
相关测试 193 项通过、3 项跳过,真实 WSL 中断与重试实验也通过了。Windows Desktop GUI 端到端测试尚未执行。
供比较的补丁:
Reuse existing-environment setup and constrain transaction recovery to Node-only configure plans. Update only catalog routes, preserving management enrollment and deployment generations. Keep POSIX fixture boundaries explicit. Adapted the reviewer-provided comparison patch from M4n5ter: https://gist.github.com/M4n5ter/f8b223703a2f719ddf97a8617619d383 . Existing Apache-2.0 source headers are retained. Added distinct-generation, cancellation, identity and platform regression coverage. Generated-by: Codex
Pushed
The missing-file decision now stays in the Desktop WSL controller and reuses Validation: 280 related tests passed, 3 conditional skips; full repository build and typecheck, Desktop/UI knip, repository lint/format, Windows inventory and diff checks passed. New-head CI is pending. Full repository tests and native Windows/WSL E2E were not run locally. No live deployment was modified. The PR description now reflects this implementation; another look at this material simplification would be useful. |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed 22faf8779930dc30c88ae027b72431ee36d7c035, including the simplification after 1b3f1cab. I found no new P0–P2 issue in this increment.
The four earlier commits are unchanged; this is one appended commit. Recovery now reuses explicit existing-environment setup. It retains the installed package and credentials, refuses an existing Node path or live Host owner, and only retries a matching interrupted Node-only configuration transaction. Ordinary discovery remains read-only.
The previous profile-removal problem remains resolved: recovery no longer writes management bindings, and catalog updates require the old route to remain current. Tests exercised aliases, deletion followed by reinstallation, constrained remount repair, and upgrade handoff with changed Node paths. Removing remount repair or loading the earlier handoff implementation made the corresponding regression fail.
Validation: clean full build:test, 167 tests across 11 relevant files, all four Desktop TypeScript configurations, and the Windows test inventory passed. The current head has 12 successful CI checks and one optional skipped check; a static merge with fetched main is clean. I did not run native WSL or build the merged tree.
liugddx
left a comment
There was a problem hiding this comment.
AI follow-up review — OpenAI Codex, posted on behalf of @liugddx. This is not independent human committer approval.
COMMENTED: the two prior P2 findings are addressed; I found no new P1/P2 issue in 22faf8779930dc30c88ae027b72431ee36d7c035.
The recovery path was simplified in the right direction. It now reuses the existing WSL setup transaction and updates only the profile catalog through a conditional operator write; it no longer writes the managed-service binding from the recovery callback. That removes the stale deployment-generation overwrite I reproduced on 1b3f1cab: a delayed result cannot replace a newly re-onboarded binding's deployment UUID. The new controller test also uses a distinct ...0002 deployment UUID and verifies the replacement binding remains unchanged while the recovery is in flight.
The new WSL authority test is explicitly skipped on native Windows, leaving the parser coverage portable and keeping POSIX filesystem/WSL fixtures from entering the Windows task-launcher path. This resolves the six fixture failures I reported against the previous head.
The earlier device-remount and stale-profile handoff findings remain covered by the retained constrained remount and recovered-operator paths. The current hosted checks are green (with the optional Eval check skipped). I did not run native WSL or the full repository suite locally.
Net: no blocking finding remains from my previous review. The smallest correct version is this catalog-only recovery simplification with the platform-scoped regression test.
liugddx
left a comment
There was a problem hiding this comment.
AI review — OpenAI Codex, posted on behalf of @liugddx. This is not a substitute for the independent human committer review required by
CONTRIBUTING.md.
APPROVE at 22faf8779930dc30c88ae027b72431ee36d7c035.
The final simplification addresses the two P2s from my previous review: recovery now reuses the existing-environment setup transaction and updates only current profile-catalog routes, so a delayed result cannot rewrite a newly re-onboarded deployment binding; the distinct-generation regression covers that case. The WSL filesystem tests are explicitly POSIX-only, while parser coverage remains portable, so the prior native-Windows fixture failures are addressed.
The earlier remount and compatibility-handoff fixes remain present. Hosted CI is green across the test, platform recovery, addon, package validation and State Root checks, with only the optional Eval check skipped. The latest independent review also reports the focused build/test and inventory checks passing. I did not run native WSL or the full repository suite locally.
No blocking correctness issue remains in the reviewed head.
Summary
A Desktop WSL Host cannot reconnect after mise/nvm deletes the Node executable recorded during setup, even when another supported Node is installed. Detect the missing file before activation and reuse the WSL login-shell bootstrap to find the current Node. Repair the existing deployment through its durable transaction and refresh Desktop's operator binding while preserving the exact Host package, deployment identity, listeners, and credentials.
Recovery is limited to on-demand WSL deployments. Existing but unusable executables, permission/probe errors, and Host startup/readiness failures never select another Node. A live State Root owner blocks repair. Desktop reuses
runtime-host setup --reuse-existing-environmentwith an expected operator and Root; interrupted repair retries settle only an otherwise unchanged Node-only configure transaction. Ordinary setup discovery remains read-only.Recovery conditionally updates catalog operator routes, including existing CLI aliases, without writing management enrollment or deployment UUIDs. Delayed results cannot recreate removed profiles or overwrite a replacement deployment binding. Constrained device-remount repair and the explicit compatibility-upgrade action are retained. No dedicated repair command, public Runtime Host recovery callback, extra mutation queue, Node cache, or schema is added.
Fixes #6112
Verification
AI use
Tool(s) and scope: Codex investigated the failure, implemented the repair and regression tests, and reviewed the design and code for scope and unnecessary complexity. The commits include
Generated-by: Codex. The simplification adapts M4n5ter’s reviewer-provided comparison patch, retaining the Apache-2.0 source headers and adding further regression coverage.Checklist
Does this PR entail a change in behavior?