diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index c4ea34e81d3..d0319908122 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -135,10 +135,7 @@ import { getWorktreeCreateCandidate, WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS } from '../worktree-create-candidates' -import { - getRetiredWorktreeNamesForRepo, - retireGeneratedWorktreeName -} from '../worktree-name-retirement' +import { getRetiredWorktreeNamesForRepo } from '../worktree-name-retirement' const SSH_WORKTREE_CREATE_FETCH_FRESHNESS_MS = 30_000 const SSH_WORKTREE_CREATE_FETCH_CACHE_MAX = 512 @@ -1555,7 +1552,7 @@ export async function createRemoteWorktree( let lastBranchConflictKind: 'local' | 'remote' | null = null let remotePathResolved = false const retiredNames = args.nameWasGenerated - ? new Set(getRetiredWorktreeNamesForRepo(store, repo, settings)) + ? new Set(getRetiredWorktreeNamesForRepo(store, repo, store.getRepos(), settings)) : null // Why: duplicate PR/MR checkouts still need a workspace; suffix branch/path while preserving review metadata and push target. for (let suffix = 1; suffix <= WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS; suffix += 1) { @@ -1773,7 +1770,7 @@ export async function createRemoteWorktree( const worktreeId = `${repo.id}::${created.path}` // Why: repo-qualified layouts decorate the physical leaf; retire the name main actually used. if (args.nameWasGenerated) { - retireGeneratedWorktreeName(store, repo, settings, effectiveSanitizedName) + store.addRetiredWorktreeName(repo.id, effectiveSanitizedName) } const now = Date.now() // Why: PR/MR worktrees start from a head ref/SHA but Source Control must compare against the review target branch. @@ -2104,7 +2101,7 @@ export async function createLocalWorktree( let lastExistingPR: Awaited> | null = null let lastExistingReviewNumber: number | null = null const retiredNames = args.nameWasGenerated - ? new Set(getRetiredWorktreeNamesForRepo(store, repo, settings)) + ? new Set(getRetiredWorktreeNamesForRepo(store, repo, store.getRepos(), settings)) : null // Why: a create-from-review branch override may already exist locally; suffix both branch and path instead of blocking the user. for (let suffix = 1; suffix <= WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS; suffix += 1) { @@ -2400,7 +2397,7 @@ export async function createLocalWorktree( const worktreeId = `${repo.id}::${created.path}` // Why: repo-qualified layouts decorate the physical leaf; retire the name main actually used. if (args.nameWasGenerated) { - retireGeneratedWorktreeName(store, repo, settings, effectiveSanitizedName) + store.addRetiredWorktreeName(repo.id, effectiveSanitizedName) } const now = Date.now() // Why: PR/MR worktrees start from a head ref/SHA but Source Control must compare against the review target branch. diff --git a/src/main/ipc/worktrees-windows.test.ts b/src/main/ipc/worktrees-windows.test.ts index 189f2e55141..8efa7c905fc 100644 --- a/src/main/ipc/worktrees-windows.test.ts +++ b/src/main/ipc/worktrees-windows.test.ts @@ -348,7 +348,7 @@ describe('registerWorktreeHandlers – Windows path handling', () => { nameWasGenerated: true }) - expect(store.addRetiredWorktreeName).toHaveBeenCalledWith(expect.any(String), 'nautilus-2') + expect(store.addRetiredWorktreeName).toHaveBeenCalledWith('repo-1', 'nautilus-2') }) it('leaves a user-typed name reusable even when the same name is retired', async () => { diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 460f6e62b85..3ea8e2cfc35 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -4715,7 +4715,7 @@ describe('registerWorktreeHandlers', () => { ) expect(provider.listWorktrees).toHaveBeenCalledTimes(1) expect(provider.worktreeIsClean).not.toHaveBeenCalled() - expect(store.addRetiredWorktreeName).toHaveBeenCalledWith(expect.any(String), 'nautilus-2') + expect(store.addRetiredWorktreeName).toHaveBeenCalledWith('repo-ssh', 'nautilus-2') expect(store.setWorktreeMeta).toHaveBeenCalledWith( 'repo-ssh::/remote/repo-nautilus-2', expect.objectContaining({ diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 62620720d21..a5335ca20de 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -1925,7 +1925,7 @@ export function registerWorktreeHandlers( // Best effort: without the seed we only under-retire, which is the pre-existing behavior. console.warn(`[worktrees] retirement backfill failed for repo ${args.repoId}:`, err) } - return getRetiredWorktreeNamesForRepo(store, repo, store.getSettings()) + return getRetiredWorktreeNamesForRepo(store, repo, store.getRepos(), store.getSettings()) }) ipcMain.handle('worktrees:list', async (_event, args: { repoId: string }) => { diff --git a/src/main/orca-profiles/profile-project-source-removal.ts b/src/main/orca-profiles/profile-project-source-removal.ts index edb923e6293..b120ba0c372 100644 --- a/src/main/orca-profiles/profile-project-source-removal.ts +++ b/src/main/orca-profiles/profile-project-source-removal.ts @@ -18,6 +18,7 @@ export function removeSourceRepo( ...state, repos: state.repos.filter((repo) => repo.id !== repoId), sparsePresetsByRepo: { ...state.sparsePresetsByRepo }, + retiredWorktreeNamesByRepo: { ...state.retiredWorktreeNamesByRepo }, worktreeMeta: { ...state.worktreeMeta }, worktreeLineageById: { ...state.worktreeLineageById }, workspaceLineageByChildKey: { ...state.workspaceLineageByChildKey }, @@ -38,6 +39,7 @@ export function removeSourceRepo( } } delete next.sparsePresetsByRepo[repoId] + delete next.retiredWorktreeNamesByRepo?.[repoId] removeRepoWorktreeMetadata(next, repoId) return rebuildRepoBackedProjectState(next) } diff --git a/src/main/orca-profiles/profile-project-state-file.ts b/src/main/orca-profiles/profile-project-state-file.ts index 4169fea6ffc..bd7fbc8f6be 100644 --- a/src/main/orca-profiles/profile-project-state-file.ts +++ b/src/main/orca-profiles/profile-project-state-file.ts @@ -45,6 +45,7 @@ export function readProfileState(profileId: string, userDataPath: string): Trans projectGroups: arrayOrEmpty(parsed.projectGroups), folderWorkspaces: arrayOrEmpty(parsed.folderWorkspaces), sparsePresetsByRepo: recordOrEmpty(parsed.sparsePresetsByRepo), + retiredWorktreeNamesByRepo: recordOrEmpty(parsed.retiredWorktreeNamesByRepo), worktreeMeta: recordOrEmpty(parsed.worktreeMeta), worktreeLineageById: recordOrEmpty(parsed.worktreeLineageById), workspaceLineageByChildKey: recordOrEmpty(parsed.workspaceLineageByChildKey), diff --git a/src/main/orca-profiles/profile-project-transfer-payload.ts b/src/main/orca-profiles/profile-project-transfer-payload.ts index f8e458df4df..524aa02ad92 100644 --- a/src/main/orca-profiles/profile-project-transfer-payload.ts +++ b/src/main/orca-profiles/profile-project-transfer-payload.ts @@ -20,6 +20,9 @@ import { export type TransferPayload = { repo: Repo sparsePresets: SparsePreset[] + /** Generated names already spent for this repo. Carried so the destination profile does not + * reissue a name whose old directory still holds agent state. */ + retiredWorktreeNames: string[] worktreeMeta: PersistedState['worktreeMeta'] worktreeLineageById: PersistedState['worktreeLineageById'] workspaceLineageByChildKey: PersistedState['workspaceLineageByChildKey'] @@ -214,6 +217,7 @@ export function createTransferPayload(args: { ...structuredClone(preset), repoId: newRepoId })), + retiredWorktreeNames: [...(sourceState.retiredWorktreeNamesByRepo?.[oldRepoId] ?? [])], worktreeMeta: rekeyWorktreeIdRecord( sourceState.worktreeMeta, worktreeIds, @@ -269,6 +273,13 @@ export function applyPayloadToTarget( ...targetState.sparsePresetsByRepo, ...(payload.sparsePresets.length > 0 ? { [payload.repo.id]: payload.sparsePresets } : {}) }, + retiredWorktreeNamesByRepo: { + ...targetState.retiredWorktreeNamesByRepo, + ...(payload.retiredWorktreeNames.length > 0 + ? { [payload.repo.id]: payload.retiredWorktreeNames } + : {}) + }, + worktreeMeta: { ...targetState.worktreeMeta, ...payload.worktreeMeta }, worktreeLineageById: { ...targetState.worktreeLineageById, ...payload.worktreeLineageById }, workspaceLineageByChildKey: { diff --git a/src/main/orca-profiles/profile-project-transfer.test.ts b/src/main/orca-profiles/profile-project-transfer.test.ts index 3c3a4643387..a8983dc4979 100644 --- a/src/main/orca-profiles/profile-project-transfer.test.ts +++ b/src/main/orca-profiles/profile-project-transfer.test.ts @@ -126,6 +126,7 @@ describe('profile project transfer', () => { } ] }, + retiredWorktreeNamesByRepo: { 'repo-1': ['nautilus'] }, worktreeMeta: { [sourceWorktreeId]: makeWorktreeMeta({ projectHostSetupId: 'repo-1' }) }, @@ -177,6 +178,9 @@ describe('profile project transfer', () => { expect(target.sparsePresetsByRepo[targetRepoId]).toEqual([ expect.objectContaining({ id: 'preset-1', repoId: targetRepoId }) ]) + // Why: without the re-key the destination profile reissues a name whose old directory may + // still hold the previous occupant's agent conversation. + expect(target.retiredWorktreeNamesByRepo?.[targetRepoId]).toEqual(['nautilus']) expect(target.workspaceSession.tabsByWorktree).toEqual({}) expect(readProfileState('personal').repos.map((repo) => repo.id)).toEqual(['repo-1']) }) diff --git a/src/main/persistence-worktree-name-retirement.test.ts b/src/main/persistence-worktree-name-retirement.test.ts index 8e296c880d4..6522e3bae0a 100644 --- a/src/main/persistence-worktree-name-retirement.test.ts +++ b/src/main/persistence-worktree-name-retirement.test.ts @@ -3,6 +3,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { getDefaultPersistedState } from '../shared/constants' +import { MARINE_CREATURES } from '../shared/marine-creatures' const testState = { dir: '' } @@ -13,9 +14,8 @@ vi.mock('electron', () => ({ vi.mock('./telemetry/client', () => ({ track: vi.fn() })) vi.mock('./telemetry/cohort-classifier', () => ({ getCohortAtEmit: vi.fn() })) -// The registry is keyed by the cwd namespace a repo creates into, not by repo id. -const KEY = 'local:posix:/workspaces/repo-1' -const OTHER_KEY = 'local:posix:/workspaces/repo-2' +const REPO = 'repo-1' +const OTHER_REPO = 'repo-2' async function createStore(persisted: Record = {}) { mkdirSync(testState.dir, { recursive: true }) @@ -41,81 +41,134 @@ afterEach(() => { describe('worktree name retirement registry', () => { it('retains a retired name so it is never reissued', async () => { const store = await createStore() - store.addRetiredWorktreeName(KEY, 'Nautilus') - expect(store.getRetiredWorktreeNames(KEY)).toEqual(['nautilus']) + store.addRetiredWorktreeName(REPO, 'Nautilus') + expect(store.getRetiredWorktreeNames(REPO)).toEqual(['nautilus']) }) - it('scopes retirement per cwd namespace so one workspace root does not burn another', async () => { + it('scopes retirement per repo so one repo does not burn another pool', async () => { const store = await createStore() - store.addRetiredWorktreeName(KEY, 'nautilus') - expect(store.getRetiredWorktreeNames(OTHER_KEY)).toEqual([]) + store.addRetiredWorktreeName(REPO, 'nautilus') + expect(store.getRetiredWorktreeNames(OTHER_REPO)).toEqual([]) }) it('normalizes case and whitespace so a name cannot be retired twice', async () => { const store = await createStore() - store.addRetiredWorktreeName(KEY, ' NaUtIlUs ') - store.addRetiredWorktreeName(KEY, 'nautilus') - expect(store.getRetiredWorktreeNames(KEY)).toEqual(['nautilus']) + store.addRetiredWorktreeName(REPO, ' NaUtIlUs ') + store.addRetiredWorktreeName(REPO, 'nautilus') + expect(store.getRetiredWorktreeNames(REPO)).toEqual(['nautilus']) }) - it('ignores an empty name or missing workspace key', async () => { + it('ignores an empty name or missing repo id', async () => { const store = await createStore() - store.addRetiredWorktreeName(KEY, ' ') + store.addRetiredWorktreeName(REPO, ' ') store.addRetiredWorktreeName('', 'nautilus') - expect(store.getRetiredWorktreeNames(KEY)).toEqual([]) + expect(store.getRetiredWorktreeNames(REPO)).toEqual([]) }) it('does not persist arbitrary user and issue-title names', async () => { const store = await createStore() - store.addRetiredWorktreeName(KEY, 'fix-login-redirect') - store.addRetiredWorktreeName(KEY, 'STA-4189-duplicate-name') - expect(store.getRetiredWorktreeNames(KEY)).toEqual([]) + store.addRetiredWorktreeName(REPO, 'fix-login-redirect') + store.addRetiredWorktreeName(REPO, 'STA-4189-duplicate-name') + expect(store.getRetiredWorktreeNames(REPO)).toEqual([]) }) it('returns a copy so callers cannot mutate the registry in place', async () => { const store = await createStore() - store.addRetiredWorktreeName(KEY, 'nautilus') - store.getRetiredWorktreeNames(KEY).push('seahorse') - expect(store.getRetiredWorktreeNames(KEY)).toEqual(['nautilus']) + store.addRetiredWorktreeName(REPO, 'nautilus') + store.getRetiredWorktreeNames(REPO).push('seahorse') + expect(store.getRetiredWorktreeNames(REPO)).toEqual(['nautilus']) }) it('merges backfilled names without dropping a concurrent retirement', async () => { const store = await createStore() - store.addRetiredWorktreeName(KEY, 'nautilus') - expect(store.mergeRetiredWorktreeNames(KEY, ['seahorse', 'starfish'])).toBe(true) - expect(store.getRetiredWorktreeNames(KEY).sort()).toEqual(['nautilus', 'seahorse', 'starfish']) + store.addRetiredWorktreeName(REPO, 'nautilus') + expect(store.mergeRetiredWorktreeNames(REPO, ['seahorse', 'starfish'])).toBe(true) + expect(store.getRetiredWorktreeNames(REPO).sort()).toEqual(['nautilus', 'seahorse', 'starfish']) }) it('reports no change when a merge adds nothing new', async () => { const store = await createStore() - store.addRetiredWorktreeName(KEY, 'nautilus') - expect(store.mergeRetiredWorktreeNames(KEY, ['NAUTILUS'])).toBe(false) + store.addRetiredWorktreeName(REPO, 'nautilus') + expect(store.mergeRetiredWorktreeNames(REPO, ['NAUTILUS'])).toBe(false) }) it('survives a reload so retirement outlives the workspace and the app session', async () => { const store = await createStore({ - retiredWorktreeNamesByWorkspaceKey: { [KEY]: ['nautilus', 'seahorse'] } + retiredWorktreeNamesByRepo: { [REPO]: ['nautilus', 'seahorse'] } }) - expect(store.getRetiredWorktreeNames(KEY).sort()).toEqual(['nautilus', 'seahorse']) + expect(store.getRetiredWorktreeNames(REPO).sort()).toEqual(['nautilus', 'seahorse']) }) it('degrades to nothing retired when the persisted map is corrupt', async () => { // Why: a load failure costs the app; over- or under-retiring costs at most a name. - const store = await createStore({ retiredWorktreeNamesByWorkspaceKey: 'not-an-object' }) - expect(store.getRetiredWorktreeNames(KEY)).toEqual([]) + const store = await createStore({ retiredWorktreeNamesByRepo: 'not-an-object' }) + expect(store.getRetiredWorktreeNames(REPO)).toEqual([]) }) it('drops non-string entries but keeps the valid ones', async () => { const store = await createStore({ - retiredWorktreeNamesByWorkspaceKey: { [KEY]: ['nautilus', 42, null, 'Seahorse'] } + retiredWorktreeNamesByRepo: { [REPO]: ['nautilus', 42, null, 'Seahorse'] } }) - expect(store.getRetiredWorktreeNames(KEY).sort()).toEqual(['nautilus', 'seahorse']) + expect(store.getRetiredWorktreeNames(REPO).sort()).toEqual(['nautilus', 'seahorse']) + }) + + it('drops the registry when the repo is removed, matching the sparse-preset convention', async () => { + // Why: entries are repo-id keyed, so without this they orphan forever — a repo id is never + // reused, and remove/re-add mints a new one. + const store = await createStore() + store.addRepo({ id: REPO, path: '/repos/a', displayName: 'a', badgeColor: '', addedAt: 0 }) + store.addRetiredWorktreeName(REPO, 'nautilus') + + store.removeProject(REPO) + + expect(store.getRetiredWorktreeNames(REPO)).toEqual([]) + }) + + it('keeps the registry when one host row is removed but the repo id survives elsewhere', async () => { + const store = await createStore() + store.addRepo({ id: REPO, path: '/repos/a', displayName: 'a', badgeColor: '', addedAt: 0 }) + store.addRepo({ + id: REPO, + path: '/repos/a', + displayName: 'a', + badgeColor: '', + addedAt: 0, + executionHostId: 'runtime:env-1' + } as never) + store.addRetiredWorktreeName(REPO, 'nautilus') + + store.removeProjectForHost(REPO, 'runtime:env-1') + + expect(store.getRetiredWorktreeNames(REPO)).toEqual(['nautilus']) + + store.removeProjectForHost(REPO, 'local') + + expect(store.getRetiredWorktreeNames(REPO)).toEqual([]) + }) + + it('bounds the per-repo registry above the pool so the cap never reissues a base name', async () => { + // The cap must sit well past the 552-name pool: evicting inside it would hand back a name whose + // agent state is still on disk, which is the bug this registry exists to prevent. + const store = await createStore() + const names = MARINE_CREATURES.map((name) => name.toLowerCase()) + for (let tier = 2; tier <= 5; tier += 1) { + store.mergeRetiredWorktreeNames( + REPO, + names.map((name) => `${name}-${tier}`) + ) + } + store.mergeRetiredWorktreeNames(REPO, names) + + const retained = store.getRetiredWorktreeNames(REPO) + expect(retained.length).toBe(2000) + // Oldest-first eviction, so the most recent merge survives intact. + expect(retained).toEqual(expect.arrayContaining(names)) }) it('drops legacy arbitrary names while loading the persisted map', async () => { const store = await createStore({ - retiredWorktreeNamesByWorkspaceKey: { [KEY]: ['fix-login', 'Nautilus'] } + retiredWorktreeNamesByRepo: { [REPO]: ['fix-login', 'Nautilus'] } }) - expect(store.getRetiredWorktreeNames(KEY)).toEqual(['nautilus']) + expect(store.getRetiredWorktreeNames(REPO)).toEqual(['nautilus']) }) }) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 22eea861a07..0aee1c2bd16 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2335,15 +2335,28 @@ const MAX_CLAUDE_LIVE_PTY_SESSION_IDS = 200 // Why: bound removed-SSH-target history so remove/re-add churn can't grow the file unbounded. const MAX_REMOVED_SSH_TARGET_TOMBSTONES = 50 +// Why: bound per-repo retirement so long-lived repos can't grow the file unbounded. Deliberately +// far above the 552-name pool — a cap below it would start reissuing names and reintroduce the bug +// this registry exists to prevent, so only `-2`/`-3` tier accumulation can ever reach it. +const MAX_RETIRED_WORKTREE_NAMES_PER_REPO = 2000 + +/** Oldest-first eviction: entries are appended in retirement order, and the oldest paths are the + * ones whose agent state is likeliest to be gone already. */ +function boundRetiredWorktreeNames(names: readonly string[]): string[] { + return names.length > MAX_RETIRED_WORKTREE_NAMES_PER_REPO + ? names.slice(names.length - MAX_RETIRED_WORKTREE_NAMES_PER_REPO) + : [...names] +} + /** Why: a corrupt or hand-edited map must degrade to "nothing retired" rather than throw during * load — over-retiring costs one name from a 552-entry pool, but a load failure costs the app. */ -function normalizeRetiredWorktreeNamesByWorkspaceKey(value: unknown): Record { +function normalizeRetiredWorktreeNamesByRepo(value: unknown): Record { if (!value || typeof value !== 'object' || Array.isArray(value)) { return {} } - const byKey: Record = {} - for (const [workspaceKey, names] of Object.entries(value as Record)) { - if (!workspaceKey || !Array.isArray(names)) { + const byRepo: Record = {} + for (const [repoId, names] of Object.entries(value as Record)) { + if (!repoId || !Array.isArray(names)) { continue } const seen = new Set() @@ -2357,10 +2370,10 @@ function normalizeRetiredWorktreeNamesByWorkspaceKey(value: unknown): Record 0) { - byKey[workspaceKey] = [...seen] + byRepo[repoId] = boundRetiredWorktreeNames([...seen]) } } - return byKey + return byRepo } function normalizeClaudeLivePtySessionIds(value: unknown): string[] { @@ -3848,8 +3861,8 @@ export class Store { (alias): alias is string => typeof alias === 'string' ) : [], - retiredWorktreeNamesByWorkspaceKey: normalizeRetiredWorktreeNamesByWorkspaceKey( - parsed.retiredWorktreeNamesByWorkspaceKey + retiredWorktreeNamesByRepo: normalizeRetiredWorktreeNamesByRepo( + parsed.retiredWorktreeNamesByRepo ), sshRemotePtyLeases: (parsed.sshRemotePtyLeases ?? []) .map(normalizeSshRemotePtyLease) @@ -4881,6 +4894,8 @@ export class Store { this.syncProjectHostSetupCompatibilityState() // Why: presets are repo-scoped and unreachable once the repo is gone, so drop them with it. delete this.state.sparsePresetsByRepo[id] + // Same convention: retirements are repo-id-keyed, so they would otherwise orphan forever. + delete this.state.retiredWorktreeNamesByRepo?.[id] this.pruneWorktreeStateForRepo(id, null) this.state.workspaceSession = removeRepoFromWorkspaceSession(this.state.workspaceSession, id) this.state.workspaceSessionsByHostId = removeRepoFromHostWorkspaceSessions( @@ -4896,9 +4911,11 @@ export class Store { (r) => !(r.id === id && getRepoExecutionHostId(r) === hostId) ) const idStillPresent = this.state.repos.some((r) => r.id === id) - // Why: presets are repo-id-scoped (not host-scoped); drop them only when the last host's copy is gone. + // Why: presets and retirements are repo-id-scoped (not host-scoped); drop them only when the + // last host's copy is gone. if (!idStillPresent) { delete this.state.sparsePresetsByRepo[id] + delete this.state.retiredWorktreeNamesByRepo?.[id] } this.syncProjectHostSetupCompatibilityState() // Why: prune only this host's worktree metas if the id survives elsewhere; otherwise prune everything (matches removeProject). @@ -7215,32 +7232,32 @@ export class Store { this.scheduleSave() } - getRetiredWorktreeNames(workspaceKey: string): string[] { - return [...(this.state.retiredWorktreeNamesByWorkspaceKey?.[workspaceKey] ?? [])] + getRetiredWorktreeNames(repoId: string): string[] { + return [...(this.state.retiredWorktreeNamesByRepo?.[repoId] ?? [])] } - /** Records a generated workspace name as spent in this cwd namespace. Called with the name main - * actually used, not the one the renderer proposed — create can advance past it on collision. */ - addRetiredWorktreeName(workspaceKey: string, name: string): void { + /** Records a generated workspace name as spent for this repo. Called with the name main actually + * used, not the one the renderer proposed — the create path can advance past it on collision. */ + addRetiredWorktreeName(repoId: string, name: string): void { const normalized = normalizeRetirableGeneratedName(name) - if (!workspaceKey || !normalized) { + if (!repoId || !normalized) { return } - this.state.retiredWorktreeNamesByWorkspaceKey ??= {} - const existing = this.state.retiredWorktreeNamesByWorkspaceKey[workspaceKey] + this.state.retiredWorktreeNamesByRepo ??= {} + const existing = this.state.retiredWorktreeNamesByRepo[repoId] if (existing?.includes(normalized)) { return } - this.state.retiredWorktreeNamesByWorkspaceKey[workspaceKey] = existing - ? [...existing, normalized] - : [normalized] + this.state.retiredWorktreeNamesByRepo[repoId] = boundRetiredWorktreeNames( + existing ? [...existing, normalized] : [normalized] + ) this.scheduleSave() } /** Seeds retirements discovered by the one-time backfill. Merges rather than replaces so a * concurrent create during backfill is not lost. Returns true when anything was added. */ - mergeRetiredWorktreeNames(workspaceKey: string, names: Iterable): boolean { - if (!workspaceKey) { + mergeRetiredWorktreeNames(repoId: string, names: Iterable): boolean { + if (!repoId) { return false } const incoming = new Set() @@ -7253,8 +7270,8 @@ export class Store { if (incoming.size === 0) { return false } - this.state.retiredWorktreeNamesByWorkspaceKey ??= {} - const existing = this.state.retiredWorktreeNamesByWorkspaceKey[workspaceKey] ?? [] + this.state.retiredWorktreeNamesByRepo ??= {} + const existing = this.state.retiredWorktreeNamesByRepo[repoId] ?? [] const merged = new Set(existing) for (const name of incoming) { merged.add(name) @@ -7262,7 +7279,7 @@ export class Store { if (merged.size === existing.length) { return false } - this.state.retiredWorktreeNamesByWorkspaceKey[workspaceKey] = [...merged] + this.state.retiredWorktreeNamesByRepo[repoId] = boundRetiredWorktreeNames([...merged]) this.scheduleSave() return true } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 7013b17b05f..9081df980d1 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -4092,7 +4092,7 @@ describe('OrcaRuntimeService', () => { worktrees: expect.arrayContaining([expect.objectContaining({ path: createdWorktree.path })]) } ) - expect(addRetiredWorktreeName).toHaveBeenCalledWith(expect.any(String), 'nautilus-2') + expect(addRetiredWorktreeName).toHaveBeenCalledWith(TEST_REPO_ID, 'nautilus-2') }) it('neither skips nor retires a name the user typed', async () => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 5c4995b776d..2efcbf73f03 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -936,8 +936,7 @@ import { } from '../worktree-create-candidates' import { ensureRetiredWorktreeNamesBackfilled, - getRetiredWorktreeNamesForRepo, - retireGeneratedWorktreeName + getRetiredWorktreeNamesForRepo } from '../worktree-name-retirement' import { normalizeSparseDirectories } from '../ipc/sparse-checkout-directories' import type { PtyBindingSourceExpectation, Store } from '../persistence' @@ -21180,7 +21179,7 @@ export class OrcaRuntimeService { } catch (error) { console.warn(`[runtime] retirement backfill failed for repo ${repo.id}:`, error) } - return { [repo.id]: getRetiredWorktreeNamesForRepo(store, repo, settings) } + return { [repo.id]: getRetiredWorktreeNamesForRepo(store, repo, store.getRepos(), settings) } } async listDetectedManagedWorktrees( @@ -22374,7 +22373,7 @@ export class OrcaRuntimeService { let worktreePath = '' let worktreePathResolved = false const retiredNames = args.nameWasGenerated - ? new Set(getRetiredWorktreeNamesForRepo(this.store, repo, settings)) + ? new Set(getRetiredWorktreeNamesForRepo(this.store, repo, this.store.getRepos(), settings)) : null // Why: runtime/mobile create-from-review callers should get a new workspace // even when the PR branch or review branch name is already in use. @@ -22707,7 +22706,7 @@ export class OrcaRuntimeService { const worktreeId = `${repo.id}::${created.path}` // Why: repo-qualified layouts decorate the physical leaf; retire the name main actually used. if (args.nameWasGenerated) { - retireGeneratedWorktreeName(this.store, repo, settings, effectiveSanitizedName) + this.store.addRetiredWorktreeName(repo.id, effectiveSanitizedName) } const now = Date.now() // Why: PR/MR-created worktrees can start from a head ref/SHA while Source diff --git a/src/main/worktree-name-retirement.test.ts b/src/main/worktree-name-retirement.test.ts index 52b23b35a4e..d298dccb431 100644 --- a/src/main/worktree-name-retirement.test.ts +++ b/src/main/worktree-name-retirement.test.ts @@ -11,8 +11,7 @@ import { extractBucketLeafCandidates, getRetiredWorktreeNamesForRepo, normalizeRetirableGeneratedName, - resetRetirementCollisionKeyCacheForTests, - retireGeneratedWorktreeName + resetRetirementCollisionKeyCacheForTests } from './worktree-name-retirement' const FIRST = MARINE_CREATURES[0].toLowerCase() @@ -201,62 +200,67 @@ describe('discoverRetiredWorktreeNames', () => { describe('getRetiredWorktreeNamesForRepo', () => { const settingsFor = (nestWorkspaces: boolean): GlobalSettings => ({ workspaceDir: '/workspaces', nestWorkspaces }) as GlobalSettings + const storeOf = (byRepo: Record) => { + const calls: string[] = [] + return { + calls, + getRetiredWorktreeNames: (repoId: string) => { + calls.push(repoId) + return byRepo[repoId] ?? [] + } + } + } it('shares retirements when two repos create into the same cwd namespace', () => { - const byKey = new Map() - const store = { - getRetiredWorktreeNames: (key: string) => byKey.get(key) ?? [], - addRetiredWorktreeName: (key: string, name: string) => - byKey.set(key, [...(byKey.get(key) ?? []), name]) - } - const settings = settingsFor(false) - const repoA = makeRepo('repo-a', '/repos/a') - const repoB = makeRepo('repo-b', '/repos/b') + const store = storeOf({ 'repo-a': [FIRST], 'repo-b': [SECOND] }) + const repos = [makeRepo('repo-a', '/repos/a'), makeRepo('repo-b', '/repos/b')] - retireGeneratedWorktreeName(store, repoA, settings, FIRST) - - expect(getRetiredWorktreeNamesForRepo(store, repoB, settings)).toEqual([FIRST]) + expect( + getRetiredWorktreeNamesForRepo(store, repos[1], repos, settingsFor(false)).sort() + ).toEqual([FIRST, SECOND].sort()) }) it('keeps independent nested repo paths in separate retirement domains', () => { - const byKey = new Map() - const store = { - getRetiredWorktreeNames: (key: string) => byKey.get(key) ?? [], - addRetiredWorktreeName: (key: string, name: string) => - byKey.set(key, [...(byKey.get(key) ?? []), name]) - } - const settings = settingsFor(true) - const repoA = makeRepo('repo-a', '/repos/a') - const repoB = makeRepo('repo-b', '/repos/b') + const store = storeOf({ 'repo-a': [FIRST], 'repo-b': [SECOND] }) + const repos = [makeRepo('repo-a', '/repos/a'), makeRepo('repo-b', '/repos/b')] - retireGeneratedWorktreeName(store, repoA, settings, FIRST) - - expect(getRetiredWorktreeNamesForRepo(store, repoB, settings)).toEqual([]) - expect(getRetiredWorktreeNamesForRepo(store, repoA, settings)).toEqual([FIRST]) + expect(getRetiredWorktreeNamesForRepo(store, repos[1], repos, settingsFor(true))).toEqual([ + SECOND + ]) }) - it('survives a repo being removed and re-added under a new id', () => { - // Why: repo ids are regenerated on re-add. Keying the registry by repo id silently dropped - // every retirement for a path that had not changed at all. - const byKey = new Map() - const store = { - getRetiredWorktreeNames: (key: string) => byKey.get(key) ?? [], - addRetiredWorktreeName: (key: string, name: string) => - byKey.set(key, [...(byKey.get(key) ?? []), name]) + it('never probes a path for peers that hold no retirements', () => { + // Why: deriving a peer's namespace runs computeWorktreePath, which for a WSL repo is a blocking + // wsl.exe call. Reading `path` is exactly what a probe does, so count that. + const pathReads: string[] = [] + const spyRepo = (id: string, path: string): Repo => { + const repo = makeRepo(id, path) + return Object.defineProperty(repo, 'path', { + get: () => { + pathReads.push(id) + return path + } + }) as Repo } - const settings = settingsFor(true) + const store = storeOf({ 'repo-a': [FIRST] }) + const repos = [ + spyRepo('repo-a', '/repos/a'), + ...Array.from({ length: 20 }, (_unused, index) => spyRepo(`peer-${index}`, `/repos/${index}`)) + ] - retireGeneratedWorktreeName(store, makeRepo('repo-old', '/repos/a'), settings, FIRST) - - expect( - getRetiredWorktreeNamesForRepo(store, makeRepo('repo-new', '/repos/a'), settings) - ).toEqual([FIRST]) + expect(getRetiredWorktreeNamesForRepo(store, repos[0], repos, settingsFor(false))).toEqual([ + FIRST + ]) + expect(pathReads.filter((id) => id.startsWith('peer-'))).toEqual([]) }) it('reports nothing for a folder workspace, which has no generated worktree names', () => { - const store = { getRetiredWorktreeNames: () => [FIRST] } + const store = storeOf({ folder: [FIRST] }) const folderRepo = { ...makeRepo('folder', '/repos/folder'), kind: 'folder' as const } - expect(getRetiredWorktreeNamesForRepo(store, folderRepo, settingsFor(false))).toEqual([]) + + expect( + getRetiredWorktreeNamesForRepo(store, folderRepo, [folderRepo], settingsFor(false)) + ).toEqual([]) }) }) @@ -265,24 +269,26 @@ describe('ensureRetiredWorktreeNamesBackfilled', () => { const root = await mkdtemp(join(tmpdir(), 'orca-retirement-backfill-')) const workspaceRoot = join(root, 'workspaces') await mkdir(join(workspaceRoot, FIRST), { recursive: true }) - const merged: { key: string; names: string[] }[] = [] + const merged: { repoId: string; names: string[] }[] = [] const store = { - mergeRetiredWorktreeNames: (key: string, names: Iterable) => { - merged.push({ key, names: [...names] }) + mergeRetiredWorktreeNames: (repoId: string, names: Iterable) => { + merged.push({ repoId, names: [...names] }) return true } } - const repo = makeRepo('repo-a', join(root, 'repos', 'a')) const settings = { workspaceDir: workspaceRoot, nestWorkspaces: false } try { - await ensureRetiredWorktreeNamesBackfilled(store, repo, settings) - expect(merged).toHaveLength(1) - expect(merged[0].names).toContain(FIRST) + await ensureRetiredWorktreeNamesBackfilled(store, makeRepo('repo-a', '/repos/a'), settings) + expect(merged).toEqual([{ repoId: 'repo-a', names: [FIRST] }]) - // A second repo in the same namespace reuses the one in-flight scan. + // Why: the scan is cached per cwd namespace but the registry is per repo, so a second repo in + // the same namespace must still receive the merge rather than silently inheriting nothing. await ensureRetiredWorktreeNamesBackfilled(store, makeRepo('repo-b', '/repos/b'), settings) - expect(merged).toHaveLength(1) + expect(merged).toEqual([ + { repoId: 'repo-a', names: [FIRST] }, + { repoId: 'repo-b', names: [FIRST] } + ]) } finally { await rm(root, { force: true, recursive: true }) } @@ -291,7 +297,7 @@ describe('ensureRetiredWorktreeNamesBackfilled', () => { it('skips repos whose agent state lives on another host', async () => { const merged: string[] = [] const store = { - mergeRetiredWorktreeNames: (_key: string, names: Iterable) => { + mergeRetiredWorktreeNames: (_repoId: string, names: Iterable) => { merged.push(...names) return true } diff --git a/src/main/worktree-name-retirement.ts b/src/main/worktree-name-retirement.ts index 7698270bc37..a46c4b0b2b2 100644 --- a/src/main/worktree-name-retirement.ts +++ b/src/main/worktree-name-retirement.ts @@ -18,16 +18,13 @@ import { import { worktreePathComparisonKey } from './ipc/worktree-path-comparison' const RETIREMENT_PROBE_NAME = 'orca-retirement-probe' -const backfillsByStore = new WeakMap>>() +const scansByStore = new WeakMap>>>() type RetirementReadStore = { - getRetiredWorktreeNames(collisionKey: string): string[] -} -type RetirementWriteStore = { - addRetiredWorktreeName(collisionKey: string, name: string): void + getRetiredWorktreeNames(repoId: string): string[] } type RetirementBackfillStore = { - mergeRetiredWorktreeNames(collisionKey: string, names: Iterable): boolean + mergeRetiredWorktreeNames(repoId: string, names: Iterable): boolean } type RetirementPathSettings = Pick @@ -121,29 +118,40 @@ export function getRetirementCollisionKey(repo: Repo, settings: RetirementPathSe return key } +/** Stored per repo id, which is stable, but read across every repo that creates into the same cwd + * namespace — that is where a reissued name would actually collide. + * + * The peer scan is deliberately lazy: a repo's own retirements never touch a path, and a peer's + * namespace is derived only once that peer is known to hold retirements. Deriving one costs a + * blocking `wsl.exe` call for a WSL repo, so it must not be paid per repo on every create. */ export function getRetiredWorktreeNamesForRepo( store: RetirementReadStore, repo: Repo, + repos: readonly Repo[], settings: RetirementPathSettings ): string[] { if (isFolderRepo(repo)) { return [] } - return store.getRetiredWorktreeNames(getRetirementCollisionKey(repo, settings)) -} - -/** Records the name main actually used, not the one the client proposed — the create path can - * advance past a taken candidate. Call only for names Orca generated. */ -export function retireGeneratedWorktreeName( - store: RetirementWriteStore, - repo: Repo, - settings: RetirementPathSettings, - name: string -): void { - if (isFolderRepo(repo)) { - return + const names = new Set(store.getRetiredWorktreeNames(repo.id)) + let collisionKey: string | null = null + for (const candidate of repos) { + if (candidate.id === repo.id || isFolderRepo(candidate)) { + continue + } + const candidateNames = store.getRetiredWorktreeNames(candidate.id) + if (candidateNames.length === 0) { + continue + } + collisionKey ??= getRetirementCollisionKey(repo, settings) + if (getRetirementCollisionKey(candidate, settings) !== collisionKey) { + continue + } + for (const name of candidateNames) { + names.add(name) + } } - store.addRetiredWorktreeName(getRetirementCollisionKey(repo, settings), name) + return [...names] } function parentPath(path: string): string { @@ -157,27 +165,29 @@ export function ensureRetiredWorktreeNamesBackfilled( repo: Repo, settings: RetirementPathSettings ): Promise { - // Remote workspaces keep their agent state on the execution host, which this scan cannot see. + // Remote workspaces keep their agent state on the execution host, which this scan cannot see, so + // a re-added SSH repo does not recover its retirements the way a local one does. if (isFolderRepo(repo) || repo.connectionId) { return Promise.resolve() } - let storeBackfills = backfillsByStore.get(store) - if (!storeBackfills) { - storeBackfills = new Map() - backfillsByStore.set(store, storeBackfills) + let storeScans = scansByStore.get(store) + if (!storeScans) { + storeScans = new Map() + scansByStore.set(store, storeScans) } const collisionKey = getRetirementCollisionKey(repo, settings) - const existing = storeBackfills.get(collisionKey) - if (existing) { - return existing + let scan = storeScans.get(collisionKey) + if (!scan) { + scan = discoverRetiredWorktreeNames({ + workspaceRoots: [parentPath(getRetirementProbePath(repo, settings))] + }) + storeScans.set(collisionKey, scan) } - const backfill = discoverRetiredWorktreeNames({ - workspaceRoots: [parentPath(getRetirementProbePath(repo, settings))] - }).then((names) => { - store.mergeRetiredWorktreeNames(collisionKey, names) + // Why the merge sits outside the cached promise: the scan is per cwd namespace but the registry + // is per repo, so every repo that asks must receive it — not only the one that triggered it. + return scan.then((names) => { + store.mergeRetiredWorktreeNames(repo.id, names) }) - storeBackfills.set(collisionKey, backfill) - return backfill } async function readDirectoryNames(path: string): Promise { diff --git a/src/shared/constants.ts b/src/shared/constants.ts index 7a860143d1f..3610ad224f7 100644 --- a/src/shared/constants.ts +++ b/src/shared/constants.ts @@ -431,7 +431,7 @@ export function getDefaultPersistedState(homedir: string): PersistedState { projectGroups: [], folderWorkspaces: [], sparsePresetsByRepo: {}, - retiredWorktreeNamesByWorkspaceKey: {}, + retiredWorktreeNamesByRepo: {}, worktreeMeta: {}, worktreeLineageById: {}, workspaceLineageByChildKey: {}, diff --git a/src/shared/types.ts b/src/shared/types.ts index a8dd14962a8..9be84019b4d 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -3742,12 +3742,13 @@ export type PersistedState = { folderWorkspaceDiffComments?: Record /** Sparse-checkout presets keyed by repoId. */ sparsePresetsByRepo: Record - /** Generated workspace names already issued, keyed by the cwd namespace repos create into - * (execution host + workspace-root path), not by repoId — the collision this prevents is on the - * path, and repo ids change on remove/re-add while the path does not. Suppresses reissuing a - * name so a recreated workspace never lands on a prior occupant's path and inherits agent state - * keyed to that cwd. Grows monotonically; entries are never removed on workspace deletion. */ - retiredWorktreeNamesByWorkspaceKey?: Record + /** Generated workspace names already issued, keyed by repoId. Suppresses reissuing a name so a + * recreated workspace never lands on a prior occupant's path and inherits agent state keyed to + * that cwd. Grows monotonically within a repo — entries are never removed on workspace deletion + * — and is dropped wholesale when the repo is removed. Readers union across every repo that + * creates into the same cwd namespace, so the storage key stays the stable repo id while the + * guarantee follows the path. */ + retiredWorktreeNamesByRepo?: Record /** Per paired device last tab selection by worktree; keeps mobile navigation across host restarts. */ mobileClientTabSelectionsByDeviceId?: PersistedMobileClientTabSelections worktreeMeta: Record