mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
perf(worktrees): fetch the base while the host's create picks a name (#26007)
* perf(worktrees): look up a branch's PR only after a name collision on the host's create The host's create (agent.launch, worktree.create, the phone and the CLI) called gh pr list for every create, a 1-3 s network round trip, while the desktop create probes only after a branch collision. A branch with an open PR exists on the remote, so the branch-conflict check already moves past it. * test(worktrees): pin first-name PR lookup skip on the host's create The runtime service specs still expected a PR lookup for a free first name. Update them to the desktop-parity behavior: no lookup without a branch collision, and a free push-target name is kept. Pin the exact suffixed name, and that a later name with a PR is skipped. * perf(worktrees): overlap the host create's base fetch with naming Keep the first-name PR lookup (restores main's naming) and take it off the critical path instead: start the base-ref refresh before naming and await it after, as the desktop create does, so the lookup and the fetch run concurrently. A refresh rejection while naming fails stays handled. * test(worktrees): pin the overlapped origin fetch on the host's create Cover the best-effort fetch origin path: it runs while the first-name PR lookup is pending, and its rejection while naming fails stays handled.
This commit is contained in:
@@ -26,6 +26,54 @@ import {
|
||||
} from '../worktree-name-retirement'
|
||||
import type { WorktreeCreateTimingRecorder } from '../worktree-create-timing'
|
||||
|
||||
export type RuntimeLocalBaseRefRefresh = {
|
||||
/** Awaits the deferred refresh started before naming, if any. */
|
||||
finish: () => Promise<void>
|
||||
}
|
||||
|
||||
// Why: the deferred refresh can change neither the base nor the name, so start it before naming
|
||||
// and await it before the add, as the desktop create does; naming's PR lookup overlaps it.
|
||||
export function startRuntimeLocalBaseRefRefresh(args: {
|
||||
repoPath: string
|
||||
base: RuntimeLocalWorktreeCreateBase
|
||||
localWorktreeGitOptions: LocalGitExecOptions
|
||||
refreshRemoteTrackingBase: (
|
||||
repoPath: string,
|
||||
base: RemoteTrackingBase,
|
||||
options?: LocalGitExecOptions
|
||||
) => Promise<RemoteFetchResult>
|
||||
fetchRemote: (repoPath: string, remote: string, options?: LocalGitExecOptions) => Promise<void>
|
||||
timing: WorktreeCreateTimingRecorder
|
||||
}): RuntimeLocalBaseRefRefresh {
|
||||
const { remoteTrackingBase, deferredRefresh } = args.base
|
||||
if (remoteTrackingBase && deferredRefresh === 'tracking_ref') {
|
||||
const refresh = args.refreshRemoteTrackingBase(
|
||||
args.repoPath,
|
||||
remoteTrackingBase,
|
||||
args.localWorktreeGitOptions
|
||||
)
|
||||
// Why: naming can throw before finish() awaits the refresh.
|
||||
void refresh.catch(() => undefined)
|
||||
return {
|
||||
finish: async () => {
|
||||
await args.timing.time('refresh_base_ref', () => refresh)
|
||||
}
|
||||
}
|
||||
}
|
||||
if (deferredRefresh === 'origin') {
|
||||
const originFetch = args.fetchRemote(args.repoPath, 'origin', args.localWorktreeGitOptions)
|
||||
void originFetch.catch(() => undefined)
|
||||
return {
|
||||
finish: async () => {
|
||||
try {
|
||||
await args.timing.time('refresh_base_ref', () => originFetch)
|
||||
} catch {}
|
||||
}
|
||||
}
|
||||
}
|
||||
return { finish: async () => {} }
|
||||
}
|
||||
|
||||
export async function createRuntimeLocalGitWorktree(args: {
|
||||
request: RuntimeManagedWorktreeCreateArgs
|
||||
repo: Repo
|
||||
@@ -43,12 +91,7 @@ export async function createRuntimeLocalGitWorktree(args: {
|
||||
effectiveSanitizedName?: string
|
||||
checkoutExistingBranch: boolean
|
||||
localWorktreeGitOptions: LocalGitExecOptions
|
||||
refreshRemoteTrackingBase: (
|
||||
repoPath: string,
|
||||
base: RemoteTrackingBase,
|
||||
options?: LocalGitExecOptions
|
||||
) => Promise<RemoteFetchResult>
|
||||
fetchRemote: (repoPath: string, remote: string, options?: LocalGitExecOptions) => Promise<void>
|
||||
baseRefRefresh: RuntimeLocalBaseRefRefresh
|
||||
rearm: PreparationRearmHolder
|
||||
timing: WorktreeCreateTimingRecorder
|
||||
}): Promise<{
|
||||
@@ -57,22 +100,8 @@ export async function createRuntimeLocalGitWorktree(args: {
|
||||
created: GitWorktreeInfo
|
||||
addResult: AddWorktreeResult
|
||||
}> {
|
||||
const { baseBranch, remoteTrackingBase, deferredRefresh } = args.base
|
||||
if (remoteTrackingBase && deferredRefresh === 'tracking_ref') {
|
||||
await args.timing.time('refresh_base_ref', () =>
|
||||
args.refreshRemoteTrackingBase(
|
||||
args.repo.path,
|
||||
remoteTrackingBase,
|
||||
args.localWorktreeGitOptions
|
||||
)
|
||||
)
|
||||
} else if (deferredRefresh === 'origin') {
|
||||
try {
|
||||
await args.timing.time('refresh_base_ref', () =>
|
||||
args.fetchRemote(args.repo.path, 'origin', args.localWorktreeGitOptions)
|
||||
)
|
||||
} catch {}
|
||||
}
|
||||
const { baseBranch, remoteTrackingBase } = args.base
|
||||
await args.baseRefRefresh.finish()
|
||||
const sparseDirectories = args.request.sparseCheckout
|
||||
? normalizeSparseDirectories(args.request.sparseCheckout.directories)
|
||||
: []
|
||||
|
||||
@@ -140,8 +140,10 @@ function createWorktree(
|
||||
createdWithAgent: undefined,
|
||||
resolveRemoteTrackingBase: mocks.remoteBase,
|
||||
hasRemoteTrackingRef: mocks.hasRemoteRef,
|
||||
refreshRemoteTrackingBase: mocks.refresh,
|
||||
fetchRemote: mocks.fetch,
|
||||
// Why: a vi.fn marks the promise it returns as handled; a fresh promise keeps
|
||||
// an unawaited fetch rejection visible to the unhandled-rejection checks.
|
||||
refreshRemoteTrackingBase: async (...refreshArgs) => await mocks.refresh(...refreshArgs),
|
||||
fetchRemote: async (...fetchArgs) => await mocks.fetch(...fetchArgs),
|
||||
onWorktreeMetadataPersisted: () => undefined,
|
||||
rearm,
|
||||
timing
|
||||
@@ -403,6 +405,135 @@ describe('runtime create Git priority', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('runtime create PR conflict probe', () => {
|
||||
const remoteTrackingBase = {
|
||||
remote: 'origin',
|
||||
branch: 'main',
|
||||
base: 'origin/main',
|
||||
ref: 'refs/remotes/origin/main'
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
mocks.branchName.mockImplementation(async (_path, _override, name: string) => name)
|
||||
})
|
||||
|
||||
it('runs the first name PR lookup while the base fetch is still in flight', async () => {
|
||||
mocks.remoteBase.mockResolvedValue(remoteTrackingBase)
|
||||
let finishFetch!: () => void
|
||||
mocks.refresh.mockReturnValue(
|
||||
new Promise((resolve) => {
|
||||
finishFetch = () => resolve({ ok: true })
|
||||
})
|
||||
)
|
||||
let fetchPendingAtLookup = false
|
||||
mocks.githubPr.mockImplementation(async () => {
|
||||
fetchPendingAtLookup = mocks.refresh.mock.calls.length === 1
|
||||
finishFetch?.()
|
||||
return null
|
||||
})
|
||||
|
||||
await createWorktree()
|
||||
|
||||
expect(fetchPendingAtLookup).toBe(true)
|
||||
expect(mocks.githubPr).toHaveBeenCalledWith('/repo', 'app', {})
|
||||
expect(mocks.consume).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
branch: 'app',
|
||||
options: expect.objectContaining({ remoteTrackingBase })
|
||||
})
|
||||
)
|
||||
})
|
||||
|
||||
it('suffixes past a PR on the first name', async () => {
|
||||
mocks.githubPr.mockResolvedValueOnce({ number: 7, state: 'closed' }).mockResolvedValue(null)
|
||||
|
||||
await createWorktree()
|
||||
|
||||
expect(mocks.githubPr.mock.calls.map(([, branch]) => branch)).toEqual(['app', 'app-2'])
|
||||
expect(mocks.consume).toHaveBeenCalledWith(expect.objectContaining({ branch: 'app-2' }))
|
||||
})
|
||||
|
||||
it.each([undefined, 'Ubuntu'])(
|
||||
'probes the next name for a PR once the first name collides, routed for %s',
|
||||
async (wslDistro) => {
|
||||
const routing = wslDistro ? { wslDistro } : {}
|
||||
mocks.routing.mockReturnValue(routing)
|
||||
mocks.branchConflict.mockResolvedValueOnce('local').mockResolvedValue(null)
|
||||
|
||||
await createWorktree()
|
||||
|
||||
expect(mocks.githubPr).toHaveBeenCalledOnce()
|
||||
expect(mocks.githubPr).toHaveBeenCalledWith('/repo', 'app-2', routing)
|
||||
}
|
||||
)
|
||||
|
||||
it('skips a later name that already has a PR', async () => {
|
||||
mocks.branchConflict.mockResolvedValueOnce('local').mockResolvedValue(null)
|
||||
mocks.githubPr.mockResolvedValueOnce({ number: 7, state: 'closed' }).mockResolvedValue(null)
|
||||
|
||||
await createWorktree()
|
||||
|
||||
expect(mocks.githubPr.mock.calls.map(([, branch]) => branch)).toEqual(['app-2', 'app-3'])
|
||||
expect(mocks.consume).toHaveBeenCalledWith(expect.objectContaining({ branch: 'app-3' }))
|
||||
})
|
||||
|
||||
it('reports the naming failure when the started base fetch also fails', async () => {
|
||||
mocks.remoteBase.mockResolvedValue(remoteTrackingBase)
|
||||
mocks.refresh.mockRejectedValue(new Error('offline'))
|
||||
mocks.branchConflict.mockResolvedValue('local')
|
||||
const unhandled = vi.fn()
|
||||
process.on('unhandledRejection', unhandled)
|
||||
try {
|
||||
await expect(createWorktree()).rejects.toThrow('Branch "app-')
|
||||
await new Promise((resolve) => setImmediate(resolve))
|
||||
expect(mocks.refresh).toHaveBeenCalledOnce()
|
||||
expect(unhandled).not.toHaveBeenCalled()
|
||||
expect(mocks.consume).not.toHaveBeenCalled()
|
||||
} finally {
|
||||
process.off('unhandledRejection', unhandled)
|
||||
}
|
||||
})
|
||||
|
||||
it('runs the first name PR lookup while the origin fetch is still in flight', async () => {
|
||||
mocks.hasBase.mockResolvedValue(false)
|
||||
let finishFetch!: () => void
|
||||
mocks.fetch.mockReturnValue(
|
||||
new Promise<void>((resolve) => {
|
||||
finishFetch = resolve
|
||||
})
|
||||
)
|
||||
let fetchPendingAtLookup = false
|
||||
mocks.githubPr.mockImplementation(async () => {
|
||||
fetchPendingAtLookup = mocks.fetch.mock.calls.length === 1
|
||||
finishFetch?.()
|
||||
return null
|
||||
})
|
||||
|
||||
await createWorktree()
|
||||
|
||||
expect(fetchPendingAtLookup).toBe(true)
|
||||
expect(mocks.fetch).toHaveBeenCalledWith('/repo', 'origin', {})
|
||||
expect(mocks.consume).toHaveBeenCalledWith(expect.objectContaining({ branch: 'app' }))
|
||||
})
|
||||
|
||||
it('reports the naming failure when the started origin fetch also fails', async () => {
|
||||
mocks.hasBase.mockResolvedValue(false)
|
||||
mocks.fetch.mockRejectedValue(new Error('offline'))
|
||||
mocks.branchConflict.mockResolvedValue('local')
|
||||
const unhandled = vi.fn()
|
||||
process.on('unhandledRejection', unhandled)
|
||||
try {
|
||||
await expect(createWorktree()).rejects.toThrow('Branch "app-')
|
||||
await new Promise((resolve) => setImmediate(resolve))
|
||||
expect(mocks.fetch).toHaveBeenCalledOnce()
|
||||
expect(unhandled).not.toHaveBeenCalled()
|
||||
expect(mocks.consume).not.toHaveBeenCalled()
|
||||
} finally {
|
||||
process.off('unhandledRejection', unhandled)
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe('runtime create base without a tracking ref', () => {
|
||||
const originMain = {
|
||||
remote: 'origin',
|
||||
|
||||
@@ -19,7 +19,10 @@ import type { RemoteFetchResult, RemoteTrackingBase } from './runtime-remote-fet
|
||||
import type { HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options'
|
||||
import { hasLocalWorktreeBaseRef } from '../git/worktree-base-ref-probe'
|
||||
import { resolveRuntimeLocalWorktreeCreateCandidate } from './runtime-local-worktree-create-candidate'
|
||||
import { createRuntimeLocalGitWorktree } from './runtime-local-git-worktree-create'
|
||||
import {
|
||||
createRuntimeLocalGitWorktree,
|
||||
startRuntimeLocalBaseRefRefresh
|
||||
} from './runtime-local-git-worktree-create'
|
||||
import { resolveRuntimeLocalWorktreeCreateBase } from './runtime-local-worktree-create-base'
|
||||
import { materializeRuntimeLocalWorktree } from './runtime-local-worktree-materialization'
|
||||
import { resolveRuntimeSetupDecision } from './runtime-local-worktree-setup'
|
||||
@@ -124,6 +127,14 @@ async function performRuntimeLocalWorktreeCreate<T>(args: RuntimeLocalWorktreeCr
|
||||
refreshRemoteTrackingBase: args.refreshRemoteTrackingBase,
|
||||
timing: args.timing
|
||||
})
|
||||
const baseRefRefresh = startRuntimeLocalBaseRefRefresh({
|
||||
repoPath: repo.path,
|
||||
base,
|
||||
localWorktreeGitOptions: worktreeGitOptions,
|
||||
refreshRemoteTrackingBase: args.refreshRemoteTrackingBase,
|
||||
fetchRemote: args.fetchRemote,
|
||||
timing: args.timing
|
||||
})
|
||||
const candidate = await args.timing.time('resolve_name', () =>
|
||||
resolveRuntimeLocalWorktreeCreateCandidate({
|
||||
request,
|
||||
@@ -150,8 +161,7 @@ async function performRuntimeLocalWorktreeCreate<T>(args: RuntimeLocalWorktreeCr
|
||||
effectiveSanitizedName: candidate.effectiveSanitizedName,
|
||||
checkoutExistingBranch: candidate.checkoutExistingBranch,
|
||||
localWorktreeGitOptions: worktreeGitOptions,
|
||||
refreshRemoteTrackingBase: args.refreshRemoteTrackingBase,
|
||||
fetchRemote: args.fetchRemote,
|
||||
baseRefRefresh,
|
||||
rearm: args.rearm,
|
||||
timing: args.timing
|
||||
})
|
||||
|
||||
@@ -0,0 +1,32 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type * as GitHubClient from '../github/client'
|
||||
|
||||
const getPRForBranchMock = vi.hoisted(() => vi.fn())
|
||||
|
||||
vi.mock('../github/client', async (importOriginal) => ({
|
||||
...(await importOriginal<typeof GitHubClient>()),
|
||||
getPRForBranch: getPRForBranchMock
|
||||
}))
|
||||
|
||||
import { getLocalGitHubPrForBranch } from './runtime-worktree-create-git'
|
||||
|
||||
beforeEach(() => {
|
||||
getPRForBranchMock.mockReset()
|
||||
getPRForBranchMock.mockResolvedValue(null)
|
||||
})
|
||||
|
||||
describe('getLocalGitHubPrForBranch', () => {
|
||||
it('routes the PR lookup through the WSL distro', async () => {
|
||||
await getLocalGitHubPrForBranch('/repo', 'app', { wslDistro: 'Ubuntu' })
|
||||
|
||||
expect(getPRForBranchMock).toHaveBeenCalledWith('/repo', 'app', null, null, null, {
|
||||
localGitExecOptions: { wslDistro: 'Ubuntu' }
|
||||
})
|
||||
})
|
||||
|
||||
it('uses the plain lookup without project git options', async () => {
|
||||
await getLocalGitHubPrForBranch('/repo', 'app', {})
|
||||
|
||||
expect(getPRForBranchMock).toHaveBeenCalledWith('/repo', 'app')
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user