-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(webapp): capture Prisma infra errors and obfuscate leaked messages #3960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f4916fb
8ed89cf
00aaf7a
fbbc9f8
0e300c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| area: webapp | ||
| type: fix | ||
| --- | ||
|
|
||
| Log Prisma infrastructure errors (P1xxx) centrally and obfuscate their messages (which carry the DB hostname) on API responses that previously returned the raw message, without changing status codes or headers. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,9 @@ | ||
| import { Prisma } from "@trigger.dev/database"; | ||
| import { Prisma, type PrismaClient, isPrismaKnownError } from "@trigger.dev/database"; | ||
| import { logger } from "~/services/logger.server"; | ||
|
|
||
| // Minimal structural logger so this stays decoupled from the concrete Logger | ||
| // (and lets tests pass a capturing logger). | ||
| type ErrorLogger = { error: (message: string, fields?: Record<string, unknown>) => void }; | ||
|
|
||
| // Prisma connectivity / infrastructure error codes — engine- and | ||
| // connection-level failures, not query- or validation-level ones. When the | ||
|
|
@@ -37,3 +42,97 @@ export function isInfrastructureError(error: unknown): boolean { | |
|
|
||
| return false; | ||
| } | ||
|
|
||
| // One-shot marker so a single infra error is logged exactly once: the client | ||
| // extension (statement level) tags it, and the $transaction-boundary loggers | ||
| // skip a tagged error rather than logging the same failure a second time. | ||
| const INFRA_ERROR_LOGGED: unique symbol = Symbol("prismaInfraErrorLogged"); | ||
|
|
||
| function markInfraErrorLogged(error: unknown): void { | ||
| if (typeof error !== "object" || error === null) { | ||
| return; | ||
| } | ||
| try { | ||
| // Non-enumerable so error-spreads/serializers can't copy the marker onto a | ||
| // different error; try/catch so a frozen error object can't make this throw | ||
| // and mask the original error as it propagates out of the catch. | ||
| Object.defineProperty(error, INFRA_ERROR_LOGGED, { | ||
| value: true, | ||
| enumerable: false, | ||
| configurable: true, | ||
| writable: true, | ||
| }); | ||
| } catch { | ||
| // best-effort: a sealed/frozen error simply won't be deduped. | ||
| } | ||
| } | ||
|
|
||
| export function infraErrorAlreadyLogged(error: unknown): boolean { | ||
| return ( | ||
| typeof error === "object" && | ||
| error !== null && | ||
| (error as Record<symbol, unknown>)[INFRA_ERROR_LOGGED] === true | ||
| ); | ||
| } | ||
|
|
||
| // Logs infrastructure failures (P1xxx-class, see isInfrastructureError) and | ||
| // rethrows the ORIGINAL error: callers branch on error.code, and this fires | ||
| // per-statement inside transactions, so converting it would break that. | ||
| export function captureInfrastructureErrors<T extends PrismaClient>( | ||
| client: T, | ||
| log: ErrorLogger = logger | ||
| ): T { | ||
| return client.$extends({ | ||
| name: "infrastructure-error-capture", | ||
| query: { | ||
| $allOperations: async ({ model, operation, args, query }) => { | ||
| try { | ||
| return await query(args); | ||
| } catch (error) { | ||
| if (isInfrastructureError(error)) { | ||
| log.error("prisma infrastructure error", { | ||
| model, | ||
| operation, | ||
| code: error instanceof Prisma.PrismaClientKnownRequestError ? error.code : undefined, | ||
| meta: error instanceof Prisma.PrismaClientKnownRequestError ? error.meta : undefined, | ||
| message: error instanceof Error ? error.message : String(error), | ||
| stack: error instanceof Error ? error.stack : undefined, | ||
| }); | ||
| markInfraErrorLogged(error); | ||
| } | ||
|
|
||
| throw error; | ||
| } | ||
| }, | ||
| }, | ||
| }) as unknown as T; | ||
| } | ||
|
|
||
| // Logs infrastructure errors that reach the $transaction boundary WITHOUT a | ||
| // Prisma error code (e.g. PrismaClientInitializationError). Coded errors there | ||
| // are already logged by transac()'s callback, and errors that bubbled up from a | ||
| // statement were already logged (and tagged) by the client extension — both are | ||
| // skipped here to avoid double-logging. Returns whether it logged. | ||
| export function logTransactionInfrastructureError( | ||
| error: unknown, | ||
| log: ErrorLogger = logger | ||
| ): boolean { | ||
| if (!isInfrastructureError(error) || isPrismaKnownError(error) || infraErrorAlreadyLogged(error)) { | ||
| return false; | ||
| } | ||
|
|
||
| log.error("prisma.$transaction infrastructure error", { | ||
| message: error instanceof Error ? error.message : String(error), | ||
| name: error instanceof Error ? error.name : undefined, | ||
| stack: error instanceof Error ? error.stack : undefined, | ||
| }); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| // Replaces a Prisma infrastructure error's message (which carries the DB | ||
| // hostname) with a generic one before it reaches an API client. Any other | ||
| // error's message is returned unchanged. Status codes/headers are unaffected. | ||
| export function clientSafeErrorMessage(error: Error): string { | ||
| return isInfrastructureError(error) ? "Internal Server Error" : error.message; | ||
| } | ||
|
Comment on lines
+136
to
+138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Several other API routes still return raw error.message to clients — not covered by this PR This PR applies Was this helpful? React with 👍 or 👎 to provide feedback. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚩 Status code mismatch: 'Internal Server Error' message returned with 400/422 status in several routes
Several routes now return
clientSafeErrorMessage(error)which yields"Internal Server Error"for infrastructure errors, but the HTTP status code remains 400 or 422 (client error codes). For example:api.v1.tasks.$taskId.batch.ts:129returns status 400 with message"Internal Server Error"api.v1.schedules.$scheduleId.ts:58-59returns status 422 for non-P2025 known Prisma errors including P1001engine.v1.worker-actions...continue.ts:35returns status 422The PR description explicitly states "without changing status codes", so this is intentional. However, for transient infrastructure errors (P1001 — DB unreachable), a 4xx status tells SDK clients not to retry, which is suboptimal for what is fundamentally a server-side transient failure. The routes at
api.v2.tasks.batch.ts:179andapi.v3.batches.ts:194correctly use status 500. The pre-existing status code issue in the v1 routes may warrant a follow-up.Was this helpful? React with 👍 or 👎 to provide feedback.