From bf5263f0fc9e52afe5b050a853c41d88b787a5dc Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:13:40 -0700 Subject: [PATCH 1/3] Shape storage error messages and classify connection faults --- .changeset/storage-error-shaping.md | 8 + packages/core/api/src/observability.test.ts | 26 +- packages/core/api/src/observability.ts | 15 +- packages/core/sdk/src/errors.ts | 3 +- packages/core/sdk/src/fuma-runtime.test.ts | 260 ++++++++++++++++++++ packages/core/sdk/src/fuma-runtime.ts | 136 +++++++++- packages/core/sdk/src/index.ts | 7 +- 7 files changed, 440 insertions(+), 15 deletions(-) create mode 100644 .changeset/storage-error-shaping.md create mode 100644 packages/core/sdk/src/fuma-runtime.test.ts diff --git a/.changeset/storage-error-shaping.md b/.changeset/storage-error-shaping.md new file mode 100644 index 000000000..590aaf5eb --- /dev/null +++ b/.changeset/storage-error-shaping.md @@ -0,0 +1,8 @@ +--- +"@executor-js/sdk": patch +"@executor-js/api": patch +--- + +Build `StorageError.message` from the call-site label plus the driver's error code instead of the driver's raw text. The driver text is drizzle's `Failed query: \nparams: `, so error reporting grouped one storage defect by statement shape and printed bound parameters into issue titles. The full driver error stays on `cause`. + +Add `StorageConnectionError`, a `StorageFailure` variant for postgres.js connection faults (`CONNECTION_ENDED`, `CONNECTION_CLOSED`, `CONNECTION_DESTROYED`, `CONNECT_TIMEOUT`, `ECONNRESET`) and workerd's cross-request I/O rejection. It carries the fault `code` and a `retryable` flag so a lost socket can be told apart from a pool-lifetime bug. diff --git a/packages/core/api/src/observability.test.ts b/packages/core/api/src/observability.test.ts index df85881e4..ffd79e866 100644 --- a/packages/core/api/src/observability.test.ts +++ b/packages/core/api/src/observability.test.ts @@ -7,7 +7,7 @@ import { describe, expect, it } from "@effect/vitest"; import { Cause, Effect, Exit, Layer, Ref, Result } from "effect"; -import { StorageError, UniqueViolationError } from "@executor-js/sdk/core"; +import { StorageConnectionError, StorageError, UniqueViolationError } from "@executor-js/sdk/core"; import { capture, ErrorCapture, InternalError } from "./observability"; @@ -47,6 +47,30 @@ describe("capture", () => { }), ); + it.effect("translates StorageConnectionError the same way as StorageError", () => + Effect.gen(function* () { + const { layer, seen } = yield* makeRecorder("trace-conn"); + const err = new StorageConnectionError({ + message: "FumaDB plugin_storage.findFirst failed: CONNECTION_ENDED", + label: "plugin_storage.findFirst", + code: "CONNECTION_ENDED", + retryable: false, + cause: "driver", + }); + + const result = yield* Effect.flip(capture(Effect.fail(err))).pipe(Effect.provide(layer)); + + expect(result).toBeInstanceOf(InternalError); + expect(result.traceId).toBe("trace-conn"); + + const causes = yield* Ref.get(seen); + expect(causes.length).toBe(1); + const squashed = Cause.squash(causes[0]!) as StorageConnectionError; + expect(squashed).toBeInstanceOf(StorageConnectionError); + expect(squashed.code).toBe("CONNECTION_ENDED"); + }), + ); + it.effect("empty traceId when no ErrorCapture is wired", () => Effect.gen(function* () { const err = new StorageError({ message: "nope", cause: undefined }); diff --git a/packages/core/api/src/observability.ts b/packages/core/api/src/observability.ts index 1bda74d5a..918c3bbd2 100644 --- a/packages/core/api/src/observability.ts +++ b/packages/core/api/src/observability.ts @@ -13,8 +13,9 @@ // the cloud Worker, console in the CLI, in-memory in tests) to // record causes and return correlation ids. Optional; absent → // empty trace ids, nothing breaks. -// 3. `capture(eff)` — the one translator. Catches `StorageError` and -// `UniqueViolationError` in the typed channel: the former is +// 3. `capture(eff)` — the one translator. Catches `StorageError`, +// `StorageConnectionError` and `UniqueViolationError` in the typed +// channel: the storage failures are // captured via `ErrorCapture` and re-failed as `InternalError({ // traceId })`; the latter dies as a defect (plugins that want to // surface it as a typed domain error should `Effect.catchTag` @@ -75,6 +76,10 @@ const resolveCapture = Effect.serviceOption(ErrorCapture).pipe( * * - `StorageError` — known backend failure. Capture the cause via * `ErrorCapture`, fail with `InternalError({ traceId })`. + * - `StorageConnectionError` — the database connection failed, so the + * statement never got a verdict. Same edge treatment as + * `StorageError` (capture, opaque 500); the tag exists so callers + * that can retry on a fresh pool are able to tell the two apart. * - `UniqueViolationError` — invariant violation at the HTTP edge: * if a plugin wanted to surface a unique-conflict as a typed * domain error (e.g. "source already exists") it should @@ -97,6 +102,12 @@ export const capture = ( Effect.flatMap((traceId) => Effect.fail(new InternalError({ traceId }))), ), ), + Effect.catchTag("StorageConnectionError", (err) => + resolveCapture.pipe( + Effect.flatMap((c) => c.captureException(Cause.fail(err))), + Effect.flatMap((traceId) => Effect.fail(new InternalError({ traceId }))), + ), + ), ) as Effect.Effect | InternalError, R>; /** diff --git a/packages/core/sdk/src/errors.ts b/packages/core/sdk/src/errors.ts index faf5e44a2..8a9e5e732 100644 --- a/packages/core/sdk/src/errors.ts +++ b/packages/core/sdk/src/errors.ts @@ -32,7 +32,8 @@ export const isUserActionableError = (value: unknown): value is UserActionableEr /* The failure set the SDK surfaces. `execute`'s invoke failures are ported from * v1 but re-keyed by `address` (the full `tools....` * handle) instead of an opaque tool id. Storage failures reuse fuma-runtime's - * `StorageError`/`UniqueViolationError` (`StorageFailure`) — not redefined here. */ + * `StorageError`/`StorageConnectionError`/`UniqueViolationError` + * (`StorageFailure`) — not redefined here. */ // --------------------------------------------------------------------------- // Tool lifecycle diff --git a/packages/core/sdk/src/fuma-runtime.test.ts b/packages/core/sdk/src/fuma-runtime.test.ts new file mode 100644 index 000000000..22e1b76b8 --- /dev/null +++ b/packages/core/sdk/src/fuma-runtime.test.ts @@ -0,0 +1,260 @@ +// Regression for the "StorageError: Failed query: …" reports in production. +// One root cause fanned out into a report per table and WHERE-clause because +// `fumaFailureFromCause` copied the driver's error text verbatim into +// `StorageError.message`: drizzle's `DrizzleQueryError` message is +// `Failed query: \nparams: `, so error reports grouped by +// SQL statement AND printed bound parameters (org ids, user ids, connection +// names) in their TITLES. +// +// Two contracts are pinned here: +// 1. `StorageError.message` is built from a stable label plus the driver's +// error CODE. Never the statement text, never the bound parameters. +// 2. postgres.js connection faults are classified as a distinct +// `StorageConnectionError` rather than melting into a generic +// `StorageError`, so a pool-lifetime bug is not indistinguishable from a +// malformed query. +// +// The fixtures below are synthetic reconstructions of the driver error shapes +// (verified against node_modules/postgres/src/errors.js and +// node_modules/drizzle-orm/errors.js); all identifiers are placeholders. + +import { describe, expect, it } from "@effect/vitest"; +import { Cause, Effect, Exit, Predicate } from "effect"; + +import { + fumaEffect, + fumaFailureFromCause, + isStorageFailure, + StorageError, + UniqueViolationError, +} from "./fuma-runtime"; + +/** Shape of `postgres.js` `Errors.connection(code, options, socket)`. */ +const postgresConnectionError = (code: string): Error => + Object.assign( + // oxlint-disable-next-line executor/no-error-constructor -- boundary: reconstructs the native driver error this module has to classify + new Error(`write ${code} db-placeholder.hyperdrive.local:5432`), + { code, errno: code, address: "db-placeholder.hyperdrive.local", port: 5432 }, + ); + +/** Shape of `postgres.js` `Errors.postgres(x)` — a server-side error report. */ +const postgresServerError = (code: string, message: string): Error => + // oxlint-disable-next-line executor/no-error-constructor -- boundary: reconstructs the native driver error this module has to classify + Object.assign(new Error(message), { code, severity: "ERROR" }); + +/** + * Shape of drizzle's `DrizzleQueryError`: the statement text and the bound + * parameters are baked into `message`, and the driver error hangs off `cause`. + */ +const drizzleQueryError = (sql: string, params: readonly unknown[], cause: unknown): Error => { + // oxlint-disable-next-line executor/no-error-constructor -- boundary: reconstructs the native driver error this module has to classify + const error = new Error(`Failed query: ${sql}\nparams: ${params.join(",")}`); + return Object.assign(error, { query: sql, params, cause }); +}; + +const SQL = 'select "key", "value" from "plugin_storage" where "scope_id" = $1 and "key" = $2'; +// Synthetic placeholders standing in for the real bound values (a WorkOS org id +// and a connection-scoped storage key) that leaked into error-report titles. +const PARAMS = ["org_placeholder_0000", "oauth:integration-placeholder:refresh"] as const; + +const expectNoDriverTextIn = (message: string): void => { + expect(message).not.toContain("Failed query"); + expect(message).not.toContain("params:"); + expect(message).not.toContain("select"); + expect(message).not.toContain('plugin_storage"'); + for (const param of PARAMS) expect(message).not.toContain(param); +}; + +describe("fumaFailureFromCause — message shaping", () => { + it("does not put the statement text or bound parameters in the message", () => { + const failure = fumaFailureFromCause( + "plugin_storage.findFirst", + drizzleQueryError(SQL, PARAMS, postgresConnectionError("CONNECTION_ENDED")), + ); + + expectNoDriverTextIn(failure.message ?? ""); + }); + + it("builds a stable message from the label and the driver error code", () => { + const failure = fumaFailureFromCause( + "plugin_storage.findFirst", + drizzleQueryError( + SQL, + PARAMS, + postgresServerError("42P01", 'relation "nope" does not exist'), + ), + ); + + expect(Predicate.isTagged(failure, "StorageError")).toBe(true); + expect(failure.message).toContain("plugin_storage.findFirst"); + expect(failure.message).toContain("42P01"); + expectNoDriverTextIn(failure.message ?? ""); + }); + + it("groups two different statements failing the same way onto one message", () => { + const a = fumaFailureFromCause( + "integration.findFirst", + drizzleQueryError( + 'select * from "integration" where "slug" = $1', + ["integration-placeholder"], + postgresServerError("57P01", "terminating connection due to administrator command"), + ), + ); + const b = fumaFailureFromCause( + "integration.findFirst", + drizzleQueryError( + 'select * from "integration" where "owner" = $1 and "slug" = $2', + ["org_placeholder_0000", "other-integration-placeholder"], + postgresServerError("57P01", "terminating connection due to administrator command"), + ), + ); + + expect(a.message).toBe(b.message); + }); + + it("still yields a stable message when the driver reports no code", () => { + const failure = fumaFailureFromCause( + "tool.findMany", + drizzleQueryError( + SQL, + PARAMS, + // oxlint-disable-next-line executor/no-error-constructor -- boundary: reconstructs a codeless native driver error + new Error("something went wrong in the driver"), + ), + ); + + expect(Predicate.isTagged(failure, "StorageError")).toBe(true); + expect(failure.message).toContain("tool.findMany"); + expectNoDriverTextIn(failure.message ?? ""); + expect(failure.message).not.toContain("something went wrong"); + }); + + it("keeps the full driver error reachable on `cause`", () => { + const driver = drizzleQueryError(SQL, PARAMS, postgresConnectionError("CONNECTION_ENDED")); + const failure = fumaFailureFromCause("plugin_storage.findFirst", driver); + + expect((failure as { readonly cause?: unknown }).cause).toBe(driver); + }); +}); + +describe("fumaFailureFromCause — classification", () => { + it.each([ + ["CONNECTION_ENDED"], + ["CONNECTION_CLOSED"], + ["CONNECTION_DESTROYED"], + ["CONNECT_TIMEOUT"], + ])("classifies postgres.js %s as a storage connection fault", (code) => { + const failure = fumaFailureFromCause( + "plugin_storage.findFirst", + drizzleQueryError(SQL, PARAMS, postgresConnectionError(code)), + ); + + expect(Predicate.isTagged(failure, "StorageConnectionError")).toBe(true); + expect(failure).toEqual( + expect.objectContaining({ + _tag: "StorageConnectionError", + code, + label: "plugin_storage.findFirst", + }), + ); + expect(typeof (failure as { readonly retryable?: unknown }).retryable).toBe("boolean"); + expectNoDriverTextIn(failure.message ?? ""); + }); + + it("classifies the workerd cross-request I/O rejection as a connection fault", () => { + const failure = fumaFailureFromCause( + "integration.findFirst", + drizzleQueryError( + 'select * from "integration" where "slug" = $1', + ["integration-placeholder"], + // oxlint-disable-next-line executor/no-error-constructor -- boundary: reconstructs workerd's cross-request I/O rejection, which carries no code + new Error( + "Cannot perform I/O on behalf of a different request. I/O objects (such as streams, request/response bodies, and others) created in the context of one request handler cannot be accessed from a different request's handler.", + ), + ), + ); + + expect(Predicate.isTagged(failure, "StorageConnectionError")).toBe(true); + expectNoDriverTextIn(failure.message ?? ""); + expect(failure.message).not.toContain("Cannot perform I/O"); + }); + + it("marks transient socket loss retryable and pool-lifetime faults not retryable", () => { + const transient = fumaFailureFromCause( + "tool.findMany", + postgresConnectionError("CONNECTION_CLOSED"), + ) as { readonly retryable?: boolean }; + const lifetime = fumaFailureFromCause( + "tool.findMany", + postgresConnectionError("CONNECTION_ENDED"), + ) as { readonly retryable?: boolean }; + + expect(transient.retryable).toBe(true); + expect(lifetime.retryable).toBe(false); + }); + + it("still recognises unique violations by SQLSTATE", () => { + const failure = fumaFailureFromCause( + "connection.create", + drizzleQueryError( + 'insert into "connection" ("owner","name") values ($1,$2)', + ["org_placeholder_0000", "connection-placeholder"], + postgresServerError( + "23505", + 'duplicate key value violates unique constraint "connection_pkey"', + ), + ), + ); + + expect(failure).toBeInstanceOf(UniqueViolationError); + expect(Predicate.isTagged(failure, "UniqueViolationError")).toBe(true); + }); + + it("does not misread a unique violation as a connection fault", () => { + const failure = fumaFailureFromCause( + "connection.create", + postgresServerError("23505", "duplicate key value violates unique constraint"), + ); + + expect(Predicate.isTagged(failure, "StorageConnectionError")).toBe(false); + }); + + it("passes an already-typed storage failure through untouched", () => { + const original = new StorageError({ + message: 'FumaDB table "secret" is not available through this storage boundary.', + cause: undefined, + }); + + expect(fumaFailureFromCause("plugin_storage.findFirst", original)).toBe(original); + }); + + it("treats a connection fault as a storage failure at the boundary", () => { + const failure = fumaFailureFromCause( + "plugin_storage.findFirst", + postgresConnectionError("CONNECTION_ENDED"), + ); + + expect(isStorageFailure(failure)).toBe(true); + expect(fumaFailureFromCause("plugin_storage.findFirst", failure)).toBe(failure); + }); +}); + +describe("fumaEffect", () => { + it("maps a rejected driver promise onto the classified failure", async () => { + const driverRejection = () => + // oxlint-disable-next-line executor/no-promise-reject -- boundary: fumaEffect's whole contract is adapting a rejected driver promise + Promise.reject(drizzleQueryError(SQL, PARAMS, postgresConnectionError("CONNECTION_ENDED"))); + + const exit = await Effect.runPromiseExit( + fumaEffect("plugin_storage.findFirst", driverRejection), + ); + + expect(Exit.isFailure(exit)).toBe(true); + const failure = Exit.isFailure(exit) + ? exit.cause.reasons.find(Cause.isFailReason)?.error + : undefined; + + expect(Predicate.isTagged(failure, "StorageConnectionError")).toBe(true); + expectNoDriverTextIn(failure?.message ?? ""); + }); +}); diff --git a/packages/core/sdk/src/fuma-runtime.ts b/packages/core/sdk/src/fuma-runtime.ts index 137c0d01f..66cef283f 100644 --- a/packages/core/sdk/src/fuma-runtime.ts +++ b/packages/core/sdk/src/fuma-runtime.ts @@ -11,7 +11,35 @@ export class UniqueViolationError extends Data.TaggedError("UniqueViolationError readonly model?: string; }> {} -export type StorageFailure = StorageError | UniqueViolationError; +/** + * The database connection itself failed — the statement never got a verdict. + * Distinct from `StorageError` (a query the backend answered with an error) + * because the two need different responses: a connection fault is either a + * transient socket loss worth retrying on a FRESH pool, or a pool-lifetime + * bug that must stay loud. + * + * `retryable` says which. It is a property of the fault, not a policy: + * - `true` — the socket died underneath a live pool (`CONNECTION_CLOSED`, + * `CONNECT_TIMEOUT`, `ECONNRESET`). Reconnecting can succeed. + * - `false` — the pool was already torn down or the socket belongs to a + * different request context (`CONNECTION_ENDED`, `CONNECTION_DESTROYED`, + * the workerd cross-request I/O rejection). Retrying is futile by + * construction — postgres.js rejects every query once `end()` has been + * called — so these must surface rather than be papered over. + * + * No retry consumes this yet; classification lands first so the retry seam + * (and the request-scope fix behind these faults) can be designed against a + * typed signal instead of driver strings. + */ +export class StorageConnectionError extends Data.TaggedError("StorageConnectionError")<{ + readonly message: string; + readonly label: string; + readonly code: string; + readonly retryable: boolean; + readonly cause: unknown; +}> {} + +export type StorageFailure = StorageError | StorageConnectionError | UniqueViolationError; export type FumaTables = Record; type EmptyFumaSchema = FumaSchema<"latest", Record>; @@ -57,23 +85,111 @@ const isUniqueViolation = (cause: unknown): boolean => { return false; }; -const causeMessage = (cause: unknown): string | undefined => { - const message = - cause && typeof cause === "object" - ? // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: preserve database driver error text inside typed StorageError - (cause as Record)["message"] - : undefined; - return typeof message === "string" && message.length > 0 ? message : undefined; +/** + * postgres.js raises connection faults through `Errors.connection(code, …)`, + * which puts the code on `error.code` (see `postgres/src/errors.js`). Workerd's + * cross-request I/O rejection is a plain `Error` with no code, so it is matched + * on its fixed runtime text and given a synthetic code. + */ +const RETRYABLE_CONNECTION_CODES: ReadonlySet = new Set([ + "CONNECTION_CLOSED", + "CONNECT_TIMEOUT", + "ECONNRESET", +]); +const FATAL_CONNECTION_CODES: ReadonlySet = new Set([ + "CONNECTION_ENDED", + "CONNECTION_DESTROYED", +]); +const CROSS_REQUEST_IO_CODE = "CROSS_REQUEST_IO"; +const CROSS_REQUEST_IO_PATTERN = /cannot perform i\/o on behalf of a different request/i; + +/** Walk a cause chain, newest first, at most 5 links deep (as `isUniqueViolation` does). */ +const walkCauses = (cause: unknown, visit: (err: Record) => boolean): boolean => { + let current = cause; + for (let i = 0; i < 5; i += 1) { + const err = + current && typeof current === "object" ? (current as Record) : null; + if (!err) return false; + if (visit(err)) return true; + const innerCause = err["cause"]; + if (!innerCause || innerCause === current) return false; + current = innerCause; + } + return false; +}; + +/** First driver error code in the cause chain, if any. Never the error text. */ +const causeCode = (cause: unknown): string | undefined => { + let found: string | undefined; + walkCauses(cause, (err) => { + const code = err["code"]; + if (typeof code === "string" && code.length > 0) { + found = code; + return true; + } + return false; + }); + return found; }; +/** Connection-fault code in the cause chain, if this is a connection fault at all. */ +const connectionFaultCode = (cause: unknown): string | undefined => { + let found: string | undefined; + walkCauses(cause, (err) => { + const code = err["code"]; + if ( + typeof code === "string" && + (RETRYABLE_CONNECTION_CODES.has(code) || FATAL_CONNECTION_CODES.has(code)) + ) { + found = code; + return true; + } + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: workerd's cross-request I/O rejection carries no code, only this fixed message + const message = err["message"]; + if (typeof message === "string" && CROSS_REQUEST_IO_PATTERN.test(message)) { + found = CROSS_REQUEST_IO_CODE; + return true; + } + return false; + }); + return found; +}; + +/** + * Build the failure message from stable inputs only — the call-site label and + * the driver's error code — never the driver's text. + * + * The driver text is `Failed query: \nparams: ` + * (`drizzle-orm/errors.js`). Copying it into the message made error reporting + * group by statement shape, splitting one defect across a report per table and + * WHERE-clause, and printed bound parameters (organization ids, user ids, + * user-chosen connection names) into report titles. The full driver error is + * preserved on `cause`, which the reporter still receives as a chained + * exception. + */ +const stableMessage = (label: string, code: string | undefined): string => + code ? `FumaDB ${label} failed: ${code}` : `FumaDB ${label} failed`; + export const isStorageFailure = (error: unknown): error is StorageFailure => - Predicate.isTagged(error, "StorageError") || Predicate.isTagged(error, "UniqueViolationError"); + Predicate.isTagged(error, "StorageError") || + Predicate.isTagged(error, "StorageConnectionError") || + Predicate.isTagged(error, "UniqueViolationError"); export const fumaFailureFromCause = (label: string, cause: unknown): StorageFailure => { if (isStorageFailure(cause)) return cause; if (isUniqueViolation(cause)) return new UniqueViolationError({ model: label }); + const connectionCode = connectionFaultCode(cause); + if (connectionCode !== undefined) { + return new StorageConnectionError({ + message: stableMessage(label, connectionCode), + label, + code: connectionCode, + retryable: RETRYABLE_CONNECTION_CODES.has(connectionCode), + cause, + }); + } return new StorageError({ - message: causeMessage(cause) ?? `FumaDB operation failed: ${label}`, + message: stableMessage(label, causeCode(cause)), cause, }); }; diff --git a/packages/core/sdk/src/index.ts b/packages/core/sdk/src/index.ts index a9b5aeb1f..aa241f371 100644 --- a/packages/core/sdk/src/index.ts +++ b/packages/core/sdk/src/index.ts @@ -36,7 +36,12 @@ export type { IFumaClient, StorageFailure, } from "./fuma-runtime"; -export { StorageError, UniqueViolationError, isStorageFailure } from "./fuma-runtime"; +export { + StorageError, + StorageConnectionError, + UniqueViolationError, + isStorageFailure, +} from "./fuma-runtime"; // IDs (branded) — the v2 set. export { From 49a351f672364d2467ca047a8a0b2e379471da33 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:05:58 -0700 Subject: [PATCH 2/3] Classify ECONNREFUSED as a retryable storage connection fault --- .changeset/storage-error-shaping.md | 2 +- packages/core/sdk/src/fuma-runtime.test.ts | 5 +++++ packages/core/sdk/src/fuma-runtime.ts | 7 +++++-- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/.changeset/storage-error-shaping.md b/.changeset/storage-error-shaping.md index 590aaf5eb..7a50ec85b 100644 --- a/.changeset/storage-error-shaping.md +++ b/.changeset/storage-error-shaping.md @@ -5,4 +5,4 @@ Build `StorageError.message` from the call-site label plus the driver's error code instead of the driver's raw text. The driver text is drizzle's `Failed query: \nparams: `, so error reporting grouped one storage defect by statement shape and printed bound parameters into issue titles. The full driver error stays on `cause`. -Add `StorageConnectionError`, a `StorageFailure` variant for postgres.js connection faults (`CONNECTION_ENDED`, `CONNECTION_CLOSED`, `CONNECTION_DESTROYED`, `CONNECT_TIMEOUT`, `ECONNRESET`) and workerd's cross-request I/O rejection. It carries the fault `code` and a `retryable` flag so a lost socket can be told apart from a pool-lifetime bug. +Add `StorageConnectionError`, a `StorageFailure` variant for postgres.js connection faults (`CONNECTION_ENDED`, `CONNECTION_CLOSED`, `CONNECTION_DESTROYED`, `CONNECT_TIMEOUT`, `ECONNREFUSED`, `ECONNRESET`) and workerd's cross-request I/O rejection. It carries the fault `code` and a `retryable` flag so a lost socket can be told apart from a pool-lifetime bug. diff --git a/packages/core/sdk/src/fuma-runtime.test.ts b/packages/core/sdk/src/fuma-runtime.test.ts index 22e1b76b8..ae18f893d 100644 --- a/packages/core/sdk/src/fuma-runtime.test.ts +++ b/packages/core/sdk/src/fuma-runtime.test.ts @@ -143,6 +143,11 @@ describe("fumaFailureFromCause — classification", () => { ["CONNECTION_CLOSED"], ["CONNECTION_DESTROYED"], ["CONNECT_TIMEOUT"], + // A backend that is simply down: postgres.js surfaces the socket errno + // verbatim, so `connect ECONNREFUSED …` is the ordinary "database + // unreachable" fault and must classify like the rest. + ["ECONNREFUSED"], + ["ECONNRESET"], ])("classifies postgres.js %s as a storage connection fault", (code) => { const failure = fumaFailureFromCause( "plugin_storage.findFirst", diff --git a/packages/core/sdk/src/fuma-runtime.ts b/packages/core/sdk/src/fuma-runtime.ts index 66cef283f..ae5c4ebcc 100644 --- a/packages/core/sdk/src/fuma-runtime.ts +++ b/packages/core/sdk/src/fuma-runtime.ts @@ -19,8 +19,10 @@ export class UniqueViolationError extends Data.TaggedError("UniqueViolationError * bug that must stay loud. * * `retryable` says which. It is a property of the fault, not a policy: - * - `true` — the socket died underneath a live pool (`CONNECTION_CLOSED`, - * `CONNECT_TIMEOUT`, `ECONNRESET`). Reconnecting can succeed. + * - `true` — the socket died underneath a live pool, or was never + * established because the backend was unreachable (`CONNECTION_CLOSED`, + * `CONNECT_TIMEOUT`, `ECONNREFUSED`, `ECONNRESET`). Reconnecting can + * succeed. * - `false` — the pool was already torn down or the socket belongs to a * different request context (`CONNECTION_ENDED`, `CONNECTION_DESTROYED`, * the workerd cross-request I/O rejection). Retrying is futile by @@ -94,6 +96,7 @@ const isUniqueViolation = (cause: unknown): boolean => { const RETRYABLE_CONNECTION_CODES: ReadonlySet = new Set([ "CONNECTION_CLOSED", "CONNECT_TIMEOUT", + "ECONNREFUSED", "ECONNRESET", ]); const FATAL_CONNECTION_CODES: ReadonlySet = new Set([ From f7112f633b32fc95aed13b8d6a63bdd3e95b656f Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:47:31 -0700 Subject: [PATCH 3/3] Add an e2e scenario for the shape of a storage failure report --- e2e/cloud/storage-error-report-shape.test.ts | 280 +++++++++++++++++++ 1 file changed, 280 insertions(+) create mode 100644 e2e/cloud/storage-error-report-shape.test.ts diff --git a/e2e/cloud/storage-error-report-shape.test.ts b/e2e/cloud/storage-error-report-shape.test.ts new file mode 100644 index 000000000..ab8a848f4 --- /dev/null +++ b/e2e/cloud/storage-error-report-shape.test.ts @@ -0,0 +1,280 @@ +// Cloud-only: what an operator SEES when a write is rejected by the database. +// +// The product guarantee: a storage failure is reported under a stable headline +// built from the operation and the database's error code — never the statement +// text, never the values that were bound into it. Two consequences, both of +// them things production got wrong: +// +// - The values bound into a rejected statement are customer data (the +// organization id, the connection name, whatever the user typed into the +// description). They must not appear in the report's headline. +// - The headline is the grouping key of the error reporter, so one defect that +// hits several tables — or the same table through several WHERE shapes — +// must arrive as ONE report, not one per statement. +// +// The failure is induced through the public typed API only: PostgreSQL cannot +// store a NUL byte in a text column, so a connection whose description carries +// one is rejected by the driver with SQLSTATE 22021 while the statement and its +// bound parameters are already assembled. That is the same class of failure the +// production reports came from, reachable without touching the database. +// +// Two surfaces are asserted, both public: +// 1. What the CALLER gets — an opaque `InternalError` carrying only a trace +// id, with no driver text anywhere in the payload. +// 2. What the OPERATOR gets — the server's own error log, where the trace id +// the caller received joins to the report the server filed. Its headline — +// the captured exception's type and message — is what the error reporter +// files the report under, and groups by. +import { randomBytes } from "node:crypto"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; + +import { expect } from "@effect/vitest"; +import { Cause, Effect, Exit, Schedule } from "effect"; +import type { HttpApiClient } from "effect/unstable/httpapi"; +import { composePluginApi } from "@executor-js/api/server"; +import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; +import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/sdk/shared"; + +import { RUNS_DIR, scenario } from "../src/scenario"; +import { Api, Target } from "../src/services"; + +const api = composePluginApi([openApiHttpPlugin()] as const); +type Client = HttpApiClient.ForApi; + +/** A text value PostgreSQL cannot store — the driver rejects it as 22021. */ +const NUL = String.fromCharCode(0); + +const SLUG = "storage-error-report-shape"; + +/** Minimal OpenAPI spec with a single GET /ping — never contacted here. */ +const pingSpec = JSON.stringify({ + openapi: "3.0.3", + info: { title: "Ping API", version: "1.0.0" }, + paths: { + "/ping": { + get: { operationId: "ping", summary: "Ping", responses: { "200": { description: "pong" } } }, + }, + }, +}); + +/** Registers a fresh apiKey-authenticated integration for connections to bind to. */ +const registerIntegration = (client: Client) => + Effect.gen(function* () { + const slug = IntegrationSlug.make(`${SLUG}-${randomBytes(4).toString("hex")}`); + yield* client.openapi.addSpec({ + payload: { + spec: { kind: "blob", value: pingSpec }, + slug, + baseUrl: "http://127.0.0.1:59999", // never contacted during registration + authenticationTemplate: [ + { + slug: "apiKey", + type: "apiKey", + headers: { authorization: ["Bearer ", { type: "variable", name: "token" }] }, + }, + ], + }, + }); + return slug; + }); + +interface RejectedWrite { + /** The trace id the caller was handed; joins to the server's report. */ + readonly traceId: string; + /** The name the caller chose for the connection — customer data. */ + readonly name: string; + /** The free text the caller typed — customer data, and the NUL carrier. */ + readonly description: string; + /** The whole client-visible failure, serialized. */ + readonly payload: string; +} + +/** + * Create a connection whose description PostgreSQL will refuse, and return what + * the caller can see about the failure. + */ +const rejectedConnectionWrite = ( + client: Client, + integration: IntegrationSlug, +): Effect.Effect => + Effect.gen(function* () { + const name = ConnectionName.make( + `${SLUG.replaceAll("-", "")}${randomBytes(4).toString("hex")}`, + ); + const description = `desc-${randomBytes(6).toString("hex")}${NUL}tail`; + + const exit = yield* Effect.exit( + client.connections.create({ + payload: { + owner: "org", + name, + integration, + template: AuthTemplateSlug.make("apiKey"), + description, + value: `sk-${randomBytes(4).toString("hex")}`, + }, + }), + ); + + const failure = Exit.isFailure(exit) + ? exit.cause.reasons.find(Cause.isFailReason)?.error + : exit.value; + const error = failure as { readonly _tag?: string; readonly traceId?: string } | undefined; + + // PostgreSQL refuses the NUL byte, so the write cannot succeed, and what + // comes back is the opaque internal failure — never the storage error. + expect(error?._tag, "the caller sees an opaque internal failure").toBe("InternalError"); + expect(error?.traceId ?? "", "the caller is handed a trace id to quote").toMatch( + /^[0-9a-f]{32}$/, + ); + + return { + traceId: error?.traceId ?? "", + name, + description, + payload: JSON.stringify(failure), + }; + }); + +/** + * The dev stack's stdout. The suite's globalsetup funnels it into the run + * artifacts; a scenario run against an already-booted instance (`cli up cloud`) + * reads that instance's log instead. + */ +const serverLogCandidates = [ + resolve(RUNS_DIR, "cloud", "server-logs", "boot.log"), + resolve(RUNS_DIR, "..", ".dev", "cloud.log"), +]; + +const readServerLog = (): string => { + const texts = serverLogCandidates.flatMap((path) => { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: probing which of the two stdout sinks this run uses + try { + return [readFileSync(path, "utf8")]; + } catch { + return []; + } + }); + return texts.join("\n"); +}; + +const REPORT_PREFIX = "[api] unhandled cause: "; + +/** A stack frame in the logged cause — where the report's headline stops. */ +const STACK_FRAME = /^\s+at /; + +interface FiledReport { + /** Type + message: what the reporter names and groups the report by. */ + readonly headline: string; + /** The whole record, headline and chained cause — what a diagnosis reads. */ + readonly full: string; +} + +/** + * The report the server filed for one request. + * + * The headline is the captured cause's type and message up to the first stack + * frame — exactly what `Cause.prettyErrors` hands the reporter as the + * exception. The message is multi-line whenever the driver's text is + * (`Failed query: …\nparams: …`), so the whole headline has to be read, not + * just its first line. + * + * Found by walking back from the correlation record carrying the caller's trace + * id, so it is THIS request's report and not a neighbour's. + */ +const reportFor = (traceId: string): Effect.Effect => + Effect.sync(() => { + const lines = readServerLog().split("\n"); + const correlated = lines.findLastIndex( + (line) => + line.includes('"event":"sentry_before_send_otel_correlation"') && + line.includes(`"sentry_event_id":"${traceId}"`), + ); + if (correlated === -1) return undefined; + const reported = lines + .slice(0, correlated) + .findLastIndex((line) => line.startsWith(REPORT_PREFIX)); + if (reported === -1) return undefined; + const block = [ + lines[reported]!.slice(REPORT_PREFIX.length), + ...lines.slice(reported + 1, correlated), + ]; + const end = block.slice(1).findIndex((line) => STACK_FRAME.test(line)); + return { + headline: block + .slice(0, end === -1 ? 1 : end + 1) + .join("\n") + .trimEnd(), + full: block.join("\n"), + }; + }).pipe( + Effect.filterOrFail( + (report): report is FiledReport => report !== undefined, + () => `no error report joined to trace id ${traceId} in the server log`, + ), + // The log is a file the dev stack appends to; the write lands moments after + // the response. Poll rather than sleep (~20s ceiling). + Effect.retry(Schedule.both(Schedule.spaced("500 millis"), Schedule.recurs(40))), + ); + +scenario( + "Storage · a rejected write is reported without its SQL or the caller's data", + { timeout: 120_000 }, + Effect.gen(function* () { + const target = yield* Target; + const { client: apiClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* apiClient(api, identity); + const integration = yield* registerIntegration(client); + + const first = yield* rejectedConnectionWrite(client, integration); + const second = yield* rejectedConnectionWrite(client, integration); + + for (const write of [first, second]) { + expect(write.payload, "the client payload carries no driver text").not.toContain( + "Failed query", + ); + expect(write.payload, "the client payload carries no bound parameter").not.toContain( + write.description, + ); + } + + const report = yield* reportFor(first.traceId); + const headline = report.headline; + + // The symptom: the statement and everything bound into it used to BE the + // headline, so the report was named after the customer's data. + expect(headline, "the report headline carries no statement text").not.toContain("Failed query"); + expect(headline, "the report headline carries no statement text").not.toContain("insert into"); + expect(headline, "the report headline carries no bound parameters").not.toContain("params:"); + expect(headline, "the report headline carries no user-typed description").not.toContain( + first.description, + ); + expect(headline, "the report headline carries no user-chosen connection name").not.toContain( + first.name, + ); + expect(headline, "the report headline carries no organization id").not.toContain("org_"); + + // What it says instead: which operation failed, and how the database + // refused it — enough to act on, stable across calls. + expect(headline, "the report still names the failing operation").toContain("connection.create"); + expect(headline, "the report still names the database's error code").toContain("22021"); + + // Shaping the headline must not mean throwing the diagnosis away: the + // driver's own text is still filed with the report, one level down, where + // it informs a fix instead of naming the report. + expect(report.full, "the driver's statement is still filed under the report").toContain( + "Failed query", + ); + + // The fan-out: the two writes bound different names, descriptions and + // secrets, so their statements differ in every parameter. One defect, one + // report — not one report per set of values. + const secondReport = yield* reportFor(second.traceId); + expect( + secondReport.headline, + "a second rejected write with different values files the same report", + ).toBe(headline); + }), +);