fix(test): leaked skills watchers flake later fake-timer tests instead of failing their file (#162454)

skills.status and skill snapshot preparation start real @openclaw/fs-safe
watchers. In the shared-worker (non-isolated) Vitest lanes a file that never
closed them left them running after the module reset, unreachable, and their
re-armed poll and change-hint timers landed on whichever fake clock a later
file installed. That file's vi.runAllTimersAsync() then aborted after 10000
timers, far from the cause (session-catalog.session-share.test.ts; leaker
fixed in #162261, an earlier occurrence patched at the victim in #114751).

The non-isolated runner now remembers every real refresh.ts generation from
Vite's evaluated module graph, pairing each closeSkillsWatchers export with
the registry instance that generation imported. It captures at task
boundaries and before every vi.resetModules(), so generations a test erases
or shadows mid-test are still seen. After each file it closes leftover
watchers and fails that file with "skills watchers failed", naming it. No
production code changes.

The composite runner test gains a producer/observer pair: the producer
leaves two reset and shadowed generations open, and the observer in the next
file asserts both were closed. Without the drain, the reset wrapper, or the
import-graph pairing, the producer passes and the observer sees a live owner.

docs/help/testing/writing-tests.md

@@ -114,6 +114,16 @@ Otherwise the raw connection can fail with `SQLITE_BUSY`, or the snapshot can
 change underneath the test. `PRAGMA locking_mode=EXCLUSIVE; BEGIN EXCLUSIVE` on
 a raw connection proves that no other connection remains.
 
+## Skills watchers
+
+`skills.status` and skill snapshot preparation start real `@openclaw/fs-safe`
+watchers. In shared-worker lanes, the non-isolated runner closes any watchers a
+file leaves open and fails that file with `skills watchers failed`; otherwise
+their re-armed timers land on a later file's fake clock and abort its
+`vi.runAllTimersAsync()`. Close them in `afterEach` with
+`closeSkillsWatchers(true)`, or set `skills.load.watch: false` when the test
+does not exercise watching.
+
 ## Flake triage
 
 A failure without a related change is a defect. Never re-run, re-push, or refresh

test/non-isolated-runner.skills-watcher-fixtures.ts

@@ -0,0 +1,51 @@
+import path from "node:path";
+
+// A file that leaves real skills watchers open must fail its own teardown, and the
+// runner must close those watchers before the next file can install a fake clock,
+// including generations a test erased with vi.resetModules() or shadowed by a
+// same-file instance.
+export function skillsWatcherFixtureFiles(
+  repoRoot: string,
+  fixtureRoot: string,
+): Record<string, string> {
+  const sourcePath = (name: string) => JSON.stringify(path.join(repoRoot, "src", name));
+  const workspaceRoot = JSON.stringify(path.join(fixtureRoot, "skills-watcher-workspaces"));
+  return {
+    "12-a-skills-watcher-leak.test.ts": `import { mkdirSync } from "node:fs";
+import path from "node:path";
+import { expect, it, vi } from "vitest";
+it("starts real skills watchers and resets modules before returning", async () => {
+  const registries = [];
+  for (const name of ["first", "second"]) {
+    vi.resetModules();
+    const { ensureSkillsWatcher } = await import(${sourcePath("skills/runtime/refresh.ts")});
+    const registry = await import(${sourcePath("skills/runtime/refresh-watch-registry.ts")});
+    const workspaceDir = path.join(${workspaceRoot}, name);
+    mkdirSync(path.join(workspaceDir, "skills"), { recursive: true });
+    ensureSkillsWatcher({ workspaceDir, config: {} });
+    expect(registry.workspaceWatchOwners.size).toBe(1);
+    expect(registry.pathWatchers.size).toBeGreaterThan(0);
+    // A same-file instance with empty maps must not hide the live registry.
+    const shadow = await import(${sourcePath("skills/runtime/refresh-watch-registry.ts")} + "?shadow-" + name);
+    expect(shadow.pathWatchers).not.toBe(registry.pathWatchers);
+    expect(shadow.pathWatchers.size).toBe(0);
+    registries.push(registry);
+  }
+  vi.resetModules();
+  Reflect.set(globalThis, Symbol.for("fixture.leakedSkillsWatchRegistries"), registries);
+});
+`,
+    "12-b-skills-watcher-observer.test.ts": `import { expect, it } from "vitest";
+it("starts after the runner closed every prior skills watcher generation", () => {
+  const key = Symbol.for("fixture.leakedSkillsWatchRegistries");
+  const registries = Reflect.get(globalThis, key);
+  Reflect.deleteProperty(globalThis, key);
+  expect(registries, "leak fixture must run first").toHaveLength(2);
+  for (const registry of registries) {
+    expect(registry.workspaceWatchOwners.size).toBe(0);
+    expect(registry.pathWatchers.size).toBe(0);
+  }
+});
+`,
+  };
+}

test/non-isolated-runner.test.ts

@@ -13,6 +13,7 @@ import { runVitestShutdownCommand } from "./helpers/vitest-shutdown-command.ts";
 import { agentReaderFixtureFiles } from "./non-isolated-runner.agent-reader-fixtures.ts";
 import { gatewayWorkerLifetimeFixtureFiles } from "./non-isolated-runner.gateway-lifecycle-fixtures.ts";
 import { mockResolutionFixtureFiles } from "./non-isolated-runner.mock-resolution-fixtures.ts";
+import { skillsWatcherFixtureFiles } from "./non-isolated-runner.skills-watcher-fixtures.ts";
 import { testApiLifecycleFixtureFiles } from "./non-isolated-runner.test-api-fixtures.ts";
 
 const repoRoot = path.resolve(import.meta.dirname, "..");
@@ -433,6 +434,7 @@ it("reloads the redirected mock after a real import", () => {
     ...testApiLifecycleFixtureFiles(repoRoot),
     ...documentFocusFixtureFiles(),
     ...agentReaderFixtureFiles(repoRoot, fixtureRoot),
+    ...skillsWatcherFixtureFiles(repoRoot, fixtureRoot),
   };
 }
 
@@ -460,7 +462,7 @@ async function assertCompletion(
     pid: expected.pid,
     root: expected.root,
     processTimedOut: false,
-    ended: { reason: "failed", unhandledErrors: 0, failedModules: 1, suiteErrors: 1 },
+    ended: { reason: "failed", unhandledErrors: 0, failedModules: 2, suiteErrors: 2 },
   });
   const project = {
     name: "non-isolated-runner",
@@ -478,8 +480,8 @@ async function assertCompletion(
   const report: JsonTestResults = JSON.parse(await fs.readFile(expected.reportPath, "utf8"));
   expect(report.testResults.map((file) => file.name).toSorted()).toEqual(expected.files);
   expect(report).toMatchObject({
-    numTotalTests: 51,
-    numPassedTests: 50,
+    numTotalTests: 53,
+    numPassedTests: 52,
     numPendingTests: 1,
     numFailedTests: 0,
     numTodoTests: 0,
@@ -487,13 +489,20 @@ async function assertCompletion(
   for (const file of report.testResults) {
     const name = path.basename(file.name);
     const crashed = name === "01-a-crash.test.ts";
+    const leakedWatchers = name === "12-a-skills-watcher-leak.test.ts";
     const skipped = name === "09-f-test-api-skipped.test.ts";
     const lifecycle = ["09-d-test-api-producer.test.ts", "09-e-test-api-observer.test.ts"].includes(
       name,
     );
     const count = crashed ? 0 : lifecycle ? 2 : 1;
-    expect(file.status, name).toBe(crashed ? "failed" : "passed");
-    expect(file.message, name).toBe(crashed ? "synthetic collect failure" : "");
+    expect(file.status, name).toBe(crashed || leakedWatchers ? "failed" : "passed");
+    if (leakedWatchers) {
+      expect(file.message, name).toMatch(
+        /^12-a-skills-watcher-leak\.test\.ts: skills watchers failed\nError: left skills watchers open /u,
+      );
+    } else {
+      expect(file.message, name).toBe(crashed ? "synthetic collect failure" : "");
+    }
     expect(file.assertionResults, name).toHaveLength(count);
     expect(new Set(file.assertionResults.map((test) => test.fullName)).size, name).toBe(count);
     for (const test of file.assertionResults) {
@@ -664,6 +673,16 @@ export default defineConfig({
             { message: "other" },
           ),
       ],
+      [
+        "unattributed skills watcher leak",
+        ({ report }) =>
+          Object.assign(
+            report.testResults.find((file) =>
+              file.name.endsWith("/12-a-skills-watcher-leak.test.ts"),
+            )!,
+            { status: "passed", message: "" },
+          ),
+      ],
       ["inconsistent totals", ({ report }) => Object.assign(report, { numPassedTests: 44 })],
     ];
     for (const patch of [
@@ -681,10 +700,10 @@ export default defineConfig({
       { reason: "interrupted" },
       { reason: "passed" },
       { unhandledErrors: 1 },
-      { failedModules: 0 },
-      { failedModules: 2 },
-      { suiteErrors: 0 },
-      { suiteErrors: 2 },
+      { failedModules: 1 },
+      { failedModules: 3 },
+      { suiteErrors: 1 },
+      { suiteErrors: 3 },
     ]) {
       faults.push([
         `invalid native end: ${JSON.stringify(patch)}`,

test/non-isolated-runner.ts

@@ -29,6 +29,10 @@ import {
   trackCustomElementRegistry,
 } from "./jsdom-custom-elements.ts";
 import { repositoryTestApiPublications } from "./repository-test-api-publications.ts";
+import {
+  closeLeakedSkillsWatchers,
+  rememberSkillsWatcherGenerations,
+} from "./skills-watcher-test-lifecycle.ts";
 import {
   drainSqliteTestAgentOwner,
   drainSqliteTestSingletons,
@@ -98,6 +102,14 @@ const nativeTimerGlobals = {
   clearImmediate: globalThis.clearImmediate,
   Date: globalThis.Date,
 };
+// vi.resetModules() inside a test clears module exports before the next task boundary.
+// Remember skills watcher generations first so the file drain can still close them.
+let beforeModuleReset: (() => void) | undefined;
+const nativeResetModules = vi.resetModules;
+vi.resetModules = () => {
+  beforeModuleReset?.();
+  return nativeResetModules();
+};
 
 function getSharedTestHome(): string | undefined {
   const globalState = globalThis as typeof globalThis & {
@@ -432,6 +444,7 @@ export default class OpenClawNonIsolatedRunner extends TestRunner {
 
   override onCollectStart(file: RunnerTestFile) {
     super.onCollectStart(file);
+    beforeModuleReset = () => this.rememberSkillsWatchers();
     if (!this.config.isolate) {
       installCustomElementTracking();
     }
@@ -447,10 +460,12 @@ export default class OpenClawNonIsolatedRunner extends TestRunner {
     await settleSqliteTestAgentCloses();
     await super.onBeforeRunTask(test);
     this.rememberSqliteAgentOwner();
+    this.rememberSkillsWatchers();
   }
 
   onTaskFinished() {
     this.rememberSqliteAgentOwner();
+    this.rememberSkillsWatchers();
   }
 
   private rememberSqliteAgentOwner() {
@@ -464,6 +479,14 @@ export default class OpenClawNonIsolatedRunner extends TestRunner {
     );
   }
 
+  private rememberSkillsWatchers() {
+    const internals = this as unknown as TestRunnerInternals;
+    rememberSkillsWatcherGenerations(
+      internals.workerState.evaluatedModules as ViteEvaluatedModules,
+      internals.workerState.moduleExecutionInfo,
+    );
+  }
+
   // Teardown may only schedule agent database closes (the synchronous test closer does).
   // Wait for that Worker retirement before each test (onBeforeRunTask) and retry attempt
   // so a lease release never overlaps later work. Like the file drain, this waits
@@ -553,6 +576,19 @@ export default class OpenClawNonIsolatedRunner extends TestRunner {
     ] as const) {
       clean(phase, run);
     }
+    // After the module reset nothing can reach this file's watchers, and their re-arms
+    // land on a later file's fake clock. Close them now and fail this file, not that one.
+    this.rememberSkillsWatchers();
+    await drain("skills watchers", async () => {
+      const leaked = await closeLeakedSkillsWatchers();
+      if (leaked > 0) {
+        throw new Error(
+          `left skills watchers open (${leaked} live watch entries); skills.status and skill snapshot preparation start real watchers, so close them in afterEach with closeSkillsWatchers(true) or disable watching with skills.load.watch: false`,
+        );
+      }
+    });
+    // The runner's own module reset below must not retain this file's closed generation.
+    beforeModuleReset = undefined;
     if (
       !(await drain("subagent registry", async () => {
         const api = (globalThis as Record<PropertyKey, unknown>)[SUBAGENT_REGISTRY_TEST_API] as

test/skills-watcher-test-lifecycle.ts

@@ -0,0 +1,83 @@
+// Shared workers reset modules between files, orphaning skills watchers a file left open.
+// Their re-armed timers then land on a later file's fake clock and abort its
+// vi.runAllTimersAsync(). Remember each real watcher generation so the runner can close
+// leftovers after the file and fail the file that leaked them.
+import path from "node:path";
+import {
+  normalizeModuleId,
+  type EvaluatedModuleNode,
+  type EvaluatedModules,
+} from "vite/module-runner";
+import { vi } from "vitest";
+
+const source = (name: string) => normalizeModuleId(path.resolve(import.meta.dirname, "..", name));
+const refreshSource = source("src/skills/runtime/refresh.ts");
+const registrySource = source("src/skills/runtime/refresh-watch-registry.ts");
+
+type RefreshModule = typeof import("../src/skills/runtime/refresh.js");
+type SkillsWatchRegistry = Pick<
+  typeof import("../src/skills/runtime/refresh-watch-registry.js"),
+  "pathWatchers" | "workspaceWatchOwners"
+>;
+
+// A test's vi.resetModules() replaces these exports; keep every generation the runner saw.
+const generations = new Map<RefreshModule["closeSkillsWatchers"], SkillsWatchRegistry>();
+
+function realExports(
+  node: EvaluatedModuleNode | undefined,
+  executions: ReadonlyMap<string, { external?: boolean }>,
+): unknown {
+  const execution =
+    node && executions.get(node.id.startsWith("mock:") ? node.id.slice(5) : node.id);
+  return execution && !execution.external ? node.exports : undefined;
+}
+
+export function rememberSkillsWatcherGenerations(
+  modules: Pick<EvaluatedModules, "fileToModulesMap" | "idToModuleMap">,
+  executions: ReadonlyMap<string, { external?: boolean }>,
+): void {
+  for (const node of modules.fileToModulesMap.get(refreshSource) ?? []) {
+    const close = (realExports(node, executions) as Partial<RefreshModule> | undefined)
+      ?.closeSkillsWatchers;
+    if (typeof close !== "function" || vi.isMockFunction(close)) {
+      continue;
+    }
+    // Same-file instances (query or importActual variants) can coexist; pair each closer
+    // with the registry instance its own evaluation imported.
+    for (const id of node.imports) {
+      const dependency = modules.idToModuleMap.get(id);
+      if (dependency?.file !== registrySource) {
+        continue;
+      }
+      const registry = realExports(dependency, executions) as
+        | Partial<SkillsWatchRegistry>
+        | undefined;
+      const { pathWatchers, workspaceWatchOwners } = registry ?? {};
+      if (pathWatchers instanceof Map && workspaceWatchOwners instanceof Map) {
+        generations.set(close, { pathWatchers, workspaceWatchOwners });
+      }
+      break;
+    }
+  }
+}
+
+/** Closes every remembered generation's open watchers and returns their live entry count. */
+export async function closeLeakedSkillsWatchers(): Promise<number> {
+  const remembered = [...generations];
+  generations.clear();
+  let leaked = 0;
+  const failures: unknown[] = [];
+  for (const [close, registry] of remembered) {
+    // Retiring watchers are already aborted; owners and path watchers can still re-arm.
+    const live = registry.workspaceWatchOwners.size + registry.pathWatchers.size;
+    if (live > 0) {
+      leaked += live;
+      // One failed shutdown must not leave the remaining generations open.
+      await close(true).catch((error: unknown) => failures.push(error));
+    }
+  }
+  if (failures.length > 0) {
+    throw new AggregateError(failures, `Closing ${leaked} leaked skills watch entries failed`);
+  }
+  return leaked;
+}