Fix the flaky Composio boot test: record only the vendor's own request (#1035)

The "key it was given is the key the vendor sends" test replaced the
process-wide fetch and process.on for the second and a half the raw client
spends retrying a refused connection. bun test runs every file in one
process, and the browser extension's tests leave timers running (its
activity heartbeat, connection check and streamer reconnect) that keep
calling fetch long after their files finish. When one of them fell inside
that window it was recorded as a keyless attempt to 127.0.0.1, and the test
failed with "Received: null" while Composio's client was behaving.

The recorders now apply only inside an AsyncLocalStorage scope around the
test body; calls from anywhere else pass through to the real globals. The
test also waits for the refusal with a plain await instead of
expect().rejects, because Bun waits on that by running the event loop
inside the call, which runs other files' timers nested in the scope.

Author: davidmckayv

server/tests/composio-adapter.test.ts

@@ -1,4 +1,5 @@
 import { describe, expect, test } from "bun:test";
+import { AsyncLocalStorage } from "node:async_hooks";
 import { Composio, telemetry } from "@composio/core";
 import {
   BrokerAccountGoneError,
@@ -6727,6 +6728,19 @@ describe("the key becoming a vendor, and which delete that vendor carries", () =
  * is opened either way, and the assertions are lists of what the vendor TRIED — which is the fact
  * worth pinning, since a flag flipped back would be a try that succeeded in production.
  *
+ * AND THE RECORDERS RECORD ONLY THE VENDOR UNDER TEST, because both globals belong to the whole
+ * process and `bun test` runs every test file in that one process. Work an EARLIER file left
+ * running reaches the same `fetch`: the browser extension's tests leave its activity heartbeat,
+ * its connection check and its streamer's reconnect timers behind, and those keep posting to
+ * `http://127.0.0.1:…/api/recorder/…` minutes after their own files have finished. One of them
+ * landing inside the second-and-a-half the raw client spends retrying below was recorded as an
+ * attempt with no key, to a host that is not Composio's, and failed this suite on CI while the
+ * vendor itself was behaving. So the body runs inside an `AsyncLocalStorage` scope, which every
+ * microtask, timer and promise it starts inherits, and anything calling either global from
+ * outside that scope is passed straight through to the real one, untouched and unrecorded, as if
+ * this test were not running. The scope holds only while `body` waits the ordinary way, which is
+ * why the helper says how it must wait.
+ *
  * THE HANDLER HALF WOULD HAVE BEEN A VACUOUS ASSERTION AND IS NOT, which is worth knowing before
  * anybody simplifies it. `registerExitHandlers` guards on an `exitHandlersRegistered` flag held by
  * a MODULE-LEVEL singleton (`src/telemetry/Telemetry.ts:46,315-321`), so it fires at most once per
@@ -6741,7 +6755,15 @@ describe("what constructing the vendor is allowed to do on boot", () => {
   type Attempt = { url: string; apiKey: string | null };
 
   /**
-   * Runs `body` with the network and the process's own listener registry replaced by recorders.
+   * Runs `body` with the network and the process's own listener registry replaced by recorders,
+   * for `body` and whatever it sets going, and for nothing else in the process.
+   *
+   * `body` MUST WAIT WITH A PLAIN `await`, NEVER WITH `expect(...).rejects` OR `.resolves`. Bun
+   * waits for the promise those are given by running the event loop inside the call, so every
+   * other file's timer that falls due meanwhile runs nested in `body`'s scope, inherits it, and is
+   * recorded as the vendor's own. That was observed with Bun 1.4.3: with the scope below in place
+   * and `expect(broker.listApps()).rejects` still in the test, the extension streamer's reconnect
+   * was recorded with no key, its stack running through `run` and this file's `body`.
    *
    * `NODE_ENV` is forced away from `test` for the duration because the SDK's own
    * `shouldSendTelemetry()` short-circuits on `test` and `ci`
@@ -6760,7 +6782,18 @@ describe("what constructing the vendor is allowed to do on boot", () => {
     const memo = telemetry as unknown as { exitHandlersRegistered: boolean };
     const realMemo = memo.exitHandlersRegistered;
 
+    /*
+     * WHOSE CALL IT IS, WHICH IS WHAT MAKES THE RECORDING THIS TEST'S. A fresh scope per run, so
+     * `getStore()` is true only in `body`'s own async descendants: the client it constructs, the
+     * request it builds, the raw client's retry timers, and the SDK's deferred telemetry. A caller
+     * outside it is some other file's leftover work (see the describe's header), and it gets the
+     * real global back as though nothing here were installed.
+     */
+    const vendorUnderTest = new AsyncLocalStorage<true>();
+
     globalThis.fetch = ((input: unknown, init?: RequestInit) => {
+      if (!vendorUnderTest.getStore())
+        return realFetch(input as Parameters<typeof fetch>[0], init);
       const request = input as { url?: string; headers?: HeadersInit };
       const headers = new Headers(init?.headers ?? request?.headers ?? {});
       attempts.push({
@@ -6772,8 +6805,10 @@ describe("what constructing the vendor is allowed to do on boot", () => {
       return Promise.reject(new Error("no network in this test"));
     }) as typeof globalThis.fetch;
 
-    process.on = ((event: string) => {
-      events.push(event);
+    process.on = ((...registration: Parameters<typeof process.on>) => {
+      if (!vendorUnderTest.getStore())
+        return realOn.apply(process, registration);
+      events.push(String(registration[0]));
       return process;
     }) as typeof process.on;
 
@@ -6792,11 +6827,13 @@ describe("what constructing the vendor is allowed to do on boot", () => {
     memo.exitHandlersRegistered = false;
 
     try {
-      await body();
-      // One turn of the macrotask queue, because the SDK defers its initialisation metric through
-      // `queueMicrotask` (`src/telemetry/Telemetry.ts:241-257`) — a `setTimeout` lands after every
-      // microtask that defer could have queued, so the recorder sees the POST before it is undone.
-      await new Promise((resolve) => setTimeout(resolve, 0));
+      await vendorUnderTest.run(true, async () => {
+        await body();
+        // One turn of the macrotask queue, because the SDK defers its initialisation metric through
+        // `queueMicrotask` (`src/telemetry/Telemetry.ts:241-257`) — a `setTimeout` lands after every
+        // microtask that defer could have queued, so the recorder sees the POST before it is undone.
+        await new Promise((resolve) => setTimeout(resolve, 0));
+      });
     } finally {
       globalThis.fetch = realFetch;
       process.on = realOn;
@@ -6837,12 +6874,18 @@ describe("what constructing the vendor is allowed to do on boot", () => {
   });
 
   test("the key it was given is the key the vendor sends, to Composio's own host", async () => {
+    let refusal: unknown;
     const { attempts } = await withRecordedTransport(async () => {
       const { broker } = createComposioClient("never-dialled");
       // It rejects because the transport above rejects; what is under test is the request that was
-      // built before it did, not the refusal that comes back.
-      await expect(broker.listApps()).rejects.toThrow();
+      // built before it did, not the refusal that comes back. Caught with a plain `await` and not
+      // `expect(...).rejects` for the reason given on `withRecordedTransport`.
+      refusal = await broker.listApps().then(
+        () => undefined,
+        (error: unknown) => error,
+      );
     });
+    expect(refusal).toBeInstanceOf(Error);
 
     /*
      * EVERY attempt, not the first one, because the claim worth making is about all of them. The