diff --git a/doc/observability.md b/doc/observability.md index e730ad21201..17e9c251de4 100644 --- a/doc/observability.md +++ b/doc/observability.md @@ -382,13 +382,17 @@ integration list of `@sentry/node@10.71.0` and `@sentry/browser@10.71.0`. **Server events the default integrations add** -- `OnUncaughtException` — each uncaught exception on the main thread, at - level `fatal`. The process still exits. - `OnUnhandledRejection` — each unhandled promise rejection. The mode is `strict`, so the process exits after the capture. - `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. + **Server context the kept integrations attach** - `RequestData` — attaches nothing under this feature's configuration. See @@ -409,6 +413,9 @@ integration list of `@sentry/node@10.71.0` and `@sentry/browser@10.71.0`. - `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. **Browser events this feature adds** @@ -450,6 +457,64 @@ Two controls belong to the operator. This feature ships neither one. an internal-only host name fails silently for the browser even when it works for the server. +## PostgreSQL Null-Socket Write Guard + +The `postgres` driver, at the version this repository pins +(`postgres@3.4.9`), has a known defect. When a database backend closes a +connection that a transaction still holds, a later query on that same +connection can still start. The driver defers that query's wire write to a +timer callback. By the time the callback runs, the driver has already set +its socket reference to `null`, and the write throws a `TypeError`. No +application code sits between that timer callback and the process, so +Node ends the process by default. + +The server adds one process-level guard against this exact fault. The +guard does not patch the driver and does not change the driver version. +It adds one `uncaughtException` listener, early in server startup, before +every other startup step. For the one known, matching `TypeError`, the +listener logs one structured line with a fixed marker +(`postgres_null_socket_write_guard_neutralized`) and the error name, then +lets the process keep running. For every other uncaught exception, the +listener keeps today's default behavior: it logs the error, reports it to +Sentry when Sentry is enabled (see "Sentry Error Monitoring" above), waits +for that report to flush, then ends the process with exit code 1. + +### `POSTGRES_NULL_SOCKET_GUARD_ENABLED` + +This environment variable is the guard's one operator control. + +- **Default:** the guard is on. An unset variable, or an empty string, + means enabled. +- **Accepted values:** `true` or `1` keep the guard on. `false` or `0` + turn it off. The comparison ignores letter case and leading or trailing + spaces. +- **An unrecognized value stops the server at startup.** A value other + than the six listed above (for example `yes` or `disabled`) throws an + error that names the variable and the value the operator set, instead + of silently guessing what the operator meant. +- **Operational effect of turning the guard off:** the server registers + 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. + +### 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. + ## Sandbox Startup Trace Spans Paperclip opens OpenTelemetry spans on the sandbox start path. These spans are diff --git a/server/src/__tests__/postgres-null-socket-guard.test.ts b/server/src/__tests__/postgres-null-socket-guard.test.ts index e3df1d87dc3..5e7b1b79cee 100644 --- a/server/src/__tests__/postgres-null-socket-guard.test.ts +++ b/server/src/__tests__/postgres-null-socket-guard.test.ts @@ -5,6 +5,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const mockLoggerError = vi.hoisted(() => vi.fn()); const mockLoggerFatal = vi.hoisted(() => vi.fn()); +const mockCaptureException = vi.hoisted(() => vi.fn()); +const mockShutdownSentry = vi.hoisted(() => vi.fn(async () => {})); vi.mock("../middleware/logger.js", () => ({ logger: { @@ -13,6 +15,11 @@ vi.mock("../middleware/logger.js", () => ({ }, })); +vi.mock("../sentry.js", () => ({ + captureException: mockCaptureException, + shutdownSentry: mockShutdownSentry, +})); + const { handlePostgresNullSocketGuardException, isGuardEnabled, @@ -167,6 +174,9 @@ describe("handlePostgresNullSocketGuardException", () => { beforeEach(() => { mockLoggerError.mockReset(); mockLoggerFatal.mockReset(); + mockCaptureException.mockReset(); + mockShutdownSentry.mockReset(); + mockShutdownSentry.mockImplementation(async () => {}); exitSpy = vi.spyOn(process, "exit").mockImplementation((() => undefined) as unknown as never); }); @@ -174,10 +184,10 @@ describe("handlePostgresNullSocketGuardException", () => { exitSpy.mockRestore(); }); - it("logs a stable marker and the error name, and does not exit, on a matching crash", () => { + it("logs a stable marker and the error name, and does not exit, on a matching crash", async () => { const error = makeDriverCrashError("Cannot read properties of null (reading 'write')"); - handlePostgresNullSocketGuardException(error); + await handlePostgresNullSocketGuardException(error); expect(exitSpy).not.toHaveBeenCalled(); expect(mockLoggerError).toHaveBeenCalledTimes(1); @@ -188,7 +198,7 @@ describe("handlePostgresNullSocketGuardException", () => { }); }); - it("never logs the query text, the message, or the stack for a matching crash", () => { + it("never logs the query text, the message, or the stack for a matching crash", async () => { const secret = "select * from accounts where token = 'hunter2-secret-token'"; const error = makeDriverCrashError(`Cannot read properties of null (reading 'write') — ${secret}`); // The classifier only matches the exact V8 message, so force a match by @@ -197,7 +207,7 @@ describe("handlePostgresNullSocketGuardException", () => { // and the fixed error name. error.message = "Cannot read properties of null (reading 'write')"; - handlePostgresNullSocketGuardException(error); + await handlePostgresNullSocketGuardException(error); const [fields] = mockLoggerError.mock.calls[0] as [Record]; expect(Object.keys(fields).sort()).toEqual(["errorName", "marker"]); @@ -206,19 +216,48 @@ describe("handlePostgresNullSocketGuardException", () => { } }); - it("logs and exits non-zero for a non-matching error", () => { + it("never reports a matching crash to Sentry — the process survives on its own", async () => { + const error = makeDriverCrashError("Cannot read properties of null (reading 'write')"); + + await handlePostgresNullSocketGuardException(error); + + expect(mockCaptureException).not.toHaveBeenCalled(); + expect(mockShutdownSentry).not.toHaveBeenCalled(); + }); + + it("logs, reports to Sentry, flushes, and exits non-zero for a non-matching error", async () => { const error = new RangeError("some unrelated fatal condition"); - handlePostgresNullSocketGuardException(error); + await handlePostgresNullSocketGuardException(error); expect(mockLoggerError).not.toHaveBeenCalled(); expect(mockLoggerFatal).toHaveBeenCalledTimes(1); + expect(mockCaptureException).toHaveBeenCalledWith(error); + expect(mockShutdownSentry).toHaveBeenCalledTimes(1); expect(exitSpy).toHaveBeenCalledWith(1); }); - it("exits non-zero for a non-Error thrown value", () => { - handlePostgresNullSocketGuardException("a thrown string"); + it("waits for the Sentry client to flush before it exits", async () => { + const error = new RangeError("some unrelated fatal condition"); + let shutdownResolved = false; + mockShutdownSentry.mockImplementation(async () => { + await Promise.resolve(); + shutdownResolved = true; + }); + exitSpy.mockImplementation((() => { + expect(shutdownResolved).toBe(true); + return undefined; + }) as unknown as never); + await handlePostgresNullSocketGuardException(error); + + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it("exits non-zero for a non-Error thrown value", async () => { + await handlePostgresNullSocketGuardException("a thrown string"); + + expect(mockCaptureException).toHaveBeenCalledWith(expect.any(Error)); expect(exitSpy).toHaveBeenCalledWith(1); }); }); diff --git a/server/src/__tests__/sentry.test.ts b/server/src/__tests__/sentry.test.ts index 60edd8fc41f..550a830612c 100644 --- a/server/src/__tests__/sentry.test.ts +++ b/server/src/__tests__/sentry.test.ts @@ -403,7 +403,7 @@ describe("buildSentryInitOptions", () => { expect(resolved.filter((i) => i.name === "OnUnhandledRejection")).toHaveLength(1); }); - it("the resolved server integration list holds no Console integration and no ContextLines integration", async () => { + it("the resolved server integration list holds no Console, ContextLines, or OnUncaughtException integration", async () => { const { buildSentryInitOptions } = await importFreshSentry(); const options = buildSentryInitOptions("https://public@o0.ingest.sentry.io/1", { @@ -415,9 +415,14 @@ describe("buildSentryInitOptions", () => { expect(names).not.toContain("Console"); expect(names).not.toContain("ContextLines"); + // Removed so this default integration never registers its own + // `uncaughtException` listener — see the module comment in sentry.ts. + // `postgres-null-socket-guard.ts` is the sole owner of that decision, + // and it reports non-neutralized exceptions to Sentry itself. + expect(names).not.toContain("OnUncaughtException"); }); - it("the resolved server integration list keeps OnUncaughtException, OnUnhandledRejection, LinkedErrors, and RequestData", async () => { + it("the resolved server integration list keeps OnUnhandledRejection, LinkedErrors, and RequestData", async () => { const { buildSentryInitOptions } = await importFreshSentry(); const options = buildSentryInitOptions("https://public@o0.ingest.sentry.io/1", { @@ -427,11 +432,10 @@ describe("buildSentryInitOptions", () => { const resolved = options.integrations(DEFAULT_INTEGRATION_NAMES.map((name) => ({ name }))); const names = resolved.map((i) => i.name); - // The full error-capture set the plan requires, not just the four named + // The full error-capture set the plan requires, not just the three named // in this test's title. expect(names).toEqual( expect.arrayContaining([ - "OnUncaughtException", "OnUnhandledRejection", "ChildProcess", "LinkedErrors", @@ -565,6 +569,23 @@ describe.skipIf(!sentryPackage)("captured event shape against the real @sentry/n Sentry.init(options); } + /** + * Proves the fix for the finding that this feature could keep a + * Sentry-enabled deployment from surviving the known, neutralized + * postgres driver crash: `Sentry.init` must add no `uncaughtException` + * listener of its own. `postgres-null-socket-guard.ts` is the sole owner + * of that event; a second, independent listener from Sentry's default + * `OnUncaughtException` integration would still end the process on the + * known fault, no matter which listener Node calls first. + */ + it("adds no uncaughtException listener", async () => { + const before = process.listenerCount("uncaughtException"); + + await initRealSentryForTest(() => {}); + + expect(process.listenerCount("uncaughtException")).toBe(before); + }); + it("a server event captured after a console.error call carries no console breadcrumb", async () => { const Sentry = sentryPackage!; let captured: Record | null = null; diff --git a/server/src/index.ts b/server/src/index.ts index 415ef0b39e5..b76c0cd871a 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -156,9 +156,10 @@ export interface StartedServer { } export async function startServer(): Promise { - // Registered before every other startup step, including Sentry, so this - // listener runs first on any later uncaught exception — see - // postgres-null-socket-guard.ts for why that order matters. + // Registered before every other startup step, so it is the process's + // `uncaughtException` handler for the entire startup sequence, not just + // once the server is listening — see postgres-null-socket-guard.ts for + // how it owns that decision, Sentry reporting included. registerPostgresNullSocketGuard(); warnIfUnsupportedNodeVersion(process.versions.node, (message) => logger.warn(message)); diff --git a/server/src/postgres-null-socket-guard.ts b/server/src/postgres-null-socket-guard.ts index 3822d05bd9a..e03c843027e 100644 --- a/server/src/postgres-null-socket-guard.ts +++ b/server/src/postgres-null-socket-guard.ts @@ -14,22 +14,27 @@ // the one known, neutralized fault below — otherwise an unrelated bug would // leave the process running in a state nobody chose. // -// This module reports no telemetry event and calls no external -// error-reporting client. It logs one structured 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. +// For the known, neutralized fault, this module reports no telemetry event +// and calls no external error-reporting client. It logs one structured +// 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: `./sentry.js` registers no `uncaughtException` -// listener today. `@sentry/node` is an optional peer dependency this -// repository does not install (`bootstrapSentry` fails its own version -// check and returns before it calls `Sentry.init`), so Sentry never adds a -// listener of its own. If an operator later installs `@sentry/node` and -// sets a Sentry DSN, the default `OnUncaughtException` integration is not -// among the integrations `sentry.ts` removes, so it would register its own -// listener too. This module registers first (from the server entry point, -// before `sentryReady`), so it runs first on every uncaught exception. +// 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 +// `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. import { logger } from "./middleware/logger.js"; +import { captureException, shutdownSentry } from "./sentry.js"; const GUARD_ENABLED_ENV_VAR = "POSTGRES_NULL_SOCKET_GUARD_ENABLED"; const GUARD_MARKER = "postgres_null_socket_write_guard_neutralized"; @@ -75,12 +80,12 @@ export function isGuardEnabled(env: NodeJS.ProcessEnv): boolean { /** * Handles one uncaught exception. The known driver fault logs one - * structured, searchable line and returns — the process survives. Every - * other error keeps today's default behavior: log, then end the process. - * Exported so a test can drive it directly instead of dispatching a real - * process event. + * structured, searchable line and returns — the process survives, and + * Sentry hears nothing about it. Every other error keeps today's default + * behavior: log, report to Sentry, then end the process. Exported so a + * test can drive it directly instead of dispatching a real process event. */ -export function handlePostgresNullSocketGuardException(error: unknown): void { +export async function handlePostgresNullSocketGuardException(error: unknown): Promise { if (isPostgresNullSocketWriteCrash(error)) { logger.error( { marker: GUARD_MARKER, errorName: (error as Error).name }, @@ -91,6 +96,8 @@ export function handlePostgresNullSocketGuardException(error: unknown): void { const rootError = error instanceof Error ? error : new Error(String(error)); logger.fatal({ err: rootError }, "uncaught exception; process exiting"); + captureException(rootError); + await shutdownSentry(); process.exit(1); } diff --git a/server/src/sentry.ts b/server/src/sentry.ts index 8cac6728c2c..9a520002391 100644 --- a/server/src/sentry.ts +++ b/server/src/sentry.ts @@ -36,6 +36,19 @@ // `mode: "strict"`: Sentry still captures the event, then exits the process, // 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. +// // Before it imports the package, the bootstrap checks the installed // `@sentry/node` version against the exact version this manifest's // `peerDependencies` declares — the same audited version documented in @@ -154,7 +167,10 @@ export function buildSentryInitOptions( integration.name !== "Console" && integration.name !== "ContextLines" && integration.name !== "Http" && - integration.name !== "OnUnhandledRejection", + integration.name !== "OnUnhandledRejection" && + // Removed so this default integration never registers its own + // `uncaughtException` listener. See the module comment above. + integration.name !== "OnUncaughtException", ); return [ ...kept,