fix(v10/server-utils): End Express layer spans when the client aborts

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,
+});

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

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();
+    });
+  });
+});

dev-packages/node-integration-tests/utils/runner/createRunner.ts

@@ -94,7 +94,7 @@ type StartResult = {
   makeRequest<T>(
     method: 'get' | 'post' | 'put' | 'delete' | 'patch',
     path: string,
-    options?: { headers?: Record<string, string>; data?: BodyInit; expectError?: boolean },
+    options?: { headers?: Record<string, string>; data?: BodyInit; expectError?: boolean; timeout?: number },
   ): Promise<T | undefined>;
 };
 
@@ -565,7 +565,12 @@ export function createRunner(...paths: string[]) {
         makeRequest: async function <T>(
           method: 'get' | 'post' | 'put' | 'delete' | 'patch',
           path: string,
-          options: { headers?: Record<string, string>; data?: BodyInit; expectError?: boolean } = {},
+          options: {
+            headers?: Record<string, string>;
+            data?: BodyInit;
+            expectError?: boolean;
+            timeout?: number;
+          } = {},
         ): Promise<T | undefined> {
           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) {

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);
         }
       }
     });

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;

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