History

fix(execution): use raceFirst so executeWithPause settles on fiber failure

Effect.race in Effect v4 has prefer-success semantics. When the codeExecutor fiber failed fast (e.g. user code with a TS type annotation, syntax error in <30ms), the race would keep waiting for the pause Deferred to succeed and never settle. Outer Effect hung until the upstream client gave up at 180s, which then poisoned the per-id JSON-RPC queue and cascaded to subsequent calls on the same MCP session. raceFirst returns the first effect to complete, success or failure, so a fiber failure now propagates immediately.

Rhys Sullivan committed 6 months ago

Commit 6b710211 parent 8945b832 files · +97 −1Browse files

Unified · Split

2 files
  1. packages/core/execution/src/engine.test.ts +88 −0
  2. packages/core/execution/src/engine.ts +9 −1
packages/core/execution/src/engine.test.ts+88−0
@@ -0,0 +1,88 @@
1import { describe, expect, it } from "@effect/vitest";
2import { Data, Effect, Exit } from "effect";
3
4import { createExecutor, definePlugin, makeTestConfig } from "@executor-js/sdk";
5import type { CodeExecutor, ExecuteResult } from "@executor-js/codemode-core";
6
7import { createExecutionEngine } from "./engine";
8
9// Regression for the hang reported as the executor-MCP "180s timeout" against
10// Cowork (Claude web). Cowork goes down the `executeWithPause` branch because
11// it doesn't advertise managed elicitation. When the dynamic worker fails
12// fast (e.g. user submits TS with a `:` type annotation, "Unexpected token
13// ':'" inside ~25ms), the failure was swallowed and the request hung until
14// the client gave up at 180s. The cause was `Effect.race` having
15// prefer-success semantics in Effect v4: the racing pause-signal Deferred
16// never resolves, so a fiber failure is never observed by the racer.
17
18class FakeRuntimeError extends Data.TaggedError("FakeRuntimeError")<{
19 readonly message: string;
20}> {}
21
22const failingExecutor: CodeExecutor<FakeRuntimeError> = {
23 execute: () =>
24 Effect.fail(new FakeRuntimeError({ message: "Unexpected token ':'" })),
25};
26
27const succeedingExecutor: CodeExecutor<FakeRuntimeError> = {
28 execute: () =>
29 Effect.succeed({ result: "ok", logs: [] } satisfies ExecuteResult),
30};
31
32const emptyPlugin = definePlugin(() => ({
33 id: "empty-test" as const,
34 storage: () => ({}),
35 staticSources: () => [],
36}));
37
38const makeExecutor = () => createExecutor(makeTestConfig({ plugins: [emptyPlugin()] as const }));
39
40describe("executeWithPause failure propagation", () => {
41 it.effect("surfaces a fast codeExecutor failure as an Exit.Failure", () =>
42 Effect.gen(function* () {
43 const executor = yield* makeExecutor();
44 const engine = createExecutionEngine({
45 executor,
46 codeExecutor: failingExecutor,
47 });
48
49 const exit = yield* Effect.exit(engine.executeWithPause("noop"));
50 expect(Exit.isFailure(exit)).toBe(true);
51 }),
52 );
53
54 it.effect("does not hang when codeExecutor fails", () =>
55 Effect.gen(function* () {
56 const executor = yield* makeExecutor();
57 const engine = createExecutionEngine({
58 executor,
59 codeExecutor: failingExecutor,
60 });
61
62 // Race the executeWithPause against a short sleep. With the bug
63 // present this resolves to "hung" because the failure is swallowed
64 // by the prefer-success race against the pause Deferred.
65 const outcome = yield* Effect.race(
66 Effect.exit(engine.executeWithPause("noop")).pipe(
67 Effect.map((exit) => ({ kind: "settled" as const, exit })),
68 ),
69 Effect.sleep("500 millis").pipe(Effect.as({ kind: "hung" as const })),
70 );
71
72 expect(outcome.kind).toBe("settled");
73 }),
74 );
75
76 it.effect("control: succeedingExecutor returns completed", () =>
77 Effect.gen(function* () {
78 const executor = yield* makeExecutor();
79 const engine = createExecutionEngine({
80 executor,
81 codeExecutor: succeedingExecutor,
82 });
83
84 const result = yield* engine.executeWithPause("noop");
85 expect(result.status).toBe("completed");
86 }),
87 );
88});
packages/core/execution/src/engine.ts+9−1
@@ -353,12 +353,20 @@ export const createExecutionEngine = < · 352 unmodified lines
353 * Race a running fiber against a pause signal. Returns when either353 * Race a running fiber against a pause signal. Returns when either
354 * the fiber completes or an elicitation handler fires (whichever354 * the fiber completes or an elicitation handler fires (whichever
355 * comes first). Re-used by both executeWithPause and resume.355 * comes first). Re-used by both executeWithPause and resume.
356 *
357 * `Effect.raceFirst` (not `Effect.race`) — `race` has prefer-success
358 * semantics in Effect v4 ("first successful result"), which means a
359 * fiber failure waits indefinitely for the pause Deferred to succeed.
360 * For a fast `codeExecutor.execute` failure (e.g. a syntax error
361 * inside the dynamic worker) the pause signal never fires, so the
362 * outer Effect hangs until the upstream client gives up. `raceFirst`
363 * settles on whichever side completes first, success or failure.
356 */364 */
357 const awaitCompletionOrPause = (365 const awaitCompletionOrPause = (
358 fiber: Fiber.Fiber<ExecuteResult, E>,366 fiber: Fiber.Fiber<ExecuteResult, E>,
359 pauseSignal: Deferred.Deferred<InternalPausedExecution<E>>,367 pauseSignal: Deferred.Deferred<InternalPausedExecution<E>>,
360 ): Effect.Effect<ExecutionResult, E> =>368 ): Effect.Effect<ExecutionResult, E> =>
361 Effect.race(369 Effect.raceFirst(
362 Fiber.join(fiber).pipe(370 Fiber.join(fiber).pipe(
363 Effect.map((result): ExecutionResult => ({ status: "completed", result })),371 Effect.map((result): ExecutionResult => ({ status: "completed", result })),
364 ),372 ),