From ddd55a44d4298ae9c236b536a08013f66acc64a8 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 25 Oct 2026 16:33:35 -0700 Subject: [PATCH] Bound cwd permission probes during terminal session creation (#33643) * Bound cwd permission probes and share pending enumerations * Skip non-macOS cwd permission probes during session creation - Prevent stalled network paths from consuming filesystem workers --- .../daemon/daemon-folder-access-reset.test.ts | 9 +- .../daemon/directory-enumeration-probe.ts | 15 +- .../terminal-host-cwd-readability.test.ts | 205 +++++++++++++++++- .../daemon/terminal-host-session-create.ts | 27 ++- 4 files changed, 244 insertions(+), 12 deletions(-) diff --git a/src/main/daemon/daemon-folder-access-reset.test.ts b/src/main/daemon/daemon-folder-access-reset.test.ts index dfa1d4ce25c..db4085f6f07 100644 --- a/src/main/daemon/daemon-folder-access-reset.test.ts +++ b/src/main/daemon/daemon-folder-access-reset.test.ts @@ -163,7 +163,12 @@ describe('resetFolderAccessForDaemon runs the remedy', () => { cwdClass: 'documents', freshDaemonAccess: 'denied' } - opendirMock.mockReturnValue(new Promise(() => {})) + let answer: () => void = () => {} + opendirMock.mockReturnValue( + new Promise((resolve) => { + answer = () => resolve(fakeDir()) + }) + ) getMismatchMock.mockReturnValue(denied) vi.useFakeTimers() try { @@ -182,6 +187,8 @@ describe('resetFolderAccessForDaemon runs the remedy', () => { cwd_class: 'documents' }) } finally { + answer() + await vi.advanceTimersByTimeAsync(0) vi.useRealTimers() } }) diff --git a/src/main/daemon/directory-enumeration-probe.ts b/src/main/daemon/directory-enumeration-probe.ts index b3139d158c6..502a967e694 100644 --- a/src/main/daemon/directory-enumeration-probe.ts +++ b/src/main/daemon/directory-enumeration-probe.ts @@ -4,6 +4,8 @@ import { opendir } from 'node:fs/promises' /** `denied` is the only outcome that proves a permission refusal; `other` keeps unknown errors apart. */ export type DirectoryEnumerationOutcome = 'ok' | 'denied' | 'missing' | 'other' +const pendingEnumerations = new Map>() + function errorCode(error: unknown): string | undefined { if (typeof error !== 'object' || error === null || !('code' in error)) { return undefined @@ -21,7 +23,18 @@ function outcomeForError(error: unknown): DirectoryEnumerationOutcome { } /** Probe enumeration because TCC can reject opendir even when access() succeeds. */ -export async function enumerateDirectoryOnce(path: string): Promise { +export function enumerateDirectoryOnce(path: string): Promise { + const pending = pendingEnumerations.get(path) + if (pending) { + return pending + } + // Keep an uninterruptible probe shared until it settles, even if its callers stop waiting. + const enumeration = enumerateDirectory(path).finally(() => pendingEnumerations.delete(path)) + pendingEnumerations.set(path, enumeration) + return enumeration +} + +async function enumerateDirectory(path: string): Promise { let dir: Dir | undefined try { dir = await opendir(path) diff --git a/src/main/daemon/terminal-host-cwd-readability.test.ts b/src/main/daemon/terminal-host-cwd-readability.test.ts index ac9af6948e5..771e37e229e 100644 --- a/src/main/daemon/terminal-host-cwd-readability.test.ts +++ b/src/main/daemon/terminal-host-cwd-readability.test.ts @@ -1,17 +1,23 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { SubprocessHandle } from './session-subprocess-handle' -import { TerminalHost, type TerminalHostOptions } from './terminal-host' +import { TerminalHost } from './terminal-host' +import { validateWorkingDirectoryAsync } from '../providers/working-directory-validation' -const { opendirMock } = vi.hoisted(() => ({ opendirMock: vi.fn() })) +const { opendirMock, statMock } = vi.hoisted(() => ({ + opendirMock: vi.fn(), + statMock: vi.fn() +})) // TCC directory enumeration can remain pending while a permission sheet is open. vi.mock('node:fs/promises', async (importOriginal) => ({ ...(await importOriginal>()), - opendir: opendirMock + opendir: opendirMock, + stat: statMock })) vi.mock('../pty-descendant-termination', () => ({ killWithDescendantSweep: vi.fn() })) const close = vi.fn(async () => {}) +const spawnSubprocess = vi.fn(() => createMockSubprocess()) function dirReading(read: () => unknown): { read: () => unknown; close: () => Promise } { return { read, close } @@ -21,6 +27,33 @@ function failWith(code: string): never { throw Object.assign(new Error(code), { code }) } +function stallEnumeration(stage: 'open' | 'read' | 'close') { + let release!: () => void + const pending = new Promise((resolve) => { + release = resolve + }) + const directory = { + read: async () => { + if (stage === 'read') { + await pending + } + return null + }, + close: vi.fn(async () => { + if (stage === 'close') { + await pending + } + }) + } + opendirMock.mockImplementation(async () => { + if (stage === 'open') { + await pending + } + return directory + }) + return { release, directory } +} + function createMockSubprocess(): SubprocessHandle { let onExitCb: ((code: number) => void) | null = null return { @@ -49,26 +82,29 @@ describe('TerminalHost cwd readability verdict', () => { beforeEach(() => { close.mockReset() + spawnSubprocess.mockClear() + statMock.mockReset() opendirMock.mockReset().mockReturnValue(dirReading(() => ({ name: 'entry' }))) platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') - Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) - const spawnSubprocess: TerminalHostOptions['spawnSubprocess'] = () => createMockSubprocess() + Object.defineProperty(process, 'platform', { configurable: true, value: 'darwin' }) host = new TerminalHost({ spawnSubprocess }) }) afterEach(async () => { + vi.useRealTimers() await host.dispose() if (platformDescriptor) { Object.defineProperty(process, 'platform', platformDescriptor) } }) - const create = (sessionId: string, cwd?: string) => + const create = (sessionId: string, cwd?: string, signal?: AbortSignal) => host.createOrAttach({ sessionId, cols: 80, rows: 24, ...(cwd ? { cwd } : {}), + ...(signal ? { cancelSignal: signal, isCanceled: () => signal.aborted } : {}), streamClient: { onData: vi.fn(), onExit: vi.fn() } }) @@ -116,10 +152,167 @@ describe('TerminalHost cwd readability verdict', () => { expect(opendirMock).not.toHaveBeenCalled() }) + it.each(['linux', 'win32'])('skips TCC enumeration on %s', async (platform) => { + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + expect((await create('non-macos', '/work/repo')).cwdReadableByDaemon).toBeUndefined() + expect(opendirMock).not.toHaveBeenCalled() + }) + + it('keeps native UNC creation within the validation lane while healthy creation proceeds', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + vi.useFakeTimers() + let release = () => {} + const unavailable = new Promise((resolve) => { + release = resolve + }) + opendirMock.mockImplementation(async () => { + await unavailable + return dirReading(() => null) + }) + const server = '\\\\unreachable-smb\\share' + let active = 0 + let peak = 0 + statMock.mockImplementation(async (cwd: string) => { + if (cwd.startsWith(server)) { + active += 1 + peak = Math.max(peak, active) + try { + await unavailable + } finally { + active -= 1 + } + } + return { isDirectory: () => true } + }) + await host.dispose() + host = new TerminalHost({ + spawnSubprocess: async (opts) => { + if (opts.cwd) { + await validateWorkingDirectoryAsync(opts.cwd, { signal: opts.cancelSignal }) + } + return createMockSubprocess() + } + }) + const abort = new AbortController() + const firstPath = `${server}\\first` + const canceled = create('unc-canceled', firstPath, abort.signal) + void canceled.catch(() => {}) + const pending = [ + create('unc-second', `${server}\\second`), + create('unc-queued', `${server}\\third`) + ] + try { + await vi.advanceTimersByTimeAsync(1_100) + expect(active + opendirMock.mock.calls.length).toBe(2) + abort.abort() + await expect(canceled).rejects.toThrow('was canceled') + pending.push(create('unc-canceled', firstPath)) + await vi.advanceTimersByTimeAsync(1_100) + expect(statMock).toHaveBeenCalledTimes(2) + expect((await create('healthy', 'C:\\healthy')).isNew).toBe(true) + expect(active).toBe(2) + } finally { + release() + await vi.advanceTimersByTimeAsync(0) + await Promise.allSettled([canceled, ...pending]) + } + expect(peak).toBe(2) + expect(host.listSessions()).toHaveLength(4) + expect(statMock).toHaveBeenCalledTimes(4) + }) + it('omits the verdict on attach to an existing session', async () => { await create('attach', '/work/repo') const attached = await create('attach', '/work/repo') expect(attached.isNew).toBe(false) expect(attached.cwdReadableByDaemon).toBeUndefined() }) + + it.each(['open', 'read', 'close'] as const)( + 'starts a shell when the diagnostic directory %s remains pending', + async (stage) => { + vi.useFakeTimers() + const { release, directory } = stallEnumeration(stage) + const creation = create(`slow-${stage}`, `/work/slow-${stage}`) + try { + await vi.advanceTimersByTimeAsync(1_000) + expect(host.listSessions()).toHaveLength(1) + await expect(creation).resolves.toMatchObject({ isNew: true }) + expect((await creation).cwdReadableByDaemon).toBeUndefined() + } finally { + release() + await vi.advanceTimersByTimeAsync(0) + } + expect(directory.close).toHaveBeenCalledOnce() + } + ) + + it('reuses a pending directory probe across timeouts and probes again after it settles', async () => { + vi.useFakeTimers() + const { release } = stallEnumeration('open') + try { + const first = create('first-prompt', '/work/prompt') + await vi.advanceTimersByTimeAsync(1_000) + expect(host.listSessions()).toHaveLength(1) + await first + const second = create('second-prompt', '/work/prompt') + await vi.advanceTimersByTimeAsync(1_000) + expect(host.listSessions()).toHaveLength(2) + await second + expect(opendirMock).toHaveBeenCalledOnce() + } finally { + release() + await vi.advanceTimersByTimeAsync(0) + } + expect((await create('after-prompt', '/work/prompt')).cwdReadableByDaemon).toBe(true) + expect(opendirMock).toHaveBeenCalledTimes(2) + }) + + it('cancels a pending probe without spawning and releases the session id for a retry', async () => { + vi.useFakeTimers() + const { release, directory } = stallEnumeration('open') + const abort = new AbortController() + const creation = create('canceled-prompt', '/work/canceled-prompt', abort.signal) + let error: unknown + void creation.catch((rejection: unknown) => { + error = rejection + }) + try { + abort.abort() + await vi.advanceTimersByTimeAsync(0) + expect(spawnSubprocess).not.toHaveBeenCalled() + expect(host.listSessions()).toEqual([]) + expect(error).toMatchObject({ message: 'Attach canceled for session canceled-prompt' }) + const retry = create('canceled-prompt', '/work/canceled-prompt') + await vi.advanceTimersByTimeAsync(1_000) + expect(host.listSessions()).toHaveLength(1) + await expect(retry).resolves.toMatchObject({ isNew: true }) + expect(opendirMock).toHaveBeenCalledOnce() + } finally { + release() + await vi.advanceTimersByTimeAsync(0) + } + expect(directory.close).toHaveBeenCalledOnce() + }) + + it('does not launch a shell when shutdown starts during a pending probe', async () => { + vi.useFakeTimers() + const { release } = stallEnumeration('open') + const creation = create('shutdown-prompt', '/work/shutdown-prompt') + let error: unknown + void creation.catch((rejection: unknown) => { + error = rejection + }) + const disposal = host.dispose() + try { + await vi.advanceTimersByTimeAsync(1_000) + expect(spawnSubprocess).not.toHaveBeenCalled() + expect(error).toMatchObject({ message: 'Terminal host is shutting down' }) + await disposal + expect(host.listSessions()).toEqual([]) + } finally { + release() + await vi.advanceTimersByTimeAsync(0) + } + }) }) diff --git a/src/main/daemon/terminal-host-session-create.ts b/src/main/daemon/terminal-host-session-create.ts index 2dcf9fda60f..89a0e65a07e 100644 --- a/src/main/daemon/terminal-host-session-create.ts +++ b/src/main/daemon/terminal-host-session-create.ts @@ -21,6 +21,7 @@ import type { LegacyRestoreDetails } from './legacy-restore-records' import { resolveDaemonSessionScrollbackRows } from './daemon-session-scrollback-window' import { TerminalAttachCanceledError } from './daemon-errors' import { waitForTerminalAttachOperation } from './terminal-attach-cancellation' +import { sharedPromiseSettlementWaiters } from '../../shared/promise-settlement-waiters' import { SessionNotFoundError } from './types' import { resolveWslSessionContext } from './wsl-session-context' import { recordSessionSpawnPaneKey } from './terminal-host-session-listing' @@ -145,9 +146,12 @@ async function spawnAndPublishSession( onPublished: (sessionId: string, isAlive: boolean) => void ): Promise { const { size, wslDistro } = ctx - // Probe the requested cwd before the shell can fall back to another path. + // Only macOS needs the TCC probe; other hosts must not pin extra filesystem workers. const cwdReadableByDaemon = - opts.cwd && !wslDistro ? await isCwdReadableByThisProcess(opts.cwd) : null + process.platform === 'darwin' && opts.cwd && !wslDistro + ? await isCwdReadableByThisProcess(opts.cwd, opts) + : null + deps.assertCreateAllowed() const subprocess = await deps.spawnSubprocess({ sessionId: opts.sessionId, cols: size.cols, @@ -310,6 +314,21 @@ function describeAgentRestore( } // TCC may reject directory enumeration despite successful access(); only explicit permission denial counts. -async function isCwdReadableByThisProcess(cwd: string): Promise { - return (await enumerateDirectoryOnce(cwd)) !== 'denied' +async function isCwdReadableByThisProcess( + cwd: string, + opts: Pick +): Promise { + const outcome = await sharedPromiseSettlementWaiters(enumerateDirectoryOnce(cwd)) + .wait({ + timeoutMs: 1_000, + signal: opts.cancelSignal, + createAbortError: () => new TerminalAttachCanceledError(opts.sessionId) + }) + .catch((error: unknown) => { + if (error instanceof TerminalAttachCanceledError) { + throw error + } + return null + }) + return outcome === null ? null : outcome !== 'denied' }