feat(remix): Complete Remix 3 server error handling

dev-packages/e2e-tests/test-applications/remix-v3/app/actions/controller.tsx

@@ -46,6 +46,8 @@ function UserPage(handle: Handle<{ id?: string }>) {
   );
 }
 
+let slowStarted = false;
+
 export default createController(routes, {
   actions: {
     async assets(context) {
@@ -60,5 +62,18 @@ export default createController(routes, {
     teapot() {
       return new Response("I'm a teapot", { status: 418 });
     },
+    boom() {
+      throw new Error('Route handler failed');
+    },
+    // Long enough for a test to disconnect mid request. `/slow-started` tells the test when the handler
+    // is running, so the disconnect lands inside it rather than before it.
+    async slow() {
+      slowStarted = true;
+      await new Promise(resolve => setTimeout(resolve, 3000));
+      return new Response('slow');
+    },
+    slowStarted() {
+      return new Response(slowStarted ? '1' : '0');
+    },
   },
 });

dev-packages/e2e-tests/test-applications/remix-v3/app/routes.ts

@@ -5,4 +5,7 @@ export const routes = route({
   home: '/',
   user: get('/users/:id'),
   teapot: get('/teapot'),
+  boom: get('/boom'),
+  slow: get('/slow'),
+  slowStarted: get('/slow-started'),
 });

dev-packages/e2e-tests/test-applications/remix-v3/server.ts

@@ -13,12 +13,13 @@ Sentry.init({
 const port = process.env.PORT ? Number.parseInt(process.env.PORT, 10) : 3060;
 
 const server = http.createServer(
-  createRequestListener(async request => {
-    try {
-      return await router.fetch(request);
-    } catch {
-      return new Response('Internal Server Error', { status: 500 });
+  createRequestListener(request => {
+    // Handled outside the router on purpose: the only hook that can see this is the listener's
+    // `onError`, which is the case the router middleware cannot cover.
+    if (new URL(request.url).pathname === '/plain-throw') {
+      throw new Error('Plain handler failed');
     }
+    return router.fetch(request);
   }),
 );
 

dev-packages/e2e-tests/test-applications/remix-v3/tests/server-errors.test.ts

@@ -0,0 +1,45 @@
+import { expect, test } from '@playwright/test';
+import { waitForError } from '@sentry-internal/test-utils';
+
+const APP_NAME = 'remix-v3';
+
+test('reports an error thrown in a route handler, with its route', async ({ baseURL }) => {
+  const errorPromise = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Route handler failed');
+
+  await fetch(`${baseURL}/boom`);
+
+  const event = await errorPromise;
+  expect(event.exception?.values?.[0]?.mechanism).toEqual({ handled: false, type: 'auto.http.remix_v3.middleware' });
+  expect(event.transaction).toBe('GET /boom');
+});
+
+test('reports an error from a fetch handler that is not the router', async ({ baseURL }) => {
+  const errorPromise = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Plain handler failed');
+
+  const response = await fetch(`${baseURL}/plain-throw`);
+
+  expect(response.status).toBe(500);
+  const event = await errorPromise;
+  expect(event.exception?.values?.[0]?.mechanism).toEqual({ handled: false, type: 'auto.http.remix_v3.on_error' });
+});
+
+test('does not report a request the client aborted', async ({ baseURL }) => {
+  const seen: string[] = [];
+  void waitForError(APP_NAME, event => {
+    seen.push(event.exception?.values?.[0]?.value ?? '');
+    return false;
+  });
+
+  const controller = new AbortController();
+  const slow = fetch(`${baseURL}/slow`, { signal: controller.signal }).catch(() => undefined);
+  await expect.poll(async () => (await fetch(`${baseURL}/slow-started`)).text()).toBe('1');
+  controller.abort();
+  await slow;
+
+  // A later real error proves the pipeline is live, so an abort error would have arrived before it.
+  const sentinel = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Route handler failed');
+  await fetch(`${baseURL}/boom`);
+  await sentinel;
+
+  expect(seen.filter(value => value !== 'Route handler failed')).toEqual([]);
+});

packages/remix/src/v3/index.server.ts

@@ -5,3 +5,5 @@ export { remixV3Integration } from './server/integration';
 export { sentryRemixMiddleware } from './server/middleware';
 export { instrumentRemixV3 } from './server/instrument';
 export { instrumentAssetServer } from './assetServer';
+export { defaultShouldHandleError } from './server/errorFilter';
+export type { ShouldHandleError } from './server/errorFilter';

packages/remix/src/v3/server/errorFilter.ts

@@ -0,0 +1,60 @@
+import { captureException } from '@sentry/core';
+
+/** Decides whether a thrown value should become a Sentry issue. */
+export type ShouldHandleError = (error: unknown) => boolean;
+
+/**
+ * Skip 3xx and 4xx errors, capture everything else. A numeric `status` in that range is an expected
+ * outcome, not a fault, and the request still produces a span.
+ */
+export function defaultShouldHandleError(error: unknown): boolean {
+  if (typeof error !== 'object' || error === null) {
+    return true;
+  }
+
+  const status = (error as { status?: unknown }).status;
+
+  return !(typeof status === 'number' && status >= 300 && status < 500);
+}
+
+// Configured at `Sentry.init()`, but read by subscribers installed earlier by the `--import` entry, so
+// it lives in module scope rather than being captured before it exists.
+let shouldHandleError: ShouldHandleError = defaultShouldHandleError;
+
+/** @internal Set by the integration during `Sentry.init()`. */
+export function setShouldHandleError(filter: ShouldHandleError | undefined): void {
+  shouldHandleError = filter ?? defaultShouldHandleError;
+}
+
+/**
+ * Report an error unless it is an aborted request or filtered out.
+ *
+ * A router failure can arrive twice, from the middleware and again from `onError`. `captureException`
+ * marks the object `__sentry_captured__` and drops the second report; only a thrown primitive cannot
+ * be marked and may be reported twice.
+ */
+export function captureRequestError(error: unknown, request: Request | undefined, mechanism: string): boolean {
+  // The app's filter can throw. Letting that out would replace the request's own error and skip the
+  // app's `onError`, so reporting is best effort.
+  try {
+    if (request && isRequestAbort(error, request)) {
+      return false;
+    }
+    if (!shouldHandleError(error)) {
+      return false;
+    }
+    captureException(error, { mechanism: { handled: false, type: mechanism } });
+    return true;
+  } catch {
+    return false;
+  }
+}
+
+/**
+ * Whether a rejection is the client giving up rather than the app failing. The router rejects with
+ * `signal.reason` when the connection drops; without this check every user navigating away mid
+ * request creates an issue.
+ */
+export function isRequestAbort(error: unknown, request: Request): boolean {
+  return request.signal.aborted && error === request.signal.reason;
+}

packages/remix/src/v3/server/instrument.ts

@@ -2,7 +2,8 @@ import * as diagnosticsChannel from 'node:diagnostics_channel';
 import { createMultiMatcher } from 'remix/route-pattern/match';
 import { remixV3Channels } from '@sentry/server-utils/orchestrion/config';
 
-import type { MatcherLike, RouterOptionsLike } from '../types';
+import type { MatcherLike, RequestListenerOptionsLike, RouterOptionsLike } from '../types';
+import { captureRequestError } from './errorFilter';
 import { sentryRemixMiddleware } from './middleware';
 
 const NOOP = (): void => {};
@@ -34,6 +35,11 @@ export function instrumentRemixV3(): void {
   }
   subscribed = true;
 
+  subscribeToCreateRouter();
+  subscribeToCreateRequestListener();
+}
+
+function subscribeToCreateRouter(): void {
   diagnosticsChannel.tracingChannel<ChannelContext, ChannelContext>(remixV3Channels.REMIX_V3_CREATE_ROUTER).subscribe({
     start(data) {
       // Node rethrows anything this handler throws as an uncaught exception, which would kill an app
@@ -52,32 +58,82 @@ export function instrumentRemixV3(): void {
   });
 }
 
+/**
+ * Report errors from any fetch handler, router or not. The middleware only sees failures inside a
+ * router; this is the only hook that covers a plain handler. No abort guard is needed, because the
+ * listener drops aborted requests before calling `onError`.
+ */
+function subscribeToCreateRequestListener(): void {
+  diagnosticsChannel
+    .tracingChannel<ChannelContext, ChannelContext>(remixV3Channels.REMIX_V3_CREATE_REQUEST_LISTENER)
+    .subscribe({
+      start(data) {
+        try {
+          injectOnError(ensureOptions(data.arguments, 1));
+        } catch {
+          // Ignored on purpose.
+        }
+      },
+      end: NOOP,
+      asyncStart: NOOP,
+      asyncEnd: NOOP,
+      error: NOOP,
+    });
+}
+
+function injectOnError(raw: Record<string, unknown> | undefined): void {
+  if (!raw || markInjected(raw)) {
+    return;
+  }
+
+  const options = raw as RequestListenerOptionsLike;
+  const appOnError = options.onError;
+
+  options.onError = error => {
+    captureRequestError(error, undefined, 'auto.http.remix_v3.on_error');
+
+    if (appOnError) {
+      // Chained so the app keeps its own response.
+      return appOnError(error);
+    }
+
+    // Setting `onError` replaced the listener's default handler, which logs the error.
+    // oxlint-disable-next-line no-console
+    console.error(error);
+    return undefined;
+  };
+}
+
 /**
  * The options object, created when the caller omitted it. `undefined` when the caller passed something
  * that is not an options object, which must not be overwritten.
  */
-function ensureOptions(args: unknown[]): Record<string, unknown> | undefined {
-  const existing = args[0];
+function ensureOptions(args: unknown[], index = 0): Record<string, unknown> | undefined {
+  const existing = args[index];
 
   if (existing === undefined || existing === null) {
     const created: Record<string, unknown> = {};
-    args[0] = created;
+    args[index] = created;
     return created;
   }
 
   return typeof existing === 'object' ? (existing as Record<string, unknown>) : undefined;
 }
 
-function injectRouterMiddleware(raw: Record<string, unknown> | undefined): void {
-  if (!raw) {
-    return;
-  }
-
+/** Whether this object was already injected into, marking it when it was not. */
+function markInjected(raw: Record<string, unknown>): boolean {
   const marker = raw as { [INJECTED]?: boolean };
   if (marker[INJECTED]) {
-    return;
+    return true;
   }
   marker[INJECTED] = true;
+  return false;
+}
+
+function injectRouterMiddleware(raw: Record<string, unknown> | undefined): void {
+  if (!raw || markInjected(raw)) {
+    return;
+  }
 
   const options = raw as RouterOptionsLike;
 

packages/remix/src/v3/server/integration.ts

@@ -1,13 +1,24 @@
 import { defineIntegration, type IntegrationFn } from '@sentry/core';
 
+import { setShouldHandleError, type ShouldHandleError } from './errorFilter';
 import { instrumentRemixV3 } from './instrument';
 
 const INTEGRATION_NAME = 'RemixV3' as const;
 
-const _remixV3Integration = (() => {
+interface RemixV3IntegrationOptions {
+  /**
+   * Decides which thrown values become issues. Return `false` to drop one. The default skips a numeric
+   * `status` from 300 to 499. Aborted requests never reach this.
+   */
+  shouldHandleError?: ShouldHandleError;
+}
+
+const _remixV3Integration = ((options: RemixV3IntegrationOptions = {}) => {
   return {
     name: INTEGRATION_NAME,
     setupOnce() {
+      setShouldHandleError(options.shouldHandleError);
+
       // Usually a no-op: `createRouter()` runs while the app's modules are imported, before any
       // `init()`, so `--import @sentry/remix/v3/node` has already subscribed. This covers setups that
       // register the module hook from `init()` instead.
@@ -17,6 +28,7 @@ const _remixV3Integration = (() => {
 }) satisfies IntegrationFn;
 
 /**
- * Names the `http.server` spans `@sentry/node` opens after the matched Remix 3 route.
+ * Names the `http.server` spans `@sentry/node` opens after the matched Remix 3 route, and reports the
+ * errors the spans alone would not show.
  */
 export const remixV3Integration = defineIntegration(_remixV3Integration);

packages/remix/src/v3/server/middleware.ts

@@ -11,6 +11,7 @@ import {
 } from '@sentry/core';
 
 import type { MatcherLike, MiddlewareLike, NextFunctionLike, RequestContextLike } from '../types';
+import { captureRequestError } from './errorFilter';
 import { resolveRoutePattern } from './route';
 
 /**
@@ -28,7 +29,16 @@ export function sentryRemixMiddleware(matcher: MatcherLike): MiddlewareLike {
     // Applied before `next()` so anything captured while the handler runs already carries the route.
     applyRoute(isolationScope, matcher, context);
 
-    const response = await next();
+    let response;
+    try {
+      response = await next();
+    } catch (error) {
+      // Captured here as well as in `onError`, so the event carries the route this middleware just put
+      // on the scope.
+      captureRequestError(error, context.request, 'auto.http.remix_v3.middleware');
+      throw error;
+    }
+
     setResponseStatus(response);
 
     return response;

packages/remix/src/v3/types.ts

@@ -37,3 +37,8 @@ export interface RouterOptionsLike {
   middleware?: MiddlewareLike[];
   matcher?: MatcherLike;
 }
+
+/** `createRequestListener`'s options, of which only the error hook is touched. */
+export interface RequestListenerOptionsLike {
+  onError?: (error: unknown) => void | Response | Promise<void | Response>;
+}