diff --git a/docs/reference/git-compatibility.md b/docs/reference/git-compatibility.md index d537c4d94de..bbfb4a84876 100644 --- a/docs/reference/git-compatibility.md +++ b/docs/reference/git-compatibility.md @@ -37,11 +37,21 @@ authority. | --------------------------- | ----------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `fetch-no-write-fetch-head` | Fetch a private rebase ref without changing worktree-local `FETCH_HEAD` | Serialize all Orca fetch/pull operations per worktree Git directory before Git 2.29 | | `worktree-list-z` | NUL-delimited worktree paths with `prunable` marks | Line-block parser for Git before `worktree list -z` (2.36); the `prunable`/`locked` annotations still parse on Git 2.31–2.35, and a path-existence probe restores `prunable` detection for Git before 2.31 | +| `worktree-add-lock-reason` | Create a prepared checkout with its ownership marker already present | Before Git 2.33, add without checkout, then exclusively create the same reason marker before materializing files | | `rev-parse-path-format` | Absolute repo metadata paths | Resolve legacy relative output against the scanned repo | | `for-each-ref-exclude` | Exclude remote HEAD before the output limit | Request extra refs, then filter remote HEAD in Orca | | `merge-tree-write-tree` | Derive real-merge conflicts and no-op tree proofs | Omit the conflict summary and keep conservative branch cleanup behavior before Git 2.38 | | `merge-tree-merge-base` | Supply the already-resolved merge base | Use the older two-commit `merge-tree --write-tree` form | +Prepared creation registers and locks without checking out files while its shared +exact-base fetch runs. A cancellable in-process barrier waits for fetch settlement, +including offline failure, then resolves the current commit OID on the owning host +and materializes files once. Existing preparations queue a tip refresh on that same +barrier before a create can claim them. Finalization still resolves the latest base +and runs the post-checkout hook only when attaching the requested branch. This uses +baseline-compatible `rev-parse` and `reset --hard`; the barrier never enters Git +transport options or the remote wire. + ### Placeholders That Fail Open `GitCapabilityCache` records commands Git _rejects_. A `git log --format` @@ -75,6 +85,10 @@ container start, so their wall clock is runner contention, not Git. Build the running alongside them is charged to whichever boundary case is in flight and surfaces as a Vitest timeout rather than as a slow setup step. +The idle maintenance contract also verifies `multi-pack-index write` and packed +object reads. The command arrived in Git 2.20 and needs no newer-Git fallback; +Orca uses only index metadata writes, respecting `core.multiPackIndex=false`. + Keep the unit tests alongside that matrix. They cover concurrent probes, native/WSL/SSH/relay isolation, and error-stream shapes that a single real binary invocation cannot exercise deterministically. diff --git a/src/main/__mocks__/worktree-create-preparation.ts b/src/main/__mocks__/worktree-create-preparation.ts new file mode 100644 index 00000000000..74e48a3f8c9 --- /dev/null +++ b/src/main/__mocks__/worktree-create-preparation.ts @@ -0,0 +1,114 @@ +import { afterEach, beforeEach, vi, type Mock } from 'vitest' +import type * as WorktreeLogic from '../ipc/worktree-logic' +import type { Store } from '../persistence' +import type { Repo } from '../../shared/repo-types' +import { resolveWorktreeAddBaseRef } from '../../shared/worktree/base-ref' + +const mocks: { + mkdir: Mock + listWorktreeGraph: Mock + prepareCheckout: Mock + refreshTip: Mock + finalize: Mock + discard: Mock + unlock: Mock + getWorktreeOptions: Mock + computeWorkspaceRoot: Mock + computeWorkspaceRootAsync: Mock + resolveBaseRef: Mock + measureDivergence: Mock +} = vi.hoisted(() => ({ + mkdir: vi.fn(), + listWorktreeGraph: vi.fn(), + prepareCheckout: vi.fn(), + refreshTip: vi.fn(), + finalize: vi.fn(), + discard: vi.fn(), + unlock: vi.fn(), + getWorktreeOptions: vi.fn(), + computeWorkspaceRoot: vi.fn(), + computeWorkspaceRootAsync: vi.fn(), + resolveBaseRef: vi.fn(), + measureDivergence: vi.fn() +})) + +export { mocks } + +vi.mock('node:fs/promises', () => ({ mkdir: mocks.mkdir })) +vi.mock('../git/worktree', () => ({ listWorktreeGraph: mocks.listWorktreeGraph })) +vi.mock('../git/worktree-create-preparation', () => ({ + prepareWorktreeCreateCheckout: mocks.prepareCheckout, + finalizePreparedWorktree: mocks.finalize, + discardPreparedWorktree: mocks.discard, + unlockPreparedWorktree: mocks.unlock +})) +vi.mock('../git/worktree-preparation-tip-refresh', () => ({ + refreshPreparedWorktreeTip: mocks.refreshTip +})) +vi.mock('../git/worktree-base-ref-probe', () => ({ + resolveLocalWorktreeBaseRef: mocks.resolveBaseRef +})) +vi.mock('../git/worktree-base-divergence', () => ({ + measureRetargetDivergence: mocks.measureDivergence +})) +vi.mock('../project-runtime-git-options', () => ({ + getLocalProjectWorktreeGitOptions: mocks.getWorktreeOptions, + getWorktreeMirrorDistro: () => undefined +})) +vi.mock('../ipc/worktree-logic', async (importOriginal) => ({ + isOrphanedWorktreeError: (await importOriginal()).isOrphanedWorktreeError, + computeWorkspaceRoot: mocks.computeWorkspaceRoot, + computeWorkspaceRootAsync: mocks.computeWorkspaceRootAsync, + getWorktreePathSettings: () => ({ + workspaceDir: process.platform === 'win32' ? 'C:\\workspace' : '/workspace', + nestWorkspaces: false + }) +})) + +import { _resetWorktreeCreatePreparationsForTests } from '../worktree-create-preparation' + +// Evictions and retries are fire-and-forget, so let them settle before asserting. +export function flushBackgroundWork(ms = 0): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + +const EXISTING_REFS = new Set([ + 'refs/heads/main', + 'refs/remotes/origin/main', + 'refs/remotes/origin/release' +]) +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The mocked flow reads only this repository identity. +export const repo = { id: 'repo-1', path: '/repo' } as Repo +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The mocked persistence boundary reads only getSettings. +export const store = { getSettings: () => ({}) } as unknown as Store + +beforeEach(() => { + mocks.mkdir.mockReset().mockResolvedValue(undefined) + mocks.listWorktreeGraph.mockReset().mockResolvedValue([]) + mocks.prepareCheckout.mockReset().mockResolvedValue(undefined) + mocks.refreshTip.mockReset().mockResolvedValue(undefined) + mocks.finalize.mockReset().mockResolvedValue({}) + mocks.discard.mockReset().mockResolvedValue(undefined) + mocks.unlock.mockReset().mockResolvedValue(undefined) + mocks.getWorktreeOptions.mockReset().mockReturnValue({}) + mocks.measureDivergence.mockReset().mockResolvedValue('within') + mocks.resolveBaseRef + .mockReset() + .mockImplementation((_repoPath: string, baseRef: string) => + resolveWorktreeAddBaseRef(baseRef, async (candidate) => EXISTING_REFS.has(candidate)) + ) + mocks.computeWorkspaceRoot.mockReset().mockImplementation(() => { + throw new Error('synchronous workspace-root lookup must not run on the main thread') + }) + mocks.computeWorkspaceRootAsync + .mockReset() + .mockImplementation(async (repoPath: string) => + process.platform === 'win32' && /^[A-Za-z]:[\\/]/.test(repoPath) + ? 'C:\\workspace' + : '/workspace' + ) +}) + +afterEach(async () => { + await _resetWorktreeCreatePreparationsForTests() +}) diff --git a/src/main/git/local-repo-ref-maintenance.test.ts b/src/main/git/local-repo-ref-maintenance.test.ts index f6a411733c3..7d084a4bd5a 100644 --- a/src/main/git/local-repo-ref-maintenance.test.ts +++ b/src/main/git/local-repo-ref-maintenance.test.ts @@ -77,7 +77,12 @@ describe('local repo ref maintenance target', () => { it('reads either Git auto-maintenance opt-out, and unset keys as consent', async () => { for (const stdout of [ 'maintenance.auto false\n', + 'maintenance.auto NO\n', + 'maintenance.auto off\n', + 'maintenance.auto 0\n', + 'maintenance.auto \n', 'gc.auto 0\n', + 'gc.auto 0k\n', 'gc.auto 6700\nmaintenance.auto false\n' ]) { gitExecFileAsyncMock.mockResolvedValue({ stdout, stderr: '' }) @@ -91,10 +96,20 @@ describe('local repo ref maintenance target', () => { await expect(target().isOptedOut?.(NO_ABORT)).resolves.toBe(false) // `git config --get-regexp` exits non-zero when nothing matches. - gitExecFileAsyncMock.mockRejectedValue(new Error('exit 1')) + gitExecFileAsyncMock.mockRejectedValue(Object.assign(new Error('key unset'), { code: 1 })) await expect(target().isOptedOut?.(NO_ABORT)).resolves.toBe(false) }) + it('fails closed when the auto-maintenance config cannot be read', async () => { + for (const error of [ + new Error('spawn failed'), + Object.assign(new Error('bad config'), { code: 128 }) + ]) { + gitExecFileAsyncMock.mockRejectedValue(error) + await expect(target().isOptedOut?.(NO_ABORT)).resolves.toBe(true) + } + }) + it('walks the POSIX refs directory for a native repo', async () => { readRepoCommonDirFromGitMock.mockResolvedValue('/repo/.git') diff --git a/src/main/git/local-repo-ref-maintenance.ts b/src/main/git/local-repo-ref-maintenance.ts index e84f7d1ae30..6c0836b58b1 100644 --- a/src/main/git/local-repo-ref-maintenance.ts +++ b/src/main/git/local-repo-ref-maintenance.ts @@ -12,6 +12,11 @@ import { import { isWslUncPath, toWindowsWslPath } from '../../shared/wsl-paths' import { withSpan } from '../observability/tracer' import { PackRefsLockOwnership } from './pack-refs-lock-ownership' +import { + clearRepoPackIndexMaintenanceCache, + isUnsetGitConfigError, + maintainRepoPackIndex +} from './repo-pack-index-maintenance' import { gitExecFileAsync } from './runner' import { readRepoCommonDirFromGit } from './worktree-list-reader' @@ -96,6 +101,7 @@ export function disposeLocalRepoRefMaintenance(): Promise { shared?.dispose() shared = null repoBusyProbes.clear() + clearRepoPackIndexMaintenanceCache() return settling } @@ -149,6 +155,7 @@ export function _resetLocalRepoRefMaintenanceForTests( shared = overrides ? new RepoRefMaintenance({ ...localMaintenanceOptions(), ...overrides }) : null activityProbe = null repoBusyProbes.clear() + clearRepoPackIndexMaintenanceCache() } /** @@ -174,8 +181,11 @@ function refsDirectoryForMainProcess(commonDir: string, wslDistro: string | unde export function isGitAutoMaintenanceDisabled(configOutput: string): boolean { return configOutput .split('\n') - .map((line) => line.trim()) - .some((line) => line === 'maintenance.auto false' || line === 'gc.auto 0') + .some( + (line) => + /^\s*maintenance\.auto\s+(?:false|no|off|0)?\s*$/i.test(line) || + /^\s*gc\.auto\s+[+-]?0+(?:[kmg])?\s*$/i.test(line) + ) } /** @@ -237,11 +247,25 @@ export function createLocalRepoRefMaintenanceTarget( { cwd: args.repoPath, ...gitOptions, admissionTier: 'background', signal } ) return isGitAutoMaintenanceDisabled(stdout) - } catch { - // Neither key set is the common case and exits non-zero; that is consent. - return false + } catch (error) { + // An unset key is consent; unreadable or invalid config must fail closed. + return !isUnsetGitConfigError(error) } }, + async maintainPackIndex(signal, span, canWrite) { + const resolved = await resolveCommonDir(signal) + if (resolved) { + return maintainRepoPackIndex({ + repoPath: args.repoPath, + commonDir: gitCommonDirForMainProcess(resolved, args.wslDistro), + ...gitOptions, + signal, + span, + canWrite + }) + } + return 'failed' + }, async packRefs(lock: PackedRefsLockReporter) { const resolved = await resolveCommonDir() const owner = resolved diff --git a/src/main/git/repo-pack-index-maintenance.test.ts b/src/main/git/repo-pack-index-maintenance.test.ts new file mode 100644 index 00000000000..1fee9644f03 --- /dev/null +++ b/src/main/git/repo-pack-index-maintenance.test.ts @@ -0,0 +1,258 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock, opendirMock, statMock, openMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + opendirMock: vi.fn(), + statMock: vi.fn(), + openMock: vi.fn() +})) + +vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +vi.mock('node:fs/promises', () => ({ opendir: opendirMock, stat: statMock, open: openMock })) + +import { + maintainRepoPackIndex, + clearRepoPackIndexMaintenanceCache, + PACK_INDEX_FORCE_REFRESH_MS, + PACK_INDEX_THRESHOLD, + PACK_INDEX_TIMEOUT_MS +} from './repo-pack-index-maintenance' +import { PACK_INDEX_PROBE_ENTRY_LIMIT } from './repo-pack-index-state' + +function directory(packs: number) { + return { + async *[Symbol.asyncIterator]() { + for (let index = 0; index < packs; index += 1) { + yield { name: `pack-${index}.idx`, isFile: () => true, isSymbolicLink: () => false } + yield { name: `pack-${index}.pack`, isFile: () => true, isSymbolicLink: () => false } + } + } + } +} + +function args(wslDistro?: string) { + const attributes: Record = {} + return { + repoPath: wslDistro ? '//wsl$/Ubuntu/repo' : '/repo', + commonDir: wslDistro ? String.raw`\\wsl.localhost\Ubuntu\repo\.git` : '/repo/.git', + ...(wslDistro ? { wslDistro } : {}), + signal: new AbortController().signal, + canWrite: () => true, + span: { + setAttribute: (key: string, value: unknown) => { + attributes[key] = value + } + }, + attributes + } +} + +beforeEach(() => { + vi.resetAllMocks() + clearRepoPackIndexMaintenanceCache() + gitExecFileAsyncMock.mockImplementation(async (argv: string[]) => { + if (argv[0] === 'config') { + throw Object.assign(new Error('unset'), { code: 1 }) + } + return { stdout: '', stderr: '' } + }) + opendirMock.mockResolvedValue(directory(PACK_INDEX_THRESHOLD)) + statMock.mockResolvedValue({ dev: 1n, ino: 2n, mtimeNs: 3n, ctimeNs: 4n }) + openMock.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })) +}) + +describe('idle pack index maintenance', () => { + it('leaves a healthy repository alone', async () => { + const options = args() + opendirMock.mockResolvedValue(directory(PACK_INDEX_THRESHOLD - 1)) + await maintainRepoPackIndex(options) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(options.attributes['git.pack_index_outcome']).toBe('below_threshold') + }) + + it('writes only the lookup index with background admission and a deadline', async () => { + const options = args() + await maintainRepoPackIndex(options) + expect(gitExecFileAsyncMock).toHaveBeenLastCalledWith(['multi-pack-index', 'write'], { + cwd: '/repo', + admissionTier: 'background', + timeout: PACK_INDEX_TIMEOUT_MS + }) + expect(options.attributes['git.pack_index_outcome']).toBe('written') + }) + + it('rechecks idle admission after probing and never aborts an admitted writer', async () => { + const options = args() + await maintainRepoPackIndex({ ...options, canWrite: () => false }) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(opendirMock).not.toHaveBeenCalled() + expect(options.attributes['git.pack_index_outcome']).toBe('deferred') + let idle = true + opendirMock.mockResolvedValueOnce({ + async *[Symbol.asyncIterator]() { + yield* directory(PACK_INDEX_THRESHOLD) + idle = false + } + }) + await expect(maintainRepoPackIndex({ ...args(), canWrite: () => idle })).resolves.toBe( + 'deferred' + ) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + await maintainRepoPackIndex(args()) + expect(gitExecFileAsyncMock.mock.lastCall?.[1]).not.toHaveProperty('signal') + }) + + it('caps a directory stream and skips writes when bitmap absence cannot be proved', async () => { + let produced = 0 + let closed = false + opendirMock.mockResolvedValue({ + async *[Symbol.asyncIterator]() { + try { + while (produced < 100_000) { + produced += 1 + yield { name: `pack-${produced}.pack`, isFile: () => true, isSymbolicLink: () => false } + } + } finally { + closed = true + } + } + }) + const options = args() + await maintainRepoPackIndex(options) + expect(produced).toBe(PACK_INDEX_PROBE_ENTRY_LIMIT) + expect(closed).toBe(true) + expect(options.attributes['git.pack_index_outcome']).toBe('protected') + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('inspects the measured 11,424-pack repository with four directory entries per pack', async () => { + let produced = 0 + opendirMock.mockResolvedValue({ + async *[Symbol.asyncIterator]() { + for (let index = 0; index < 11_424; index += 1) { + for (const suffix of ['pack', 'idx', 'rev', 'keep']) { + produced += 1 + yield { + name: `pack-${index}.${suffix}`, + isFile: () => true, + isSymbolicLink: () => false + } + } + } + } + }) + await expect(maintainRepoPackIndex(args())).resolves.toBe('written') + expect(produced).toBe(11_424 * 4) + expect(opendirMock).toHaveBeenCalledOnce() + }) + + it('honours an explicit multi-pack-index opt-out before walking objects', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'false\n', stderr: '' }) + await maintainRepoPackIndex(args()) + expect(opendirMock).not.toHaveBeenCalled() + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('fails closed on config errors other than an unset key', async () => { + gitExecFileAsyncMock.mockRejectedValue( + Object.assign(new Error('invalid config'), { code: 128 }) + ) + const options = args() + await maintainRepoPackIndex(options) + expect(opendirMock).not.toHaveBeenCalled() + expect(options.attributes['git.pack_index_outcome']).toBe('failed') + }) + + it('does not infer consent from an unreadable boolean or a missing Git binary', async () => { + for (const config of [ + () => Promise.resolve({ stdout: 'unexpected', stderr: '' }), + () => Promise.reject(Object.assign(new Error('missing Git'), { code: 'ENOENT' })) + ]) { + gitExecFileAsyncMock.mockImplementationOnce(config) + const options = args() + await maintainRepoPackIndex(options) + expect(options.attributes['git.pack_index_outcome']).toBe('failed') + } + expect(opendirMock).not.toHaveBeenCalled() + }) + + it('records index failures without preventing later ref maintenance', async () => { + gitExecFileAsyncMock + .mockRejectedValueOnce(Object.assign(new Error('unset'), { code: 1 })) + .mockRejectedValueOnce(new Error('index locked')) + const options = args() + await expect(maintainRepoPackIndex(options)).resolves.toBe('failed') + expect(options.attributes['git.pack_index_outcome']).toBe('failed') + }) + + it('skips repositories without pack files and cancelled attempts', async () => { + statMock.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })) + const options = args() + await maintainRepoPackIndex(options) + expect(options.attributes['git.pack_index_outcome']).toBe('below_threshold') + const abort = new AbortController() + abort.abort() + gitExecFileAsyncMock.mockClear() + await maintainRepoPackIndex({ ...args(), signal: abort.signal }) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('walks the WSL share and runs Git on that execution host', async () => { + const options = args('Ubuntu') + await maintainRepoPackIndex(options) + expect(opendirMock).toHaveBeenCalledWith( + String.raw`\\wsl.localhost\Ubuntu\repo\.git\objects\pack` + ) + expect(gitExecFileAsyncMock).toHaveBeenLastCalledWith( + ['multi-pack-index', 'write'], + expect.objectContaining({ cwd: '//wsl$/Ubuntu/repo', wslDistro: 'Ubuntu' }) + ) + }) + + it('skips unchanged directory stamps but refreshes changed packs and periodically rechecks', async () => { + const options = args() + await maintainRepoPackIndex(options) + await expect(maintainRepoPackIndex(options)).resolves.toBe('unchanged') + expect(opendirMock).toHaveBeenCalledOnce() + statMock.mockResolvedValue({ dev: 1n, ino: 2n, mtimeNs: 5n, ctimeNs: 6n }) + await expect(maintainRepoPackIndex(options)).resolves.toBe('written') + const now = Date.now() + vi.spyOn(Date, 'now').mockReturnValue(now + PACK_INDEX_FORCE_REFRESH_MS + 1) + await expect(maintainRepoPackIndex(options)).resolves.toBe('written') + vi.restoreAllMocks() + }) + + it('isolates directory stamps by the execution host', async () => { + const options = args() + await maintainRepoPackIndex(options) + await expect(maintainRepoPackIndex({ ...options, wslDistro: 'Ubuntu' })).resolves.toBe( + 'written' + ) + }) + + it('protects metadata discovered after the pack threshold during the final probe', async () => { + opendirMock.mockResolvedValueOnce({ + async *[Symbol.asyncIterator]() { + yield* directory(PACK_INDEX_THRESHOLD) + yield { name: 'multi-pack-index-old.bitmap', isFile: () => true } + } + }) + const options = args() + await expect(maintainRepoPackIndex(options)).resolves.toBe('protected') + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('does not start the writer when the final probe is cancelled', async () => { + const controller = new AbortController() + opendirMock.mockResolvedValueOnce({ + async *[Symbol.asyncIterator]() { + yield* directory(PACK_INDEX_THRESHOLD) + controller.abort() + } + }) + const options = { ...args(), signal: controller.signal } + await expect(maintainRepoPackIndex(options)).resolves.toBe('deferred') + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(options.attributes['git.pack_index_pack_count_floor']).toBe(PACK_INDEX_THRESHOLD) + }) +}) diff --git a/src/main/git/repo-pack-index-maintenance.ts b/src/main/git/repo-pack-index-maintenance.ts new file mode 100644 index 00000000000..2177f4d2166 --- /dev/null +++ b/src/main/git/repo-pack-index-maintenance.ts @@ -0,0 +1,122 @@ +import { posix, win32 } from 'node:path' +import { BoundedMap } from '../../shared/bounded-map' +import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' +import type { PackIndexMaintenanceOutcome } from '../../shared/repo-pack-index-maintenance-policy' +import type { RefMaintenanceSpan } from '../../shared/repo-ref-maintenance-policy' +import { probeRepoPackIndexDirectory, readRepoPackDirectoryStamp } from './repo-pack-index-state' +import { gitExecFileAsync } from './runner' + +// Git's default auto-GC pack limit is 50; defer indexing until fragmentation is clear. +export const PACK_INDEX_THRESHOLD = 64 +export const PACK_INDEX_TIMEOUT_MS = 60_000 +export const PACK_INDEX_FORCE_REFRESH_MS = 6 * 60 * 60_000 + +const indexedDirectories = new BoundedMap({ + maxEntries: 64 +}) + +export function clearRepoPackIndexMaintenanceCache(): void { + indexedDirectories.clear() +} + +export function isUnsetGitConfigError(error: unknown): boolean { + return typeof error === 'object' && error !== null && 'code' in error && error.code === 1 +} + +type PackIndexMaintenanceArgs = { + repoPath: string + /** Common directory in the spelling the main process can open. */ + commonDir: string + wslDistro?: string + signal: AbortSignal + span: RefMaintenanceSpan + canWrite: () => boolean +} + +export async function maintainRepoPackIndex( + args: PackIndexMaintenanceArgs +): Promise { + const { signal, span } = args + const outcome = (value: PackIndexMaintenanceOutcome): PackIndexMaintenanceOutcome => { + span.setAttribute('git.pack_index_outcome', value) + return value + } + if (signal.aborted) { + return outcome('deferred') + } + const gitOptions = { + cwd: args.repoPath, + ...(args.wslDistro ? { wslDistro: args.wslDistro } : {}), + admissionTier: 'background' as const, + signal + } + try { + try { + const { stdout } = await gitExecFileAsync( + ['config', '--bool', '--get', 'core.multiPackIndex'], + gitOptions + ) + const configured = stdout.trim() + if (configured === 'false') { + return outcome('opted_out') + } + if (configured !== 'true') { + throw new Error('Unrecognized core.multiPackIndex config') + } + } catch (error) { + if (!isUnsetGitConfigError(error)) { + throw error + } + } + if (signal.aborted) { + return outcome('deferred') + } + const paths = isWindowsAbsolutePathLike(args.commonDir) ? win32 : posix + const directory = paths.join(args.commonDir, 'objects', 'pack') + const stamp = await readRepoPackDirectoryStamp(directory) + if (!stamp) { + return outcome('below_threshold') + } + const key = `${args.wslDistro ? `wsl:${args.wslDistro}` : 'local'}::${args.commonDir}` + const previous = indexedDirectories.get(key) + if ( + previous?.stamp === stamp && + Date.now() - previous.writtenAt < PACK_INDEX_FORCE_REFRESH_MS + ) { + return outcome('unchanged') + } + if (signal.aborted || !args.canWrite()) { + return outcome('deferred') + } + const probe = await probeRepoPackIndexDirectory(directory, PACK_INDEX_THRESHOLD, signal) + span.setAttribute('git.pack_index_pack_count_floor', probe.packCountFloor) + if (signal.aborted || !args.canWrite()) { + return outcome('deferred') + } + if (probe.protected) { + return outcome('protected') + } + if (probe.packCountFloor < PACK_INDEX_THRESHOLD) { + return outcome('below_threshold') + } + const startedAt = Date.now() + // Git 2.20+: writes lookup metadata atomically without rewriting or deleting packs. + // Let the writer finish: force-killing it on Windows can strand its index lock. + await gitExecFileAsync(['multi-pack-index', 'write'], { + cwd: args.repoPath, + ...(args.wslDistro ? { wslDistro: args.wslDistro } : {}), + admissionTier: 'background', + timeout: PACK_INDEX_TIMEOUT_MS + }) + const writtenStamp = await readRepoPackDirectoryStamp(directory).catch(() => undefined) + // A racing new pack remains readable; the forced refresh bounds a missed directory change. + if (writtenStamp) { + indexedDirectories.set(key, { stamp: writtenStamp, writtenAt: Date.now() }) + } + span.setAttribute('git.pack_index_write_ms', Date.now() - startedAt) + return outcome('written') + } catch (error) { + span.setAttribute('git.pack_index_error', String(error)) + return outcome('failed') + } +} diff --git a/src/main/git/repo-pack-index-state.test.ts b/src/main/git/repo-pack-index-state.test.ts new file mode 100644 index 00000000000..86689877351 --- /dev/null +++ b/src/main/git/repo-pack-index-state.test.ts @@ -0,0 +1,63 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { probeRepoPackIndexDirectory } from './repo-pack-index-state' + +const roots: string[] = [] +async function directory(): Promise { + const root = await mkdtemp(join(tmpdir(), 'orca-pack-index-state-')) + roots.push(root) + return root +} + +function indexHeader(version = 1, hash = 1): Buffer { + const value = Buffer.alloc(1200) + value.write('MIDX') + value[4] = version + value[5] = hash + value[6] = 4 + return value +} + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +describe('pack-index metadata protection', () => { + it('protects every retained bitmap regardless of its checksum', async () => { + const packs = await directory() + await writeFile(join(packs, 'multi-pack-index'), indexHeader()) + await writeFile(join(packs, 'multi-pack-index-old.bitmap'), 'retained bitmap') + await expect( + probeRepoPackIndexDirectory(packs, 64, new AbortController().signal) + ).resolves.toMatchObject({ protected: true }) + }) + + it('protects an incremental chain without reading its layers', async () => { + const packs = await directory() + await mkdir(join(packs, 'multi-pack-index.d')) + await expect( + probeRepoPackIndexDirectory(packs, 64, new AbortController().signal) + ).resolves.toMatchObject({ protected: true }) + }) + + it.each([indexHeader(2), indexHeader(1, 3), Buffer.from('MIDX'), Buffer.alloc(1200)])( + 'fails closed on an unknown or truncated MIDX', + async (header) => { + const packs = await directory() + await writeFile(join(packs, 'multi-pack-index'), header) + await expect( + probeRepoPackIndexDirectory(packs, 64, new AbortController().signal) + ).resolves.toMatchObject({ protected: true }) + } + ) + + it.each([1, 2])('accepts the standalone v1 SHA hash format %s', async (hash) => { + const packs = await directory() + await writeFile(join(packs, 'multi-pack-index'), indexHeader(1, hash)) + await expect( + probeRepoPackIndexDirectory(packs, 64, new AbortController().signal) + ).resolves.toEqual({ protected: false, packCountFloor: 0 }) + }) +}) diff --git a/src/main/git/repo-pack-index-state.ts b/src/main/git/repo-pack-index-state.ts new file mode 100644 index 00000000000..92ba425d8fd --- /dev/null +++ b/src/main/git/repo-pack-index-state.ts @@ -0,0 +1,94 @@ +import { open, opendir, stat } from 'node:fs/promises' +import { posix, win32 } from 'node:path' +import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' + +export const PACK_INDEX_PROBE_ENTRY_LIMIT = 65_536 +export const PACK_INDEX_PROBE_TIMEOUT_MS = 1_000 + +export function isMissingPackIndexPath(error: unknown): boolean { + return typeof error === 'object' && error !== null && 'code' in error && error.code === 'ENOENT' +} + +export async function readRepoPackDirectoryStamp(directory: string): Promise { + try { + const value = await stat(directory, { bigint: true }) + return `${value.dev}:${value.ino}:${value.mtimeNs}:${value.ctimeNs}` + } catch (error) { + if (isMissingPackIndexPath(error)) { + return undefined + } + throw error + } +} + +async function hasUnsupportedPackIndex(directory: string): Promise { + const paths = isWindowsAbsolutePathLike(directory) ? win32 : posix + const handle = await open(paths.join(directory, 'multi-pack-index'), 'r').catch( + (error: unknown) => { + if (isMissingPackIndexPath(error)) { + return undefined + } + throw error + } + ) + if (!handle) { + return false + } + try { + const header = Buffer.alloc(12) + const { bytesRead } = await handle.read(header, 0, header.length, 0) + const hashBytes = header[5] === 1 ? 20 : header[5] === 2 ? 32 : 0 + const chunks = header[6] ?? 0 + const size = (await handle.stat()).size + if ( + bytesRead !== header.length || + header.subarray(0, 4).toString() !== 'MIDX' || + header[4] !== 1 || + !hashBytes || + chunks < 4 || + header[7] !== 0 || + size < 12 + (chunks + 1) * 12 + 1024 + hashBytes + ) { + return true + } + const trailer = Buffer.alloc(hashBytes) + return (await handle.read(trailer, 0, hashBytes, size - hashBytes)).bytesRead !== hashBytes + } finally { + await handle.close() + } +} + +/** A capped name walk protects every retained bitmap, including an older index's bitmap. */ +export async function probeRepoPackIndexDirectory( + directory: string, + packThreshold: number, + signal: AbortSignal +): Promise<{ packCountFloor: number; protected: boolean }> { + const entries = await opendir(directory) + const startedAt = Date.now() + let visited = 0 + let packCountFloor = 0 + for await (const entry of entries) { + if (signal.aborted) { + return { packCountFloor, protected: true } + } + if ( + entry.name === 'multi-pack-index.d' || + (entry.name.startsWith('multi-pack-index') && entry.name.endsWith('.bitmap')) || + (entry.name === 'multi-pack-index' && !entry.isFile()) + ) { + return { packCountFloor, protected: true } + } + if (entry.name.endsWith('.pack') && (entry.isFile() || entry.isSymbolicLink())) { + packCountFloor = Math.min(packThreshold, packCountFloor + 1) + } + visited += 1 + if ( + visited >= PACK_INDEX_PROBE_ENTRY_LIMIT || + Date.now() - startedAt >= PACK_INDEX_PROBE_TIMEOUT_MS + ) { + return { packCountFloor, protected: true } + } + } + return { packCountFloor, protected: await hasUnsupportedPackIndex(directory) } +} diff --git a/src/main/git/repo-ref-maintenance-real-git.test.ts b/src/main/git/repo-ref-maintenance-real-git.test.ts index 30c67b0365a..7c3041e6dfe 100644 --- a/src/main/git/repo-ref-maintenance-real-git.test.ts +++ b/src/main/git/repo-ref-maintenance-real-git.test.ts @@ -1,10 +1,11 @@ import { execFileSync } from 'node:child_process' -import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' import { countLooseRefs } from '../../shared/loose-ref-count' import { RepoRefMaintenance } from '../../shared/repo-ref-maintenance' +import { PACK_INDEX_MAINTENANCE_COOLDOWN_MS } from '../../shared/repo-pack-index-maintenance-policy' import { _resetLocalRepoRefMaintenanceForTests, createLocalRepoRefMaintenanceTarget, @@ -12,6 +13,7 @@ import { setRepoMaintenanceActivityProbe } from './local-repo-ref-maintenance' import { forceDeleteLocalBranch } from './worktree-branch-removal' +import { maintainRepoPackIndex, PACK_INDEX_THRESHOLD } from './repo-pack-index-maintenance' const roots: string[] = [] // Large enough that the deferral ladder (1x, 2x, 4x ... capped at 8x) outlasts @@ -19,14 +21,51 @@ const roots: string[] = [] const QUIET_MS = 25 const THRESHOLD = 20 -function git(cwd: string, args: string[]): string { +function git(cwd: string, args: string[], input?: string): string { return execFileSync('git', args, { cwd, encoding: 'utf8', + input, stdio: ['pipe', 'pipe', 'pipe'] }).trim() } +function hasWriteOption(option: string): boolean { + try { + git(process.cwd(), ['multi-pack-index', 'write', '-h']) + } catch (error) { + if (typeof error === 'object' && error !== null) { + return ( + ('stderr' in error && String(error.stderr).includes(option)) || + ('stdout' in error && String(error.stdout).includes(option)) + ) + } + } + return false +} + +async function createFragmentedPacks(repoPath: string): Promise { + const objects = join(repoPath, '.git', 'objects') + const blobs: string[] = [] + for (let index = 0; index < PACK_INDEX_THRESHOLD; index += 1) { + const blob = git(repoPath, ['hash-object', '-w', '--stdin'], `packed-${index}\n`) + blobs.push(blob) + git(repoPath, ['pack-objects', join(objects, 'pack', 'pack')], `${blob}\n`) + await rm(join(objects, blob.slice(0, 2), blob.slice(2))) + } + return blobs +} + +function maintainIndex(repoPath: string) { + return maintainRepoPackIndex({ + repoPath, + commonDir: join(repoPath, '.git'), + signal: new AbortController().signal, + span: { setAttribute: () => {} }, + canWrite: () => true + }) +} + /** A repo whose only loose-ref backlog is the one the test asks for. */ async function createRepo(looseRefs: number): Promise<{ repoPath: string; refsDir: string }> { const root = await mkdtemp(join(tmpdir(), 'orca-ref-maintenance-git-')) @@ -49,13 +88,17 @@ async function createRepo(looseRefs: number): Promise<{ repoPath: string; refsDi return { repoPath, refsDir: join(repoPath, '.git', 'refs') } } -function createMaintenance(onPackRefs: () => void = () => {}): { +function createMaintenance( + onPackRefs: () => void = () => {}, + now?: () => number +): { maintenance: RepoRefMaintenance arm: (repoPath: string) => void } { const maintenance = new RepoRefMaintenance({ quietPeriodMs: QUIET_MS, - looseRefThreshold: THRESHOLD + looseRefThreshold: THRESHOLD, + ...(now ? { now } : {}) }) return { maintenance, @@ -99,6 +142,117 @@ afterEach(async () => { }) describe('idle ref maintenance against real Git', () => { + it('indexes fragmented packs without rewriting objects or requiring loose-ref debt', async () => { + const { repoPath } = await createRepo(0) + git(repoPath, ['config', 'maintenance.auto', 'true']) + git(repoPath, ['config', 'gc.auto', '6700']) + const objects = join(repoPath, '.git', 'objects') + const packs = join(objects, 'pack') + const blobs = await createFragmentedPacks(repoPath) + const originalPacks = (await readdir(packs)).sort() + const lock = join(packs, 'multi-pack-index.lock') + await writeFile(lock, 'another writer') + const blocked = createMaintenance() + blocked.arm(repoPath) + await settle(blocked.maintenance) + blocked.maintenance.dispose() + await expect(readFile(lock, 'utf8')).resolves.toBe('another writer') + await expect(readFile(join(packs, 'multi-pack-index'))).rejects.toMatchObject({ + code: 'ENOENT' + }) + await rm(lock) + const { maintenance, arm } = createMaintenance() + arm(repoPath) + await settle(maintenance) + maintenance.dispose() + const index = await readFile(join(packs, 'multi-pack-index')) + expect(index.subarray(0, 4).toString()).toBe('MIDX') + expect((await readdir(packs)).filter((name) => name !== 'multi-pack-index').sort()).toEqual( + originalPacks + ) + expect(git(repoPath, ['multi-pack-index', 'verify'])).toBe('') + expect(git(repoPath, ['-c', 'core.multiPackIndex=true', 'cat-file', '-p', blobs[0]])).toBe( + 'packed-0' + ) + expect( + git(repoPath, [ + '-c', + 'core.multiPackIndex=true', + 'cat-file', + '-p', + blobs[PACK_INDEX_THRESHOLD - 1] + ]) + ).toBe(`packed-${PACK_INDEX_THRESHOLD - 1}`) + await expect(readFile(join(repoPath, '.git', 'packed-refs'))).rejects.toMatchObject({ + code: 'ENOENT' + }) + }) + + it('refreshes new packs during ref cooldown and keeps readers working between writes', async () => { + const { repoPath } = await createRepo(0) + const blobs = await createFragmentedPacks(repoPath) + const packs = join(repoPath, '.git', 'objects', 'pack') + let clock = 0 + const { maintenance, arm } = createMaintenance( + () => {}, + () => clock + ) + arm(repoPath) + await settle(maintenance) + expect((await readFile(join(packs, 'multi-pack-index'))).readUInt32BE(8)).toBe( + PACK_INDEX_THRESHOLD + ) + const added = git(repoPath, ['hash-object', '-w', '--stdin'], 'new fetched object\n') + git(repoPath, ['pack-objects', join(packs, 'pack')], `${added}\n`) + await rm(join(repoPath, '.git', 'objects', added.slice(0, 2), added.slice(2))) + expect(git(repoPath, ['cat-file', '-p', added])).toBe('new fetched object') + expect(git(repoPath, ['cat-file', '-p', blobs[0]])).toBe('packed-0') + clock = PACK_INDEX_MAINTENANCE_COOLDOWN_MS + 1 + arm(repoPath) + await settle(maintenance) + maintenance.dispose() + expect((await readFile(join(packs, 'multi-pack-index'))).readUInt32BE(8)).toBe( + PACK_INDEX_THRESHOLD + 1 + ) + expect(git(repoPath, ['multi-pack-index', 'verify'])).toBe('') + await expect(readFile(join(repoPath, '.git', 'packed-refs'))).rejects.toMatchObject({ + code: 'ENOENT' + }) + }) + + it.skipIf(!hasWriteOption('bitmap'))( + 'preserves a real MIDX bitmap and its index byte for byte', + async () => { + const { repoPath } = await createRepo(0) + await createFragmentedPacks(repoPath) + const packs = join(repoPath, '.git', 'objects', 'pack') + git(repoPath, ['pack-objects', '--revs', join(packs, 'pack')], 'HEAD\n') + git(repoPath, ['multi-pack-index', 'write', '--bitmap']) + const metadata = (await readdir(packs)).filter((name) => name.startsWith('multi-pack-index')) + expect(metadata.some((name) => name.endsWith('.bitmap'))).toBe(true) + const before = await Promise.all(metadata.map((name) => readFile(join(packs, name)))) + await expect(maintainIndex(repoPath)).resolves.toBe('protected') + const after = await Promise.all(metadata.map((name) => readFile(join(packs, name)))) + expect(after).toEqual(before) + } + ) + + it.skipIf(!hasWriteOption('incremental'))( + 'preserves a real incremental MIDX chain and its layers', + async () => { + const { repoPath } = await createRepo(0) + await createFragmentedPacks(repoPath) + const chain = join(repoPath, '.git', 'objects', 'pack', 'multi-pack-index.d') + git(repoPath, ['multi-pack-index', 'write', '--incremental']) + const metadata = (await readdir(chain)).sort() + expect(metadata).toContain('multi-pack-index-chain') + const before = await Promise.all(metadata.map((name) => readFile(join(chain, name)))) + await expect(maintainIndex(repoPath)).resolves.toBe('protected') + expect((await readdir(chain)).sort()).toEqual(metadata) + expect(await Promise.all(metadata.map((name) => readFile(join(chain, name))))).toEqual(before) + } + ) + it('packs a backlogged repository down to zero loose refs', async () => { const { repoPath, refsDir } = await createRepo(THRESHOLD + 30) const { maintenance, arm } = createMaintenance() diff --git a/src/main/git/worktree-create-admission-tier.test.ts b/src/main/git/worktree-create-admission-tier.test.ts index f84311f14f6..84bcce00237 100644 --- a/src/main/git/worktree-create-admission-tier.test.ts +++ b/src/main/git/worktree-create-admission-tier.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as WorktreePreparationLock from './worktree-preparation-lock' type GitExec = ( args: string[], @@ -8,6 +9,12 @@ type GitExec = ( const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +vi.mock('./worktree-preparation-lock', async (importOriginal) => ({ + ...(await importOriginal()), + verifyWorktreePreparationLock: vi.fn(async () => '/owned-lock'), + verifyWorktreePreparationLockAtPath: vi.fn(), + unlockWorktreePreparationAtPath: vi.fn() +})) import { addWorktree } from './worktree-add' import { listWorktreesSharedStrict } from './worktree-scan-cache' @@ -51,11 +58,18 @@ describe('worktree create admission tier', () => { }) it('runs the prepared-checkout finalize at the tier the caller asked for', async () => { - await finalizePreparedWorktree('/repo', '/prepared', '/repo-wt', 'feature', 'main', false, { - admissionTier: 'interactive' - }) + await finalizePreparedWorktree( + '/repo', + '/prepared', + '/repo-wt', + 'feature', + 'main', + false, + { admissionTier: 'interactive' }, + 'owner' + ) - for (const match of ['worktree move', 'checkout --no-track', 'worktree unlock']) { + for (const match of ['worktree move', 'checkout --no-track']) { const options = optionsForCommand(match) expect(options, match).toHaveLength(1) expect(options[0], match).toMatchObject({ admissionTier: 'interactive' }) diff --git a/src/main/git/worktree-create-git-executor-real-git.test.ts b/src/main/git/worktree-create-git-executor-real-git.test.ts index 4aa5322e7eb..02989fc995e 100644 --- a/src/main/git/worktree-create-git-executor-real-git.test.ts +++ b/src/main/git/worktree-create-git-executor-real-git.test.ts @@ -81,7 +81,10 @@ it('creates cold and prepared worktrees with real Git while status capacity is o prepared.preparedPath, join(root, 'warm'), 'warm', - 'main' + 'main', + false, + {}, + prepared.lockReason ) expect(await listWorktrees(repo)).toHaveLength(3) }) 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 08795fcab1f..7233cc681fc 100644 --- a/src/main/git/worktree-create-preparation-real-git.test.ts +++ b/src/main/git/worktree-create-preparation-real-git.test.ts @@ -55,6 +55,241 @@ afterEach(async () => { }) describe('prepared worktree creation with real Git', () => { + it('registers during fetch and materializes its settled tip only once before the final hook', async () => { + const { repoPath, root } = await createRepo() + const preparedPath = join(root, 'prepared-barrier') + const finalPath = join(root, 'final-barrier') + const base = 'refs/remotes/origin/main' + const reason = createWorktreePreparationLockReason('barrier-tip') + const originalHead = git(repoPath, ['rev-parse', 'HEAD']) + git(repoPath, ['update-ref', base, originalHead]) + const hooksPath = join(root, 'hooks') + await mkdir(hooksPath) + await writeFile( + join(hooksPath, 'post-checkout'), + '#!/bin/sh\nprintf \'%s\\n\' "$@" >> checkout-hook.txt\n', + { mode: 0o755 } + ) + git(repoPath, ['config', 'core.hooksPath', hooksPath]) + let release!: () => void + const barrier = new Promise((resolve) => { + release = resolve + }) + const spy = vi.spyOn(gitRunner, 'gitExecFileAsync') + const preparing = prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + base, + reason, + {}, + barrier + ) + await vi.waitFor(() => expect(existsSync(join(preparedPath, '.git'))).toBe(true)) + await expect(readFile(join(preparedPath, 'version.txt'))).rejects.toMatchObject({ + code: 'ENOENT' + }) + expect(spy.mock.calls.some(([args]) => args.includes('reset'))).toBe(false) + await writeFile(join(repoPath, 'version.txt'), 'fetched\n') + git(repoPath, ['commit', '--quiet', '-am', 'fetched tip']) + const fetchedHead = git(repoPath, ['rev-parse', 'HEAD']) + git(repoPath, ['update-ref', base, fetchedHead]) + release() + try { + await preparing + expect(git(preparedPath, ['rev-parse', 'HEAD'])).toBe(fetchedHead) + expect(await readFile(join(preparedPath, 'version.txt'), 'utf8')).toBe('fetched\n') + expect(existsSync(join(preparedPath, 'checkout-hook.txt'))).toBe(false) + await finalizePreparedWorktree( + repoPath, + preparedPath, + finalPath, + 'feature/barrier', + base, + false, + {}, + reason + ) + const resets = spy.mock.calls.filter(([args]) => args.includes('reset')) + expect(resets).toHaveLength(1) + expect(resets[0]?.[0].at(-1)).toBe(fetchedHead) + expect(await readFile(join(finalPath, 'checkout-hook.txt'), 'utf8')).toBe( + `${fetchedHead}\n${fetchedHead}\n1\n` + ) + expect(git(finalPath, ['symbolic-ref', '--short', 'HEAD'])).toBe('feature/barrier') + await rm(join(finalPath, 'checkout-hook.txt')) + expect(git(finalPath, ['status', '--porcelain'])).toBe('') + } finally { + release() + spy.mockRestore() + } + }) + + it('cancels a never-settling fetch barrier and cleans the registered checkout before it resolves', async () => { + const { repoPath, root } = await createRepo() + const preparedPath = join(root, 'canceled-barrier') + const reason = createWorktreePreparationLockReason('barrier-cancel') + const controller = new AbortController() + let release!: () => void + const barrier = new Promise((resolve) => { + release = resolve + }) + const spy = vi.spyOn(gitRunner, 'gitExecFileAsync') + const preparing = prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'main', + reason, + { signal: controller.signal }, + barrier + ) + const assertion = expect(preparing).rejects.toThrow('expired while fetching') + try { + await vi.waitFor(() => expect(existsSync(join(preparedPath, '.git'))).toBe(true)) + const lock = git(preparedPath, ['rev-parse', '--git-path', 'locked']) + await vi.waitFor(async () => expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`)) + controller.abort(new Error('expired while fetching')) + await assertion + expect(existsSync(preparedPath)).toBe(false) + expect(git(repoPath, ['worktree', 'list', '--porcelain'])).not.toContain(preparedPath) + release() + await Promise.resolve() + expect(spy.mock.calls.some(([args]) => args.includes('reset'))).toBe(false) + } finally { + release() + spy.mockRestore() + } + }) + + it('preserves a replacement owner after the fetch barrier before probing or materializing', async () => { + const { repoPath, root } = await createRepo() + const preparedPath = join(root, 'replaced-barrier') + const reason = createWorktreePreparationLockReason('barrier-replacement') + let release!: () => void + const barrier = new Promise((resolve) => { + release = resolve + }) + const preparing = prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'main', + reason, + {}, + barrier + ) + const assertion = expect(preparing).rejects.toThrow('lock owner changed') + await vi.waitFor(() => expect(existsSync(join(preparedPath, '.git'))).toBe(true)) + const lock = git(preparedPath, ['rev-parse', '--git-path', 'locked']) + await vi.waitFor(async () => expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`)) + await writeFile(lock, 'manual barrier owner\n') + await writeFile(join(preparedPath, 'version.txt'), 'manual content\n') + const spy = vi.spyOn(gitRunner, 'gitExecFileAsync') + try { + release() + await assertion + expect(spy).not.toHaveBeenCalled() + expect(await readFile(lock, 'utf8')).toBe('manual barrier owner\n') + expect(await readFile(join(preparedPath, 'version.txt'), 'utf8')).toBe('manual content\n') + } finally { + release() + spy.mockRestore() + } + }) + + it('reports an unrelated barrier failure and removes only its owned registration', async () => { + const { repoPath, root } = await createRepo() + const preparedPath = join(root, 'failed-barrier') + const failure = new Error('unexpected barrier failure') + const spy = vi.spyOn(gitRunner, 'gitExecFileAsync') + try { + await expect( + prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'main', + createWorktreePreparationLockReason('barrier-failure'), + {}, + Promise.reject(failure) + ) + ).rejects.toBe(failure) + expect(existsSync(preparedPath)).toBe(false) + expect(spy.mock.calls.some(([args]) => args.includes('reset'))).toBe(false) + expect(git(repoPath, ['worktree', 'list', '--porcelain'])).not.toContain(preparedPath) + } finally { + spy.mockRestore() + } + }) + + it('pins first materialization to the resolved fetched OID when the ref moves before reset', async () => { + const { repoPath, root } = await createRepo() + const preparedPath = join(root, 'moving-barrier-tip') + const originalHead = git(repoPath, ['rev-parse', 'HEAD']) + await writeFile(join(repoPath, 'version.txt'), 'newer\n') + git(repoPath, ['commit', '--quiet', '-am', 'later tip']) + const newerHead = git(repoPath, ['rev-parse', 'HEAD']) + git(repoPath, ['update-ref', 'refs/remotes/origin/main', originalHead]) + const run = gitRunner.gitExecFileAsync + const spy = vi.spyOn(gitRunner, 'gitExecFileAsync').mockImplementation((args, options) => { + if (args.includes('reset')) { + git(repoPath, ['update-ref', 'refs/remotes/origin/main', newerHead]) + } + return run(args, options) + }) + try { + await prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'refs/remotes/origin/main', + createWorktreePreparationLockReason('barrier-oid-race'), + {}, + Promise.resolve() + ) + expect(git(preparedPath, ['rev-parse', 'HEAD'])).toBe(originalHead) + expect(await readFile(join(preparedPath, 'version.txt'), 'utf8')).toBe('one\n') + expect(spy.mock.calls.find(([args]) => args.includes('reset'))?.[0].at(-1)).toBe(originalHead) + } finally { + spy.mockRestore() + } + }) + + it('rechecks the barrier marker after reading the fresh commit before the first reset', async () => { + const { repoPath, root } = await createRepo() + const preparedPath = join(root, 'replaced-after-tip-read') + const reason = createWorktreePreparationLockReason('barrier-tip-owner') + const run = gitRunner.gitExecFileAsync + let lock = '' + const spy = vi + .spyOn(gitRunner, 'gitExecFileAsync') + .mockImplementation(async (args, options) => { + const result = await run(args, options) + if (args[0] === 'rev-parse' && args.includes('refs/heads/main^{commit}')) { + lock = git(preparedPath, ['rev-parse', '--git-path', 'locked']) + await writeFile(lock, 'manual after tip read\n') + } + return result + }) + try { + await expect( + prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'refs/heads/main', + reason, + {}, + Promise.resolve() + ) + ).rejects.toThrow('lock owner changed') + expect( + spy.mock.calls.some(([args]) => args.includes('reset') || args.includes('remove')) + ).toBe(false) + expect(await readFile(lock, 'utf8')).toBe('manual after tip read\n') + await expect(readFile(join(preparedPath, 'version.txt'))).rejects.toMatchObject({ + code: 'ENOENT' + }) + } finally { + spy.mockRestore() + } + }) + it.each([false, true])( 'attaches the prepared HEAD and runs the hook (base advanced: %s)', async (advanceBase) => { @@ -70,19 +305,24 @@ describe('prepared worktree creation with real Git', () => { ) git(repoPath, ['config', 'core.hooksPath', hooksPath]) git(repoPath, ['config', 'branch.autoSetupMerge', 'always']) - await prepareWorktreeCreateCheckout( - repoPath, - preparedPath, - 'main', - createWorktreePreparationLockReason('attach-with-hook') - ) + const lockReason = createWorktreePreparationLockReason('attach-with-hook') + await prepareWorktreeCreateCheckout(repoPath, preparedPath, 'main', lockReason) expect(existsSync(join(preparedPath, 'checkout-hook.txt'))).toBe(false) if (advanceBase) { await writeFile(join(repoPath, 'version.txt'), 'advanced\n') git(repoPath, ['commit', '--quiet', '-am', 'advance base']) } const targetHead = git(repoPath, ['rev-parse', 'HEAD']) - await finalizePreparedWorktree(repoPath, preparedPath, finalPath, 'feature/attached', 'main') + await finalizePreparedWorktree( + repoPath, + preparedPath, + finalPath, + 'feature/attached', + 'main', + false, + {}, + lockReason + ) expect(git(finalPath, ['rev-parse', 'HEAD'])).toBe(targetHead) expect(git(finalPath, ['symbolic-ref', '--short', 'HEAD'])).toBe('feature/attached') @@ -104,12 +344,8 @@ describe('prepared worktree creation with real Git', () => { const { repoPath, root } = await createRepo() const preparedPath = join(root, 'prepared-race') const finalPath = join(root, 'final-race') - await prepareWorktreeCreateCheckout( - repoPath, - preparedPath, - 'main', - createWorktreePreparationLockReason('head-race') - ) + const lockReason = createWorktreePreparationLockReason('head-race') + await prepareWorktreeCreateCheckout(repoPath, preparedPath, 'main', lockReason) const expectedHead = git(repoPath, ['rev-parse', 'HEAD']) git(repoPath, ['checkout', '--quiet', '-b', 'other']) await writeFile(join(repoPath, 'version.txt'), 'other\n') @@ -125,7 +361,16 @@ describe('prepared worktree creation with real Git', () => { return original(args, options) }) try { - await finalizePreparedWorktree(repoPath, preparedPath, finalPath, 'feature/race', 'main') + await finalizePreparedWorktree( + repoPath, + preparedPath, + finalPath, + 'feature/race', + 'main', + false, + {}, + lockReason + ) } finally { spy.mockRestore() } @@ -147,15 +392,20 @@ describe('prepared worktree creation with real Git', () => { { mode: 0o755 } ) git(repoPath, ['config', 'core.hooksPath', hooksPath]) - await prepareWorktreeCreateCheckout( - repoPath, - preparedPath, - 'main', - createWorktreePreparationLockReason('hook-commit') - ) + const lockReason = createWorktreePreparationLockReason('hook-commit') + await prepareWorktreeCreateCheckout(repoPath, preparedPath, 'main', lockReason) const baseHead = git(repoPath, ['rev-parse', 'HEAD']) - await finalizePreparedWorktree(repoPath, preparedPath, finalPath, 'feature/hook-commit', 'main') + await finalizePreparedWorktree( + repoPath, + preparedPath, + finalPath, + 'feature/hook-commit', + 'main', + false, + {}, + lockReason + ) expect(git(finalPath, ['symbolic-ref', '--short', 'HEAD'])).toBe('feature/hook-commit') expect(git(finalPath, ['rev-parse', 'HEAD^'])).toBe(baseHead) @@ -172,15 +422,20 @@ describe('prepared worktree creation with real Git', () => { await mkdir(hooksPath) await writeFile(join(hooksPath, 'post-checkout'), '#!/bin/sh\nexit 1\n', { mode: 0o755 }) git(repoPath, ['config', 'core.hooksPath', hooksPath]) - await prepareWorktreeCreateCheckout( - repoPath, - preparedPath, - 'main', - createWorktreePreparationLockReason('hook-failure') - ) + const lockReason = createWorktreePreparationLockReason('hook-failure') + await prepareWorktreeCreateCheckout(repoPath, preparedPath, 'main', lockReason) await expect( - finalizePreparedWorktree(repoPath, preparedPath, finalPath, 'feature/hook-failure', 'main') + finalizePreparedWorktree( + repoPath, + preparedPath, + finalPath, + 'feature/hook-failure', + 'main', + false, + {}, + lockReason + ) ).rejects.toThrow() expect(existsSync(finalPath)).toBe(false) expect(git(repoPath, ['branch', '--list', 'feature/hook-failure'])).toBe('') @@ -202,7 +457,7 @@ describe('prepared worktree creation with real Git', () => { return original(args, options) }) try { - await expect(discardPreparedWorktree(repoPath, preparedPath)).rejects.toThrow( + await expect(discardPreparedWorktree(repoPath, preparedPath, {}, lockReason)).rejects.toThrow( 'injected removal launch failure' ) const remaining = await listWorktrees(repoPath, { includeCreatePreparations: true }) @@ -214,7 +469,7 @@ describe('prepared worktree creation with real Git', () => { expect(await readFile(join(preparedPath, 'version.txt'), 'utf8')).toBe('one\n') } finally { spy.mockRestore() - await discardPreparedWorktree(repoPath, preparedPath) + await discardPreparedWorktree(repoPath, preparedPath, {}, lockReason) } expect(existsSync(preparedPath)).toBe(false) }) @@ -265,7 +520,16 @@ describe('prepared worktree creation with real Git', () => { expect(entry).toBeDefined() takePreparation(entry) const finalPath = join(root, 'fresh-worktree') - await finalizePreparedWorktree(repoPath, entry.preparedPath, finalPath, 'fresh', 'main') + await finalizePreparedWorktree( + repoPath, + entry.preparedPath, + finalPath, + 'fresh', + 'main', + false, + {}, + entry.lockReason + ) expect(git(finalPath, ['status', '--porcelain'])).toBe('') expect(git(finalPath, ['symbolic-ref', '--short', 'HEAD'])).toBe('fresh') expect(await readFile(join(finalPath, 'version.txt'), 'utf8')).toBe('one\n') @@ -346,17 +610,13 @@ describe('prepared worktree creation with real Git', () => { const preparedPath = join(preparationRoot, `${process.pid}-canceled`) await mkdir(preparationRoot, { recursive: true }) - await prepareWorktreeCreateCheckout( - repoPath, - preparedPath, - 'main', - createWorktreePreparationLockReason('canceled-test') - ) + const lockReason = createWorktreePreparationLockReason('canceled-test') + await prepareWorktreeCreateCheckout(repoPath, preparedPath, 'main', lockReason) const controller = new AbortController() controller.abort() await expect( - discardPreparedWorktree(repoPath, preparedPath, { signal: controller.signal }) + discardPreparedWorktree(repoPath, preparedPath, { signal: controller.signal }, lockReason) ).resolves.toBeUndefined() expect(await listWorktrees(repoPath, { includeCreatePreparations: true })).toHaveLength(1) @@ -393,7 +653,16 @@ describe('prepared worktree creation with real Git', () => { ) expect(git(preparedPath, ['rev-parse', 'HEAD'])).not.toBe(localMainHead) - await finalizePreparedWorktree(repoPath, preparedPath, finalPath, 'feature/retargeted', 'main') + await finalizePreparedWorktree( + repoPath, + preparedPath, + finalPath, + 'feature/retargeted', + 'main', + false, + {}, + createWorktreePreparationLockReason('retarget-test') + ) expect(git(finalPath, ['rev-parse', 'HEAD'])).toBe(localMainHead) // A retarget that left stale files behind would be a wrong checkout, not just a slow one. @@ -452,7 +721,10 @@ describe('prepared worktree creation with real Git', () => { preparedPath, finalPath, 'feature/overlap', - 'refs/remotes/origin/main' + 'refs/remotes/origin/main', + false, + {}, + createWorktreePreparationLockReason('fetch-overlap') ) expect(git(finalPath, ['rev-parse', 'HEAD'])).toBe(refreshed) expect(await readFile(join(finalPath, 'version.txt'), 'utf8')).toBe('refreshed\n') @@ -497,7 +769,9 @@ describe('prepared worktree creation with real Git', () => { finalPath, 'feature/prepared', 'main', - false + false, + {}, + createWorktreePreparationLockReason('real-git-test') ) expect(git(finalPath, ['rev-parse', 'HEAD'])).toBe(latestHead) diff --git a/src/main/git/worktree-create-preparation-real-wsl.test.ts b/src/main/git/worktree-create-preparation-real-wsl.test.ts index 8ccaf3c23bf..50f46342502 100644 --- a/src/main/git/worktree-create-preparation-real-wsl.test.ts +++ b/src/main/git/worktree-create-preparation-real-wsl.test.ts @@ -4,10 +4,10 @@ import { expect, it } from 'vitest' import { createWorktreePreparationLockReason } from '../../shared/worktree/create-preparation' import { gitExecFileAsync } from './runner' import { - discardPreparedWorktree, finalizePreparedWorktree, prepareWorktreeCreateCheckout } from './worktree-create-preparation' +import { removeWorktree } from './worktree-removal' // Opt in on Windows with a running distro; all Git commands use the production WSL router. const wslDistro = process.env.ORCA_TEST_WSL_DISTRO @@ -34,13 +34,8 @@ it.skipIf(process.platform !== 'win32' || !wslDistro)( await writeFile(join(repoPath, 'version.txt'), 'one\n') await git(repoPath, ['add', 'version.txt']) await git(repoPath, ['commit', '--quiet', '-m', 'initial']) - await prepareWorktreeCreateCheckout( - repoPath, - preparedPath, - 'main', - createWorktreePreparationLockReason('real-wsl-test'), - options - ) + const lockReason = createWorktreePreparationLockReason('real-wsl-test') + await prepareWorktreeCreateCheckout(repoPath, preparedPath, 'main', lockReason, options) expect(await git(repoPath, ['worktree', 'list', '--porcelain'])).toContain( 'locked orca-create-preparation:v1:' ) @@ -55,7 +50,8 @@ it.skipIf(process.platform !== 'win32' || !wslDistro)( 'feature/routed', 'main', false, - options + options, + lockReason ) expect(await git(finalPath, ['rev-parse', 'HEAD'])).toBe(target) expect(await git(finalPath, ['symbolic-ref', '--short', 'HEAD'])).toBe('feature/routed') @@ -65,7 +61,7 @@ it.skipIf(process.platform !== 'win32' || !wslDistro)( 'refs/heads/main' ) expect(await git(repoPath, ['worktree', 'list', '--porcelain'])).not.toContain('locked ') - await discardPreparedWorktree(repoPath, finalPath, options) + await removeWorktree(repoPath, finalPath, true, options) expect( (await git(repoPath, ['worktree', 'list', '--porcelain'])).match(/^worktree /gm) ).toHaveLength(1) diff --git a/src/main/git/worktree-create-preparation.ts b/src/main/git/worktree-create-preparation.ts index 5a4f65314af..cb51e1ca8ae 100644 --- a/src/main/git/worktree-create-preparation.ts +++ b/src/main/git/worktree-create-preparation.ts @@ -1,7 +1,8 @@ import { windowsLongPathGitArgs } from '../../shared/windows-long-path-git-args' +import { waitForPromiseWithSignal } from '../../shared/abort-signal-reason' import { resolveWorktreeAddBaseRef } from '../../shared/worktree/base-ref' import type { AddWorktreeOptions, AddWorktreeResult, GitWorktreeExecOptions } from './worktree' -import { gitExecOptions, type GitExecOptionsForWorktree } from './worktree-operation-options' +import { gitExecOptions } from './worktree-operation-options' import { configurePushAutoSetupRemote, notifyPreparedWorktreeMutation, @@ -10,93 +11,77 @@ import { resolveWorktreeAddTimeoutMs, WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS } from './worktree' +import { + gitCleanupOptions, + performDiscardPreparedWorktree, + removeFailedFinalization +} from './worktree-preparation-discard' import { hasWorktreeBaseCommitRef } from './worktree-base-ref-probe' import { withRepoRefMaintenancePaused } from './local-repo-ref-maintenance' import { gitExecFileAsync } from './runner' import { runWithGitReadCacheInvalidation } from './status' import { invalidateWslLinkedWorktreeGitRouting } from './wsl-linked-worktree-git-routing' - -function gitCleanupOptions( - cwd: string, - options: GitWorktreeExecOptions -): GitExecOptionsForWorktree { - // Why: cancellation must not strand a partially moved worktree; cleanup is bounded separately. - return gitExecOptions(cwd, { ...options, signal: undefined }) -} - -async function performDiscardPreparedWorktree( - repoPath: string, - worktreePath: string, - options: GitWorktreeExecOptions -): Promise { - const cleanupGitOptions = { - ...gitCleanupOptions(repoPath, options), - timeout: options.timeout ?? WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS - } - try { - // Preserve the ownership lock if removal cannot start; Git 2.25 supports locked removal. - await gitExecFileAsync( - [ - ...windowsLongPathGitArgs(repoPath), - 'worktree', - 'remove', - '--force', - '--force', - worktreePath - ], - cleanupGitOptions - ) - } finally { - invalidateWslLinkedWorktreeGitRouting(worktreePath) - } -} +import { + unlockWorktreePreparation, + unlockWorktreePreparationAtPath, + verifyWorktreePreparationLock, + verifyWorktreePreparationLockAtPath, + WorktreePreparationLockOwnershipError +} from './worktree-preparation-lock' +import { addLockedWorktreePreparation } from './worktree-preparation-add' export async function prepareWorktreeCreateCheckout( repoPath: string, worktreePath: string, baseBranch: string, lockReason: string, - options: GitWorktreeExecOptions = {} + options: GitWorktreeExecOptions = {}, + beforeMaterialization?: Promise ): Promise { + // Observe early rejection while registration runs; awaiting still reports the original error. + void beforeMaterialization?.catch(() => {}) try { await withRepoRefMaintenancePaused('worktree-prepare', () => runWithGitReadCacheInvalidation(async () => { const effectiveBase = await resolveWorktreeAddBaseRef(baseBranch, (qualifiedRef) => hasWorktreeBaseCommitRef(repoPath, qualifiedRef, options) ) + let lockPath: string | undefined try { - await gitExecFileAsync( - [ - ...windowsLongPathGitArgs(repoPath), - 'worktree', - 'add', - '--detach', - '--no-checkout', - worktreePath, - effectiveBase - ], - { ...gitExecOptions(repoPath, options), timeout: resolveWorktreeAddTimeoutMs() } + lockPath = await addLockedWorktreePreparation( + repoPath, + worktreePath, + effectiveBase, + lockReason, + { + ...options, + timeout: resolveWorktreeAddTimeoutMs() + } ) - // The add just wrote the marker; drop any pre-create route before the reset routes Git. - invalidateWslLinkedWorktreeGitRouting(worktreePath) + await verifyWorktreePreparationLockAtPath(lockPath, lockReason, options.signal) + let materializationBase = effectiveBase + if (beforeMaterialization) { + await waitForPromiseWithSignal(beforeMaterialization, options.signal) + await verifyWorktreePreparationLockAtPath(lockPath, lockReason, options.signal) + const { stdout } = await gitExecFileAsync( + ['rev-parse', '--verify', `${effectiveBase}^{commit}`], + gitExecOptions(repoPath, options) + ) + materializationBase = stdout.trim() + await verifyWorktreePreparationLockAtPath(lockPath, lockReason, options.signal) + } // Why: reset materializes files without running user post-checkout hooks before submit. await gitExecFileAsync( - [...windowsLongPathGitArgs(worktreePath), 'reset', '--hard', effectiveBase], + [...windowsLongPathGitArgs(worktreePath), 'reset', '--hard', materializationBase], { ...gitExecOptions(worktreePath, options), timeout: resolveWorktreeAddTimeoutMs() } ) - await gitExecFileAsync( - [ - ...windowsLongPathGitArgs(repoPath), - 'worktree', - 'lock', - '--reason', - lockReason, - worktreePath - ], - { ...gitExecOptions(repoPath, options), timeout: resolveWorktreeAddTimeoutMs() } - ) + await verifyWorktreePreparationLockAtPath(lockPath, lockReason, options.signal) } catch (error) { - await performDiscardPreparedWorktree(repoPath, worktreePath, options).catch(() => {}) + if (lockPath !== undefined && !(error instanceof WorktreePreparationLockOwnershipError)) { + await performDiscardPreparedWorktree(repoPath, worktreePath, options, lockReason).catch( + () => {} + ) + } throw error } }) @@ -109,11 +94,12 @@ export async function prepareWorktreeCreateCheckout( export async function discardPreparedWorktree( repoPath: string, worktreePath: string, - options: GitWorktreeExecOptions = {} + options: GitWorktreeExecOptions = {}, + expectedLockReason: string ): Promise { try { await runWithGitReadCacheInvalidation(() => - performDiscardPreparedWorktree(repoPath, worktreePath, options) + performDiscardPreparedWorktree(repoPath, worktreePath, options, expectedLockReason) ) } finally { notifyPreparedWorktreeMutation(repoPath) @@ -123,52 +109,22 @@ export async function discardPreparedWorktree( export async function unlockPreparedWorktree( repoPath: string, worktreePath: string, - options: GitWorktreeExecOptions = {} + options: GitWorktreeExecOptions = {}, + expectedLockReason: string ): Promise { const cleanupGitOptions = { ...gitCleanupOptions(repoPath, options), timeout: options.timeout ?? WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS } try { - await runWithGitReadCacheInvalidation(() => - gitExecFileAsync( - [...windowsLongPathGitArgs(repoPath), 'worktree', 'unlock', worktreePath], - cleanupGitOptions - ) - ) + await runWithGitReadCacheInvalidation(async () => { + await unlockWorktreePreparation(worktreePath, expectedLockReason, cleanupGitOptions) + }) } finally { notifyPreparedWorktreeMutation(repoPath) } } -async function removeFailedFinalization( - repoPath: string, - cleanupPath: string, - branch: string, - moved: boolean, - options: GitWorktreeExecOptions -): Promise { - let branchAttached = false - if (moved) { - try { - const { stdout } = await gitExecFileAsync( - ['symbolic-ref', '--short', 'HEAD'], - gitCleanupOptions(cleanupPath, options) - ) - branchAttached = stdout.trim() === branch - } catch { - // Detached or no longer readable. - } - } - await performDiscardPreparedWorktree(repoPath, cleanupPath, options).catch(() => {}) - if (branchAttached) { - await gitExecFileAsync( - ['branch', '-D', '--', branch], - gitCleanupOptions(repoPath, options) - ).catch(() => {}) - } -} - export async function finalizePreparedWorktree( repoPath: string, preparedPath: string, @@ -176,7 +132,8 @@ export async function finalizePreparedWorktree( branch: string, baseBranch: string, refreshLocalBaseRef = false, - options: AddWorktreeOptions = {} + options: AddWorktreeOptions = {}, + expectedLockReason: string ): Promise { const finalizeGitOptions: AddWorktreeOptions = { ...options, @@ -184,6 +141,13 @@ export async function finalizePreparedWorktree( } try { return await runWithGitReadCacheInvalidation(async () => { + const lockPath = await verifyWorktreePreparationLock( + preparedPath, + expectedLockReason, + finalizeGitOptions + ) + const verifyOwnership = (): Promise => + verifyWorktreePreparationLockAtPath(lockPath, expectedLockReason, finalizeGitOptions.signal) const [targetResult, preparedResult] = await Promise.allSettled([ (async () => { const baseContext = await resolveWorktreeAddBaseContext( @@ -219,6 +183,7 @@ export async function finalizePreparedWorktree( } const preparedHeadOutput = preparedResult.value.stdout if (preparedHeadOutput.trim() !== targetHead) { + await verifyOwnership() await gitExecFileAsync( [...windowsLongPathGitArgs(preparedPath), 'reset', '--hard', targetHead], gitExecOptions(preparedPath, finalizeGitOptions) @@ -228,6 +193,7 @@ export async function finalizePreparedWorktree( let moved = false try { try { + await verifyOwnership() // Why: `-f -f` moves the locked preparation while preserving its lock reason (Git >=2.25). await gitExecFileAsync( [ @@ -247,6 +213,7 @@ export async function finalizePreparedWorktree( invalidateWslLinkedWorktreeGitRouting(preparedPath) invalidateWslLinkedWorktreeGitRouting(worktreePath) } + await verifyOwnership() await gitExecFileAsync( [ ...windowsLongPathGitArgs(worktreePath), @@ -258,6 +225,7 @@ export async function finalizePreparedWorktree( ], gitExecOptions(worktreePath, finalizeGitOptions) ) + await verifyOwnership() await persistWorktreeCreationBase( worktreePath, branch, @@ -265,18 +233,22 @@ export async function finalizePreparedWorktree( finalizeGitOptions ) await configurePushAutoSetupRemote(worktreePath, finalizeGitOptions) - await gitExecFileAsync( - [...windowsLongPathGitArgs(repoPath), 'worktree', 'unlock', worktreePath], - gitExecOptions(repoPath, finalizeGitOptions) + await unlockWorktreePreparationAtPath( + lockPath, + expectedLockReason, + finalizeGitOptions.signal ) } catch (error) { - await removeFailedFinalization( - repoPath, - moved ? worktreePath : preparedPath, - branch, - moved, - finalizeGitOptions - ) + if (!(error instanceof WorktreePreparationLockOwnershipError)) { + await removeFailedFinalization( + repoPath, + moved ? worktreePath : preparedPath, + branch, + moved, + finalizeGitOptions, + expectedLockReason + ) + } await baseContext.pendingLocalBaseRefRefresh throw error } diff --git a/src/main/git/worktree-mutation-route-invalidation.test.ts b/src/main/git/worktree-mutation-route-invalidation.test.ts index 785ed3d1391..48db9133114 100644 --- a/src/main/git/worktree-mutation-route-invalidation.test.ts +++ b/src/main/git/worktree-mutation-route-invalidation.test.ts @@ -1,6 +1,7 @@ // Worktree add/move/remove/rollback rewrite the `.git` marker the WSL Git route was derived from. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type * as WorktreeModule from './worktree' +import type * as WorktreePreparationLock from './worktree-preparation-lock' const { gitExecFileAsyncMock, @@ -38,6 +39,14 @@ vi.mock('./worktree-scan-cache', () => ({ listWorktrees: listWorktreesMock })) +vi.mock('./worktree-preparation-lock', async (importOriginal) => ({ + ...(await importOriginal()), + lockWorktreePreparation: vi.fn(async () => '/owned-lock'), + verifyWorktreePreparationLock: vi.fn(async () => '/owned-lock'), + verifyWorktreePreparationLockAtPath: vi.fn(), + unlockWorktreePreparationAtPath: vi.fn() +})) + import { addWorktree } from './worktree-add' import { discardPreparedWorktree, @@ -120,7 +129,7 @@ describe('worktree mutations invalidate the WSL linked-worktree Git route', () = it('drops the route after a prepared worktree is discarded', async () => { seedWslLinkedWorktreeGitRoutingForTests(LINKED) - await discardPreparedWorktree(REPO, LINKED) + await discardPreparedWorktree(REPO, LINKED, {}, 'owner') expect(hasCachedHostRoute(LINKED)).toBe(false) }) @@ -133,11 +142,41 @@ describe('worktree mutations invalidate the WSL linked-worktree Git route', () = expect(hasCachedHostRoute(PREPARED)).toBe(false) }) + it('reads and materializes the barrier tip on the preparation Git host', async () => { + const head = 'b'.repeat(40) + gitExecFileAsyncMock.mockResolvedValue({ stdout: `${head}\n`, stderr: '' }) + await prepareWorktreeCreateCheckout( + REPO, + PREPARED, + 'refs/remotes/origin/main', + 'owner', + { wslDistro: 'Ubuntu' }, + Promise.resolve() + ) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['rev-parse', '--verify', 'refs/remotes/origin/main^{commit}'], + { cwd: REPO, wslDistro: 'Ubuntu' } + ) + expect(gitExecFileAsyncMock).toHaveBeenLastCalledWith( + ['reset', '--hard', head], + expect.objectContaining({ cwd: PREPARED, wslDistro: 'Ubuntu' }) + ) + }) + it('drops both routes after the prepared checkout is moved into place', async () => { seedWslLinkedWorktreeGitRoutingForTests(PREPARED) seedWslLinkedWorktreeGitRoutingForTests(LINKED) - await finalizePreparedWorktree(REPO, PREPARED, LINKED, 'feature', 'origin/main') + await finalizePreparedWorktree( + REPO, + PREPARED, + LINKED, + 'feature', + 'origin/main', + false, + {}, + 'owner' + ) expect(hasCachedHostRoute(PREPARED)).toBe(false) expect(hasCachedHostRoute(LINKED)).toBe(false) @@ -153,7 +192,7 @@ describe('worktree mutations invalidate the WSL linked-worktree Git route', () = ) await expect( - finalizePreparedWorktree(REPO, PREPARED, LINKED, 'feature', 'origin/main') + finalizePreparedWorktree(REPO, PREPARED, LINKED, 'feature', 'origin/main', false, {}, 'owner') ).rejects.toThrow('destination exists') expect(hasCachedHostRoute(PREPARED)).toBe(false) diff --git a/src/main/git/worktree-preparation-add.test.ts b/src/main/git/worktree-preparation-add.test.ts new file mode 100644 index 00000000000..f96db71aebb --- /dev/null +++ b/src/main/git/worktree-preparation-add.test.ts @@ -0,0 +1,332 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import type * as FilePromises from 'node:fs/promises' +import type * as PreparationLock from './worktree-preparation-lock' +import { isUnsupportedWorktreeAddLockReasonError } from '../../shared/git-worktree-command-capabilities' + +const mocks = vi.hoisted(() => ({ + git: vi.fn(), + lstat: vi.fn(), + lock: vi.fn(), + verify: vi.fn(), + discard: vi.fn() +})) +vi.mock('./runner', () => ({ gitExecFileAsync: mocks.git })) +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + lstat: mocks.lstat +})) +vi.mock('./worktree-preparation-lock', async (importOriginal) => ({ + ...(await importOriginal()), + lockWorktreePreparation: mocks.lock, + verifyWorktreePreparationLock: mocks.verify +})) +vi.mock('./worktree-preparation-discard', () => ({ performDiscardPreparedWorktree: mocks.discard })) + +import { addLockedWorktreePreparation } from './worktree-preparation-add' +import { clearGitCapabilityStateForTests, getLocalGitCapabilityCache } from './git-capability-state' +import { WorktreePreparationLockOwnershipError } from './worktree-preparation-lock' +import { WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS } from './worktree-operation-options' +import { + resetWslLinkedWorktreeGitRoutingForTests, + seedWslLinkedWorktreeGitRoutingForTests +} from './wsl-linked-worktree-git-routing' + +const reason = 'orca-create-preparation:v1:123:atomic' +const unsupported = Object.assign(new Error("error: unknown option 'reason'"), { code: 129 }) +const prepare = (path = '/prepared', options = {}) => + addLockedWorktreePreparation('/repo', path, 'refs/heads/main', reason, options) + +beforeEach(() => { + clearGitCapabilityStateForTests() + resetWslLinkedWorktreeGitRoutingForTests() + mocks.git.mockReset().mockResolvedValue({ stdout: '', stderr: '' }) + mocks.lstat.mockReset().mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })) + mocks.lock.mockReset().mockResolvedValue('/fallback-lock') + mocks.verify.mockReset().mockResolvedValue('/atomic-lock') + mocks.discard.mockReset().mockResolvedValue(undefined) +}) + +it('asks Git to create the exact marker atomically and only verifies its ownership', async () => { + const controller = new AbortController() + const options = { signal: controller.signal, timeout: 6000, admissionTier: 'background' as const } + await expect(prepare('/prepared', options)).resolves.toBe('/atomic-lock') + expect(mocks.git).toHaveBeenCalledExactlyOnceWith( + [ + 'worktree', + 'add', + '--detach', + '--no-checkout', + '--lock', + '--reason', + reason, + '/prepared', + 'refs/heads/main' + ], + { cwd: '/repo', ...options } + ) + expect(mocks.verify).toHaveBeenCalledExactlyOnceWith('/prepared', reason, options) + expect(mocks.lock).not.toHaveBeenCalled() +}) + +it('falls back once on the old-Git reason rejection and caches the absence', async () => { + mocks.git.mockRejectedValueOnce(unsupported) + await expect(prepare()).resolves.toBe('/fallback-lock') + await expect(prepare('/second')).resolves.toBe('/fallback-lock') + expect(mocks.git.mock.calls.map(([args]) => args.includes('--reason'))).toEqual([ + true, + false, + false + ]) + expect(mocks.lock.mock.calls.map(([path]) => path)).toEqual(['/prepared', '/second']) + expect(mocks.verify).not.toHaveBeenCalled() + expect(mocks.discard).not.toHaveBeenCalled() +}) + +it.each([ + ['first', 'cancellation'], + ['cached', 'cancellation'], + ['first', 'path probe'], + ['cached', 'path probe'], + ['first', 'marker write'], + ['cached', 'marker write'] +])('cleans its successful %s fallback add after a %s failure', async (fallback, failureKind) => { + const controller = new AbortController() + const options = { + signal: controller.signal, + timeout: 180_000, + wslDistro: 'Ubuntu', + admissionTier: 'background' as const + } + if (fallback === 'cached') { + getLocalGitCapabilityCache(options).rememberUnsupported('worktree-add-lock-reason') + } else { + mocks.git.mockRejectedValueOnce(unsupported) + } + const failure = new Error(`${failureKind} failed after registration`) + mocks.lock.mockImplementationOnce(async () => { + if (failureKind === 'cancellation') { + controller.abort(failure) + } + throw failure + }) + await expect(prepare('/prepared', options)).rejects.toBe(failure) + expect(mocks.discard).toHaveBeenCalledExactlyOnceWith('/repo', '/prepared', { + ...options, + signal: undefined, + timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + }) + expect(mocks.lstat).toHaveBeenCalled() + expect(options.timeout).toBe(180_000) + expect(options.signal).toBe(controller.signal) +}) + +it('preserves the original lock failure when bounded fallback cleanup also fails', async () => { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + const failure = new Error('lock path unavailable') + mocks.lock.mockRejectedValueOnce(failure) + mocks.discard.mockRejectedValueOnce(new Error('cleanup unavailable')) + await expect(prepare()).rejects.toBe(failure) + expect(mocks.discard).toHaveBeenCalledOnce() +}) + +it.each(['first', 'cached'])( + 'preserves a pre-existing target after %s fallback locking fails', + async (fallback) => { + if (fallback === 'cached') { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + } else { + mocks.git.mockRejectedValueOnce(unsupported) + } + mocks.lstat.mockResolvedValue({}) + const failure = new Error('marker write denied') + mocks.lock.mockRejectedValueOnce(failure) + await expect(prepare()).rejects.toBe(failure) + expect(mocks.discard).not.toHaveBeenCalled() + } +) + +it.each(['first', 'cached'])( + 'preserves a competing marker during %s fallback locking', + async (fallback) => { + if (fallback === 'cached') { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + } else { + mocks.git.mockRejectedValueOnce(unsupported) + } + mocks.lock.mockRejectedValueOnce(new WorktreePreparationLockOwnershipError()) + await expect(prepare()).rejects.toThrow('lock owner changed') + expect(mocks.discard).not.toHaveBeenCalled() + } +) + +it.each(['first', 'cached'])( + 'preserves an incomplete or rejected %s fallback add', + async (fallback) => { + if (fallback === 'cached') { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + } else { + mocks.git.mockRejectedValueOnce(unsupported) + } + const failure = new Error('add did not complete') + mocks.git.mockRejectedValueOnce(failure) + await expect(prepare()).rejects.toBe(failure) + expect(mocks.lock).not.toHaveBeenCalled() + expect(mocks.discard).not.toHaveBeenCalled() + } +) + +it('fails closed before a cached fallback add if the target cannot be inspected', async () => { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + const failure = Object.assign(new Error('target unavailable'), { code: 'EACCES' }) + mocks.lstat.mockRejectedValueOnce(failure) + await expect(prepare()).rejects.toBe(failure) + expect(mocks.git).not.toHaveBeenCalled() + expect(mocks.lock).not.toHaveBeenCalled() + expect(mocks.discard).not.toHaveBeenCalled() +}) + +it('coalesces concurrent unsupported probes while creating each checkout separately', async () => { + let reject!: (error: unknown) => void + mocks.git.mockImplementationOnce( + () => + new Promise((_resolve, rejectProbe) => { + reject = rejectProbe + }) + ) + const first = prepare('/first') + await vi.waitFor(() => expect(mocks.git).toHaveBeenCalledTimes(1)) + const second = prepare('/second') + await Promise.resolve() + expect(mocks.git).toHaveBeenCalledTimes(1) + reject(unsupported) + await expect(Promise.all([first, second])).resolves.toEqual(['/fallback-lock', '/fallback-lock']) + expect(mocks.git.mock.calls.filter(([args]) => args.includes('--reason'))).toHaveLength(1) + expect(new Set(mocks.lock.mock.calls.map(([path]) => path))).toEqual( + new Set(['/first', '/second']) + ) +}) + +it('runs each concurrent supported add rather than sharing the first checkout result', async () => { + let resolve!: (value: { stdout: string }) => void + mocks.git.mockImplementationOnce( + () => + new Promise((resolveProbe) => { + resolve = resolveProbe + }) + ) + mocks.verify.mockImplementation(async (path: string) => `${path}/owned-lock`) + const first = prepare('/first') + await vi.waitFor(() => expect(mocks.git).toHaveBeenCalledTimes(1)) + const second = prepare('/second') + resolve({ stdout: '' }) + await expect(Promise.all([first, second])).resolves.toEqual([ + '/first/owned-lock', + '/second/owned-lock' + ]) + expect(mocks.git.mock.calls.every(([args]) => args.includes('--reason'))).toBe(true) + expect(mocks.git).toHaveBeenCalledTimes(2) +}) + +it('isolates native rejection from individual WSL distros', async () => { + mocks.git.mockRejectedValueOnce(unsupported) + await prepare() + await prepare('/ubuntu', { wslDistro: 'Ubuntu' }) + await prepare('/debian', { wslDistro: 'Debian' }) + await prepare('/native-again') + expect(mocks.git.mock.calls.map(([args]) => args.includes('--reason'))).toEqual([ + true, + false, + true, + true, + false + ]) +}) + +it('uses native cached rejection when WSL routing executes the host Git binary', async () => { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + const platform = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + try { + const repoPath = String.raw`C:\repo\linked` + seedWslLinkedWorktreeGitRoutingForTests(repoPath) + await addLockedWorktreePreparation(repoPath, String.raw`C:\prepared`, 'main', reason, { + wslDistro: 'Ubuntu' + }) + expect(mocks.git).toHaveBeenCalledTimes(1) + expect(mocks.git.mock.calls[0][0]).not.toContain('--reason') + expect( + getLocalGitCapabilityCache({ wslDistro: 'Ubuntu' }).shouldTry('worktree-add-lock-reason') + ).toBe(true) + } finally { + platform.mockRestore() + } +}) + +it('checks a guest directory through its execution distro rather than the Windows root', async () => { + const platform = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + try { + await prepare('/home/prepared', { wslDistro: 'Ubuntu' }) + expect(mocks.lstat).toHaveBeenCalledExactlyOnceWith( + String.raw`\\wsl.localhost\Ubuntu\home\prepared` + ) + } finally { + platform.mockRestore() + } +}) + +it('fails closed when the pre-existing target cannot be inspected', async () => { + const failure = Object.assign(new Error('path access denied'), { code: 'EACCES' }) + mocks.lstat.mockRejectedValueOnce(failure) + await expect(prepare()).rejects.toBe(failure) + expect(mocks.git).not.toHaveBeenCalled() + expect(mocks.discard).not.toHaveBeenCalled() +}) + +it('keeps a general add failure visible without poisoning the capability or claiming a marker', async () => { + const failure = new Error('permission denied') + mocks.git.mockRejectedValueOnce(failure) + await expect(prepare()).rejects.toBe(failure) + expect(mocks.lock).not.toHaveBeenCalled() + expect(getLocalGitCapabilityCache().shouldTry('worktree-add-lock-reason')).toBe(true) + expect(mocks.discard).toHaveBeenCalledExactlyOnceWith('/repo', '/prepared', {}, reason) + await prepare('/next') + expect(mocks.git.mock.calls.every(([args]) => args.includes('--reason'))).toBe(true) +}) + +it('never removes a pre-existing checkout after add fails', async () => { + mocks.lstat.mockResolvedValueOnce({}) + mocks.git.mockRejectedValueOnce(new Error('path already exists')) + await expect(prepare()).rejects.toThrow('path already exists') + expect(mocks.discard).not.toHaveBeenCalled() + expect(mocks.lock).not.toHaveBeenCalled() +}) + +it('does not rewrite or discard a generic or competing marker returned after add', async () => { + mocks.verify.mockRejectedValueOnce(new WorktreePreparationLockOwnershipError()) + await expect(prepare()).rejects.toThrow('lock owner changed') + expect(mocks.lock).not.toHaveBeenCalled() + expect(mocks.discard).not.toHaveBeenCalled() +}) + +it('attempts ownership-checked cleanup if cancellation interrupts the newly registered add', async () => { + const controller = new AbortController() + const failure = new Error('add canceled') + mocks.git.mockImplementationOnce(async () => { + controller.abort() + throw failure + }) + const options = { signal: controller.signal } + await expect(prepare('/prepared', options)).rejects.toBe(failure) + expect(mocks.discard).toHaveBeenCalledExactlyOnceWith('/repo', '/prepared', options, reason) + expect(mocks.lock).not.toHaveBeenCalled() +}) + +it.each([ + [new Error("unknown option 'reason'"), true], + [{ stderr: "error: unrecognized option '--reason'" }, true], + [{ stdout: 'invalid switch --reason' }, true], + [{ code: 129, stderr: "unknown option 'detach'" }, false], + [{ code: 129 }, false], + [new Error('permission denied while writing lock reason'), false] +])('recognizes only an unsupported lock-reason option: %j', (error, expected) => { + expect(isUnsupportedWorktreeAddLockReasonError(error)).toBe(expected) +}) diff --git a/src/main/git/worktree-preparation-add.ts b/src/main/git/worktree-preparation-add.ts new file mode 100644 index 00000000000..bcbeb4dae8f --- /dev/null +++ b/src/main/git/worktree-preparation-add.ts @@ -0,0 +1,119 @@ +import { lstat } from 'node:fs/promises' +import { isUnsupportedWorktreeAddLockReasonError } from '../../shared/git-worktree-command-capabilities' +import { resolveWorktreeHostPath } from '../../shared/git-metadata-path' +import { windowsLongPathGitArgs } from '../../shared/windows-long-path-git-args' +import { toHostFilesystemPath } from '../host-tree-removal' +import { withLocalGitCapabilityCacheForExecution } from './git-capability-state' +import { gitExecFileAsync } from './runner' +import { + getErrorCode, + gitExecOptions, + WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS, + type GitWorktreeExecOptions +} from './worktree-operation-options' +import { performDiscardPreparedWorktree } from './worktree-preparation-discard' +import { + lockWorktreePreparation, + verifyWorktreePreparationLock, + WorktreePreparationLockOwnershipError +} from './worktree-preparation-lock' +import { invalidateWslLinkedWorktreeGitRouting } from './wsl-linked-worktree-git-routing' + +async function isAbsent(worktreePath: string, options: GitWorktreeExecOptions): Promise { + const hostPath = resolveWorktreeHostPath(worktreePath, options) + if (!hostPath) { + throw new Error('The prepared worktree path is empty') + } + try { + await lstat(toHostFilesystemPath(hostPath)) + return false + } catch (error) { + if (getErrorCode(error) === 'ENOENT') { + return true + } + throw error + } +} + +export function addLockedWorktreePreparation( + repoPath: string, + worktreePath: string, + baseRef: string, + lockReason: string, + options: GitWorktreeExecOptions +): Promise { + const add = async (lockArgs: string[]): Promise => { + try { + await gitExecFileAsync( + [ + ...windowsLongPathGitArgs(repoPath), + 'worktree', + 'add', + '--detach', + '--no-checkout', + ...lockArgs, + worktreePath, + baseRef + ], + gitExecOptions(repoPath, options) + ) + } finally { + invalidateWslLinkedWorktreeGitRouting(worktreePath) + } + } + return withLocalGitCapabilityCacheForExecution( + { cwd: repoPath, wslDistro: options.wslDistro, signal: options.signal }, + async (capabilities) => { + const initiallyAbsent = await isAbsent(worktreePath, options) + return capabilities.runWithFallback( + 'worktree-add-lock-reason', + async () => { + let added = false + try { + await add(['--lock', '--reason', lockReason]) + added = true + return await verifyWorktreePreparationLock(worktreePath, lockReason, options) + } catch (error) { + // A canceled add can leave its marker; a pre-existing checkout is never ours to remove. + if ( + (added || initiallyAbsent) && + !isUnsupportedWorktreeAddLockReasonError(error) && + !(error instanceof WorktreePreparationLockOwnershipError) + ) { + await performDiscardPreparedWorktree( + repoPath, + worktreePath, + options, + lockReason + ).catch(() => {}) + } + throw error + } + }, + async () => { + let added = false + try { + await add([]) + added = true + return await lockWorktreePreparation(worktreePath, lockReason, options) + } catch (error) { + if ( + added && + initiallyAbsent && + !(error instanceof WorktreePreparationLockOwnershipError) + ) { + // Single force reclaims our unlocked add while preserving any competing lock. + await performDiscardPreparedWorktree(repoPath, worktreePath, { + ...options, + signal: undefined, + timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + }).catch(() => {}) + } + throw error + } + }, + isUnsupportedWorktreeAddLockReasonError + ) + } + ) +} diff --git a/src/main/git/worktree-preparation-base-oid.test.ts b/src/main/git/worktree-preparation-base-oid.test.ts index ecd6f4eab19..508707dbd9e 100644 --- a/src/main/git/worktree-preparation-base-oid.test.ts +++ b/src/main/git/worktree-preparation-base-oid.test.ts @@ -1,8 +1,15 @@ import { beforeEach, expect, it, vi } from 'vitest' import type * as WorktreeBaseRefresh from './worktree-base-refresh' +import type * as WorktreePreparationLock from './worktree-preparation-lock' const gitExec = vi.hoisted(() => vi.fn()) vi.mock('./runner', () => ({ gitExecFileAsync: gitExec })) +vi.mock('./worktree-preparation-lock', async (importOriginal) => ({ + ...(await importOriginal()), + verifyWorktreePreparationLock: vi.fn(async () => '/owned-lock'), + verifyWorktreePreparationLockAtPath: vi.fn(), + unlockWorktreePreparationAtPath: vi.fn() +})) vi.mock('./worktree-base-refresh', async (importOriginal) => ({ ...(await importOriginal()), refreshLocalBaseRefForWorktreeCreate: vi.fn(async () => undefined), @@ -30,10 +37,16 @@ beforeEach(() => { }) it('reuses the current base-resolution oid and preserves WSL routing', async () => { - await finalizePreparedWorktree('/repo', '/prepared', '/final', 'feature', 'main', false, { - wslDistro: 'Ubuntu', - timeout: 8000 - }) + await finalizePreparedWorktree( + '/repo', + '/prepared', + '/final', + 'feature', + 'main', + false, + { wslDistro: 'Ubuntu', timeout: 8000 }, + 'owner' + ) const revisions = gitExec.mock.calls.filter(([args]) => args[0] === 'rev-parse') expect(revisions.map(([args]) => args)).toEqual([ ['rev-parse', '--verify', '--quiet', 'refs/heads/main^{commit}'], @@ -58,7 +71,8 @@ it.each([ 'feature', test.base, test.refresh, - test.options + test.options, + 'owner' ) expect(gitExec).toHaveBeenCalledWith( ['rev-parse', '--verify', 'refs/heads/main^{commit}'], @@ -84,7 +98,16 @@ it('starts both independent probes before either resolves and settles them befor }) let settled = false const error = new Error('prepared HEAD unreadable') - const result = finalizePreparedWorktree('/repo', '/prepared', '/final', 'feature', 'main') + const result = finalizePreparedWorktree( + '/repo', + '/prepared', + '/final', + 'feature', + 'main', + false, + {}, + 'owner' + ) const checked = expect(result).rejects.toBe(error) void result.then( () => (settled = true), diff --git a/src/main/git/worktree-preparation-cancel-latency.bench.test.ts b/src/main/git/worktree-preparation-cancel-latency.bench.test.ts index e2750df839d..c5f30b4788b 100644 --- a/src/main/git/worktree-preparation-cancel-latency.bench.test.ts +++ b/src/main/git/worktree-preparation-cancel-latency.bench.test.ts @@ -43,16 +43,28 @@ function nextPreparedPath(label: string): string { return join(preparationRoot, `${process.pid}-${label}-${sequence}`) } +const lockReasons = new Map() + function checkout(preparedPath: string, signal?: AbortSignal): Promise { + const lockReason = createWorktreePreparationLockReason(`bench-${sequence}`) + lockReasons.set(preparedPath, lockReason) return prepareWorktreeCreateCheckout( repoPath, preparedPath, 'main', - createWorktreePreparationLockReason(`bench-${sequence}`), + lockReason, signal ? { signal } : {} ) } +function discard(preparedPath: string): Promise { + const lockReason = lockReasons.get(preparedPath) + if (!lockReason) { + throw new Error('The benchmark checkout has no lock reason') + } + return discardPreparedWorktree(repoPath, preparedPath, {}, lockReason) +} + async function runTrial(variant: Variant, obsolete: number): Promise { const controllers = Array.from({ length: obsolete }, () => new AbortController()) const obsoletePaths = controllers.map(() => nextPreparedPath('obsolete')) @@ -69,11 +81,7 @@ async function runTrial(variant: Variant, obsolete: number): Promise { await checkout(freshPath) const freshCheckoutMs = performance.now() - started await Promise.all(obsoleteWork) - await Promise.all( - [...obsoletePaths, freshPath].map((path) => - discardPreparedWorktree(repoPath, path).catch(() => {}) - ) - ) + await Promise.all([...obsoletePaths, freshPath].map((path) => discard(path).catch(() => {}))) return { variant, obsolete, freshCheckoutMs } } @@ -118,7 +126,7 @@ describeBench('obsolete preparation cancellation latency', () => { // 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) + await discard(warm) const samples: Sample[] = [] for (const obsolete of OBSOLETE_COUNTS) { diff --git a/src/main/git/worktree-preparation-discard.ts b/src/main/git/worktree-preparation-discard.ts new file mode 100644 index 00000000000..bca815e8dc3 --- /dev/null +++ b/src/main/git/worktree-preparation-discard.ts @@ -0,0 +1,89 @@ +import { windowsLongPathGitArgs } from '../../shared/windows-long-path-git-args' +import { gitExecFileAsync } from './runner' +import { + gitExecOptions, + WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS, + type GitExecOptionsForWorktree, + type GitWorktreeExecOptions +} from './worktree-operation-options' +import { verifyWorktreePreparationLock } from './worktree-preparation-lock' +import { invalidateWslLinkedWorktreeGitRouting } from './wsl-linked-worktree-git-routing' + +export function gitCleanupOptions( + cwd: string, + options: GitWorktreeExecOptions +): GitExecOptionsForWorktree { + // Why: cancellation must not strand a partially moved worktree; cleanup is bounded separately. + return gitExecOptions(cwd, { ...options, signal: undefined }) +} + +export async function performDiscardPreparedWorktree( + repoPath: string, + worktreePath: string, + options: GitWorktreeExecOptions, + expectedLockReason?: string +): Promise { + const cleanupGitOptions = { + ...gitCleanupOptions(repoPath, options), + timeout: options.timeout ?? WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + } + try { + if (expectedLockReason !== undefined) { + await verifyWorktreePreparationLock(worktreePath, expectedLockReason, cleanupGitOptions) + } + // Double force requires a freshly verified ownership marker. + await gitExecFileAsync( + [ + ...windowsLongPathGitArgs(repoPath), + 'worktree', + 'remove', + '--force', + ...(expectedLockReason === undefined ? [] : ['--force']), + worktreePath + ], + cleanupGitOptions + ) + } finally { + invalidateWslLinkedWorktreeGitRouting(worktreePath) + } +} + +export async function removeFailedFinalization( + repoPath: string, + cleanupPath: string, + branch: string, + moved: boolean, + options: GitWorktreeExecOptions, + expectedLockReason?: string +): Promise { + let branchAttached = false + if (moved) { + try { + const { stdout } = await gitExecFileAsync( + ['symbolic-ref', '--short', 'HEAD'], + gitCleanupOptions(cleanupPath, options) + ) + branchAttached = stdout.trim() === branch + } catch { + // Detached or no longer readable. + } + } + const removed = await performDiscardPreparedWorktree( + repoPath, + cleanupPath, + options, + expectedLockReason + ).then( + () => true, + () => false + ) + if (!removed) { + return + } + if (branchAttached) { + await gitExecFileAsync( + ['branch', '-D', '--', branch], + gitCleanupOptions(repoPath, options) + ).catch(() => {}) + } +} diff --git a/src/main/git/worktree-preparation-fallback-cleanup-real-git.test.ts b/src/main/git/worktree-preparation-fallback-cleanup-real-git.test.ts new file mode 100644 index 00000000000..2f2e7b05590 --- /dev/null +++ b/src/main/git/worktree-preparation-fallback-cleanup-real-git.test.ts @@ -0,0 +1,207 @@ +import { existsSync } 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, beforeEach, expect, it, vi } from 'vitest' +import { createWorktreePreparationLockReason } from '../../shared/worktree/create-preparation' +import { clearGitCapabilityStateForTests, getLocalGitCapabilityCache } from './git-capability-state' +import * as runner from './runner' +import { prepareWorktreeCreateCheckout } from './worktree-create-preparation' +import { WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS } from './worktree-operation-options' + +const roots: string[] = [] +const unsupported = Object.assign(new Error("error: unknown option 'reason'"), { code: 129 }) + +beforeEach(() => { + clearGitCapabilityStateForTests() +}) + +afterEach(async () => { + vi.restoreAllMocks() + clearGitCapabilityStateForTests() + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +async function git(cwd: string, args: string[]): Promise { + return (await runner.gitExecFileAsync(args, { cwd })).stdout.trim() +} + +async function fixture() { + const root = await realpath(await mkdtemp(join(tmpdir(), 'orca-fallback-cleanup-'))) + roots.push(root) + const repo = join(root, 'repo') + const prepared = join(root, 'prepared') + await git(root, ['init', '--quiet', repo]) + await git(repo, ['symbolic-ref', 'HEAD', 'refs/heads/main']) + await writeFile(join(repo, 'tracked.txt'), 'original\n') + await git(repo, ['add', 'tracked.txt']) + await git(repo, [ + '-c', + 'user.name=Test', + '-c', + 'user.email=test@example.invalid', + 'commit', + '--quiet', + '-m', + 'initial' + ]) + return { repo, prepared } +} + +it.each([ + ['first', 'cancellation'], + ['cached', 'cancellation'], + ['first', 'path probe'], + ['cached', 'path probe'] +])('reclaims its registered %s fallback after %s before locking', async (fallback, failureKind) => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('fallback-cleanup') + const controller = new AbortController() + const failure = new Error(`${failureKind} after completed add`) + const run = runner.gitExecFileAsync + let registered = false + if (fallback === 'cached') { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + } + const spy = vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + if (args.includes('--reason')) { + throw unsupported + } + const result = await run(args, options) + if (args.includes('--no-checkout')) { + registered = true + expect(existsSync(join(prepared, '.git'))).toBe(true) + } + if (registered && options?.cwd === prepared && args.includes('--git-path')) { + expect(existsSync(join(prepared, 'tracked.txt'))).toBe(false) + if (failureKind === 'cancellation') { + controller.abort(failure) + } + throw failure + } + return result + }) + await expect( + prepareWorktreeCreateCheckout(repo, prepared, 'main', reason, { + signal: controller.signal, + timeout: 180_000 + }) + ).rejects.toBe(failure) + const removals = spy.mock.calls.filter(([args]) => args.includes('remove')) + expect(removals).toHaveLength(1) + const [removeArgs, removeOptions] = removals[0]! + expect(removeArgs.filter((arg) => arg === '--force')).toHaveLength(1) + expect(removeOptions).toMatchObject({ + cwd: repo, + timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + }) + expect(removeOptions).not.toHaveProperty('signal') + expect(spy.mock.calls.some(([args]) => args.includes('reset'))).toBe(false) + expect(existsSync(prepared)).toBe(false) + expect((await run(['worktree', 'list', '--porcelain'], { cwd: repo })).stdout).not.toContain( + prepared + ) +}) + +it.each(['first', 'cached'])( + 'preserves another marker after a successful %s fallback add', + async (fallback) => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('competing-fallback') + const run = runner.gitExecFileAsync + let lock = '' + if (fallback === 'cached') { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + } + const spy = vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + if (args.includes('--reason')) { + throw unsupported + } + const result = await run(args, options) + if (args.includes('--no-checkout')) { + lock = (await run(['rev-parse', '--git-path', 'locked'], { cwd: prepared })).stdout.trim() + await writeFile(lock, 'manual competing owner\n') + await writeFile(join(prepared, 'user.txt'), 'preserve this file\n') + } + return result + }) + await expect(prepareWorktreeCreateCheckout(repo, prepared, 'main', reason)).rejects.toThrow( + 'lock owner changed' + ) + expect(await readFile(lock, 'utf8')).toBe('manual competing owner\n') + expect(await readFile(join(prepared, 'user.txt'), 'utf8')).toBe('preserve this file\n') + expect(spy.mock.calls.some(([args]) => args.includes('remove') || args.includes('reset'))).toBe( + false + ) + expect((await run(['worktree', 'list', '--porcelain'], { cwd: repo })).stdout).toContain( + prepared + ) + } +) + +it.each(['first', 'cached'])( + 'lets Git protect a competing lock from %s fallback cleanup after a generic probe failure', + async (fallback) => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('protected-fallback-cleanup') + const failure = new Error('lock path became unreadable') + const run = runner.gitExecFileAsync + let lock = '' + if (fallback === 'cached') { + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + } + const spy = vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + if (args.includes('--reason')) { + throw unsupported + } + if (lock && options?.cwd === prepared && args.includes('--git-path')) { + throw failure + } + const result = await run(args, options) + if (args.includes('--no-checkout')) { + lock = (await run(['rev-parse', '--git-path', 'locked'], { cwd: prepared })).stdout.trim() + await writeFile(lock, 'manual protected owner\n') + await writeFile(join(prepared, 'user.txt'), 'preserve after cleanup attempt\n') + } + return result + }) + await expect(prepareWorktreeCreateCheckout(repo, prepared, 'main', reason)).rejects.toBe( + failure + ) + const removals = spy.mock.calls.filter(([args]) => args.includes('remove')) + expect(removals).toHaveLength(1) + expect(removals[0]![0].filter((arg) => arg === '--force')).toHaveLength(1) + expect(removals[0]![1]).not.toHaveProperty('signal') + expect(removals[0]![1]).toMatchObject({ timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS }) + expect(await readFile(lock, 'utf8')).toBe('manual protected owner\n') + expect(await readFile(join(prepared, 'user.txt'), 'utf8')).toBe( + 'preserve after cleanup attempt\n' + ) + expect(spy.mock.calls.some(([args]) => args.includes('reset'))).toBe(false) + expect((await run(['worktree', 'list', '--porcelain'], { cwd: repo })).stdout).toContain( + prepared + ) + } +) + +it('preserves a pre-existing empty target if its cached fallback lock-path probe fails', async () => { + const { repo, prepared } = await fixture() + await mkdir(prepared) + getLocalGitCapabilityCache().rememberUnsupported('worktree-add-lock-reason') + const failure = new Error('pre-existing target lock path unavailable') + const run = runner.gitExecFileAsync + const spy = vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + if (options?.cwd === prepared && args.includes('--git-path')) { + throw failure + } + return run(args, options) + }) + await expect(prepareWorktreeCreateCheckout(repo, prepared, 'main', 'reason')).rejects.toBe( + failure + ) + expect(existsSync(join(prepared, '.git'))).toBe(true) + expect(spy.mock.calls.some(([args]) => args.includes('remove') || args.includes('reset'))).toBe( + false + ) + expect((await run(['worktree', 'list', '--porcelain'], { cwd: repo })).stdout).toContain(prepared) +}) diff --git a/src/main/git/worktree-preparation-lock-real-git.test.ts b/src/main/git/worktree-preparation-lock-real-git.test.ts new file mode 100644 index 00000000000..55c31fdc44d --- /dev/null +++ b/src/main/git/worktree-preparation-lock-real-git.test.ts @@ -0,0 +1,336 @@ +import { mkdtemp, readFile, realpath, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, relative } from 'node:path' +import { afterEach, expect, it, vi } from 'vitest' +import { createWorktreePreparationLockReason } from '../../shared/worktree/create-preparation' +import * as runner from './runner' +import { + discardPreparedWorktree, + finalizePreparedWorktree, + prepareWorktreeCreateCheckout +} from './worktree-create-preparation' +import { unlockWorktreePreparation } from './worktree-preparation-lock' +import { + _resetPreparationPoolForTests, + listPreparations, + startPreparation, + WORKTREE_CREATE_PREPARATION_TTL_MS +} from '../worktree-create-preparation-pool' + +const roots: string[] = [] +afterEach(async () => { + vi.restoreAllMocks() + await _resetPreparationPoolForTests() + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +async function git(cwd: string, args: string[]): Promise { + return (await runner.gitExecFileAsync(args, { cwd })).stdout.trim() +} + +async function fixture(): Promise<{ root: string; repo: string; prepared: string; final: string }> { + const root = await realpath(await mkdtemp(join(tmpdir(), 'orca-preparation-lock-'))) + roots.push(root) + const repo = join(root, 'repo') + await git(root, ['init', '--quiet', repo]) + await git(repo, ['symbolic-ref', 'HEAD', 'refs/heads/main']) + await writeFile(join(repo, 'tracked.txt'), 'original\n') + await git(repo, ['add', 'tracked.txt']) + await git(repo, [ + '-c', + 'user.name=Test', + '-c', + 'user.email=test@example.com', + 'commit', + '--quiet', + '-m', + 'initial' + ]) + return { root, repo, prepared: join(root, 'prepared'), final: join(root, 'final') } +} + +it('creates and consumes its marker without worktree lock or unlock inventory scans', async () => { + const { repo, prepared, final } = await fixture() + const reason = createWorktreePreparationLockReason('targeted') + const spy = vi.spyOn(runner, 'gitExecFileAsync') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`) + spy.mockClear() + await finalizePreparedWorktree(repo, prepared, final, 'feature', 'main', false, {}, reason) + expect(spy.mock.calls.filter(([args]) => args.includes('--git-path'))).toHaveLength(1) + expect(spy.mock.calls.filter(([args]) => args.includes('--git-common-dir'))).toHaveLength(1) + expect(await git(final, ['symbolic-ref', '--short', 'HEAD'])).toBe('feature') + expect(await git(final, ['status', '--porcelain'])).toBe('') + expect(await readFile(join(final, 'tracked.txt'), 'utf8')).toBe('original\n') + await expect(readFile(lock, 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }) + expect(spy.mock.calls.some(([args]) => args.includes('lock') || args.includes('unlock'))).toBe( + false + ) +}) + +it('has its exact ownership marker before the atomic add returns', async () => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('atomic-add') + const run = runner.gitExecFileAsync + let observed = false + vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + const result = await run(args, options) + if (args.includes('--reason')) { + const lock = ( + await run(['rev-parse', '--git-path', 'locked'], { cwd: prepared }) + ).stdout.trim() + expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`) + await expect(readFile(join(prepared, 'tracked.txt'))).rejects.toMatchObject({ + code: 'ENOENT' + }) + observed = true + } + return result + }) + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const version = await git(repo, ['--version']) + const minor = Number(version.match(/git version 2\.(\d+)/)?.[1]) + expect(observed).toBe(minor >= 33) +}) + +it('cleans only its newly registered marker when cancellation follows atomic add', async () => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('atomic-cancellation') + const controller = new AbortController() + const run = runner.gitExecFileAsync + const spy = vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + const result = await run(args, options) + if (args.includes('--no-checkout') && args.includes('--reason')) { + controller.abort() + throw new Error('canceled after atomic add') + } + return result + }) + await expect( + prepareWorktreeCreateCheckout(repo, prepared, 'main', reason, { signal: controller.signal }) + ).rejects.toThrow('canceled after atomic add') + expect(spy.mock.calls.some(([args]) => args.includes('reset'))).toBe(false) + await expect(readFile(join(prepared, '.git'))).rejects.toMatchObject({ code: 'ENOENT' }) + expect(await git(repo, ['worktree', 'list', '--porcelain'])).not.toContain(prepared) +}) + +it('preserves an existing owned checkout when another add fails at its path', async () => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('existing-add') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + const spy = vi.spyOn(runner, 'gitExecFileAsync') + await expect(prepareWorktreeCreateCheckout(repo, prepared, 'main', reason)).rejects.toThrow() + expect(spy.mock.calls.some(([args]) => args.includes('remove'))).toBe(false) + expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`) + expect(await readFile(join(prepared, 'tracked.txt'), 'utf8')).toBe('original\n') +}) + +it('resolves a relative gitfile and retains a competing unlock marker', async () => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('relative') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const adminDir = await git(prepared, ['rev-parse', '--git-dir']) + await writeFile(join(prepared, '.git'), `gitdir: ${relative(prepared, adminDir)}\n`) + const lock = join(adminDir, 'locked') + await writeFile(lock, 'manual user lock\n') + await expect(unlockWorktreePreparation(prepared, reason, {})).rejects.toThrow( + 'lock owner changed' + ) + expect(await readFile(lock, 'utf8')).toBe('manual user lock\n') + expect(await git(prepared, ['rev-parse', '--verify', 'HEAD'])).toBe( + await git(repo, ['rev-parse', 'HEAD']) + ) +}) + +it('preserves a competing marker and registration through preparation failure and pool reset', async () => { + const { root, repo } = await fixture() + let prepared = '' + let lock = '' + const run = runner.gitExecFileAsync + vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + const result = await run(args, options) + if (args.includes('reset') && options?.cwd) { + prepared = options.cwd + lock = (await run(['rev-parse', '--git-path', 'locked'], options)).stdout.trim() + await writeFile(lock, 'manual competing preparation\n') + } + return result + }) + await expect( + startPreparation({ + repoPath: repo, + workspaceRoot: root, + baseBranch: 'main', + canonicalBase: 'refs/heads/main', + options: {} + }) + ).rejects.toThrow('lock owner changed') + await _resetPreparationPoolForTests() + expect(await readFile(lock, 'utf8')).toBe('manual competing preparation\n') + expect(await readFile(join(prepared, 'tracked.txt'), 'utf8')).toBe('original\n') + expect(await git(repo, ['worktree', 'list', '--porcelain'])).toContain( + 'manual competing preparation' + ) +}) + +it('claims the marker before materialization and preserves a competing owner after add', async () => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('before-materialization') + const run = runner.gitExecFileAsync + let lock = '' + const spy = vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + const result = await run(args, options) + if (args.includes('--no-checkout')) { + lock = (await run(['rev-parse', '--git-path', 'locked'], { cwd: prepared })).stdout.trim() + await writeFile(lock, 'manual before materialization\n') + await writeFile(join(prepared, 'tracked.txt'), 'user checkout content\n') + } + return result + }) + await expect(prepareWorktreeCreateCheckout(repo, prepared, 'main', reason)).rejects.toThrow( + 'lock owner changed' + ) + expect(spy.mock.calls.some(([args]) => args.includes('reset') || args.includes('remove'))).toBe( + false + ) + expect(await readFile(lock, 'utf8')).toBe('manual before materialization\n') + expect(await readFile(join(prepared, 'tracked.txt'), 'utf8')).toBe('user checkout content\n') +}) + +it.each(['checkout', 'push.autoSetupRemote'])( + 'preserves the finalized checkout when its marker is replaced during %s', + async (command) => { + const { repo, prepared, final } = await fixture() + const reason = createWorktreePreparationLockReason('replacement') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + const run = runner.gitExecFileAsync + vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + if (args.includes(command)) { + await writeFile(lock, 'manual finalized lock\n') + } + return run(args, options) + }) + await expect( + finalizePreparedWorktree(repo, prepared, final, 'feature', 'main', false, {}, reason) + ).rejects.toThrow('lock owner changed') + expect(await readFile(lock, 'utf8')).toBe('manual finalized lock\n') + expect(await readFile(join(final, 'tracked.txt'), 'utf8')).toBe('original\n') + expect(await git(final, ['symbolic-ref', '--short', 'HEAD'])).toBe('feature') + } +) + +it('leaves a replacement owner untouched before any reset, move, or branch attachment', async () => { + const { repo, prepared, final } = await fixture() + const reason = createWorktreePreparationLockReason('replaced-before-finalize') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + await writeFile(lock, 'manual replacement\n') + await writeFile(join(prepared, 'tracked.txt'), 'user edits\n') + const spy = vi.spyOn(runner, 'gitExecFileAsync') + await expect( + finalizePreparedWorktree(repo, prepared, final, 'feature', 'main', false, {}, reason) + ).rejects.toThrow('lock owner changed') + expect( + spy.mock.calls.some( + ([args]) => args.includes('reset') || args.includes('move') || args.includes('checkout') + ) + ).toBe(false) + expect(await readFile(lock, 'utf8')).toBe('manual replacement\n') + expect(await readFile(join(prepared, 'tracked.txt'), 'utf8')).toBe('user edits\n') + expect(await git(repo, ['branch', '--list', 'feature'])).toBe('') +}) + +it('checks replacement ownership after move before attaching a branch', async () => { + const { repo, prepared, final } = await fixture() + const reason = createWorktreePreparationLockReason('replaced-after-move') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + const run = runner.gitExecFileAsync + const spy = vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + const result = await run(args, options) + if (args.includes('move')) { + await writeFile(lock, 'manual moved lock\n') + } + return result + }) + await expect( + finalizePreparedWorktree(repo, prepared, final, 'feature', 'main', false, {}, reason) + ).rejects.toThrow('lock owner changed') + expect( + spy.mock.calls.some(([args]) => args.includes('checkout') || args.includes('remove')) + ).toBe(false) + expect(await readFile(lock, 'utf8')).toBe('manual moved lock\n') + expect(await git(final, ['symbolic-ref', '--quiet', 'HEAD']).catch(() => 'detached')).toBe( + 'detached' + ) + expect(await git(repo, ['branch', '--list', 'feature'])).toBe('') +}) + +it.each(['replacement', 'missing'])( + 'preserves checkout and branch if failure cleanup finds a %s marker', + async (marker) => { + const { repo, prepared, final } = await fixture() + const reason = createWorktreePreparationLockReason('failure-cleanup') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + const run = runner.gitExecFileAsync + vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + const result = await run(args, options) + if (args.includes('checkout')) { + await (marker === 'replacement' ? writeFile(lock, 'manual failed lock\n') : rm(lock)) + throw new Error('injected checkout failure') + } + return result + }) + await expect( + finalizePreparedWorktree(repo, prepared, final, 'feature', 'main', false, {}, reason) + ).rejects.toThrow('injected checkout failure') + expect(await readFile(join(final, 'tracked.txt'), 'utf8')).toBe('original\n') + expect(await git(final, ['symbolic-ref', '--short', 'HEAD'])).toBe('feature') + expect(await git(repo, ['branch', '--list', 'feature'])).toContain('feature') + if (marker === 'replacement') { + expect(await readFile(lock, 'utf8')).toBe('manual failed lock\n') + } + } +) + +it('refuses a force discard after the marker was replaced', async () => { + const { repo, prepared } = await fixture() + const reason = createWorktreePreparationLockReason('discard-replacement') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + await writeFile(lock, 'manual discard lock\n') + await expect(discardPreparedWorktree(repo, prepared, {}, reason)).rejects.toThrow( + 'lock owner changed' + ) + expect(await readFile(lock, 'utf8')).toBe('manual discard lock\n') + expect(await readFile(join(prepared, 'tracked.txt'), 'utf8')).toBe('original\n') +}) + +it.each(['expiry', 'pool reset'])('preserves a replacement marker during %s', async (kind) => { + const { root, repo } = await fixture() + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + try { + await startPreparation({ + repoPath: repo, + workspaceRoot: root, + baseBranch: 'main', + canonicalBase: 'refs/heads/main', + options: {} + }) + const entry = listPreparations()[0] + const lock = await git(entry.preparedPath, ['rev-parse', '--git-path', 'locked']) + await writeFile(lock, 'manual expired lock\n') + if (kind === 'expiry') { + await vi.advanceTimersByTimeAsync(WORKTREE_CREATE_PREPARATION_TTL_MS) + } + await _resetPreparationPoolForTests() + expect(await readFile(lock, 'utf8')).toBe('manual expired lock\n') + expect(await readFile(join(entry.preparedPath, 'tracked.txt'), 'utf8')).toBe('original\n') + } finally { + vi.useRealTimers() + } +}) diff --git a/src/main/git/worktree-preparation-lock.test.ts b/src/main/git/worktree-preparation-lock.test.ts new file mode 100644 index 00000000000..8120514a695 --- /dev/null +++ b/src/main/git/worktree-preparation-lock.test.ts @@ -0,0 +1,193 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { toHostFilesystemPath } from '../host-tree-removal' +import type * as FilePromises from 'node:fs/promises' + +const mocks = vi.hoisted(() => ({ + git: vi.fn(), + readFile: vi.fn(), + writeFile: vi.fn(), + unlink: vi.fn() +})) +vi.mock('./runner', () => ({ gitExecFileAsync: mocks.git })) +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + readFile: mocks.readFile, + writeFile: mocks.writeFile, + unlink: mocks.unlink +})) + +import { + lockWorktreePreparation, + resolveWorktreePreparationLockPath, + unlockWorktreePreparation, + unlockWorktreePreparationAtPath +} from './worktree-preparation-lock' + +const lockReason = 'orca-create-preparation:v1:123:exact-session' +const commonDir = join(tmpdir(), 'repo', '.git') +const gitLockPath = join(commonDir, 'worktrees', 'prepared', 'locked') +const lockPath = toHostFilesystemPath(gitLockPath) + +beforeEach(() => { + mocks.git.mockReset().mockImplementation(async (args: string[]) => ({ + stdout: `${args.includes('--git-path') ? gitLockPath : commonDir}\n` + })) + mocks.readFile.mockReset().mockResolvedValue(`${lockReason}\n`) + mocks.writeFile.mockReset().mockResolvedValue(undefined) + mocks.unlink.mockReset().mockResolvedValue(undefined) +}) + +describe('targeted preparation lock ownership', () => { + it('creates Git’s reason marker exclusively without enumerating worktrees', async () => { + const options = { wslDistro: 'Ubuntu', timeout: 8000, admissionTier: 'interactive' as const } + await lockWorktreePreparation('/prepared', lockReason, options) + expect(mocks.git).toHaveBeenCalledTimes(2) + expect(mocks.git).toHaveBeenCalledWith(['rev-parse', '--git-path', 'locked'], { + cwd: '/prepared', + ...options + }) + expect(mocks.git).toHaveBeenCalledWith(['rev-parse', '--git-common-dir'], { + cwd: '/prepared', + ...options + }) + expect(mocks.writeFile).toHaveBeenCalledExactlyOnceWith(lockPath, `${lockReason}\n`, { + flag: 'wx' + }) + }) + + it('preserves an existing lock if exclusive creation fails', async () => { + const error = Object.assign(new Error('lock exists'), { code: 'EEXIST' }) + mocks.writeFile.mockRejectedValueOnce(error) + await expect(lockWorktreePreparation('/prepared', lockReason, {})).rejects.toThrow( + 'lock owner changed' + ) + expect(mocks.unlink).not.toHaveBeenCalled() + }) + + it('unlinks only the exact reason minted for this checkout', async () => { + await unlockWorktreePreparation('/final', lockReason, {}) + expect(mocks.readFile).toHaveBeenCalledExactlyOnceWith(lockPath, 'utf8') + expect(mocks.unlink).toHaveBeenCalledExactlyOnceWith(lockPath) + }) + + it('reuses a verified administrative path and reads the owner again before unlinking', async () => { + await unlockWorktreePreparationAtPath(lockPath, lockReason) + expect(mocks.git).not.toHaveBeenCalled() + expect(mocks.readFile).toHaveBeenCalledExactlyOnceWith(lockPath, 'utf8') + expect(mocks.unlink).toHaveBeenCalledExactlyOnceWith(lockPath) + }) + + it.each([ + 'user lock\n', + 'orca-create-preparation:v1:123:another-session\n', + lockReason, + `${lockReason}\n\n` + ])('preserves a replacement owner: %s', async (reason) => { + mocks.readFile.mockResolvedValueOnce(reason) + await expect(unlockWorktreePreparation('/final', lockReason, {})).rejects.toThrow( + 'lock owner changed' + ) + expect(mocks.unlink).not.toHaveBeenCalled() + }) + + it.each(['ENOENT', 'EACCES'])( + 'preserves the checkout when marker ownership cannot be read: %s', + async (code) => { + mocks.readFile.mockRejectedValueOnce(Object.assign(new Error('marker unavailable'), { code })) + await expect(unlockWorktreePreparation('/final', lockReason, {})).rejects.toThrow( + 'lock owner changed' + ) + expect(mocks.unlink).not.toHaveBeenCalled() + } + ) + + it('honors cancellation after the path probe before writing a lock', async () => { + const controller = new AbortController() + mocks.git.mockImplementationOnce(async () => { + controller.abort() + return { stdout: `${gitLockPath}\n` } + }) + await expect( + lockWorktreePreparation('/prepared', lockReason, { signal: controller.signal }) + ).rejects.toThrow() + expect(mocks.writeFile).not.toHaveBeenCalled() + }) + + it('honors cancellation during the ownership read before unlinking', async () => { + const controller = new AbortController() + const cancellation = new Error('unlock canceled') + mocks.readFile.mockImplementationOnce(async () => { + controller.abort(cancellation) + return `${lockReason}\n` + }) + await expect( + unlockWorktreePreparationAtPath(lockPath, lockReason, controller.signal) + ).rejects.toBe(cancellation) + expect(mocks.unlink).not.toHaveBeenCalled() + }) +}) + +describe('Git preparation lock path resolution', () => { + it.each([ + { + path: '/prepared', + lock: '/repo/.git/worktrees/prepared/locked\n', + common: '/repo/.git\n', + expected: '/repo/.git/worktrees/prepared/locked', + options: { platform: 'linux' as const } + }, + { + path: '/workspace/prepared', + lock: '../../repo/.git/worktrees/prepared/locked\n', + common: '../../repo/.git\n', + expected: '/repo/.git/worktrees/prepared/locked', + options: { platform: 'linux' as const } + }, + { + path: String.raw`C:\workspace\prepared`, + lock: 'C:/repo/.git/worktrees/prepared/locked\n', + common: 'C:/repo/.git\n', + expected: 'C:/repo/.git/worktrees/prepared/locked', + options: { platform: 'win32' as const, wslDistro: 'Ubuntu' } + }, + { + path: String.raw`\\wsl.localhost\Ubuntu\home\workspace\prepared`, + lock: '/home/repo/.git/worktrees/prepared/locked\n', + common: '/home/repo/.git\n', + expected: String.raw`\\wsl.localhost\Ubuntu\home\repo\.git\worktrees\prepared\locked`, + options: { platform: 'win32' as const, wslDistro: 'Ubuntu' } + }, + { + path: String.raw`C:\workspace\prepared`, + lock: '/mnt/c/repo/.git/worktrees/prepared/locked\n', + common: '/mnt/c/repo/.git\n', + expected: String.raw`C:\repo\.git\worktrees\prepared\locked`, + options: { platform: 'win32' as const, wslDistro: 'Ubuntu' } + }, + { + path: '/workspace/new\nline/prepared', + lock: '/repo/new\nline/.git/worktrees/prepared/locked\n', + common: '/repo/new\nline/.git\n', + expected: '/repo/new\nline/.git/worktrees/prepared/locked', + options: { platform: 'linux' as const } + } + ])( + 'resolves $path in the filesystem namespace that owns Git', + ({ path, lock, common, expected, options }) => { + expect(resolveWorktreePreparationLockPath(path, lock, common, options)).toBe(expected) + } + ) + + it.each([ + { lock: '/repo/.git/locked\n', common: '/repo/.git\n' }, + { lock: '/other/.git/worktrees/prepared/locked\n', common: '/repo/.git\n' }, + { lock: '/repo/.git/worktrees/prepared/nested/locked\n', common: '/repo/.git\n' }, + { lock: '/repo/.git/worktrees/prepared/locked\n', common: '' } + ])('rejects a path without one linked administration entry: $lock', ({ lock, common }) => { + expect(() => resolveWorktreePreparationLockPath('/prepared', lock, common)).toThrow( + 'linked worktree lock path' + ) + }) +}) diff --git a/src/main/git/worktree-preparation-lock.ts b/src/main/git/worktree-preparation-lock.ts new file mode 100644 index 00000000000..ba60280b90e --- /dev/null +++ b/src/main/git/worktree-preparation-lock.ts @@ -0,0 +1,130 @@ +import { readFile, unlink, writeFile } from 'node:fs/promises' +import { posix, win32 } from 'node:path' +import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' +import { resolveGitMetadataPath, type GitMetadataPathOptions } from '../../shared/git-metadata-path' +import { toHostFilesystemPath } from '../host-tree-removal' +import { gitExecFileAsync } from './runner' +import { + getErrorCode, + gitExecOptions, + type GitWorktreeExecOptions +} from './worktree-operation-options' + +export class WorktreePreparationLockOwnershipError extends Error { + constructor(cause?: unknown) { + super('The prepared worktree lock owner changed', { cause }) + } +} + +export function resolveWorktreePreparationLockPath( + worktreePath: string, + rawLockPath: string, + rawCommonDir: string, + options: GitMetadataPathOptions = {} +): string { + const lockPath = resolveGitMetadataPath(worktreePath, rawLockPath, options) + const commonDir = resolveGitMetadataPath(worktreePath, rawCommonDir, options) + if (!lockPath || !commonDir) { + throw new Error('Git did not return a linked worktree lock path') + } + const paths = isWindowsAbsolutePathLike(lockPath) ? win32 : posix + const segments = paths.relative(commonDir, lockPath).split(paths.sep) + if ( + segments.length !== 3 || + segments[0] !== 'worktrees' || + !segments[1] || + segments[2] !== 'locked' + ) { + throw new Error('Git did not return a linked worktree lock path') + } + return lockPath +} + +async function readPreparationLockPath( + worktreePath: string, + options: GitWorktreeExecOptions +): Promise { + const results = await Promise.allSettled([ + gitExecFileAsync(['rev-parse', '--git-path', 'locked'], gitExecOptions(worktreePath, options)), + gitExecFileAsync(['rev-parse', '--git-common-dir'], gitExecOptions(worktreePath, options)) + ]) + const [lock, common] = results + if (lock.status === 'rejected') { + throw lock.reason + } + if (common.status === 'rejected') { + throw common.reason + } + return toHostFilesystemPath( + resolveWorktreePreparationLockPath( + worktreePath, + lock.value.stdout, + common.value.stdout, + options + ) + ) +} + +export async function lockWorktreePreparation( + worktreePath: string, + lockReason: string, + options: GitWorktreeExecOptions +): Promise { + const lockPath = await readPreparationLockPath(worktreePath, options) + options.signal?.throwIfAborted() + // Git's own lock marker is a reason plus newline; exclusive creation preserves another owner. + try { + await writeFile(lockPath, `${lockReason}\n`, { flag: 'wx' }) + } catch (error) { + if (getErrorCode(error) === 'EEXIST') { + throw new WorktreePreparationLockOwnershipError() + } + throw error + } + return lockPath +} + +export async function unlockWorktreePreparation( + worktreePath: string, + expectedLockReason: string, + options: GitWorktreeExecOptions +): Promise { + const lockPath = await readPreparationLockPath(worktreePath, options) + await unlockWorktreePreparationAtPath(lockPath, expectedLockReason, options.signal) +} + +/** A move preserves the linked administration directory already verified by finalization. */ +export async function unlockWorktreePreparationAtPath( + lockPath: string, + expectedLockReason: string, + signal?: AbortSignal +): Promise { + await verifyWorktreePreparationLockAtPath(lockPath, expectedLockReason, signal) + await unlink(lockPath).catch((error: unknown) => { + throw new WorktreePreparationLockOwnershipError(error) + }) +} + +export async function verifyWorktreePreparationLock( + worktreePath: string, + expectedLockReason: string, + options: GitWorktreeExecOptions +): Promise { + const lockPath = await readPreparationLockPath(worktreePath, options) + await verifyWorktreePreparationLockAtPath(lockPath, expectedLockReason, options.signal) + return lockPath +} + +export async function verifyWorktreePreparationLockAtPath( + lockPath: string, + expectedLockReason: string, + signal?: AbortSignal +): Promise { + const lockReason = await readFile(lockPath, 'utf8').catch((error: unknown) => { + throw new WorktreePreparationLockOwnershipError(error) + }) + if (lockReason !== `${expectedLockReason}\n`) { + throw new WorktreePreparationLockOwnershipError() + } + signal?.throwIfAborted() +} diff --git a/src/main/git/worktree-preparation-tip-refresh-real-git.test.ts b/src/main/git/worktree-preparation-tip-refresh-real-git.test.ts new file mode 100644 index 00000000000..a1bc9082f57 --- /dev/null +++ b/src/main/git/worktree-preparation-tip-refresh-real-git.test.ts @@ -0,0 +1,156 @@ +import { existsSync } 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, expect, it, vi } from 'vitest' +import { createWorktreePreparationLockReason } from '../../shared/worktree/create-preparation' +import * as runner from './runner' +import { + finalizePreparedWorktree, + prepareWorktreeCreateCheckout +} from './worktree-create-preparation' +import { refreshPreparedWorktreeTip } from './worktree-preparation-tip-refresh' +import { + _resetPreparationPoolForTests, + listPreparations, + releasePreparationClaim, + startPreparation, + takePreparation +} from '../worktree-create-preparation-pool' + +const roots: string[] = [] +afterEach(async () => { + vi.restoreAllMocks() + await _resetPreparationPoolForTests() + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +async function git(cwd: string, args: string[]): Promise { + return (await runner.gitExecFileAsync(args, { cwd })).stdout.trim() +} + +async function fixture() { + const root = await realpath(await mkdtemp(join(tmpdir(), 'orca-fetched-preparation-'))) + roots.push(root) + const repo = join(root, 'repo') + const prepared = join(root, 'prepared') + const final = join(root, 'final') + await git(root, ['init', '--quiet', repo]) + await git(repo, ['symbolic-ref', 'HEAD', 'refs/heads/main']) + await git(repo, ['config', 'user.name', 'Test']) + await git(repo, ['config', 'user.email', 'test@example.com']) + await writeFile(join(repo, 'version.txt'), 'original\n') + await git(repo, ['add', 'version.txt']) + await git(repo, ['commit', '--quiet', '-m', 'initial']) + const hooks = join(root, 'hooks') + await mkdir(hooks) + await writeFile(join(hooks, 'post-checkout'), '#!/bin/sh\necho checkout >> checkout-hook.txt\n', { + mode: 0o755 + }) + await git(repo, ['config', 'core.hooksPath', hooks]) + const base = 'refs/remotes/origin/main' + await git(repo, ['update-ref', base, 'HEAD']) + const reason = createWorktreePreparationLockReason('fetched-tip') + await prepareWorktreeCreateCheckout(repo, prepared, base, reason) + return { root, repo, prepared, final, base, reason } +} + +async function advance(repo: string, base: string, text: string): Promise { + await writeFile(join(repo, 'version.txt'), text) + await git(repo, ['commit', '--quiet', '-am', text.trim()]) + const head = await git(repo, ['rev-parse', 'HEAD']) + await git(repo, ['update-ref', base, head]) + return head +} + +it('moves changed tip work into prefetch while submit still runs exactly one checkout hook', async () => { + const { repo, prepared, final, base, reason } = await fixture() + const target = await advance(repo, base, 'fetched\n') + const spy = vi.spyOn(runner, 'gitExecFileAsync') + await refreshPreparedWorktreeTip(repo, prepared, base, reason) + expect(await readFile(join(prepared, 'version.txt'), 'utf8')).toBe('fetched\n') + expect(await git(prepared, ['rev-parse', 'HEAD'])).toBe(target) + expect(existsSync(join(prepared, 'checkout-hook.txt'))).toBe(false) + expect(spy.mock.calls.filter(([args]) => args.includes('reset'))).toHaveLength(1) + spy.mockClear() + await refreshPreparedWorktreeTip(repo, prepared, base, reason) + await finalizePreparedWorktree(repo, prepared, final, 'feature', base, false, {}, reason) + expect(spy.mock.calls.filter(([args]) => args.includes('reset'))).toHaveLength(0) + expect(await readFile(join(final, 'checkout-hook.txt'), 'utf8')).toBe('checkout\n') + expect(await git(final, ['rev-parse', 'HEAD'])).toBe(target) + expect(await git(final, ['status', '--porcelain', '--untracked-files=no'])).toBe('') +}) + +it('revalidates a newer fetched tip that arrives after the background refresh', async () => { + const { repo, prepared, final, base, reason } = await fixture() + await advance(repo, base, 'first fetch\n') + await refreshPreparedWorktreeTip(repo, prepared, base, reason) + const newest = await advance(repo, base, 'second fetch\n') + await finalizePreparedWorktree(repo, prepared, final, 'feature', base, false, {}, reason) + expect(await git(final, ['rev-parse', 'HEAD'])).toBe(newest) + expect(await readFile(join(final, 'version.txt'), 'utf8')).toBe('second fetch\n') + expect(await readFile(join(final, 'checkout-hook.txt'), 'utf8')).toBe('checkout\n') +}) + +it('preserves a competing lock and files when ownership changes before refresh', async () => { + const { repo, prepared, base, reason } = await fixture() + await advance(repo, base, 'new fetch\n') + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + await writeFile(lock, 'manual owner\n') + await expect(refreshPreparedWorktreeTip(repo, prepared, base, reason)).rejects.toThrow( + 'lock owner changed' + ) + expect(await readFile(lock, 'utf8')).toBe('manual owner\n') + expect(await readFile(join(prepared, 'version.txt'), 'utf8')).toBe('original\n') + expect(await git(repo, ['worktree', 'list', '--porcelain'])).toContain('locked manual owner') +}) + +it('makes a racing claim wait for one fetched-tip reset on an already ready checkout', async () => { + const { root, repo, final, base } = await fixture() + const args = { + repoPath: repo, + workspaceRoot: root, + baseBranch: base, + canonicalBase: base, + options: {} + } + await startPreparation(args) + const entry = listPreparations()[0]! + let settle!: () => void + const beforeMaterialization = new Promise((resolve) => { + settle = resolve + }) + const spy = vi.spyOn(runner, 'gitExecFileAsync') + const refreshing = startPreparation({ ...args, beforeMaterialization }) + const claim = takePreparation(entry) + let finalized = false + const create = entry.ready.then(async () => { + await finalizePreparedWorktree( + repo, + entry.preparedPath, + final, + 'feature', + base, + false, + {}, + entry.lockReason + ) + finalized = true + }) + try { + await Promise.resolve() + expect(finalized).toBe(false) + expect(spy.mock.calls.filter(([argv]) => argv.includes('reset'))).toHaveLength(0) + const target = await advance(repo, base, 'fetched ready tip\n') + expect(finalized).toBe(false) + settle() + await Promise.all([refreshing, create]) + expect(spy.mock.calls.filter(([argv]) => argv.includes('reset'))).toHaveLength(1) + expect(await git(final, ['rev-parse', 'HEAD'])).toBe(target) + expect(await readFile(join(final, 'version.txt'), 'utf8')).toBe('fetched ready tip\n') + expect(await readFile(join(final, 'checkout-hook.txt'), 'utf8')).toBe('checkout\n') + } finally { + settle() + releasePreparationClaim(claim) + } +}) diff --git a/src/main/git/worktree-preparation-tip-refresh.test.ts b/src/main/git/worktree-preparation-tip-refresh.test.ts new file mode 100644 index 00000000000..0e377e1bc77 --- /dev/null +++ b/src/main/git/worktree-preparation-tip-refresh.test.ts @@ -0,0 +1,120 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + git: vi.fn(), + verify: vi.fn(), + verifyAt: vi.fn(), + mutation: vi.fn(), + invalidation: vi.fn() +})) +vi.mock('./runner', () => ({ gitExecFileAsync: mocks.git })) +vi.mock('./worktree', () => ({ notifyPreparedWorktreeMutation: mocks.mutation })) +vi.mock('./status', () => ({ runWithGitReadCacheInvalidation: mocks.invalidation })) +vi.mock('./local-repo-ref-maintenance', () => ({ + withRepoRefMaintenancePaused: (_reason: string, run: () => Promise) => run() +})) +vi.mock('./worktree-preparation-lock', () => ({ + verifyWorktreePreparationLock: mocks.verify, + verifyWorktreePreparationLockAtPath: mocks.verifyAt +})) + +import { refreshPreparedWorktreeTip } from './worktree-preparation-tip-refresh' + +const OLD = 'a'.repeat(40) +const NEW = 'b'.repeat(40) +const BASE = 'refs/remotes/origin/main' + +beforeEach(() => { + mocks.git.mockReset().mockImplementation(async (args: string[], options: { cwd?: string }) => ({ + stdout: args[0] === 'rev-parse' && options.cwd === '/repo' ? `${NEW}\n` : `${OLD}\n` + })) + mocks.verify.mockReset().mockResolvedValue('/repo/.git/worktrees/prepared/locked') + mocks.verifyAt.mockReset().mockImplementation(async (_lock, _reason, signal?: AbortSignal) => { + signal?.throwIfAborted() + }) + mocks.mutation.mockReset() + mocks.invalidation.mockReset().mockImplementation((run: () => Promise) => run()) +}) + +describe('prepared checkout fetched tip materialization', () => { + it('does no index or file mutation when the fetched tip is unchanged', async () => { + mocks.git.mockResolvedValue({ stdout: `${OLD}\n` }) + await refreshPreparedWorktreeTip('/repo', '/prepared', BASE, 'owner') + expect(mocks.git.mock.calls.map(([args]) => args)).toEqual([ + ['rev-parse', '--verify', `${BASE}^{commit}`], + ['rev-parse', '--verify', 'HEAD'] + ]) + expect(mocks.invalidation).not.toHaveBeenCalled() + expect(mocks.mutation).not.toHaveBeenCalled() + }) + + it('resets only to the fetched commit on the owning WSL host without checkout hooks', async () => { + const signal = new AbortController().signal + await refreshPreparedWorktreeTip('/repo', '/prepared', BASE, 'owner', { + wslDistro: 'Ubuntu', + admissionTier: 'status', + signal + }) + expect(mocks.git).toHaveBeenLastCalledWith( + ['reset', '--hard', NEW], + expect.objectContaining({ + cwd: '/prepared', + wslDistro: 'Ubuntu', + admissionTier: 'status', + signal + }) + ) + expect(mocks.git.mock.calls.some(([args]) => args.includes('checkout'))).toBe(false) + expect(mocks.verifyAt).toHaveBeenCalledTimes(2) + expect(mocks.mutation).toHaveBeenCalledOnce() + }) + + it('stops before any probe when the stored lock belongs to another owner', async () => { + mocks.verify.mockRejectedValueOnce(new Error('ownership lost')) + await expect(refreshPreparedWorktreeTip('/repo', '/prepared', BASE, 'owner')).rejects.toThrow( + 'ownership lost' + ) + expect(mocks.git).not.toHaveBeenCalled() + }) + + it('rechecks ownership after both reads before touching files', async () => { + mocks.verifyAt.mockRejectedValueOnce(new Error('ownership changed during probes')) + await expect(refreshPreparedWorktreeTip('/repo', '/prepared', BASE, 'owner')).rejects.toThrow( + 'ownership changed' + ) + expect(mocks.git).toHaveBeenCalledTimes(2) + expect(mocks.invalidation).not.toHaveBeenCalled() + }) + + it('settles both probes before reporting a failed ref read', async () => { + let release!: () => void + mocks.git + .mockRejectedValueOnce(new Error('base unavailable')) + .mockImplementationOnce( + () => new Promise((resolve) => (release = () => resolve({ stdout: OLD }))) + ) + let settled = false + const refresh = refreshPreparedWorktreeTip('/repo', '/prepared', BASE, 'owner').finally(() => { + settled = true + }) + const assertion = expect(refresh).rejects.toThrow('base unavailable') + await vi.waitFor(() => expect(mocks.git).toHaveBeenCalledTimes(2)) + expect(settled).toBe(false) + release() + await assertion + expect(mocks.invalidation).not.toHaveBeenCalled() + }) + + it('honors cancellation between probes and materialization', async () => { + const controller = new AbortController() + mocks.git.mockImplementation(async (_args: string[], options: { cwd?: string }) => { + controller.abort() + return { stdout: options.cwd === '/repo' ? NEW : OLD } + }) + await expect( + refreshPreparedWorktreeTip('/repo', '/prepared', BASE, 'owner', { signal: controller.signal }) + ).rejects.toThrow() + expect(mocks.git).toHaveBeenCalledTimes(2) + expect(mocks.invalidation).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/git/worktree-preparation-tip-refresh.ts b/src/main/git/worktree-preparation-tip-refresh.ts new file mode 100644 index 00000000000..fd8bde49aeb --- /dev/null +++ b/src/main/git/worktree-preparation-tip-refresh.ts @@ -0,0 +1,60 @@ +import { windowsLongPathGitArgs } from '../../shared/windows-long-path-git-args' +import { withRepoRefMaintenancePaused } from './local-repo-ref-maintenance' +import { gitExecFileAsync } from './runner' +import { runWithGitReadCacheInvalidation } from './status' +import { notifyPreparedWorktreeMutation } from './worktree' +import { + gitExecOptions, + resolveWorktreeAddTimeoutMs, + type GitWorktreeExecOptions +} from './worktree-operation-options' +import { + verifyWorktreePreparationLock, + verifyWorktreePreparationLockAtPath +} from './worktree-preparation-lock' + +export async function refreshPreparedWorktreeTip( + repoPath: string, + preparedPath: string, + canonicalBase: string, + lockReason: string, + options: GitWorktreeExecOptions = {} +): Promise { + const refreshOptions = { ...options, timeout: options.timeout ?? resolveWorktreeAddTimeoutMs() } + await withRepoRefMaintenancePaused('worktree-prepare', async () => { + const lockPath = await verifyWorktreePreparationLock(preparedPath, lockReason, refreshOptions) + const [target, prepared] = await Promise.allSettled([ + gitExecFileAsync( + ['rev-parse', '--verify', `${canonicalBase}^{commit}`], + gitExecOptions(repoPath, refreshOptions) + ), + gitExecFileAsync( + ['rev-parse', '--verify', 'HEAD'], + gitExecOptions(preparedPath, refreshOptions) + ) + ]) + if (target.status === 'rejected') { + throw target.reason + } + if (prepared.status === 'rejected') { + throw prepared.reason + } + await verifyWorktreePreparationLockAtPath(lockPath, lockReason, refreshOptions.signal) + const targetHead = target.value.stdout.trim() + if (prepared.value.stdout.trim() === targetHead) { + return + } + try { + // Reset preserves detached HEAD and does not run post-checkout hooks before submit. + await runWithGitReadCacheInvalidation(() => + gitExecFileAsync( + [...windowsLongPathGitArgs(preparedPath), 'reset', '--hard', targetHead], + gitExecOptions(preparedPath, refreshOptions) + ) + ) + await verifyWorktreePreparationLockAtPath(lockPath, lockReason, refreshOptions.signal) + } finally { + notifyPreparedWorktreeMutation(repoPath) + } + }) +} diff --git a/src/main/ipc/created-worktree-reconciliation.test.ts b/src/main/ipc/created-worktree-reconciliation.test.ts index b62088ef105..8d72742920e 100644 --- a/src/main/ipc/created-worktree-reconciliation.test.ts +++ b/src/main/ipc/created-worktree-reconciliation.test.ts @@ -109,25 +109,26 @@ describe('resolveCreatedWorktree', () => { await expect( resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature') ).resolves.toEqual({ created: CREATED, worktrees: [MAIN, CREATED], listingComplete: true }) - expect(describeCreatedWorktree).not.toHaveBeenCalled() + expect(describeCreatedWorktree).toHaveBeenCalledOnce() }) - it('completes the create from the direct read when the listing fails', async () => { - vi.mocked(listWorktreesSharedStrict).mockRejectedValue(new Error('git timed out.')) + it('verifies only the new checkout without listing or probing every existing worktree', async () => { vi.mocked(describeCreatedWorktree).mockResolvedValue(CREATED) await expect( resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature') ).resolves.toEqual({ created: CREATED, worktrees: [], listingComplete: false }) + expect(describeCreatedWorktree).toHaveBeenCalledOnce() + expect(listWorktreesSharedStrict).not.toHaveBeenCalled() }) - it('completes the create from the direct read when the listing omits the row', async () => { - vi.mocked(listWorktreesSharedStrict).mockResolvedValue([MAIN]) - vi.mocked(describeCreatedWorktree).mockResolvedValue(CREATED) + it('uses the whole listing when the direct checkout witness cannot be read', async () => { + vi.mocked(listWorktreesSharedStrict).mockResolvedValue([MAIN, CREATED]) + vi.mocked(describeCreatedWorktree).mockRejectedValue(new Error('rev-parse exploded')) await expect( resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature') - ).resolves.toMatchObject({ created: CREATED, listingComplete: false }) + ).resolves.toEqual({ created: CREATED, worktrees: [MAIN, CREATED], listingComplete: true }) }) it("surfaces the listing's own failure rather than an opaque message", async () => { @@ -186,35 +187,50 @@ describe('resolveCreatedWorktree', () => { }) }) - it('charges the recovery what the listing left of the budget, not a fresh one', async () => { - vi.mocked(listWorktreesSharedStrict).mockImplementation(async () => { + it('charges the listing fallback what the direct read left of the budget', async () => { + vi.mocked(describeCreatedWorktree).mockImplementation(async () => { await new Promise((resolve) => setTimeout(resolve, 60)) - throw new Error('git worktree list timed out.') + return undefined }) - vi.mocked(describeCreatedWorktree).mockResolvedValue(CREATED) + vi.mocked(listWorktreesSharedStrict).mockResolvedValue([CREATED]) await resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature') - const options = vi.mocked(describeCreatedWorktree).mock.lastCall?.[3] + const options = vi.mocked(listWorktreesSharedStrict).mock.lastCall?.[1] expect(options?.timeout).toBeGreaterThanOrEqual(5_000) expect(options?.timeout).toBeLessThan(30_000) }) it("keeps the caller's own deadline instead of the shared budget", async () => { - vi.mocked(listWorktreesSharedStrict).mockRejectedValue(new Error('git worktree list failed.')) - vi.mocked(describeCreatedWorktree).mockResolvedValue(CREATED) + vi.mocked(listWorktreesSharedStrict).mockResolvedValue([CREATED]) await resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature', { timeout: 1_234 }) expect(vi.mocked(describeCreatedWorktree).mock.lastCall?.[3]).toMatchObject({ timeout: 1_234 }) + expect(vi.mocked(listWorktreesSharedStrict).mock.lastCall?.[1]).toMatchObject({ + timeout: 1_234 + }) }) - it('forwards exec options only when the caller supplied them', async () => { + it('forwards execution-host options to both checkout verification and the fallback', async () => { vi.mocked(listWorktreesSharedStrict).mockResolvedValue([CREATED]) await resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature') - expect(listWorktreesSharedStrict).toHaveBeenLastCalledWith('/repo') + expect(describeCreatedWorktree).toHaveBeenLastCalledWith( + '/repo', + '/workspaces/feature', + 'feature' + ) await resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature', { wslDistro: 'Ubuntu' }) - expect(listWorktreesSharedStrict).toHaveBeenLastCalledWith('/repo', { wslDistro: 'Ubuntu' }) + expect(describeCreatedWorktree).toHaveBeenLastCalledWith( + '/repo', + '/workspaces/feature', + 'feature', + { wslDistro: 'Ubuntu' } + ) + expect(listWorktreesSharedStrict).toHaveBeenLastCalledWith( + '/repo', + expect.objectContaining({ wslDistro: 'Ubuntu' }) + ) }) }) diff --git a/src/main/ipc/created-worktree-reconciliation.ts b/src/main/ipc/created-worktree-reconciliation.ts index ac5b79954c2..3369ae6f646 100644 --- a/src/main/ipc/created-worktree-reconciliation.ts +++ b/src/main/ipc/created-worktree-reconciliation.ts @@ -23,7 +23,7 @@ export function findCreatedWorktree export type CreatedWorktreeResolution = { created: GitWorktreeInfo - /** Rows `git worktree list` returned; empty when only the direct read found the worktree. */ + /** Rows `git worktree list` returned; empty when the direct read found the worktree. */ worktrees: readonly GitWorktreeInfo[] /** Whether `worktrees` is the repo's whole listing, and so usable as its authorized-root set. */ listingComplete: boolean @@ -36,16 +36,10 @@ export function createdWorktreeNotFoundError(worktreePath: string, branchName: s ) } -/** - * Find the row for a worktree `git worktree add` just created, preferring the repo listing and - * falling back to asking Git about the worktree itself. - * - * Why the fallback: the listing was the only witness the old code had, so any Git-level listing - * failure failed a create whose worktree and branch were already on disk, orphaning both (#16520). - */ -/** A listing that burned the whole budget still leaves the direct read a chance to answer. */ -const MIN_CREATED_WORKTREE_RECOVERY_MS = 5_000 +/** A failed direct read still leaves the listing a chance to verify the create. */ +const MIN_CREATED_WORKTREE_LIST_FALLBACK_MS = 5_000 +/** Verify the new checkout directly; listing every existing worktree is only a fallback. */ export async function resolveCreatedWorktree( repoPath: string, worktreePath: string, @@ -53,11 +47,28 @@ export async function resolveCreatedWorktree( options?: GitWorktreeExecOptions ): Promise { const startedAt = Date.now() + let directReadError: Error | undefined + try { + const created = options + ? await describeCreatedWorktree(repoPath, worktreePath, branchName, options) + : await describeCreatedWorktree(repoPath, worktreePath, branchName) + if (created) { + return { created, worktrees: [], listingComplete: false } + } + } catch (err) { + directReadError = err instanceof Error ? err : new Error(String(err)) + } + + const remainingMs = Math.max( + (options?.timeout ?? WORKTREE_LIST_TIMEOUT_MS) - (Date.now() - startedAt), + MIN_CREATED_WORKTREE_LIST_FALLBACK_MS + ) let listingError: Error | undefined try { - const worktrees = options - ? await listWorktreesSharedStrict(repoPath, options) - : await listWorktreesSharedStrict(repoPath) + const worktrees = await listWorktreesSharedStrict(repoPath, { + ...options, + timeout: options?.timeout ?? remainingMs + }) const created = findCreatedWorktree(worktrees, worktreePath, branchName) if (created) { return { created, worktrees, listingComplete: true } @@ -65,39 +76,18 @@ export async function resolveCreatedWorktree( } catch (err) { listingError = err instanceof Error ? err : new Error(String(err)) } - - try { - // One budget for verifying the create, not one per attempt: a hung Git already spent the - // listing's deadline, and charging the recovery a fresh one doubles the wait before the error. - const remainingMs = Math.max( - WORKTREE_LIST_TIMEOUT_MS - (Date.now() - startedAt), - MIN_CREATED_WORKTREE_RECOVERY_MS - ) - const described = await describeCreatedWorktree(repoPath, worktreePath, branchName, { - ...options, - timeout: options?.timeout ?? remainingMs - }) - if (described) { - return { created: described, worktrees: [], listingComplete: false } - } - } catch (err) { - if (listingError) { - // The listing's failure stays the thrown one, but the recovery's reason -- often - // `repo common dir unverifiable: ...` -- would otherwise vanish from the record entirely. + if (listingError) { + if (directReadError) { console.warn('[worktrees:create] created-worktree recovery also failed', { - err, + err: directReadError, worktreePath }) - throw listingError } - // The listing simply omitted the row, so the direct read holds the only actionable failure. - const notFound = createdWorktreeNotFoundError(worktreePath, branchName) - throw new Error(`${notFound.message}: ${err instanceof Error ? err.message : String(err)}`, { - cause: err - }) - } - if (listingError) { throw listingError } - throw createdWorktreeNotFoundError(worktreePath, branchName) + const notFound = createdWorktreeNotFoundError(worktreePath, branchName) + if (directReadError) { + throw new Error(`${notFound.message}: ${directReadError.message}`, { cause: directReadError }) + } + throw notFound } diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 256cb87fe9f..11b897b81db 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -371,7 +371,7 @@ export function recordWorkspaceLineageForCreatedWorktree( async function spawnLocalStartupAndSetupTerminals(args: { runtime: OrcaRuntimeService | undefined - worktree: Pick + worktree: Worktree startup: CreateWorktreeArgs['startup'] setup: CreateWorktreeResult['setup'] defaultTabs: CreateWorktreeResult['defaultTabs'] @@ -410,18 +410,22 @@ async function spawnLocalStartupAndSetupTerminals(args: { try { // Why: only after `git worktree add` + metadata registration is the path safe for a runtime PTY to boot the agent while setup runs alongside. - const terminal = await runtime.createTerminal(`id:${worktree.id}`, { - command: sequencedStartup.command, - ...(setup ? { claudeAgentTeamsSourceCommand: startup.command } : {}), - env: sequencedStartup.env, - ...(sequencedStartup.launchConfig ? { launchConfig: sequencedStartup.launchConfig } : {}), - ...(isTuiAgent(createdWithAgent) ? { launchAgent: createdWithAgent } : {}), - ...(sequencedStartup.viewMode ? { viewMode: sequencedStartup.viewMode } : {}), - startupCommandDelivery: sequencedStartup.startupCommandDelivery, - telemetry: sequencedStartup.telemetry, - // Why: the submitting renderer decides whether to open the workspace; activating here yanked users who moved on (#9944). - surfaceOwner: false - }) + const terminal = await runtime.createTerminal( + `id:${worktree.id}`, + { + command: sequencedStartup.command, + ...(setup ? { claudeAgentTeamsSourceCommand: startup.command } : {}), + env: sequencedStartup.env, + ...(sequencedStartup.launchConfig ? { launchConfig: sequencedStartup.launchConfig } : {}), + ...(isTuiAgent(createdWithAgent) ? { launchAgent: createdWithAgent } : {}), + ...(sequencedStartup.viewMode ? { viewMode: sequencedStartup.viewMode } : {}), + startupCommandDelivery: sequencedStartup.startupCommandDelivery, + telemetry: sequencedStartup.telemetry, + // Why: the submitting renderer decides whether to open the workspace; activating here yanked users who moved on (#9944). + surfaceOwner: false + }, + worktree + ) startupTerminalHandle = terminal.handle startupTerminal = { spawned: true, @@ -454,21 +458,29 @@ async function spawnLocalStartupAndSetupTerminals(args: { if (!startupTerminalHandle) { throw new Error('startup_terminal_missing') } - await runtime.splitTerminal(startupTerminalHandle, { - direction: setupLaunchMode === 'split-horizontal' ? 'horizontal' : 'vertical', - command: setupCommand, - env: setup.envVars, - activate: false, - surfaceOwner: false - }) + await runtime.splitTerminal( + startupTerminalHandle, + { + direction: setupLaunchMode === 'split-horizontal' ? 'horizontal' : 'vertical', + command: setupCommand, + env: setup.envVars, + activate: false, + surfaceOwner: false + }, + worktree + ) } else { - await runtime.createTerminal(`id:${worktree.id}`, { - title: 'Setup', - command: setupCommand, - env: setup.envVars, - activate: false, - surfaceOwner: false - }) + await runtime.createTerminal( + `id:${worktree.id}`, + { + title: 'Setup', + command: setupCommand, + env: setup.envVars, + activate: false, + surfaceOwner: false + }, + worktree + ) } didSpawnSetup = true } catch (error) { diff --git a/src/main/ipc/worktrees-local-create-flow.test.ts b/src/main/ipc/worktrees-local-create-flow.test.ts index 7da7756bd11..96e1b804194 100644 --- a/src/main/ipc/worktrees-local-create-flow.test.ts +++ b/src/main/ipc/worktrees-local-create-flow.test.ts @@ -485,7 +485,7 @@ describe('registerWorktreeHandlers', () => { expect(listWorktreesMock).toHaveBeenCalledTimes(listWorktreesCallsAfterCreate) }) - it('completes a create the listing failed and keeps sibling worktrees authorized', async () => { + it('verifies a create without re-listing and keeps sibling worktrees authorized', async () => { const sibling = { path: '/workspace/existing-sibling', head: 'sib123', @@ -504,8 +504,9 @@ describe('registerWorktreeHandlers', () => { sibling ]) await handlers['worktrees:create'](null, { repoId: 'repo-1', name: 'existing-sibling' }) + const listingCalls = listWorktreesMock.mock.calls.length - // The create Git could no longer list, recovered by reading the worktree directly. + // Only the new checkout needs verification, even when the full listing is unavailable. listWorktreesMock.mockRejectedValue(new Error('git worktree list timed out.')) describeCreatedWorktreeMock.mockResolvedValue({ path: '/workspace/improve-dashboard', @@ -526,6 +527,7 @@ describe('registerWorktreeHandlers', () => { await expect( resolveRegisteredWorktreePath('/workspace/improve-dashboard', store as never) ).resolves.toBe(resolve('/workspace/improve-dashboard')) + expect(listWorktreesMock).toHaveBeenCalledTimes(listingCalls) }) it('uses branchNameOverride for the git branch while keeping the sanitized worktree path', async () => { @@ -687,7 +689,8 @@ describe('registerWorktreeHandlers', () => { request_kind: 'new' }, surfaceOwner: false - } + }, + expect.objectContaining({ id: 'repo-1::/workspace/improve-dashboard' }) ) expect(runtimeStub.createTerminal).toHaveBeenNthCalledWith( 2, @@ -701,7 +704,8 @@ describe('registerWorktreeHandlers', () => { }, activate: false, surfaceOwner: false - } + }, + expect.objectContaining({ id: 'repo-1::/workspace/improve-dashboard' }) ) const startupCreateCall = runtimeStub.createTerminal.mock.calls[0] const setupCreateCall = runtimeStub.createTerminal.mock.calls[1] @@ -818,13 +822,17 @@ describe('registerWorktreeHandlers', () => { expect(runtimeStub.createTerminal).toHaveBeenCalledTimes(1) // A user who moved on must not be scrolled to the new workspace by its setup pane (#9944). - expect(runtimeStub.splitTerminal).toHaveBeenCalledWith('term-startup', { - direction: 'vertical', - command: expect.stringContaining('setup-runner.sh'), - env: expect.any(Object), - activate: false, - surfaceOwner: false - }) + expect(runtimeStub.splitTerminal).toHaveBeenCalledWith( + 'term-startup', + { + direction: 'vertical', + command: expect.stringContaining('setup-runner.sh'), + env: expect.any(Object), + activate: false, + surfaceOwner: false + }, + expect.objectContaining({ id: 'repo-1::/workspace/improve-dashboard' }) + ) }) it('rejects ask-policy creates before mutating git state when setup decision is missing', async () => { diff --git a/src/main/ipc/worktrees-wsl-runtime-routing.test.ts b/src/main/ipc/worktrees-wsl-runtime-routing.test.ts index 1708a561c23..029ffc6171a 100644 --- a/src/main/ipc/worktrees-wsl-runtime-routing.test.ts +++ b/src/main/ipc/worktrees-wsl-runtime-routing.test.ts @@ -211,9 +211,10 @@ describe('registerWorktreeHandlers', () => { 'origin/main', { wslDistro: 'Ubuntu' } ) - expect(listWorktreesMock).toHaveBeenCalledWith('/workspace/repo', { - wslDistro: 'Ubuntu' - }) + expect(listWorktreesMock).toHaveBeenCalledWith( + '/workspace/repo', + expect.objectContaining({ wslDistro: 'Ubuntu' }) + ) expectEveryGitCallRoutedTo('Ubuntu') }) diff --git a/src/main/ipc/worktrees/create/register-worktree-prefetch-handler.ts b/src/main/ipc/worktrees/create/register-worktree-prefetch-handler.ts index c1bcbbf3d59..bbc7eb2b25e 100644 --- a/src/main/ipc/worktrees/create/register-worktree-prefetch-handler.ts +++ b/src/main/ipc/worktrees/create/register-worktree-prefetch-handler.ts @@ -20,7 +20,8 @@ export function registerWorktreePrefetchHandler(context: WorktreeIpcContext): vo baseBranch: args.baseBranch, runtime, gitOptions: getWorktreeCreatePrefetchGitOptions(store, repo), - prepareCheckout: (base) => prepareWorktreeCreateForRepo(store, repo, base) + prepareCheckout: (base, beforeMaterialization) => + prepareWorktreeCreateForRepo(store, repo, base, beforeMaterialization) }) } catch { // Why: optimistic warm-up; the real create path awaits the same refresh and reports failures there. diff --git a/src/main/runtime/orca-runtime-activate-managed-worktree.ts b/src/main/runtime/orca-runtime-activate-managed-worktree.ts index 1629c529921..2f7e59a6d1f 100644 --- a/src/main/runtime/orca-runtime-activate-managed-worktree.ts +++ b/src/main/runtime/orca-runtime-activate-managed-worktree.ts @@ -204,25 +204,31 @@ export class OrcaRuntimeWithActivateManagedWorktree extends OrcaRuntimeWithListM sendWorktreeStartupFollowupWhenReady(this.getWorktreeStartupReadinessHost(), handle, followup) } - protected async provisionManagedWorktreeTerminals(args: { - worktreeSelector: string - worktreeId: string - worktreePath: string - setup?: CreateWorktreeResult['setup'] - defaultTabs?: CreateWorktreeResult['defaultTabs'] - primaryTerminalHandle?: string | null - hasStartupTerminal: boolean - setupCommandPlatform: 'windows' | 'posix' - observeSetupCompletion?: boolean - // Why: when the agent startup is sequenced to wait for setup - // (waitForAgentStartup), the startup PTY runs a wrapper that already embeds - // the setup command. Pass that wrapped command through so the Setup tab runs - // the same script the agent is waiting on instead of a bare runner. - wrappedSetupCommand?: string - // Why: a workspace provisioned in the background must not pull the sidebar - // to itself; the user never asked to look at these tabs. - surfaceOwner?: false - }): Promise<{ setupSpawned: boolean; setupTerminalHandle: string | null }> { - return provisionWorktreeTerminals(this.getWorktreeTerminalProvisioningHost(), args) + protected async provisionManagedWorktreeTerminals( + args: { + worktreeSelector: string + worktreeId: string + worktreePath: string + setup?: CreateWorktreeResult['setup'] + defaultTabs?: CreateWorktreeResult['defaultTabs'] + primaryTerminalHandle?: string | null + hasStartupTerminal: boolean + setupCommandPlatform: 'windows' | 'posix' + observeSetupCompletion?: boolean + // Why: when the agent startup is sequenced to wait for setup + // (waitForAgentStartup), the startup PTY runs a wrapper that already embeds + // the setup command. Pass that wrapped command through so the Setup tab runs + // the same script the agent is waiting on instead of a bare runner. + wrappedSetupCommand?: string + // Why: a workspace provisioned in the background must not pull the sidebar + // to itself; the user never asked to look at these tabs. + surfaceOwner?: false + }, + createdWorktree?: Worktree + ): Promise<{ setupSpawned: boolean; setupTerminalHandle: string | null }> { + return provisionWorktreeTerminals( + this.getWorktreeTerminalProvisioningHost(createdWorktree), + args + ) } } diff --git a/src/main/runtime/orca-runtime-create-base-prefetch.test.ts b/src/main/runtime/orca-runtime-create-base-prefetch.test.ts index d65209b9cc4..4390ded16d6 100644 --- a/src/main/runtime/orca-runtime-create-base-prefetch.test.ts +++ b/src/main/runtime/orca-runtime-create-base-prefetch.test.ts @@ -118,7 +118,8 @@ describe('prefetchManagedWorktreeCreateBase (orca-runtime-get-worktree-terminal- expect(mocks.prepareWorktreeCreateForRepo).toHaveBeenCalledWith( expect.anything(), repo, - 'origin/main' + 'origin/main', + undefined ) }) }) diff --git a/src/main/runtime/orca-runtime-create-managed-worktree.ts b/src/main/runtime/orca-runtime-create-managed-worktree.ts index d0885a1dfe1..ce2e4d1bc2f 100644 --- a/src/main/runtime/orca-runtime-create-managed-worktree.ts +++ b/src/main/runtime/orca-runtime-create-managed-worktree.ts @@ -241,10 +241,10 @@ export class OrcaRuntimeWithCreateManagedWorktree extends OrcaRuntimeWithGetWork warning, ports: { canSpawn: Boolean(this.ptyController?.spawn), - createTerminal: (selector, options) => this.createTerminal(selector, options), + createTerminal: (selector, options) => this.createTerminal(selector, options, worktree), pasteDraft: (handle, draft) => this.pasteStartupDraftWhenReady(handle, draft), sendFollowup: (handle, followup) => this.sendStartupFollowupWhenReady(handle, followup), - provision: (options) => this.provisionManagedWorktreeTerminals(options), + provision: (options) => this.provisionManagedWorktreeTerminals(options, worktree), activate: (repoId, worktreeId, activationSetup, startup, activationDefaultTabs) => this.notifyActivateWorktree( repoId, diff --git a/src/main/runtime/orca-runtime-create-terminal-dependencies.ts b/src/main/runtime/orca-runtime-create-terminal-dependencies.ts index 20172740460..9175fa8dec5 100644 --- a/src/main/runtime/orca-runtime-create-terminal-dependencies.ts +++ b/src/main/runtime/orca-runtime-create-terminal-dependencies.ts @@ -1,14 +1,17 @@ export type { TerminalCreateOptions } from './runtime-terminal-contracts' +export type { Worktree } from '../../shared/worktree/types' export type { RuntimeTerminalCreate } from '../../shared/runtime-types' export { createTerminalRevealWarning, ownerSurfacing, resolveTerminalPresentation } from './orca-runtime-core' -export { isValidHostTerminalTabId } from '../../shared/terminal-tab-id' -export { isTerminalLeafId, makePaneKey } from '../../shared/stable-pane-id' +export { makePaneKey } from '../../shared/stable-pane-id' export { randomUUID } from 'node:crypto' -export { admitStablePaneAdoption } from './runtime-terminal-pane-identity' +export { + admitStablePaneAdoption, + allocateTerminalPaneIdentity +} from './runtime-terminal-pane-identity' export { copySleepingAgentLaunchConfig, inferCapturedClaudeAgentTeamsMode, diff --git a/src/main/runtime/orca-runtime-create-terminal-desktop.ts b/src/main/runtime/orca-runtime-create-terminal-desktop.ts index 9cd955428ee..6a56bff56b0 100644 --- a/src/main/runtime/orca-runtime-create-terminal-desktop.ts +++ b/src/main/runtime/orca-runtime-create-terminal-desktop.ts @@ -2,18 +2,20 @@ import * as dependencies from './orca-runtime-create-terminal-dependencies' import type { OrcaRuntimeWithCreateTerminal } from './orca-runtime-create-terminal' import type { RuntimeTerminalPresentation } from '../../shared/runtime-types' +import type { Worktree } from '../../shared/worktree/types' export async function createDesktopTerminal( runtime: OrcaRuntimeWithCreateTerminal, worktreeSelector: string | undefined, opts: dependencies.TerminalCreateOptions, presentation: RuntimeTerminalPresentation | undefined, - rendererWindow: Electron.BrowserWindow | null + rendererWindow: Electron.BrowserWindow | null, + createdWorktree?: Worktree ): Promise { runtime.assertGraphReady() const win = rendererWindow ?? runtime.getAuthoritativeWindow() const workspace = worktreeSelector - ? await runtime.resolveTerminalWorkspaceLaunchScope(worktreeSelector) + ? await runtime.resolveTerminalWorkspaceLaunchScope(worktreeSelector, createdWorktree) : null const launchOpts = workspace ? await runtime.resolveAgentTerminalCreateOptions(workspace, opts) diff --git a/src/main/runtime/orca-runtime-create-terminal.ts b/src/main/runtime/orca-runtime-create-terminal.ts index 242e0f7f454..c8f965fcf7e 100644 --- a/src/main/runtime/orca-runtime-create-terminal.ts +++ b/src/main/runtime/orca-runtime-create-terminal.ts @@ -9,14 +9,15 @@ import { recordPtySurface, spawnSurfaceClaimSequence } from './pty-recorded-surf export class OrcaRuntimeWithCreateTerminal extends OrcaRuntimeWithTerminalCreateDeduplication { async createTerminal( worktreeSelector?: string, - opts: dependencies.TerminalCreateOptions = {} + opts: dependencies.TerminalCreateOptions = {}, + // Internal creation evidence; RPC and preload callers never supply it. + created?: dependencies.Worktree ): Promise { if (opts.startupAgent && worktreeSelector === undefined) { throw new Error(`startupAgent ${opts.startupAgent} requires a workspace selector.`) } const callerColors = dependencies.normalizeColorQueryReplyColors(opts.terminalColorQueryReplies) - // Why: only a paired client sends colours here; a client that predates - // terminal.setViewerColors reports its theme to a headless host only this way. + // Older paired clients report their theme only at creation. if (callerColors) { dependencies.setPairedViewerColors(callerColors) } @@ -33,22 +34,14 @@ export class OrcaRuntimeWithCreateTerminal extends OrcaRuntimeWithTerminalCreate if (!this.ptyController?.spawn) { throw new Error('runtime_unavailable') } - const workspace = await this.resolveTerminalWorkspaceLaunchScope(worktreeSelector) + const workspace = await this.resolveTerminalWorkspaceLaunchScope(worktreeSelector, created) const launchOpts = await this.resolveAgentTerminalCreateOptions(workspace, opts) const reportPtySpawnCommitted = createPtySpawnCommitReporter(launchOpts.onPtySpawnCommitted) const cwd = this.resolveWorkspaceTerminalStartupCwd(workspace, launchOpts.cwd) ?? workspace.path let preAllocatedHandle = launchOpts.preAllocatedHandle ?? this.createPreAllocatedTerminalHandle() - const hintedTabId = launchOpts.tabId?.trim() - const canAdoptPaneIdentity = - hintedTabId !== undefined && - dependencies.isValidHostTerminalTabId(hintedTabId) && - launchOpts.leafId !== undefined && - dependencies.isTerminalLeafId(launchOpts.leafId) - let tabId = canAdoptPaneIdentity ? (hintedTabId as string) : dependencies.randomUUID() - let leafId = canAdoptPaneIdentity ? (launchOpts.leafId as string) : dependencies.randomUUID() - let paneKey = dependencies.makePaneKey(tabId, leafId) + let { tabId, leafId, paneKey } = dependencies.allocateTerminalPaneIdentity(launchOpts) const claimedStablePaneCreate = this.ptyController.claimStablePaneCreate?.({ worktreeId: workspace.id, connectionId: workspace.connectionId, @@ -299,6 +292,13 @@ export class OrcaRuntimeWithCreateTerminal extends OrcaRuntimeWithTerminalCreate } // The renderer owns this spawn, so this process cannot see when it is requested. opts.onPtySpawnDispatched?.() - return createDesktopTerminal(this, worktreeSelector, opts, presentation, rendererWindow) + return createDesktopTerminal( + this, + worktreeSelector, + opts, + presentation, + rendererWindow, + created + ) } } diff --git a/src/main/runtime/orca-runtime-get-worktree-terminal-provisioning-host.ts b/src/main/runtime/orca-runtime-get-worktree-terminal-provisioning-host.ts index 7f42304eb8b..30542bc6ff8 100644 --- a/src/main/runtime/orca-runtime-get-worktree-terminal-provisioning-host.ts +++ b/src/main/runtime/orca-runtime-get-worktree-terminal-provisioning-host.ts @@ -1,23 +1,21 @@ // @ts-nocheck -- mechanically split from OrcaRuntimeService; behavior is covered by AST equivalence and characterization tests. import { OrcaRuntimeWithActivateManagedWorktree } from './orca-runtime-activate-managed-worktree' -import type { - WorktreeProvisionTerminalOptions, - WorktreeTerminalProvisioningHost -} from './runtime-worktree-terminal-provisioning' -import type { TerminalCreateOptions } from './runtime-terminal-contracts' +import type { WorktreeTerminalProvisioningHost } from './runtime-worktree-terminal-provisioning' import type { WorktreeStartupReadinessHost } from './runtime-worktree-startup-readiness' import { prefetchWorktreeCreateBase } from '../worktree-create-base-prefetch' import { prepareWorktreeCreateForRepo } from '../worktree-create-preparation' import { getWorktreeCreatePrefetchGitOptions } from '../project-runtime-git-options' +import type { Worktree } from '../../shared/worktree/types' export class OrcaRuntimeWithGetWorktreeTerminalProvisioningHost extends OrcaRuntimeWithActivateManagedWorktree { - protected getWorktreeTerminalProvisioningHost(): WorktreeTerminalProvisioningHost { + protected getWorktreeTerminalProvisioningHost( + createdWorktree?: Worktree + ): WorktreeTerminalProvisioningHost { return { canSpawn: () => Boolean(this.ptyController?.spawn), createTerminal: (selector, options) => - this.createTerminal(selector, options as TerminalCreateOptions), - splitTerminal: (handle, options) => - this.splitTerminal(handle, options as WorktreeProvisionTerminalOptions), + this.createTerminal(selector, options, createdWorktree), + splitTerminal: (handle, options) => this.splitTerminal(handle, options, createdWorktree), setTabColor: async (worktreeId, tabId, color) => { await this.setMobileSessionTabProps(`id:${worktreeId}`, { tabId, color }) }, @@ -55,7 +53,8 @@ export class OrcaRuntimeWithGetWorktreeTerminalProvisioningHost extends OrcaRunt baseBranch: args.baseBranch, runtime: this, gitOptions: getWorktreeCreatePrefetchGitOptions(store, repo), - prepareCheckout: (base) => prepareWorktreeCreateForRepo(store, repo, base) + prepareCheckout: (base, beforeMaterialization) => + prepareWorktreeCreateForRepo(store, repo, base, beforeMaterialization) }) } } diff --git a/src/main/runtime/orca-runtime-resolve-browser-network-execution-host-for-worktree.ts b/src/main/runtime/orca-runtime-resolve-browser-network-execution-host-for-worktree.ts index decc28aa45b..960e36bd78d 100644 --- a/src/main/runtime/orca-runtime-resolve-browser-network-execution-host-for-worktree.ts +++ b/src/main/runtime/orca-runtime-resolve-browser-network-execution-host-for-worktree.ts @@ -25,6 +25,8 @@ import { getExplicitWorktreeIdSelector } from './runtime-worktree-selection' import { WORKTREE_ID_SEPARATOR } from '../../shared/worktree/id' import { WorktreeIdRequiresFullPathError } from './runtime-worktree-lineage-resolution' import { triggerTerminalSpawnPushTargetMaterialization } from './runtime-terminal-spawn-push-target-materialization' +import type { Worktree } from '../../shared/worktree/types' +import { resolveCreatedWorktreeTerminalTarget } from './runtime-created-worktree-terminal-target' export class OrcaRuntimeWithResolveBrowserNetworkExecutionHostForWorktree extends OrcaRuntimeWithTransitionGraphReloadToTerminalState { protected resolveBrowserNetworkExecutionHostForWorktree(worktree?: { @@ -88,13 +90,15 @@ export class OrcaRuntimeWithResolveBrowserNetworkExecutionHostForWorktree extend } protected async resolveTerminalWorkspaceLaunchScope( - selector: string + selector: string, + createdWorktree?: Worktree ): Promise { - return (await this.resolveTerminalWorkspaceLaunchTarget(selector)).scope + return (await this.resolveTerminalWorkspaceLaunchTarget(selector, createdWorktree)).scope } protected async resolveTerminalWorkspaceLaunchTarget( - selector: string + selector: string, + createdWorktree?: Worktree ): Promise { const floatingTerminalSelector = selector === FLOATING_TERMINAL_WORKTREE_ID || @@ -124,7 +128,9 @@ export class OrcaRuntimeWithResolveBrowserNetworkExecutionHostForWorktree extend const workspaceSelector = selector.startsWith('id:') ? selector.slice(3) : selector const parsed = parseWorkspaceKey(workspaceSelector) const worktreeSelector = parsed?.type === 'worktree' ? `id:${parsed.worktreeId}` : selector - const worktree = await this.resolveWorktreeSelector(worktreeSelector) + const worktree = + resolveCreatedWorktreeTerminalTarget(this.store, selector, createdWorktree) ?? + (await this.resolveWorktreeSelector(worktreeSelector)) // Why: `getRepo(id)` is host-blind and the same repo id can exist on local, SSH and runtime // hosts. Reading `connectionId` off an arbitrary row reports "local" for a remote worktree and // spawns its PTY on the client with the remote cwd (#11163). Loss of a usable answer is diff --git a/src/main/runtime/orca-runtime-split-pty-backed-terminal.ts b/src/main/runtime/orca-runtime-split-pty-backed-terminal.ts index 8fe43de6950..6b9fd364b04 100644 --- a/src/main/runtime/orca-runtime-split-pty-backed-terminal.ts +++ b/src/main/runtime/orca-runtime-split-pty-backed-terminal.ts @@ -7,6 +7,7 @@ import { makePaneKey, parsePaneKey } from '../../shared/stable-pane-id' import { recordPtySurface, spawnSurfaceClaimSequence } from './pty-recorded-surface-topology' import { randomUUID } from 'node:crypto' import { REJECTED_SPLIT_PTY_STOP_TIMEOUT_MS, ownerSurfacing } from './orca-runtime-core' +import type { Worktree } from '../../shared/worktree/types' export class OrcaRuntimeWithSplitPtyBackedTerminal extends OrcaRuntimeWithSplitTerminal { protected async splitPtyBackedTerminal( @@ -21,7 +22,8 @@ export class OrcaRuntimeWithSplitPtyBackedTerminal extends OrcaRuntimeWithSplitT // workspace, for splits the user never asked to see. surfaceOwner?: false telemetrySource?: TerminalPaneSplitSource - } = {} + } = {}, + createdWorktree?: Worktree ): Promise { if (!this.ptyController?.spawn) { throw new Error('runtime_unavailable') @@ -35,7 +37,10 @@ export class OrcaRuntimeWithSplitPtyBackedTerminal extends OrcaRuntimeWithSplitT throw new Error('terminal_handle_stale') } const direction = opts.direction ?? 'horizontal' - const workspace = await this.resolveTerminalWorkspaceLaunchScope(`id:${pty.worktreeId}`) + const workspace = await this.resolveTerminalWorkspaceLaunchScope( + `id:${pty.worktreeId}`, + createdWorktree + ) const sourceAuthority = this.resolveTerminalSplitSourceAuthority( workspace.id, parentTabId, diff --git a/src/main/runtime/orca-runtime-split-terminal.ts b/src/main/runtime/orca-runtime-split-terminal.ts index f259f56e1a6..7dc006ef24e 100644 --- a/src/main/runtime/orca-runtime-split-terminal.ts +++ b/src/main/runtime/orca-runtime-split-terminal.ts @@ -3,6 +3,7 @@ import { OrcaRuntimeWithStopExplicitlyClosedTabPtys } from './orca-runtime-stop- import type { TerminalPaneSplitSource } from '../../shared/feature-education-telemetry' import type { RuntimeTerminalSplit } from '../../shared/runtime-types' import { randomUUID } from 'node:crypto' +import type { Worktree } from '../../shared/worktree/types' export class OrcaRuntimeWithSplitTerminal extends OrcaRuntimeWithStopExplicitlyClosedTabPtys { async splitTerminal( @@ -17,11 +18,13 @@ export class OrcaRuntimeWithSplitTerminal extends OrcaRuntimeWithStopExplicitlyC // workspace, for splits the user never asked to see. surfaceOwner?: false telemetrySource?: TerminalPaneSplitSource - } = {} + } = {}, + // Internal creation evidence; RPC and preload callers never supply it. + createdWorktree?: Worktree ): Promise { const livePty = this.getLivePtyForHandle(handle) if (livePty) { - return await this.splitPtyBackedTerminal(livePty.pty, opts) + return await this.splitPtyBackedTerminal(livePty.pty, opts, createdWorktree) } this.assertGraphReady() const { leaf } = this.getLiveLeafForHandle(handle) diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation.spec.ts index 6d57069feab..79ff0be6698 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation.spec.ts @@ -6,6 +6,7 @@ import { closeLocalWatcherForWorktreePathMock, computeWorktreePathMock, deleteWorktreeHistoryDirMock, + describeCreatedWorktree, ensurePathWithinWorkspaceMock, findExistingWorktreeSymlinkPathsMock, forgetLocalWatcherRemovalSnapshotMock, @@ -330,7 +331,7 @@ describe('OrcaRuntimeService', () => { } computeWorktreePathMock.mockReturnValue(createdWorktree.path) ensurePathWithinWorkspaceMock.mockReturnValue(createdWorktree.path) - vi.mocked(listWorktrees).mockResolvedValue([createdWorktree]) + vi.mocked(describeCreatedWorktree).mockResolvedValue(createdWorktree) const gitSpy = vi.spyOn(gitRunner, 'gitExecFileAsync').mockImplementation(async (args) => { if (args[0] === 'symbolic-ref') { return { stdout: 'refs/remotes/origin/main\n', stderr: '' } @@ -353,6 +354,7 @@ describe('OrcaRuntimeService', () => { return { stdout: '', stderr: '' } }) + const inventoryCallsBefore = vi.mocked(listWorktrees).mock.calls.length try { const result = await runtime.createManagedWorktree({ repoSelector: 'id:repo-1', @@ -446,7 +448,13 @@ describe('OrcaRuntimeService', () => { branchName: 'contributor/runtime-wsl', remoteUrl: 'git@github.com:contributor/orca.git' }) - expect(listWorktrees).toHaveBeenCalledWith(TEST_REPO_PATH, { wslDistro: 'Ubuntu' }) + expect(describeCreatedWorktree).toHaveBeenCalledWith( + TEST_REPO_PATH, + createdWorktree.path, + 'runtime-wsl', + { wslDistro: 'Ubuntu' } + ) + expect(listWorktrees).toHaveBeenCalledTimes(inventoryCallsBefore) } finally { gitSpy.mockRestore() } diff --git a/src/main/runtime/orca-runtime-tests/worktree-setup-and-startup-part-02.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-setup-and-startup-part-02.spec.ts index 6a0a99f4ee8..eca4bd12514 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-setup-and-startup-part-02.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-setup-and-startup-part-02.spec.ts @@ -94,7 +94,13 @@ describe('OrcaRuntimeService', () => { expect(runHook).not.toHaveBeenCalled() expect(createTerminal).toHaveBeenCalledWith( `id:${result.worktree.id}`, - expect.objectContaining({ viewMode: 'chat' }) + expect.objectContaining({ viewMode: 'chat' }), + expect.objectContaining({ + id: result.worktree.id, + path: result.worktree.path, + repoId: result.worktree.repoId, + identity: result.worktree.identity + }) ) // Why: setup is provisioned fire-and-forget; the wait-for-setup guarantee comes from the shell nonce/marker, not JS spawn ordering. await vi.waitFor(() => expect(spawn).toHaveBeenCalledTimes(2)) diff --git a/src/main/runtime/runtime-created-worktree-terminal-target.test.ts b/src/main/runtime/runtime-created-worktree-terminal-target.test.ts new file mode 100644 index 00000000000..dd7162e98c1 --- /dev/null +++ b/src/main/runtime/runtime-created-worktree-terminal-target.test.ts @@ -0,0 +1,238 @@ +import { describe, expect, it, vi } from 'vitest' +import type { ExecutionHostId } from '../../shared/execution-host' +import type { Repo } from '../../shared/repo-types' +import type { WorktreeMeta } from '../../shared/worktree/meta-types' +import type { Worktree } from '../../shared/worktree/types' +import type { WorktreeLineage } from '../../shared/worktree/lineage-types' +import type { RuntimeStore } from './runtime-store-contract' +import type { ResolvedWorktree } from './runtime-worktree-path-identity' +import { mergeWorktree } from '../ipc/worktree-metadata-merge' +import { resolveCreatedWorktreeTerminalTarget } from './runtime-created-worktree-terminal-target' + +vi.mock('electron', () => ({ + BrowserWindow: { fromId: vi.fn(() => null) }, + webContents: { fromId: vi.fn(() => null) }, + ipcMain: { on: vi.fn(), removeListener: vi.fn() }, + app: { getPath: vi.fn(() => '/tmp'), isPackaged: false } +})) + +import { OrcaRuntimeService } from './orca-runtime' + +class StartupRuntime extends OrcaRuntimeService { + readonly lookup = vi.fn<(selector: string) => Promise>() + + protected override resolveWorktreeSelector(selector: string): Promise { + return this.lookup(selector) + } +} + +function owner(hostId: ExecutionHostId = 'local'): Repo { + return { + id: 'repo-1', + path: '/repos/app', + displayName: 'app', + badgeColor: 'blue', + addedAt: 1, + executionHostId: hostId + } +} + +function makeFixture(repos: Repo[] = [owner()], hostId: ExecutionHostId = 'local') { + const meta: WorktreeMeta = { + instanceId: 'new-instance', + hostId, + displayName: '', + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0 + } + const worktree = mergeWorktree( + 'repo-1', + { + path: '/workspaces/new', + head: 'abc123', + branch: 'refs/heads/new', + isBare: false, + isMainWorktree: false + }, + meta + ) + const metadata: Record = { [worktree.id]: meta } + const lineageById: Record = {} + const store: RuntimeStore = { + getRepos: () => repos, + getRepo: (id) => repos.find((repo) => repo.id === id), + addRepo: vi.fn(), + updateRepo: vi.fn(), + getAllWorktreeMeta: () => metadata, + getWorktreeMeta: (id) => metadata[id], + getAllWorktreeLineage: () => lineageById, + setWorktreeMeta: (id, patch) => (metadata[id] = { ...meta, ...metadata[id], ...patch }), + removeWorktreeMeta: (id) => { + delete metadata[id] + }, + getGitHubCache: vi.fn(), + getSettings: () => ({ + workspaceDir: '/workspaces', + nestWorkspaces: false, + refreshLocalBaseRefOnWorktreeCreate: false, + branchPrefix: 'none', + branchPrefixCustom: '' + }) + } + const runtime = new StartupRuntime(store) + const fallback: ResolvedWorktree = { + ...worktree, + git: worktree, + parentWorktreeId: null, + childWorktreeIds: [], + lineage: null + } + runtime.lookup.mockResolvedValue(fallback) + let ptySequence = 0 + const spawn = vi.fn(async () => ({ id: `new-pty-${++ptySequence}` })) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + return { runtime, store, worktree, meta, metadata, lineageById, spawn } +} + +describe('created worktree terminal evidence', () => { + it('starts a terminal in the new checkout without querying the worktree inventory', async () => { + const { runtime, worktree, spawn } = makeFixture() + await runtime.createTerminal(`id:${worktree.id}`, { presentation: 'background' }, worktree) + expect(runtime.lookup).not.toHaveBeenCalled() + expect(spawn).toHaveBeenCalledWith( + expect.objectContaining({ + cwd: worktree.path, + worktreeId: worktree.id, + connectionId: null, + initiallyHidden: true + }) + ) + }) + + it('still resolves normally without internal creation evidence', async () => { + const { runtime, worktree } = makeFixture() + await runtime.createTerminal(`id:${worktree.id}`, { presentation: 'background' }) + expect(runtime.lookup).toHaveBeenCalledOnce() + }) + + it('starts a setup split without another worktree inventory query', async () => { + const { runtime, worktree, spawn } = makeFixture() + const terminal = await runtime.createTerminal( + `id:${worktree.id}`, + { presentation: 'background' }, + worktree + ) + await runtime.splitTerminal(terminal.handle, { surfaceOwner: false }, worktree) + expect(runtime.lookup).not.toHaveBeenCalled() + expect(spawn).toHaveBeenCalledTimes(2) + expect(spawn).toHaveBeenLastCalledWith( + expect.objectContaining({ cwd: worktree.path, worktreeId: worktree.id, connectionId: null }) + ) + }) + + it('uses ordinary resolution when the created instance has been replaced', async () => { + const { runtime, worktree, metadata } = makeFixture() + metadata[worktree.id] = { ...metadata[worktree.id]!, instanceId: 'replacement' } + await runtime.createTerminal(`id:${worktree.id}`, { presentation: 'background' }, worktree) + expect(runtime.lookup).toHaveBeenCalledOnce() + }) + + it('does not reuse creation evidence for a different selector or a replaced instance', async () => { + const { store, worktree, metadata } = makeFixture() + expect(resolveCreatedWorktreeTerminalTarget(store, 'branch:new', worktree)).toBeNull() + expect( + resolveCreatedWorktreeTerminalTarget(store, 'id:repo-1::/elsewhere', worktree) + ).toBeNull() + metadata[worktree.id] = { ...metadata[worktree.id]!, instanceId: 'replacement' } + expect(resolveCreatedWorktreeTerminalTarget(store, `id:${worktree.id}`, worktree)).toBeNull() + }) + + it('routes an SSH-owned created checkout through its host when repo ids collide', async () => { + const { runtime, worktree, spawn } = makeFixture([owner(), owner('ssh:builder')], 'ssh:builder') + await runtime.createTerminal(`id:${worktree.id}`, { presentation: 'background' }, worktree) + expect(runtime.lookup).not.toHaveBeenCalled() + expect(spawn).toHaveBeenCalledWith(expect.objectContaining({ connectionId: 'builder' })) + }) + + it('checks the owner host metadata when a rival host shares the same locator', async () => { + const { runtime, store, worktree, meta, metadata, spawn } = makeFixture( + [owner(), owner('ssh:builder')], + 'ssh:builder' + ) + Object.assign(store, { + getWorktreeMetaForHost: (id: string, hostId: ExecutionHostId) => + id === worktree.id && hostId === 'ssh:builder' ? meta : undefined + }) + metadata[worktree.id] = { ...meta, hostId: 'local', instanceId: 'rival-instance' } + await runtime.createTerminal(`id:${worktree.id}`, { presentation: 'background' }, worktree) + expect(runtime.lookup).not.toHaveBeenCalled() + expect(spawn).toHaveBeenCalledWith(expect.objectContaining({ connectionId: 'builder' })) + }) + + it('rejects another host metadata and ambiguous legacy ownership', async () => { + const { store, worktree, metadata } = makeFixture([owner(), owner('ssh:builder')]) + metadata[worktree.id] = { ...metadata[worktree.id]!, hostId: 'ssh:builder' } + expect(resolveCreatedWorktreeTerminalTarget(store, `id:${worktree.id}`, worktree)).toBeNull() + metadata[worktree.id] = { ...metadata[worktree.id]!, hostId: undefined } + const unstamped: Worktree = { ...worktree, hostId: undefined } + expect(resolveCreatedWorktreeTerminalTarget(store, `id:${worktree.id}`, unstamped)).toBeNull() + expect(resolveCreatedWorktreeTerminalTarget(store, `id:${worktree.id}`, worktree)).toBeNull() + }) + + it('keeps folder workspaces on their existing launch lookup', () => { + const { store, worktree } = makeFixture([{ ...owner(), kind: 'folder' }]) + expect(resolveCreatedWorktreeTerminalTarget(store, `id:${worktree.id}`, worktree)).toBeNull() + }) + + it('preserves stored lineage while rejecting stale parents and cycles', () => { + const { store, worktree, meta, metadata, lineageById } = makeFixture() + const parentId = 'repo-1::/workspaces/parent' + const childId = 'repo-1::/workspaces/child' + metadata[parentId] = { ...meta, instanceId: 'parent-instance' } + metadata[childId] = { ...meta, instanceId: 'child-instance' } + const lineage: WorktreeLineage = { + worktreeId: worktree.id, + worktreeInstanceId: 'new-instance', + parentWorktreeId: parentId, + parentWorktreeInstanceId: 'parent-instance', + origin: 'cli', + capture: { source: 'explicit-cli-flag', confidence: 'explicit' }, + createdAt: 1 + } + lineageById[worktree.id] = lineage + lineageById[childId] = { + ...lineage, + worktreeId: childId, + worktreeInstanceId: 'child-instance', + parentWorktreeId: worktree.id, + parentWorktreeInstanceId: 'new-instance' + } + const resolve = () => resolveCreatedWorktreeTerminalTarget(store, `id:${worktree.id}`, worktree) + expect(resolve()).toEqual( + expect.objectContaining({ parentWorktreeId: parentId, childWorktreeIds: [childId], lineage }) + ) + metadata[parentId] = { ...meta, instanceId: 'replacement' } + expect(resolve()).toEqual(expect.objectContaining({ parentWorktreeId: null, lineage: null })) + metadata[parentId] = { ...meta, instanceId: 'parent-instance' } + lineageById[parentId] = { + ...lineage, + worktreeId: parentId, + worktreeInstanceId: 'parent-instance', + parentWorktreeId: worktree.id, + parentWorktreeInstanceId: 'new-instance' + } + expect(resolve()).toEqual(expect.objectContaining({ parentWorktreeId: null, lineage: null })) + }) +}) diff --git a/src/main/runtime/runtime-created-worktree-terminal-target.ts b/src/main/runtime/runtime-created-worktree-terminal-target.ts new file mode 100644 index 00000000000..7f24f9261e0 --- /dev/null +++ b/src/main/runtime/runtime-created-worktree-terminal-target.ts @@ -0,0 +1,106 @@ +import type { Store } from '../persistence' +import type { Worktree } from '../../shared/worktree/types' +import type { ResolvedWorktree } from './runtime-worktree-path-identity' +import { splitWorktreeIdForFilesystem } from '../../shared/worktree/id' +import { readWorktreeMetaForHost } from '../persistence/host-qualified-worktree-meta' +import { resolveWorktreeHostRouting } from './worktree-launch-host-repo' +import { isWorktreeMetaOwnedByRepo } from '../worktree-metadata-ownership' +import { isFolderRepo } from '../../shared/repo-kind' +import type { WorktreeMeta } from '../../shared/worktree/meta-types' +import type { Repo } from '../../shared/repo-types' +import type { ExecutionHostId } from '../../shared/execution-host' +import { mergeWorktree } from '../ipc/worktree-metadata-merge' +import { projectResolvedWorktreeLineage } from '../../shared/resolved-worktree-lineage' + +type CreationEvidenceStore = Pick & + Partial> + +function readOwnedMetadata( + store: CreationEvidenceStore, + worktreeId: string, + repo: Repo, + hostId: ExecutionHostId, + repoOwnerCount: number +): WorktreeMeta | undefined { + const qualifiedMeta = readWorktreeMetaForHost(store, worktreeId, hostId) + const meta = qualifiedMeta ?? store.getWorktreeMeta(worktreeId) + return meta && + (!meta.hostId || meta.hostId === hostId) && + (qualifiedMeta || isWorktreeMetaOwnedByRepo(repo, meta, repoOwnerCount)) + ? meta + : undefined +} + +/** Reuse same-operation creation evidence only while its persisted instance and host still agree. */ +export function resolveCreatedWorktreeTerminalTarget( + store: CreationEvidenceStore | null | undefined, + selector: string, + worktree: Worktree | undefined +): ResolvedWorktree | null { + if (!store || !worktree || selector !== `id:${worktree.id}` || !worktree.instanceId) { + return null + } + const parsed = splitWorktreeIdForFilesystem(worktree.id) + if (parsed?.repoId !== worktree.repoId || parsed.worktreePath !== worktree.path) { + return null + } + const repos = store.getRepos() + const routing = resolveWorktreeHostRouting(repos, worktree) + if (routing.kind !== 'resolved' || !routing.repo || isFolderRepo(routing.repo)) { + return null + } + const repoOwnerCount = repos.filter((repo) => repo.id === worktree.repoId).length + const meta = readOwnedMetadata(store, worktree.id, routing.repo, routing.hostId, repoOwnerCount) + if (meta?.instanceId !== worktree.instanceId) { + return null + } + const target = mergeWorktree( + worktree.repoId, + worktree, + { ...meta, hostId: routing.hostId }, + routing.repo.displayName + ) + const lineageById = store.getAllWorktreeLineage?.() ?? {} + const relatedIds = new Set([target.id]) + for (const lineage of Object.values(lineageById)) { + if (lineage.parentWorktreeId === target.id) { + relatedIds.add(lineage.worktreeId) + } + } + // Include ancestors so the existing projection can reject stale edges and cycles. + for (const id of relatedIds) { + const parentId = lineageById[id]?.parentWorktreeId + if (parentId) { + relatedIds.add(parentId) + } + } + const rows = [target] + for (const id of relatedIds) { + if (id === target.id) { + continue + } + const parsedRelated = splitWorktreeIdForFilesystem(id) + if (parsedRelated?.repoId !== target.repoId) { + continue + } + const relatedMeta = readOwnedMetadata(store, id, routing.repo, routing.hostId, repoOwnerCount) + if (!relatedMeta) { + continue + } + rows.push( + mergeWorktree( + target.repoId, + { + path: parsedRelated.worktreePath, + head: '', + branch: '', + isBare: false, + isMainWorktree: false + }, + { ...relatedMeta, hostId: routing.hostId } + ) + ) + } + const projected = projectResolvedWorktreeLineage(rows, lineageById)[0] + return projected ? { ...projected, git: worktree } : null +} diff --git a/src/main/runtime/runtime-terminal-pane-identity.ts b/src/main/runtime/runtime-terminal-pane-identity.ts index 90909f6ef35..225ac564262 100644 --- a/src/main/runtime/runtime-terminal-pane-identity.ts +++ b/src/main/runtime/runtime-terminal-pane-identity.ts @@ -1,5 +1,24 @@ import { AgentLaunchPaneAlreadyLiveError } from '../../shared/agent-launch-pane-already-live' -import { parsePaneKey } from '../../shared/stable-pane-id' +import { isTerminalLeafId, makePaneKey, parsePaneKey } from '../../shared/stable-pane-id' +import { isValidHostTerminalTabId } from '../../shared/terminal-tab-id' +import { randomUUID } from 'node:crypto' + +export function allocateTerminalPaneIdentity(opts: { tabId?: string; leafId?: string }): { + tabId: string + leafId: string + paneKey: string +} { + const hintedTabId = opts.tabId?.trim() + const hintedLeafId = opts.leafId + const canAdopt = + hintedTabId !== undefined && + isValidHostTerminalTabId(hintedTabId) && + hintedLeafId !== undefined && + isTerminalLeafId(hintedLeafId) + const tabId = canAdopt ? hintedTabId : randomUUID() + const leafId = canAdopt ? hintedLeafId : randomUUID() + return { tabId, leafId, paneKey: makePaneKey(tabId, leafId) } +} /** A caller-minted `tabId:leafId` as the pair `createTerminal` adopts; an unparsable key yields * nothing, so the runtime mints its own and the reported `paneKey` shows the caller it lost. */ diff --git a/src/main/worktree-create-base-prefetch.test.ts b/src/main/worktree-create-base-prefetch.test.ts index af80afe4b41..abc338cdad7 100644 --- a/src/main/worktree-create-base-prefetch.test.ts +++ b/src/main/worktree-create-base-prefetch.test.ts @@ -186,7 +186,7 @@ describe('prefetchWorktreeCreateBase local git routing', () => { describe('checkout and refresh overlap', () => { it.each([{}, WSL])( - 'starts one checkout before a blocked refresh finishes on %j', + 'starts one checkout with a shared refresh barrier before fetch settles on %j', async (gitOptions) => { const base = { remote: 'origin', @@ -199,11 +199,17 @@ describe('checkout and refresh overlap', () => { let release!: () => void mocks.getOrStartRemoteTrackingBaseRefresh.mockImplementation( () => - new Promise((resolve) => { - release = resolve + new Promise<{ ok: boolean }>((resolve) => { + release = () => resolve({ ok: true }) }) ) - const prepareCheckout = vi.fn().mockResolvedValue(undefined) + let materialized = false + const prepareCheckout = vi.fn( + async (_base: string, beforeMaterialization?: Promise) => { + await beforeMaterialization + materialized = true + } + ) let settled = false const result = prefetchWorktreeCreateBase({ repo, @@ -214,11 +220,16 @@ describe('checkout and refresh overlap', () => { }).finally(() => { settled = true }) - await vi.waitFor(() => expect(prepareCheckout).toHaveBeenCalledWith('origin/main')) + await vi.waitFor(() => + expect(prepareCheckout).toHaveBeenCalledWith('origin/main', expect.any(Promise)) + ) expect(settled).toBe(false) + expect(materialized).toBe(false) + expect(mocks.getOrStartRemoteTrackingBaseRefresh).toHaveBeenCalledOnce() release() await expect(result).resolves.toBe('origin/main') - expect(prepareCheckout).toHaveBeenCalledTimes(1) + expect(materialized).toBe(true) + expect(prepareCheckout).toHaveBeenCalledOnce() } ) @@ -232,8 +243,8 @@ describe('checkout and refresh overlap', () => { let release!: () => void mocks.getOrStartRemoteTrackingBaseRefresh.mockImplementation( () => - new Promise((resolve) => { - release = resolve + new Promise<{ ok: boolean }>((resolve) => { + release = () => resolve({ ok: true }) }) ) const prepareCheckout = vi.fn().mockResolvedValue(undefined) @@ -307,6 +318,35 @@ describe('checkout and refresh overlap', () => { release() await assertion }) + + it('settles the materialization barrier on fetch failure without hiding the fetch error', async () => { + mocks.resolveRemoteTrackingBase.mockResolvedValue({ + remote: 'origin', + branch: 'main', + ref: 'refs/remotes/origin/main', + base: 'origin/main' + }) + mocks.hasRemoteTrackingRef.mockResolvedValue(true) + const error = new Error('offline') + mocks.getOrStartRemoteTrackingBaseRefresh.mockRejectedValue(error) + let materialized = false + const prepareCheckout = vi.fn(async (_base: string, beforeMaterialization?: Promise) => { + await beforeMaterialization + materialized = true + }) + await expect( + prefetchWorktreeCreateBase({ + repo, + baseBranch: 'origin/main', + runtime: runtime(), + gitOptions: {}, + prepareCheckout + }) + ).rejects.toBe(error) + expect(materialized).toBe(true) + expect(prepareCheckout).toHaveBeenCalledOnce() + expect(mocks.getOrStartRemoteTrackingBaseRefresh).toHaveBeenCalledOnce() + }) }) it('does not prepare folder repositories', async () => { diff --git a/src/main/worktree-create-base-prefetch.ts b/src/main/worktree-create-base-prefetch.ts index 8db177a3e9c..c322ce077c8 100644 --- a/src/main/worktree-create-base-prefetch.ts +++ b/src/main/worktree-create-base-prefetch.ts @@ -33,7 +33,7 @@ type WorktreeCreateBasePrefetchRuntime = { repoPath: string, base: RemoteTrackingBaseForPrefetch, options?: WorktreeCreateBaseGitOptions - ) => Promise + ) => Promise<{ ok: boolean }> fetchRemoteWithCache: ( repoPath: string, remote: string, @@ -46,7 +46,7 @@ async function prefetchLocalWorktreeCreateBase( baseBranch: string | undefined, runtime: WorktreeCreateBasePrefetchRuntime, options: WorktreeCreateBaseGitOptions, - prepareLocalCheckout: (base: string) => void + prepareLocalCheckout: (base: string, beforeMaterialization?: Promise) => void ): Promise { // Keep host-routed calls at their original arity so they stay on the runtime's default options. const optionArgs: [] | [WorktreeCreateBaseGitOptions] = options.wslDistro ? [options] : [] @@ -89,19 +89,24 @@ async function prefetchLocalWorktreeCreateBase( remoteTrackingBase, ...optionArgs ) - if (hasTrackingRef) { - // Finalization revalidates the refreshed commit before exposing the checkout. - prepareLocalCheckout(resolvedBaseBranch) - } if ( hasTrackingRef || !(await hasLocalWorktreeBaseRef(repo.path, resolvedBaseBranch, options)) ) { - await runtime.getOrStartRemoteTrackingBaseRefresh( + const refresh = runtime.getOrStartRemoteTrackingBaseRefresh( repo.path, remoteTrackingBase, ...optionArgs ) + if (hasTrackingRef) { + // Register during fetch, then materialize the settled base once; offline keeps its local tip. + const beforeMaterialization = refresh.then( + () => {}, + () => {} + ) + prepareLocalCheckout(resolvedBaseBranch, beforeMaterialization) + } + await refresh return resolvedBaseBranch } } @@ -124,7 +129,7 @@ export async function prefetchWorktreeCreateBase(args: { /** Routing for the project's Git host; required so a caller cannot silently * warm up the wrong ref store — pass `{}` for host Git. */ gitOptions: WorktreeCreateBaseGitOptions - prepareCheckout?: (base: string) => Promise + prepareCheckout?: (base: string, beforeMaterialization?: Promise) => Promise }): Promise { if (isFolderRepo(args.repo)) { return undefined @@ -139,12 +144,11 @@ export async function prefetchWorktreeCreateBase(args: { } const prepareCheckout = args.prepareCheckout let preparation: Promise | undefined - const prepare = (base: string): void => { + const prepare = (base: string, beforeMaterialization?: Promise): void => { if (!preparation && prepareCheckout) { - preparation = Promise.resolve() - .then(() => prepareCheckout(base)) - .catch(() => {}) + preparation = Promise.resolve().then(() => prepareCheckout(base, beforeMaterialization)) } + void preparation?.catch(() => {}) } try { const base = await prefetchLocalWorktreeCreateBase( @@ -160,6 +164,6 @@ export async function prefetchWorktreeCreateBase(args: { return base } finally { // Settle speculative work even if refresh fails; Create owns error reporting. - await preparation + await preparation?.catch(() => {}) } } diff --git a/src/main/worktree-create-preparation-cancellation.test.ts b/src/main/worktree-create-preparation-cancellation.test.ts index 7974dcb7e3d..fc171f498db 100644 --- a/src/main/worktree-create-preparation-cancellation.test.ts +++ b/src/main/worktree-create-preparation-cancellation.test.ts @@ -118,7 +118,7 @@ describe('worktree create preparation cancellation', () => { expect(signal?.aborted).toBe(true) expect((await settled)[0].status).toBe('rejected') await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, obsoletePath, {}) + expect(mocks.discard).toHaveBeenCalledWith(repo.path, obsoletePath, {}, expect.any(String)) }) it('does not retry a discard whose registration the aborted checkout already removed', async () => { diff --git a/src/main/worktree-create-preparation-pool.ts b/src/main/worktree-create-preparation-pool.ts index a72972c9d20..280be5af895 100644 --- a/src/main/worktree-create-preparation-pool.ts +++ b/src/main/worktree-create-preparation-pool.ts @@ -9,6 +9,8 @@ import { } from '../shared/worktree/create-preparation' import type { AddWorktreeOptions } from './git/worktree' import { prepareWorktreeCreateCheckout } from './git/worktree-create-preparation' +import { WorktreePreparationLockOwnershipError } from './git/worktree-preparation-lock' +import { queuePreparedWorktreeTipRefresh } from './worktree-preparation-refresh-queue' import { toHostFilesystemPath } from './host-tree-removal' import { preparationEntryKey, preparationPathKey } from './worktree-create-preparation-claim' import { @@ -17,7 +19,8 @@ import { resetStalePreparationCleanupForTests } from './worktree-create-preparation-stale-cleanup' import { - discardPreparationWithRetry, + discardPreparationEntry, + preparationHostKey, resetPendingPreparationDiscardsForTests, trackPreparationDiscard } from './worktree-preparation-discard-retry' @@ -35,6 +38,7 @@ export type PreparationEntry = { baseBranch: string canonicalBase: string preparedPath: string + lockReason: string options: AddWorktreeOptions createdAt: number ready: Promise @@ -49,6 +53,7 @@ export type StartPreparationArgs = { baseBranch: string canonicalBase: string options: AddWorktreeOptions + beforeMaterialization?: Promise } export type DeferredPreparation = { @@ -64,38 +69,14 @@ export type PreparationClaim = { } const claims = new Set() -/** One repo on one Git host: the scope a stranded discard is retried under. */ -function preparationHostKey(repoPathKey: string, wslDistro: string): string { - return `${repoPathKey}\0${wslDistro}` -} - /** A prepared checkout is a create that is either in flight or imminent. */ export function hasPendingPreparations(): boolean { return preparations.size > 0 || claims.size > 0 || hasPendingStalePreparationCleanup() } -function pathOps(path: string): Pick { - return isWindowsAbsolutePathLike(path) ? win32 : posix -} - -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, - preparedPath: entry.preparedPath, - options: entry.options - }) -} - function discardEntryInBackground(entry: PreparationEntry): void { // Tracked, not bare `void`: the test reset must be able to settle it before dropping the registry. - trackPreparationDiscard(worktreePreparationGit.run(() => discardEntry(entry))) + trackPreparationDiscard(worktreePreparationGit.run(() => discardPreparationEntry(entry))) } function expireEntry(entry: PreparationEntry): void { @@ -107,16 +88,7 @@ function expireEntry(entry: PreparationEntry): void { discardEntryInBackground(entry) } -/** - * Frees a slot for an incoming preparation, preferring one the same workspace already owns. - * - * The cap is a disk bound — a prepared checkout is a full tree, ~200 MB of tracked content in the - * repo this was measured against — so it stays small. But flipping through the composer's base - * picker arms several preparations for one repo, and a plain oldest-first eviction let that churn - * throw away another project's warm checkout, which is a structural miss for anyone working across - * several repos. Evict the incoming workspace's own oldest entry first; only reach across - * workspaces when this one holds none. - */ +/** Bound full-checkout disk use, evicting the requesting workspace's oldest preparation first. */ function enforcePreparationLimit( repoPathKey: string, workspaceRootKey: string, @@ -230,7 +202,9 @@ export function startPreparation( args.options.wslDistro ?? '' ) if (existing) { - return existing.ready + return args.beforeMaterialization + ? refreshPreparationTip(existing, args.beforeMaterialization) + : existing.ready } if (deferPreparationForClaim(args, kind)) { return Promise.resolve() @@ -238,12 +212,34 @@ export function startPreparation( return worktreePreparationGit.run(() => startBackgroundPreparation(args)) } +function refreshPreparationTip( + entry: PreparationEntry, + beforeMaterialization: Promise +): Promise { + return queuePreparedWorktreeTipRefresh( + entry, + () => { + const available = preparations.get(entry.key) === entry + if (!available && ![...claims].some((claim) => claim.entry === entry)) { + return + } + if (available) { + preparations.delete(entry.key) + clearTimeout(entry.expiration) + } + discardEntryInBackground(entry) + }, + beforeMaterialization + ) +} + function startBackgroundPreparation({ repoPath, workspaceRoot, baseBranch, canonicalBase, - options + options, + beforeMaterialization }: StartPreparationArgs): Promise { const repoPathKey = preparationPathKey(repoPath) const workspaceRootKey = preparationPathKey(workspaceRoot) @@ -252,19 +248,16 @@ function startBackgroundPreparation({ enforcePreparationLimit(repoPathKey, workspaceRootKey, wslDistro) const preparationId = `${process.pid}-${randomUUID()}` const lockReason = createWorktreePreparationLockReason(preparationId) - const preparationRoot = pathOps(workspaceRoot).join( - workspaceRoot, - WORKTREE_CREATE_PREPARATION_DIRECTORY - ) - const preparedPath = pathOps(workspaceRoot).join(preparationRoot, preparationId) + const paths = isWindowsAbsolutePathLike(workspaceRoot) ? win32 : posix + const preparationRoot = paths.join(workspaceRoot, WORKTREE_CREATE_PREPARATION_DIRECTORY) + const preparedPath = paths.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() - Object.assign(entry, { + const entry: PreparationEntry = { key, repoPath, repoPathKey, @@ -274,28 +267,41 @@ function startBackgroundPreparation({ baseBranch, canonicalBase, preparedPath, + lockReason, options, createdAt: Date.now(), expiration, controller, checkoutStarted: false, - ready: (async () => { - await startStalePreparationCleanup( - preparationHostKey(repoPathKey, wslDistro), + ready: Promise.resolve() + } + entry.ready = (async () => { + await startStalePreparationCleanup( + preparationHostKey(repoPathKey, wslDistro), + repoPath, + options + ) + signal.throwIfAborted() + await mkdir(toHostFilesystemPath(preparationRoot), { recursive: true }) + signal.throwIfAborted() + // Already canonical, so the add re-resolves nothing. + entry.checkoutStarted = true + try { + await prepareWorktreeCreateCheckout( repoPath, - options + preparedPath, + canonicalBase, + lockReason, + { ...options, signal }, + beforeMaterialization ) - signal.throwIfAborted() - await mkdir(toHostFilesystemPath(preparationRoot), { recursive: true }) - signal.throwIfAborted() - // Already canonical, so the add re-resolves nothing. - entry.checkoutStarted = true - await prepareWorktreeCreateCheckout(repoPath, preparedPath, canonicalBase, lockReason, { - ...options, - signal - }) - })() - } satisfies PreparationEntry) + } catch (error) { + if (error instanceof WorktreePreparationLockOwnershipError) { + entry.checkoutStarted = false + } + throw error + } + })() preparations.set(key, entry) void entry.ready.catch(() => { if (preparations.get(key) === entry) { @@ -314,7 +320,7 @@ export async function _resetPreparationPoolForTests(): Promise { await Promise.all( entries.map(async (entry) => { clearTimeout(entry.expiration) - await discardEntry(entry) + await discardPreparationEntry(entry) }) ) await resetPendingPreparationDiscardsForTests() diff --git a/src/main/worktree-create-preparation-stale-cleanup.ts b/src/main/worktree-create-preparation-stale-cleanup.ts index d0f146eda98..3c8c39750c3 100644 --- a/src/main/worktree-create-preparation-stale-cleanup.ts +++ b/src/main/worktree-create-preparation-stale-cleanup.ts @@ -51,15 +51,25 @@ export async function startStalePreparationCleanup( nextIndex += 1 const lockOwnerPid = parseWorktreePreparationOwnerPid(worktree.lockReason) const pathOwnerPid = parseWorktreePreparationPathOwnerPid(worktree.path) - if (!lockOwnerPid || isProcessAlive(lockOwnerPid)) { + if (!worktree.lockReason || !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, reclaimOptions).catch(() => {}) + await unlockPreparedWorktree( + repoPath, + worktree.path, + reclaimOptions, + worktree.lockReason + ).catch(() => {}) } else if (pathOwnerPid === lockOwnerPid) { - await discardPreparedWorktree(repoPath, worktree.path, reclaimOptions).catch(() => {}) + await discardPreparedWorktree( + repoPath, + worktree.path, + reclaimOptions, + worktree.lockReason + ).catch(() => {}) } } } diff --git a/src/main/worktree-create-preparation.test.ts b/src/main/worktree-create-preparation.test.ts index e8c2bb97bce..052181da228 100644 --- a/src/main/worktree-create-preparation.test.ts +++ b/src/main/worktree-create-preparation.test.ts @@ -1,53 +1,9 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import type * as WorktreeLogic from './ipc/worktree-logic' -import type { Store } from './persistence' -import { hasPendingStalePreparationCleanup } from './worktree-create-preparation-stale-cleanup' +import { mocks, repo, store, flushBackgroundWork } from './__mocks__/worktree-create-preparation' +import { describe, expect, it } from 'vitest' import type { Repo } from '../shared/repo-types' +import { hasPendingStalePreparationCleanup } from './worktree-create-preparation-stale-cleanup' import { WORKTREE_CREATE_PREPARATION_DIRECTORY } from '../shared/worktree/create-preparation' -import { resolveWorktreeAddBaseRef } from '../shared/worktree/base-ref' - -const mocks = vi.hoisted(() => ({ - mkdir: vi.fn(), - listWorktreeGraph: vi.fn(), - prepareCheckout: vi.fn(), - finalize: vi.fn(), - discard: vi.fn(), - unlock: vi.fn(), - getWorktreeOptions: vi.fn(), - computeWorkspaceRoot: vi.fn(), - computeWorkspaceRootAsync: vi.fn(), - resolveBaseRef: vi.fn(), - measureDivergence: vi.fn() -})) - -vi.mock('node:fs/promises', () => ({ mkdir: mocks.mkdir })) -vi.mock('./git/worktree', () => ({ listWorktreeGraph: mocks.listWorktreeGraph })) -vi.mock('./git/worktree-create-preparation', () => ({ - prepareWorktreeCreateCheckout: mocks.prepareCheckout, - finalizePreparedWorktree: mocks.finalize, - discardPreparedWorktree: mocks.discard, - unlockPreparedWorktree: mocks.unlock -})) -vi.mock('./git/worktree-base-ref-probe', () => ({ - resolveLocalWorktreeBaseRef: mocks.resolveBaseRef -})) -vi.mock('./git/worktree-base-divergence', () => ({ - measureRetargetDivergence: mocks.measureDivergence -})) -vi.mock('./project-runtime-git-options', () => ({ - getLocalProjectWorktreeGitOptions: mocks.getWorktreeOptions, - getWorktreeMirrorDistro: () => undefined -})) -vi.mock('./ipc/worktree-logic', async (importOriginal) => ({ - isOrphanedWorktreeError: (await importOriginal()).isOrphanedWorktreeError, - computeWorkspaceRoot: mocks.computeWorkspaceRoot, - computeWorkspaceRootAsync: mocks.computeWorkspaceRootAsync, - getWorktreePathSettings: () => ({ - workspaceDir: process.platform === 'win32' ? 'C:\\workspace' : '/workspace', - nestWorkspaces: false - }) -})) - +import { WorktreePreparationLockOwnershipError } from './git/worktree-preparation-lock' import { _resetWorktreeCreatePreparationsForTests, consumePreparedWorktreeCreate, @@ -55,49 +11,6 @@ import { prepareWorktreeCreateForRepo } from './worktree-create-preparation' -// Evictions and retries are fire-and-forget, so let them settle before asserting. -function flushBackgroundWork(ms = 0): Promise { - return new Promise((resolve) => setTimeout(resolve, ms)) -} - -const EXISTING_REFS = new Set([ - 'refs/heads/main', - 'refs/remotes/origin/main', - 'refs/remotes/origin/release' -]) -const repo = { id: 'repo-1', path: '/repo' } as Repo -const store = { getSettings: () => ({}) } as unknown as Store - -beforeEach(() => { - mocks.mkdir.mockReset().mockResolvedValue(undefined) - mocks.listWorktreeGraph.mockReset().mockResolvedValue([]) - mocks.prepareCheckout.mockReset().mockResolvedValue(undefined) - mocks.finalize.mockReset().mockResolvedValue({}) - mocks.discard.mockReset().mockResolvedValue(undefined) - mocks.unlock.mockReset().mockResolvedValue(undefined) - mocks.getWorktreeOptions.mockReset().mockReturnValue({}) - mocks.measureDivergence.mockReset().mockResolvedValue('within') - mocks.resolveBaseRef - .mockReset() - .mockImplementation((_repoPath: string, baseRef: string) => - resolveWorktreeAddBaseRef(baseRef, async (candidate) => EXISTING_REFS.has(candidate)) - ) - mocks.computeWorkspaceRoot.mockReset().mockImplementation(() => { - throw new Error('synchronous workspace-root lookup must not run on the main thread') - }) - mocks.computeWorkspaceRootAsync - .mockReset() - .mockImplementation(async (repoPath: string) => - process.platform === 'win32' && /^[A-Za-z]:[\\/]/.test(repoPath) - ? 'C:\\workspace' - : '/workspace' - ) -}) - -afterEach(async () => { - await _resetWorktreeCreatePreparationsForTests() -}) - describe('worktree create preparation registry', () => { it.each([undefined, 'Ubuntu'])( 'preserves create priority through claim probes on %s', @@ -229,7 +142,8 @@ describe('worktree create preparation registry', () => { 'feature/test', 'main', undefined, - {} + {}, + mocks.prepareCheckout.mock.calls[0]?.[3] ) }) @@ -407,7 +321,8 @@ describe('worktree create preparation registry', () => { expect.any(String), 'refs/remotes/origin/main', expect.any(String), - { ...options, signal: expect.any(AbortSignal) } + { ...options, signal: expect.any(AbortSignal) }, + undefined ) expect(mocks.finalize).toHaveBeenCalledWith( repo.path, @@ -416,7 +331,8 @@ describe('worktree create preparation registry', () => { 'feature/test', 'origin/main', undefined, - options + options, + mocks.prepareCheckout.mock.calls[0]?.[3] ) }) @@ -454,9 +370,14 @@ describe('worktree create preparation registry', () => { }) try { await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, stalePath, { - admissionTier: 'background' - }) + expect(mocks.discard).toHaveBeenCalledWith( + repo.path, + stalePath, + { + admissionTier: 'background' + }, + 'orca-create-preparation:v1:999999999:stale' + ) expect(ready).toBe(true) await prepareWorktreeCreateForRepo(store, repo, 'origin/release') expect(mocks.prepareCheckout).toHaveBeenCalledTimes(2) @@ -498,9 +419,12 @@ describe('worktree create preparation registry', () => { await prepareWorktreeCreateForRepo(store, repo, 'origin/main') - expect(mocks.unlock).toHaveBeenCalledWith(repo.path, '/workspace/final', { - admissionTier: 'background' - }) + expect(mocks.unlock).toHaveBeenCalledWith( + repo.path, + '/workspace/final', + { admissionTier: 'background' }, + 'orca-create-preparation:v1:999999999:stale' + ) expect(mocks.discard).not.toHaveBeenCalledWith(repo.path, '/workspace/final', expect.anything()) }) @@ -560,6 +484,21 @@ describe('worktree create preparation registry', () => { expect(mocks.discard).toHaveBeenCalledTimes(1) }) + it('preserves another owner’s checkout when finalization loses its lock', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + mocks.finalize.mockRejectedValueOnce(new WorktreePreparationLockOwnershipError()) + await expect( + consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot: '/workspace', + worktreePath: '/workspace/final', + branch: 'feature/test', + baseBranch: 'origin/main' + }) + ).resolves.toMatchObject({ status: 'miss', reason: 'finalize_failed' }) + expect(mocks.discard).not.toHaveBeenCalled() + }) + /** Mirrors a real create: consume, then run the deferred re-arm once the create has returned. */ async function consumeOnce(name: string): Promise { const attempt = await consumePreparedWorktreeCreate({ @@ -676,218 +615,6 @@ describe('worktree create preparation registry', () => { expect(mocks.prepareCheckout).not.toHaveBeenCalled() }) - it('retries a discard that failed while this process is still alive', async () => { - await prepareWorktreeCreateForRepo(store, repo, 'origin/main') - const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string - mocks.discard.mockRejectedValueOnce(new Error('EBUSY')) - - // Fill the registry so the first preparation is evicted while its owner pid is still alive. - for (const base of ['origin/one', 'origin/two', 'origin/three']) { - await prepareWorktreeCreateForRepo(store, repo, base) - } - await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedPath, {}) - - mocks.discard.mockClear() - await prepareWorktreeCreateForRepo(store, repo, 'origin/four') - await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedPath, {}) - - mocks.discard.mockClear() - await prepareWorktreeCreateForRepo(store, repo, 'origin/five') - await flushBackgroundWork() - expect(mocks.discard).not.toHaveBeenCalledWith(repo.path, leakedPath, {}) - }) - - it('retries only the leaked paths belonging to the host being prepared', async () => { - const otherRepo = { ...repo, id: 'repo-2', path: '/other-repo' } as Repo - const unremovable = new Set() - mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { - if (unremovable.has(path)) { - throw new Error('EBUSY') - } - }) - - await prepareWorktreeCreateForRepo(store, repo, 'origin/main') - const leakedHere = mocks.prepareCheckout.mock.calls[0][1] as string - await prepareWorktreeCreateForRepo(store, otherRepo, 'origin/main') - const leakedElsewhere = mocks.prepareCheckout.mock.calls[1][1] as string - unremovable.add(leakedHere) - unremovable.add(leakedElsewhere) - - // Evict through each host's own arming: eviction prefers the incoming workspace's oldest - // entry, so preparing for `repo` no longer reaches across and takes `otherRepo`'s. - for (const base of ['origin/one', 'origin/two']) { - await prepareWorktreeCreateForRepo(store, repo, base) - } - for (const base of ['origin/one', 'origin/two']) { - await prepareWorktreeCreateForRepo(store, otherRepo, base) - } - await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedHere, {}) - expect(mocks.discard).toHaveBeenCalledWith(otherRepo.path, leakedElsewhere, {}) - - mocks.discard.mockClear() - await prepareWorktreeCreateForRepo(store, repo, 'origin/four') - await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedHere, {}) - expect(mocks.discard.mock.calls.some((call) => call[1] === leakedElsewhere)).toBe(false) - }) - - it('stops retrying a preparation that never becomes removable', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - try { - await prepareWorktreeCreateForRepo(store, repo, 'origin/main') - const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string - mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { - if (path === leakedPath) { - throw new Error('EBUSY') - } - }) - const leakedDiscards = (): number => - mocks.discard.mock.calls.filter((call) => call[1] === leakedPath).length - - for (const base of ['origin/one', 'origin/two', 'origin/three']) { - await prepareWorktreeCreateForRepo(store, repo, base) - } - await flushBackgroundWork() - expect(leakedDiscards()).toBe(1) - - for (const base of ['origin/four', 'origin/five', 'origin/six']) { - await prepareWorktreeCreateForRepo(store, repo, base) - await flushBackgroundWork() - } - expect(leakedDiscards()).toBe(3) - expect(warn).toHaveBeenCalledWith( - expect.stringContaining(`could not be discarded in 3 attempts; ${leakedPath}`), - expect.any(Error) - ) - } finally { - warn.mockRestore() - } - }) - - it('retries a failed checkout whose own self-discard also left the path registered', async () => { - let failCheckout!: (error: Error) => void - mocks.prepareCheckout.mockImplementationOnce( - () => - new Promise((_resolve, reject) => { - failCheckout = reject - }) - ) - const failing = prepareWorktreeCreateForRepo(store, repo, 'origin/main') - await flushBackgroundWork() - const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string - - // Evict it while its checkout is still in flight, so discardEntry runs on a failed preparation. - for (const base of ['origin/one', 'origin/two', 'origin/three']) { - await prepareWorktreeCreateForRepo(store, repo, base) - } - mocks.discard.mockRejectedValueOnce(new Error('EBUSY')) - failCheckout(new Error('worktree add failed')) - await failing.catch(() => {}) - await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedPath, {}) - - mocks.discard.mockClear() - await prepareWorktreeCreateForRepo(store, repo, 'origin/four') - await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedPath, {}) - }) - - it('scopes retries to the WSL distro whose preparation leaked', async () => { - const unremovable = new Set() - mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { - if (unremovable.has(path)) { - throw new Error('EBUSY') - } - }) - - mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Ubuntu' }) - await prepareWorktreeCreateForRepo(store, repo, 'origin/main') - const leakedOnUbuntu = mocks.prepareCheckout.mock.calls[0][1] as string - mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Debian' }) - await prepareWorktreeCreateForRepo(store, repo, 'origin/main') - const leakedOnDebian = mocks.prepareCheckout.mock.calls[1][1] as string - unremovable.add(leakedOnUbuntu) - unremovable.add(leakedOnDebian) - - // Evict through each distro's own arming: the eviction scope includes the distro, so arming - // under Ubuntu no longer reaches across and takes the Debian entry. - mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Ubuntu' }) - for (const base of ['origin/one', 'origin/two']) { - await prepareWorktreeCreateForRepo(store, repo, base) - } - mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Debian' }) - await prepareWorktreeCreateForRepo(store, repo, 'origin/one') - mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Ubuntu' }) - await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedOnUbuntu, { wslDistro: 'Ubuntu' }) - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedOnDebian, { wslDistro: 'Debian' }) - - mocks.discard.mockClear() - await prepareWorktreeCreateForRepo(store, repo, 'origin/four') - await flushBackgroundWork() - expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedOnUbuntu, { wslDistro: 'Ubuntu' }) - expect(mocks.discard.mock.calls.some((call) => call[1] === leakedOnDebian)).toBe(false) - }) - - it('drops recorded discards when the registry is reset for tests', async () => { - await prepareWorktreeCreateForRepo(store, repo, 'origin/main') - const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string - // Reject on a real timer so the fire-and-forget discard is still in flight at reset. - mocks.discard.mockImplementationOnce(async () => { - await new Promise((resolve) => setTimeout(resolve, 5)) - throw new Error('EBUSY') - }) - - for (const base of ['origin/one', 'origin/two', 'origin/three']) { - await prepareWorktreeCreateForRepo(store, repo, base) - } - await _resetWorktreeCreatePreparationsForTests() - // Past the rejection timer: the reset must have absorbed the failure, not raced ahead of it. - await flushBackgroundWork(20) - - mocks.discard.mockClear() - await prepareWorktreeCreateForRepo(store, repo, 'origin/four') - await flushBackgroundWork() - expect(mocks.discard).not.toHaveBeenCalledWith(repo.path, leakedPath, {}) - }) - - it("settles an evicted preparation's discard before the reset drops the registry", async () => { - let failCheckout!: (error: Error) => void - mocks.prepareCheckout.mockImplementationOnce( - () => - new Promise((_resolve, reject) => { - failCheckout = reject - }) - ) - const failing = prepareWorktreeCreateForRepo(store, repo, 'origin/main') - await flushBackgroundWork() - const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string - mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { - if (path === leakedPath) { - throw new Error('EBUSY') - } - }) - - for (const base of ['origin/one', 'origin/two', 'origin/three']) { - await prepareWorktreeCreateForRepo(store, repo, base) - } - // The eviction's discard is still parked on the checkout, so the reset has to wait for it. - const reset = _resetWorktreeCreatePreparationsForTests() - await flushBackgroundWork(5) - failCheckout(new Error('worktree add failed')) - await failing.catch(() => {}) - await reset - await flushBackgroundWork(5) - - mocks.discard.mockClear() - await prepareWorktreeCreateForRepo(store, repo, 'origin/four') - await flushBackgroundWork() - expect(mocks.discard).not.toHaveBeenCalledWith(repo.path, leakedPath, {}) - }) - it('reports a pending create while a stale-cleanup scan is running', async () => { let releaseListing!: () => void mocks.listWorktreeGraph.mockReturnValueOnce( diff --git a/src/main/worktree-create-preparation.ts b/src/main/worktree-create-preparation.ts index 3ac022910eb..0bca1160d8e 100644 --- a/src/main/worktree-create-preparation.ts +++ b/src/main/worktree-create-preparation.ts @@ -9,6 +9,7 @@ import type { PreparedCheckoutMissReason } from '../shared/worktree/create-types import type { AddWorktreeOptions, AddWorktreeResult } from './git/worktree' import { measureRetargetDivergence } from './git/worktree-base-divergence' import { resolveLocalWorktreeBaseRef } from './git/worktree-base-ref-probe' +import { WorktreePreparationLockOwnershipError } from './git/worktree-preparation-lock' import { preparationPathKey, selectPreparationForCreate } from './worktree-create-preparation-claim' import { _resetPreparationPoolForTests, @@ -86,17 +87,19 @@ function canonicalBaseRef( export function prepareWorktreeCreateForRepo( store: Store, repo: Repo, - baseBranch: string + baseBranch: string, + beforeMaterialization?: Promise ): Promise { return worktreePreparationGit.run(() => - prepareWorktreeCreateInBackground(store, repo, baseBranch) + prepareWorktreeCreateInBackground(store, repo, baseBranch, beforeMaterialization) ) } async function prepareWorktreeCreateInBackground( store: Store, repo: Repo, - baseBranch: string + baseBranch: string, + beforeMaterialization?: Promise ): Promise { if (repo.connectionId || isFolderRepo(repo)) { return @@ -116,7 +119,8 @@ async function prepareWorktreeCreateInBackground( workspaceRoot, baseBranch, canonicalBase, - options + options, + beforeMaterialization }) } @@ -285,7 +289,8 @@ export async function consumePreparedWorktreeCreate( args.branch, args.baseBranch, args.refreshLocalBaseRef, - options + options, + entry.lockReason ) const result = args.timing ? await args.timing.time('prepared_checkout_finalize', finalize) @@ -295,7 +300,14 @@ export async function consumePreparedWorktreeCreate( const rearm = deferRearmPreparation(entry, reservation, args.baseBranch, claim.canonicalBase) return { status: 'hit', retargeted: claim.retargeted, result, rearm } } catch (error) { - await discardPreparedWorktree(args.repoPath, entry.preparedPath, options).catch(() => {}) + if (!(error instanceof WorktreePreparationLockOwnershipError)) { + await discardPreparedWorktree( + args.repoPath, + entry.preparedPath, + options, + entry.lockReason + ).catch(() => {}) + } console.warn( '[worktree-create] prepared checkout could not be finalized; using normal add', error diff --git a/src/main/worktree-preparation-discard-retry.test.ts b/src/main/worktree-preparation-discard-retry.test.ts new file mode 100644 index 00000000000..5a4b2d8b52d --- /dev/null +++ b/src/main/worktree-preparation-discard-retry.test.ts @@ -0,0 +1,241 @@ +import { mocks, repo, store, flushBackgroundWork } from './__mocks__/worktree-create-preparation' +import { describe, expect, it, vi } from 'vitest' +import type { Repo } from '../shared/repo-types' +import { + _resetWorktreeCreatePreparationsForTests, + prepareWorktreeCreateForRepo +} from './worktree-create-preparation' + +describe('worktree preparation discard retries', () => { + it('retries a discard that failed while this process is still alive', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string + mocks.discard.mockRejectedValueOnce(new Error('EBUSY')) + + // Fill the registry so the first preparation is evicted while its owner pid is still alive. + for (const base of ['origin/one', 'origin/two', 'origin/three']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedPath, {}, expect.any(String)) + + mocks.discard.mockClear() + await prepareWorktreeCreateForRepo(store, repo, 'origin/four') + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedPath, {}, expect.any(String)) + + mocks.discard.mockClear() + await prepareWorktreeCreateForRepo(store, repo, 'origin/five') + await flushBackgroundWork() + expect(mocks.discard).not.toHaveBeenCalledWith(repo.path, leakedPath, {}, expect.any(String)) + }) + + it('retries only the leaked paths belonging to the host being prepared', async () => { + const otherRepo = { ...repo, id: 'repo-2', path: '/other-repo' } as Repo + const unremovable = new Set() + mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { + if (unremovable.has(path)) { + throw new Error('EBUSY') + } + }) + + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const leakedHere = mocks.prepareCheckout.mock.calls[0][1] as string + await prepareWorktreeCreateForRepo(store, otherRepo, 'origin/main') + const leakedElsewhere = mocks.prepareCheckout.mock.calls[1][1] as string + unremovable.add(leakedHere) + unremovable.add(leakedElsewhere) + + // Evict through each host's own arming: eviction prefers the incoming workspace's oldest + // entry, so preparing for `repo` no longer reaches across and takes `otherRepo`'s. + for (const base of ['origin/one', 'origin/two']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + for (const base of ['origin/one', 'origin/two']) { + await prepareWorktreeCreateForRepo(store, otherRepo, base) + } + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedHere, {}, expect.any(String)) + expect(mocks.discard).toHaveBeenCalledWith( + otherRepo.path, + leakedElsewhere, + {}, + expect.any(String) + ) + + mocks.discard.mockClear() + await prepareWorktreeCreateForRepo(store, repo, 'origin/four') + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedHere, {}, expect.any(String)) + expect(mocks.discard.mock.calls.some((call) => call[1] === leakedElsewhere)).toBe(false) + }) + + it('stops retrying a preparation that never becomes removable', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string + mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { + if (path === leakedPath) { + throw new Error('EBUSY') + } + }) + const leakedDiscards = (): number => + mocks.discard.mock.calls.filter((call) => call[1] === leakedPath).length + + for (const base of ['origin/one', 'origin/two', 'origin/three']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + await flushBackgroundWork() + expect(leakedDiscards()).toBe(1) + + for (const base of ['origin/four', 'origin/five', 'origin/six']) { + await prepareWorktreeCreateForRepo(store, repo, base) + await flushBackgroundWork() + } + expect(leakedDiscards()).toBe(3) + expect(warn).toHaveBeenCalledWith( + expect.stringContaining(`could not be discarded in 3 attempts; ${leakedPath}`), + expect.any(Error) + ) + } finally { + warn.mockRestore() + } + }) + + it('retries a failed checkout whose own self-discard also left the path registered', async () => { + let failCheckout!: (error: Error) => void + mocks.prepareCheckout.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + failCheckout = reject + }) + ) + const failing = prepareWorktreeCreateForRepo(store, repo, 'origin/main') + await flushBackgroundWork() + const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string + + // Evict it while its checkout is still in flight, so discardEntry runs on a failed preparation. + for (const base of ['origin/one', 'origin/two', 'origin/three']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + mocks.discard.mockRejectedValueOnce(new Error('EBUSY')) + failCheckout(new Error('worktree add failed')) + await failing.catch(() => {}) + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedPath, {}, expect.any(String)) + + mocks.discard.mockClear() + await prepareWorktreeCreateForRepo(store, repo, 'origin/four') + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith(repo.path, leakedPath, {}, expect.any(String)) + }) + + it('scopes retries to the WSL distro whose preparation leaked', async () => { + const unremovable = new Set() + mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { + if (unremovable.has(path)) { + throw new Error('EBUSY') + } + }) + + mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Ubuntu' }) + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const leakedOnUbuntu = mocks.prepareCheckout.mock.calls[0][1] as string + mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Debian' }) + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const leakedOnDebian = mocks.prepareCheckout.mock.calls[1][1] as string + unremovable.add(leakedOnUbuntu) + unremovable.add(leakedOnDebian) + + // Evict through each distro's own arming: the eviction scope includes the distro, so arming + // under Ubuntu no longer reaches across and takes the Debian entry. + mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Ubuntu' }) + for (const base of ['origin/one', 'origin/two']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Debian' }) + await prepareWorktreeCreateForRepo(store, repo, 'origin/one') + mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Ubuntu' }) + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith( + repo.path, + leakedOnUbuntu, + { wslDistro: 'Ubuntu' }, + expect.any(String) + ) + expect(mocks.discard).toHaveBeenCalledWith( + repo.path, + leakedOnDebian, + { wslDistro: 'Debian' }, + expect.any(String) + ) + + mocks.discard.mockClear() + await prepareWorktreeCreateForRepo(store, repo, 'origin/four') + await flushBackgroundWork() + expect(mocks.discard).toHaveBeenCalledWith( + repo.path, + leakedOnUbuntu, + { wslDistro: 'Ubuntu' }, + expect.any(String) + ) + expect(mocks.discard.mock.calls.some((call) => call[1] === leakedOnDebian)).toBe(false) + }) + + it('drops recorded discards when the registry is reset for tests', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string + // Reject on a real timer so the fire-and-forget discard is still in flight at reset. + mocks.discard.mockImplementationOnce(async () => { + await new Promise((resolve) => setTimeout(resolve, 5)) + throw new Error('EBUSY') + }) + + for (const base of ['origin/one', 'origin/two', 'origin/three']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + await _resetWorktreeCreatePreparationsForTests() + // Past the rejection timer: the reset must have absorbed the failure, not raced ahead of it. + await flushBackgroundWork(20) + + mocks.discard.mockClear() + await prepareWorktreeCreateForRepo(store, repo, 'origin/four') + await flushBackgroundWork() + expect(mocks.discard).not.toHaveBeenCalledWith(repo.path, leakedPath, {}, expect.any(String)) + }) + + it("settles an evicted preparation's discard before the reset drops the registry", async () => { + let failCheckout!: (error: Error) => void + mocks.prepareCheckout.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + failCheckout = reject + }) + ) + const failing = prepareWorktreeCreateForRepo(store, repo, 'origin/main') + await flushBackgroundWork() + const leakedPath = mocks.prepareCheckout.mock.calls[0][1] as string + mocks.discard.mockImplementation(async (_repoPath: string, path: string) => { + if (path === leakedPath) { + throw new Error('EBUSY') + } + }) + + for (const base of ['origin/one', 'origin/two', 'origin/three']) { + await prepareWorktreeCreateForRepo(store, repo, base) + } + // The eviction's discard is still parked on the checkout, so the reset has to wait for it. + const reset = _resetWorktreeCreatePreparationsForTests() + await flushBackgroundWork(5) + failCheckout(new Error('worktree add failed')) + await failing.catch(() => {}) + await reset + await flushBackgroundWork(5) + + mocks.discard.mockClear() + await prepareWorktreeCreateForRepo(store, repo, 'origin/four') + await flushBackgroundWork() + expect(mocks.discard).not.toHaveBeenCalledWith(repo.path, leakedPath, {}, expect.any(String)) + }) +}) diff --git a/src/main/worktree-preparation-discard-retry.ts b/src/main/worktree-preparation-discard-retry.ts index 8602bebb39a..69ea2c4edd3 100644 --- a/src/main/worktree-preparation-discard-retry.ts +++ b/src/main/worktree-preparation-discard-retry.ts @@ -1,6 +1,8 @@ import type { AddWorktreeOptions } from './git/worktree' import { discardPreparedWorktree } from './git/worktree-create-preparation' +import { WorktreePreparationLockOwnershipError } from './git/worktree-preparation-lock' import { isOrphanedWorktreeError } from './ipc/worktree-logic' +import type { PreparationEntry } from './worktree-create-preparation-pool' // 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 @@ -11,12 +13,34 @@ export type PreparationDiscardTarget = { hostKey: string repoPath: string preparedPath: string + lockReason: string options: AddWorktreeOptions } const pendingDiscards = new Map() const inFlightDiscards = new Set>() +/** One repository on one Git host owns preparation cleanup and its retries. */ +export function preparationHostKey(repoPathKey: string, wslDistro: string): string { + return `${repoPathKey}\0${wslDistro}` +} + +export async function discardPreparationEntry(entry: PreparationEntry): Promise { + await entry.ready.catch(() => {}) + if (!entry.checkoutStarted) { + return + } + // Claim cleanup before yielding so queued refresh failures cannot discard the same checkout. + entry.checkoutStarted = false + await discardPreparationWithRetry({ + hostKey: preparationHostKey(entry.repoPathKey, entry.wslDistro), + repoPath: entry.repoPath, + preparedPath: entry.preparedPath, + lockReason: entry.lockReason, + options: entry.options + }) +} + /** Keeps a fire-and-forget discard settleable by the test reset, which would otherwise race it. */ export function trackPreparationDiscard(work: Promise): void { inFlightDiscards.add(work) @@ -30,11 +54,16 @@ function pendingKey(target: PreparationDiscardTarget): string { async function runDiscard(target: PreparationDiscardTarget, attempts: number): Promise { try { - await discardPreparedWorktree(target.repoPath, target.preparedPath, target.options) + await discardPreparedWorktree( + target.repoPath, + target.preparedPath, + target.options, + target.lockReason + ) } 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)) { + if (error instanceof WorktreePreparationLockOwnershipError || isOrphanedWorktreeError(error)) { return } // Bounded: a path that never becomes removable must not tax every later preparation. diff --git a/src/main/worktree-preparation-fetch-barrier.test.ts b/src/main/worktree-preparation-fetch-barrier.test.ts new file mode 100644 index 00000000000..9d2c8d397a7 --- /dev/null +++ b/src/main/worktree-preparation-fetch-barrier.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it, vi } from 'vitest' +import { waitForPromiseWithSignal } from '../shared/abort-signal-reason' +import { flushBackgroundWork, mocks, repo, store } from './__mocks__/worktree-create-preparation' +import { + consumePreparedWorktreeCreate, + prepareWorktreeCreateForRepo +} from './worktree-create-preparation' +import { + listPreparations, + WORKTREE_CREATE_PREPARATION_TTL_MS +} from './worktree-create-preparation-pool' + +function deferredFetch() { + let settle!: () => void + const beforeMaterialization = new Promise((resolve) => { + settle = resolve + }) + return { beforeMaterialization, settle } +} + +function consume() { + return consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot: '/workspace', + worktreePath: '/workspace/new', + branch: 'new', + baseBranch: 'origin/main' + }) +} + +describe('prepared checkout shared fetch barrier', () => { + it('passes initial materialization settlement separately from owning WSL Git options', async () => { + mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Ubuntu' }) + const fetch = deferredFetch() + await prepareWorktreeCreateForRepo(store, repo, 'origin/main', fetch.beforeMaterialization) + const entry = listPreparations()[0]! + expect(mocks.prepareCheckout).toHaveBeenCalledWith( + repo.path, + entry.preparedPath, + 'refs/remotes/origin/main', + entry.lockReason, + { wslDistro: 'Ubuntu', signal: expect.any(AbortSignal) }, + fetch.beforeMaterialization + ) + expect(mocks.refreshTip).not.toHaveBeenCalled() + fetch.settle() + }) + + it('lets a cold racing consume join the first materialization without a second checkout or refresh', async () => { + const fetch = deferredFetch() + let materializations = 0 + mocks.prepareCheckout.mockImplementationOnce( + async (_repo, _path, _base, _lock, options, barrier) => { + await waitForPromiseWithSignal(barrier, options.signal) + materializations++ + } + ) + const preparation = prepareWorktreeCreateForRepo( + store, + repo, + 'origin/main', + fetch.beforeMaterialization + ) + await vi.waitFor(() => expect(mocks.prepareCheckout).toHaveBeenCalledOnce()) + const create = consume() + await flushBackgroundWork() + expect(materializations).toBe(0) + expect(mocks.finalize).not.toHaveBeenCalled() + fetch.settle() + await preparation + expect(await create).toMatchObject({ status: 'hit' }) + expect(materializations).toBe(1) + expect(mocks.prepareCheckout).toHaveBeenCalledOnce() + expect(mocks.refreshTip).not.toHaveBeenCalled() + expect(mocks.finalize).toHaveBeenCalledOnce() + }) + + it('publishes an existing checkout refresh before a racing consume can finalize', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const entry = listPreparations()[0]! + const oldReady = entry.ready + const fetch = deferredFetch() + const preparation = prepareWorktreeCreateForRepo( + store, + repo, + 'origin/main', + fetch.beforeMaterialization + ) + await vi.waitFor(() => expect(entry.ready).not.toBe(oldReady)) + const create = consume() + await flushBackgroundWork() + expect(mocks.refreshTip).not.toHaveBeenCalled() + expect(mocks.finalize).not.toHaveBeenCalled() + fetch.settle() + await preparation + expect(await create).toMatchObject({ status: 'hit' }) + expect(mocks.prepareCheckout).toHaveBeenCalledOnce() + expect(mocks.refreshTip).toHaveBeenCalledOnce() + expect(mocks.finalize).toHaveBeenCalledOnce() + }) + + it('expires a ready checkout while its shared fetch is still pending without waiting for the network', async () => { + vi.useFakeTimers() + const fetch = deferredFetch() + try { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const entry = listPreparations()[0]! + const settled = Promise.allSettled([ + prepareWorktreeCreateForRepo(store, repo, 'origin/main', fetch.beforeMaterialization) + ]) + await vi.advanceTimersByTimeAsync(0) + await vi.advanceTimersByTimeAsync(WORKTREE_CREATE_PREPARATION_TTL_MS) + expect((await settled)[0]?.status).toBe('rejected') + expect(mocks.refreshTip).not.toHaveBeenCalled() + expect(mocks.discard).toHaveBeenCalledOnce() + expect(mocks.discard).toHaveBeenCalledWith( + repo.path, + entry.preparedPath, + {}, + entry.lockReason + ) + expect(listPreparations()).toEqual([]) + } finally { + fetch.settle() + vi.useRealTimers() + } + }) +}) diff --git a/src/main/worktree-preparation-refresh-queue.ts b/src/main/worktree-preparation-refresh-queue.ts new file mode 100644 index 00000000000..5e9b69c9725 --- /dev/null +++ b/src/main/worktree-preparation-refresh-queue.ts @@ -0,0 +1,42 @@ +import { worktreePreparationGit } from './git/worktree-create-git-executor' +import { refreshPreparedWorktreeTip } from './git/worktree-preparation-tip-refresh' +import { WorktreePreparationLockOwnershipError } from './git/worktree-preparation-lock' +import type { PreparationEntry } from './worktree-create-preparation-pool' +import { waitForPromiseWithSignal } from '../shared/abort-signal-reason' + +/** Publish the refresh before yielding so a racing create waits for the same checkout. */ +export function queuePreparedWorktreeTipRefresh( + entry: PreparationEntry, + retire: () => void, + beforeMaterialization?: Promise +): Promise { + const signal = entry.options.signal + ? AbortSignal.any([entry.options.signal, entry.controller.signal]) + : entry.controller.signal + const ready = entry.ready.then(async () => { + signal.throwIfAborted() + if (beforeMaterialization) { + await waitForPromiseWithSignal(beforeMaterialization, signal) + } + return worktreePreparationGit.run(() => + refreshPreparedWorktreeTip( + entry.repoPath, + entry.preparedPath, + entry.canonicalBase, + entry.lockReason, + { + ...entry.options, + signal + } + ) + ) + }) + entry.ready = ready + void ready.catch((error: unknown) => { + if (error instanceof WorktreePreparationLockOwnershipError) { + entry.checkoutStarted = false + } + retire() + }) + return ready +} diff --git a/src/main/worktree-preparation-tip-refresh.test.ts b/src/main/worktree-preparation-tip-refresh.test.ts new file mode 100644 index 00000000000..cc22314a8b8 --- /dev/null +++ b/src/main/worktree-preparation-tip-refresh.test.ts @@ -0,0 +1,225 @@ +import { describe, expect, it, vi } from 'vitest' +import { flushBackgroundWork, mocks, repo, store } from './__mocks__/worktree-create-preparation' +import { WorktreePreparationLockOwnershipError } from './git/worktree-preparation-lock' +import { + consumePreparedWorktreeCreate, + prepareWorktreeCreateForRepo +} from './worktree-create-preparation' +import { + listPreparations, + WORKTREE_CREATE_PREPARATION_TTL_MS +} from './worktree-create-preparation-pool' + +const beforeMaterialization = Promise.resolve() + +function consume() { + return consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot: '/workspace', + worktreePath: '/workspace/new', + branch: 'new', + baseBranch: 'origin/main' + }) +} + +describe('prepared checkout tip refresh', () => { + it('refreshes the existing checkout on its Git host without preparing another tree', async () => { + mocks.getWorktreeOptions.mockReturnValue({ wslDistro: 'Ubuntu' }) + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const entry = listPreparations()[0]! + await prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + expect(mocks.prepareCheckout).toHaveBeenCalledOnce() + expect(mocks.refreshTip).toHaveBeenCalledWith( + repo.path, + entry.preparedPath, + 'refs/remotes/origin/main', + entry.lockReason, + { wslDistro: 'Ubuntu', signal: expect.any(AbortSignal) } + ) + }) + + it('lets consume join an in-flight refresh before finalizing', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + let release!: () => void + mocks.refreshTip.mockImplementationOnce( + () => new Promise((resolve) => (release = resolve)) + ) + const refresh = prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + await vi.waitFor(() => expect(mocks.refreshTip).toHaveBeenCalledOnce()) + const create = consume() + await flushBackgroundWork() + expect(mocks.finalize).not.toHaveBeenCalled() + release() + await refresh + expect(await create).toMatchObject({ status: 'hit' }) + expect(mocks.finalize).toHaveBeenCalledOnce() + }) + + it('defers another preparation while the previous checkout is still claimed', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + expect(await consume()).toMatchObject({ status: 'hit' }) + mocks.computeWorkspaceRootAsync.mockClear() + mocks.resolveBaseRef.mockClear() + await prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + expect(mocks.computeWorkspaceRootAsync).toHaveBeenCalledOnce() + expect(mocks.resolveBaseRef).toHaveBeenCalledOnce() + expect(mocks.refreshTip).not.toHaveBeenCalled() + expect(mocks.prepareCheckout).toHaveBeenCalledOnce() + }) + + it('cancels an expired tip refresh before cleanup', async () => { + vi.useFakeTimers() + let signal: AbortSignal | undefined + try { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const entry = listPreparations()[0]! + mocks.refreshTip.mockImplementationOnce((_repo, _path, _base, _lock, options) => { + signal = options.signal + return new Promise((_resolve, reject) => { + options.signal.addEventListener('abort', () => reject(options.signal.reason), { + once: true + }) + }) + }) + const refresh = prepareWorktreeCreateForRepo( + store, + repo, + 'origin/main', + beforeMaterialization + ) + const settled = Promise.allSettled([refresh]) + 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') + expect(mocks.discard).toHaveBeenCalledOnce() + expect(mocks.discard).toHaveBeenCalledWith( + repo.path, + entry.preparedPath, + {}, + entry.lockReason + ) + expect(listPreparations()).toEqual([]) + } finally { + vi.useRealTimers() + } + }) + + it('cancels an evicted tip refresh and preserves its original cleanup scope', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const entry = listPreparations()[0]! + let signal: AbortSignal | undefined + mocks.refreshTip.mockImplementationOnce((_repo, _path, _base, _lock, options) => { + signal = options.signal + return new Promise((_resolve, reject) => { + options.signal.addEventListener('abort', () => reject(options.signal.reason), { + once: true + }) + }) + }) + const settled = Promise.allSettled([ + prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + ]) + await vi.waitFor(() => expect(mocks.refreshTip).toHaveBeenCalledOnce()) + 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).toHaveBeenCalledOnce() + expect(mocks.discard).toHaveBeenCalledWith(repo.path, entry.preparedPath, {}, entry.lockReason) + }) + + it('preserves a checkout whose lock was taken over', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + mocks.refreshTip.mockRejectedValueOnce(new WorktreePreparationLockOwnershipError()) + await expect( + prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + ).rejects.toThrow('lock owner changed') + await flushBackgroundWork() + expect(listPreparations()).toEqual([]) + expect(mocks.discard).not.toHaveBeenCalled() + expect(await consume()).toMatchObject({ status: 'miss', reason: 'none_armed' }) + }) + + it('serializes successive fetched tips and lets consume wait for the newest queued refresh', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + const order: string[] = [] + let release!: () => void + mocks.refreshTip + .mockImplementationOnce( + () => + new Promise((resolve) => { + release = () => { + order.push('first-tip') + resolve() + } + }) + ) + .mockImplementationOnce(async () => { + order.push('second-tip') + }) + mocks.finalize.mockImplementationOnce(async () => { + order.push('finalize') + return {} + }) + const first = prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + await vi.waitFor(() => expect(mocks.refreshTip).toHaveBeenCalledOnce()) + const second = prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + await flushBackgroundWork() + const create = consume() + await flushBackgroundWork() + expect(mocks.refreshTip).toHaveBeenCalledOnce() + expect(mocks.finalize).not.toHaveBeenCalled() + release() + const results = await Promise.all([first, second, create]) + expect(results[2]).toMatchObject({ status: 'hit' }) + expect(order).toEqual(['first-tip', 'second-tip', 'finalize']) + expect(mocks.prepareCheckout).toHaveBeenCalledOnce() + }) + + it('discards a claimed checkout once when successive queued refreshes fail', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + let rejectRefresh = (_error: Error) => {} + mocks.refreshTip.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectRefresh = reject + }) + ) + const first = prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + await vi.waitFor(() => expect(mocks.refreshTip).toHaveBeenCalledOnce()) + const second = prepareWorktreeCreateForRepo(store, repo, 'origin/main', beforeMaterialization) + await flushBackgroundWork() + const create = consume() + const settled = Promise.allSettled([first, second]) + await flushBackgroundWork() + const finishes: (() => void)[] = [] + mocks.discard.mockImplementation( + () => + new Promise((resolve) => { + finishes.push(resolve) + }) + ) + rejectRefresh(new Error('first tip reset failed')) + await settled + const attempt = await create + expect(attempt).toMatchObject({ status: 'miss', reason: 'prepare_failed' }) + await vi.waitFor(() => expect(mocks.discard).toHaveBeenCalled()) + await flushBackgroundWork() + try { + expect(mocks.discard).toHaveBeenCalledOnce() + expect(mocks.finalize).not.toHaveBeenCalled() + } finally { + for (const finish of finishes) { + finish() + } + if (attempt.status === 'miss') { + attempt.rearm?.() + } + await flushBackgroundWork() + } + }) +}) diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index 8575f0fddd2..a692c2890c0 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -1,5 +1,5 @@ import { execFile } from 'node:child_process' -import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readFile, rm, unlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { promisify } from 'node:util' @@ -13,6 +13,7 @@ import { isForEachRefExcludeUnsupportedError } from './git-ref-command-capabilit import { isNoWriteFetchHeadUnsupportedError } from './git-fetch-head-capability' import { hasUnsupportedRevParsePathFormatEcho, + isUnsupportedWorktreeAddLockReasonError, isUnsupportedWorktreeListZError } from './git-worktree-command-capabilities' import { gitCredentialPromptGuardEnv } from './git-credential-prompt-env' @@ -206,15 +207,36 @@ describeBinaryCompatibility('real Git binary compatibility', () => { it('supports prepared worktree creation and finalization', async () => { const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() - await runGit(['worktree', 'add', '--detach', '--no-checkout', 'compat-prepared', 'HEAD']) + const lockPath = join(repoPath, '.git', 'worktrees', 'compat-prepared', 'locked') + const lockReason = 'orca-create-preparation:v1:compat\n' + try { + await runGit([ + 'worktree', + 'add', + '--detach', + '--no-checkout', + '--lock', + '--reason', + lockReason.slice(0, -1), + 'compat-prepared', + 'HEAD' + ]) + expect(supports(2, 33)).toBe(true) + } catch (error) { + expect(supports(2, 33)).toBe(false) + expect(isUnsupportedWorktreeAddLockReasonError(error)).toBe(true) + await runGit(['worktree', 'add', '--detach', '--no-checkout', 'compat-prepared', 'HEAD']) + await writeFile(lockPath, lockReason, { flag: 'wx' }) + } + await expect(readFile(lockPath, 'utf8')).resolves.toBe(lockReason) + await expect(readFile(join(repoPath, 'compat-prepared', 'tracked.txt'))).rejects.toThrow() await runGit(['-C', 'compat-prepared', 'reset', '--hard', 'HEAD']) - await runGit([ - 'worktree', - 'lock', - '--reason', - 'orca-create-preparation:v1:compat', - 'compat-prepared' - ]) + expect( + (await runGit(['-C', 'compat-prepared', 'rev-parse', '--git-path', 'locked'])).stdout.trim() + ).toContain('worktrees/compat-prepared/locked') + expect( + (await runGit(['-C', 'compat-prepared', 'rev-parse', '--git-common-dir'])).stdout.trim() + ).toContain('.git') // Why: `-f -f` moves a locked preparation while preserving its lock reason (Git >=2.25). await runGit(['worktree', 'move', '-f', '-f', 'compat-prepared', 'compat-final']) await runGit([ @@ -233,7 +255,8 @@ describeBinaryCompatibility('real Git binary compatibility', () => { await expect(runGit(['-C', 'compat-final', 'rev-parse', 'HEAD'])).resolves.toMatchObject({ stdout: `${head}\n` }) - await runGit(['worktree', 'unlock', 'compat-final']) + await expect(readFile(lockPath, 'utf8')).resolves.toBe(lockReason) + await unlink(lockPath) await runGit(['worktree', 'remove', '--force', 'compat-final']) await runGit(['branch', '-D', 'compat-prepared-final']) }) @@ -433,6 +456,23 @@ describeBinaryCompatibility('real Git binary compatibility', () => { await runGit(['config', '--unset', 'maintenance.auto']) }) + it('writes a multi-pack-index and reads packed objects at the baseline', async () => { + const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() + await runGit(['-c', 'gc.auto=0', 'repack', '-d']) + await expect(runGit(['multi-pack-index', 'write'])).resolves.toBeDefined() + const midx = await readFile(join(repoPath, '.git', 'objects', 'pack', 'multi-pack-index')) + expect(midx.subarray(0, 4).toString()).toBe('MIDX') + await expect(runGit(['multi-pack-index', 'verify'])).resolves.toMatchObject({ stderr: '' }) + await expect( + runGit(['-c', 'core.multiPackIndex=true', 'cat-file', '-t', head]) + ).resolves.toMatchObject({ stdout: 'commit\n' }) + await runGit(['config', 'core.multiPackIndex', 'false']) + await expect( + runGit(['config', '--bool', '--get', 'core.multiPackIndex']) + ).resolves.toMatchObject({ stdout: 'false\n' }) + await runGit(['config', '--unset', 'core.multiPackIndex']) + }) + it('fetches hosted review heads into dedicated refs', async () => { const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() await runGit(['update-ref', 'refs/pull/42/head', head]) diff --git a/src/shared/git-capability-cache.ts b/src/shared/git-capability-cache.ts index 06d0e957c74..5b77e01d1fb 100644 --- a/src/shared/git-capability-cache.ts +++ b/src/shared/git-capability-cache.ts @@ -10,6 +10,7 @@ export type GitCapability = | 'merge-tree-merge-base' | 'merge-tree-write-tree' | 'rev-parse-path-format' + | 'worktree-add-lock-reason' | 'worktree-list-z' export class GitCapabilityCache extends CapabilityProbeCache { diff --git a/src/shared/git-worktree-command-capabilities.ts b/src/shared/git-worktree-command-capabilities.ts index 76bd705432c..fa3f38dab0f 100644 --- a/src/shared/git-worktree-command-capabilities.ts +++ b/src/shared/git-worktree-command-capabilities.ts @@ -29,6 +29,12 @@ export function isUnsupportedRevParsePathFormatError(error: unknown): boolean { ) } +export function isUnsupportedWorktreeAddLockReasonError(error: unknown): boolean { + return /\b(?:unknown|invalid|unrecognized) (?:switch|option)[^\r\n]*(?:--)?reason\b/i.test( + getGitErrorText(error) + ) +} + export function hasUnsupportedRevParsePathFormatEcho(output: string): boolean { return output.split(/\r?\n/).some((line) => line.startsWith('--path-format')) } diff --git a/src/shared/repo-maintenance-schedule.ts b/src/shared/repo-maintenance-schedule.ts new file mode 100644 index 00000000000..cbc5764b695 --- /dev/null +++ b/src/shared/repo-maintenance-schedule.ts @@ -0,0 +1,69 @@ +import { BoundedMap } from './bounded-map' +import { + PACK_INDEX_MAINTENANCE_COOLDOWN_MS, + PACK_INDEX_MAINTENANCE_FAILURE_COOLDOWN_MS, + type PackIndexMaintenanceOutcome +} from './repo-pack-index-maintenance-policy' +import { + REF_MAINTENANCE_CLEAN_COOLDOWN_MS, + type RefMaintenanceOutcome, + type RefMaintenanceSpan, + type RepoRefMaintenanceTarget +} from './repo-ref-maintenance-policy' + +export class RepoMaintenanceSchedule { + private readonly refCooldownUntil = new BoundedMap({ maxEntries: 256 }) + private readonly indexCooldownUntil = new BoundedMap({ maxEntries: 256 }) + + constructor(private readonly now: () => number) {} + + refDueAt(key: string): number { + return this.refCooldownUntil.peek(key) ?? 0 + } + + indexDueAt(key: string): number { + return this.indexCooldownUntil.get(key) ?? 0 + } + + postponeIndex(key: string, cooldownMs: number): void { + this.indexCooldownUntil.set(key, this.now() + cooldownMs) + } + + clear(): void { + this.refCooldownUntil.clear() + this.indexCooldownUntil.clear() + } + + settleRefs( + key: string, + span: RefMaintenanceSpan, + outcome: RefMaintenanceOutcome, + cooldownMs: number + ): void { + span.setAttribute('repo.maintenance_outcome', outcome) + this.refCooldownUntil.set(key, this.now() + cooldownMs) + } + + async maintain( + target: RepoRefMaintenanceTarget, + signal: AbortSignal, + span: RefMaintenanceSpan, + canWrite: () => boolean + ): Promise { + if (!target.maintainPackIndex || this.now() < this.indexDueAt(target.key)) { + return + } + const outcome = await target.maintainPackIndex(signal, span, canWrite) + if (signal.aborted || outcome === 'deferred') { + return outcome + } + const cooldown = + outcome === 'failed' + ? PACK_INDEX_MAINTENANCE_FAILURE_COOLDOWN_MS + : outcome === 'opted_out' || outcome === 'protected' + ? REF_MAINTENANCE_CLEAN_COOLDOWN_MS + : PACK_INDEX_MAINTENANCE_COOLDOWN_MS + this.postponeIndex(target.key, cooldown) + return outcome + } +} diff --git a/src/shared/repo-pack-index-maintenance-policy.ts b/src/shared/repo-pack-index-maintenance-policy.ts new file mode 100644 index 00000000000..25795e2d2a5 --- /dev/null +++ b/src/shared/repo-pack-index-maintenance-policy.ts @@ -0,0 +1,11 @@ +export const PACK_INDEX_MAINTENANCE_COOLDOWN_MS = 60 * 60_000 +export const PACK_INDEX_MAINTENANCE_FAILURE_COOLDOWN_MS = 30 * 60_000 + +export type PackIndexMaintenanceOutcome = + | 'written' + | 'unchanged' + | 'below_threshold' + | 'opted_out' + | 'protected' + | 'deferred' + | 'failed' diff --git a/src/shared/repo-pack-index-scheduling.test.ts b/src/shared/repo-pack-index-scheduling.test.ts new file mode 100644 index 00000000000..109e7d252a0 --- /dev/null +++ b/src/shared/repo-pack-index-scheduling.test.ts @@ -0,0 +1,155 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + PACK_INDEX_MAINTENANCE_COOLDOWN_MS, + PACK_INDEX_MAINTENANCE_FAILURE_COOLDOWN_MS +} from './repo-pack-index-maintenance-policy' +import { RepoRefMaintenance } from './repo-ref-maintenance' +import { + REF_MAINTENANCE_CLEAN_COOLDOWN_MS, + REF_MAINTENANCE_PACKED_COOLDOWN_MS, + type RepoRefMaintenanceTarget +} from './repo-ref-maintenance-policy' + +const { countLooseRefsMock } = vi.hoisted(() => ({ countLooseRefsMock: vi.fn() })) +vi.mock('./loose-ref-count', () => ({ countLooseRefs: countLooseRefsMock })) + +const QUIET_MS = 10 +const engines: RepoRefMaintenance[] = [] + +function fixture(withIndex = true) { + const resolveRefsDirectory = vi.fn(async () => '/repo/.git/refs') + const packRefs = vi.fn(async () => {}) + const maintainPackIndex = vi.fn(async () => 'written' as const) + const target: RepoRefMaintenanceTarget = { + key: 'local::/repo/.git', + resolveRefsDirectory, + packRefs, + ...(withIndex ? { maintainPackIndex } : {}) + } + const maintenance = new RepoRefMaintenance({ quietPeriodMs: QUIET_MS, looseRefThreshold: 10 }) + engines.push(maintenance) + return { maintenance, target, resolveRefsDirectory, packRefs, maintainPackIndex } +} + +async function advance(maintenance: RepoRefMaintenance, milliseconds: number): Promise { + await vi.advanceTimersByTimeAsync(milliseconds) + await maintenance.whenAttemptSettled() +} + +beforeEach(() => { + vi.useFakeTimers() + countLooseRefsMock.mockResolvedValue({ count: 0, saturated: false }) +}) + +afterEach(() => { + for (const maintenance of engines.splice(0)) { + maintenance.dispose() + } + vi.useRealTimers() + vi.resetAllMocks() +}) + +describe('independent idle pack-index scheduling', () => { + it('keeps a dirty ref arm until its cooldown ends without needing another fetch', async () => { + countLooseRefsMock.mockResolvedValue({ count: 20, saturated: true }) + const { maintenance, target, packRefs } = fixture(false) + maintenance.arm(target) + await advance(maintenance, QUIET_MS) + expect(packRefs).toHaveBeenCalledTimes(1) + maintenance.arm(target) + await advance(maintenance, REF_MAINTENANCE_PACKED_COOLDOWN_MS - 1) + expect(packRefs).toHaveBeenCalledTimes(1) + await advance(maintenance, 1) + expect(packRefs).toHaveBeenCalledTimes(2) + }) + + it('refreshes changed packs while a low-ref repository is still on ref cooldown', async () => { + const { maintenance, target, resolveRefsDirectory, maintainPackIndex } = fixture() + maintenance.arm(target) + await advance(maintenance, QUIET_MS) + maintenance.arm(target) + await advance(maintenance, PACK_INDEX_MAINTENANCE_COOLDOWN_MS - 1) + expect(maintainPackIndex).toHaveBeenCalledTimes(1) + await advance(maintenance, 1) + expect(maintainPackIndex).toHaveBeenCalledTimes(2) + expect(resolveRefsDirectory).toHaveBeenCalledTimes(1) + expect(PACK_INDEX_MAINTENANCE_COOLDOWN_MS).toBeLessThan(REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + }) + + it('keeps an index-only arm until its ref cooldown ends without needing another fetch', async () => { + const { maintenance, target, resolveRefsDirectory, maintainPackIndex } = fixture() + maintenance.arm(target) + await advance(maintenance, QUIET_MS) + await advance(maintenance, PACK_INDEX_MAINTENANCE_COOLDOWN_MS) + maintenance.arm(target) + await advance(maintenance, QUIET_MS) + expect(maintainPackIndex).toHaveBeenCalledTimes(2) + expect(resolveRefsDirectory).toHaveBeenCalledTimes(1) + await advance(maintenance, REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + expect(resolveRefsDirectory).toHaveBeenCalledTimes(2) + }) + + it('retries a failed index sooner without re-probing healthy refs', async () => { + const { maintenance, target, maintainPackIndex, resolveRefsDirectory } = fixture() + target.maintainPackIndex = vi.fn(async () => 'failed' as const) + maintenance.arm(target) + await advance(maintenance, QUIET_MS) + maintenance.arm(target) + await advance(maintenance, PACK_INDEX_MAINTENANCE_FAILURE_COOLDOWN_MS) + expect(target.maintainPackIndex).toHaveBeenCalledTimes(2) + expect(resolveRefsDirectory).toHaveBeenCalledTimes(1) + expect(maintainPackIndex).not.toHaveBeenCalled() + }) + + it('honours idle admission again when a preserved cooldown arm becomes eligible', async () => { + const { maintenance, target, maintainPackIndex } = fixture() + let busy = false + target.isBusy = () => busy + maintenance.arm(target) + await advance(maintenance, QUIET_MS) + maintenance.arm(target) + busy = true + await advance(maintenance, PACK_INDEX_MAINTENANCE_COOLDOWN_MS) + expect(maintainPackIndex).toHaveBeenCalledTimes(1) + busy = false + await advance(maintenance, QUIET_MS * 2) + expect(maintainPackIndex).toHaveBeenCalledTimes(2) + }) + + it('preserves protected user indexes for the longer clean cooldown', async () => { + const { maintenance, target } = fixture() + target.maintainPackIndex = vi.fn(async () => 'protected' as const) + maintenance.arm(target) + await advance(maintenance, QUIET_MS) + maintenance.arm(target) + await advance(maintenance, PACK_INDEX_MAINTENANCE_COOLDOWN_MS) + expect(target.maintainPackIndex).toHaveBeenCalledTimes(1) + await advance(maintenance, REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + expect(target.maintainPackIndex).toHaveBeenCalledTimes(2) + }) + + it('lets a create pause return while an admitted index writer is still running', async () => { + const { maintenance, target, packRefs } = fixture() + let started = () => {} + const writerStarted = new Promise((resolve) => { + started = resolve + }) + let finish = () => {} + const writer = new Promise<'written'>((resolve) => { + finish = () => resolve('written') + }) + target.maintainPackIndex = async () => { + started() + return writer + } + maintenance.arm(target) + await vi.advanceTimersByTimeAsync(QUIET_MS) + await writerStarted + const release = await maintenance.pause('worktree create') + expect(packRefs).not.toHaveBeenCalled() + finish() + await maintenance.whenAttemptSettled() + expect(packRefs).not.toHaveBeenCalled() + release() + }) +}) diff --git a/src/shared/repo-ref-maintenance-policy.ts b/src/shared/repo-ref-maintenance-policy.ts index 7dc8edd1de3..763bfb1d0dc 100644 --- a/src/shared/repo-ref-maintenance-policy.ts +++ b/src/shared/repo-ref-maintenance-policy.ts @@ -1,3 +1,5 @@ +import type { PackIndexMaintenanceOutcome } from './repo-pack-index-maintenance-policy' + /** * Idle-time loose-ref packing for repositories Orca itself degrades. * @@ -101,6 +103,7 @@ export type RefMaintenanceOutcome = | 'locked' | 'timed_out' | 'failed' + | 'index_only' /** Structurally satisfied by the tracer's `ActiveSpan`. */ export type RefMaintenanceSpan = { @@ -116,6 +119,12 @@ export type RepoRefMaintenanceTarget = { isOptedOut?(signal: AbortSignal): Promise /** True while work on *this repo* is in flight -- a fetch, a create, a removal. */ isBusy?(): boolean + /** Repair object lookup metadata before counting refs, under the same idle admission. */ + maintainPackIndex?( + signal: AbortSignal, + span: RefMaintenanceSpan, + canWrite: () => boolean + ): Promise /** * Runs `pack-refs` to completion. Deliberately takes no abort signal: killing * a pack is measurably worse than waiting for it (see `PACKED_REFS_LOCK_*`). diff --git a/src/shared/repo-ref-maintenance.test.ts b/src/shared/repo-ref-maintenance.test.ts index f59b894748f..c9940d13dd3 100644 --- a/src/shared/repo-ref-maintenance.test.ts +++ b/src/shared/repo-ref-maintenance.test.ts @@ -203,6 +203,52 @@ describe('RepoRefMaintenance gating', () => { }) }) + it('maintains the pack index even when the loose-ref backlog is small', async () => { + const refs = await refsDirectoryWith(1) + const { maintenance, packRefs } = createHarness() + const maintainPackIndex = vi.fn(async () => {}) + maintenance.arm(target('local::/fragmented/.git', refs, packRefs, { maintainPackIndex })) + await elapseQuietPeriod(maintenance) + expect(maintainPackIndex).toHaveBeenCalledTimes(1) + expect(packRefs).not.toHaveBeenCalled() + }) + + it('rechecks activity after config probes before starting object maintenance', async () => { + const refs = await refsDirectoryWith(THRESHOLD + 2) + let busy = false + const { maintenance, packRefs } = createHarness({ isBusy: () => busy }) + const maintainPackIndex = vi.fn(async () => {}) + maintenance.arm( + target('local::/busy-after-probe/.git', refs, packRefs, { + isOptedOut: async () => { + busy = true + return false + }, + maintainPackIndex + }) + ) + await elapseQuietPeriod(maintenance) + expect(maintainPackIndex).not.toHaveBeenCalled() + expect(packRefs).not.toHaveBeenCalled() + maintenance.dispose() + }) + + it('rechecks activity after object maintenance before packing refs', async () => { + const refs = await refsDirectoryWith(THRESHOLD + 2) + let busy = false + const { maintenance, packRefs } = createHarness({ isBusy: () => busy }) + maintenance.arm( + target('local::/busy-after-index/.git', refs, packRefs, { + maintainPackIndex: async () => { + busy = true + } + }) + ) + await elapseQuietPeriod(maintenance) + expect(packRefs).not.toHaveBeenCalled() + maintenance.dispose() + }) + it('does not run while the app is busy', async () => { const refs = await refsDirectoryWith(THRESHOLD + 2) let busy = true @@ -232,12 +278,17 @@ describe('RepoRefMaintenance gating', () => { const refs = await refsDirectoryWith(THRESHOLD + 2) const { maintenance, spans, packRefs } = createHarness() + const maintainPackIndex = vi.fn(async () => {}) maintenance.arm( - target('local::/opted-out/.git', refs, packRefs, { isOptedOut: async () => true }) + target('local::/opted-out/.git', refs, packRefs, { + isOptedOut: async () => true, + maintainPackIndex + }) ) await elapseQuietPeriod(maintenance) expect(packRefs).not.toHaveBeenCalled() + expect(maintainPackIndex).not.toHaveBeenCalled() expect(attributesOf(spans[0])['repo.maintenance_outcome']).toBe('opted_out') }) @@ -286,6 +337,37 @@ describe('RepoRefMaintenance gating', () => { }) describe('RepoRefMaintenance single-flight and backoff', () => { + it('serializes pack-index writes across execution hosts without merging their identities', async () => { + const refs = await refsDirectoryWith(1) + const { maintenance, packRefs } = createHarness() + const releases: (() => void)[] = [] + const started: string[] = [] + let concurrent = 0 + let peak = 0 + const indexTarget = (key: string) => + target(key, refs, packRefs, { + maintainPackIndex: async () => { + started.push(key) + concurrent += 1 + peak = Math.max(peak, concurrent) + await new Promise((resolve) => releases.push(resolve)) + concurrent -= 1 + } + }) + maintenance.arm(indexTarget('local::/repo/.git')) + maintenance.arm(indexTarget('wsl:Ubuntu::/repo/.git')) + await vi.advanceTimersByTimeAsync(QUIET_MS) + await until(() => concurrent === 1, 'the first index write') + releases.shift()?.() + await maintenance.whenAttemptSettled() + await untilWithTimers(() => started.length === 2, 'the second execution host') + releases.shift()?.() + await maintenance.whenAttemptSettled() + expect(peak).toBe(1) + expect(started).toEqual(['local::/repo/.git', 'wsl:Ubuntu::/repo/.git']) + expect(packRefs).not.toHaveBeenCalled() + }) + it('runs one repository at a time', async () => { const refs = await refsDirectoryWith(THRESHOLD + 2) let concurrent = 0 diff --git a/src/shared/repo-ref-maintenance.ts b/src/shared/repo-ref-maintenance.ts index 97e228fab52..bc4cf3ed9d8 100644 --- a/src/shared/repo-ref-maintenance.ts +++ b/src/shared/repo-ref-maintenance.ts @@ -1,5 +1,6 @@ import { countLooseRefs } from './loose-ref-count' import { PackedRefsLockGate } from './packed-refs-lock-gate' +import { RepoMaintenanceSchedule } from './repo-maintenance-schedule' import { LOOSE_REF_PACK_THRESHOLD, REF_MAINTENANCE_ATTEMPT_DEADLINE_MS, @@ -45,7 +46,7 @@ function hitDeadline(signal: AbortSignal): boolean { export class RepoRefMaintenance { private readonly tracked = new Map() - private readonly cooldownUntil = new Map() + private readonly phases: RepoMaintenanceSchedule private readonly now: () => number private readonly isAppBusy: () => boolean private readonly observe: NonNullable @@ -67,6 +68,7 @@ export class RepoRefMaintenance { constructor(options: RepoRefMaintenanceOptions = {}) { this.now = options.now ?? Date.now + this.phases = new RepoMaintenanceSchedule(this.now) this.isAppBusy = options.isBusy ?? (() => false) this.observe = options.observe ?? ((attempt) => attempt(noopSpan)) this.quietPeriodMs = options.quietPeriodMs ?? REF_MAINTENANCE_QUIET_PERIOD_MS @@ -167,7 +169,7 @@ export class RepoRefMaintenance { } } this.tracked.clear() - this.cooldownUntil.clear() + this.phases.clear() } private isBusy(tracked: TrackedRepo): boolean { @@ -227,8 +229,12 @@ export class RepoRefMaintenance { return } this.tracked.delete(key) - const cooldownUntil = this.cooldownUntil.get(key) - if (cooldownUntil !== undefined && this.now() < cooldownUntil) { + const refDueAt = this.phases.refDueAt(key) + const indexDueAt = tracked.target.maintainPackIndex ? this.phases.indexDueAt(key) : refDueAt + const nextDueAt = Math.min(refDueAt, indexDueAt) + if (this.now() < nextDueAt) { + this.tracked.set(key, tracked) + this.schedule(key, tracked, nextDueAt - this.now()) return } if (this.inFlight !== null) { @@ -266,19 +272,53 @@ export class RepoRefMaintenance { signal: AbortSignal ): Promise { span.setAttribute('repo.maintenance_key', key) - // Every await below carries the signal, so a caller waiting in `pause()` is - // never stuck behind a probe that has already been told to stop. + // Probes are cancellable; admitted index and ref writers finish before releasing the slot. if (await tracked.target.isOptedOut?.(signal)) { - this.settle(key, span, 'opted_out', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + this.phases.postponeIndex(key, REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + this.phases.settleRefs(key, span, 'opted_out', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) return } if (signal.aborted) { this.yieldTo(key, tracked, span, signal) return } + if (this.suspensions > 0 || this.isBusy(tracked)) { + span.setAttribute('repo.maintenance_outcome', 'deferred' satisfies RefMaintenanceOutcome) + this.defer(key, tracked, true) + return + } + const indexOutcome = await this.phases.maintain( + tracked.target, + signal, + span, + () => !signal.aborted && this.suspensions === 0 && !this.isBusy(tracked) + ) + if (indexOutcome === 'deferred') { + span.setAttribute('repo.maintenance_outcome', 'deferred' satisfies RefMaintenanceOutcome) + this.defer(key, tracked, true) + return + } + if (signal.aborted) { + this.yieldTo(key, tracked, span, signal) + return + } + if (this.suspensions > 0 || this.isBusy(tracked)) { + span.setAttribute('repo.maintenance_outcome', 'deferred' satisfies RefMaintenanceOutcome) + this.defer(key, tracked, true) + return + } + if (this.now() < this.phases.refDueAt(key)) { + span.setAttribute('repo.maintenance_outcome', 'index_only' satisfies RefMaintenanceOutcome) + // Keep the arm for its ref phase; the write that armed it may have left loose refs. + if (!this.disposed && !this.tracked.has(key)) { + this.tracked.set(key, tracked) + this.schedule(key, tracked, this.phases.refDueAt(key) - this.now()) + } + return + } const refsDirectory = await tracked.target.resolveRefsDirectory(signal) if (!refsDirectory) { - this.settle(key, span, 'unresolved', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + this.phases.settleRefs(key, span, 'unresolved', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) return } const budget = this.looseRefThreshold + 1 @@ -291,7 +331,7 @@ export class RepoRefMaintenance { span.setAttribute('git.loose_ref_threshold', this.looseRefThreshold) // A saturated walk stopped early, so `count` is a floor -- never read it as "clean". if (!before.saturated && before.count < this.looseRefThreshold) { - this.settle(key, span, 'below_threshold', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + this.phases.settleRefs(key, span, 'below_threshold', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) return } // The quiet window can close while the probe walks; re-check before spending a git slot. @@ -309,7 +349,7 @@ export class RepoRefMaintenance { } catch (error) { span.setAttribute('repo.maintenance_error', String(error)) if (error instanceof RefMaintenanceRepoLocked) { - this.settle(key, span, 'locked', REF_MAINTENANCE_LOCKED_COOLDOWN_MS) + this.phases.settleRefs(key, span, 'locked', REF_MAINTENANCE_LOCKED_COOLDOWN_MS) return } partial = true @@ -324,11 +364,11 @@ export class RepoRefMaintenance { const after = await countLooseRefs(refsDirectory, budget, signal) span.setAttribute('git.loose_ref_count_after', after.count) if (partial && (after.saturated || after.count >= this.looseRefThreshold)) { - this.settle(key, span, 'failed', REF_MAINTENANCE_FAILURE_COOLDOWN_MS) + this.phases.settleRefs(key, span, 'failed', REF_MAINTENANCE_FAILURE_COOLDOWN_MS) return } span.setAttribute('git.pack_refs_partial', partial) - this.settle(key, span, 'packed', REF_MAINTENANCE_PACKED_COOLDOWN_MS) + this.phases.settleRefs(key, span, 'packed', REF_MAINTENANCE_PACKED_COOLDOWN_MS) } /** Record an aborted attempt: retry soon if Orca yielded, back off if it stalled. */ @@ -339,28 +379,10 @@ export class RepoRefMaintenance { signal: AbortSignal ): void { if (hitDeadline(signal)) { - this.settle(key, span, 'timed_out', REF_MAINTENANCE_FAILURE_COOLDOWN_MS) + this.phases.settleRefs(key, span, 'timed_out', REF_MAINTENANCE_FAILURE_COOLDOWN_MS) return } span.setAttribute('repo.maintenance_outcome', 'interrupted' satisfies RefMaintenanceOutcome) this.defer(key, tracked, false) } - - private settle( - key: string, - span: RefMaintenanceSpan, - outcome: RefMaintenanceOutcome, - cooldownMs: number - ): void { - span.setAttribute('repo.maintenance_outcome', outcome) - // Re-insert so Map order stays newest-last and the eviction below drops the oldest. - this.cooldownUntil.delete(key) - this.cooldownUntil.set(key, this.now() + cooldownMs) - if (this.cooldownUntil.size > MAX_TRACKED_REPOS * 4) { - const oldest = this.cooldownUntil.keys().next() - if (!oldest.done) { - this.cooldownUntil.delete(oldest.value) - } - } - } }