Skip to content

Commit ad194d4

Browse files
authored
fix(nestjs): Fork isolation scope in @nestjs/event-emitter instrumentation (#19725)
We should fork the isolation scope when processing events to ensure that data (e.g. breadcrumbs) set during event processing does not leak into subsequent http requests. Closes #19705
1 parent d991f3a commit ad194d4

File tree

6 files changed

+74
-19
lines changed

6 files changed

+74
-19
lines changed

dev-packages/e2e-tests/test-applications/nestjs-distributed-tracing/src/events.controller.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,9 @@ export class EventsController {
1818

1919
return { message: 'Events emitted' };
2020
}
21+
22+
@Get('test-isolation')
23+
testIsolation() {
24+
return { message: 'ok' };
25+
}
2126
}

dev-packages/e2e-tests/test-applications/nestjs-distributed-tracing/src/events.service.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,15 @@ import { EventEmitter2 } from '@nestjs/event-emitter';
33

44
@Injectable()
55
export class EventsService {
6-
constructor(private readonly eventEmitter: EventEmitter2) {}
6+
constructor(private readonly eventEmitter: EventEmitter2) {
7+
// Emit event periodically outside of HTTP context to test isolation scope behavior.
8+
// setInterval runs in the default async context (no HTTP request), so without proper
9+
// isolation scope forking, the breadcrumb set by the handler leaks into the default
10+
// isolation scope and gets cloned into subsequent HTTP requests.
11+
setInterval(() => {
12+
this.eventEmitter.emit('test-isolation.breadcrumb');
13+
}, 2000);
14+
}
715

816
async emitEvents() {
917
await this.eventEmitter.emit('myEvent.pass', { data: 'test' });

dev-packages/e2e-tests/test-applications/nestjs-distributed-tracing/src/listeners/test-event.listener.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,14 @@ export class TestEventListener {
1515
throw new Error('Test error from event handler');
1616
}
1717

18+
@OnEvent('test-isolation.breadcrumb')
19+
handleIsolationBreadcrumbEvent(): void {
20+
Sentry.addBreadcrumb({
21+
message: 'leaked-breadcrumb-from-event-handler',
22+
level: 'info',
23+
});
24+
}
25+
1826
@OnEvent('multiple.first')
1927
@OnEvent('multiple.second')
2028
async handleMultipleEvents(payload: any): Promise<void> {

dev-packages/e2e-tests/test-applications/nestjs-distributed-tracing/tests/events.test.ts

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,28 @@ test('Event emitter', async () => {
4444
});
4545
});
4646

47+
test('Event handler breadcrumbs do not leak into subsequent HTTP requests', async () => {
48+
// The app emits 'test-isolation.breadcrumb' every 2s via setInterval (outside HTTP context).
49+
// The handler adds a breadcrumb. Without isolation scope forking, this breadcrumb leaks
50+
// into the default isolation scope and gets cloned into subsequent HTTP requests.
51+
52+
// Wait for at least one setInterval tick to fire and add the breadcrumb
53+
await new Promise(resolve => setTimeout(resolve, 3000));
54+
55+
const transactionPromise = waitForTransaction('nestjs-distributed-tracing', transactionEvent => {
56+
return transactionEvent.transaction === 'GET /events/test-isolation';
57+
});
58+
59+
await fetch('http://localhost:3050/events/test-isolation');
60+
61+
const transaction = await transactionPromise;
62+
63+
const leakedBreadcrumb = (transaction.breadcrumbs || []).find(
64+
(b: any) => b.message === 'leaked-breadcrumb-from-event-handler',
65+
);
66+
expect(leakedBreadcrumb).toBeUndefined();
67+
});
68+
4769
test('Multiple OnEvent decorators', async () => {
4870
const firstTxPromise = waitForTransaction('nestjs-distributed-tracing', transactionEvent => {
4971
return transactionEvent.transaction === 'event multiple.first|multiple.second';
@@ -64,6 +86,10 @@ test('Multiple OnEvent decorators', async () => {
6486

6587
expect(firstTx).toBeDefined();
6688
expect(secondTx).toBeDefined();
67-
// assert that the correct payloads were added
68-
expect(rootTx.tags).toMatchObject({ 'test-first': true, 'test-second': true });
89+
90+
// Tags should be on the event handler transactions, not the root HTTP transaction
91+
expect(firstTx.tags?.['test-first'] || firstTx.tags?.['test-second']).toBe(true);
92+
expect(secondTx.tags?.['test-first'] || secondTx.tags?.['test-second']).toBe(true);
93+
expect(rootTx.tags?.['test-first']).toBeUndefined();
94+
expect(rootTx.tags?.['test-second']).toBeUndefined();
6995
});

packages/nestjs/src/integrations/sentry-nest-event-instrumentation.ts

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import {
55
InstrumentationNodeModuleFile,
66
isWrapped,
77
} from '@opentelemetry/instrumentation';
8-
import { captureException, SDK_VERSION, startSpan } from '@sentry/core';
8+
import { captureException, SDK_VERSION, startSpan, withIsolationScope } from '@sentry/core';
99
import { getEventSpanOptions } from './helpers';
1010
import type { OnEventTarget } from './types';
1111

@@ -110,21 +110,23 @@ export class SentryNestEventInstrumentation extends InstrumentationBase {
110110
}
111111
}
112112

113-
return startSpan(getEventSpanOptions(eventName), async () => {
114-
try {
115-
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
116-
const result = await originalHandler.apply(this, args);
117-
return result;
118-
} catch (error) {
119-
// exceptions from event handlers are not caught by global error filter
120-
captureException(error, {
121-
mechanism: {
122-
handled: false,
123-
type: 'auto.event.nestjs',
124-
},
125-
});
126-
throw error;
127-
}
113+
return withIsolationScope(() => {
114+
return startSpan(getEventSpanOptions(eventName), async () => {
115+
try {
116+
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
117+
const result = await originalHandler.apply(this, args);
118+
return result;
119+
} catch (error) {
120+
// exceptions from event handlers are not caught by global error filter
121+
captureException(error, {
122+
mechanism: {
123+
handled: false,
124+
type: 'auto.event.nestjs',
125+
},
126+
});
127+
throw error;
128+
}
129+
});
128130
});
129131
};
130132

packages/nestjs/test/integrations/nest.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ describe('Nest', () => {
3838
} as OnEventTarget;
3939
vi.spyOn(core, 'startSpan');
4040
vi.spyOn(core, 'captureException');
41+
vi.spyOn(core, 'withIsolationScope');
4142
});
4243

4344
afterEach(() => {
@@ -75,6 +76,7 @@ describe('Nest', () => {
7576

7677
await descriptor.value();
7778

79+
expect(core.withIsolationScope).toHaveBeenCalled();
7880
expect(core.startSpan).toHaveBeenCalledWith(
7981
expect.objectContaining({
8082
name: 'event test.event',
@@ -90,6 +92,7 @@ describe('Nest', () => {
9092

9193
await descriptor.value();
9294

95+
expect(core.withIsolationScope).toHaveBeenCalled();
9396
expect(core.startSpan).toHaveBeenCalledWith(
9497
expect.objectContaining({
9598
name: 'event Symbol(test.event)',
@@ -105,6 +108,7 @@ describe('Nest', () => {
105108

106109
await descriptor.value();
107110

111+
expect(core.withIsolationScope).toHaveBeenCalled();
108112
expect(core.startSpan).toHaveBeenCalledWith(
109113
expect.objectContaining({
110114
name: 'event test.event1,test.event2',
@@ -120,6 +124,7 @@ describe('Nest', () => {
120124

121125
await descriptor.value();
122126

127+
expect(core.withIsolationScope).toHaveBeenCalled();
123128
expect(core.startSpan).toHaveBeenCalledWith(
124129
expect.objectContaining({
125130
name: 'event Symbol(test.event1),Symbol(test.event2)',
@@ -135,6 +140,7 @@ describe('Nest', () => {
135140

136141
await descriptor.value();
137142

143+
expect(core.withIsolationScope).toHaveBeenCalled();
138144
expect(core.startSpan).toHaveBeenCalledWith(
139145
expect.objectContaining({
140146
name: 'event Symbol(test.event1),test.event2,Symbol(test.event3)',

0 commit comments

Comments
 (0)