diff --git a/src/main/git/worktree-create-preparation-real-git.test.ts b/src/main/git/worktree-create-preparation-real-git.test.ts index 63f8021a7ae..f28349f77ca 100644 --- a/src/main/git/worktree-create-preparation-real-git.test.ts +++ b/src/main/git/worktree-create-preparation-real-git.test.ts @@ -1,8 +1,10 @@ import { execFileSync } from 'node:child_process' +import { existsSync, watch, type FSWatcher } from 'node:fs' import { mkdir, mkdtemp, readFile, realpath, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' +import * as gitRunner from './runner' import { createWorktreePreparationLockReason, isWorktreeCreatePreparation, @@ -46,6 +48,60 @@ afterEach(async () => { }) describe('prepared worktree creation with real Git', () => { + it('removes partial checkout files and registration after materialization is aborted', async () => { + const { repoPath, root } = await createRepo() + await Promise.all( + Array.from({ length: 1000 }, (_, index) => + writeFile( + join(repoPath, `payload-${index.toString().padStart(4, '0')}.txt`), + 'payload'.repeat(128) + ) + ) + ) + git(repoPath, ['add', '.']) + git(repoPath, ['commit', '--quiet', '-m', 'materialization fixture']) + const preparationRoot = join(root, WORKTREE_CREATE_PREPARATION_DIRECTORY) + const preparedPath = join(preparationRoot, `${process.pid}-partial`) + await mkdir(preparationRoot, { recursive: true }) + const controller = new AbortController() + const original = gitRunner.gitExecFileAsync + let watcher: FSWatcher | undefined + let observedMaterialization = false + const calls: string[][] = [] + const spy = vi.spyOn(gitRunner, 'gitExecFileAsync').mockImplementation((args, options) => { + calls.push([...args]) + if (args.includes('reset')) { + watcher = watch(preparedPath, (_event, filename) => { + // Only the reset writes here, so an event without a filename is still materialization. + if (filename === null || filename.toString().startsWith('payload-')) { + observedMaterialization = true + watcher?.close() + controller.abort() + } + }) + } + return original(args, options) + }) + try { + await expect( + prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'main', + createWorktreePreparationLockReason('partial-test'), + { signal: controller.signal } + ) + ).rejects.toThrow() + expect(observedMaterialization).toBe(true) + expect(calls.some((args) => args[args.indexOf('worktree') + 1] === 'lock')).toBe(false) + expect(existsSync(preparedPath)).toBe(false) + expect(await listWorktrees(repoPath, { includeCreatePreparations: true })).toHaveLength(1) + } finally { + watcher?.close() + spy.mockRestore() + } + }) + it('cleans up when the create signal is canceled', async () => { const { repoPath, root } = await createRepo() const preparationRoot = join(root, WORKTREE_CREATE_PREPARATION_DIRECTORY) diff --git a/src/main/git/worktree-preparation-cancel-latency.bench.test.ts b/src/main/git/worktree-preparation-cancel-latency.bench.test.ts new file mode 100644 index 00000000000..e2750df839d --- /dev/null +++ b/src/main/git/worktree-preparation-cancel-latency.bench.test.ts @@ -0,0 +1,158 @@ +// Opt in: ORCA_WORKTREE_PREPARATION_CANCEL_BENCH=1 pnpm exec vitest run --config config/vitest.config.ts src/main/git/worktree-preparation-cancel-latency.bench.test.ts +// +// Measures the create-side cost of an obsolete preparation: the wall time of a fresh checkout +// (the next Create's critical path) while an evicted preparation's checkout is either left running +// (main before #18951) or aborted (after). Same code, same fixture; only the abort differs. +import { execFileSync } from 'node:child_process' +import { existsSync } from 'node:fs' +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { performance } from 'node:perf_hooks' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { + createWorktreePreparationLockReason, + WORKTREE_CREATE_PREPARATION_DIRECTORY +} from '../../shared/worktree/create-preparation' +import { + discardPreparedWorktree, + prepareWorktreeCreateCheckout +} from './worktree-create-preparation' + +const describeBench = process.env.ORCA_WORKTREE_PREPARATION_CANCEL_BENCH ? describe : describe.skip +const FILE_COUNT = Number(process.env.ORCA_WORKTREE_PREPARATION_CANCEL_BENCH_FILES ?? 6000) +const FILE_BYTES = 48 * 1024 +const TRIALS = Number(process.env.ORCA_WORKTREE_PREPARATION_CANCEL_BENCH_TRIALS ?? 5) +const OBSOLETE_COUNTS = [1, 3] +const RESULT_PATH = process.env.ORCA_WORKTREE_PREPARATION_CANCEL_BENCH_RESULT + +type Variant = 'running' | 'aborted' +type Sample = { variant: Variant; obsolete: number; freshCheckoutMs: number } + +let root = '' +let repoPath = '' +let preparationRoot = '' +let sequence = 0 + +function git(cwd: string, args: string[]): void { + execFileSync('git', args, { cwd, stdio: ['ignore', 'ignore', 'pipe'] }) +} + +function nextPreparedPath(label: string): string { + sequence += 1 + return join(preparationRoot, `${process.pid}-${label}-${sequence}`) +} + +function checkout(preparedPath: string, signal?: AbortSignal): Promise { + return prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'main', + createWorktreePreparationLockReason(`bench-${sequence}`), + signal ? { signal } : {} + ) +} + +async function runTrial(variant: Variant, obsolete: number): Promise { + const controllers = Array.from({ length: obsolete }, () => new AbortController()) + const obsoletePaths = controllers.map(() => nextPreparedPath('obsolete')) + const obsoleteWork = obsoletePaths.map((path, index) => + checkout(path, controllers[index].signal).catch(() => {}) + ) + if (variant === 'aborted') { + // Eviction aborts in the same turn the incoming preparation is armed, so abort before the + // fresh checkout starts. + controllers.forEach((controller) => controller.abort()) + } + const freshPath = nextPreparedPath('fresh') + const started = performance.now() + await checkout(freshPath) + const freshCheckoutMs = performance.now() - started + await Promise.all(obsoleteWork) + await Promise.all( + [...obsoletePaths, freshPath].map((path) => + discardPreparedWorktree(repoPath, path).catch(() => {}) + ) + ) + return { variant, obsolete, freshCheckoutMs } +} + +function median(values: number[]): number { + const sorted = [...values].sort((left, right) => left - right) + const middle = Math.floor(sorted.length / 2) + return sorted.length % 2 ? sorted[middle] : (sorted[middle - 1] + sorted[middle]) / 2 +} + +describeBench('obsolete preparation cancellation latency', () => { + beforeAll(async () => { + root = await mkdtemp(join(tmpdir(), 'orca-preparation-cancel-bench-')) + repoPath = join(root, 'repo') + preparationRoot = join(root, WORKTREE_CREATE_PREPARATION_DIRECTORY) + await mkdir(preparationRoot, { recursive: true }) + execFileSync('git', ['init', '--quiet', repoPath]) + git(repoPath, ['symbolic-ref', 'HEAD', 'refs/heads/main']) + git(repoPath, ['config', 'user.email', 'bench@example.com']) + git(repoPath, ['config', 'user.name', 'Bench']) + git(repoPath, ['config', 'core.autocrlf', 'false']) + // Unique content per file so the object store cannot dedupe the materialization work. + for (let batch = 0; batch < FILE_COUNT; batch += 500) { + await Promise.all( + Array.from({ length: Math.min(500, FILE_COUNT - batch) }, (_, offset) => { + const index = batch + offset + return writeFile( + join(repoPath, `payload-${index.toString().padStart(5, '0')}.txt`), + `${index}\n`.repeat(Math.ceil(FILE_BYTES / `${index}\n`.length)) + ) + }) + ) + } + git(repoPath, ['add', '.']) + git(repoPath, ['commit', '--quiet', '-m', 'bench fixture']) + }, 600_000) + + afterAll(async () => { + await rm(root, { recursive: true, force: true }) + }) + + it('reports fresh checkout wall time with obsolete checkouts running vs aborted', async () => { + // Warm the object store and page cache once so the first variant is not penalised. + const warm = nextPreparedPath('warm') + await checkout(warm) + await discardPreparedWorktree(repoPath, warm) + + const samples: Sample[] = [] + for (const obsolete of OBSOLETE_COUNTS) { + for (let trial = 0; trial < TRIALS; trial += 1) { + // Alternate order so drift in cache or thermal state does not favour one variant. + const order: Variant[] = trial % 2 ? ['aborted', 'running'] : ['running', 'aborted'] + for (const variant of order) { + samples.push(await runTrial(variant, obsolete)) + } + } + } + const summary = OBSOLETE_COUNTS.map((obsolete) => { + const pick = (variant: Variant): number[] => + samples + .filter((sample) => sample.variant === variant && sample.obsolete === obsolete) + .map((sample) => sample.freshCheckoutMs) + const running = median(pick('running')) + const aborted = median(pick('aborted')) + return { + obsolete, + trials: TRIALS, + freshCheckoutMedianMs: { obsoleteRunning: running, obsoleteAborted: aborted }, + speedup: running / aborted + } + }) + const report = JSON.stringify( + { fixture: { files: FILE_COUNT, bytesPerFile: FILE_BYTES }, samples, summary }, + null, + 2 + ) + console.log(report) + if (RESULT_PATH) { + await writeFile(RESULT_PATH, `${report}\n`) + } + expect(existsSync(preparationRoot)).toBe(true) + }, 900_000) +}) diff --git a/src/main/worktree-create-preparation-pool.ts b/src/main/worktree-create-preparation-pool.ts index 7539c6076e4..1581f404b20 100644 --- a/src/main/worktree-create-preparation-pool.ts +++ b/src/main/worktree-create-preparation-pool.ts @@ -38,6 +38,8 @@ export type PreparationEntry = { createdAt: number ready: Promise expiration: NodeJS.Timeout + controller: AbortController + checkoutStarted: boolean } export type StartPreparationArgs = { @@ -68,6 +70,9 @@ async function discardEntry(entry: PreparationEntry): Promise { // A failed checkout self-discards, but that self-discard is best-effort too, so it can strand the // registration for the same reason the discard here can. Enrol either way. await entry.ready.catch(() => {}) + if (!entry.checkoutStarted) { + return + } await discardPreparationWithRetry({ hostKey: preparationHostKey(entry.repoPathKey, entry.wslDistro), repoPath: entry.repoPath, @@ -86,6 +91,7 @@ function expireEntry(entry: PreparationEntry): void { return } preparations.delete(entry.key) + entry.controller.abort() discardEntryInBackground(entry) } @@ -118,6 +124,7 @@ function enforcePreparationLimit( } preparations.delete(victim.key) clearTimeout(victim.expiration) + victim.controller.abort() discardEntryInBackground(victim) } } @@ -163,6 +170,10 @@ export function startPreparation({ WORKTREE_CREATE_PREPARATION_DIRECTORY ) const preparedPath = pathOps(workspaceRoot).join(preparationRoot, preparationId) + const controller = new AbortController() + const signal = options.signal + ? AbortSignal.any([options.signal, controller.signal]) + : controller.signal const entry = {} as PreparationEntry const expiration = setTimeout(() => expireEntry(entry), WORKTREE_CREATE_PREPARATION_TTL_MS) expiration.unref() @@ -179,17 +190,19 @@ export function startPreparation({ options, createdAt: Date.now(), expiration, + controller, + checkoutStarted: false, ready: (async () => { await cleanupStalePreparations(preparationHostKey(repoPathKey, wslDistro), repoPath, options) + signal.throwIfAborted() await mkdir(toHostFilesystemPath(preparationRoot), { recursive: true }) + signal.throwIfAborted() // Already canonical, so the add re-resolves nothing. - await prepareWorktreeCreateCheckout( - repoPath, - preparedPath, - canonicalBase, - lockReason, - options - ) + entry.checkoutStarted = true + await prepareWorktreeCreateCheckout(repoPath, preparedPath, canonicalBase, lockReason, { + ...options, + signal + }) })() } satisfies PreparationEntry) preparations.set(key, entry) diff --git a/src/main/worktree-create-preparation.test.ts b/src/main/worktree-create-preparation.test.ts index 06818fec422..f6f7e295497 100644 --- a/src/main/worktree-create-preparation.test.ts +++ b/src/main/worktree-create-preparation.test.ts @@ -1,5 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as WorktreeLogic from './ipc/worktree-logic' import type { Store } from './persistence' +import { WORKTREE_CREATE_PREPARATION_TTL_MS } from './worktree-create-preparation-pool' import type { Repo } from '../shared/repo-types' import { WORKTREE_CREATE_PREPARATION_DIRECTORY } from '../shared/worktree/create-preparation' import { resolveWorktreeAddBaseRef } from '../shared/worktree/base-ref' @@ -36,7 +38,8 @@ vi.mock('./project-runtime-git-options', () => ({ getLocalProjectWorktreeGitOptions: mocks.getWorktreeOptions, getWorktreeMirrorDistro: () => undefined })) -vi.mock('./ipc/worktree-logic', () => ({ +vi.mock('./ipc/worktree-logic', async (importOriginal) => ({ + isOrphanedWorktreeError: (await importOriginal()).isOrphanedWorktreeError, computeWorkspaceRoot: mocks.computeWorkspaceRoot, computeWorkspaceRootAsync: mocks.computeWorkspaceRootAsync, getWorktreePathSettings: () => ({ @@ -96,6 +99,163 @@ afterEach(async () => { }) describe('worktree create preparation registry', () => { + it('cancels an evicted checkout and cleans up with the original options', async () => { + let signal: AbortSignal | undefined + mocks.prepareCheckout.mockImplementationOnce((_repo, _path, _base, _lock, options) => { + signal = options.signal + return new Promise((_resolve, reject) => { + signal!.addEventListener('abort', () => reject(signal!.reason), { once: true }) + }) + }) + const obsolete = prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const settled = Promise.allSettled([obsolete]) + await flushBackgroundWork() + const obsoletePath = mocks.prepareCheckout.mock.calls[0][1] + for (const base of ['origin/one', 'origin/two', 'origin/three']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + expect(signal?.aborted).toBe(true) + expect((await settled)[0].status).toBe('rejected') + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith(repo.path, obsoletePath, {}) + }) + + it('does not retry a discard whose registration the aborted checkout already removed', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + mocks.prepareCheckout.mockImplementationOnce((_repo, _path, _base, _lock, options) => { + const signal = options.signal! + return new Promise((_resolve, reject) => { + signal.addEventListener('abort', () => reject(signal.reason), { once: true }) + }) + }) + try { + const obsolete = prepareWorktreeCreateForRepo(store, repo, 'origin/main').catch(() => {}) + await flushBackgroundWork() + const obsoletePath = mocks.prepareCheckout.mock.calls[0][1] as string + mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { + if (path === obsoletePath) { + throw Object.assign(new Error(`fatal: '${path}' is not a working tree`), { + stderr: `fatal: '${path}' is not a working tree` + }) + } + }) + for (const base of ['origin/one', 'origin/two', 'origin/three']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + await obsolete + await flushBackgroundWork() + const obsoleteDiscards = (): number => + mocks.discard.mock.calls.filter((call) => call[1] === obsoletePath).length + expect(obsoleteDiscards()).toBe(1) + + for (const base of ['origin/four', 'origin/five']) { + await prepareWorktreeCreateForRepo(store, repo, base) + await flushBackgroundWork() + } + expect(obsoleteDiscards()).toBe(1) + expect(warn).not.toHaveBeenCalled() + } finally { + warn.mockRestore() + } + }) + + it('does not start obsolete checkout work after shared cleanup finishes', async () => { + let releaseCleanup!: () => void + mocks.listWorktreeGraph.mockImplementationOnce( + () => + new Promise<[]>((resolve) => { + releaseCleanup = () => resolve([]) + }) + ) + const requests = ['main', 'one', 'two', 'three'].map((base) => + prepareWorktreeCreateForRepo(store, repo, `origin/${base}`) + ) + const settled = Promise.allSettled(requests) + await flushBackgroundWork() + expect(mocks.prepareCheckout).not.toHaveBeenCalled() + releaseCleanup() + const results = await settled + expect(results.map((result) => result.status)).toEqual([ + 'rejected', + 'fulfilled', + 'fulfilled', + 'fulfilled' + ]) + expect(mocks.prepareCheckout).toHaveBeenCalledTimes(3) + await flushBackgroundWork() + expect(mocks.discard).not.toHaveBeenCalled() + }) + + it('keeps a claimed in-flight checkout alive when new preparations fill the pool', async () => { + let signal: AbortSignal | undefined + let finishCheckout!: () => void + mocks.prepareCheckout.mockImplementationOnce((_repo, _path, _base, _lock, options) => { + signal = options.signal + return new Promise((resolve) => { + finishCheckout = resolve + }) + }) + const preparation = prepareWorktreeCreateForRepo(store, repo, 'origin/main') + await flushBackgroundWork() + const create = consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot: '/workspace', + worktreePath: '/workspace/claimed', + branch: 'claimed', + baseBranch: 'origin/main' + }) + await flushBackgroundWork() + for (const base of ['origin/one', 'origin/two', 'origin/three', 'origin/four']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + expect(signal?.aborted).toBe(false) + finishCheckout() + await preparation + expect(await create).toMatchObject({ status: 'hit' }) + }) + + it('cancels an expired in-flight checkout', async () => { + vi.useFakeTimers() + let signal: AbortSignal | undefined + mocks.prepareCheckout.mockImplementationOnce((_repo, _path, _base, _lock, options) => { + signal = options.signal + return new Promise((_resolve, reject) => { + signal!.addEventListener('abort', () => reject(signal!.reason), { once: true }) + }) + }) + try { + const settled = Promise.allSettled([prepareWorktreeCreateForRepo(store, repo, 'origin/main')]) + await vi.advanceTimersByTimeAsync(0) + expect(signal?.aborted).toBe(false) + await vi.advanceTimersByTimeAsync(WORKTREE_CREATE_PREPARATION_TTL_MS) + expect(signal?.aborted).toBe(true) + expect((await settled)[0].status).toBe('rejected') + } finally { + vi.useRealTimers() + } + }) + + it('preserves caller cancellation without mutating its options', async () => { + const controller = new AbortController() + const options = { signal: controller.signal } + mocks.getWorktreeOptions.mockReturnValue(options) + let signal: AbortSignal | undefined + mocks.prepareCheckout.mockImplementationOnce((_repo, _path, _base, _lock, executionOptions) => { + signal = executionOptions.signal + return new Promise((_resolve, reject) => { + signal!.addEventListener('abort', () => reject(signal!.reason), { once: true }) + }) + }) + const preparation = prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const settled = Promise.allSettled([preparation]) + await flushBackgroundWork() + controller.abort() + expect(signal?.aborted).toBe(true) + expect((await settled)[0].status).toBe('rejected') + expect(options.signal).toBe(controller.signal) + expect(signal).not.toBe(controller.signal) + }) + it('starts the checkout only once the async workspace root resolves', async () => { let resolveRoot!: (root: string) => void mocks.computeWorkspaceRootAsync.mockReturnValue( @@ -364,7 +524,7 @@ describe('worktree create preparation registry', () => { expect.any(String), 'refs/remotes/origin/main', expect.any(String), - options + { ...options, signal: expect.any(AbortSignal) } ) expect(mocks.finalize).toHaveBeenCalledWith( repo.path, diff --git a/src/main/worktree-preparation-discard-retry.ts b/src/main/worktree-preparation-discard-retry.ts index e18f890084c..8602bebb39a 100644 --- a/src/main/worktree-preparation-discard-retry.ts +++ b/src/main/worktree-preparation-discard-retry.ts @@ -1,5 +1,6 @@ import type { AddWorktreeOptions } from './git/worktree' import { discardPreparedWorktree } from './git/worktree-create-preparation' +import { isOrphanedWorktreeError } from './ipc/worktree-logic' // Stale cleanup only reclaims preparations whose owner pid is dead, so a discard that fails inside // the live process would strand its scratch checkout until the app restarts. Remember the failure @@ -31,6 +32,11 @@ async function runDiscard(target: PreparationDiscardTarget, attempts: number): P try { await discardPreparedWorktree(target.repoPath, target.preparedPath, target.options) } catch (error) { + // An aborted or failed checkout self-discards first, so the registration is usually already + // gone by the time the pool discards; retrying that would only spawn Git to fail again. + if (isOrphanedWorktreeError(error)) { + return + } // Bounded: a path that never becomes removable must not tax every later preparation. if (attempts >= PREPARATION_DISCARD_ATTEMPT_LIMIT) { console.warn(