From f1e183864b74c5dfe1f15a5bf096737eb158ebdf Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 1 Aug 2026 14:27:20 -0700 Subject: [PATCH] fix(watcher/explorer): resilient commit batches on callback error Cache writes precede callbacks, so a throwing callback cannot strand the batch with stale marks. All callbacks complete despite errors, with the first error thrown after. - Fix commitBatchSize calculation for empty dirs - Add scheduler discard-report test - Move disposed variable before handleFsChanged - Expand batching-strategy comments --- src/relay/relay-grace-branch.test.ts | 4 +- ...ile-explorer-expanded-dirs-refresh.test.ts | 38 +++++++++++++++++++ .../file-explorer-expanded-dirs-refresh.ts | 22 +++++++++-- ...e-explorer-watch-refresh-scheduler.test.ts | 15 ++++++++ .../right-sidebar/useFileExplorerWatch.ts | 9 ++++- 5 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/relay/relay-grace-branch.test.ts b/src/relay/relay-grace-branch.test.ts index 6882d463cf4..580ab7e179c 100644 --- a/src/relay/relay-grace-branch.test.ts +++ b/src/relay/relay-grace-branch.test.ts @@ -69,7 +69,9 @@ describe('decideRelayGrace', () => { }) it('preserves shutdown-deferred when grace is reconfigured to zero', () => { - const current = decide({ retryDeferredShutdown: true }) + // Why a non-zero starting grace: configureRelayGraceTime only re-arms on an actual change, + // so the reconfiguration this models must cross a real boundary, not 0 → 0. + const current = decide({ configuredGraceMs: 10_000, retryDeferredShutdown: true }) expect( decide({ diff --git a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts index 906b4d4a298..f1faa368e44 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts @@ -375,6 +375,44 @@ describe('refreshFileExplorerExpandedDirs', () => { }) }) + it('still notifies the rest of a commit batch after one commit callback throws', async () => { + let cache: Record = {} + const setDirCache = vi.fn((update: CacheUpdate) => { + cache = typeof update === 'function' ? update(cache) : update + }) + const commitError = new Error('commit failed') + const onDirCommitted = vi.fn((dirPath: string) => { + if (dirPath === '/repo/a') { + throw commitError + } + }) + + await expect( + refreshFileExplorerExpandedDirs({ + dirs: [ + { dirPath: '/repo/a', depth: 0 }, + { dirPath: '/repo/b', depth: 0 } + ], + worktreePath: '/repo', + dirLoadTracker: createFileExplorerDirLoadTracker(), + setDirCache, + readDirectory: async () => ({ + entries: [entry('index.ts')], + operationOwner: { kind: 'local' as const } + }), + // Both dirs land in one commit batch, so the throw must not strand the other's mark. + maxConcurrentReads: 2, + onDirCommitted + }) + ).rejects.toBe(commitError) + + expect(onDirCommitted.mock.calls.map(([dirPath]) => dirPath).sort()).toEqual([ + '/repo/a', + '/repo/b' + ]) + expect(cache['/repo/b']).toMatchObject({ loading: false, children: [{ name: 'index.ts' }] }) + }) + it('drops a queued directory superseded while an earlier read is blocked', async () => { const tracker = createFileExplorerDirLoadTracker() let cache: Record = {} 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 index 085cb8530c9..06c5347b4f7 100644 --- 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 @@ -44,7 +44,7 @@ export async function refreshFileExplorerExpandedDirs({ ) const commitBatchSize = maxConcurrentReads === Number.POSITIVE_INFINITY - ? uniqueDirs.length + ? Math.max(1, uniqueDirs.length) : Number.isFinite(maxConcurrentReads) ? Math.max(1, Math.floor(maxConcurrentReads)) : 1 @@ -82,10 +82,24 @@ export async function refreshFileExplorerExpandedDirs({ } return next }) - for (const result of currentResults) { - onDirCommitted?.(result.dirPath) - } committedDirs += currentResults.length + // Why: the cache write above already landed for every result, so a throwing callback must not + // strand the rest of the batch with a staleness mark no later commit will clear. + let firstCommitError: unknown + let commitFailed = false + for (const result of currentResults) { + try { + onDirCommitted?.(result.dirPath) + } catch (error) { + if (!commitFailed) { + commitFailed = true + firstCommitError = error + } + } + } + if (commitFailed) { + throw firstCommitError + } } const settleRead = (result?: { dirPath: string; cache: DirCache }): void => { 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 index 51236ce7ac7..fc515608394 100644 --- 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 @@ -239,6 +239,21 @@ describe('createFileExplorerWatchRefreshScheduler', () => { expect(scheduler.cancel()).toBe(false) }) + it('reports discarded work when cancelled with a refresh in flight and nothing queued', async () => { + const gate = createDeferred() + const { refreshTree, scheduler } = setup({ refreshTree: () => gate.promise }) + + scheduler.requestFullRefresh() + await vi.advanceTimersByTimeAsync(TRAILING_MS) + expect(refreshTree).toHaveBeenCalledTimes(1) + + // Nothing pending and no timer armed: the in-flight run is the only discarded work. + expect(scheduler.cancel()).toBe(true) + + gate.resolve() + await vi.advanceTimersByTimeAsync(MAX_WAIT_MS) + }) + 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 diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts index 232d3bd3a02..0d9c42af68e 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts @@ -158,7 +158,10 @@ export function useFileExplorerWatch({ normalizeRuntimePathForComparison(currentWorktreePath) || expandedRef.current.has(dirPath), dirConcurrency: fileExplorerRefreshConcurrency(owner), - // Every producer batches before delivery; a 0ms turn keeps renderer dedup without another 150ms. + // Why 0ms for every transport: each producer already flushes on the shared + // WATCH_BATCH_* window — local/SSH via filesystem-watcher, runtime files.watch via + // createFileWatchEventBatcher — so a second 150ms here only adds latency, not coalescing. + // A new transport must batch at the producer before joining this path. trailingMs: 0, maxWaitMs: 0 }) @@ -188,6 +191,9 @@ export function useFileExplorerWatch({ // Why: expose processPayload to the flush effect so it can replay deferred payloads without re-subscribing. processPayloadRef.current = processPayload + // Declared before handleFsChanged so its `disposed` read can never hit the temporal dead zone. + let disposed = false + const handleFsChanged = (payload: FsChangedPayload): void => { if (disposed) { if ( @@ -216,7 +222,6 @@ export function useFileExplorerWatch({ processPayload(payload) } - let disposed = false let unsubscribeListener: (() => void) | null = null if (activeRuntimeEnvironmentId?.trim() && activeWorktreeId) { // Why: remote runtime watch events don't enter the local Electron fs:changed bus, so subscribe directly.