diff --git a/docs/help/testing/writing-tests.md b/docs/help/testing/writing-tests.md index 06f761c362d9..3696f2a3997c 100644 --- a/docs/help/testing/writing-tests.md +++ b/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 diff --git a/test/non-isolated-runner.skills-watcher-fixtures.ts b/test/non-isolated-runner.skills-watcher-fixtures.ts new file mode 100644 index 000000000000..b9db67dc9f4e --- /dev/null +++ b/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 { + 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); + } +}); +`, + }; +} diff --git a/test/non-isolated-runner.test.ts b/test/non-isolated-runner.test.ts index f33f11a3e505..311d188dcbfa 100644 --- a/test/non-isolated-runner.test.ts +++ b/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)}`, diff --git a/test/non-isolated-runner.ts b/test/non-isolated-runner.ts index 9b79cab4b047..f7eaa2da1888 100644 --- a/test/non-isolated-runner.ts +++ b/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)[SUBAGENT_REGISTRY_TEST_API] as diff --git a/test/skills-watcher-test-lifecycle.ts b/test/skills-watcher-test-lifecycle.ts new file mode 100644 index 000000000000..45c8d6e4be6b --- /dev/null +++ b/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(); + +function realExports( + node: EvaluatedModuleNode | undefined, + executions: ReadonlyMap, +): 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, + executions: ReadonlyMap, +): void { + for (const node of modules.fileToModulesMap.get(refreshSource) ?? []) { + const close = (realExports(node, executions) as Partial | 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 + | 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 { + 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; +}