From 528f2a848f926a5d470fe85b622403fddde5df26 Mon Sep 17 00:00:00 2001 From: Warren <5959690+wrn14897@users.noreply.github.com> Date: Tue, 11 Nov 2025 18:20:11 -0500 Subject: [PATCH 1/2] fix: setTraceAttributes cross trace attributes leakage issue --- .../MutableAsyncLocalStorageContextManager.ts | 5 +- .../src/__tests__/setTraceAttributes.test.ts | 150 ++++++++++++++++++ 2 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 packages/node-opentelemetry/src/__tests__/setTraceAttributes.test.ts diff --git a/packages/node-opentelemetry/src/MutableAsyncLocalStorageContextManager.ts b/packages/node-opentelemetry/src/MutableAsyncLocalStorageContextManager.ts index 1b49eaf9..8a439536 100644 --- a/packages/node-opentelemetry/src/MutableAsyncLocalStorageContextManager.ts +++ b/packages/node-opentelemetry/src/MutableAsyncLocalStorageContextManager.ts @@ -44,8 +44,9 @@ export class MutableAsyncLocalStorageContextManager extends AbstractAsyncHooksCo thisArg?: ThisParameterType, ...args: A ): ReturnType { - const mutableContext = this._asyncLocalStorage.getStore() - ?.mutableContext ?? { + // Create a fresh mutableContext for each new context to prevent + // cross-contamination between concurrent requests + const mutableContext = { traceAttributes: new Map(), }; diff --git a/packages/node-opentelemetry/src/__tests__/setTraceAttributes.test.ts b/packages/node-opentelemetry/src/__tests__/setTraceAttributes.test.ts new file mode 100644 index 00000000..1f585f33 --- /dev/null +++ b/packages/node-opentelemetry/src/__tests__/setTraceAttributes.test.ts @@ -0,0 +1,150 @@ +import { ROOT_CONTEXT } from '@opentelemetry/api'; + +import { MutableAsyncLocalStorageContextManager } from '../MutableAsyncLocalStorageContextManager'; + +describe('MutableAsyncLocalStorageContextManager - trace attributes isolation', () => { + let contextManager: MutableAsyncLocalStorageContextManager; + + beforeEach(() => { + contextManager = new MutableAsyncLocalStorageContextManager(); + }); + + it('should not leak trace attributes between concurrent contexts', async () => { + const results: Array<{ userId: string; requestId: string }> = []; + + // Simulate two concurrent requests with different trace attributes + await Promise.all([ + new Promise((resolve) => { + contextManager.with(ROOT_CONTEXT, () => { + const ctx = contextManager.getMutableContext(); + ctx?.traceAttributes.set('userId', 'user-1'); + ctx?.traceAttributes.set('requestId', 'req-1'); + + // Simulate async work + setTimeout(() => { + const finalCtx = contextManager.getMutableContext(); + results.push({ + userId: finalCtx?.traceAttributes.get('userId'), + requestId: finalCtx?.traceAttributes.get('requestId'), + }); + resolve(); + }, 10); + }); + }), + + new Promise((resolve) => { + contextManager.with(ROOT_CONTEXT, () => { + const ctx = contextManager.getMutableContext(); + ctx?.traceAttributes.set('userId', 'user-2'); + ctx?.traceAttributes.set('requestId', 'req-2'); + + // Simulate async work + setTimeout(() => { + const finalCtx = contextManager.getMutableContext(); + results.push({ + userId: finalCtx?.traceAttributes.get('userId'), + requestId: finalCtx?.traceAttributes.get('requestId'), + }); + resolve(); + }, 10); + }); + }), + ]); + + // Each context should have its own attributes without cross-contamination + expect(results).toHaveLength(2); + expect(results).toEqual( + expect.arrayContaining([ + { userId: 'user-1', requestId: 'req-1' }, + { userId: 'user-2', requestId: 'req-2' }, + ]), + ); + }); + + it('should create fresh trace attributes for each new context', () => { + // First context sets some attributes + contextManager.with(ROOT_CONTEXT, () => { + const ctx1 = contextManager.getMutableContext(); + ctx1?.traceAttributes.set('userId', 'user-1'); + ctx1?.traceAttributes.set('sharedKey', 'parent-value'); + + expect(ctx1?.traceAttributes.get('userId')).toBe('user-1'); + expect(ctx1?.traceAttributes.get('sharedKey')).toBe('parent-value'); + + // Nested context should have its own fresh trace attributes + contextManager.with(ROOT_CONTEXT, () => { + const ctx2 = contextManager.getMutableContext(); + + // Should NOT have parent's attributes (fresh Map) + expect(ctx2?.traceAttributes.get('userId')).toBeUndefined(); + expect(ctx2?.traceAttributes.get('sharedKey')).toBeUndefined(); + + // Set new attributes in child context + ctx2?.traceAttributes.set('userId', 'user-2'); + ctx2?.traceAttributes.set('sharedKey', 'child-value'); + + expect(ctx2?.traceAttributes.get('userId')).toBe('user-2'); + expect(ctx2?.traceAttributes.get('sharedKey')).toBe('child-value'); + }); + + // After exiting child context, parent should still have original attributes + const ctx1Again = contextManager.getMutableContext(); + expect(ctx1Again?.traceAttributes.get('userId')).toBe('user-1'); + expect(ctx1Again?.traceAttributes.get('sharedKey')).toBe('parent-value'); + }); + }); + + it('should handle rapid sequential context creation without contamination', async () => { + const results: string[] = []; + + // Simulate rapid sequential requests + for (let i = 0; i < 5; i++) { + await new Promise((resolve) => { + contextManager.with(ROOT_CONTEXT, () => { + const ctx = contextManager.getMutableContext(); + const userId = `user-${i}`; + ctx?.traceAttributes.set('userId', userId); + + // Verify the correct userId is set in this context + const actualUserId = ctx?.traceAttributes.get('userId'); + results.push(actualUserId as string); + + resolve(); + }); + }); + } + + // Each request should have had its own userId + expect(results).toEqual(['user-0', 'user-1', 'user-2', 'user-3', 'user-4']); + }); + + it('should maintain separate mutable contexts across parallel operations', async () => { + const contextSnapshots: Array> = []; + + await Promise.all( + Array.from( + { length: 10 }, + (_, i) => + new Promise((resolve) => { + contextManager.with(ROOT_CONTEXT, () => { + const ctx = contextManager.getMutableContext(); + ctx?.traceAttributes.set('index', i); + + setTimeout(() => { + const finalCtx = contextManager.getMutableContext(); + // Clone the Map to preserve its state + contextSnapshots.push( + new Map(finalCtx?.traceAttributes || new Map()), + ); + resolve(); + }, Math.random() * 10); + }); + }), + ), + ); + + // Each context should have its own unique index + const indices = contextSnapshots.map((map) => map.get('index')); + expect(indices.sort()).toEqual([0, 1, 2, 3, 4, 5, 6, 7, 8, 9]); + }); +}); From 4a9441f295ba501565ab5b135ed20c1be8d8f0f4 Mon Sep 17 00:00:00 2001 From: Warren <5959690+wrn14897@users.noreply.github.com> Date: Tue, 11 Nov 2025 18:23:56 -0500 Subject: [PATCH 2/2] docs: add a changeset --- .changeset/funny-bags-switch.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/funny-bags-switch.md diff --git a/.changeset/funny-bags-switch.md b/.changeset/funny-bags-switch.md new file mode 100644 index 00000000..20b90b03 --- /dev/null +++ b/.changeset/funny-bags-switch.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/node-opentelemetry': patch +--- + +fix: setTraceAttributes cross trace attributes leakage issue