From 8d0b516d018c713d9c96779f735d0a08b9ed6d5d Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:33:14 -0700 Subject: [PATCH 1/3] Classify Durable Object platform failures as retryable protocol errors --- apps/cloud/src/mcp/agent-handler.ts | 81 ++++++- apps/cloud/src/mcp/session-durable-object.ts | 7 + apps/cloud/src/observability/index.ts | 41 +++- .../src/observability/observability.test.ts | 72 +++++++ .../mcp-destroyed-session-envelope.test.ts | 202 ++++++++++++++++++ packages/hosts/cloudflare/package.json | 4 + .../mcp/agent-session-durable-object.test.ts | 154 +++++++++++++ .../src/mcp/agent-session-durable-object.ts | 111 +++++++++- .../src/mcp/durable-object-errors.test.ts | 185 ++++++++++++++++ .../src/mcp/durable-object-errors.ts | 181 ++++++++++++++++ 10 files changed, 1017 insertions(+), 21 deletions(-) create mode 100644 e2e/cloud/mcp-destroyed-session-envelope.test.ts create mode 100644 packages/hosts/cloudflare/src/mcp/durable-object-errors.test.ts create mode 100644 packages/hosts/cloudflare/src/mcp/durable-object-errors.ts diff --git a/apps/cloud/src/mcp/agent-handler.ts b/apps/cloud/src/mcp/agent-handler.ts index 9d75300bf..6d46bbd80 100644 --- a/apps/cloud/src/mcp/agent-handler.ts +++ b/apps/cloud/src/mcp/agent-handler.ts @@ -17,6 +17,11 @@ import { withVerifiedIdentityHeaders, } from "@executor-js/cloudflare/mcp/do-headers"; import type { McpSessionProps } from "@executor-js/cloudflare/mcp/agent-durable-object"; +import { + classifyDurableObjectError, + durableObjectFailureResponse, + type DurableObjectFailure, +} from "@executor-js/cloudflare/mcp/durable-object-errors"; import { mcpSessionStub } from "@executor-js/cloudflare/mcp/session-stub"; import { wrapMcpSseResponse } from "../observability/memory-metrics"; @@ -80,6 +85,41 @@ const renderAuthError = ( }); }; +/** + * A Cloudflare *platform* Durable Object failure happened at one of this + * handler's stub touchpoints. Record what kind it was — on an exported span and + * in a structured log — so the production volume stays countable per cause + * (deploy reset vs storage timeout vs destroyed session) now that it is no + * longer a pile of 500s. + * + * Talking to a session DO means talking to a process the platform can reset out + * from under us: a deploy, a storage timeout, a backend blip, the session's own + * `ctx.abort("destroyed")`. None of those are application defects. An + * unrecognized failure never reaches here and keeps escaping as before. + */ +const recordDurableObjectFailure = ( + failure: DurableObjectFailure, + operation: string, +): Effect.Effect => + Effect.sync(() => { + console.warn( + JSON.stringify({ + event: "mcp_durable_object_platform_failure", + operation, + resetKind: failure.kind, + disposition: failure.disposition, + }), + ); + }).pipe( + Effect.withSpan("mcp.do.platform_failure", { + attributes: { + "mcp.do.reset_kind": failure.kind, + "mcp.do.reset_disposition": failure.disposition, + "mcp.do.reset_operation": operation, + }, + }), + ); + const authenticate = (request: Request) => Effect.gen(function* () { const auth = yield* McpAuthProvider; @@ -201,10 +241,27 @@ export const makeCloudMcpAgentHandler = () => { } if (sessionId) { - const owner = await mcpSessionStub(env.MCP_SESSION, sessionId).validateMcpSessionOwner({ - accountId: outcome.principal.accountId, - organizationId: outcome.principal.organizationId, - }); + let owner: "ok" | "not_found" | "forbidden" | "terminated"; + // oxlint-disable-next-line executor/no-try-catch-or-throw -- adapter boundary: a Durable Object stub RPC rejects with a plain platform Error, never a typed failure + try { + owner = await mcpSessionStub(env.MCP_SESSION, sessionId).validateMcpSessionOwner({ + accountId: outcome.principal.accountId, + organizationId: outcome.principal.organizationId, + }); + } catch (error) { + // The sibling stub touchpoints in this handler are both guarded — the + // `_cf_scheduleDestroy` call above with `Effect.ignore`, the + // `target.fetch` below with a catch — and this one was not, so a session + // whose DO had been destroyed or reset by the platform 500ed here before + // any of that handling could run. + const failure = classifyDurableObjectError(error); + if (!failure) { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- adapter boundary: an unrecognized failure is a real defect and must reach the runtime unchanged + throw error; + } + await runTraced(request, recordDurableObjectFailure(failure, "validate_session_owner")); + return durableObjectFailureResponse(failure); + } if (owner === "not_found") { return jsonRpcResponse(404, -32001, "Session not found"); } @@ -244,12 +301,18 @@ export const makeCloudMcpAgentHandler = () => { // DO ever getting to answer. Map it to the old envelope's reconnect // error for a dead session (e2e/cloud/mcp-protocol.test.ts expects the // client to be told to reconnect, matching a timed-out session). - // oxlint-disable-next-line executor/no-unknown-error-message -- adapter boundary: the abort reason is a plain runtime Error whose message IS the signal - if (Predicate.isError(error) && error.message === "destroyed") { - return jsonRpcResponse(404, -32001, "Session timed out, please reconnect"); + // + // The same catch now also covers the rest of the platform's reset + // vocabulary — a deploy, a storage timeout, a cancelled + // blockConcurrencyWhile — which reaches here through the agents SDK's own + // `getServerByName` retry and used to 500 identically. + const failure = classifyDurableObjectError(error); + if (!failure) { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- adapter boundary: rethrow anything that isn't a recognized platform failure to the Workers runtime unchanged + throw error; } - // oxlint-disable-next-line executor/no-try-catch-or-throw -- adapter boundary: rethrow anything that isn't the condemned-DO abort to the Workers runtime unchanged - throw error; + await runTraced(request, recordDurableObjectFailure(failure, "session_fetch")); + return durableObjectFailureResponse(failure); } // The agents SDK answers a bare DELETE with 204; the old envelope's // contract (see above) was 200 — rewrite for consistency. diff --git a/apps/cloud/src/mcp/session-durable-object.ts b/apps/cloud/src/mcp/session-durable-object.ts index c272e5026..50354810f 100644 --- a/apps/cloud/src/mcp/session-durable-object.ts +++ b/apps/cloud/src/mcp/session-durable-object.ts @@ -71,6 +71,7 @@ import { DoTelemetryLive, flushTracerProvider } from "../observability/telemetry import { captureCause as reportCause, captureCauseEffect as reportCauseEffect, + claimCauseHandledByDurableObject, tagCurrentSentryScopeWithCurrentOtelSpan, } from "../observability"; import { parseTraceparent } from "./traceparent"; @@ -368,6 +369,12 @@ export class McpSessionDOSqlite extends McpAgentSessionDOBase): Effect.Effect { + return claimCauseHandledByDurableObject; + } + // Best-effort export the DO isolate's buffered spans after the RPC settles, // so a dying init/handleRequest can ship its own spans (and the exception + // stack recorded on them) — not just the worker-side `mcp.do.*` span. Keep it diff --git a/apps/cloud/src/observability/index.ts b/apps/cloud/src/observability/index.ts index 6d1cd7e99..a56118a6c 100644 --- a/apps/cloud/src/observability/index.ts +++ b/apps/cloud/src/observability/index.ts @@ -30,6 +30,21 @@ export const OTEL_TRACE_ID_TAG = "otel_trace_id"; export const OTEL_SPAN_ID_TAG = "otel_span_id"; export const SENTRY_EVENT_ID_ATTRIBUTE = "sentry.event_id"; +/** + * Set by the MCP session Durable Object when it has finished deciding what to + * do about a cause — reported it, or classified it as an expected Cloudflare + * platform reset and deliberately not reported it. + * + * `instrumentDurableObjectWithSentry` wraps the DO's entry points and captures + * the same rejection again as it escapes, which is why one platform reset + * opened two issues for the same event. The DO is the better owner — it has + * the session, the org, the OTEL correlation and the classification — so its + * claim wins and the auto-instrumentation's echo is dropped in `beforeSend`. + * Nothing the DO does not claim is affected. + */ +export const DO_CAUSE_OWNER_TAG = "mcp.do.cause_owner"; +export const DO_CAUSE_OWNER_VALUE = "durable_object"; + export type OtelCorrelationContext = { readonly traceId: string; readonly spanId: string; @@ -99,10 +114,25 @@ export const tagCurrentSentryScopeWithCurrentOtelSpan: Effect.Effect { + if (event.tags?.[DO_CAUSE_OWNER_TAG] !== DO_CAUSE_OWNER_VALUE) return false; + const mechanism = event.exception?.values?.[0]?.mechanism?.type; + return typeof mechanism === "string" && mechanism.startsWith("auto."); +}; + export const beforeSendWithOtelCorrelation = ( event: ErrorEvent, options?: { readonly logPayload?: boolean }, -): ErrorEvent => { +): ErrorEvent | null => { + if (isClaimedDurableObjectEcho(event)) return null; if (options?.logPayload) { console.info( JSON.stringify({ @@ -167,6 +197,15 @@ export const captureCauseEffect = (input: unknown): Effect.Effect = Effect.sync(() => { + Sentry.getCurrentScope().setTag(DO_CAUSE_OWNER_TAG, DO_CAUSE_OWNER_VALUE); +}); + export const ErrorCaptureLive: Layer.Layer = Layer.succeed( ErrorCapture, ErrorCapture.of({ diff --git a/apps/cloud/src/observability/observability.test.ts b/apps/cloud/src/observability/observability.test.ts index b4f69a8d8..9185cbec8 100644 --- a/apps/cloud/src/observability/observability.test.ts +++ b/apps/cloud/src/observability/observability.test.ts @@ -2,8 +2,13 @@ import { describe, expect, it } from "@effect/vitest"; import { Cause, Effect } from "effect"; import type * as Tracer from "effect/Tracer"; +import type { ErrorEvent } from "@sentry/cloudflare"; + import { addCurrentOtelCorrelationTags, + beforeSendWithOtelCorrelation, + DO_CAUSE_OWNER_TAG, + DO_CAUSE_OWNER_VALUE, OTEL_SPAN_ID_TAG, OTEL_TRACE_ID_TAG, sentryPayloadForCause, @@ -91,3 +96,70 @@ describe("Sentry OTel correlation", () => { }).pipe(Effect.withSpan("test.sentry_capture"), Effect.withTracer(makeFixedTracer())), ); }); + +// One Durable Object failure used to open two Sentry issues: the DO's own +// `captureCause` seam reported it (mechanism `generic`), and then +// `instrumentDurableObjectWithSentry` reported the very same rejection again as +// it escaped the method (mechanism `auto.faas.cloudflare.durable_object`). The +// DO is the owner — it has the session, the classification and the OTel +// correlation — so its claim suppresses the echo and nothing else. +describe("Durable Object capture ownership", () => { + const doEcho = (overrides: Partial = {}): ErrorEvent => ({ + type: undefined, + tags: { [DO_CAUSE_OWNER_TAG]: DO_CAUSE_OWNER_VALUE }, + exception: { + values: [ + { + type: "Error", + value: "Durable Object reset because its code was updated.", + mechanism: { type: "auto.faas.cloudflare.durable_object", handled: false }, + }, + ], + }, + ...overrides, + }); + + it("drops the auto-instrumentation's copy of a cause the DO already claimed", () => { + expect(beforeSendWithOtelCorrelation(doEcho())).toBeNull(); + }); + + it("keeps the DO's own report, which carries no auto mechanism", () => { + const own = doEcho({ + exception: { + values: [ + { + type: "Error", + value: "Durable Object reset because its code was updated.", + mechanism: { type: "generic", handled: true }, + }, + ], + }, + }); + expect(beforeSendWithOtelCorrelation(own)).not.toBeNull(); + }); + + // An alarm crash or a transport fault is never claimed by the DO seam, and + // the auto-instrumentation is the ONLY thing that reports it. Dropping those + // would trade duplicate noise for silence. + it("keeps an unclaimed Durable Object failure", () => { + const unclaimed = doEcho({ tags: {} }); + expect(beforeSendWithOtelCorrelation(unclaimed)).not.toBeNull(); + }); + + it("keeps ordinary worker events untouched", () => { + const workerEvent: ErrorEvent = { + type: undefined, + tags: { [OTEL_TRACE_ID_TAG]: traceId }, + exception: { + values: [ + { + type: "TypeError", + value: "x is not a function", + mechanism: { type: "auto.http.cloudflare", handled: false }, + }, + ], + }, + }; + expect(beforeSendWithOtelCorrelation(workerEvent)).not.toBeNull(); + }); +}); diff --git a/e2e/cloud/mcp-destroyed-session-envelope.test.ts b/e2e/cloud/mcp-destroyed-session-envelope.test.ts new file mode 100644 index 000000000..07032f8be --- /dev/null +++ b/e2e/cloud/mcp-destroyed-session-envelope.test.ts @@ -0,0 +1,202 @@ +// Cloud: a terminated MCP session id must keep answering in the protocol's own +// vocabulary for its whole death sequence — not just in the first instant. +// +// `DELETE /mcp` condemns the session DO with a durable marker and defers the +// real teardown to an alarm ~1s later; that alarm's `destroy()` wipes storage +// and then `ctx.abort("destroyed")`s the isolate. e2e/cloud/mcp-protocol.test.ts +// already covers the FIRST millisecond of that window (the marker is read and a +// 404 reconnect comes back). This scenario covers the rest of it: a client that +// keeps talking to the dead id across the alarm and the abort — exactly what a +// retrying MCP client does — must always get a well-formed JSON-RPC envelope, +// either 404 (the id is dead, reconnect) or a retryable 503, and never a bare +// unhandled 500 from a Durable Object platform error escaping the handler. +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; + +import { scenario } from "../src/scenario"; +import { Mcp, Target } from "../src/services"; +import type { Identity } from "../src/target"; + +const JSON_AND_SSE = "application/json, text/event-stream"; +const PROTOCOL_VERSION = "2025-03-26"; + +const INITIALIZE_REQUEST = { + jsonrpc: "2.0" as const, + id: 1, + method: "initialize", + params: { + protocolVersion: PROTOCOL_VERSION, + capabilities: {}, + clientInfo: { name: "executor-e2e-destroyed-session-envelope", version: "0.0.1" }, + }, +}; + +const INITIALIZED_NOTIFICATION = { + jsonrpc: "2.0" as const, + method: "notifications/initialized", +}; + +const TOOLS_LIST_REQUEST = { + jsonrpc: "2.0" as const, + id: 2, + method: "tools/list", + params: {}, +}; + +const emailOf = (identity: Identity): string => identity.credentials?.email ?? identity.label; + +const mcpPost = ( + url: string | URL, + init: { readonly bearer: string; readonly sessionId?: string; readonly body: unknown }, +): Promise => + fetch(url, { + method: "POST", + headers: { + accept: JSON_AND_SSE, + "content-type": "application/json", + authorization: `Bearer ${init.bearer}`, + ...(init.sessionId ? { "mcp-session-id": init.sessionId } : {}), + }, + body: JSON.stringify(init.body), + }); + +const openSession = async (mcpUrl: string, bearer: string): Promise => { + const initialize = await mcpPost(mcpUrl, { bearer, body: INITIALIZE_REQUEST }); + const sessionId = initialize.headers.get("mcp-session-id"); + await initialize.text(); + if (initialize.status !== 200 || !sessionId) { + throw new Error(`openSession: initialize failed (${initialize.status})`); + } + const initialized = await mcpPost(mcpUrl, { bearer, sessionId, body: INITIALIZED_NOTIFICATION }); + await initialized.text(); + if (initialized.status !== 202) { + throw new Error(`openSession: notifications/initialized failed (${initialized.status})`); + } + return sessionId; +}; + +type Probe = { + readonly atMs: number; + readonly status: number; + readonly body: string; + readonly retryAfter: string | null; +}; + +/** One JSON-RPC error shape, or null when the body is not the protocol envelope. */ +const jsonRpcError = (body: string): { readonly code: number; readonly message: string } | null => { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- test helper: a non-JSON body is itself the signal we assert on + try { + const parsed = JSON.parse(body) as { + readonly jsonrpc?: string; + readonly error?: { readonly code?: number; readonly message?: string }; + }; + if (parsed.jsonrpc !== "2.0" || typeof parsed.error?.code !== "number") return null; + return { code: parsed.error.code, message: parsed.error.message ?? "" }; + } catch { + return null; + } +}; + +scenario( + "MCP protocol · a terminated session id stays a protocol error while its Durable Object is torn down", + { timeout: 180_000 }, + Effect.gen(function* () { + const target = yield* Target; + const mcp = yield* Mcp; + const identity = yield* target.newIdentity(); + const bearer = yield* mcp.mintBearer(emailOf(identity)); + + // Several sessions, because the fatal window is the isolate abort that + // follows the deferred destroy alarm and one session only crosses it once. + const sessions = 3; + // The destroy alarm is deferred ~1s; keep probing well past it so the + // probes straddle the alarm, the storage wipe and the abort that follows. + const probeWindowMs = 2_500; + const probeIntervalMs = 25; + const probeFanOut = 4; + + const probes: Probe[] = []; + + for (let attempt = 0; attempt < sessions; attempt += 1) { + const sessionId = yield* Effect.promise(() => openSession(target.mcpUrl, bearer)); + const terminate = yield* Effect.promise(() => + fetch(target.mcpUrl, { + method: "DELETE", + headers: { authorization: `Bearer ${bearer}`, "mcp-session-id": sessionId }, + }), + ); + expect(terminate.status, "the client can terminate its session").toBe(200); + yield* Effect.promise(() => terminate.text()); + + const startedAt = Date.now(); + const probe = async (): Promise => { + const at = Date.now() - startedAt; + const response = await mcpPost(target.mcpUrl, { + bearer, + sessionId, + body: TOOLS_LIST_REQUEST, + }); + probes.push({ + atMs: at, + status: response.status, + body: await response.text(), + retryAfter: response.headers.get("retry-after"), + }); + }; + // Concurrent, not sequential: the fatal moment is `ctx.abort("destroyed")` + // itself, which kills whatever is in flight. A one-at-a-time poll almost + // always misses it, so keep a fan of requests open across the whole + // teardown. + yield* Effect.promise(async () => { + const inFlight: Promise[] = []; + while (Date.now() - startedAt < probeWindowMs) { + for (let i = 0; i < probeFanOut; i += 1) inFlight.push(probe()); + await new Promise((resolve) => setTimeout(resolve, probeIntervalMs)); + } + await Promise.all(inFlight); + }); + } + + // What the dead id actually answered, so a reviewer can see the shape of + // the teardown window and not just the verdict. + const bucket = new Map(); + for (const probe of probes) { + const key = `${probe.status} ${jsonRpcError(probe.body)?.message ?? probe.body.slice(0, 80)}`; + bucket.set(key, (bucket.get(key) ?? 0) + 1); + } + console.info( + `[destroyed-session-envelope] ${probes.length} probes: ${[...bucket] + .map(([key, count]) => `${count}× ${key}`) + .join(" | ")}`, + ); + + const unhandled = probes.filter((probe) => probe.status >= 500 && probe.status !== 503); + expect( + unhandled.map((probe) => `+${probe.atMs}ms ${probe.status} ${probe.body.slice(0, 200)}`), + "no probe on a dead session id produces an unhandled server error", + ).toEqual([]); + + const malformed = probes.filter((probe) => jsonRpcError(probe.body) === null); + expect( + malformed.map((probe) => `+${probe.atMs}ms ${probe.status} ${probe.body.slice(0, 200)}`), + "every rejection is a JSON-RPC error envelope", + ).toEqual([]); + + for (const probe of probes) { + expect([404, 503], `+${probe.atMs}ms is a reconnect or a retry verdict`).toContain( + probe.status, + ); + } + + // A 503 in this window means "the platform is mid-reset, come back" — it is + // only actionable if the client is told how long to wait, so the retryable + // envelope must carry Retry-After. + const retryableWithoutBackoff = probes.filter( + (probe) => probe.status === 503 && probe.retryAfter === null, + ); + expect( + retryableWithoutBackoff.map((probe) => `+${probe.atMs}ms`), + "every retryable rejection tells the client how long to back off", + ).toEqual([]); + }), +); diff --git a/packages/hosts/cloudflare/package.json b/packages/hosts/cloudflare/package.json index f6a292759..1503f31fc 100644 --- a/packages/hosts/cloudflare/package.json +++ b/packages/hosts/cloudflare/package.json @@ -16,6 +16,10 @@ "types": "./src/mcp/agent-session-durable-object.ts", "default": "./src/mcp/agent-session-durable-object.ts" }, + "./mcp/durable-object-errors": { + "types": "./src/mcp/durable-object-errors.ts", + "default": "./src/mcp/durable-object-errors.ts" + }, "./mcp/execution-owner-directory": { "types": "./src/mcp/execution-owner-directory.ts", "default": "./src/mcp/execution-owner-directory.ts" diff --git a/packages/hosts/cloudflare/src/mcp/agent-session-durable-object.test.ts b/packages/hosts/cloudflare/src/mcp/agent-session-durable-object.test.ts index 8072df7e5..125b3990d 100644 --- a/packages/hosts/cloudflare/src/mcp/agent-session-durable-object.test.ts +++ b/packages/hosts/cloudflare/src/mcp/agent-session-durable-object.test.ts @@ -1,3 +1,4 @@ +// oxlint-disable executor/no-error-constructor, executor/no-try-catch-or-throw -- boundary: the storage fake reproduces the plain Errors the Cloudflare runtime throws, and rejecting is the only way a DurableObjectStorage reports them import { describe, expect, it } from "@effect/vitest"; import { Cause, Effect } from "effect"; import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; @@ -483,3 +484,156 @@ describe("McpAgentSessionDOBase transport restore", () => { expect(restoredEngine.calls).toEqual([{ executionId: "exec-model", response: approval }]); }); }); + +// Every Cloudflare deploy resets live Durable Objects: workerd aborts whatever +// storage operation is in flight with "Durable Object reset because its code was +// updated." That is the guaranteed consequence of shipping, not a defect — but +// it lands on whichever write `init()` happens to be doing, and the last thing +// `init()` does is `markActivity`, which writes a timestamp and arms the idle +// alarm. Nothing about a session depends on that write succeeding: the in-memory +// clock is already set, and every later touch re-arms the alarm. Losing a fully +// built, working session over it — and paging for the privilege — is the bug. +describe("McpAgentSessionDOBase init survives a platform reset of its bookkeeping write", () => { + const CODE_UPDATE_RESET = "Durable Object reset because its code was updated."; + + class ResettingStorage extends MemoryStorage { + /** Storage keys whose `put` should fail, and with what. */ + readonly putFailures = new Map Error>(); + setAlarmFailure: (() => Error) | null = null; + + override async put(key: string, value: unknown): Promise { + const failure = this.putFailures.get(key); + if (failure) { + this.putFailures.delete(key); + throw failure(); + } + await super.put(key, value); + } + + override async setAlarm(time: number | Date): Promise { + if (this.setAlarmFailure) { + const failure = this.setAlarmFailure; + this.setAlarmFailure = null; + throw failure(); + } + await super.setAlarm(time); + } + } + + type InitSession = { + ctx: ResettingStorage; + captureCause: (cause: Cause.Cause) => void; + dbHandle: { readonly end: () => void } | null; + engine: ExecutionEngine | null; + getSessionId: () => string; + init: () => Promise; + initialized: boolean; + lastActivityMs: number; + pendingApprovalLeases: Map; + props: Record; + server?: McpServer; + sessionTimeoutMs: () => number; + buildMcpServer: () => Effect.Effect<{ mcpServer: McpServer; engine: unknown }>; + openSessionDb: () => { readonly end: () => void }; + resolveSessionMeta: () => Effect.Effect; + validateMcpSessionOwner: (identity: McpApprovalOwner) => Promise; + }; + + const sessionMeta: SessionMeta = { + organizationId: "org-1", + organizationName: "Org 1", + userId: "user-1", + resource: defaultMcpResource, + }; + + const makeInitSession = (): { + session: InitSession; + storage: ResettingStorage; + captured: Cause.Cause[]; + } => { + const storage = new ResettingStorage(); + const captured: Cause.Cause[] = []; + const session = Object.create(McpAgentSessionDOBase.prototype) as InitSession; + session.ctx = storage; + session.captureCause = (cause) => { + captured.push(cause); + }; + session.dbHandle = null; + session.engine = null; + session.getSessionId = () => "session-init"; + session.initialized = false; + session.lastActivityMs = 0; + session.pendingApprovalLeases = new Map(); + session.props = { session: { organizationId: "org-1", userId: "user-1" } }; + session.sessionTimeoutMs = () => 60_000; + session.resolveSessionMeta = () => Effect.succeed(sessionMeta); + session.openSessionDb = () => ({ end: () => undefined }); + session.buildMcpServer = () => + Effect.succeed({ mcpServer: makeServer(), engine: makeEngine().engine }); + return { session, storage, captured }; + }; + + it("keeps the session when a deploy resets the last-activity write", async () => { + const { session, storage, captured } = makeInitSession(); + storage.putFailures.set("last-activity-ms", () => new Error(CODE_UPDATE_RESET)); + + await expect( + session.init(), + "a healthy session is not torn down by a lost timestamp", + ).resolves.toBeUndefined(); + + expect(session.initialized, "the runtime stays installed").toBe(true); + expect(session.engine, "the execution engine survives").not.toBeNull(); + expect(session.server, "the MCP server survives").toBeDefined(); + expect(captured, "a platform reset of bookkeeping is not paged as a defect").toEqual([]); + }); + + it("keeps the session when a deploy resets the idle-alarm write", async () => { + const { session, storage, captured } = makeInitSession(); + storage.setAlarmFailure = () => new Error(CODE_UPDATE_RESET); + + await expect(session.init()).resolves.toBeUndefined(); + + expect(session.initialized).toBe(true); + expect(captured).toEqual([]); + }); + + // The alarm is the only durable consequence of a dropped markActivity, and it + // must self-heal: the next request re-arms it. Otherwise "best effort" would + // quietly mean "this session never times out". + it("re-arms the idle alarm on the next touch after a lost bookkeeping write", async () => { + const { session, storage } = makeInitSession(); + storage.setAlarmFailure = () => new Error(CODE_UPDATE_RESET); + + await session.init(); + expect(storage.alarm, "the write that failed left no alarm").toBeUndefined(); + + await expect( + session.validateMcpSessionOwner({ accountId: "user-1", organizationId: "org-1" }), + ).resolves.toBe("ok"); + expect(storage.alarm, "the next request re-establishes the idle clock").toBeGreaterThan(0); + }); + + // Best-effort is scoped to the platform's own resets. A bookkeeping write that + // fails for any other reason is still a defect and must still be reported — + // otherwise this change trades a noisy bug for a silent one. + it("still fails and reports when the bookkeeping write breaks for an unknown reason", async () => { + const { session, storage, captured } = makeInitSession(); + storage.putFailures.set("last-activity-ms", () => new Error("quota exceeded for namespace")); + + await expect(session.init()).rejects.toThrow(/quota exceeded/); + expect(captured.length, "an unrecognized failure is still captured").toBe(1); + }); + + // Session meta is not bookkeeping — ownership validation reads it back — so a + // reset there must still fail init. What it must NOT do is escape as an + // unclassified defect: the caller renders it as a retryable error, and the DO + // stops paging for a condition every deploy guarantees. + it("fails a meta write reset without paging, so the caller can render a retry", async () => { + const { session, storage, captured } = makeInitSession(); + storage.putFailures.set("session-meta", () => new Error(CODE_UPDATE_RESET)); + + await expect(session.init()).rejects.toThrow(/code was updated/); + expect(captured, "a deploy reset is expected platform behaviour, not a defect").toEqual([]); + }); +}); diff --git a/packages/hosts/cloudflare/src/mcp/agent-session-durable-object.ts b/packages/hosts/cloudflare/src/mcp/agent-session-durable-object.ts index 9ff71cfed..a1dd27a3f 100644 --- a/packages/hosts/cloudflare/src/mcp/agent-session-durable-object.ts +++ b/packages/hosts/cloudflare/src/mcp/agent-session-durable-object.ts @@ -21,6 +21,7 @@ import { import { defaultMcpResource, type McpResource } from "@executor-js/host-mcp"; import type { IncomingPropagationHeaders, McpElicitationMode } from "./do-headers"; +import { classifyDurableObjectError, type DurableObjectFailure } from "./durable-object-errors"; import type { McpExecutionOwnerDirectory, McpExecutionOwnerRecord, @@ -273,6 +274,23 @@ export abstract class McpAgentSessionDOBase< return Effect.void; } + /** + * Declare that the Durable Object has finished deciding what to do about this + * cause — either it reported it through `captureCauseEffect`, or it + * recognized it as expected platform behaviour and deliberately did not. + * + * Either way the cause is *owned here*. The host's outer error + * instrumentation wraps the DO's entry points and would otherwise report the + * same rejection a second time as it escapes, producing two issues per + * failure; this is the hook that lets the host drop its own echo. Anything the + * DO never claims (an alarm crash, a transport fault) is untouched and keeps + * being reported by that instrumentation, which is the whole point of + * claiming explicitly instead of disabling it. + */ + protected claimCauseHandled(_cause: Cause.Cause): Effect.Effect { + return Effect.void; + } + protected flushTelemetry(): Promise { return Promise.resolve(); } @@ -524,6 +542,61 @@ export abstract class McpAgentSessionDOBase< }).pipe(Effect.withSpan("mcp.session.resolve_and_store_meta")); } + /** + * A Cloudflare platform reset happened. Record what KIND it was, on the span + * and in a structured log, so the volume stays countable per cause (deploy + * reset vs storage timeout vs backend blip) instead of collapsing into one + * opaque bucket the moment it stops being an error report. + */ + private recordDurableObjectReset(input: { + readonly operation: string; + readonly failure: DurableObjectFailure; + readonly cause: Cause.Cause; + }): Effect.Effect { + const self = this; + return Effect.gen(function* () { + console.warn( + JSON.stringify({ + event: "mcp_session_durable_object_reset", + operation: input.operation, + sessionId: self.sessionId, + resetKind: input.failure.kind, + disposition: input.failure.disposition, + cause: Cause.pretty(input.cause), + }), + ); + yield* Effect.annotateCurrentSpan({ + "mcp.do.reset_kind": input.failure.kind, + "mcp.do.reset_disposition": input.failure.disposition, + "mcp.do.reset_operation": input.operation, + }); + }); + } + + /** + * Run a storage write whose only job is bookkeeping, and let the Cloudflare + * platform take it away without taking the session with it. + * + * A platform reset (a deploy, a storage timeout, a backend blip) cancels + * whatever write is in flight. For a write nothing depends on, the right + * answer is to note it and carry on — the alternative, which is what used to + * happen, is that a fully built and perfectly healthy session is torn down and + * the user's request fails because a timestamp did not land. + * + * Scoped deliberately: only failures the classifier RECOGNIZES as platform + * resets are absorbed. Anything else is still a defect and still fails. + */ + private bestEffortBookkeeping(operation: string, run: () => Promise): Effect.Effect { + const self = this; + return Effect.promise(run).pipe( + Effect.catchCause((cause) => { + const failure = classifyDurableObjectError(cause); + if (!failure) return Effect.failCause(cause); + return self.recordDurableObjectReset({ operation, failure, cause }); + }), + ); + } + private recordCauseOnSpan(cause: Cause.Cause): Effect.Effect { const errors = Cause.prettyErrors(cause); if (errors.length === 0) return Effect.void; @@ -722,15 +795,31 @@ export abstract class McpAgentSessionDOBase< self.server = mcpServer; self.engine = engine; self.initialized = true; - yield* Effect.promise(() => self.markActivity()).pipe( - Effect.withSpan("McpSessionDO.markActivity"), - ); + // Last statement, and pure bookkeeping: the runtime above is already + // installed and serving. Losing the timestamp/alarm write to a platform + // reset must not undo any of it — the in-memory clock is already set and + // the next request re-arms the alarm. + yield* self + .bestEffortBookkeeping("init.mark_activity", () => self.markActivity()) + .pipe(Effect.withSpan("McpSessionDO.markActivity")); }).pipe( Effect.tapCause((cause) => Effect.gen(function* () { - console.error("[mcp-session] init failed:", Cause.pretty(cause)); - yield* self.captureCauseEffect(cause); + // A Cloudflare platform reset of an in-flight init is not a defect — + // every deploy causes one, by design. Record the kind so the volume + // stays measurable, let the caller render it as a retry, and do not + // page for it. Everything else is reported exactly as before. + const failure = classifyDurableObjectError(cause); + if (failure) { + yield* self.recordDurableObjectReset({ operation: "init", failure, cause }); + } else { + console.error("[mcp-session] init failed:", Cause.pretty(cause)); + yield* self.captureCauseEffect(cause); + } yield* self.recordCauseOnSpan(cause); + // Claimed AFTER any capture above, so the DO's own event is not + // mistaken for the host instrumentation's duplicate of it. + yield* self.claimCauseHandled(cause); }), ), Effect.catchCause((cause) => @@ -777,9 +866,9 @@ export abstract class McpAgentSessionDOBase< const sessionMeta = yield* self.loadSessionMeta(); if (!sessionMeta) return "not_found" as const; if (self.initialized) { - yield* Effect.promise(() => self.markActivity()).pipe( - Effect.withSpan("McpSessionDO.markActivity"), - ); + yield* self + .bestEffortBookkeeping("validate_owner.mark_activity", () => self.markActivity()) + .pipe(Effect.withSpan("McpSessionDO.markActivity")); } else { yield* Effect.promise(() => self.onStart()).pipe( Effect.withSpan("McpSessionDO.restore_transport_runtime"), @@ -1178,9 +1267,9 @@ export abstract class McpAgentSessionDOBase< // which used to hold a ref across the pause and mask this by keeping // the ref transition away from 0->1.) const disposeKeepAlive = yield* Effect.promise(() => self.keepAlive()); - yield* Effect.promise(() => self.markActivity()).pipe( - Effect.withSpan("McpSessionDO.markActivity"), - ); + yield* self + .bestEffortBookkeeping("approval_lease.mark_activity", () => self.markActivity()) + .pipe(Effect.withSpan("McpSessionDO.markActivity")); const timeout = setTimeout(() => { self.queuePendingApprovalLeaseExpiration(executionId); }, PAUSED_APPROVAL_TIMEOUT_MS); diff --git a/packages/hosts/cloudflare/src/mcp/durable-object-errors.test.ts b/packages/hosts/cloudflare/src/mcp/durable-object-errors.test.ts new file mode 100644 index 000000000..3ae9fe3a3 --- /dev/null +++ b/packages/hosts/cloudflare/src/mcp/durable-object-errors.test.ts @@ -0,0 +1,185 @@ +// oxlint-disable executor/no-error-constructor, executor/no-try-catch-or-throw -- boundary: every case here is a verbatim reproduction of a plain Error the Cloudflare runtime itself throws, and the test helper throws to abort a case whose premise did not hold +import { describe, expect, it } from "@effect/vitest"; +import { Cause } from "effect"; + +import { UNAVAILABLE_RETRY_AFTER_SECONDS } from "@executor-js/host-mcp"; + +import { classifyDurableObjectError, durableObjectFailureResponse } from "./durable-object-errors"; + +// Every string here is a real Cloudflare runtime message. They arrive as plain +// `Error`s with no code, no type and no marker of any kind — the text IS the +// contract, which is exactly why it deserves one place and a test. +describe("classifyDurableObjectError", () => { + it("reads a self-abort as a session that is gone for good", () => { + // `destroy()` ends in `ctx.abort("destroyed")`; the abort reason arrives + // verbatim as the error message. + expect(classifyDurableObjectError(new Error("destroyed"))).toEqual({ + kind: "destroyed", + disposition: "session_dead", + }); + }); + + it("reads a deploy-time reset as transient", () => { + expect( + classifyDurableObjectError(new Error("Durable Object reset because its code was updated.")), + ).toEqual({ kind: "code_update", disposition: "transient" }); + }); + + it("reads a storage timeout reset as transient", () => { + expect( + classifyDurableObjectError( + new Error( + "Durable Object storage operation exceeded timeout which caused object to be reset.", + ), + ), + ).toEqual({ kind: "storage_timeout", disposition: "transient" }); + }); + + it("reads a storage backend fault as transient", () => { + expect( + classifyDurableObjectError( + new Error("Internal error in Durable Object storage caused object to be reset."), + ), + ).toEqual({ kind: "storage_internal", disposition: "transient" }); + }); + + it("reads a blockConcurrencyWhile cancellation as transient", () => { + expect( + classifyDurableObjectError( + new Error( + "A call to blockConcurrencyWhile() in a Durable Object waited for too long. The call was canceled and the Durable Object was reset.", + ), + ), + ).toEqual({ kind: "concurrency_reset", disposition: "transient" }); + }); + + it("reads a platform blip as transient, ignoring the reference id", () => { + // The reference id differs on every event; it must not defeat the match. + expect( + classifyDurableObjectError(new Error("internal error; reference = 0000aaaa1111bbbb")), + ).toEqual({ kind: "internal_error", disposition: "transient" }); + expect( + classifyDurableObjectError(new Error("internal error; reference = ffff9999eeee8888")), + ).toEqual({ kind: "internal_error", disposition: "transient" }); + }); + + it("honours the runtime's own retryable flag when the message says nothing", () => { + const error = Object.assign(new Error("Network connection lost."), { retryable: true }); + expect(classifyDurableObjectError(error)).toEqual({ + kind: "retryable", + disposition: "transient", + }); + }); + + it("looks inside an Effect cause, because that is how the DO seam sees it", () => { + const cause = Cause.die(new Error("Durable Object reset because its code was updated.")); + + expect(classifyDurableObjectError(cause)).toEqual({ + kind: "code_update", + disposition: "transient", + }); + }); + + it("unwraps a wrapped platform error", () => { + const wrapped = new Error("session restore failed", { + cause: new Error("Durable Object reset because its code was updated."), + }); + expect(classifyDurableObjectError(wrapped)).toEqual({ + kind: "code_update", + disposition: "transient", + }); + }); + + // The whole point of returning null is that unknown failures keep their + // current behaviour: rethrown, reported, paged. Silently swallowing an + // application bug as "transient" would be far worse than the noise this + // module removes. + it("refuses to classify anything it does not recognize", () => { + expect(classifyDurableObjectError(new Error("Cannot read properties of undefined"))).toBeNull(); + expect(classifyDurableObjectError(new TypeError("x is not a function"))).toBeNull(); + expect(classifyDurableObjectError(undefined)).toBeNull(); + expect(classifyDurableObjectError(null)).toBeNull(); + expect(classifyDurableObjectError({})).toBeNull(); + }); + + // "destroyed" is the abort reason and nothing else. A message that merely + // mentions the word describes a different failure and must not be allowed to + // condemn a live session id. + it("does not condemn a session for a message that merely mentions destruction", () => { + expect( + classifyDurableObjectError(new Error("the widget was destroyed by the user")), + ).toBeNull(); + }); +}); + +// What a client actually receives when the platform takes a session Durable +// Object away mid-request. The cloud e2e suite proves the reachable half of +// this black-box — a terminated session id always answers with a protocol +// envelope, never a bare 500 — but the dev stack never produces the platform +// errors themselves (an isolate abort, a deploy reset, a storage timeout), so +// the mapping from those errors to the wire response is pinned here. +describe("durableObjectFailureResponse", () => { + const envelope = async ( + error: unknown, + ): Promise<{ + readonly status: number; + readonly retryAfter: string | null; + readonly body: { + readonly jsonrpc?: string; + readonly error?: { + readonly code?: number; + readonly message?: string; + readonly data?: unknown; + }; + }; + }> => { + const failure = classifyDurableObjectError(error); + if (!failure) throw new Error("expected the error to be classified as a platform failure"); + const response = durableObjectFailureResponse(failure); + return { + status: response.status, + retryAfter: response.headers.get("retry-after"), + body: await response.json(), + }; + }; + + // `destroy()` ends in `ctx.abort("destroyed")`. That rejection used to escape + // the unguarded ownership RPC in the cloud MCP handler, so a client got a + // bare 500 for a session it had itself just terminated. + it("tells the client to reconnect when the session object is gone for good", async () => { + const result = await envelope(new Error("destroyed")); + + expect(result.status, "the id is dead, not retryable").toBe(404); + expect(result.body.jsonrpc).toBe("2.0"); + expect(result.body.error?.code).toBe(-32001); + expect(result.body.error?.message).toBe("Session timed out, please reconnect"); + }); + + // A deploy resets every live Durable Object. The session id is still valid, + // so the client must be told to retry it — and told how long to wait, or the + // 503 is not actionable. + it("tells the client to retry the same session after a transient platform reset", async () => { + const result = await envelope(new Error("Durable Object reset because its code was updated.")); + + expect(result.status, "HTTP status is the discriminator clients act on").toBe(503); + expect(result.body.jsonrpc).toBe("2.0"); + expect(result.body.error?.code).toBe(-32001); + // `retryAfterSeconds` renders as the standard `Retry-After` header, which + // is what a polite client (and any generic retry layer) actually reads. + expect(result.retryAfter, "the client is told how long to back off").toBe( + String(UNAVAILABLE_RETRY_AFTER_SECONDS), + ); + }); + + it("renders the same retry verdict for a storage timeout and a backend blip", async () => { + for (const message of [ + "Durable Object storage operation exceeded timeout which caused object to be reset.", + "internal error; reference = 0000aaaa1111bbbb", + "A call to blockConcurrencyWhile() in a Durable Object waited for too long. The call was canceled and the Durable Object was reset.", + ]) { + const result = await envelope(new Error(message)); + expect(result.status, message).toBe(503); + expect(result.retryAfter, message).not.toBeNull(); + } + }); +}); diff --git a/packages/hosts/cloudflare/src/mcp/durable-object-errors.ts b/packages/hosts/cloudflare/src/mcp/durable-object-errors.ts new file mode 100644 index 000000000..cbf62f059 --- /dev/null +++ b/packages/hosts/cloudflare/src/mcp/durable-object-errors.ts @@ -0,0 +1,181 @@ +/** + * Classification of Cloudflare *platform* Durable Object failures. + * + * A Durable Object can be torn out from under its own code by the runtime, and + * when that happens the only thing the caller receives is a plain `Error` whose + * message is the entire signal. Those are not application defects: the object + * was healthy, the request was valid, and the correct answer to the client is + * "this id is dead, reconnect" or "come back in a moment" — never an unhandled + * 500 and never an error report. + * + * The codebase already classifies transient-vs-definitive failures carefully + * for the auth provider; this is the same discipline for the one other + * dependency that fails without a typed error: the DO platform itself. + * + * Everything not listed here stays UNCLASSIFIED on purpose. An unrecognized + * failure keeps its current behaviour — rethrow, report, page — because the + * cost of silently swallowing a real defect is much higher than the cost of one + * more retryable envelope. + */ +import { Cause } from "effect"; + +import { jsonRpcErrorBody, UNAVAILABLE_RETRY_AFTER_SECONDS } from "@executor-js/host-mcp"; + +/** + * The specific platform condition, kept granular so it can be recorded on a + * span (`mcp.do.reset_kind`) and the aggregate bucket split by cause in + * production instead of being one opaque pile. + */ +export type DurableObjectFailureKind = + /** The object called `ctx.abort("destroyed")` on itself — session teardown. */ + | "destroyed" + /** A deploy replaced the script and the runtime reset every live object. */ + | "code_update" + /** A storage op ran past the platform's ceiling; the object was reset. */ + | "storage_timeout" + /** The storage backend failed internally and reset the object. */ + | "storage_internal" + /** `blockConcurrencyWhile()` ran past its cap and was cancelled. */ + | "concurrency_reset" + /** A generic platform blip: `internal error; reference = `. */ + | "internal_error" + /** The runtime itself flagged the error as retryable. */ + | "retryable"; + +/** + * What the caller should do about it. + * + * - `session_dead` — the object is gone for good; the id will never work again, + * so the client must mint a new session. + * - `transient` — the object was reset but the id is still valid; the very next + * attempt is likely to succeed. + */ +export type DurableObjectFailureDisposition = "session_dead" | "transient"; + +export type DurableObjectFailure = { + readonly kind: DurableObjectFailureKind; + readonly disposition: DurableObjectFailureDisposition; +}; + +/** + * Message fragments workerd puts on the plain `Error` it throws. Matched + * case-insensitively on a substring because the runtime appends ids and + * punctuation ("… reference = 0123abcd") and has reworded these before. + */ +const MESSAGE_PATTERNS: ReadonlyArray<{ + readonly fragment: string; + readonly failure: DurableObjectFailure; +}> = [ + { + fragment: "durable object reset because its code was updated", + failure: { kind: "code_update", disposition: "transient" }, + }, + { + fragment: "storage operation exceeded timeout", + failure: { kind: "storage_timeout", disposition: "transient" }, + }, + { + fragment: "internal error in durable object storage", + failure: { kind: "storage_internal", disposition: "transient" }, + }, + { + fragment: "blockconcurrencywhile", + failure: { kind: "concurrency_reset", disposition: "transient" }, + }, + { + fragment: "internal error; reference =", + failure: { kind: "internal_error", disposition: "transient" }, + }, +]; + +/** + * `ctx.abort("destroyed")` surfaces as an `Error` whose message is exactly the + * abort reason, so this one is matched whole rather than as a substring — a + * message that merely mentions the word must not condemn a live session. + */ +const DESTROYED_MESSAGE = "destroyed"; + +/** How deep to follow `error.cause` before giving up. */ +const MAX_UNWRAP_DEPTH = 5; + +const messageOf = (error: unknown): string | null => { + if (typeof error === "string") return error; + if (typeof error !== "object" || error === null) return null; + // oxlint-disable-next-line executor/no-unknown-error-message -- platform boundary: workerd rejects with an untyped Error whose message IS the only signal; reading it here is the entire purpose of this module, and it exists so no other file has to + const message = (error as { readonly message?: unknown }).message; + return typeof message === "string" ? message : null; +}; + +const isRuntimeRetryable = (error: unknown): boolean => + typeof error === "object" && + error !== null && + (error as { readonly retryable?: unknown }).retryable === true; + +const classifyOne = (error: unknown): DurableObjectFailure | null => { + const message = messageOf(error); + if (message !== null) { + const normalized = message.trim().toLowerCase(); + if (normalized === DESTROYED_MESSAGE) { + return { kind: "destroyed", disposition: "session_dead" }; + } + for (const pattern of MESSAGE_PATTERNS) { + if (normalized.includes(pattern.fragment)) return pattern.failure; + } + } + // Checked last: the message is the more specific signal, and the runtime sets + // `retryable` on some of the same errors. + if (isRuntimeRetryable(error)) return { kind: "retryable", disposition: "transient" }; + return null; +}; + +/** + * Recognize a Cloudflare platform Durable Object failure. + * + * Accepts whatever the caller happens to be holding: the raw `Error` a stub RPC + * rejected with, an `Error` that wrapped it as its `cause`, or an Effect + * `Cause` — the DO seam only ever sees the last of those, and forcing every + * call site to unwrap first is how a classifier ends up with three subtly + * different copies. + * + * Returns `null` for anything unrecognized — the caller must then treat the + * error exactly as it did before this module existed. + */ +export const classifyDurableObjectError = (error: unknown): DurableObjectFailure | null => { + if (Cause.isCause(error)) { + for (const inner of Cause.prettyErrors(error)) { + const failure = classifyDurableObjectError(inner); + if (failure) return failure; + } + return null; + } + let current: unknown = error; + for (let depth = 0; depth < MAX_UNWRAP_DEPTH && current !== null && current !== undefined; ) { + const failure = classifyOne(current); + if (failure) return failure; + depth += 1; + if (typeof current !== "object") break; + current = (current as { readonly cause?: unknown }).cause; + } + return null; +}; + +/** + * Render a classified platform failure as the MCP protocol error the client + * should act on. + * + * Both branches reuse envelopes the MCP host already speaks, because the client + * side of this is not new: a dead session id has always been "reconnect", and a + * transient dependency failure has always been a 503 carrying `Retry-After`. + * The only thing that was missing is that a platform reset never reached + * either, and fell out of the worker as an unhandled 500 instead. + * + * The JSON-RPC code is -32001 for both; the HTTP STATUS is the discriminator + * clients act on — 404 = the id is dead, mint a new session; 503 = retry the + * SAME id after the advertised delay. + */ +export const durableObjectFailureResponse = (failure: DurableObjectFailure): Response => + failure.disposition === "session_dead" + ? jsonRpcErrorBody(404, -32001, "Session timed out, please reconnect") + : jsonRpcErrorBody(503, -32001, "MCP session is restarting, please retry", { + retryAfterSeconds: UNAVAILABLE_RETRY_AFTER_SECONDS, + }); From 6a13cf375650630cdd148936823ba2feb60969f6 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:28:46 -0700 Subject: [PATCH 2/3] Reproduce the session teardown race black-box in the cloud e2e suite Terminate a session while requests are already in flight instead of after they drain, so the isolate abort at the end of the teardown lands on a request the handler is holding. That reaches the unhandled 500 without the fix and passes with it. Also narrow the blockConcurrencyWhile match to the runtime's own cancellation message, so a defect thrown from inside the callback is not read as a platform reset. --- .../mcp-destroyed-session-envelope.test.ts | 185 +++++++++++------- .../src/mcp/durable-object-errors.test.ts | 37 ++-- .../src/mcp/durable-object-errors.ts | 6 +- 3 files changed, 137 insertions(+), 91 deletions(-) diff --git a/e2e/cloud/mcp-destroyed-session-envelope.test.ts b/e2e/cloud/mcp-destroyed-session-envelope.test.ts index 07032f8be..817c404f0 100644 --- a/e2e/cloud/mcp-destroyed-session-envelope.test.ts +++ b/e2e/cloud/mcp-destroyed-session-envelope.test.ts @@ -1,15 +1,19 @@ -// Cloud: a terminated MCP session id must keep answering in the protocol's own -// vocabulary for its whole death sequence — not just in the first instant. +// Cloud: a session id must keep answering in the MCP protocol's own vocabulary +// for the whole time its Durable Object is being torn down — not just in the +// first instant. // -// `DELETE /mcp` condemns the session DO with a durable marker and defers the -// real teardown to an alarm ~1s later; that alarm's `destroy()` wipes storage -// and then `ctx.abort("destroyed")`s the isolate. e2e/cloud/mcp-protocol.test.ts -// already covers the FIRST millisecond of that window (the marker is read and a -// 404 reconnect comes back). This scenario covers the rest of it: a client that -// keeps talking to the dead id across the alarm and the abort — exactly what a -// retrying MCP client does — must always get a well-formed JSON-RPC envelope, -// either 404 (the id is dead, reconnect) or a retryable 503, and never a bare -// unhandled 500 from a Durable Object platform error escaping the handler. +// `DELETE /mcp` condemns the session object with a durable marker and defers +// the real teardown to an immediate alarm; that alarm wipes the object's +// storage and then aborts the isolate. e2e/cloud/mcp-protocol.test.ts covers +// the calm case (terminate, then ask again, get a 404 reconnect). This scenario +// covers the violent middle of it: a client whose requests are ALREADY IN +// FLIGHT when the termination lands, which is what a real client with an open +// tool loop looks like when a session ends underneath it. +// +// Every one of those in-flight requests must come back as a well-formed +// JSON-RPC error — 404 (this id is dead, reconnect) or 503 (the object is +// restarting, retry the same id, here is how long to wait) — and never as a +// bare unhandled 500 from a platform failure escaping the request handler. import { expect } from "@effect/vitest"; import { Effect } from "effect"; @@ -97,105 +101,140 @@ const jsonRpcError = (body: string): { readonly code: number; readonly message: } }; +const describeProbe = (probe: Probe): string => + `+${probe.atMs}ms ${probe.status} ${probe.body.slice(0, 200)}`; + scenario( - "MCP protocol · a terminated session id stays a protocol error while its Durable Object is torn down", + "MCP protocol · in-flight requests survive their session's teardown as protocol errors", { timeout: 180_000 }, Effect.gen(function* () { const target = yield* Target; const mcp = yield* Mcp; - const identity = yield* target.newIdentity(); - const bearer = yield* mcp.mintBearer(emailOf(identity)); - - // Several sessions, because the fatal window is the isolate abort that - // follows the deferred destroy alarm and one session only crosses it once. - const sessions = 3; - // The destroy alarm is deferred ~1s; keep probing well past it so the - // probes straddle the alarm, the storage wipe and the abort that follows. - const probeWindowMs = 2_500; - const probeIntervalMs = 25; - const probeFanOut = 4; + + // The fatal instant is the isolate abort at the end of the teardown, and a + // session only crosses it once — so cross it several times. Before the fix + // roughly one teardown in four leaked an unhandled 500. + const sessions = 6; + // Requests are kept continuously in flight rather than polled on a timer: + // the point is to have work ALREADY inside the session object when the + // termination lands, not to sample the window from outside it. + const concurrency = 8; + // Covers the deferred destroy alarm, the storage wipe and the abort that + // follows, and then stops. Kept tight on purpose: the traffic only has to + // straddle the teardown, and a longer stream just piles avoidable load onto + // the shared auth path for no extra coverage. + const streamAfterDeleteMs = 1_600; + // Enough in-flight requests to be mid-teardown, without racing the DELETE + // itself before the session is fully established. + const warmupMs = 250; const probes: Probe[] = []; - for (let attempt = 0; attempt < sessions; attempt += 1) { + // One identity per teardown, all minted BEFORE any load starts. Two + // reasons, both about keeping this scenario's traffic off the shared auth + // path: a single identity carrying every teardown's requests degrades the + // org-membership lookup into a 403, and signing new users in while the + // probe stream is running fails the sign-in itself. Neither has anything to + // do with what is being tested here. + const bearers: string[] = []; + for (let index = 0; index < sessions; index += 1) { + const identity = yield* target.newIdentity(); + bearers.push(yield* mcp.mintBearer(emailOf(identity))); + } + + for (const bearer of bearers) { const sessionId = yield* Effect.promise(() => openSession(target.mcpUrl, bearer)); - const terminate = yield* Effect.promise(() => - fetch(target.mcpUrl, { + + yield* Effect.promise(async () => { + const startedAt = Date.now(); + let stopAt = Number.POSITIVE_INFINITY; + + const probeOnce = async (): Promise => { + const at = Date.now() - startedAt; + const response = await mcpPost(target.mcpUrl, { + bearer, + sessionId, + body: TOOLS_LIST_REQUEST, + }); + probes.push({ + atMs: at, + status: response.status, + body: await response.text(), + retryAfter: response.headers.get("retry-after"), + }); + }; + + // One worker replenishes its request the moment the previous one + // settles, so the session object is never idle and the DELETE has to + // land on top of real traffic. + const worker = async (): Promise => { + while (Date.now() < stopAt) await probeOnce(); + }; + const workers = Array.from({ length: concurrency }, () => worker()); + + await new Promise((resolve) => setTimeout(resolve, warmupMs)); + // Terminate WITHOUT draining the stream: this is the whole scenario. + const terminate = await fetch(target.mcpUrl, { method: "DELETE", headers: { authorization: `Bearer ${bearer}`, "mcp-session-id": sessionId }, - }), - ); - expect(terminate.status, "the client can terminate its session").toBe(200); - yield* Effect.promise(() => terminate.text()); - - const startedAt = Date.now(); - const probe = async (): Promise => { - const at = Date.now() - startedAt; - const response = await mcpPost(target.mcpUrl, { - bearer, - sessionId, - body: TOOLS_LIST_REQUEST, - }); - probes.push({ - atMs: at, - status: response.status, - body: await response.text(), - retryAfter: response.headers.get("retry-after"), }); - }; - // Concurrent, not sequential: the fatal moment is `ctx.abort("destroyed")` - // itself, which kills whatever is in flight. A one-at-a-time poll almost - // always misses it, so keep a fan of requests open across the whole - // teardown. - yield* Effect.promise(async () => { - const inFlight: Promise[] = []; - while (Date.now() - startedAt < probeWindowMs) { - for (let i = 0; i < probeFanOut; i += 1) inFlight.push(probe()); - await new Promise((resolve) => setTimeout(resolve, probeIntervalMs)); - } - await Promise.all(inFlight); + await terminate.text(); + expect(terminate.status, "the client can terminate its session").toBe(200); + + stopAt = Date.now() + streamAfterDeleteMs; + await Promise.all(workers); }); } - // What the dead id actually answered, so a reviewer can see the shape of - // the teardown window and not just the verdict. + // What the dying session actually answered, so a reviewer can see the shape + // of the teardown window and not just the verdict. const bucket = new Map(); for (const probe of probes) { const key = `${probe.status} ${jsonRpcError(probe.body)?.message ?? probe.body.slice(0, 80)}`; bucket.set(key, (bucket.get(key) ?? 0) + 1); } console.info( - `[destroyed-session-envelope] ${probes.length} probes: ${[...bucket] + `[destroyed-session-envelope] ${probes.length} probes across ${sessions} teardowns: ${[ + ...bucket, + ] .map(([key, count]) => `${count}× ${key}`) .join(" | ")}`, ); + expect(probes.length, "the stream actually exercised the teardown").toBeGreaterThan(sessions); + const unhandled = probes.filter((probe) => probe.status >= 500 && probe.status !== 503); expect( - unhandled.map((probe) => `+${probe.atMs}ms ${probe.status} ${probe.body.slice(0, 200)}`), - "no probe on a dead session id produces an unhandled server error", + unhandled.map(describeProbe), + "no request on a terminating session produces an unhandled server error", ).toEqual([]); - const malformed = probes.filter((probe) => jsonRpcError(probe.body) === null); + // Only rejections are asserted on: a request the session still served + // answers 200 over SSE, which is not a JSON-RPC error body and not what + // this scenario is about. + const malformed = probes.filter( + (probe) => probe.status !== 200 && jsonRpcError(probe.body) === null, + ); expect( - malformed.map((probe) => `+${probe.atMs}ms ${probe.status} ${probe.body.slice(0, 200)}`), - "every rejection is a JSON-RPC error envelope", + malformed.map(describeProbe), + "every rejection is a JSON-RPC error envelope the client can parse", ).toEqual([]); - for (const probe of probes) { - expect([404, 503], `+${probe.atMs}ms is a reconnect or a retry verdict`).toContain( - probe.status, - ); - } + // Proves the traffic actually straddled the teardown rather than finishing + // before it: the dead id has to have been reported dead at least once. + const reconnectVerdicts = probes.filter((probe) => probe.status === 404); + expect( + reconnectVerdicts.length, + "the stream reached the terminated session and was told to reconnect", + ).toBeGreaterThan(0); - // A 503 in this window means "the platform is mid-reset, come back" — it is - // only actionable if the client is told how long to wait, so the retryable - // envelope must carry Retry-After. + // A 503 here means "the platform is mid-reset, come back" — only actionable + // if the client is told how long to wait, so it must carry Retry-After. const retryableWithoutBackoff = probes.filter( (probe) => probe.status === 503 && probe.retryAfter === null, ); expect( - retryableWithoutBackoff.map((probe) => `+${probe.atMs}ms`), + retryableWithoutBackoff.map(describeProbe), "every retryable rejection tells the client how long to back off", ).toEqual([]); }), diff --git a/packages/hosts/cloudflare/src/mcp/durable-object-errors.test.ts b/packages/hosts/cloudflare/src/mcp/durable-object-errors.test.ts index 3ae9fe3a3..2eca5e77d 100644 --- a/packages/hosts/cloudflare/src/mcp/durable-object-errors.test.ts +++ b/packages/hosts/cloudflare/src/mcp/durable-object-errors.test.ts @@ -102,6 +102,18 @@ describe("classifyDurableObjectError", () => { expect(classifyDurableObjectError({})).toBeNull(); }); + // A callback that throws also resets the object, so an application defect + // raised inside `blockConcurrencyWhile` can carry the method's name. Only the + // runtime's own cancellation message is a platform reset; the rest is a bug + // and must keep being rethrown and reported. + it("does not read an application defect inside blockConcurrencyWhile as a platform reset", () => { + expect( + classifyDurableObjectError( + new Error("Error in blockConcurrencyWhile(): TypeError: x is not a function"), + ), + ).toBeNull(); + }); + // "destroyed" is the abort reason and nothing else. A message that merely // mentions the word describes a different failure and must not be allowed to // condemn a live session id. @@ -113,11 +125,14 @@ describe("classifyDurableObjectError", () => { }); // What a client actually receives when the platform takes a session Durable -// Object away mid-request. The cloud e2e suite proves the reachable half of -// this black-box — a terminated session id always answers with a protocol -// envelope, never a bare 500 — but the dev stack never produces the platform -// errors themselves (an isolate abort, a deploy reset, a storage timeout), so -// the mapping from those errors to the wire response is pinned here. +// Object away mid-request. +// +// The self-abort at the end of a session teardown IS reachable black-box, and +// e2e/cloud/mcp-destroyed-session-envelope.test.ts owns that case end to end — +// it is deliberately not re-tested here. The remaining platform resets (a +// deploy replacing the script, a storage timeout, a backend blip, a cancelled +// blockConcurrencyWhile) cannot be provoked on the dev stack at all, so the +// mapping from those errors to the wire response is pinned here instead. describe("durableObjectFailureResponse", () => { const envelope = async ( error: unknown, @@ -143,18 +158,6 @@ describe("durableObjectFailureResponse", () => { }; }; - // `destroy()` ends in `ctx.abort("destroyed")`. That rejection used to escape - // the unguarded ownership RPC in the cloud MCP handler, so a client got a - // bare 500 for a session it had itself just terminated. - it("tells the client to reconnect when the session object is gone for good", async () => { - const result = await envelope(new Error("destroyed")); - - expect(result.status, "the id is dead, not retryable").toBe(404); - expect(result.body.jsonrpc).toBe("2.0"); - expect(result.body.error?.code).toBe(-32001); - expect(result.body.error?.message).toBe("Session timed out, please reconnect"); - }); - // A deploy resets every live Durable Object. The session id is still valid, // so the client must be told to retry it — and told how long to wait, or the // 503 is not actionable. diff --git a/packages/hosts/cloudflare/src/mcp/durable-object-errors.ts b/packages/hosts/cloudflare/src/mcp/durable-object-errors.ts index cbf62f059..4b77f1520 100644 --- a/packages/hosts/cloudflare/src/mcp/durable-object-errors.ts +++ b/packages/hosts/cloudflare/src/mcp/durable-object-errors.ts @@ -79,7 +79,11 @@ const MESSAGE_PATTERNS: ReadonlyArray<{ failure: { kind: "storage_internal", disposition: "transient" }, }, { - fragment: "blockconcurrencywhile", + // Deliberately the whole phrase, not the bare method name: an application + // defect thrown from inside a `blockConcurrencyWhile` callback also resets + // the object, and a message that merely names the method must not be read + // as a platform reset and quietly turned into a retry. + fragment: "blockconcurrencywhile() in a durable object waited for too long", failure: { kind: "concurrency_reset", disposition: "transient" }, }, { From 040aa82dac3e36b2959cce4c3d358cdc93148e87 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:11:04 -0700 Subject: [PATCH 3/3] Make the destroyed-session scenario reproduce the teardown race every run Stop each teardown stream on the observed post-wipe verdict instead of a fixed timer, which cuts enough wasted load to raise the crossing count and in-flight concurrency. Assert every teardown was straddled, and that a terminated session is never advertised as retryable. --- .../mcp-destroyed-session-envelope.test.ts | 178 +++++++++++++++--- 1 file changed, 151 insertions(+), 27 deletions(-) diff --git a/e2e/cloud/mcp-destroyed-session-envelope.test.ts b/e2e/cloud/mcp-destroyed-session-envelope.test.ts index 817c404f0..c461fc035 100644 --- a/e2e/cloud/mcp-destroyed-session-envelope.test.ts +++ b/e2e/cloud/mcp-destroyed-session-envelope.test.ts @@ -101,6 +101,14 @@ const jsonRpcError = (body: string): { readonly code: number; readonly message: } }; +/** + * The verdict a request gets once the destroy alarm has wiped the session's + * storage — i.e. the teardown this scenario is timing itself against is over. + * Matched on the protocol answer, not on any server internal. + */ +const isWipedVerdict = (probe: Probe): boolean => + probe.status === 404 && jsonRpcError(probe.body)?.message === "Session not found"; + const describeProbe = (probe: Probe): string => `+${probe.atMs}ms ${probe.status} ${probe.body.slice(0, 200)}`; @@ -112,23 +120,36 @@ scenario( const mcp = yield* Mcp; // The fatal instant is the isolate abort at the end of the teardown, and a - // session only crosses it once — so cross it several times. Before the fix - // roughly one teardown in four leaked an unhandled 500. - const sessions = 6; + // session only crosses it once — so cross it many times. A single crossing + // leaks the unhandled 500 only when a `validateMcpSessionOwner` RPC is + // inside the object at exactly that instant, so the scenario has to make + // that likely rather than hope for it: `sessions` crossings, each blanketed + // by `concurrency` requests. At 6 × 8 the pre-fix bug reproduced in only + // about half of runs; the numbers below are the measured point where it + // reproduces every run without pushing the shared auth path into 403s. + const sessions = 12; // Requests are kept continuously in flight rather than polled on a timer: // the point is to have work ALREADY inside the session object when the // termination lands, not to sample the window from outside it. - const concurrency = 8; - // Covers the deferred destroy alarm, the storage wipe and the abort that - // follows, and then stops. Kept tight on purpose: the traffic only has to - // straddle the teardown, and a longer stream just piles avoidable load onto - // the shared auth path for no extra coverage. - const streamAfterDeleteMs = 1_600; + const concurrency = 24; + // Ceiling, not the plan: the stream normally stops as soon as the teardown + // is OBSERVED to have completed (below). This only bounds a teardown whose + // alarm never lands, so a hung platform fails the scenario instead of + // hanging it. + const streamAfterDeleteMs = 3_000; + // Once the storage wipe is visible the abort has already happened, so the + // window this scenario exists to cover is behind us. Keep going briefly — + // the abort and the wipe are not the same instant — then stop, because + // every further request is pure load on the shared auth path. + const graceAfterWipeMs = 150; // Enough in-flight requests to be mid-teardown, without racing the DELETE // itself before the session is fully established. const warmupMs = 250; const probes: Probe[] = []; + // Per teardown, so the scenario can PROVE each crossing was straddled + // rather than assume it. + const perSession: Probe[][] = []; // One identity per teardown, all minted BEFORE any load starts. Two // reasons, both about keeping this scenario's traffic off the shared auth @@ -145,9 +166,16 @@ scenario( for (const bearer of bearers) { const sessionId = yield* Effect.promise(() => openSession(target.mcpUrl, bearer)); + const thisSession: Probe[] = []; + perSession.push(thisSession); + yield* Effect.promise(async () => { const startedAt = Date.now(); - let stopAt = Number.POSITIVE_INFINITY; + let hardStopAt = Number.POSITIVE_INFINITY; + // Set the first time the wiped-storage verdict is seen, which is the + // observable end of the teardown — the stream stops on this, not a + // timer. + let stopAfterWipeAt = Number.POSITIVE_INFINITY; const probeOnce = async (): Promise => { const at = Date.now() - startedAt; @@ -156,19 +184,27 @@ scenario( sessionId, body: TOOLS_LIST_REQUEST, }); - probes.push({ + const probe: Probe = { atMs: at, status: response.status, body: await response.text(), retryAfter: response.headers.get("retry-after"), - }); + }; + probes.push(probe); + thisSession.push(probe); + // "Session not found" is the post-wipe verdict: the destroy alarm has + // run, storage is gone, and the object has been aborted. Anything + // after this point is a request against an already-dead id. + if (isWipedVerdict(probe) && stopAfterWipeAt === Number.POSITIVE_INFINITY) { + stopAfterWipeAt = Date.now() + graceAfterWipeMs; + } }; // One worker replenishes its request the moment the previous one // settles, so the session object is never idle and the DELETE has to // land on top of real traffic. const worker = async (): Promise => { - while (Date.now() < stopAt) await probeOnce(); + while (Date.now() < hardStopAt && Date.now() < stopAfterWipeAt) await probeOnce(); }; const workers = Array.from({ length: concurrency }, () => worker()); @@ -181,7 +217,7 @@ scenario( await terminate.text(); expect(terminate.status, "the client can terminate its session").toBe(200); - stopAt = Date.now() + streamAfterDeleteMs; + hardStopAt = Date.now() + streamAfterDeleteMs; await Promise.all(workers); }); } @@ -220,22 +256,110 @@ scenario( "every rejection is a JSON-RPC error envelope the client can parse", ).toEqual([]); - // Proves the traffic actually straddled the teardown rather than finishing - // before it: the dead id has to have been reported dead at least once. - const reconnectVerdicts = probes.filter((probe) => probe.status === 404); + // Coverage precondition, checked per teardown rather than in aggregate: a + // run in which some session's stream finished before its object was torn + // down never entered the window this scenario exists to cover, and its + // green is worth nothing. Requiring EVERY crossing to have been observed is + // what stops the assertions above from passing vacuously. + const notStraddled = perSession + .map((session, index) => ({ index, wiped: session.some(isWipedVerdict) })) + .filter((entry) => !entry.wiped) + .map((entry) => `teardown #${entry.index + 1}`); expect( - reconnectVerdicts.length, - "the stream reached the terminated session and was told to reconnect", - ).toBeGreaterThan(0); - - // A 503 here means "the platform is mid-reset, come back" — only actionable - // if the client is told how long to wait, so it must carry Retry-After. - const retryableWithoutBackoff = probes.filter( - (probe) => probe.status === 503 && probe.retryAfter === null, - ); + notStraddled, + "every teardown was still under load when its session object was destroyed", + ).toEqual([]); + + // The disposition half of the contract, on the one failure this scenario + // can actually produce. Every platform failure reachable here is the + // session's own `ctx.abort` after a DELETE the client itself sent, and that + // id is dead for good — so the answer has to be "reconnect", never + // "restarting, retry". A retryable verdict for a deliberately terminated + // session is worse than the 500 this PR removed: the client is told to keep + // asking, and the loop never ends. + const deadIdCalledRetryable = probes.filter((probe) => probe.status === 503); expect( - retryableWithoutBackoff.map(describeProbe), + deadIdCalledRetryable.map(describeProbe), + "a session the client terminated is never advertised as retryable", + ).toEqual([]); + + // A 503 here means "the platform is mid-reset, come back". It is only + // actionable if the client is told how long to wait, so it must carry + // Retry-After AND a positive delay — a `retry-after: 0`, or a header the + // renderer dropped, is the same dead end as no answer at all. + const badBackoff = probes.filter((probe) => { + if (probe.status !== 503) return false; + const seconds = probe.retryAfter === null ? null : Number(probe.retryAfter); + return seconds === null || !Number.isFinite(seconds) || seconds <= 0; + }); + expect( + badBackoff.map(describeProbe), "every retryable rejection tells the client how long to back off", ).toEqual([]); + + // The other half of the classifier's contract, and the half a teardown + // cannot demonstrate: a DEFINITIVE refusal must stay definitive. If the + // platform-failure path ever widens far enough to swallow ordinary + // rejections, these are the two answers that would silently turn into "the + // session is restarting, retry" and send a client into a loop it can never + // exit. + const liveIdentity = yield* target.newIdentity(); + const liveBearer = yield* mcp.mintBearer(emailOf(liveIdentity)); + const liveSessionId = yield* Effect.promise(() => openSession(target.mcpUrl, liveBearer)); + + const strangerIdentity = yield* target.newIdentity(); + const strangerBearer = yield* mcp.mintBearer(emailOf(strangerIdentity)); + + yield* Effect.gen(function* () { + const hijack = yield* Effect.promise(() => + mcpPost(target.mcpUrl, { + bearer: strangerBearer, + sessionId: liveSessionId, + body: TOOLS_LIST_REQUEST, + }), + ); + const hijackBody = yield* Effect.promise(() => hijack.text()); + expect( + hijack.status, + "another account's session id is refused outright, not advertised as retryable", + ).toBe(403); + expect(hijack.headers.get("retry-after"), "a refusal carries no backoff advice").toBeNull(); + expect(jsonRpcError(hijackBody)?.code, "the refusal is the session-ownership error").toBe( + -32003, + ); + + const unauthenticated = yield* Effect.promise(() => + fetch(target.mcpUrl, { + method: "POST", + headers: { accept: JSON_AND_SSE, "content-type": "application/json" }, + body: JSON.stringify(TOOLS_LIST_REQUEST), + }), + ); + yield* Effect.promise(() => unauthenticated.text()); + expect( + unauthenticated.status, + "a missing credential is refused outright, not advertised as retryable", + ).toBe(401); + expect( + unauthenticated.headers.get("retry-after"), + "a refusal carries no backoff advice", + ).toBeNull(); + }).pipe( + // Runs even when an assertion above fails, so the extra session never + // outlives the scenario. `tryPromise` so a refused cleanup is a failure + // `ignore` can actually swallow — cleanup must not mask the real verdict. + Effect.ensuring( + Effect.tryPromise(async () => { + const closed = await fetch(target.mcpUrl, { + method: "DELETE", + headers: { + authorization: `Bearer ${liveBearer}`, + "mcp-session-id": liveSessionId, + }, + }); + await closed.text(); + }).pipe(Effect.ignore), + ), + ); }), );