From 5ba55848d01e8252220670c2ca928750d8462f7e Mon Sep 17 00:00:00 2001 From: Chuan-kai Lin Date: Thu, 4 Jun 2026 14:31:19 -0700 Subject: [PATCH] Fix flaky Windows tests: dispose file watchers and use retried cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The root cause was twofold: 1. MultiFileSystemWatcher.addWatcher() never disposed the FileSystemWatcher object itself — only its event subscriptions. This leaked OS file handles on Windows, preventing temp directory deletion. 2. The tests used tmp's synchronous rimrafSync for cleanup, which has no retry logic for Windows' asynchronous handle release. Fix: - Dispose the FileSystemWatcher itself in MultiFileSystemWatcher - Replace removeCallback with async fs.rm with maxRetries, giving Windows time to release handles between retries Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/vscode/multi-file-system-watcher.ts | 1 + .../common/vscode/file-path-discovery.test.ts | 17 +++++++++-------- .../queries-panel/query-discovery.test.ts | 17 +++++++++-------- .../queries-panel/query-pack-discovery.test.ts | 17 +++++++++-------- 4 files changed, 28 insertions(+), 24 deletions(-) diff --git a/extensions/ql-vscode/src/common/vscode/multi-file-system-watcher.ts b/extensions/ql-vscode/src/common/vscode/multi-file-system-watcher.ts index 8fdbad76ac5..2bcdb911454 100644 --- a/extensions/ql-vscode/src/common/vscode/multi-file-system-watcher.ts +++ b/extensions/ql-vscode/src/common/vscode/multi-file-system-watcher.ts @@ -20,6 +20,7 @@ class WatcherCollection extends DisposableObject { */ public addWatcher(pattern: GlobPattern, listener: (e: Uri) => void): void { const watcher = workspace.createFileSystemWatcher(pattern); + this.push(watcher); this.push(watcher.onDidCreate(listener)); this.push(watcher.onDidChange(listener)); this.push(watcher.onDidDelete(listener)); diff --git a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/common/vscode/file-path-discovery.test.ts b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/common/vscode/file-path-discovery.test.ts index 134ce87af63..99dadc0cd85 100644 --- a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/common/vscode/file-path-discovery.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/common/vscode/file-path-discovery.test.ts @@ -7,6 +7,7 @@ import { EventEmitter, Uri, workspace } from "vscode"; import { FilePathDiscovery } from "../../../../../src/common/vscode/file-path-discovery"; import { basename, dirname, join } from "path"; import { mkdirSync, readFileSync, rmSync, writeFileSync } from "fs"; +import { rm } from "fs/promises"; import { dirSync } from "tmp"; import { normalizePath } from "../../../../../src/common/files"; import { extLogger } from "../../../../../src/common/logging/vscode/loggers"; @@ -60,7 +61,6 @@ class TestFilePathDiscovery extends FilePathDiscovery { describe("FilePathDiscovery", () => { let tmpDir: string; - let tmpDirRemoveCallback: (() => void) | undefined; let workspaceFolder: WorkspaceFolder; let workspacePath: string; @@ -81,11 +81,7 @@ describe("FilePathDiscovery", () => { let discovery: TestFilePathDiscovery; beforeEach(() => { - const t = dirSync({ - unsafeCleanup: true, - }); - tmpDir = normalizePath(t.name); - tmpDirRemoveCallback = t.removeCallback; + tmpDir = normalizePath(dirSync().name); workspaceFolder = { uri: Uri.file(join(tmpDir, "workspace")), @@ -117,9 +113,14 @@ describe("FilePathDiscovery", () => { discovery = new TestFilePathDiscovery(); }); - afterEach(() => { - tmpDirRemoveCallback?.(); + afterEach(async () => { discovery.dispose(); + await rm(tmpDir, { + recursive: true, + force: true, + maxRetries: 10, + retryDelay: 200, + }); }); describe("initialRefresh", () => { diff --git a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts index 7ec97882d61..dbde79ef81e 100644 --- a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-discovery.test.ts @@ -4,6 +4,7 @@ import { QueryDiscovery } from "../../../../src/queries-panel/query-discovery"; import { createMockApp } from "../../../__mocks__/appMock"; import { basename, dirname, join } from "path"; import { dirSync } from "tmp"; +import { rm } from "fs/promises"; import { FileTreeDirectory, FileTreeLeaf, @@ -15,7 +16,6 @@ import { LanguageContextStore } from "../../../../src/language-context-store"; describe("Query pack discovery", () => { let tmpDir: string; - let tmpDirRemoveCallback: (() => void) | undefined; let workspacePath: string; @@ -29,11 +29,7 @@ describe("Query pack discovery", () => { let discovery: QueryDiscovery; beforeEach(() => { - const t = dirSync({ - unsafeCleanup: true, - }); - tmpDir = t.name; - tmpDirRemoveCallback = t.removeCallback; + tmpDir = dirSync().name; const workspaceFolder = { uri: Uri.file(join(tmpDir, "workspace")), @@ -52,9 +48,14 @@ describe("Query pack discovery", () => { discovery = new QueryDiscovery(app, queryPackDiscoverer, languageContext); }); - afterEach(() => { - tmpDirRemoveCallback?.(); + afterEach(async () => { discovery.dispose(); + await rm(tmpDir, { + recursive: true, + force: true, + maxRetries: 10, + retryDelay: 200, + }); }); describe("buildQueryTree", () => { diff --git a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-pack-discovery.test.ts b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-pack-discovery.test.ts index e09be397d2c..326c4e5813e 100644 --- a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-pack-discovery.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/queries-panel/query-pack-discovery.test.ts @@ -1,24 +1,20 @@ import { Uri, workspace } from "vscode"; import { QueryPackDiscovery } from "../../../../src/queries-panel/query-pack-discovery"; import { dirSync } from "tmp"; +import { rm } from "fs/promises"; import { dirname, join } from "path"; import { ensureDir, writeJSON } from "fs-extra"; import { QueryLanguage } from "../../../../src/common/query-language"; describe("Query pack discovery", () => { let tmpDir: string; - let tmpDirRemoveCallback: (() => void) | undefined; let workspacePath: string; let discovery: QueryPackDiscovery; beforeEach(() => { - const t = dirSync({ - unsafeCleanup: true, - }); - tmpDir = t.name; - tmpDirRemoveCallback = t.removeCallback; + tmpDir = dirSync().name; const workspaceFolder = { uri: Uri.file(join(tmpDir, "workspace")), @@ -33,9 +29,14 @@ describe("Query pack discovery", () => { discovery = new QueryPackDiscovery(); }); - afterEach(() => { - tmpDirRemoveCallback?.(); + afterEach(async () => { discovery.dispose(); + await rm(tmpDir, { + recursive: true, + force: true, + maxRetries: 10, + retryDelay: 200, + }); }); describe("findQueryPack", () => {