From 48bcdf0130dfb078e283a9ff787e06acce6c8494 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:01:51 -0700 Subject: [PATCH] fix(git): preserve interactive admission invariants --- .../git-exec-admission-lifetime.test.ts | 57 ++++++++- src/main/git/command-runner/git-exec-file.ts | 19 ++- src/main/runtime/orca-runtime-git.test.ts | 63 +--------- .../runtime/runtime-git-sync-commands.test.ts | 119 ++++++++++++++++++ src/main/runtime/runtime-git-sync-commands.ts | 30 ++++- .../use-checks-panel-foreground-effects.tsx | 1 + ...e-checks-panel-git-status-effects.test.tsx | 55 +++++++- .../use-checks-panel-git-status-effects.tsx | 2 +- .../source-control/sync/use-branch-compare.ts | 1 + .../use-worktree-card-lifecycle-effects.ts | 1 + .../lib/window-visibility-interval.test.ts | 20 ++- .../src/lib/window-visibility-interval.ts | 19 +-- .../run-process-termination-failure.test.ts | 5 +- src/shared/child-process/run-process.ts | 4 +- 14 files changed, 291 insertions(+), 105 deletions(-) create mode 100644 src/main/runtime/runtime-git-sync-commands.test.ts diff --git a/src/main/git/command-runner/git-exec-admission-lifetime.test.ts b/src/main/git/command-runner/git-exec-admission-lifetime.test.ts index efd18e32ca9..93c4dbe2de4 100644 --- a/src/main/git/command-runner/git-exec-admission-lifetime.test.ts +++ b/src/main/git/command-runner/git-exec-admission-lifetime.test.ts @@ -130,7 +130,7 @@ describe('git exec admission lifetime', () => { expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) }) - it('releases barrier admission at bounded unverifiable-exit settlement', async () => { + it('retains barrier admission past bounded settlement until termination is observed', async () => { const child = mockChild() spawnMock.mockReturnValue(child) const pending = gitExecFileAsync(['status'], { @@ -145,6 +145,61 @@ describe('git exec admission lifetime', () => { expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(1) await vi.advanceTimersByTimeAsync(10_000) await rejection + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(1) + + child.emit('close', null, 'SIGKILL') + await Promise.resolve() expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) }) + + it('admits interactive fetch before a background fetch can queue on FETCH_HEAD', async () => { + _resetGitAdmissionForTests(new GitAdmissionScheduler({ networkCap: 1, networkHeadroom: 1 })) + const children = new Map() + const callbacks = new Map() + execFileMock.mockImplementation( + (_command: string, args: string[], _options: unknown, callback: ExecCallback) => { + const label = args[1] ?? '' + const child = mockChild() + children.set(label, child) + callbacks.set(label, callback) + return child + } + ) + + const first = gitExecFileAsync(['fetch', 'first'], { + cwd: '/repo', + admissionTier: 'background' + }) + await vi.waitFor(() => expect(callbacks.has('first')).toBe(true)) + const background = gitExecFileAsync(['fetch', 'background'], { + cwd: '/repo', + admissionTier: 'background' + }) + const interactive = gitExecFileAsync(['fetch', 'interactive'], { + cwd: '/repo', + admissionTier: 'interactive' + }) + + await vi.waitFor(() => expect(_gitAdmissionSnapshotForTests().queued).toBe(1)) + expect([...callbacks.keys()]).toEqual(['first']) + + callbacks.get('first')?.(null, '', '') + await expect(first).resolves.toEqual({ stdout: '', stderr: '' }) + await vi.waitFor(() => expect(callbacks.has('interactive')).toBe(true)) + expect([...callbacks.keys()]).toEqual(['first', 'interactive']) + + children.get('first')?.emit('close', 0, null) + await Promise.resolve() + expect(callbacks.has('background')).toBe(false) + + callbacks.get('interactive')?.(null, '', '') + await expect(interactive).resolves.toEqual({ stdout: '', stderr: '' }) + await vi.waitFor(() => expect(callbacks.has('background')).toBe(true)) + expect([...callbacks.keys()]).toEqual(['first', 'interactive', 'background']) + + children.get('interactive')?.emit('close', 0, null) + callbacks.get('background')?.(null, '', '') + await expect(background).resolves.toEqual({ stdout: '', stderr: '' }) + children.get('background')?.emit('close', 0, null) + }) }) diff --git a/src/main/git/command-runner/git-exec-file.ts b/src/main/git/command-runner/git-exec-file.ts index 5d24f2f3d5b..b28e3abc14e 100644 --- a/src/main/git/command-runner/git-exec-file.ts +++ b/src/main/git/command-runner/git-exec-file.ts @@ -104,7 +104,7 @@ async function gitExecFileAsyncUnlocked( ) : execFileCapture(command.binary, command.args, captureOptions) } - try { + const runCapturedCommand = async (): Promise<{ stdout: string; stderr: string }> => { let result: { stdout: string | Buffer; stderr: string | Buffer } try { result = await capture(resolved) @@ -136,6 +136,17 @@ async function gitExecFileAsyncUnlocked( stdout: readCapturedGitString(stdout as string, resolved), stderr: stderr as string } + } + try { + const command = resolveGitFetchHeadCommand(args, options.cwd) + return command.needsLock + ? await runWithGitFetchHeadLock( + command.cwd, + options.signal, + runCapturedCommand, + command.gitDir + ) + : await runCapturedCommand() } finally { const termination = terminationState.current if (termination) { @@ -152,11 +163,7 @@ export function gitExecFileAsync( args: string[], options: GitExecOptions ): Promise<{ stdout: string; stderr: string }> { - const run = () => gitExecFileAsyncUnlocked(args, options) - const command = resolveGitFetchHeadCommand(args, options.cwd) - return command.needsLock - ? runWithGitFetchHeadLock(command.cwd, options.signal, run, command.gitDir) - : run() + return gitExecFileAsyncUnlocked(args, options) } /** diff --git a/src/main/runtime/orca-runtime-git.test.ts b/src/main/runtime/orca-runtime-git.test.ts index 26b993704f2..e4465314c72 100644 --- a/src/main/runtime/orca-runtime-git.test.ts +++ b/src/main/runtime/orca-runtime-git.test.ts @@ -9,8 +9,6 @@ import type * as PullRequestContextModule from '../text-generation/pull-request- import { RuntimeGitCommands, type ResolvedRuntimeGitWorktree } from './orca-runtime-git' const mocks = vi.hoisted(() => ({ - abortMerge: vi.fn(), - abortRebase: vi.fn(), checkoutBranch: vi.fn(), listLocalBranches: vi.fn(), getStagedCommitContext: vi.fn(), @@ -26,8 +24,6 @@ const mocks = vi.hoisted(() => ({ vi.mock('../git/status', async () => ({ ...(await vi.importActual('../git/status')), - abortMerge: mocks.abortMerge, - abortRebase: mocks.abortRebase, getStagedCommitContext: mocks.getStagedCommitContext, getStatus: mocks.getStatus })) @@ -97,8 +93,6 @@ function makeCommands(worktreePath: string): RuntimeGitCommands { describe('RuntimeGitCommands', () => { beforeEach(() => { - mocks.abortMerge.mockReset() - mocks.abortRebase.mockReset() mocks.getStagedCommitContext.mockReset() mocks.getPullRequestDraftContext.mockReset() mocks.generateCommitMessageFromContext.mockReset() @@ -120,17 +114,6 @@ describe('RuntimeGitCommands', () => { } }) - it('aborts a local merge through the resolved worktree', async () => { - const worktreePath = mkdtempSync(join(tmpdir(), 'orca-runtime-git-')) - tempDirs.push(worktreePath) - const commands = makeCommands(worktreePath) - mocks.abortMerge.mockResolvedValue(undefined) - - await expect(commands.abortRuntimeGitMerge('id:wt-1')).resolves.toEqual({ ok: true }) - - expect(mocks.abortMerge).toHaveBeenCalledWith(worktreePath, {}) - }) - // Why: a directory-only ignore rule (`node_modules/`) never matches the shared // symlink, so Git reports it untracked forever. Runtime/CLI status has to tell // getStatus which untracked entries are Orca's own (issue #10451); nothing else @@ -148,6 +131,7 @@ describe('RuntimeGitCommands', () => { await commands.getRuntimeGitStatus('id:wt-1') expect(mocks.getStatus).toHaveBeenCalledWith('/workspace/feature', { + admissionTier: 'interactive', sharedLinkPaths: ['node_modules'] }) }) @@ -170,51 +154,6 @@ describe('RuntimeGitCommands', () => { expect(mocks.getStatus).not.toHaveBeenCalled() }) - it('aborts a remote merge through the SSH git provider', async () => { - const provider = { abortMerge: vi.fn().mockResolvedValue(undefined) } - mocks.getSshGitProvider.mockReturnValue(provider) - const commands = new RuntimeGitCommands({ - resolveRuntimeGitTarget: async () => ({ - worktree: makeWorktree('/remote/repo'), - connectionId: 'conn-1' - }), - getRuntimeSettings: () => ({}) as GlobalSettings - }) - - await expect(commands.abortRuntimeGitMerge('id:wt-1')).resolves.toEqual({ ok: true }) - - expect(provider.abortMerge).toHaveBeenCalledWith('/remote/repo') - expect(mocks.abortMerge).not.toHaveBeenCalled() - }) - - it('aborts a local rebase through the resolved worktree', async () => { - const worktreePath = mkdtempSync(join(tmpdir(), 'orca-runtime-git-')) - tempDirs.push(worktreePath) - const commands = makeCommands(worktreePath) - mocks.abortRebase.mockResolvedValue(undefined) - - await expect(commands.abortRuntimeGitRebase('id:wt-1')).resolves.toEqual({ ok: true }) - - expect(mocks.abortRebase).toHaveBeenCalledWith(worktreePath, {}) - }) - - it('aborts a remote rebase through the SSH git provider', async () => { - const provider = { abortRebase: vi.fn().mockResolvedValue(undefined) } - mocks.getSshGitProvider.mockReturnValue(provider) - const commands = new RuntimeGitCommands({ - resolveRuntimeGitTarget: async () => ({ - worktree: makeWorktree('/remote/repo'), - connectionId: 'conn-1' - }), - getRuntimeSettings: () => ({}) as GlobalSettings - }) - - await expect(commands.abortRuntimeGitRebase('id:wt-1')).resolves.toEqual({ ok: true }) - - expect(provider.abortRebase).toHaveBeenCalledWith('/remote/repo') - expect(mocks.abortRebase).not.toHaveBeenCalled() - }) - it('checks out a local branch through the resolved worktree', async () => { const worktreePath = mkdtempSync(join(tmpdir(), 'orca-runtime-git-')) tempDirs.push(worktreePath) diff --git a/src/main/runtime/runtime-git-sync-commands.test.ts b/src/main/runtime/runtime-git-sync-commands.test.ts new file mode 100644 index 00000000000..5913b710042 --- /dev/null +++ b/src/main/runtime/runtime-git-sync-commands.test.ts @@ -0,0 +1,119 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { GlobalSettings } from '../../shared/global-settings-types' +import type { GitPushTarget } from '../../shared/worktree/types' +import type * as GitRemoteModule from '../git/remote' +import type * as GitStatusModule from '../git/status' +import type { ResolvedRuntimeGitWorktree } from './runtime-git-command-target' +import { RuntimeGitSyncCommands } from './runtime-git-sync-commands' + +const mocks = vi.hoisted(() => ({ + abortMerge: vi.fn(), + abortRebase: vi.fn(), + commitChanges: vi.fn(), + getSshGitProvider: vi.fn(), + gitFastForward: vi.fn(), + gitFetch: vi.fn(), + gitPull: vi.fn() +})) + +vi.mock('../git/status', async () => ({ + ...(await vi.importActual('../git/status')), + abortMerge: mocks.abortMerge, + abortRebase: mocks.abortRebase, + commitChanges: mocks.commitChanges +})) + +vi.mock('../git/remote', async () => ({ + ...(await vi.importActual('../git/remote')), + gitFastForward: mocks.gitFastForward, + gitFetch: mocks.gitFetch, + gitPull: mocks.gitPull +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: mocks.getSshGitProvider +})) + +const worktree = { + id: 'wt-1', + path: '/workspace/repo' +} as ResolvedRuntimeGitWorktree +const pushTarget = { + remoteName: 'origin', + branchName: 'main' +} satisfies GitPushTarget + +describe('RuntimeGitSyncCommands admission', () => { + beforeEach(() => { + vi.resetAllMocks() + }) + + it('prioritizes local runtime git actions and preserves host routing', async () => { + const commands = new RuntimeGitSyncCommands({ + resolveRuntimeGitTarget: async () => ({ + worktree, + localGitOptions: { wslDistro: 'Ubuntu' } + }), + getRuntimeSettings: () => ({}) as GlobalSettings + }) + mocks.commitChanges.mockResolvedValue({ success: true }) + + await commands.abortRuntimeGitMerge('id:wt-1') + await commands.abortRuntimeGitRebase('id:wt-1') + await commands.fetchRuntimeGit('id:wt-1', pushTarget) + await commands.pullRuntimeGit('id:wt-1', pushTarget) + await commands.fastForwardRuntimeGit('id:wt-1', pushTarget) + await commands.commitRuntimeGit('id:wt-1', 'feat: prioritize user action') + + const options = { admissionTier: 'interactive', wslDistro: 'Ubuntu' } + expect(mocks.abortMerge).toHaveBeenCalledWith(worktree.path, options) + expect(mocks.abortRebase).toHaveBeenCalledWith(worktree.path, options) + expect(mocks.gitFetch).toHaveBeenCalledWith(worktree.path, pushTarget, options) + expect(mocks.gitPull).toHaveBeenCalledWith(worktree.path, pushTarget, options) + expect(mocks.gitFastForward).toHaveBeenCalledWith(worktree.path, pushTarget, options) + expect(mocks.commitChanges).toHaveBeenCalledWith( + worktree.path, + 'feat: prioritize user action', + options + ) + }) + + it('keeps remote runtime git actions owned by the SSH provider', async () => { + const provider = { + abortMerge: vi.fn(), + abortRebase: vi.fn(), + commit: vi.fn().mockResolvedValue({ success: true }), + fastForwardBranch: vi.fn(), + fetchRemote: vi.fn(), + pullBranch: vi.fn() + } + mocks.getSshGitProvider.mockReturnValue(provider) + const commands = new RuntimeGitSyncCommands({ + resolveRuntimeGitTarget: async () => ({ + worktree, + connectionId: 'conn-1' + }), + getRuntimeSettings: () => ({}) as GlobalSettings + }) + + await commands.abortRuntimeGitMerge('id:wt-1') + await commands.abortRuntimeGitRebase('id:wt-1') + await commands.fetchRuntimeGit('id:wt-1', pushTarget) + await commands.pullRuntimeGit('id:wt-1', pushTarget) + await commands.fastForwardRuntimeGit('id:wt-1', pushTarget) + await commands.commitRuntimeGit('id:wt-1', 'feat: keep execution remote') + + expect(provider.abortMerge).toHaveBeenCalledWith(worktree.path) + expect(provider.abortRebase).toHaveBeenCalledWith(worktree.path) + expect(provider.fetchRemote).toHaveBeenCalledWith(worktree.path, pushTarget) + expect(provider.pullBranch).toHaveBeenCalledWith(worktree.path, pushTarget) + expect(provider.fastForwardBranch).toHaveBeenCalledWith(worktree.path, pushTarget) + expect(provider.commit).toHaveBeenCalledWith(worktree.path, 'feat: keep execution remote') + expect(mocks.abortMerge).not.toHaveBeenCalled() + expect(mocks.abortRebase).not.toHaveBeenCalled() + expect(mocks.gitFetch).not.toHaveBeenCalled() + expect(mocks.gitPull).not.toHaveBeenCalled() + expect(mocks.gitFastForward).not.toHaveBeenCalled() + expect(mocks.commitChanges).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/runtime/runtime-git-sync-commands.ts b/src/main/runtime/runtime-git-sync-commands.ts index a2aae9aa3f5..8e0b25cfd60 100644 --- a/src/main/runtime/runtime-git-sync-commands.ts +++ b/src/main/runtime/runtime-git-sync-commands.ts @@ -24,7 +24,10 @@ export class RuntimeGitSyncCommands { await provider.abortMerge(target.worktree.path) return { ok: true } } - await abortMerge(target.worktree.path, localGitOptionsForTarget(target)) + await abortMerge(target.worktree.path, { + ...localGitOptionsForTarget(target), + admissionTier: 'interactive' + }) return { ok: true } } @@ -38,7 +41,10 @@ export class RuntimeGitSyncCommands { await provider.abortRebase(target.worktree.path) return { ok: true } } - await abortRebase(target.worktree.path, localGitOptionsForTarget(target)) + await abortRebase(target.worktree.path, { + ...localGitOptionsForTarget(target), + admissionTier: 'interactive' + }) return { ok: true } } @@ -70,7 +76,10 @@ export class RuntimeGitSyncCommands { await provider.fetchRemote(target.worktree.path, pushTarget) return { ok: true } } - await gitFetch(target.worktree.path, pushTarget, localGitOptionsForTarget(target)) + await gitFetch(target.worktree.path, pushTarget, { + ...localGitOptionsForTarget(target), + admissionTier: 'interactive' + }) return { ok: true } } @@ -106,7 +115,10 @@ export class RuntimeGitSyncCommands { await provider.pullBranch(target.worktree.path, pushTarget) return { ok: true } } - await gitPull(target.worktree.path, pushTarget, localGitOptionsForTarget(target)) + await gitPull(target.worktree.path, pushTarget, { + ...localGitOptionsForTarget(target), + admissionTier: 'interactive' + }) return { ok: true } } @@ -123,7 +135,10 @@ export class RuntimeGitSyncCommands { await provider.fastForwardBranch(target.worktree.path, pushTarget) return { ok: true } } - await gitFastForward(target.worktree.path, pushTarget, localGitOptionsForTarget(target)) + await gitFastForward(target.worktree.path, pushTarget, { + ...localGitOptionsForTarget(target), + admissionTier: 'interactive' + }) return { ok: true } } @@ -184,6 +199,9 @@ export class RuntimeGitSyncCommands { } return provider.commit(target.worktree.path, message) } - return commitChanges(target.worktree.path, message, localGitOptionsForTarget(target)) + return commitChanges(target.worktree.path, message, { + ...localGitOptionsForTarget(target), + admissionTier: 'interactive' + }) } } diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-foreground-effects.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-foreground-effects.tsx index a9e2cfe639a..c451dcf4152 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-foreground-effects.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-foreground-effects.tsx @@ -143,6 +143,7 @@ export function useChecksPanelForegroundEffects(model: ChecksPanelForegroundEffe } setGitStatusRefreshNonce((value) => value + 1) }, + jitterOnVisible: true, intervalMs: RUNTIME_SSH_STATUS_REFRESH_MS }) }, [isPanelVisible, repoConnectionId, runtimeEnvironmentId, setGitStatusRefreshNonce]) diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.test.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.test.tsx index 59be405b2b1..0037055328e 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.test.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.test.tsx @@ -21,11 +21,22 @@ const setGitStatusProbeErrorContextKey = vi.fn() const setGitStatusRefreshNonce = vi.fn() const updateWorktreeGitIdentity = vi.fn() -function Probe({ nonce }: { nonce: number }): null { +function Probe({ + nonce, + contextKey = 'context-A', + worktreeId = 'worktree-A', + worktreePath = '/repo' +}: { + nonce: number + contextKey?: string + worktreeId?: string + worktreePath?: string +}): null { + panelContextKeyRef.current = contextKey useChecksPanelGitStatusEffects({ activeConnectionId: null, - activeWorktreeId: 'worktree-A', - activeWorktreePath: '/repo', + activeWorktreeId: worktreeId, + activeWorktreePath: worktreePath, activeWorktreePushTarget: null, branch: 'feature', eligibilityHeadOidRef: { current: null }, @@ -47,7 +58,7 @@ function Probe({ nonce }: { nonce: number }): null { fallbackGitHubPRNumber: null, localExecutionScope: 'host', ownerSettings: null, - panelContextKey: 'context-A', + panelContextKey: contextKey, panelContextKeyRef, remoteStatus: undefined, remoteStatusInvalidation: 0, @@ -123,4 +134,40 @@ describe('useChecksPanelGitStatusEffects poll runner', () => { }) expect(mocks.getRuntimeGitStatus).toHaveBeenCalledTimes(2) }) + + it('does not carry a slow worktree backoff into the next panel context', async () => { + const first = deferred<{ + entries: never[] + head: string + branch: string + upstreamStatus: { hasUpstream: boolean; ahead: number; behind: number } + }>() + const status = { + entries: [], + head: 'head-A', + branch: 'feature', + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 } + } + mocks.getRuntimeGitStatus.mockReturnValueOnce(first.promise).mockResolvedValue(status) + const root: Root = await mountProbe() + expect(mocks.getRuntimeGitStatus).toHaveBeenCalledTimes(1) + + await act(async () => { + await vi.advanceTimersByTimeAsync(10_000) + first.resolve(status) + }) + await flush() + + await act(async () => { + root.render( + + ) + }) + await flush() + + expect(mocks.getRuntimeGitStatus).toHaveBeenCalledTimes(2) + expect(mocks.getRuntimeGitStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ worktreeId: 'worktree-B', worktreePath: '/repo-b' }) + ) + }) }) diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.tsx index d33b266d8fa..282a5d59782 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.tsx @@ -124,7 +124,7 @@ export function useChecksPanelGitStatusEffects(model: ChecksPanelGitStatusEffect gitStatusPollRunnerRef.current = null } } - }, []) + }, [panelContextKey]) useEffect(() => { if ( diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare.ts b/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare.ts index 6762749ee04..00df9951ec3 100644 --- a/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare.ts +++ b/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare.ts @@ -276,6 +276,7 @@ export function useSourceControlBranchCompare({ branchComparePollEnabledRef.current = true const stopInterval = installWindowVisibilityInterval({ run: () => void startBranchCompareRef.current('interval'), + jitterOnVisible: true, intervalMs: BRANCH_REFRESH_INTERVAL_MS }) return () => { diff --git a/src/renderer/src/components/sidebar/use-worktree-card-lifecycle-effects.ts b/src/renderer/src/components/sidebar/use-worktree-card-lifecycle-effects.ts index 77dfa0331c3..971c7bf0408 100644 --- a/src/renderer/src/components/sidebar/use-worktree-card-lifecycle-effects.ts +++ b/src/renderer/src/components/sidebar/use-worktree-card-lifecycle-effects.ts @@ -90,6 +90,7 @@ export function useWorktreeCardLifecycleEffects({ // Why: PRs created outside Orca (e.g. `gh pr create`) emit no renderer event; poll visible cards to discover them. return installWindowVisibilityInterval({ run: refreshHostedReview, + jitterOnVisible: true, intervalMs: HOSTED_REVIEW_CARD_REFRESH_INTERVAL_MS }) }, [ diff --git a/src/renderer/src/lib/window-visibility-interval.test.ts b/src/renderer/src/lib/window-visibility-interval.test.ts index f6329ef39f8..5f097cd7eeb 100644 --- a/src/renderer/src/lib/window-visibility-interval.test.ts +++ b/src/renderer/src/lib/window-visibility-interval.test.ts @@ -7,8 +7,7 @@ describe('installWindowVisibilityInterval', () => { vi.unstubAllGlobals() }) - it('runs intervals only while the document is visible', async () => { - vi.useFakeTimers() + it('runs intervals only while the document is visible', () => { let visibilityState: DocumentVisibilityState = 'hidden' const documentListeners = new Map void>() const clearIntervalMock = vi.fn() @@ -33,8 +32,7 @@ describe('installWindowVisibilityInterval', () => { run, intervalMs: 3000, setIntervalFn: setIntervalMock, - clearIntervalFn: clearIntervalMock, - jitterFn: () => 0 + clearIntervalFn: clearIntervalMock }) expect(run).not.toHaveBeenCalled() @@ -42,7 +40,6 @@ describe('installWindowVisibilityInterval', () => { visibilityState = 'visible' documentListeners.get('visibilitychange')?.() - await vi.advanceTimersByTimeAsync(0) expect(run).toHaveBeenCalledTimes(1) expect(setIntervalMock).toHaveBeenCalledTimes(1) @@ -52,7 +49,6 @@ describe('installWindowVisibilityInterval', () => { visibilityState = 'visible' documentListeners.get('visibilitychange')?.() - await vi.advanceTimersByTimeAsync(0) expect(run).toHaveBeenCalledTimes(2) expect(setIntervalMock).toHaveBeenCalledTimes(2) @@ -61,11 +57,9 @@ describe('installWindowVisibilityInterval', () => { 'visibilitychange', documentListeners.get('visibilitychange') ) - vi.useRealTimers() }) - it('uses runOnVisible for the becoming-visible run and run for interval ticks', async () => { - vi.useFakeTimers() + it('uses runOnVisible for the becoming-visible run and run for interval ticks', () => { let visibilityState: DocumentVisibilityState = 'hidden' const documentListeners = new Map void>() const intervalCallbacks: (() => void)[] = [] @@ -95,13 +89,11 @@ describe('installWindowVisibilityInterval', () => { runOnVisible, intervalMs: 3000, setIntervalFn: setIntervalMock, - clearIntervalFn: vi.fn(), - jitterFn: () => 0 + clearIntervalFn: vi.fn() }) visibilityState = 'visible' documentListeners.get('visibilitychange')?.() - await vi.advanceTimersByTimeAsync(0) expect(runOnVisible).toHaveBeenCalledTimes(1) expect(run).not.toHaveBeenCalled() @@ -110,7 +102,6 @@ describe('installWindowVisibilityInterval', () => { expect(runOnVisible).toHaveBeenCalledTimes(1) cleanup() - vi.useRealTimers() }) it('starts while visible even when the window is not focused', () => { @@ -153,6 +144,7 @@ describe('installWindowVisibilityInterval', () => { installWindowVisibilityInterval({ run, intervalMs: 10_000, + jitterOnVisible: true, jitterFn: () => [0, 100, 400][index] ?? 0 }) ) @@ -186,6 +178,7 @@ describe('installWindowVisibilityInterval', () => { const cleanup = installWindowVisibilityInterval({ run, intervalMs: 3000, + jitterOnVisible: true, jitterFn: () => 400 }) @@ -212,6 +205,7 @@ describe('installWindowVisibilityInterval', () => { const cleanup = installWindowVisibilityInterval({ run, intervalMs: 3000, + jitterOnVisible: true, jitterFn: () => 400 }) visibilityState = 'visible' diff --git a/src/renderer/src/lib/window-visibility-interval.ts b/src/renderer/src/lib/window-visibility-interval.ts index 6897a38312d..35e4d294b60 100644 --- a/src/renderer/src/lib/window-visibility-interval.ts +++ b/src/renderer/src/lib/window-visibility-interval.ts @@ -20,6 +20,7 @@ export function installWindowVisibilityInterval(args: { intervalMs: number setIntervalFn?: (callback: () => void, intervalMs: number) => WindowVisibilityIntervalTimer clearIntervalFn?: (handle: WindowVisibilityIntervalTimer) => void + jitterOnVisible?: boolean jitterFn?: () => number }): () => void { const setIntervalFn = @@ -30,13 +31,15 @@ export function installWindowVisibilityInterval(args: { args.clearIntervalFn ?? ((handle: WindowVisibilityIntervalTimer): void => clearInterval(handle)) let intervalId: WindowVisibilityIntervalTimer | null = null let visibilityJitterId: WindowVisibilityJitterTimer | null = null - const visibilityJitterMs = Math.max( - 0, - Math.min( - MAX_VISIBILITY_JITTER_MS, - Math.floor(args.jitterFn?.() ?? Math.random() * (MAX_VISIBILITY_JITTER_MS + 1)) - ) - ) + const visibilityJitterMs = args.jitterOnVisible + ? Math.max( + 0, + Math.min( + MAX_VISIBILITY_JITTER_MS, + Math.floor(args.jitterFn?.() ?? Math.random() * (MAX_VISIBILITY_JITTER_MS + 1)) + ) + ) + : 0 const stop = (): void => { if (visibilityJitterId !== null) { @@ -70,7 +73,7 @@ export function installWindowVisibilityInterval(args: { } const reconcile = (): void => { if (isWindowVisible()) { - start(true) + start(args.jitterOnVisible === true) } else { stop() } diff --git a/src/shared/child-process/run-process-termination-failure.test.ts b/src/shared/child-process/run-process-termination-failure.test.ts index 8c6dd092a85..3ba2e6b2610 100644 --- a/src/shared/child-process/run-process-termination-failure.test.ts +++ b/src/shared/child-process/run-process-termination-failure.test.ts @@ -108,7 +108,8 @@ describe('runProcess termination failure', () => { it('settles on the barrier deadline when the root never reports', async () => { const onChildTerminated = vi.fn() - spawnMock.mockReturnValue(mockChild()) + const child = mockChild() + spawnMock.mockReturnValue(child) const pending = runProcess({ program: 'git', timeoutMs: 10, @@ -126,6 +127,8 @@ describe('runProcess termination failure', () => { await vi.advanceTimersByTimeAsync(10_000) await expect(pending).resolves.toMatchObject({ code: null, timedOut: true }) + expect(onChildTerminated).not.toHaveBeenCalled() + child.emit('close', null, 'SIGKILL') expect(onChildTerminated).toHaveBeenCalledOnce() }) diff --git a/src/shared/child-process/run-process.ts b/src/shared/child-process/run-process.ts index 2fbe487642a..612ab13c173 100644 --- a/src/shared/child-process/run-process.ts +++ b/src/shared/child-process/run-process.ts @@ -55,7 +55,7 @@ export type ProcessSpec = { stdio?: NodeSpawnOptions['stdio'] /** Kill the whole process tree and do not settle until termination is verified. */ terminationBarrier?: boolean | ProcessTerminationBarrier - /** Called once when the child exits, or when a termination barrier gives up safely. */ + /** Called once when the child exits or tree termination is verified. */ onChildTerminated?: () => void } @@ -202,8 +202,6 @@ export function runProcess(spec: ProcessSpec): Promise { clearTimeout(graceTimer) clearTimeout(barrierDeadlineTimer) spec.signal?.removeEventListener('abort', onAbort) - // A barrier's bounded grace must release rather than pin capacity forever. - terminationReporter.reportIf(Boolean(spec.terminationBarrier)) act() }