chore: effect rpc and layer based startup#929
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 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
the
vouch:trusted
![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.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dev URL redirect drops request path and query
- Fixed the redirect to construct a new URL from the request's pathname and search params against the dev origin, and updated the test to assert the correct behavior.
Or push these changes by commenting:
@cursor push 089fb595df
Preview (089fb595df)
diff --git a/apps/server/src/http.ts b/apps/server/src/http.ts
--- a/apps/server/src/http.ts
+++ b/apps/server/src/http.ts
@@ -90,7 +90,8 @@
const config = yield* ServerConfig;
if (config.devUrl) {
- return HttpServerResponse.redirect(config.devUrl.href, { status: 302 });
+ const devTarget = new URL(`${url.pathname}${url.search}`, config.devUrl);
+ return HttpServerResponse.redirect(devTarget.href, { status: 302 });
}
if (!config.staticDir) {
diff --git a/apps/server/src/server.test.ts b/apps/server/src/server.test.ts
--- a/apps/server/src/server.test.ts
+++ b/apps/server/src/server.test.ts
@@ -266,7 +266,7 @@
const response = yield* Effect.promise(() => fetch(url, { redirect: "manual" }));
assert.equal(response.status, 302);
- assert.equal(response.headers.get("location"), "http://127.0.0.1:5173/");
+ assert.equal(response.headers.get("location"), "http://127.0.0.1:5173/foo/bar");
}).pipe(Effect.provide(NodeHttpServer.layerTest)),
);|
|
||
| const config = yield* ServerConfig; | ||
| if (config.devUrl) { | ||
| return HttpServerResponse.redirect(config.devUrl.href, { status: 302 }); |
There was a problem hiding this comment.
Dev URL redirect drops request path and query
High Severity
The dev URL redirect uses config.devUrl.href without appending the incoming request's pathname or search params. Every GET request (e.g. /settings?tab=general) redirects to the dev server root (http://127.0.0.1:5173/) instead of the matching route (http://127.0.0.1:5173/settings?tab=general). The reference implementation correctly constructs new URL(url.pathname + url.search, origin). The test at line 269 asserts the broken behavior, masking the regression.
Additional Locations (1)
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
github-actions
bot
added
the
size:XXL
juliusmarminge
force-pushed
the
effect-http-router
branch
from
ede8523 to
8ea82bc
Compare
| ? Deferred.succeed(deferred, exit.value) | ||
| : Deferred.failCause(deferred, exit.cause); | ||
|
|
||
| export const makeCommandGate = Effect.gen(function* () { |
There was a problem hiding this comment.
🟢 Low src/serverRuntimeStartup.ts:62
Once signalCommandReady sets commandReadinessState to "ready", enqueueCommand bypasses the queue and runs effects directly (line 86-87), while commands already in the queue are still being processed by commandWorker. This allows commands arriving after the state transition to execute concurrently with queued commands, breaking the sequential ordering guarantee that the queue was designed to provide.
Also found in 1 other location(s)
.reference/server/src/model-store.ts:218
Race condition in
subscribe: Events published toeventsPubSubduring thecatchupquery execution will be missed.Stream.concat(catchup, live)only subscribes to the PubSub aftercatchupcompletes. Ifappendpublishes events whilecatchupis querying the database, those events won't be in the catchup results (they occurred after the query started) and won't be captured bylive(subscription wasn't established yet). This causes event loss for concurrent subscriptions.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/serverRuntimeStartup.ts around line 62:
Once `signalCommandReady` sets `commandReadinessState` to `"ready"`, `enqueueCommand` bypasses the queue and runs effects directly (line 86-87), while commands already in the queue are still being processed by `commandWorker`. This allows commands arriving after the state transition to execute concurrently with queued commands, breaking the sequential ordering guarantee that the queue was designed to provide.
Evidence trail:
apps/server/src/serverRuntimeStartup.ts lines 64-67 (commandWorker forked in separate fiber), lines 69-72 (signalCommandReady sets state to ready first, then succeeds deferred), lines 79-97 (enqueueCommand checks state and runs directly if ready at lines 82-83, or queues with Deferred.await(commandReady) at lines 90-95)
Also found in 1 other location(s):
- .reference/server/src/model-store.ts:218 -- Race condition in `subscribe`: Events published to `eventsPubSub` during the `catchup` query execution will be missed. `Stream.concat(catchup, live)` only subscribes to the PubSub after `catchup` completes. If `append` publishes events while `catchup` is querying the database, those events won't be in the catchup results (they occurred after the query started) and won't be captured by `live` (subscription wasn't established yet). This causes event loss for concurrent subscriptions.
- Replace state-dir config with T3CODE_HOME/base-dir derivation - Route attachments and provider logs through derived paths - Lazily load Bun/Node HTTP, PTY, and platform services
Co-authored-by: codex <codex@users.noreply.github.com>
| [WS_METHODS.subscribeOrchestrationDomainEvents]: (_input) => | ||
| Stream.unwrap( | ||
| Effect.gen(function* () { | ||
| const snapshot = yield* orchestrationEngine.getReadModel(); | ||
| const fromSequenceExclusive = snapshot.snapshotSequence; | ||
| const replayEvents: Array<OrchestrationEvent> = yield* Stream.runCollect( | ||
| orchestrationEngine.readEvents(fromSequenceExclusive), | ||
| ).pipe( | ||
| Effect.map((events) => Array.from(events)), | ||
| Effect.catch(() => Effect.succeed([] as Array<OrchestrationEvent>)), | ||
| ); |
There was a problem hiding this comment.
🟡 Medium src/ws.ts:148
In subscribeOrchestrationDomainEvents, when orchestrationEngine.readEvents() fails, the error is silently caught and replaced with an empty array. The state machine then initializes nextSequence = fromSequenceExclusive + 1, but since the replay failed, events with that sequence number will never arrive. Live events accumulate in pendingBySequence waiting for the missing sequence, causing the subscription to hang indefinitely and leak memory. Consider propagating the replay error instead of silently succeeding so the client receives a failure indication.
const replayEvents: Array<OrchestrationEvent> = yield* Stream.runCollect(
orchestrationEngine.readEvents(fromSequenceExclusive),
).pipe(
Effect.map((events) => Array.from(events)),
- Effect.catch(() => Effect.succeed([] as Array<OrchestrationEvent>)),
+ Effect.mapError(
+ (cause) =>
+ new OrchestrationReplayEventsError({
+ message: "Failed to replay orchestration events for subscription",
+ cause,
+ }),
+ ),
);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/ws.ts around lines 148-158:
In `subscribeOrchestrationDomainEvents`, when `orchestrationEngine.readEvents()` fails, the error is silently caught and replaced with an empty array. The state machine then initializes `nextSequence = fromSequenceExclusive + 1`, but since the replay failed, events with that sequence number will never arrive. Live events accumulate in `pendingBySequence` waiting for the missing sequence, causing the subscription to hang indefinitely and leak memory. Consider propagating the replay error instead of silently succeeding so the client receives a failure indication.
Evidence trail:
apps/server/src/ws.ts lines 148-200 at REVIEWED_COMMIT:
- Line 157: `Effect.catch(() => Effect.succeed([] as Array<OrchestrationEvent>))` silently catches errors
- Lines 164-167: State initialization with `nextSequence: fromSequenceExclusive + 1`
- Line 160: `Stream.merge(replayStream, orchestrationEngine.streamDomainEvents)` merges replay with live
- Lines 170-198: State machine logic that buffers out-of-order events in `pendingBySequence` and only emits when `nextSequence` matches
Co-authored-by: codex <codex@users.noreply.github.com>
1 participant
What Changed
Why
UI Changes
Checklist
Note
Medium Risk
Refactors core server startup and WebSocket request handling to a new Effect
RpcServer/HttpRoutercomposition, so regressions could break connectivity or endpoint behavior. Adds new path/attachment handling and command normalization that touches filesystem writes and validation logic.Overview
Migrates the server’s WebSocket request/response handling from the legacy
wsServer.tsswitch/envelope path to Effect RPC routes inapps/server/src/ws.ts, wiring many existingWS_METHODS/orchestration procedures into a sharedWsRpcGroupwith typed errors and a new integration-heavyserver.test.ts.Restructures startup into a layer-based CLI entrypoint (
src/bin.ts+src/cli.ts) and a newserver.tsthat composes HTTP routes (health, static/dev redirect, attachments) plus the WebSocket RPC route, and tightens filesystem safety viaresolveWorkspaceWritePathand orchestration command normalization for attachment persistence.Also centralizes several error types by re-exporting them from
@t3tools/contracts, updates package bin/start scripts accordingly, and adds a.plans/ws-rpc-endpoint-port-plan.mdplus formatter ignore for.reference.Written by Cursor Bugbot for commit e3ac9ed. This will update automatically on new commits. Configure here.
Note
Replace WebSocket request handling with Effect RPC layer and restructure server startup
/wsviaWsRpcLayerin ws.ts, covering orchestration, git, terminal, project file ops, keybindings, and shell open. Contracts are defined in the new wsRpc.ts.ServerLiveservice/layer pattern withServerLayer(aLayer.effectDiscard) and adds a CLI entrypoint in bin.ts with flag/env-based config resolution in cli.ts.GitManagerServiceError,TerminalError,OrchestrationDispatchCommandError,OpenError, etc.) used as typed RPC error channels.RouteRequestError; clients must migrate to the/wsRPC endpoint.📊 Macroscope summarized eb8ee3f. 24 files reviewed, 3 issues evaluated, 1 issue filtered, 1 comment posted
🗂️ Filtered Issues
apps/web/src/wsTransport.ts — 0 comments posted, 1 evaluated, 1 filtered
subscribe(), when the RPC method doesn't exist,WsTransportStreamMethodErroris thrown and caught byEffect.catch(), which logs a warning, sleeps, and thenEffect.foreverretries the operation. Since the method still won't exist on retry, this creates an infinite retry loop with console spam. The catch handler should distinguish between retryable errors (like connection failures) and non-retryable errors (like invalid method names) and only retry the former. [ Already posted ]