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..2432f65a401 100644 --- a/src/main/ipc/filesystem-watcher.ts +++ b/src/main/ipc/filesystem-watcher.ts @@ -16,6 +16,11 @@ import { onSshFilesystemProviderRegistered } from '../providers/ssh-filesystem-dispatch' import { MAX_BATCHED_WATCHER_EVENTS, queueWatcherEvents } from './filesystem-watcher-event-batch' +import { createRemoteWatcherEventBatch } 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 +37,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 +349,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 +361,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 ───────────────────────────────────────────────── @@ -1287,23 +1287,36 @@ async function doInstallRemoteWatcher( cancelToken: RemoteWatcherInstallToken ): Promise { let unwatch: () => void + // Why: only the two abandon paths below close this batch; a timer that outlives any other teardown by + // ≤500ms fires into deliver, fails the installToken check and drops — today's behavior for late events. + 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 + } + listener.send('fs:changed', { + worktreePath, + events + } satisfies FsChangedPayload) + } + } + }) try { unwatch = await provider.watch( worktreePath, (events) => { - const state = remoteWatchers.get(key) - if (!state) { - return - } - for (const listener of state.listeners.values()) { - if (listener.isDestroyed()) { - continue - } - listener.send('fs:changed', { - worktreePath, - events - } satisfies FsChangedPayload) - } + batch.push(events) }, { signal: cancelToken.abortController.signal, @@ -1326,6 +1339,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,6 +1348,7 @@ async function doInstallRemoteWatcher( return 'cancelled' } if (cancelToken.terminalError) { + batch.close() return 'unavailable' } remoteWatchers.set(key, { unwatch, listeners: liveListeners, installToken: cancelToken }) 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..6cf086a2e77 --- /dev/null +++ b/src/main/ipc/remote-watcher-event-batch.test.ts @@ -0,0 +1,258 @@ +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() + }) + + // 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..581d91b9aec --- /dev/null +++ b/src/main/ipc/remote-watcher-event-batch.ts @@ -0,0 +1,142 @@ +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 +} + +// Why: mirrors coalesceEvents in filesystem-watcher.ts — last event wins per path, delete→create emits +// both (delete cleans the subtree, create refreshes the parent), create→delete is a net no-op. Diverges +// deliberately on two points the local path compensates for by stat'ing at flush time: a net delete is +// never cancelled away, and a hoisted delete survives a later event of unknown isDirectory. +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 + + 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) { + 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() { + if (timer) { + clearTimeout(timer) + timer = null + } + buffered = [] + overflowed = false + firstEventAt = 0 + } + } +} diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index c81d2b5a6a1..c0dad2965d3 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -3316,6 +3316,154 @@ 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('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..619b5eeb86c 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,9 @@ export class PtyHandler { } } + // Why: dead path — startGraceTimer's only caller (relay.ts) always passes an explicit timeoutMs, + // so this value never reaches the grace timer and only serves as an unused default parameter. + // Full caller chain: docs/reference/relay-grace-time-reconfiguration.md. setGraceTimeMs(graceTimeMs: number): void { this.graceTimeMs = Math.max(0, Math.floor(graceTimeMs)) } @@ -494,6 +499,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 +669,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 +726,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 +1211,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) { @@ -1535,7 +1592,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 +2122,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 +2162,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.ts b/src/relay/relay.ts index f282cbb483e..534f7ddd598 100644 --- a/src/relay/relay.ts +++ b/src/relay/relay.ts @@ -68,6 +68,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 @@ -829,6 +832,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: 'startup-empty-detached' | 'idle-no-ptys' | 'configured' | null = null dispatcher.onRequest('relay.status', async () => ({ pid: process.pid, @@ -864,6 +870,7 @@ async function main(): Promise { } graceDeadlineAt = null graceReason = null + graceBranch = null ptyHandler.cancelGraceTimer() } @@ -1120,22 +1127,46 @@ async function main(): Promise { 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 + // 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 = isRelayIdle() && graceTimeMs === 0 + graceBranch = startupEmptyDetached + ? 'startup-empty-detached' + : idleNoPtys + ? 'idle-no-ptys' + : 'configured' + const timeoutMs = + graceBranch === 'startup-empty-detached' + ? graceTimeMs === 0 + ? EMPTY_DETACHED_STARTUP_GRACE_MS + : Math.min(graceTimeMs, EMPTY_DETACHED_STARTUP_GRACE_MS) + : graceBranch === 'idle-no-ptys' + ? IDLE_RELAY_GRACE_MS + : graceTimeMs 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 +1207,12 @@ async function main(): Promise { ) graceDeadlineAt = null graceReason = null + graceBranch = null void ptyHandler .dispose() .then(() => { + stopPoolWatch() + stopPoolActiveWatch() dispatcher.dispose() fsHandler.dispose() gitHandler.dispose() @@ -1196,9 +1230,35 @@ async function main(): Promise { 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') + } }) } + // 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..cafdbe60743 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts @@ -0,0 +1,131 @@ +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 +} + +export async function refreshFileExplorerExpandedDirs({ + dirs, + worktreePath, + dirLoadTracker, + setDirCache, + readDirectory, + maxConcurrentReads +}: 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 + }) + 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-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..0bb26d22f9e --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-watch-refresh-scheduler.test.ts @@ -0,0 +1,285 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { createFileExplorerWatchRefreshScheduler } from './file-explorer-watch-refresh-scheduler' + +const TRAILING_MS = 150 +const MAX_WAIT_MS = 500 + +function createDeferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void + const promise = new Promise((res) => { + resolve = res + }) + return { promise, resolve } +} + +function setup( + overrides: Partial[0]> = {} +) { + const { refreshTree: treeImpl, refreshDir: dirImpl, ...rest } = overrides + const refreshTree = vi.fn(treeImpl ?? (async () => {})) + 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('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 () => { + order.push('tree') + }, + refreshDir: async (dirPath) => { + order.push(dirPath) + } + }) + + scheduler.requestDirRefresh('/repo/collapsed') + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + + expect(order).toEqual(['tree', '/repo/collapsed']) + }) +}) 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..bcc10206be3 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-watch-refresh-scheduler.ts @@ -0,0 +1,141 @@ +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 outstanding = full ? dirs.filter((dirPath) => !isCoveredByFullRefresh(dirPath)) : dirs + if (full && !disposed) { + await refreshTree() + } + // 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/useFileExplorerTree.stale-dirs.test.tsx b/src/renderer/src/components/right-sidebar/useFileExplorerTree.stale-dirs.test.tsx new file mode 100644 index 00000000000..221a0b74943 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/useFileExplorerTree.stale-dirs.test.tsx @@ -0,0 +1,94 @@ +// @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('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..a03ce83b7ec 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts @@ -3,10 +3,7 @@ import { useCallback, useRef, useState } from 'react' import type { DirCache } 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 @@ -32,111 +31,11 @@ type UseFileExplorerTreeResult = { markPathAsDirectory: (path: string) => void 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()) + const expandedRef = useRef(expanded) + expandedRef.current = expanded 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 @@ -255,6 +161,12 @@ export function useFileExplorerTree( // 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. Replacing (not merging) also clears dirs this refresh does cover. + staleDirsRef.current = new Set( + collectStaleDirCachePaths(dirCacheRef.current, worktreePath, expandedRef.current) + ) const refreshSession = dirLoadTrackerRef.current.getSession() const rootLoadCompleted = await loadDir(worktreePath, -1, { force: true }) if (!rootLoadCompleted || !dirLoadTrackerRef.current.isSessionCurrent(refreshSession)) { @@ -273,7 +185,11 @@ export function useFileExplorerTree( worktreePath, dirLoadTracker: dirLoadTrackerRef.current, setDirCache, - readDirectory: (dirPath) => readFileExplorerDirectory(activeWorktreeId, worktreePath, dirPath) + readDirectory: (dirPath) => + readFileExplorerDirectory(activeWorktreeId, worktreePath, dirPath), + maxConcurrentReads: fileExplorerRefreshConcurrency( + getFileExplorerOperationOwner(activeWorktreeId) + ) }) }, [activeWorktreeId, expanded, loadDir, worktreePath]) @@ -291,12 +207,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 +233,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..125aa94d849 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.ts @@ -1,4 +1,4 @@ -import { useCallback, useMemo } from 'react' +import { useCallback, useMemo, useRef } from 'react' import { useAppStore } from '@/store' import { isDotfileRelativePath } from './file-explorer-entries' import type { DirCache, TreeNode } from './file-explorer-types' @@ -109,6 +109,24 @@ 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[]): string[] { + // NUL separator: it is the one byte no path segment can contain. + const signature = relativePaths.join('\u0000') + const cacheRef = useRef<{ signature: string; value: string[] } | null>(null) + if (cacheRef.current === null || cacheRef.current.signature !== signature) { + cacheRef.current = { signature, value: relativePaths } + } + return cacheRef.current.value +} + export function useFileExplorerVisibleRowProjection( activeWorktreeId: string | null, worktreePath: string | null, @@ -128,7 +146,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 +158,10 @@ export function useFileExplorerVisibleRowProjection( : EMPTY_RELATIVE_PATHS, [activeRepoSupportsGit, dirCache, expanded, nameFilter, showDotfiles, worktreePath] ) + const stableTreeRelativePaths = useContentStableRelativePaths(rebuiltRelativePaths) + // 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 = nameFilter ? rebuiltRelativePaths : stableTreeRelativePaths 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..01d5d2271ae 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts @@ -4,10 +4,14 @@ import type { DirCache, FileExplorerOperationOwner } 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 { @@ -127,6 +131,22 @@ 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 scheduler = createFileExplorerWatchRefreshScheduler({ + refreshTree: () => refreshTreeRef.current(), + refreshDir: (dirPath) => refreshDirRef.current(dirPath), + isCoveredByFullRefresh: (dirPath) => + normalizeRuntimePathForComparison(dirPath) === + normalizeRuntimePathForComparison(currentWorktreePath) || + expandedRef.current.has(dirPath), + dirConcurrency: fileExplorerRefreshConcurrency( + getFileExplorerOperationOwner(worktreeIdRef.current) + ) + }) + function processPayload(payload: FsChangedPayload): void { const wtId = worktreeIdRef.current if (!wtId) { @@ -140,12 +160,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 +223,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