fix(node): Fix domain scope inheritance (#7799)

Author: timfish

packages/core/src/hub.ts

@@ -568,10 +568,10 @@ function getGlobalHub(registry: Carrier = getMainCarrier()): Hub {
  *
  * If the carrier does not contain a hub, a new hub is created with the global hub client and scope.
  */
-export function ensureHubOnCarrier(carrier: Carrier): void {
+export function ensureHubOnCarrier(carrier: Carrier, parent: Hub = getGlobalHub()): void {
   // If there's no hub on current domain, or it's an old API, assign a new one
   if (!hasHubOnCarrier(carrier) || getHubFromCarrier(carrier).isOlderThan(API_VERSION)) {
-    const globalHubTopStack = getGlobalHub().getStackTop();
+    const globalHubTopStack = parent.getStackTop();
     setHubOnCarrier(carrier, new Hub(globalHubTopStack.client, Scope.clone(globalHubTopStack.scope)));
   }
 }

packages/node/src/async/domain.ts

@@ -1,10 +1,5 @@
 import type { Carrier, Hub, RunWithAsyncContextOptions } from '@sentry/core';
-import {
-  ensureHubOnCarrier,
-  getCurrentHub as getCurrentHubCore,
-  getHubFromCarrier,
-  setAsyncContextStrategy,
-} from '@sentry/core';
+import { ensureHubOnCarrier, getHubFromCarrier, setAsyncContextStrategy, setHubOnCarrier } from '@sentry/core';
 import * as domain from 'domain';
 import { EventEmitter } from 'events';
 
@@ -26,33 +21,40 @@ function getCurrentHub(): Hub | undefined {
   return getHubFromCarrier(activeDomain);
 }
 
+function createNewHub(parent: Hub | undefined): Hub {
+  const carrier: Carrier = {};
+  ensureHubOnCarrier(carrier, parent);
+  return getHubFromCarrier(carrier);
+}
+
 function runWithAsyncContext<T>(callback: (hub: Hub) => T, options: RunWithAsyncContextOptions): T {
-  if (options?.reuseExisting) {
-    const activeDomain = getActiveDomain<domain.Domain & Carrier>();
+  const activeDomain = getActiveDomain<domain.Domain & Carrier>();
 
-    if (activeDomain) {
-      for (const emitter of options.emitters || []) {
-        if (emitter instanceof EventEmitter) {
-          activeDomain.add(emitter);
-        }
+  if (activeDomain && options?.reuseExisting) {
+    for (const emitter of options.emitters || []) {
+      if (emitter instanceof EventEmitter) {
+        activeDomain.add(emitter);
       }
-
-      // We're already in a domain, so we don't need to create a new one, just call the callback with the current hub
-      return callback(getHubFromCarrier(activeDomain));
     }
+
+    // We're already in a domain, so we don't need to create a new one, just call the callback with the current hub
+    return callback(getHubFromCarrier(activeDomain));
   }
 
-  const local = domain.create();
+  const local = domain.create() as domain.Domain & Carrier;
 
   for (const emitter of options.emitters || []) {
     if (emitter instanceof EventEmitter) {
       local.add(emitter);
     }
   }
 
+  const parentHub = activeDomain ? getHubFromCarrier(activeDomain) : undefined;
+  const newHub = createNewHub(parentHub);
+  setHubOnCarrier(local, newHub);
+
   return local.bind(() => {
-    const hub = getCurrentHubCore();
-    return callback(hub);
+    return callback(newHub);
   })();
 }
 

packages/node/test/async/domain.test.ts

@@ -16,27 +16,37 @@ describe('domains', () => {
     expect(hub).toEqual(new Hub());
   });
 
-  test('domain hub scope inheritance', () => {
+  test('hub scope inheritance', () => {
+    setDomainAsyncContextStrategy();
+
     const globalHub = getCurrentHub();
-    globalHub.configureScope(scope => {
-      scope.setExtra('a', 'b');
-      scope.setTag('a', 'b');
-      scope.addBreadcrumb({ message: 'a' });
-    });
-    runWithAsyncContext(hub => {
-      expect(globalHub).toEqual(hub);
+    globalHub.setExtra('a', 'b');
+
+    runWithAsyncContext(hub1 => {
+      expect(hub1).toEqual(globalHub);
+
+      hub1.setExtra('c', 'd');
+      expect(hub1).not.toEqual(globalHub);
+
+      runWithAsyncContext(hub2 => {
+        expect(hub2).toEqual(hub1);
+        expect(hub2).not.toEqual(globalHub);
+
+        hub2.setExtra('e', 'f');
+        expect(hub2).not.toEqual(hub1);
+      });
     });
   });
 
-  test('domain hub single instance', () => {
+  test('hub single instance', () => {
     setDomainAsyncContextStrategy();
 
     runWithAsyncContext(hub => {
       expect(hub).toBe(getCurrentHub());
     });
   });
 
-  test('domain within a domain not reused', () => {
+  test('within a domain not reused', () => {
     setDomainAsyncContextStrategy();
 
     runWithAsyncContext(hub1 => {
@@ -46,7 +56,7 @@ describe('domains', () => {
     });
   });
 
-  test('domain within a domain reused when requested', () => {
+  test('within a domain reused when requested', () => {
     setDomainAsyncContextStrategy();
 
     runWithAsyncContext(hub1 => {
@@ -59,7 +69,7 @@ describe('domains', () => {
     });
   });
 
-  test('concurrent domain hubs', done => {
+  test('concurrent hub contexts', done => {
     setDomainAsyncContextStrategy();
 
     let d1done = false;

packages/node/test/handlers.test.ts

@@ -1,16 +1,26 @@
+import type { Hub } from '@sentry/core';
 import * as sentryCore from '@sentry/core';
-import { Transaction } from '@sentry/core';
+import { setAsyncContextStrategy, Transaction } from '@sentry/core';
 import type { Event } from '@sentry/types';
 import { SentryError } from '@sentry/utils';
 import * as http from 'http';
 
-import { setDomainAsyncContextStrategy } from '../src/async/domain';
 import { NodeClient } from '../src/client';
 import { errorHandler, requestHandler, tracingHandler } from '../src/handlers';
 import * as SDK from '../src/sdk';
 import { getDefaultNodeClientOptions } from './helper/node-client-options';
 
-setDomainAsyncContextStrategy();
+function mockAsyncContextStrategy(getHub: () => Hub): void {
+  function getCurrentHub(): Hub | undefined {
+    return getHub();
+  }
+
+  function runWithAsyncContext<T>(fn: (hub: Hub) => T): T {
+    return fn(getHub());
+  }
+
+  setAsyncContextStrategy({ getCurrentHub, runWithAsyncContext });
+}
 
 describe('requestHandler', () => {
   const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' };
@@ -52,6 +62,7 @@ describe('requestHandler', () => {
     const hub = new sentryCore.Hub(client);
 
     jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+    mockAsyncContextStrategy(() => hub);
 
     sentryRequestMiddleware(req, res, next);
 
@@ -65,6 +76,7 @@ describe('requestHandler', () => {
     const hub = new sentryCore.Hub(client);
 
     jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+    mockAsyncContextStrategy(() => hub);
 
     sentryRequestMiddleware(req, res, next);
 
@@ -78,6 +90,7 @@ describe('requestHandler', () => {
     const hub = new sentryCore.Hub(client);
 
     jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+    mockAsyncContextStrategy(() => hub);
 
     const captureRequestSession = jest.spyOn<any, any>(client, '_captureRequestSession');
 
@@ -97,7 +110,9 @@ describe('requestHandler', () => {
     const options = getDefaultNodeClientOptions({ autoSessionTracking: false, release: '1.2' });
     client = new NodeClient(options);
     const hub = new sentryCore.Hub(client);
+
     jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+    mockAsyncContextStrategy(() => hub);
 
     const captureRequestSession = jest.spyOn<any, any>(client, '_captureRequestSession');
 
@@ -142,6 +157,7 @@ describe('requestHandler', () => {
   it('stores request and request data options in `sdkProcessingMetadata`', () => {
     const hub = new sentryCore.Hub(new NodeClient(getDefaultNodeClientOptions()));
     jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+    mockAsyncContextStrategy(() => hub);
 
     const requestHandlerOptions = { include: { ip: false } };
     const sentryRequestMiddleware = requestHandler(requestHandlerOptions);
@@ -177,6 +193,7 @@ describe('tracingHandler', () => {
   beforeEach(() => {
     hub = new sentryCore.Hub(new NodeClient(getDefaultNodeClientOptions({ tracesSampleRate: 1.0 })));
     jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+    mockAsyncContextStrategy(() => hub);
     req = {
       headers,
       method,
@@ -274,6 +291,8 @@ describe('tracingHandler', () => {
     const tracesSampler = jest.fn();
     const options = getDefaultNodeClientOptions({ tracesSampler });
     const hub = new sentryCore.Hub(new NodeClient(options));
+    mockAsyncContextStrategy(() => hub);
+
     hub.run(() => {
       sentryTracingMiddleware(req, res, next);
 
@@ -296,6 +315,7 @@ describe('tracingHandler', () => {
     const hub = new sentryCore.Hub(new NodeClient(options));
 
     jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+    mockAsyncContextStrategy(() => hub);
 
     sentryTracingMiddleware(req, res, next);
 
@@ -502,14 +522,17 @@ describe('errorHandler()', () => {
     client.initSessionFlusher();
     const scope = new sentryCore.Scope();
     const hub = new sentryCore.Hub(client, scope);
+    mockAsyncContextStrategy(() => hub);
 
     jest.spyOn<any, any>(client, '_captureRequestSession');
 
     hub.run(() => {
       scope?.setRequestSession({ status: 'ok' });
-      sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next);
-      const requestSession = scope?.getRequestSession();
-      expect(requestSession).toEqual({ status: 'crashed' });
+      sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, () => {
+        const scope = sentryCore.getCurrentHub().getScope();
+        const requestSession = scope?.getRequestSession();
+        expect(requestSession).toEqual({ status: 'crashed' });
+      });
     });
   });
 
@@ -535,6 +558,7 @@ describe('errorHandler()', () => {
     client = new NodeClient(options);
 
     const hub = new sentryCore.Hub(client);
+    mockAsyncContextStrategy(() => hub);
     sentryCore.makeMain(hub);
 
     // `sentryErrorMiddleware` uses `withScope`, and we need access to the temporary scope it creates, so monkeypatch