Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 5.2k
fix(desktop): prevent WSL backend exiting with code 0 by keeping stdi…#3613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
1402e0b82c2ec5215bd10267c72a489111353b9ceabfa1e1d47a2ebc59986e27f3ff48f6bd2c64545bb90b8f9288ea4d805df6b2384ef28b543154bd3ae449ffe39c4865db916fe07dba6ef2d396907b3b860e16b300d6a4721a2aFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1085,13 +1085,19 @@ describe("DesktopBackendManager", () => { | ||
| assert.equal(yield* Queue.take(starts), 1); | ||
| assert.equal(yield* Queue.take(failures), "pid=123 code=1"); | ||
| // yieldNow gives the forked restart fiber time to park on TestClock | ||
| // before we advance it, preventing a race between forkIn and adjust. | ||
| yield* Effect.yieldNow; | ||
| yield* TestClock.adjust(Duration.millis(499)); | ||
| assert.equal(yield* Queue.size(starts), 0); | ||
| yield* Effect.yieldNow; | ||
| yield* TestClock.adjust(Duration.millis(1)); | ||
| assert.equal(yield* Queue.take(starts), 2); | ||
| yield* Effect.yieldNow; | ||
| yield* TestClock.adjust(Duration.millis(999)); | ||
| assert.equal(yield* Queue.size(starts), 0); | ||
| yield* Effect.yieldNow; | ||
| yield* TestClock.adjust(Duration.millis(1)); | ||
| assert.equal(yield* Queue.take(starts), 3); | ||
| }).pipe(Effect.provide(TestClock.layer())), | ||
| @@ -1397,4 +1403,167 @@ describe("DesktopBackendManager", () => { | ||
| }).pipe(Effect.provide(TestClock.layer())), | ||
| ), | ||
| ); | ||
| it.effect("never-ready cap invokes onPreflightFailed exactly once and stops when false", () => | ||
| Effect.scoped( | ||
| Effect.gen(function* () { | ||
| const waitForNextSpawn = Effect.gen(function* () { | ||
| const sc = spawnCount; | ||
| while (true) { | ||
| const state = yield* instance.snapshot; | ||
| if (!state.desiredRunning) break; | ||
| if (spawnCount > sc) break; | ||
| if (state.ready) break; | ||
| if (state.restartScheduled) break; | ||
| yield* Effect.yieldNow; | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| }); | ||
| let spawnCount = 0; | ||
| let preflightFailedCount = 0; | ||
| let lastPreflightFailure: DesktopBackendManager.PreflightFailure | undefined; | ||
| const preflightFailed = yield* Deferred.make<void>(); | ||
| const spawnerLayer = Layer.succeed( | ||
| ChildProcessSpawner.ChildProcessSpawner, | ||
| ChildProcessSpawner.make(() => | ||
| Effect.gen(function* () { | ||
| spawnCount++; | ||
| return makeProcess({ | ||
| exitCode: Effect.succeed(ChildProcessSpawner.ExitCode(1)), | ||
| }); | ||
| }) | ||
| ), | ||
| ); | ||
| const instance = yield* makeTestInstance({ | ||
| spawnerLayer, | ||
| httpClientLayer: httpClientLayer(() => Effect.never), | ||
| config: { | ||
| ...baseConfig, | ||
| bootstrapDelivery: "stdin", | ||
| }, | ||
| onPreflightFailed: (failure) => | ||
| Effect.sync(() => { | ||
| preflightFailedCount++; | ||
| lastPreflightFailure = failure; | ||
| }).pipe( | ||
| Effect.andThen(Deferred.succeed(preflightFailed, void 0)), | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one more test-side race: this deferred is completed inside the callback before the callback returns | ||
| Effect.as(false) | ||
| ), | ||
| }); | ||
| yield* instance.start; | ||
| yield* waitForNextSpawn; yield* Effect.yieldNow; yield* TestClock.adjust(Duration.millis(500)); | ||
| yield* waitForNextSpawn; yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(1)); | ||
| yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(2)); | ||
| yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(4)); | ||
| yield* Deferred.await(preflightFailed); | ||
| assert.equal(preflightFailedCount, 1); | ||
| assert.equal(spawnCount, 5); | ||
| assert.isDefined(lastPreflightFailure); | ||
| assert.include(lastPreflightFailure!.reason, "exited before becoming ready"); | ||
| assert.equal(lastPreflightFailure!.fatal, false); | ||
| const state = yield* instance.snapshot; | ||
| assert.isFalse(state.ready); | ||
| assert.isFalse(state.desiredRunning); | ||
| }).pipe(Effect.provide(TestClock.layer())) | ||
| ) | ||
| ); | ||
| it.effect("readiness resets the never-ready cap for stdin-delivery backends", () => | ||
| Effect.scoped( | ||
| Effect.gen(function* () { | ||
| const waitForNextSpawn = Effect.gen(function* () { | ||
| const sc = spawnCount; | ||
| while (true) { | ||
| const state = yield* instance.snapshot; | ||
| if (!state.desiredRunning) break; | ||
| if (spawnCount > sc) break; | ||
| if (state.ready) break; | ||
| if (state.restartScheduled) break; | ||
| yield* Effect.yieldNow; | ||
| } | ||
| }); | ||
| let spawnCount = 0; | ||
| let preflightFailedCount = 0; | ||
| const preflightFailed = yield* Deferred.make<void>(); | ||
| const readyDeferred = yield* Deferred.make<void>(); | ||
| const closeReadyProcess = yield* Deferred.make<void>(); | ||
| const spawnerLayer = Layer.succeed( | ||
| ChildProcessSpawner.ChildProcessSpawner, | ||
| ChildProcessSpawner.make(() => | ||
| Effect.gen(function* () { | ||
| spawnCount++; | ||
| if (spawnCount === 5) { | ||
| // On the 5th spawn, reach readiness, but exit when closeReadyProcess is completed. | ||
| return makeProcess({ | ||
| exitCode: Deferred.await(closeReadyProcess).pipe(Effect.as(ChildProcessSpawner.ExitCode(1))), | ||
| }); | ||
| } else { | ||
| // All other spawns fail before readiness | ||
| return makeProcess({ | ||
| exitCode: Effect.succeed(ChildProcessSpawner.ExitCode(1)), | ||
| }); | ||
| } | ||
| }) | ||
| ), | ||
| ); | ||
| const instance = yield* makeTestInstance({ | ||
| spawnerLayer, | ||
| // 5th spawn will get 200 OK and trigger onReady. | ||
| httpClientLayer: httpClientLayer((request) => | ||
| spawnCount === 5 ? Effect.succeed(responseForRequest(request, 200)) : Effect.never | ||
| ), | ||
| config: { | ||
| ...baseConfig, | ||
| bootstrapDelivery: "stdin", | ||
| }, | ||
| onReady: Deferred.succeed(readyDeferred, void 0).pipe(Effect.asVoid), | ||
| onPreflightFailed: () => | ||
| Effect.sync(() => { | ||
| preflightFailedCount++; | ||
| }).pipe( | ||
| Effect.andThen(Deferred.succeed(preflightFailed, void 0)), | ||
| Effect.as(false) // stop retries | ||
| ), | ||
| }); | ||
| yield* instance.start; | ||
| yield* waitForNextSpawn; yield* Effect.yieldNow; yield* TestClock.adjust(Duration.millis(500)); | ||
| yield* waitForNextSpawn; yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(1)); | ||
| yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(2)); | ||
| yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(4)); | ||
| // Wait for the 5th spawn to become ready | ||
| yield* Deferred.await(readyDeferred); | ||
| // At this point, it was spawned 5 times, and counter should be reset. | ||
| assert.equal(spawnCount, 5); | ||
| // Trigger exit of the ready process | ||
| yield* Deferred.succeed(closeReadyProcess, void 0); | ||
| // Now advance time for the next 4 failures before readiness | ||
| yield* waitForNextSpawn; yield* Effect.yieldNow; yield* TestClock.adjust(Duration.millis(500)); | ||
| yield* waitForNextSpawn; yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(1)); | ||
| yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(2)); | ||
| yield* Effect.yieldNow; yield* TestClock.adjust(Duration.seconds(4)); | ||
| // Wait for the preflight failure which should happen on the 10th spawn | ||
| yield* Deferred.await(preflightFailed); | ||
| // Total spawns: 5 (initial) + 5 (new failures before readiness) = 10 | ||
| assert.equal(spawnCount, 10); | ||
| assert.equal(preflightFailedCount, 1); | ||
| const state = yield* instance.snapshot; | ||
| assert.isFalse(state.ready); | ||
| assert.isFalse(state.desiredRunning); | ||
| }).pipe(Effect.provide(TestClock.layer())) | ||
| ) | ||
| ); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -314,6 +314,11 @@ interface BackendManagerState { | ||
| // Consecutive bounded/fatal preflight failures, reset on a clean or | ||
| // unbounded-transient preflight. restartAttempt counts all restarts. | ||
| readonly preflightFailureAttempt: number; | ||
| // Consecutive post-spawn exits before HTTP readiness, for stdin-delivery | ||
| // (WSL) backends only. Reset to 0 when a run reaches readiness. Used to | ||
| // cap the restart loop and invoke onPreflightFailed after | ||
| // MAX_PREFLIGHT_FAILURE_ATTEMPTS never-ready exits. | ||
| readonly neverReadyAttempt: number; | ||
| readonly restartFiber: Option.Option<Fiber.Fiber<void, never>>; | ||
| readonly nextRunId: number; | ||
| } | ||
| @@ -325,6 +330,7 @@ const initialState: BackendManagerState = { | ||
| active: Option.none(), | ||
| restartAttempt: 0, | ||
| preflightFailureAttempt: 0, | ||
| neverReadyAttempt: 0, | ||
| restartFiber: Option.none(), | ||
| nextRunId: 1, | ||
| }; | ||
| @@ -449,7 +455,10 @@ export const runBackendProcess = Effect.fn("runBackendProcess")(function* ( | ||
| ), | ||
| ); | ||
| const onOutput = options.onOutput ?? (() => Effect.void); | ||
| const bootstrapStream = Stream.encodeText(Stream.make(`${bootstrapJson}\n`)); | ||
| const bootstrapStream = Stream.make(`${bootstrapJson}\n`).pipe( | ||
| options.bootstrapDelivery === "stdin" ? Stream.concat(Stream.never) : (s) => s, | ||
| Stream.encodeText, | ||
| ); | ||
| const additionalFds: Record<`fd${number}`, ChildProcess.AdditionalFdConfig> = {}; | ||
| if (options.bootstrapDelivery === "fd3") { | ||
| additionalFds.fd3 = { | ||
| @@ -717,6 +726,7 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* ( | ||
| ready: false, | ||
| config: Option.some(config.value), | ||
| preflightFailureAttempt: resetFatalPreflightCounter ? 0 : latest.preflightFailureAttempt, | ||
| neverReadyAttempt: !current.desiredRunning ? 0 : latest.neverReadyAttempt, | ||
| })); | ||
| const preflightFailure = config.value.preflightFailure; | ||
| @@ -811,6 +821,7 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* ( | ||
| const finalizeRun = Effect.fn("desktop.backendInstance.finalizeRun")(function* ( | ||
| reason: string, | ||
| ) { | ||
| yield* Effect.yieldNow; | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocking: this scheduler yield is not a lifecycle receipt. | ||
| yield* mutex.withPermits(1)( | ||
| Effect.gen(function* () { | ||
| const { isCurrentRun, nextState, pid, exitObserved, stopRequested, wasReady } = | ||
| @@ -825,6 +836,9 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* ( | ||
| readonly pid: Option.Option<number>; | ||
| readonly exitObserved: boolean; | ||
| readonly stopRequested: boolean; | ||
| // Whether this specific run had reached readiness before | ||
| // exiting. Captured inside the Ref.modify so we see the | ||
| // pre-transition value of latest.ready. | ||
| readonly wasReady: boolean; | ||
| }, | ||
| BackendManagerState, | ||
| @@ -880,6 +894,42 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* ( | ||
| } | ||
| if (isCurrentRun && nextState.desiredRunning) { | ||
| // 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) && exitObserved && !stopRequested) { | ||
| 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; | ||
| } | ||
| } | ||
Comment on lines
+897
to
+932
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new never-ready cap changes backend restart/fallback behavior (bounded restarts plus an Posted via Macroscope — Effect Service Conventions | ||
| yield* scheduleRestart(reason); | ||
| } | ||
| }), | ||
| @@ -908,7 +958,7 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* ( | ||
| onReady: Effect.fn("desktop.backendInstance.onReady")(function* () { | ||
| const isCurrentRun = yield* Ref.modify(state, (latest) => { | ||
| const activeRun = Option.getOrUndefined(latest.active); | ||
| if (activeRun?.id !== runId) { | ||
| if (activeRun?.id !== runId || activeRun.exitObserved || activeRun.stopRequested) { | ||
| return [false, latest] as const; | ||
| } | ||
| @@ -917,6 +967,9 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* ( | ||
| { | ||
| ...latest, | ||
| restartAttempt: 0, | ||
| // This run reached readiness — reset the never-ready counter | ||
| // so it only tracks *consecutive* failures from this point on. | ||
| neverReadyAttempt: 0, | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocker: a readiness response can complete after | ||
| ready: true, | ||
| }, | ||
| ] as const; | ||
| @@ -1017,7 +1070,6 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* ( | ||
| }), | ||
| }); | ||
| }); | ||
| const stop = Effect.fn("desktop.backendInstance.stop")(function* (options?: { | ||
| readonly timeout?: Duration.Duration; | ||
| }) { | ||
| @@ -1039,6 +1091,7 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* ( | ||
| ready: false, | ||
| active, | ||
| restartFiber: Option.none<Fiber.Fiber<void, never>>(), | ||
| neverReadyAttempt: 0, | ||
| }, | ||
| ] as const; | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking:
restartScheduledonly proves the restart fiber was stored, not that itsTestClock.sleepis registered. this helper can return too early, and later backoff advances do not call it at all. both new tests still time out in isolation. wait for an explicit timer-registration receipt before everyTestClock.adjust, then wait for the next spawn.