From 6eece0b41eb192e85d051dfef2327aa51872559c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:38:20 -0700 Subject: [PATCH] fix(scm): keep git-status pacing across scheduler rebuilds --- .../git-status-refresh-scheduler.test.ts | 58 ++++++++++++++++++- .../git-status-refresh-scheduler.ts | 28 ++++++--- .../useGitStatusPolling.rerender.test.ts | 36 ++++++++++++ .../right-sidebar/useGitStatusPolling.ts | 16 ++++- 4 files changed, 128 insertions(+), 10 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..1f433100722 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,56 @@ 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('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..f1cf63c5bfc 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,18 @@ 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 +} + +export function createGitStatusRefreshPacing(): GitStatusRefreshPacing { + return { lastRunEndedAt: -Infinity, lastRunDurationMs: 0 } +} + export type GitStatusRefreshScheduler = { resumeSafety: () => void pause: () => void @@ -27,6 +39,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 +50,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 +68,7 @@ export function createGitStatusRefreshScheduler( const requiredActivityIdleMs = (): number => slowTaskRequiredIdleMs( - lastRunDurationMs, + pacing.lastRunDurationMs, options.slowTaskBackoff.changeSignalMultiplier, options.activityMinGapMs, options.slowTaskBackoff.maxIntervalMs @@ -68,7 +80,7 @@ export function createGitStatusRefreshScheduler( const delay = Math.max( options.safetyIntervalMs, slowTaskRequiredIdleMs( - lastRunDurationMs, + pacing.lastRunDurationMs, options.slowTaskBackoff.idleMultiplier, 0, options.slowTaskBackoff.maxIntervalMs @@ -91,7 +103,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 @@ -134,12 +146,14 @@ export function createGitStatusRefreshScheduler( // Status refresh errors are transient; the next signal or safety run retries. }) .finally(() => { - lastRunEndedAt = Date.now() + 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. - lastRunDurationMs = controller.signal.aborted ? 0 : Math.max(0, lastRunEndedAt - startedAt) + 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..85918410356 100644 --- a/src/renderer/src/components/right-sidebar/useGitStatusPolling.rerender.test.ts +++ b/src/renderer/src/components/right-sidebar/useGitStatusPolling.rerender.test.ts @@ -241,6 +241,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..75d19d00c4b 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,19 @@ export function useGitStatusPolling(options: { enabled?: boolean } = {}): void { const statusRefreshGenerationRef = useRef(0) const statusSchedulerRef = useRef(null) + // Why: pacing is keyed by worktree, not by scheduler instance — the rebuilds + // below (execution-host/push-target changes) must not reset it, or a flapping + // host id lets sustained change signals run git at the bare debounce. + const statusPacingByWorktreeRef = useRef | null>(null) useEffect(() => { const generation = ++statusRefreshGenerationRef.current + const pacingByWorktree = (statusPacingByWorktreeRef.current ??= new Map()) + const pacingKey = `${activeWorktreeId}\0${worktreePath}` + let pacing = pacingByWorktree.get(pacingKey) + if (!pacing) { + pacing = createGitStatusRefreshPacing() + pacingByWorktree.set(pacingKey, pacing) + } const scheduler = createGitStatusRefreshScheduler( ({ reason, signal }) => runFetchStatusRef.current({ @@ -179,7 +192,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