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.
This commit is contained in:
Neil
2026-09-05 22:16:57 -07:00
committed by GitHub
parent bf5f3c2ec4
commit e2270fe94d
5 changed files with 403 additions and 10 deletions
@@ -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)
@@ -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<void> {
return prepareWorktreeCreateCheckout(
repoPath,
preparedPath,
'main',
createWorktreePreparationLockReason(`bench-${sequence}`),
signal ? { signal } : {}
)
}
async function runTrial(variant: Variant, obsolete: number): Promise<Sample> {
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)
})
+20 -7
View File
@@ -38,6 +38,8 @@ export type PreparationEntry = {
createdAt: number
ready: Promise<void>
expiration: NodeJS.Timeout
controller: AbortController
checkoutStarted: boolean
}
export type StartPreparationArgs = {
@@ -68,6 +70,9 @@ async function discardEntry(entry: PreparationEntry): Promise<void> {
// 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)
+162 -2
View File
@@ -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<typeof WorktreeLogic>()).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<void>((_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<void>((_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<void>((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<void>((_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<void>((_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,
@@ -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(