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

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

@@ -105,7 +105,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>;
 };
 
@@ -774,7 +774,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, 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) {

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;

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