Port desktop backend readiness checks to Effect#2546
Port desktop backend readiness checks to Effect#2546juliusmarminge wants to merge 4 commits intomainfrom
Conversation
- Replace promise-based readiness polling with Effect layers and services - Update desktop startup flow to use URL-backed readiness state - Add Effect-based tests for HTTP and startup readiness
|
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 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)
Comment |
github-actions
Bot
added
size:L
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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 and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Timeout silently succeeds instead of failing with error
- Added Option.match after Effect.timeoutOption to convert Option.None (timeout) into a BackendTimeoutError failure instead of silently succeeding, matching the established pattern used in all other timeoutOption call sites in the codebase.
Or push these changes by commenting:
@cursor push 27f2b65c9b
Preview (27f2b65c9b)
diff --git a/apps/desktop/src/backendReadiness.ts b/apps/desktop/src/backendReadiness.ts
--- a/apps/desktop/src/backendReadiness.ts
+++ b/apps/desktop/src/backendReadiness.ts
@@ -3,6 +3,7 @@
import * as Data from "effect/Data";
import * as Duration from "effect/Duration";
import * as Effect from "effect/Effect";
+import * as Option from "effect/Option";
import * as Predicate from "effect/Predicate";
import * as Schedule from "effect/Schedule";
import { HttpClient } from "effect/unstable/http";
@@ -60,9 +61,13 @@
yield* client.get(requestUrl).pipe(
Effect.asVoid,
Effect.timeoutOption(timeout),
+ Effect.flatMap(
+ Option.match({
+ onNone: () => Effect.fail(new BackendTimeoutError({ url: baseUrl })),
+ onSome: () => Effect.void,
+ }),
+ ),
Effect.catchTags({
- TimeoutError: () => Effect.fail(new BackendTimeoutError({ url: baseUrl })),
- // Maybe map this to different error kind?
HttpClientError: () => Effect.fail(new BackendTimeoutError({ url: baseUrl })),
}),
);You can send follow-ups to the cloud agent here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. Major refactor replacing Promise-based process management with Effect patterns, with unresolved review comments identifying a race condition bug in backend state management and concerns about timeout handling behavior. You can customize Macroscope's approvability policy. Learn more. |
Effect.timeoutOption returns Option.None on timeout (on the success channel), not a TimeoutError on the error channel. The previous code discarded the Option result, so a timeout was treated as success. Now we explicitly match the Option: None maps to BackendTimeoutError, Some maps to Effect.void. The now-unreachable TimeoutError catchTag is removed. Applied via @cursor push command
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate d8f0d72
- Spawn the backend through a shared Effect runner with fd3 bootstrap - Replace ad hoc readiness/listening plumbing with HTTP readiness hooks - Add contract support for desktop bootstrap payloads
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate b5c8ea4
github-actions
Bot
added
size:XL
| const clearStartedBackendState = (): void => { | ||
| if (backendProcessFiber === startedFiber) { | ||
| backendProcessFiber = null; | ||
| } | ||
| backendReady = false; | ||
| }; |
There was a problem hiding this comment.
🟠 High src/main.ts:1443
clearStartedBackendState unconditionally sets backendReady = false even when the fiber being finalized is not the current one. If stopBackend() is called and startBackend() is called again before the old fiber's finalizer runs, the old fiber's Effect.ensuring handler will incorrectly clear backendReady after the new backend has already signaled readiness via handleBackendReady(), causing the application to think the backend is not ready when it actually is.
const clearStartedBackendState = (): void => {
if (backendProcessFiber === startedFiber) {
backendProcessFiber = null;
+ backendReady = false;
}
- backendReady = false;
};🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/desktop/src/main.ts around lines 1443-1448:
`clearStartedBackendState` unconditionally sets `backendReady = false` even when the fiber being finalized is not the current one. If `stopBackend()` is called and `startBackend()` is called again before the old fiber's finalizer runs, the old fiber's `Effect.ensuring` handler will incorrectly clear `backendReady` after the new backend has already signaled readiness via `handleBackendReady()`, causing the application to think the backend is not ready when it actually is.
Evidence trail:
apps/desktop/src/main.ts lines 1443-1447: `clearStartedBackendState` definition showing `backendReady = false` is outside the `if (backendProcessFiber === startedFiber)` guard.
apps/desktop/src/main.ts lines 1524-1527: `Effect.ensuring` handler that calls `finalizeBackendSession` which calls `clearStartedBackendState`.
apps/desktop/src/main.ts lines 1536-1544: `stopBackend()` sets `backendProcessFiber = null` and forks interrupt asynchronously.
apps/desktop/src/main.ts lines 1420-1421: `startBackend()` guard only checks `backendProcessFiber !== null`, allowing re-entry after `stopBackend()`.
apps/desktop/src/main.ts lines 516-517: `handleBackendReady()` sets `backendReady = true`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Port fields lose range validation in bootstrap schema
- Replaced Schema.Number with a shared PortSchema (Schema.Int with isBetween 1-65535) for both port and tailscaleServePort fields in DesktopBackendBootstrap, restoring the validation that was present in the old BootstrapEnvelopeSchema.
- ✅ Fixed: Readiness timeout fires but no window ever created
- Added fallback window creation in the onReadinessFailure handler so users get a window (showing reconnection state) instead of being stuck with no UI when the readiness probe times out.
Or push these changes by commenting:
@cursor push 1291f41e07
Preview (1291f41e07)
diff --git a/apps/desktop/src/main.ts b/apps/desktop/src/main.ts
--- a/apps/desktop/src/main.ts
+++ b/apps/desktop/src/main.ts
@@ -1496,6 +1496,12 @@
`bootstrap backend readiness warning message=${formatErrorMessage(error)}`,
);
console.warn("[desktop] backend readiness check failed during bootstrap", error);
+
+ if (isDevelopment) return;
+ const existingWindow = mainWindow ?? BrowserWindow.getAllWindows()[0] ?? null;
+ if (existingWindow !== null) return;
+ mainWindow = createWindow();
+ writeDesktopLogHeader("bootstrap main window created (readiness timeout fallback)");
}),
onOutput: (streamName, chunk) =>
Effect.sync(() => {
diff --git a/apps/server/src/cli.ts b/apps/server/src/cli.ts
--- a/apps/server/src/cli.ts
+++ b/apps/server/src/cli.ts
@@ -5,6 +5,7 @@
CommandId,
DesktopBackendBootstrap,
OrchestrationReadModel,
+ PortSchema,
ProjectId,
type ClientOrchestrationCommand,
} from "@t3tools/contracts";
@@ -67,8 +68,6 @@
import { WorkspacePaths } from "./workspace/Services/WorkspacePaths.ts";
import { WorkspacePathsLive } from "./workspace/Layers/WorkspacePaths.ts";
-const PortSchema = Schema.Int.check(Schema.isBetween({ minimum: 1, maximum: 65535 }));
-
const modeFlag = Flag.choice("mode", RuntimeMode.literals).pipe(
Flag.withDescription("Runtime mode. `desktop` keeps loopback defaults unless overridden."),
Flag.optional,
diff --git a/packages/contracts/src/baseSchemas.ts b/packages/contracts/src/baseSchemas.ts
--- a/packages/contracts/src/baseSchemas.ts
+++ b/packages/contracts/src/baseSchemas.ts
@@ -5,6 +5,7 @@
export const NonNegativeInt = Schema.Int.check(Schema.isGreaterThanOrEqualTo(0));
export const PositiveInt = Schema.Int.check(Schema.isGreaterThanOrEqualTo(1));
+export const PortSchema = Schema.Int.check(Schema.isBetween({ minimum: 1, maximum: 65535 }));
export const IsoDateTime = Schema.String;
export type IsoDateTime = typeof IsoDateTime.Type;
diff --git a/packages/contracts/src/desktopBootstrap.ts b/packages/contracts/src/desktopBootstrap.ts
--- a/packages/contracts/src/desktopBootstrap.ts
+++ b/packages/contracts/src/desktopBootstrap.ts
@@ -1,14 +1,16 @@
import { Schema } from "effect";
+import { PortSchema } from "./baseSchemas.ts";
+
export const DesktopBackendBootstrap = Schema.Struct({
mode: Schema.Literal("desktop"),
noBrowser: Schema.Boolean,
- port: Schema.Number,
+ port: PortSchema,
t3Home: Schema.String,
host: Schema.String,
desktopBootstrapToken: Schema.String,
tailscaleServeEnabled: Schema.Boolean,
- tailscaleServePort: Schema.Number,
+ tailscaleServePort: PortSchema,
otlpTracesUrl: Schema.optional(Schema.String),
otlpMetricsUrl: Schema.optional(Schema.String),
});You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit b5c8ea4. Configure here.
| host: Schema.String, | ||
| desktopBootstrapToken: Schema.String, | ||
| tailscaleServeEnabled: Schema.Boolean, | ||
| tailscaleServePort: Schema.Number, |
There was a problem hiding this comment.
Port fields lose range validation in bootstrap schema
Low Severity
The port and tailscaleServePort fields in DesktopBackendBootstrap use Schema.Number instead of the PortSchema (Schema.Int.check(Schema.isBetween({ minimum: 1, maximum: 65535 }))) that was previously used in the old BootstrapEnvelopeSchema inside cli.ts. This means the server-side bootstrap decoder no longer validates that these values are integers within the valid port range, accepting fractional numbers, zero, negative values, or ports above 65535.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b5c8ea4. Configure here.
| Effect.tap(() => options.onReady?.() ?? Effect.void), | ||
| Effect.catch((error) => options.onReadinessFailure?.(error) ?? Effect.void), | ||
| Effect.forkScoped, | ||
| ); |
There was a problem hiding this comment.
Readiness timeout fires but no window ever created
Medium Severity
When the readiness probe times out (after 1 minute), onReadinessFailure logs a warning but the process keeps running. Meanwhile backendReady stays false, so handleBackendReady never fires and no window is created. The process continues indefinitely without scheduling a restart or creating a window. In the old code, the timeout would log and the ensureInitialBackendWindowOpen promise chain would end—but the process also kept running, so this is comparable. However, the new code's Effect.match onSuccess handler only fires when the process exits, meaning a backend that starts serving but takes longer than the timeout window will leave the user staring at nothing with no recovery path until the process happens to exit.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b5c8ea4. Configure here.
2 participants
Summary
URLhandling for backend endpoints and the new readiness error type.Testing
bun fmtbun lintbun typecheckbun run testforapps/desktop/src/backendReadiness.test.tsandapps/desktop/src/backendStartupReadiness.test.tsNote
Medium Risk
Touches desktop startup/process lifecycle and server config bootstrap parsing; regressions could prevent the backend from starting or the window from loading, though changes are scoped and covered by new Effect-based tests.
Overview
Replaces the desktop app’s Promise/Node-based backend readiness + child-process management with Effect-based implementations:
waitForHttpReadyEffect(EffectHttpClient+ Effect clock timeouts/retries) and a newrunBackendProcessthat spawns the server with a typed bootstrap payload over fd3 and reports readiness via the new probe.Updates Electron
main.tsto manage the backend via an EffectFiber, switch backend URL handling toOption<URL>, and simplify window creation to occur on HTTP readiness (removing the log-based listening detector/startup race helpers). Adds a sharedDesktopBackendBootstrapschema in@t3tools/contractsand updates the server CLI + tests to decode that schema from--bootstrap-fd.Reviewed by Cursor Bugbot for commit b5c8ea4. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Port desktop backend readiness checks and process lifecycle to Effect
apps/desktopwith Effect fibers and layers;startBackend,stopBackend, andstopBackendAndWaitForExitare all rewritten to use Effect primitives.waitForHttpReady(promise + AbortSignal) withwaitForHttpReadyEffectusingHttpClient, per-request timeouts,Schedule.spacedretry, and a taggedBackendTimeoutError.runBackendProcessin backendProcess.ts to orchestrate spawning, fd3 bootstrap JSON encoding, readiness probing, and structured exit reporting.DesktopBackendBootstrapschema to a shared desktopBootstrap.ts in@t3tools/contracts; the server'sresolveServerConfignow uses this schema instead of its own inline one.devUrl,autoBootstrapProjectFromCwd, andlogWebSocketEventsare no longer read from the bootstrap envelope by the server; the new readiness API no longer accepts an externalAbortSignalor customisReadypredicate; the main window is now created only after backend readiness is signaled.Macroscope summarized b5c8ea4.