diff --git a/doc/observability.md b/doc/observability.md index 17e9c251de4..ba58b72d1fb 100644 --- a/doc/observability.md +++ b/doc/observability.md @@ -387,11 +387,13 @@ integration list of `@sentry/node@10.71.0` and `@sentry/browser@10.71.0`. - `ChildProcess` — one event for each worker-thread `error`. - `LinkedErrors` — the `error.cause` chain of each captured error. -An uncaught exception on the main thread is a server event too, but it does -not come from a default Sentry integration. See "PostgreSQL Null-Socket -Write Guard" below: that guard, not `OnUncaughtException`, owns the -process's `uncaughtException` decision, and it reports the exception to -Sentry itself before the process exits. +An uncaught exception on the main thread is a server event too. While the +PostgreSQL null-socket write guard is on, it does not come from a default +Sentry integration — see "PostgreSQL Null-Socket Write Guard" below: that +guard, not `OnUncaughtException`, owns the process's `uncaughtException` +decision, and it reports the exception to Sentry itself before the process +exits. When an operator turns that guard off, `OnUncaughtException` stays +active and reports the exception instead. **Server context the kept integrations attach** @@ -413,9 +415,10 @@ Sentry itself before the process exits. - `ContextLines` — 7 local source lines around each stack frame. - The outbound breadcrumb of `Http` — outbound request URLs and query strings. -- `OnUncaughtException` — removed so it never registers its own - `uncaughtException` listener. See "PostgreSQL Null-Socket Write Guard" - below for why one listener, not two, must own that event. +- `OnUncaughtException` — removed while the PostgreSQL null-socket write + guard is on, so it never registers a second, independent + `uncaughtException` listener. Kept active when an operator turns that + guard off. See "PostgreSQL Null-Socket Write Guard" below. **Browser events this feature adds** @@ -496,24 +499,34 @@ This environment variable is the guard's one operator control. no `uncaughtException` listener of its own for this fault, so Node's default behavior returns for it: the known, otherwise-neutralized `TypeError` ends the server process the same way it did before this - guard existed. Turn the guard off only to compare behavior against the - unguarded process, or once the operator has upgraded past the - underlying driver defect. + guard existed. When Sentry error monitoring is also on, Sentry's own + default `OnUncaughtException` integration stays active in this case (see + "Sentry interaction" below), so the exception still reaches Sentry + before the process ends. Turn the guard off only to compare behavior + against the unguarded process, or once the operator has upgraded past + the underlying driver defect. ### Sentry interaction An operator who also enables server-side Sentry error monitoring gets no -second, independent `uncaughtException` listener from Sentry. `sentry.ts` -removes Sentry's default `OnUncaughtException` integration for exactly -this reason: Node calls every listener registered for an event, not only -the first one, so a second listener would still end the process on the -known, neutralized fault, regardless of which listener runs first. This -guard is the sole owner of the process's `uncaughtException` decision. It -reports every exception it does not neutralize to Sentry itself, through -the same `captureException` and `shutdownSentry` calls a server startup -failure uses (see "Sentry Error Monitoring" above). A deployment with no -Sentry DSN set sees no change from this interaction: both calls are -no-ops. +second, independent `uncaughtException` listener from Sentry while this +guard is on. `sentry.ts` removes Sentry's default `OnUncaughtException` +integration for exactly this reason: Node calls every listener registered +for an event, not only the first one, so a second listener would still end +the process on the known, neutralized fault, regardless of which listener +runs first. While the guard is on, it is the sole owner of the process's +`uncaughtException` decision. It reports every exception it does not +neutralize to Sentry itself, through the same `captureException` and +`shutdownSentry` calls a server startup failure uses (see "Sentry Error +Monitoring" above). A deployment with no Sentry DSN set sees no change +from this interaction: both calls are no-ops. + +When an operator turns the guard off +(`POSTGRES_NULL_SOCKET_GUARD_ENABLED=false`), no module registers an +`uncaughtException` listener on the guard's behalf, so `sentry.ts` keeps +Sentry's default `OnUncaughtException` integration active instead. An +unrelated uncaught exception then reaches Sentry through that default +integration rather than through the guard's own reporting call. ## Sandbox Startup Trace Spans diff --git a/server/src/__tests__/sentry.test.ts b/server/src/__tests__/sentry.test.ts index 550a830612c..468403b9995 100644 --- a/server/src/__tests__/sentry.test.ts +++ b/server/src/__tests__/sentry.test.ts @@ -23,9 +23,11 @@ import * as sentryModule from "../sentry.js"; const DSN_ENV = "SENTRY_DSN"; const FRONTEND_DSN_ENV = "SENTRY_DSN_FRONTEND"; const BACKEND_DSN_ENV = "SENTRY_DSN_BACKEND"; +const GUARD_ENABLED_ENV = "POSTGRES_NULL_SOCKET_GUARD_ENABLED"; const originalDsn = process.env[DSN_ENV]; const originalFrontendDsn = process.env[FRONTEND_DSN_ENV]; const originalBackendDsn = process.env[BACKEND_DSN_ENV]; +const originalGuardEnabled = process.env[GUARD_ENABLED_ENV]; async function importFreshSentry() { vi.resetModules(); @@ -96,6 +98,7 @@ beforeEach(() => { delete process.env[DSN_ENV]; delete process.env[FRONTEND_DSN_ENV]; delete process.env[BACKEND_DSN_ENV]; + delete process.env[GUARD_ENABLED_ENV]; }); afterEach(() => { @@ -105,6 +108,8 @@ afterEach(() => { else process.env[FRONTEND_DSN_ENV] = originalFrontendDsn; if (originalBackendDsn === undefined) delete process.env[BACKEND_DSN_ENV]; else process.env[BACKEND_DSN_ENV] = originalBackendDsn; + if (originalGuardEnabled === undefined) delete process.env[GUARD_ENABLED_ENV]; + else process.env[GUARD_ENABLED_ENV] = originalGuardEnabled; vi.restoreAllMocks(); vi.doUnmock("@sentry/node"); vi.doUnmock("../peer-version-check.js"); @@ -422,6 +427,26 @@ describe("buildSentryInitOptions", () => { expect(names).not.toContain("OnUncaughtException"); }); + it("keeps OnUncaughtException in the resolved list when the guard does not own it", async () => { + const { buildSentryInitOptions } = await importFreshSentry(); + + const options = buildSentryInitOptions( + "https://public@o0.ingest.sentry.io/1", + { + httpIntegration: () => ({ name: "Http" }), + onUnhandledRejectionIntegration: () => ({ name: "OnUnhandledRejection" }), + }, + false, + ); + const resolved = options.integrations(DEFAULT_INTEGRATION_NAMES.map((name) => ({ name }))); + + // The postgres null-socket write guard registers no `uncaughtException` + // listener when an operator disables it, so Sentry's own default + // integration must stay active — otherwise an unrelated uncaught + // exception would reach no error reporter at all. + expect(resolved.map((i) => i.name)).toContain("OnUncaughtException"); + }); + it("the resolved server integration list keeps OnUnhandledRejection, LinkedErrors, and RequestData", async () => { const { buildSentryInitOptions } = await importFreshSentry(); @@ -480,6 +505,40 @@ describe("with @sentry/node mocked", () => { await shutdownSentry(); expect(mocks.close).toHaveBeenCalledWith(5_000); }); + + it("removes the default OnUncaughtException integration when the postgres null-socket write guard is on", async () => { + process.env[DSN_ENV] = "https://public@o0.ingest.sentry.io/1"; + const mocks = mockSentryPackage(); + + const { sentryReady } = await importFreshSentry(); + await sentryReady; + + const initOptions = mocks.init.mock.calls[0][0] as { + integrations: (defaults: Array<{ name: string }>) => Array<{ name: string }>; + }; + const resolved = initOptions.integrations([{ name: "OnUncaughtException" }]); + + expect(resolved.map((i) => i.name)).not.toContain("OnUncaughtException"); + }); + + it("keeps the default OnUncaughtException integration when an operator disables the guard", async () => { + process.env[DSN_ENV] = "https://public@o0.ingest.sentry.io/1"; + process.env[GUARD_ENABLED_ENV] = "false"; + const mocks = mockSentryPackage(); + + const { sentryReady } = await importFreshSentry(); + await sentryReady; + + const initOptions = mocks.init.mock.calls[0][0] as { + integrations: (defaults: Array<{ name: string }>) => Array<{ name: string }>; + }; + const resolved = initOptions.integrations([{ name: "OnUncaughtException" }]); + + // The guard registers no `uncaughtException` listener when disabled, so + // Sentry's own default integration must stay active — otherwise an + // unrelated uncaught exception reaches no error reporter at all. + expect(resolved.map((i) => i.name)).toContain("OnUncaughtException"); + }); }); /** diff --git a/server/src/postgres-null-socket-guard-env.ts b/server/src/postgres-null-socket-guard-env.ts new file mode 100644 index 00000000000..4a96ab3033e --- /dev/null +++ b/server/src/postgres-null-socket-guard-env.ts @@ -0,0 +1,22 @@ +// The `POSTGRES_NULL_SOCKET_GUARD_ENABLED` env-var contract, split into its +// own leaf module so both `postgres-null-socket-guard.ts` (to decide whether +// to register its `uncaughtException` listener) and `sentry.ts` (to decide +// whether Sentry's own `OnUncaughtException` integration must stay active) +// can read the same value without importing each other. + +const GUARD_ENABLED_ENV_VAR = "POSTGRES_NULL_SOCKET_GUARD_ENABLED"; + +/** + * Reads the runtime opt-out. An operator sets `false` or `0` to disable the + * guard; every other value, including an unset variable, keeps it enabled. + * Follows the same boolean convention as `envBoolean` in + * `packages/db/src/client.ts`: a value other than a recognized true/false + * spelling is a configuration mistake, so this throws instead of guessing. + */ +export function isGuardEnabled(env: NodeJS.ProcessEnv): boolean { + const value = env[GUARD_ENABLED_ENV_VAR]?.trim().toLowerCase(); + if (value === undefined || value === "") return true; + if (value === "true" || value === "1") return true; + if (value === "false" || value === "0") return false; + throw new Error(`${GUARD_ENABLED_ENV_VAR} must be "true" or "false", got: ${env[GUARD_ENABLED_ENV_VAR]}`); +} diff --git a/server/src/postgres-null-socket-guard.ts b/server/src/postgres-null-socket-guard.ts index e03c843027e..14f0bb6c186 100644 --- a/server/src/postgres-null-socket-guard.ts +++ b/server/src/postgres-null-socket-guard.ts @@ -19,24 +19,32 @@ // line, with a fixed marker and the error name only — never a query, a // query parameter, a connection URL, a host name, or a database name. // -// Interaction with Sentry: this module is the sole owner of the process's -// `uncaughtException` decision. `sentry.ts` removes the default -// `OnUncaughtException` integration from `Sentry.init`, so an operator who -// installs `@sentry/node` and sets a Sentry DSN gets no second, -// independent `uncaughtException` listener — Node calls every registered -// listener for an event, not just the first one, so a second listener -// would still end the process on the known, neutralized fault regardless -// of listener order. Instead, for every uncaught exception this module -// does not neutralize, it reports the error to Sentry itself (through +// Interaction with Sentry: while this guard is enabled, it is the sole +// owner of the process's `uncaughtException` decision. `sentry.ts` reads +// the same opt-out this module reads (`postgres-null-socket-guard-env.ts`) +// and removes the default `OnUncaughtException` integration from +// `Sentry.init` only in that case, so an operator who installs +// `@sentry/node` and sets a Sentry DSN gets no second, independent +// `uncaughtException` listener — Node calls every registered listener for +// an event, not just the first one, so a second listener would still end +// the process on the known, neutralized fault regardless of listener +// order. Instead, for every uncaught exception this module does not +// neutralize, it reports the error to Sentry itself (through // `captureException`) and waits for the client to flush (through // `shutdownSentry`) before it ends the process — the same sequence // `index.ts` uses for a startup failure. A deployment with no Sentry DSN // set sees no change: both calls are no-ops. +// +// An operator who disables this guard +// (`POSTGRES_NULL_SOCKET_GUARD_ENABLED=false`) registers no listener here, +// so `sentry.ts` keeps the default `OnUncaughtException` integration active +// instead — an unrelated uncaught exception still reaches Sentry through +// that integration rather than Node's silent default handler. import { logger } from "./middleware/logger.js"; import { captureException, shutdownSentry } from "./sentry.js"; +import { isGuardEnabled } from "./postgres-null-socket-guard-env.js"; -const GUARD_ENABLED_ENV_VAR = "POSTGRES_NULL_SOCKET_GUARD_ENABLED"; const GUARD_MARKER = "postgres_null_socket_write_guard_neutralized"; // Both V8 message spellings for a read of a property on `null`. The wording @@ -63,20 +71,10 @@ export function isPostgresNullSocketWriteCrash(error: unknown): boolean { return stack.split("\n").some((line) => NEXT_WRITE_FRAME_PATTERN.test(line)); } -/** - * Reads the runtime opt-out. An operator sets `false` or `0` to disable the - * guard; every other value, including an unset variable, keeps it enabled. - * Follows the same boolean convention as `envBoolean` in - * `packages/db/src/client.ts`: a value other than a recognized true/false - * spelling is a configuration mistake, so this throws instead of guessing. - */ -export function isGuardEnabled(env: NodeJS.ProcessEnv): boolean { - const value = env[GUARD_ENABLED_ENV_VAR]?.trim().toLowerCase(); - if (value === undefined || value === "") return true; - if (value === "true" || value === "1") return true; - if (value === "false" || value === "0") return false; - throw new Error(`${GUARD_ENABLED_ENV_VAR} must be "true" or "false", got: ${env[GUARD_ENABLED_ENV_VAR]}`); -} +// Re-exported so existing callers (including tests) can keep importing the +// opt-out check from this module. See postgres-null-socket-guard-env.ts for +// why the check itself lives in a separate leaf module. +export { isGuardEnabled }; /** * Handles one uncaught exception. The known driver fault logs one diff --git a/server/src/sentry.ts b/server/src/sentry.ts index 9a520002391..2f0aba1b953 100644 --- a/server/src/sentry.ts +++ b/server/src/sentry.ts @@ -37,17 +37,26 @@ // so the existing crash-and-restart behavior stays. // // `OnUncaughtException` is a default integration too, and the initializer -// removes it. `postgres-null-socket-guard.ts` registers its own -// `uncaughtException` listener before the server does anything else, -// including this Sentry gate — the process must survive one known, -// neutralized driver defect and end for every other uncaught exception. A -// second, independent listener from this default integration would end the -// process for that known defect too, regardless of registration order, -// because Node calls every registered `uncaughtException` listener, not -// just the first one. So this repository gives one module exclusive -// ownership of the decision: `postgres-null-socket-guard.ts` calls this -// module's `captureException` itself for every uncaught exception it does -// not neutralize, before it ends the process. +// removes it — but only while `postgres-null-socket-guard.ts` registers its +// own `uncaughtException` listener (the default: see +// `postgres-null-socket-guard-env.ts` for the shared opt-out this module +// reads). That guard runs before the server does anything else, including +// this Sentry gate, and the process must survive one known, neutralized +// driver defect and end for every other uncaught exception. A second, +// independent listener from this default integration would end the process +// for that known defect too, regardless of registration order, because +// Node calls every registered `uncaughtException` listener, not just the +// first one. So while the guard is enabled, this repository gives one +// module exclusive ownership of the decision: `postgres-null-socket-guard.ts` +// calls this module's `captureException` itself for every uncaught +// exception it does not neutralize, before it ends the process. +// +// When an operator disables that guard +// (`POSTGRES_NULL_SOCKET_GUARD_ENABLED=false`), no module registers an +// `uncaughtException` listener on its behalf, so this initializer keeps the +// default `OnUncaughtException` integration active instead. Without that, +// an unrelated uncaught exception would reach no error reporter at all — +// Node's default handler exits the process without telling Sentry. // // Before it imports the package, the bootstrap checks the installed // `@sentry/node` version against the exact version this manifest's @@ -59,6 +68,7 @@ import { checkExactPeerVersions } from "./peer-version-check.js"; import { resolveSentryDsns } from "./sentry-dsn.js"; +import { isGuardEnabled } from "./postgres-null-socket-guard-env.js"; const { backend: dsn, legacyFallbackUsed } = resolveSentryDsns(); @@ -151,10 +161,19 @@ export interface SentryInitOptions { * `bootstrapSentry` so a test can call it with a real `@sentry/node` module * and assert the resolved integration list and the captured-event shape * against the true SDK, not a stand-in. + * + * `guardOwnsUncaughtException` defaults to `true` — the common case, where + * `postgres-null-socket-guard.ts` registers its own listener and this + * initializer must remove the default `OnUncaughtException` integration so + * Node does not call two independent listeners for the same event. Pass + * `false` only when that guard is disabled + * (`POSTGRES_NULL_SOCKET_GUARD_ENABLED=false`), so the default integration + * stays active and an unrelated uncaught exception still reaches Sentry. */ export function buildSentryInitOptions( dsn: string, Sentry: SentryModuleLike, + guardOwnsUncaughtException: boolean = true, ): SentryInitOptions { return { dsn, @@ -162,16 +181,16 @@ export function buildSentryInitOptions( tracesSampleRate: 0, sendDefaultPii: false, integrations: (defaults: Array<{ name: string }>) => { - const kept = defaults.filter( - (integration) => - integration.name !== "Console" && - integration.name !== "ContextLines" && - integration.name !== "Http" && - integration.name !== "OnUnhandledRejection" && - // Removed so this default integration never registers its own - // `uncaughtException` listener. See the module comment above. - integration.name !== "OnUncaughtException", - ); + const kept = defaults.filter((integration) => { + if (integration.name === "Console") return false; + if (integration.name === "ContextLines") return false; + if (integration.name === "Http") return false; + if (integration.name === "OnUnhandledRejection") return false; + // Removed only while `postgres-null-socket-guard.ts` owns the + // `uncaughtException` decision. See the module comment above. + if (integration.name === "OnUncaughtException") return !guardOwnsUncaughtException; + return true; + }); return [ ...kept, // Keep the rest of the Http integration — RequestData and request @@ -210,7 +229,13 @@ async function bootstrapSentry(dsn: string): Promise { // @ts-ignore optional peer dep const Sentry = await import("@sentry/node"); - Sentry.init(buildSentryInitOptions(dsn, Sentry)); + // Reads the same opt-out `postgres-null-socket-guard.ts` reads, so this + // module strips the default `OnUncaughtException` integration only when + // that guard actually owns the process's `uncaughtException` decision. + // `registerPostgresNullSocketGuard()` runs before this bootstrap + // settles and validates the same variable, so a startup with an + // unrecognized value never reaches this line. + Sentry.init(buildSentryInitOptions(dsn, Sentry, isGuardEnabled(process.env))); sentryHandle = { captureException: (error) => Sentry.captureException(error),