From 0c1e91d977ed79e0adcad4226a9d5aa4e42d41b9 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Fri, 14 Nov 2025 11:27:49 -0800 Subject: [PATCH 1/3] Revert "fix: `setTraceAttributes` cross trace attributes leakage issue (#200)" This reverts commit 7038775985221bb6cba947cdb91a41ed27c684a9. --- .../MutableAsyncLocalStorageContextManager.ts | 5 +- .../src/__tests__/setTraceAttributes.test.ts | 150 ------------------ 2 files changed, 2 insertions(+), 153 deletions(-) delete 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 8a439536..1b49eaf9 100644 --- a/packages/node-opentelemetry/src/MutableAsyncLocalStorageContextManager.ts +++ b/packages/node-opentelemetry/src/MutableAsyncLocalStorageContextManager.ts @@ -44,9 +44,8 @@ export class MutableAsyncLocalStorageContextManager extends AbstractAsyncHooksCo thisArg?: ThisParameterType, ...args: A ): ReturnType { - // Create a fresh mutableContext for each new context to prevent - // cross-contamination between concurrent requests - const mutableContext = { + const mutableContext = this._asyncLocalStorage.getStore() + ?.mutableContext ?? { traceAttributes: new Map(), }; diff --git a/packages/node-opentelemetry/src/__tests__/setTraceAttributes.test.ts b/packages/node-opentelemetry/src/__tests__/setTraceAttributes.test.ts deleted file mode 100644 index 1f585f33..00000000 --- a/packages/node-opentelemetry/src/__tests__/setTraceAttributes.test.ts +++ /dev/null @@ -1,150 +0,0 @@ -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 677fc7ec614e7a94d2fb36e1c5b85cf2d08a0208 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Fri, 14 Nov 2025 12:18:00 -0800 Subject: [PATCH 2/3] docs: add a changeset --- .changeset/early-mayflies-tickle.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/early-mayflies-tickle.md diff --git a/.changeset/early-mayflies-tickle.md b/.changeset/early-mayflies-tickle.md new file mode 100644 index 00000000..eadc4d96 --- /dev/null +++ b/.changeset/early-mayflies-tickle.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/node-opentelemetry': patch +--- + +revert: fix: setTraceAttributes cross trace attributes leakage issue + From 9a5fb09fd048fef132b356b38e659dc250014e7e Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Fri, 14 Nov 2025 12:54:23 -0800 Subject: [PATCH 3/3] ci: keep tests --- .../src/__tests__/setTraceAttributes.test.ts | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 packages/node-opentelemetry/src/__tests__/setTraceAttributes.test.ts 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..1e20bb0f --- /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(); + + // FIXME: the ctx2 should be fresh + expect(ctx2?.traceAttributes.get('userId')).toBe('user-1'); + expect(ctx2?.traceAttributes.get('sharedKey')).toBe('parent-value'); + + // 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'); + }); + + // FIXME: After exiting child context, parent context shouldn't be overwritten + const ctx1Again = contextManager.getMutableContext(); + expect(ctx1Again?.traceAttributes.get('userId')).toBe('user-2'); + expect(ctx1Again?.traceAttributes.get('sharedKey')).toBe('child-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]); + }); +});