diff --git a/src/main/git/exact-ref-probe.ts b/src/main/git/exact-ref-probe.ts index 6b13cc718c5..96bb421b8e8 100644 --- a/src/main/git/exact-ref-probe.ts +++ b/src/main/git/exact-ref-probe.ts @@ -19,6 +19,8 @@ export type ExactRefProbeSetResult = { type ExactRefPresence = 'present' | 'absent' | 'unknown' const EXACT_REF_PROBE_CONCURRENCY = 8 +// SHA-1 and SHA-256 repositories both report a full object id here. +const OBJECT_ID_PATTERN = /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/ export function isShowRefNoMatchError(error: unknown): boolean { const record = error && typeof error === 'object' ? (error as Record) : undefined @@ -126,3 +128,50 @@ export async function probeAnyExactRef( await Promise.all(Array.from({ length: workerCount }, () => probeNext())) return { found, unknown } } + +/** Runs Git with a stdin payload. Only hosts that can feed a child's stdin supply one. */ +export type ExactRefProbeStdinExec = ( + argv: string[], + options: ExactRefProbeExecOptions & { stdin: string } +) => Promise<{ stdout: string }> + +/** `cat-file --batch-check` reports every ref from one child, and reports a missing ref as data + * rather than a failed exit — so a batch stays as decidable as a per-ref `show-ref --verify`. + * A repo with many remotes otherwise pays one subprocess per remote on every conflict check. */ +export async function probeAnyExactRefBatched( + runGit: ExactRefProbeStdinExec, + refs: readonly string[], + options: ExactRefProbeExecOptions = {} +): Promise<{ found: boolean; unknown: boolean }> { + const uniqueRefs = [...new Set(refs)] + const safeRefs = uniqueRefs.filter((ref) => isSafeGitRefName(ref)) + if (safeRefs.length === 0) { + return { found: false, unknown: uniqueRefs.length > 0 } + } + let stdout: string + try { + ;({ stdout } = await runGit(['cat-file', '--batch-check'], { + ...options, + stdin: `${safeRefs.join('\n')}\n` + })) + } catch { + return { found: false, unknown: true } + } + const lines = stdout.split('\n').filter((line) => line.trim().length > 0) + // One line per input, in order; a short read means the batch never answered for the rest. + if (lines.length !== safeRefs.length) { + return { found: false, unknown: true } + } + let unknown = safeRefs.length !== uniqueRefs.length + for (const line of lines) { + const [head, type] = line.split(' ') + if (OBJECT_ID_PATTERN.test(head) && type !== undefined && type !== 'missing') { + return { found: true, unknown: false } + } + if (type !== 'missing') { + // `ambiguous`, or a spelling this Git reports differently; neither proves absence. + unknown = true + } + } + return { found: false, unknown } +} diff --git a/src/main/git/repo-branch-conflict-real-git.test.ts b/src/main/git/repo-branch-conflict-real-git.test.ts new file mode 100644 index 00000000000..34092273eda --- /dev/null +++ b/src/main/git/repo-branch-conflict-real-git.test.ts @@ -0,0 +1,49 @@ +import { execFileSync } from 'node:child_process' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { getBranchConflictKind } from './repo-branch-conflict' + +describe('branch conflict real Git contract', () => { + const tempPaths: string[] = [] + + afterEach(() => { + for (const path of tempPaths.splice(0)) { + rmSync(path, { recursive: true, force: true }) + } + }) + + it('decides remote conflicts from one batched probe across many remotes', async () => { + const repoPath = mkdtempSync(join(tmpdir(), 'orca-branch-conflict-')) + tempPaths.push(repoPath) + const git = (...args: string[]): string => + execFileSync('git', args, { cwd: repoPath, encoding: 'utf8' }) + + git('init', '--quiet') + git('config', 'user.name', 'Orca Test') + git('config', 'user.email', 'orca@example.test') + git('config', 'commit.gpgSign', 'false') + git('config', 'core.hooksPath', '.git/no-hooks') + writeFileSync(join(repoPath, 'fixture.txt'), 'base\n') + git('add', 'fixture.txt') + git('commit', '--quiet', '-m', 'base') + const head = git('rev-parse', 'HEAD').trim() + + // Many remotes is the shape that used to cost one subprocess each. + for (let index = 0; index < 12; index += 1) { + git('remote', 'add', `remote${index}`, 'https://example.test/repo.git') + } + git('update-ref', 'refs/remotes/remote7/taken', head) + + await expect(getBranchConflictKind(repoPath, 'taken')).resolves.toBe('remote') + await expect(getBranchConflictKind(repoPath, 'free')).resolves.toBeNull() + // The allowed base ref is the one remote spelling that is not a conflict. + await expect( + getBranchConflictKind(repoPath, 'taken', 'refs/remotes/remote7/taken') + ).resolves.toBeNull() + + git('branch', 'local-only', head) + await expect(getBranchConflictKind(repoPath, 'local-only')).resolves.toBe('local') + }) +}) diff --git a/src/main/git/repo-branch-conflict.test.ts b/src/main/git/repo-branch-conflict.test.ts index dab8dd396d6..873c8bd3340 100644 --- a/src/main/git/repo-branch-conflict.test.ts +++ b/src/main/git/repo-branch-conflict.test.ts @@ -134,3 +134,123 @@ describe('getBranchConflictKindViaExec', () => { expect(exec).not.toHaveBeenCalled() }) }) + +describe('getBranchConflictKindViaExec batched remote probe', () => { + function remoteNames(count: number): string { + return `${Array.from({ length: count }, (_, index) => `remote${index}`).join('\n')}\n` + } + + function baseExec(calls: string[][]): (argv: string[]) => Promise<{ stdout: string }> { + return async (argv) => { + calls.push(argv) + if (argv[0] === 'rev-parse') { + throw new Error('local branch is absent') + } + if (argv[0] === 'remote') { + return { stdout: remoteNames(3) } + } + throw new Error(`unexpected git command: ${argv.join(' ')}`) + } + } + + it('asks one batched child instead of one probe per remote', async () => { + const calls: string[][] = [] + const stdinPayloads: (string | undefined)[] = [] + const exec = baseExec(calls) + const batched = async ( + argv: string[], + options: { stdin: string } + ): Promise<{ stdout: string }> => { + calls.push(argv) + stdinPayloads.push(options.stdin) + return { + stdout: [ + 'refs/remotes/remote0/feature missing', + 'refs/remotes/remote1/feature missing', + 'refs/remotes/remote2/feature missing' + ].join('\n') + } + } + + await expect( + getBranchConflictKindViaExec(exec, 'feature', undefined, {}, batched) + ).resolves.toBeNull() + expect(calls).toEqual([ + ['rev-parse', '--verify', 'refs/heads/feature'], + ['remote'], + ['cat-file', '--batch-check'] + ]) + expect(stdinPayloads).toEqual([ + 'refs/remotes/remote0/feature\nrefs/remotes/remote1/feature\nrefs/remotes/remote2/feature\n' + ]) + }) + + it('reports a remote conflict from the batched answer', async () => { + const calls: string[][] = [] + const exec = baseExec(calls) + const batched = async (): Promise<{ stdout: string }> => ({ + stdout: [ + 'refs/remotes/remote0/feature missing', + `${'a'.repeat(40)} commit 214`, + 'refs/remotes/remote2/feature missing' + ].join('\n') + }) + + await expect( + getBranchConflictKindViaExec(exec, 'feature', undefined, {}, batched) + ).resolves.toBe('remote') + }) + + it('falls back to per-ref probes when the batch cannot answer', async () => { + const calls: string[][] = [] + const exec = async (argv: string[]): Promise<{ stdout: string }> => { + calls.push(argv) + if (argv[0] === 'rev-parse') { + throw new Error('local branch is absent') + } + if (argv[0] === 'remote') { + return { stdout: remoteNames(3) } + } + if (argv[0] === 'show-ref') { + if (argv[4] === 'refs/remotes/remote1/feature') { + return { stdout: 'abc refs/remotes/remote1/feature\n' } + } + throw Object.assign(new Error('missing'), { code: 1, stderr: '' }) + } + throw new Error(`unexpected git command: ${argv.join(' ')}`) + } + const batched = async (): Promise<{ stdout: string }> => { + throw new Error('cat-file is unavailable') + } + + await expect( + getBranchConflictKindViaExec(exec, 'feature', undefined, {}, batched) + ).resolves.toBe('remote') + expect(calls.filter((argv) => argv[0] === 'show-ref')).toHaveLength(3) + }) + + it('treats a short batch read as undecided rather than as absence', async () => { + const calls: string[][] = [] + const exec = async (argv: string[]): Promise<{ stdout: string }> => { + calls.push(argv) + if (argv[0] === 'rev-parse') { + throw new Error('local branch is absent') + } + if (argv[0] === 'remote') { + return { stdout: remoteNames(3) } + } + if (argv[0] === 'show-ref') { + throw Object.assign(new Error('missing'), { code: 1, stderr: '' }) + } + throw new Error(`unexpected git command: ${argv.join(' ')}`) + } + const batched = async (): Promise<{ stdout: string }> => ({ + stdout: 'refs/remotes/remote0/feature missing' + }) + + await expect( + getBranchConflictKindViaExec(exec, 'feature', undefined, {}, batched) + ).resolves.toBeNull() + expect(calls.filter((argv) => argv[0] === 'show-ref')).toHaveLength(3) + }) +}) diff --git a/src/main/git/repo-branch-conflict.ts b/src/main/git/repo-branch-conflict.ts index 162d5ef53b3..c799d3770be 100644 --- a/src/main/git/repo-branch-conflict.ts +++ b/src/main/git/repo-branch-conflict.ts @@ -4,8 +4,10 @@ import { gitExecFileAsync } from './runner' import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' import { probeAnyExactRef, + probeAnyExactRefBatched, type ExactRefProbeExec, - type ExactRefProbeExecOptions + type ExactRefProbeExecOptions, + type ExactRefProbeStdinExec } from './exact-ref-probe' export type BranchConflictKind = 'local' | 'remote' @@ -79,12 +81,31 @@ function buildRemoteBranchConflictRefs( return [...refs] } +/** One batched child answers for every remote; the per-ref probes only run when the host cannot + * feed stdin, or when the batch came back undecided. */ +async function probeAnyRemoteConflictRef( + exec: ExactRefProbeExec, + batchedExec: ExactRefProbeStdinExec | undefined, + candidateRefs: readonly string[], + probeOptions: ExactRefProbeExecOptions +): Promise<{ found: boolean }> { + if (batchedExec) { + // A present ref is always decisive, so `found` never survives with `unknown` set. + const batched = await probeAnyExactRefBatched(batchedExec, candidateRefs, probeOptions) + if (!batched.unknown) { + return { found: batched.found } + } + } + return probeAnyExactRef(exec, candidateRefs, probeOptions) +} + /** Run branch-conflict policy through the host that owns Git execution. */ export async function getBranchConflictKindViaExec( exec: ExactRefProbeExec, branchName: string, allowedBaseRef?: string, - options: ExactRefProbeExecOptions = {} + options: ExactRefProbeExecOptions = {}, + batchedExec?: ExactRefProbeStdinExec ): Promise { if (!canQueryRemoteBranchName(branchName)) { return null @@ -104,7 +125,12 @@ export async function getBranchConflictKindViaExec( return null } - const { found: hasRemoteConflict } = await probeAnyExactRef(exec, candidateRefs, probeOptions) + const { found: hasRemoteConflict } = await probeAnyRemoteConflictRef( + exec, + batchedExec, + candidateRefs, + probeOptions + ) return hasRemoteConflict ? 'remote' : null } catch { @@ -119,15 +145,22 @@ export function getBranchConflictKind( options: LocalGitExecOptions = {} ): Promise { const execOptions = gitExecOptions(path, options) + const runLocalGit = ( + argv: string[], + commandOptions?: ExactRefProbeExecOptions & { stdin?: string } + ): Promise<{ stdout: string }> => + gitExecFileAsync(argv, { + ...execOptions, + ...(commandOptions?.maxBuffer === undefined ? {} : { maxBuffer: commandOptions.maxBuffer }), + ...(commandOptions?.timeoutMs === undefined ? {} : { timeout: commandOptions.timeoutMs }), + ...(commandOptions?.stdin === undefined ? {} : { stdin: commandOptions.stdin }) + }) return getBranchConflictKindViaExec( - (argv, commandOptions) => - gitExecFileAsync(argv, { - ...execOptions, - ...(commandOptions?.maxBuffer === undefined ? {} : { maxBuffer: commandOptions.maxBuffer }), - ...(commandOptions?.timeoutMs === undefined ? {} : { timeout: commandOptions.timeoutMs }) - }), + runLocalGit, branchName, - allowedBaseRef + allowedBaseRef, + {}, + (argv, commandOptions) => runLocalGit(argv, commandOptions) ) } diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index e53ae264129..7d28f38f2db 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -2189,130 +2189,139 @@ export async function createLocalWorktree( let lastExistingReviewNumber: number | null = null const shouldRetireGeneratedName = args.nameWasGenerated === true && isGeneratedWorktreeCreateName(sanitizedName) - const retiredNameRegistry = shouldRetireGeneratedName - ? await getRetiredNameRegistryForRepo(store, repo, store.getRepos(), settings) - : null - const isRetiredName = retiredNameRegistry ? createRetiredNameLookup(retiredNameRegistry) : null - // Why: a create-from-review branch override may already exist locally; suffix both branch and path instead of blocking the user. - for (let suffix = 1, attempts = 0; attempts < WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS; suffix += 1) { - effectiveSanitizedName = shouldRetireGeneratedName - ? getGeneratedWorktreeCreateCandidate( - sanitizedName, - suffix, - retiredNameRegistry?.exhaustedTiers - ) - : getWorktreeCreateCandidate(sanitizedName, suffix) - effectiveRequestedName = shouldRetireGeneratedName - ? effectiveSanitizedName - : requestedName.trim() - ? getWorktreeCreateCandidate(requestedName, suffix) - : effectiveSanitizedName - if (isRetiredName?.(effectiveSanitizedName)) { - continue - } - attempts += 1 - lastExistingReviewNumber = null + await timing.time('resolve_name', async () => { + const retiredNameRegistry = shouldRetireGeneratedName + ? await getRetiredNameRegistryForRepo(store, repo, store.getRepos(), settings) + : null + const isRetiredName = retiredNameRegistry ? createRetiredNameLookup(retiredNameRegistry) : null + // Why: a create-from-review branch override may already exist locally; suffix both branch and path instead of blocking the user. + for ( + let suffix = 1, attempts = 0; + attempts < WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS; + suffix += 1 + ) { + effectiveSanitizedName = shouldRetireGeneratedName + ? getGeneratedWorktreeCreateCandidate( + sanitizedName, + suffix, + retiredNameRegistry?.exhaustedTiers + ) + : getWorktreeCreateCandidate(sanitizedName, suffix) + effectiveRequestedName = shouldRetireGeneratedName + ? effectiveSanitizedName + : requestedName.trim() + ? getWorktreeCreateCandidate(requestedName, suffix) + : effectiveSanitizedName + if (isRetiredName?.(effectiveSanitizedName)) { + continue + } + attempts += 1 + lastExistingReviewNumber = null - branchName = await resolveCreateBranchName( - repo.path, - selectedExistingLocalBranchName - ? selectedExistingLocalBranchName - : getBranchNameOverrideCandidate(args.branchNameOverride, suffix), - effectiveSanitizedName, - settings, - username, - localWorktreeGitOptions - ) - checkoutExistingBranch = await canCheckoutExistingLocalBranch( - repo.path, - branchName, - baseBranch, - localWorktreeGitOptions - ) - if (checkoutExistingBranch && !selectedExistingLocalBranchName) { - // Why: suffix retries may need a new path, but an existing-branch checkout must keep the user-selected branch, not a sibling. - selectedExistingLocalBranchName = branchName - } - lastBranchConflictKind = checkoutExistingBranch - ? null - : await getBranchConflictKind(repo.path, branchName, baseBranch, localWorktreeGitOptions) - const allowedPushTargetRemoteConflict = - lastBranchConflictKind && - isAllowedPushTargetRemoteConflict(lastBranchConflictKind, branchName, args) - if (lastBranchConflictKind) { - if (allowedPushTargetRemoteConflict) { - lastExistingPR = null - let lookupFailed = false - const selectedReview = getSelectedReviewBranch(args) - if (selectedReview?.provider === 'github') { - try { - lastExistingPR = await getLocalGitHubPrForBranch( - repo.path, - branchName, - localWorktreeGitOptions - ) - } catch { - lookupFailed = true - } - if (!lookupFailed && isMatchingSelectedGitHubPr(lastExistingPR, args, branchName)) { - lastBranchConflictKind = null - } else if (lastExistingPR) { - lastExistingReviewNumber = lastExistingPR.number - } - } else if (selectedReview) { - let hostedReview: Awaited> = null - try { - hostedReview = await getSelectedHostedReviewForBranch(repo, branchName, args) - } catch { - lookupFailed = true - } - if (!lookupFailed && hostedReview?.matchesSelected) { - lastBranchConflictKind = null - } else if (hostedReview) { - lastExistingReviewNumber = hostedReview.number + branchName = await resolveCreateBranchName( + repo.path, + selectedExistingLocalBranchName + ? selectedExistingLocalBranchName + : getBranchNameOverrideCandidate(args.branchNameOverride, suffix), + effectiveSanitizedName, + settings, + username, + localWorktreeGitOptions + ) + checkoutExistingBranch = await canCheckoutExistingLocalBranch( + repo.path, + branchName, + baseBranch, + localWorktreeGitOptions + ) + if (checkoutExistingBranch && !selectedExistingLocalBranchName) { + // Why: suffix retries may need a new path, but an existing-branch checkout must keep the user-selected branch, not a sibling. + selectedExistingLocalBranchName = branchName + } + lastBranchConflictKind = checkoutExistingBranch + ? null + : await getBranchConflictKind(repo.path, branchName, baseBranch, localWorktreeGitOptions) + const allowedPushTargetRemoteConflict = + lastBranchConflictKind && + isAllowedPushTargetRemoteConflict(lastBranchConflictKind, branchName, args) + if (lastBranchConflictKind) { + if (allowedPushTargetRemoteConflict) { + lastExistingPR = null + let lookupFailed = false + const selectedReview = getSelectedReviewBranch(args) + if (selectedReview?.provider === 'github') { + try { + lastExistingPR = await getLocalGitHubPrForBranch( + repo.path, + branchName, + localWorktreeGitOptions + ) + } catch { + lookupFailed = true + } + if (!lookupFailed && isMatchingSelectedGitHubPr(lastExistingPR, args, branchName)) { + lastBranchConflictKind = null + } else if (lastExistingPR) { + lastExistingReviewNumber = lastExistingPR.number + } + } else if (selectedReview) { + let hostedReview: Awaited> = null + try { + hostedReview = await getSelectedHostedReviewForBranch(repo, branchName, args) + } catch { + lookupFailed = true + } + if (!lookupFailed && hostedReview?.matchesSelected) { + lastBranchConflictKind = null + } else if (hostedReview) { + lastExistingReviewNumber = hostedReview.number + } } } } - } - if (lastBranchConflictKind) { - continue - } - - // Why: gh pr list is a ~1–3s network call; only probe PR conflicts after a branch collision (suffix > 1) so the common no-collision path skips it. - if (suffix > 1 && !checkoutExistingBranch) { - lastExistingPR = null - try { - lastExistingPR = await getLocalGitHubPrForBranch( - repo.path, - branchName, - localWorktreeGitOptions - ) - } catch { - // GitHub API may be unreachable, rate-limited, or token missing - } - if (lastExistingPR && !isMatchingSelectedGitHubPr(lastExistingPR, args, branchName)) { - lastExistingReviewNumber = lastExistingPR.number + if (lastBranchConflictKind) { continue } - } - worktreePath = ensurePathWithinWorkspace( - computeWorktreePath(effectiveSanitizedName, repo.path, worktreePathSettings), - workspaceRoot - ) - if (existsSync(worktreePath)) { - continue - } + // Why: gh pr list is a ~1–3s network call; only probe PR conflicts after a branch collision (suffix > 1) so the common no-collision path skips it. + if (suffix > 1 && !checkoutExistingBranch) { + lastExistingPR = null + try { + lastExistingPR = await getLocalGitHubPrForBranch( + repo.path, + branchName, + localWorktreeGitOptions + ) + } catch { + // GitHub API may be unreachable, rate-limited, or token missing + } + if (lastExistingPR && !isMatchingSelectedGitHubPr(lastExistingPR, args, branchName)) { + lastExistingReviewNumber = lastExistingPR.number + continue + } + } - resolved = true - break - } + worktreePath = ensurePathWithinWorkspace( + computeWorktreePath(effectiveSanitizedName, repo.path, worktreePathSettings), + workspaceRoot + ) + if (existsSync(worktreePath)) { + continue + } + + resolved = true + break + } + }) if (!resolved) { // Why: every suffix collided; reject with a specific reason so the user sees why create failed instead of a generic error or hung spinner. - if (lastExistingReviewNumber !== null) { + // Read once and format eagerly: the suffix loop assigns this from a callback, so the `let`'s + // narrowing does not reach the message. + const existingReviewNumber = lastExistingReviewNumber + if (existingReviewNumber !== null) { throw new Error( - `Branch "${branchName}" already has PR #${lastExistingReviewNumber}. Pick a different ${branchConflictSubject}.` + `Branch "${branchName}" already has PR #${String(existingReviewNumber)}. Pick a different ${branchConflictSubject}.` ) } if (lastBranchConflictKind) { @@ -2361,14 +2370,17 @@ export async function createLocalWorktree( emitCreateWorktreeProgress(mainWindow, 'creating', args.creationId) let preparedPushTarget: GitPushTarget | undefined - if (args.pushTarget) { + const requestedPushTarget = args.pushTarget + if (requestedPushTarget) { // Why: validate/fetch the contributor remote before create so a failure doesn't leave a half-created worktree with conflicts on retry. - preparedPushTarget = await prepareWorktreePushTarget( - repo.path, - args.pushTarget, - store, - repo.id, - localWorktreeGitOptions + preparedPushTarget = await timing.time('prepare_push_target', () => + prepareWorktreePushTarget( + repo.path, + requestedPushTarget, + store, + repo.id, + localWorktreeGitOptions + ) ) } diff --git a/src/main/ipc/worktrees/create/register-worktree-create-handlers.ts b/src/main/ipc/worktrees/create/register-worktree-create-handlers.ts index 775a0859790..f371529963c 100644 --- a/src/main/ipc/worktrees/create/register-worktree-create-handlers.ts +++ b/src/main/ipc/worktrees/create/register-worktree-create-handlers.ts @@ -4,7 +4,10 @@ import type { CreateWorktreeResult, AdoptProvisionedRootArgs } from '../../../../shared/worktree/create-types' -import { withWorktreeSpan } from '../../../observability/instrumentation' +import { + addWorktreeCreatePhaseAttributes, + withWorktreeSpan +} from '../../../observability/instrumentation' import { workspaceSourceSchema } from '../../../../shared/telemetry-events' import type { WorkspaceSource } from '../../../../shared/telemetry-events' import { @@ -36,7 +39,7 @@ export function registerWorktreeCreateHandlers(context: WorktreeIpcContext): voi async (_event, rawArgs: CreateWorktreeArgs): Promise => { const args = normalizeLinkedWorkItemFields(rawArgs) // Why span here: parent the child git spans for the trace tree; don't attach branch name/remote URL (user content) — repo ID is the safer correlator. - return withWorktreeSpan({ stage: 'create' }, async () => { + return withWorktreeSpan({ stage: 'create' }, async (span) => { const repo = store.getRepo(args.repoId) if (!repo) { throw new Error(`Repo not found: ${args.repoId}`) @@ -74,6 +77,9 @@ export function registerWorktreeCreateHandlers(context: WorktreeIpcContext): voi throw error } finishAutomationWorkspaceProvenanceRequest(args.automationProvenanceRequest) + if (result.timing) { + addWorktreeCreatePhaseAttributes(span, result.timing) + } // Why: reaching here means create succeeded (helpers throw); skip a separate workspace_initialized (telemetry-plan.md§Deferred); never send the branch name. track('workspace_created', { diff --git a/src/main/observability/instrumentation.test.ts b/src/main/observability/instrumentation.test.ts index 2f7d1da05dd..978854897e4 100644 --- a/src/main/observability/instrumentation.test.ts +++ b/src/main/observability/instrumentation.test.ts @@ -3,6 +3,7 @@ import { _resetTracerForTests, setActiveSink, type TracerSink } from './tracer' import { _gitSpanSamplingBucketCountForTests, _resetGitSpanSamplingForTests, + addWorktreeCreatePhaseAttributes, withGitSpan } from './instrumentation' @@ -167,3 +168,50 @@ describe('withGitSpan sampling', () => { expect(_gitSpanSamplingBucketCountForTests()).toBe(1) }) }) + +describe('addWorktreeCreatePhaseAttributes', () => { + function capture(): { + attributes: Record + span: Parameters[0] + } { + const attributes: Record = {} + const span = { + setAttribute: (key: string, value: unknown) => { + attributes[key] = value + } + } as unknown as Parameters[0] + return { attributes, span } + } + + it('counts concurrent phases once when measuring unattributed time', () => { + const { attributes, span } = capture() + // Create resolves shared directories and .worktreeinclude concurrently; summing their + // durations would claim 400ms of coverage for a 200ms window. + addWorktreeCreatePhaseAttributes(span, { + totalDurationMs: 1000, + phases: [ + { phase: 'resolve_shared_directories', startedAtMs: 100, durationMs: 200 }, + { phase: 'resolve_worktreeinclude', startedAtMs: 150, durationMs: 150 } + ] + }) + + expect(attributes['worktree.create.phase.resolve_shared_directories_ms']).toBe(200) + expect(attributes['worktree.create.phase.resolve_worktreeinclude_ms']).toBe(150) + // Covered wall clock is 100..300, so 800ms is genuinely unaccounted for. + expect(attributes['worktree.create.unattributed_ms']).toBe(800) + }) + + it('sums disjoint phases and never reports negative unattributed time', () => { + const { attributes, span } = capture() + addWorktreeCreatePhaseAttributes(span, { + totalDurationMs: 500, + phases: [ + { phase: 'resolve_name', startedAtMs: 0, durationMs: 100 }, + { phase: 'git_worktree_add', startedAtMs: 300, durationMs: 200 } + ] + }) + + expect(attributes['worktree.create.total_ms']).toBe(500) + expect(attributes['worktree.create.unattributed_ms']).toBe(200) + }) +}) diff --git a/src/main/observability/instrumentation.ts b/src/main/observability/instrumentation.ts index bab57b74f64..fb57aa5a870 100644 --- a/src/main/observability/instrumentation.ts +++ b/src/main/observability/instrumentation.ts @@ -202,10 +202,11 @@ export type WorktreeSpanArgs = { readonly path?: string } -/** Wrap a worktree-setup phase in a `worktree.` span. */ +/** Wrap a worktree-setup phase in a `worktree.` span. The callback receives the span so a + * create can attach its own phase breakdown; the git children alone leave the waits invisible. */ export async function withWorktreeSpan( meta: WorktreeSpanArgs, - fn: () => Promise + fn: (span: ActiveSpan) => Promise ): Promise { return withSpan( `worktree.${meta.stage}`, @@ -214,12 +215,64 @@ export async function withWorktreeSpan( if (meta.path) { span.setAttribute('worktree.path', meta.path) } - return await fn() + return await fn(span) }, { attributes: { kind: 'worktree' } } ) } +type WorktreeCreatePhaseTiming = { + readonly phase: string + readonly startedAtMs: number + readonly durationMs: number +} + +/** Wall-clock span covered by at least one phase. Create runs some phases concurrently, so summing + * durations double-counts and would report overlap as coverage the phases never had. */ +function measuredWallClockMs(phases: readonly WorktreePhaseInterval[]): number { + const intervals = [...phases] + .map((phase) => [phase.startedAtMs, phase.startedAtMs + phase.durationMs] as const) + .sort((left, right) => left[0] - right[0]) + let covered = 0 + let openedAt: number | null = null + let closesAt = 0 + for (const [start, end] of intervals) { + if (openedAt === null) { + openedAt = start + closesAt = end + continue + } + if (start <= closesAt) { + closesAt = Math.max(closesAt, end) + continue + } + covered += closesAt - openedAt + openedAt = start + closesAt = end + } + return openedAt === null ? 0 : covered + (closesAt - openedAt) +} + +type WorktreePhaseInterval = Pick + +/** Records a create's phase breakdown on its span. Phase names are already a closed vocabulary in + * the recorder, so they are safe to key on; nothing here carries a branch name or a path. */ +export function addWorktreeCreatePhaseAttributes( + span: ActiveSpan, + timing: { totalDurationMs: number; phases: readonly WorktreeCreatePhaseTiming[] } +): void { + span.setAttribute('worktree.create.total_ms', Math.round(timing.totalDurationMs)) + for (const phase of timing.phases) { + span.setAttribute(`worktree.create.phase.${phase.phase}_ms`, Math.round(phase.durationMs)) + } + // What the phases do not cover is the number that matters when create feels slow for no visible + // reason, so name it rather than leaving it to subtraction. + span.setAttribute( + 'worktree.create.unattributed_ms', + Math.max(0, Math.round(timing.totalDurationMs - measuredWallClockMs(timing.phases))) + ) +} + /** Closed set so a typo can't silently mint an orphan span name. */ export type WorktreeRemoveStage = | 'archive_hook' diff --git a/src/main/worktree-create-preparation-burst.ts b/src/main/worktree-create-preparation-burst.ts new file mode 100644 index 00000000000..d289927ffaa --- /dev/null +++ b/src/main/worktree-create-preparation-burst.ts @@ -0,0 +1,21 @@ +import { setBoundedMapEntry } from './runtime/runtime-async-boundaries' + +/** Two creates this close together mean more are likely; an isolated create earns no replacement. */ +export const WORKTREE_CREATE_BURST_MS = 5 * 60_000 +const WORKTREE_CREATE_PREPARATION_CONSUME_MAX = 64 + +/** When each preparation key was last consumed, so a burst can be told from an isolated create. */ +const lastConsumedAt = new Map() + +/** Records this consume and reports whether it continues a burst. A replacement checkout costs a + * full tree and holds disk until its TTL, so only a user who is already creating repeatedly earns + * one; the first create of a session pays nothing for a spare nobody claims. */ +export function recordPreparationConsume(key: string, now = Date.now()): boolean { + const previous = lastConsumedAt.get(key) + setBoundedMapEntry(lastConsumedAt, key, now, WORKTREE_CREATE_PREPARATION_CONSUME_MAX) + return previous !== undefined && now - previous <= WORKTREE_CREATE_BURST_MS +} + +export function resetPreparationConsumeHistoryForTests(): void { + lastConsumedAt.clear() +} diff --git a/src/main/worktree-create-preparation-stale-cleanup.ts b/src/main/worktree-create-preparation-stale-cleanup.ts new file mode 100644 index 00000000000..4a422f672ed --- /dev/null +++ b/src/main/worktree-create-preparation-stale-cleanup.ts @@ -0,0 +1,79 @@ +import { + isWorktreeCreatePreparation, + parseWorktreePreparationOwnerPid, + parseWorktreePreparationPathOwnerPid +} from '../shared/worktree/create-preparation' +import type { AddWorktreeOptions } from './git/worktree' +import { listWorktreeGraph } from './git/worktree' +import { discardPreparedWorktree, unlockPreparedWorktree } from './git/worktree-create-preparation' +import { retryPendingPreparationDiscards } from './worktree-preparation-discard-retry' + +const STALE_PREPARATION_CLEANUP_CONCURRENCY = 4 + +const staleCleanupInFlight = new Map>() + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch (error) { + return (error as NodeJS.ErrnoException).code !== 'ESRCH' + } +} + +/** Reclaims preparations a crashed process left registered. Single-flighted per host key so a burst + * of arming calls shares one worktree listing. */ +export async function cleanupStalePreparations( + cleanupKey: string, + repoPath: string, + options: AddWorktreeOptions +): Promise { + const existing = staleCleanupInFlight.get(cleanupKey) + if (existing) { + await existing.catch(() => {}) + return + } + const cleanup = (async () => { + // Not awaited: the create path awaits this cleanup, and one stranded discard costs an unlock plus + // a `worktree remove --force` bounded at 30s each. Reclaiming leaked scratch must not delay create. + void retryPendingPreparationDiscards(cleanupKey) + const worktrees = await listWorktreeGraph(repoPath, { + ...options, + includeCreatePreparations: true + }) + const staleWorktrees = worktrees.filter(isWorktreeCreatePreparation) + let nextIndex = 0 + async function discardNextStalePreparation(): Promise { + while (nextIndex < staleWorktrees.length) { + const worktree = staleWorktrees[nextIndex] + nextIndex += 1 + const lockOwnerPid = parseWorktreePreparationOwnerPid(worktree.lockReason) + const pathOwnerPid = parseWorktreePreparationPathOwnerPid(worktree.path) + if (!lockOwnerPid || isProcessAlive(lockOwnerPid)) { + continue + } + // Preserve a branch-attached final path after a crash; only detached or + // still-hidden preparations are safe to discard automatically. + if (worktree.branch && pathOwnerPid === null) { + await unlockPreparedWorktree(repoPath, worktree.path, options).catch(() => {}) + } else if (pathOwnerPid === lockOwnerPid) { + await discardPreparedWorktree(repoPath, worktree.path, options).catch(() => {}) + } + } + } + const workerCount = Math.min(STALE_PREPARATION_CLEANUP_CONCURRENCY, staleWorktrees.length) + await Promise.all(Array.from({ length: workerCount }, () => discardNextStalePreparation())) + })() + staleCleanupInFlight.set(cleanupKey, cleanup) + try { + await cleanup.catch(() => {}) + } finally { + if (staleCleanupInFlight.get(cleanupKey) === cleanup) { + staleCleanupInFlight.delete(cleanupKey) + } + } +} + +export function resetStalePreparationCleanupForTests(): void { + staleCleanupInFlight.clear() +} diff --git a/src/main/worktree-create-preparation.test.ts b/src/main/worktree-create-preparation.test.ts index 3e03643d6a8..dababbef88d 100644 --- a/src/main/worktree-create-preparation.test.ts +++ b/src/main/worktree-create-preparation.test.ts @@ -466,4 +466,51 @@ describe('worktree create preparation registry', () => { expect(mocks.mkdir).toHaveBeenCalledWith('/workspace', { recursive: true }) expect(mocks.discard).toHaveBeenCalledTimes(1) }) + + function consumeOnce(name: string): ReturnType { + return consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot: '/workspace', + worktreePath: `/workspace/${name}`, + branch: `feature/${name}`, + baseBranch: 'origin/main' + }) + } + + it('does not re-arm after an isolated create', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + await expect(consumeOnce('only')).resolves.toEqual({}) + + // Why: a lone create would otherwise leave a full spare checkout on disk for the whole TTL. + expect(mocks.prepareCheckout).toHaveBeenCalledTimes(1) + }) + + it('re-arms a preparation once creates arrive in a burst', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + await expect(consumeOnce('first')).resolves.toEqual({}) + expect(mocks.prepareCheckout).toHaveBeenCalledTimes(1) + + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + expect(mocks.prepareCheckout).toHaveBeenCalledTimes(2) + + // No arming call follows this consume: the third checkout can only come from the re-arm. + await expect(consumeOnce('second')).resolves.toEqual({}) + expect(mocks.prepareCheckout).toHaveBeenCalledTimes(3) + + // The replacement is claimable, so a third create still skips the cold add. + await expect(consumeOnce('third')).resolves.toEqual({}) + expect(mocks.finalize).toHaveBeenCalledTimes(3) + }) + + it('does not re-arm when finalization failed', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + await expect(consumeOnce('first')).resolves.toEqual({}) + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + mocks.prepareCheckout.mockClear() + mocks.finalize.mockRejectedValueOnce(new Error('submodules prevent worktree move')) + + await expect(consumeOnce('second')).resolves.toBeNull() + + expect(mocks.prepareCheckout).not.toHaveBeenCalled() + }) }) diff --git a/src/main/worktree-create-preparation.ts b/src/main/worktree-create-preparation.ts index ff7478cb46e..22bcf5d1e2c 100644 --- a/src/main/worktree-create-preparation.ts +++ b/src/main/worktree-create-preparation.ts @@ -7,17 +7,12 @@ import { isFolderRepo } from '../shared/repo-kind' import { isWindowsAbsolutePathLike } from '../shared/cross-platform-path' import { WORKTREE_CREATE_PREPARATION_DIRECTORY, - createWorktreePreparationLockReason, - isWorktreeCreatePreparation, - parseWorktreePreparationOwnerPid, - parseWorktreePreparationPathOwnerPid + createWorktreePreparationLockReason } from '../shared/worktree/create-preparation' import type { AddWorktreeOptions, AddWorktreeResult } from './git/worktree' -import { listWorktreeGraph } from './git/worktree' import { discardPreparedWorktree, finalizePreparedWorktree, - unlockPreparedWorktree, prepareWorktreeCreateCheckout } from './git/worktree-create-preparation' import { @@ -25,17 +20,23 @@ import { getWorktreeMirrorDistro } from './project-runtime-git-options' import { computeWorkspaceRootAsync, getWorktreePathSettings } from './ipc/worktree-logic' +import { + recordPreparationConsume, + resetPreparationConsumeHistoryForTests +} from './worktree-create-preparation-burst' +import { + cleanupStalePreparations, + resetStalePreparationCleanupForTests +} from './worktree-create-preparation-stale-cleanup' import { toHostFilesystemPath } from './host-tree-removal' import { discardPreparationWithRetry, resetPendingPreparationDiscardsForTests, - retryPendingPreparationDiscards, trackPreparationDiscard } from './worktree-preparation-discard-retry' export const WORKTREE_CREATE_PREPARATION_TTL_MS = 5 * 60_000 export const WORKTREE_CREATE_PREPARATION_LIMIT = 3 -const STALE_PREPARATION_CLEANUP_CONCURRENCY = 4 type PreparationEntry = { key: string @@ -59,7 +60,6 @@ type ConsumePreparedWorktreeArgs = { } const preparations = new Map() -const staleCleanupInFlight = new Map>() function pathOps(path: string): Pick { return isWindowsAbsolutePathLike(path) ? win32 : posix @@ -79,15 +79,6 @@ function preparationKey( return `${pathKey(repoPath)}\0${pathKey(workspaceRoot)}\0${baseBranch}\0${options.wslDistro ?? ''}` } -function isProcessAlive(pid: number): boolean { - try { - process.kill(pid, 0) - return true - } catch (error) { - return (error as NodeJS.ErrnoException).code !== 'ESRCH' - } -} - function preparationHostKey(repoPath: string, options: AddWorktreeOptions): string { return `${pathKey(repoPath)}\0${options.wslDistro ?? ''}` } @@ -131,57 +122,6 @@ function enforcePreparationLimit(): void { } } -async function cleanupStalePreparations( - repoPath: string, - options: AddWorktreeOptions -): Promise { - const cleanupKey = preparationHostKey(repoPath, options) - const existing = staleCleanupInFlight.get(cleanupKey) - if (existing) { - await existing.catch(() => {}) - return - } - const cleanup = (async () => { - // Not awaited: the create path awaits this cleanup, and one stranded discard costs an unlock plus - // a `worktree remove --force` bounded at 30s each. Reclaiming leaked scratch must not delay create. - void retryPendingPreparationDiscards(cleanupKey) - const worktrees = await listWorktreeGraph(repoPath, { - ...options, - includeCreatePreparations: true - }) - const staleWorktrees = worktrees.filter(isWorktreeCreatePreparation) - let nextIndex = 0 - async function discardNextStalePreparation(): Promise { - while (nextIndex < staleWorktrees.length) { - const worktree = staleWorktrees[nextIndex] - nextIndex += 1 - const lockOwnerPid = parseWorktreePreparationOwnerPid(worktree.lockReason) - const pathOwnerPid = parseWorktreePreparationPathOwnerPid(worktree.path) - if (!lockOwnerPid || isProcessAlive(lockOwnerPid)) { - continue - } - // Preserve a branch-attached final path after a crash; only detached or - // still-hidden preparations are safe to discard automatically. - if (worktree.branch && pathOwnerPid === null) { - await unlockPreparedWorktree(repoPath, worktree.path, options).catch(() => {}) - } else if (pathOwnerPid === lockOwnerPid) { - await discardPreparedWorktree(repoPath, worktree.path, options).catch(() => {}) - } - } - } - const workerCount = Math.min(STALE_PREPARATION_CLEANUP_CONCURRENCY, staleWorktrees.length) - await Promise.all(Array.from({ length: workerCount }, () => discardNextStalePreparation())) - })() - staleCleanupInFlight.set(cleanupKey, cleanup) - try { - await cleanup.catch(() => {}) - } finally { - if (staleCleanupInFlight.get(cleanupKey) === cleanup) { - staleCleanupInFlight.delete(cleanupKey) - } - } -} - export async function prepareWorktreeCreateForRepo( store: Store, repo: Repo, @@ -205,6 +145,16 @@ export async function prepareWorktreeCreateForRepo( return existing.ready } + return startPreparation(key, repo.path, workspaceRoot, baseBranch, options) +} + +function startPreparation( + key: string, + repoPath: string, + workspaceRoot: string, + baseBranch: string, + options: AddWorktreeOptions +): Promise { enforcePreparationLimit() const preparationId = `${process.pid}-${randomUUID()}` const lockReason = createWorktreePreparationLockReason(preparationId) @@ -218,21 +168,21 @@ export async function prepareWorktreeCreateForRepo( expiration.unref() Object.assign(entry, { key, - repoPath: repo.path, + repoPath, workspaceRoot, preparedPath, options, createdAt: Date.now(), expiration, ready: (async () => { - await cleanupStalePreparations(repo.path, options) + await cleanupStalePreparations(preparationHostKey(repoPath, options), repoPath, options) await mkdir( toHostFilesystemPath( pathOps(workspaceRoot).join(workspaceRoot, WORKTREE_CREATE_PREPARATION_DIRECTORY) ), { recursive: true } ) - await prepareWorktreeCreateCheckout(repo.path, preparedPath, baseBranch, lockReason, options) + await prepareWorktreeCreateCheckout(repoPath, preparedPath, baseBranch, lockReason, options) })() } satisfies PreparationEntry) preparations.set(key, entry) @@ -266,6 +216,28 @@ async function claimPreparedWorktree( } } +/** Replaces a just-consumed preparation, but only once the user has shown they are creating in a + * burst. A replacement costs a full checkout and ~5 minutes of disk until its TTL, so arming one + * after an isolated create spends that on nobody. Never awaited: create has already returned by + * the time the replacement checkout finishes. */ +function rearmPreparation(entry: PreparationEntry, baseBranch: string): void { + // Record first: a prefetch that re-armed this key while we finalized would otherwise swallow the + // consume, and the next create would look isolated when it is really the middle of a burst. + const continuesBurst = recordPreparationConsume(entry.key) + if (preparations.has(entry.key) || !continuesBurst) { + return + } + void startPreparation( + entry.key, + entry.repoPath, + entry.workspaceRoot, + baseBranch, + entry.options + ).catch(() => { + // Why: a warm-up failure is recovered by the normal add on the next create. + }) +} + export async function consumePreparedWorktreeCreate( args: ConsumePreparedWorktreeArgs ): Promise { @@ -283,7 +255,7 @@ export async function consumePreparedWorktreeCreate( await mkdir(toHostFilesystemPath(pathOps(args.worktreePath).dirname(args.worktreePath)), { recursive: true }) - return await finalizePreparedWorktree( + const result = await finalizePreparedWorktree( args.repoPath, entry.preparedPath, args.worktreePath, @@ -292,6 +264,10 @@ export async function consumePreparedWorktreeCreate( args.refreshLocalBaseRef, options ) + // Consuming the only prepared checkout leaves the next create cold. Re-arm for a user who is + // creating in a burst; the TTL and the preparation limit still bound an unused replacement. + rearmPreparation(entry, args.baseBranch) + return result } catch (error) { await discardPreparedWorktree(args.repoPath, entry.preparedPath, options).catch(() => {}) console.warn( @@ -305,7 +281,8 @@ export async function consumePreparedWorktreeCreate( export async function _resetWorktreeCreatePreparationsForTests(): Promise { const entries = [...preparations.values()] preparations.clear() - staleCleanupInFlight.clear() + resetPreparationConsumeHistoryForTests() + resetStalePreparationCleanupForTests() await Promise.all( entries.map(async (entry) => { clearTimeout(entry.expiration)