diff --git a/.gitignore b/.gitignore index e6f62909c97..6f3978e7060 100644 --- a/.gitignore +++ b/.gitignore @@ -99,6 +99,7 @@ docs/** !docs/reference/git-compatibility.md !docs/reference/headless-linux-server.md !docs/reference/linux-glibc-compatibility.md +!docs/reference/relay-grace-time-reconfiguration.md # Stably CLI (only docs/ are tracked) .stably/* diff --git a/src/main/ipc/filesystem-watcher-large-batch.test.ts b/src/main/ipc/filesystem-watcher-large-batch.test.ts index 01f7ccc90cc..80d45c90e43 100644 --- a/src/main/ipc/filesystem-watcher-large-batch.test.ts +++ b/src/main/ipc/filesystem-watcher-large-batch.test.ts @@ -32,6 +32,11 @@ import { closeAllWatchers, registerFilesystemWatcherHandlers } from './filesyste import { stat } from 'node:fs/promises' import { subscribe as subscribeParcelWatcher } from '@parcel/watcher' import type { Event as WatcherEvent } from '@parcel/watcher' +import type { FsChangedPayload } from '../../shared/types' +import { + WATCH_BATCH_MAX_WAIT_MS, + WATCH_BATCH_TRAILING_MS +} from '../../shared/filesystem-watch-batch-window' type HandlerMap = Record Promise | unknown> @@ -111,4 +116,61 @@ describe('local filesystem watcher large batches', () => { await closeAllWatchers() vi.useRealTimers() }) + + it('still coalesces a local burst on the shared trailing window', async () => { + vi.useFakeTimers() + vi.mocked(stat).mockResolvedValue({ isDirectory: () => true } as never) + let watcherCallback: ((err: Error | null, events: WatcherEvent[]) => void) | undefined + vi.mocked(subscribeParcelWatcher).mockImplementation(async (_root, callback) => { + watcherCallback = callback as typeof watcherCallback + return { unsubscribe: vi.fn() } as never + }) + const worktreePath = resolve('/tmp/repo') + const filePath = join(worktreePath, 'a.ts') + const sender = { isDestroyed: () => false, send: vi.fn(), once: vi.fn(), id: 1 } + + await handlers['fs:watchWorktree']({ sender }, { worktreePath }) + watcherCallback?.(null, [{ type: 'update', path: filePath }]) + watcherCallback?.(null, [{ type: 'update', path: filePath }]) + + await vi.advanceTimersByTimeAsync(WATCH_BATCH_TRAILING_MS - 1) + expect(sender.send).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(1) + expect(sender.send).toHaveBeenCalledTimes(1) + expect(sender.send).toHaveBeenCalledWith('fs:changed', { + worktreePath, + events: [{ kind: 'update', absolutePath: filePath, isDirectory: true }] + }) + await closeAllWatchers() + vi.useRealTimers() + }) + + it('still flushes a sustained local stream at the shared max wait', async () => { + vi.useFakeTimers() + vi.mocked(stat).mockResolvedValue({ isDirectory: () => true } as never) + let watcherCallback: ((err: Error | null, events: WatcherEvent[]) => void) | undefined + vi.mocked(subscribeParcelWatcher).mockImplementation(async (_root, callback) => { + watcherCallback = callback as typeof watcherCallback + return { unsubscribe: vi.fn() } as never + }) + const worktreePath = resolve('/tmp/repo') + const sender = { isDestroyed: () => false, send: vi.fn(), once: vi.fn(), id: 1 } + + await handlers['fs:watchWorktree']({ sender }, { worktreePath }) + // Step under the trailing window so only the max wait can force a flush. + const step = 100 + for (let elapsed = 0; elapsed <= WATCH_BATCH_MAX_WAIT_MS; elapsed += step) { + watcherCallback?.(null, [{ type: 'update', path: join(worktreePath, `f-${elapsed}.ts`) }]) + if (elapsed < WATCH_BATCH_MAX_WAIT_MS) { + expect(sender.send).not.toHaveBeenCalled() + } + await vi.advanceTimersByTimeAsync(step) + } + + expect(sender.send).toHaveBeenCalledTimes(1) + expect((sender.send.mock.calls[0][1] as FsChangedPayload).events).toHaveLength(6) + await closeAllWatchers() + vi.useRealTimers() + }) }) diff --git a/src/main/ipc/filesystem-watcher-remote-batch.test.ts b/src/main/ipc/filesystem-watcher-remote-batch.test.ts new file mode 100644 index 00000000000..6d964c2b2d4 --- /dev/null +++ b/src/main/ipc/filesystem-watcher-remote-batch.test.ts @@ -0,0 +1,191 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { FsChangeEvent } from '../../shared/types' + +const { handleMock, getSshFilesystemProviderMock, providerRegistrationListeners } = vi.hoisted( + () => ({ + handleMock: vi.fn(), + getSshFilesystemProviderMock: vi.fn(), + providerRegistrationListeners: new Set<(connectionId: string) => void>() + }) +) + +vi.mock('electron', () => ({ ipcMain: { handle: handleMock } })) +vi.mock('fs/promises', () => ({ stat: vi.fn() })) +vi.mock('@parcel/watcher', () => ({ subscribe: vi.fn() })) +vi.mock('./filesystem-watcher-wsl', () => ({ createWslWatcher: vi.fn() })) +vi.mock('../providers/ssh-filesystem-dispatch', () => ({ + getSshFilesystemProvider: getSshFilesystemProviderMock, + onSshFilesystemProviderRegistered: (listener: (connectionId: string) => void) => { + providerRegistrationListeners.add(listener) + return () => providerRegistrationListeners.delete(listener) + } +})) + +import { closeAllWatchers, registerFilesystemWatcherHandlers } from './filesystem-watcher' + +type HandlerMap = Record Promise | unknown> +type WatchCallback = (events: FsChangeEvent[]) => void + +const WORKTREE_PATH = '/home/me/repo' +const WATCH_ARGS = { worktreePath: WORKTREE_PATH, connectionId: 'conn-1' } + +describe('remote filesystem watcher batching', () => { + const handlers: HandlerMap = {} + const watchCallbacks: WatchCallback[] = [] + + function makeSender(overrides: Partial<{ isDestroyed: () => boolean }> = {}) { + return { isDestroyed: () => false, send: vi.fn(), once: vi.fn(), id: 1, ...overrides } + } + + beforeEach(async () => { + vi.useRealTimers() + handleMock.mockReset() + getSshFilesystemProviderMock.mockReset() + watchCallbacks.length = 0 + for (const key of Object.keys(handlers)) { + delete handlers[key] + } + handleMock.mockImplementation((channel, handler) => { + handlers[channel] = handler + }) + registerFilesystemWatcherHandlers() + await closeAllWatchers() + getSshFilesystemProviderMock.mockReturnValue({ + watch: vi.fn(async (_root: string, callback: WatchCallback) => { + watchCallbacks.push(callback) + return vi.fn() + }) + }) + }) + + it('coalesces a burst of remote events into a single fs:changed send', async () => { + vi.useFakeTimers() + const sender = makeSender() + await handlers['fs:watchWorktree']({ sender }, WATCH_ARGS) + + for (let i = 0; i < 50; i++) { + watchCallbacks[0]([{ kind: 'update', absolutePath: `${WORKTREE_PATH}/a.ts` }]) + } + expect(sender.send).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(150) + + expect(sender.send).toHaveBeenCalledTimes(1) + expect(sender.send).toHaveBeenCalledWith('fs:changed', { + worktreePath: WORKTREE_PATH, + events: [{ kind: 'update', absolutePath: `${WORKTREE_PATH}/a.ts` }] + }) + await closeAllWatchers() + vi.useRealTimers() + }) + + // An atomic replace (delete+create) followed by an rm inside one window must still tombstone the path. + it('sends the net delete when a remote replace is removed in the same window', async () => { + vi.useFakeTimers() + const sender = makeSender() + await handlers['fs:watchWorktree']({ sender }, WATCH_ARGS) + + watchCallbacks[0]([ + { kind: 'delete', absolutePath: `${WORKTREE_PATH}/dist/app.js` }, + { kind: 'create', absolutePath: `${WORKTREE_PATH}/dist/app.js` } + ]) + watchCallbacks[0]([{ kind: 'delete', absolutePath: `${WORKTREE_PATH}/dist/app.js` }]) + await vi.advanceTimersByTimeAsync(150) + + expect(sender.send).toHaveBeenCalledTimes(1) + expect(sender.send).toHaveBeenCalledWith('fs:changed', { + worktreePath: WORKTREE_PATH, + events: [{ kind: 'delete', absolutePath: `${WORKTREE_PATH}/dist/app.js` }] + }) + await closeAllWatchers() + vi.useRealTimers() + }) + + it('collapses an oversized remote burst into one overflow payload', async () => { + vi.useFakeTimers() + const sender = makeSender() + await handlers['fs:watchWorktree']({ sender }, WATCH_ARGS) + + watchCallbacks[0]( + Array.from( + { length: 6_000 }, + (_unused, index): FsChangeEvent => ({ + kind: 'update', + absolutePath: `${WORKTREE_PATH}/file-${index}.ts` + }) + ) + ) + await vi.advanceTimersByTimeAsync(150) + + expect(sender.send).toHaveBeenCalledTimes(1) + expect(sender.send).toHaveBeenCalledWith('fs:changed', { + worktreePath: WORKTREE_PATH, + events: [{ kind: 'overflow', absolutePath: WORKTREE_PATH }] + }) + await closeAllWatchers() + vi.useRealTimers() + }) + + it('sends nothing when the worktree is unwatched before the flush deadline', async () => { + vi.useFakeTimers() + const sender = makeSender() + await handlers['fs:watchWorktree']({ sender }, WATCH_ARGS) + + watchCallbacks[0]([{ kind: 'update', absolutePath: `${WORKTREE_PATH}/a.ts` }]) + await handlers['fs:unwatchWorktree']({ sender }, WATCH_ARGS) + await vi.advanceTimersByTimeAsync(1_000) + + expect(sender.send).not.toHaveBeenCalled() + await closeAllWatchers() + vi.useRealTimers() + }) + + it('sends nothing when all watchers close before the flush deadline', async () => { + vi.useFakeTimers() + const sender = makeSender() + await handlers['fs:watchWorktree']({ sender }, WATCH_ARGS) + + watchCallbacks[0]([{ kind: 'update', absolutePath: `${WORKTREE_PATH}/a.ts` }]) + await closeAllWatchers() + await vi.advanceTimersByTimeAsync(1_000) + + expect(sender.send).not.toHaveBeenCalled() + vi.useRealTimers() + }) + + it('skips a WebContents destroyed while the batch was buffering', async () => { + vi.useFakeTimers() + let destroyed = false + const sender = makeSender({ isDestroyed: () => destroyed }) + await handlers['fs:watchWorktree']({ sender }, WATCH_ARGS) + + watchCallbacks[0]([{ kind: 'update', absolutePath: `${WORKTREE_PATH}/a.ts` }]) + destroyed = true + await vi.advanceTimersByTimeAsync(1_000) + + expect(sender.send).not.toHaveBeenCalled() + await closeAllWatchers() + vi.useRealTimers() + }) + + it('drops a stale flush once a newer install owns the key', async () => { + vi.useFakeTimers() + const sender = makeSender() + await handlers['fs:watchWorktree']({ sender }, WATCH_ARGS) + + for (const listener of providerRegistrationListeners) { + listener('conn-1') + } + await vi.advanceTimersByTimeAsync(1_000) + expect(watchCallbacks).toHaveLength(2) + sender.send.mockClear() + + // The replaced transport's callback still holds its own batch; its flush must not reach the renderer. + watchCallbacks[0]([{ kind: 'update', absolutePath: `${WORKTREE_PATH}/a.ts` }]) + await vi.advanceTimersByTimeAsync(1_000) + + expect(sender.send).not.toHaveBeenCalled() + await closeAllWatchers() + vi.useRealTimers() + }) +}) diff --git a/src/main/ipc/filesystem-watcher-remote-cancellation.test.ts b/src/main/ipc/filesystem-watcher-remote-cancellation.test.ts index f4af15588be..6a67f3a8fb7 100644 --- a/src/main/ipc/filesystem-watcher-remote-cancellation.test.ts +++ b/src/main/ipc/filesystem-watcher-remote-cancellation.test.ts @@ -113,7 +113,8 @@ describe('remote filesystem watcher cancellation', () => { await Promise.all([first, second]) expect(watchMock).toHaveBeenCalledTimes(2) secondCallback?.([{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }]) - expect(secondSender.send).toHaveBeenCalledTimes(1) + // Why: remote fs:changed rides the shared debounce window, so the send lands a flush later. + await vi.waitFor(() => expect(secondSender.send).toHaveBeenCalledTimes(1)) handlers['fs:unwatchWorktree']({ sender: { id: 2 } }, args) expect(secondUnwatch).toHaveBeenCalledTimes(1) diff --git a/src/main/ipc/filesystem-watcher.test.ts b/src/main/ipc/filesystem-watcher.test.ts index b2f83baaf22..1e9e3cf509a 100644 --- a/src/main/ipc/filesystem-watcher.test.ts +++ b/src/main/ipc/filesystem-watcher.test.ts @@ -58,9 +58,18 @@ import { WatcherChildCapacityError } from './parcel-watcher-child-registry' import { acquireWatcherRemovalGate } from './watcher-removal-gate' +import { WATCH_BATCH_TRAILING_MS } from '../../shared/filesystem-watch-batch-window' type HandlerMap = Record Promise | unknown> +/** Remote fs:changed rides the shared debounce window, so drain it before asserting sends. */ +const emitRemote = async (onEvents: (e: unknown[]) => void, events: unknown[]) => { + onEvents(events) + await (vi.isFakeTimers() + ? vi.advanceTimersByTimeAsync(WATCH_BATCH_TRAILING_MS) + : new Promise((resolve) => setTimeout(resolve, WATCH_BATCH_TRAILING_MS + 25))) +} + describe('registerFilesystemWatcherHandlers', () => { const handlers: HandlerMap = {} const originalPlatform = process.platform @@ -234,7 +243,7 @@ describe('registerFilesystemWatcherHandlers', () => { onTerminalError: expect.any(Function) }) const onEvents = watchMock.mock.calls[0][1] - onEvents([{ path: '/home/me/repo/file.txt', type: 'update' }]) + await emitRemote(onEvents, [{ path: '/home/me/repo/file.txt', type: 'update' }]) expect(sendMock).toHaveBeenCalledWith('fs:changed', { worktreePath: '/home/me/repo', events: [{ path: '/home/me/repo/file.txt', type: 'update' }] @@ -329,7 +338,7 @@ describe('registerFilesystemWatcherHandlers', () => { expect(watchMock).toHaveBeenCalledTimes(2) const recoveredEvents = watchMock.mock.calls[1][1] as (events: unknown[]) => void - recoveredEvents([{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }]) + await emitRemote(recoveredEvents, [{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }]) expect(senderOne.send).not.toHaveBeenCalled() expect(senderTwo.send).toHaveBeenCalledWith('fs:changed', { worktreePath: '/home/me/repo', @@ -369,7 +378,7 @@ describe('registerFilesystemWatcherHandlers', () => { ) const reinstalledEvents = watchMock.mock.calls[1][1] as (events: unknown[]) => void - reinstalledEvents([{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }]) + await emitRemote(reinstalledEvents, [{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }]) expect(sender.send).toHaveBeenCalledWith('fs:changed', { worktreePath: '/home/me/repo', events: [{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }] @@ -552,7 +561,7 @@ describe('registerFilesystemWatcherHandlers', () => { expect(watchMock).toHaveBeenCalledTimes(1) const onEvents = watchMock.mock.calls[0][1] - onEvents([{ path: '/home/me/repo/file.txt', type: 'update' }]) + await emitRemote(onEvents, [{ path: '/home/me/repo/file.txt', type: 'update' }]) expect(sendOne).toHaveBeenCalledTimes(1) expect(sendTwo).toHaveBeenCalledTimes(1) @@ -621,7 +630,7 @@ describe('registerFilesystemWatcherHandlers', () => { events: [{ kind: 'overflow', absolutePath: '/home/me/repo' }] }) const replacementEvents = watchMock.mock.calls[1][1] as (events: unknown[]) => void - replacementEvents([{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }]) + await emitRemote(replacementEvents, [{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }]) expect(sender.send).toHaveBeenLastCalledWith('fs:changed', { worktreePath: '/home/me/repo', events: [{ kind: 'update', absolutePath: '/home/me/repo/file.ts' }] @@ -716,7 +725,7 @@ describe('registerFilesystemWatcherHandlers', () => { 'still watched by another client' ) const onEvents = watchMock.mock.calls[0][1] - onEvents([{ path: '/home/me/repo/file.txt', type: 'update' }]) + await emitRemote(onEvents, [{ path: '/home/me/repo/file.txt', type: 'update' }]) expect(sendOne).toHaveBeenCalledTimes(1) await handlers['fs:watchWorktree']( @@ -724,7 +733,7 @@ describe('registerFilesystemWatcherHandlers', () => { { worktreePath: '/home/me/repo', connectionId: 'conn-1' } ) expect(watchMock).toHaveBeenCalledTimes(1) - onEvents([{ path: '/home/me/repo/file-2.txt', type: 'update' }]) + await emitRemote(onEvents, [{ path: '/home/me/repo/file-2.txt', type: 'update' }]) expect(sendOne).toHaveBeenCalledTimes(2) expect(sendTwo).toHaveBeenCalledTimes(1) @@ -763,7 +772,7 @@ describe('registerFilesystemWatcherHandlers', () => { await Promise.all([firstWatch, secondWatch]) const onEvents = watchMock.mock.calls[0][1] - onEvents([{ path: '/home/me/repo/file.txt', type: 'update' }]) + await emitRemote(onEvents, [{ path: '/home/me/repo/file.txt', type: 'update' }]) expect(sendOne).toHaveBeenCalledTimes(1) expect(sendTwo).toHaveBeenCalledTimes(1) @@ -809,7 +818,7 @@ describe('registerFilesystemWatcherHandlers', () => { await Promise.all([firstWatch, secondWatch]) const onEvents = watchMock.mock.calls[0][1] - onEvents([{ path: '/home/me/repo/file.txt', type: 'update' }]) + await emitRemote(onEvents, [{ path: '/home/me/repo/file.txt', type: 'update' }]) expect(sendOne).toHaveBeenCalledTimes(1) expect(sendTwo).not.toHaveBeenCalled() @@ -855,7 +864,7 @@ describe('registerFilesystemWatcherHandlers', () => { expect(unwatchMock).toHaveBeenCalledTimes(1) const onEvents = watchMock.mock.calls[0][1] - onEvents([{ path: '/home/me/repo/file.txt', type: 'update' }]) + await emitRemote(onEvents, [{ path: '/home/me/repo/file.txt', type: 'update' }]) expect(sender.send).not.toHaveBeenCalled() }) @@ -886,7 +895,7 @@ describe('registerFilesystemWatcherHandlers', () => { await Promise.all([firstWatch, secondWatch]) const onEvents = watchMock.mock.calls[0][1] - onEvents([{ path: '/home/me/repo/file.txt', type: 'update' }]) + await emitRemote(onEvents, [{ path: '/home/me/repo/file.txt', type: 'update' }]) expect(senderOne.send).not.toHaveBeenCalled() expect(senderTwo.send).toHaveBeenCalledTimes(1) diff --git a/src/main/ipc/filesystem-watcher.ts b/src/main/ipc/filesystem-watcher.ts index 58401722494..571f6c46b3a 100644 --- a/src/main/ipc/filesystem-watcher.ts +++ b/src/main/ipc/filesystem-watcher.ts @@ -16,6 +16,14 @@ import { onSshFilesystemProviderRegistered } from '../providers/ssh-filesystem-dispatch' import { MAX_BATCHED_WATCHER_EVENTS, queueWatcherEvents } from './filesystem-watcher-event-batch' +import { + createRemoteWatcherEventBatch, + type RemoteWatcherEventBatch +} from './remote-watcher-event-batch' +import { + WATCH_BATCH_MAX_WAIT_MS, + WATCH_BATCH_TRAILING_MS +} from '../../shared/filesystem-watch-batch-window' import { disposeWatcherProcess, subscribeViaWatcherProcess } from './parcel-watcher-process' import { isWatcherProcessFailure } from './parcel-watcher-process-failure' import { @@ -32,11 +40,6 @@ import { // Why: suppress high-churn dirs at the watcher level (separate from the File Explorer display filter, which only hides rows). import { WATCHER_IGNORE_DIRS, buildParcelWatcherIgnoreOptions } from './filesystem-watcher-ignore' -// ── Debounce helpers ───────────────────────────────────────────────── - -const DEBOUNCE_TRAILING_MS = 150 -const DEBOUNCE_MAX_WAIT_MS = 500 - // ── Per-root watcher state ─────────────────────────────────────────── // WatchedRoot/WatcherSubscription live in filesystem-watcher-wsl.ts so native and WSL watchers share one shape. @@ -349,7 +352,7 @@ function scheduleBatchFlush(root: WatchedRoot): void { } // If we've exceeded the max wait, flush immediately - if (now - root.batch.firstEventAt >= DEBOUNCE_MAX_WAIT_MS) { + if (now - root.batch.firstEventAt >= WATCH_BATCH_MAX_WAIT_MS) { if (root.batch.timer) { clearTimeout(root.batch.timer) } @@ -361,7 +364,7 @@ function scheduleBatchFlush(root: WatchedRoot): void { if (root.batch.timer) { clearTimeout(root.batch.timer) } - root.batch.timer = setTimeout(() => void flushBatch(root), DEBOUNCE_TRAILING_MS) + root.batch.timer = setTimeout(() => void flushBatch(root), WATCH_BATCH_TRAILING_MS) } // ── Watcher creation ───────────────────────────────────────────────── @@ -944,6 +947,8 @@ type RemoteWatcherState = { unwatch: () => void listeners: Map installToken: RemoteWatcherInstallToken + /** Held so every teardown can drop the trailing flush timer instead of letting it fire into a dead watch. */ + batch: RemoteWatcherEventBatch } type RemoteWatcherInstallToken = { @@ -1049,6 +1054,7 @@ export async function closeRemoteWatcherForWorktreePath( await (provider?.closeWatch ? provider.closeWatch(worktreePath) : Promise.resolve(state?.unwatch())) + state?.batch.close() remoteWatchers.delete(key) loggedUnavailableRemoteWatchers.delete(key) } @@ -1161,6 +1167,7 @@ function releaseRemoteWatchListener(key: string, senderId: number): void { return } state.unwatch() + state.batch.close() remoteWatchers.delete(key) } @@ -1287,23 +1294,47 @@ async function doInstallRemoteWatcher( cancelToken: RemoteWatcherInstallToken ): Promise { let unwatch: () => void - try { - unwatch = await provider.watch( - worktreePath, - (events) => { - const state = remoteWatchers.get(key) - if (!state) { - return + // Why: the abandon paths below close the batch directly; once it is stored on the remoteWatchers + // entry, every later teardown closes it through that handle. deliver's state/installToken guard + // still covers a flush that races a close. + const batch = createRemoteWatcherEventBatch({ + rootPath: worktreePath, + trailingMs: WATCH_BATCH_TRAILING_MS, + maxWaitMs: WATCH_BATCH_MAX_WAIT_MS, + maxEvents: MAX_BATCHED_WATCHER_EVENTS, + deliver: (events) => { + const state = remoteWatchers.get(key) + // Why: buffering defers delivery past install, so a bare non-null check would let a stale flush + // land on a different install generation. + if (!state || state.installToken !== cancelToken) { + return + } + for (const listener of state.listeners.values()) { + if (listener.isDestroyed()) { + continue } - for (const listener of state.listeners.values()) { - if (listener.isDestroyed()) { - continue - } + try { listener.send('fs:changed', { worktreePath, events } satisfies FsChangedPayload) + } catch (err) { + // Why: batching moved this send out of the SSH mux's notification try/catch into a bare + // timer, so a frame disposed mid-window would escape as a fatal main-process exception. + console.warn( + `[filesystem-watch] failed to deliver remote fs:changed for ${worktreePath}: ${ + err instanceof Error ? err.message : String(err) + }` + ) } + } + } + }) + try { + unwatch = await provider.watch( + worktreePath, + (events) => { + batch.push(events) }, { signal: cancelToken.abortController.signal, @@ -1312,6 +1343,9 @@ async function doInstallRemoteWatcher( } ) } catch (err) { + // Why: the provider can fire its callback before rejecting (an aborted reinstall), so this + // abandon path owes the same close as the two below — nothing else holds the batch. + batch.close() if (cancelToken.cancelled || cancelToken.abortController.signal.aborted) { return 'cancelled' } @@ -1326,6 +1360,7 @@ async function doInstallRemoteWatcher( Array.from(cancelToken.listeners.entries()).filter(([, listener]) => !listener.isDestroyed()) ) if (cancelToken.cancelled || liveListeners.size === 0) { + batch.close() try { unwatch() } catch (err) { @@ -1334,9 +1369,10 @@ async function doInstallRemoteWatcher( return 'cancelled' } if (cancelToken.terminalError) { + batch.close() return 'unavailable' } - remoteWatchers.set(key, { unwatch, listeners: liveListeners, installToken: cancelToken }) + remoteWatchers.set(key, { unwatch, listeners: liveListeners, installToken: cancelToken, batch }) for (const listener of liveListeners.values()) { registerSenderCleanup(listener) } @@ -1357,6 +1393,7 @@ function handleRemoteWatcherTerminalError( return } remoteWatchers.delete(key) + state.batch.close() if (remoteWatchersClosed || suspendedRemoteWatcherListeners.has(key)) { return } @@ -1779,6 +1816,7 @@ function reinstallRemoteWatchersForConnection(connectionId: string): void { const stale = remoteWatchers.get(key) if (stale) { remoteWatchers.delete(key) + stale.batch.close() try { stale.unwatch() } catch { @@ -1912,6 +1950,7 @@ export async function closeAllWatchers(): Promise { // Why: remote watchers are separate from local @parcel/watcher subs; unwatch here or the relay keeps polling FS after shutdown. for (const [key, state] of remoteWatchers) { + state.batch.close() try { state.unwatch() } catch (err) { diff --git a/src/main/ipc/remote-watcher-event-batch.test.ts b/src/main/ipc/remote-watcher-event-batch.test.ts new file mode 100644 index 00000000000..b659c446aed --- /dev/null +++ b/src/main/ipc/remote-watcher-event-batch.test.ts @@ -0,0 +1,270 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { FsChangeEvent } from '../../shared/types' +import { createRemoteWatcherEventBatch } from './remote-watcher-event-batch' + +const ROOT = '/home/u/repo' + +function makeBatch(deliver: (events: FsChangeEvent[]) => void, maxEvents = 5_000) { + return createRemoteWatcherEventBatch({ + rootPath: ROOT, + deliver, + trailingMs: 150, + maxWaitMs: 500, + maxEvents + }) +} + +describe('createRemoteWatcherEventBatch', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('collapses many pushes inside the trailing window into one delivery', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + for (let i = 0; i < 50; i++) { + batch.push([{ kind: 'update', absolutePath: `${ROOT}/file-${i}.ts` }]) + vi.advanceTimersByTime(1) + } + expect(deliver).not.toHaveBeenCalled() + + vi.advanceTimersByTime(150) + expect(deliver).toHaveBeenCalledTimes(1) + expect(deliver.mock.calls[0][0]).toHaveLength(50) + }) + + it('flushes at the max wait when pushes never stop', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + for (let i = 0; i < 10; i++) { + batch.push([{ kind: 'update', absolutePath: `${ROOT}/a.ts` }]) + vi.advanceTimersByTime(100) + } + + expect(deliver).toHaveBeenCalled() + expect(deliver.mock.calls[0][0]).toEqual([{ kind: 'update', absolutePath: `${ROOT}/a.ts` }]) + }) + + it('emits both events for delete then create, delete first', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([ + { kind: 'delete', absolutePath: `${ROOT}/a.ts` }, + { kind: 'create', absolutePath: `${ROOT}/a.ts` } + ]) + vi.advanceTimersByTime(150) + + expect(deliver.mock.calls[0][0]).toEqual([ + { kind: 'delete', absolutePath: `${ROOT}/a.ts` }, + { kind: 'create', absolutePath: `${ROOT}/a.ts` } + ]) + }) + + it('drops both events for create then delete', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([ + { kind: 'create', absolutePath: `${ROOT}/tmp.ts` }, + { kind: 'delete', absolutePath: `${ROOT}/tmp.ts` } + ]) + vi.advanceTimersByTime(150) + + expect(deliver).not.toHaveBeenCalled() + }) + + it('clears a stale delete when a later update is known to be a file', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([ + { kind: 'delete', absolutePath: `${ROOT}/a.ts` }, + { kind: 'create', absolutePath: `${ROOT}/a.ts` }, + { kind: 'update', absolutePath: `${ROOT}/a.ts`, isDirectory: false } + ]) + vi.advanceTimersByTime(150) + + expect(deliver.mock.calls[0][0]).toEqual([ + { kind: 'update', absolutePath: `${ROOT}/a.ts`, isDirectory: false } + ]) + }) + + it('keeps the delete when a later update has unknown isDirectory', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([ + { kind: 'delete', absolutePath: `${ROOT}/dist` }, + { kind: 'create', absolutePath: `${ROOT}/dist` }, + { kind: 'update', absolutePath: `${ROOT}/dist` } + ]) + vi.advanceTimersByTime(150) + + expect(deliver.mock.calls[0][0]).toEqual([ + { kind: 'delete', absolutePath: `${ROOT}/dist` }, + { kind: 'update', absolutePath: `${ROOT}/dist` } + ]) + }) + + it('emits the net delete for delete then create then delete', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([ + { kind: 'delete', absolutePath: `${ROOT}/dist/app.js` }, + { kind: 'create', absolutePath: `${ROOT}/dist/app.js` }, + { kind: 'delete', absolutePath: `${ROOT}/dist/app.js` } + ]) + vi.advanceTimersByTime(150) + + expect(deliver).toHaveBeenCalledTimes(1) + expect(deliver.mock.calls[0][0]).toEqual([ + { kind: 'delete', absolutePath: `${ROOT}/dist/app.js` } + ]) + }) + + it('re-hoists the delete when the window ends on a recreate', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([ + { kind: 'delete', absolutePath: `${ROOT}/a.ts` }, + { kind: 'create', absolutePath: `${ROOT}/a.ts` }, + { kind: 'delete', absolutePath: `${ROOT}/a.ts` }, + { kind: 'create', absolutePath: `${ROOT}/a.ts` } + ]) + vi.advanceTimersByTime(150) + + expect(deliver.mock.calls[0][0]).toEqual([ + { kind: 'delete', absolutePath: `${ROOT}/a.ts` }, + { kind: 'create', absolutePath: `${ROOT}/a.ts` } + ]) + }) + + it('keeps only the last update for one path', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([{ kind: 'update', absolutePath: `${ROOT}/a.ts`, isDirectory: false }]) + batch.push([{ kind: 'update', absolutePath: `${ROOT}/a.ts`, isDirectory: false }]) + vi.advanceTimersByTime(150) + + expect(deliver).toHaveBeenCalledTimes(1) + expect(deliver.mock.calls[0][0]).toEqual([ + { kind: 'update', absolutePath: `${ROOT}/a.ts`, isDirectory: false } + ]) + }) + + it('discards the buffer and delivers one overflow when an overflow event arrives', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([{ kind: 'update', absolutePath: `${ROOT}/a.ts` }]) + batch.push([{ kind: 'overflow', absolutePath: ROOT }]) + batch.push([{ kind: 'update', absolutePath: `${ROOT}/b.ts` }]) + vi.advanceTimersByTime(150) + + expect(deliver).toHaveBeenCalledTimes(1) + expect(deliver.mock.calls[0][0]).toEqual([{ kind: 'overflow', absolutePath: ROOT }]) + }) + + it('latches overflow once the accumulated batch exceeds maxEvents', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver, 100) + + for (let i = 0; i < 3; i++) { + batch.push( + Array.from( + { length: 50 }, + (_unused, index): FsChangeEvent => ({ + kind: 'update', + absolutePath: `${ROOT}/wave-${i}-${index}.ts` + }) + ) + ) + } + vi.advanceTimersByTime(150) + + expect(deliver).toHaveBeenCalledTimes(1) + expect(deliver.mock.calls[0][0]).toEqual([{ kind: 'overflow', absolutePath: ROOT }]) + }) + + it('never invents isDirectory for events that arrive without it', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([{ kind: 'create', absolutePath: `${ROOT}/dir` }]) + vi.advanceTimersByTime(150) + + const [delivered] = deliver.mock.calls[0][0] as FsChangeEvent[] + expect(delivered.isDirectory).toBeUndefined() + expect('isDirectory' in delivered).toBe(false) + }) + + it('passes an unknown kind through verbatim after the coalesced set', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([ + { kind: 'rename', absolutePath: `${ROOT}/new.ts`, oldAbsolutePath: `${ROOT}/old.ts` }, + { kind: 'update', absolutePath: `${ROOT}/a.ts` } + ]) + vi.advanceTimersByTime(150) + + expect(deliver.mock.calls[0][0]).toEqual([ + { kind: 'update', absolutePath: `${ROOT}/a.ts` }, + { kind: 'rename', absolutePath: `${ROOT}/new.ts`, oldAbsolutePath: `${ROOT}/old.ts` } + ]) + }) + + it('delivers nothing after close', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([{ kind: 'update', absolutePath: `${ROOT}/a.ts` }]) + batch.close() + vi.advanceTimersByTime(1_000) + + expect(deliver).not.toHaveBeenCalled() + }) + + it('ignores a push that lands after close instead of arming a timer', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.close() + batch.push([{ kind: 'update', absolutePath: `${ROOT}/a.ts` }]) + + expect(vi.getTimerCount()).toBe(0) + vi.advanceTimersByTime(1_000) + expect(deliver).not.toHaveBeenCalled() + }) + + // Why: the host platform is irrelevant — the coalesce key must stay shape-based, so a case-sensitive + // remote pair must survive it on any host, and neither path may pick up drive/backslash rewriting. + it('keeps case-distinct remote POSIX paths separate and byte-identical', () => { + const deliver = vi.fn() + const batch = makeBatch(deliver) + + batch.push([{ kind: 'update', absolutePath: '/home/u/A' }]) + batch.push([{ kind: 'update', absolutePath: '/home/u/a' }]) + vi.advanceTimersByTime(150) + + const delivered = deliver.mock.calls[0][0] as FsChangeEvent[] + expect(delivered).toEqual([ + { kind: 'update', absolutePath: '/home/u/A' }, + { kind: 'update', absolutePath: '/home/u/a' } + ]) + for (const event of delivered) { + expect(event.absolutePath).not.toContain('\\') + expect(event.absolutePath).not.toMatch(/^[a-zA-Z]:/) + } + }) +}) diff --git a/src/main/ipc/remote-watcher-event-batch.ts b/src/main/ipc/remote-watcher-event-batch.ts new file mode 100644 index 00000000000..86f10e81d96 --- /dev/null +++ b/src/main/ipc/remote-watcher-event-batch.ts @@ -0,0 +1,147 @@ +import type { FsChangeEvent } from '../../shared/types' +import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-path' + +export type RemoteWatcherEventBatchOptions = { + rootPath: string + deliver: (events: FsChangeEvent[]) => void + trailingMs: number + maxWaitMs: number + maxEvents: number +} + +export type RemoteWatcherEventBatch = { + push: (events: FsChangeEvent[]) => void + close: () => void +} + +// Mirrors coalesceEvents in filesystem-watcher.ts, with two deliberate divergences: a hoisted delete +// survives a later event of unknown isDirectory, so delete→create→update over-emits a delete; and +// delete→create→delete emits that delete, which local drops — the path is gone, so the renderer needs it. +function coalesceRemoteEvents(raw: FsChangeEvent[]): FsChangeEvent[] { + const lastByKey = new Map() + const deleteBeforeCreate = new Map() + const passthrough: FsChangeEvent[] = [] + + for (const event of raw) { + if (event.kind !== 'create' && event.kind !== 'update' && event.kind !== 'delete') { + // Why: 'rename' has no producer on this transport; forward it untouched rather than invent a rule. + passthrough.push(event) + continue + } + // Why: key on the comparison form, never node path.resolve — resolving a remote POSIX path on a + // Windows host mints C:\home\u\f, which the renderer's watch canonicalizer then rejects outright. + // Residual cost: its NFC fold merges canonically equivalent byte-distinct names on a POSIX host. + const key = normalizeRuntimePathForComparison(event.absolutePath) + const prev = lastByKey.get(key) + + if (prev) { + if (prev.kind === 'delete' && event.kind === 'create') { + deleteBeforeCreate.set(key, prev) + } + if (prev.kind === 'create' && event.kind === 'delete') { + // Why: cancel only a path created and removed entirely inside the window; an earlier delete means + // the file predates it, so the window nets out to a delete the renderer still has to act on. + const netNoOp = !deleteBeforeCreate.delete(key) + if (netNoOp) { + lastByKey.delete(key) + continue + } + } + } + + lastByKey.set(key, event) + + // Why: drop the hoisted delete only when the later event proves the path is a live file, or repeats the + // delete. Remote events carry no isDirectory, and discarding it there strands a dead dir's cached subtree. + if (event.kind === 'delete' || (event.kind === 'update' && event.isDirectory === false)) { + deleteBeforeCreate.delete(key) + } + } + + return [...deleteBeforeCreate.values(), ...lastByKey.values(), ...passthrough] +} + +export function createRemoteWatcherEventBatch({ + rootPath, + deliver, + trailingMs, + maxWaitMs, + maxEvents +}: RemoteWatcherEventBatchOptions): RemoteWatcherEventBatch { + let buffered: FsChangeEvent[] = [] + let overflowed = false + let firstEventAt = 0 + let timer: ReturnType | null = null + let closed = false + + function flush(): void { + if (timer) { + clearTimeout(timer) + timer = null + } + const pending = buffered + const latched = overflowed + buffered = [] + overflowed = false + firstEventAt = 0 + + if (latched) { + deliver([{ kind: 'overflow', absolutePath: rootPath }]) + return + } + const coalesced = coalesceRemoteEvents(pending) + if (coalesced.length > 0) { + deliver(coalesced) + } + } + + function schedule(): void { + const now = Date.now() + if (firstEventAt === 0) { + firstEventAt = now + } + if (now - firstEventAt >= maxWaitMs) { + flush() + return + } + if (timer) { + clearTimeout(timer) + } + timer = setTimeout(flush, trailingMs) + } + + return { + push(events) { + // Why: an in-flight provider receive can land after teardown; re-arming there would strand a timer. + if (closed) { + return + } + if (!overflowed) { + if ( + buffered.length + events.length > maxEvents || + events.some((event) => event.kind === 'overflow') + ) { + // Why: past the cap precision only burns memory; one overflow buys the same conservative refresh. + buffered = [] + overflowed = true + } else { + // Why: a deletion storm can exceed V8's argument limit for push(...events). + for (const event of events) { + buffered.push(event) + } + } + } + schedule() + }, + close() { + closed = true + if (timer) { + clearTimeout(timer) + timer = null + } + buffered = [] + overflowed = false + firstEventAt = 0 + } + } +} diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 13534a7d560..e89bf7fb214 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -32411,6 +32411,8 @@ describe('OrcaRuntimeService', () => { ] }) + // No renderer graph on purpose: headless rename attribution must work from + // the persisted session alone. const { worktrees } = await runtime.getWorktreePs() const oldSummary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) const renamedSummary = worktrees.find((worktree) => worktree.worktreeId === renamedWorktreeId) @@ -32502,16 +32504,498 @@ describe('OrcaRuntimeService', () => { } ] }) + // Why: local rows only project while their tab exists (#6072); freshness + // is what varies here, so keep the tab present in the runtime graph. + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { + tabs: [ + { + tabId: 'tab-1', + worktreeId: TEST_WORKTREE_ID, + title: 'Codex', + activeLeafId: '33333333-3333-4333-8333-333333333333', + layout: null + } + ], + leaves: [] + }) const { worktrees } = await runtime.getWorktreePs() - expect(worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID)).toMatchObject({ - hasHostSidebarActivity, - status - }) + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + expect(summary).toMatchObject({ hasHostSidebarActivity, status }) + // Why: inactive must mean "projected but not fresh", never "row dropped". + expect(summary?.agents).toHaveLength(1) } ) + it('drops a hydrated done hook row after its local tab is closed', async () => { + // Why (#6072): last-status.json hydrates hook rows for days; a closed tab's + // agent must not resurface on mobile as current worktree activity. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: {} + }) + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey: 'closed-tab:66666666-6666-4666-8666-666666666666', + worktreeId: TEST_WORKTREE_ID, + tabId: 'closed-tab', + state: 'done', + prompt: 'refactor the parser', + agentType: 'claude', + connectionId: null, + receivedAt: now, + stateStartedAt: now - 60_000 + } + ] + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary).toMatchObject({ + liveTerminalCount: 0, + hasHostSidebarActivity: false, + status: 'inactive', + agents: [] + }) + }) + + it('keeps a hydrated row while its persisted session tab exists and no renderer graph is attached', async () => { + // Why: headless serve has no renderer graph; session.tabs.list serves this + // tab to mobile as current, so its agent row must stay (desktop parity). + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + [TEST_WORKTREE_ID]: [ + { + id: 'headless-tab', + ptyId: null, + worktreeId: TEST_WORKTREE_ID, + title: 'Codex', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ] + } + }) + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey: 'headless-tab:aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa', + worktreeId: TEST_WORKTREE_ID, + tabId: 'headless-tab', + state: 'done', + prompt: 'finished while headless', + agentType: 'codex', + connectionId: null, + receivedAt: now, + stateStartedAt: now - 60_000 + } + ] + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([ + expect.objectContaining({ state: 'done', prompt: 'finished while headless' }) + ]) + }) + + it('resolves legacy numeric pane keys through the stale filter too', async () => { + // Why: non-UUID leaves produce `tabId:paneRuntimeId` keys with no tabId + // field; they still name a real tab and must not bypass the filter. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + [TEST_WORKTREE_ID]: [ + { + id: 'open-tab', + ptyId: null, + worktreeId: TEST_WORKTREE_ID, + title: 'Codex', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ] + } + }) + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey: 'closed-tab:7', + worktreeId: TEST_WORKTREE_ID, + state: 'done', + prompt: 'stale legacy pane', + agentType: 'codex', + connectionId: null, + receivedAt: now, + stateStartedAt: now - 60_000 + }, + { + paneKey: 'open-tab:9', + worktreeId: TEST_WORKTREE_ID, + state: 'working', + prompt: 'live legacy pane', + agentType: 'codex', + connectionId: null, + receivedAt: now, + stateStartedAt: now - 100 + } + ] + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([ + expect.objectContaining({ paneKey: 'open-tab:9', prompt: 'live legacy pane' }) + ]) + }) + + it('keeps a local hook row while a connected PTY still backs its pane', async () => { + // Why: daemon-held terminals stay live across renderer graph gaps even when + // no session tab records them; a connected PTY is proof the pane exists. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: {} + }) + const paneKey = 'daemon-tab:77777777-7777-4777-8777-777777777777' + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey, + worktreeId: TEST_WORKTREE_ID, + tabId: 'daemon-tab', + state: 'working', + prompt: 'long-running daemon agent', + agentType: 'codex', + connectionId: null, + receivedAt: now, + stateStartedAt: now - 100 + } + ] + }) + // paneKey-only record: the tabId rescue must not be what keeps this row. + runtime['recordPtyWorktree']('daemon-pty', TEST_WORKTREE_ID, { + connected: true, + paneKey + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([ + expect.objectContaining({ paneKey, state: 'working', prompt: 'long-running daemon agent' }) + ]) + expect(summary).toMatchObject({ hasHostSidebarActivity: true, status: 'working' }) + }) + + it('keeps a local hook row when a connected PTY matches only its tab id', async () => { + // Split-pane sibling: the PTY's paneKey names another leaf of the same tab, + // so only the tabId conjunct can rescue this row. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: {} + }) + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey: 'daemon-tab:88888888-8888-4888-8888-888888888887', + worktreeId: TEST_WORKTREE_ID, + tabId: 'daemon-tab', + state: 'working', + prompt: 'sibling pane agent', + agentType: 'codex', + connectionId: null, + receivedAt: now, + stateStartedAt: now - 100 + } + ] + }) + runtime['recordPtyWorktree']('daemon-pty-2', TEST_WORKTREE_ID, { + connected: true, + tabId: 'daemon-tab', + paneKey: 'daemon-tab:99999999-9999-4999-8999-999999999998' + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([ + expect.objectContaining({ prompt: 'sibling pane agent', state: 'working' }) + ]) + }) + + it('keeps a retained OSC row via its connected PTY after the pane binding is cleared', async () => { + // A controller incarnation change nulls pty.tabId/paneKey while the PTY + // stays connected (adoptControllerTerminalHandle); the ptyId conjunct is + // then the only rescue for the retained OSC row. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: {} + }) + const runtime = new OrcaRuntimeService(runtimeStore as never) + runtime['recordPtyWorktree']('osc-pty', TEST_WORKTREE_ID, { + connected: true, + tabId: 'osc-tab', + paneKey: 'osc-tab:bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb' + }) + runtime.onPtyData( + 'osc-pty', + '\x1b]9999;{"state":"working","prompt":"osc reporter","agentType":"codex"}\x07', + 1 + ) + const pty = runtime['ptysById'].get('osc-pty')! + pty.tabId = null + pty.paneKey = null + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([expect.objectContaining({ prompt: 'osc reporter' })]) + }) + + it('keeps the connected-PTY rescue when a hook row outraces the OSC row for the same pane', async () => { + // Hook payloads carry no ptyId; the OSC-observed one must survive the + // hook row winning the freshness race or the ptyId rescue goes dead. + const paneKey = 'race-tab:dddddddd-dddd-4ddd-8ddd-dddddddddddd' + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: {} + }) + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey, + worktreeId: TEST_WORKTREE_ID, + tabId: 'race-tab', + state: 'working', + prompt: 'hook-fresh agent', + agentType: 'codex', + connectionId: null, + receivedAt: Date.now() + 60_000, + stateStartedAt: Date.now() - 100 + } + ] + }) + runtime['recordPtyWorktree']('race-pty', TEST_WORKTREE_ID, { + connected: true, + tabId: 'race-tab', + paneKey + }) + runtime.onPtyData( + 'race-pty', + '\x1b]9999;{"state":"working","prompt":"osc ping","agentType":"codex"}\x07', + 1 + ) + const pty = runtime['ptysById'].get('race-pty')! + pty.tabId = null + pty.paneKey = null + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([expect.objectContaining({ prompt: 'hook-fresh agent' })]) + }) + + it('keeps a retained OSC row from an SSH pane after its PTY disconnects', async () => { + // Why: OSC snapshots must carry the pane transport; hardcoding local would + // strip the SSH exemption off rows whose freshest update arrived via OSC. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: {} + }) + const runtime = new OrcaRuntimeService(runtimeStore as never) + runtime['recordPtyWorktree']('ssh-osc-pty', TEST_WORKTREE_ID, { + connected: true, + connectionId: 'ssh-osc-1', + tabId: 'ssh-tab', + paneKey: 'ssh-tab:cccccccc-cccc-4ccc-8ccc-cccccccccccc' + }) + runtime.onPtyData( + 'ssh-osc-pty', + '\x1b]9999;{"state":"working","prompt":"remote osc agent","agentType":"codex"}\x07', + 1 + ) + runtime['recordPtyWorktree']('ssh-osc-pty', TEST_WORKTREE_ID, { connected: false }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([expect.objectContaining({ prompt: 'remote osc agent' })]) + }) + + it('keeps a hook row with an unresolvable pane key', async () => { + // Why: no tabId means staleness is unprovable; the filter must pass it. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: {} + }) + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey: 'opaque-key-without-structure', + worktreeId: TEST_WORKTREE_ID, + state: 'working', + prompt: 'unattributable pane', + agentType: 'codex', + connectionId: null, + receivedAt: now, + stateStartedAt: now - 100 + } + ] + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([expect.objectContaining({ prompt: 'unattributable pane' })]) + }) + + it('keeps a WSL hook row while its tab is still in the session', async () => { + // Guards the WSL clause against over-filtering: local-tab evidence must + // rescue WSL rows exactly like null-connection rows. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + [TEST_WORKTREE_ID]: [ + { + id: 'open-wsl-tab', + ptyId: null, + worktreeId: TEST_WORKTREE_ID, + title: 'Codex', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ] + } + }) + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey: 'open-wsl-tab:99999999-9999-4999-8999-999999999997', + worktreeId: TEST_WORKTREE_ID, + tabId: 'open-wsl-tab', + state: 'working', + prompt: 'live in WSL', + agentType: 'codex', + connectionId: 'wsl:Ubuntu', + receivedAt: now, + stateStartedAt: now - 100 + } + ] + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([expect.objectContaining({ prompt: 'live in WSL' })]) + }) + + it('drops a hydrated WSL hook row after its local tab is closed', async () => { + // Why: WSL relay ids are transport provenance; the pane remains local. + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: {} + }) + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey: 'closed-wsl-tab:99999999-9999-4999-8999-999999999999', + worktreeId: TEST_WORKTREE_ID, + tabId: 'closed-wsl-tab', + state: 'done', + prompt: 'finished in WSL', + agentType: 'codex', + connectionId: 'wsl:Ubuntu', + receivedAt: now, + stateStartedAt: now - 60_000 + } + ] + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === TEST_WORKTREE_ID) + + expect(summary?.agents).toEqual([]) + }) + + it('keeps remote hook rows whose tabs are only tracked on the remote host', async () => { + // Why: the local session partition for an SSH host can be empty while the + // remote host owns the terminals; absence there is not proof of a close. + const remoteRepo = { + id: 'repo-ssh-6072', + path: '/home/me/project', + displayName: 'remote-vm', + badgeColor: 'blue', + addedAt: 1, + connectionId: 'ssh-6072' + } + const remoteWorktree = { + path: '/home/me/project/.worktrees/feature-agents', + head: 'def', + branch: 'refs/heads/feature/agents', + isBare: false, + isMainWorktree: false + } + const remoteWorktreeId = `${remoteRepo.id}::${remoteWorktree.path}` + const metaById: Record = { + [remoteWorktreeId]: makeWorktreeMeta({ displayName: 'Remote agents' }) + } + const runtimeStore = { + ...store, + getRepos: () => [remoteRepo], + getRepo: (id: string) => (id === remoteRepo.id ? remoteRepo : undefined), + getAllWorktreeMeta: () => metaById, + getWorktreeMeta: (worktreeId: string) => metaById[worktreeId], + getWorkspaceSession: () => getDefaultWorkspaceSession() + } + registerSshGitProvider('ssh-6072', { + listWorktrees: vi.fn().mockResolvedValue([remoteWorktree]) + } as never) + const now = Date.now() + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getAgentStatusSnapshot: () => [ + { + paneKey: 'remote-tab:88888888-8888-4888-8888-888888888888', + worktreeId: remoteWorktreeId, + tabId: 'remote-tab', + state: 'working', + prompt: 'remote agent without local tab records', + agentType: 'codex', + connectionId: 'ssh-6072', + receivedAt: now, + stateStartedAt: now - 100 + } + ] + }) + + const { worktrees } = await runtime.getWorktreePs() + const summary = worktrees.find((worktree) => worktree.worktreeId === remoteWorktreeId) + + expect(summary?.agents).toEqual([ + expect.objectContaining({ prompt: 'remote agent without local tab records' }) + ]) + }) + it('marks the desktop-active worktree as isActive', async () => { const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession( makeWorkspaceSessionWithHeadlessTerminal() diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 701bfb496e2..963c91811b9 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -368,9 +368,15 @@ import { } from '../../shared/setup-agent-sequencing' import { TASK_PROVIDERS } from '../../shared/task-providers' import { FIRST_PANE_ID } from '../../shared/pane-key' -import { isTerminalLeafId, makePaneKey, parsePaneKey } from '../../shared/stable-pane-id' +import { + isTerminalLeafId, + makePaneKey, + parseLegacyNumericPaneKey, + parsePaneKey +} from '../../shared/stable-pane-id' import { parseAppSshPtyId } from '../../shared/ssh-pty-id' import { isValidHostTerminalTabId, isValidTerminalTabId } from '../../shared/terminal-tab-id' +import { isWslHookRelayConnectionId } from '../../shared/wsl-hook-relay-contract' import { applyTerminalQuickCommandMutation, MAX_QUICK_COMMANDS, @@ -1449,6 +1455,9 @@ type RuntimeAgentRowSnapshot = { ptyId: string worktreeId?: string tabId?: string + // Transport of the pane's PTY at retain time; null for local. Without it, + // OSC rows for SSH panes would lose the remote exemption in worktree.ps. + connectionId: string | null payload: ParsedAgentStatusPayload // When the current payload.state was first observed for this pane (ms). stateStartedAt: number @@ -10056,6 +10065,7 @@ export class OrcaRuntimeService { target.paneKey, target.worktreeId, target.tabId, + target.connectionId ?? null, payload ) || retainedChanged if (!this.onTerminalAgentStatus) { @@ -10086,6 +10096,7 @@ export class OrcaRuntimeService { paneKey: string, worktreeId: string | undefined, tabId: string | undefined, + connectionId: string | null, payload: ParsedAgentStatusPayload ): boolean { const now = Date.now() @@ -10100,6 +10111,7 @@ export class OrcaRuntimeService { ptyId, worktreeId, tabId, + connectionId, payload, stateStartedAt, updatedAt: now @@ -16962,11 +16974,34 @@ export class OrcaRuntimeService { } } + // Why: a connected PTY proves a pane is still live even when its tab has + // already left every session record (daemon-held terminals, graph gaps). + // Deliberately trusts the optimistic connected flag (no freshPtyLiveness + // gate, unlike the count loops above): evidence only KEEPS rows. + const connectedPtyEvidence = { + tabIds: new Set(), + paneKeys: new Set(), + ptyIds: new Set() + } + for (const pty of this.ptysById.values()) { + if (!pty.connected) { + continue + } + connectedPtyEvidence.ptyIds.add(pty.ptyId) + if (pty.tabId) { + connectedPtyEvidence.tabIds.add(pty.tabId) + } + if (pty.paneKey) { + connectedPtyEvidence.paneKeys.add(pty.paneKey) + } + } + this.attachAgentRowsToSummaries( summaries, runtimeWorktreeSummaryPathIndex, missingRuntimeWorktreeIds, - mirroredWorktreeIdByTabId + mirroredWorktreeIdByTabId, + connectedPtyEvidence ) const sorted = [...summaries.values()].sort(compareWorktreePs) @@ -16985,7 +17020,12 @@ export class OrcaRuntimeService { summaries: Map, runtimeWorktreeSummaryPathIndex: RuntimeWorktreeSummaryPathIndex, missingRuntimeWorktreeIds: Set, - mirroredWorktreeIdByTabId: ReadonlyMap + mirroredWorktreeIdByTabId: ReadonlyMap, + connectedPtyEvidence: { + tabIds: ReadonlySet + paneKeys: ReadonlySet + ptyIds: ReadonlySet + } ): void { // Why: most agents report via hooks (agent-hooks/server), not OSC, so the // hook snapshot is the primary source — same one the desktop sidebar reads. @@ -16994,8 +17034,10 @@ export class OrcaRuntimeService { string, { paneKey: string + ptyId?: string tabId?: string worktreeId?: string + connectionId: string | null state: ParsedAgentStatusPayload['state'] agentType: string | null prompt: string @@ -17011,8 +17053,10 @@ export class OrcaRuntimeService { const { payload } = snapshot rowSources.set(snapshot.paneKey, { paneKey: snapshot.paneKey, + ptyId: snapshot.ptyId, tabId: snapshot.tabId, worktreeId: snapshot.worktreeId, + connectionId: snapshot.connectionId, state: payload.state, agentType: payload.agentType ?? null, prompt: payload.prompt, @@ -17033,8 +17077,12 @@ export class OrcaRuntimeService { } rowSources.set(entry.paneKey, { paneKey: entry.paneKey, + // Hook payloads carry no ptyId; keep the OSC-observed one so the + // connected-PTY rescue survives a hook row winning the freshness race. + ptyId: existing?.ptyId, tabId: entry.tabId, worktreeId: entry.worktreeId, + connectionId: entry.connectionId, state: entry.state, agentType: entry.agentType ?? null, prompt: entry.prompt, @@ -17055,9 +17103,31 @@ export class OrcaRuntimeService { for (const src of rowSources.values()) { // Why: hooks retain launch-time attribution across automatic workspace // renames; the tab's current mirrored owner is authoritative when present. - const tabId = src.tabId ?? parsePaneKey(src.paneKey)?.tabId - const worktreeId = - (tabId ? mirroredWorktreeIdByTabId.get(tabId) : undefined) ?? src.worktreeId + // Legacy numeric pane keys (non-UUID leaves) still name a real tab, so + // parse them too — otherwise their rows would bypass the stale filter. + const tabId = + src.tabId ?? + parsePaneKey(src.paneKey)?.tabId ?? + parseLegacyNumericPaneKey(src.paneKey)?.tabId + const mirroredWorktreeId = tabId ? mirroredWorktreeIdByTabId.get(tabId) : undefined + if ( + tabId !== undefined && + mirroredWorktreeId === undefined && + (src.connectionId === null || isWslHookRelayConnectionId(src.connectionId)) && + !connectedPtyEvidence.tabIds.has(tabId) && + !connectedPtyEvidence.paneKeys.has(src.paneKey) && + (src.ptyId === undefined || !connectedPtyEvidence.ptyIds.has(src.ptyId)) + ) { + // Why: hook snapshots hydrate from last-status.json for days, so a row + // from a local or WSL-relayed pane whose tab left every session and the + // live graph, with no connected PTY, is retained history — surfacing it + // resurrects closed agents on mobile (#6072). SSH rows are exempt (their + // tabs may exist only remotely), as are rows with no resolvable tabId + // (staleness unprovable). Session tabs count as existence: headless + // serve has no renderer graph, and session.tabs.list serves them. + continue + } + const worktreeId = mirroredWorktreeId ?? src.worktreeId if (!worktreeId) { continue } diff --git a/src/main/runtime/rpc/methods/file-watch-event-batcher.ts b/src/main/runtime/rpc/methods/file-watch-event-batcher.ts index de96b7b6855..24e93e2affc 100644 --- a/src/main/runtime/rpc/methods/file-watch-event-batcher.ts +++ b/src/main/runtime/rpc/methods/file-watch-event-batcher.ts @@ -1,8 +1,9 @@ +import { + WATCH_BATCH_MAX_WAIT_MS, + WATCH_BATCH_TRAILING_MS +} from '../../../../shared/filesystem-watch-batch-window' import type { FsChangeEvent } from '../../../../shared/types' - -const FILE_WATCH_FLUSH_MS = 150 -const FILE_WATCH_MAX_WAIT_MS = 500 -const MAX_FILE_WATCH_BATCH_EVENTS = 5_000 +import { MAX_BATCHED_WATCHER_EVENTS } from '../../../ipc/filesystem-watcher-event-batch' export function createFileWatchEventBatcher( worktree: string, @@ -46,7 +47,7 @@ export function createFileWatchEventBatcher( if (incomingOverflow) { events = [incomingOverflow] overflowed = true - } else if (events.length + nextEvents.length > MAX_FILE_WATCH_BATCH_EVENTS) { + } else if (events.length + nextEvents.length > MAX_BATCHED_WATCHER_EVENTS) { // Why: once precision is too expensive to retain and stream, one // overflow asks clients to refresh safely without a huge payload. events = [ @@ -66,14 +67,14 @@ export function createFileWatchEventBatcher( if (firstEventAt === 0) { firstEventAt = now } - if (now - firstEventAt >= FILE_WATCH_MAX_WAIT_MS) { + if (now - firstEventAt >= WATCH_BATCH_MAX_WAIT_MS) { flush() return } clearTimer() // Why: remote file-watch events cross the runtime WebSocket before the // renderer refreshes the tree. Match local watcher batching here. - timer = setTimeout(flush, FILE_WATCH_FLUSH_MS) + timer = setTimeout(flush, WATCH_BATCH_TRAILING_MS) if (typeof timer.unref === 'function') { timer.unref() } diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index c81d2b5a6a1..1c1ff41328d 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -3316,6 +3316,195 @@ describe('PtyHandler', () => { ]) expect(handler.activePtyCount).toBe(0) }) + + describe('onPtyPoolEmpty', () => { + it('fires once when natural exit drains the last PTY, never while one remains', async () => { + const onExitCallbacks: ((evt: { exitCode: number }) => void)[] = [] + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + onData: vi.fn(), + onExit: vi.fn((cb: (evt: { exitCode: number }) => void) => { + onExitCallbacks.push(cb) + }) + }) + const poolEmpty = vi.fn() + handler.onPtyPoolEmpty(poolEmpty) + + await spawnPty() + await spawnPty() + expect(onExitCallbacks).toHaveLength(2) + + onExitCallbacks[0]({ exitCode: 0 }) + expect(handler.activePtyCount).toBe(1) + expect(poolEmpty).not.toHaveBeenCalled() + + onExitCallbacks[1]({ exitCode: 0 }) + expect(handler.activePtyCount).toBe(0) + expect(poolEmpty).toHaveBeenCalledTimes(1) + }) + + it('fires when the dead-shell reap inside attach removes the last PTY', async () => { + mockPtySpawn.mockReturnValue({ ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() }) + const poolEmpty = vi.fn() + handler.onPtyPoolEmpty(poolEmpty) + await spawnPty() + + const aliveSpy = vi.spyOn(ptyShellUtils, 'isProcessAlive').mockReturnValue(false) + try { + await expect(attachPty({ id: 'pty-1', suppressReplayNotification: true })).rejects.toThrow( + 'PTY "pty-1" not found' + ) + } finally { + aliveSpy.mockRestore() + } + + expect(handler.activePtyCount).toBe(0) + expect(poolEmpty).toHaveBeenCalledTimes(1) + }) + + it('fires when dispose-for-shutdown removes the last PTY', async () => { + mockPtySpawn.mockReturnValue({ ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() }) + const poolEmpty = vi.fn() + handler.onPtyPoolEmpty(poolEmpty) + await spawnPty() + await spawnPty() + + await handler.dispose({ waitForPhysicalExit: false }) + + expect(handler.activePtyCount).toBe(0) + expect(poolEmpty).toHaveBeenCalledTimes(1) + }) + + it('fires when a spawn fails before the PTY ever reaches the pool', async () => { + // Why: removePty can only announce a PTY it stored, so a creation that dies mid-flight + // would otherwise leave the relay believing it is still non-idle forever. + mockPtySpawn.mockImplementation(() => { + throw new Error('posix_spawnp failed') + }) + const poolEmpty = vi.fn() + handler.onPtyPoolEmpty(poolEmpty) + + await expect(spawnPty()).rejects.toThrow() + + expect(handler.activePtyCount).toBe(0) + expect(handler.pendingPtyCreationCount).toBe(0) + expect(poolEmpty).toHaveBeenCalledTimes(1) + }) + + it('stays silent when a failing creation settles while another is still admitted', async () => { + let spawnCall = 0 + mockPtySpawn.mockImplementation(() => { + spawnCall += 1 + if (spawnCall === 1) { + throw new Error('posix_spawnp failed') + } + return { ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() } + }) + const poolEmpty = vi.fn() + handler.onPtyPoolEmpty(poolEmpty) + + // Both admissions land before either creation resolves, so the failing one must not + // announce an empty pool while the surviving one still owns a shell. + const failing = spawnPty() + const succeeding = spawnPty() + expect(handler.pendingPtyCreationCount).toBe(2) + + await expect(failing).rejects.toThrow() + await succeeding + + expect(handler.activePtyCount).toBe(1) + expect(poolEmpty).not.toHaveBeenCalled() + }) + + it('stops notifying after the returned unsubscribe runs', async () => { + const onExitCallbacks: ((evt: { exitCode: number }) => void)[] = [] + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + onData: vi.fn(), + onExit: vi.fn((cb: (evt: { exitCode: number }) => void) => { + onExitCallbacks.push(cb) + }) + }) + const poolEmpty = vi.fn() + const unsubscribe = handler.onPtyPoolEmpty(poolEmpty) + + await spawnPty() + onExitCallbacks[0]({ exitCode: 0 }) + expect(poolEmpty).toHaveBeenCalledTimes(1) + + unsubscribe() + await spawnPty() + onExitCallbacks[1]({ exitCode: 0 }) + expect(handler.activePtyCount).toBe(0) + expect(poolEmpty).toHaveBeenCalledTimes(1) + }) + }) + + describe('onPtyPoolActive', () => { + it('fires at admission, while the pool is still empty', async () => { + mockPtySpawn.mockReturnValue({ ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() }) + const poolCounts: number[] = [] + handler.onPtyPoolActive(() => { + poolCounts.push(handler.activePtyCount) + }) + + const pending = spawnPty() + // The relay must learn it is non-idle here, not after the creation resolves. + expect(poolCounts).toEqual([0]) + + await pending + expect(poolCounts).toEqual([0, 1]) + }) + + it('fires for a revived PTY whose creation was admitted before the pool was empty', async () => { + await spawnPty({ cols: 80, rows: 24, cwd: '/tmp' }) + const state = (await dispatcher.callRequest('pty.serialize', { ids: ['pty-1'] })) as string + await handler.dispose({ waitForPhysicalExit: false }) + dispatcher = createMockDispatcher() + handler = new PtyHandler(dispatcher as unknown as RelayDispatcher) + mockPtySpawn.mockReturnValue({ ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() }) + const poolActive = vi.fn() + handler.onPtyPoolActive(poolActive) + + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + try { + await dispatcher.callRequest('pty.revive', { state }) + } finally { + killSpy.mockRestore() + } + + expect(handler.activePtyCount).toBe(1) + expect(poolActive).toHaveBeenCalled() + }) + + it('stops notifying after the returned unsubscribe runs', async () => { + mockPtySpawn.mockReturnValue({ ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() }) + const poolActive = vi.fn() + const unsubscribe = handler.onPtyPoolActive(poolActive) + + await spawnPty() + const callsWhileSubscribed = poolActive.mock.calls.length + expect(callsWhileSubscribed).toBeGreaterThan(0) + + unsubscribe() + await spawnPty() + expect(poolActive).toHaveBeenCalledTimes(callsWhileSubscribed) + }) + }) + + it('counts a spawn admitted but not yet pooled as a pending creation', async () => { + mockPtySpawn.mockReturnValue({ ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() }) + expect(handler.pendingPtyCreationCount).toBe(0) + + const pending = spawnPty() + // The shell is already owned even though activePtyCount still reads zero. + expect(handler.activePtyCount).toBe(0) + expect(handler.pendingPtyCreationCount).toBe(1) + + await pending + expect(handler.activePtyCount).toBe(1) + expect(handler.pendingPtyCreationCount).toBe(0) + }) }) describe('attachIdentityMismatches', () => { diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 67bfc4c6e15..cc685389f0a 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -372,6 +372,8 @@ export class PtyHandler { private reloadPtyModuleFromDisk = false // Why: single optional slot is intentional — callers compose externally; a throw is swallowed so it can't block cleanup. private exitListener: PtyExitListener | null = null + private ptyPoolEmptyListener: (() => void) | null = null + private ptyPoolActiveListener: (() => void) | null = null // Why: env augmenters run on every spawn so each PTY sees live hook coords without the dispatcher knowing about agent hooks. private envAugmenters: PtyEnvAugmenter[] = [] private readonly agentSessionOwners = new ClaimedAgentPtyOwnerRegistry() @@ -462,6 +464,10 @@ export class PtyHandler { } } + // Why: this value never reaches the grace *timer* — startGraceTimer's only caller always passes an + // explicit timeoutMs — but relay.startGrace reads it back through configuredGraceTimeMs to pick the + // grace branch, so a host-sent change does affect shutdown behavior. Callers and the host-sleep + // consequence: docs/reference/relay-grace-time-reconfiguration.md. setGraceTimeMs(graceTimeMs: number): void { this.graceTimeMs = Math.max(0, Math.floor(graceTimeMs)) } @@ -494,6 +500,53 @@ export class PtyHandler { this.exitListener = listener } + /** Notified when the last PTY leaves the pool, so the relay can re-arm its idle grace. */ + onPtyPoolEmpty(listener: () => void): () => void { + this.ptyPoolEmptyListener = listener + return () => { + if (this.ptyPoolEmptyListener === listener) { + this.ptyPoolEmptyListener = null + } + } + } + + /** + * Notified when a PTY creation is admitted or a PTY joins the pool. + * + * Why: the relay arms its idle cap on an empty pool, and a spawn/revive that lands after that + * decision must disarm it — creation is asynchronous, so the pool-empty edge alone can't see it. + */ + onPtyPoolActive(listener: () => void): () => void { + this.ptyPoolActiveListener = listener + return () => { + if (this.ptyPoolActiveListener === listener) { + this.ptyPoolActiveListener = null + } + } + } + + private notifyPoolListener(listener: (() => void) | null, label: string): void { + if (!listener) { + return + } + try { + listener() + } catch (err) { + process.stderr.write( + `[pty-handler] ${label} listener threw: ${err instanceof Error ? err.message : String(err)}\n` + ) + } + } + + // Why: the sole removal path, so the three exit routes can't drift on who announces an empty pool. + private removePty(id: string): void { + this.ptys.delete(id) + if (this.ptys.size > 0) { + return + } + this.notifyPoolListener(this.ptyPoolEmptyListener, 'pty-pool-empty') + } + /** Register an env augmenter merged into every spawn env *after* process.env and renderer env. * Used by the relay-hook server to inject ORCA_AGENT_HOOK_* coords. See docs/design/agent-status-over-ssh.md §3. */ addEnvAugmenter(augmenter: PtyEnvAugmenter): () => void { @@ -617,6 +670,8 @@ export class PtyHandler { private wireAndStore(managed: ManagedPty): void { managed.physicalExit = new PhysicalExitTracker() this.ptys.set(managed.id, managed) + // Why: a second announce covers any store whose admission window has already closed. + this.notifyPoolListener(this.ptyPoolActiveListener, 'pty-pool-active') const emitIngressData = (emission: PtyIngressEmission): void => { const rawLength = emission.rawEndSeq - emission.rawStartSeq this.appendReplayBuffer(managed, emission.data) @@ -672,7 +727,7 @@ export class PtyHandler { this.publishPendingExit(managed.id) this.notifyExitListener(managed) this.agentSessionOwners.release(managed.id) - this.ptys.delete(managed.id) + this.removePty(managed.id) this.clearPtyInputState(managed.id) // Why: release the ptmx fd on natural exit, else the master fd leaks until GC (docs/fix-pty-fd-leak.md). disposeManagedPty(managed) @@ -1157,6 +1212,9 @@ export class PtyHandler { throw error } this.pendingSpawnCount++ + // Why: announce at admission, not at store — the relay must stop treating itself as idle before + // the creation parks on its first await, or an armed idle timer kills the shell it produces. + this.notifyPoolListener(this.ptyPoolActiveListener, 'pty-pool-active') let finished = false return () => { if (finished) { @@ -1171,6 +1229,11 @@ export class PtyHandler { this.pendingCreationDrainResolvers.clear() } finishPtyCreationOperations(finishRemovalOperations) + // Why: a creation that failed before wireAndStore still leaves the pool empty, and removePty + // can't announce a PTY that was never stored — without this the relay never re-arms its idle cap. + if (this.ptys.size === 0 && this.pendingSpawnCount === 0) { + this.notifyPoolListener(this.ptyPoolEmptyListener, 'pty-pool-empty') + } } } @@ -1535,7 +1598,7 @@ export class PtyHandler { this.notifyExitListener(managed) this.agentSessionOwners.release(managed.id) disposeManagedPty(managed) - this.ptys.delete(id) + this.removePty(id) this.clearPtyFlowState(id) throw new Error(`PTY "${id}" not found`) } @@ -2065,7 +2128,7 @@ export class PtyHandler { this.notifyExitListener(managed) this.agentSessionOwners.release(managed.id) disposeManagedPty(managed) - this.ptys.delete(managed.id) + this.removePty(managed.id) this.clearPtyFlowState(managed.id) } } @@ -2105,6 +2168,11 @@ export class PtyHandler { return this.ptys.size } + /** Spawns admitted but not yet in the pool — each already owns a shell the relay must not treat as idle. */ + get pendingPtyCreationCount(): number { + return this.pendingSpawnCount + } + get retainedStartupCommandCount(): number { let count = 0 for (const managed of this.ptys.values()) { diff --git a/src/relay/relay-grace-branch.test.ts b/src/relay/relay-grace-branch.test.ts new file mode 100644 index 00000000000..1361715a4ea --- /dev/null +++ b/src/relay/relay-grace-branch.test.ts @@ -0,0 +1,100 @@ +import { describe, expect, it } from 'vitest' +import { decideRelayGrace, type RelayGraceDecisionInput } from './relay-grace-branch' + +const EMPTY_DETACHED_STARTUP_GRACE_MS = 30_000 +const IDLE_RELAY_GRACE_MS = 15 * 60_000 + +function decide(overrides: Partial = {}) { + return decideRelayGrace({ + configuredGraceMs: 0, + relayIdle: false, + detached: false, + hasAcceptedSocketClient: true, + activePtyCount: 1, + retryDeferredShutdown: false, + emptyDetachedStartupGraceMs: EMPTY_DETACHED_STARTUP_GRACE_MS, + idleRelayGraceMs: IDLE_RELAY_GRACE_MS, + ...overrides + }) +} + +describe('decideRelayGrace', () => { + describe('the zero-only gate', () => { + it('honors a grace raised after launch instead of clamping it to the idle cap', () => { + // Why: the reported bug. startGrace used to read the launch-time argv closure, so a host that + // raised the grace to 24h via relay.configureGraceTime still got the 15-minute idle cap. + expect(decide({ configuredGraceMs: 86_400_000, relayIdle: true, activePtyCount: 0 })).toEqual( + { + branch: 'configured', + timeoutMs: 86_400_000 + } + ) + }) + + it('caps an idle relay running the unlimited default at the idle grace', () => { + // Why: prepareForHostSleep notifies graceTimeSeconds:0, which now reaches this selector. + // Deliberate behavior change: a host-sleep relay holding zero PTYs exits after the idle cap + // instead of living forever. Pinned so the revived path stays a decision, not an accident. + expect(decide({ configuredGraceMs: 0, relayIdle: true, activePtyCount: 0 })).toEqual({ + branch: 'idle-no-ptys', + timeoutMs: IDLE_RELAY_GRACE_MS + }) + }) + + it('leaves a non-idle relay on the configured branch even at the unlimited default', () => { + // Why: an admitted-but-unpooled creation keeps relayIdle false, so the cap must not apply — + // it would kill the shell that creation is about to produce (#6955). + expect(decide({ configuredGraceMs: 0, relayIdle: false })).toEqual({ + branch: 'configured', + timeoutMs: 0 + }) + }) + }) + + describe('branch precedence', () => { + it('prefers shutdown-deferred over every other branch', () => { + expect( + decide({ + retryDeferredShutdown: true, + detached: true, + hasAcceptedSocketClient: false, + relayIdle: true, + activePtyCount: 0 + }) + ).toEqual({ branch: 'shutdown-deferred', timeoutMs: IDLE_RELAY_GRACE_MS }) + }) + + it('prefers startup-empty-detached over the idle cap', () => { + expect( + decide({ + detached: true, + hasAcceptedSocketClient: false, + relayIdle: true, + activePtyCount: 0 + }) + ).toEqual({ branch: 'startup-empty-detached', timeoutMs: EMPTY_DETACHED_STARTUP_GRACE_MS }) + }) + + it('bounds a configured grace on the startup-empty-detached branch', () => { + expect( + decide({ + configuredGraceMs: 5_000, + detached: true, + hasAcceptedSocketClient: false, + activePtyCount: 0 + }) + ).toEqual({ branch: 'startup-empty-detached', timeoutMs: 5_000 }) + }) + + it('does not treat a detached relay that already accepted a client as startup-empty', () => { + expect( + decide({ + detached: true, + hasAcceptedSocketClient: true, + relayIdle: true, + activePtyCount: 0 + }) + ).toEqual({ branch: 'idle-no-ptys', timeoutMs: IDLE_RELAY_GRACE_MS }) + }) + }) +}) diff --git a/src/relay/relay-grace-branch.ts b/src/relay/relay-grace-branch.ts new file mode 100644 index 00000000000..aa8aabcdd63 --- /dev/null +++ b/src/relay/relay-grace-branch.ts @@ -0,0 +1,63 @@ +/** Which rule decided the current relay grace window. */ +export type RelayGraceBranch = + | 'shutdown-deferred' + | 'startup-empty-detached' + | 'idle-no-ptys' + | 'configured' + +export type RelayGraceDecisionInput = { + /** The live configured grace, not the launch-time argv value; 0 means unlimited. */ + configuredGraceMs: number + /** Zero pooled PTYs *and* zero admitted-but-unpooled creations. */ + relayIdle: boolean + detached: boolean + hasAcceptedSocketClient: boolean + activePtyCount: number + retryDeferredShutdown: boolean + emptyDetachedStartupGraceMs: number + idleRelayGraceMs: number +} + +export type RelayGraceDecision = { + branch: RelayGraceBranch + timeoutMs: number +} + +/** + * Picks the relay's shutdown grace window. + * + * Why a separate module: relay.ts runs `main()` on import and exports nothing, so the zero-only + * gate below — the property that keeps a configured grace from being clamped to the idle cap — + * could not otherwise be tested. + */ +export function decideRelayGrace(input: RelayGraceDecisionInput): RelayGraceDecision { + // Why: a detached relay that never accepted a client has no PTY state and shouldn't linger forever. + const startupEmptyDetached = + input.detached && !input.hasAcceptedSocketClient && input.activePtyCount === 0 + // Why: zero PTYs means nothing left to preserve, so only the unlimited default is capped — an + // explicitly configured grace is honored verbatim rather than clamped down to the idle cap. + // Why: a spawn parked mid-creation is not in the pool yet, so capping on it would kill the live + // shell it is about to produce. + const idleNoPtys = input.relayIdle && input.configuredGraceMs === 0 + + const branch: RelayGraceBranch = input.retryDeferredShutdown + ? 'shutdown-deferred' + : startupEmptyDetached + ? 'startup-empty-detached' + : idleNoPtys + ? 'idle-no-ptys' + : 'configured' + + const timeoutMs = + branch === 'startup-empty-detached' + ? input.configuredGraceMs === 0 + ? input.emptyDetachedStartupGraceMs + : Math.min(input.configuredGraceMs, input.emptyDetachedStartupGraceMs) + : // Why: a refused kill leaves the PTY pooled, so the idle branch can't be reached — this + // branch must supply its own bound or the shipped grace=0 default arms no retry at all. + branch === 'idle-no-ptys' || branch === 'shutdown-deferred' + ? input.idleRelayGraceMs + : input.configuredGraceMs + + return { branch, timeoutMs } +} diff --git a/src/relay/relay.ts b/src/relay/relay.ts index f282cbb483e..f807fc6ac2f 100644 --- a/src/relay/relay.ts +++ b/src/relay/relay.ts @@ -51,6 +51,7 @@ import { } from '../shared/pi-agent-kind' import { resolveSetupAgentSequenceLaunchCommand } from '../shared/setup-agent-sequencing' import { pickRemoteCliEnv } from './remote-cli-env' +import { decideRelayGrace, type RelayGraceBranch } from './relay-grace-branch' import { relayLogLine } from './relay-diagnostic-log' import { remoteCliRequestTimeoutMs } from './remote-cli-timeout' import { shouldReadRemoteCliStdin } from './remote-cli-stdin' @@ -68,6 +69,9 @@ const EMPTY_DETACHED_STARTUP_GRACE_MS = parseNonNegativeIntEnv( 'ORCA_RELAY_EMPTY_STARTUP_GRACE_MS', 60_000 ) +// Why: a relay holding zero PTYs preserves nothing, so an unlimited grace only accumulates idle daemons. +// The env override is test-only — the remote relay is launched over a non-interactive SSH exec channel that carries no client env. +const IDLE_RELAY_GRACE_MS = parseNonNegativeIntEnv('ORCA_RELAY_IDLE_GRACE_MS', 15 * 60_000) type SocketIdentity = { dev: bigint @@ -679,8 +683,21 @@ async function main(): Promise { function configureRelayGraceTime(params: Record): { graceTimeMs: number } { const seconds = Number(params.graceTimeSeconds) if (Number.isFinite(seconds) && seconds >= 0) { + const previousGraceMs = ptyHandler.configuredGraceTimeMs // Why: the host sends 0 before system sleep so live remote PTYs survive longer than the ordinary grace window. ptyHandler.setGraceTimeMs(Math.floor(seconds) * 1000) + // Why: startGrace samples the configured value at arm time, so a raise that lands while the idle + // timer is already running would still fire at the old deadline. Re-arm only on an actual change + // — the host re-asserts the same value on every establish, and restarting the window on those + // would keep a grace alive indefinitely. + if ( + ptyHandler.configuredGraceTimeMs !== previousGraceMs && + graceDeadlineAt !== null && + graceReason !== null && + !shutdownInFlight + ) { + startGrace('grace reconfigured') + } } return { graceTimeMs: ptyHandler.configuredGraceTimeMs } } @@ -829,6 +846,9 @@ async function main(): Promise { let hasAcceptedSocketClient = false let graceDeadlineAt: number | null = null let graceReason: string | null = null + // Why: only the idle branch is a "nothing left to preserve" bet, so only it may be revoked when a + // PTY appears mid-window; the other branches keep their armed deadline. + let graceBranch: RelayGraceBranch | null = null dispatcher.onRequest('relay.status', async () => ({ pid: process.pid, @@ -864,6 +884,7 @@ async function main(): Promise { } graceDeadlineAt = null graceReason = null + graceBranch = null ptyHandler.cancelGraceTimer() } @@ -1116,26 +1137,45 @@ async function main(): Promise { dispatcher.invalidateClient() }) - function startGrace(reason: string): void { - const startupEmptyDetached = - detached && !hasAcceptedSocketClient && ptyHandler.activePtyCount === 0 - // Why: a detached relay that never accepted a client has no PTY state and shouldn't linger forever. - const timeoutMs = startupEmptyDetached - ? graceTimeMs === 0 - ? EMPTY_DETACHED_STARTUP_GRACE_MS - : Math.min(graceTimeMs, EMPTY_DETACHED_STARTUP_GRACE_MS) - : graceTimeMs + function startGrace(reason: string, options?: { retryDeferredShutdown?: boolean }): void { + // Why: the live configured value, not the launch-time argv closure — the host can raise the grace + // after launch via relay.configureGraceTime, and a zero-only gate reading a stale zero would be + // zero-at-launch-only, i.e. correct only by coincidence. + const decision = decideRelayGrace({ + configuredGraceMs: ptyHandler.configuredGraceTimeMs, + relayIdle: isRelayIdle(), + detached, + hasAcceptedSocketClient, + activePtyCount: ptyHandler.activePtyCount, + retryDeferredShutdown: options?.retryDeferredShutdown === true, + emptyDetachedStartupGraceMs: EMPTY_DETACHED_STARTUP_GRACE_MS, + idleRelayGraceMs: IDLE_RELAY_GRACE_MS + }) + graceBranch = decision.branch + const timeoutMs = decision.timeoutMs graceDeadlineAt = timeoutMs === 0 ? null : Date.now() + timeoutMs graceReason = reason relayLogLine( - `[relay] Grace started (${reason}): timeoutMs=${timeoutMs}, startupEmptyDetached=${startupEmptyDetached}, ptys=${ptyHandler.activePtyCount}, clients=${socketClients.size}` + `[relay] Grace started (${reason}): timeoutMs=${timeoutMs}, branch=${graceBranch}, ptys=${ptyHandler.activePtyCount}, clients=${socketClients.size}` ) ptyHandler.startGraceTimer(() => { + // Why: last line of defense for the idle cap — a PTY that appeared without announcing itself + // must not be killed by a timer armed while the relay was still empty (#6955). + if (graceBranch === 'idle-no-ptys' && !isRelayIdle()) { + relayLogLine(`[relay] Grace expired (${reason}) but relay is no longer idle; re-evaluating`) + startGrace(reason) + return + } relayLogLine(`[relay] Grace expired (${reason}); shutting down`) shutdown() }, timeoutMs) } + // Why: a creation admitted but not yet pooled already owns a shell, so it counts as non-idle. + function isRelayIdle(): boolean { + return ptyHandler.activePtyCount === 0 && ptyHandler.pendingPtyCreationCount === 0 + } + if (detached) { // Why: detached stdin is /dev/null, so listening would EOF → grace → shutdown before --connect arrives; use the socket instead. startGrace('detached startup') @@ -1176,9 +1216,12 @@ async function main(): Promise { ) graceDeadlineAt = null graceReason = null + graceBranch = null void ptyHandler .dispose() .then(() => { + stopPoolWatch() + stopPoolActiveWatch() dispatcher.dispose() fsHandler.dispose() gitHandler.dispose() @@ -1192,13 +1235,41 @@ async function main(): Promise { }) .catch((error) => { // Why: keep owning a PTY whose native kill was rejected so a transient signal failure doesn't orphan a remote shell. + // Why: the pool watches stay registered — the socket server is still listening, so a client can + // reconnect and cancel this grace, and that revived relay still needs both re-evaluations. shutdownInFlight = false relayLogLine( `[relay] Shutdown deferred: ${error instanceof Error ? error.message : String(error)}` ) + // Why: shutdown() already cleared graceReason, so without re-arming a client-less relay + // whose kill was refused would stay resident forever with nothing left to retry it. + if (socketClients.size === 0) { + startGrace('shutdown deferred', { retryDeferredShutdown: true }) + } }) } + // Why: with the shipped unlimited default the grace timer is never armed, so an expiry can't + // notice the pool emptying; re-evaluate on the last exit instead. graceReason (not + // graceTimerActive) is the only signal that a grace window is open in that configuration. + // Why: a null deadline is exactly that unlimited case; re-arming a scheduled one would restart + // an explicitly configured window and double it. + const stopPoolWatch = ptyHandler.onPtyPoolEmpty(() => { + if (graceReason !== null && graceDeadlineAt === null && !shutdownInFlight) { + startGrace('last pty exited') + } + }) + + // Why: the idle cap is armed on an empty pool, but creation is asynchronous — a spawn or revive + // admitted after that decision (the client dropped mid-`pty.revive`, say) makes the relay non-idle + // again, and re-evaluating disarms the timer instead of letting it kill a live PTY (#6955). + // The grace window itself stays open, so the PTY's own exit still re-arms the cap. + const stopPoolActiveWatch = ptyHandler.onPtyPoolActive(() => { + if (graceBranch === 'idle-no-ptys' && graceReason !== null && !shutdownInFlight) { + startGrace(graceReason) + } + }) + process.on('SIGTERM', shutdown) process.on('SIGINT', shutdown) // Why: default SIGHUP exits immediately, killing PTYs before grace; ignore it so the relay survives SSH disconnect. diff --git a/src/relay/subprocess.test.ts b/src/relay/subprocess.test.ts index e80a3ccec86..57b3466707d 100644 --- a/src/relay/subprocess.test.ts +++ b/src/relay/subprocess.test.ts @@ -108,6 +108,57 @@ const WORKING_NODE_PTY_MODULE = `module.exports = { spawn() { return { onData() {}, onExit() {}, write() {}, resize() {}, kill() {}, clear() {} } } }\n` +// A shell that reports its own exit after a delay, so the relay sees the pool drain on its own. +function selfExitingNodePtyModule(exitAfterMs: number): string { + return `module.exports = { spawn() { + const exitHandlers = [] + setTimeout(() => { for (const cb of exitHandlers) { cb({ exitCode: 0, signal: 0 }) } }, ${exitAfterMs}) + return { + pid: process.pid, + process: 'mock-shell', + onData() {}, onExit(cb) { exitHandlers.push(cb) }, write() {}, resize() {}, kill() {}, clear() {} + } +} }\n` +} + +// A shell whose first dispose is refused, so ptyHandler.dispose() rejects once before a retry can succeed. +const KILL_REJECTS_FIRST_DISPOSE_MODULE = `let killAttempts = 0 +module.exports = { spawn() { + const exitHandlers = [] + return { + pid: 2147483646, + process: 'mock-shell', + onData() {}, onExit(cb) { exitHandlers.push(cb) }, write() {}, resize() {}, clear() {}, + kill() { + killAttempts++ + if (killAttempts <= 2) { throw new Error('kill refused') } + setTimeout(() => { for (const cb of exitHandlers) { cb({ exitCode: 0, signal: 0 }) } }, 0) + } + } +} }\n` + +// Why: an ESM mock with top-level await parks loadPty() in the window where a spawn is admitted but not yet pooled. +function writeSlowLoadingNodePty(root: string, loadDelayMs: number): void { + const nodePtyDir = path.join(root, 'node_modules', 'node-pty') + mkdirSync(path.join(nodePtyDir, 'lib'), { recursive: true }) + writeFileSync(path.join(nodePtyDir, 'package.json'), '{"type":"module","main":"lib/index.js"}\n') + writeFileSync( + path.join(nodePtyDir, 'lib', 'index.js'), + `await new Promise((resolve) => setTimeout(resolve, ${loadDelayMs})) +export function spawn() { + const exitHandlers = [] + return { + pid: process.pid, + process: 'mock-shell', + onData() {}, onExit(cb) { exitHandlers.push(cb) }, write() {}, resize() {}, clear() {}, + // Report the exit so relay shutdown can complete instead of parking on waitForPhysicalExit. + kill() { setTimeout(() => { for (const cb of exitHandlers) { cb({ exitCode: 0, signal: 0 }) } }, 0) } + } +} +` + ) +} + describe('Subprocess: Relay entry point', () => { let relay: RelayProcess | null = null let tmpDir: string @@ -504,6 +555,210 @@ describe('Subprocess: Relay entry point', () => { 10_000 ) + // Why: a relay holding zero PTYs preserves nothing, so the unlimited default must still be + // bounded once a client has come and gone — but an explicitly configured grace is never shortened. + function spawnIdleGraceDaemon( + nodePtyModule: string, + graceTimeSeconds: string, + idleGraceMs: string + ): { daemon: RelayProcess; sockPath: string } { + const daemonEntry = path.join(tmpDir, 'relay.js') + copyFileSync(relayEntry, daemonEntry) + writeMockNodePty(tmpDir, nodePtyModule) + const sockPath = path.join(tmpDir, 'relay.sock') + const daemon = spawnRelayEntry( + daemonEntry, + ['--detached', '--grace-time', graceTimeSeconds, '--sock-path', sockPath], + { ...process.env, ORCA_RELAY_IDLE_GRACE_MS: idleGraceMs } + ) + return { daemon, sockPath } + } + + async function connectAndDisconnect( + sockPath: string, + whileConnected?: (bridge: RelayProcess) => Promise + ): Promise { + const bridge = spawn(['--connect', '--sock-path', sockPath]) + try { + await bridge.sentinelReceived + await whileConnected?.(bridge) + } finally { + bridge.kill('SIGTERM') + await bridge.waitForExit().catch(() => {}) + } + } + + it.skipIf(process.platform === 'win32')( + 'shuts down an idle relay with no PTYs after the idle grace even with --grace-time 0', + async () => { + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-idle-')) + const sockPath = path.join(tmpDir, 'relay.sock') + relay = spawn(['--detached', '--grace-time', '0', '--sock-path', sockPath], { + ...process.env, + ORCA_RELAY_IDLE_GRACE_MS: '200' + }) + await relay.sentinelReceived + + // Accepting a client defeats the startup-empty branch, so only the idle cap can end this relay. + await connectAndDisconnect(sockPath) + + await relay.waitForExit(2000) + expect(relay.proc.exitCode).toBe(0) + }, + 15_000 + ) + + it.skipIf(process.platform === 'win32')( + 'keeps a relay with a live PTY alive past the idle grace', + async () => { + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-idle-live-pty-')) + const { daemon, sockPath } = spawnIdleGraceDaemon(WORKING_NODE_PTY_MODULE, '0', '200') + relay = daemon + await relay.sentinelReceived + + await connectAndDisconnect(sockPath, async (bridge) => { + const resp = await bridge.waitForResponse(bridge.send('pty.spawn', { cols: 80, rows: 24 })) + expect(resp.error).toBeUndefined() + }) + + await new Promise((resolve) => setTimeout(resolve, 1200)) + expect(relay.proc.exitCode).toBeNull() + }, + 15_000 + ) + + it.skipIf(process.platform === 'win32')( + 're-arms the idle grace when the last PTY exits during grace', + async () => { + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-idle-rearm-')) + const { daemon, sockPath } = spawnIdleGraceDaemon(selfExitingNodePtyModule(1500), '0', '200') + relay = daemon + await relay.sentinelReceived + + await connectAndDisconnect(sockPath, async (bridge) => { + const resp = await bridge.waitForResponse(bridge.send('pty.spawn', { cols: 80, rows: 24 })) + expect(resp.error).toBeUndefined() + }) + + // The live PTY must suppress the idle grace at disconnect; only its own exit re-arms it. + await new Promise((resolve) => setTimeout(resolve, 300)) + expect(relay.proc.exitCode).toBeNull() + + await relay.waitForExit(5000) + expect(relay.proc.exitCode).toBe(0) + }, + 20_000 + ) + + it.skipIf(process.platform === 'win32')( + 'honors an explicitly configured grace instead of clamping it to the idle cap', + async () => { + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-idle-configured-')) + const sockPath = path.join(tmpDir, 'relay.sock') + relay = spawn(['--detached', '--grace-time', '3600', '--sock-path', sockPath], { + ...process.env, + ORCA_RELAY_IDLE_GRACE_MS: '200' + }) + await relay.sentinelReceived + + await connectAndDisconnect(sockPath) + + await new Promise((resolve) => setTimeout(resolve, 1000)) + expect(relay.proc.exitCode).toBeNull() + }, + 15_000 + ) + + it.skipIf(process.platform === 'win32')( + 'keeps the relay alive when the client drops while a PTY creation is still in flight', + async () => { + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-idle-inflight-')) + const daemonEntry = path.join(tmpDir, 'relay.js') + copyFileSync(relayEntry, daemonEntry) + writeSlowLoadingNodePty(tmpDir, 1500) + const sockPath = path.join(tmpDir, 'relay.sock') + relay = spawnRelayEntry( + daemonEntry, + ['--detached', '--grace-time', '0', '--sock-path', sockPath], + { ...process.env, ORCA_RELAY_IDLE_GRACE_MS: '200' } + ) + await relay.sentinelReceived + + const bridge = spawn(['--connect', '--sock-path', sockPath]) + await bridge.sentinelReceived + // Revive, not spawn: it has no client-disconnect abort, so it really does produce a live shell + // after the parked module load resolves. + bridge.send('pty.revive', { + state: JSON.stringify([ + { id: 'pty-restored', pid: process.pid, cols: 80, rows: 24, cwd: tmpDir } + ]) + }) + // Let the creation park on the module load: it already owns a shell but is not in the pool yet. + await new Promise((resolve) => setTimeout(resolve, 400)) + bridge.kill('SIGTERM') + await bridge.waitForExit().catch(() => {}) + + // Well past the idle cap and past the point where the parked creation lands in the pool. + await new Promise((resolve) => setTimeout(resolve, 2500)) + expect(relay.proc.exitCode).toBeNull() + + const probe = spawn(['--connect', '--sock-path', sockPath]) + try { + await probe.sentinelReceived + const status = await probe.waitForResponse(probe.send('relay.status')) + expect((status.result as { ptys: { active: number } }).ptys.active).toBe(1) + } finally { + probe.kill('SIGTERM') + await probe.waitForExit().catch(() => {}) + } + }, + 20_000 + ) + + it.skipIf(process.platform === 'win32')( + 'does not extend an explicitly configured grace when the last PTY exits mid-window', + async () => { + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-configured-rearm-')) + const { daemon, sockPath } = spawnIdleGraceDaemon(selfExitingNodePtyModule(2500), '3', '200') + relay = daemon + await relay.sentinelReceived + + await connectAndDisconnect(sockPath, async (bridge) => { + const resp = await bridge.waitForResponse(bridge.send('pty.spawn', { cols: 80, rows: 24 })) + expect(resp.error).toBeUndefined() + }) + + // The 3s window keeps governing; re-arming at the ~2.5s PTY exit would push shutdown past 5s. + await relay.waitForExit(4200) + expect(relay.proc.exitCode).toBe(0) + }, + 20_000 + ) + + it.skipIf(process.platform === 'win32')( + 're-arms grace after a shutdown deferred by a rejected kill, so the relay still exits', + async () => { + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-shutdown-deferred-')) + const { daemon, sockPath } = spawnIdleGraceDaemon( + KILL_REJECTS_FIRST_DISPOSE_MODULE, + '2', + '200' + ) + relay = daemon + await relay.sentinelReceived + + await connectAndDisconnect(sockPath, async (bridge) => { + const resp = await bridge.waitForResponse(bridge.send('pty.spawn', { cols: 80, rows: 24 })) + expect(resp.error).toBeUndefined() + }) + + // First grace expiry hits the refused kill; only the re-armed window can retry it. + await relay.waitForExit(8000) + expect(relay.proc.exitCode).toBe(0) + }, + 20_000 + ) + it('reports relay diagnostics over relay.status', async () => { relay = spawn() await relay.sentinelReceived diff --git a/src/renderer/src/components/right-sidebar/FileExplorer.tsx b/src/renderer/src/components/right-sidebar/FileExplorer.tsx index 81a41e20aea..9e22f01ac5d 100644 --- a/src/renderer/src/components/right-sidebar/FileExplorer.tsx +++ b/src/renderer/src/components/right-sidebar/FileExplorer.tsx @@ -42,6 +42,7 @@ import { useFileExplorerDragDrop } from './useFileExplorerDragDrop' import { useFileExplorerImport } from './useFileExplorerImport' import { useFileExplorerManualRefresh } from './useFileExplorerManualRefresh' import { useFileExplorerTree } from './useFileExplorerTree' +import { decideExpandedDirLoad } from './file-explorer-stale-dir-cache' import { useFileExplorerWatch } from './useFileExplorerWatch' import { buildAddProjectFromFolderModalData, @@ -148,6 +149,7 @@ function FileExplorerFiles(): React.JSX.Element { markPathAsDirectory, refreshTree, refreshDir, + isDirStale, resetAndLoad } = useFileExplorerTree(worktreePath, expanded, activeWorktreeId) const hasNameFilterQuery = nameFilterQuery.trim().length > 0 @@ -347,11 +349,14 @@ function FileExplorerFiles(): React.JSX.Element { return } for (const dirPath of expanded) { - if (!dirCache[dirPath]?.children.length && !dirCache[dirPath]?.loading) { - const depth = - splitPathSegments(dirPath.slice(visibleFilesWorktreePath.length + 1)).length - 1 - void loadDir(dirPath, depth) + // Why: a full refresh (watcher overflow) re-reads only root and the dirs expanded at the time, + // so a listing cached while collapsed is unverified — re-read it here instead of trusting it. + const decision = decideExpandedDirLoad(dirCache[dirPath], isDirStale(dirPath)) + if (decision === 'skip') { + continue } + const depth = splitPathSegments(dirPath.slice(visibleFilesWorktreePath.length + 1)).length - 1 + void loadDir(dirPath, depth, decision === 'reload' ? { force: true } : undefined) } }, [expanded, visibleFilesWorktreePath]) // eslint-disable-line react-hooks/exhaustive-deps diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerTree.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts similarity index 58% rename from src/renderer/src/components/right-sidebar/useFileExplorerTree.test.ts rename to src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts index b98e61c825e..97e2dbd25d3 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerTree.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts @@ -3,7 +3,7 @@ import type { SetStateAction } from 'react' import type { DirEntry } from '../../../../shared/types' import type { DirCache } from './file-explorer-types' import { createFileExplorerDirLoadTracker } from './file-explorer-dir-load-tracker' -import { refreshFileExplorerExpandedDirs } from './useFileExplorerTree' +import { refreshFileExplorerExpandedDirs } from './file-explorer-expanded-dirs-refresh' type CacheUpdate = SetStateAction> @@ -44,7 +44,10 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: createFileExplorerDirLoadTracker(), setDirCache, - readDirectory + readDirectory, + // A limit at or above the dir count must stay a single wave, i.e. exactly + // today's two batched writes. + maxConcurrentReads: 16 }) expect(refreshed).toBe(true) @@ -128,7 +131,8 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: tracker, setDirCache, - readDirectory + readDirectory, + maxConcurrentReads: 16 }) // Not every dir was still current, so the refresh reports partial completion. @@ -172,7 +176,8 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: tracker, setDirCache, - readDirectory + readDirectory, + maxConcurrentReads: 16 }) // Let /repo/src resolve (and pass its resolve-time token check) while @@ -207,4 +212,137 @@ describe('refreshFileExplorerExpandedDirs', () => { children: [{ name: 'guide.md' }] }) }) + + it('never exceeds maxConcurrentReads in flight and still commits every directory', async () => { + const dirs = Array.from({ length: 20 }, (_, index) => ({ + dirPath: `/repo/d${index}`, + depth: 0 + })) + let cache: Record = {} + const setDirCache = vi.fn((update: CacheUpdate) => { + cache = typeof update === 'function' ? update(cache) : update + }) + let inFlight = 0 + let peakInFlight = 0 + const readDirectory = vi.fn(async (dirPath: string) => { + inFlight++ + peakInFlight = Math.max(peakInFlight, inFlight) + await new Promise((resolve) => setTimeout(resolve, 0)) + inFlight-- + return { + entries: [entry(`${dirPath.slice('/repo/'.length)}.ts`)], + operationOwner: { kind: 'local' as const } + } + }) + + const refreshed = await refreshFileExplorerExpandedDirs({ + dirs, + worktreePath: '/repo', + dirLoadTracker: createFileExplorerDirLoadTracker(), + setDirCache, + readDirectory, + maxConcurrentReads: 4 + }) + + expect(refreshed).toBe(true) + expect(peakInFlight).toBeLessThanOrEqual(4) + expect(readDirectory).toHaveBeenCalledTimes(20) + // One up-front loading write plus one result write per wave (five waves). + expect(setDirCache).toHaveBeenCalledTimes(6) + for (const { dirPath } of dirs) { + expect(cache[dirPath]).toMatchObject({ + loading: false, + children: [{ name: expect.any(String) }] + }) + } + }) + + it('marks queued dirs loading up front so only the first wave reads', async () => { + const dirs = Array.from({ length: 9 }, (_, index) => ({ + dirPath: `/repo/d${index}`, + depth: 0 + })) + let cache: Record = {} + const setDirCache = vi.fn((update: CacheUpdate) => { + cache = typeof update === 'function' ? update(cache) : update + }) + let releaseFirstWave!: () => void + const firstWaveGate = new Promise((resolve) => { + releaseFirstWave = resolve + }) + const readDirectory = vi.fn(async (dirPath: string) => { + if (['/repo/d0', '/repo/d1', '/repo/d2'].includes(dirPath)) { + await firstWaveGate + } + return { entries: [], operationOwner: { kind: 'local' as const } } + }) + + const refreshPromise = refreshFileExplorerExpandedDirs({ + dirs, + worktreePath: '/repo', + dirLoadTracker: createFileExplorerDirLoadTracker(), + setDirCache, + readDirectory, + maxConcurrentReads: 3 + }) + await Promise.resolve() + + expect(cache['/repo/d0']).toMatchObject({ loading: true }) + // A queued dir must already advertise loading:true, or FileExplorer's + // auto-load effect fans out an unbounded loadDir for it on the next + // `expanded` change — the reads this cap exists to bound. + expect(cache['/repo/d6']).toMatchObject({ loading: true }) + expect(readDirectory).toHaveBeenCalledTimes(3) + + releaseFirstWave() + await refreshPromise + + expect(cache['/repo/d6']).toMatchObject({ loading: false }) + }) + + it('drops a later-wave directory superseded while an earlier wave was reading', async () => { + const tracker = createFileExplorerDirLoadTracker() + let cache: Record = {} + const setDirCache = vi.fn((update: CacheUpdate) => { + cache = typeof update === 'function' ? update(cache) : update + }) + let releaseFirst!: () => void + const firstGate = new Promise((resolve) => { + releaseFirst = resolve + }) + const readDirectory = vi.fn(async (dirPath: string) => { + if (dirPath === '/repo/a') { + await firstGate + } + return { entries: [entry('x.ts')], operationOwner: { kind: 'local' as const } } + }) + + const refreshPromise = refreshFileExplorerExpandedDirs({ + dirs: [ + { dirPath: '/repo/a', depth: 0 }, + { dirPath: '/repo/b', depth: 0 } + ], + worktreePath: '/repo', + dirLoadTracker: tracker, + setDirCache, + readDirectory, + maxConcurrentReads: 1 + }) + await Promise.resolve() + + // A watcher-driven refreshDir supersedes the queued wave-2 dir before it + // ever starts reading; its token was already taken up front. + tracker.begin('/repo/b') + const newerBCache: DirCache = { loading: false, children: [] } + setDirCache((prev) => ({ ...prev, '/repo/b': newerBCache })) + + releaseFirst() + const refreshed = await refreshPromise + + expect(refreshed).toBe(false) + expect(cache['/repo/a']).toMatchObject({ loading: false, children: [{ name: 'x.ts' }] }) + // Wave 2 must neither read nor commit the superseded dir. + expect(readDirectory).toHaveBeenCalledTimes(1) + expect(cache['/repo/b']).toEqual(newerBCache) + }) }) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts new file mode 100644 index 00000000000..280520c6425 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts @@ -0,0 +1,137 @@ +import type { Dispatch, SetStateAction } from 'react' +import type { DirCache } from './file-explorer-types' +import type { FileExplorerDirLoadTracker } from './file-explorer-dir-load-tracker' +import { + fileExplorerEntriesToTreeNodes, + type FileExplorerDirectoryListing +} from './file-explorer-directory-listing' + +export type RefreshFileExplorerTreeDir = { + dirPath: string + depth: number +} + +export type RefreshFileExplorerExpandedDirsParams = { + dirs: RefreshFileExplorerTreeDir[] + worktreePath: string + dirLoadTracker: FileExplorerDirLoadTracker + setDirCache: Dispatch>> + readDirectory: (dirPath: string) => Promise + maxConcurrentReads: number + /** Called per dir whose fresh listing was committed, so callers can clear a staleness mark. */ + onDirCommitted?: (dirPath: string) => void +} + +export async function refreshFileExplorerExpandedDirs({ + dirs, + worktreePath, + dirLoadTracker, + setDirCache, + readDirectory, + maxConcurrentReads, + onDirCommitted +}: RefreshFileExplorerExpandedDirsParams): Promise { + if (dirs.length === 0) { + return true + } + + const uniqueDirs = Array.from(new Map(dirs.map((dir) => [dir.dirPath, dir])).values()) + // Why: begin every token before the first read so a concurrent refreshDir or + // worktree reset supersedes dirs still queued in a later wave, exactly as the + // single-batch version did. + const loadTokens = new Map( + uniqueDirs.map((dir) => [dir.dirPath, dirLoadTracker.begin(dir.dirPath)]) + ) + const waveSize = Math.max(1, Math.floor(maxConcurrentReads)) + let committedDirs = 0 + + // Why: mark every dir loading up front, not per wave — FileExplorer's auto-load + // effect re-runs on any `expanded` change and fans out an unbounded loadDir per + // dir that is neither cached nor loading, which would defeat the wave cap. + setDirCache((prev) => { + const next = { ...prev } + for (const { dirPath } of uniqueDirs) { + next[dirPath] = { + children: prev[dirPath]?.children ?? [], + loading: true + } + } + return next + }) + + for (let start = 0; start < uniqueDirs.length; start += waveSize) { + // Why: a dir superseded while an earlier wave was reading is left to its + // newer load — reading it here would only burn a round trip for a result + // that is dropped at commit time. + const wave = uniqueDirs + .slice(start, start + waveSize) + .filter(({ dirPath }) => dirLoadTracker.isCurrent(loadTokens.get(dirPath)!)) + if (wave.length === 0) { + continue + } + + // Why: batch each wave's results into one setDirCache write, so the whole + // refresh costs 1 + ceil(N / waveSize) cache spreads rather than one per dir. + const results = await Promise.all( + wave.map(async ({ dirPath, depth }) => { + const loadToken = loadTokens.get(dirPath)! + try { + const listing = await readDirectory(dirPath) + if (!dirLoadTracker.isCurrent(loadToken)) { + return { current: false as const } + } + return { + current: true as const, + dirPath, + cache: { + children: fileExplorerEntriesToTreeNodes( + listing.entries, + dirPath, + depth, + worktreePath, + listing.operationOwner + ), + loading: false, + operationOwner: listing.operationOwner + } + } + } catch { + if (!dirLoadTracker.isCurrent(loadToken)) { + return { current: false as const } + } + return { + current: true as const, + dirPath, + cache: { children: [], loading: false } + } + } + }) + ) + + // Why: the batch commits only after the slowest read in the wave, so a dir + // can be superseded (watcher refreshDir, worktree reset) after its own read + // resolved. Re-check tokens at commit time so the batched write never + // clobbers a newer load — preserving the old per-dir commit ordering. + const currentResults = results.filter( + (result): result is Extract => + result.current && dirLoadTracker.isCurrent(loadTokens.get(result.dirPath)!) + ) + if (currentResults.length === 0) { + continue + } + + setDirCache((prev) => { + const next = { ...prev } + for (const result of currentResults) { + next[result.dirPath] = result.cache + } + return next + }) + for (const result of currentResults) { + onDirCommitted?.(result.dirPath) + } + committedDirs += currentResults.length + } + + return committedDirs === uniqueDirs.length +} diff --git a/src/renderer/src/components/right-sidebar/file-explorer-refresh-concurrency.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-refresh-concurrency.test.ts new file mode 100644 index 00000000000..b1d319933b7 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-refresh-concurrency.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from 'vitest' +import { fileExplorerRefreshConcurrency } from './file-explorer-refresh-concurrency' + +describe('fileExplorerRefreshConcurrency', () => { + it('allows the widest fan-out for local disk reads', () => { + expect(fileExplorerRefreshConcurrency({ kind: 'local' })).toBe(16) + }) + + it('caps SSH-owned worktrees at the remote tier', () => { + expect(fileExplorerRefreshConcurrency({ kind: 'ssh', connectionId: 'target-1' })).toBe(4) + }) + + it('treats an SSH-backed runtime as remote', () => { + expect( + fileExplorerRefreshConcurrency({ + kind: 'runtime', + environmentId: 'env-1', + executionHostId: 'ssh:target-1' + }) + ).toBe(4) + }) + + it('uses the runtime tier for a non-SSH runtime host', () => { + expect( + fileExplorerRefreshConcurrency({ + kind: 'runtime', + environmentId: 'env-1', + executionHostId: 'runtime:env-1' + }) + ).toBe(8) + }) + + it('falls back to the conservative remote tier when ownership is unresolved', () => { + expect(fileExplorerRefreshConcurrency({ kind: 'unresolved' })).toBe(4) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-refresh-concurrency.ts b/src/renderer/src/components/right-sidebar/file-explorer-refresh-concurrency.ts new file mode 100644 index 00000000000..49c6170ad6b --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-refresh-concurrency.ts @@ -0,0 +1,31 @@ +import { parseExecutionHostId } from '../../../../shared/execution-host' +import type { FileExplorerOperationOwner } from './file-explorer-types' + +export const FILE_EXPLORER_LOCAL_REFRESH_CONCURRENCY = 16 +export const FILE_EXPLORER_RUNTIME_REFRESH_CONCURRENCY = 8 +export const FILE_EXPLORER_REMOTE_REFRESH_CONCURRENCY = 4 + +/** + * In-flight directory-read cap for one File Explorer refresh, by how expensive a + * read is on the owning transport. + * + * Why: tiers come from the owner, not from `getFileExplorerOperationRoute`, + * which maps every non-SSH runtime host to `expectedExecutionHostId: 'local'` — + * including an `orca serve` runtime reached over a network socket, whose reads + * are RPC round trips rather than disk reads. + */ +export function fileExplorerRefreshConcurrency(owner: FileExplorerOperationOwner): number { + switch (owner.kind) { + case 'local': + return FILE_EXPLORER_LOCAL_REFRESH_CONCURRENCY + case 'ssh': + return FILE_EXPLORER_REMOTE_REFRESH_CONCURRENCY + case 'runtime': + return parseExecutionHostId(owner.executionHostId)?.kind === 'ssh' + ? FILE_EXPLORER_REMOTE_REFRESH_CONCURRENCY + : FILE_EXPLORER_RUNTIME_REFRESH_CONCURRENCY + case 'unresolved': + // Why: unknown ownership could be the slowest transport; stay conservative. + return FILE_EXPLORER_REMOTE_REFRESH_CONCURRENCY + } +} diff --git a/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.test.ts new file mode 100644 index 00000000000..e0cf1c1eccf --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from 'vitest' +import type { DirCache, TreeNode } from './file-explorer-types' +import { collectStaleDirCachePaths, decideExpandedDirLoad } from './file-explorer-stale-dir-cache' + +function cache(...dirPaths: string[]): Record { + return Object.fromEntries(dirPaths.map((dirPath) => [dirPath, { children: [], loading: false }])) +} + +describe('collectStaleDirCachePaths', () => { + it('marks a cached dir that a full refresh will not re-read', () => { + expect( + collectStaleDirCachePaths(cache('/repo', '/repo/src', '/repo/docs'), '/repo', new Set()) + ).toEqual(['/repo/src', '/repo/docs']) + }) + + it('never marks the root or a currently expanded dir', () => { + expect( + collectStaleDirCachePaths( + cache('/repo', '/repo/src', '/repo/docs'), + '/repo', + new Set(['/repo/src']) + ) + ).toEqual(['/repo/docs']) + }) + + it('returns nothing when the cache holds only refreshed dirs', () => { + expect( + collectStaleDirCachePaths(cache('/repo', '/repo/src'), '/repo', new Set(['/repo/src'])) + ).toEqual([]) + }) +}) + +describe('decideExpandedDirLoad', () => { + const children = [{ name: 'gone.ts', path: '/repo/src/gone.ts' } as TreeNode] + + it('re-reads a cached dir whose listing the last full refresh skipped', () => { + expect(decideExpandedDirLoad({ children, loading: false }, true)).toBe('reload') + }) + + it('trusts a cached listing that is not stale', () => { + expect(decideExpandedDirLoad({ children, loading: false }, false)).toBe('skip') + }) + + it('reads a dir that has never been listed', () => { + expect(decideExpandedDirLoad(undefined, false)).toBe('load') + expect(decideExpandedDirLoad({ children: [], loading: false }, false)).toBe('load') + }) + + it('never stacks a read on one already in flight, stale or not', () => { + expect(decideExpandedDirLoad({ children, loading: true }, true)).toBe('skip') + expect(decideExpandedDirLoad({ children: [], loading: true }, false)).toBe('skip') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.ts b/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.ts new file mode 100644 index 00000000000..007e18960c8 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.ts @@ -0,0 +1,34 @@ +import type { DirCache } from './file-explorer-types' + +/** + * Cached directories a full tree refresh does not re-read. + * + * Why: `refreshTree` reads the root plus the currently expanded dirs, but `toggleDir` drops a dir + * from `expandedDirs` without purging `dirCache`. After a watcher overflow discarded the precise + * events, those collapsed-but-cached listings are unverified and must be re-read on re-expansion — + * otherwise a file deleted while the dir was collapsed stays visible forever, since the expansion + * effect skips any dir that already has children. + */ +export function collectStaleDirCachePaths( + cache: Record, + worktreePath: string, + expanded: Set +): string[] { + return Object.keys(cache).filter((dirPath) => dirPath !== worktreePath && !expanded.has(dirPath)) +} + +export type ExpandedDirLoadDecision = 'skip' | 'load' | 'reload' + +/** What the expansion effect owes a newly expanded dir: nothing, a first read, or a forced re-read. */ +export function decideExpandedDirLoad( + cached: DirCache | undefined, + stale: boolean +): ExpandedDirLoadDecision { + if (cached?.loading) { + return 'skip' + } + if (!cached?.children.length) { + return 'load' + } + return stale ? 'reload' : 'skip' +} diff --git a/src/renderer/src/components/right-sidebar/file-explorer-types.ts b/src/renderer/src/components/right-sidebar/file-explorer-types.ts index 7cf4bbb96d3..8fa8a432544 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-types.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-types.ts @@ -22,3 +22,12 @@ export type DirCache = { loading: boolean operationOwner?: FileExplorerOperationOwner } + +/** + * How a full tree refresh ended. + * + * Why three states, not a boolean: a caller that dropped its own pending dir refreshes because the + * tree refresh covered them must re-issue on `superseded` (someone newer owns those reads) but NOT + * on `root-unreadable` — there the transport is down, and re-issuing buys one dead timeout per dir. + */ +export type FileExplorerTreeRefreshOutcome = 'refreshed' | 'superseded' | 'root-unreadable' diff --git a/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.test.ts index d62420f7915..c6d221b4fad 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.test.ts @@ -439,3 +439,32 @@ describe('processFileExplorerFsPayload update reconciliation', () => { expect(remainingExpanded).toEqual(new Set()) }) }) + +describe('processFileExplorerFsPayload overflow reconciliation', () => { + it('routes an overflow event to a single tree refresh and skips per-dir refreshes', () => { + const root = '/repo' + const refreshDir = vi.fn() + const refreshTree = vi.fn() + processFileExplorerFsPayload({ + payload: { + worktreePath: root, + events: [ + { kind: 'create', absolutePath: `${root}/a.txt` }, + { kind: 'overflow', absolutePath: root }, + { kind: 'create', absolutePath: `${root}/src/b.txt` } + ] + }, + currentWorktreePath: root, + worktreeId: 'wt-1', + cache: { [root]: cacheWithChildren([]), [`${root}/src`]: cacheWithChildren([]) }, + expanded: new Set([`${root}/src`]), + setDirCache: vi.fn(), + setSelectedPath: vi.fn(), + refreshDir, + refreshTree + }) + + expect(refreshTree).toHaveBeenCalledOnce() + expect(refreshDir).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-watch-refresh-scheduler.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-watch-refresh-scheduler.test.ts new file mode 100644 index 00000000000..376479eb388 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-watch-refresh-scheduler.test.ts @@ -0,0 +1,382 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { createFileExplorerWatchRefreshScheduler } from './file-explorer-watch-refresh-scheduler' +import type { FileExplorerTreeRefreshOutcome } from './file-explorer-types' + +const TRAILING_MS = 150 +const MAX_WAIT_MS = 500 + +function createDeferred(): { + promise: Promise + resolve: (outcome?: FileExplorerTreeRefreshOutcome) => void +} { + let resolve!: (outcome?: FileExplorerTreeRefreshOutcome) => void + const promise = new Promise((res) => { + resolve = (outcome = 'refreshed') => res(outcome) + }) + return { promise, resolve } +} + +function setup( + overrides: Partial[0]> = {} +) { + const { refreshTree: treeImpl, refreshDir: dirImpl, ...rest } = overrides + const refreshTree = vi.fn( + treeImpl ?? (async (): Promise => 'refreshed') + ) + const refreshDir = vi.fn(dirImpl ?? (async (_dirPath: string) => {})) + const scheduler = createFileExplorerWatchRefreshScheduler({ + refreshTree, + refreshDir, + isCoveredByFullRefresh: () => false, + dirConcurrency: 4, + ...rest + }) + return { refreshTree, refreshDir, scheduler } +} + +describe('createFileExplorerWatchRefreshScheduler', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('coalesces a burst of full-refresh requests into one tree refresh', async () => { + const { refreshTree, scheduler } = setup() + + for (let index = 0; index < 20; index++) { + scheduler.requestFullRefresh() + } + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshTree).toHaveBeenCalledTimes(1) + }) + + it('flushes at the max wait even when requests keep arriving inside the trailing window', async () => { + const { refreshTree, scheduler } = setup() + + scheduler.requestFullRefresh() + for (let index = 0; index < 4; index++) { + await vi.advanceTimersByTimeAsync(100) + scheduler.requestFullRefresh() + } + expect(refreshTree).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(MAX_WAIT_MS - 400) + + expect(refreshTree).toHaveBeenCalledTimes(1) + }) + + it('dedupes dir refreshes that differ only in path spelling', async () => { + const { refreshDir, scheduler } = setup() + + // Spellings only the runtime-path normalizer folds: Windows separators and a + // trailing slash. Raw-string keying would issue three reads of one dir. + scheduler.requestDirRefresh('C:/repo/src/') + scheduler.requestDirRefresh('C:\\repo\\src') + scheduler.requestDirRefresh('C:/repo/src') + scheduler.requestDirRefresh('/repo/docs/') + scheduler.requestDirRefresh('/repo/docs') + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshDir).toHaveBeenCalledTimes(2) + expect(refreshDir.mock.calls.map(([dirPath]) => dirPath)).toEqual(['C:/repo/src', '/repo/docs']) + }) + + it('still refreshes a pending dir that a full refresh does not cover', async () => { + // Regression guard: dirCache keys are a strict superset of expanded dirs, so + // a full refresh must not subsume pending dir refreshes. + const { refreshTree, refreshDir, scheduler } = setup() + + scheduler.requestDirRefresh('/repo/collapsed') + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshTree).toHaveBeenCalledTimes(1) + expect(refreshDir).toHaveBeenCalledTimes(1) + expect(refreshDir).toHaveBeenCalledWith('/repo/collapsed') + }) + + it('skips a pending dir the full refresh already re-read', async () => { + const { refreshTree, refreshDir, scheduler } = setup({ + isCoveredByFullRefresh: (dirPath) => dirPath === '/repo/src' + }) + + scheduler.requestDirRefresh('/repo/src') + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshTree).toHaveBeenCalledTimes(1) + expect(refreshDir).not.toHaveBeenCalled() + }) + + it('re-issues covered dir refreshes when the tree refresh was superseded', async () => { + // Regression guard: refreshTree bails without re-reading the expanded dirs when its root + // load is superseded. Nothing marks an expanded dir stale, so dropping these would leave + // them stale until a manual refresh — collapse/re-expand does not heal it. + const gate = createDeferred() + const { refreshDir, scheduler } = setup({ + refreshTree: () => gate.promise, + isCoveredByFullRefresh: (dirPath) => dirPath === '/repo/src' + }) + + scheduler.requestDirRefresh('/repo/src') + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + gate.resolve('superseded') + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshDir).toHaveBeenCalledTimes(1) + expect(refreshDir).toHaveBeenCalledWith('/repo/src') + }) + + it('drops pending dir refreshes when the tree refresh could not read the root', async () => { + // Regression guard: a failed root read means the transport is down, so re-issuing buys one + // dead timeout per dir and holds inFlight open, blocking every later refresh. + const gate = createDeferred() + const { refreshDir, scheduler } = setup({ + refreshTree: () => gate.promise, + isCoveredByFullRefresh: () => false + }) + + scheduler.requestDirRefresh('/repo/src') + scheduler.requestDirRefresh('/repo/docs') + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + gate.resolve('root-unreadable') + await vi.advanceTimersByTimeAsync(MAX_WAIT_MS) + + expect(refreshDir).not.toHaveBeenCalled() + }) + + it('refreshes a covered dir when no full refresh is pending', async () => { + const { refreshTree, refreshDir, scheduler } = setup({ + isCoveredByFullRefresh: () => true + }) + + scheduler.requestDirRefresh('/repo/src') + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshTree).not.toHaveBeenCalled() + expect(refreshDir).toHaveBeenCalledTimes(1) + }) + + it('does not start a second run while the first is still in flight', async () => { + const gate = createDeferred() + const { refreshTree, scheduler } = setup({ refreshTree: () => gate.promise }) + + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + expect(refreshTree).toHaveBeenCalledTimes(1) + + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(MAX_WAIT_MS) + expect(refreshTree).toHaveBeenCalledTimes(1) + + gate.resolve() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshTree).toHaveBeenCalledTimes(2) + }) + + it('flushes a dir request that arrived during an in-flight run', async () => { + const gate = createDeferred() + const { refreshDir, scheduler } = setup({ refreshTree: () => gate.promise }) + + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + scheduler.requestDirRefresh('/repo/late') + await vi.advanceTimersByTimeAsync(MAX_WAIT_MS) + expect(refreshDir).not.toHaveBeenCalled() + + gate.resolve() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshDir).toHaveBeenCalledTimes(1) + expect(refreshDir).toHaveBeenCalledWith('/repo/late') + }) + + it('never runs more dir refreshes at once than dirConcurrency', async () => { + let inFlight = 0 + let peakInFlight = 0 + const refreshDir = vi.fn(async (_dirPath: string) => { + inFlight++ + peakInFlight = Math.max(peakInFlight, inFlight) + await new Promise((resolve) => setTimeout(resolve, 10)) + inFlight-- + }) + const { scheduler } = setup({ refreshDir, dirConcurrency: 4 }) + + for (let index = 0; index < 20; index++) { + scheduler.requestDirRefresh(`/repo/d${index}`) + } + await vi.advanceTimersByTimeAsync(TRAILING_MS + 20 * 10) + + expect(refreshDir).toHaveBeenCalledTimes(20) + expect(peakInFlight).toBeLessThanOrEqual(4) + }) + + it('cancels a pending flush', async () => { + const { refreshTree, refreshDir, scheduler } = setup() + + scheduler.requestFullRefresh() + scheduler.requestDirRefresh('/repo/src') + scheduler.cancel() + await vi.advanceTimersByTimeAsync(MAX_WAIT_MS) + + expect(refreshTree).not.toHaveBeenCalled() + expect(refreshDir).not.toHaveBeenCalled() + }) + + it('stops refreshing queued dirs when cancelled mid-run', async () => { + // Regression guard: refreshDir is bound to a live ref that React reassigns + // before the effect cleanup runs, so a queued path surviving cancel() would + // be read against the NEXT worktree's binding. + const gate = createDeferred() + const refreshDir = vi.fn(async (dirPath: string) => { + if (dirPath === '/repo/d0') { + await gate.promise + } + }) + const { scheduler } = setup({ refreshDir, dirConcurrency: 1 }) + + for (let index = 0; index < 4; index++) { + scheduler.requestDirRefresh(`/repo/d${index}`) + } + await vi.advanceTimersByTimeAsync(TRAILING_MS) + expect(refreshDir).toHaveBeenCalledTimes(1) + + scheduler.cancel() + gate.resolve() + await vi.advanceTimersByTimeAsync(MAX_WAIT_MS) + + expect(refreshDir).toHaveBeenCalledTimes(1) + }) + + it('still refreshes a dir first expanded while the tree refresh was in flight', async () => { + const gate = createDeferred() + const expanded = new Set() + const { refreshDir, scheduler } = setup({ + refreshTree: () => gate.promise, + isCoveredByFullRefresh: (dirPath) => expanded.has(dirPath) + }) + + scheduler.requestDirRefresh('/repo/late') + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + // refreshTree already ran against the old expanded set, so this dir was + // never re-read and its pending refresh must survive. + expanded.add('/repo/late') + gate.resolve() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(refreshDir).toHaveBeenCalledTimes(1) + expect(refreshDir).toHaveBeenCalledWith('/repo/late') + }) + + it('drops a dir queued during an in-flight run when cancel lands first', async () => { + const gate = createDeferred() + const { refreshTree, refreshDir, scheduler } = setup({ refreshTree: () => gate.promise }) + + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + scheduler.requestDirRefresh('/repo/src') + scheduler.cancel() + + gate.resolve() + await vi.advanceTimersByTimeAsync(MAX_WAIT_MS) + + expect(refreshTree).toHaveBeenCalledTimes(1) + expect(refreshDir).not.toHaveBeenCalled() + }) + + it('uses the injected timer functions', async () => { + const schedule = vi.fn((callback: () => void, delayMs: number) => setTimeout(callback, delayMs)) + const clear = vi.fn((timer: ReturnType) => { + clearTimeout(timer) + }) + const { refreshTree, scheduler } = setup({ schedule, clear }) + + scheduler.requestFullRefresh() + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(schedule).toHaveBeenCalledTimes(2) + expect(clear).toHaveBeenCalledTimes(1) + expect(refreshTree).toHaveBeenCalledTimes(1) + }) + + it('keeps a full refresh ahead of the dir refreshes it does not cover', async () => { + const order: string[] = [] + const { scheduler } = setup({ + refreshTree: async (): Promise => { + order.push('tree') + return 'refreshed' + }, + refreshDir: async (dirPath) => { + order.push(dirPath) + } + }) + + scheduler.requestDirRefresh('/repo/collapsed') + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(order).toEqual(['tree', '/repo/collapsed']) + }) + + // Why: local workspaces pass a zero window because the main process already coalesced the burst + // on the same 150/500 timings. Stacking a second debounce there only doubled paint latency. + describe('zero window (local transport)', () => { + const zeroWindow = { trailingMs: 0, maxWaitMs: 0 } + + it('flushes on the next tick instead of waiting out a trailing window', async () => { + const { refreshTree, scheduler } = setup(zeroWindow) + + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(0) + + expect(refreshTree).toHaveBeenCalledTimes(1) + }) + + it('still coalesces one payload burst into a single refresh', async () => { + const { refreshTree, refreshDir, scheduler } = setup(zeroWindow) + + for (let index = 0; index < 20; index++) { + scheduler.requestFullRefresh() + scheduler.requestDirRefresh('/repo/src') + } + await vi.advanceTimersByTimeAsync(0) + + expect(refreshTree).toHaveBeenCalledTimes(1) + expect(refreshDir).toHaveBeenCalledTimes(1) + expect(refreshDir).toHaveBeenCalledWith('/repo/src') + }) + + it('does not start a second run while one is in flight', async () => { + const deferred = createDeferred() + const { refreshTree, scheduler } = setup({ + ...zeroWindow, + refreshTree: () => deferred.promise + }) + + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(0) + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(0) + + expect(refreshTree).toHaveBeenCalledTimes(1) + + deferred.resolve() + await vi.advanceTimersByTimeAsync(0) + + expect(refreshTree).toHaveBeenCalledTimes(2) + }) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-watch-refresh-scheduler.ts b/src/renderer/src/components/right-sidebar/file-explorer-watch-refresh-scheduler.ts new file mode 100644 index 00000000000..246ead05519 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-watch-refresh-scheduler.ts @@ -0,0 +1,152 @@ +import type { FileExplorerTreeRefreshOutcome } from './file-explorer-types' +import { normalizeRuntimePathForComparison } from '../../../../shared/cross-platform-path' +import { + WATCH_BATCH_MAX_WAIT_MS, + WATCH_BATCH_TRAILING_MS +} from '../../../../shared/filesystem-watch-batch-window' +import { forEachWithConcurrency } from '../../../../shared/map-with-concurrency' + +type SchedulerTimer = ReturnType + +export type FileExplorerWatchRefreshScheduler = { + requestFullRefresh: () => void + requestDirRefresh: (dirPath: string) => void + cancel: () => void +} + +export type CreateFileExplorerWatchRefreshSchedulerParams = { + refreshTree: () => Promise + refreshDir: (dirPath: string) => Promise + /** Dirs a full tree refresh already re-reads; the scheduler has no view of `expanded`. */ + isCoveredByFullRefresh: (dirPath: string) => boolean + dirConcurrency: number + trailingMs?: number + maxWaitMs?: number + schedule?: (callback: () => void, delayMs: number) => SchedulerTimer + clear?: (timer: SchedulerTimer) => void +} + +/** + * Coalesces watcher-driven File Explorer refreshes into one debounced run. + * + * Why: an overflow storm otherwise issues K × (1 + N_expanded) readDirectory + * round trips on the same SSH mux that carries PTY output. Transport-agnostic on + * purpose — it sits above both the Electron fs:changed bus and the runtime-RPC + * file-change subscription. + */ +export function createFileExplorerWatchRefreshScheduler({ + refreshTree, + refreshDir, + isCoveredByFullRefresh, + dirConcurrency, + trailingMs = WATCH_BATCH_TRAILING_MS, + maxWaitMs = WATCH_BATCH_MAX_WAIT_MS, + schedule = setTimeout, + clear = clearTimeout +}: CreateFileExplorerWatchRefreshSchedulerParams): FileExplorerWatchRefreshScheduler { + let pendingFull = false + const pendingDirs = new Map() + let firstRequestAt: number | null = null + let timer: SchedulerTimer | null = null + let inFlight: Promise | null = null + let disposed = false + + function clearTimer(): void { + if (timer !== null) { + clear(timer) + timer = null + } + } + + function arm(delayMs: number): void { + clearTimer() + timer = schedule(() => { + timer = null + // Why: a run started here would race the in-flight one; the settle + // handler re-arms instead, so nothing pending is dropped. + if (inFlight === null) { + run() + } + }, delayMs) + } + + function armForRequest(): void { + firstRequestAt ??= Date.now() + arm(Math.max(0, Math.min(trailingMs, firstRequestAt + maxWaitMs - Date.now()))) + } + + function run(): void { + const full = pendingFull + const dirs = Array.from(pendingDirs.values()) + pendingFull = false + pendingDirs.clear() + firstRequestAt = null + + const started = (async () => { + // Why: isCoveredByFullRefresh reads a live `expanded` ref, so resolve it + // against the set refreshTree is about to read — a dir expanded during the + // tree refresh was never re-read and must keep its pending refresh. + const covered = full ? new Set(dirs.filter(isCoveredByFullRefresh)) : null + const outcome = full && !disposed ? await refreshTree() : null + // Why: refreshTree bails without touching the expanded dirs when its root load is + // superseded, so trusting `covered` there would drop those refreshes for good — nothing + // marks an expanded dir stale, so a later re-expansion won't re-read it either. + // Why: 'root-unreadable' is the opposite case. The transport just failed a read, so + // re-issuing buys one dead timeout per dir and holds `inFlight` open the whole time, + // blocking every later refresh. Drop them; a recovered watcher resyncs with an overflow. + const outstanding = + outcome === 'root-unreadable' + ? [] + : covered && outcome === 'refreshed' + ? dirs.filter((dirPath) => !covered.has(dirPath)) + : dirs + // Why: forEachWithConcurrency has no cancel hook, and the bound refreshDir + // is a live ref — a queued path would otherwise be read against the next + // worktree's binding after a switch. + await forEachWithConcurrency(outstanding, dirConcurrency, (dirPath) => + disposed ? Promise.resolve() : refreshDir(dirPath) + ) + })() + inFlight = started + void started + .catch(() => { + // Refresh failures are transient; the next watcher event retries. + }) + .finally(() => { + if (inFlight === started) { + inFlight = null + } + if (!disposed && (pendingFull || pendingDirs.size > 0)) { + arm(trailingMs) + } + }) + } + + return { + requestFullRefresh: () => { + if (disposed) { + return + } + // Why: pendingDirs is deliberately NOT cleared. toggleDir drops a dir from + // expandedDirs without purging dirCache, so cache keys are a strict + // superset of what refreshTree re-reads; subsuming them would strand a + // stale listing for a collapsed-but-cached dir. + pendingFull = true + armForRequest() + }, + requestDirRefresh: (dirPath: string) => { + if (disposed) { + return + } + pendingDirs.set(normalizeRuntimePathForComparison(dirPath), dirPath) + armForRequest() + }, + cancel: () => { + disposed = true + clearTimer() + pendingFull = false + pendingDirs.clear() + firstRequestAt = null + } + } +} diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerManualRefresh.ts b/src/renderer/src/components/right-sidebar/useFileExplorerManualRefresh.ts index 36fc1d24caf..b5c48de77da 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerManualRefresh.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerManualRefresh.ts @@ -2,7 +2,7 @@ import { useCallback, useEffect, useRef, useState } from 'react' const REFRESH_SPINNER_DELAY_MS = 200 -export function useFileExplorerManualRefresh(refreshTree: () => Promise): { +export function useFileExplorerManualRefresh(refreshTree: () => Promise): { isRefreshing: boolean showRefreshSpinner: boolean handleRefresh: () => void diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerTree.stale-dirs.test.tsx b/src/renderer/src/components/right-sidebar/useFileExplorerTree.stale-dirs.test.tsx new file mode 100644 index 00000000000..9c665f4cce6 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/useFileExplorerTree.stale-dirs.test.tsx @@ -0,0 +1,155 @@ +// @vitest-environment happy-dom + +import { act, cleanup, renderHook } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { DirEntry } from '../../../../shared/types' +import { useFileExplorerTree } from './useFileExplorerTree' + +const readDirectoryMock = vi.hoisted(() => vi.fn()) +vi.mock('./file-explorer-directory-listing', async (importOriginal) => ({ + ...(await importOriginal>()), + readFileExplorerDirectory: readDirectoryMock +})) +vi.mock('./file-explorer-operation-owner', async (importOriginal) => ({ + ...(await importOriginal>()), + getFileExplorerOperationOwner: () => ({ kind: 'local' as const }) +})) + +function entry(name: string, isDirectory = false): DirEntry { + return { name, isDirectory } as DirEntry +} + +function listing(...entries: DirEntry[]) { + return { entries, operationOwner: { kind: 'local' as const } } +} + +describe('useFileExplorerTree stale collapsed dirs', () => { + beforeEach(() => { + readDirectoryMock.mockReset() + readDirectoryMock.mockResolvedValue(listing()) + }) + + afterEach(() => { + cleanup() + }) + + it('marks a collapsed cached dir the full refresh skipped, and clears it on the next read', async () => { + const { result } = renderHook(() => useFileExplorerTree('/repo', new Set(), 'wt-1')) + + readDirectoryMock.mockResolvedValueOnce(listing(entry('gone.ts'))) + await act(async () => { + await result.current.loadDir('/repo/src', 0) + }) + expect(result.current.dirCache['/repo/src'].children).toHaveLength(1) + expect(result.current.isDirStale('/repo/src')).toBe(false) + + // The overflow refresh reads root and the (empty) expanded set — not the collapsed /repo/src. + readDirectoryMock.mockClear() + await act(async () => { + await result.current.refreshTree() + }) + expect(readDirectoryMock).not.toHaveBeenCalledWith('wt-1', '/repo', '/repo/src') + expect(result.current.isDirStale('/repo/src')).toBe(true) + // Its cached children stay put until something re-reads them. + expect(result.current.dirCache['/repo/src'].children).toHaveLength(1) + + await act(async () => { + await result.current.loadDir('/repo/src', 0, { force: true }) + }) + expect(result.current.isDirStale('/repo/src')).toBe(false) + expect(result.current.dirCache['/repo/src'].children).toHaveLength(0) + }) + + it('does not mark the root or a dir the full refresh re-read', async () => { + const expanded = new Set(['/repo/src']) + const { result } = renderHook(() => useFileExplorerTree('/repo', expanded, 'wt-1')) + + await act(async () => { + await result.current.loadDir('/repo/src', 0) + }) + await act(async () => { + await result.current.refreshTree() + }) + + expect(result.current.isDirStale('/repo')).toBe(false) + expect(result.current.isDirStale('/repo/src')).toBe(false) + }) + + it('keeps a mark a superseded refresh never verified, even once the dir is expanded', async () => { + // Regression guard: refreshTree used to REPLACE the mark set. A dir marked while collapsed and + // then expanded was dropped by the replacement (expanded dirs are never marked) — and when that + // refresh bailed on a superseded root load, nothing re-read it and nothing still called it + // unverified, so re-expanding served the pre-overflow listing forever. + const expanded = new Set() + const { result, rerender } = renderHook(() => useFileExplorerTree('/repo', expanded, 'wt-1')) + + readDirectoryMock.mockResolvedValueOnce(listing(entry('gone.ts'))) + await act(async () => { + await result.current.loadDir('/repo/src', 0) + }) + await act(async () => { + await result.current.refreshTree() + }) + expect(result.current.isDirStale('/repo/src')).toBe(true) + + expanded.add('/repo/src') + rerender() + + let releaseRoot!: () => void + const rootGate = new Promise((resolve) => { + releaseRoot = resolve + }) + readDirectoryMock.mockImplementationOnce(async () => { + await rootGate + return listing() + }) + + let refresh!: Promise + act(() => { + refresh = result.current.refreshTree() + }) + await act(async () => { + // A concurrent root read supersedes this refresh's token, so it bails before the expanded set. + void result.current.refreshDir('/repo') + releaseRoot() + expect(await refresh).toBe('superseded') + }) + + expect(result.current.isDirStale('/repo/src')).toBe(true) + }) + + it('does not read the expanded dirs when the root read failed', async () => { + // Regression guard: loadDir swallows read errors unless failOnError is set, so a dead + // transport used to report a completed root read and fan out one doomed wave per 4 dirs. + const expanded = new Set(['/repo/a', '/repo/b']) + const { result } = renderHook(() => useFileExplorerTree('/repo', expanded, 'wt-1')) + + readDirectoryMock.mockReset() + readDirectoryMock.mockRejectedValue(new Error('ETIMEDOUT')) + + await act(async () => { + expect(await result.current.refreshTree()).toBe('root-unreadable') + }) + + // Only the root read was attempted; the expanded dirs were not. + expect(readDirectoryMock).toHaveBeenCalledTimes(1) + expect(readDirectoryMock).toHaveBeenCalledWith('wt-1', '/repo', '/repo') + }) + + it('drops stale marks when the tree is reset for a new worktree', async () => { + const { result } = renderHook(() => useFileExplorerTree('/repo', new Set(), 'wt-1')) + + await act(async () => { + await result.current.loadDir('/repo/src', 0) + }) + await act(async () => { + await result.current.refreshTree() + }) + expect(result.current.isDirStale('/repo/src')).toBe(true) + + await act(async () => { + result.current.resetAndLoad() + }) + expect(result.current.isDirStale('/repo/src')).toBe(false) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts b/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts index 591d9fd4d9f..d6f695c2ac3 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts @@ -1,12 +1,9 @@ import type { Dispatch, SetStateAction } from 'react' import { useCallback, useRef, useState } from 'react' -import type { DirCache } from './file-explorer-types' +import type { DirCache, FileExplorerTreeRefreshOutcome } from './file-explorer-types' import { splitPathSegments } from './path-tree' import { statRuntimePath } from '@/runtime/runtime-file-client' -import { - createFileExplorerDirLoadTracker, - type FileExplorerDirLoadTracker -} from './file-explorer-dir-load-tracker' +import { createFileExplorerDirLoadTracker } from './file-explorer-dir-load-tracker' import { getFileExplorerOperationOwner, getFileExplorerOwnerUnresolvedMessage, @@ -14,9 +11,11 @@ import { } from './file-explorer-operation-owner' import { fileExplorerEntriesToTreeNodes, - readFileExplorerDirectory, - type FileExplorerDirectoryListing + readFileExplorerDirectory } from './file-explorer-directory-listing' +import { refreshFileExplorerExpandedDirs } from './file-explorer-expanded-dirs-refresh' +import { collectStaleDirCachePaths } from './file-explorer-stale-dir-cache' +import { fileExplorerRefreshConcurrency } from './file-explorer-refresh-concurrency' type UseFileExplorerTreeResult = { dirCache: Record @@ -30,113 +29,13 @@ type UseFileExplorerTreeResult = { ) => Promise statPath: (path: string) => Promise<{ isDirectory: boolean }> markPathAsDirectory: (path: string) => void - refreshTree: () => Promise + refreshTree: () => Promise refreshDir: (dirPath: string) => Promise + /** True while a dir's cached listing predates the last full refresh that skipped it. */ + isDirStale: (dirPath: string) => boolean resetAndLoad: () => void } -type RefreshFileExplorerTreeDir = { - dirPath: string - depth: number -} - -type RefreshFileExplorerExpandedDirsParams = { - dirs: RefreshFileExplorerTreeDir[] - worktreePath: string - dirLoadTracker: FileExplorerDirLoadTracker - setDirCache: Dispatch>> - readDirectory: (dirPath: string) => Promise -} - -export async function refreshFileExplorerExpandedDirs({ - dirs, - worktreePath, - dirLoadTracker, - setDirCache, - readDirectory -}: RefreshFileExplorerExpandedDirsParams): Promise { - if (dirs.length === 0) { - return true - } - - const uniqueDirs = Array.from(new Map(dirs.map((dir) => [dir.dirPath, dir])).values()) - const loadTokens = new Map( - uniqueDirs.map((dir) => [dir.dirPath, dirLoadTracker.begin(dir.dirPath)]) - ) - - // Why: expanded refresh can touch many directories; commit the loading and - // result states in two batched setDirCache writes (rather than per-directory) - // so refreshing large worktrees stays O(N) instead of O(N²) cache spreads. - setDirCache((prev) => { - const next = { ...prev } - for (const { dirPath } of uniqueDirs) { - next[dirPath] = { - children: prev[dirPath]?.children ?? [], - loading: true - } - } - return next - }) - - const results = await Promise.all( - uniqueDirs.map(async ({ dirPath, depth }) => { - const loadToken = loadTokens.get(dirPath)! - try { - const listing = await readDirectory(dirPath) - if (!dirLoadTracker.isCurrent(loadToken)) { - return { current: false as const } - } - return { - current: true as const, - dirPath, - cache: { - children: fileExplorerEntriesToTreeNodes( - listing.entries, - dirPath, - depth, - worktreePath, - listing.operationOwner - ), - loading: false, - operationOwner: listing.operationOwner - } - } - } catch { - if (!dirLoadTracker.isCurrent(loadToken)) { - return { current: false as const } - } - return { - current: true as const, - dirPath, - cache: { children: [], loading: false } - } - } - }) - ) - - // Why: the batch commits only after the slowest read, so a dir can be - // superseded (watcher refreshDir, worktree reset) after its own read - // resolved. Re-check tokens at commit time so the batched write never - // clobbers a newer load — preserving the old per-dir commit ordering. - const currentResults = results.filter( - (result): result is Extract => - result.current && dirLoadTracker.isCurrent(loadTokens.get(result.dirPath)!) - ) - if (currentResults.length === 0) { - return false - } - - setDirCache((prev) => { - const next = { ...prev } - for (const result of currentResults) { - next[result.dirPath] = result.cache - } - return next - }) - - return currentResults.length === uniqueDirs.length -} - export function useFileExplorerTree( worktreePath: string | null, expanded: Set, @@ -147,6 +46,11 @@ export function useFileExplorerTree( const dirCacheRef = useRef(dirCache) dirCacheRef.current = dirCache const dirLoadTrackerRef = useRef(createFileExplorerDirLoadTracker()) + // Why: a ref, not state — the expansion effect must read the mark set by a refresh that landed + // after the effect's render, and a state write would only be visible one render too late. + const staleDirsRef = useRef(new Set()) + // Why: separates a failed root read from a superseded one — loadDir returns false for both. + const rootReadFailedRef = useRef(false) const loadDir = useCallback( async ( @@ -159,6 +63,8 @@ export function useFileExplorerTree( return true } const loadToken = dirLoadTrackerRef.current.begin(dirPath) + // Why: this read starts after the refresh that marked the dir, so its result is current. + staleDirsRef.current.delete(dirPath) // Why: when force-reloading a directory (e.g. after a file is created, // duplicated, or deleted), keep the previous children visible while the // fresh listing loads. Clearing to [] would momentarily shrink the @@ -200,6 +106,7 @@ export function useFileExplorerTree( // empty worktree. Preserve the message so the UI can distinguish // "no files" from "could not read this worktree". setRootError(error instanceof Error ? error.message : String(error)) + rootReadFailedRef.current = true } setDirCache((prev) => ({ ...prev, [dirPath]: { children: [], loading: false } })) return !options?.failOnError @@ -248,17 +155,42 @@ export function useFileExplorerTree( [activeWorktreeId, worktreePath] ) - const refreshTree = useCallback(async () => { + const refreshTree = useCallback(async (): Promise => { if (!worktreePath) { - return + // Why: not 'root-unreadable' — no read was attempted, and that outcome tells callers to DROP + // their pending refreshes. Report the refresh as not-done so they keep them instead. + return 'superseded' } // Why: clearing the entire dirCache here would momentarily empty the // visible projection and jump the virtualizer to the top. Instead we rely // on force-reload keeping existing children visible until fresh data lands. + // Why: mark before the first read, against the live expanded set — a dir expanded after this + // point loads through the expansion effect, which would otherwise trust a listing this refresh + // never re-read. + // Why: union, not replace. This refresh can bail below without re-reading a single expanded + // dir, so replacing would erase an earlier mark for a dir nothing has verified since. Marks are + // cleared only where a read actually lands: loadDir, and onDirCommitted per committed wave. + // Why: prune first — a mark only means anything while the dir still has a cached listing to + // distrust, and unioning without this would accumulate every path visited in the session. + for (const dirPath of staleDirsRef.current) { + if (dirCacheRef.current[dirPath] === undefined) { + staleDirsRef.current.delete(dirPath) + } + } + // Why: callers use the latest refreshTree identity, so this closure has the live expanded set. + for (const dirPath of collectStaleDirCachePaths(dirCacheRef.current, worktreePath, expanded)) { + staleDirsRef.current.add(dirPath) + } const refreshSession = dirLoadTrackerRef.current.getSession() - const rootLoadCompleted = await loadDir(worktreePath, -1, { force: true }) + rootReadFailedRef.current = false + // Why: failOnError, else a dead transport reports a completed root read and we fan out one + // doomed wave per 4 expanded dirs — 200 dirs is ~50 sequential 15s timeouts, and the watch + // scheduler cannot start another refresh for that entire window. + const rootLoadCompleted = await loadDir(worktreePath, -1, { force: true, failOnError: true }) if (!rootLoadCompleted || !dirLoadTrackerRef.current.isSessionCurrent(refreshSession)) { - return + // Why: the expanded dirs below were never re-read either way, but the two reasons want + // opposite handling from callers — see FileExplorerTreeRefreshOutcome. + return rootReadFailedRef.current ? 'root-unreadable' : 'superseded' } // Why: root (worktreePath) was just force-loaded above; exclude it here so // refreshFileExplorerExpandedDirs doesn't queue a duplicate read of root. @@ -268,13 +200,19 @@ export function useFileExplorerTree( dirPath, depth: splitPathSegments(dirPath.slice(worktreePath.length + 1)).length - 1 })) - await refreshFileExplorerExpandedDirs({ + const allDirsCommitted = await refreshFileExplorerExpandedDirs({ dirs: expandedDirs, worktreePath, dirLoadTracker: dirLoadTrackerRef.current, setDirCache, - readDirectory: (dirPath) => readFileExplorerDirectory(activeWorktreeId, worktreePath, dirPath) + readDirectory: (dirPath) => + readFileExplorerDirectory(activeWorktreeId, worktreePath, dirPath), + maxConcurrentReads: fileExplorerRefreshConcurrency( + getFileExplorerOperationOwner(activeWorktreeId) + ), + onDirCommitted: (dirPath) => staleDirsRef.current.delete(dirPath) }) + return allDirsCommitted ? 'refreshed' : 'superseded' }, [activeWorktreeId, expanded, loadDir, worktreePath]) const refreshDir = useCallback( @@ -291,12 +229,15 @@ export function useFileExplorerTree( [worktreePath, loadDir] ) + const isDirStale = useCallback((dirPath: string) => staleDirsRef.current.has(dirPath), []) + const rootCache = worktreePath ? dirCache[worktreePath] : undefined const resetAndLoad = useCallback(() => { // Why: stale readDir responses from the previous worktree/reset session // must not repopulate the explorer after the tree has been cleared. dirLoadTrackerRef.current.reset() + staleDirsRef.current.clear() setDirCache({}) setRootError(null) if (worktreePath) { @@ -314,6 +255,7 @@ export function useFileExplorerTree( markPathAsDirectory, refreshTree, refreshDir, + isDirStale, resetAndLoad } } diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.debounce.test.tsx b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.debounce.test.tsx index b6dd71c8a05..00ffeee274d 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.debounce.test.tsx +++ b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.debounce.test.tsx @@ -22,24 +22,28 @@ function useProjection(query: string) { }) } -function useTreeProjection() { +function treeDirCache(loading: boolean) { + return { + '/repo': { + children: [ + { + name: 'src', + path: '/repo/src', + relativePath: 'src', + isDirectory: true, + depth: 0 + } + ], + loading + } + } +} + +function useTreeProjection(dirCache = treeDirCache(false)) { return useFileExplorerVisibleRowProjection( 'worktree-1', '/repo', - { - '/repo': { - children: [ - { - name: 'src', - path: '/repo/src', - relativePath: 'src', - isDirectory: true, - depth: 0 - } - ], - loading: false - } - }, + dirCache, new Set(), true, true, @@ -95,4 +99,19 @@ describe('file explorer ignored-path query debounce', () => { expect(getRuntimeGitIgnoredPathsMock).toHaveBeenCalledTimes(1) expect(getRuntimeGitIgnoredPathsMock.mock.calls[0]?.[1]).toEqual(['src']) }) + + it('does not re-issue the ignored check for a dirCache commit that changes no path', () => { + // A wave-batched tree refresh commits a fresh dirCache object per wave. The + // ignored query is an uncancellable remote git check-ignore over the whole + // visible tree, so identical contents must not re-issue it. + const hook = renderHook(({ dirCache }) => useTreeProjection(dirCache), { + initialProps: { dirCache: treeDirCache(false) } + }) + expect(getRuntimeGitIgnoredPathsMock).toHaveBeenCalledTimes(1) + + hook.rerender({ dirCache: treeDirCache(true) }) + hook.rerender({ dirCache: treeDirCache(false) }) + + expect(getRuntimeGitIgnoredPathsMock).toHaveBeenCalledTimes(1) + }) }) diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.ts b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.ts index 813555af1c5..06a5e110ad4 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.ts @@ -109,6 +109,28 @@ export function createVisibleFileExplorerRowProjection( return createFileExplorerRowProjectionFromParts(visibleFlatRows, rowsByPath) } +/** + * Holds the array identity while its contents are unchanged. + * + * Why: a tree refresh commits dirCache once per read wave, and every commit + * rebuilds this list. Each new identity would re-issue the uncancellable git + * check-ignore over the whole visible tree — the remote round trips the wave cap + * exists to bound. + */ +function useContentStableRelativePaths(relativePaths: string[], enabled: boolean): string[] { + // Why: filters need fresh identities per keystroke and must not evict the tree signature. + // Why: NUL cannot occur in paths, so the signature can reconstruct the list losslessly. + const signature = useMemo( + () => (enabled ? relativePaths.join('\u0000') : null), + [enabled, relativePaths] + ) + const stableTreePaths = useMemo( + () => (signature ? signature.split('\u0000') : EMPTY_RELATIVE_PATHS), + [signature] + ) + return enabled ? stableTreePaths : relativePaths +} + export function useFileExplorerVisibleRowProjection( activeWorktreeId: string | null, worktreePath: string | null, @@ -128,7 +150,7 @@ export function useFileExplorerVisibleRowProjection( const settings = useAppStore((s) => s.settings) const updateSettings = useAppStore((s) => s.updateSettings) const showGitIgnoredFiles = settings?.showGitIgnoredFiles ?? true - const relativePaths = useMemo( + const rebuiltRelativePaths = useMemo( () => activeRepoSupportsGit ? nameFilter @@ -140,6 +162,9 @@ export function useFileExplorerVisibleRowProjection( : EMPTY_RELATIVE_PATHS, [activeRepoSupportsGit, dirCache, expanded, nameFilter, showDotfiles, worktreePath] ) + // Why: the name-filter list is debounced per keystroke, so it must keep a fresh + // identity; only the dirCache-derived list needs stability across wave commits. + const relativePaths = useContentStableRelativePaths(rebuiltRelativePaths, !nameFilter) const canLoadIgnoredPaths = activeRepoSupportsGit && Boolean(activeWorktreeId) && diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts index be089be1666..c75dfd4fac8 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts @@ -1,13 +1,21 @@ import { useEffect, useRef, type Dispatch, type SetStateAction } from 'react' import type { FsChangedPayload } from '../../../../shared/types' -import type { DirCache, FileExplorerOperationOwner } from './file-explorer-types' +import type { + DirCache, + FileExplorerOperationOwner, + FileExplorerTreeRefreshOutcome +} from './file-explorer-types' import type { InlineInput } from './FileExplorerRow' import { useAppStore } from '@/store' import { subscribeRuntimeFileChanges } from '@/runtime/runtime-file-client' +import { normalizeRuntimePathForComparison } from '../../../../shared/cross-platform-path' import { + getFileExplorerOperationOwner, getFileExplorerOperationOwnerFromState, type FileExplorerOwnerState } from './file-explorer-operation-owner' +import { fileExplorerRefreshConcurrency } from './file-explorer-refresh-concurrency' +import { createFileExplorerWatchRefreshScheduler } from './file-explorer-watch-refresh-scheduler' import { processFileExplorerFsPayload } from './file-explorer-watch-reconcile' export { @@ -30,7 +38,7 @@ type UseFileExplorerWatchParams = { expanded: Set setSelectedPath: Dispatch> refreshDir: (dirPath: string) => Promise - refreshTree: () => Promise + refreshTree: () => Promise inlineInput: InlineInput | null dragSourcePath: string | null isNativeDragOver: boolean @@ -127,6 +135,29 @@ export function useFileExplorerWatch({ const currentWorktreePath = worktreePath + // Why: one scheduler per subscription covers BOTH remote transports (the + // Electron fs:changed bus and the runtime-RPC subscription below), and its + // lifetime is tied to the watched worktree so a switch can't refresh the + // previous root. + const owner = getFileExplorerOperationOwner(worktreeIdRef.current) + const scheduler = createFileExplorerWatchRefreshScheduler({ + refreshTree: () => refreshTreeRef.current(), + refreshDir: (dirPath) => refreshDirRef.current(dirPath), + // Why exact-match on `expanded`, not a normalized compare: dirPath already arrives as a + // resolved dirCache key (resolveCachedDirPath), and refreshTree re-reads dirCache under the + // verbatim expanded strings — a differently-spelled key it never wrote would be reported + // covered and left stale. Only the root is normalized: it is keyed by worktreePath. + isCoveredByFullRefresh: (dirPath) => + normalizeRuntimePathForComparison(dirPath) === + normalizeRuntimePathForComparison(currentWorktreePath) || + expandedRef.current.has(dirPath), + dirConcurrency: fileExplorerRefreshConcurrency(owner), + // Why: main already coalesced the burst on this same 150/500 window before it reached the + // fs:changed bus, so a second debounce here only doubles paint latency. Zero still arms a 0ms + // timer, keeping intra-payload dedup and the concurrency cap. + ...(owner.kind === 'local' ? { trailingMs: 0, maxWaitMs: 0 } : {}) + }) + function processPayload(payload: FsChangedPayload): void { const wtId = worktreeIdRef.current if (!wtId) { @@ -140,12 +171,8 @@ export function useFileExplorerWatch({ expanded: expandedRef.current, setDirCache, setSelectedPath, - refreshDir: (dirPath) => { - void refreshDirRef.current(dirPath) - }, - refreshTree: () => { - void refreshTreeRef.current() - } + refreshDir: scheduler.requestDirRefresh, + refreshTree: scheduler.requestFullRefresh }) } @@ -207,6 +234,7 @@ export function useFileExplorerWatch({ return () => { disposed = true unsubscribeListener?.() + scheduler.cancel() deferredRef.current = [] processPayloadRef.current = null } diff --git a/src/shared/filesystem-watch-batch-window.ts b/src/shared/filesystem-watch-batch-window.ts new file mode 100644 index 00000000000..fbbae133b78 --- /dev/null +++ b/src/shared/filesystem-watch-batch-window.ts @@ -0,0 +1,5 @@ +// Why: the main-process watcher batcher and the renderer's Explorer refresh +// scheduler share one flush window so local and remote latency can't drift. + +export const WATCH_BATCH_TRAILING_MS = 150 +export const WATCH_BATCH_MAX_WAIT_MS = 500