From 18994bf5c6bf7f5b523f5d67f4ac38624eff0911 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Thu, 16 Jul 2026 15:16:32 -0400 Subject: [PATCH 1/2] fix: make the code transform idempotent per channel Re-running the transform over already-instrumented output wrapped the target function a second time instead of detecting and skipping it. The name-based esquery selectors re-match the outer wrapper on a second pass (the `[async]` token is structural and the wrapper keeps the target's name), and while the top-of-file channel-const injection was deduped, the per-function body wrap had no guard. Two wraps on the same channel publish to the same channel constant, so one real call fires the channel lifecycle twice and every subscriber double-fires (duplicate spans). This happens whenever the same code is transformed twice on the same channel, e.g. a build-time bundler pass plus a load-time `--import` pass, or two vendors using the shared `orchestrion::` naming. Guard each function wrap with a per-channel check. `traceFunction` walks down the `__apm$wrapped` closure chain and skips if the function is already wrapped for this channel at any depth, so a re-run is a no-op even after a different channel has been nested on top. A different channel name is still allowed to wrap (independent APMs coexist, each firing once). Runtime instance-method patching stays per-method since its constructor patch reuses a single `const __apm$` binding. Idempotency is verified for Sync/Async/Callback across function declarations, expressions, class methods, and constructor-patched instance methods. --- lib/transforms.js | 116 +++++++++++++++++++++++++++++++++++++++++++ tests/tests.test.mjs | 69 +++++++++++++++++++++++++ 2 files changed, 185 insertions(+) diff --git a/lib/transforms.js b/lib/transforms.js index 9ed138d..4b610e4 100644 --- a/lib/transforms.js +++ b/lib/transforms.js @@ -124,6 +124,107 @@ function traceAny (state, node, _parent, ancestry) { } } +/** + * Returns `true` if `stmt` is a `return .start.runStores(...)` + * statement, the shape every wrapper template + * (`wrapSync`/`wrapPromise`/`wrapCallback`/`wrapAuto`) ends with. + * + * @param {import('estree').Node} stmt + * @param {string} channelVariable + * @returns {boolean} + */ +function isChannelRunStoresReturn (stmt, channelVariable) { + if (stmt.type !== 'ReturnStatement') return false + const callee = stmt.argument?.type === 'CallExpression' ? stmt.argument.callee : null + return callee?.type === 'MemberExpression' && + callee.property?.name === 'runStores' && + callee.object?.type === 'MemberExpression' && + callee.object.property?.name === 'start' && + callee.object.object?.type === 'Identifier' && + callee.object.object.name === channelVariable +} + +/** + * Given the body statements of a wrapper function, returns the function that was + * moved into its `__apm$traced` closure as `const __apm$wrapped = ` (i.e. the + * next layer down: either an inner wrapper or the original function). Returns + * `null` when there is no such closure, which marks the bottom of the stack (the + * original, un-wrapped body). + * + * @param {import('estree').Statement[]} body + * @returns {import('estree').Function|null} + */ +function nextWrappedFunction (body) { + const declInit = (stmts, name) => { + const decl = stmts?.find(stmt => + stmt.type === 'VariableDeclaration' && + stmt.declarations.some(d => d.id?.name === name) + ) + return decl?.declarations.find(d => d.id?.name === name)?.init ?? null + } + + const traced = declInit(body, '__apm$traced') + const wrapped = declInit(traced?.body?.body, '__apm$wrapped') + const type = wrapped?.type + return type === 'FunctionExpression' || type === 'ArrowFunctionExpression' || type === 'FunctionDeclaration' + ? wrapped + : null +} + +/** + * Returns `true` if `node` is already wrapped for `channelVariable` at any layer + * of the wrapper stack this transform builds. + * + * The check is scoped to the specific channel (not just "is this instrumented at + * all") on purpose: re-wrapping the same function on the *same* channel + * double-publishes to one channel and is the double-instrumentation bug we skip, + * whereas wrapping it on a *different* channel is a second, independent APM whose + * instrumentation must not be silently dropped. + * + * It walks down the `__apm$wrapped` closure chain rather than only inspecting the + * outermost layer, so an already-wrapped channel is still detected after a + * different channel has been wrapped on top of it. The walk only follows this + * transform's own scaffolding, so unrelated nested functions in the original body + * are never mistaken for a wrapper. + * + * @param {import('estree').Node} node - A function-like node. + * @param {string} channelVariable - The channel variable name for this config. + * @returns {boolean} + */ +function isWrappedForChannel (node, channelVariable) { + const visited = new Set() + let fn = node + while (fn && !visited.has(fn)) { + visited.add(fn) + const body = fn.body?.body + if (!Array.isArray(body)) return false + if (body.some(stmt => isChannelRunStoresReturn(stmt, channelVariable))) return true + fn = nextWrappedFunction(body) + } + return false +} + +/** + * Returns `true` if `ctor` already contains the constructor-patch scaffolding for + * `methodName`, i.e. a `const __apm$ = this[""]` + * declaration injected by {@link traceInstanceMethod}. Used to keep + * re-transforming already instrumented output idempotent for instance methods + * that are wrapped at runtime inside the constructor. + * + * @param {import('estree').MethodDefinition} ctor - The class constructor node. + * @param {string} methodName + * @returns {boolean} + */ +function constructorPatchesMethod (ctor, methodName) { + const marker = `__apm$${methodName}` + const body = ctor?.value?.body?.body + if (!Array.isArray(body)) return false + return body.some(stmt => + stmt.type === 'VariableDeclaration' && + stmt.declarations.some(decl => decl.id?.name === marker) + ) +} + /** * Wraps a function node's body with diagnostics_channel tracing. * @@ -135,6 +236,13 @@ function traceAny (state, node, _parent, ancestry) { * @param {import('estree').Program} program */ function traceFunction (state, node, program) { + // Idempotency guard: the name-based esquery selectors re-match the outer + // wrapper function on a second pass (the `[async]` token is structural, and + // the wrapper keeps the target's name), so bail out if this node is already + // wrapped for this channel instead of double-publishing to it. A different + // channel (a second, independent APM) is intentionally allowed to wrap it. + if (isWrappedForChannel(node, formatChannelVariable(state.channelName))) return + transforms.tracingChannelDeclaration(state, program) const { functionQuery: { methodName, privateMethodName, functionName, expressionName, propertyName } } = state @@ -196,6 +304,14 @@ function traceInstanceMethod (state, node, program) { // wrap it in the constructor instead. let ctor = classBody.body.find(({ kind }) => kind === 'constructor') + // Idempotency guard: a prior pass injects the runtime patch into the + // constructor, so skip if this method has already been patched there. This is + // per-method (not per-channel): the patch reuses a `const __apm$` + // binding in the constructor, so a second wrapper for a different channel + // cannot be nested here without a redeclaration. Runtime-patched instance + // methods therefore support a single instrumentation. + if (ctor && constructorPatchesMethod(ctor, methodName)) return + transforms.tracingChannelDeclaration(state, program) if (!ctor) { diff --git a/tests/tests.test.mjs b/tests/tests.test.mjs index 8b893a3..69dcbd7 100644 --- a/tests/tests.test.mjs +++ b/tests/tests.test.mjs @@ -813,3 +813,72 @@ describe('async_iterator_cjs', () => { ]) }) }) + +describe('idempotency', () => { + const M = { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH } + + const transform = (code, configs) => + create(configs) + .getTransformer(TEST_MODULE_NAME, TEST_MODULE_VERSION, TEST_MODULE_PATH) + .transform(code, 'cjs').code + + const cases = [ + ['class method (Async)', 'class Undici { async fetch (url) { return 42 } }\nmodule.exports = { Undici }\n', + [{ channelName: 'Undici:fetch', module: M, functionQuery: { className: 'Undici', methodName: 'fetch', kind: 'Async' } }]], + ['class method (Sync)', 'class Undici { fetch (url) { return 42 } }\nmodule.exports = { Undici }\n', + [{ channelName: 'Undici:fetch', module: M, functionQuery: { className: 'Undici', methodName: 'fetch', kind: 'Sync' } }]], + ['class method (Callback)', 'class Undici { fetch (url, cb) { cb(null, 42) } }\nmodule.exports = { Undici }\n', + [{ channelName: 'Undici:fetch', module: M, functionQuery: { className: 'Undici', methodName: 'fetch', kind: 'Callback', callbackIndex: 1 } }]], + ['function declaration (Async)', 'async function fetch (url) { return 42 }\nmodule.exports = { fetch }\n', + [{ channelName: 'fetch', module: M, functionQuery: { functionName: 'fetch', kind: 'Async' } }]], + ['function declaration (Sync)', 'function fetch (url) { return 42 }\nmodule.exports = { fetch }\n', + [{ channelName: 'fetch', module: M, functionQuery: { functionName: 'fetch', kind: 'Sync' } }]], + ['function declaration (Callback)', 'function fetch (url, cb) { cb(null, 42) }\nmodule.exports = { fetch }\n', + [{ channelName: 'fetch', module: M, functionQuery: { functionName: 'fetch', kind: 'Callback', callbackIndex: 1 } }]], + ['function expression (Async)', 'const fetch = async function fetch (url) { return 42 }\nmodule.exports = { fetch }\n', + [{ channelName: 'fetch', module: M, functionQuery: { expressionName: 'fetch', kind: 'Async' } }]], + ['instance method via constructor (Async)', + 'class Base {}\nBase.prototype.fetch = async function (url) { return 42 }\nclass Sub extends Base {}\nmodule.exports = { Sub }\n', + [{ channelName: 'Base:fetch', module: M, functionQuery: { className: 'Base', methodName: 'fetch', kind: 'Async' } }]], + ] + + const runStoresCount = (code) => code.split('.start.runStores(').length - 1 + + for (const [label, code, configs] of cases) { + test(`a second pass is a no-op: ${label}`, () => { + const once = transform(code, configs) + const twice = transform(once, configs) + assert.equal(twice, once, 'second pass should not change already-instrumented output') + assert.equal(runStoresCount(twice), runStoresCount(once)) + }) + } + + test('a second config with a different channel wraps independently without double-publishing to either channel', () => { + const code = 'class Undici { async fetch (url) { return 42 } }\nmodule.exports = { Undici }\n' + const once = transform(code, [{ channelName: 'chanA', module: M, functionQuery: { className: 'Undici', methodName: 'fetch', kind: 'Async' } }]) + const twice = transform(once, [{ channelName: 'chanB', module: M, functionQuery: { className: 'Undici', methodName: 'fetch', kind: 'Async' } }]) + + // Both APMs coexist (both channels are wrapped) ... + assert.equal(runStoresCount(twice), 2) + // ... but neither channel is published to more than once, so no channel + // double-fires (the actual duplicate-span bug is same-channel only). + assert.equal(twice.split('tr_ch_apm$chanA.start.runStores(').length - 1, 1) + assert.equal(twice.split('tr_ch_apm$chanB.start.runStores(').length - 1, 1) + }) + + test('does not re-wrap a channel that is nested under a different channel', () => { + const code = 'class Undici { async fetch (url) { return 42 } }\nmodule.exports = { Undici }\n' + const chan = (name) => [{ channelName: name, module: M, functionQuery: { className: 'Undici', methodName: 'fetch', kind: 'Async' } }] + + // chanA (innermost) -> chanB wraps over it -> chanA again: the second chanA + // must be skipped even though chanA is no longer the outermost wrapper. + const a = transform(code, chan('chanA')) + const ab = transform(a, chan('chanB')) + const aba = transform(ab, chan('chanA')) + + assert.equal(aba.split('tr_ch_apm$chanA.start.runStores(').length - 1, 1) + assert.equal(aba.split('tr_ch_apm$chanB.start.runStores(').length - 1, 1) + // Re-applying chanA to the already-chanA-wrapped output is a no-op. + assert.equal(aba, ab) + }) +}) From ecc6291dcee19c75f5da703051a852a454cdeee2 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Thu, 16 Jul 2026 15:32:19 -0400 Subject: [PATCH 2/2] fix: make runtime-patched instance-method guard per-channel The constructor-patch path (for prototype/inherited methods not on the class body) guarded per method, not per channel, so it silently dropped a second, different-channel patch: it assumed any existing patch was ours, even when it belonged to another channel/APM. That was only because the emitted binding reused a single `const __apm$` that could not be redeclared. Namespace the saved-original binding by channel (`const $ = this[""]`) so patches for different channels stack in the constructor (each wraps the previous one), and scope the idempotency guard to that per-channel binding. A re-run of the same config is still a no-op, while a different channel now coexists and fires exactly once, consistent with `traceFunction`. This changes first-pass output for this path only (the injected local variable name). --- lib/transforms.js | 44 +++++++++++++++++++++++--------------------- tests/tests.test.mjs | 19 +++++++++++++++++++ 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/lib/transforms.js b/lib/transforms.js index 4b610e4..380d60f 100644 --- a/lib/transforms.js +++ b/lib/transforms.js @@ -205,23 +205,22 @@ function isWrappedForChannel (node, channelVariable) { } /** - * Returns `true` if `ctor` already contains the constructor-patch scaffolding for - * `methodName`, i.e. a `const __apm$ = this[""]` - * declaration injected by {@link traceInstanceMethod}. Used to keep - * re-transforming already instrumented output idempotent for instance methods - * that are wrapped at runtime inside the constructor. + * Returns `true` if `ctor` already declares `bindingName`, the per-channel + * `const $ = this[""]` binding injected by + * {@link traceInstanceMethod}. Used to keep re-transforming already instrumented + * output idempotent for instance methods wrapped at runtime inside the + * constructor, without dropping a different channel's patch. * * @param {import('estree').MethodDefinition} ctor - The class constructor node. - * @param {string} methodName + * @param {string} bindingName - The channel-namespaced saved-original binding. * @returns {boolean} */ -function constructorPatchesMethod (ctor, methodName) { - const marker = `__apm$${methodName}` +function constructorPatchesMethod (ctor, bindingName) { const body = ctor?.value?.body?.body if (!Array.isArray(body)) return false return body.some(stmt => stmt.type === 'VariableDeclaration' && - stmt.declarations.some(decl => decl.id?.name === marker) + stmt.declarations.some(decl => decl.id?.name === bindingName) ) } @@ -285,7 +284,7 @@ function traceFunction (state, node, program) { * @param {import('estree').Program} program */ function traceInstanceMethod (state, node, program) { - const { functionQuery, operator } = state + const { functionQuery, operator, channelName } = state const { methodName } = functionQuery // No methodName means a constructor-only config — the constructor FunctionExpression @@ -304,13 +303,16 @@ function traceInstanceMethod (state, node, program) { // wrap it in the constructor instead. let ctor = classBody.body.find(({ kind }) => kind === 'constructor') - // Idempotency guard: a prior pass injects the runtime patch into the - // constructor, so skip if this method has already been patched there. This is - // per-method (not per-channel): the patch reuses a `const __apm$` - // binding in the constructor, so a second wrapper for a different channel - // cannot be nested here without a redeclaration. Runtime-patched instance - // methods therefore support a single instrumentation. - if (ctor && constructorPatchesMethod(ctor, methodName)) return + // The binding that captures the previous `this[method]` is namespaced by + // channel so that patches for different channels stack in the constructor + // (each wraps the previous one) rather than colliding on one `const`. + const savedBinding = `${formatChannelVariable(channelName)}$${methodName}` + + // Idempotency guard: skip if this channel has already patched this method into + // the constructor. Scoped per channel (not per method) so a re-run of the same + // config is a no-op while a different channel is still allowed to add its own + // patch, keeping this path consistent with `traceFunction`. + if (ctor && constructorPatchesMethod(ctor, savedBinding)) return transforms.tracingChannelDeclaration(state, program) @@ -325,11 +327,11 @@ function traceInstanceMethod (state, node, program) { } const ctorBody = parse(` - const __apm$${methodName} = this["${methodName}"] + const ${savedBinding} = this["${methodName}"] this["${methodName}"] = function () {} - if (typeof __apm$${methodName} === 'function') { + if (typeof ${savedBinding} === 'function') { Object.defineProperty(this["${methodName}"], 'length', { - value: __apm$${methodName}.length, + value: ${savedBinding}.length, configurable: true }) } @@ -340,7 +342,7 @@ function traceInstanceMethod (state, node, program) { fn.params = [{ type: 'RestElement', argument: { type: 'Identifier', name: '__apm$args' } }] fn.async = operator === 'tracePromise' - fn.body = wrap(state, { type: 'Identifier', name: `__apm$${methodName}` }, program) + fn.body = wrap(state, { type: 'Identifier', name: savedBinding }, program) wrapSuper(state, fn) ctor.value.body.body.push(...ctorBody) diff --git a/tests/tests.test.mjs b/tests/tests.test.mjs index 69dcbd7..b257711 100644 --- a/tests/tests.test.mjs +++ b/tests/tests.test.mjs @@ -881,4 +881,23 @@ describe('idempotency', () => { // Re-applying chanA to the already-chanA-wrapped output is a no-op. assert.equal(aba, ab) }) + + test('runtime-patched instance method: same channel is idempotent, different channel coexists', () => { + // Inherited/prototype method (not on the class body) is wrapped at runtime + // inside the constructor. + const code = 'class Base {}\nBase.prototype.fetch = async function (url) { return 42 }\nclass Sub extends Base {}\nmodule.exports = { Sub }\n' + const chan = (name) => [{ channelName: name, module: M, functionQuery: { className: 'Base', methodName: 'fetch', kind: 'Async' } }] + + // Same channel twice is a no-op. + const a = transform(code, chan('chanA')) + assert.equal(transform(a, chan('chanA')), a) + + // A different channel adds its own patch (both APMs coexist) ... + const ab = transform(a, chan('chanB')) + assert.ok(ab.includes('tracingChannel("orchestrion:undici:chanA")')) + assert.ok(ab.includes('tracingChannel("orchestrion:undici:chanB")')) + // ... and each channel is still patched exactly once. + assert.equal(ab.split('tr_ch_apm$chanA.start.runStores(').length - 1, 1) + assert.equal(ab.split('tr_ch_apm$chanB.start.runStores(').length - 1, 1) + }) })