diff --git a/apps/api/src/lib/error-report.ts b/apps/api/src/lib/error-report.ts index 5f88c7d3..bbfc2253 100644 --- a/apps/api/src/lib/error-report.ts +++ b/apps/api/src/lib/error-report.ts @@ -13,6 +13,7 @@ */ import { connectivityClass, + extractErrorCode, isClientAbort, isSafeMessageError, isToolInputError, @@ -38,6 +39,10 @@ export interface ReportContext { export function classifyError(err: unknown, source?: ReportContext["source"]): ErrorClass { if (isToolInputError(err)) return "expected"; const e = err as { name?: string; message?: string; code?: string } | null; + // InputValidationError (apps/api/src/modality/contract.ts) is a 400 the user + // caused with a bad file/args. Tools throw it from processV2 inside the worker + // (e.g. sprite-sheet), so it is expected wherever it surfaces, not only http. + if (e?.name === "InputValidationError") return "expected"; if (e && typeof e.message === "string" && /^(Canceled$|Timed out after )/.test(e.message)) { return "expected"; } @@ -48,9 +53,9 @@ export function classifyError(err: unknown, source?: ReportContext["source"]): E // worker-side parse failure is our bug. if (source === "http" || source === undefined) { if (isClientAbort(err)) return "expected"; - // ZodError = settings validation; InputValidationError = upload validation - // (apps/api/src/modality/contract.ts). Both are user-input problems. - if (e?.name === "ZodError" || e?.name === "InputValidationError") return "expected"; + // ZodError = settings validation. Settings are validated at the boundary, so + // a worker-side ZodError is schema drift (our bug); only expected on http. + if (e?.name === "ZodError") return "expected"; } if (isSafeMessageError(err)) return err.kind === "bug" ? "bug" : "operational"; if (connectivityClass(err)) return "operational"; @@ -106,7 +111,7 @@ export async function reportError(err: unknown, ctx: ReportContext): Promise { @@ -557,7 +559,9 @@ export class PythonDispatcher { } if (timedOut) { - rejectPromise(new Error("Python script timed out")); + rejectPromise( + new SafeError("Python script timed out", { kind: "operational", code: "timeout" }), + ); return; } diff --git a/packages/shared/src/analytics/error-sanitize.ts b/packages/shared/src/analytics/error-sanitize.ts index f02d006c..b12968af 100644 --- a/packages/shared/src/analytics/error-sanitize.ts +++ b/packages/shared/src/analytics/error-sanitize.ts @@ -102,6 +102,30 @@ export function rebuildErrorValue(err: unknown): string | null { } } +/** + * The most specific, non-sensitive error code in the cause chain, for the + * Sentry `error_code` tag. Prefers a pg SQLSTATE, then a node E-code, else the + * first short string code found (e.g. a SafeError's authored code). Returns + * null when none is present. reportError used to read only the top-level + * `.code`, but pg/undici bury the real code under a drizzle/wrapper Error whose + * own `.code` is undefined, so the tag was always empty on those events. + */ +export function extractErrorCode(err: unknown): string | null { + try { + let fallback: string | null = null; + for (const l of chain(err)) { + const code = l.code; + if (typeof code !== "string" || code.length === 0 || code.length > 40) continue; + if (SQLSTATE.test(code) && !NODE_CODE.test(code)) return code; + if (NODE_CODE.test(code)) return code; + if (fallback === null) fallback = code; + } + return fallback; + } catch { + return null; + } +} + export type ConnectivityClass = "pg-unavailable" | "redis-unavailable" | "net-unavailable"; /** Infra-connectivity classification used for fingerprinting + throttling. */ diff --git a/tests/unit/ai/bridge.test.ts b/tests/unit/ai/bridge.test.ts index ef84257f..10536d73 100644 --- a/tests/unit/ai/bridge.test.ts +++ b/tests/unit/ai/bridge.test.ts @@ -271,6 +271,30 @@ describe("bridge - runPythonWithProgress (per-request fallback)", () => { vi.useRealTimers(); }); + it("rejects a per-request timeout as an operational SafeError with a 'timeout' code", async () => { + // A timeout is environmental (huge input / slow box), not our bug. Sentry + // must see it as an operational SafeError, not a message-less "Error: Error". + vi.useFakeTimers(); + const mock = createMockProcess(); + vi.mocked(spawn).mockReturnValue(mock.process); + + const promise = runPythonWithProgress("slow.py", [], { timeout: 1000 }); + const settled = promise.catch((e: unknown) => e); // attach before firing the timer + vi.advanceTimersByTime(1500); + mock.emitEvent("close", null, "SIGTERM"); + + const err = (await settled) as Error & { + isSafeMessage?: unknown; + kind?: string; + code?: string; + }; + expect(err.message).toBe("Python script timed out"); + expect(err.isSafeMessage).toBe(true); + expect(err.kind).toBe("operational"); + expect(err.code).toBe("timeout"); + vi.useRealTimers(); + }); + it("invokes onProgress callback for JSON progress lines on stderr", async () => { const mock = createMockProcess(); vi.mocked(spawn).mockReturnValue(mock.process); @@ -1390,6 +1414,36 @@ describe("bridge - dispatcher stdin JSON-RPC protocol", () => { expect(result.stdout).toBe('{"success": true}'); }); + it("rejects a dispatcher-path timeout as an operational SafeError (NODE-26 regression)", async () => { + // NODE-26: the dispatcher request() timeout rejected with a bare + // new Error("Python script timed out"), which the Sentry sanitizer scrubbed + // to a message-less "Error: Error" bug. It must be an operational SafeError. + vi.useFakeTimers(); + const mock = createMockProcess(); + vi.mocked(spawn).mockReturnValue(mock.process); + + const initPromise = initDispatcher(); + mock.stderr.emit("data", Buffer.from('{"ready": true, "gpu": false}\n')); + await vi.advanceTimersByTimeAsync(60); // init() polls childReady every 50ms + await initPromise; + + const promise = runPythonWithProgress("slow.py", [], { timeout: 1000 }); + const settled = promise.catch((e: unknown) => e); // attach before firing the timer + await vi.advanceTimersByTimeAsync(0); // flush the request's stdin write + await vi.advanceTimersByTimeAsync(1100); // fire the dispatcher request timeout + + const err = (await settled) as Error & { + isSafeMessage?: unknown; + kind?: string; + code?: string; + }; + expect(err.message).toBe("Python script timed out"); + expect(err.isSafeMessage).toBe(true); + expect(err.kind).toBe("operational"); + expect(err.code).toBe("timeout"); + vi.useRealTimers(); + }); + it("strips .py extension from script name in dispatcher request", async () => { const mock = await setupReadyDispatcher(); diff --git a/tests/unit/api/capture-path.test.ts b/tests/unit/api/capture-path.test.ts index 7ac65000..18d90a74 100644 --- a/tests/unit/api/capture-path.test.ts +++ b/tests/unit/api/capture-path.test.ts @@ -46,6 +46,21 @@ describe("capture path", () => { expect(h.scope.setTag).toHaveBeenCalledWith("pool", "image"); }); + it("tags error_code from a code nested in the cause chain, not just the top level", async () => { + // pg/undici bury the real code under a drizzle/wrapper Error whose own + // .code is undefined; reading only the top level left error_code empty. + const pg = Object.assign(new Error("password authentication failed"), { + code: "28P01", + severity: "FATAL", + }); + const wrapped = Object.assign(new Error("Failed query: select 1"), { cause: pg }); + + await reportError(wrapped, { source: "worker", pool: "docs" }); + + expect(h.captureException).toHaveBeenCalledTimes(1); + expect(h.scope.setTag).toHaveBeenCalledWith("error_code", "28P01"); + }); + it("captures once per distinct signature; operational repeats are throttled", async () => { const full = Object.assign(new Error("disk full"), { code: "ENOSPC" }); await reportError(full, { source: "worker", pool: "image" }); diff --git a/tests/unit/api/error-report.test.ts b/tests/unit/api/error-report.test.ts index 63b49a1b..b0164b3d 100644 --- a/tests/unit/api/error-report.test.ts +++ b/tests/unit/api/error-report.test.ts @@ -50,6 +50,17 @@ describe("classifyError", () => { expect(classifyError(reset, "worker")).toBe("operational"); expect(classifyError(reset, "http")).toBe("expected"); }); + it("InputValidationError is a user 400 wherever it surfaces, not only on http", () => { + // Tools throw InputValidationError from processV2 in the worker (e.g. + // sprite-sheet "Provide at least two images"); it must not be logged as a bug. + const e = Object.assign(new Error("Provide at least two images"), { + name: "InputValidationError", + }); + expect(classifyError(e, "worker")).toBe("expected"); + expect(classifyError(e, "cron")).toBe("expected"); + expect(classifyError(e, "http")).toBe("expected"); + expect(classifyError(e)).toBe("expected"); + }); }); describe("throttle", () => { diff --git a/tests/unit/shared/error-sanitize.test.ts b/tests/unit/shared/error-sanitize.test.ts index b894aba2..7c17863d 100644 --- a/tests/unit/shared/error-sanitize.test.ts +++ b/tests/unit/shared/error-sanitize.test.ts @@ -1,4 +1,10 @@ -import { connectivityClass, isClientAbort, rebuildErrorValue, SafeError } from "@snapotter/shared"; +import { + connectivityClass, + extractErrorCode, + isClientAbort, + rebuildErrorValue, + SafeError, +} from "@snapotter/shared"; import { describe, expect, it } from "vitest"; const sysErr = (code: string, syscall?: string) => @@ -79,6 +85,45 @@ describe("rebuildErrorValue", () => { }); }); +describe("extractErrorCode", () => { + it("finds a pg SQLSTATE through a drizzle-style cause chain", () => { + const pg = Object.assign(new Error("password authentication failed"), { + code: "28P01", + severity: "FATAL", + }); + const wrapped = Object.assign(new Error("Failed query: select 1"), { cause: pg }); + expect(extractErrorCode(wrapped)).toBe("28P01"); + }); + it("finds a node syscall code nested in the chain", () => { + const net = Object.assign(new Error("getaddrinfo ENOTFOUND db"), { code: "ENOTFOUND" }); + const wrapped = Object.assign(new Error("connect failed"), { cause: net }); + expect(extractErrorCode(wrapped)).toBe("ENOTFOUND"); + }); + it("prefers a pg SQLSTATE over an earlier non-standard code", () => { + const pg = Object.assign(new Error("deadlock detected"), { code: "40P01" }); + const wrapped = Object.assign(new Error("wrap"), { code: "generic", cause: pg }); + expect(extractErrorCode(wrapped)).toBe("40P01"); + }); + it("falls back to a SafeError's authored code when no pg/node code is present", () => { + const err = new SafeError("Python script timed out", { kind: "operational", code: "timeout" }); + expect(extractErrorCode(err)).toBe("timeout"); + }); + it("returns null when no code exists anywhere in the chain", () => { + expect(extractErrorCode(new Error("plain boom"))).toBeNull(); + expect(extractErrorCode("string")).toBeNull(); + expect(extractErrorCode(null)).toBeNull(); + }); + it("returns null when a cause getter throws (hostile)", () => { + const hostile = new Error("boom"); + Object.defineProperty(hostile, "cause", { + get() { + throw new Error("gotcha"); + }, + }); + expect(extractErrorCode(hostile)).toBeNull(); + }); +}); + describe("connectivityClass", () => { it("classifies pg-unavailable via SQLSTATE 08/57P and drizzle wrapping", () => { const pg = Object.assign(new Error("terminating connection"), { code: "57P01" });