diff --git a/src/main/git/remote-rebase.ts b/src/main/git/remote-rebase.ts index 69eb03f1676..d978f644695 100644 --- a/src/main/git/remote-rebase.ts +++ b/src/main/git/remote-rebase.ts @@ -1,6 +1,7 @@ import { randomUUID } from 'node:crypto' import { normalizeGitErrorMessage } from '../../shared/git-remote-error' import { isNoWriteFetchHeadUnsupportedError } from '../../shared/git-fetch-head-capability' +import { runWithGitWorktreeOperationLock } from '../../shared/git-worktree-operation-lock' import { REBASE_SOURCE_FETCH_TIMEOUT_MS, resolveGitRemoteRebaseSource @@ -15,6 +16,16 @@ export async function gitPullRebaseFromBase( worktreePath: string, baseRef: string, options: GitRuntimeOptions = {} +): Promise { + await runWithGitWorktreeOperationLock(worktreePath, options.signal, () => + gitPullRebaseFromBaseUnlocked(worktreePath, baseRef, options) + ) +} + +async function gitPullRebaseFromBaseUnlocked( + worktreePath: string, + baseRef: string, + options: GitRuntimeOptions ): Promise { await runWithGitReadCacheInvalidation(async () => { const operationOptions = { diff --git a/src/relay/git-handler-remote-sync.test.ts b/src/relay/git-handler-remote-sync.test.ts index 9a43fe4697b..5fad0380e06 100644 --- a/src/relay/git-handler-remote-sync.test.ts +++ b/src/relay/git-handler-remote-sync.test.ts @@ -241,6 +241,92 @@ describe('GitHandler', () => { } }, 15_000) + it('rebases the selected linked worktree without moving the source worktree', async () => { + const bareDir = mkdtempSync(path.join(tmpdir(), 'relay-git-linked-rebase-bare-')) + const producerParent = mkdtempSync(path.join(tmpdir(), 'relay-git-linked-rebase-producer-')) + const producerDir = path.join(producerParent, 'repo') + const targetParent = mkdtempSync(path.join(tmpdir(), 'relay-git-linked-rebase-target-')) + const targetDir = path.join(targetParent, 'feature') + try { + execFileSync('git', ['init', '--bare'], { cwd: bareDir, stdio: 'pipe' }) + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'base.txt'), 'base') + gitCommit(tmpDir, 'base') + const baseBranch = execFileSync('git', ['branch', '--show-current'], { + cwd: tmpDir, + encoding: 'utf-8' + }).trim() + execFileSync('git', ['remote', 'add', 'origin', bareDir], { cwd: tmpDir, stdio: 'pipe' }) + execFileSync('git', ['push', '--set-upstream', 'origin', baseBranch], { + cwd: tmpDir, + stdio: 'pipe' + }) + execFileSync('git', ['worktree', 'add', '-b', 'feature', targetDir, 'HEAD'], { + cwd: tmpDir, + stdio: 'pipe' + }) + writeFileSync(path.join(targetDir, 'topic.txt'), 'topic') + gitCommit(targetDir, 'topic') + writeFileSync(path.join(tmpDir, 'source-dirty.txt'), 'leave me alone') + const sourceHeadBefore = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: tmpDir, + encoding: 'utf-8' + }).trim() + const sourceStatusBefore = execFileSync('git', ['status', '--short'], { + cwd: tmpDir, + encoding: 'utf-8' + }) + + execFileSync('git', ['clone', bareDir, producerDir], { stdio: 'pipe' }) + execFileSync('git', ['config', 'user.email', 'test@test.com'], { + cwd: producerDir, + stdio: 'pipe' + }) + execFileSync('git', ['config', 'user.name', 'Test'], { cwd: producerDir, stdio: 'pipe' }) + execFileSync('git', ['checkout', baseBranch], { cwd: producerDir, stdio: 'pipe' }) + writeFileSync(path.join(producerDir, 'latest.txt'), 'latest') + gitCommit(producerDir, 'latest base') + const latestBaseOid = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: producerDir, + encoding: 'utf-8' + }).trim() + execFileSync('git', ['push', 'origin', baseBranch], { cwd: producerDir, stdio: 'pipe' }) + + await dispatcher.callRequest('git.rebaseFromBase', { + worktreePath: targetDir, + baseRef: `origin/${baseBranch}` + }) + + expect(execFileSync('git', ['rev-parse', 'HEAD'], { cwd: tmpDir }).toString().trim()).toBe( + sourceHeadBefore + ) + expect(execFileSync('git', ['status', '--short'], { cwd: tmpDir, encoding: 'utf-8' })).toBe( + sourceStatusBefore + ) + expect( + execFileSync('git', ['rev-parse', 'HEAD~1'], { cwd: targetDir, encoding: 'utf-8' }).trim() + ).toBe(latestBaseOid) + expect( + execFileSync('git', ['reflog', '-1', '--format=%gs'], { + cwd: targetDir, + encoding: 'utf-8' + }) + ).toContain('rebase (finish)') + expect( + execFileSync('git', ['for-each-ref', '--format=%(refname)', 'refs/orca/rebase'], { + cwd: targetDir, + encoding: 'utf-8' + }).trim() + ).toBe('') + } finally { + await Promise.all([ + fs.rm(bareDir, { recursive: true, force: true }), + fs.rm(producerParent, { recursive: true, force: true }), + fs.rm(targetParent, { recursive: true, force: true }) + ]) + } + }, 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-')) diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 65a3242b7dc..d3d6f57bbaa 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -62,6 +62,7 @@ import { upstreamOnlyCommitsArePatchEquivalent } from '../shared/git-upstream-st 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 { runWithGitWorktreeOperationLock } from '../shared/git-worktree-operation-lock' import { resolveGitFetchHeadCommand, runWithGitFetchHeadLock } from '../shared/git-fetch-head-lock' import { REBASE_FROM_BASE_OPERATION_TIMEOUT_MS, @@ -1190,6 +1191,12 @@ export class GitHandler { } private async rebaseFromBase(params: Record, context?: RequestContext) { + return runWithGitWorktreeOperationLock(params.worktreePath as string, context?.signal, () => + this.runRebaseFromBase(params, context) + ) + } + + private async runRebaseFromBase(params: Record, context?: RequestContext) { this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string const baseRef = params.baseRef as string diff --git a/src/renderer/src/runtime/runtime-git-client.test.ts b/src/renderer/src/runtime/runtime-git-client.test.ts index 61d058d8f45..8754aee2b23 100644 --- a/src/renderer/src/runtime/runtime-git-client.test.ts +++ b/src/renderer/src/runtime/runtime-git-client.test.ts @@ -22,6 +22,7 @@ import { type RuntimeEnvironmentCallRequest } from './runtime-compatibility-test-fixture' import { clearRuntimeCompatibilityCacheForTests } from './runtime-rpc-client' +import { REBASE_FROM_BASE_RPC_TIMEOUT_MS } from '../../../shared/git-rebase-source' const gitStatus = vi.fn() const gitCancelStatus = vi.fn() @@ -616,7 +617,7 @@ describe('runtime git client', () => { selector: 'env-1', method: 'git.rebaseFromBase', params: { worktree: 'id:wt-1', baseRef: 'origin/main' }, - timeoutMs: 30_000 + timeoutMs: REBASE_FROM_BASE_RPC_TIMEOUT_MS }) }) diff --git a/src/renderer/src/runtime/runtime-git-client.ts b/src/renderer/src/runtime/runtime-git-client.ts index eeb31ab3124..b3b1c54f61e 100644 --- a/src/renderer/src/runtime/runtime-git-client.ts +++ b/src/renderer/src/runtime/runtime-git-client.ts @@ -23,6 +23,7 @@ import type { HostedReviewProvider } from '../../../shared/hosted-review' import type { ResolvedSourceControlAiGenerationParams } from '../../../shared/source-control-ai' import { getCommitMessageModelDiscoveryHostKeyForScope } from '../../../shared/commit-message-host-key' import type { GitHistoryOptions, GitHistoryResult } from '../../../shared/git-history' +import { REBASE_FROM_BASE_RPC_TIMEOUT_MS } from '../../../shared/git-rebase-source' import { getRepoIdFromWorktreeId, splitWorktreeIdForFilesystem } from '../../../shared/worktree/id' import { callRuntimeRpc, getActiveRuntimeTarget } from './runtime-rpc-client' import { toRuntimeWorktreeSelector } from './runtime-worktree-selector' @@ -548,7 +549,7 @@ export async function rebaseRuntimeGitFromBase( target, 'git.rebaseFromBase', { worktree: toRuntimeWorktreeSelector(context.worktreeId), baseRef }, - { timeoutMs: 30_000 } + { timeoutMs: REBASE_FROM_BASE_RPC_TIMEOUT_MS } ) } 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 51ebc3e5970..d608540ee30 100644 --- a/src/shared/child-process/run-process-termination-failure.test.ts +++ b/src/shared/child-process/run-process-termination-failure.test.ts @@ -38,7 +38,7 @@ describe('runProcess termination failure', () => { vi.clearAllMocks() }) - it('waits for root exit when bounded tree termination cannot be verified', async () => { + it('holds the result until the barrier deadline when tree termination cannot be verified', async () => { const child = mockChild() spawnMock.mockReturnValue(child) const pending = runProcess({ program: 'git', timeoutMs: 10, terminationBarrier: true }) @@ -52,6 +52,8 @@ describe('runProcess termination failure', () => { expect(settled).toBe(false) child.emit('exit', null, 'SIGKILL') + expect(settled).toBe(false) + await vi.advanceTimersByTimeAsync(10_000) await expect(pending).resolves.toMatchObject({ timedOut: true }) }) @@ -118,6 +120,28 @@ describe('runProcess termination failure', () => { expect(settled).toBe(false) child.emit('exit', null, 'SIGKILL') + await vi.advanceTimersByTimeAsync(10_000) await rejection }) + + it('kills the root when an object barrier cannot verify tree termination', async () => { + const child = mockChild() + spawnMock.mockReturnValue(child) + const pending = runProcess({ + program: 'wsl.exe', + timeoutMs: 10, + terminationBarrier: { + signal: vi.fn().mockResolvedValue(false), + force: vi.fn().mockResolvedValue(false) + } + }) + void pending.catch(() => {}) + + await vi.advanceTimersByTimeAsync(2_010) + + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + child.emit('exit', null, 'SIGKILL') + await vi.advanceTimersByTimeAsync(10_000) + await expect(pending).resolves.toMatchObject({ timedOut: true }) + }) }) diff --git a/src/shared/child-process/run-process.ts b/src/shared/child-process/run-process.ts index 53fb1102ea0..7a329928078 100644 --- a/src/shared/child-process/run-process.ts +++ b/src/shared/child-process/run-process.ts @@ -198,6 +198,7 @@ export function runProcess(spec: ProcessSpec): Promise { let deferredExit: { code: number | null; signal: NodeJS.Signals | null } | null = null let deferredClose: { code: number | null; signal: NodeJS.Signals | null } | null = null let deferredError: Error | null = null + let rootExitedBeforeBarrier = false const settle = (act: () => void): void => { if (settled) { @@ -257,7 +258,7 @@ export function runProcess(spec: ProcessSpec): Promise { const resolveBarrierIfSafe = (): void => { const rootExit = deferredClose ?? deferredExit - if (barrierTerminationVerified || (barrierAttemptComplete && rootExit)) { + if (barrierTerminationVerified || (rootExitedBeforeBarrier && rootExit)) { settleBarrierOutcome() return } @@ -304,6 +305,11 @@ export function runProcess(spec: ProcessSpec): Promise { ([initialTerminated, forceTerminated]) => { barrierAttemptComplete = true barrierTerminationVerified = initialTerminated || forceTerminated + if (!barrierTerminationVerified) { + // The barrier never confirmed the tree died, so the root + // would otherwise outlive the abort or timeout. + terminate(child, 'SIGKILL') + } resolveBarrierIfSafe() } ) @@ -323,6 +329,9 @@ export function runProcess(spec: ProcessSpec): Promise { ([_initialTerminated, forceTerminated]) => { barrierAttemptComplete = true barrierTerminationVerified = forceTerminated + if (!barrierTerminationVerified) { + terminate(child, 'SIGKILL') + } resolveBarrierIfSafe() } ) @@ -360,12 +369,18 @@ export function runProcess(spec: ProcessSpec): Promise { settle(() => reject(error)) }) child.once('exit', (code, signal) => { + if (!barrierStopping) { + rootExitedBeforeBarrier = true + } deferredExit = { code, signal } if (barrierStopping) { resolveBarrierIfSafe() } }) child.once('close', (code, signal) => { + if (!barrierStopping) { + rootExitedBeforeBarrier = true + } if (barrierStopping) { deferredClose = { code, signal } resolveBarrierIfSafe() diff --git a/src/shared/git-rebase-source.ts b/src/shared/git-rebase-source.ts index e016a23a6e3..2f878112445 100644 --- a/src/shared/git-rebase-source.ts +++ b/src/shared/git-rebase-source.ts @@ -1,7 +1,8 @@ // 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 +// Include the process barrier's 2s grace plus its 10s unverified-tree deadline. +export const REBASE_FROM_BASE_RPC_TIMEOUT_MS = REBASE_FROM_BASE_OPERATION_TIMEOUT_MS + 15_000 export type GitCommandRunner = (args: string[]) => Promise<{ stdout: string }> diff --git a/src/shared/git-worktree-operation-lock.test.ts b/src/shared/git-worktree-operation-lock.test.ts new file mode 100644 index 00000000000..27b102bf197 --- /dev/null +++ b/src/shared/git-worktree-operation-lock.test.ts @@ -0,0 +1,48 @@ +import { mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { runWithGitWorktreeOperationLock } from './git-worktree-operation-lock' + +describe('git worktree operation lock', () => { + let root: string | undefined + + afterEach(async () => { + if (root) { + await rm(root, { recursive: true, force: true }) + root = undefined + } + }) + + it('serializes mutations for the same linked worktree path', async () => { + root = await mkdtemp(join(tmpdir(), 'git-worktree-operation-lock-')) + const worktreePath = join(root, 'worktree') + const events: string[] = [] + let releaseFirst!: () => void + let markFirstStarted!: () => void + const firstStarted = new Promise((resolve) => { + markFirstStarted = resolve + }) + const firstRelease = new Promise((resolve) => { + releaseFirst = resolve + }) + + const first = runWithGitWorktreeOperationLock(worktreePath, undefined, async () => { + events.push('first:start') + markFirstStarted() + await firstRelease + events.push('first:end') + }) + await firstStarted + + const second = runWithGitWorktreeOperationLock(worktreePath, undefined, async () => { + events.push('second:start') + }) + await Promise.resolve() + expect(events).toEqual(['first:start']) + + releaseFirst() + await Promise.all([first, second]) + expect(events).toEqual(['first:start', 'first:end', 'second:start']) + }) +}) diff --git a/src/shared/git-worktree-operation-lock.ts b/src/shared/git-worktree-operation-lock.ts new file mode 100644 index 00000000000..4e116284168 --- /dev/null +++ b/src/shared/git-worktree-operation-lock.ts @@ -0,0 +1,69 @@ +import { realpath } from 'node:fs/promises' +import { resolve } from 'node:path' + +type OperationLane = { + tail: Promise + release: () => void +} +const lanes = new Map() + +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) + } +} + +/** Serialize mutations that leave per-worktree state in progress (for example, rebase). */ +export async function runWithGitWorktreeOperationLock( + worktreePath: string, + signal: AbortSignal | undefined, + run: () => Promise +): Promise { + const key = await realpath(worktreePath).catch(() => resolve(worktreePath)) + const predecessor = lanes.get(key)?.tail ?? Promise.resolve() + let release!: () => void + const current = new Promise((resolveCurrent) => { + release = resolveCurrent + }) + 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() + } +}