From 751b6b119bf99428db3ddd21bda8932b7d530036 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:13:04 -0700 Subject: [PATCH] fix(scm): keep git-status pacing across scheduler rebuilds (#11820) * fix(scm): keep git-status pacing across scheduler rebuilds * fix(scm): order shared refresh pacing updates --- .../git-status-refresh-scheduler.test.ts | 118 +++++++++++++++++- .../git-status-refresh-scheduler.ts | 47 +++++-- .../useGitStatusPolling.rerender.test.ts | 43 +++++++ .../right-sidebar/useGitStatusPolling.ts | 19 ++- 4 files changed, 213 insertions(+), 14 deletions(-) diff --git a/src/renderer/src/components/right-sidebar/git-status-refresh-scheduler.test.ts b/src/renderer/src/components/right-sidebar/git-status-refresh-scheduler.test.ts index 8421e4b56d6..1e00ac163b5 100644 --- a/src/renderer/src/components/right-sidebar/git-status-refresh-scheduler.test.ts +++ b/src/renderer/src/components/right-sidebar/git-status-refresh-scheduler.test.ts @@ -1,6 +1,8 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { + createGitStatusRefreshPacing, createGitStatusRefreshScheduler, + type GitStatusRefreshPacing, type GitStatusRefreshReason } from './git-status-refresh-scheduler' @@ -19,7 +21,8 @@ async function flushMicrotasks(): Promise { } function createScheduler( - task: (request: { reason: GitStatusRefreshReason; signal: AbortSignal }) => Promise + task: (request: { reason: GitStatusRefreshReason; signal: AbortSignal }) => Promise, + pacing?: GitStatusRefreshPacing ) { return createGitStatusRefreshScheduler(task, { safetyIntervalMs: 60_000, @@ -29,7 +32,8 @@ function createScheduler( idleMultiplier: 5, changeSignalMultiplier: 1, maxIntervalMs: 5 * 60_000 - } + }, + ...(pacing ? { pacing } : {}) }) } @@ -258,6 +262,116 @@ describe('createGitStatusRefreshScheduler', () => { expect(task).toHaveBeenCalledTimes(2) }) + it('keeps the activity floor across scheduler recreation when pacing is shared', async () => { + vi.useFakeTimers() + const task = vi.fn(async () => {}) + const pacing = createGitStatusRefreshPacing() + + const first = createScheduler(task, pacing) + first.resumeSafety() + await flushMicrotasks() + expect(task).toHaveBeenCalledTimes(1) + first.dispose() + + // A rebuild (execution-host/push-target change) must not grant a fresh + // immediate run; the floor from the previous run still applies. + const second = createScheduler(task, pacing) + second.resumeSafety() + await flushMicrotasks() + expect(task).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(2999) + expect(task).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(1) + expect(task).toHaveBeenCalledTimes(2) + }) + + it('keeps slow-scan backoff across scheduler recreation when pacing is shared', async () => { + vi.useFakeTimers() + const calls: ReturnType[] = [] + const task = vi.fn(() => { + const call = deferred() + calls.push(call) + return call.promise + }) + const pacing = createGitStatusRefreshPacing() + + const first = createScheduler(task, pacing) + first.resumeSafety() + await vi.advanceTimersByTimeAsync(30_000) + calls[0]?.resolve() + await flushMicrotasks() + first.dispose() + + const second = createScheduler(task, pacing) + second.resumeSafety() + await flushMicrotasks() + // The 30s scan duration still paces the next run: max(3s floor, 1x 30s). + await vi.advanceTimersByTimeAsync(29_999) + expect(task).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(1) + expect(task).toHaveBeenCalledTimes(2) + }) + + it('does not let an older disposed run erase replacement backoff', async () => { + vi.useFakeTimers() + const calls: ReturnType[] = [] + const task = vi.fn(() => { + const call = deferred() + calls.push(call) + return call.promise + }) + const pacing = createGitStatusRefreshPacing() + + const first = createScheduler(task, pacing) + first.resumeSafety() + first.dispose() + + const second = createScheduler(task, pacing) + second.resumeSafety() + await vi.advanceTimersByTimeAsync(30_000) + calls[1]?.resolve() + await flushMicrotasks() + + await vi.advanceTimersByTimeAsync(1) + calls[0]?.resolve() + await flushMicrotasks() + second.signal() + + await vi.advanceTimersByTimeAsync(29_998) + expect(task).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(1) + expect(task).toHaveBeenCalledTimes(3) + }) + + it('lets an aborted old run pace repeated rebuilds until a replacement finishes', async () => { + vi.useFakeTimers() + const calls: ReturnType[] = [] + const task = vi.fn(() => { + const call = deferred() + calls.push(call) + return call.promise + }) + const pacing = createGitStatusRefreshPacing() + + const first = createScheduler(task, pacing) + first.resumeSafety() + first.dispose() + + const second = createScheduler(task, pacing) + second.resumeSafety() + calls[0]?.resolve() + await flushMicrotasks() + second.dispose() + + const third = createScheduler(task, pacing) + third.resumeSafety() + expect(task).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(2999) + expect(task).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(1) + expect(task).toHaveBeenCalledTimes(3) + }) + it('cleans up debounce and safety timers on dispose', async () => { vi.useFakeTimers() const task = vi.fn(async () => {}) diff --git a/src/renderer/src/components/right-sidebar/git-status-refresh-scheduler.ts b/src/renderer/src/components/right-sidebar/git-status-refresh-scheduler.ts index f33a6d7afc7..43459befd47 100644 --- a/src/renderer/src/components/right-sidebar/git-status-refresh-scheduler.ts +++ b/src/renderer/src/components/right-sidebar/git-status-refresh-scheduler.ts @@ -2,6 +2,25 @@ import { slowTaskRequiredIdleMs, type SlowTaskBackoffOptions } from './coalesced export type GitStatusRefreshReason = 'activity' | 'safety' +// Why: run pacing must be shareable across scheduler instances — a rebuild +// (execution-host or push-target change) that resets it lets sustained change +// signals run git at the bare debounce, bypassing the #7983 floor. +export type GitStatusRefreshPacing = { + lastRunEndedAt: number + lastRunDurationMs: number + nextRunId: number + latestFinishedRunId: number +} + +export function createGitStatusRefreshPacing(): GitStatusRefreshPacing { + return { + lastRunEndedAt: -Infinity, + lastRunDurationMs: 0, + nextRunId: 0, + latestFinishedRunId: 0 + } +} + export type GitStatusRefreshScheduler = { resumeSafety: () => void pause: () => void @@ -27,6 +46,7 @@ export function createGitStatusRefreshScheduler( // task backoff when the previous scan itself was slow. activityMinGapMs: number slowTaskBackoff: SlowTaskBackoffOptions + pacing?: GitStatusRefreshPacing } ): GitStatusRefreshScheduler { let disposed = false @@ -37,8 +57,7 @@ export function createGitStatusRefreshScheduler( let activityTimerFiresAt = Infinity let safetyTimer: ReturnType | null = null let activeController: AbortController | null = null - let lastRunEndedAt = -Infinity - let lastRunDurationMs = 0 + const pacing = options.pacing ?? createGitStatusRefreshPacing() const clearActivityTimer = (): void => { if (activityTimer !== null) { @@ -56,7 +75,7 @@ export function createGitStatusRefreshScheduler( const requiredActivityIdleMs = (): number => slowTaskRequiredIdleMs( - lastRunDurationMs, + pacing.lastRunDurationMs, options.slowTaskBackoff.changeSignalMultiplier, options.activityMinGapMs, options.slowTaskBackoff.maxIntervalMs @@ -68,7 +87,7 @@ export function createGitStatusRefreshScheduler( const delay = Math.max( options.safetyIntervalMs, slowTaskRequiredIdleMs( - lastRunDurationMs, + pacing.lastRunDurationMs, options.slowTaskBackoff.idleMultiplier, 0, options.slowTaskBackoff.maxIntervalMs @@ -91,7 +110,7 @@ export function createGitStatusRefreshScheduler( return } const now = Date.now() - const delay = Math.max(minDelayMs, lastRunEndedAt + requiredActivityIdleMs() - now) + const delay = Math.max(minDelayMs, pacing.lastRunEndedAt + requiredActivityIdleMs() - now) if (delay <= 0) { startRun('activity') return @@ -121,6 +140,7 @@ export function createGitStatusRefreshScheduler( clearSafetyTimer() inFlight = true const startedAt = Date.now() + const runId = ++pacing.nextRunId const controller = new AbortController() activeController = controller let result: Promise @@ -134,12 +154,17 @@ export function createGitStatusRefreshScheduler( // Status refresh errors are transient; the next signal or safety run retries. }) .finally(() => { - lastRunEndedAt = Date.now() - // Why: cancelled scans never delivered a useful result, so their wall - // time must not stretch the next activity/safety gap. Otherwise hide → - // reveal after a slow abort waits out the aborted scan's full duration - // before the catch-up refresh can start. - lastRunDurationMs = controller.signal.aborted ? 0 : Math.max(0, lastRunEndedAt - startedAt) + if (runId > pacing.latestFinishedRunId) { + pacing.latestFinishedRunId = runId + pacing.lastRunEndedAt = Date.now() + // Why: cancelled scans never delivered a useful result, so their wall + // time must not stretch the next activity/safety gap. Otherwise hide → + // reveal after a slow abort waits out the aborted scan's full duration + // before the catch-up refresh can start. + pacing.lastRunDurationMs = controller.signal.aborted + ? 0 + : Math.max(0, pacing.lastRunEndedAt - startedAt) + } if (activeController === controller) { activeController = null } diff --git a/src/renderer/src/components/right-sidebar/useGitStatusPolling.rerender.test.ts b/src/renderer/src/components/right-sidebar/useGitStatusPolling.rerender.test.ts index 3fab77c327f..2f9c3ddabdb 100644 --- a/src/renderer/src/components/right-sidebar/useGitStatusPolling.rerender.test.ts +++ b/src/renderer/src/components/right-sidebar/useGitStatusPolling.rerender.test.ts @@ -172,6 +172,13 @@ describe('useGitStatusPolling rerender stability', () => { // Should trigger an immediate poll on the new worktree (total 2 calls) // without having to wait for the 3000ms timer. expect(refreshMock).toHaveBeenCalledTimes(2) + + await act(async () => { + useAppStore.setState({ activeWorktreeId: WORKTREE_ID }) + }) + await flushMicrotasks() + + expect(refreshMock).toHaveBeenCalledTimes(3) }) it('refreshes immediately when Source Control becomes visible', async () => { @@ -241,6 +248,42 @@ describe('useGitStatusPolling rerender stability', () => { expect(refreshMock.mock.calls[1]?.[0].request.reuseLineStats).toBe(true) }) + it('keeps the activity floor when the execution host flaps for the same worktree', async () => { + await renderHook() + await flushMicrotasks() + // Mount refresh settles and stamps pacing for this worktree. + expect(refreshMock).toHaveBeenCalledTimes(1) + + // Flap the resolved execution host back and forth. Each flip rebuilds the + // scheduler; pacing must survive or every flip grants an immediate run + // (the rc.3 storm: sustained git at the debounce floor). + await act(async () => { + useAppStore.setState({ + worktreesByRepo: { + [REPO_ID]: [{ ...worktree, hostId: 'runtime:env-2' }], + [REPO_ID2]: [worktree2] + } + }) + }) + await flushMicrotasks() + await act(async () => { + useAppStore.setState({ + worktreesByRepo: { + [REPO_ID]: [worktree], + [REPO_ID2]: [worktree2] + } + }) + }) + await flushMicrotasks() + expect(refreshMock).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(2999) + expect(refreshMock).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(1) + await flushMicrotasks() + expect(refreshMock).toHaveBeenCalledTimes(2) + }) + it('aborts and rejects stale work when the execution host changes', async () => { let resolveFirst!: () => void const firstRefresh = new Promise((resolve) => { diff --git a/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts b/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts index 8511d796630..d22e6591c41 100644 --- a/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts +++ b/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts @@ -15,7 +15,9 @@ import { useGitStatusFileWatchRefresh } from './git-status-file-watch-refresh' import { useGitStatusPushSignalRefresh } from './git-status-push-signal-refresh' import { useStaleConflictOperationPolling } from './stale-conflict-operation-poll' import { + createGitStatusRefreshPacing, createGitStatusRefreshScheduler, + type GitStatusRefreshPacing, type GitStatusRefreshReason, type GitStatusRefreshScheduler } from './git-status-refresh-scheduler' @@ -162,8 +164,22 @@ export function useGitStatusPolling(options: { enabled?: boolean } = {}): void { const statusRefreshGenerationRef = useRef(0) const statusSchedulerRef = useRef(null) + // Why: pacing belongs to the current worktree, not to scheduler instances — + // execution-host/push-target rebuilds must not reset it, or a flapping host id + // lets sustained change signals run git at the bare debounce. + const statusPacingRef = useRef<{ + key: string + pacing: GitStatusRefreshPacing + } | null>(null) useEffect(() => { const generation = ++statusRefreshGenerationRef.current + const pacingKey = `${activeWorktreeId}\0${worktreePath}` + let pacing = + statusPacingRef.current?.key === pacingKey ? statusPacingRef.current.pacing : undefined + if (!pacing) { + pacing = createGitStatusRefreshPacing() + statusPacingRef.current = { key: pacingKey, pacing } + } const scheduler = createGitStatusRefreshScheduler( ({ reason, signal }) => runFetchStatusRef.current({ @@ -179,7 +195,8 @@ export function useGitStatusPolling(options: { enabled?: boolean } = {}): void { safetyIntervalMs: STATUS_SAFETY_INTERVAL_MS, activityDebounceMs: STATUS_ACTIVITY_DEBOUNCE_MS, activityMinGapMs: STATUS_ACTIVITY_MIN_GAP_MS, - slowTaskBackoff: SLOW_GIT_POLL_BACKOFF + slowTaskBackoff: SLOW_GIT_POLL_BACKOFF, + pacing } ) statusSchedulerRef.current = scheduler