From 6a6846703efd5ad20e02d5efc55b28a26d5b784f Mon Sep 17 00:00:00 2001 From: Jordan Baczuk Date: Wed, 23 Sep 2026 13:33:27 -0600 Subject: [PATCH] fix(meteor-promise): create pool fibers without inherited async context Pool fibers are shared between unrelated callers, but each one was created inside whatever AsyncLocalStorage context happened to be active when the pool needed to grow, and the fibers fork copies that context onto the fiber's own AsyncResource. The fiber's own scope is what is active while it parks itself with Fiber.yield() between jobs, so every idle yield reported the creator's context (e.g. the OpenTelemetry root span of a method that finished days ago) for the rest of the process. In prod this showed up as ~11k/s of meteor_fiber_yields_total attributed to methods that were not running. - Create pool fibers inside a module-load-time AsyncResource so they inherit no stores from their creator. - Flag the fiber with _meteorPromisePoolIdle around the idle yield so instrumentation wrapping Fiber.yield can tell the park apart from a real yield. - Add a test that fails against the previous fiber_pool.js. Job callbacks still run inside their own per-job AsyncResource and are unaffected. meteor-promise 0.9.1-2 -> 0.9.1-3, promise 0.12.2 -> 0.12.3. Co-Authored-By: Claude Fable 5.1 --- npm-packages/meteor-promise/fiber_pool.js | 16 ++++- npm-packages/meteor-promise/package.json | 2 +- npm-packages/meteor-promise/test/tests.js | 72 +++++++++++++++++++++++ packages/promise/package.js | 4 +- 4 files changed, 88 insertions(+), 6 deletions(-) diff --git a/npm-packages/meteor-promise/fiber_pool.js b/npm-packages/meteor-promise/fiber_pool.js index 99565229b62..e2cfb29a77e 100644 --- a/npm-packages/meteor-promise/fiber_pool.js +++ b/npm-packages/meteor-promise/fiber_pool.js @@ -1,6 +1,13 @@ var assert = require("assert"); const { AsyncResource } = require("async_hooks"); +// Pool fibers are shared infrastructure: they must not inherit the AsyncLocalStorage stores +// (OpenTelemetry context, etc.) of whichever caller happened to create them, because the +// fiber's own async scope is what is active while it is parked between jobs. This resource is +// created at module load, before any request context exists, so anything created inside it +// starts with no stores at all. +const PRISTINE_ASYNC_SCOPE = new AsyncResource("FiberPoolRoot"); + function FiberPool(targetFiberCount) { assert.ok(this instanceof FiberPool); assert.strictEqual(typeof targetFiberCount, "number"); @@ -12,10 +19,13 @@ function FiberPool(targetFiberCount) { // with our processing of the callback queue. var originalYield = Fiber.yield; - var fiber = new Fiber(function () { + var fiber = PRISTINE_ASYNC_SCOPE.runInAsyncScope(() => new Fiber(function () { while (fiber) { - // Call Fiber.yield() to await further instructions. + // Call Fiber.yield() to await further instructions. Flag the fiber as idle so that + // instrumentation wrapping Fiber.yield can tell this park apart from a real yield. + fiber._meteorPromisePoolIdle = true; var entry = originalYield.call(Fiber); + delete fiber._meteorPromisePoolIdle; if (! (entry && typeof entry.callback === "function" && @@ -64,7 +74,7 @@ function FiberPool(targetFiberCount) { return; } } - }); + })); // Run the new Fiber up to the first yield point, so that it will be // ready to receive entries. diff --git a/npm-packages/meteor-promise/package.json b/npm-packages/meteor-promise/package.json index 90d783ca367..f9f049c4590 100644 --- a/npm-packages/meteor-promise/package.json +++ b/npm-packages/meteor-promise/package.json @@ -1,7 +1,7 @@ { "name": "meteor-promise", "author": "Ben Newman ", - "version": "0.9.1-2", + "version": "0.9.1-3", "description": "ES6 Promise polyfill with Fiber support", "keywords": [ "meteor", diff --git a/npm-packages/meteor-promise/test/tests.js b/npm-packages/meteor-promise/test/tests.js index 5fbb872121b..916fdf2dba6 100644 --- a/npm-packages/meteor-promise/test/tests.js +++ b/npm-packages/meteor-promise/test/tests.js @@ -409,3 +409,75 @@ describe("stack traces", function () { }); })); }); + +describe("fiber pool async context", function () { + var AsyncLocalStorage = require("async_hooks").AsyncLocalStorage; + var als = new AsyncLocalStorage(); + + // Pool fibers are shared between unrelated callers. Between jobs each pool fiber parks + // itself with Fiber.yield() in its own async scope, so that scope must not carry the + // AsyncLocalStorage stores of whichever caller happened to create the fiber. + // + // Only meaningful with native promises: the `promise` polyfill schedules reactions through + // asap, which batches unrelated callbacks under a single async context. + var itWithNativePromise = Promise === global.Promise ? it : it.skip; + itWithNativePromise("does not expose the creator's ALS store at the idle yield", function () { + var GROW = 30; // > targetFiberCount, so new pool fibers must be created under "creator" + var USE = 50; + var recording = false; + var seenAtYields = []; + var seenAtIdleYields = []; + var originalYield = Fiber.yield; + Fiber.yield = function () { + if (recording) { + seenAtYields.push(als.getStore()); + if (Fiber.current && Fiber.current._meteorPromisePoolIdle) { + seenAtIdleYields.push(als.getStore()); + } + } + return originalYield.apply(this, arguments); + }; + + return als.run("creator", function () { + return Promise.all(Array.from({ length: GROW }, function () { + return Promise.asyncApply(function () { Promise.await(wait(20)); }); + })); + }).then(function () { + recording = true; + return als.run("user", function () { + var chain = Promise.resolve(); + var seenInsideJobs = []; + for (var i = 0; i < USE; ++i) { + chain = chain.then(function () { + return Promise.asyncApply(function () { return als.getStore(); }); + }).then(function (store) { seenInsideJobs.push(store); }); + } + return chain.then(function () { return seenInsideJobs; }); + }); + }).then(function (seenInsideJobs) { + recording = false; + Fiber.yield = originalYield; + + // Inside a job the store is the submitter's. + assert.strictEqual(seenInsideJobs.length, USE); + seenInsideJobs.forEach(function (store) { assert.strictEqual(store, "user"); }); + + // No yield made while only "user" was running may see "creator". Before the fix every + // pool fiber's idle yield did, because the fiber inherited its creator's stores. + assert.ok(seenAtYields.length >= USE, "expected at least one yield per job, saw " + seenAtYields.length); + seenAtYields.forEach(function (store) { + assert.notStrictEqual(store, "creator", "a yield during the user phase saw the creator's store"); + }); + + // The pool flags its idle yield, and that yield carries no store at all. + assert.ok(seenAtIdleYields.length >= USE, "expected a flagged idle yield per job, saw " + seenAtIdleYields.length); + seenAtIdleYields.forEach(function (store) { + assert.strictEqual(store, undefined, "idle yield saw store " + JSON.stringify(store)); + }); + }, function (error) { + recording = false; + Fiber.yield = originalYield; + throw error; + }); + }); +}); diff --git a/packages/promise/package.js b/packages/promise/package.js index fbf4bc5cde1..daf8e72047a 100644 --- a/packages/promise/package.js +++ b/packages/promise/package.js @@ -1,6 +1,6 @@ Package.describe({ name: "promise", - version: "0.12.2", + version: "0.12.3", summary: "ECMAScript 2015 Promise polyfill with Fiber support", git: "https://github.com/meteor/promise", documentation: "README.md" @@ -8,7 +8,7 @@ Package.describe({ Npm.depends({ // TODO: can we just drop this now? - "meteor-promise": "0.9.1-2", + "meteor-promise": "0.9.1-3", "promise": "8.1.0" });