From 4e7ad768d29c64264dd7617a04064e6b8a692864 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 23 Aug 2026 10:41:17 -0700 Subject: [PATCH] Fix rebase race by fetching to private ref with timeout Concurrent fetches can interfere with remote-tracking refs between fetch and rebase. Use a unique private ref and 60-second timeout to isolate each rebase operation and prevent hangs on stalled remotes. Extract gitPullRebaseFromBase to a dedicated module. --- src/main/git/remote-rebase.ts | 59 +++++++++++++++++++++++++++++++++ src/main/git/remote.test.ts | 25 +++++--------- src/main/git/remote.ts | 57 ++----------------------------- src/relay/git-handler.ts | 14 ++++---- src/shared/git-rebase-source.ts | 3 ++ 5 files changed, 79 insertions(+), 79 deletions(-) create mode 100644 src/main/git/remote-rebase.ts diff --git a/src/main/git/remote-rebase.ts b/src/main/git/remote-rebase.ts new file mode 100644 index 00000000000..3d7eb169879 --- /dev/null +++ b/src/main/git/remote-rebase.ts @@ -0,0 +1,59 @@ +import { randomUUID } from 'node:crypto' +import { normalizeGitErrorMessage } from '../../shared/git-remote-error' +import { + REBASE_SOURCE_FETCH_TIMEOUT_MS, + resolveGitRemoteRebaseSource +} from '../../shared/git-rebase-source' +import type { GitRuntimeOptions } from './git-runtime-options' +import { gitOptionsForWorktree } from './git-runtime-options' +import { gitExecFileAsync } from './runner' +import { runWithGitReadCacheInvalidation } from './status' + +export async function gitPullRebaseFromBase( + worktreePath: string, + baseRef: string, + options: GitRuntimeOptions = {} +): Promise { + await runWithGitReadCacheInvalidation(async () => { + let rebaseRef: string | null = null + try { + const source = await resolveGitRemoteRebaseSource( + (args) => gitExecFileAsync(args, gitOptionsForWorktree(worktreePath, options)), + baseRef + ) + let forkPoint: string | null = null + try { + const { stdout } = await gitExecFileAsync( + ['merge-base', '--fork-point', `refs/remotes/${source.displayName}`, 'HEAD'], + gitOptionsForWorktree(worktreePath, options) + ) + forkPoint = stdout.trim() || null + } catch { + // A first fetch or an unhelpful reflog falls back to Git's merge-base behavior. + } + // Why: concurrent fetches can replace FETCH_HEAD and remote-tracking refs between fetch and rebase. + rebaseRef = `refs/orca/rebase/${randomUUID()}` + await gitExecFileAsync( + ['fetch', source.remoteName, `+refs/heads/${source.branchName}:${rebaseRef}`], + { ...gitOptionsForWorktree(worktreePath, options), timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } + ) + await gitExecFileAsync( + forkPoint ? ['rebase', '--onto', rebaseRef, forkPoint] : ['rebase', rebaseRef], + gitOptionsForWorktree(worktreePath, options) + ) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error, 'pull')) + } finally { + if (rebaseRef) { + try { + await gitExecFileAsync( + ['update-ref', '-d', rebaseRef], + gitOptionsForWorktree(worktreePath, options) + ) + } catch { + // Cleanup must not hide the fetch or rebase result. + } + } + } + }) +} diff --git a/src/main/git/remote.test.ts b/src/main/git/remote.test.ts index df43f3e5205..2ff837a2001 100644 --- a/src/main/git/remote.test.ts +++ b/src/main/git/remote.test.ts @@ -8,6 +8,7 @@ vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +import { REBASE_SOURCE_FETCH_TIMEOUT_MS } from '../../shared/git-rebase-source' import { gitFastForward, gitFetch, gitPull, gitPullRebaseFromBase, gitPush } from './remote' describe('git remote operations', () => { @@ -527,12 +528,8 @@ describe('git remote operations', () => { [['check-ref-format', '--branch', 'main'], { cwd: '/repo' }], [['merge-base', '--fork-point', 'refs/remotes/upstream/main', 'HEAD'], { cwd: '/repo' }], [ - [ - 'fetch', - 'upstream', - expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//) - ], - { cwd: '/repo' } + ['fetch', 'upstream', expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//)], + { cwd: '/repo', timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } ], [ ['rebase', '--onto', expect.stringMatching(/^refs\/orca\/rebase\//), 'fork-point'], @@ -579,12 +576,8 @@ describe('git remote operations', () => { expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( 4, - [ - 'fetch', - 'upstream', - expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//) - ], - { cwd: '/repo' } + ['fetch', 'upstream', expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//)], + { cwd: '/repo', timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } ) }) @@ -602,11 +595,9 @@ describe('git remote operations', () => { ) const rebasedRef = gitExecFileAsyncMock.mock.calls[4][0][2] - expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( - 6, - ['update-ref', '-d', rebasedRef], - { cwd: '/repo' } - ) + expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith(6, ['update-ref', '-d', rebasedRef], { + cwd: '/repo' + }) }) it('normalizes pull authentication errors to a friendly message', async () => { diff --git a/src/main/git/remote.ts b/src/main/git/remote.ts index d0061a2156b..85aafcb9c87 100644 --- a/src/main/git/remote.ts +++ b/src/main/git/remote.ts @@ -1,11 +1,9 @@ -import { randomUUID } from 'node:crypto' import { normalizeGitErrorMessage, runPullWithDivergenceFallback } from '../../shared/git-remote-error' import { resolveEffectiveGitUpstream } from '../../shared/git-effective-upstream' import { gitRefTargetsBranchOnRemote } from '../../shared/git-remote-branch-name' -import { resolveGitRemoteRebaseSource } from '../../shared/git-rebase-source' import type { GitPushTarget } from '../../shared/worktree/types' import type { GitRuntimeOptions } from './git-runtime-options' import { gitOptionsForWorktree } from './git-runtime-options' @@ -13,6 +11,8 @@ import { validateGitPushTarget } from './push-target-validation' import { gitExecFileAsync } from './runner' import { runWithGitReadCacheInvalidation } from './status' +export { gitPullRebaseFromBase } from './remote-rebase' + async function getConfiguredPushTarget( worktreePath: string, options: GitRuntimeOptions = {} @@ -273,59 +273,6 @@ export async function gitFastForward( ) } -export async function gitPullRebaseFromBase( - worktreePath: string, - baseRef: string, - options: GitRuntimeOptions = {} -): Promise { - await runWithGitReadCacheInvalidation(async () => { - let rebaseRef: string | null = null - try { - const source = await resolveGitRemoteRebaseSource( - (args) => gitExecFileAsync(args, gitOptionsForWorktree(worktreePath, options)), - baseRef - ) - let forkPoint: string | null = null - try { - const { stdout } = await gitExecFileAsync( - ['merge-base', '--fork-point', `refs/remotes/${source.displayName}`, 'HEAD'], - gitOptionsForWorktree(worktreePath, options) - ) - forkPoint = stdout.trim() || null - } catch { - // A first fetch or an unhelpful reflog falls back to Git's merge-base behavior. - } - // Why: concurrent fetches can replace FETCH_HEAD and remote-tracking refs between fetch and rebase. - rebaseRef = `refs/orca/rebase/${randomUUID()}` - await gitExecFileAsync( - [ - 'fetch', - source.remoteName, - `+refs/heads/${source.branchName}:${rebaseRef}` - ], - gitOptionsForWorktree(worktreePath, options) - ) - await gitExecFileAsync( - forkPoint ? ['rebase', '--onto', rebaseRef, forkPoint] : ['rebase', rebaseRef], - gitOptionsForWorktree(worktreePath, options) - ) - } catch (error) { - throw new Error(normalizeGitErrorMessage(error, 'pull')) - } finally { - if (rebaseRef) { - try { - await gitExecFileAsync( - ['update-ref', '-d', rebaseRef], - gitOptionsForWorktree(worktreePath, options) - ) - } catch { - // Cleanup must not hide the fetch or rebase result. - } - } - } - }) -} - export async function gitFetch( worktreePath: string, pushTarget?: GitPushTarget, diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index effea967a8e..963a4f67017 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -61,7 +61,10 @@ import { import { upstreamOnlyCommitsArePatchEquivalent } from '../shared/git-upstream-status' import { assertGitPushTargetShape } from '../shared/git-push-target-validation' import { getPublishTargetStatus, type GitCommandRunner } from '../shared/git-publish-target-status' -import { resolveGitRemoteRebaseSource } from '../shared/git-rebase-source' +import { + REBASE_SOURCE_FETCH_TIMEOUT_MS, + resolveGitRemoteRebaseSource +} from '../shared/git-rebase-source' import type { GitPushTarget } from '../shared/worktree/types' import { getEffectiveGitUpstreamStatus, @@ -1153,12 +1156,9 @@ export class GitHandler { // Why: concurrent fetches can replace FETCH_HEAD and remote-tracking refs between fetch and rebase. rebaseRef = `refs/orca/rebase/${randomUUID()}` await this.git( - [ - 'fetch', - source.remoteName, - `+refs/heads/${source.branchName}:${rebaseRef}` - ], - worktreePath + ['fetch', source.remoteName, `+refs/heads/${source.branchName}:${rebaseRef}`], + worktreePath, + { timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } ) await this.git( forkPoint ? ['rebase', '--onto', rebaseRef, forkPoint] : ['rebase', rebaseRef], diff --git a/src/shared/git-rebase-source.ts b/src/shared/git-rebase-source.ts index 27b05d1d203..a4b44b46d79 100644 --- a/src/shared/git-rebase-source.ts +++ b/src/shared/git-rebase-source.ts @@ -1,3 +1,6 @@ +// Why: a stalled remote must fail the rebase fetch, not hang the rebase; client and relay share one bound. +export const REBASE_SOURCE_FETCH_TIMEOUT_MS = 60_000 + export type GitCommandRunner = (args: string[]) => Promise<{ stdout: string }> export type GitRemoteRebaseSource = {