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..b9cac9af42d8 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/client-abort/test.ts @@ -0,0 +1,36 @@ +import { afterAll, describe, expect } from 'vitest'; +import { RUNTIME } from '../../../utils'; +import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; + +describe('express client abort', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + // 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({ + 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