From 818e30a38dd8833bf63263d87ebff2057ac6af17 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 9 Oct 2026 10:22:55 +0200 Subject: [PATCH 1/2] fix(server-utils): End Express layer spans when the client aborts Express layer spans ended only on the response's finish event. When the client aborts, finish never fires, so the handler span never ended. Static transactions were sent without it and its subtree, and with span streaming its children were orphaned. Also end the layer span on close, which is when the http.server span ends, and mark it as cancelled. Fixes #25183 Co-Authored-By: Claude Opus 5.5 --- .../express/client-abort/instrument.mjs | 9 +++++ .../suites/express/client-abort/scenario.mjs | 12 +++++++ .../suites/express/client-abort/test.ts | 33 +++++++++++++++++++ .../utils/runner/createRunner.ts | 16 +++++++-- .../integrations/express/instrumentation.ts | 19 +++++++++-- .../src/integrations/express/types.ts | 2 +- 6 files changed, 85 insertions(+), 6 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..c7d88f04a8cf --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/client-abort/test.ts @@ -0,0 +1,33 @@ +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() + .unordered() + .expect({ + span: container => { + const segmentSpan = container.items.find(item => item.is_segment); + const handlerSpan = container.items.find( + item => item.attributes['express.type']?.value === 'request_handler', + ); + + expect(segmentSpan?.name).toBe('GET /test/slow'); + expect(handlerSpan).toBeDefined(); + expect(handlerSpan!.parent_span_id).toBe(segmentSpan!.span_id); + // Ends when the client aborts, not when the handler's 500ms of work finishes. + expect(handlerSpan!.end_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 de845c3063ac..9a1dcf8ba4a0 100644 --- a/dev-packages/node-integration-tests/utils/runner/createRunner.ts +++ b/dev-packages/node-integration-tests/utils/runner/createRunner.ts @@ -105,7 +105,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; }; @@ -774,7 +774,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, PORT_TIMEOUT, 'Timed out waiting for server port'); @@ -791,7 +796,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/server-utils/src/integrations/express/instrumentation.ts b/packages/server-utils/src/integrations/express/instrumentation.ts index 753ebbc90660..a3d079c6bd2b 100644 --- a/packages/server-utils/src/integrations/express/instrumentation.ts +++ b/packages/server-utils/src/integrations/express/instrumentation.ts @@ -12,6 +12,7 @@ import { hasSpanStreamingEnabled, REQUEST_HANDLER_SPAN_NAME_FALLBACK, ROUTER_SPAN_NAME_FALLBACK, + SPAN_STATUS_ERROR, startInactiveSpan, stringMatchesSomePattern, withActiveSpan, @@ -350,13 +351,27 @@ function getSpanForLayer(data: HandleChannelContext, options: ExpressIntegration // A layer that sends the response (route handlers, typically) never calls // `next`, so the channel's `asyncStart` never fires. End on the response's // `finish` in that case. When `next` *is* called, `endLayerSpanOnNext` ends - // the span and removes this now-redundant listener. + // the span and 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/express/types.ts b/packages/server-utils/src/integrations/express/types.ts index ea1d979a18af..acffe0c9900c 100644 --- a/packages/server-utils/src/integrations/express/types.ts +++ b/packages/server-utils/src/integrations/express/types.ts @@ -34,7 +34,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). `_sentrySpan` is the span bound From 8d40ec41f334807704fb5fd3629617d4e4d6d271 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 9 Oct 2026 11:26:53 +0200 Subject: [PATCH 2/2] test(node): Skip Express client abort test on Bun Bun 1.3.14, the version CI runs, emits no close event on the response when the client aborts, so the http.server span never ends there. Bun 1.4.0 emits it and the test passes. Co-Authored-By: Claude Opus 5.5 --- .../suites/express/client-abort/test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 index c7d88f04a8cf..b9cac9af42d8 100644 --- a/dev-packages/node-integration-tests/suites/express/client-abort/test.ts +++ b/dev-packages/node-integration-tests/suites/express/client-abort/test.ts @@ -1,4 +1,5 @@ import { afterAll, describe, expect } from 'vitest'; +import { RUNTIME } from '../../../utils'; import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; describe('express client abort', () => { @@ -7,7 +8,9 @@ describe('express client abort', () => { }); createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { - test('ends the request handler span when the client aborts', async () => { + // Bun 1.3.14 emits no `close` on the response when the client aborts, so the `http.server` + // span never ends either. Bun 1.4.0 emits it. + test.skipIf(RUNTIME === 'bun')('ends the request handler span when the client aborts', async () => { const runner = createRunner() .unordered() .expect({