History

Make MCP pause/resume idempotent with collision-proof ids and honest expiry errors (#976)

* Add failing e2e: paused approval must survive the session runtime idle teardown Reproduces the reported bug: with model elicitation over streamable HTTP, execute returns a paused executionId, but the session DO's 5-minute idle alarm calls closeRuntime() and rebuilds the engine from storage on the next request — the in-memory pause map (fiber + Deferred) is gone, so resume answers 'No paused execution: exec_1'. A 20-second gap passes (the runtime is still warm); the 6-minute gap fails deterministically, so the loss is the app's own idle teardown, not a workerd eviction race. * Make pause/resume honest and idempotent; ids unique across engine rebuilds Production telemetry showed two failure classes behind 'No paused execution': duplicate resumes (client retries a lost response seconds after a successful resume) and pauses outliving the session runtime (idle teardown, isolate eviction). Durable pause state is intentionally out of scope; instead: - resume() is idempotent per executionId: settled outcomes are cached (exits, so failures re-fail typed) and replayed on retry; a concurrent duplicate joins the in-flight resume via a Deferred instead of missing the consumed pause. - execution ids are exec_<uuid> instead of a per-engine exec_N counter, so a rebuilt engine (cold restore, redeploy) can never reissue an id a stale client still holds and bind its resume to a different execution's pause. - a sandbox fiber that settles without a resume (timeout, failure) drops its abandoned pause entries and records the terminal outcome, so getPausedExecution stops reporting dead pauses and the map no longer leaks. - the resume miss now explains itself: paused executions expire, recovery is re-running execute. Structured status execution_not_found, recovery re_execute. The idle-teardown e2e scenario now specs the accepted contract: an approval paused past the idle window expires with the re-run guidance and the session keeps working (fresh execute pauses under a new id and resumes to completion); a new scenario locks duplicate-resume replay end-to-end.

Rhys Sullivan committed 5 months ago

Commit 8244fee1 parent 04350de5 files · +355 −11Browse files

Unified · Split

5 files
  1. .changeset/pause-resume-idempotent.md +12 −0
  2. e2e/cloud/mcp-client-sessions.test.ts +148 −0
  3. packages/core/execution/src/engine.ts +82 −3
  4. packages/core/execution/src/tool-invoker.test.ts +88 −0
  5. packages/hosts/mcp/src/tool-server.ts +25 −8
.changeset/pause-resume-idempotent.md+12−0
@@ -0,0 +1,12 @@
1+---
2+"@executor-js/execution": patch
3+"@executor-js/host-mcp": patch
4+---
5+
6+Make paused-execution resume reliable: `resume` is now idempotent (a retried
7+resume replays the recorded outcome instead of failing with "No paused
8+execution"), execution ids are globally unique so a rebuilt engine can never
9+re-mint an id a stale client still holds, pauses abandoned by a dead sandbox
10+are dropped and their terminal outcome kept for late resumes, and an expired
11+or lost pause now returns recovery guidance (re-run execute) instead of a bare
12+miss.
e2e/cloud/mcp-client-sessions.test.ts+148−0
@@ -213,6 +213,154 @@ const result = await tools.executor.coreTools.policies.list({}); · 212 unmodified lines
213213 return JSON.stringify(result);
214214 `;
215215
216+const executionIdOf = (text: string): string | undefined =>
217+ /\bexecutionId:\s*(\S+)/.exec(text)?.[1];
218+
219+// The session DO tears its runtime down after 5 minutes without a request
220+// (SESSION_TIMEOUT_MS in McpSessionDOBase) and rebuilds it from storage on
221+// the next one — the same engine-state wipe a workerd eviction or a deploy
222+// causes. Paused approvals deliberately do NOT survive this (durable pause
223+// state is out of scope); the contract is that an expired pause fails with
224+// recovery guidance and the session keeps working.
225+const IDLE_TEARDOWN_GAP = "6 minutes";
226+
227+scenario(
228+ "MCP sessions · an approval paused past the idle window expires with re-run guidance, not a dead end",
229+ { timeout: 480_000 },
230+ Effect.gen(function* () {
231+ const target = yield* Target;
232+ const { client } = yield* Api;
233+ const mcp = yield* Mcp;
234+ const identity = yield* target.newIdentity();
235+ const api = yield* client(coreApi, identity);
236+ const bearer = yield* mcp.mintBearer(emailOf(identity));
237+
238+ const policy = yield* api.policies.create({
239+ payload: { owner: "org", pattern: APPROVAL_TARGET_TOOL, action: "require_approval" },
240+ });
241+
242+ yield* Effect.gen(function* () {
243+ const first = yield* Effect.promise(() => connectClient(target.mcpUrl, bearer));
244+ const sessionId = first.transport.sessionId;
245+ const paused = yield* Effect.promise(() =>
246+ first.client.callTool({ name: "execute", arguments: { code: GATED_CODE } }),
247+ ).pipe(Effect.ensuring(closeQuietly(first)));
248+ const pausedText = textOf(paused);
249+ expect(pausedText, "the gated call pauses instead of completing").toContain(
250+ "Execution paused",
251+ );
252+ const executionId = executionIdOf(pausedText);
253+ expect(executionId, "the paused result carries the executionId").toEqual(expect.any(String));
254+
255+ // The user thinks the approval over for longer than the session keeps
256+ // its runtime warm; the pause is gone when they come back.
257+ yield* Effect.sleep(IDLE_TEARDOWN_GAP);
258+
259+ const second = yield* Effect.promise(() => connectClient(target.mcpUrl, bearer, sessionId));
260+ yield* Effect.gen(function* () {
261+ const resumed = yield* Effect.promise(() =>
262+ second.client.callTool({
263+ name: "resume",
264+ arguments: { executionId: executionId ?? "", action: "accept", content: "{}" },
265+ }),
266+ );
267+ expect(resumed.isError, "the expired resume is an error, not a silent success").toBe(true);
268+ expect(
269+ textOf(resumed),
270+ "the error tells the model how to recover, not just that the pause is gone",
271+ ).toContain("run the execute tool again");
272+
273+ // The advertised recovery actually works on the same session: a fresh
274+ // execute pauses with a NEW id (stale ids are never reused), and
275+ // resuming that completes the gated call.
276+ const reExecuted = yield* Effect.promise(() =>
277+ second.client.callTool({ name: "execute", arguments: { code: GATED_CODE } }),
278+ );
279+ const reExecutedText = textOf(reExecuted);
280+ expect(reExecutedText, "the re-run pauses for approval again").toContain(
281+ "Execution paused",
282+ );
283+ const freshExecutionId = executionIdOf(reExecutedText);
284+ expect(freshExecutionId, "the re-run mints a different executionId").not.toBe(executionId);
285+
286+ const resumedFresh = yield* Effect.promise(() =>
287+ second.client.callTool({
288+ name: "resume",
289+ arguments: { executionId: freshExecutionId ?? "", action: "accept", content: "{}" },
290+ }),
291+ );
292+ expect(resumedFresh.isError, "the fresh approval resumes to completion").not.toBe(true);
293+ expect(textOf(resumedFresh), "the gated tool's result comes back after approval").toContain(
294+ APPROVAL_TARGET_TOOL,
295+ );
296+ }).pipe(Effect.ensuring(closeQuietly(second)));
297+ }).pipe(
298+ Effect.ensuring(
299+ api.policies
300+ .remove({ params: { policyId: policy.id }, payload: { owner: "org" } })
301+ .pipe(Effect.ignore),
302+ ),
303+ );
304+ }),
305+);
306+
307+scenario(
308+ "MCP sessions · a duplicate resume replays the outcome instead of losing the approval",
309+ {},
310+ Effect.gen(function* () {
311+ const target = yield* Target;
312+ const { client } = yield* Api;
313+ const mcp = yield* Mcp;
314+ const identity = yield* target.newIdentity();
315+ const api = yield* client(coreApi, identity);
316+ const bearer = yield* mcp.mintBearer(emailOf(identity));
317+
318+ const policy = yield* api.policies.create({
319+ payload: { owner: "org", pattern: APPROVAL_TARGET_TOOL, action: "require_approval" },
320+ });
321+
322+ yield* Effect.gen(function* () {
323+ const session = yield* Effect.promise(() => connectClient(target.mcpUrl, bearer));
324+ yield* Effect.gen(function* () {
325+ const paused = yield* Effect.promise(() =>
326+ session.client.callTool({ name: "execute", arguments: { code: GATED_CODE } }),
327+ );
328+ const executionId = executionIdOf(textOf(paused));
329+ expect(executionId, "the paused result carries the executionId").toEqual(
330+ expect.any(String),
331+ );
332+
333+ const resume = () =>
334+ Effect.promise(() =>
335+ session.client.callTool({
336+ name: "resume",
337+ arguments: { executionId: executionId ?? "", action: "accept", content: "{}" },
338+ }),
339+ );
340+
341+ const first = yield* resume();
342+ expect(first.isError, "the first resume completes the execution").not.toBe(true);
343+
344+ // MCP clients retry resume when a response is lost in transit. The
345+ // retry must replay the same completed outcome — the production
346+ // failure mode was "No paused execution" seconds after a successful
347+ // resume.
348+ const retry = yield* resume();
349+ expect(retry.isError, "the duplicate resume is not an error").not.toBe(true);
350+ expect(textOf(retry), "the duplicate resume returns the same completed result").toContain(
351+ APPROVAL_TARGET_TOOL,
352+ );
353+ }).pipe(Effect.ensuring(closeQuietly(session)));
354+ }).pipe(
355+ Effect.ensuring(
356+ api.policies
357+ .remove({ params: { policyId: policy.id }, payload: { owner: "org" } })
358+ .pipe(Effect.ignore),
359+ ),
360+ );
361+ }),
362+);
363+
216364 scenario(
217365 "MCP sessions · a paused approval survives the client reconnecting and resumes",
218366 {},
packages/core/execution/src/engine.ts+82−3
@@ -1,5 +1,6 @@
11 import { Deferred, Effect, Fiber, Predicate, Queue } from "effect";
22 import type * as Cause from "effect/Cause";
3+import * as Exit from "effect/Exit";
34
45 import type {
56 Executor,
@@ -377,7 +378,29 @@ export const createExecutionEngine = <E extends Cause.YieldableError = CodeExecu · 371 unmodified lines
377378 ): ExecutionEngine<E> => {
378379 const { executor, codeExecutor, toolDiscoveryProvider = defaultToolDiscoveryProvider } = config;
379380 const pausedExecutions = new Map<string, InternalPausedExecution<E>>();
380− let nextId = 0;
381+ // Outcomes of executions that already settled (resumed to completion, hit a
382+ // new pause, or died while paused). MCP clients retry `resume` when a
383+ // response gets lost in transit; without this cache the retry of an
384+ // already-delivered resume answers "no paused execution" (observed in
385+ // production seconds after a successful resume). Bounded FIFO — pause
386+ // volume is tiny (human approvals), so a small window is plenty.
387+ const settledOutcomes = new Map<string, Exit.Exit<ExecutionResult, E>>();
388+ const SETTLED_OUTCOME_LIMIT = 64;
389+ // Resumes whose outcome is still being computed, so a concurrent duplicate
390+ // awaits the same result instead of missing the (already-consumed) pause.
391+ const pendingResumes = new Map<string, Deferred.Deferred<ExecutionResult, E>>();
392+
393+ // Exits (not just successes) so a replayed failure re-fails through the
394+ // typed channel — hosts render engine failures opaquely, and a replay must
395+ // not bypass that by flattening the cause into result text.
396+ const recordSettledOutcome = (executionId: string, exit: Exit.Exit<ExecutionResult, E>): void => {
397+ settledOutcomes.set(executionId, exit);
398+ while (settledOutcomes.size > SETTLED_OUTCOME_LIMIT) {
399+ const oldest = settledOutcomes.keys().next().value;
400+ if (oldest === undefined) break;
401+ settledOutcomes.delete(oldest);
402+ }
403+ };
381404
382405 /**
383406 * Race a running fiber against the pause queue. Returns when either
@@ -427,7 +450,11 @@ export const createExecutionEngine = <E extends Cause.YieldableError = CodeExecu · 43 unmodified lines
427450 const elicitationHandler: ElicitationHandler = (ctx) =>
428451 Effect.gen(function* () {
429452 const responseDeferred = yield* Deferred.make<typeof ElicitationResponse.Type>();
430− const id = `exec_${++nextId}`;
453+ // Globally unique — engine instances are rebuilt on host restarts
454+ // (Durable Object cold restores, redeploys), so a counter would
455+ // re-mint the same ids and let a stale client resume bind to a
456+ // different execution's pause.
457+ const id = `exec_${crypto.randomUUID()}`;
431458
432459 const paused: InternalPausedExecution<E> = {
433460 id,
@@ -453,12 +480,41 @@ export const createExecutionEngine = <E extends Cause.YieldableError = CodeExecu · 19 unmodified lines
453480 codeExecutor.execute(code, invoker).pipe(Effect.withSpan("executor.code.exec")),
454481 );
455482
483+ // When the fiber settles on its own (sandbox timeout, failure) while
484+ // pauses are still outstanding, drop them: getPausedExecution must not
485+ // report a pause whose fiber can no longer consume a response, and the
486+ // map must not grow forever. A resume retry still finds the terminal
487+ // outcome via the settled-outcome cache.
488+ const sandboxFiber = fiber;
489+ yield* Effect.forkDetach(
490+ Fiber.await(sandboxFiber).pipe(
491+ Effect.flatMap((exit) =>
492+ Effect.sync(() => {
493+ const outcome = Exit.map(
494+ exit,
495+ (result): ExecutionResult => ({ status: "completed", result }),
496+ );
497+ for (const [id, paused] of pausedExecutions) {
498+ if (paused.fiber !== sandboxFiber) continue;
499+ pausedExecutions.delete(id);
500+ recordSettledOutcome(id, outcome);
501+ }
502+ }),
503+ ),
504+ ),
505+ );
506+
456507 return (yield* awaitCompletionOrPause(fiber, pauseQueue)) as ExecutionResult;
457508 });
458509
459510 /**
460511 * Resume a paused execution. Completes the response Deferred to unblock the
461512 * fiber, then races completion against the next queued or future pause.
513+ *
514+ * Idempotent per executionId: MCP clients retry `resume` when a response is
515+ * lost in transit, so a duplicate of an already-delivered resume replays the
516+ * recorded outcome, and a duplicate that arrives while the first is still
517+ * in flight awaits the same outcome instead of reporting a missing pause.
462518 */
463519 const resumeExecution = Effect.fn("mcp.execute.resume")(function* (
464520 executionId: string,
@@ -468,16 +524,39 @@ export const createExecutionEngine = <E extends Cause.YieldableError = CodeExecu · 3 unmodified lines
468524 "mcp.execute.resume.action": response.action,
469525 });
470526
527+ const settled = settledOutcomes.get(executionId);
528+ if (settled) {
529+ yield* Effect.annotateCurrentSpan({ "mcp.execute.resume.replayed": true });
530+ return (yield* settled) as ExecutionResult;
531+ }
532+
533+ const pending = pendingResumes.get(executionId);
534+ if (pending) {
535+ yield* Effect.annotateCurrentSpan({ "mcp.execute.resume.joined_inflight": true });
536+ return (yield* Deferred.await(pending)) as ExecutionResult;
537+ }
538+
471539 const paused = pausedExecutions.get(executionId);
472540 if (!paused) return null;
473541 pausedExecutions.delete(executionId);
474542
543+ const inflight = yield* Deferred.make<ExecutionResult, E>();
544+ pendingResumes.set(executionId, inflight);
545+
475546 yield* Deferred.succeed(paused.response, {
476547 action: response.action as typeof ElicitationResponse.Type.action,
477548 content: response.content,
478549 });
479550
480− return (yield* awaitCompletionOrPause(paused.fiber, paused.pauseQueue)) as ExecutionResult;
551+ return (yield* awaitCompletionOrPause(paused.fiber, paused.pauseQueue).pipe(
552+ Effect.onExit((exit) =>
553+ Effect.gen(function* () {
554+ recordSettledOutcome(executionId, exit);
555+ pendingResumes.delete(executionId);
556+ yield* Deferred.done(inflight, exit);
557+ }),
558+ ),
559+ )) as ExecutionResult;
481560 });
482561
483562 /**
packages/core/execution/src/tool-invoker.test.ts+88−0
@@ -1290,6 +1290,94 @@ describe("pause/resume with multiple elicitations", () => { · 1289 unmodified lines
12901290 { timeout: 10000 },
12911291 );
12921292
1293+ it.effect(
1294+ "execution ids are unique across engine instances (no counter reuse)",
1295+ () =>
1296+ Effect.gen(function* () {
1297+ const executor = yield* makeElicitingExecutor();
1298+ const engineA = createExecutionEngine({ executor, codeExecutor });
1299+ const engineB = createExecutionEngine({ executor, codeExecutor });
1300+ const code = "return await tools.api.org.main.singleApproval({});";
1301+
1302+ const pausedA = yield* engineA.executeWithPause(code);
1303+ const pausedB = yield* engineB.executeWithPause(code);
1304+ expect(pausedA.status).toBe("paused");
1305+ expect(pausedB.status).toBe("paused");
1306+ if (pausedA.status !== "paused" || pausedB.status !== "paused") return;
1307+
1308+ // A rebuilt engine (host restart) must never re-mint an id a client
1309+ // may still hold — a stale resume would bind to the wrong pause.
1310+ expect(pausedA.execution.id).not.toBe(pausedB.execution.id);
1311+
1312+ yield* engineA.resume(pausedA.execution.id, { action: "accept" });
1313+ yield* engineB.resume(pausedB.execution.id, { action: "accept" });
1314+ }),
1315+ { timeout: 10000 },
1316+ );
1317+
1318+ it.effect(
1319+ "a duplicate resume replays the delivered outcome instead of reporting a missing pause",
1320+ () =>
1321+ Effect.gen(function* () {
1322+ const executor = yield* makeElicitingExecutor();
1323+ const engine = createExecutionEngine({ executor, codeExecutor });
1324+ const code = "return await tools.api.org.main.singleApproval({});";
1325+
1326+ const outcome1 = yield* engine.executeWithPause(code);
1327+ expect(outcome1.status).toBe("paused");
1328+ if (outcome1.status !== "paused") return;
1329+ const executionId = outcome1.execution.id;
1330+
1331+ const first = yield* engine.resume(executionId, { action: "accept" });
1332+ expect(first?.status).toBe("completed");
1333+
1334+ // The MCP client retries resume when the response is lost in
1335+ // transit; the retry must return the same completed outcome.
1336+ const retry = yield* engine.resume(executionId, { action: "accept" });
1337+ expect(retry?.status).toBe("completed");
1338+ const completed = retry as Extract<NonNullable<typeof retry>, { status: "completed" }>;
1339+ expect(completed.result.error).toBeUndefined();
1340+ expect(completed.result.result).toMatchObject({ ok: true });
1341+ }),
1342+ { timeout: 10000 },
1343+ );
1344+
1345+ // live clock: the sandbox timeout is a real timer, so the test must
1346+ // actually wait for it rather than suspend on the virtual TestClock.
1347+ it.live(
1348+ "a pause abandoned by a failing sandbox is dropped and its resume replays the failure outcome",
1349+ () =>
1350+ Effect.gen(function* () {
1351+ const executor = yield* makeElicitingExecutor();
1352+ // Sandbox times out while suspended on the elicitation, so the fiber
1353+ // settles without a resume ever arriving.
1354+ const engine = createExecutionEngine({
1355+ executor,
1356+ codeExecutor: makeQuickJsExecutor({ timeoutMs: 250 }),
1357+ });
1358+ const code = "return await tools.api.org.main.singleApproval({});";
1359+
1360+ const outcome1 = yield* engine.executeWithPause(code);
1361+ expect(outcome1.status).toBe("paused");
1362+ if (outcome1.status !== "paused") return;
1363+ const executionId = outcome1.execution.id;
1364+
1365+ // Wait for the sandbox timeout to settle the detached fiber.
1366+ yield* Effect.sleep("600 millis");
1367+
1368+ // The dead pause must no longer be reported as live...
1369+ const stillPaused = yield* engine.getPausedExecution(executionId);
1370+ expect(stillPaused).toBeNull();
1371+
1372+ // ...and a late resume surfaces the terminal outcome, not a miss.
1373+ const late = yield* engine.resume(executionId, { action: "accept" });
1374+ expect(late?.status).toBe("completed");
1375+ const completed = late as Extract<NonNullable<typeof late>, { status: "completed" }>;
1376+ expect(completed.result.error).toContain("timed out");
1377+ }),
1378+ { timeout: 10000 },
1379+ );
1380+
12931381 // Regression: use separate top-level runPromise calls to match HTTP/CLI
12941382 // pause/resume, and a single-elicit tool so no later pause can mask a dead
12951383 // sandbox fiber.
packages/hosts/mcp/src/tool-server.ts+25−8
@@ -340,6 +340,29 @@ const toMcpFailureResult = (cause: Cause.Cause<unknown>): McpToolResult => { · 339 unmodified lines
340340 };
341341 };
342342
343+// A paused execution lives in the session runtime's memory: it expires when
344+// the user takes too long to answer, and dies early when the runtime is
345+// rebuilt (host restart, redeploy). Either way the recovery is the same and
346+// the model should be told it, not just handed a miss.
347+const missingExecutionResult = (executionId: string): McpToolResult => ({
348+ content: [
349+ {
350+ type: "text" as const,
351+ text: [
352+ `No paused execution: ${executionId}.`,
353+ "The paused execution expired or was lost when its session was restarted — paused executions only stay resumable for a few minutes.",
354+ "To recover, run the execute tool again with the original code; if it pauses, a fresh executionId will be issued.",
355+ ].join(" "),
356+ },
357+ ],
358+ structuredContent: {
359+ status: "execution_not_found",
360+ executionId,
361+ recovery: "re_execute",
362+ },
363+ isError: true,
364+});
365+
343366 const JsonObjectFromString = Schema.fromJsonString(Schema.Record(Schema.String, Schema.Unknown));
344367 const decodeJsonObjectString = Schema.decodeUnknownOption(JsonObjectFromString);
345368
@@ -460,10 +483,7 @@ export const createExecutorMcpServer = <E extends Cause.YieldableError>( · 114 unmodified lines
460483 const outcome = yield* engine.resume(executionId, { action, content });
461484 if (!outcome) {
462485 debugLog("resume.missing_execution", { executionId });
463− return {
464− content: [{ type: "text" as const, text: `No paused execution: ${executionId}` }],
465− isError: true,
466− } satisfies McpToolResult;
486+ return missingExecutionResult(executionId);
467487 }
468488 debugLog("resume.result", {
469489 executionId,
@@ -535,10 +555,7 @@ export const createExecutorMcpServer = <E extends Cause.YieldableError>( · 65 unmodified lines
535555
536556 const outcome = yield* engine.resume(executionId, response);
537557 if (!outcome) {
538− return {
539− content: [{ type: "text" as const, text: `No paused execution: ${executionId}` }],
540− isError: true,
541− } satisfies McpToolResult;
558+ return missingExecutionResult(executionId);
542559 }
543560 return outcome.status === "completed"
544561 ? toMcpResult(formatExecuteResult(outcome.result))