From 360b58ef02c949260257a1e9c713e4bfb54875c2 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 12 Oct 2026 08:30:32 +0000 Subject: [PATCH] fix(v10/server-utils): End Express layer spans when the client aborts Backport of: #25209 --- .../express/client-abort/instrument.mjs | 9 ++++++ .../suites/express/client-abort/scenario.mjs | 12 ++++++++ .../suites/express/client-abort/test.ts | 30 +++++++++++++++++++ .../utils/runner/createRunner.ts | 16 ++++++++-- .../src/integrations/express/patch-layer.ts | 24 +++++++++++++-- .../express/instrumentation.ts | 19 ++++++++++-- .../tracing-channel/express/types.ts | 2 +- 7 files changed, 103 insertions(+), 9 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/client-abort/instrument.mjs create mode 100644 dev-packages/node-integration-tests/suites/express/client-abort/scenario.mjs create mode 100644 dev-packages/node-integration-tests/suites/express/client-abort/test.ts diff --git a/dev-packages/node-integration-tests/suites/express/client-abort/instrument.mjs b/dev-packages/node-integration-tests/suites/express/client-abort/instrument.mjs new file mode 100644 index 000000000000..46a27dd03b74 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/client-abort/instrument.mjs @@ -0,0 +1,9 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, +}); diff --git a/dev-packages/node-integration-tests/suites/express/client-abort/scenario.mjs b/dev-packages/node-integration-tests/suites/express/client-abort/scenario.mjs new file mode 100644 index 000000000000..8461cf662cb5 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/client-abort/scenario.mjs @@ -0,0 +1,12 @@ +import * as Sentry from '@sentry/node'; +import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; +import express from 'express'; + +const app = express(); + +app.get('/test/slow', async (_req, res) => { + await Sentry.startSpan({ name: 'slow-work', op: 'function' }, () => new Promise(resolve => setTimeout(resolve, 500))); + res.send('ok'); +}); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/client-abort/test.ts b/dev-packages/node-integration-tests/suites/express/client-abort/test.ts new file mode 100644 index 000000000000..87d7d3efcc0d --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/client-abort/test.ts @@ -0,0 +1,30 @@ +import { afterAll, describe, expect } from 'vitest'; +import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; + +describe('express client abort', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + test('ends the request handler span when the client aborts', async () => { + const runner = createRunner() + .expect({ + transaction: transaction => { + const handlerSpan = transaction.spans?.find(span => span.data?.['express.type'] === 'request_handler'); + + expect(transaction.transaction).toBe('GET /test/slow'); + expect(handlerSpan).toBeDefined(); + expect(handlerSpan!.parent_span_id).toBe(transaction.contexts?.trace?.span_id); + expect(handlerSpan!.status).toBe('cancelled'); + // Ends when the client aborts, not when the handler's 500ms of work finishes. + expect(handlerSpan!.timestamp! - handlerSpan!.start_timestamp).toBeLessThan(0.4); + }, + }) + .start(); + + await runner.makeRequest('get', '/test/slow', { timeout: 100, expectError: true }); + await runner.completed(); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/utils/runner/createRunner.ts b/dev-packages/node-integration-tests/utils/runner/createRunner.ts index 069de2c26249..455818bd88c4 100644 --- a/dev-packages/node-integration-tests/utils/runner/createRunner.ts +++ b/dev-packages/node-integration-tests/utils/runner/createRunner.ts @@ -94,7 +94,7 @@ type StartResult = { makeRequest( method: 'get' | 'post' | 'put' | 'delete' | 'patch', path: string, - options?: { headers?: Record; data?: BodyInit; expectError?: boolean }, + options?: { headers?: Record; data?: BodyInit; expectError?: boolean; timeout?: number }, ): Promise; }; @@ -565,7 +565,12 @@ export function createRunner(...paths: string[]) { makeRequest: async function ( method: 'get' | 'post' | 'put' | 'delete' | 'patch', path: string, - options: { headers?: Record; data?: BodyInit; expectError?: boolean } = {}, + options: { + headers?: Record; + data?: BodyInit; + expectError?: boolean; + timeout?: number; + } = {}, ): Promise { try { await waitForEvent(portReady.promise, 10_000, 'Timed out waiting for server port'); @@ -582,7 +587,12 @@ export function createRunner(...paths: string[]) { if (process.env.DEBUG) log('making request', method, url, headers, body); try { - const res = await fetch(url, { headers, method: method.toUpperCase(), body }); + const res = await fetch(url, { + headers, + method: method.toUpperCase(), + body, + signal: options.timeout !== undefined ? AbortSignal.timeout(options.timeout) : undefined, + }); if (!res.ok) { if (!expectError) { diff --git a/packages/core/src/integrations/express/patch-layer.ts b/packages/core/src/integrations/express/patch-layer.ts index 6a026afa67f8..a043b970e9be 100644 --- a/packages/core/src/integrations/express/patch-layer.ts +++ b/packages/core/src/integrations/express/patch-layer.ts @@ -161,10 +161,26 @@ export function patchLayer( span.end(); spanHasEnded = true; } - // listener for response.on('finish') + // listeners for response.on('finish') and response.on('close') + const removeResponseListeners = () => { + res.removeListener('finish', onResponseFinish); + res.removeListener('close', onResponseClose); + }; const onResponseFinish = () => { + removeResponseListeners(); + if (!spanHasEnded) { + spanHasEnded = true; + span.end(); + } + }; + // `finish` never fires when the client aborts, so also end on `close`, + // which is when the `http.server` span ends. Otherwise the span and its + // subtree would be dropped from the transaction. + const onResponseClose = () => { + removeResponseListeners(); if (!spanHasEnded) { spanHasEnded = true; + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'cancelled' }); span.end(); } }; @@ -197,7 +213,7 @@ export function patchLayer( if (!spanHasEnded) { spanHasEnded = true; - res.removeListener('finish', onResponseFinish); + removeResponseListeners(); span.end(); } if (!(req.route && isError) && isLayerPathStored) { @@ -230,9 +246,11 @@ export function patchLayer( // either the layer is asynchronous (so it will call the // callback later on) or that the layer directly ends the // http response, so we'll hook into the "finish" event to - // handle the later case. + // handle the later case. We also hook into "close", which fires + // instead of "finish" when the client aborts the request. if (!spanHasEnded) { res.once('finish', onResponseFinish); + res.once('close', onResponseClose); } } }); diff --git a/packages/server-utils/src/integrations/tracing-channel/express/instrumentation.ts b/packages/server-utils/src/integrations/tracing-channel/express/instrumentation.ts index 19db7a815d14..a8efb7c86d53 100644 --- a/packages/server-utils/src/integrations/tracing-channel/express/instrumentation.ts +++ b/packages/server-utils/src/integrations/tracing-channel/express/instrumentation.ts @@ -9,6 +9,7 @@ import { getRootSpan, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SPAN_STATUS_ERROR, spanToJSON, startInactiveSpan, stringMatchesSomePattern, @@ -230,13 +231,27 @@ function getSpanForLayer(data: HandleChannelContext, options: ExpressIntegration // A layer that sends the response (route handlers, typically) never calls // `next`, so the channel's `asyncEnd` never fires. End on the response's // `finish` in that case. When `next` *is* called, the helper ends the span - // (via `asyncEnd`) and `beforeSpanEnd` removes this now-redundant listener. + // (via `asyncEnd`) and `beforeSpanEnd` removes these now-redundant listeners. + // `finish` never fires when the client aborts, so also end on `close`, which + // is when the `http.server` span ends. Otherwise the span and its subtree + // would be dropped from the transaction. if (res && typeof res.once === 'function') { + const cleanup = (): void => { + res.removeListener('finish', onFinish); + res.removeListener('close', onClose); + }; const onFinish = (): void => { + cleanup(); + span.end(); + }; + const onClose = (): void => { + cleanup(); + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'cancelled' }); span.end(); }; res.once('finish', onFinish); - data._sentryCleanup = () => res.removeListener('finish', onFinish); + res.once('close', onClose); + data._sentryCleanup = cleanup; } return span; diff --git a/packages/server-utils/src/integrations/tracing-channel/express/types.ts b/packages/server-utils/src/integrations/tracing-channel/express/types.ts index 4f9241404578..a272cebb05bc 100644 --- a/packages/server-utils/src/integrations/tracing-channel/express/types.ts +++ b/packages/server-utils/src/integrations/tracing-channel/express/types.ts @@ -33,7 +33,7 @@ export interface ExpressResponse { * object for `Layer.prototype.handle_request`/`handleRequest`: `self` is the * Layer the method was invoked on and `arguments` are `[req, res, next]`. * - * `_sentryCleanup` is ours: a teardown for the `res.on('finish')` listener we + * `_sentryCleanup` is ours: a teardown for the `res` `finish`/`close` listeners we * register, invoked from `beforeSpanEnd` when the span ends via `next()`. * `_sentryStoredLayer` marks that this invocation pushed a layer path (so the * matching pop on `asyncStart` stays symmetric).