fix(core): Do not count dropped errors in request sessions

dev-packages/bun-integration-tests/node-suites/excludes.ts

@@ -166,6 +166,7 @@ const FETCH_INTEGRATION_DIFFERS = [
 // that sets `emit` back to the `emit` it had before its first request gets no spans after that.
 const HTTP_SERVER_OPTIONS_IGNORED = [
   'suites/sessions/exited-session-aggregate/test.ts',
+  'suites/sessions/filtered-errors-session-aggregate/test.ts',
   'suites/sessions/server-without-process-session/test.ts',
   'suites/tracing/httpIntegration/test.ts',
 ];

dev-packages/node-integration-tests/suites/sessions/filtered-errors-session-aggregate/test.ts

@@ -0,0 +1,33 @@
+import { afterAll, expect, test } from 'vitest';
+import { join } from 'path';
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('does not count errors dropped by ignoreErrors or beforeSend', async () => {
+  const runner = createRunner(__dirname, '..', 'server.mjs')
+    .withInstrument(join(__dirname, '..', 'instrument.mjs'))
+    .ignore('span', 'event')
+    .unignore('sessions')
+    .expect({
+      sessions: agg => {
+        // Sessions are bucketed by minute; tolerate splits across a minute boundary by summing.
+        const totals = agg.aggregates.reduce(
+          (acc, b) => ({
+            exited: acc.exited + (b.exited ?? 0),
+            errored: acc.errored + (b.errored ?? 0),
+            crashed: acc.crashed + (b.crashed ?? 0),
+          }),
+          { exited: 0, errored: 0, crashed: 0 },
+        );
+        expect(totals).toEqual({ exited: 2, errored: 0, crashed: 0 });
+      },
+    })
+    .start();
+
+  runner.makeRequest('get', '/test/error_ignored', { expectError: true });
+  runner.makeRequest('get', '/test/error_dropped');
+  await runner.completed();
+});

dev-packages/node-integration-tests/suites/sessions/instrument.mjs

@@ -5,6 +5,8 @@ Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
   release: '1.0',
   transport: loggingTransport,
+  ignoreErrors: ['Ignored!'],
+  beforeSend: event => (event.exception?.values?.[0]?.value === 'Dropped!' ? null : event),
   integrations: [
     Sentry.httpIntegration({
       // Flush after 2 seconds (to avoid waiting for the default 60s)

dev-packages/node-integration-tests/suites/sessions/server.mjs

@@ -32,4 +32,13 @@ app.get('/test/error_handled', (_req, res) => {
   res.send('Crash!');
 });
 
+app.get('/test/error_ignored', () => {
+  throw new Error('Ignored!');
+});
+
+app.get('/test/error_dropped', (_req, res) => {
+  Sentry.captureException(new Error('Dropped!'));
+  res.send('Dropped!');
+});
+
 startExpressServerAndSendPortToRunner(app);

packages/core/src/client.ts

@@ -1362,6 +1362,17 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
     afterSetupIntegrations(this, integrations);
   }
 
+  /**
+   * Updates the sessions an error event counts towards. Runs after event processors and `beforeSend` kept the
+   * event, but before `sampleRate` is applied: dropped errors must not affect sessions, sampled-out errors must.
+   */
+  protected _updateSessionsFromEvent(event: Event, currentScope: Scope, isolationScope: Scope): void {
+    const session = currentScope.getSession() || isolationScope.getSession();
+    if (session) {
+      this._updateSessionFromEvent(session, event);
+    }
+  }
+
   /** Updates existing session based on the provided event */
   protected _updateSessionFromEvent(session: Session, event: Event): void {
     // initially, set `unhandled` based on the event level and update from exceptions if there are any later on
@@ -1602,9 +1613,8 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
           throw _makeDoNotSendEventError(`${dropMessage}, will not send event.`);
         }
 
-        const session = currentScope.getSession() || isolationScope.getSession();
-        if (isError && session) {
-          this._updateSessionFromEvent(session, processedEvent);
+        if (isError) {
+          this._updateSessionsFromEvent(processedEvent, currentScope, isolationScope);
         }
 
         if (isError && typeof parsedSampleRate === 'number' && safeMathRandom() > parsedSampleRate) {

packages/core/src/integrations/http/record-request-session.ts

@@ -65,11 +65,7 @@ export function startRequestSession(
   client.emit('startRequestSession');
 
   let ended = false;
-  return () => {
-    if (ended) {
-      return;
-    }
-    ended = true;
+  const record = (flushNow: boolean): void => {
     const requestSession = requestIsolationScope.getScopeData().sdkProcessingMetadata.requestSession;
 
     if (client && requestSession) {
@@ -115,7 +111,37 @@ export function startRequestSession(
           flushPendingClientAggregates();
         }, sessionFlushingDelayMS);
         safeUnref(timeout);
+
+        if (flushNow) {
+          flushPendingClientAggregates();
+        }
       }
     }
   };
+
+  return () => {
+    if (ended) {
+      return;
+    }
+    ended = true;
+
+    // The event pipeline decides whether an error counts towards the session, and it can still run after the
+    // response ended (e.g. async event processors), so the status is final only once those errors settle.
+    const pendingErrors = requestIsolationScope.getScopeData().sdkProcessingMetadata.requestSession?.pendingErrors;
+    if (!pendingErrors?.size) {
+      record(false);
+      return;
+    }
+
+    // `client.flush()` sends the aggregate before it waits for event processing. A session that settles during
+    // that wait is sent right away, so that the transport still has it when that flush drains it.
+    let flushedWhileSettling = false;
+    const unregisterClientFlushHook = client.on('flush', () => {
+      flushedWhileSettling = true;
+    });
+    void Promise.allSettled(pendingErrors).then(() => {
+      unregisterClientFlushHook();
+      record(flushedWhileSettling);
+    });
+  };
 }

packages/core/src/scope.ts

@@ -57,6 +57,8 @@ export interface SdkProcessingMetadata {
   [key: string]: unknown;
   requestSession?: {
     status: 'ok' | 'errored' | 'crashed';
+    /** Error events of the request that are still in the event pipeline. */
+    pendingErrors?: Set<PromiseLike<unknown>>;
   };
   normalizedRequest?: RequestEventData;
   dynamicSamplingContext?: Partial<DynamicSamplingContext>;

packages/core/src/server-runtime-client.ts

@@ -1,6 +1,5 @@
 import { createCheckInEnvelope } from './checkin';
 import { Client } from './client';
-import { getIsolationScope } from './currentScopes';
 import { DEBUG_BUILD } from './debug-build';
 import {
   spanStreamingIntegration,
@@ -87,22 +86,33 @@ export class ServerRuntimeClient<
   /**
    * @inheritDoc
    */
-  public captureException(exception: unknown, hint?: EventHint, scope?: Scope): string {
-    setCurrentRequestSessionErroredOrCrashed(hint);
-    return super.captureException(exception, hint, scope);
+  protected override _processEvent(
+    event: Event,
+    hint: EventHint,
+    currentScope: Scope,
+    isolationScope: Scope,
+  ): PromiseLike<Event> {
+    const processedEvent = super._processEvent(event, hint, currentScope, isolationScope);
+
+    const requestSession = isolationScope.getScopeData().sdkProcessingMetadata.requestSession;
+    if (requestSession && !event.type) {
+      const pendingErrors = (requestSession.pendingErrors ||= new Set());
+      const settle = (): void => {
+        pendingErrors.delete(processedEvent);
+      };
+      pendingErrors.add(processedEvent);
+      processedEvent.then(settle, settle);
+    }
+
+    return processedEvent;
   }
 
   /**
    * @inheritDoc
    */
-  public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string {
-    // If the event is of type Exception, then a request session should be captured
-    const isException = !event.type && event.exception?.values && event.exception.values.length > 0;
-    if (isException) {
-      setCurrentRequestSessionErroredOrCrashed(hint);
-    }
-
-    return super.captureEvent(event, hint, scope);
+  protected override _updateSessionsFromEvent(event: Event, currentScope: Scope, isolationScope: Scope): void {
+    super._updateSessionsFromEvent(event, currentScope, isolationScope);
+    setRequestSessionErroredOrCrashed(isolationScope, event);
   }
 
   /**
@@ -254,13 +264,14 @@ export class ServerRuntimeClient<
   }
 }
 
-function setCurrentRequestSessionErroredOrCrashed(eventHint?: EventHint): void {
-  const requestSession = getIsolationScope().getScopeData().sdkProcessingMetadata.requestSession;
-  if (requestSession) {
+function setRequestSessionErroredOrCrashed(isolationScope: Scope, event: Event): void {
+  const requestSession = isolationScope.getScopeData().sdkProcessingMetadata.requestSession;
+  const exceptions = event.exception?.values;
+  if (requestSession && exceptions?.length) {
     // We mutate instead of doing `setSdkProcessingMetadata` because the http integration stores away a particular
     // isolationScope. If that isolation scope is forked, setting the processing metadata here will not mutate the
     // original isolation scope that the http integration stored away.
-    const isHandledException = eventHint?.mechanism?.handled ?? true;
+    const isHandledException = !exceptions.some(exception => exception.mechanism?.handled === false);
     // A request session can go from "errored" -> "crashed" but not "crashed" -> "errored".
     // Crashed (unhandled exception) is worse than errored (handled exception).
     if (isHandledException && requestSession.status !== 'crashed') {

packages/core/test/lib/server-runtime-client.test.ts

@@ -1,9 +1,11 @@
-import { describe, expect, it, test, vi } from 'vitest';
-import { applySdkMetadata, createTransport, Scope } from '../../src';
+import { beforeEach, describe, expect, it, test, vi } from 'vitest';
+import { applySdkMetadata, createTransport, getIsolationScope, Scope } from '../../src';
+import { startRequestSession } from '../../src/integrations/http/record-request-session';
 import { _INTERNAL_captureMetric, _INTERNAL_getMetricBuffer } from '../../src/metrics/internal';
 import type { ServerRuntimeClientOptions } from '../../src/server-runtime-client';
 import { ServerRuntimeClient } from '../../src/server-runtime-client';
-import type { Event, EventHint } from '../../src/types/event';
+import type { ErrorEvent, Event, EventHint } from '../../src/types/event';
+import { resetGlobals } from '../testutils';
 
 const PUBLIC_DSN = 'https://username@domain/123';
 
@@ -183,6 +185,36 @@ describe('ServerRuntimeClient', () => {
     });
   });
 
+  describe('request sessions', () => {
+    beforeEach(() => {
+      resetGlobals();
+    });
+
+    it.each([
+      ['errored for a handled exception', {}, undefined, { errored: 1 }],
+      ['crashed for an unhandled exception', {}, { mechanism: { type: 'test', handled: false } }, { crashed: 1 }],
+      ['exited for an exception that `beforeSend` drops', { beforeSend: () => null }, undefined, { exited: 1 }],
+      ['errored for an exception that `sampleRate` drops', { sampleRate: 0 }, undefined, { errored: 1 }],
+      [
+        'errored for an exception that is processed after the request ended',
+        { beforeSend: (event: ErrorEvent) => new Promise<ErrorEvent>(resolve => setTimeout(() => resolve(event), 10)) },
+        undefined,
+        { errored: 1 },
+      ],
+    ] as const)('records the session as %s', async (_, options, hint, counts) => {
+      client = new ServerRuntimeClient(getDefaultClientOptions({ dsn: PUBLIC_DSN, ...options }));
+      const sendSessionSpy = vi.spyOn(client, 'sendSession');
+      const endRequestSession = startRequestSession(client, { requestIsolationScope: getIsolationScope() });
+      client.captureException(new Error('foo'), hint);
+      endRequestSession();
+      await client.flush();
+
+      expect(sendSessionSpy).toHaveBeenCalledExactlyOnceWith({
+        aggregates: [{ started: expect.any(String), exited: 0, errored: 0, crashed: 0, ...counts }],
+      });
+    });
+  });
+
   describe('captureMessage', () => {
     it('sends a message event with level info', () => {
       const options = getDefaultClientOptions({ dsn: PUBLIC_DSN });