mirror of
https://github.com/stablyai/orca.git
synced 2026-10-07 16:02:29 +00:00
perf: avoid blocking base-ref resolution (#8182)
* perf: avoid blocking base-ref resolution * fix(git): bound async default-base probes * fix(git): bound default-base probes during creation * test(git): cover bounded Windows and WSL probes
This commit is contained in:
@@ -0,0 +1,46 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const { gitExecFileAsyncMock } = vi.hoisted(() => ({
|
||||
gitExecFileAsyncMock: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('./runner', () => ({
|
||||
gitExecFileAsync: gitExecFileAsyncMock,
|
||||
gitExecFileSync: vi.fn()
|
||||
}))
|
||||
|
||||
import { getBaseRefDefault } from './repo'
|
||||
|
||||
describe('getBaseRefDefault async subprocess bounds', () => {
|
||||
beforeEach(() => {
|
||||
gitExecFileAsyncMock.mockReset()
|
||||
})
|
||||
|
||||
it('bounds every local probe and degrades a timeout to no default', async () => {
|
||||
gitExecFileAsyncMock.mockRejectedValue(new Error('git timed out.'))
|
||||
|
||||
await expect(getBaseRefDefault('/repo')).resolves.toBeNull()
|
||||
|
||||
expect(gitExecFileAsyncMock).toHaveBeenCalled()
|
||||
for (const [, options] of gitExecFileAsyncMock.mock.calls) {
|
||||
expect(options).toEqual({ cwd: '/repo', timeout: 15_000 })
|
||||
}
|
||||
})
|
||||
|
||||
it('preserves the same timeout when routing probes through WSL', async () => {
|
||||
gitExecFileAsyncMock.mockRejectedValue(new Error('git timed out.'))
|
||||
|
||||
await expect(
|
||||
getBaseRefDefault('\\\\wsl.localhost\\Ubuntu\\repo', { wslDistro: 'Ubuntu' })
|
||||
).resolves.toBeNull()
|
||||
|
||||
expect(gitExecFileAsyncMock).toHaveBeenCalled()
|
||||
for (const [, options] of gitExecFileAsyncMock.mock.calls) {
|
||||
expect(options).toEqual({
|
||||
cwd: '\\\\wsl.localhost\\Ubuntu\\repo',
|
||||
timeout: 15_000,
|
||||
wslDistro: 'Ubuntu'
|
||||
})
|
||||
}
|
||||
})
|
||||
})
|
||||
+20
-3
@@ -15,6 +15,13 @@ type LocalGitExecOptions = {
|
||||
wslDistro?: string
|
||||
}
|
||||
|
||||
type LocalDefaultBaseRefGitOptions = {
|
||||
cwd: string
|
||||
wslDistro?: string
|
||||
}
|
||||
|
||||
const DEFAULT_BASE_REF_PROBE_TIMEOUT_MS = 15_000
|
||||
|
||||
type GitRepoProbeResult = 'repo' | 'not-repo' | 'indeterminate'
|
||||
type GitMarkerScanResult = { status: 'valid'; rootPath: string } | { status: 'absent' | 'invalid' }
|
||||
|
||||
@@ -651,13 +658,23 @@ export async function resolveDefaultBaseRefViaExec(exec: GitExec): Promise<strin
|
||||
return resolveDefaultBaseRefFromProbes((ref) => hasGitRefViaExec(exec, ref))
|
||||
}
|
||||
|
||||
export function resolveDefaultBaseRefWithLocalGit(
|
||||
options: LocalDefaultBaseRefGitOptions
|
||||
): Promise<string | null> {
|
||||
return resolveDefaultBaseRefViaExec((argv) =>
|
||||
gitExecFileAsync(argv, {
|
||||
...options,
|
||||
// Why: async avoids main-thread stalls, but dead local/WSL filesystems still need a bound.
|
||||
timeout: DEFAULT_BASE_REF_PROBE_TIMEOUT_MS
|
||||
})
|
||||
)
|
||||
}
|
||||
|
||||
async function getDefaultBaseRefAsync(
|
||||
path: string,
|
||||
options: LocalGitExecOptions = {}
|
||||
): Promise<string | null> {
|
||||
return resolveDefaultBaseRefViaExec((argv) =>
|
||||
gitExecFileAsync(argv, gitExecOptions(path, options))
|
||||
)
|
||||
return resolveDefaultBaseRefWithLocalGit(gitExecOptions(path, options))
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -27,7 +27,11 @@ import type {
|
||||
import { getPRForBranch } from '../github/client'
|
||||
import { listWorktrees, addWorktree, addSparseWorktree } from '../git/worktree'
|
||||
import type { AddWorktreeOptions, AddWorktreeResult } from '../git/worktree'
|
||||
import { getBranchConflictKind, resolveDefaultBaseRefViaExec } from '../git/repo'
|
||||
import {
|
||||
getBranchConflictKind,
|
||||
resolveDefaultBaseRefViaExec,
|
||||
resolveDefaultBaseRefWithLocalGit
|
||||
} from '../git/repo'
|
||||
import { resolveLocalGitUsername } from '../git/git-username'
|
||||
import { hasCommitObjectViaGitExec } from '../git/commit-object-ref'
|
||||
import { resolveWorktreeCreateBase } from '../worktree-create-base'
|
||||
@@ -1999,8 +2003,7 @@ export async function createLocalWorktree(
|
||||
const baseBranch = await resolveWorktreeCreateBase({
|
||||
requestedBaseBranch: args.baseBranch,
|
||||
repoWorktreeBaseRef: repo.worktreeBaseRef,
|
||||
resolveDefaultBaseRef: () =>
|
||||
resolveDefaultBaseRefViaExec((argv) => gitExecFileAsync(argv, localGitExecOptions)),
|
||||
resolveDefaultBaseRef: () => resolveDefaultBaseRefWithLocalGit(localGitExecOptions),
|
||||
isBaseUsable: async (baseBranchCandidate) => {
|
||||
if (runtime) {
|
||||
const remoteTrackingBase = await runtime.resolveRemoteTrackingBase(
|
||||
|
||||
@@ -10,6 +10,7 @@ const {
|
||||
removeWorktreeMock,
|
||||
resolveLocalGitUsernameMock,
|
||||
getDefaultBaseRefMock,
|
||||
resolveDefaultBaseRefWithLocalGitMock,
|
||||
resolveDefaultBaseRefViaExecMock,
|
||||
getBranchConflictKindMock,
|
||||
getPRForBranchMock,
|
||||
@@ -38,6 +39,7 @@ const {
|
||||
removeWorktreeMock: vi.fn(),
|
||||
resolveLocalGitUsernameMock: vi.fn(),
|
||||
getDefaultBaseRefMock: vi.fn(),
|
||||
resolveDefaultBaseRefWithLocalGitMock: vi.fn(),
|
||||
resolveDefaultBaseRefViaExecMock: vi.fn(),
|
||||
getBranchConflictKindMock: vi.fn(),
|
||||
getPRForBranchMock: vi.fn(),
|
||||
@@ -84,6 +86,7 @@ vi.mock('../git/runner', () => ({
|
||||
|
||||
vi.mock('../git/repo', () => ({
|
||||
getDefaultBaseRef: getDefaultBaseRefMock,
|
||||
resolveDefaultBaseRefWithLocalGit: resolveDefaultBaseRefWithLocalGitMock,
|
||||
resolveDefaultBaseRefViaExec: resolveDefaultBaseRefViaExecMock,
|
||||
getBranchConflictKind: getBranchConflictKindMock
|
||||
}))
|
||||
@@ -164,6 +167,7 @@ describe('registerWorktreeHandlers – Windows path handling', () => {
|
||||
removeWorktreeMock.mockReset()
|
||||
resolveLocalGitUsernameMock.mockReset()
|
||||
getDefaultBaseRefMock.mockReset()
|
||||
resolveDefaultBaseRefWithLocalGitMock.mockReset()
|
||||
resolveDefaultBaseRefViaExecMock.mockReset()
|
||||
getBranchConflictKindMock.mockReset()
|
||||
getPRForBranchMock.mockReset()
|
||||
@@ -230,6 +234,7 @@ describe('registerWorktreeHandlers – Windows path handling', () => {
|
||||
store.setWorktreeMeta.mockReturnValue({})
|
||||
resolveLocalGitUsernameMock.mockResolvedValue('')
|
||||
getDefaultBaseRefMock.mockReturnValue('origin/main')
|
||||
resolveDefaultBaseRefWithLocalGitMock.mockResolvedValue('origin/main')
|
||||
resolveDefaultBaseRefViaExecMock.mockResolvedValue('origin/main')
|
||||
getBranchConflictKindMock.mockResolvedValue(null)
|
||||
getPRForBranchMock.mockResolvedValue(null)
|
||||
|
||||
@@ -28,7 +28,8 @@ const {
|
||||
removeWorktreeMock,
|
||||
forceDeleteLocalBranchMock,
|
||||
resolveLocalGitUsernameMock,
|
||||
getDefaultBaseRefMock,
|
||||
getBaseRefDefaultMock,
|
||||
resolveDefaultBaseRefWithLocalGitMock,
|
||||
resolveDefaultBaseRefViaExecMock,
|
||||
getDefaultRemoteMock,
|
||||
getBranchConflictKindMock,
|
||||
@@ -77,7 +78,8 @@ const {
|
||||
removeWorktreeMock: vi.fn(),
|
||||
forceDeleteLocalBranchMock: vi.fn(),
|
||||
resolveLocalGitUsernameMock: vi.fn(),
|
||||
getDefaultBaseRefMock: vi.fn(),
|
||||
getBaseRefDefaultMock: vi.fn(),
|
||||
resolveDefaultBaseRefWithLocalGitMock: vi.fn(),
|
||||
resolveDefaultBaseRefViaExecMock: vi.fn(),
|
||||
getDefaultRemoteMock: vi.fn(),
|
||||
getBranchConflictKindMock: vi.fn(),
|
||||
@@ -130,7 +132,8 @@ vi.mock('../git/runner', () => ({
|
||||
}))
|
||||
|
||||
vi.mock('../git/repo', () => ({
|
||||
getDefaultBaseRef: getDefaultBaseRefMock,
|
||||
getBaseRefDefault: getBaseRefDefaultMock,
|
||||
resolveDefaultBaseRefWithLocalGit: resolveDefaultBaseRefWithLocalGitMock,
|
||||
resolveDefaultBaseRefViaExec: resolveDefaultBaseRefViaExecMock,
|
||||
getDefaultRemote: getDefaultRemoteMock,
|
||||
getBranchConflictKind: getBranchConflictKindMock
|
||||
@@ -300,7 +303,8 @@ describe('registerWorktreeHandlers', () => {
|
||||
removeWorktreeMock,
|
||||
forceDeleteLocalBranchMock,
|
||||
resolveLocalGitUsernameMock,
|
||||
getDefaultBaseRefMock,
|
||||
getBaseRefDefaultMock,
|
||||
resolveDefaultBaseRefWithLocalGitMock,
|
||||
resolveDefaultBaseRefViaExecMock,
|
||||
getDefaultRemoteMock,
|
||||
getBranchConflictKindMock,
|
||||
@@ -401,7 +405,8 @@ describe('registerWorktreeHandlers', () => {
|
||||
])
|
||||
store.getAllWorktreeLineage.mockReturnValue({})
|
||||
resolveLocalGitUsernameMock.mockResolvedValue('')
|
||||
getDefaultBaseRefMock.mockReturnValue('origin/main')
|
||||
getBaseRefDefaultMock.mockResolvedValue('origin/main')
|
||||
resolveDefaultBaseRefWithLocalGitMock.mockResolvedValue('origin/main')
|
||||
resolveDefaultBaseRefViaExecMock.mockResolvedValue('origin/main')
|
||||
getDefaultRemoteMock.mockResolvedValue('origin')
|
||||
getBranchConflictKindMock.mockResolvedValue(null)
|
||||
@@ -525,6 +530,7 @@ describe('registerWorktreeHandlers', () => {
|
||||
|
||||
await handlers['worktrees:prefetchCreateBase'](null, { repoId: 'repo-1' })
|
||||
|
||||
expect(getBaseRefDefaultMock).toHaveBeenCalledWith('/workspace/repo')
|
||||
expect(runtimeStub.resolveRemoteTrackingBase).toHaveBeenCalledWith(
|
||||
'/workspace/repo',
|
||||
'origin/master'
|
||||
@@ -2050,12 +2056,6 @@ describe('registerWorktreeHandlers', () => {
|
||||
|
||||
it('routes local worktree creation through the selected WSL project runtime', async () => {
|
||||
mockSelectedWslProjectRuntime()
|
||||
resolveDefaultBaseRefViaExecMock.mockImplementation(
|
||||
async (exec: (args: string[]) => Promise<{ stdout: string }>) => {
|
||||
await exec(['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'])
|
||||
return 'origin/main'
|
||||
}
|
||||
)
|
||||
listWorktreesMock.mockResolvedValue([
|
||||
{
|
||||
path: '/workspace/repo',
|
||||
@@ -2087,10 +2087,10 @@ describe('registerWorktreeHandlers', () => {
|
||||
false,
|
||||
{ wslDistro: 'Ubuntu' }
|
||||
)
|
||||
expect(gitExecFileAsyncMock).toHaveBeenCalledWith(
|
||||
['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'],
|
||||
{ cwd: '/workspace/repo', wslDistro: 'Ubuntu' }
|
||||
)
|
||||
expect(resolveDefaultBaseRefWithLocalGitMock).toHaveBeenCalledWith({
|
||||
cwd: '/workspace/repo',
|
||||
wslDistro: 'Ubuntu'
|
||||
})
|
||||
expect(getBranchConflictKindMock).toHaveBeenCalledWith(
|
||||
'/workspace/repo',
|
||||
'improve-dashboard',
|
||||
@@ -4980,7 +4980,7 @@ describe('registerWorktreeHandlers', () => {
|
||||
// no origin/main, no origin/master, and no local main/master), we must
|
||||
// fail loudly with a message that prompts the user to pick a base
|
||||
// branch, not hand a non-existent ref to `git worktree add`.
|
||||
resolveDefaultBaseRefViaExecMock.mockResolvedValue(null)
|
||||
resolveDefaultBaseRefWithLocalGitMock.mockResolvedValue(null)
|
||||
store.getRepo.mockReturnValue({
|
||||
id: 'repo-1',
|
||||
path: '/workspace/repo',
|
||||
|
||||
@@ -45,7 +45,7 @@ import {
|
||||
runHook,
|
||||
shouldRunSetupForCreate
|
||||
} from '../hooks'
|
||||
import { getBranchConflictKind, getDefaultBaseRef } from '../git/repo'
|
||||
import { getBaseRefDefault, getBranchConflictKind } from '../git/repo'
|
||||
import type { OrchestrationDb } from './orchestration/db'
|
||||
import type { MessagePriority, MessageRow, MessageType } from './orchestration/types'
|
||||
import {
|
||||
@@ -521,16 +521,23 @@ vi.mock('../github/issues', async (importOriginal) => {
|
||||
}
|
||||
})
|
||||
|
||||
// Why: the CLI create-worktree path calls getDefaultBaseRef to resolve a
|
||||
// fallback base branch. Real resolution shells out to `git` against the
|
||||
// test's fabricated repo path, which has no refs, so we stub it to a
|
||||
// predictable 'origin/main'. The runtime no longer silently fabricates this
|
||||
// default, so tests that want the legacy behavior must express it via the mock.
|
||||
// Why: CLI worktree creation resolves a default against fabricated repo paths
|
||||
// in these tests, so keep the async resolver deterministic.
|
||||
vi.mock('../git/repo', async (importOriginal) => {
|
||||
const actual = (await importOriginal()) as Record<string, unknown>
|
||||
const actualGetBaseRefDefault = actual.getBaseRefDefault as (
|
||||
path: string,
|
||||
options?: { wslDistro?: string }
|
||||
) => Promise<string | null>
|
||||
return {
|
||||
...actual,
|
||||
getDefaultBaseRef: vi.fn().mockReturnValue('origin/main'),
|
||||
// Why: fabricated local test repos need a deterministic default, while
|
||||
// WSL coverage must still exercise the real async Git-options path.
|
||||
getBaseRefDefault: vi
|
||||
.fn()
|
||||
.mockImplementation((path: string, options?: { wslDistro?: string }) =>
|
||||
options?.wslDistro ? actualGetBaseRefDefault(path, options) : Promise.resolve('origin/main')
|
||||
),
|
||||
getBranchConflictKind: vi.fn().mockResolvedValue(null)
|
||||
}
|
||||
})
|
||||
@@ -2857,7 +2864,7 @@ describe('OrcaRuntimeService', () => {
|
||||
suggestLocalBaseRefUpdate: true
|
||||
}
|
||||
)
|
||||
expect(getDefaultBaseRef).toHaveBeenCalled()
|
||||
expect(getBaseRefDefault).toHaveBeenCalled()
|
||||
} finally {
|
||||
getReposSpy.mockRestore()
|
||||
gitSpy.mockRestore()
|
||||
@@ -5363,7 +5370,7 @@ describe('OrcaRuntimeService', () => {
|
||||
|
||||
it('treats SSH worktree drift as unknown without local git probes', async () => {
|
||||
vi.mocked(listWorktrees).mockClear()
|
||||
vi.mocked(getDefaultBaseRef).mockClear()
|
||||
vi.mocked(getBaseRefDefault).mockClear()
|
||||
const remoteStore = {
|
||||
...store,
|
||||
getRepos: () => [
|
||||
@@ -5399,7 +5406,7 @@ describe('OrcaRuntimeService', () => {
|
||||
}
|
||||
|
||||
expect(gitProvider.listWorktrees).toHaveBeenCalledWith('/remote/repo')
|
||||
expect(getDefaultBaseRef).not.toHaveBeenCalled()
|
||||
expect(getBaseRefDefault).not.toHaveBeenCalled()
|
||||
expect(listWorktrees).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
@@ -5465,7 +5472,7 @@ describe('OrcaRuntimeService', () => {
|
||||
})
|
||||
expect(asyncGitSpy).toHaveBeenCalledWith(
|
||||
['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'],
|
||||
wslGitOptions
|
||||
{ ...wslGitOptions, timeout: 15_000 }
|
||||
)
|
||||
expect(asyncGitSpy).toHaveBeenCalledWith(['remote'], wslGitOptions)
|
||||
expect(asyncGitSpy).toHaveBeenCalledWith(
|
||||
@@ -26473,6 +26480,7 @@ describe('OrcaRuntimeService', () => {
|
||||
})
|
||||
expect(gitSpy).toHaveBeenCalledWith(['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'], {
|
||||
cwd: TEST_REPO_PATH,
|
||||
timeout: 15_000,
|
||||
wslDistro: 'Ubuntu'
|
||||
})
|
||||
expect(gitSpy).toHaveBeenCalledWith(
|
||||
@@ -27140,6 +27148,7 @@ describe('OrcaRuntimeService', () => {
|
||||
})
|
||||
expect(gitSpy).toHaveBeenCalledWith(['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'], {
|
||||
cwd: TEST_REPO_PATH,
|
||||
timeout: 15_000,
|
||||
wslDistro: 'Ubuntu'
|
||||
})
|
||||
expect(getBranchConflictKind).toHaveBeenCalledWith(
|
||||
|
||||
@@ -600,7 +600,6 @@ import type {
|
||||
} from '../../shared/github-project-types'
|
||||
import {
|
||||
getBaseRefDefault,
|
||||
getDefaultBaseRef,
|
||||
getDefaultRemote,
|
||||
getBranchConflictKind,
|
||||
isGitRepo,
|
||||
@@ -611,6 +610,7 @@ import {
|
||||
parseAndFilterSearchRefDetails,
|
||||
parseRemoteCount,
|
||||
resolveDefaultBaseRefViaExec,
|
||||
resolveDefaultBaseRefWithLocalGit,
|
||||
buildSearchBaseRefsArgv,
|
||||
isForEachRefExcludeUnsupportedError,
|
||||
mergeBaseRefSearchResultGroups,
|
||||
@@ -14264,8 +14264,8 @@ export class OrcaRuntimeService {
|
||||
repoWorktreeBaseRef: repo.worktreeBaseRef,
|
||||
resolveDefaultBaseRef: () =>
|
||||
hasLocalWorktreeGitOptions
|
||||
? resolveDefaultBaseRefViaExec((argv) => gitExecFileAsync(argv, localGitExecOptions))
|
||||
: Promise.resolve(getDefaultBaseRef(repo.path)),
|
||||
? resolveDefaultBaseRefWithLocalGit(localGitExecOptions)
|
||||
: getBaseRefDefault(repo.path),
|
||||
isBaseUsable: async (baseBranchCandidate) => {
|
||||
const remoteTrackingBase = await this.resolveRemoteTrackingBase(
|
||||
repo.path,
|
||||
@@ -14296,10 +14296,8 @@ export class OrcaRuntimeService {
|
||||
}
|
||||
})
|
||||
if (!baseBranch) {
|
||||
// Why: getDefaultBaseRef returns null when no suitable ref exists.
|
||||
// Don't fabricate 'origin/main' — passing it to addWorktree would
|
||||
// produce an opaque git failure. Surface a clear error so the CLI
|
||||
// caller can pick an explicit --base ref.
|
||||
// Why: a null default means no suitable ref exists; fail clearly instead
|
||||
// of handing Git a fabricated origin/main ref.
|
||||
throw new Error(
|
||||
'Could not resolve a default base ref for this repo. Pass an explicit --base and try again.'
|
||||
)
|
||||
|
||||
@@ -2,7 +2,7 @@ import { isFolderRepo } from '../shared/repo-kind'
|
||||
import type { Repo } from '../shared/types'
|
||||
import { hasLocalCommitObject, isFullGitObjectId } from './git/commit-object-ref'
|
||||
import { hasWorktreeBaseCommitRef } from './git/worktree-base-ref-probe'
|
||||
import { getDefaultBaseRef } from './git/repo'
|
||||
import { getBaseRefDefault } from './git/repo'
|
||||
import { getSshGitProvider } from './providers/ssh-git-dispatch'
|
||||
import { prefetchRemoteWorktreeCreateBase } from './ipc/worktree-remote'
|
||||
import { resolveWorktreeCreateBase } from './worktree-create-base'
|
||||
@@ -48,7 +48,7 @@ async function prefetchLocalWorktreeCreateBase(
|
||||
const resolvedBaseBranch = await resolveWorktreeCreateBase({
|
||||
requestedBaseBranch: baseBranch,
|
||||
repoWorktreeBaseRef: repo.worktreeBaseRef,
|
||||
resolveDefaultBaseRef: async () => getDefaultBaseRef(repo.path),
|
||||
resolveDefaultBaseRef: () => getBaseRefDefault(repo.path),
|
||||
isBaseUsable: async (baseBranchCandidate) => {
|
||||
const remoteTrackingBase = await runtime.resolveRemoteTrackingBase(
|
||||
repo.path,
|
||||
|
||||
Reference in New Issue
Block a user