fix(desktop): prevent WSL backend exiting with code 0 by keeping stdi… - #3613
fix(desktop): prevent WSL backend exiting with code 0 by keeping stdi…#3613jibin7jose wants to merge 23 commits into
Conversation
…n open Fixes pingdotgg#3611 by appending Stream.never to the bootstrap stream so the Windows-side pipe does not close before the backend process completes reading it.
|
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces a new restart-capping mechanism for WSL backends with fallback logic, changes how stdin streams are managed, and modifies error handling behavior. These are significant runtime behavior changes affecting backend lifecycle management that warrant human review. You can customize Macroscope's approvability policy. Learn more. |
Tests using fd3 delivery were hanging because the stream never terminated. Only stdin delivery (WSL) needs the stream kept open.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
…gg#3611) Consolidates fixes from PRs pingdotgg#3613, pingdotgg#3621, and pingdotgg#3623 to address both root causes and add a fail-safe: 1. DesktopWslEnvironment: Parse the Node version in the WSL node-pty probe and validate it against options.nodeEngineRange. Preflight now fails cleanly with an actionable error if the distro's default Node is incompatible. 2. DesktopBackendManager: Track neverReadyAttempt to cap consecutive post-spawn exits on stdin delivery. Prevents the desktop from permanently getting stuck restarting if the WSL backend consistently fails before readiness. 3. bootstrap: Clean up the readline interface on all event paths to prevent leaks when stdin stays open, and register the error listener on the readline interface to safely catch early stream errors.
340fb76 to
215bd10
Compare
github-actions
Bot
added
size:L
Dismissing prior approval to re-evaluate 215bd10
|
@juliusmarminge Could you please approve the workflows for this PR so the checks can run? Thank you! |
|
Hi @juliusmarminge! When you have time, could you please take a look at this PR? It addresses issue #3611 by fixing the WSL backend exiting early when using stdin bootstrap delivery. I’ve incorporated the follow-up fixes and kept the PR up to date with main. If you have any feedback or would like any changes, I’d be happy to update the PR. Thank you! |
|
Hi @juliusmarminge, @t3dotgg, and @codex! When you have a chance, could you please take a look at this PR? It fixes the WSL backend exiting early when using stdin bootstrap delivery, includes the follow-up fixes from earlier feedback, and has been kept up to date with main. Once the workflows are approved and the checks complete, I'd really appreciate your review. Thank you! |
|
To use Codex here, create a Codex account and connect to github. |
|
Hi @juliusmarminge, @t3dotgg, and @codex! Just a friendly follow-up on this PR. All requested changes have been addressed, the branch has been kept up to date with main, and it's ready for review. When you have time, could you please take another look? I'd really appreciate your feedback. Thank yo |
|
To use Codex here, create a Codex account and connect to github. |
|
I still have the WSL stuck at connection issue on the latest alpha T3 Code build, any updates for this PR? |
865db91 to
ea01cf3
Compare
github-actions
Bot
added
size:XXL
ea01cf3 to
865db91
Compare
github-actions
Bot
added
size:L
There was a problem hiding this comment.
Effect service conventions: no violations found in imports, service definition, dependency acquisition, or error modeling. Two change-discipline findings: the behavior changes in DesktopBackendManager.ts and apps/server/src/bootstrap.ts land without focused tests, although both modules already have test suites with the needed harnesses.
Posted via Macroscope — Effect Service Conventions
| // Register the error listener on the readline interface, not the raw | ||
| // stream. Node's readline.Interface re-emits stream errors onto itself; | ||
| // if a stream error arrives before the first line event, listening only | ||
| // on the stream leaves the readline interface with an unhandled error — | ||
| // registering on `input` ensures every error path is handled regardless | ||
| // of when the error occurs in the reader lifecycle. | ||
| input.once("error", handleError); |
There was a problem hiding this comment.
Registering the error listener on the readline interface (plus the added cleanup() calls in each handler) changes bootstrap read behavior, and bootstrap.test.ts currently has no case for a stream error arriving before the first line. Consider adding a focused test that emits an error on the input stream and asserts BootstrapEnvelopeReadError for a generic error and Option.none() for EBADF/ENOENT.
Posted via Macroscope — Effect Service Conventions
| // For stdin-delivery (WSL) backends, track exits that happen | ||
| // before HTTP readiness. A run that reached readiness resets | ||
| // the counter (in onReady), so this correctly counts only | ||
| // *consecutive* never-ready exits. When the cap fires, invoke | ||
| // onPreflightFailed so the UI falls back to Windows instead of | ||
| // looping forever. fd3 (Windows-native) keeps uncapped restarts. | ||
| if (!wasReady && config.value.bootstrapDelivery === "stdin" && Option.isSome(pid)) { | ||
| const attempt = yield* Ref.modify(state, (s) => { | ||
| const next = s.neverReadyAttempt + 1; | ||
| return [next, { ...s, neverReadyAttempt: next }] as const; | ||
| }); | ||
| if (attempt >= MAX_PREFLIGHT_FAILURE_ATTEMPTS) { | ||
| yield* logInstanceError( | ||
| "WSL backend exited before readiness too many times; surfacing and falling back", | ||
| { reason, attempt }, | ||
| ); | ||
| // Reset so a future re-enable gets a fresh allowance. | ||
| yield* Ref.update(state, (s) => ({ ...s, neverReadyAttempt: 0 })); | ||
| const shouldRestart = yield* ( | ||
| spec.onPreflightFailed?.({ | ||
| reason: `WSL backend exited before becoming ready ${attempt} times in a row. ${reason}`, | ||
| fatal: false, | ||
| }) ?? Effect.succeed(false) | ||
| ); | ||
| if (!shouldRestart) { | ||
| yield* Ref.update(state, (s) => ({ | ||
| ...s, | ||
| desiredRunning: false, | ||
| ready: false, | ||
| })); | ||
| } else { | ||
| yield* scheduleRestart(reason); | ||
| } | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
This new never-ready cap changes backend restart/fallback behavior (bounded restarts plus an onPreflightFailed fallback for stdin-delivery runs) but no test accompanies it. Consider adding a focused case to DesktopBackendManager.test.ts — the harness already supports bootstrapDelivery and onPreflightFailed — asserting that a stdin-delivery backend which exits before readiness MAX_PREFLIGHT_FAILURE_ATTEMPTS times invokes onPreflightFailed exactly once, stops when it returns false, and that a run reaching readiness resets the counter.
Posted via Macroscope — Effect Service Conventions
|
@jibin7jose Can you address the reviews and update your PR? Considering to get this PR merged |
|
Ok 👍 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ 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 a6ef2d3. Configure here.
|
@UtkarshUsername I have addressed all the reviews and updated the PR! I've added the missing focused tests for the error handling paths in DesktopBackendManager.ts and bootstrap.ts (including properly handling the TestClock in the backend restart loops). The Macroscope bot findings should now be fully resolved. |
4 participants
…n open
Fixes #3611 by appending Stream.never to the bootstrap stream so the Windows-side pipe does not close before the backend process completes reading it.
What Changed
Why
UI Changes
Checklist
Note
Medium Risk
Touches desktop backend lifecycle (WSL spawn, restart caps, stdin bootstrap) and server bootstrap I/O; behavior changes are scoped but affect startup and fallback paths.
Overview
Fixes WSL backend premature exit by keeping stdin open after the bootstrap JSON line (
Stream.neverforstdindelivery) so the Windows-side pipe does not EOF before the child finishes reading.Adds a
neverReadyAttemptcounter forstdin/WSL runs that exit before HTTP readiness: after five consecutive failures,onPreflightFailedruns once and the instance stops (or restarts only if the callback returns true); the counter resets on readiness, stop, or a fresh start. fd3 Windows-native backends are unchanged.Server bootstrap now listens for read errors on the readline interface (with cleanup on error/line/close) instead of the raw stream, avoiding unhandled errors and mapping
EBADF/ENOENTto no envelope.WSL node-pty probe only prints
nodeVersionwhen Node exists; engine-range validation runs on successful probes with clearer fatal messages when version is missing or incompatible.Reviewed by Cursor Bugbot for commit 96907b3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix WSL backend exiting with code 0 by keeping stdin open and capping never-ready retries
neverReadyAttemptcounter; afterMAX_PREFLIGHT_FAILURE_ATTEMPTSfailures, invokesonPreflightFailedwith a non-fatal reason and stops or schedules a restart based on the callback return value.readBootstrapEnvelopein bootstrap.ts to handle stream errors re-emitted byreadline.Interface, returningnoneforEBADF/ENOENTand aBootstrapEnvelopeReadErrorfor generic errors, with consistent cleanup on all paths.Macroscope summarized 96907b3.