fix(core): handle async op promise id wraparound (#35126)

The async op promise id is incremented with `(id + 1) & 0xffffffff`,
which coerces to a signed int32 in JS, so after 2^31 dispatched ops the
counter goes negative. From that point the `promiseId < nextPromiseId -
RING_SIZE` check in `__resolvePromise()` misfires for ops that started before the
wraparound and were evicted from the promise ring to the overflow map
while still pending: the lookup goes to the ring instead of the map and the
process crashes with "Missing promise in ring @ <id>" (or could resolve
a wrong promise). This is what killed long-running servers after billions
of ops. The new regression test reproduces that exact error against the old
code.

The counter now wraps back to 0 instead of going negative (promise ids
must stay non-negative i32s for the Rust side), and each ring entry stores
its own promise id, so ring-vs-map membership is decided by an identity
check instead of arithmetic on the global counter, which is wraparound safe.

Fixes #22759

Author: bartlomieju

libs/core/00_infra.js

@@ -29,6 +29,8 @@
     URIError,
   } = window.__bootstrap.primordials;
 
+  // Promise ids must be non-negative and fit in an i32 (Rust's `PromiseId`);
+  // the increment in the async op stubs wraps back to 0 past 2^31 - 1.
   let nextPromiseId = 0;
   const promiseMap = new SafeMap();
   const RING_SIZE = 4 * 1024;
@@ -41,6 +43,11 @@
   let isLeakTracingEnabled = false;
   let submitLeakTrace;
 
+  // Exposed for testing promise id wraparound behavior.
+  function __setNextPromiseId(promiseId) {
+    nextPromiseId = promiseId;
+  }
+
   function __setLeakTracingEnabled(enabled) {
     isLeakTracingEnabled = enabled;
   }
@@ -140,12 +147,17 @@
     // Move old promise from ring to map
     const oldPromise = promiseRing[idx];
     if (oldPromise !== NO_PROMISE) {
-      const oldPromiseId = promiseId - RING_SIZE;
-      MapPrototypeSet(promiseMap, oldPromiseId, oldPromise);
+      // Keyed on the entry's own id rather than `promiseId - RING_SIZE` so it
+      // stays correct across the id counter wrapping back to 0. The one
+      // unavoidable limit: if a single promise stays pending while 2^31 more
+      // ids are dispatched its id is reused, and this set silently overwrites
+      // (loses) the older map entry. Not a regression: the old code broke far
+      // sooner, and no real workload keeps an op pending across 2^31 dispatches.
+      MapPrototypeSet(promiseMap, oldPromise[2], oldPromise);
     }
 
     const promise = new Promise((resolve, reject) => {
-      promiseRing[idx] = [resolve, reject];
+      promiseRing[idx] = [resolve, reject, promiseId];
     });
     const wrappedPromise = PromisePrototypeCatch(
       promise,
@@ -160,44 +172,39 @@
   }
 
   function __resolvePromise(promiseId, res, isOk) {
-    // Check if out of ring bounds, fallback to map
-    const outOfBounds = promiseId < nextPromiseId - RING_SIZE;
-    if (outOfBounds) {
-      const promise = MapPrototypeGet(promiseMap, promiseId);
+    // A promise stays in the ring until its slot is reclaimed by a newer
+    // promise, at which point it moves to the map. The entry stores its own
+    // promise id, so a slot reused after the id counter wrapped around is
+    // never mistaken for an older promise that is now in the map.
+    const idx = promiseId % RING_SIZE;
+    let promise = promiseRing[idx];
+    if (promise !== NO_PROMISE && promise[2] === promiseId) {
+      promiseRing[idx] = NO_PROMISE;
+    } else {
+      promise = MapPrototypeGet(promiseMap, promiseId);
       if (!promise) {
-        throw "Missing promise in map @ " + promiseId;
+        throw "Missing promise @ " + promiseId;
       }
       MapPrototypeDelete(promiseMap, promiseId);
-      if (isOk) {
-        promise[0](res);
-      } else {
-        promise[1](res);
-      }
+    }
+    if (isOk) {
+      promise[0](res);
     } else {
-      // Otherwise take from ring
-      const idx = promiseId % RING_SIZE;
-      const promise = promiseRing[idx];
-      if (!promise) {
-        throw "Missing promise in ring @ " + promiseId;
-      }
-      promiseRing[idx] = NO_PROMISE;
-      if (isOk) {
-        promise[0](res);
-      } else {
-        promise[1](res);
-      }
+      promise[1](res);
     }
   }
 
   function hasPromise(promiseId) {
-    // Check if out of ring bounds, fallback to map
-    const outOfBounds = promiseId < nextPromiseId - RING_SIZE;
-    if (outOfBounds) {
-      return MapPrototypeHas(promiseMap, promiseId);
-    }
-    // Otherwise check it in ring
     const idx = promiseId % RING_SIZE;
-    return promiseRing[idx] != NO_PROMISE;
+    // Loose comparison on purpose: `promiseId` can be `undefined` (e.g.
+    // `unrefOpPromise()` with a promise from an op that completed eagerly and
+    // has no promise id attached), making `idx` NaN and the ring lookup
+    // return `undefined` instead of the `NO_PROMISE` (null) sentinel.
+    const promise = promiseRing[idx];
+    if (promise != NO_PROMISE && promise[2] === promiseId) {
+      return true;
+    }
+    return MapPrototypeHas(promiseMap, promiseId);
   }
 
   function setUpAsyncStub(opName, originalOp, maybeProto) {
@@ -223,7 +230,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -243,7 +250,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -263,7 +270,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -283,7 +290,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -303,7 +310,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -323,7 +330,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -343,7 +350,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -363,7 +370,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -383,7 +390,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -403,7 +410,7 @@
           if (isLeakTracingEnabled) {
             submitLeakTrace(id);
           }
-          nextPromiseId = (id + 1) & 0xffffffff;
+          nextPromiseId = (id + 1) & 0x7fffffff;
           return setPromise(id);
         };
         break;
@@ -524,6 +531,7 @@
     setUpAsyncStub,
     hasPromise,
     promiseIdSymbol,
+    __setNextPromiseId,
   });
 
   const infra = {

libs/core/core.d.ts

@@ -30,6 +30,10 @@ export namespace core {
    * if there are only "unref" promises left. */
   function unrefOpPromise<T>(promise: Promise<T>): void;
 
+  /** Set the id assigned to the next async op promise. Exposed solely for
+   * testing promise id wraparound behavior. */
+  function __setNextPromiseId(promiseId: number): void;
+
   /**
    * Enables collection of stack traces for sanitizers. This allows for
    * debugging of where a given async op was started. Deno CLI uses this for

libs/core/rebuild_async_stubs.js

@@ -21,7 +21,7 @@ function __TEMPLATE__(__ARGS_PARAM__) {
   if (isLeakTracingEnabled) {
     submitLeakTrace(id);
   }
-  nextPromiseId = (id + 1) & 0xffffffff;
+  nextPromiseId = (id + 1) & 0x7fffffff;
   return setPromise(id);
 }
 

libs/core_testing/unit/ops_async_test.ts

@@ -60,3 +60,42 @@ test(async function promiseId() {
   assert(typeof id === "number");
   assert(id > 0);
 });
+
+// Promise ids are i32s that wrap back to 0 past 2^31 - 1. Test that an op
+// that is still pending when the wraparound happens (and whose ring slot has
+// been reclaimed in the meantime, moving it to the overflow map) resolves
+// correctly. https://github.com/denoland/deno/issues/22759
+test(async function testPromiseIdWraparound() {
+  const distanceToWrap = 5000;
+  Deno.core.__setNextPromiseId(2 ** 31 - distanceToWrap);
+
+  barrierCreate("wraparound_barrier", 2);
+  const pending = barrierAwait("wraparound_barrier");
+
+  // Churn through enough ops that `pending` is evicted from the promise ring
+  // (RING_SIZE == 4096 ids later) and the id counter wraps past 2^31 - 1.
+  for (let i = 0; i < distanceToWrap + 1000; i++) {
+    await asyncYield();
+  }
+
+  // The counter must have wrapped back to a small non-negative id.
+  const id = await asyncPromiseId();
+  assert(id >= 0);
+  assert(id < distanceToWrap);
+
+  // Releasing the barrier resolves `pending`, which by now lives in the
+  // overflow map on the far side of the wraparound.
+  await Promise.all([pending, barrierAwait("wraparound_barrier")]);
+});
+
+// Ops that complete eagerly return a plain resolved promise with no promise
+// id attached, so ref/unref of such a promise must be a silent no-op.
+// `hasPromise(undefined)` reads `promiseRing[NaN]`, which would throw on the
+// `promise[2]` identity check if the sentinel comparison were strict; the
+// loose `!=` against `NO_PROMISE` keeps it a no-op.
+test(async function testRefUnrefPromiseWithoutId() {
+  const eager = Promise.resolve("eager");
+  Deno.core.refOpPromise(eager);
+  Deno.core.unrefOpPromise(eager);
+  await eager;
+});