feat(core)!: Always use session from isolation scope (#14860)

This PR ensures that we always take the session from the isolation
scope, never from the current scope.

This has the implication that we need to be sure to pass the isolation
scope to `_processEvent`, as this is where the session may be marked as
errored. For this, I updated the internal method `_processEvent` to take
the isolation scope as last argument, as well as streamlining this
slightly.

I opted to update the signature of the protected `_prepareEvent` method
too, and make currentScope/isolationScope required there. We already
always pass this in now, so it safes a few bytes to avoid the fallback
everywhere. This should not really affect users unless they overwrite
the `_processEvent` method, which is internal/private anyhow, so IMHO
this should be fine. I added a small note to the migration guide anyhow!

Author: mydea

dev-packages/browser-integration-tests/suites/sessions/update-session/test.ts

@@ -2,14 +2,16 @@ import { expect } from '@playwright/test';
 import type { SessionContext } from '@sentry/core';
 
 import { sentryTest } from '../../../utils/fixtures';
-import { getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
+import { getFirstSentryEnvelopeRequest, waitForSession } from '../../../utils/helpers';
 
 sentryTest('should update session when an error is thrown.', async ({ getLocalTestUrl, page }) => {
   const url = await getLocalTestUrl({ testDir: __dirname });
+
   const pageloadSession = await getFirstSentryEnvelopeRequest<SessionContext>(page, url);
-  const updatedSession = (
-    await Promise.all([page.locator('#throw-error').click(), getFirstSentryEnvelopeRequest<SessionContext>(page)])
-  )[1];
+
+  const updatedSessionPromise = waitForSession(page);
+  await page.locator('#throw-error').click();
+  const updatedSession = await updatedSessionPromise;
 
   expect(pageloadSession).toBeDefined();
   expect(pageloadSession.init).toBe(true);
@@ -25,9 +27,10 @@ sentryTest('should update session when an exception is captured.', async ({ getL
   const url = await getLocalTestUrl({ testDir: __dirname });
 
   const pageloadSession = await getFirstSentryEnvelopeRequest<SessionContext>(page, url);
-  const updatedSession = (
-    await Promise.all([page.locator('#capture-exception').click(), getFirstSentryEnvelopeRequest<SessionContext>(page)])
-  )[1];
+
+  const updatedSessionPromise = waitForSession(page);
+  await page.locator('#capture-exception').click();
+  const updatedSession = await updatedSessionPromise;
 
   expect(pageloadSession).toBeDefined();
   expect(pageloadSession.init).toBe(true);

dev-packages/browser-integration-tests/utils/helpers.ts

@@ -7,6 +7,7 @@ import type {
   Event,
   EventEnvelope,
   EventEnvelopeHeaders,
+  SessionContext,
   TransactionEvent,
 } from '@sentry/core';
 
@@ -157,7 +158,7 @@ export const countEnvelopes = async (
  * @param {{ path?: string; content?: string }} impl
  * @return {*}  {Promise<void>}
  */
-async function runScriptInSandbox(
+export async function runScriptInSandbox(
   page: Page,
   impl: {
     path?: string;
@@ -178,7 +179,7 @@ async function runScriptInSandbox(
  * @param {string} [url]
  * @return {*}  {Promise<Array<Event>>}
  */
-async function getSentryEvents(page: Page, url?: string): Promise<Array<Event>> {
+export async function getSentryEvents(page: Page, url?: string): Promise<Array<Event>> {
   if (url) {
     await page.goto(url);
   }
@@ -250,6 +251,25 @@ export function waitForTransactionRequest(
   });
 }
 
+export async function waitForSession(page: Page): Promise<SessionContext> {
+  const req = await page.waitForRequest(req => {
+    const postData = req.postData();
+    if (!postData) {
+      return false;
+    }
+
+    try {
+      const event = envelopeRequestParser<SessionContext>(req);
+
+      return typeof event.init === 'boolean' && event.started !== undefined;
+    } catch {
+      return false;
+    }
+  });
+
+  return envelopeRequestParser<SessionContext>(req);
+}
+
 /**
  * We can only test tracing tests in certain bundles/packages:
  * - NPM (ESM, CJS)
@@ -353,7 +373,7 @@ async function getMultipleRequests<T>(
 /**
  * Wait and get multiple envelope requests at the given URL, or the current page
  */
-async function getMultipleSentryEnvelopeRequests<T>(
+export async function getMultipleSentryEnvelopeRequests<T>(
   page: Page,
   count: number,
   options?: {
@@ -374,7 +394,7 @@ async function getMultipleSentryEnvelopeRequests<T>(
  * @param {string} [url]
  * @return {*}  {Promise<T>}
  */
-async function getFirstSentryEnvelopeRequest<T>(
+export async function getFirstSentryEnvelopeRequest<T>(
   page: Page,
   url?: string,
   requestParser: (req: Request) => T = envelopeRequestParser as (req: Request) => T,
@@ -388,5 +408,3 @@ async function getFirstSentryEnvelopeRequest<T>(
 
   return req;
 }
-
-export { runScriptInSandbox, getMultipleSentryEnvelopeRequests, getFirstSentryEnvelopeRequest, getSentryEvents };

docs/migration/v8-to-v9.md

@@ -143,10 +143,14 @@ Sentry.init({
 - The `flatten` export has been removed. There is no replacement.
 - The `urlEncode` method has been removed. There is no replacement.
 - The `getDomElement` method has been removed. There is no replacement.
-- The `Request` type has been removed. Use `RequestEventData` type instead.
-- The `TransactionNamingScheme` type has been removed. There is no replacement.
 - The `memoBuilder` method has been removed. There is no replacement.
 
+#### Other/Internal Changes
+
+The following changes are unlikely to affect users of the SDK. They are listed here only for completion sake, and to alert users that may be relying on internal behavior.
+
+- `client._prepareEvent()` now requires a currentScope & isolationScope to be passed as last arugments
+
 ### `@sentry/browser`
 
 - The `captureUserFeedback` method has been removed. Use `captureFeedback` instead and update the `comments` field to `message`.
@@ -210,6 +214,8 @@ This led to some duplication, where we had to keep an interface in `@sentry/type
 Since v9, the types have been merged into `@sentry/core`, which removed some of this duplication. This means that certain things that used to be a separate interface, will not expect an actual instance of the class/concrete implementation. This should not affect most users, unless you relied on passing things with a similar shape to internal methods. The following types are affected:
 
 - `Scope` now always expects the `Scope` class
+- The `TransactionNamingScheme` type has been removed. There is no replacement.
+- The `Request` type has been removed. Use `RequestEventData` type instead.
 - The `IntegrationClass` type is no longer exported - it was not used anymore. Instead, use `Integration` or `IntegrationFn`.
 
 # No Version Support Timeline

packages/browser/src/client.ts

@@ -105,8 +105,13 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
   /**
    * @inheritDoc
    */
-  protected _prepareEvent(event: Event, hint: EventHint, scope?: Scope): PromiseLike<Event | null> {
+  protected _prepareEvent(
+    event: Event,
+    hint: EventHint,
+    currentScope: Scope,
+    isolationScope: Scope,
+  ): PromiseLike<Event | null> {
     event.platform = event.platform || 'javascript';
-    return super._prepareEvent(event, hint, scope);
+    return super._prepareEvent(event, hint, currentScope, isolationScope);
   }
 }

packages/core/src/baseclient.ts

@@ -216,8 +216,11 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
 
     const sdkProcessingMetadata = event.sdkProcessingMetadata || {};
     const capturedSpanScope: Scope | undefined = sdkProcessingMetadata.capturedSpanScope;
+    const capturedSpanIsolationScope: Scope | undefined = sdkProcessingMetadata.capturedSpanIsolationScope;
 
-    this._process(this._captureEvent(event, hintWithEventId, capturedSpanScope || currentScope));
+    this._process(
+      this._captureEvent(event, hintWithEventId, capturedSpanScope || currentScope, capturedSpanIsolationScope),
+    );
 
     return hintWithEventId.event_id;
   }
@@ -676,8 +679,8 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   protected _prepareEvent(
     event: Event,
     hint: EventHint,
-    currentScope = getCurrentScope(),
-    isolationScope = getIsolationScope(),
+    currentScope: Scope,
+    isolationScope: Scope,
   ): PromiseLike<Event | null> {
     const options = this.getOptions();
     const integrations = Object.keys(this._integrations);
@@ -718,12 +721,17 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
    * @param hint
    * @param scope
    */
-  protected _captureEvent(event: Event, hint: EventHint = {}, scope?: Scope): PromiseLike<string | undefined> {
+  protected _captureEvent(
+    event: Event,
+    hint: EventHint = {},
+    currentScope = getCurrentScope(),
+    isolationScope = getIsolationScope(),
+  ): PromiseLike<string | undefined> {
     if (DEBUG_BUILD && isErrorEvent(event)) {
       logger.log(`Captured error event \`${getPossibleEventMessages(event)[0] || '<unknown>'}\``);
     }
 
-    return this._processEvent(event, hint, scope).then(
+    return this._processEvent(event, hint, currentScope, isolationScope).then(
       finalEvent => {
         return finalEvent.event_id;
       },
@@ -756,7 +764,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
    * @param currentScope A scope containing event metadata.
    * @returns A SyncPromise that resolves with the event or rejects in case event was/will not be send.
    */
-  protected _processEvent(event: Event, hint: EventHint, currentScope?: Scope): PromiseLike<Event> {
+  protected _processEvent(
+    event: Event,
+    hint: EventHint,
+    currentScope: Scope,
+    isolationScope: Scope,
+  ): PromiseLike<Event> {
     const options = this.getOptions();
     const { sampleRate } = options;
 
@@ -779,12 +792,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
       );
     }
 
-    const dataCategory: DataCategory = eventType === 'replay_event' ? 'replay' : eventType;
-
-    const sdkProcessingMetadata = event.sdkProcessingMetadata || {};
-    const capturedSpanIsolationScope: Scope | undefined = sdkProcessingMetadata.capturedSpanIsolationScope;
+    const dataCategory = (eventType === 'replay_event' ? 'replay' : eventType) satisfies DataCategory;
 
-    return this._prepareEvent(event, hint, currentScope, capturedSpanIsolationScope)
+    return this._prepareEvent(event, hint, currentScope, isolationScope)
       .then(prepared => {
         if (prepared === null) {
           this.recordDroppedEvent('event_processor', dataCategory, event);
@@ -811,8 +821,8 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
           throw new SentryError(`${beforeSendLabel} returned \`null\`, will not send event.`, 'log');
         }
 
-        const session = currentScope && currentScope.getSession();
-        if (!isTransaction && session) {
+        const session = currentScope.getSession() || isolationScope.getSession();
+        if (isError && session) {
           this._updateSessionFromEvent(session, processedEvent);
         }
 

packages/core/src/exports.ts

@@ -287,10 +287,6 @@ export function startSession(context?: SessionContext): Session {
   // Afterwards we set the new session on the scope
   isolationScope.setSession(session);
 
-  // TODO (v8): Remove this and only use the isolation scope(?).
-  // For v7 though, we can't "soft-break" people using getCurrentHub().getScope().setSession()
-  currentScope.setSession(session);
-
   return session;
 }
 
@@ -309,22 +305,15 @@ export function endSession(): void {
 
   // the session is over; take it off of the scope
   isolationScope.setSession();
-
-  // TODO (v8): Remove this and only use the isolation scope(?).
-  // For v7 though, we can't "soft-break" people using getCurrentHub().getScope().setSession()
-  currentScope.setSession();
 }
 
 /**
  * Sends the current Session on the scope
  */
 function _sendSessionUpdate(): void {
   const isolationScope = getIsolationScope();
-  const currentScope = getCurrentScope();
   const client = getClient();
-  // TODO (v8): Remove currentScope and only use the isolation scope(?).
-  // For v7 though, we can't "soft-break" people using getCurrentHub().getScope().setSession()
-  const session = currentScope.getSession() || isolationScope.getSession();
+  const session = isolationScope.getSession();
   if (session && client) {
     client.captureSession(session);
   }

packages/core/src/getCurrentHubShim.ts

@@ -2,6 +2,7 @@ import { addBreadcrumb } from './breadcrumbs';
 import { getClient, getCurrentScope, getIsolationScope, withScope } from './currentScopes';
 import {
   captureEvent,
+  captureSession,
   endSession,
   setContext,
   setExtra,
@@ -54,15 +55,7 @@ export function getCurrentHubShim(): Hub {
 
     startSession,
     endSession,
-    captureSession(end?: boolean): void {
-      // both send the update and pull the session from the scope
-      if (end) {
-        return endSession();
-      }
-
-      // only send the update
-      _sendSessionUpdate();
-    },
+    captureSession,
   };
 }
 
@@ -77,16 +70,3 @@ export function getCurrentHubShim(): Hub {
  */
 // eslint-disable-next-line deprecation/deprecation
 export const getCurrentHub = getCurrentHubShim;
-
-/**
- * Sends the current Session on the scope
- */
-function _sendSessionUpdate(): void {
-  const scope = getCurrentScope();
-  const client = getClient();
-
-  const session = scope.getSession();
-  if (client && session) {
-    client.captureSession(session);
-  }
-}

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

@@ -166,8 +166,8 @@ export class ServerRuntimeClient<
   protected _prepareEvent(
     event: Event,
     hint: EventHint,
-    scope?: Scope,
-    isolationScope?: Scope,
+    currentScope: Scope,
+    isolationScope: Scope,
   ): PromiseLike<Event | null> {
     if (this._options.platform) {
       event.platform = event.platform || this._options.platform;
@@ -184,7 +184,7 @@ export class ServerRuntimeClient<
       event.server_name = event.server_name || this._options.serverName;
     }
 
-    return super._prepareEvent(event, hint, scope, isolationScope);
+    return super._prepareEvent(event, hint, currentScope, isolationScope);
   }
 
   /** Extract trace information from scope */

packages/core/test/lib/serverruntimeclient.test.ts renamed to packages/core/test/lib/server-runtime-client.test.ts

@@ -1,6 +1,6 @@
 import type { Event, EventHint } from '../../src/types-hoist';
 
-import { createTransport } from '../../src';
+import { Scope, createTransport } from '../../src';
 import type { ServerRuntimeClientOptions } from '../../src/server-runtime-client';
 import { ServerRuntimeClient } from '../../src/server-runtime-client';
 
@@ -18,14 +18,17 @@ function getDefaultClientOptions(options: Partial<ServerRuntimeClientOptions> =
 describe('ServerRuntimeClient', () => {
   let client: ServerRuntimeClient;
 
+  const currentScope = new Scope();
+  const isolationScope = new Scope();
+
   describe('_prepareEvent', () => {
     test('adds platform to event', () => {
       const options = getDefaultClientOptions({ dsn: PUBLIC_DSN });
       const client = new ServerRuntimeClient({ ...options, platform: 'blargh' });
 
       const event: Event = {};
       const hint: EventHint = {};
-      (client as any)._prepareEvent(event, hint);
+      client['_prepareEvent'](event, hint, currentScope, isolationScope);
 
       expect(event.platform).toEqual('blargh');
     });
@@ -36,7 +39,7 @@ describe('ServerRuntimeClient', () => {
 
       const event: Event = {};
       const hint: EventHint = {};
-      (client as any)._prepareEvent(event, hint);
+      client['_prepareEvent'](event, hint, currentScope, isolationScope);
 
       expect(event.server_name).toEqual('server');
     });
@@ -47,7 +50,7 @@ describe('ServerRuntimeClient', () => {
 
       const event: Event = {};
       const hint: EventHint = {};
-      (client as any)._prepareEvent(event, hint);
+      client['_prepareEvent'](event, hint, currentScope, isolationScope);
 
       expect(event.contexts?.runtime).toEqual({
         name: 'edge',
@@ -60,7 +63,7 @@ describe('ServerRuntimeClient', () => {
 
       const event: Event = { contexts: { runtime: { name: 'foo', version: '1.2.3' } } };
       const hint: EventHint = {};
-      (client as any)._prepareEvent(event, hint);
+      client['_prepareEvent'](event, hint, currentScope, isolationScope);
 
       expect(event.contexts?.runtime).toEqual({ name: 'foo', version: '1.2.3' });
       expect(event.contexts?.runtime).not.toEqual({ name: 'edge' });