Skip to content

Commit d5cfe2f

Browse files
chargomeclaude
andcommitted
feat(remix): Complete Remix 3 server error handling
Covers the error paths the router middleware alone does not see, and stops reporting things that are not faults. `createRequestListener` in `@remix-run/node-fetch-server` is patched to install an `onError` that chains to the app's own. This is the only hook that covers an app whose fetch handler is not a router. It needs no abort guard: the listener checks `isRequestAbortError` itself and returns before calling `onError`. The middleware captures too, so an event carries the route and request already on the scope. That path does need the abort guard, because `raceRequestAbort` rejects with `signal.reason` when the client disconnects. Without it every user navigating away mid request creates an issue. Removing the guard makes the new e2e test fail. `shouldHandleError` follows `@sentry/hono`: skip 3xx and 4xx errors carrying a numeric `status`, capture the rest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 617f5b4 commit d5cfe2f

13 files changed

Lines changed: 369 additions & 20 deletions

File tree

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,5 +60,13 @@ export default createController(routes, {
6060
teapot() {
6161
return new Response("I'm a teapot", { status: 418 });
6262
},
63+
boom() {
64+
throw new Error('Route handler failed');
65+
},
66+
// Long enough for a test to disconnect mid request.
67+
async slow() {
68+
await new Promise(resolve => setTimeout(resolve, 3000));
69+
return new Response('slow');
70+
},
6371
},
6472
});

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,4 +5,6 @@ export const routes = route({
55
home: '/',
66
user: get('/users/:id'),
77
teapot: get('/teapot'),
8+
boom: get('/boom'),
9+
slow: get('/slow'),
810
});

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,13 @@ Sentry.init({
1313
const port = process.env.PORT ? Number.parseInt(process.env.PORT, 10) : 3060;
1414

1515
const server = http.createServer(
16-
createRequestListener(async request => {
17-
try {
18-
return await router.fetch(request);
19-
} catch {
20-
return new Response('Internal Server Error', { status: 500 });
16+
createRequestListener(request => {
17+
// Handled outside the router on purpose: the only hook that can see this is the listener's
18+
// `onError`, which is the case the router middleware cannot cover.
19+
if (new URL(request.url).pathname === '/plain-throw') {
20+
throw new Error('Plain handler failed');
2121
}
22+
return router.fetch(request);
2223
}),
2324
);
2425

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import { expect, test } from '@playwright/test';
2+
import { waitForError } from '@sentry-internal/test-utils';
3+
4+
const APP_NAME = 'remix-v3';
5+
6+
test('reports an error thrown in a route handler, with its route', async ({ baseURL }) => {
7+
const errorPromise = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Route handler failed');
8+
9+
await fetch(`${baseURL}/boom`);
10+
11+
const event = await errorPromise;
12+
expect(event.exception?.values?.[0]?.mechanism).toEqual({ handled: false, type: 'auto.http.remix_v3.middleware' });
13+
expect(event.transaction).toBe('GET /boom');
14+
});
15+
16+
test('reports an error from a fetch handler that is not the router', async ({ baseURL }) => {
17+
const errorPromise = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Plain handler failed');
18+
19+
const response = await fetch(`${baseURL}/plain-throw`);
20+
21+
expect(response.status).toBe(500);
22+
const event = await errorPromise;
23+
expect(event.exception?.values?.[0]?.mechanism).toEqual({ handled: false, type: 'auto.http.remix_v3.on_error' });
24+
});
25+
26+
test('does not report a request the client aborted', async ({ baseURL }) => {
27+
const seen: string[] = [];
28+
void waitForError(APP_NAME, event => {
29+
seen.push(event.exception?.values?.[0]?.value ?? '');
30+
return false;
31+
});
32+
33+
const controller = new AbortController();
34+
setTimeout(() => controller.abort(), 200);
35+
await fetch(`${baseURL}/slow`, { signal: controller.signal }).catch(() => undefined);
36+
37+
// A later real error proves the pipeline is live, so an abort error would have arrived before it.
38+
const sentinel = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Route handler failed');
39+
await fetch(`${baseURL}/boom`);
40+
await sentinel;
41+
42+
expect(seen.filter(value => value !== 'Route handler failed')).toEqual([]);
43+
});

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,5 @@ export { remixV3Integration } from './server/integration';
55
export { sentryRemixMiddleware } from './server/middleware';
66
export { instrumentRemixV3 } from './server/instrument';
77
export { instrumentAssetServer } from './assetServer';
8+
export { defaultShouldHandleError } from './server/errorFilter';
9+
export type { ShouldHandleError } from './server/errorFilter';
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import { captureException } from '@sentry/core';
2+
3+
/** Decides whether a thrown value should become a Sentry issue. */
4+
export type ShouldHandleError = (error: unknown) => boolean;
5+
6+
/**
7+
* Skip 3xx and 4xx errors, capture everything else.
8+
*
9+
* Mirrors `@sentry/hono`'s default so the two SDKs behave alike: anything carrying a numeric `status`
10+
* in the redirect or client error range is an expected outcome rather than a fault. Such requests
11+
* still produce spans, so they stay visible in tracing.
12+
*/
13+
export function defaultShouldHandleError(error: unknown): boolean {
14+
if (typeof error !== 'object' || error === null) {
15+
return true;
16+
}
17+
18+
const status = (error as { status?: unknown }).status;
19+
20+
return !(typeof status === 'number' && status >= 300 && status < 500);
21+
}
22+
23+
// The filter is configured on the integration at `Sentry.init()`, but the subscribers that read it are
24+
// installed earlier, by `--import @sentry/remix/v3/node`. Holding it in module scope lets the request
25+
// path read whatever is current rather than capture a value that does not exist yet.
26+
let shouldHandleError: ShouldHandleError = defaultShouldHandleError;
27+
28+
/** @internal Set by the integration during `Sentry.init()`. */
29+
export function setShouldHandleError(filter: ShouldHandleError | undefined): void {
30+
shouldHandleError = filter ?? defaultShouldHandleError;
31+
}
32+
33+
/**
34+
* Report an error unless it is an aborted request or filtered out.
35+
*
36+
* An app that hands `router.fetch` straight to `createRequestListener` offers a router failure here
37+
* twice, once from the Sentry middleware and again from the listener's `onError`. That needs no
38+
* de-duplication: `captureException` marks the value `__sentry_captured__` and ignores the second
39+
* report of the same object. Only a thrown primitive cannot be marked, and may be reported twice.
40+
*
41+
* @returns whether the error was captured.
42+
*/
43+
export function captureRequestError(error: unknown, request: Request | undefined, mechanism: string): boolean {
44+
if (request && isRequestAbort(error, request)) {
45+
return false;
46+
}
47+
48+
if (!shouldHandleError(error)) {
49+
return false;
50+
}
51+
52+
captureException(error, { mechanism: { handled: false, type: mechanism } });
53+
return true;
54+
}
55+
56+
/**
57+
* Whether a rejection is the client giving up rather than the app failing.
58+
*
59+
* `raceRequestAbort` in `@remix-run/fetch-router` settles every handler and middleware against the
60+
* request signal, rejecting with `signal.reason` when the connection drops. Without this check a user
61+
* navigating away mid request produces an issue, which on a busy route buries the real errors. This is
62+
* the same comparison `node-fetch-server` makes in its own `isRequestAbortError`.
63+
*/
64+
export function isRequestAbort(error: unknown, request: Request): boolean {
65+
return request.signal.aborted && error === request.signal.reason;
66+
}

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

Lines changed: 63 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import * as diagnosticsChannel from 'node:diagnostics_channel';
22
import { createMultiMatcher } from '@remix-run/route-pattern/match';
33
import { remixV3Channels } from '@sentry/server-utils/orchestrion/config';
44

5-
import type { MatcherLike, RouterOptionsLike } from '../types';
5+
import type { MatcherLike, RequestListenerOptionsLike, RouterOptionsLike } from '../types';
6+
import { captureRequestError } from './errorFilter';
67
import { sentryRemixMiddleware } from './middleware';
78

89
const NOOP = (): void => {};
@@ -34,6 +35,11 @@ export function instrumentRemixV3(): void {
3435
}
3536
subscribed = true;
3637

38+
subscribeToCreateRouter();
39+
subscribeToCreateRequestListener();
40+
}
41+
42+
function subscribeToCreateRouter(): void {
3743
diagnosticsChannel.tracingChannel<ChannelContext, ChannelContext>(remixV3Channels.REMIX_V3_CREATE_ROUTER).subscribe({
3844
start(data) {
3945
// Node rethrows anything this handler throws as an uncaught exception, which would kill an app
@@ -52,32 +58,79 @@ export function instrumentRemixV3(): void {
5258
});
5359
}
5460

61+
/**
62+
* Report errors from any fetch handler, router or not.
63+
*
64+
* The router middleware only sees failures inside a router. An app is free to hand
65+
* `createRequestListener` a plain fetch handler, and this is the only hook that covers it.
66+
*
67+
* No abort guard is needed here: `node-fetch-server` checks `isRequestAbortError` itself and returns
68+
* before calling `onError`, so a client disconnect never reaches this.
69+
*/
70+
function subscribeToCreateRequestListener(): void {
71+
diagnosticsChannel
72+
.tracingChannel<ChannelContext, ChannelContext>(remixV3Channels.REMIX_V3_CREATE_REQUEST_LISTENER)
73+
.subscribe({
74+
start(data) {
75+
try {
76+
injectOnError(ensureOptions(data.arguments, 1));
77+
} catch {
78+
// Ignored on purpose.
79+
}
80+
},
81+
end: NOOP,
82+
asyncStart: NOOP,
83+
asyncEnd: NOOP,
84+
error: NOOP,
85+
});
86+
}
87+
88+
function injectOnError(raw: Record<string, unknown> | undefined): void {
89+
if (!raw || markInjected(raw)) {
90+
return;
91+
}
92+
93+
const options = raw as RequestListenerOptionsLike;
94+
const appOnError = options.onError;
95+
96+
options.onError = error => {
97+
captureRequestError(error, undefined, 'auto.http.remix_v3.on_error');
98+
99+
// Chained so the app keeps whatever response or logging it had.
100+
return appOnError?.(error);
101+
};
102+
}
103+
55104
/**
56105
* The options object, created when the caller omitted it. `undefined` when the caller passed something
57106
* that is not an options object, which must not be overwritten.
58107
*/
59-
function ensureOptions(args: unknown[]): Record<string, unknown> | undefined {
60-
const existing = args[0];
108+
function ensureOptions(args: unknown[], index = 0): Record<string, unknown> | undefined {
109+
const existing = args[index];
61110

62111
if (existing === undefined || existing === null) {
63112
const created: Record<string, unknown> = {};
64-
args[0] = created;
113+
args[index] = created;
65114
return created;
66115
}
67116

68117
return typeof existing === 'object' ? (existing as Record<string, unknown>) : undefined;
69118
}
70119

71-
function injectRouterMiddleware(raw: Record<string, unknown> | undefined): void {
72-
if (!raw) {
73-
return;
74-
}
75-
120+
/** Whether this object was already injected into, marking it when it was not. */
121+
function markInjected(raw: Record<string, unknown>): boolean {
76122
const marker = raw as { [INJECTED]?: boolean };
77123
if (marker[INJECTED]) {
78-
return;
124+
return true;
79125
}
80126
marker[INJECTED] = true;
127+
return false;
128+
}
129+
130+
function injectRouterMiddleware(raw: Record<string, unknown> | undefined): void {
131+
if (!raw || markInjected(raw)) {
132+
return;
133+
}
81134

82135
const options = raw as RouterOptionsLike;
83136

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,26 @@
11
import { defineIntegration, type IntegrationFn } from '@sentry/core';
22

3+
import { setShouldHandleError, type ShouldHandleError } from './errorFilter';
34
import { instrumentRemixV3 } from './instrument';
45

56
const INTEGRATION_NAME = 'RemixV3' as const;
67

7-
const _remixV3Integration = (() => {
8+
interface RemixV3IntegrationOptions {
9+
/**
10+
* Decides which thrown values become issues. Return `false` to drop one.
11+
*
12+
* The default skips anything carrying a numeric `status` from 300 to 499, matching
13+
* `@sentry/hono`. Aborted requests are always dropped and never reach this.
14+
*/
15+
shouldHandleError?: ShouldHandleError;
16+
}
17+
18+
const _remixV3Integration = ((options: RemixV3IntegrationOptions = {}) => {
819
return {
920
name: INTEGRATION_NAME,
1021
setupOnce() {
22+
setShouldHandleError(options.shouldHandleError);
23+
1124
// Usually a no-op: `createRouter()` runs while the app's modules are imported, before any
1225
// `init()`, so `--import @sentry/remix/v3/node` has already subscribed. This covers setups that
1326
// register the module hook from `init()` instead.
@@ -17,6 +30,7 @@ const _remixV3Integration = (() => {
1730
}) satisfies IntegrationFn;
1831

1932
/**
20-
* Names the `http.server` spans `@sentry/node` opens after the matched Remix 3 route.
33+
* Names the `http.server` spans `@sentry/node` opens after the matched Remix 3 route, and reports the
34+
* errors the spans alone would not show.
2135
*/
2236
export const remixV3Integration = defineIntegration(_remixV3Integration);

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
} from '@sentry/core';
1212

1313
import type { MatcherLike, MiddlewareLike, NextFunctionLike, RequestContextLike } from '../types';
14+
import { captureRequestError } from './errorFilter';
1415
import { resolveRoutePattern } from './route';
1516

1617
/**
@@ -28,7 +29,17 @@ export function sentryRemixMiddleware(matcher: MatcherLike): MiddlewareLike {
2829
// Applied before `next()` so anything captured while the handler runs already carries the route.
2930
applyRoute(isolationScope, matcher, context);
3031

31-
const response = await next();
32+
let response;
33+
try {
34+
response = await next();
35+
} catch (error) {
36+
// Captured here rather than only in `onError` so the event carries the route and the request
37+
// this middleware just put on the scope. `dispatchMatches` has no try/catch of its own, so
38+
// without this the router's own failures arrive with no route attached.
39+
captureRequestError(error, context.request, 'auto.http.remix_v3.middleware');
40+
throw error;
41+
}
42+
3243
setResponseStatus(response);
3344

3445
return response;

‎packages/remix/src/v3/types.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,8 @@ export interface RouterOptionsLike {
3737
middleware?: MiddlewareLike[];
3838
matcher?: MatcherLike;
3939
}
40+
41+
/** `createRequestListener`'s options, of which only the error hook is touched. */
42+
export interface RequestListenerOptionsLike {
43+
onError?: (error: unknown) => void | Response | Promise<void | Response>;
44+
}

0 commit comments

Comments
 (0)