From b665708c7ff473a237222c968ff0117c3065db78 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 8 Jul 2026 12:01:06 -0700 Subject: [PATCH] fix(ssh): make fs.listFiles cancellable and single-flight per relay client (#7769) Co-authored-by: Orca --- src/main/ipc/filesystem.test.ts | 35 +++ src/main/ipc/filesystem.ts | 56 +++-- .../providers/ssh-filesystem-provider.test.ts | 35 ++- src/main/providers/ssh-filesystem-provider.ts | 12 +- src/main/providers/types.ts | 5 +- src/preload/api-types.ts | 2 + src/preload/index.ts | 3 + src/relay/fs-handler-git-fallback.ts | 29 ++- .../fs-handler-list-files-cancel.test.ts | 127 ++++++++++ src/relay/fs-handler-list-files.ts | 29 ++- src/relay/fs-handler-readdir-fallback.ts | 6 +- src/relay/fs-handler.ts | 38 ++- .../fs-list-files-cancel.integration.test.ts | 209 +++++++++++++++++ .../fs-list-files-scan-coordinator.test.ts | 222 ++++++++++++++++++ src/relay/fs-list-files-scan-coordinator.ts | 104 ++++++++ .../quick-open-file-list.react.test.tsx | 35 ++- .../src/components/quick-open-file-list.ts | 36 +-- .../src/runtime/runtime-file-client.test.ts | 58 +++++ .../src/runtime/runtime-file-client.ts | 23 +- src/renderer/src/web/web-preload-api.ts | 4 + src/shared/file-listing-cancellation.ts | 38 +++ src/shared/quick-open-readdir-walk.test.ts | 32 +++ src/shared/quick-open-readdir-walk.ts | 11 +- 23 files changed, 1090 insertions(+), 59 deletions(-) create mode 100644 src/relay/fs-handler-list-files-cancel.test.ts create mode 100644 src/relay/fs-list-files-cancel.integration.test.ts create mode 100644 src/relay/fs-list-files-scan-coordinator.test.ts create mode 100644 src/relay/fs-list-files-scan-coordinator.ts create mode 100644 src/shared/file-listing-cancellation.ts diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts index 25894ffb723..072f8239dcb 100644 --- a/src/main/ipc/filesystem.test.ts +++ b/src/main/ipc/filesystem.test.ts @@ -2316,4 +2316,39 @@ describe('registerFilesystemHandlers', () => { excludePaths: ['/home/user/repo/worktrees/feature'] }) }) + + // Why #7721: without a cancel path, every workspace switch left the previous + // workspace's full-tree SSH scan running, stacking scans on the relay until + // interactive fs.readDir/fs.stat starved past their 30s timeout. + it('fs:cancelListFiles aborts an in-flight SSH listing by request token (#7721)', async () => { + let capturedSignal: AbortSignal | undefined + const listFilesMock = vi.fn( + (_rootPath: string, options: { signal?: AbortSignal }) => + new Promise((_resolve, reject) => { + capturedSignal = options.signal + options.signal?.addEventListener('abort', () => reject(new Error('listing cancelled')), { + once: true + }) + }) + ) + getSshFilesystemProviderMock.mockReturnValue({ listFiles: listFilesMock }) + + registerFilesystemHandlers(store as never) + + const pending = handlers.get('fs:listFiles')!(null, { + rootPath: '/home/user/repo', + connectionId: 'conn-1', + requestToken: 'token-1' + }) as Promise + + expect(capturedSignal?.aborted).toBe(false) + await handlers.get('fs:cancelListFiles')!(null, { requestToken: 'token-1' }) + expect(capturedSignal?.aborted).toBe(true) + await expect(pending).rejects.toThrow('listing cancelled') + + // Unknown or already-settled tokens are a no-op, not an error. + expect(() => + handlers.get('fs:cancelListFiles')!(null, { requestToken: 'unknown' }) + ).not.toThrow() + }) }) diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 85c0ce56e31..7e7006a375e 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -1026,30 +1026,58 @@ export function registerFilesystemHandlers( ) // ─── List all files (for quick-open) ───────────────────── + // Why #7721: keyed by renderer-generated token so a workspace switch can + // abort the previous workspace's full-tree scan (SSH relays otherwise stack + // scans that starve interactive fs.readDir/fs.stat past their 30s timeout). + const listFilesCancellations = new Map() ipcMain.handle( 'fs:listFiles', async ( _event, - args: { rootPath: string; connectionId?: string; excludePaths?: string[] } + args: { + rootPath: string + connectionId?: string + excludePaths?: string[] + requestToken?: string + } ): Promise => { - if (args.connectionId) { - const provider = getSshFilesystemProvider(args.connectionId) - // Why: when the SSH connection is not yet established (cold start) or - // temporarily disconnected, return [] so quick-open shows "No matching - // files" instead of an error banner. The file list will repopulate when - // the user re-opens quick-open after the connection is restored. - if (!provider) { - return [] + const controller = args.requestToken ? new AbortController() : null + if (controller && args.requestToken) { + listFilesCancellations.set(args.requestToken, controller) + } + try { + if (args.connectionId) { + const provider = getSshFilesystemProvider(args.connectionId) + // Why: when the SSH connection is not yet established (cold start) or + // temporarily disconnected, return [] so quick-open shows "No matching + // files" instead of an error banner. The file list will repopulate when + // the user re-opens quick-open after the connection is restored. + if (!provider) { + return [] + } + // Why: forward excludePaths through to the remote provider. + // Dropping it here would silently double-scan nested linked worktrees + // over SSH and contribute to timeout-induced partial results. + return await provider.listFiles(args.rootPath, { + excludePaths: args.excludePaths, + signal: controller?.signal + }) } - // Why: forward excludePaths through to the remote provider. - // Dropping it here would silently double-scan nested linked worktrees - // over SSH and contribute to timeout-induced partial results. - return provider.listFiles(args.rootPath, { excludePaths: args.excludePaths }) + return await listQuickOpenFiles(args.rootPath, store, args.excludePaths) + } finally { + if (args.requestToken) { + listFilesCancellations.delete(args.requestToken) + } } - return listQuickOpenFiles(args.rootPath, store, args.excludePaths) } ) + ipcMain.handle('fs:cancelListFiles', (_event, args: { requestToken: string }): void => { + // Why: best-effort — the entry is gone once the listing settles, and + // local scans are fast enough to simply let them finish. + listFilesCancellations.get(args.requestToken)?.abort() + }) + // ─── Git operations ───────────────────────────────────── ipcMain.handle( 'git:status', diff --git a/src/main/providers/ssh-filesystem-provider.test.ts b/src/main/providers/ssh-filesystem-provider.test.ts index 6d4170209e7..d35460bd489 100644 --- a/src/main/providers/ssh-filesystem-provider.test.ts +++ b/src/main/providers/ssh-filesystem-provider.test.ts @@ -417,7 +417,11 @@ describe('SshFilesystemProvider', () => { it('listFiles sends fs.listFiles request', async () => { mux.request.mockResolvedValue(['src/index.ts', 'package.json']) const result = await provider.listFiles('/home/user/project') - expect(mux.request).toHaveBeenCalledWith('fs.listFiles', { rootPath: '/home/user/project' }) + expect(mux.request).toHaveBeenCalledWith( + 'fs.listFiles', + { rootPath: '/home/user/project' }, + { signal: undefined } + ) expect(result).toEqual(['src/index.ts', 'package.json']) }) @@ -426,16 +430,35 @@ describe('SshFilesystemProvider', () => { await provider.listFiles('/home/user/project', { excludePaths: ['/home/user/project/worktrees/b'] }) - expect(mux.request).toHaveBeenCalledWith('fs.listFiles', { - rootPath: '/home/user/project', - excludePaths: ['/home/user/project/worktrees/b'] - }) + expect(mux.request).toHaveBeenCalledWith( + 'fs.listFiles', + { + rootPath: '/home/user/project', + excludePaths: ['/home/user/project/worktrees/b'] + }, + { signal: undefined } + ) }) it('listFiles omits excludePaths when empty', async () => { mux.request.mockResolvedValue([]) await provider.listFiles('/home/user/project', { excludePaths: [] }) - expect(mux.request).toHaveBeenCalledWith('fs.listFiles', { rootPath: '/home/user/project' }) + expect(mux.request).toHaveBeenCalledWith( + 'fs.listFiles', + { rootPath: '/home/user/project' }, + { signal: undefined } + ) + }) + + it('listFiles forwards the cancellation signal to the mux request (#7721)', async () => { + mux.request.mockResolvedValue([]) + const controller = new AbortController() + await provider.listFiles('/home/user/project', { signal: controller.signal }) + expect(mux.request).toHaveBeenCalledWith( + 'fs.listFiles', + { rootPath: '/home/user/project' }, + { signal: controller.signal } + ) }) describe('watch', () => { diff --git a/src/main/providers/ssh-filesystem-provider.ts b/src/main/providers/ssh-filesystem-provider.ts index 3adeeec6cb8..ce5256d2744 100644 --- a/src/main/providers/ssh-filesystem-provider.ts +++ b/src/main/providers/ssh-filesystem-provider.ts @@ -283,12 +283,20 @@ export class SshFilesystemProvider implements IFilesystemProvider { return (await this.mux.request('fs.search', opts)) as SearchResult } - async listFiles(rootPath: string, options?: { excludePaths?: string[] }): Promise { + async listFiles( + rootPath: string, + options?: { excludePaths?: string[]; signal?: AbortSignal } + ): Promise { const params: Record = { rootPath } if (options?.excludePaths && options.excludePaths.length > 0) { params.excludePaths = options.excludePaths } - return (await this.mux.request('fs.listFiles', params)) as string[] + // Why #7721: the signal lets a workspace switch send rpc.cancel so the + // relay aborts the full-tree scan instead of stacking abandoned scans + // that starve interactive fs.readDir/fs.stat on the shared SSH channel. + return (await this.mux.request('fs.listFiles', params, { + signal: options?.signal + })) as string[] } async watch(rootPath: string, callback: (events: FsChangeEvent[]) => void): Promise<() => void> { diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index 28d5d1d9447..c8e2680366b 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -197,7 +197,10 @@ export type IFilesystemProvider = { copy(source: string, destination: string): Promise realpath(filePath: string): Promise search(opts: SearchOptions): Promise - listFiles(rootPath: string, options?: { excludePaths?: string[] }): Promise + listFiles( + rootPath: string, + options?: { excludePaths?: string[]; signal?: AbortSignal } + ): Promise scanWorkspaceSpace?( rootPath: string, options?: { signal?: AbortSignal } diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 35ac8802402..78d7eb38e79 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -2247,7 +2247,9 @@ export type PreloadApi = { rootPath: string connectionId?: string excludePaths?: string[] + requestToken?: string }) => Promise + cancelListFiles: (args: { requestToken: string }) => Promise search: (args: SearchOptions & { connectionId?: string }) => Promise importExternalPaths: (args: { sourcePaths: string[] diff --git a/src/preload/index.ts b/src/preload/index.ts index b3b6a2978db..e47ae057cd8 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -2648,7 +2648,10 @@ const api = { rootPath: string connectionId?: string excludePaths?: string[] + requestToken?: string }): Promise => ipcRenderer.invoke('fs:listFiles', args), + cancelListFiles: (args: { requestToken: string }): Promise => + ipcRenderer.invoke('fs:cancelListFiles', args), search: (args: { query: string rootPath: string diff --git a/src/relay/fs-handler-git-fallback.ts b/src/relay/fs-handler-git-fallback.ts index da1f67c7ffa..8a98b9fa444 100644 --- a/src/relay/fs-handler-git-fallback.ts +++ b/src/relay/fs-handler-git-fallback.ts @@ -7,6 +7,7 @@ * a git-focused app. */ import { spawn } from 'node:child_process' +import { fileListingCancellationError } from '../shared/file-listing-cancellation' import type { SearchOptions, SearchResult } from './fs-handler-utils' import { buildGitLsFilesArgsForQuickOpen } from '../shared/quick-open-filter' import { expandQuickOpenGitFilesWithNestedRepos } from '../shared/quick-open-readdir-walk' @@ -31,8 +32,13 @@ import { buildRelayCommandEnv } from './relay-command-env' */ export function listFilesWithGit( rootPath: string, - excludePathPrefixes: readonly string[] = [] + excludePathPrefixes: readonly string[] = [], + options: { signal?: AbortSignal } = {} ): Promise { + const { signal } = options + if (signal?.aborted) { + return Promise.reject(fileListingCancellationError(signal)) + } const gitPaths = new Set() const { primary, ignoredPass } = buildGitLsFilesArgsForQuickOpen(excludePathPrefixes) const children: { @@ -145,7 +151,7 @@ export function listFilesWithGit( }) } - const killSurvivors = (): void => { + const killSurvivors = (reason: string): void => { // Why: Promise.all returns after the first failed pass, but the sibling // git process can keep streaming on SSH unless we cancel it explicitly. for (const entry of children) { @@ -155,22 +161,35 @@ export function listFilesWithGit( if (entry.child.exitCode === null && entry.child.signalCode === null) { entry.child.kill() } - entry.reject(new Error('git ls-files canceled after sibling failure')) + entry.reject(new Error(reason)) } } + // Why: a cancelled scan (workspace switch, superseded request) must stop + // its git children right away instead of streaming a huge tree the caller + // has already abandoned over the shared SSH channel. + const onAbort = (): void => killSurvivors('git ls-files cancelled') + signal?.addEventListener('abort', onAbort, { once: true }) + return Promise.all([runGitLsFiles(primary), runGitLsFiles(ignoredPass)]) .then(() => expandQuickOpenGitFilesWithNestedRepos({ rootPath, gitPaths, - excludePathPrefixes + excludePathPrefixes, + signal }) ) .catch((err) => { - killSurvivors() + killSurvivors('git ls-files canceled after sibling failure') + if (signal?.aborted) { + throw fileListingCancellationError(signal) + } throw err }) + .finally(() => { + signal?.removeEventListener('abort', onAbort) + }) } /** diff --git a/src/relay/fs-handler-list-files-cancel.test.ts b/src/relay/fs-handler-list-files-cancel.test.ts new file mode 100644 index 00000000000..02cd8da6afc --- /dev/null +++ b/src/relay/fs-handler-list-files-cancel.test.ts @@ -0,0 +1,127 @@ +/** + * Cancellation behavior of the relay file-list scanners (#7721): an aborted + * scan must kill its child processes immediately and reject with a + * cancellation error instead of streaming an abandoned tree to completion. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { spawnMock } = vi.hoisted(() => ({ + spawnMock: vi.fn() +})) + +vi.mock('child_process', () => ({ + spawn: spawnMock +})) + +import { EventEmitter } from 'node:events' +import type { ChildProcess } from 'node:child_process' +import { listFilesWithRg } from './fs-handler-list-files' +import { listFilesWithGit } from './fs-handler-git-fallback' +import { isFileListingCancellation } from '../shared/file-listing-cancellation' + +function createMockProcess(): ChildProcess { + const p = new EventEmitter() as unknown as ChildProcess + ;(p as unknown as Record).stdout = new EventEmitter() + ;( + (p as unknown as Record).stdout as EventEmitter & { + setEncoding: () => void + } + ).setEncoding = vi.fn() + ;(p as unknown as Record).stderr = new EventEmitter() + ;(p as unknown as Record).kill = vi.fn() + ;(p as unknown as Record).exitCode = null + ;(p as unknown as Record).signalCode = null + return p +} + +describe('relay list-files cancellation', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('listFilesWithRg kills both rg passes and rejects when aborted mid-flight', async () => { + const primaryProc = createMockProcess() + const ignoredProc = createMockProcess() + spawnMock.mockImplementation((_cmd: string, args: string[]) => + args.includes('--no-ignore-vcs') ? ignoredProc : primaryProc + ) + + const controller = new AbortController() + const promise = listFilesWithRg('/remote/root', [], { signal: controller.signal }) + + // Partial output before the abort — must be discarded, not resolved. + ;(primaryProc.stdout as unknown as EventEmitter).emit('data', 'src/index.ts\n') + controller.abort() + + await expect(promise).rejects.toSatisfy(isFileListingCancellation) + expect(primaryProc.kill).toHaveBeenCalled() + expect(ignoredProc.kill).toHaveBeenCalled() + + // Late close events after cancellation must not fire anything. + primaryProc.emit('close', null, 'SIGTERM') + ignoredProc.emit('close', null, 'SIGTERM') + }) + + it('listFilesWithRg rejects without spawning when the signal is already aborted', async () => { + const controller = new AbortController() + controller.abort() + + await expect( + listFilesWithRg('/remote/root', [], { signal: controller.signal }) + ).rejects.toSatisfy(isFileListingCancellation) + expect(spawnMock).not.toHaveBeenCalled() + }) + + it('listFilesWithRg still resolves normally when a signal is provided but never aborted', async () => { + const primaryProc = createMockProcess() + const ignoredProc = createMockProcess() + spawnMock.mockImplementation((_cmd: string, args: string[]) => + args.includes('--no-ignore-vcs') ? ignoredProc : primaryProc + ) + + const controller = new AbortController() + const promise = listFilesWithRg('/remote/root', [], { signal: controller.signal }) + + setTimeout(() => { + ;(primaryProc.stdout as unknown as EventEmitter).emit('data', 'src/index.ts\n') + primaryProc.emit('close', 0, null) + ;(ignoredProc.stdout as unknown as EventEmitter).emit('data', 'dist/out.js\n') + ignoredProc.emit('close', 0, null) + }, 5) + + await expect(promise).resolves.toEqual(['src/index.ts', 'dist/out.js']) + }) + + it('listFilesWithGit kills both git passes and rejects when aborted mid-flight', async () => { + const procs: ChildProcess[] = [] + spawnMock.mockImplementation(() => { + const proc = createMockProcess() + procs.push(proc) + return proc + }) + + const controller = new AbortController() + const promise = listFilesWithGit('/remote/root', [], { signal: controller.signal }) + + expect(procs).toHaveLength(2) + controller.abort() + + await expect(promise).rejects.toSatisfy(isFileListingCancellation) + expect(procs[0].kill).toHaveBeenCalled() + expect(procs[1].kill).toHaveBeenCalled() + }) + + it('listFilesWithGit rejects without spawning when the signal is already aborted', async () => { + const controller = new AbortController() + controller.abort() + + await expect( + listFilesWithGit('/remote/root', [], { signal: controller.signal }) + ).rejects.toSatisfy(isFileListingCancellation) + expect(spawnMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/relay/fs-handler-list-files.ts b/src/relay/fs-handler-list-files.ts index 99e3eb97ff9..bbe7912ee61 100644 --- a/src/relay/fs-handler-list-files.ts +++ b/src/relay/fs-handler-list-files.ts @@ -16,6 +16,7 @@ * denied on a single subdir is expected on home-dir roots) */ import { spawn, type ChildProcess } from 'node:child_process' +import { fileListingCancellationError } from '../shared/file-listing-cancellation' import { buildRgArgsForQuickOpen, normalizeQuickOpenRgLine, @@ -27,8 +28,13 @@ export const LIST_FILES_TIMEOUT_MS = 25_000 export function listFilesWithRg( rootPath: string, - excludePathPrefixes: readonly string[] = [] + excludePathPrefixes: readonly string[] = [], + options: { signal?: AbortSignal } = {} ): Promise { + const { signal } = options + if (signal?.aborted) { + return Promise.reject(fileListingCancellationError(signal)) + } return new Promise((resolve, reject) => { const files = new Set() let done = false @@ -177,7 +183,7 @@ export function listFilesWithRg( child.once('close', handleClose) }) - const killSurvivors = (): void => { + const killSurvivors = (reason: string): void => { // Why: when one pass rejects, Promise.all surfaces the error immediately // but the sibling rg keeps running up to LIST_FILES_TIMEOUT_MS. Kill it // so repeated Quick Open opens don't pile up orphan rg processes on the @@ -189,16 +195,30 @@ export function listFilesWithRg( if (entry.child.exitCode === null && entry.child.signalCode === null) { entry.child.kill() } - entry.reject(new Error('rg list canceled after sibling failure')) + entry.reject(new Error(reason)) } } + // Why: a cancelled scan (workspace switch, superseded request) must stop + // its rg children immediately instead of letting them walk the tree to + // completion and flood the relay with stdout it will only discard. + const onAbort = (): void => { + if (done) { + return + } + done = true + killSurvivors('rg list cancelled') + reject(fileListingCancellationError(signal)) + } + signal?.addEventListener('abort', onAbort, { once: true }) + Promise.all([runPass(primary), runPass(ignoredPass)]) .then(() => { if (done) { return } done = true + signal?.removeEventListener('abort', onAbort) resolve(Array.from(files)) }) .catch((err) => { @@ -206,7 +226,8 @@ export function listFilesWithRg( return } done = true - killSurvivors() + signal?.removeEventListener('abort', onAbort) + killSurvivors('rg list canceled after sibling failure') reject(err instanceof Error ? err : new Error(String(err))) }) }) diff --git a/src/relay/fs-handler-readdir-fallback.ts b/src/relay/fs-handler-readdir-fallback.ts index e49609996d1..d7c9172183b 100644 --- a/src/relay/fs-handler-readdir-fallback.ts +++ b/src/relay/fs-handler-readdir-fallback.ts @@ -23,10 +23,12 @@ import { */ export async function listFilesWithReaddir( rootPath: string, - excludePathPrefixes: readonly string[] = [] + excludePathPrefixes: readonly string[] = [], + options: { signal?: AbortSignal } = {} ): Promise { return listQuickOpenFilesWithReaddir(rootPath, { excludePathPrefixes, - budget: createQuickOpenReaddirBudget() + budget: createQuickOpenReaddirBudget(), + signal: options.signal }) } diff --git a/src/relay/fs-handler.ts b/src/relay/fs-handler.ts index c6564c51fbe..f6417b38d3c 100644 --- a/src/relay/fs-handler.ts +++ b/src/relay/fs-handler.ts @@ -17,6 +17,11 @@ import { } from './fs-handler-utils' import { listFilesWithGit, searchWithGitGrep } from './fs-handler-git-fallback' import { listFilesWithReaddir } from './fs-handler-readdir-fallback' +import { ListFilesScanCoordinator } from './fs-list-files-scan-coordinator' +import { + isFileListingCancellation, + throwIfFileListingCancelled +} from '../shared/file-listing-cancellation' import { isQuickOpenReaddirBudgetError } from '../shared/quick-open-readdir-walk' import { buildExcludePathPrefixes } from '../shared/quick-open-filter' import { buildInstallRgMessage } from './fs-handler-install-rg' @@ -82,6 +87,7 @@ export class FsHandler { private dispatcher: RelayDispatcher private watches = new Map() private streamRegistry = new RelayStreamRegistry() + private listFilesScans = new ListFilesScanCoordinator() constructor(dispatcher: RelayDispatcher, _context: RelayContext) { this.dispatcher = dispatcher @@ -114,7 +120,7 @@ export class FsHandler { this.dispatcher.onRequest('fs.copy', (p) => this.copy(p)) this.dispatcher.onRequest('fs.realpath', (p) => this.realpath(p)) this.dispatcher.onRequest('fs.search', (p) => this.search(p)) - this.dispatcher.onRequest('fs.listFiles', (p) => this.listFiles(p)) + this.dispatcher.onRequest('fs.listFiles', (p, c) => this.listFiles(p, c)) this.dispatcher.onRequest('fs.workspaceSpaceScan', (p, c) => this.workspaceSpaceScan(p, c)) this.dispatcher.onRequest('fs.watch', (p, context) => this.watch(p, context)) this.dispatcher.onNotification('fs.unwatch', (p, context) => this.unwatch(p, context)) @@ -331,16 +337,33 @@ export class FsHandler { }) } - private async listFiles(params: Record): Promise { + private listFiles(params: Record, context?: RequestContext): Promise { const rootPath = expandTilde(params.rootPath as string) // Why: the main-to-relay RPC adds excludePaths so nested linked worktrees // don't get double-scanned. The shared helper validates the shape and // normalizes into root-relative prefixes; malformed input yields [] so // the request still succeeds (older apps omit the field entirely). const excludePathPrefixes = buildExcludePathPrefixes(rootPath, params.excludePaths) + // Why #7721: full-tree scans are the relay's most expensive request; the + // coordinator caps them at one per client, coalescing duplicates and + // aborting a stale scan when the workspace changes or the host cancels. + return this.listFilesScans.run({ + clientId: context?.clientId ?? 0, + key: JSON.stringify([rootPath, excludePathPrefixes]), + signal: context?.signal, + start: (signal) => this.runListFilesScan(rootPath, excludePathPrefixes, signal) + }) + } + + private async runListFilesScan( + rootPath: string, + excludePathPrefixes: string[], + signal: AbortSignal + ): Promise { const rgAvailable = await checkRgAvailable() + throwIfFileListingCancelled(signal) if (rgAvailable) { - return listFilesWithRg(rootPath, excludePathPrefixes) + return listFilesWithRg(rootPath, excludePathPrefixes, { signal }) } // Why: git ls-files only works inside git repos. Use rev-parse to detect // git ancestry — unlike checking for a local .git entry, this works from @@ -361,7 +384,7 @@ export class FsHandler { // budget errors into install-rg guidance; genuine git failures keep // their own messages. try { - return await listFilesWithGit(rootPath, excludePathPrefixes) + return await listFilesWithGit(rootPath, excludePathPrefixes, { signal }) } catch (err) { if (isQuickOpenReaddirBudgetError(err)) { throw new Error(await buildInstallRgMessage(err)) @@ -375,8 +398,13 @@ export class FsHandler { // problem, so translate the opaque cap error into actionable guidance // the user can act on directly from the error toast. try { - return await listFilesWithReaddir(rootPath, excludePathPrefixes) + return await listFilesWithReaddir(rootPath, excludePathPrefixes, { signal }) } catch (err) { + // Why: a cancelled scan is not an rg-availability problem; wrapping it + // in install-rg guidance would surface bogus advice on the client. + if (isFileListingCancellation(err)) { + throw err + } throw new Error(await buildInstallRgMessage(err)) } } diff --git a/src/relay/fs-list-files-cancel.integration.test.ts b/src/relay/fs-list-files-cancel.integration.test.ts new file mode 100644 index 00000000000..588ddb5fe51 --- /dev/null +++ b/src/relay/fs-list-files-cancel.integration.test.ts @@ -0,0 +1,209 @@ +/** + * End-to-end in-process regression tests for #7721: cancellable fs.listFiles. + * + * Wires the client-side SshChannelMultiplexer to the relay-side + * RelayDispatcher + FsHandler through an in-memory pipe (no SSH). The scan + * body is a controllable fake so the tests are deterministic: + * - aborting the client request sends rpc.cancel and stops the relay scan, + * - interactive fs.readDir is served while a scan is in flight, + * - a scan for a different workspace supersedes the previous one, + * - identical concurrent requests coalesce into one scan. + */ +import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest' +import { mkdtempSync, writeFileSync } from 'node:fs' +import { rm } from 'node:fs/promises' +import * as path from 'node:path' +import { tmpdir } from 'node:os' + +const { fakeListFiles } = vi.hoisted(() => { + type ScanRecord = { + rootPath: string + signal: AbortSignal | undefined + resolve: (files: string[]) => void + } + const scans: ScanRecord[] = [] + const fakeListFiles = Object.assign( + vi.fn( + ( + rootPath: string, + _excludes: readonly string[] = [], + options: { signal?: AbortSignal } = {} + ) => + new Promise((resolve, reject) => { + scans.push({ rootPath, signal: options.signal, resolve }) + options.signal?.addEventListener( + 'abort', + () => + // Mirror the real scanners: surface the abort reason (e.g. the + // "superseded" error) so the dispatcher reports it to the host. + reject( + options.signal?.reason instanceof Error + ? options.signal.reason + : new Error('File listing cancelled') + ), + { once: true } + ) + }) + ), + { scans } + ) + return { fakeListFiles } +}) + +vi.mock('./fs-handler-utils', async (importOriginal) => { + const original = (await importOriginal()) as Record + return { + ...original, + checkRgAvailable: () => Promise.resolve(true), + listFilesWithRg: fakeListFiles + } +}) + +import { + SshChannelMultiplexer, + type MultiplexerTransport +} from '../main/ssh/ssh-channel-multiplexer' +import { RelayDispatcher } from './dispatcher' +import { RelayContext } from './context' +import { FsHandler } from './fs-handler' +import { LIST_FILES_SUPERSEDED_MESSAGE } from './fs-list-files-scan-coordinator' + +async function flushPipe(): Promise { + // The in-memory pipe defers each hop with setImmediate; a few macrotask + // turns guarantee request/notification frames have crossed both directions. + for (let i = 0; i < 10; i++) { + await new Promise((resolve) => setImmediate(resolve)) + } +} + +describe('Integration: cancellable fs.listFiles (#7721)', () => { + let tmpDir: string + let mux: SshChannelMultiplexer + let dispatcher: RelayDispatcher + let fsHandler: FsHandler + + beforeEach(() => { + fakeListFiles.mockClear() + fakeListFiles.scans.length = 0 + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-listfiles-cancel-')) + + let relayFeedFn: (data: Buffer) => void + const clientDataCallbacks: ((data: Buffer) => void)[] = [] + const clientTransport: MultiplexerTransport = { + write: (data: Buffer) => { + setImmediate(() => relayFeedFn?.(data)) + }, + onData: (cb) => { + clientDataCallbacks.push(cb) + }, + onClose: () => {} + } + dispatcher = new RelayDispatcher((data: Buffer) => { + setImmediate(() => { + for (const cb of clientDataCallbacks) { + cb(data) + } + }) + }) + relayFeedFn = (data: Buffer) => dispatcher.feed(data) + fsHandler = new FsHandler(dispatcher, new RelayContext()) + mux = new SshChannelMultiplexer(clientTransport) + }) + + afterEach(async () => { + mux.dispose() + dispatcher.dispose() + fsHandler.dispose() + await rm(tmpDir, { recursive: true, force: true }) + }) + + it('serves fs.readDir while a full-tree scan is in flight', async () => { + writeFileSync(path.join(tmpDir, 'a.txt'), 'a') + + const scanPromise = mux.request('fs.listFiles', { rootPath: '/big/workspace' }) + await flushPipe() + expect(fakeListFiles.scans).toHaveLength(1) + + // The interactive request must complete while the scan is still pending. + const entries = (await mux.request('fs.readDir', { dirPath: tmpDir })) as { name: string }[] + expect(entries.map((e) => e.name)).toEqual(['a.txt']) + + fakeListFiles.scans[0].resolve(['src/index.ts']) + await expect(scanPromise).resolves.toEqual(['src/index.ts']) + }) + + it('client abort sends rpc.cancel and stops the relay-side scan', async () => { + const controller = new AbortController() + const scanPromise = mux.request( + 'fs.listFiles', + { rootPath: '/big/workspace' }, + { signal: controller.signal } + ) + await flushPipe() + expect(fakeListFiles.scans).toHaveLength(1) + expect(fakeListFiles.scans[0].signal?.aborted).toBe(false) + + controller.abort() + await expect(scanPromise).rejects.toThrow('was cancelled') + await flushPipe() + + // The rpc.cancel notification must have aborted the scan on the relay. + expect(fakeListFiles.scans[0].signal?.aborted).toBe(true) + }) + + it('a scan for a different workspace supersedes the previous scan', async () => { + const first = mux.request('fs.listFiles', { rootPath: '/workspace/a' }) + // Attach the rejection handler up front: the error response arrives on a + // macrotask inside flushPipe, before any later `await expect` could. + const firstOutcome = first.then( + () => null, + (err: Error) => err + ) + await flushPipe() + expect(fakeListFiles.scans).toHaveLength(1) + + const second = mux.request('fs.listFiles', { rootPath: '/workspace/b' }) + await flushPipe() + expect(fakeListFiles.scans).toHaveLength(2) + + // The stale scan is aborted and its request fails fast — it does not run + // to completion behind the new one and does not hit the 30s timeout. + expect(fakeListFiles.scans[0].signal?.aborted).toBe(true) + const firstError = await firstOutcome + expect(firstError?.message).toContain(LIST_FILES_SUPERSEDED_MESSAGE) + + fakeListFiles.scans[1].resolve(['b.ts']) + await expect(second).resolves.toEqual(['b.ts']) + }) + + it('identical concurrent requests coalesce into a single scan', async () => { + const first = mux.request('fs.listFiles', { rootPath: '/workspace/a' }) + const second = mux.request('fs.listFiles', { rootPath: '/workspace/a' }) + await flushPipe() + + expect(fakeListFiles.scans).toHaveLength(1) + fakeListFiles.scans[0].resolve(['shared.ts']) + await expect(first).resolves.toEqual(['shared.ts']) + await expect(second).resolves.toEqual(['shared.ts']) + }) + + it('cancelling one coalesced requester keeps the scan alive for the other', async () => { + const controller = new AbortController() + const first = mux.request( + 'fs.listFiles', + { rootPath: '/workspace/a' }, + { signal: controller.signal } + ) + const second = mux.request('fs.listFiles', { rootPath: '/workspace/a' }) + await flushPipe() + expect(fakeListFiles.scans).toHaveLength(1) + + controller.abort() + await expect(first).rejects.toThrow('was cancelled') + await flushPipe() + expect(fakeListFiles.scans[0].signal?.aborted).toBe(false) + + fakeListFiles.scans[0].resolve(['still.ts']) + await expect(second).resolves.toEqual(['still.ts']) + }) +}) diff --git a/src/relay/fs-list-files-scan-coordinator.test.ts b/src/relay/fs-list-files-scan-coordinator.test.ts new file mode 100644 index 00000000000..a142c120cdb --- /dev/null +++ b/src/relay/fs-list-files-scan-coordinator.test.ts @@ -0,0 +1,222 @@ +import { describe, expect, it, vi } from 'vitest' +import { + LIST_FILES_SUPERSEDED_MESSAGE, + ListFilesScanCoordinator +} from './fs-list-files-scan-coordinator' +import { + FileListingCancelledError, + isFileListingCancellation +} from '../shared/file-listing-cancellation' + +type Deferred = { + promise: Promise + resolve: (files: string[]) => void + reject: (error: Error) => void +} + +function deferred(): Deferred { + let resolve!: (files: string[]) => void + let reject!: (error: Error) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { promise, resolve, reject } +} + +/** Scan runner that resolves/rejects on demand and rejects when aborted. */ +function controllableScan(): { + start: (signal: AbortSignal) => Promise + starts: AbortSignal[] + finish: (files: string[]) => void +} { + const starts: AbortSignal[] = [] + let current: Deferred | null = null + return { + starts, + start: (signal: AbortSignal) => { + starts.push(signal) + const scan = deferred() + current = scan + signal.addEventListener( + 'abort', + () => { + scan.reject( + signal.reason instanceof Error ? signal.reason : new FileListingCancelledError() + ) + }, + { once: true } + ) + return scan.promise + }, + finish: (files: string[]) => current?.resolve(files) + } +} + +describe('ListFilesScanCoordinator', () => { + it('coalesces same-key concurrent requests into one scan', async () => { + const coordinator = new ListFilesScanCoordinator() + const scan = controllableScan() + + const first = coordinator.run({ clientId: 1, key: 'a', start: scan.start }) + const second = coordinator.run({ clientId: 1, key: 'a', start: scan.start }) + + expect(scan.starts).toHaveLength(1) + scan.finish(['x.ts']) + await expect(first).resolves.toEqual(['x.ts']) + await expect(second).resolves.toEqual(['x.ts']) + }) + + it('supersedes a different-key scan: aborts the old one and rejects it fast', async () => { + const coordinator = new ListFilesScanCoordinator() + const scan = controllableScan() + + const first = coordinator.run({ clientId: 1, key: 'workspace-a', start: scan.start }) + const second = coordinator.run({ clientId: 1, key: 'workspace-b', start: scan.start }) + + expect(scan.starts).toHaveLength(2) + expect(scan.starts[0].aborted).toBe(true) + await expect(first).rejects.toThrow(LIST_FILES_SUPERSEDED_MESSAGE) + await first.catch((err) => expect(isFileListingCancellation(err)).toBe(true)) + + expect(scan.starts[1].aborted).toBe(false) + scan.finish(['b.ts']) + await expect(second).resolves.toEqual(['b.ts']) + }) + + it('keeps scans from different clients independent', async () => { + const coordinator = new ListFilesScanCoordinator() + const scanA = controllableScan() + const scanB = controllableScan() + + const first = coordinator.run({ clientId: 1, key: 'workspace-a', start: scanA.start }) + const second = coordinator.run({ clientId: 2, key: 'workspace-b', start: scanB.start }) + + expect(scanA.starts[0].aborted).toBe(false) + scanA.finish(['a.ts']) + scanB.finish(['b.ts']) + await expect(first).resolves.toEqual(['a.ts']) + await expect(second).resolves.toEqual(['b.ts']) + }) + + it('aborts the scan when its only requester cancels', async () => { + const coordinator = new ListFilesScanCoordinator() + const scan = controllableScan() + const requester = new AbortController() + + const result = coordinator.run({ + clientId: 1, + key: 'a', + signal: requester.signal, + start: scan.start + }) + + requester.abort() + expect(scan.starts[0].aborted).toBe(true) + await expect(result).rejects.toSatisfy(isFileListingCancellation) + }) + + it('keeps a coalesced scan alive while another requester still waits', async () => { + const coordinator = new ListFilesScanCoordinator() + const scan = controllableScan() + const first = new AbortController() + const second = new AbortController() + + const firstResult = coordinator.run({ + clientId: 1, + key: 'a', + signal: first.signal, + start: scan.start + }) + const secondResult = coordinator.run({ + clientId: 1, + key: 'a', + signal: second.signal, + start: scan.start + }) + + first.abort() + expect(scan.starts[0].aborted).toBe(false) + // The aborting requester observes its own cancellation immediately, + // while the shared scan keeps running for the sibling. + await expect(firstResult).rejects.toSatisfy(isFileListingCancellation) + + second.abort() + expect(scan.starts[0].aborted).toBe(true) + await expect(secondResult).rejects.toSatisfy(isFileListingCancellation) + }) + + it('rejects an aborted coalesced requester while the sibling still resolves', async () => { + const coordinator = new ListFilesScanCoordinator() + const scan = controllableScan() + const first = new AbortController() + + const firstResult = coordinator.run({ + clientId: 1, + key: 'a', + signal: first.signal, + start: scan.start + }) + const secondResult = coordinator.run({ clientId: 1, key: 'a', start: scan.start }) + + first.abort() + scan.finish(['kept.ts']) + + await expect(firstResult).rejects.toSatisfy(isFileListingCancellation) + await expect(secondResult).resolves.toEqual(['kept.ts']) + }) + + it('rejects immediately when the requester is already cancelled', async () => { + const coordinator = new ListFilesScanCoordinator() + const start = vi.fn() + const requester = new AbortController() + requester.abort() + + await expect( + coordinator.run({ + clientId: 1, + key: 'a', + signal: requester.signal, + start: start as unknown as (signal: AbortSignal) => Promise + }) + ).rejects.toSatisfy(isFileListingCancellation) + expect(start).not.toHaveBeenCalled() + }) + + it('starts a fresh scan for the same key after the previous one settled', async () => { + const coordinator = new ListFilesScanCoordinator() + const scan = controllableScan() + + const first = coordinator.run({ clientId: 1, key: 'a', start: scan.start }) + scan.finish(['old.ts']) + await expect(first).resolves.toEqual(['old.ts']) + + const second = coordinator.run({ clientId: 1, key: 'a', start: scan.start }) + expect(scan.starts).toHaveLength(2) + scan.finish(['new.ts']) + await expect(second).resolves.toEqual(['new.ts']) + }) + + it('does not join a scan that is already aborted; starts a replacement', async () => { + const coordinator = new ListFilesScanCoordinator() + const scan = controllableScan() + const requester = new AbortController() + + const first = coordinator.run({ + clientId: 1, + key: 'a', + signal: requester.signal, + start: scan.start + }) + requester.abort() + await expect(first).rejects.toSatisfy(isFileListingCancellation) + + // The aborted entry may still be in the map until its promise settles; + // a new same-key request must get a live scan, not the dead one. + const second = coordinator.run({ clientId: 1, key: 'a', start: scan.start }) + expect(scan.starts).toHaveLength(2) + expect(scan.starts[1].aborted).toBe(false) + scan.finish(['fresh.ts']) + await expect(second).resolves.toEqual(['fresh.ts']) + }) +}) diff --git a/src/relay/fs-list-files-scan-coordinator.ts b/src/relay/fs-list-files-scan-coordinator.ts new file mode 100644 index 00000000000..6e6e30c5f0d --- /dev/null +++ b/src/relay/fs-list-files-scan-coordinator.ts @@ -0,0 +1,104 @@ +/** + * Single-flight coordinator for fs.listFiles full-tree scans (#7721). + * + * Why: rapid workspace switching used to stack N concurrent full-tree scans + * on the single-threaded relay and its one SSH channel, starving small + * interactive fs.readDir/fs.stat requests past their 30s timeout. This + * coordinator guarantees at most one scan in flight per client: + * - a request for the same root/excludes joins the in-flight scan + * (Quick Open + file-explorer filter share one scan), + * - a request for a different root supersedes it — the old scan is aborted + * and its request fails fast instead of running to completion (this also + * protects against older Orca clients that never send rpc.cancel), + * - when every joined requester cancels (rpc.cancel / client detach), the + * scan is aborted so abandoned work stops immediately. + */ +import { + FileListingCancelledError, + fileListingCancellationError +} from '../shared/file-listing-cancellation' + +export const LIST_FILES_SUPERSEDED_MESSAGE = 'File listing superseded by a newer request' + +type ScanEntry = { + key: string + controller: AbortController + promise: Promise + attachedCount: number +} + +export class ListFilesScanCoordinator { + private readonly scansByClient = new Map() + + run(opts: { + clientId: number + key: string + signal?: AbortSignal + start: (signal: AbortSignal) => Promise + }): Promise { + const { clientId, key, signal, start } = opts + if (signal?.aborted) { + return Promise.reject(fileListingCancellationError(signal)) + } + + const existing = this.scansByClient.get(clientId) + if (existing && existing.key === key && !existing.controller.signal.aborted) { + return this.attach(existing, signal) + } + if (existing) { + existing.controller.abort(new FileListingCancelledError(LIST_FILES_SUPERSEDED_MESSAGE)) + } + + const controller = new AbortController() + const entry: ScanEntry = { + key, + controller, + promise: Promise.resolve([]), + attachedCount: 0 + } + this.scansByClient.set(clientId, entry) + entry.promise = start(controller.signal).finally(() => { + if (this.scansByClient.get(clientId) === entry) { + this.scansByClient.delete(clientId) + } + }) + return this.attach(entry, signal) + } + + private attach(entry: ScanEntry, signal?: AbortSignal): Promise { + entry.attachedCount++ + if (!signal) { + return entry.promise + } + // Why: an aborting requester must see its own cancellation even though + // the shared scan keeps running (and may later resolve) for coalesced + // siblings — so each attachment gets its own promise. + return new Promise((resolve, reject) => { + let settled = false + const settle = (complete: () => void): void => { + if (settled) { + return + } + settled = true + signal.removeEventListener('abort', onAbort) + complete() + } + const onAbort = (): void => + settle(() => { + entry.attachedCount-- + const cancellation = fileListingCancellationError(signal) + // Why: only stop the shared scan when nobody is left waiting on it — + // one requester cancelling must not break a coalesced sibling. + if (entry.attachedCount <= 0) { + entry.controller.abort(cancellation) + } + reject(cancellation) + }) + signal.addEventListener('abort', onAbort, { once: true }) + entry.promise.then( + (files) => settle(() => resolve(files)), + (error) => settle(() => reject(error)) + ) + }) + } +} diff --git a/src/renderer/src/components/quick-open-file-list.react.test.tsx b/src/renderer/src/components/quick-open-file-list.react.test.tsx index ff76e430553..4a2305e06e3 100644 --- a/src/renderer/src/components/quick-open-file-list.react.test.tsx +++ b/src/renderer/src/components/quick-open-file-list.react.test.tsx @@ -10,9 +10,11 @@ import type { AppState } from '@/store/types' import { useRuntimeFileListForWorktree, type RuntimeFileListState } from './quick-open-file-list' const listRuntimeFilesMock = vi.hoisted(() => vi.fn()) +const cancelRuntimeFileListMock = vi.hoisted(() => vi.fn()) vi.mock('@/runtime/runtime-file-client', () => ({ - listRuntimeFiles: listRuntimeFilesMock + listRuntimeFiles: listRuntimeFilesMock, + cancelRuntimeFileList: cancelRuntimeFileListMock })) const initialAppState = useAppStore.getInitialState() @@ -104,6 +106,7 @@ async function renderProbe(args: { beforeEach(() => { useAppStore.setState(initialAppState, true) listRuntimeFilesMock.mockReset().mockResolvedValue(['packages/app/package.json']) + cancelRuntimeFileListMock.mockReset() }) afterEach(async () => { @@ -155,9 +158,37 @@ describe('useRuntimeFileListForWorktree', () => { }), { rootPath: '/srv/platform', - excludePaths: undefined + excludePaths: undefined, + requestToken: expect.any(String) } ) expect(states.at(-1)?.files).toEqual(['packages/app/package.json']) }) + + it('cancels the in-flight scan with the same request token on unmount (#7721)', async () => { + const workspaceKey = folderWorkspaceKey('folder-workspace-1') + + useAppStore.setState({ + folderWorkspaces: [makeFolderWorkspace({ connectionId: 'ssh-1' })], + projectGroups: [makeProjectGroup({ connectionId: 'ssh-1' })], + repos: [], + worktreesByRepo: {} + } as Partial) + + const root = await renderProbe({ + enabled: true, + onState: () => {}, + worktreeId: workspaceKey + }) + await waitForListRuntimeFilesCall() + + const [listContext, listRequest] = listRuntimeFilesMock.mock.calls[0] + expect(cancelRuntimeFileListMock).not.toHaveBeenCalled() + + await act(async () => { + root.unmount() + }) + + expect(cancelRuntimeFileListMock).toHaveBeenCalledWith(listContext, listRequest.requestToken) + }) }) diff --git a/src/renderer/src/components/quick-open-file-list.ts b/src/renderer/src/components/quick-open-file-list.ts index 9a7ffe8d596..13e3738e226 100644 --- a/src/renderer/src/components/quick-open-file-list.ts +++ b/src/renderer/src/components/quick-open-file-list.ts @@ -3,8 +3,9 @@ import { useEffect, useMemo, useRef, useState } from 'react' import type { Worktree } from '../../../shared/types' import { isWindowsAbsolutePathLike } from '../../../shared/cross-platform-path' import { getConnectionIdFromState } from '@/lib/connection-context' +import { createBrowserUuid } from '@/lib/browser-uuid' import { getSettingsForWorktreeRuntimeOwner } from '@/lib/worktree-runtime-owner' -import { listRuntimeFiles } from '@/runtime/runtime-file-client' +import { cancelRuntimeFileList, listRuntimeFiles } from '@/runtime/runtime-file-client' import { useAppStore } from '@/store' import { useWorktreesForRepo } from '@/store/selectors' @@ -152,21 +153,21 @@ export function useRuntimeFileListForWorktree({ setLoading(true) const excludePaths = excludeRequest.paths.length > 0 ? excludeRequest.paths : undefined + const requestToken = createBrowserUuid() + const requestContext = { + // Why: Quick Open lists files for the selected workspace. It must + // follow that workspace's owner host, not the globally focused host. + settings: getSettingsForWorktreeRuntimeOwner(useAppStore.getState(), worktreeId), + worktreeId, + worktreePath, + connectionId + } - void listRuntimeFiles( - { - // Why: Quick Open lists files for the selected workspace. It must - // follow that workspace's owner host, not the globally focused host. - settings: getSettingsForWorktreeRuntimeOwner(useAppStore.getState(), worktreeId), - worktreeId, - worktreePath, - connectionId - }, - { - rootPath: worktreePath, - excludePaths - } - ) + void listRuntimeFiles(requestContext, { + rootPath: worktreePath, + excludePaths, + requestToken + }) .then((result) => { if (!cancelled) { setFiles(result) @@ -186,6 +187,11 @@ export function useRuntimeFileListForWorktree({ return () => { cancelled = true + // Why #7721: switching workspaces (or closing the palette) must abort + // the previous full-tree scan host- and relay-side. Over SSH, abandoned + // scans otherwise stack up and starve fs.readDir/fs.stat past their + // 30s timeout ("Could not load files for this workspace"). + cancelRuntimeFileList(requestContext, requestToken) } }, [connectionId, enabled, excludeRequest, requestKey, target.canList, worktreeId, worktreePath]) diff --git a/src/renderer/src/runtime/runtime-file-client.test.ts b/src/renderer/src/runtime/runtime-file-client.test.ts index f1ce3f69e9c..90366acbfa4 100644 --- a/src/renderer/src/runtime/runtime-file-client.test.ts +++ b/src/renderer/src/runtime/runtime-file-client.test.ts @@ -3,6 +3,7 @@ preload API plus remote fallbacks; keeping route coverage together makes local versus environment behavior easy to audit. */ import { beforeEach, describe, expect, it, vi } from 'vitest' import { + cancelRuntimeFileList, copyRuntimePath, createRuntimePath, deleteRuntimePath, @@ -37,6 +38,8 @@ const fsDeletePath = vi.fn() const fsStat = vi.fn() const fsPathExists = vi.fn() const fsSearch = vi.fn() +const fsListFiles = vi.fn() +const fsCancelListFiles = vi.fn() const fsDownloadFile = vi.fn() const fsSaveDownloadedFile = vi.fn() const fsStartDownloadedFile = vi.fn() @@ -63,6 +66,9 @@ beforeEach(() => { fsStat.mockReset() fsPathExists.mockReset() fsSearch.mockReset() + fsListFiles.mockReset() + fsCancelListFiles.mockReset() + fsCancelListFiles.mockResolvedValue(undefined) fsDownloadFile.mockReset() fsSaveDownloadedFile.mockReset() fsStartDownloadedFile.mockReset() @@ -102,6 +108,8 @@ beforeEach(() => { stat: fsStat, pathExists: fsPathExists, search: fsSearch, + listFiles: fsListFiles, + cancelListFiles: fsCancelListFiles, downloadFile: fsDownloadFile, saveDownloadedFile: fsSaveDownloadedFile, startDownloadedFile: fsStartDownloadedFile, @@ -1357,6 +1365,56 @@ describe('runtime file client', () => { }) }) + it('passes the cancellation token through the IPC file listing path (#7721)', async () => { + fsListFiles.mockResolvedValue(['src/index.ts']) + + await expect( + listRuntimeFiles( + { + settings: {}, + worktreeId: 'wt-1', + worktreePath: '/remote/repo', + connectionId: 'ssh-1' + }, + { + rootPath: '/remote/repo', + requestToken: 'token-1' + } + ) + ).resolves.toEqual(['src/index.ts']) + + expect(fsListFiles).toHaveBeenCalledWith({ + rootPath: '/remote/repo', + connectionId: 'ssh-1', + excludePaths: undefined, + requestToken: 'token-1' + }) + }) + + it('cancelRuntimeFileList aborts the IPC listing but not environment listings (#7721)', () => { + cancelRuntimeFileList( + { + settings: {}, + worktreeId: 'wt-1', + worktreePath: '/remote/repo', + connectionId: 'ssh-1' + }, + 'token-1' + ) + expect(fsCancelListFiles).toHaveBeenCalledWith({ requestToken: 'token-1' }) + + fsCancelListFiles.mockClear() + cancelRuntimeFileList( + { + settings: { activeRuntimeEnvironmentId: 'env-1' }, + worktreeId: 'wt-1', + worktreePath: '/remote/repo' + }, + 'token-2' + ) + expect(fsCancelListFiles).not.toHaveBeenCalled() + }) + it('routes markdown document listing and stat through the selected runtime', async () => { runtimeEnvironmentCall.mockResolvedValue({ id: 'rpc-1', diff --git a/src/renderer/src/runtime/runtime-file-client.ts b/src/renderer/src/runtime/runtime-file-client.ts index e9161f754c1..eddda11a6c1 100644 --- a/src/renderer/src/runtime/runtime-file-client.ts +++ b/src/renderer/src/runtime/runtime-file-client.ts @@ -654,14 +654,15 @@ export async function searchRuntimeFiles( export async function listRuntimeFiles( context: RuntimeFileOperationArgs, - args: { rootPath: string; excludePaths?: string[] } + args: { rootPath: string; excludePaths?: string[]; requestToken?: string } ): Promise { const target = getActiveRuntimeTarget(context.settings) if (target.kind !== 'environment' || !context.worktreeId) { return window.api.fs.listFiles({ rootPath: args.rootPath, connectionId: context.connectionId, - excludePaths: args.excludePaths + excludePaths: args.excludePaths, + requestToken: args.requestToken }) } return callRuntimeRpc( @@ -675,6 +676,24 @@ export async function listRuntimeFiles( ) } +/** + * Best-effort abort of an in-flight listRuntimeFiles call (#7721). Switching + * workspaces must stop the previous workspace's full-tree scan — over SSH an + * abandoned scan keeps loading the relay and starves fs.readDir/fs.stat. + */ +export function cancelRuntimeFileList( + context: RuntimeFileOperationArgs, + requestToken: string +): void { + const target = getActiveRuntimeTarget(context.settings) + if (target.kind !== 'environment' || !context.worktreeId) { + void window.api.fs.cancelListFiles({ requestToken }).catch(() => { + /* cancellation is advisory; the request path has its own timeouts */ + }) + } + // Environment runtimes bound files.listAll with their own RPC timeout. +} + export async function listRuntimeMarkdownDocuments( context: RuntimeFileOperationArgs, rootPath: string diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index af231d8062c..7e181cdbb2b 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -1509,6 +1509,10 @@ function createFileApi(): NonNullable['fs']> { ) return result.files.map((entry) => entry.relativePath) }, + cancelListFiles: async () => { + // Why: the paired-web path lists files over runtime RPC with its own + // request timeout; there is no host-side scan to abort from here. + }, search: async (args) => { const file = await resolveRuntimeFilePath(args.rootPath) return callRuntimeResult('files.search', { diff --git a/src/shared/file-listing-cancellation.ts b/src/shared/file-listing-cancellation.ts new file mode 100644 index 00000000000..3296b80c925 --- /dev/null +++ b/src/shared/file-listing-cancellation.ts @@ -0,0 +1,38 @@ +/** + * Cancellation identity for workspace file-list scans (fs.listFiles). + * + * Why a dedicated error class: the readdir fallback wraps budget errors into + * "install rg" guidance and Quick Open surfaces load errors verbatim, so a + * cancelled scan must stay distinguishable from a genuine listing failure at + * every layer (relay rg/git/readdir, main-process local scan, renderer). + */ +export class FileListingCancelledError extends Error { + constructor(message = 'File listing cancelled') { + super(message) + this.name = 'FileListingCancelledError' + } +} + +/** + * Build the rejection for an aborted scan, preferring the abort reason so a + * superseded scan reports "superseded" rather than a generic cancellation. + * Only FileListingCancelledError reasons pass through — a bare abort() sets + * signal.reason to a DOMException that must not leak past the classifier. + */ +export function fileListingCancellationError(signal?: AbortSignal): Error { + const reason = signal?.reason + if (reason instanceof FileListingCancelledError) { + return reason + } + return new FileListingCancelledError() +} + +export function isFileListingCancellation(error: unknown): boolean { + return error instanceof FileListingCancelledError +} + +export function throwIfFileListingCancelled(signal?: AbortSignal): void { + if (signal?.aborted) { + throw fileListingCancellationError(signal) + } +} diff --git a/src/shared/quick-open-readdir-walk.test.ts b/src/shared/quick-open-readdir-walk.test.ts index 66029032a2c..cb73572bc34 100644 --- a/src/shared/quick-open-readdir-walk.test.ts +++ b/src/shared/quick-open-readdir-walk.test.ts @@ -24,6 +24,7 @@ import { listQuickOpenFilesWithReaddir, parseQuickOpenGitLsFilesEntry } from './quick-open-readdir-walk' +import { isFileListingCancellation } from './file-listing-cancellation' const tempDirs: string[] = [] const SHA1 = '0123456789abcdef0123456789abcdef01234567' @@ -259,4 +260,35 @@ describe('quick-open readdir walk', () => { }) ).resolves.toEqual(['packages/app [one] space/src/main.ts']) }) + + it('stops the walk with a cancellation error when the signal aborts (#7721)', async () => { + const root = await makeTempRoot() + await writeRel(root, 'src/a.ts') + await writeRel(root, 'src/b.ts') + + const controller = new AbortController() + controller.abort() + + const rejection = listQuickOpenFilesWithReaddir(root, { signal: controller.signal }) + await expect(rejection).rejects.toSatisfy(isFileListingCancellation) + // Cancellation must never be mistaken for a budget error, which callers + // translate into "install rg" guidance. + await rejection.catch((err) => expect(isQuickOpenReaddirBudgetError(err)).toBe(false)) + }) + + it('stops nested-repo expansion when the signal aborts (#7721)', async () => { + const root = await makeTempRoot() + await writeRel(root, 'src/kept.ts') + + const controller = new AbortController() + controller.abort() + + await expect( + expandQuickOpenGitFilesWithNestedRepos({ + rootPath: root, + gitPaths: ['src/kept.ts'], + signal: controller.signal + }) + ).rejects.toSatisfy(isFileListingCancellation) + }) }) diff --git a/src/shared/quick-open-readdir-walk.ts b/src/shared/quick-open-readdir-walk.ts index 5d3a0a52f53..90ce3ea9fdf 100644 --- a/src/shared/quick-open-readdir-walk.ts +++ b/src/shared/quick-open-readdir-walk.ts @@ -1,5 +1,6 @@ import { lstat, readdir } from 'node:fs/promises' import { join, relative } from 'node:path' +import { throwIfFileListingCancelled } from './file-listing-cancellation' import { HIDDEN_DIR_BLOCKLIST, shouldExcludeQuickOpenRelPath, @@ -157,6 +158,7 @@ export async function listQuickOpenFilesWithReaddir( opts: { excludePathPrefixes?: readonly string[] budget?: QuickOpenReaddirBudget + signal?: AbortSignal } = {} ): Promise { const files: string[] = [] @@ -164,6 +166,9 @@ export async function listQuickOpenFilesWithReaddir( const excludePathPrefixes = opts.excludePathPrefixes ?? [] async function walk(dir: string): Promise { + // Why: an abandoned scan (workspace switch) must stop consuming IO and + // event-loop time on the single-threaded relay, not just run to budget. + throwIfFileListingCancelled(opts.signal) assertWithinDeadline(budget) let entries @@ -176,6 +181,7 @@ export async function listQuickOpenFilesWithReaddir( } for (const entry of entries) { + throwIfFileListingCancelled(opts.signal) assertWithinDeadline(budget) const name = entry.name @@ -206,6 +212,7 @@ export async function expandQuickOpenGitFilesWithNestedRepos(opts: { gitPaths: Iterable excludePathPrefixes?: readonly string[] budget?: QuickOpenReaddirBudget + signal?: AbortSignal }): Promise { const files = new Set() const excludePathPrefixes = opts.excludePathPrefixes ?? [] @@ -224,6 +231,7 @@ export async function expandQuickOpenGitFilesWithNestedRepos(opts: { } for (const rawPath of opts.gitPaths) { + throwIfFileListingCancelled(opts.signal) assertWithinDeadline(budget) const { kind, relPath } = await classifyQuickOpenGitEntry(opts.rootPath, rawPath) @@ -240,7 +248,8 @@ export async function expandQuickOpenGitFilesWithNestedRepos(opts: { // nested repo so the walk prunes excluded subtrees during traversal // instead of burning the shared budget and filtering them at the end. excludePathPrefixes: rebaseExcludePrefixesForNestedRepo(excludePathPrefixes, relPath), - budget + budget, + signal: opts.signal }) for (const nestedFile of nestedFiles) { addFinalPath(`${relPath}/${nestedFile}`)