Skip to content

Commit efcec61

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 78f8a55 commit efcec61

13 files changed

Lines changed: 372 additions & 20 deletions

File tree

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ function UserPage(handle: Handle<{ id?: string }>) {
4646
);
4747
}
4848

49+
let slowStarted = false;
50+
4951
export default createController(routes, {
5052
actions: {
5153
async assets(context) {
@@ -60,5 +62,18 @@ export default createController(routes, {
6062
teapot() {
6163
return new Response("I'm a teapot", { status: 418 });
6264
},
65+
boom() {
66+
throw new Error('Route handler failed');
67+
},
68+
// Long enough for a test to disconnect mid request. `/slow-started` tells the test when the handler
69+
// is running, so the disconnect lands inside it rather than before it.
70+
async slow() {
71+
slowStarted = true;
72+
await new Promise(resolve => setTimeout(resolve, 3000));
73+
return new Response('slow');
74+
},
75+
slowStarted() {
76+
return new Response(slowStarted ? '1' : '0');
77+
},
6378
},
6479
});

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

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

‎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: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
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+
const slow = fetch(`${baseURL}/slow`, { signal: controller.signal }).catch(() => undefined);
35+
await expect.poll(async () => (await fetch(`${baseURL}/slow-started`)).text()).toBe('1');
36+
controller.abort();
37+
await slow;
38+
39+
// A later real error proves the pipeline is live, so an abort error would have arrived before it.
40+
const sentinel = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Route handler failed');
41+
await fetch(`${baseURL}/boom`);
42+
await sentinel;
43+
44+
expect(seen.filter(value => value !== 'Route handler failed')).toEqual([]);
45+
});

‎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: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
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. A numeric `status` in that range is an expected
8+
* outcome, not a fault, and the request still produces a span.
9+
*/
10+
export function defaultShouldHandleError(error: unknown): boolean {
11+
if (typeof error !== 'object' || error === null) {
12+
return true;
13+
}
14+
15+
const status = (error as { status?: unknown }).status;
16+
17+
return !(typeof status === 'number' && status >= 300 && status < 500);
18+
}
19+
20+
// Configured at `Sentry.init()`, but read by subscribers installed earlier by the `--import` entry, so
21+
// it lives in module scope rather than being captured before it exists.
22+
let shouldHandleError: ShouldHandleError = defaultShouldHandleError;
23+
24+
/** @internal Set by the integration during `Sentry.init()`. */
25+
export function setShouldHandleError(filter: ShouldHandleError | undefined): void {
26+
shouldHandleError = filter ?? defaultShouldHandleError;
27+
}
28+
29+
/**
30+
* Report an error unless it is an aborted request or filtered out.
31+
*
32+
* A router failure can arrive twice, from the middleware and again from `onError`. `captureException`
33+
* marks the object `__sentry_captured__` and drops the second report; only a thrown primitive cannot
34+
* be marked and may be reported twice.
35+
*/
36+
export function captureRequestError(error: unknown, request: Request | undefined, mechanism: string): boolean {
37+
// The app's filter can throw. Letting that out would replace the request's own error and skip the
38+
// app's `onError`, so reporting is best effort.
39+
try {
40+
if (request && isRequestAbort(error, request)) {
41+
return false;
42+
}
43+
if (!shouldHandleError(error)) {
44+
return false;
45+
}
46+
captureException(error, { mechanism: { handled: false, type: mechanism } });
47+
return true;
48+
} catch {
49+
return false;
50+
}
51+
}
52+
53+
/**
54+
* Whether a rejection is the client giving up rather than the app failing. The router rejects with
55+
* `signal.reason` when the connection drops; without this check every user navigating away mid
56+
* request creates an issue.
57+
*/
58+
export function isRequestAbort(error: unknown, request: Request): boolean {
59+
return request.signal.aborted && error === request.signal.reason;
60+
}

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

Lines changed: 59 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/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,75 @@ export function instrumentRemixV3(): void {
5258
});
5359
}
5460

61+
/**
62+
* Report errors from any fetch handler, router or not. The middleware only sees failures inside a
63+
* router; this is the only hook that covers a plain handler. No abort guard is needed, because the
64+
* listener drops aborted requests before calling `onError`.
65+
*/
66+
function subscribeToCreateRequestListener(): void {
67+
diagnosticsChannel
68+
.tracingChannel<ChannelContext, ChannelContext>(remixV3Channels.REMIX_V3_CREATE_REQUEST_LISTENER)
69+
.subscribe({
70+
start(data) {
71+
try {
72+
injectOnError(ensureOptions(data.arguments, 1));
73+
} catch {
74+
// Ignored on purpose.
75+
}
76+
},
77+
end: NOOP,
78+
asyncStart: NOOP,
79+
asyncEnd: NOOP,
80+
error: NOOP,
81+
});
82+
}
83+
84+
function injectOnError(raw: Record<string, unknown> | undefined): void {
85+
if (!raw || markInjected(raw)) {
86+
return;
87+
}
88+
89+
const options = raw as RequestListenerOptionsLike;
90+
const appOnError = options.onError;
91+
92+
options.onError = error => {
93+
captureRequestError(error, undefined, 'auto.http.remix_v3.on_error');
94+
95+
// Chained so the app keeps its own response.
96+
return appOnError?.(error);
97+
};
98+
}
99+
55100
/**
56101
* The options object, created when the caller omitted it. `undefined` when the caller passed something
57102
* that is not an options object, which must not be overwritten.
58103
*/
59-
function ensureOptions(args: unknown[]): Record<string, unknown> | undefined {
60-
const existing = args[0];
104+
function ensureOptions(args: unknown[], index = 0): Record<string, unknown> | undefined {
105+
const existing = args[index];
61106

62107
if (existing === undefined || existing === null) {
63108
const created: Record<string, unknown> = {};
64-
args[0] = created;
109+
args[index] = created;
65110
return created;
66111
}
67112

68113
return typeof existing === 'object' ? (existing as Record<string, unknown>) : undefined;
69114
}
70115

71-
function injectRouterMiddleware(raw: Record<string, unknown> | undefined): void {
72-
if (!raw) {
73-
return;
74-
}
75-
116+
/** Whether this object was already injected into, marking it when it was not. */
117+
function markInjected(raw: Record<string, unknown>): boolean {
76118
const marker = raw as { [INJECTED]?: boolean };
77119
if (marker[INJECTED]) {
78-
return;
120+
return true;
79121
}
80122
marker[INJECTED] = true;
123+
return false;
124+
}
125+
126+
function injectRouterMiddleware(raw: Record<string, unknown> | undefined): void {
127+
if (!raw || markInjected(raw)) {
128+
return;
129+
}
81130

82131
const options = raw as RouterOptionsLike;
83132

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,24 @@
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. The default skips a numeric
11+
* `status` from 300 to 499. Aborted requests never reach this.
12+
*/
13+
shouldHandleError?: ShouldHandleError;
14+
}
15+
16+
const _remixV3Integration = ((options: RemixV3IntegrationOptions = {}) => {
817
return {
918
name: INTEGRATION_NAME,
1019
setupOnce() {
20+
setShouldHandleError(options.shouldHandleError);
21+
1122
// Usually a no-op: `createRouter()` runs while the app's modules are imported, before any
1223
// `init()`, so `--import @sentry/remix/v3/node` has already subscribed. This covers setups that
1324
// register the module hook from `init()` instead.
@@ -17,6 +28,7 @@ const _remixV3Integration = (() => {
1728
}) satisfies IntegrationFn;
1829

1930
/**
20-
* Names the `http.server` spans `@sentry/node` opens after the matched Remix 3 route.
31+
* Names the `http.server` spans `@sentry/node` opens after the matched Remix 3 route, and reports the
32+
* errors the spans alone would not show.
2133
*/
2234
export const remixV3Integration = defineIntegration(_remixV3Integration);

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

Lines changed: 11 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,16 @@ 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 as well as in `onError`, so the event carries the route this middleware just put
37+
// on the scope.
38+
captureRequestError(error, context.request, 'auto.http.remix_v3.middleware');
39+
throw error;
40+
}
41+
3242
setResponseStatus(response);
3343

3444
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)