(fix): Prevent duplicate method chain wrapping

Author: mvogttech

.versions

@@ -46,7 +46,7 @@ reactive-var@1.0.13
 reload@1.3.2
 retry@1.1.1
 routepolicy@1.1.2
-skysignal:agent@1.0.23
+skysignal:agent@1.0.24
 socket-stream-client@0.6.1
 tracker@1.3.4
 typescript@5.9.3

README.md

@@ -854,6 +854,13 @@ Main agent singleton instance.
 
 ## Changelog
 
+### v1.0.25 (Stack Overflow Root Cause Fix)
+
+- **Fix shared `protocol_handlers` wrapper chain buildup (ROOT CAUSE)** - In Meteor, `session.protocol_handlers` is a shared object on the Session prototype — all DDP sessions reference the same object. `_hijackMethodHandler` and `_hijackSubHandler` captured `session.protocol_handlers.method` and replaced it with a wrapper, but because the object is shared, each new session's wrap captured the PREVIOUS session's wrapper, building an N-deep call chain. After ~200 concurrent connections, calling `protocol_handlers.method` would recurse through hundreds of chained wrappers and overflow V8's stack. The fix adds a `_skySignalDDPQueueWrapped` flag on the wrapper function itself, ensuring the handler is wrapped exactly once globally regardless of how many sessions connect. (fixes [#7](https://github.com/SkySignalAPM/agent/issues/7))
+- **Auto-disable DDPQueueCollector when another APM agent is detected** - When `montiapm:agent` or `mdg:meteor-apm-agent` is installed alongside `skysignal:agent`, `DDPQueueCollector.start()` now automatically disables itself instead of merely logging a warning. Both agents wrap the same DDP session internals, and the compounded wrapper depth compounds the stack overflow risk. Users can force-enable via `{ forceEnable: true }` if they accept the risk.
+- **Skip MethodTracer unblock wrapping when another APM agent is detected** - `MethodTracer._wrapMethod()` now skips wrapping `methodInvocation.unblock` when a conflicting APM agent is present. Both agents wrapping unblock adds ~3-5 extra frames per method call. Unblock timing analysis is unavailable in this mode; all other MethodTracer features remain fully functional.
+- **New tests: shared prototype regression** - Added tests simulating 50 sessions sharing the same `protocol_handlers` object, verifying the handler is wrapped exactly once and does not build a deep chain.
+
 ### v1.0.24 (DDP Queue Stack Overflow Fix)
 
 - **Fix secondary stack overflow in `wrapUnblock`** - When `_recordBlockingTime()` threw an error with a very deep stack trace (e.g. from mutual-recursion across wrapper layers), `console.error(error)` triggered `source-map-support`'s `prepareStackTrace` which re-overflowed, causing a secondary `RangeError`. All `console.error` calls in `DDPQueueCollector` now use `String(error)` to serialize the error message without triggering stack trace reprocessing. See [#12](https://github.com/SkySignalAPM/agent/pull/12).

lib/collectors/DDPQueueCollector.js

@@ -13,6 +13,7 @@ import { Meteor } from 'meteor/meteor';
 export default class DDPQueueCollector {
   constructor(options = {}) {
     this.enabled = options.enabled !== false;
+    this.forceEnable = options.forceEnable || false;
     this.debug = options.debug || false;
 
     // Store wait lists for messages (who they're waiting for)
@@ -49,13 +50,31 @@ export default class DDPQueueCollector {
     }
 
     try {
-      // Detect conflicting APM agents that also wrap DDP internals
-      if (Package['montiapm:agent'] || Package['mdg:meteor-apm-agent']) {
+      // Detect conflicting APM agents that also wrap DDP internals.
+      // When another agent wraps the same session methods (processMessage,
+      // protocol_handlers, unblock), the combined wrapper depth causes stack
+      // overflows. Auto-disable to prevent this — the user can force-enable
+      // via `forceEnable: true` in options if needed. See #7.
+      this._conflictingAgent = !!(
+        typeof Package !== 'undefined' &&
+        (Package['montiapm:agent'] || Package['mdg:meteor-apm-agent'])
+      );
+
+      if (this._conflictingAgent) {
         console.warn(
-          '⚠️ DDPQueueCollector: Detected another APM agent (montiapm:agent) that also ' +
-          'wraps DDP session internals. Running multiple APM agents simultaneously can ' +
-          'cause stack overflow errors. Consider removing one of the agents.'
+          '⚠️ DDPQueueCollector: Detected another APM agent (montiapm:agent / mdg:meteor-apm-agent) ' +
+          'that also wraps DDP session internals. Automatically disabling DDPQueueCollector to prevent ' +
+          'stack overflow errors from compounded wrapper depth. DDP queue wait times will not be tracked. ' +
+          'To fix: remove the other APM agent package. ' +
+          'To force-enable (at your own risk): pass { forceEnable: true } to DDPQueueCollector.'
         );
+
+        if (!this.forceEnable) {
+          this.started = false;
+          return;
+        }
+
+        console.warn('⚠️ DDPQueueCollector: Force-enabled despite conflicting agent. Stack overflows may occur.');
       }
 
       this._hijackSessionProcessing();
@@ -237,7 +256,14 @@ export default class DDPQueueCollector {
   }
 
   /**
-   * Hijack method handler to track wait time
+   * Hijack method handler to track wait time.
+   *
+   * CRITICAL: session.protocol_handlers is a SHARED object on the Session
+   * prototype — all sessions reference the SAME protocol_handlers. If we
+   * naively wrap per-session, each new session captures the previous wrapper,
+   * building an N-deep chain that overflows the stack after ~200 sessions.
+   * We must wrap ONCE and use a flag on the wrapper function to prevent
+   * re-wrapping. See #7.
    */
   _hijackMethodHandler(session) {
     const self = this;
@@ -247,6 +273,11 @@ export default class DDPQueueCollector {
       return;
     }
 
+    // If already wrapped (shared across sessions), skip.
+    if (session.protocol_handlers.method._skySignalDDPQueueWrapped) {
+      return;
+    }
+
     // Get the current handler (might already be wrapped by another collector)
     const currentMethodHandler = session.protocol_handlers.method;
 
@@ -255,7 +286,7 @@ export default class DDPQueueCollector {
       session._skySignalOriginalMethodHandler = currentMethodHandler;
     }
 
-    session.protocol_handlers.method = function (msg, unblock) {
+    const wrappedHandler = function (msg, unblock) {
       // Check if we're tracking this message
       if (msg._queueEnterTime) {
         // Calculate wait time
@@ -278,10 +309,15 @@ export default class DDPQueueCollector {
       // more than once (e.g. agent restart during hot code push). See #5.
       return currentMethodHandler.call(this, msg, unblock);
     };
+
+    wrappedHandler._skySignalDDPQueueWrapped = true;
+    session.protocol_handlers.method = wrappedHandler;
   }
 
   /**
-   * Hijack subscription handler to track wait time
+   * Hijack subscription handler to track wait time.
+   *
+   * Same shared-prototype concern as _hijackMethodHandler — wrap ONCE. See #7.
    */
   _hijackSubHandler(session) {
     const self = this;
@@ -291,6 +327,11 @@ export default class DDPQueueCollector {
       return;
     }
 
+    // If already wrapped (shared across sessions), skip.
+    if (session.protocol_handlers.sub._skySignalDDPQueueWrapped) {
+      return;
+    }
+
     // Get the current handler (might already be wrapped by another collector)
     const currentSubHandler = session.protocol_handlers.sub;
 
@@ -299,7 +340,7 @@ export default class DDPQueueCollector {
       session._skySignalOriginalSubHandler = currentSubHandler;
     }
 
-    session.protocol_handlers.sub = function (msg, unblock) {
+    const wrappedHandler = function (msg, unblock) {
       // Check if we're tracking this message
       if (msg._queueEnterTime) {
         // Calculate wait time
@@ -318,6 +359,9 @@ export default class DDPQueueCollector {
       // Call the version we captured (chains to other wrappers if present)
       return currentSubHandler.call(this, msg, unblock);
     };
+
+    wrappedHandler._skySignalDDPQueueWrapped = true;
+    session.protocol_handlers.sub = wrappedHandler;
   }
 
   /**

lib/collectors/MethodTracer.js

@@ -131,6 +131,23 @@ export default class MethodTracer {
       return;
     }
 
+    // Detect conflicting APM agents. When another agent wraps the same
+    // Meteor internals (methods, unblock, DDP handlers), the combined
+    // wrapper depth can cause stack overflows. We skip wrapping unblock
+    // in _wrapMethod to reduce stack frame overhead. See #7.
+    this._hasConflictingAgent = !!(
+      typeof Package !== 'undefined' &&
+      (Package['montiapm:agent'] || Package['mdg:meteor-apm-agent'])
+    );
+
+    if (this._hasConflictingAgent) {
+      this._warn(
+        'Detected another APM agent (montiapm:agent / mdg:meteor-apm-agent). ' +
+        'Reducing wrapper depth to prevent stack overflow. ' +
+        'Unblock timing will not be tracked. Consider removing the other agent.'
+      );
+    }
+
     // Register tracer globally for user access
     global.SkySignalTracer = this;
 
@@ -321,22 +338,26 @@ export default class MethodTracer {
         context: context  // Store reference to context for adding child method operations
       });
 
-      // Wrap this.unblock() to detect when it's called
+      // Wrap this.unblock() to detect when it's called.
+      // Skip when another APM agent is detected — both wrapping unblock
+      // compounds stack depth and is the primary cause of overflow. See #7.
       const methodInvocation = this;
-      const originalUnblock = methodInvocation.unblock;
-      let unblockCalled = false;
-
-      if (originalUnblock && typeof originalUnblock === 'function') {
-        methodInvocation.unblock = function() {
-          if (!unblockCalled) {
-            unblockCalled = true;
-            const unblockTime = Date.now();
-            context.unblockAnalysis.called = true;
-            context.unblockAnalysis.timeToUnblock = unblockTime - startTime;
-            context.unblockAnalysis.callPosition = unblockTime - startTime;
-          }
-          return originalUnblock.call(this);
-        };
+      if (!self._hasConflictingAgent) {
+        const originalUnblock = methodInvocation.unblock;
+        let unblockCalled = false;
+
+        if (originalUnblock && typeof originalUnblock === 'function') {
+          methodInvocation.unblock = function() {
+            if (!unblockCalled) {
+              unblockCalled = true;
+              const unblockTime = Date.now();
+              context.unblockAnalysis.called = true;
+              context.unblockAnalysis.timeToUnblock = unblockTime - startTime;
+              context.unblockAnalysis.callPosition = unblockTime - startTime;
+            }
+            return originalUnblock.call(this);
+          };
+        }
       }
 
       // Helper function to record this method call in the parent's operations array

lib/collectors/SystemMetricsCollector.js

@@ -10,7 +10,7 @@ import v8 from "v8";
 const execAsync = promisify(exec);
 
 // Agent version - must be updated alongside package.js on each release
-const AGENT_VERSION = '1.0.24';
+const AGENT_VERSION = '1.0.25';
 
 // cgroup v1 "unlimited" sentinel: values >= 2^62 mean no limit is set
 const CGROUP_V1_UNLIMITED = 2 ** 62;

package.js

@@ -1,6 +1,6 @@
 Package.describe({
   name: "skysignal:agent",
-  version: "1.0.24",
+  version: "1.0.25",
   summary:
     "SkySignal APM agent for Meteor applications - monitors performance, errors, and system metrics",
   git: "https://github.com/skysignalapm/agent.git",

tests/unit/collectors/DDPQueueCollector.test.js

@@ -50,6 +50,60 @@ describe('DDPQueueCollector', function () {
       const c = new DDPQueueCollector({ enabled: false });
       expect(c.enabled).to.be.false;
     });
+
+    it('accepts forceEnable option', function () {
+      const c = new DDPQueueCollector({ forceEnable: true });
+      expect(c.forceEnable).to.be.true;
+    });
+  });
+
+  describe('conflicting APM agent detection', function () {
+    let collector;
+
+    afterEach(function () {
+      // Stop collector to clear any setInterval handles
+      if (collector) {
+        try { collector.stop(); } catch (_e) { /* ignore */ }
+        collector = null;
+      }
+      // Clean up global Package mock
+      delete global.Package;
+    });
+
+    it('auto-disables when montiapm:agent is detected', function () {
+      global.Package = { 'montiapm:agent': {} };
+      collector = new DDPQueueCollector({ enabled: true });
+      collector.start();
+      expect(collector.started).to.be.false;
+      expect(collector._conflictingAgent).to.be.true;
+    });
+
+    it('auto-disables when mdg:meteor-apm-agent is detected', function () {
+      global.Package = { 'mdg:meteor-apm-agent': {} };
+      collector = new DDPQueueCollector({ enabled: true });
+      collector.start();
+      expect(collector.started).to.be.false;
+    });
+
+    it('does not set _conflictingAgent when no conflicting agent present', function () {
+      global.Package = {};
+      collector = new DDPQueueCollector({ enabled: true });
+      // We can't call start() without Meteor.server (it would try to hijack sessions),
+      // but we can test the detection logic directly by checking what start() would set.
+      // Calling start will error at _hijackSessionProcessing, but _conflictingAgent
+      // is set before that call.
+      try { collector.start(); } catch (_e) { /* expected without Meteor.server */ }
+      expect(collector._conflictingAgent).to.be.false;
+    });
+
+    it('force-enables despite conflicting agent when forceEnable is true', function () {
+      global.Package = { 'montiapm:agent': {} };
+      collector = new DDPQueueCollector({ enabled: true, forceEnable: true });
+      // start() proceeds past the guard and tries _hijackSessionProcessing,
+      // which errors without Meteor.server — but it proves it didn't early-return.
+      try { collector.start(); } catch (_e) { /* expected without Meteor.server */ }
+      expect(collector._conflictingAgent).to.be.true;
+    });
   });
 
   // ==========================================
@@ -298,6 +352,65 @@ describe('DDPQueueCollector', function () {
     });
   });
 
+  describe('shared protocol_handlers (Bug #7 root cause)', function () {
+
+    it('does NOT build N-deep wrapper chain when sessions share protocol_handlers', function () {
+      // In Meteor, session.protocol_handlers is on the Session prototype.
+      // All sessions share the SAME protocol_handlers object. Without the
+      // _skySignalDDPQueueWrapped guard, each _wrapSession call adds another
+      // wrapper layer. After N sessions, calling protocol_handlers.method
+      // recurses N levels deep, causing stack overflow. See #7.
+      const sharedHandlers = {
+        method: sinon.stub(),
+        sub: sinon.stub()
+      };
+
+      // Create 50 sessions sharing the same protocol_handlers object
+      const sessions = [];
+      for (let i = 0; i < 50; i++) {
+        const session = createMockSession(`shared-${i}`);
+        session.protocol_handlers = sharedHandlers; // Shared reference
+        sessions.push(session);
+      }
+
+      // Wrap all 50 sessions
+      sessions.forEach(s => collector._wrapSession(s));
+
+      // protocol_handlers.method should be wrapped exactly ONCE, not 50 times
+      expect(sharedHandlers.method._skySignalDDPQueueWrapped).to.be.true;
+
+      // Calling the handler should NOT overflow — it's 1 wrapper deep, not 50
+      const msg = { id: 'test', msg: 'method', method: 'test.method', _queueEnterTime: Date.now() };
+      expect(() => {
+        sharedHandlers.method.call(sessions[0], msg, () => {});
+      }).to.not.throw();
+    });
+
+    it('wraps protocol_handlers.sub only once across shared sessions', function () {
+      const sharedHandlers = {
+        method: sinon.stub(),
+        sub: sinon.stub()
+      };
+
+      const sessions = [];
+      for (let i = 0; i < 20; i++) {
+        const session = createMockSession(`shared-sub-${i}`);
+        session.protocol_handlers = sharedHandlers;
+        sessions.push(session);
+      }
+
+      sessions.forEach(s => collector._wrapSession(s));
+
+      expect(sharedHandlers.sub._skySignalDDPQueueWrapped).to.be.true;
+
+      // Call the sub handler — should not overflow
+      const msg = { id: 'sub-test', msg: 'sub', name: 'test.pub', _queueEnterTime: Date.now() };
+      expect(() => {
+        sharedHandlers.sub.call(sessions[0], msg, () => {});
+      }).to.not.throw();
+    });
+  });
+
   describe('_wrapSession', function () {
 
     it('sets _skySignalDDPQueueWrapped flag', function () {