From 6e2a88c0914d1820f4737ca5fa9af3df7b450da9 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 1 Aug 2026 21:59:41 -0700 Subject: [PATCH] perf(worktrees): avoid redundant fetch during deletion (#11918) --- src/main/git/remove-worktree.test.ts | 16 ++-- src/main/git/worktree.ts | 8 +- src/relay/git-handler-branch-cleanup.test.ts | 18 +++-- src/relay/git-handler-branch-cleanup.ts | 13 ++-- src/shared/git-branch-cleanup.test.ts | 74 +++++++++++++++++++ src/shared/git-branch-cleanup.ts | 27 +++++++ .../worktree-deletion-branch-fixture.mjs | 69 +++++++++++++++++ .../worktree-deletion-dev-bench.mjs | 28 +++++-- 8 files changed, 223 insertions(+), 30 deletions(-) create mode 100644 tests/tools/benchmarks/worktree-deletion-branch-fixture.mjs diff --git a/src/main/git/remove-worktree.test.ts b/src/main/git/remove-worktree.test.ts index 7a9bda07f..db008799d 100644 --- a/src/main/git/remove-worktree.test.ts +++ b/src/main/git/remove-worktree.test.ts @@ -574,6 +574,9 @@ branch refs/heads/main 'git rev-parse --verify --quiet refs/remotes/origin/main^{commit}': { stdout: 'base123\n' }, + 'git rev-parse --verify --quiet HEAD^{commit}': { + stdout: 'base123\n' + }, 'git merge-tree --write-tree base123 refs/heads/feature/test': { stdout: 'tree123\n' }, @@ -589,6 +592,7 @@ branch refs/heads/main expect(calls).toContain('git merge-tree --write-tree base123 refs/heads/feature/test') expect(calls).toContain('git update-ref -d refs/heads/feature/test def456') expect(calls).toContain('git config --remove-section branch.feature/test') + expect(calls).not.toContain('git remote') }) it('deletes a squash-merged branch with branch-only merge commits via expected head', async () => { @@ -732,13 +736,15 @@ branch refs/heads/main await expect(removeWorktree('/repo', '/repo-feature')).resolves.toEqual({}) const calls = getGitCalls() + const mergeTreeCall = 'git merge-tree --write-tree base123 refs/heads/feature/test' + const mergeTreeIndexes = calls.flatMap((call, index) => (call === mergeTreeCall ? [index] : [])) + const fetchIndex = calls.indexOf('git fetch --prune origin') + const updateRefIndex = calls.indexOf('git update-ref -d refs/heads/feature/test def456') expect(calls).toContain('git fetch --prune origin') expect(calls).toContain('git update-ref -d refs/heads/feature/test def456') - expectGitCallOrder( - calls, - 'git fetch --prune origin', - 'git merge-tree --write-tree base123 refs/heads/feature/test' - ) + expect(mergeTreeIndexes).toHaveLength(1) + expect(fetchIndex).toBeLessThan(mergeTreeIndexes[0]) + expect(mergeTreeIndexes[0]).toBeLessThan(updateRefIndex) expectGitCallOrder( calls, 'git fetch --prune origin', diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index 6862b9723..4ea08ef68 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -2,9 +2,8 @@ import { readFile, stat } from 'node:fs/promises' import { isAbsolute, join, posix, resolve, win32 } from 'node:path' import { - branchHasNoUnmergedChangesOnAnyTarget, - getBranchCleanupTargetRefs, - refreshBranchCleanupTargetRefs + branchHasNoUnmergedChangesWithLazyTargetRefresh, + getBranchCleanupTargetRefs } from '../../shared/git-branch-cleanup' import { resolveWorktreeAddBaseRef } from '../../shared/worktree-base-ref' import { withSpan } from '../observability/tracer' @@ -1267,10 +1266,9 @@ async function deleteAlreadyMergedBranchAfterSafeDeleteFailure( ...(execOptions?.stdin !== undefined ? { stdin: execOptions.stdin } : {}) }) const targetRefs = await getBranchCleanupTargetRefs(runGit, branchName) - await refreshBranchCleanupTargetRefs(runGit, targetRefs) // Why: squash merges rewrite commit IDs, so `branch -d` rejects already-merged branches; delete only when Git proves no unmerged tree changes. if ( - !(await branchHasNoUnmergedChangesOnAnyTarget( + !(await branchHasNoUnmergedChangesWithLazyTargetRefresh( runGit, branchName, targetRefs, diff --git a/src/relay/git-handler-branch-cleanup.test.ts b/src/relay/git-handler-branch-cleanup.test.ts index 8058d55ae..40607bdac 100644 --- a/src/relay/git-handler-branch-cleanup.test.ts +++ b/src/relay/git-handler-branch-cleanup.test.ts @@ -191,6 +191,9 @@ describe('removeWorktreeOp branch cleanup', () => { if (args[0] === 'rev-parse' && args.includes('refs/remotes/origin/main^{commit}')) { return { stdout: 'base123\n', stderr: '' } } + if (args[0] === 'rev-parse' && args.includes('HEAD^{commit}')) { + return { stdout: 'base123\n', stderr: '' } + } if (args[0] === 'merge-tree') { return { stdout: 'tree123\n', stderr: '' } } @@ -217,6 +220,7 @@ describe('removeWorktreeOp branch cleanup', () => { ['config', '--remove-section', 'branch.feature/test'], expect.any(String) ) + expect(git).not.toHaveBeenCalledWith(['remote'], expect.any(String)) }) it('deletes a squash-merged SSH branch with branch-only merge commits via expected head', async () => { @@ -360,17 +364,17 @@ describe('removeWorktreeOp branch cleanup', () => { const commandIndex = (expectedArgs: string[]) => calls.findIndex(({ args }) => JSON.stringify(args) === JSON.stringify(expectedArgs)) const fetchIndex = commandIndex(['fetch', '--prune', 'origin']) - const mergeTreeIndex = commandIndex([ - 'merge-tree', - '--write-tree', - 'base123', - 'refs/heads/feature/test' - ]) + const mergeTreeArgs = ['merge-tree', '--write-tree', 'base123', 'refs/heads/feature/test'] + const mergeTreeIndexes = calls.flatMap(({ args }, index) => + JSON.stringify(args) === JSON.stringify(mergeTreeArgs) ? [index] : [] + ) const updateRefIndex = commandIndex(['update-ref', '-d', 'refs/heads/feature/test', '1']) expect(fetchIndex).toBeGreaterThanOrEqual(0) expect(calls[fetchIndex]?.cwd).toBe(resolvedRepoPath()) - expect(fetchIndex).toBeLessThan(mergeTreeIndex) + expect(mergeTreeIndexes).toHaveLength(1) + expect(fetchIndex).toBeLessThan(mergeTreeIndexes[0]) + expect(mergeTreeIndexes[0]).toBeLessThan(updateRefIndex) expect(fetchIndex).toBeLessThan(updateRefIndex) }) diff --git a/src/relay/git-handler-branch-cleanup.ts b/src/relay/git-handler-branch-cleanup.ts index 51aa0be52..1a195d59c 100644 --- a/src/relay/git-handler-branch-cleanup.ts +++ b/src/relay/git-handler-branch-cleanup.ts @@ -1,7 +1,6 @@ import { - branchHasNoUnmergedChangesOnAnyTarget, - getBranchCleanupTargetRefs, - refreshBranchCleanupTargetRefs + branchHasNoUnmergedChangesWithLazyTargetRefresh, + getBranchCleanupTargetRefs } from '../shared/git-branch-cleanup' import type { GitCapabilityCache } from '../shared/git-capability-cache' import type { GitExec } from './git-handler-ops' @@ -17,12 +16,16 @@ export async function deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure( const runGit = (args: string[], options?: { stdin?: string }) => options ? git(args, repoPath, options) : git(args, repoPath) const targetRefs = await getBranchCleanupTargetRefs(runGit, branchName) - await refreshBranchCleanupTargetRefs(runGit, targetRefs) // Why: SSH worktrees hit the same squash-merge shape as local worktrees. // Git's no-op merge proof lets us clean up only branches whose changes // already exist on the saved base ref. if ( - !(await branchHasNoUnmergedChangesOnAnyTarget(runGit, branchName, targetRefs, capabilities)) + !(await branchHasNoUnmergedChangesWithLazyTargetRefresh( + runGit, + branchName, + targetRefs, + capabilities + )) ) { return false } diff --git a/src/shared/git-branch-cleanup.test.ts b/src/shared/git-branch-cleanup.test.ts index 5c25089d5..ab59cdb8b 100644 --- a/src/shared/git-branch-cleanup.test.ts +++ b/src/shared/git-branch-cleanup.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import { branchHasNoUnmergedChangesOnAnyTarget, + branchHasNoUnmergedChangesWithLazyTargetRefresh, refreshBranchCleanupTargetRefs, type GitBranchCleanupExec } from './git-branch-cleanup' @@ -195,3 +196,76 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { expect(mergeTreeCalls).toHaveLength(1) }) }) + +describe('branchHasNoUnmergedChangesWithLazyTargetRefresh', () => { + it('skips refresh when local HEAD proves the branch changes are retained', async () => { + const runGit = vi.fn(async (args) => { + const command = args.join(' ') + const stdout = + { + 'rev-parse --verify --quiet HEAD^{commit}': 'local-target\n', + 'merge-tree --write-tree local-target refs/heads/feature/test': 'local-tree\n', + 'rev-parse --verify --quiet local-target^{tree}': 'local-tree\n' + }[command] ?? '' + return { stdout } + }) + + await expect( + branchHasNoUnmergedChangesWithLazyTargetRefresh( + runGit, + 'feature/test', + ['refs/remotes/origin/main', 'HEAD'], + new GitCapabilityCache() + ) + ).resolves.toBe(true) + + expect(runGit.mock.calls.map(([args]) => args)).not.toContainEqual(['remote']) + }) + + it('refreshes before trusting a stale remote-tracking proof', async () => { + let refreshed = false + const runGit = vi.fn(async (args) => { + const command = args.join(' ') + if (command === 'remote') { + return { stdout: 'origin\n' } + } + if (command === 'fetch --prune origin') { + refreshed = true + return { stdout: '' } + } + if (command === 'rev-parse --verify --quiet refs/remotes/origin/main^{commit}') { + return { stdout: 'remote-target\n' } + } + if (command === 'rev-parse --verify --quiet origin/main^{commit}') { + return { stdout: 'short-remote-target\n' } + } + if (command === 'rev-parse --verify --quiet HEAD^{commit}') { + return { stdout: 'local-target\n' } + } + if (command === 'merge-tree --write-tree remote-target refs/heads/feature/test') { + return { stdout: refreshed ? 'changed-tree\n' : 'remote-tree\n' } + } + if (command === 'merge-tree --write-tree short-remote-target refs/heads/feature/test') { + return { stdout: refreshed ? 'changed-tree\n' : 'short-remote-tree\n' } + } + if (command === 'rev-parse --verify --quiet remote-target^{tree}') { + return { stdout: 'remote-tree\n' } + } + if (command === 'rev-parse --verify --quiet short-remote-target^{tree}') { + return { stdout: 'short-remote-tree\n' } + } + return { stdout: '' } + }) + + await expect( + branchHasNoUnmergedChangesWithLazyTargetRefresh( + runGit, + 'feature/test', + ['refs/remotes/origin/main', 'origin/main', 'HEAD'], + new GitCapabilityCache() + ) + ).resolves.toBe(false) + + expect(runGit.mock.calls.map(([args]) => args)).toContainEqual(['fetch', '--prune', 'origin']) + }) +}) diff --git a/src/shared/git-branch-cleanup.ts b/src/shared/git-branch-cleanup.ts index 2addd33ec..6766bb97d 100644 --- a/src/shared/git-branch-cleanup.ts +++ b/src/shared/git-branch-cleanup.ts @@ -8,6 +8,10 @@ export type GitBranchCleanupExec = ( const SQUASH_PATCH_SCAN_LIMIT = 200 +function isLocalTargetRef(ref: string): boolean { + return ref === 'HEAD' || ref.startsWith('refs/heads/') || ref.startsWith('refs/tags/') +} + async function readOptionalGitStdout( runGit: GitBranchCleanupExec, argv: string[], @@ -253,3 +257,26 @@ export async function branchHasNoUnmergedChangesOnAnyTarget( return false } + +export async function branchHasNoUnmergedChangesWithLazyTargetRefresh( + runGit: GitBranchCleanupExec, + branchName: string, + targetRefs: string[], + capabilities: GitCapabilityCache +): Promise { + // Why: an unrefreshed remote-tracking ref may no longer represent the remote's branch contents. + const localTargetRefs = targetRefs.filter(isLocalTargetRef) + const refreshDependentTargetRefs = targetRefs.filter((targetRef) => !isLocalTargetRef(targetRef)) + if ( + await branchHasNoUnmergedChangesOnAnyTarget(runGit, branchName, localTargetRefs, capabilities) + ) { + return true + } + await refreshBranchCleanupTargetRefs(runGit, targetRefs) + return branchHasNoUnmergedChangesOnAnyTarget( + runGit, + branchName, + refreshDependentTargetRefs, + capabilities + ) +} diff --git a/tests/tools/benchmarks/worktree-deletion-branch-fixture.mjs b/tests/tools/benchmarks/worktree-deletion-branch-fixture.mjs new file mode 100644 index 000000000..c655e059b --- /dev/null +++ b/tests/tools/benchmarks/worktree-deletion-branch-fixture.mjs @@ -0,0 +1,69 @@ +import { spawnSync } from 'node:child_process' +import net from 'node:net' +import path from 'node:path' + +function runGit(args, cwd) { + const result = spawnSync('git', args, { cwd, encoding: 'utf8' }) + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed (${result.status})\n${result.stderr}`) + } + return result.stdout.trim() +} + +export function initializeBranchCleanupRemote(fixtureRoot, repoPath) { + const remotePath = path.join(fixtureRoot, 'remote.git') + runGit(['init', '--bare', remotePath], fixtureRoot) + runGit(['symbolic-ref', 'HEAD', 'refs/heads/main'], remotePath) + runGit(['remote', 'add', 'origin', remotePath], repoPath) + runGit(['push', 'origin', 'main'], repoPath) +} + +export function seedBranchCleanupRepro(repoPath, worktreePath) { + runGit( + ['commit', '--allow-empty', '-m', 'Trigger safe branch cleanup', '--no-gpg-sign'], + worktreePath + ) + const branch = runGit(['symbolic-ref', '--short', 'HEAD'], worktreePath) + runGit(['config', `branch.${branch}.base`, 'refs/remotes/origin/main'], repoPath) +} + +export async function startDelayedFetchServer(delayMs) { + const sockets = new Set() + const timers = new Set() + const server = net.createServer((socket) => { + sockets.add(socket) + const timer = setTimeout(() => { + timers.delete(timer) + socket.end('HTTP/1.1 503 Service Unavailable\r\nContent-Length: 0\r\n\r\n') + }, delayMs) + timers.add(timer) + socket.on('close', () => { + clearTimeout(timer) + timers.delete(timer) + sockets.delete(socket) + }) + socket.on('error', () => undefined) + }) + await new Promise((resolve, reject) => { + server.once('error', reject) + server.listen(0, '127.0.0.1', resolve) + }) + const address = server.address() + if (!address || typeof address === 'string') { + throw new Error('Delayed fetch server did not expose a TCP port') + } + return { + url: `http://127.0.0.1:${address.port}/remote.git`, + close: () => + new Promise((resolve) => { + for (const timer of timers) { + clearTimeout(timer) + } + timers.clear() + for (const socket of sockets) { + socket.destroy() + } + server.close(resolve) + }) + } +} diff --git a/tests/tools/benchmarks/worktree-deletion-dev-bench.mjs b/tests/tools/benchmarks/worktree-deletion-dev-bench.mjs index e00e5423e..325f02095 100644 --- a/tests/tools/benchmarks/worktree-deletion-dev-bench.mjs +++ b/tests/tools/benchmarks/worktree-deletion-dev-bench.mjs @@ -16,9 +16,11 @@ import path from 'node:path' // Why @playwright/test, not playwright: only the former is a declared devDependency; it re-exports // the same browser types, so the benchmark resolves without relying on a hoisted transitive install. import { chromium } from '@playwright/test' +import * as branchFixture from './worktree-deletion-branch-fixture.mjs' const DEFAULT_ITERATIONS = 3 const DEFAULT_HISTORY_FILES = 10_000 +const DEFAULT_FETCH_DELAY_MS = 1_500 const CDP_START_PORT = 9_700 const START_TIMEOUT_MS = 180_000 const IPC_TIMEOUT_MS = 90_000 @@ -29,6 +31,7 @@ function parseArgs(argv) { instances: [], iterations: DEFAULT_ITERATIONS, historyFiles: DEFAULT_HISTORY_FILES, + fetchDelayMs: DEFAULT_FETCH_DELAY_MS, keepFixture: false } for (let index = 2; index < argv.length; index += 1) { @@ -65,6 +68,8 @@ function parseArgs(argv) { options.iterations = readPositiveInteger(value, next()) } else if (value === '--history-files') { options.historyFiles = readPositiveInteger(value, next()) + } else if (value === '--fetch-delay-ms') { + options.fetchDelayMs = readPositiveInteger(value, next()) } else { throw new Error(`Unknown argument: ${value}`) } @@ -91,6 +96,7 @@ Options: --instance Dev checkout to launch; repeat for A/B comparison --iterations Deletions per instance (default: ${DEFAULT_ITERATIONS}) --history-files Files seeded in each worktree history (default: ${DEFAULT_HISTORY_FILES}) + --fetch-delay-ms Slow-remote delay for branch cleanup (default: ${DEFAULT_FETCH_DELAY_MS}) --keep-fixture Keep disposable profiles and repos for inspection`) } @@ -117,6 +123,7 @@ function createFixture(instanceLabel) { run('git', ['add', 'README.md'], repoPath) run('git', ['commit', '-m', 'Initialize benchmark fixture', '--no-gpg-sign'], repoPath) run('git', ['branch', '-m', 'main'], repoPath) + branchFixture.initializeBranchCleanupRemote(root, repoPath) return { root, repoPath, userDataPath } } @@ -406,6 +413,7 @@ async function verifyDeletion(page, worktree, historyPath, measurement) { async function runIteration(page, fixture, repoState, iteration, historyFiles) { const worktree = await createMeasuredWorktree(page, repoState.repoId, iteration) await assertWorktreeRowVisible(page, worktree.id) + branchFixture.seedBranchCleanupRepro(fixture.repoPath, worktree.path) const historyPath = seedTerminalHistory(fixture.userDataPath, worktree.id, historyFiles) const measurement = await measureDeletion(page, worktree.id, repoState.rootWorktreeId) await verifyDeletion(page, worktree, historyPath, measurement) @@ -467,14 +475,14 @@ async function benchmarkInstance(instanceConfig, index, options) { instanceConfig.repoRoot ) const fixture = createFixture(instanceConfig.label) - const port = await findAvailablePort(CDP_START_PORT + index) - const instance = launchDevInstance(instanceConfig, fixture, port) - let browser = null - console.log(`[${instanceConfig.label}] launching ${instanceConfig.repoRoot}`) + let delayedFetchServer, instance, browser try { - const connection = await connectToOrca(instance) - browser = connection.browser - const { page } = connection + delayedFetchServer = await branchFixture.startDelayedFetchServer(options.fetchDelayMs) + run('git', ['remote', 'set-url', 'origin', delayedFetchServer.url], fixture.repoPath) + const port = await findAvailablePort(CDP_START_PORT + index) + instance = launchDevInstance(instanceConfig, fixture, port) + const { browser: connectedBrowser, page } = await connectToOrca(instance) + browser = connectedBrowser const repoState = await addFixtureRepo(page, fixture.repoPath) const iterations = [] for (let iteration = 1; iteration <= options.iterations; iteration += 1) { @@ -500,6 +508,7 @@ async function benchmarkInstance(instanceConfig, index, options) { label: instanceConfig.label, repoRoot: instanceConfig.repoRoot, historyFiles: options.historyFiles, + fetchDelayMs: options.fetchDelayMs, iterations, summary: summarize(iterations.map((entry) => entry.totalMs)), ipcSummary: summarize(iterations.flatMap((entry) => entry.ipcLatencyMs.samples)), @@ -508,7 +517,10 @@ async function benchmarkInstance(instanceConfig, index, options) { } } finally { await browser?.close().catch(() => undefined) - await stopDevInstance(instance) + if (instance) { + await stopDevInstance(instance) + } + await delayedFetchServer?.close() if (!options.keepFixture) { await rm(fixture.root, { recursive: true,