From c74496bf175ecf8517434c3f413cbb4c77e71ac6 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 23 Aug 2026 14:19:35 -0700 Subject: [PATCH] fix rebase race by fetching to private ref with timeouts Concurrent fetches can replace FETCH_HEAD and remote-tracking refs between fetch and rebase, causing the rebase to fail. Fetch to a temporary private ref instead, use --no-write-fetch-head when available (Git 2.29+), and serialize FETCH_HEAD access for older versions. Add process termination barriers to ensure proper cleanup and extend timeouts for SSH operations. --- docs/reference/git-compatibility.md | 17 +- src/main/git/remote-rebase.ts | 53 +++++- src/main/git/remote.test.ts | 96 ++++++++-- src/main/git/runner-command-exec.test.ts | 44 +++++ src/main/git/runner-wsl-direct-read.test.ts | 28 +++ src/main/git/runner.ts | 126 +++++++++++-- .../ssh-git-provider-remote-sync.test.ts | 13 +- src/main/providers/ssh-git-provider.ts | 7 +- src/relay/git-handler-remote-sync.test.ts | 91 +++++++++- src/relay/git-handler-termination.test.ts | 56 ++++++ src/relay/git-handler.ts | 150 ++++++++++++--- .../process-tree-termination.test.ts | 74 ++++++++ .../child-process/process-tree-termination.ts | 150 +++++++++++++++ src/shared/child-process/run-process.test.ts | 42 ++++- src/shared/child-process/run-process.ts | 94 +++++++--- src/shared/git-binary-compatibility.test.ts | 11 +- src/shared/git-capability-cache.ts | 1 + src/shared/git-fetch-head-capability.test.ts | 18 ++ src/shared/git-fetch-head-capability.ts | 16 ++ src/shared/git-fetch-head-lock.test.ts | 145 +++++++++++++++ src/shared/git-fetch-head-lock.ts | 171 ++++++++++++++++++ src/shared/git-rebase-source.ts | 2 + 22 files changed, 1295 insertions(+), 110 deletions(-) create mode 100644 src/relay/git-handler-termination.test.ts create mode 100644 src/shared/child-process/process-tree-termination.test.ts create mode 100644 src/shared/child-process/process-tree-termination.ts create mode 100644 src/shared/git-fetch-head-capability.test.ts create mode 100644 src/shared/git-fetch-head-capability.ts create mode 100644 src/shared/git-fetch-head-lock.test.ts create mode 100644 src/shared/git-fetch-head-lock.ts diff --git a/docs/reference/git-compatibility.md b/docs/reference/git-compatibility.md index b740982febb..3004b8888ab 100644 --- a/docs/reference/git-compatibility.md +++ b/docs/reference/git-compatibility.md @@ -33,13 +33,14 @@ authority. ## Current Capabilities -| Capability | Preferred behavior | Compatibility behavior | -| ----------------------- | ------------------------------------------------- | --------------------------------------------------------------------------------------- | -| `worktree-list-z` | NUL-delimited worktree paths with `prunable` marks | Line-block parser for Git before `worktree list -z` (2.36); the `prunable`/`locked` annotations still parse on Git 2.31–2.35, and a path-existence probe restores `prunable` detection for Git before 2.31 | -| `rev-parse-path-format` | Absolute repo metadata paths | Resolve legacy relative output against the scanned repo | -| `for-each-ref-exclude` | Exclude remote HEAD before the output limit | Request extra refs, then filter remote HEAD in Orca | -| `merge-tree-write-tree` | Derive real-merge conflicts and no-op tree proofs | Omit the conflict summary and keep conservative branch cleanup behavior before Git 2.38 | -| `merge-tree-merge-base` | Supply the already-resolved merge base | Use the older two-commit `merge-tree --write-tree` form | +| Capability | Preferred behavior | Compatibility behavior | +| --------------------------- | ----------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `fetch-no-write-fetch-head` | Fetch a private rebase ref without changing worktree-local `FETCH_HEAD` | Serialize all Orca fetch/pull operations per worktree Git directory before Git 2.29 | +| `worktree-list-z` | NUL-delimited worktree paths with `prunable` marks | Line-block parser for Git before `worktree list -z` (2.36); the `prunable`/`locked` annotations still parse on Git 2.31–2.35, and a path-existence probe restores `prunable` detection for Git before 2.31 | +| `rev-parse-path-format` | Absolute repo metadata paths | Resolve legacy relative output against the scanned repo | +| `for-each-ref-exclude` | Exclude remote HEAD before the output limit | Request extra refs, then filter remote HEAD in Orca | +| `merge-tree-write-tree` | Derive real-merge conflicts and no-op tree proofs | Omit the conflict summary and keep conservative branch cleanup behavior before Git 2.38 | +| `merge-tree-merge-base` | Supply the already-resolved merge base | Use the older two-commit `merge-tree --write-tree` form | ## Why Not `simple-git` @@ -54,7 +55,7 @@ capability problem. ## CI Contract PR checks run the capability contract against real Git 2.25.5, 2.38.1, and -2.49.1 binaries. This spans the core-workflow baseline, the transitional +2.49.1 binaries. This spans the pre-2.29 serialized `FETCH_HEAD` fallback, the transitional `merge-tree --write-tree` behavior before `--merge-base`, and current Git. Keep the unit tests alongside that matrix. They cover concurrent probes, diff --git a/src/main/git/remote-rebase.ts b/src/main/git/remote-rebase.ts index 3d7eb169879..63b76ce193d 100644 --- a/src/main/git/remote-rebase.ts +++ b/src/main/git/remote-rebase.ts @@ -1,11 +1,13 @@ import { randomUUID } from 'node:crypto' import { normalizeGitErrorMessage } from '../../shared/git-remote-error' +import { isNoWriteFetchHeadUnsupportedError } from '../../shared/git-fetch-head-capability' 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 { withLocalGitCapabilityCacheForExecution } from './git-capability-state' import { gitExecFileAsync } from './runner' import { runWithGitReadCacheInvalidation } from './status' @@ -15,41 +17,72 @@ export async function gitPullRebaseFromBase( options: GitRuntimeOptions = {} ): Promise { await runWithGitReadCacheInvalidation(async () => { + const operationOptions = { + ...gitOptionsForWorktree(worktreePath, options), + terminationBarrier: true, + captureWslLoginShellOutput: true + } let rebaseRef: string | null = null try { const source = await resolveGitRemoteRebaseSource( - (args) => gitExecFileAsync(args, gitOptionsForWorktree(worktreePath, options)), + (args) => gitExecFileAsync(args, operationOptions), baseRef ) let forkPoint: string | null = null + let hasHead = true try { const { stdout } = await gitExecFileAsync( ['merge-base', '--fork-point', `refs/remotes/${source.displayName}`, 'HEAD'], - gitOptionsForWorktree(worktreePath, options) + operationOptions ) forkPoint = stdout.trim() || null } catch { // A first fetch or an unhelpful reflog falls back to Git's merge-base behavior. + try { + await gitExecFileAsync(['rev-parse', '--verify', 'HEAD'], operationOptions) + } catch { + hasHead = false + } } // 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 } + const fetchArgs = [source.remoteName, `+refs/heads/${source.branchName}:${rebaseRef}`] + await withLocalGitCapabilityCacheForExecution( + { cwd: worktreePath, ...options }, + (capabilities) => + capabilities.runWithFallback( + 'fetch-no-write-fetch-head', + () => + gitExecFileAsync(['fetch', '--no-write-fetch-head', ...fetchArgs], { + ...operationOptions, + timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS + }), + () => + gitExecFileAsync(['fetch', ...fetchArgs], { + ...operationOptions, + timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS + }), + isNoWriteFetchHeadUnsupportedError + ) ) await gitExecFileAsync( - forkPoint ? ['rebase', '--onto', rebaseRef, forkPoint] : ['rebase', rebaseRef], - gitOptionsForWorktree(worktreePath, options) + hasHead + ? forkPoint + ? ['rebase', '--onto', rebaseRef, forkPoint] + : ['rebase', rebaseRef] + : ['merge', '--ff-only', rebaseRef], + operationOptions ) } catch (error) { throw new Error(normalizeGitErrorMessage(error, 'pull')) } finally { if (rebaseRef) { try { - await gitExecFileAsync( - ['update-ref', '-d', rebaseRef], - gitOptionsForWorktree(worktreePath, options) + const { signal: _abortedSignal, ...cleanupOptions } = gitOptionsForWorktree( + worktreePath, + options ) + await gitExecFileAsync(['update-ref', '-d', rebaseRef], cleanupOptions) } 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 2ff837a2001..fab8bb7c595 100644 --- a/src/main/git/remote.test.ts +++ b/src/main/git/remote.test.ts @@ -9,11 +9,19 @@ vi.mock('./runner', () => ({ })) import { REBASE_SOURCE_FETCH_TIMEOUT_MS } from '../../shared/git-rebase-source' +import { clearGitCapabilityStateForTests } from './git-capability-state' import { gitFastForward, gitFetch, gitPull, gitPullRebaseFromBase, gitPush } from './remote' +const REBASE_OPERATION_OPTIONS = { + cwd: '/repo', + terminationBarrier: true, + captureWslLoginShellOutput: true +} + describe('git remote operations', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() + clearGitCapabilityStateForTests() }) it('pushes to origin when no upstream is configured', async () => { @@ -524,21 +532,29 @@ describe('git remote operations', () => { await gitPullRebaseFromBase('/repo', 'upstream/main') expect(gitExecFileAsyncMock.mock.calls).toEqual([ - [['remote'], { cwd: '/repo' }], - [['check-ref-format', '--branch', 'main'], { cwd: '/repo' }], - [['merge-base', '--fork-point', 'refs/remotes/upstream/main', 'HEAD'], { cwd: '/repo' }], + [['remote'], REBASE_OPERATION_OPTIONS], + [['check-ref-format', '--branch', 'main'], REBASE_OPERATION_OPTIONS], [ - ['fetch', 'upstream', expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//)], - { cwd: '/repo', timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } + ['merge-base', '--fork-point', 'refs/remotes/upstream/main', 'HEAD'], + REBASE_OPERATION_OPTIONS + ], + [ + [ + 'fetch', + '--no-write-fetch-head', + 'upstream', + expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//) + ], + { ...REBASE_OPERATION_OPTIONS, timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } ], [ ['rebase', '--onto', expect.stringMatching(/^refs\/orca\/rebase\//), 'fork-point'], - { cwd: '/repo' } + REBASE_OPERATION_OPTIONS ], [['update-ref', '-d', expect.stringMatching(/^refs\/orca\/rebase\//)], { cwd: '/repo' }] ]) - const fetchRefspec = gitExecFileAsyncMock.mock.calls[3][0][2] + const fetchRefspec = gitExecFileAsyncMock.mock.calls[3][0][3] const rebasedRef = gitExecFileAsyncMock.mock.calls[4][0][2] const deletedRef = gitExecFileAsyncMock.mock.calls[5][0][2] expect(fetchRefspec).toBe(`+refs/heads/main:${rebasedRef}`) @@ -559,7 +575,7 @@ describe('git remote operations', () => { expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( 5, ['rebase', '--onto', expect.stringMatching(/^refs\/orca\/rebase\//), 'fork-point'], - { cwd: '/repo' } + REBASE_OPERATION_OPTIONS ) }) @@ -568,6 +584,7 @@ describe('git remote operations', () => { .mockResolvedValueOnce({ stdout: 'upstream\n', stderr: '' }) .mockResolvedValueOnce({ stdout: '', stderr: '' }) .mockRejectedValueOnce(new Error('missing remote-tracking ref')) + .mockResolvedValueOnce({ stdout: 'head\n', stderr: '' }) .mockResolvedValueOnce({ stdout: '', stderr: '' }) .mockResolvedValueOnce({ stdout: '', stderr: '' }) .mockResolvedValueOnce({ stdout: '', stderr: '' }) @@ -575,24 +592,53 @@ describe('git remote operations', () => { await expect(gitPullRebaseFromBase('/repo', 'upstream/main')).resolves.toBeUndefined() expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( - 4, - ['fetch', 'upstream', expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//)], - { cwd: '/repo', timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } + 5, + [ + 'fetch', + '--no-write-fetch-head', + 'upstream', + expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//) + ], + { ...REBASE_OPERATION_OPTIONS, timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } + ) + }) + + it('fast-forwards an unborn branch from the fetched private ref', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'upstream\n', stderr: '' }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + .mockRejectedValueOnce(new Error('unborn HEAD')) + .mockRejectedValueOnce(new Error('unborn HEAD')) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + await gitPullRebaseFromBase('/repo', 'upstream/main') + + const fetchedRef = gitExecFileAsyncMock.mock.calls[4][0][3] + expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( + 6, + ['merge', '--ff-only', fetchedRef.slice(fetchedRef.indexOf(':') + 1)], + REBASE_OPERATION_OPTIONS ) }) it('removes the private ref when rebase fails', async () => { + const controller = new AbortController() gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'upstream\n', stderr: '' }) .mockResolvedValueOnce({ stdout: '', stderr: '' }) .mockResolvedValueOnce({ stdout: 'fork-point\n', stderr: '' }) .mockResolvedValueOnce({ stdout: '', stderr: '' }) - .mockRejectedValueOnce(new Error('fatal: rebase conflict')) + .mockImplementationOnce(async () => { + controller.abort() + throw new Error('fatal: rebase conflict') + }) .mockResolvedValueOnce({ stdout: '', stderr: '' }) - await expect(gitPullRebaseFromBase('/repo', 'upstream/main')).rejects.toThrow( - 'fatal: rebase conflict' - ) + await expect( + gitPullRebaseFromBase('/repo', 'upstream/main', { signal: controller.signal }) + ).rejects.toThrow('fatal: rebase conflict') const rebasedRef = gitExecFileAsyncMock.mock.calls[4][0][2] expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith(6, ['update-ref', '-d', rebasedRef], { @@ -600,6 +646,26 @@ describe('git remote operations', () => { }) }) + it('serializes the private fetch when Git cannot avoid writing FETCH_HEAD', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'upstream\n', stderr: '' }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + .mockResolvedValueOnce({ stdout: 'fork-point\n', stderr: '' }) + .mockRejectedValueOnce(new Error("error: unknown option `no-write-fetch-head'")) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + await gitPullRebaseFromBase('/repo', 'upstream/main') + + const preferredRefspec = gitExecFileAsyncMock.mock.calls[3][0][3] + expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( + 5, + ['fetch', 'upstream', preferredRefspec], + { ...REBASE_OPERATION_OPTIONS, timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } + ) + }) + it('normalizes pull authentication errors to a friendly message', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'feature\n', stderr: '' }) diff --git a/src/main/git/runner-command-exec.test.ts b/src/main/git/runner-command-exec.test.ts index e29c9f650cc..f145b311521 100644 --- a/src/main/git/runner-command-exec.test.ts +++ b/src/main/git/runner-command-exec.test.ts @@ -218,6 +218,50 @@ describe('runner execFile timeout handling', () => { expect(child.kill).toHaveBeenCalled() }) + it.skipIf(process.platform === 'win32')( + 'keeps a barrier Git abort pending until the process group closes', + async () => { + const child = createMockChildProcess(1234) + spawnMock.mockImplementation((command: string) => { + if (command !== 'ps') { + return child + } + const probe = createMockChildProcess(4321) + queueMicrotask(() => probe.emit('close', 0, null)) + return probe + }) + const processKill = vi.spyOn(process, 'kill').mockImplementation(() => true) + const controller = new AbortController() + try { + const pending = gitExecFileAsync(['status'], { + cwd: '/repo', + signal: controller.signal, + terminationBarrier: true + }) + let settled = false + void pending.then( + () => { + settled = true + }, + () => { + settled = true + } + ) + + controller.abort() + child.emit('close', 0, null) + await vi.advanceTimersByTimeAsync(1_999) + expect(settled).toBe(false) + await vi.advanceTimersByTimeAsync(1) + expect(processKill).toHaveBeenCalledWith(-1234, 'SIGKILL') + + await expect(pending).rejects.toMatchObject({ name: 'AbortError' }) + } finally { + processKill.mockRestore() + } + } + ) + it('rejects gh executions that never call back using the default timeout', async () => { const child = createMockChildProcess(1234) execFileMock.mockReturnValue(child) diff --git a/src/main/git/runner-wsl-direct-read.test.ts b/src/main/git/runner-wsl-direct-read.test.ts index 3aca1dd738e..50fb77567fa 100644 --- a/src/main/git/runner-wsl-direct-read.test.ts +++ b/src/main/git/runner-wsl-direct-read.test.ts @@ -253,6 +253,34 @@ describe('WSL direct Git reads', () => { }) }) + it('fences parsed output from a barrier Git login-shell fallback', async () => { + await withPlatform('win32', async () => { + seedWslGitReadEnvironmentForTests(DISTRO, LOGIN_ENVIRONMENT) + spawnMock.mockImplementation((_command, args) => { + const child = createMockChild() + queueMicrotask(() => { + const fenced = fencedProbeStdout(args?.[5], 'fork-point\n') + const echoedMarker = fenced.match(/__ORCA_WSL_CAPTURE_BEGIN_[^_]+__/)?.[0] ?? '' + child.stdout.emit('data', Buffer.from(`${echoedMarker}shell trace\n${fenced}`)) + child.emit('close', 0, null) + }) + return child + }) + + await expect( + gitExecFileAsync(['merge-base', '--fork-point', 'upstream/main', 'HEAD'], { + cwd: String.raw`C:\repo`, + env: { GIT_CONFIG_GLOBAL: '/home/user/custom.gitconfig' }, + wslDistro: DISTRO, + captureWslLoginShellOutput: true, + terminationBarrier: true + }) + ).resolves.toEqual({ stdout: 'fork-point\n', stderr: '' }) + + expect(spawnMock.mock.calls[0]?.[1]?.[5]).toContain('__ORCA_WSL_CAPTURE_BEGIN_') + }) + }) + it('ignores unchanged ambient host Git variables when selecting the direct path', async () => { await withPlatform('win32', async () => { const originalAskpass = process.env.GIT_ASKPASS diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 1ae53551f29..e2145327924 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -44,6 +44,11 @@ import { } from '../../shared/wsl-login-shell-command' import { UNTRANSLATED_GIT_OUTPUT_ENV } from '../../shared/git-output-locale' import { endSubprocessStdin } from '../../shared/subprocess-stdin-write' +import { runProcess } from '../../shared/child-process/run-process' +import { + resolveGitFetchHeadCommand, + runWithGitFetchHeadLock +} from '../../shared/git-fetch-head-lock' import { disableWslGitReadEnvironment, getWslGitReadEnvironment, @@ -418,6 +423,8 @@ type GitExecOptions = { wslDistro?: string preferWslDirectGit?: boolean useConfiguredSshCommandForNetwork?: boolean + terminationBarrier?: boolean + captureWslLoginShellOutput?: boolean } type CommandExecOptions = { @@ -605,6 +612,49 @@ function killSpawnedCommandTree(child: ChildProcess): Promise { type ExecFileCaptureOptions = Omit & { timeout?: number stdin?: string + terminationBarrier?: boolean +} + +const GIT_TERMINATION_BARRIER_FALLBACK_TIMEOUT_MS = 2_147_000_000 + +async function execFileCaptureToTermination( + command: string, + args: string[], + options: ExecFileCaptureOptions +): Promise<{ stdout: string | Buffer; stderr: string | Buffer }> { + const result = await runProcess({ + program: command, + args, + cwd: typeof options.cwd === 'string' ? options.cwd : undefined, + env: options.env, + timeoutMs: options.timeout ?? GIT_TERMINATION_BARRIER_FALLBACK_TIMEOUT_MS, + maxOutputBytes: options.maxBuffer ?? DEFAULT_GIT_MAX_BUFFER, + signal: options.signal, + terminationBarrier: true, + ...(options.stdin === undefined ? {} : { input: options.stdin }) + }) + const stdout = options.encoding === 'buffer' ? Buffer.from(result.stdout) : result.stdout + const stderr = options.encoding === 'buffer' ? Buffer.from(result.stderr) : result.stderr + if (result.code === 0 && !result.timedOut && !options.signal?.aborted) { + return { stdout, stderr } + } + const error = new Error( + result.timedOut + ? `${command} timed out.` + : options.signal?.aborted + ? 'The operation was aborted.' + : result.stderr.trim() || `${command} exited with ${result.code}.` + ) + if (options.signal?.aborted) { + error.name = 'AbortError' + } + throw Object.assign(error, { + code: result.code, + killed: result.timedOut || result.signal !== null || options.signal?.aborted === true, + signal: result.signal, + stdout, + stderr + }) } function emptyExecFileOutput(options: ExecFileCaptureOptions): string | Buffer { @@ -1077,7 +1127,7 @@ async function buildNetworkSshPolicyEnv(options: GitExecOptions): Promise<{ * Async git command execution. Drop-in replacement for * `execFileAsync('git', args, { cwd, encoding, ... })`. */ -export async function gitExecFileAsync( +async function gitExecFileAsyncUnlocked( args: string[], options: GitExecOptions ): Promise<{ stdout: string; stderr: string }> { @@ -1090,7 +1140,7 @@ export async function gitExecFileAsync( signal: options.signal }) } - let resolved = resolveGitCommand(args, options) + let resolved = resolveGitCommand(args, options, false, options.captureWslLoginShellOutput) const environmentReady = prepareWindowsHostGitEnvironment( resolved, options.env, @@ -1098,33 +1148,52 @@ export async function gitExecFileAsync( ) const env = environmentReady ? await environmentReady : options.env const effectiveOptions = env === options.env ? options : { ...options, env } - resolved = resolveGitCommand(args, effectiveOptions) + resolved = resolveGitCommand( + args, + effectiveOptions, + false, + effectiveOptions.captureWslLoginShellOutput + ) const policy = effectiveOptions.useConfiguredSshCommandForNetwork ? await buildNetworkSshPolicyEnv(effectiveOptions) : { env: nonInteractiveGitEnv(effectiveOptions.env), mode: 'default' as const } const capture = ( command: ResolvedCommand ): Promise<{ stdout: string | Buffer; stderr: string | Buffer }> => - execFileCapture(command.binary, command.args, { - cwd: command.cwd, - encoding: (options.encoding ?? 'utf-8') as BufferEncoding, - maxBuffer: options.maxBuffer, - timeout: options.timeout, - stdin: options.stdin, - env: policy.env, - signal: options.signal - }) + (options.terminationBarrier ? execFileCaptureToTermination : execFileCapture)( + command.binary, + command.args, + { + cwd: command.cwd, + encoding: (options.encoding ?? 'utf-8') as BufferEncoding, + maxBuffer: options.maxBuffer, + timeout: options.timeout, + stdin: options.stdin, + env: policy.env, + signal: options.signal, + terminationBarrier: options.terminationBarrier + } + ) let result: { stdout: string | Buffer; stderr: string | Buffer } try { result = await capture(resolved) } catch (error) { if (directWslGitExitCode(error, resolved) !== null && !options.signal?.aborted) { const wasMissing = invalidateMissingDirectWslGit(error, resolved) - result = await capture(resolveGitCommand(args, effectiveOptions, true)) + const fallback = resolveGitCommand( + args, + effectiveOptions, + true, + effectiveOptions.captureWslLoginShellOutput + ) + result = await capture(fallback) // Why: matching failures can be normal Git control flow; only a successful login retry proves the direct environment was insufficient. disableDirectWslGitAfterSuccessfulFallback(wasMissing, resolved) const { stdout, stderr } = result - return { stdout: stdout as string, stderr: stderr as string } + return { + stdout: readCapturedGitString(stdout as string, fallback), + stderr: stderr as string + } } if (options.useConfiguredSshCommandForNetwork && error && typeof error === 'object') { Object.assign(error, { gitSshPolicyMode: policy.mode }) @@ -1132,11 +1201,22 @@ export async function gitExecFileAsync( throw error } const { stdout, stderr } = result - return { stdout: stdout as string, stderr: stderr as string } + return { stdout: readCapturedGitString(stdout as string, resolved), stderr: stderr as string } } ) } +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() +} + /** * Async command execution with the same WSL cwd translation as repo-scoped git. * Keep this for fixed binary+argv call sites; never pass shell fragments. @@ -1222,7 +1302,7 @@ function readCapturedGitBuffer(stdout: Buffer, resolved: ResolvedCommand): Buffe if (!captured) { return stdout } - const beginIndex = stdout.indexOf(captured.beginMarker, 0, 'utf8') + const beginIndex = stdout.lastIndexOf(captured.beginMarker, undefined, 'utf8') if (beginIndex === -1) { return stdout } @@ -1231,6 +1311,20 @@ function readCapturedGitBuffer(stdout: Buffer, resolved: ResolvedCommand): Buffe return endIndex === -1 ? stdout.subarray(payloadStart) : stdout.subarray(payloadStart, endIndex) } +function readCapturedGitString(stdout: string, resolved: ResolvedCommand): string { + const captured = resolved.captured + if (!captured) { + return stdout + } + const beginIndex = stdout.lastIndexOf(captured.beginMarker) + if (beginIndex === -1) { + return stdout + } + const payloadStart = beginIndex + captured.beginMarker.length + const endIndex = stdout.indexOf(captured.endMarker, payloadStart) + return endIndex === -1 ? stdout.slice(payloadStart) : stdout.slice(payloadStart, endIndex) +} + /** Result of a streamed git command; `stoppedEarly` is true when onStdout asked to stop before the child exited. */ export type GitStreamResult = { stoppedEarly: boolean } diff --git a/src/main/providers/ssh-git-provider-remote-sync.test.ts b/src/main/providers/ssh-git-provider-remote-sync.test.ts index 969448516ef..4a25d54b8a6 100644 --- a/src/main/providers/ssh-git-provider-remote-sync.test.ts +++ b/src/main/providers/ssh-git-provider-remote-sync.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, beforeEach } from 'vitest' import { SshGitProvider } from './ssh-git-provider' import { createMockMux, type MockMultiplexer } from './ssh-git-provider-test-harness' +import { REBASE_FROM_BASE_RPC_TIMEOUT_MS } from '../../shared/git-rebase-source' describe('SshGitProvider', () => { let mux: MockMultiplexer @@ -101,10 +102,14 @@ describe('SshGitProvider', () => { it('rebaseFromBase sends git.rebaseFromBase request', async () => { await provider.rebaseFromBase('/home/user/repo', 'upstream/main') - expect(mux.request).toHaveBeenCalledWith('git.rebaseFromBase', { - worktreePath: '/home/user/repo', - baseRef: 'upstream/main' - }) + expect(mux.request).toHaveBeenCalledWith( + 'git.rebaseFromBase', + { + worktreePath: '/home/user/repo', + baseRef: 'upstream/main' + }, + { timeoutMs: REBASE_FROM_BASE_RPC_TIMEOUT_MS } + ) }) it('fetchRemote sends git.fetch request', async () => { diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 6aa09b2f8ec..8086548ed5d 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -33,6 +33,7 @@ import { import { InFlightPromiseDedupe, stableInFlightKey } from '../../shared/in-flight-promise-dedupe' import { gitExecMutatesRepository } from '../../shared/git-exec-mutation' import { GitStatusReadLeaseOwner } from '../git/git-status-read-lease-owner' +import { REBASE_FROM_BASE_RPC_TIMEOUT_MS } from '../../shared/git-rebase-source' import { GitUpstreamStatusReadOwner } from '../git/git-upstream-status-read-owner' type NonInteractiveExecQueueEntry = { @@ -565,7 +566,11 @@ export class SshGitProvider implements IGitProvider { async rebaseFromBase(worktreePath: string, baseRef: string): Promise { await this.runWithGitReadInvalidation(async () => { - await this.mux.request('git.rebaseFromBase', { worktreePath, baseRef }) + await this.mux.request( + 'git.rebaseFromBase', + { worktreePath, baseRef }, + { timeoutMs: REBASE_FROM_BASE_RPC_TIMEOUT_MS } + ) }) } diff --git a/src/relay/git-handler-remote-sync.test.ts b/src/relay/git-handler-remote-sync.test.ts index 6cb120e7d75..0db8863b1ae 100644 --- a/src/relay/git-handler-remote-sync.test.ts +++ b/src/relay/git-handler-remote-sync.test.ts @@ -3,7 +3,7 @@ * sync, fast-forward, and the narrow review-head fetches for GitHub pull * requests and GitLab merge requests. */ -import { describe, expect, it, beforeEach, afterEach } from 'vitest' +import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest' import * as fs from 'node:fs/promises' import * as path from 'node:path' import { mkdtempSync, writeFileSync } from 'node:fs' @@ -14,16 +14,20 @@ import { gitInit, gitCommit, type MockDispatcher } from './git-handler-test-setu import { createGitHandlerRelay, createGitTempDir, - removeGitTempDir + removeGitTempDir, + type GitSpyTarget } from './git-handler-test-harness' describe('GitHandler', () => { let dispatcher: MockDispatcher let tmpDir: string + let gitTarget: GitSpyTarget beforeEach(() => { tmpDir = createGitTempDir() - ;({ dispatcher } = createGitHandlerRelay()) + const relay = createGitHandlerRelay() + dispatcher = relay.dispatcher + gitTarget = relay.handler as unknown as GitSpyTarget }) afterEach(async () => { @@ -199,7 +203,10 @@ describe('GitHandler', () => { execFileSync('git', ['reset', '--hard', forkPoint], { cwd: producerDir, stdio: 'pipe' }) writeFileSync(path.join(producerDir, 'replacement.txt'), 'replacement') gitCommit(producerDir, 'replacement remote commit') - execFileSync('git', ['push', '--force', 'origin', branch], { cwd: producerDir, stdio: 'pipe' }) + execFileSync('git', ['push', '--force', 'origin', branch], { + cwd: producerDir, + stdio: 'pipe' + }) await dispatcher.callRequest('git.rebaseFromBase', { worktreePath: tmpDir, @@ -223,6 +230,82 @@ describe('GitHandler', () => { } }, 15_000) + it('fast-forwards an unborn branch from the selected remote base', async () => { + const bareDir = mkdtempSync(path.join(tmpdir(), 'relay-git-unborn-bare-')) + const producerDir = mkdtempSync(path.join(tmpdir(), 'relay-git-unborn-producer-')) + try { + execFileSync('git', ['init', '--bare'], { cwd: bareDir, stdio: 'pipe' }) + gitInit(producerDir) + writeFileSync(path.join(producerDir, 'base.txt'), 'base') + gitCommit(producerDir, 'base') + const branch = execFileSync('git', ['branch', '--show-current'], { + cwd: producerDir, + encoding: 'utf-8' + }).trim() + execFileSync('git', ['remote', 'add', 'origin', bareDir], { + cwd: producerDir, + stdio: 'pipe' + }) + execFileSync('git', ['push', 'origin', branch], { cwd: producerDir, stdio: 'pipe' }) + + gitInit(tmpDir) + execFileSync('git', ['remote', 'add', 'origin', bareDir], { cwd: tmpDir, stdio: 'pipe' }) + + await dispatcher.callRequest('git.rebaseFromBase', { + worktreePath: tmpDir, + baseRef: `origin/${branch}` + }) + + await expect(fs.access(path.join(tmpDir, 'base.txt'))).resolves.toBeUndefined() + expect(execFileSync('git', ['rev-parse', '--verify', 'HEAD'], { cwd: tmpDir })).toBeTruthy() + } finally { + await Promise.all([ + fs.rm(bareDir, { recursive: true, force: true }), + fs.rm(producerDir, { recursive: true, force: true }) + ]) + } + }) + + it('cancels the active rebase fetch and still removes its private ref', async () => { + const controller = new AbortController() + let rejectFetch!: (error: Error) => void + const fetchStarted = new Promise((resolve) => { + vi.spyOn(gitTarget, 'git').mockImplementation(async (args, _cwd, options) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'merge-base') { + return { stdout: 'fork-point\n', stderr: '' } + } + if (args[0] === 'fetch') { + resolve() + return new Promise((_resolve, reject) => { + rejectFetch = reject + options?.signal?.addEventListener('abort', () => reject(new Error('aborted')), { + once: true + }) + }) + } + return { stdout: '', stderr: '' } + }) + }) + + const request = dispatcher.callRequest( + 'git.rebaseFromBase', + { worktreePath: tmpDir, baseRef: 'origin/main' }, + { isStale: () => false, signal: controller.signal } + ) + await fetchStarted + controller.abort() + await expect(request).rejects.toThrow('aborted') + + const calls = vi.mocked(gitTarget.git).mock.calls + expect(calls.some(([args]) => args[0] === 'rebase')).toBe(false) + const cleanup = calls.find(([args]) => args[0] === 'update-ref') + expect(cleanup?.[2]?.signal).toBeUndefined() + expect(rejectFetch).toBeTypeOf('function') + }) + it('fetches the explicit publish target remote', async () => { const bareDir = mkdtempSync(path.join(tmpdir(), 'relay-git-fork-bare-')) try { diff --git a/src/relay/git-handler-termination.test.ts b/src/relay/git-handler-termination.test.ts new file mode 100644 index 00000000000..86189374c53 --- /dev/null +++ b/src/relay/git-handler-termination.test.ts @@ -0,0 +1,56 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { runProcessMock } = vi.hoisted(() => ({ runProcessMock: vi.fn() })) + +vi.mock('../shared/child-process/run-process', () => ({ runProcess: runProcessMock })) + +import { createGitHandlerRelay } from './git-handler-test-harness' + +type GitTerminationTarget = { + git( + args: string[], + cwd: string, + options: { signal?: AbortSignal; terminationBarrier: true; timeout?: number } + ): Promise<{ stdout: string; stderr: string }> +} + +describe('GitHandler termination barrier', () => { + beforeEach(() => runProcessMock.mockReset()) + + it('rejects a zero-exit result that crossed its timeout', async () => { + runProcessMock.mockResolvedValue({ + code: 0, + signal: null, + stdout: '', + stderr: '', + timedOut: true + }) + const { handler } = createGitHandlerRelay() + const target = handler as unknown as GitTerminationTarget + + await expect( + target.git(['status'], '/repo', { terminationBarrier: true, timeout: 1 }) + ).rejects.toThrow('git status timed out.') + }) + + it('rejects a zero-exit result after caller cancellation', async () => { + runProcessMock.mockResolvedValue({ + code: 0, + signal: null, + stdout: '', + stderr: '', + timedOut: false + }) + const controller = new AbortController() + controller.abort() + const { handler } = createGitHandlerRelay() + const target = handler as unknown as GitTerminationTarget + + await expect( + target.git(['status'], '/repo', { + signal: controller.signal, + terminationBarrier: true + }) + ).rejects.toMatchObject({ name: 'AbortError' }) + }) +}) diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 963a4f67017..9d586479c06 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 { isNoWriteFetchHeadUnsupportedError } from '../shared/git-fetch-head-capability' +import { resolveGitFetchHeadCommand, runWithGitFetchHeadLock } from '../shared/git-fetch-head-lock' import { + REBASE_FROM_BASE_OPERATION_TIMEOUT_MS, REBASE_SOURCE_FETCH_TIMEOUT_MS, resolveGitRemoteRebaseSource } from '../shared/git-rebase-source' @@ -100,10 +103,12 @@ import { endSubprocessStdin } from '../shared/subprocess-stdin-write' import { clearGitStatusLineStatsCache } from '../shared/git-status-line-stats-cache' import { invalidateGitBranchLineTotalInFlight } from '../shared/git-branch-line-total' import { streamRelayGitStdout } from './git-stdout-stream' +import { runProcess } from '../shared/child-process/run-process' const execFileAsync = promisify(execFile) const MAX_GIT_BUFFER = 10 * 1024 * 1024 const BULK_CHUNK_SIZE = 100 +const GIT_REBASE_PROCESS_FALLBACK_TIMEOUT_MS = 2_147_000_000 function resolveSubmoduleStatusArea( params: Record @@ -182,6 +187,47 @@ function execFileWithStdin( }) } +async function runGitToTermination( + args: string[], + options: ExecFileOptions, + stdin: string | undefined +): Promise<{ stdout: string; stderr: string }> { + const result = await runProcess({ + program: 'git', + args, + cwd: typeof options.cwd === 'string' ? options.cwd : undefined, + env: options.env, + timeoutMs: + typeof options.timeout === 'number' + ? options.timeout + : GIT_REBASE_PROCESS_FALLBACK_TIMEOUT_MS, + maxOutputBytes: typeof options.maxBuffer === 'number' ? options.maxBuffer : MAX_GIT_BUFFER, + signal: options.signal, + terminationBarrier: true, + ...(stdin === undefined ? {} : { input: stdin }) + }) + if (result.code === 0 && !result.timedOut && !options.signal?.aborted) { + return { stdout: result.stdout, stderr: result.stderr } + } + const error = new Error( + result.timedOut + ? `git ${args[0] ?? 'command'} timed out.` + : options.signal?.aborted + ? 'The operation was aborted.' + : result.stderr.trim() || `git ${args[0] ?? 'command'} failed.` + ) + if (options.signal?.aborted) { + error.name = 'AbortError' + } + throw Object.assign(error, { + code: result.code, + killed: result.timedOut || result.signal !== null || options.signal?.aborted === true, + signal: result.signal, + stdout: result.stdout, + stderr: result.stderr + }) +} + export class GitHandler { private dispatcher: RelayDispatcher private readonly gitDiffReadDedupe = new InFlightPromiseDedupe() @@ -252,7 +298,7 @@ export class GitHandler { this.dispatcher.onRequest('git.push', (p) => this.push(p)) this.dispatcher.onRequest('git.pull', (p) => this.pull(p)) this.dispatcher.onRequest('git.fastForward', (p) => this.fastForward(p)) - this.dispatcher.onRequest('git.rebaseFromBase', (p) => this.rebaseFromBase(p)) + this.dispatcher.onRequest('git.rebaseFromBase', (p, context) => this.rebaseFromBase(p, context)) this.dispatcher.onRequest('git.branchDiff', (p, context) => this.branchDiff(p, context)) this.dispatcher.onRequest('git.commitDiff', (p, context) => this.commitDiff(p, context)) this.dispatcher.onRequest('git.listWorktrees', (p, context) => this.listWorktrees(p, context)) @@ -333,25 +379,36 @@ export class GitHandler { nonInteractive?: boolean stdin?: string timeout?: number + terminationBarrier?: boolean } ): Promise<{ stdout: string; stderr: string }> { - const env = opts?.nonInteractive ? buildRelayUnattendedGitEnv() : buildRelayGitEnv() - if (opts?.disableOptionalLocks) { - env.GIT_OPTIONAL_LOCKS = '0' + const expandedCwd = expandTilde(cwd) + const run = async (): Promise<{ stdout: string; stderr: string }> => { + const env = opts?.nonInteractive ? buildRelayUnattendedGitEnv() : buildRelayGitEnv() + if (opts?.disableOptionalLocks) { + env.GIT_OPTIONAL_LOCKS = '0' + } + const execOptions = { + cwd: expandedCwd, + env, + encoding: 'utf-8', + maxBuffer: opts?.maxBuffer ?? MAX_GIT_BUFFER, + timeout: opts?.timeout, + signal: opts?.signal + } satisfies ExecFileOptions + if (opts?.terminationBarrier) { + return runGitToTermination(args, execOptions, opts.stdin) + } + if (opts?.stdin !== undefined) { + return execFileWithStdin('git', args, execOptions, opts.stdin) + } + const { stdout, stderr } = await execFileAsync('git', args, execOptions) + return { stdout: String(stdout), stderr: String(stderr) } } - const execOptions = { - cwd: expandTilde(cwd), - env, - encoding: 'utf-8', - maxBuffer: opts?.maxBuffer ?? MAX_GIT_BUFFER, - timeout: opts?.timeout, - signal: opts?.signal - } satisfies ExecFileOptions - if (opts?.stdin !== undefined) { - return execFileWithStdin('git', args, execOptions, opts.stdin) - } - const { stdout, stderr } = await execFileAsync('git', args, execOptions) - return { stdout: String(stdout), stderr: String(stderr) } + const command = resolveGitFetchHeadCommand(args, expandedCwd) + return command.needsLock + ? runWithGitFetchHeadLock(command.cwd, opts?.signal, run, command.gitDir) + : run() } private async gitBuffer(args: string[], cwd: string): Promise { @@ -1132,37 +1189,76 @@ export class GitHandler { await this.pullWithArgs(params, ['--ff-only']) } - private async rebaseFromBase(params: Record) { + private async rebaseFromBase(params: Record, context?: RequestContext) { this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const baseRef = params.baseRef as string let rebaseRef: string | null = null + const controller = new AbortController() + const abortFromContext = () => controller.abort() + if (context?.signal?.aborted) { + controller.abort() + } else { + context?.signal?.addEventListener('abort', abortFromContext, { once: true }) + } + const timeout = setTimeout(() => controller.abort(), REBASE_FROM_BASE_OPERATION_TIMEOUT_MS) try { try { const source = await resolveGitRemoteRebaseSource( - ((args) => this.git(args, worktreePath)) as GitCommandRunner, + ((args) => + this.git(args, worktreePath, { + signal: controller.signal, + terminationBarrier: true + })) as GitCommandRunner, baseRef ) let forkPoint: string | null = null + let hasHead = true try { const { stdout } = await this.git( ['merge-base', '--fork-point', `refs/remotes/${source.displayName}`, 'HEAD'], - worktreePath + worktreePath, + { signal: controller.signal, terminationBarrier: true } ) forkPoint = stdout.trim() || null } catch { // A first fetch or an unhelpful reflog falls back to Git's merge-base behavior. + try { + await this.git(['rev-parse', '--verify', 'HEAD'], worktreePath, { + signal: controller.signal, + terminationBarrier: true + }) + } catch { + hasHead = false + } } // 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, - { timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS } + const fetchArgs = [source.remoteName, `+refs/heads/${source.branchName}:${rebaseRef}`] + await this.gitCapabilities.runWithFallback( + 'fetch-no-write-fetch-head', + () => + this.git(['fetch', '--no-write-fetch-head', ...fetchArgs], worktreePath, { + timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS, + signal: controller.signal, + terminationBarrier: true + }), + () => + this.git(['fetch', ...fetchArgs], worktreePath, { + timeout: REBASE_SOURCE_FETCH_TIMEOUT_MS, + signal: controller.signal, + terminationBarrier: true + }), + isNoWriteFetchHeadUnsupportedError ) await this.git( - forkPoint ? ['rebase', '--onto', rebaseRef, forkPoint] : ['rebase', rebaseRef], - worktreePath + hasHead + ? forkPoint + ? ['rebase', '--onto', rebaseRef, forkPoint] + : ['rebase', rebaseRef] + : ['merge', '--ff-only', rebaseRef], + worktreePath, + { signal: controller.signal, terminationBarrier: true } ) } catch (error) { throw new Error(normalizeGitErrorMessage(error, 'pull')) @@ -1175,6 +1271,8 @@ export class GitHandler { // Cleanup must not hide the fetch or rebase result. } } + clearTimeout(timeout) + context?.signal?.removeEventListener('abort', abortFromContext) this.clearGitMutationReadCaches() } } diff --git a/src/shared/child-process/process-tree-termination.test.ts b/src/shared/child-process/process-tree-termination.test.ts new file mode 100644 index 00000000000..7d090a6aa81 --- /dev/null +++ b/src/shared/child-process/process-tree-termination.test.ts @@ -0,0 +1,74 @@ +import { EventEmitter } from 'node:events' +import type { ChildProcess } from 'node:child_process' +import { afterEach, describe, expect, it, vi } from 'vitest' + +const { spawnMock } = vi.hoisted(() => ({ spawnMock: vi.fn() })) + +vi.mock('node:child_process', () => ({ spawn: spawnMock })) + +import { forceTerminateProcessTree } from './process-tree-termination' + +function mockProcess(pid: number): ChildProcess { + const child = new EventEmitter() as EventEmitter & { + pid: number + kill: ReturnType + } + child.pid = pid + child.kill = vi.fn((_signal?: NodeJS.Signals | number) => true) + return child as unknown as ChildProcess +} + +async function withWindows(run: () => Promise): Promise { + const original = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + await run() + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: original }) + } +} + +describe('forceTerminateProcessTree', () => { + afterEach(() => { + spawnMock.mockReset() + vi.useRealTimers() + }) + + it('waits for Windows taskkill tree completion', async () => { + await withWindows(async () => { + const child = mockProcess(1234) + const taskkill = mockProcess(5678) + spawnMock.mockReturnValue(taskkill) + let settled = false + const pending = forceTerminateProcessTree(child) + void pending.then(() => { + settled = true + }) + + await Promise.resolve() + expect(settled).toBe(false) + expect(spawnMock).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '1234', '/t', '/f'], + expect.objectContaining({ shell: false, windowsHide: true }) + ) + + taskkill.emit('close', 0) + await expect(pending).resolves.toBe(true) + expect(child.kill).not.toHaveBeenCalled() + }) + }) + + it('falls back to the root when Windows tree termination fails', async () => { + await withWindows(async () => { + const child = mockProcess(1234) + const taskkill = mockProcess(5678) + spawnMock.mockReturnValue(taskkill) + const pending = forceTerminateProcessTree(child) + + taskkill.emit('close', 1) + await expect(pending).resolves.toBe(false) + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + }) + }) +}) diff --git a/src/shared/child-process/process-tree-termination.ts b/src/shared/child-process/process-tree-termination.ts new file mode 100644 index 00000000000..53ea7f4bb96 --- /dev/null +++ b/src/shared/child-process/process-tree-termination.ts @@ -0,0 +1,150 @@ +import { spawn as nodeSpawn, type ChildProcess } from 'node:child_process' + +const PROBE_INTERVAL_MS = 25 +const SUBPROCESS_TIMEOUT_MS = 2_000 +const MAX_PS_OUTPUT_BYTES = 8 * 1024 * 1024 + +export function signalProcessTree(child: ChildProcess, signal?: NodeJS.Signals): Promise { + if (!child.pid) { + killRoot(child, signal) + return Promise.resolve(true) + } + if (process.platform === 'win32') { + return taskkillTree(child, signal) + } + try { + process.kill(-child.pid, signal) + return Promise.resolve(true) + } catch { + return Promise.resolve(!processGroupExists(child.pid)) + } +} + +export async function forceTerminateProcessTree(child: ChildProcess): Promise { + const signaled = await signalProcessTree(child, 'SIGKILL') + if (!signaled) { + return false + } + if (process.platform !== 'win32' && child.pid) { + await waitForPosixProcessGroupQuiescence(child.pid) + } + return true +} + +function taskkillTree(child: ChildProcess, signal?: NodeJS.Signals): Promise { + return new Promise((resolve) => { + let killer: ChildProcess + try { + killer = nodeSpawn('taskkill', ['/pid', String(child.pid), '/t', '/f'], { + stdio: 'ignore', + windowsHide: true, + shell: false + }) + } catch { + killRoot(child, signal) + resolve(false) + return + } + let settled = false + const finish = (fallback: boolean): void => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + if (fallback) { + killRoot(child, signal) + } + resolve(!fallback) + } + killer.once('error', () => finish(true)) + killer.once('close', (code) => finish(code !== 0)) + const timer = setTimeout(() => { + killer.kill() + finish(true) + }, SUBPROCESS_TIMEOUT_MS) + timer.unref?.() + }) +} + +async function waitForPosixProcessGroupQuiescence(processGroupId: number): Promise { + while (true) { + const states = await readPosixProcessGroupStates(processGroupId) + if ( + states ? states.every((state) => state.startsWith('Z')) : !processGroupExists(processGroupId) + ) { + return + } + await new Promise((resolve) => setTimeout(resolve, PROBE_INTERVAL_MS)) + } +} + +function readPosixProcessGroupStates(processGroupId: number): Promise { + return new Promise((resolve) => { + let probe: ChildProcess + try { + probe = nodeSpawn('ps', ['-axo', 'pgid=,state='], { + stdio: ['ignore', 'pipe', 'ignore'], + windowsHide: true, + shell: false + }) + } catch { + resolve(null) + return + } + let output = '' + let truncated = false + let settled = false + const finish = (states: string[] | null): void => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + resolve(states) + } + probe.stdout?.on('data', (chunk: Buffer | string) => { + const text = chunk.toString() + if (output.length + text.length > MAX_PS_OUTPUT_BYTES) { + truncated = true + return + } + output += text + }) + probe.stdout?.on('error', () => {}) + probe.once('error', () => finish(null)) + probe.once('close', (code) => { + if (code !== 0 || truncated) { + finish(null) + return + } + const states = output.split('\n').flatMap((line) => { + const match = line.trim().match(/^(\d+)\s+(\S+)/) + return match && Number(match[1]) === processGroupId ? [match[2]] : [] + }) + finish(states) + }) + const timer = setTimeout(() => { + probe.kill() + finish(null) + }, SUBPROCESS_TIMEOUT_MS) + timer.unref?.() + }) +} + +function processGroupExists(processGroupId: number): boolean { + try { + process.kill(-processGroupId, 0) + return true + } catch (error) { + return (error as NodeJS.ErrnoException).code !== 'ESRCH' + } +} + +function killRoot(child: ChildProcess, signal?: NodeJS.Signals): void { + try { + child.kill(signal) + } catch { + /* already gone */ + } +} diff --git a/src/shared/child-process/run-process.test.ts b/src/shared/child-process/run-process.test.ts index 19cea26096e..55ee378c69e 100644 --- a/src/shared/child-process/run-process.test.ts +++ b/src/shared/child-process/run-process.test.ts @@ -1,4 +1,7 @@ import { describe, expect, it } from 'vitest' +import { mkdtemp, readFile, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import * as path from 'node:path' import { resolveSpawn, runProcess, runProcessSync } from './run-process' import { WINDOWS_ARGUMENT_CORPUS } from './__fixtures__/windows-argument-corpus' @@ -126,6 +129,42 @@ describe('abort', () => { // Not a timeout: the caller asked it to stop. expect(result.timedOut).toBe(false) }, 20_000) + + it.skipIf(process.platform === 'win32')( + 'kills the process group and waits for confirmed root exit with a termination barrier', + async () => { + const root = await mkdtemp(path.join(tmpdir(), 'run-process-barrier-')) + const marker = path.join(root, 'descendant-state') + const descendantScript = + `printf ready > "$1";trap 'printf signaled > "$1"' TERM;` + `while :;do sleep 1;done` + const controller = new AbortController() + const pending = runProcess({ + program: process.execPath, + args: [ + '-e', + `const {spawn}=require('node:child_process');` + + `spawn('/bin/sh',${JSON.stringify(['-c', descendantScript, 'sh', marker])},{stdio:'ignore'});` + + `setInterval(()=>{},1000)` + ], + timeoutMs: 60_000, + signal: controller.signal, + terminationBarrier: true + }) + try { + await expect + .poll(() => readFile(marker, 'utf8').catch(() => ''), { timeout: 10_000 }) + .toBe('ready') + controller.abort() + await pending + await expect.poll(() => readFile(marker, 'utf8')).toBe('signaled') + } finally { + controller.abort() + await pending + await rm(root, { recursive: true, force: true }) + } + }, + 30_000 + ) }) describe('stdin delivery failures', () => { @@ -151,8 +190,7 @@ describe('a signal that is already aborted', () => { controller.abort() const startedAt = Date.now() const result = await runProcess({ - program: process.execPath, - args: ['-e', 'setInterval(() => {}, 1000)'], + program: path.join(tmpdir(), 'orca-must-not-spawn'), timeoutMs: 30_000, signal: controller.signal }) diff --git a/src/shared/child-process/run-process.ts b/src/shared/child-process/run-process.ts index af9a75ba01b..3afd161a392 100644 --- a/src/shared/child-process/run-process.ts +++ b/src/shared/child-process/run-process.ts @@ -5,6 +5,7 @@ import { type SpawnOptions as NodeSpawnOptions } from 'node:child_process' import { buildWindowsCmdShimCommandLine, isCmdInterpretedProgram } from './windows-command-line' +import { forceTerminateProcessTree, signalProcessTree } from './process-tree-termination' /** * The single place Orca starts a child process. @@ -39,6 +40,8 @@ export type ProcessSpec = { maxOutputBytes?: number /** Kills the process when aborted; the result still reports the exit. */ signal?: AbortSignal + /** Kill the whole process tree and do not settle until the root confirms exit. */ + terminationBarrier?: boolean } export type ProcessResult = { @@ -89,7 +92,8 @@ export function resolveSpawn(spec: ProcessSpec, platform: NodeJS.Platform): Reso windowsHide: true, // Why never `shell: true`: it concatenates arguments without escaping (Node // itself warns DEP0190) and it silently makes windowsHide a no-op. - shell: false + shell: false, + ...(spec.terminationBarrier && platform !== 'win32' ? { detached: true } : {}) } if (platform !== 'win32' || !isCmdInterpretedProgram(spec.program)) { @@ -155,6 +159,9 @@ function createOutputSink(maxBytes: number): { * the process could not be started at all. */ export function runProcess(spec: ProcessSpec): Promise { + if (spec.signal?.aborted) { + return Promise.resolve({ code: null, signal: null, stdout: '', stderr: '', timedOut: false }) + } const maxOutputBytes = spec.maxOutputBytes ?? DEFAULT_MAX_OUTPUT_BYTES return new Promise((resolve, reject) => { @@ -170,6 +177,10 @@ export function runProcess(spec: ProcessSpec): Promise { const stderr = createOutputSink(maxOutputBytes) let timedOut = false let settled = false + let barrierStopping = false + let barrierReady = false + let initialBarrierTermination: Promise | undefined + let deferredClose: { code: number | null; signal: NodeJS.Signals | null } | null = null const settle = (act: () => void): void => { if (settled) { @@ -196,6 +207,11 @@ export function runProcess(spec: ProcessSpec): Promise { let graceTimer: ReturnType | undefined + const resolveFromClose = (code: number | null, signal: NodeJS.Signals | null): void => + settle(() => + resolve({ code, signal, stdout: stdout.text(), stderr: stderr.text(), timedOut }) + ) + /** * Stop the child, then settle whether or not it complies. * @@ -205,18 +221,53 @@ export function runProcess(spec: ProcessSpec): Promise { * promise, so a single unkillable child would wedge every one of them. */ const stopAndSettle = (): void => { - terminate(child) - graceTimer ??= setTimeout(() => { - terminate(child, 'SIGKILL') - settle(() => - resolve({ - code: null, - signal: null, - stdout: stdout.text(), - stderr: stderr.text(), - timedOut + if (spec.terminationBarrier) { + barrierStopping = true + initialBarrierTermination ??= signalProcessTree(child) + if (process.platform === 'win32') { + void initialBarrierTermination.then((terminated) => { + if (!terminated) { + return + } + barrierReady = true + if (deferredClose) { + resolveFromClose(deferredClose.code, deferredClose.signal) + } }) - ) + } + } else { + terminate(child) + } + graceTimer ??= setTimeout(() => { + if (spec.terminationBarrier) { + const initialTermination = initialBarrierTermination ?? Promise.resolve(false) + if (process.platform === 'win32') { + void initialTermination.then((terminated) => { + if (!terminated) { + terminate(child, 'SIGKILL') + } + barrierReady = true + if (deferredClose) { + resolveFromClose(deferredClose.code, deferredClose.signal) + } + }) + return + } + void Promise.all([initialTermination, forceTerminateProcessTree(child)]).then( + ([initialTerminated, forceTerminated]) => { + if (!initialTerminated || !forceTerminated) { + return + } + barrierReady = true + if (deferredClose) { + resolveFromClose(deferredClose.code, deferredClose.signal) + } + } + ) + return + } + terminate(child, 'SIGKILL') + resolveFromClose(null, null) }, PROCESS_EXIT_GRACE_MS) graceTimer.unref?.() } @@ -239,11 +290,13 @@ export function runProcess(spec: ProcessSpec): Promise { } child.once('error', (error) => settle(() => reject(error))) - child.once('close', (code, signal) => - settle(() => - resolve({ code, signal, stdout: stdout.text(), stderr: stderr.text(), timedOut }) - ) - ) + child.once('close', (code, signal) => { + if (barrierStopping && !barrierReady) { + deferredClose = { code, signal } + return + } + resolveFromClose(code, signal) + }) // Why close rather than leave open: a child that reads stdin (a hook // draining its payload, a CLI probing for a TTY) otherwise blocks until the @@ -252,12 +305,7 @@ export function runProcess(spec: ProcessSpec): Promise { }) } -/** - * Best-effort termination of a captured child. - * - * Deliberately root-only: descendant reaping is the job-object owner's - * responsibility, not every caller's. - */ +/** Best-effort root termination, or whole-tree termination for barrier callers. */ function terminate(child: ChildProcess, signal?: NodeJS.Signals): void { try { child.kill(signal) diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index a33adcd7d76..712910cef6d 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -1,5 +1,5 @@ import { execFile } from 'node:child_process' -import { mkdtemp, rename, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, readFile, rename, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { promisify } from 'node:util' @@ -9,6 +9,7 @@ import { isUnsupportedMergeTreeWriteTreeError } from './git-merge-tree-capability' import { isForEachRefExcludeUnsupportedError } from './git-ref-command-capabilities' +import { isNoWriteFetchHeadUnsupportedError } from './git-fetch-head-capability' import { hasUnsupportedRevParsePathFormatEcho, isUnsupportedWorktreeListZError @@ -151,6 +152,14 @@ describeBinaryCompatibility('real Git binary compatibility', () => { }) it('recognizes ref and merge-tree compatibility boundaries', async () => { + const fetchHeadPath = join(repoPath, '.git', 'FETCH_HEAD') + await writeFile(fetchHeadPath, 'sentinel\n') + await expectPreferredOrRecognizedFallback( + ['fetch', '--no-write-fetch-head', '.', '+HEAD:refs/orca/compat/no-write-fetch-head'], + supports(2, 29), + isNoWriteFetchHeadUnsupportedError + ) + await expect(readFile(fetchHeadPath, 'utf-8')).resolves.toBe('sentinel\n') await expectPreferredOrRecognizedFallback( ['for-each-ref', '--format=%(refname)', '--exclude=refs/remotes/**/HEAD', '--count=10'], supports(2, 42), diff --git a/src/shared/git-capability-cache.ts b/src/shared/git-capability-cache.ts index a3959cc959b..9e3d4ce89e0 100644 --- a/src/shared/git-capability-cache.ts +++ b/src/shared/git-capability-cache.ts @@ -3,6 +3,7 @@ export const GIT_CAPABILITY_RETRY_INTERVAL_MS = 30 * 60_000 export type GitCapability = + | 'fetch-no-write-fetch-head' | 'for-each-ref-exclude' | 'merge-tree-merge-base' | 'merge-tree-write-tree' diff --git a/src/shared/git-fetch-head-capability.test.ts b/src/shared/git-fetch-head-capability.test.ts new file mode 100644 index 00000000000..8ea57167bdd --- /dev/null +++ b/src/shared/git-fetch-head-capability.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from 'vitest' +import { isNoWriteFetchHeadUnsupportedError } from './git-fetch-head-capability' + +describe('isNoWriteFetchHeadUnsupportedError', () => { + it.each([ + new Error("error: unknown option `no-write-fetch-head'"), + { stderr: "error: unrecognized option '--no-write-fetch-head'" }, + { stdout: "fatal: invalid option: 'no-write-fetch-head'" } + ])('recognizes old-Git option rejection shapes', (error) => { + expect(isNoWriteFetchHeadUnsupportedError(error)).toBe(true) + }) + + it('does not classify an ordinary fetch failure as unsupported', () => { + expect( + isNoWriteFetchHeadUnsupportedError({ stderr: 'fatal: could not read from remote repository' }) + ).toBe(false) + }) +}) diff --git a/src/shared/git-fetch-head-capability.ts b/src/shared/git-fetch-head-capability.ts new file mode 100644 index 00000000000..b06856ed298 --- /dev/null +++ b/src/shared/git-fetch-head-capability.ts @@ -0,0 +1,16 @@ +function gitErrorText(error: unknown): string { + if (typeof error !== 'object' || error === null) { + return error instanceof Error ? error.message : String(error) + } + return ['message', 'stderr', 'stdout'] + .map((key) => (error as Record)[key]) + .filter((value): value is string => typeof value === 'string') + .join('\n') +} + +export function isNoWriteFetchHeadUnsupportedError(error: unknown): boolean { + const output = gitErrorText(error) + return /(?:unknown|invalid|unrecognized) option(?::\s*|\s+)[`']?(?:--?)?no-write-fetch-head[`']?(?:\s|$)/i.test( + output + ) +} diff --git a/src/shared/git-fetch-head-lock.test.ts b/src/shared/git-fetch-head-lock.test.ts new file mode 100644 index 00000000000..fd6833b9b82 --- /dev/null +++ b/src/shared/git-fetch-head-lock.test.ts @@ -0,0 +1,145 @@ +import { describe, expect, it } from 'vitest' +import { mkdtemp, mkdir, rm, symlink } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { resolveGitFetchHeadCommand, runWithGitFetchHeadLock } from './git-fetch-head-lock' + +describe('runWithGitFetchHeadLock', () => { + it.each([ + { args: ['fetch', 'origin'], expected: true }, + { args: ['pull', '--rebase'], expected: true }, + { args: ['-c', 'maintenance.auto=false', 'fetch', 'origin'], expected: true }, + { args: ['fetch', '--no-write-fetch-head', 'origin'], expected: false }, + { args: ['fetch', '--no-write-fetch-head', '--write-fetch-head'], expected: true }, + { args: ['rev-parse', 'fetch'], expected: false } + ])('classifies FETCH_HEAD operations: $args', ({ args, expected }) => { + expect(resolveGitFetchHeadCommand(args, '/repo').needsLock).toBe(expected) + }) + + it('resolves -C and --git-dir before deriving the lock identity', () => { + expect(resolveGitFetchHeadCommand(['-C', 'repo', 'fetch'], '/tmp')).toMatchObject({ + cwd: '/tmp/repo', + needsLock: true + }) + expect(resolveGitFetchHeadCommand(['--git-dir=/repo/.git', 'fetch'], '/tmp')).toMatchObject({ + gitDir: '/repo/.git', + needsLock: true + }) + }) + + it('serializes FETCH_HEAD users in one worktree without blocking another', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'fetch-head-lock-isolation-')) + const repo = path.join(root, 'repo') + const otherRepo = path.join(root, 'other') + await Promise.all([ + mkdir(path.join(repo, '.git'), { recursive: true }), + mkdir(path.join(otherRepo, '.git'), { recursive: true }) + ]) + let releaseFirst!: () => void + const firstGate = new Promise((resolve) => { + releaseFirst = resolve + }) + let markFirstStarted!: () => void + const firstStarted = new Promise((resolve) => { + markFirstStarted = resolve + }) + const order: string[] = [] + + const first = runWithGitFetchHeadLock(repo, undefined, async () => { + order.push('first:start') + markFirstStarted() + await firstGate + order.push('first:end') + }) + await firstStarted + const second = runWithGitFetchHeadLock(repo, undefined, async () => { + order.push('second') + }) + const other = runWithGitFetchHeadLock(otherRepo, undefined, async () => { + order.push('other') + }) + + try { + await other + expect(order).toEqual(['first:start', 'other']) + releaseFirst() + await Promise.all([first, second]) + expect(order).toEqual(['first:start', 'other', 'first:end', 'second']) + } finally { + releaseFirst() + await Promise.allSettled([first, second, other]) + await rm(root, { recursive: true, force: true }) + } + }) + + it('does not run a queued operation after cancellation', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'fetch-head-lock-cancel-')) + const repo = path.join(root, 'repo') + await mkdir(path.join(repo, '.git'), { recursive: true }) + let releaseFirst!: () => void + const firstGate = new Promise((resolve) => { + releaseFirst = resolve + }) + let markFirstStarted!: () => void + const firstStarted = new Promise((resolve) => { + markFirstStarted = resolve + }) + const first = runWithGitFetchHeadLock(repo, undefined, async () => { + markFirstStarted() + await firstGate + }) + await firstStarted + const controller = new AbortController() + const queued = runWithGitFetchHeadLock(repo, controller.signal, async () => 'ran') + + try { + controller.abort() + await expect(queued).rejects.toMatchObject({ name: 'AbortError' }) + } finally { + releaseFirst() + await Promise.allSettled([first, queued]) + await rm(root, { recursive: true, force: true }) + } + }) + + it('serializes root, nested, and symlink aliases of one worktree', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'fetch-head-lock-')) + const repo = path.join(root, 'repo') + const nested = path.join(repo, 'nested') + const alias = path.join(root, 'alias') + await mkdir(path.join(repo, '.git'), { recursive: true }) + await mkdir(nested) + await symlink(repo, alias) + let release!: () => void + const gate = new Promise((resolve) => { + release = resolve + }) + let markFirstStarted!: () => void + const firstStarted = new Promise((resolve) => { + markFirstStarted = resolve + }) + const order: string[] = [] + try { + const first = runWithGitFetchHeadLock(repo, undefined, async () => { + order.push('root') + markFirstStarted() + await gate + }) + await firstStarted + const nestedRun = runWithGitFetchHeadLock(nested, undefined, async () => { + order.push('nested') + }) + const aliasRun = runWithGitFetchHeadLock(alias, undefined, async () => { + order.push('alias') + }) + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(order).toEqual(['root']) + release() + await Promise.all([first, nestedRun, aliasRun]) + expect(order[0]).toBe('root') + expect(new Set(order.slice(1))).toEqual(new Set(['nested', 'alias'])) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) +}) diff --git a/src/shared/git-fetch-head-lock.ts b/src/shared/git-fetch-head-lock.ts new file mode 100644 index 00000000000..e8d1a3c395f --- /dev/null +++ b/src/shared/git-fetch-head-lock.ts @@ -0,0 +1,171 @@ +import * as path from 'node:path' +import { readFile, realpath, stat } from 'node:fs/promises' + +type GitFetchHeadLockLane = { + tail: Promise + release: () => void +} + +const lanes = new Map() +const GLOBAL_OPTIONS_WITH_VALUE = new Set([ + '-c', + '-C', + '--git-dir', + '--work-tree', + '--namespace', + '--super-prefix', + '--config-env', + '--exec-path' +]) + +type GitFetchHeadCommand = { needsLock: boolean; cwd: string; gitDir?: string } + +export function resolveGitFetchHeadCommand( + args: readonly string[], + initialCwd: string +): GitFetchHeadCommand { + let cwd = initialCwd + let gitDir: string | undefined + let subcommandIndex = -1 + for (let index = 0; index < args.length; index += 1) { + const arg = args[index] + if (arg === '-C' && args[index + 1]) { + cwd = path.resolve(cwd, args[index + 1]) + index += 1 + continue + } + if (arg.startsWith('-C') && arg.length > 2) { + cwd = path.resolve(cwd, arg.slice(2)) + continue + } + if (arg === '--git-dir' && args[index + 1]) { + gitDir = path.resolve(cwd, args[index + 1]) + index += 1 + continue + } + if (arg.startsWith('--git-dir=')) { + gitDir = path.resolve(cwd, arg.slice('--git-dir='.length)) + continue + } + if (GLOBAL_OPTIONS_WITH_VALUE.has(arg)) { + index += 1 + continue + } + if (arg.startsWith('-')) { + continue + } + subcommandIndex = index + break + } + const subcommand = args[subcommandIndex] + if (subcommand === 'pull') { + return { needsLock: true, cwd, gitDir } + } + if (subcommand !== 'fetch') { + return { needsLock: false, cwd, gitDir } + } + let writesFetchHead = true + for (const arg of args.slice(subcommandIndex + 1)) { + if (arg === '--no-write-fetch-head') { + writesFetchHead = false + } else if (arg === '--write-fetch-head') { + writesFetchHead = true + } + } + return { needsLock: writesFetchHead, cwd, gitDir } +} + +async function fetchHeadPath( + worktreePath: string, + signal: AbortSignal | undefined, + explicitGitDir?: string +): Promise { + let current = await realpath(worktreePath).catch(() => path.resolve(worktreePath)) + let gitDir = explicitGitDir + while (!gitDir) { + const dotGitPath = path.join(current, '.git') + try { + const metadata = await stat(dotGitPath) + if (metadata.isDirectory()) { + gitDir = dotGitPath + break + } + const contents = await readFile(dotGitPath, { encoding: 'utf-8', signal }) + const match = contents.match(/^gitdir:\s*(.+)\s*$/m) + if (match) { + gitDir = path.resolve(current, match[1]) + break + } + } catch { + if (signal?.aborted) { + throw abortError() + } + } + const parent = path.dirname(current) + if (parent === current) { + gitDir = path.join(current, '.git') + break + } + current = parent + } + const canonicalGitDir = await realpath(gitDir).catch(() => path.resolve(gitDir)) + return path.join(canonicalGitDir, 'FETCH_HEAD') +} + +function abortError(): Error { + const error = new Error('The operation was aborted.') + error.name = 'AbortError' + return error +} + +async function waitForPredecessor( + predecessor: Promise, + signal: AbortSignal | undefined +): Promise { + if (!signal) { + await predecessor.catch(() => undefined) + return + } + if (signal.aborted) { + throw abortError() + } + let rejectAbort!: (error: Error) => void + const aborted = new Promise((_resolve, reject) => { + rejectAbort = reject + }) + const onAbort = () => rejectAbort(abortError()) + signal.addEventListener('abort', onAbort, { once: true }) + try { + await Promise.race([predecessor.catch(() => undefined), aborted]) + } finally { + signal.removeEventListener('abort', onAbort) + } +} + +export async function runWithGitFetchHeadLock( + worktreePath: string, + signal: AbortSignal | undefined, + run: () => Promise, + explicitGitDir?: string +): Promise { + const key = await fetchHeadPath(worktreePath, signal, explicitGitDir) + const predecessor = lanes.get(key)?.tail ?? Promise.resolve() + let release!: () => void + const current = new Promise((resolve) => { + release = resolve + }) + const lane = { tail: predecessor.catch(() => undefined).then(() => current), release } + lanes.set(key, lane) + void lane.tail.then(() => { + if (lanes.get(key) === lane) { + lanes.delete(key) + } + }) + + try { + await waitForPredecessor(predecessor, signal) + return await run() + } finally { + lane.release() + } +} diff --git a/src/shared/git-rebase-source.ts b/src/shared/git-rebase-source.ts index a4b44b46d79..e016a23a6e3 100644 --- a/src/shared/git-rebase-source.ts +++ b/src/shared/git-rebase-source.ts @@ -1,5 +1,7 @@ // 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 const REBASE_FROM_BASE_OPERATION_TIMEOUT_MS = REBASE_SOURCE_FETCH_TIMEOUT_MS + 60_000 +export const REBASE_FROM_BASE_RPC_TIMEOUT_MS = REBASE_FROM_BASE_OPERATION_TIMEOUT_MS + 5_000 export type GitCommandRunner = (args: string[]) => Promise<{ stdout: string }>