From e2270fe94d1699207a2cf2a3f80bd5acf6c49e52 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 5 Sep 2026 22:16:57 -0700 Subject: [PATCH] Stop evicted and expired worktree preparations (#18951) * Stop obsolete worktree preparations when evicted or expired * fix(worktree): skip discard retries for registrations an aborted checkout already removed An evicted or expired preparation now aborts its checkout, which self-discards the registration before the pool's own discard runs. That second discard failed with "is not a working tree" and was enrolled for up to three retries on later preparations for the same host, spawning Git only to fail again and warning that the path stays registered when it was already gone. Also accept fs.watch events without a filename in the abort real-Git test, and add an opt-in bench (ORCA_WORKTREE_PREPARATION_CANCEL_BENCH=1) that measures a fresh checkout's wall time with obsolete checkouts left running versus aborted. --- ...rktree-create-preparation-real-git.test.ts | 58 ++++++- ...e-preparation-cancel-latency.bench.test.ts | 158 +++++++++++++++++ src/main/worktree-create-preparation-pool.ts | 27 ++- src/main/worktree-create-preparation.test.ts | 164 +++++++++++++++++- .../worktree-preparation-discard-retry.ts | 6 + 5 files changed, 403 insertions(+), 10 deletions(-) create mode 100644 src/main/git/worktree-preparation-cancel-latency.bench.test.ts 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(