Merge pull request #827 from Wibias/fix/storage-worker-isolate-beforeeach

fix(storage): await Worker join in test beforeEach under isolate

Author: Wibias

.github/workflows/ci.yml

@@ -120,11 +120,13 @@ jobs:
     # the result rather than the code under review — #711's rerun finished at
     # 11.8min and passed while #653's was killed at 12.0min (issue #717).
     # A cancelled job renders as `fail` in `gh pr checks`, so that flakiness
-    # reads as a broken PR. 20 minutes keeps a green Windows run green with
-    # real margin; it is not a licence for the suite to grow into it. If
-    # Windows approaches this ceiling too, fix the 2.5x platform gap instead
-    # of raising the number again.
-    timeout-minutes: 20
+    # reads as a broken PR. After the 2026-08-01 state-store admission merge,
+    # Windows under `bun test --isolate` on #827 hit the 20-minute kill while
+    # still green mid-suite (~19m of tests). 30 minutes is the margin for that
+    # tip — not a licence to absorb hung tests (see oauth mutation waitMs
+    # unref fix). Shrink the suite / close the platform gap rather than raising
+    # this again for ordinary variance.
+    timeout-minutes: 30
     strategy:
       fail-fast: false
       matrix:

gui/tests/client-resource-poll.test.tsx

@@ -1,4 +1,4 @@
-import { afterEach, beforeEach, expect, test } from "bun:test";
+import { afterEach, beforeEach, expect, test as bunTest } from "bun:test";
 import { Window } from "happy-dom";
 import { act, useEffect, useState } from "react";
 import type { Root } from "react-dom/client";
@@ -9,6 +9,13 @@ import {
   useKeyedClientResource,
 } from "../src/client-resource";
 
+// Bun's default per-test budget is 5s. waitFor below is 15s (busy CI runners),
+// so every case in this file needs a higher ceiling or the suite fails as
+// "waitFor timed out" while the predicate never got its full window.
+function test(name: string, fn: () => void | Promise<void>): void {
+  bunTest(name, fn, { timeout: 30_000 });
+}
+
 const globals = ["document", "window", "navigator", "IS_REACT_ACT_ENVIRONMENT"] as const;
 let previousGlobals: Record<(typeof globals)[number], unknown>;
 let testWindow: Window;
@@ -37,11 +44,13 @@ afterEach(() => {
  * 1s was enough on an idle machine and not enough on a loaded CI runner: the
  * visibility test waits for a poll that only fires after a real 20ms interval
  * plus a React commit, and macOS CI blew through the budget mid-suite while the
- * same file passed in isolation. The assertions below are about *whether* the
- * fetch happens, never about how fast, so a longer ceiling costs nothing on a
- * healthy run — it only stops a busy runner from reading as a product bug.
+ * same file passed in isolation. Windows GHA later timed out the same waits at
+ * 5s under suite load (`waitFor timed out` on #827). The assertions below are
+ * about *whether* the fetch happens, never about how fast, so a longer ceiling
+ * costs nothing on a healthy run — it only stops a busy runner from reading as
+ * a product bug.
  */
-async function waitFor(predicate: () => boolean, timeoutMs = 5000): Promise<void> {
+async function waitFor(predicate: () => boolean, timeoutMs = 15_000): Promise<void> {
   const start = Date.now();
   while (!predicate()) {
     if (Date.now() - start > timeoutMs) throw new Error("waitFor timed out");

src/oauth/store.ts

@@ -416,7 +416,11 @@ function serializeMutation<T>(work: () => Promise<T>, retainedValues: readonly u
     release();
     rejectResult(new OAuthMutationBusyError("OAuth mutation queue wait timed out"));
   }, waitMs);
-  entry.timeout.unref?.();
+  // Only unref the long default wait. Short waitMs (tests) must stay ref'd:
+  // on Windows Bun under `bun test --isolate`, an unref'd timer can fail to
+  // fire while the head mutation holds an unresolved Promise, hanging the
+  // waiter forever (#827).
+  if (waitMs >= OAUTH_MUTATION_WAIT_MS) entry.timeout.unref?.();
   mutationWaiters.push(entry);
   drainOAuthMutations();
   return result;

src/server/lifecycle.ts

@@ -217,7 +217,9 @@ export async function drainAndShutdown(
     setStorageCleanupPolicyJobLiveApply(null);
   } finally {
     try {
-      s?.stop(true);
+      // Bun's Server.stop returns Promise<void>; fire-and-forget races the next
+      // isolate reclaim / follow-on listen the same way unterminated Workers did.
+      if (s) await s.stop(true);
     } finally {
       draining = false;
     }

src/storage/policy-job.ts

@@ -131,8 +131,16 @@ function disownActiveRun(): void {
   cancel?.();
 }
 
+/**
+ * Fire-and-forget reset. Prefer {@link resetStorageCleanupPolicyJobForTestsAsync}
+ * from test beforeEach/afterEach — sync terminate races Windows
+ * `bun test --isolate` reclaim when the next case spawns immediately.
+ */
 export function resetStorageCleanupPolicyJobForTests(): void {
   disownActiveRun();
+  // Invalidate spawnGate callbacks that have not created a Worker yet — otherwise
+  // they can still spawn after this sync path releases the mutation slot.
+  cancelQueuedStorageWorkerSpawns();
   if (activeWorker) {
     void terminateStorageWorker(activeWorker);
     activeWorker = null;
@@ -176,6 +184,7 @@ export async function resetStorageCleanupPolicyJobForTestsAsync(): Promise<void>
 /** Terminate an in-flight worker during process shutdown. */
 export function abortStorageCleanupPolicyJob(): void {
   disownActiveRun();
+  cancelQueuedStorageWorkerSpawns();
   if (activeWorker) {
     void terminateStorageWorker(activeWorker);
     activeWorker = null;

src/storage/restore-job.ts

@@ -75,7 +75,12 @@ export function setRestoreTrashJobTestHooks(hooks: RestoreJobTestHooks | null):
   setStorageMutationCoordinatorTestHooks(hooks);
 }
 
+/**
+ * Fire-and-forget reset. Prefer {@link resetRestoreTrashJobForTestsAsync} from
+ * test beforeEach/afterEach under `bun test --isolate` on Windows.
+ */
 export function resetRestoreTrashJobForTests(): void {
+  cancelQueuedStorageWorkerSpawns();
   if (activeWorker) {
     void terminateStorageWorker(activeWorker);
     activeWorker = null;
@@ -98,13 +103,19 @@ export async function resetRestoreTrashJobForTestsAsync(): Promise<void> {
   cancelActiveRun?.();
   cancelActiveRun = null;
   testHooks = null;
-  resetStorageMutationCoordinatorForTests();
-  if (worker) await terminateStorageWorker(worker);
-  await drainStorageWorkers();
+  // Join the worker before clearing the mutation coordinator so a concurrent
+  // run cannot acquire CODEX_HOME while the aborted thread is still mutating.
+  try {
+    if (worker) await terminateStorageWorker(worker);
+    await drainStorageWorkers();
+  } finally {
+    resetStorageMutationCoordinatorForTests();
+  }
 }
 
 /** Terminate an in-flight worker during process shutdown. */
 export function abortRestoreTrashJob(): void {
+  cancelQueuedStorageWorkerSpawns();
   if (activeWorker) {
     void terminateStorageWorker(activeWorker);
     activeWorker = null;

src/storage/worker-lifecycle.ts

@@ -52,8 +52,10 @@ let spawnCancelEpoch = 0;
 /**
  * OS-join gap after the `close` event on platforms where Bun's Worker reclaim
  * races the isolate/file boundary (not a CI job-timeout bump).
+ * Windows GHA at 250ms still left `workers_spawned(N) workers_terminated(N-1)`
+ * panics under isolate; 750ms covers deferred reclaim. Darwin uses 250ms.
  */
-const WORKER_OS_JOIN_MS = 250;
+const WORKER_OS_JOIN_MS = process.platform === "win32" ? 750 : 250;
 
 function needsWorkerOsJoinSettle(): boolean {
   return process.platform === "win32" || process.platform === "darwin";

tests/api-storage-policy-already-running.test.ts

@@ -9,7 +9,7 @@ import {
   seedArchived,
   setStorageCleanupPolicyJobTestHooks,
   startServer,
-  stopStorageCleanupScheduler,
+  stopPolicyServer,
   uninstallPolicyApiHarness,
   waitForJobIdle,
   type PolicyApiHarness,
@@ -18,8 +18,8 @@ import {
 
 let harness: PolicyApiHarness;
 
-beforeEach(() => {
-  harness = installPolicyApiHarness("ocx-api-storage-policy-busy");
+beforeEach(async () => {
+  harness = await installPolicyApiHarness("ocx-api-storage-policy-busy");
 });
 
 afterEach(async () => {
@@ -60,8 +60,7 @@ test("POST run rejects when a job is already running", async () => {
 
     await waitForJobIdle(server.url, firstBody.job.startedAt);
   } finally {
-    await server.stop(true);
-    stopStorageCleanupScheduler();
+    await stopPolicyServer(server);
     await resetStorageCleanupPolicyJobForTestsAsync();
   }
 }, { timeout: 30_000 });

tests/api-storage-policy-mutation-busy.test.ts

@@ -9,7 +9,7 @@ import {
   seedArchived,
   setArchivedCleanupJobTestHooks,
   startServer,
-  stopStorageCleanupScheduler,
+  stopPolicyServer,
   uninstallPolicyApiHarness,
   waitForJobIdle,
   type PolicyApiHarness,
@@ -18,8 +18,8 @@ import {
 
 let harness: PolicyApiHarness;
 
-beforeEach(() => {
-  harness = installPolicyApiHarness("ocx-api-storage-policy-mut-busy");
+beforeEach(async () => {
+  harness = await installPolicyApiHarness("ocx-api-storage-policy-mut-busy");
 });
 
 afterEach(async () => {
@@ -78,8 +78,7 @@ test("storage_mutation_busy clears inflight so a later policy run can start", as
     expect(retryDone.job.lastOutcome?.skipped).toBeUndefined();
     expect(retryDone.job.lastOutcome?.removed).toBe(1);
   } finally {
-    await server.stop(true);
-    stopStorageCleanupScheduler();
+    await stopPolicyServer(server);
     await resetStorageCleanupPolicyJobForTestsAsync();
   }
 }, { timeout: 30_000 });

tests/api-storage-policy-put-race.test.ts

@@ -9,7 +9,7 @@ import {
   seedArchived,
   setStorageCleanupPolicyJobTestHooks,
   startServer,
-  stopStorageCleanupScheduler,
+  stopPolicyServer,
   uninstallPolicyApiHarness,
   waitForJobIdle,
   type PolicyApiHarness,
@@ -18,8 +18,8 @@ import {
 
 let harness: PolicyApiHarness;
 
-beforeEach(() => {
-  harness = installPolicyApiHarness("ocx-api-storage-policy-put-race");
+beforeEach(async () => {
+  harness = await installPolicyApiHarness("ocx-api-storage-policy-put-race");
 });
 
 afterEach(async () => {
@@ -109,8 +109,7 @@ test("blocked worker completion preserves concurrent policy PUT edits", async ()
     expect(typeof body.lastRun?.at).toBe("number");
     expect(typeof body.nextRun).toBe("number");
   } finally {
-    await server.stop(true);
-    stopStorageCleanupScheduler();
+    await stopPolicyServer(server);
     await resetStorageCleanupPolicyJobForTestsAsync();
   }
 }, { timeout: 30_000 });