mirror of
https://github.com/stablyai/orca.git
synced 2026-09-27 00:02:37 +00:00
Fix rebase race by fetching to private ref before rebasing
`git pull --rebase` is vulnerable to concurrent fetches modifying remote-tracking refs during execution. Fetch to a temporary private ref (refs/orca/rebase/*) first, then rebase from that stable ref to avoid the race condition.
This commit is contained in:
@@ -511,10 +511,13 @@ describe('git remote operations', () => {
|
||||
])
|
||||
})
|
||||
|
||||
it('rebases from the selected remote base ref', async () => {
|
||||
it('fetches to a private ref then rebases from the selected remote base ref', async () => {
|
||||
gitExecFileAsyncMock
|
||||
.mockResolvedValueOnce({ stdout: 'origin\nupstream\n', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: 'fork-point\n', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
|
||||
await gitPullRebaseFromBase('/repo', 'upstream/main')
|
||||
@@ -522,20 +525,86 @@ describe('git remote operations', () => {
|
||||
expect(gitExecFileAsyncMock.mock.calls).toEqual([
|
||||
[['remote'], { cwd: '/repo' }],
|
||||
[['check-ref-format', '--branch', 'main'], { cwd: '/repo' }],
|
||||
[['pull', '--rebase', 'upstream', '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' }
|
||||
],
|
||||
[
|
||||
['rebase', '--onto', expect.stringMatching(/^refs\/orca\/rebase\//), 'fork-point'],
|
||||
{ cwd: '/repo' }
|
||||
],
|
||||
[['update-ref', '-d', expect.stringMatching(/^refs\/orca\/rebase\//)], { cwd: '/repo' }]
|
||||
])
|
||||
|
||||
const fetchRefspec = gitExecFileAsyncMock.mock.calls[3][0][2]
|
||||
const rebasedRef = gitExecFileAsyncMock.mock.calls[4][0][2]
|
||||
const deletedRef = gitExecFileAsyncMock.mock.calls[5][0][2]
|
||||
expect(fetchRefspec).toBe(`+refs/heads/main:${rebasedRef}`)
|
||||
expect(deletedRef).toBe(rebasedRef)
|
||||
})
|
||||
|
||||
it('uses the longest configured remote name when rebasing from a base ref', async () => {
|
||||
gitExecFileAsyncMock
|
||||
.mockResolvedValueOnce({ stdout: 'fork\nfork/team\n', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: 'fork-point\n', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
|
||||
await gitPullRebaseFromBase('/repo', 'fork/team/feature/base')
|
||||
|
||||
expect(gitExecFileAsyncMock).toHaveBeenLastCalledWith(
|
||||
['pull', '--rebase', 'fork/team', 'feature/base'],
|
||||
expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
5,
|
||||
['rebase', '--onto', expect.stringMatching(/^refs\/orca\/rebase\//), 'fork-point'],
|
||||
{ cwd: '/repo' }
|
||||
)
|
||||
})
|
||||
|
||||
it('rebases when the selected remote has not been fetched before', async () => {
|
||||
gitExecFileAsyncMock
|
||||
.mockResolvedValueOnce({ stdout: 'upstream\n', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockRejectedValueOnce(new Error('missing remote-tracking ref'))
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
|
||||
await expect(gitPullRebaseFromBase('/repo', 'upstream/main')).resolves.toBeUndefined()
|
||||
|
||||
expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
4,
|
||||
[
|
||||
'fetch',
|
||||
'upstream',
|
||||
expect.stringMatching(/^\+refs\/heads\/main:refs\/orca\/rebase\//)
|
||||
],
|
||||
{ cwd: '/repo' }
|
||||
)
|
||||
})
|
||||
|
||||
it('removes the private ref when rebase fails', async () => {
|
||||
gitExecFileAsyncMock
|
||||
.mockResolvedValueOnce({ stdout: 'upstream\n', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: 'fork-point\n', stderr: '' })
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
.mockRejectedValueOnce(new Error('fatal: rebase conflict'))
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '' })
|
||||
|
||||
await expect(gitPullRebaseFromBase('/repo', 'upstream/main')).rejects.toThrow(
|
||||
'fatal: rebase conflict'
|
||||
)
|
||||
|
||||
const rebasedRef = gitExecFileAsyncMock.mock.calls[4][0][2]
|
||||
expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
6,
|
||||
['update-ref', '-d', rebasedRef],
|
||||
{ cwd: '/repo' }
|
||||
)
|
||||
})
|
||||
|
||||
+34
-1
@@ -1,3 +1,4 @@
|
||||
import { randomUUID } from 'node:crypto'
|
||||
import {
|
||||
normalizeGitErrorMessage,
|
||||
runPullWithDivergenceFallback
|
||||
@@ -278,17 +279,49 @@ export async function gitPullRebaseFromBase(
|
||||
options: GitRuntimeOptions = {}
|
||||
): Promise<void> {
|
||||
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(
|
||||
['pull', '--rebase', source.remoteName, source.branchName],
|
||||
[
|
||||
'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.
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -148,7 +148,7 @@ describe('submodule path cache', () => {
|
||||
if (args[0] === 'remote') {
|
||||
return Promise.resolve({ stdout: 'origin\n' })
|
||||
}
|
||||
if (args[0] === 'pull') {
|
||||
if (args[0] === 'pull' || args[0] === 'fetch') {
|
||||
modulePath = 'fresh-lib'
|
||||
return Promise.resolve({ stdout: '' })
|
||||
}
|
||||
|
||||
@@ -154,6 +154,75 @@ describe('GitHandler', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('rebases from the original fork point after a remote force-push', async () => {
|
||||
const bareDir = mkdtempSync(path.join(tmpdir(), 'relay-git-rebase-bare-'))
|
||||
const producerParent = mkdtempSync(path.join(tmpdir(), 'relay-git-rebase-producer-'))
|
||||
const producerDir = path.join(producerParent, 'repo')
|
||||
try {
|
||||
execFileSync('git', ['init', '--bare'], { cwd: bareDir, stdio: 'pipe' })
|
||||
gitInit(tmpDir)
|
||||
writeFileSync(path.join(tmpDir, 'base.txt'), 'base')
|
||||
gitCommit(tmpDir, 'base')
|
||||
const branch = execFileSync('git', ['branch', '--show-current'], {
|
||||
cwd: tmpDir,
|
||||
encoding: 'utf-8'
|
||||
}).trim()
|
||||
const forkPoint = execFileSync('git', ['rev-parse', 'HEAD'], {
|
||||
cwd: tmpDir,
|
||||
encoding: 'utf-8'
|
||||
}).trim()
|
||||
execFileSync('git', ['remote', 'add', 'origin', bareDir], { cwd: tmpDir, stdio: 'pipe' })
|
||||
execFileSync('git', ['push', '--set-upstream', 'origin', branch], {
|
||||
cwd: tmpDir,
|
||||
stdio: 'pipe'
|
||||
})
|
||||
|
||||
execFileSync('git', ['clone', bareDir, producerDir], { stdio: 'pipe' })
|
||||
execFileSync('git', ['checkout', branch], { cwd: 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' })
|
||||
writeFileSync(path.join(producerDir, 'discarded.txt'), 'discarded')
|
||||
gitCommit(producerDir, 'discarded remote commit')
|
||||
execFileSync('git', ['push'], { cwd: producerDir, stdio: 'pipe' })
|
||||
execFileSync('git', ['fetch', 'origin'], { cwd: tmpDir, stdio: 'pipe' })
|
||||
|
||||
execFileSync('git', ['checkout', '-b', 'feature', `origin/${branch}`], {
|
||||
cwd: tmpDir,
|
||||
stdio: 'pipe'
|
||||
})
|
||||
writeFileSync(path.join(tmpDir, 'topic.txt'), 'topic')
|
||||
gitCommit(tmpDir, 'topic commit')
|
||||
|
||||
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' })
|
||||
|
||||
await dispatcher.callRequest('git.rebaseFromBase', {
|
||||
worktreePath: tmpDir,
|
||||
baseRef: `origin/${branch}`
|
||||
})
|
||||
|
||||
await expect(fs.access(path.join(tmpDir, 'replacement.txt'))).resolves.toBeUndefined()
|
||||
await expect(fs.access(path.join(tmpDir, 'topic.txt'))).resolves.toBeUndefined()
|
||||
await expect(fs.access(path.join(tmpDir, 'discarded.txt'))).rejects.toThrow()
|
||||
expect(
|
||||
execFileSync('git', ['for-each-ref', '--format=%(refname)', 'refs/orca/rebase'], {
|
||||
cwd: tmpDir,
|
||||
encoding: 'utf-8'
|
||||
}).trim()
|
||||
).toBe('')
|
||||
} finally {
|
||||
await Promise.all([
|
||||
fs.rm(bareDir, { recursive: true, force: true }),
|
||||
fs.rm(producerParent, { recursive: true, force: true })
|
||||
])
|
||||
}
|
||||
}, 15_000)
|
||||
|
||||
it('fetches the explicit publish target remote', async () => {
|
||||
const bareDir = mkdtempSync(path.join(tmpdir(), 'relay-git-fork-bare-'))
|
||||
try {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
/* eslint-disable max-lines -- Why: centralizes the git RPC protocol surface so local and SSH git behavior stay in one dispatch table. */
|
||||
import { randomUUID } from 'node:crypto'
|
||||
import { execFile, spawn, type ExecFileOptions } from 'node:child_process'
|
||||
import { promisify } from 'node:util'
|
||||
import * as path from 'node:path'
|
||||
@@ -1132,17 +1133,48 @@ export class GitHandler {
|
||||
this.clearGitMutationReadCaches()
|
||||
const worktreePath = params.worktreePath as string
|
||||
const baseRef = params.baseRef as string
|
||||
let rebaseRef: string | null = null
|
||||
try {
|
||||
try {
|
||||
const source = await resolveGitRemoteRebaseSource(
|
||||
((args) => this.git(args, worktreePath)) as GitCommandRunner,
|
||||
baseRef
|
||||
)
|
||||
await this.git(['pull', '--rebase', source.remoteName, source.branchName], worktreePath)
|
||||
let forkPoint: string | null = null
|
||||
try {
|
||||
const { stdout } = await this.git(
|
||||
['merge-base', '--fork-point', `refs/remotes/${source.displayName}`, 'HEAD'],
|
||||
worktreePath
|
||||
)
|
||||
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 this.git(
|
||||
[
|
||||
'fetch',
|
||||
source.remoteName,
|
||||
`+refs/heads/${source.branchName}:${rebaseRef}`
|
||||
],
|
||||
worktreePath
|
||||
)
|
||||
await this.git(
|
||||
forkPoint ? ['rebase', '--onto', rebaseRef, forkPoint] : ['rebase', rebaseRef],
|
||||
worktreePath
|
||||
)
|
||||
} catch (error) {
|
||||
throw new Error(normalizeGitErrorMessage(error, 'pull'))
|
||||
}
|
||||
} finally {
|
||||
if (rebaseRef) {
|
||||
try {
|
||||
await this.git(['update-ref', '-d', rebaseRef], worktreePath)
|
||||
} catch {
|
||||
// Cleanup must not hide the fetch or rebase result.
|
||||
}
|
||||
}
|
||||
this.clearGitMutationReadCaches()
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user