From 34ae0933e4275675b28041d1ee7d84667d84de26 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 1 Oct 2026 23:37:05 -0700 Subject: [PATCH] fix(worktrees): keep creation fast in large repositories (#24346) * fix(worktrees): remove repeated scans and keep prepared checkouts fresh * fix(worktrees): reclaim unlocked fallback preparations safely * refactor(worktrees): simplify creation ownership and idle maintenance * fix(git): keep ref maintenance armed after an index-only pass An idle attempt that found the pack index due but refs still cooling down returned without rescheduling, so loose refs from the arming fetch waited for the next write instead of the ref cooldown. --- docs/reference/git-compatibility.md | 14 + .../__mocks__/worktree-create-preparation.ts | 114 ++++++ .../git/local-repo-ref-maintenance.test.ts | 17 +- src/main/git/local-repo-ref-maintenance.ts | 34 +- .../git/repo-pack-index-maintenance.test.ts | 258 +++++++++++++ src/main/git/repo-pack-index-maintenance.ts | 122 ++++++ src/main/git/repo-pack-index-state.test.ts | 63 ++++ src/main/git/repo-pack-index-state.ts | 94 +++++ .../git/repo-ref-maintenance-real-git.test.ts | 162 +++++++- .../worktree-create-admission-tier.test.ts | 22 +- ...ktree-create-git-executor-real-git.test.ts | 5 +- ...rktree-create-preparation-real-git.test.ts | 356 ++++++++++++++++-- ...rktree-create-preparation-real-wsl.test.ts | 16 +- src/main/git/worktree-create-preparation.ts | 194 ++++------ ...rktree-mutation-route-invalidation.test.ts | 45 ++- src/main/git/worktree-preparation-add.test.ts | 332 ++++++++++++++++ src/main/git/worktree-preparation-add.ts | 119 ++++++ .../git/worktree-preparation-base-oid.test.ts | 35 +- ...e-preparation-cancel-latency.bench.test.ts | 22 +- src/main/git/worktree-preparation-discard.ts | 89 +++++ ...paration-fallback-cleanup-real-git.test.ts | 207 ++++++++++ ...worktree-preparation-lock-real-git.test.ts | 336 +++++++++++++++++ .../git/worktree-preparation-lock.test.ts | 193 ++++++++++ src/main/git/worktree-preparation-lock.ts | 130 +++++++ ...e-preparation-tip-refresh-real-git.test.ts | 156 ++++++++ .../worktree-preparation-tip-refresh.test.ts | 120 ++++++ .../git/worktree-preparation-tip-refresh.ts | 60 +++ .../created-worktree-reconciliation.test.ts | 50 ++- .../ipc/created-worktree-reconciliation.ts | 74 ++-- src/main/ipc/worktree-remote.ts | 66 ++-- .../ipc/worktrees-local-create-flow.test.ts | 30 +- .../ipc/worktrees-wsl-runtime-routing.test.ts | 7 +- .../register-worktree-prefetch-handler.ts | 3 +- .../orca-runtime-activate-managed-worktree.ts | 46 ++- .../orca-runtime-create-base-prefetch.test.ts | 3 +- .../orca-runtime-create-managed-worktree.ts | 4 +- ...ca-runtime-create-terminal-dependencies.ts | 9 +- .../orca-runtime-create-terminal-desktop.ts | 6 +- .../runtime/orca-runtime-create-terminal.ts | 28 +- ...get-worktree-terminal-provisioning-host.ts | 19 +- ...ser-network-execution-host-for-worktree.ts | 14 +- .../orca-runtime-split-pty-backed-terminal.ts | 9 +- .../runtime/orca-runtime-split-terminal.ts | 7 +- ...orktree-removal-and-reconciliation.spec.ts | 12 +- ...worktree-setup-and-startup-part-02.spec.ts | 8 +- ...e-created-worktree-terminal-target.test.ts | 238 ++++++++++++ ...untime-created-worktree-terminal-target.ts | 106 ++++++ .../runtime/runtime-terminal-pane-identity.ts | 21 +- .../worktree-create-base-prefetch.test.ts | 56 ++- src/main/worktree-create-base-prefetch.ts | 30 +- ...ee-create-preparation-cancellation.test.ts | 2 +- src/main/worktree-create-preparation-pool.ts | 128 ++++--- ...rktree-create-preparation-stale-cleanup.ts | 16 +- src/main/worktree-create-preparation.test.ts | 351 ++--------------- src/main/worktree-create-preparation.ts | 24 +- ...worktree-preparation-discard-retry.test.ts | 241 ++++++++++++ .../worktree-preparation-discard-retry.ts | 33 +- ...worktree-preparation-fetch-barrier.test.ts | 128 +++++++ .../worktree-preparation-refresh-queue.ts | 42 +++ .../worktree-preparation-tip-refresh.test.ts | 225 +++++++++++ src/shared/git-binary-compatibility.test.ts | 60 ++- src/shared/git-capability-cache.ts | 1 + .../git-worktree-command-capabilities.ts | 6 + src/shared/repo-maintenance-schedule.ts | 69 ++++ .../repo-pack-index-maintenance-policy.ts | 11 + src/shared/repo-pack-index-scheduling.test.ts | 155 ++++++++ src/shared/repo-ref-maintenance-policy.ts | 9 + src/shared/repo-ref-maintenance.test.ts | 84 ++++- src/shared/repo-ref-maintenance.ts | 84 +++-- 69 files changed, 5024 insertions(+), 806 deletions(-) create mode 100644 src/main/__mocks__/worktree-create-preparation.ts create mode 100644 src/main/git/repo-pack-index-maintenance.test.ts create mode 100644 src/main/git/repo-pack-index-maintenance.ts create mode 100644 src/main/git/repo-pack-index-state.test.ts create mode 100644 src/main/git/repo-pack-index-state.ts create mode 100644 src/main/git/worktree-preparation-add.test.ts create mode 100644 src/main/git/worktree-preparation-add.ts create mode 100644 src/main/git/worktree-preparation-discard.ts create mode 100644 src/main/git/worktree-preparation-fallback-cleanup-real-git.test.ts create mode 100644 src/main/git/worktree-preparation-lock-real-git.test.ts create mode 100644 src/main/git/worktree-preparation-lock.test.ts create mode 100644 src/main/git/worktree-preparation-lock.ts create mode 100644 src/main/git/worktree-preparation-tip-refresh-real-git.test.ts create mode 100644 src/main/git/worktree-preparation-tip-refresh.test.ts create mode 100644 src/main/git/worktree-preparation-tip-refresh.ts create mode 100644 src/main/runtime/runtime-created-worktree-terminal-target.test.ts create mode 100644 src/main/runtime/runtime-created-worktree-terminal-target.ts create mode 100644 src/main/worktree-preparation-discard-retry.test.ts create mode 100644 src/main/worktree-preparation-fetch-barrier.test.ts create mode 100644 src/main/worktree-preparation-refresh-queue.ts create mode 100644 src/main/worktree-preparation-tip-refresh.test.ts create mode 100644 src/shared/repo-maintenance-schedule.ts create mode 100644 src/shared/repo-pack-index-maintenance-policy.ts create mode 100644 src/shared/repo-pack-index-scheduling.test.ts 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) - } - } - } }