fix(desktop): harden Windows and WSL support - #6068
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| Scope.provide(runScope), | ||
| Effect.matchEffect({ | ||
| onFailure: (error) => finalizeRun(error.message), | ||
| onFailure: (error) => |
There was a problem hiding this comment.
🟡 Medium backend/DesktopBackendManager.ts:982
spec.onFailure is called twice for a single backend exit when the process exit-status read itself fails. The Effect.matchEffect failure branch at line 983 invokes spec.onFailure, then finalizeRun invokes it again because exitObserved was set to true (via onExitObserved) before the error surfaced and stopRequested is false. Each exit-status read failure is therefore reported as two failure notifications (e.g., duplicate WSL diagnostics), potentially with inconsistent restartAttempt values since the second call uses nextState.restartAttempt after the state mutation. Consider suppressing the onFailure call in one of the two paths, or clearing exitObserved / adding a flag so finalizeRun skips the notification when matchEffect already surfaced it.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/backend/DesktopBackendManager.ts around line 982:
`spec.onFailure` is called twice for a single backend exit when the process exit-status read itself fails. The `Effect.matchEffect` failure branch at line 983 invokes `spec.onFailure`, then `finalizeRun` invokes it again because `exitObserved` was set to `true` (via `onExitObserved`) before the error surfaced and `stopRequested` is `false`. Each exit-status read failure is therefore reported as two failure notifications (e.g., duplicate WSL diagnostics), potentially with inconsistent `restartAttempt` values since the second call uses `nextState.restartAttempt` after the state mutation. Consider suppressing the `onFailure` call in one of the two paths, or clearing `exitObserved` / adding a flag so `finalizeRun` skips the notification when `matchEffect` already surfaced it.
|
|
||
| const prebuildDir = path.join(nodePtyDir, "prebuilds", `linux-${options.arch}`); |
There was a problem hiding this comment.
🟡 Medium scripts/stage-wsl-ci-node-pty.mjs:57
stageWslCiNodePty copies pty.node into prebuilds/linux-${options.arch} and writes a marker claiming that architecture, but never checks that runtimeManifest.baseline.arch matches options.arch. When the runtime directory contains an artifact built for a different architecture, its SHA-256 still matches the manifest entry and the script stages it under the requested arch directory, producing a load-time failure instead of failing the staging gate. Consider comparing runtimeManifest.baseline.arch to options.arch and throwing on mismatch before copying.
if (runtimeManifest.baseline?.arch !== options.arch) {
throw new Error(
`WSL ABI manifest arch mismatch: manifest ${runtimeManifest.baseline?.arch ?? "unknown"}, requested ${options.arch}.`,
);
}
+
const prebuildDir = path.join(nodePtyDir, "prebuilds", `linux-${options.arch}`);🤖 Copy this AI Prompt to have your agent fix this:
In file @scripts/stage-wsl-ci-node-pty.mjs around lines 57-58:
`stageWslCiNodePty` copies `pty.node` into `prebuilds/linux-${options.arch}` and writes a marker claiming that architecture, but never checks that `runtimeManifest.baseline.arch` matches `options.arch`. When the runtime directory contains an artifact built for a different architecture, its SHA-256 still matches the manifest entry and the script stages it under the requested arch directory, producing a load-time failure instead of failing the staging gate. Consider comparing `runtimeManifest.baseline.arch` to `options.arch` and throwing on mismatch before copying.
| yield* instance.stop({ timeout: BACKEND_STOP_TIMEOUT }); | ||
| yield* instance.start; |
There was a problem hiding this comment.
🟠 High app/DesktopPowerRecovery.ts:64
reconcileInstanceAfterResume reads desiredRunning from the snapshot taken before the probe, then unconditionally calls instance.start after instance.stop. If the user stops the backend concurrently during the probe/restart sequence, the restart here sets desiredRunning back to true and resurrects a backend the user explicitly stopped. The stale snapshot checked at the top no longer reflects the desired state by the time instance.start runs. Re-read instance.snapshot.desiredRunning immediately before calling instance.start, or use an atomic conditional restart that only starts if the backend is still desired.
yield* instance.stop({ timeout: BACKEND_STOP_TIMEOUT });
+ const after = yield* instance.snapshot;
+ if (!after.desiredRunning) return;
yield* instance.start;🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/app/DesktopPowerRecovery.ts around lines 64-65:
`reconcileInstanceAfterResume` reads `desiredRunning` from the snapshot taken *before* the probe, then unconditionally calls `instance.start` after `instance.stop`. If the user stops the backend concurrently during the probe/restart sequence, the restart here sets `desiredRunning` back to `true` and resurrects a backend the user explicitly stopped. The stale snapshot checked at the top no longer reflects the desired state by the time `instance.start` runs. Re-read `instance.snapshot.desiredRunning` immediately before calling `instance.start`, or use an atomic conditional restart that only starts if the backend is still desired.
| server.close(); | ||
| return; | ||
| } | ||
| process.stdout.write(resultPrefix + String(address.port) + ":" + (usedEphemeralFallback ? "1" : "0") + "\n"); |
There was a problem hiding this comment.
🟠 High wsl/DesktopWslEnvironment.ts:307
buildWslTcpPortAllocationScript binds a TCP port, prints it, and immediately calls server.close(), so allocateTcpPortImpl reports ok: true without actually holding the port. Another process can claim the port after the probe exits but before the backend binds it, causing the backend to fail despite a successful allocation result. Consider keeping the listener open (or handing the bound socket to the backend) so the port stays reserved across the gap between allocation and backend startup.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/wsl/DesktopWslEnvironment.ts around line 307:
`buildWslTcpPortAllocationScript` binds a TCP port, prints it, and immediately calls `server.close()`, so `allocateTcpPortImpl` reports `ok: true` without actually holding the port. Another process can claim the port after the probe exits but before the backend binds it, causing the backend to fail despite a successful allocation result. Consider keeping the listener open (or handing the bound socket to the backend) so the port stays reserved across the gap between allocation and backend startup.
| const libcCompatibilityReason = formatWslLibcCompatibilityReason( | ||
| distro, | ||
| libcFamily, | ||
| glibcVersion, | ||
| ); | ||
| if (libcCompatibilityReason !== null) { | ||
| return { | ||
| ok: false, | ||
| reason: libcCompatibilityReason, | ||
| fatal: true, | ||
| } as const; | ||
| } |
There was a problem hiding this comment.
🟠 High wsl/DesktopWslEnvironment.ts:769
The formatWslLibcCompatibilityReason check at line 769 returns a fatal error for glibc versions older than 2.35, but in development mode (options.allowBuild === true), node-pty is compiled from source against the distro's own libc, which works fine on older glibc distros like Ubuntu 20.04 (glibc 2.31). The check fires before the allowBuild branch, so the source build path is blocked entirely. Gate this compatibility check on the prebuilt/packaged runtime path (bundledNodePath !== null) so dev builds are unaffected.
+ if (bundledNodePath !== null) {
+ const libcCompatibilityReason = formatWslLibcCompatibilityReason(
+ distro,
+ libcFamily,
+ glibcVersion,
+ );
+ if (libcCompatibilityReason !== null) {
+ return {
+ ok: false,
+ reason: libcCompatibilityReason,
+ fatal: true,
+ } as const;
+ }
+ }🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/wsl/DesktopWslEnvironment.ts around lines 769-780:
The `formatWslLibcCompatibilityReason` check at line 769 returns a fatal error for glibc versions older than 2.35, but in development mode (`options.allowBuild === true`), `node-pty` is compiled from source against the distro's own libc, which works fine on older glibc distros like Ubuntu 20.04 (glibc 2.31). The check fires before the `allowBuild` branch, so the source build path is blocked entirely. Gate this compatibility check on the prebuilt/packaged runtime path (`bundledNodePath !== null`) so dev builds are unaffected.
| const portStrategy = input.portStrategy ?? "fixed"; | ||
| const portAllocation = | ||
| preflight._tag === "Ready" | ||
| ? yield* wslEnvironment.allocateTcpPort({ |
There was a problem hiding this comment.
🟢 Low backend/DesktopBackendConfiguration.ts:527
allocateTcpPort probes a port by opening a temporary listener and closing it before the WSL backend is launched, so another Linux process can bind the same port in the gap. The backend then fails to bind effectivePort, which is already advertised in httpBaseUrl and bootstrap, causing an intermittent WSL startup failure. The port must be held reserved until the backend takes over, or the backend should bind port 0 and report its actual listening port back.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/backend/DesktopBackendConfiguration.ts around line 527:
`allocateTcpPort` probes a port by opening a temporary listener and closing it before the WSL backend is launched, so another Linux process can bind the same port in the gap. The backend then fails to bind `effectivePort`, which is already advertised in `httpBaseUrl` and `bootstrap`, causing an intermittent WSL startup failure. The port must be held reserved until the backend takes over, or the backend should bind port `0` and report its actual listening port back.
| Write-SmokeLogs -StdoutPath $stdoutPath -StderrPath $stderrPath | ||
| throw "Packaged portable wrapper exited with code $($process.ExitCode) before producing a lifecycle receipt." | ||
| } | ||
| if ($null -eq $wrapperExitedAt) { |
There was a problem hiding this comment.
🟡 Medium scripts/run-windows-packaged-lifecycle-smoke.ps1:117
The hard-coded 30-second receipt timeout after portable wrapper handoff (line 121) can falsely fail a lifecycle run that stays within the requested $TimeoutSeconds budget. After the wrapper hands off to Electron, Windows startup may take up to 45 seconds and WSL startup up to 120 seconds per cycle (and there are two cycles), so a backend that starts successfully within those declared component limits is killed and reported as a lifecycle failure at 30 seconds post-handoff. This contradicts the outer $TimeoutSeconds deadline (default 300), which should remain the governing bound. Consider deriving the post-handoff window from $TimeoutSeconds remaining time rather than a fixed 30 seconds, or otherwise ensuring it cannot be shorter than the sum of the component readiness budgets the lifecycle is allowed to use.
🤖 Copy this AI Prompt to have your agent fix this:
In file @scripts/run-windows-packaged-lifecycle-smoke.ps1 around line 117:
The hard-coded 30-second receipt timeout after portable wrapper handoff (line 121) can falsely fail a lifecycle run that stays within the requested `$TimeoutSeconds` budget. After the wrapper hands off to Electron, Windows startup may take up to 45 seconds and WSL startup up to 120 seconds per cycle (and there are two cycles), so a backend that starts successfully within those declared component limits is killed and reported as a lifecycle failure at 30 seconds post-handoff. This contradicts the outer `$TimeoutSeconds` deadline (default 300), which should remain the governing bound. Consider deriving the post-handoff window from `$TimeoutSeconds` remaining time rather than a fixed 30 seconds, or otherwise ensuring it cannot be shorter than the sum of the component readiness budgets the lifecycle is allowed to use.
| // "wsl:default" sentinel; the orchestrator uses the same fallback | ||
| // for the actual backend. | ||
| const wslDistro = useWsl ? (wslDistroFromTarget ?? settings.wslDistro) : null; | ||
| const wslDistro = useWsl |
There was a problem hiding this comment.
🟠 High methods/window.ts:269
The fail-closed guard at line 280 is ineffective for the wsl:default sentinel when settings.wslDistro is configured. resolveWslPickerDistro falls back to the persisted configuredDistro, so wslDistro is non-null even when no running WSL identity backs the environment. The guard never fires, and the picker proceeds against a distro that may not be the one actually backing the environment — the exact stale/cross-filesystem case this change intends to block. For the wsl:default sentinel, the configured fallback should not be treated as a running identity. Consider making resolveWslPickerDistro return null for the default sentinel when no running identity is available, or excluding the configuredDistro fallback for that case.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/ipc/methods/window.ts around line 269:
The fail-closed guard at line 280 is ineffective for the `wsl:default` sentinel when `settings.wslDistro` is configured. `resolveWslPickerDistro` falls back to the persisted `configuredDistro`, so `wslDistro` is non-null even when no running WSL identity backs the environment. The guard never fires, and the picker proceeds against a distro that may not be the one actually backing the environment — the exact stale/cross-filesystem case this change intends to block. For the `wsl:default` sentinel, the configured fallback should not be treated as a running identity. Consider making `resolveWslPickerDistro` return `null` for the default sentinel when no running identity is available, or excluding the `configuredDistro` fallback for that case.
There was a problem hiding this comment.
Effect service conventions review: 4 findings, all in newly added or newly modified Effect code (single-tag catchTag usage, direct process.platform/process.arch reads inside Effect code instead of the HostProcess* references, and a new service module that skips the canonical exported make).
Posted via Macroscope — Effect Service Conventions
| timeout, | ||
| }).pipe( | ||
| Effect.as(true), | ||
| Effect.catchTag("BackendReadinessTimeoutError", () => Effect.succeed(false)), |
There was a problem hiding this comment.
Known tagged failures should be recovered with Effect.catchTags({ ... }) rather than Effect.catchTag, even when handling a single tag.
| Effect.catchTag("BackendReadinessTimeoutError", () => Effect.succeed(false)), | |
| Effect.catchTags({ | |
| BackendReadinessTimeoutError: () => Effect.succeed(false), | |
| }), |
Posted via Macroscope — Effect Service Conventions
| readonly delaysMs?: readonly number[]; | ||
| }, | ||
| ): Effect.Effect<A, PlatformError.PlatformError> { | ||
| const platform = options?.platform ?? process.platform; |
There was a problem hiding this comment.
This Effect helper falls back to the global process.platform, which hides the platform dependency from the Effect environment (and trips t3code/no-global-process-runtime). Suggest reading HostProcessPlatform from @t3tools/shared/hostProcess instead of the platform option — the reference already defaults to the host platform, and the tests can override it with Effect.provideService(HostProcessPlatform, "win32" | "linux") instead of passing platform.
Posted via Macroscope — Effect Service Conventions
| platform: process.platform, | ||
| arch: process.arch, |
There was a problem hiding this comment.
Reading the host platform/arch from the global process inside an Effect handler hides a runtime dependency (and trips the repo-wide t3code/no-global-process-runtime rule). Consider taking them from the HostProcess* references so tests can provide them.
- platform: process.platform,
- arch: process.arch,
+ platform: yield* HostProcessPlatform,
+ arch: yield* HostProcessArchitecture,Also add import { HostProcessArchitecture, HostProcessPlatform } from "@t3tools/shared/hostProcess";.
Posted via Macroscope — Effect Service Conventions
| export const layer = Layer.effect( | ||
| DesktopWslDiagnostics, | ||
| Effect.gen(function* () { | ||
| const ref = yield* Ref.make(Option.none<DesktopWslDiagnosticRecord>()); | ||
| return DesktopWslDiagnostics.of({ | ||
| current: Ref.get(ref), | ||
| record: (diagnostic) => | ||
| Ref.set( | ||
| ref, | ||
| Option.some({ | ||
| ...diagnostic, | ||
| occurredAt: diagnostic.occurredAt ?? new Date().toISOString(), | ||
| }), | ||
| ), | ||
| clear: Ref.set(ref, Option.none()), | ||
| }); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
This module owns construction, so it should export a real make and keep the canonical order (tag, make, layer), matching the rest of the repo's Layer.effect(Tag, make) services.
| export const layer = Layer.effect( | |
| DesktopWslDiagnostics, | |
| Effect.gen(function* () { | |
| const ref = yield* Ref.make(Option.none<DesktopWslDiagnosticRecord>()); | |
| return DesktopWslDiagnostics.of({ | |
| current: Ref.get(ref), | |
| record: (diagnostic) => | |
| Ref.set( | |
| ref, | |
| Option.some({ | |
| ...diagnostic, | |
| occurredAt: diagnostic.occurredAt ?? new Date().toISOString(), | |
| }), | |
| ), | |
| clear: Ref.set(ref, Option.none()), | |
| }); | |
| }), | |
| ); | |
| export const make = Effect.gen(function* () { | |
| const ref = yield* Ref.make(Option.none<DesktopWslDiagnosticRecord>()); | |
| return DesktopWslDiagnostics.of({ | |
| current: Ref.get(ref), | |
| record: (diagnostic) => | |
| Ref.set( | |
| ref, | |
| Option.some({ | |
| ...diagnostic, | |
| occurredAt: diagnostic.occurredAt ?? new Date().toISOString(), | |
| }), | |
| ), | |
| clear: Ref.set(ref, Option.none()), | |
| }); | |
| }); | |
| export const layer = Layer.effect(DesktopWslDiagnostics, make); |
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 35e0331. Configure here.
| distro, | ||
| BUNDLED_NODE_PREP_SCRIPT(bundledSource.value), | ||
| PROBE_TIMEOUT, | ||
| { resolveUserNode: false }, |
There was a problem hiding this comment.
Bundled Node prep timeout too short
High Severity
Packaged WSL Node preparation runs BUNDLED_NODE_PREP_SCRIPT under PROBE_TIMEOUT (10s). That script copies the full Linux Node binary from /mnt/c, hashes it twice, and executes it—work that often exceeds 10s on real WSL9p/antivirus hosts. Timeouts are returned as non-fatal transport failures with no retryLimit, so preflight can restart forever and never finish the cache copy.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 35e0331. Configure here.
| ), | ||
| }); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Resume probe omits HttpClient
High Severity
probeReady calls waitForHttpReady without providing HttpClient, unlike the backend start path. DesktopPowerRecovery runs resume work via Effect.runPromiseWith with only power-recovery services, so a post-resume health probe cannot satisfy that dependency and recovery aborts in catchCause instead of restarting stale WSL/Windows backends.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 35e0331. Configure here.
ApprovabilityVerdict: Needs human review 7 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
1 participant
What Changed
Why
UI Changes
Checklist
Note
High Risk
Touches core Windows/WSL startup, networking, packaging, and release gates; regressions could break packaged WSL backends or Windows CI/release builds despite broad new test coverage.
Overview
Adds Windows/WSL CI and release gates: native Windows checks, Ubuntu 22.04 ABI-floor builds that audit
node-pty+ bundled Linux Node, Ubuntu 24.04 smoke validation, and a Windows+real-WSL2 job that runs integration tests and packaged portable lifecycle smoke with artifact uploads.WSL backend behavior shifts from binding
0.0.0.0and host-side port scans to distro-specific bind addresses, Linux-side TCP port allocation (fixedfor WSL-only primary,wsl-autowith ephemeral fallback for secondaries), WSL2-only and packaged Node + glibc floor preflight, and stricter failures for missing bundled runtime. Adds structured WSL diagnostics, IPC retry / diagnostic export, backendprobeReadyand failure hooks, and Windows suspend/resume reconciliation for backends.Windows reliability:
taskkill /Tfor dev Electron shutdown, Electron repair via upstreaminstall.js, and retried atomic file writes for settings/catalog/logs. WSL folder picker resolves the distro from the running backend and blocks cross-distro UNC selections.Release packaging now requires successful WSL ABI audit + validation before Windows artifacts consume the full
wsl-prebuild/runtime (not justpty.node).Reviewed by Cursor Bugbot for commit 35e0331. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Harden Windows and WSL2 support with filesystem retries, diagnostics, and ABI-audited runtime packaging
retryWindowsFileSystemOperationinpackages/shared/src/windowsFileRetry.tsand applies it across all atomic file writes in desktop settings, server state, secret store, connection catalog, and log rotation to recover from transient Windows sharing violations.DesktopWslDiagnosticsrecords failure events across preflight, spawn, readiness, and runtime-exit phases;lastDiagnosticandretryare exposed onDesktopWslBackendand surfaced to the renderer via new IPC channels and the settings UI.wsl-autostrategy) rather than scanning Windows loopback ports, and binds to the distro's concrete NAT IP in NAT mode.DesktopPowerRecoveryto probe and restart backends after system resume events on Windows.node-pty,fff,ffi-rs) inside an Ubuntu 22.04 container enforcing GLIBC ≤ 2.35, uploads an ABI receipt, and gates Windows packaging on passing audit and smoke validation.stageWslNodePtyPrebuildverifies SHA-256 of the auditedpty.nodeand bundled Linux Node against the ABI receipt before staging; packaging fails closed if any artifact is missing or mismatched.ensureNodePtycan now provision a packaged Linux Node runtime into the distro, enforce a minimum glibc 2.35 floor, and reject musl environments.install.jsinstead ofcurl/python3; Windows dev shutdown usestaskkill /T /Ffor full process-tree termination.--without-wslis passed (arm64 Windows already opts out).📊 Macroscope summarized 35e0331. 42 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.