Merge pull request #2334 from modelcontextprotocol/v2/fix/2323-test-gate-timeouts

Closes #2323 — state every test-gate timeout instead of inheriting a library default.

.claude/skills/pre-push-gate/SKILL.md

@@ -87,6 +87,22 @@ versions. **Align the versions** — bump it in every install that declares it.
 Do not raise the heap with `--max-old-space-size`; that hides the class rather
 than fixing it.
 
+### `verify:test-timeouts`
+
+A Vitest project resolves to a wall-clock budget nobody stated, or stopped
+loading `vitest.setup.shared.mts`. (A `retry` itself fails at **runtime**, from
+that setup file, with a message naming the test — not here.) The shared values live in `vitest.shared.mts` (`TIMEOUTS` /
+`INTEGRATION_TIMEOUTS`) and every project spreads one of them — so **raise a
+budget there**, not with a per-suite `}, 30_000)` argument, which only moves the
+one site and leaves every future file on the default. A per-suite raise is
+right only where the work is genuinely different (real cross-process lock
+contention, a full OAuth round trip); say so at the site. `retry` stays unset:
+it turns a load-induced red into a silent green on the only pre-push gate here.
+
+A **failing test** is a different problem from a budget — read the failure
+before reaching for a number. An assertion that races is #1596's class and is
+fixed with fake timers or an awaited condition, not with headroom.
+
 ### `lint`
 
 **There is no warning tier** — every `lint` script runs `--max-warnings 0`, so a

.claude/skills/testing/SKILL.md

@@ -255,9 +255,16 @@ renders. The rule stands on consistency, not on timer safety.
   `defaultColorScheme="dark"`.
 - **Mid-flight transition state** (e.g. asserting a `data-anim="out"` cell during
   an exit crossfade) is the only reason to use `renderWithMantineTransitions`.
-  Pass `settleMs` derived from the component's real animation duration
+  Pass `settleMs` derived from the component's real animation duration **plus
-  (`HEADER_ANIM_MS + 200`), do **not** also use `vi.useFakeTimers()` in that test
+  the helper's shared slack** — `HEADER_ANIM_MS + RAF_SLACK_MS`, both imported,
-  (the auto-settle no-ops under fake timers), and if the test unmounts the tree
+  never a literal: the first term tracks the component and the second tracks how
+  busy the machine is, and only the second should move when the machine gets
+  busier (#2323). Do **not** also use `vi.useFakeTimers()` in that test:
+  the auto-settle awaits a real `setTimeout`, so under fake timers it **throws**
+  with a message telling you to call `vi.useRealTimers()` first — it does not
+  silently skip. That is deliberate (a deadlock would otherwise hang until the
+  project's `hookTimeout`), but it means the combination fails the test rather
+  than degrading. If the test unmounts the tree
   itself use the `unmount()` the helper returns. The mechanism is documented at
   length on the helper — read there before changing it.
 

.github/workflows/main.yml

@@ -38,8 +38,9 @@ jobs:
         run: npm install
 
       - name: Validate (coverage guards, format, lint, typecheck, build, fast tests)
-        # Runs the four durable guards first (verify:format-coverage,
+        # Runs the five durable guards first (verify:format-coverage,
-        # verify:skills, verify:typecheck-coverage, verify:dep-lockstep), then
+        # verify:skills, verify:typecheck-coverage, verify:dep-lockstep,
+        # verify:test-timeouts), then
         # test:scripts, then validate:core, then each client's
         # self-validation: format:check + lint + typecheck + build + test (no
         # coverage instrumentation — fast). This also builds every client bundle

AGENTS.md

@@ -396,6 +396,80 @@ diagnose a failing gate — is the `testing` skill. These are the rules.
 - **Render React components through `renderWithMantine`** (`src/test/renderWithMantine.tsx`); do not hand-roll a bare `MantineProvider`, which skips the project theme and the helper's options and drifts from every other test. Pass the `colorScheme` option to exercise a forced scheme rather than hand-rolling `defaultColorScheme`. Use `renderWithMantineTransitions` **only** when a test must assert mid-flight transition state, and read the long comment on the helper before changing anything about it.
 - **The web coverage `include` is a whitelist.** It names `components`/`hooks`/`theme`/`lib`/`utils`/`server` plus the browser-consumed `core/*` runtime, so a module placed **outside** those directories falls out of the gate entirely, silently. Place new modules inside a gated directory. The documented exceptions — `src/App.tsx` and the `src/main.tsx` / `src/index.ts` bootstraps — are called out in a comment on the `include` array itself.
 
+### Test-gate timeouts are chosen values, stated once
+
+**Every wall-clock budget a test gate runs under is a value somebody picked,
+and it lives in one place (#2323).** The shared budgets are `TIMEOUTS` and
+`INTEGRATION_TIMEOUTS` in `vitest.shared.mts`; all six Vitest projects spread
+one of them, and Testing Library's `asyncUtilTimeout` — which no Vitest config
+can see — is configured in each web project's setup file
+(`src/test/setup.ts`, `src/test/storybookSetup.ts`).
+`npm run verify:test-timeouts` resolves each project through Vitest itself and
+enforces all of it.
+
+This is not a licence to relax the bar. #1596 settled that: **a test that races
+is fixed with fake timers or an awaited condition, never with headroom.** The
+complementary case is what these budgets exist for — a test that is correct and
+deterministic and is simply cut off, because the number bounding it was a
+library default sized for an idle machine rather than for the one this repo is
+worked on (three or four concurrent agent sessions in separate worktrees, each
+free to run a full `local:gate`). Raising a budget nobody chose hides no race.
+
+- **Raise a budget in the shared object, not at a suite.** A per-suite
+  `}, 30_000)` moves one site and leaves every future file where it was — which
+  is how five of the six projects came to have no stated budget at all. Delete a
+  restatement of the project's own value rather than keeping it in sync.
+- **A per-suite raise is right only where the work is genuinely different**, and
+  then it says so: real cross-process lock contention (`file-lock.test.ts`), a
+  full interactive OAuth round trip (`oauth-interactive.test.ts`). Name the
+  constant and state the reason at the site.
+- **A rule about what the code _does_ is asserted at runtime, not read out of
+  the source.** Both of this section's runtime checks
+  (`vitest.setup.shared.mts`, `clients/web/src/test/asyncUtilTimeout.test.ts`)
+  began as source scanning in #2334, and the review found a new valid
+  JavaScript spelling the scanner missed in five consecutive rounds — each fix
+  correct, each making it more parser-shaped, until it carried eight helpers
+  doing quote tracking and bracket balancing. Asking the runtime needs no
+  spelling to be anticipated and covers cases the scan could not reach at all.
+  Reach for `verify:*` when the question is about **configuration**, which a
+  tool can be asked to resolve; assert at runtime when it is about behavior.
+- **A budget is stated, which is not the same as raised.** Testing Library's
+  `asyncUtilTimeout` is pinned at its own default in both web setup files
+  because raising it was *measured worse*: a wait that is meant to expire —
+  a test asserting something never appears, a poll allowed to run out — spends
+  its whole budget on the happy path, so a raise is a proportional cost on
+  exactly those tests and starves the worker pool. Measure before raising a
+  budget that a passing test can spend in full; the call-site comment records
+  the three-arm run.
+- **An inner budget must be strictly smaller than the budget enclosing it.**
+  `waitFor({ timeout: N })` inside a test whose own budget is `N` can never win:
+  the test expires first and reports a timeout naming neither the wait nor its
+  subject (#2292). So when two bounds cover the same work they must differ, and
+  the **inner** one — the one carrying the useful diagnostic — has to be the
+  tighter, which is arranged by raising the outer rather than shrinking the
+  inner. Both shapes in this repo read that way: `AppRenderer.test.tsx`'s 5s
+  `waitFor` inside a 15s test, and `clients/cli/__tests__/e2e.test.ts`'s 15s
+  child timer (which kills the process group and names the CLI) inside a 25s
+  test.
+- **`retry` stays unset.** A retry turns a load-induced red into a silent green
+  on the only pre-push gate this repo has, and hides a real race behind a second
+  attempt. `vitest.setup.shared.mts` asserts it **at runtime**, in every
+  project, reading the value Vitest resolved for the test — so a per-test
+  option, a `describe` option, a project setting and a `--retry` flag all land
+  on the same check.
+- **The Playwright locator budgets the web smokes use are named constants in
+  `scripts/lib/browser-timeouts.mjs`** — raise one there, not at a call site.
+  The *remaining* smoke-helper budgets and CI's missing `timeout-minutes` are
+  tracked on #2333 rather than settled here.
+- **Do not scale a fixed sleep.** A `setTimeout(r, N)` with no condition is not
+  a timeout: it always waits the full window, so raising it slows every passing
+  run and still races on a loaded one. Replace one with a condition wait when it
+  actually flakes (#2250). The single exception is the transition auto-settle in
+  `clients/web/src/test/renderWithMantine.tsx`, whose failure is silent and
+  displaced rather than loud and local — and there only its `RAF_SLACK_MS` term
+  moves, never the term derived from the component's own animation constant.
+
+
 ## Mandatory pre-push gate
 
 - **ALWAYS run `npm run format` before committing.** The **root** `format` auto-fixes `core/`, the root `scripts/` tooling, the root shared surface, and every client's scope in one shot. `validate` runs the non-fixing `format:check` and will fail in CI on any unformatted file, so run the auto-fixer first rather than letting `format:check` catch it.

clients/cli/__tests__/e2e.test.ts

@@ -7,6 +7,27 @@ import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-se
 const here = dirname(fileURLToPath(import.meta.url));
 const BIN = resolve(here, "../build/index.js");
 
+/**
+ * The two bounds on a spawned child, which used to be the identical 15000 —
+ * so which one fired was arbitrary, and they do different things (#2323).
+ *
+ * `CHILD_TIMEOUT_MS` kills the child's process group and rejects with a message
+ * naming the CLI. The enclosing per-test budget just fails the test generically
+ * and leaves a detached `node` behind. The useful diagnostic is the child
+ * timer's, so it has to be the bound that wins — and the way to arrange that is
+ * to raise the test's budget above it, not to shrink the child's. Shrinking it
+ * would take load tolerance away from the one suite here that spawns a real
+ * process, which is the opposite of this change's purpose (Copilot).
+ *
+ * `E2E_SPAWN_MS` is therefore an exception to the cli project's shared 15000,
+ * and one of the few sites that should be: booting the built binary in a fresh
+ * `node` and waiting for it to speak to an MCP server is elapsed work, not
+ * slack. It is the child's own budget plus room for the spawn and the reject to
+ * be observed.
+ */
+const CHILD_TIMEOUT_MS = 15_000;
+const E2E_SPAWN_MS = 25_000;
+
 interface SpawnResult {
   exitCode: number | null;
   stdout: string;
@@ -36,7 +57,7 @@ function spawnCli(args: string[]): Promise<SpawnResult> {
         child.kill("SIGTERM");
       }
       reject(new Error("E2E CLI timed out"));
-    }, 15000);
+    }, CHILD_TIMEOUT_MS);
     child.stdout.on("data", (d) => (stdout += d.toString()));
     child.stderr.on("data", (d) => (stderr += d.toString()));
     child.on("error", (err) => {
@@ -53,18 +74,31 @@ function spawnCli(args: string[]): Promise<SpawnResult> {
 describe("CLI binary (out-of-process E2E)", () => {
   const { command, args } = getTestMcpServerCommand();
 
-  it("exits 0 and prints tools JSON on a successful run", async () => {
+  it(
-    const result = await spawnCli([command, ...args, "--method", "tools/list"]);
+    "exits 0 and prints tools JSON on a successful run",
+    async () => {
+      const result = await spawnCli([
+        command,
+        ...args,
+        "--method",
+        "tools/list",
+      ]);
 
-    expect(result.exitCode).toBe(0);
+      expect(result.exitCode).toBe(0);
-    const json = JSON.parse(result.stdout);
+      const json = JSON.parse(result.stdout);
-    expect(Array.isArray(json.tools)).toBe(true);
+      expect(Array.isArray(json.tools)).toBe(true);
-  });
+    },
+    E2E_SPAWN_MS,
+  );
 
-  it("exits non-zero when required --method is missing", async () => {
+  it(
-    const result = await spawnCli([command, ...args]);
+    "exits non-zero when required --method is missing",
+    async () => {
+      const result = await spawnCli([command, ...args]);
 
-    expect(result.exitCode).not.toBe(0);
+      expect(result.exitCode).not.toBe(0);
-    expect(result.stderr).toContain("Method is required");
+      expect(result.stderr).toContain("Method is required");
-  });
+    },
+    E2E_SPAWN_MS,
+  );
 });

clients/cli/__tests__/oauth-interactive.test.ts

@@ -1,3 +1,12 @@
+/**
+ * Interactive OAuth against a real HTTP test server.
+ *
+ * Every test here raises past the cli project's shared 15s budget, and is meant
+ * to: each boots a server, waits for its well-known documents, runs a full
+ * authorization round trip, then disconnects. That is elapsed work, not slack
+ * for a loaded machine, so it stays stated per test rather than folding into
+ * the shared value (#2323).
+ */
 import { describe, it, expect, afterEach, vi } from "vitest";
 import { rmSync } from "node:fs";
 import { tmpdir } from "node:os";
@@ -90,6 +99,9 @@ const callbackUrlConfig = {
 };
 const presetRedirectUrl = "http://127.0.0.1:6276/oauth/callback";
 
+/** A full interactive OAuth round trip against a freshly booted server. */
+const OAUTH_ROUND_TRIP_MS = 30_000;
+
 describe("CLI interactive OAuth (integration)", () => {
   let mcpServer: TestServerHttp | null = null;
 
@@ -104,148 +116,160 @@ describe("CLI interactive OAuth (integration)", () => {
     } catch {
       // ignore
     }
-  }, 30_000);
+    // No budget: stopping the server and unlinking a file is not the round trip
+    // this file's constant describes, and the shared `hookTimeout` is already
+    // 30000 — so a per-hook argument here would restate it and say the wrong
+    // thing about it (Copilot).
+  });
 
-  it("connects to an OAuth-protected server via the loopback callback server", async () => {
+  it(
-    const serverConfig = {
+    "connects to an OAuth-protected server via the loopback callback server",
-      ...getDefaultServerConfig(),
+    async () => {
-      serverType: "streamable-http" as const,
+      const serverConfig = {
-      ...createOAuthTestServerConfig({
+        ...getDefaultServerConfig(),
-        requireAuth: true,
+        serverType: "streamable-http" as const,
-        supportDCR: true,
+        ...createOAuthTestServerConfig({
-      }),
+          requireAuth: true,
-    };
+          supportDCR: true,
+        }),
+      };
 
-    mcpServer = new TestServerHttp(serverConfig);
+      mcpServer = new TestServerHttp(serverConfig);
-    const port = await mcpServer.start();
+      const port = await mcpServer.start();
-    const serverUrl = `http://localhost:${port}`;
+      const serverUrl = `http://localhost:${port}`;
-    await waitForOAuthWellKnown(serverUrl);
+      await waitForOAuthWellKnown(serverUrl);
 
-    const redirectUrlProvider = new MutableRedirectUrlProvider();
+      const redirectUrlProvider = new MutableRedirectUrlProvider();
-    redirectUrlProvider.redirectUrl = presetRedirectUrl;
+      redirectUrlProvider.redirectUrl = presetRedirectUrl;
-    const navigation = createAutoCompleteNavigation(redirectUrlProvider);
+      const navigation = createAutoCompleteNavigation(redirectUrlProvider);
-    const client = new InspectorClient(
+      const client = new InspectorClient(
-      {
+        {
-        type: "streamable-http",
+          type: "streamable-http",
-        url: `${serverUrl}/mcp`,
+          url: `${serverUrl}/mcp`,
-      } as MCPServerConfig,
+        } as MCPServerConfig,
-      {
+        {
-        environment: {
+          environment: {
-          transport: createTransportNode,
+            transport: createTransportNode,
-          oauth: {
+            oauth: {
-            storage: new NodeOAuthStorage(oauthTestStatePath),
+              storage: new NodeOAuthStorage(oauthTestStatePath),
-            navigation,
+              navigation,
-            redirectUrlProvider,
+              redirectUrlProvider,
+            },
           },
+          directAuthRecovery: true,
+          oauth: {},
         },
-        directAuthRecovery: true,
+      );
-        oauth: {},
-      },
-    );
 
-    await connectInspectorWithOAuth(
+      await connectInspectorWithOAuth(
-      client,
-      { type: "streamable-http", url: `${serverUrl}/mcp` },
-      redirectUrlProvider,
-      callbackUrlConfig,
-      undefined,
-      { isTTY: true },
-    );
-
-    const tools = await client.listTools();
-    expect(tools.tools.length).toBeGreaterThan(0);
-    await client.disconnect();
-  }, 30_000);
-
-  it("retries an RPC after step-up authorization when the user confirms", async () => {
-    const baseConfig = getDefaultServerConfig();
-
-    const serverConfig = {
-      ...baseConfig,
-      serverType: "streamable-http" as const,
-      tools: baseConfig.tools!.map((tool) =>
-        tool.name === "get_temp"
-          ? { ...tool, requiredScopes: ["weather:read"] }
-          : tool,
-      ),
-      ...createOAuthTestServerConfig({
-        requireAuth: true,
-        supportRefreshTokens: true,
-        supportDCR: true,
-      }),
-    };
-
-    mcpServer = new TestServerHttp(serverConfig);
-    const port = await mcpServer.start();
-    const serverUrl = `http://localhost:${port}`;
-    await waitForOAuthWellKnown(serverUrl);
-
-    const redirectUrlProvider = new MutableRedirectUrlProvider();
-    redirectUrlProvider.redirectUrl = presetRedirectUrl;
-    const navigation = createAutoCompleteNavigation(redirectUrlProvider);
-    const client = new InspectorClient(
-      {
-        type: "streamable-http",
-        url: `${serverUrl}/mcp`,
-      } as MCPServerConfig,
-      {
-        environment: {
-          transport: createTransportNode,
-          oauth: {
-            storage: new NodeOAuthStorage(oauthTestStatePath),
-            navigation,
-            redirectUrlProvider,
-          },
-        },
-        directAuthRecovery: true,
-        oauth: {
-          scope: "mcp tools:read",
-        },
-      },
-    );
-
-    await connectInspectorWithOAuth(
-      client,
-      { type: "streamable-http", url: `${serverUrl}/mcp` },
-      redirectUrlProvider,
-      callbackUrlConfig,
-      undefined,
-      { isTTY: true },
-    );
-
-    const tools = await client.listTools();
-    const getTempTool = tools.tools.find((tool) => tool.name === "get_temp");
-    expect(getTempTool).toBeDefined();
-
-    const stderrSpy = vi
-      .spyOn(process.stderr, "write")
-      .mockImplementation(() => true);
-
-    try {
-      const result = await withCliAuthRecoveryRetry(
         client,
         { type: "streamable-http", url: `${serverUrl}/mcp` },
         redirectUrlProvider,
         callbackUrlConfig,
-        makeFakeServerSettings(),
+        undefined,
-        () =>
+        { isTTY: true },
-          client.callTool(getTempTool!, {
-            city: "NYC",
-            units: "C",
-          }),
-        { confirmStepUp: async () => true, isTTY: true },
       );
 
-      const stepUpPrompted = stderrSpy.mock.calls.some(
+      const tools = await client.listTools();
-        ([chunk]) =>
+      expect(tools.tools.length).toBeGreaterThan(0);
-          typeof chunk === "string" &&
-          chunk.includes("Proceed with step-up authorization?"),
-      );
-      expect(stepUpPrompted).toBe(true);
-      expect(result.success).toBe(true);
-    } finally {
-      stderrSpy.mockRestore();
       await client.disconnect();
-    }
+    },
-  }, 30_000);
+    OAUTH_ROUND_TRIP_MS,
+  );
+
+  it(
+    "retries an RPC after step-up authorization when the user confirms",
+    async () => {
+      const baseConfig = getDefaultServerConfig();
+
+      const serverConfig = {
+        ...baseConfig,
+        serverType: "streamable-http" as const,
+        tools: baseConfig.tools!.map((tool) =>
+          tool.name === "get_temp"
+            ? { ...tool, requiredScopes: ["weather:read"] }
+            : tool,
+        ),
+        ...createOAuthTestServerConfig({
+          requireAuth: true,
+          supportRefreshTokens: true,
+          supportDCR: true,
+        }),
+      };
+
+      mcpServer = new TestServerHttp(serverConfig);
+      const port = await mcpServer.start();
+      const serverUrl = `http://localhost:${port}`;
+      await waitForOAuthWellKnown(serverUrl);
+
+      const redirectUrlProvider = new MutableRedirectUrlProvider();
+      redirectUrlProvider.redirectUrl = presetRedirectUrl;
+      const navigation = createAutoCompleteNavigation(redirectUrlProvider);
+      const client = new InspectorClient(
+        {
+          type: "streamable-http",
+          url: `${serverUrl}/mcp`,
+        } as MCPServerConfig,
+        {
+          environment: {
+            transport: createTransportNode,
+            oauth: {
+              storage: new NodeOAuthStorage(oauthTestStatePath),
+              navigation,
+              redirectUrlProvider,
+            },
+          },
+          directAuthRecovery: true,
+          oauth: {
+            scope: "mcp tools:read",
+          },
+        },
+      );
+
+      await connectInspectorWithOAuth(
+        client,
+        { type: "streamable-http", url: `${serverUrl}/mcp` },
+        redirectUrlProvider,
+        callbackUrlConfig,
+        undefined,
+        { isTTY: true },
+      );
+
+      const tools = await client.listTools();
+      const getTempTool = tools.tools.find((tool) => tool.name === "get_temp");
+      expect(getTempTool).toBeDefined();
+
+      const stderrSpy = vi
+        .spyOn(process.stderr, "write")
+        .mockImplementation(() => true);
+
+      try {
+        const result = await withCliAuthRecoveryRetry(
+          client,
+          { type: "streamable-http", url: `${serverUrl}/mcp` },
+          redirectUrlProvider,
+          callbackUrlConfig,
+          makeFakeServerSettings(),
+          () =>
+            client.callTool(getTempTool!, {
+              city: "NYC",
+              units: "C",
+            }),
+          { confirmStepUp: async () => true, isTTY: true },
+        );
+
+        const stepUpPrompted = stderrSpy.mock.calls.some(
+          ([chunk]) =>
+            typeof chunk === "string" &&
+            chunk.includes("Proceed with step-up authorization?"),
+        );
+        expect(stepUpPrompted).toBe(true);
+        expect(result.success).toBe(true);
+      } finally {
+        stderrSpy.mockRestore();
+        await client.disconnect();
+      }
+    },
+    OAUTH_ROUND_TRIP_MS,
+  );
 });

clients/cli/vitest.config.ts

@@ -1,7 +1,11 @@
 import path from "node:path";
 import { fileURLToPath } from "node:url";
 import { defineConfig } from "vitest/config";
-import { vitestSharedPaths } from "../../vitest.shared.mts";
+import {
+  NO_RETRY_SETUP,
+  TIMEOUTS,
+  vitestSharedPaths,
+} from "../../vitest.shared.mts";
 
 const dirname = path.dirname(fileURLToPath(import.meta.url));
 const { projectResolve } = vitestSharedPaths(dirname);
@@ -12,8 +16,10 @@ export default defineConfig({
     globals: false,
     environment: "node",
     include: ["__tests__/**/*.test.ts"],
-    setupFiles: ["__tests__/helpers/mock-open-url.ts"],
+    setupFiles: ["__tests__/helpers/mock-open-url.ts", NO_RETRY_SETUP],
-    testTimeout: 15000,
+    // Shared budgets (#2323). `testTimeout` was already 15000 here by hand;
+    // the hook and teardown budgets were Vitest's defaults until now.
+    ...TIMEOUTS,
     // The in-process runner (__tests__/helpers/cli-runner.ts) patches
     // process.std{out,err}.write to capture CLI output. Test files run in
     // separate forked processes (and tests within a file run sequentially), so

clients/launcher/tsconfig.test.json

@@ -6,7 +6,15 @@
   "extends": "./tsconfig.json",
   "compilerOptions": {
     "noEmit": true,
-    "rootDir": ".",
+    // Wide enough to contain `vitest.shared.mts`, which `vitest.config.ts`
+    // imports for the shared test budgets (#2323). `rootDir` only shapes error
+    // reporting under `noEmit`, and the emitting build config next door keeps
+    // its own `./src` — this project compiles nothing. `cli` and `tui` reach
+    // the same file with no `rootDir` at all, which infers the same thing.
+    "rootDir": "../..",
+    // Same import: `.mts` in a specifier needs this, and it is safe for the
+    // same reason — nothing here is emitted. `cli` and `tui` both set it.
+    "allowImportingTsExtensions": true,
     "types": ["node"]
   },
   // `vitest.config.ts` lives here (not the src project) because the build

clients/launcher/vitest.config.ts

@@ -1,10 +1,18 @@
 import { defineConfig } from "vitest/config";
+import { NO_RETRY_SETUP, TIMEOUTS } from "../../vitest.shared.mts";
 
 export default defineConfig({
   test: {
     globals: false,
     environment: "node",
     include: ["__tests__/**/*.test.ts"],
+    setupFiles: [NO_RETRY_SETUP],
+    // Shared budgets (#2323). This project holds a single test file, which is
+    // exactly why it is pinned rather than left on the defaults: it is cheap to
+    // include and expensive to remember when a second file arrives. This import
+    // is the only thing this config takes from the shared module — the resolve
+    // aliases are for clients that load React or `core/`.
+    ...TIMEOUTS,
     coverage: {
       provider: "v8",
       reporter: ["text", "html", "json-summary"],

clients/tui/__tests__/ResourcesTab.test.tsx

@@ -27,11 +27,23 @@ const tick = async () => {
     await new Promise((resolve) => setTimeout(resolve, 4));
 };
 
-/** Poll the frame until it contains `substr` — stable under coverage load. */
+/**
+ * Poll the frame until it contains `substr` — stable under coverage load.
+ *
+ * The default budget (`tries` x the ~32ms `tick` below) matches `App.test.tsx`'s
+ * `POLL_TRIES`, which was raised twice for exactly this shape (#1742, #1942).
+ * It used to be 25 here — one third of the same renderer's budget under the
+ * same v8 instrumentation, for no stated reason, which made this the next one
+ * to go red. A high ceiling costs a passing assertion nothing: the loop returns
+ * the instant the predicate holds and only the slow path ever spends it
+ * (#2323).
+ */
+const POLL_TRIES = 100;
+
 async function waitForFrame(
   getFrame: () => string | undefined,
   substr: string,
-  tries = 25,
+  tries = POLL_TRIES,
 ) {
   for (let i = 0; i < tries; i++) {
     if ((getFrame() ?? "").includes(substr)) return;

clients/tui/__tests__/logger.test.ts

@@ -9,8 +9,19 @@ import { join } from "node:path";
  * file (and any parent dir) appears a tick later. Awaiting this before cleanup
  * also guarantees the background mkdir/open has finished, so `afterEach`'s
  * rmSync can't race it into an ENOENT.
+ *
+ * The budget is a ceiling, not a wait: the loop exits on the first `existsSync`
+ * that succeeds, so a passing run pays only the 20ms poll interval and the
+ * headroom is spent only when a real filesystem write is genuinely slow — which
+ * is what an oversubscribed machine produces (#2323). 2000ms was Vitest-era
+ * guesswork against an idle one.
  */
-async function waitForFile(filePath: string, timeoutMs = 2000): Promise<void> {
+const FILE_APPEAR_TIMEOUT_MS = 10_000;
+
+async function waitForFile(
+  filePath: string,
+  timeoutMs = FILE_APPEAR_TIMEOUT_MS,
+): Promise<void> {
   const start = Date.now();
   while (!existsSync(filePath)) {
     if (Date.now() - start > timeoutMs) {

clients/tui/vitest.config.ts

@@ -1,7 +1,11 @@
 import path from "node:path";
 import { fileURLToPath } from "node:url";
 import { defineConfig } from "vitest/config";
-import { vitestSharedPaths } from "../../vitest.shared.mts";
+import {
+  NO_RETRY_SETUP,
+  TIMEOUTS,
+  vitestSharedPaths,
+} from "../../vitest.shared.mts";
 
 const dirname = path.dirname(fileURLToPath(import.meta.url));
 const { projectResolve } = vitestSharedPaths(dirname);
@@ -12,7 +16,11 @@ export default defineConfig({
     globals: false,
     environment: "node",
     include: ["__tests__/**/*.test.{ts,tsx}"],
-    setupFiles: ["./__tests__/setup.ts"],
+    setupFiles: ["./__tests__/setup.ts", NO_RETRY_SETUP],
+    // Shared budgets (#2323). Ink renders through React, so every assertion
+    // here waits on a React commit under v8 instrumentation — the shape behind
+    // the poll-budget raises in #1742 and #1942.
+    ...TIMEOUTS,
     coverage: {
       provider: "v8",
       reporter: ["text", "html", "json-summary"],

clients/web/src/App.test.tsx

@@ -4295,11 +4295,18 @@ describe("App dedupes concurrent OAuth clears that share a storage key (#2217)",
     );
   }
 
-  // Three full modal interaction sequences against the whole App tree, so this
+  // Three full modal interaction sequences against the whole App tree — three
-  // one runs long enough to trip the 5s default when the suite is under load.
+  // times the work of any other test in this file, and the only one here that
+  // ever tripped the old 5s default under load. It keeps its own budget after
+  // #2323 rather than folding into the shared 15000: this is the "genuinely
+  // different work" exception that change establishes, and the 20000 was a
+  // value somebody had already chosen against an observed failure. Lowering it
+  // to the shared ceiling would trade a measured number for a general one and
+  // could reintroduce the flake (Copilot).
+  const THREE_MODAL_SEQUENCE_MS = 20_000;
+
   it(
     "suppresses a second clear for another entry with the same URL, and allows one after it settles",
-    { timeout: 20000 },
     async () => {
       // `delay: null` drops userEvent's inter-event waits, which dominate here.
       const user = userEvent.setup({ delay: null });
@@ -4349,5 +4356,6 @@ describe("App dedupes concurrent OAuth clears that share a storage key (#2217)",
         expect(client.clearOAuthTokens).toHaveBeenCalledTimes(2),
       );
     },
+    THREE_MODAL_SEQUENCE_MS,
   );
 });

clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx

@@ -557,6 +557,15 @@ describe("AppRenderer", () => {
       // budget that is too tight rather than of a broken observer. Raised
       // rather than retried: a `waitFor` that is generous costs nothing on the
       // passing path, since it returns as soon as the assertion holds.
+      //
+      // #2323 briefly deleted this argument as one of its "a budget inside an
+      // equal budget can never be observed" cases, which it was: 5000 inside a
+      // 5000ms test could never win. That is fixed from the other end now — the
+      // enclosing budget is 15000 — so the raise is observable again and has to
+      // stay, because the value it beats is a measured failure rather than a
+      // default. It does NOT ride on `asyncUtilTimeout`, which is pinned at
+      // 1000 precisely because raising it globally was measured worse
+      // (Copilot).
       await waitFor(
         () =>
           expect(bridge.sendHostContextChange).toHaveBeenCalledWith(

clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx

@@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
 import userEvent from "@testing-library/user-event";
 import {
   act,
+  RAF_SLACK_MS,
   renderWithMantine,
   renderWithMantineTransitions,
   screen,
@@ -18,7 +19,18 @@ import { HEADER_ANIM_MS, ViewHeader } from "./ViewHeader";
 // duration plus rAF scheduling slack, so bumping HEADER_ANIM_MS can't silently
 // make the settle insufficient. (This is why ViewHeader.tsx exports
 // HEADER_ANIM_MS — the settle window must track the animation it settles.)
-const TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200;
+//
+// The slack half is `RAF_SLACK_MS` rather than a bare `+ 200` (#2323): the two
+// terms answer to different things — the first to the component, the second to
+// how busy the machine is — and only the second should move when the machine
+// gets busier. Naming it is what lets it move in one place, here and in the
+// helper's own fallback.
+//
+// No `enterDelay`/`exitDelay` is passed to any of ViewHeader's four
+// `Transition`s, so HEADER_ANIM_MS really is the longest JS chain. The 150ms
+// stagger visible in the UI is a CSS `animation-delay` (`App.css`), and a CSS
+// animation schedules no JS timer for the settle to drain.
+const TRANSITION_SETTLE_MS = HEADER_ANIM_MS + RAF_SLACK_MS;
 
 // Mock @mantine/hooks so we can control useMediaQuery results per test.
 const mediaQueryMock = vi.hoisted(() => ({ value: false }));

clients/web/src/test/PreviewAnnotations.stories.tsx

@@ -8,8 +8,15 @@ import { theme } from "../theme/theme";
 // `.storybook/vitest.setup.ts` used to call `setProjectAnnotations([...])` to
 // hand the preview annotations to the Storybook vitest project. Since Storybook
 // 10.3 `@storybook/addon-vitest` applies them automatically — and *skips* doing
-// so when it finds such a setup file — so the file was removed and the
+// so when it finds a setup file that is both inside `configDir` and calls
-// `setupFiles` entry dropped from the `storybook` project in `vite.config.ts`.
+// `setProjectAnnotations` — so that file was removed.
+//
+// The project does have a `setupFiles` entry again (#2323), and it is a
+// different thing: `src/test/storybookSetup.ts` configures Testing Library's
+// `asyncUtilTimeout` and nothing else. It satisfies *neither* half of the
+// condition above — it lives outside `.storybook/` and names no annotations —
+// so the automatic path is still the one in use, and this guard still asserts
+// that it worked.
 //
 // A green suite is not evidence that the automatic path works. Two things the
 // setup file used to provision are silently losable:

clients/web/src/test/asyncUtilTimeout.test.ts

@@ -0,0 +1,43 @@
+import { describe, it, expect } from "vitest";
+import { getConfig } from "@testing-library/react";
+
+/**
+ * Pins Testing Library's `asyncUtilTimeout` for the `unit` project (#2323).
+ *
+ * That value governs every `waitFor` and `findBy*` here — 788 call sites — and
+ * is the binding bound on any async assertion, being tighter than the project's
+ * own per-test budget. So it is a number this repo states rather than inherits,
+ * and a Testing Library upgrade that changed its default would fail here rather
+ * than silently move all 788.
+ *
+ * ⚠️ The value is the library's own default, and that is a MEASURED decision,
+ * not a shrug. #2323 proposed 5000, on the reasoning that a contended
+ * happy-dom render is the worst case. Measured on the machine this team uses,
+ * 5000 made the suite worse: three arms of three full unit runs each, same
+ * worktree, comparable load —
+ *
+ *   • unmodified `v2/main`:         3/3 green
+ *   • the raise to 5000:            1, 4 and 0 files timing out at 15000ms
+ *   • the raise reverted:           3/3 green
+ *
+ * A different unrelated set failed each time and every one passed in about a
+ * second in isolation, which is the shape of CPU starvation rather than of a
+ * slow assertion. The mechanism: `asyncUtilTimeout` is not only a ceiling — any
+ * wait that is *meant* to expire spends the whole budget on the happy path, so
+ * a 5x raise is a 5x cost on exactly those tests, which saturates the worker
+ * pool and starves tests that were never slow. The tests that assert absence
+ * that way are the real defect and are #2335; revisit this when they are gone.
+ *
+ * ⚠️ This asserts the EFFECTIVE value at runtime rather than checking that
+ * `setup.ts` contains a `configure()` call. An earlier revision of #2334 did
+ * the latter by scanning source text, and the review found five ways to satisfy
+ * the scan without configuring anything — a commented-out call, a call on
+ * another object, a non-literal value, and so on. `getConfig()` is what the
+ * `waitFor`s in this project actually read, so there is nothing left to spell
+ * around.
+ */
+describe("Testing Library configuration (unit project)", () => {
+  it("uses the asyncUtilTimeout this repo states, not whatever the library ships", () => {
+    expect(getConfig().asyncUtilTimeout).toBe(1000);
+  });
+});

clients/web/src/test/integration/auth/node/file-lock.test.ts

@@ -7,6 +7,14 @@
  * is one process-wide queue per path, so two in-process callers are ordered
  * before the lock ever sees them. A child process is not scaffolding here —
  * it is the only participant that can produce the interleaving.
+ *
+ * Four tests here raise their own budget past the integration project's 30s,
+ * and are among the handful in the repo that still should (#2323). What they
+ * wait on is not React or a renderer — it is `proper-lockfile`'s stale-lock
+ * timeout and a second OS process actually contending for the file, which is
+ * real elapsed work rather than slack. The named constants below say so at
+ * each site; the restatements of the project's own 30s that used to sit
+ * alongside them are gone.
  */
 import {
   describe,
@@ -28,6 +36,20 @@ import {
   withSecretFileLock,
   resetFileLockWarnings,
 } from "@inspector/core/auth/node/file-lock.js";
+
+/**
+ * A test that waits for `proper-lockfile`'s retries to run out against a holder
+ * that never yields — a live second process, or a stale lock it cannot clear.
+ * Nothing is taken over here; the budget is spent in full by construction,
+ * which is why it is the longer of the two.
+ */
+const LOCK_RETRIES_EXHAUSTED_MS = 90_000;
+/**
+ * A test that waits out the stale window and then *succeeds* by taking the dead
+ * holder's lock over. Shorter, because it ends the moment the takeover lands
+ * rather than when the retries stop.
+ */
+const STALE_LOCK_TAKEOVER_MS = 60_000;
 import { FileSecretStore } from "@inspector/core/auth/node/file-secret-store.js";
 import { SecretStoreUnavailableError } from "@inspector/core/auth/node/secret-store.js";
 
@@ -150,7 +172,7 @@ describe("withSecretFileLock across processes", () => {
     // …and having waited, it did not report a degraded write.
     expect(warnings()).toBe("");
     await done;
-  }, 20_000);
+  });
 
   it("locks a file that does not exist yet", async () => {
     // The first `set` on a fresh install has no `secrets.json` — and
@@ -204,7 +226,7 @@ describe("withSecretFileLock across processes", () => {
     expect(await reader.get("srv", "env:FROM_CHILD")).toBe("1");
     expect(await reader.get("srv", "env:FROM_PARENT")).toBe("2");
     expect(warnings()).toBe("");
-  }, 20_000);
+  });
 
   it("creates the storage directory so the very first save is locked too", async () => {
     // `writeStoreFile` creates the parent directory, but from *inside* the
@@ -232,7 +254,7 @@ describe("withSecretFileLock across processes", () => {
     expect(await reader.get("srv", "env:FIRST_EVER")).toBe("1");
     // No degrade warning: the lock was genuinely held, not skipped.
     expect(warnings()).toBe("");
-  }, 20_000);
+  });
 });
 
 describe("withSecretFileLock degrades rather than failing", () => {
@@ -263,131 +285,147 @@ describe("withSecretFileLock degrades rather than failing", () => {
     expect(warn).toHaveBeenCalledTimes(1);
   });
 
-  it("refuses the save rather than writing alongside a live holder", async () => {
+  it(
-    // `ELOCKED` is evidence the lock is *working*, so degrading here would
+    "refuses the save rather than writing alongside a live holder",
-    // enter the exact interleaving the lock exists to prevent — and enter it
+    async () => {
-    // knowing another writer is there. Held from this process, which is
+      // `ELOCKED` is evidence the lock is *working*, so degrading here would
-    // indistinguishable to `proper-lockfile` from a remote holder (it is not
+      // enter the exact interleaving the lock exists to prevent — and enter it
-    // reentrant); the in-process queue is what keeps that out of the way in
+      // knowing another writer is there. Held from this process, which is
-    // production.
+      // indistinguishable to `proper-lockfile` from a remote holder (it is not
-    //
+      // reentrant); the in-process queue is what keeps that out of the way in
-    // The wait is real: the retry budget deliberately outlasts the 10s stale
+      // production.
-    // window so a *crashed* holder resolves by takeover instead of failing
+      //
-    // everyone else's saves. This holder is alive and refreshing, so it never
+      // The wait is real: the retry budget deliberately outlasts the 10s stale
-    // goes stale and the budget is spent in full.
+      // window so a *crashed* holder resolves by takeover instead of failing
-    const target = filePath();
+      // everyone else's saves. This holder is alive and refreshing, so it never
-    const lockfile = require_(LOCKFILE_MODULE) as {
+      // goes stale and the budget is spent in full.
-      lock: (f: string, o: object) => Promise<() => Promise<void>>;
+      const target = filePath();
-    };
+      const lockfile = require_(LOCKFILE_MODULE) as {
-    // The **same** `stale` production uses, and that is not incidental:
+        lock: (f: string, o: object) => Promise<() => Promise<void>>;
-    // `isLockStale` is evaluated against the *waiter's* threshold while the
+      };
-    // holder refreshes on its own `stale / 2`. A holder configured looser
+      // The **same** `stale` production uses, and that is not incidental:
-    // (say 60s) refreshes every 30s and is therefore declared stale by a
+      // `isLockStale` is evaluated against the *waiter's* threshold while the
-    // 10s waiter after 10s — the waiter takes over and the save succeeds,
+      // holder refreshes on its own `stale / 2`. A holder configured looser
-    // quietly testing the opposite of what this test claims.
+      // (say 60s) refreshes every 30s and is therefore declared stale by a
-    const release = await lockfile.lock(target, {
+      // 10s waiter after 10s — the waiter takes over and the save succeeds,
-      realpath: false,
+      // quietly testing the opposite of what this test claims.
-      stale: 10_000,
+      const release = await lockfile.lock(target, {
-    });
+        realpath: false,
+        stale: 10_000,
+      });
 
-    const store = new FileSecretStore({ filePath: target });
+      const store = new FileSecretStore({ filePath: target });
-    // One call, both assertions off the same rejection: each attempt spends
+      // One call, both assertions off the same rejection: each attempt spends
-    // the full retry budget, so a second would double the test's runtime to
+      // the full retry budget, so a second would double the test's runtime to
-    // re-prove the same thing.
+      // re-prove the same thing.
-    const err = await store
+      const err = await store
-      .set("srv", "env:MINE", "1")
+        .set("srv", "env:MINE", "1")
-      .catch((e: unknown) => e);
+        .catch((e: unknown) => e);
-    expect(err).toBeInstanceOf(SecretStoreUnavailableError);
+      expect(err).toBeInstanceOf(SecretStoreUnavailableError);
-    expect((err as Error).message).toMatch(/was not saved/);
+      expect((err as Error).message).toMatch(/was not saved/);
-    await release();
+      await release();
 
-    // Nothing was written behind the holder's back.
+      // Nothing was written behind the holder's back.
-    expect(existsSync(target)).toBe(false);
+      expect(existsSync(target)).toBe(false);
-  }, 90_000);
+    },
+    LOCK_RETRIES_EXHAUSTED_MS,
+  );
 
-  it("refuses rather than degrading when a stale lock cannot be cleared", async () => {
+  it(
-    // `acquireLock` does not only *create* directories — on finding a stale
+    "refuses rather than degrading when a stale lock cannot be cleared",
-    // one it removes it and retries, and that removal can fail. A stale lock
+    async () => {
-    // with anything inside it fails `ENOTEMPTY`, which is not `ELOCKED`, and
+      // `acquireLock` does not only *create* directories — on finding a stale
-    // treating every non-`ELOCKED` error as "locks do not work here" meant
+      // one it removes it and retries, and that removal can fail. A stale lock
-    // every Inspector on the box quietly bypassed the *same* stuck lock and
+      // with anything inside it fails `ENOTEMPTY`, which is not `ELOCKED`, and
-    // raced its writes — while the release-failure message was telling the
+      // treating every non-`ELOCKED` error as "locks do not work here" meant
-    // operator saves would keep failing until they cleared it.
+      // every Inspector on the box quietly bypassed the *same* stuck lock and
-    const target = filePath();
+      // raced its writes — while the release-failure message was telling the
-    const lockPath = `${target}.lock`;
+      // operator saves would keep failing until they cleared it.
-    await fs.mkdir(path.dirname(target), { recursive: true });
+      const target = filePath();
-    await fs.mkdir(lockPath);
+      const lockPath = `${target}.lock`;
-    await fs.writeFile(`${lockPath}/stray`, "", "utf-8");
+      await fs.mkdir(path.dirname(target), { recursive: true });
-    // Backdated so it reads as stale — which is what sends `acquireLock` down
+      await fs.mkdir(lockPath);
-    // the remove-and-retry path rather than straight to `ELOCKED`.
+      await fs.writeFile(`${lockPath}/stray`, "", "utf-8");
-    const longDead = new Date(Date.now() - 60_000);
+      // Backdated so it reads as stale — which is what sends `acquireLock` down
-    await fs.utimes(lockPath, longDead, longDead);
+      // the remove-and-retry path rather than straight to `ELOCKED`.
+      const longDead = new Date(Date.now() - 60_000);
+      await fs.utimes(lockPath, longDead, longDead);
 
-    const store = new FileSecretStore({ filePath: target });
+      const store = new FileSecretStore({ filePath: target });
-    const err = await store
+      const err = await store
-      .set("srv", "env:MINE", "1")
+        .set("srv", "env:MINE", "1")
-      .catch((e: unknown) => e);
+        .catch((e: unknown) => e);
 
-    expect(err).toBeInstanceOf(SecretStoreUnavailableError);
+      expect(err).toBeInstanceOf(SecretStoreUnavailableError);
-    expect((err as Error).message).toMatch(/was not saved/);
+      expect((err as Error).message).toMatch(/was not saved/);
-    // Nothing written behind the stuck lock, and no "unprotected" warning:
+      // Nothing written behind the stuck lock, and no "unprotected" warning:
-    // this is a refusal, not a degrade.
+      // this is a refusal, not a degrade.
-    expect(existsSync(target)).toBe(false);
+      expect(existsSync(target)).toBe(false);
-    expect(warnings()).not.toContain("not protected");
+      expect(warnings()).not.toContain("not protected");
-  }, 90_000);
+    },
+    LOCK_RETRIES_EXHAUSTED_MS,
+  );
 
-  it("takes over the lock of a holder that died, rather than failing the save", async () => {
+  it(
-    // The invariant behind refusing on `ELOCKED`: refusing is only defensible
+    "takes over the lock of a holder that died, rather than failing the save",
-    // because a *crashed* holder resolves on its own first. `RETRY` therefore
+    async () => {
-    // has to outlast `STALE_MS` — if the budget were the shorter of the two,
+      // The invariant behind refusing on `ELOCKED`: refusing is only defensible
-    // one Inspector killed mid-save would make every later save on the box
+      // because a *crashed* holder resolves on its own first. `RETRY` therefore
-    // fail until someone deleted the lock by hand.
+      // has to outlast `STALE_MS` — if the budget were the shorter of the two,
-    //
+      // one Inspector killed mid-save would make every later save on the box
-    // A dead holder is exactly a lock directory nobody is refreshing, so it
+      // fail until someone deleted the lock by hand.
-    // is staged directly: no child to race, and no dependence on how quickly
+      //
-    // a killed process is reaped.
+      // A dead holder is exactly a lock directory nobody is refreshing, so it
-    const target = filePath();
+      // is staged directly: no child to race, and no dependence on how quickly
-    const lockPath = `${target}.lock`;
+      // a killed process is reaped.
-    await fs.mkdir(path.dirname(target), { recursive: true });
+      const target = filePath();
-    await fs.mkdir(lockPath);
+      const lockPath = `${target}.lock`;
-    const longDead = new Date(Date.now() - 60_000);
+      await fs.mkdir(path.dirname(target), { recursive: true });
-    await fs.utimes(lockPath, longDead, longDead);
+      await fs.mkdir(lockPath);
+      const longDead = new Date(Date.now() - 60_000);
+      await fs.utimes(lockPath, longDead, longDead);
 
-    const store = new FileSecretStore({ filePath: target });
+      const store = new FileSecretStore({ filePath: target });
-    await store.set("srv", "env:AFTER_CRASH", "1");
+      await store.set("srv", "env:AFTER_CRASH", "1");
 
-    const reader = new FileSecretStore({ filePath: target });
+      const reader = new FileSecretStore({ filePath: target });
-    expect(await reader.get("srv", "env:AFTER_CRASH")).toBe("1");
+      expect(await reader.get("srv", "env:AFTER_CRASH")).toBe("1");
-    // Took the lock over — did not fall through to an unlocked write.
+      // Took the lock over — did not fall through to an unlocked write.
-    expect(warnings()).toBe("");
+      expect(warnings()).toBe("");
-  }, 60_000);
+    },
+    STALE_LOCK_TAKEOVER_MS,
+  );
 
-  it("stays silent per the delete contract when the lock is held", async () => {
+  it(
-    // `delete` reports nothing by contract — only `set` hard-fails — so the
+    "stays silent per the delete contract when the lock is held",
-    // refusal above must not turn a delete into a throw.
+    async () => {
-    const target = filePath();
+      // `delete` reports nothing by contract — only `set` hard-fails — so the
-    const store = new FileSecretStore({ filePath: target });
+      // refusal above must not turn a delete into a throw.
-    await store.set("srv", "env:A", "1");
+      const target = filePath();
+      const store = new FileSecretStore({ filePath: target });
+      await store.set("srv", "env:A", "1");
 
-    const lockfile = require_(LOCKFILE_MODULE) as {
+      const lockfile = require_(LOCKFILE_MODULE) as {
-      lock: (f: string, o: object) => Promise<() => Promise<void>>;
+        lock: (f: string, o: object) => Promise<() => Promise<void>>;
-    };
+      };
-    // The **same** `stale` production uses, and that is not incidental:
+      // The **same** `stale` production uses, and that is not incidental:
-    // `isLockStale` is evaluated against the *waiter's* threshold while the
+      // `isLockStale` is evaluated against the *waiter's* threshold while the
-    // holder refreshes on its own `stale / 2`. A holder configured looser
+      // holder refreshes on its own `stale / 2`. A holder configured looser
-    // (say 60s) refreshes every 30s and is therefore declared stale by a
+      // (say 60s) refreshes every 30s and is therefore declared stale by a
-    // 10s waiter after 10s — the waiter takes over and the save succeeds,
+      // 10s waiter after 10s — the waiter takes over and the save succeeds,
-    // quietly testing the opposite of what this test claims.
+      // quietly testing the opposite of what this test claims.
-    const release = await lockfile.lock(target, {
+      const release = await lockfile.lock(target, {
-      realpath: false,
+        realpath: false,
-      stale: 10_000,
+        stale: 10_000,
-    });
+      });
-    await expect(store.delete("srv", "env:A")).resolves.toBeUndefined();
+      await expect(store.delete("srv", "env:A")).resolves.toBeUndefined();
-    await release();
+      await release();
 
-    // …and the entry it could not delete is still there, not half-removed.
+      // …and the entry it could not delete is still there, not half-removed.
-    expect(await store.get("srv", "env:A")).toBe("1");
+      expect(await store.get("srv", "env:A")).toBe("1");
-  }, 90_000);
+    },
+    LOCK_RETRIES_EXHAUSTED_MS,
+  );
 });
 
 describe("withSecretFileLock reports what it cannot clean up", () => {
@@ -457,7 +495,7 @@ describe("withSecretFileLock reports what it cannot clean up", () => {
 
     // Unguarded, the winner's directory is gone.
     expect(existsSync(lockPath)).toBe(false);
-  }, 30_000);
+  });
 
   it("tells the operator to clear a lock that cannot expire on its own", async () => {
     // `rmdir` refuses a non-empty directory — and so does stale takeover,
@@ -510,5 +548,5 @@ describe("withSecretFileLock reports what it cannot clean up", () => {
     // that did nothing wrong. `onCompromised` has already said what happened.
     expect(warnings()).not.toContain("by hand");
     expect(warnings()).not.toContain("Could not release the lock");
-  }, 30_000);
+  });
 });

clients/web/src/test/integration/auth/node/secret-store-selection.test.ts

@@ -82,6 +82,15 @@ async function loadWithProbe(available: boolean, detail = "no D-Bus session") {
   return import("@inspector/core/auth/node/secret-store-selection.js");
 }
 
+/**
+ * Past the integration project's own 30s deliberately, and one of the few sites
+ * that should be (#2323): the test using this waits on a second process holding
+ * the secrets-file lock until `proper-lockfile`'s retries run out, which is real
+ * elapsed contention rather than slack for a loaded machine. Named to match
+ * `file-lock.test.ts`, which bounds the same wait.
+ */
+const LOCK_RETRIES_EXHAUSTED_MS = 60_000;
+
 describe("parseSecretStoreEnv", () => {
   it("accepts each kind, case- and whitespace-insensitively", () => {
     expect(parseSecretStoreEnv("keyring")).toBe("keyring");
@@ -684,38 +693,42 @@ describe("absorbFileSecretsIntoKeyring", () => {
     return filePath;
   }
 
-  it("does not throw when another process holds the lock (#2082)", async () => {
+  it(
-    // `withSecretFileLock` throws on a lock held past its retry budget, which
+    "does not throw when another process holds the lock (#2082)",
-    // is right for a `set` — the user is waiting on that value — and wrong
+    async () => {
-    // here. This function is awaited directly by both `resolveSecretStore`
+      // `withSecretFileLock` throws on a lock held past its retry budget, which
-    // branches, so an escaping error fails store resolution and with it the
+      // is right for a `set` — the user is waiting on that value — and wrong
-    // whole session: a stuck writer elsewhere on the box would stop the
+      // here. This function is awaited directly by both `resolveSecretStore`
-    // Inspector from starting. Leaving the file for the next run is the only
+      // branches, so an escaping error fails store resolution and with it the
-    // acceptable outcome.
+      // whole session: a stuck writer elsewhere on the box would stop the
-    const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
+      // Inspector from starting. Leaving the file for the next run is the only
-    process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
+      // acceptable outcome.
-    const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
+      const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
-    const mod = await loadWithProbe(true);
+      process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
+      const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
+      const mod = await loadWithProbe(true);
 
-    const properLockfile = (await import("proper-lockfile")).default;
+      const properLockfile = (await import("proper-lockfile")).default;
-    const release = await properLockfile.lock(filePath, {
+      const release = await properLockfile.lock(filePath, {
-      realpath: false,
+        realpath: false,
-      stale: 10_000,
+        stale: 10_000,
-    });
+      });
 
-    await expect(
+      await expect(
-      mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()),
+        mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()),
-    ).resolves.toBeUndefined();
+      ).resolves.toBeUndefined();
-    await release();
+      await release();
 
-    // Left exactly as it was, and said so — asserting the *lock* message
+      // Left exactly as it was, and said so — asserting the *lock* message
-    // specifically, since the pre-existing claim-failure warning also ends in
+      // specifically, since the pre-existing claim-failure warning also ends in
-    // "left in place" and would let this pass without the lock ever being hit.
+      // "left in place" and would let this pass without the lock ever being hit.
-    expect(existsSync(filePath)).toBe(true);
+      expect(existsSync(filePath)).toBe(true);
-    expect(warn).toHaveBeenCalledWith(
+      expect(warn).toHaveBeenCalledWith(
-      expect.stringContaining("Could not lock the secrets file"),
+        expect.stringContaining("Could not lock the secrets file"),
-    );
+      );
-  }, 60_000);
+    },
+    LOCK_RETRIES_EXHAUSTED_MS,
+  );
 
   it("takes no lock when there is nothing to migrate (#2082)", async () => {
     // The overwhelmingly common startup: a keychain is available and no file

clients/web/src/test/integration/auth/revocation-e2e.test.ts

@@ -96,7 +96,7 @@ describe("OAuth token revocation (RFC 7009)", () => {
     metadata = (await (
       await fetch(`${serverUrl}/.well-known/oauth-authorization-server`)
     ).json()) as OAuthMetadata;
-  }, 30_000);
+  });
 
   afterAll(async () => {
     await mcpServer?.stop();
@@ -105,7 +105,7 @@ describe("OAuth token revocation (RFC 7009)", () => {
       rmSync(storageDir, { recursive: true, force: true });
       storageDir = "";
     }
-  }, 30_000);
+  });
 
   /** Run a real authorization-code exchange and return the issued tokens. */
   async function authorize(