diff --git a/src/main/runtime/construction/runtime-host-services.ts b/src/main/runtime/construction/runtime-host-services.ts index 2c0dc38f650..f060c41eee5 100644 --- a/src/main/runtime/construction/runtime-host-services.ts +++ b/src/main/runtime/construction/runtime-host-services.ts @@ -308,9 +308,13 @@ export abstract class OrcaRuntimeWithPreservedBranchCleanup extends OrcaRuntimeW }, ...target ), - cleanupRemovedFolderWorkspaceState: (worktreeId) => { + cleanupRemovedFolderWorkspaceState: (worktreeId): void | Promise => { if (this.store) { - this.removeWorktreeMetadataAndHistory(this.store, worktreeId) + // Returned, not voided: the controller awaits it at the end of the + // delete, so a forgotten folder workspace cannot report before the + // Codex pretrust deletion inside the purge has landed. Failures + // degrade inside the cleanup. + return this.removeWorktreeMetadataAndHistory(this.store, worktreeId) } // Folder ids are minted by this store and shared with no other host. this.dropAgentStatusForRemovedWorktreeFn?.(worktreeId, ALL_EXECUTION_HOSTS_SCOPE) diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts index 0b160abb231..b9490b5e6ec 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts @@ -163,12 +163,12 @@ class RemovalRuntime extends OrcaRuntimeService { return this.removeManagedWorktreeInFlight } - protected override purgeRemovedWorktree( + protected override async purgeRemovedWorktree( runtimeStore: RuntimeStore, worktreeId: string, repoId: string, hostId?: ExecutionHostId - ): void { + ): Promise { this.purges.push({ store: runtimeStore, worktreeId, repoId, hostId }) } diff --git a/src/main/runtime/repositories/runtime-project-group-controller.ts b/src/main/runtime/repositories/runtime-project-group-controller.ts index 0e0299a99c1..d6cc54ba68f 100644 --- a/src/main/runtime/repositories/runtime-project-group-controller.ts +++ b/src/main/runtime/repositories/runtime-project-group-controller.ts @@ -40,7 +40,7 @@ type RuntimeProjectGroupDependencies = { path: string, connectionId?: string ) => Promise<{ finish(removed: boolean): Promise }> - cleanupRemovedFolderWorkspaceState: (worktreeId: string) => void + cleanupRemovedFolderWorkspaceState: (worktreeId: string) => void | Promise emitWorktreeLifecycle: (event: RuntimeWorktreeLifecycleEvent) => void } @@ -274,6 +274,7 @@ export class RuntimeProjectGroupController { let gate: | Awaited> | undefined + let cleanupState: void | Promise = undefined try { if (workspace) { const worktreeId = folderWorkspaceKey(folderWorkspaceId) @@ -291,7 +292,11 @@ export class RuntimeProjectGroupController { ) await this.deps.teardownFolderWorkspacePtys(worktreeId, connectionId) } - this.deps.cleanupRemovedFolderWorkspaceState(worktreeId) + // Fired here (timing unchanged) but awaited at the end: the delete may + // only resolve after the purge has landed. `folder:` ids parse to + // no filesystem path, so the pretrust drop inside the purge skips here — + // safe: the folder itself stays and sibling workspaces may share its path. + cleanupState = this.deps.cleanupRemovedFolderWorkspaceState(worktreeId) } const deleted = store.removeFolderWorkspace(folderWorkspaceId) if (deleted) { @@ -305,6 +310,7 @@ export class RuntimeProjectGroupController { }) } } + await cleanupState return { deleted } } finally { await gate?.finish(false) diff --git a/src/main/runtime/runtime-project-group-controller-folder-delete.test.ts b/src/main/runtime/runtime-project-group-controller-folder-delete.test.ts new file mode 100644 index 00000000000..bae40510dff --- /dev/null +++ b/src/main/runtime/runtime-project-group-controller-folder-delete.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it, vi } from 'vitest' +import { RuntimeProjectGroupController } from './runtime-project-group-controller' +import type { FolderWorkspace } from '../../shared/folder-workspace-types' + +const workspace = { + id: 'ws-1', + projectGroupId: 'group-1', + folderPath: '/tmp/ws' +} as FolderWorkspace + +function createController( + resolveFolderConnectionId: (workspace: FolderWorkspace) => string | null +) { + const removeFolderWorkspace = vi.fn(() => true) + const teardownFolderWorkspacePtys = vi.fn(async () => undefined) + const cleanupRemovedFolderWorkspaceState = vi.fn() + const finishFileWatcherRemoval = vi.fn(async () => undefined) + const notifyReposChanged = vi.fn() + const emitWorktreeLifecycle = vi.fn() + const controller = new RuntimeProjectGroupController({ + getStore: () => ({ getFolderWorkspaces: () => [workspace], removeFolderWorkspace }) as never, + resolveRepo: async () => { + throw new Error('unused') + }, + notifyReposChanged, + resolveFolderConnectionId, + // Rebase adaptation: the delete now runs inside the file-watcher removal + // gate; the fake hands back a gate that resolves immediately. + acquireFileWatcherRemoval: async () => ({ finish: finishFileWatcherRemoval }), + teardownFolderWorkspacePtys, + cleanupRemovedFolderWorkspaceState, + emitWorktreeLifecycle + }) + return { + emitWorktreeLifecycle, + controller, + removeFolderWorkspace, + teardownFolderWorkspacePtys, + cleanupRemovedFolderWorkspaceState, + notifyReposChanged, + finishFileWatcherRemoval + } +} + +describe('RuntimeProjectGroupController.deleteFolderWorkspace', () => { + it('does not resolve the delete before the removed state cleanup lands', async () => { + let releaseCleanup!: () => void + const cleanupParked = new Promise((resolve) => { + releaseCleanup = resolve + }) + const deps = createController(() => 'ssh-1') + deps.cleanupRemovedFolderWorkspaceState.mockReturnValue(cleanupParked) + + const deletion = deps.controller.deleteFolderWorkspace('ws-1') + let settledEarly = false + void deletion.then( + () => { + settledEarly = true + }, + () => {} + ) + // One macrotask is past the PTY teardown await: the cleanup has fired and + // the catalog row is removed, while the cleanup itself is still parked. A + // delete that resolves here is the bug — the ack must wait for the purge + // (and the Codex pretrust deletion inside it). + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(deps.removeFolderWorkspace).toHaveBeenCalledWith('ws-1') + expect(settledEarly).toBe(false) + releaseCleanup() + await expect(deletion).resolves.toEqual({ deleted: true }) + }) +}) diff --git a/src/main/runtime/workspaces/removal/runtime-failed-local-worktree-removal.test.ts b/src/main/runtime/workspaces/removal/runtime-failed-local-worktree-removal.test.ts index abaf61605db..9f159416039 100644 --- a/src/main/runtime/workspaces/removal/runtime-failed-local-worktree-removal.test.ts +++ b/src/main/runtime/workspaces/removal/runtime-failed-local-worktree-removal.test.ts @@ -117,7 +117,9 @@ function jobHost(purged: string[], stopPtys = vi.fn(async () => {}), pushTarget? closeWatchers: async () => {}, stopPtys, preservedBranchCleanup: { preserveHead: (result) => result ?? {}, remember: vi.fn() }, - purge: ({ worktreeId: id }: WorktreeRemovalRecord) => purged.push(id), + purge: ({ worktreeId: id }: WorktreeRemovalRecord) => { + purged.push(id) + }, onRemoved: () => {}, publish: () => {} } satisfies Parameters[1] diff --git a/src/main/runtime/workspaces/removal/runtime-interrupted-local-worktree-removal.test.ts b/src/main/runtime/workspaces/removal/runtime-interrupted-local-worktree-removal.test.ts index 3a377917103..80183c08690 100644 --- a/src/main/runtime/workspaces/removal/runtime-interrupted-local-worktree-removal.test.ts +++ b/src/main/runtime/workspaces/removal/runtime-interrupted-local-worktree-removal.test.ts @@ -718,6 +718,85 @@ describe('finishing an interrupted worktree removal after a restart', () => { expect(await listedRows()).toEqual([{ path: worktreePath, removalError: refusal }]) }) + it('does not report an unregistered leftover removed while its purge is still in flight', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + // The folder vanished outside Orca and the stale registration was pruned, + // so the resume finishes through the unregistered branch with nothing left + // to delete; the branch is kept by the record. + await removeTree(worktreePath) + await git(['worktree', 'prune']) + + const record: WorktreeRemovalRecord = { + worktreeId: `repo-1::${worktreePath}`, + repoId: 'repo-1', + repoPath, + worktreePath, + branch: 'feature', + head: (await git(['rev-parse', 'feature'])).trim(), + deleteBranch: false, + force: false, + requestedAt: 1 + } + await writeWorktreeRemovalRecords(recordsDir, () => [record]) + await loadWorktreeRemovalRecords(recordsDir) + const joined = waitForPendingWorktreeRemoval(record.worktreeId) + expect(joined).toBeDefined() + + // Why parked: the unregistered finish used to skip its await, reporting + // the removal while the purge — and the Codex pretrust deletion inside + // it — was still in flight. + let releasePurge!: () => void + const purgeParked = new Promise((resolve) => { + releasePurge = resolve + }) + const purged: string[] = [] + const storeStub = { + getRepo: () => repo, + getRepos: () => [repo], + getWorktreeMeta: () => undefined + } + resumeInterruptedWorktreeRemovals((interrupted) => + interruptedLocalWorktreeRemovalJob(interrupted, { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the finish reads only repos and worktree metadata from the store here; git options and push-target cleanup are stubbed or short-circuit without a push target. + store: storeStub as unknown as Store, + acquireWatcherRemoval: async (path) => { + const gate = acquireWatcherRemovalGate(path) + return { finish: async () => gate.release() } + }, + closeWatchers: async () => {}, + preservedBranchCleanup: { + preserveHead: (result) => result ?? {}, + remember: () => {} + }, + purge: ({ worktreeId }) => { + purged.push(worktreeId) + return purgeParked + }, + onRemoved: () => {}, + publish: () => {} + }) + ) + + // The leftover finish has reached its purge; the removal must not be + // reported until that purge lands. + await vi.waitFor(() => expect(purged).toEqual([record.worktreeId])) + let settledEarly = false + void joined!.then( + () => { + settledEarly = true + }, + () => {} + ) + await new Promise((resolve) => setTimeout(resolve, 50)) + expect(settledEarly).toBe(false) + releasePurge() + const outcome = await joined!.then( + (result) => ({ status: 'removed' as const, ...result }), + (error: unknown) => ({ status: 'failed' as const, error: String(error) }) + ) + expect(outcome).toMatchObject({ status: 'removed' }) + }) + it('leaves a different checkout created at the same path since the quit', async () => { vi.spyOn(console, 'warn').mockImplementation(() => {}) const head = (await git(['rev-parse', 'feature'])).trim() diff --git a/src/main/runtime/workspaces/removal/runtime-interrupted-local-worktree-removal.ts b/src/main/runtime/workspaces/removal/runtime-interrupted-local-worktree-removal.ts index d67fe79713a..3c81aec3391 100644 --- a/src/main/runtime/workspaces/removal/runtime-interrupted-local-worktree-removal.ts +++ b/src/main/runtime/workspaces/removal/runtime-interrupted-local-worktree-removal.ts @@ -52,7 +52,7 @@ type InterruptedWorktreeRemovalHost = { /** A retry's teardown: terminals opened in the folder before the user removed it. */ stopPtys?: () => Promise /** Drops the worktree's host state (metadata, history, caches), as every removal path does. */ - purge: (record: WorktreeRemovalRecord) => void + purge: (record: WorktreeRemovalRecord) => void | Promise onRemoved: (record: WorktreeRemovalRecord) => void publish: (repoId: string) => void } @@ -87,7 +87,7 @@ export function interruptedLocalWorktreeRemovalJob( preserveBranchHead: (result, fallbackHead) => host.preservedBranchCleanup.preserveHead(result, fallbackHead), // remember() clears the cleanup target when no branch was preserved. - finishRemoval: (result, _rememberBranch, fallbackHead) => { + finishRemoval: async (result, _rememberBranch, fallbackHead) => { host.preservedBranchCleanup.remember( record.worktreeId, undefined, @@ -95,7 +95,7 @@ export function interruptedLocalWorktreeRemovalJob( fallbackHead, removedPushTarget ) - host.purge(record) + await host.purge(record) } }) host.onRemoved(record) @@ -248,7 +248,9 @@ async function finishInterruptedLocalWorktreeRemoval( await gate.finish(removed) } await cleanupRemovedWorktreePushTarget(finishArgs) - args.finishRemoval(result, true, record.head) + // Awaited like the registered branch: the removal may only be reported + // after the purge — and the Codex pretrust deletion inside it — landed. + await args.finishRemoval(result, true, record.head) return result } diff --git a/src/main/runtime/workspaces/removal/runtime-registered-local-worktree-removal.ts b/src/main/runtime/workspaces/removal/runtime-registered-local-worktree-removal.ts index 4abbee83dba..3b06f4c2fa9 100644 --- a/src/main/runtime/workspaces/removal/runtime-registered-local-worktree-removal.ts +++ b/src/main/runtime/workspaces/removal/runtime-registered-local-worktree-removal.ts @@ -80,7 +80,7 @@ async function acceptRuntimeRegisteredLocalWorktreeRemoval(args: { rememberBranch: boolean, // Archive hooks may move the branch; re-read the fallback head afterward. fallbackHead: string | undefined - ) => void + ) => void | Promise /** Fired when Git finished, not on acceptance. */ onRemoved: () => void publish: () => void @@ -274,7 +274,7 @@ export async function finishRuntimeLocalWorktreeRemoval( ) await pruneGitWorktrees({ cwd: repo.path, ...localOptions }).catch(() => {}) await cleanupRemovedWorktreePushTarget(args) - args.finishRemoval(undefined, false, refreshed.head) + await args.finishRemoval(undefined, false, refreshed.head) completed = true return {} } else { @@ -287,7 +287,9 @@ export async function finishRuntimeLocalWorktreeRemoval( try { await cleanupRemovedWorktreePushTarget(args) } finally { - args.finishRemoval(removalResult, true, refreshed.head) + // Awaited so the removal result lands only after the host-state purge (and + // the Codex pretrust deletion inside it) has finished. + await args.finishRemoval(removalResult, true, refreshed.head) } return removalResult ?? {} } finally { diff --git a/src/main/runtime/workspaces/removal/runtime-unregistered-worktree-removal.ts b/src/main/runtime/workspaces/removal/runtime-unregistered-worktree-removal.ts index cd334d059a6..dfef2417514 100644 --- a/src/main/runtime/workspaces/removal/runtime-unregistered-worktree-removal.ts +++ b/src/main/runtime/workspaces/removal/runtime-unregistered-worktree-removal.ts @@ -42,7 +42,7 @@ export async function removeRuntimeUnregisteredWorktree(args: { store: RuntimeStore acquireWatcherRemoval: (path: string) => Promise stopPtys: (worktreeId: string, allow: boolean) => Promise - finishRemoval: () => void + finishRemoval: () => void | Promise }): Promise<{}> { const { repo, target, registeredWorktrees, removedMeta, route } = args const removalHome = resolveWorktreeRemovalHome(route) @@ -77,7 +77,7 @@ export async function removeRuntimeUnregisteredWorktree(args: { try { await cleanupPushTarget(args) } finally { - args.finishRemoval() + await args.finishRemoval() } } finally { await gate.finish(true) @@ -105,7 +105,7 @@ async function deleteUnregisteredDirectory( try { await cleanupPushTarget(args) } finally { - args.finishRemoval() + await args.finishRemoval() } } finally { await gate.finish(completed) diff --git a/src/main/runtime/workspaces/removal/workspace-registration-removal.ts b/src/main/runtime/workspaces/removal/workspace-registration-removal.ts index 72759ae42e5..5ae168ef807 100644 --- a/src/main/runtime/workspaces/removal/workspace-registration-removal.ts +++ b/src/main/runtime/workspaces/removal/workspace-registration-removal.ts @@ -85,7 +85,7 @@ export async function removeOrphanOrFolderWorktree({ .catch(() => {}) } runtime.clearOptimisticReconcileToken(removalTarget.id) - runtime.removeWorktreeMetadataAndHistory( + await runtime.removeWorktreeMetadataAndHistory( store, removalTarget.id, cleanupHostId ?? orphanHost?.id @@ -120,7 +120,7 @@ export async function removeOrphanOrFolderWorktree({ removalTarget.id, folderConnectionId ) - runtime.removeWorktreeMetadataAndHistory(store, removalTarget.id, removalHostId) + await runtime.removeWorktreeMetadataAndHistory(store, removalTarget.id, removalHostId) runtime.preservedBranchCleanup.delete(removalTarget.id, cleanupHostId) runtime.invalidateResolvedWorktreeCache() runtime.notifyWorktreesChanged(repo.id) diff --git a/src/main/runtime/workspaces/removal/workspace-removal.ts b/src/main/runtime/workspaces/removal/workspace-removal.ts index d9986a28cc6..ca0405185f8 100644 --- a/src/main/runtime/workspaces/removal/workspace-removal.ts +++ b/src/main/runtime/workspaces/removal/workspace-removal.ts @@ -43,7 +43,7 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi store: RuntimeStore, worktreeId: string, hostId?: ExecutionHostId - ): void + ): Promise protected abstract resolveWorktreeRemovalTarget( worktreeSelector: string, requiredHostId?: ExecutionHostId @@ -57,7 +57,7 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi worktreeId: string, repoId: string, removalHostId?: ExecutionHostId - ): void + ): Promise protected abstract retryFailedRemoval( route: { kind: string }, target: { id: string; path: string }, @@ -136,7 +136,7 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi } const repo = repoOwner.kind === 'resolved' ? repoOwner.repo : undefined const removalHostId = repo ? (cleanupHostId ?? getRepoExecutionHostId(repo)) : cleanupHostId - const purgeRemovedWorktree = (): void => + const purgeRemovedWorktree = (): Promise => this.purgeRemovedWorktree(store, removalTarget.id, removalTarget.repoId, removalHostId) const orphanOrFolderResult = await removeOrphanOrFolderWorktree({ runtime: this, @@ -205,9 +205,9 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi this.stopPtysForDestructiveWorktreeRemoval(worktreeId, { allowUnverifiedStop: allowUnverifiedPtyStop }), - finishRemoval: () => { + finishRemoval: async () => { this.preservedBranchCleanup.delete(removalTarget.id, cleanupHostId) - purgeRemovedWorktree() + await purgeRemovedWorktree() this.notifyWorktreesChanged(repo!.id) } }) @@ -243,7 +243,7 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi registeredWorktree.head, removedPushTarget ) - purgeRemovedWorktree() + await purgeRemovedWorktree() this.notifyWorktreesChanged(repo!.id) return removalResult ?? {} } @@ -269,7 +269,7 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi closeWatchers: (path) => this.closeFileWatchersForRemoval(path), preserveBranchHead: (result, fallbackHead) => this.preservedBranchCleanup.preserveHead(result, fallbackHead), - finishRemoval: (result, rememberBranch, fallbackHead) => { + finishRemoval: async (result, rememberBranch, fallbackHead) => { if (rememberBranch) { this.preservedBranchCleanup.remember( removalTarget.id, @@ -281,7 +281,7 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi } else { this.preservedBranchCleanup.delete(removalTarget.id, cleanupHostId) } - purgeRemovedWorktree() + await purgeRemovedWorktree() }, onRemoved: emitRemoved, publish: () => this.publishWorktreeRemovalChange(repo!.id) diff --git a/src/main/runtime/workspaces/workspace-removal-target.ts b/src/main/runtime/workspaces/workspace-removal-target.ts index 51ea3e41849..8160460a7b5 100644 --- a/src/main/runtime/workspaces/workspace-removal-target.ts +++ b/src/main/runtime/workspaces/workspace-removal-target.ts @@ -40,16 +40,35 @@ import { resumeInterruptedWorktreeRemovals, retryFailedWorktreeRemoval, waitForPendingWorktreeRemoval +<<<<<<< HEAD } from '../../workspaces/removal/worktree-background-removal' import { interruptedLocalWorktreeRemovalJob } from './removal/runtime-interrupted-local-worktree-removal' +======= +} from '../../worktree-background-removal' +import { interruptedLocalWorktreeRemovalJob } from '../runtime-interrupted-local-worktree-removal' +import { invalidateAuthorizedRootsCacheForRepo } from '../../ipc/registered-worktree-roots-scoped-invalidation' +import { dropOrcaCreatedCodexPretrustForRemovedWorktree } from '../worktree-removal-codex-pretrust-cleanup' +import { splitWorktreeId, worktreeIdsEqual } from '../../../shared/worktree/id' +import { hasWorktreeRemovalRepoOwnerOnOtherHost } from '../../worktree-removal-repo-owner' +import { + dropRemovedWorktreeProcessState, + pruneRemovedWorktreeSnapshots +} from '../../worktree-removal-process-cleanup' +import { removeWorktreeMetaRow } from '../../persistence/host-qualified-worktree-meta' +>>>>>>> 738ffaefdd3 (fix(codex): await the pretrust cleanup before reporting a worktree removed) import { ORCA_LAYOUT_OWNER_IS_MAIN } from '../../../shared/layout-owner-switch' import { retryFailedRemovalUnlessRegistered } from '../../workspaces/removal/worktree-removal-leftover' import type { GitWorktreeInfo } from '../../../shared/worktree/types' import { resolveAgentTerminalCreateOptions, +<<<<<<< HEAD prepareHostAgentTerminalLaunch } from '../terminal/launch/runtime-agent-terminal-startup' import { removeWorktreeMetaRow } from '../../persistence/host-qualified-worktree-meta' +======= + withHostAccountLaunchEnv +} from '../runtime-agent-terminal-startup' +>>>>>>> 738ffaefdd3 (fix(codex): await the pretrust cleanup before reporting a worktree removed) export abstract class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRuntimeWithRemoveManagedWorktree { protected abstract resolveExplicitWorktreeIdScoped( @@ -142,24 +161,31 @@ export abstract class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRu } // Host state every removal path drops once Git has let go of the checkout. - protected purgeRemovedWorktree( + // Resolves only after the Codex pretrust deletion landed, so a removal ack + // cannot precede the trust table purge. + protected async purgeRemovedWorktree( store: RuntimeStore, worktreeId: string, repoId: string, removalHostId?: ExecutionHostId - ): void { + ): Promise { this.clearOptimisticReconcileToken(worktreeId) - this.removeWorktreeMetadataAndHistory(store, worktreeId, removalHostId) + const metadataAndHistory = this.removeWorktreeMetadataAndHistory( + store, + worktreeId, + removalHostId + ) this.invalidateResolvedWorktreeCache() this.invalidateWorktreeScanCacheForRepo(repoId) invalidateAuthorizedRootsCacheForRepo(store, repoId) + await metadataAndHistory } - removeWorktreeMetadataAndHistory( + async removeWorktreeMetadataAndHistory( store: RuntimeStore, worktreeId: string, hostId?: ExecutionHostId - ): void { + ): Promise { // Purge history/caches before path-derived ids can name a recreated workspace. const persistedHostId = store.getWorktreeMeta(worktreeId)?.hostId const repoId = splitWorktreeId(worktreeId)?.repoId @@ -176,7 +202,7 @@ export abstract class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRu if (!preservesSameIdOwner) { // Why: worktree IDs are path-derived and can be recreated; a stale Codex // pretrust entry must not pre-trust the next occupant of the path. - dropOrcaCreatedCodexPretrustForRemovedWorktree(worktreeId) + const pretrustDrop = dropOrcaCreatedCodexPretrustForRemovedWorktree(worktreeId) // Do not rescue surviving paired PTYs into a recreated occupant of the same path-derived id. for (const ptyId of this.pairedRendererSessionOwnedPtyIds) { const ptyWorktreeId = this.ptysById.get(ptyId)?.worktreeId @@ -200,6 +226,10 @@ export abstract class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRu this.notifyMobileSessionTabsRemoved(worktreeId) dropRemovedWorktreeProcessState(worktreeId) this.releaseRemovedWorktreeHostSessions(worktreeId, 'worktree-removal') + // Awaited last so every in-memory invalidation above stays synchronous, + // while callers holding the returned promise still wait for the entry + // deletion (failures degrade inside the helper, never fail the removal). + await pretrustDrop } const snapshotDirectory = store.getProfileStorageDirectory?.() if (snapshotDirectory) { diff --git a/src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts b/src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts index 9576805ec88..b54b78daa3c 100644 --- a/src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts +++ b/src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts @@ -9,6 +9,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { OrcaRuntimeService } from './orca-runtime' import { upsertOrcaCreatedProjectTrustLevel } from '../codex/config-toml-trust' +import { runExclusivelyForCodexTrustConfig } from '../codex/codex-trust-config-mutation-queue' const WORKTREE_ID = 'repo-1::/tmp/worktrees/feature-r1' const PROJECT = '/tmp/worktrees/feature-r1' @@ -18,20 +19,23 @@ const FOLDER_SUFFIX = '::workspace:5e8cc6b2-1d3f-4a5b-9c2d-7f6e5a4b3c2d' function makeRuntimePurge(meta: Record = {}) { const store = { getWorktreeMeta: () => meta, + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the fake only supplies the meta/query members these cases drive. getRepos: () => [] as never[], removeWorktreeMeta: () => {}, getRepo: () => undefined, getSettings: () => ({}) } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the service constructor only reads the store members the fake supplies. const runtime = new OrcaRuntimeService(store as never) return (worktreeId: string, hostId?: string) => + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the suite spies one private member by its exact declared shape. ( runtime as unknown as { removeWorktreeMetadataAndHistory: ( store: unknown, worktreeId: string, hostId?: string - ) => void + ) => void | Promise } ).removeWorktreeMetadataAndHistory(store, worktreeId, hostId) } @@ -103,4 +107,90 @@ describe('worktree removal cleans up the Codex pretrust Orca wrote', () => { expect(readFileSync(configPath, 'utf-8')).not.toContain(`[projects."${PROJECT}"]`) }) }) + + it('does not complete the purge before the recorded entry is deleted', async () => { + upsertOrcaCreatedProjectTrustLevel(configPath, PROJECT, 'trusted') + // Why: holding the config's mutation lane parks the cleanup, so "the purge + // finished" and "the entry is gone" can be told apart. A recreated path + // inherits trust_level = "trusted" if the purge reports completion first. + let releaseLane!: () => void + const laneHeld = new Promise((resolve) => { + releaseLane = resolve + }) + const hold = runExclusivelyForCodexTrustConfig(configPath, () => laneHeld) + const purge = makeRuntimePurge() + + const removalAck = purge(WORKTREE_ID) + + // The cleanup is queued behind the held lane: nothing landed yet. + expect(readFileSync(configPath, 'utf-8')).toContain(`[projects."${PROJECT}"]`) + // A settled purge here is the bug: the removal ack must wait for the + // deletion that is still parked on the config lane. A 0ms macrotask loses + // to any settled ack (microtasks first) and wins only against a pending one. + const settledBeforeCleanup = await Promise.race([ + Promise.resolve(removalAck).then(() => true), + new Promise((resolve) => { + setTimeout(() => resolve(false), 0) + }) + ]) + expect(settledBeforeCleanup).toBe(false) + releaseLane() + await hold + await removalAck + expect(readFileSync(configPath, 'utf-8')).not.toContain(`[projects."${PROJECT}"]`) + }) + + it('hands the folder-workspace forgetting dep the purge promise, not void', async () => { + // Why parked: the wired dep used to void the purge promise, so the folder + // delete had nothing to await and a forgotten workspace could report + // before the recorded entry was deleted. + let releasePurge!: () => void + const purgeParked = new Promise((resolve) => { + releasePurge = resolve + }) + const store = { + getWorktreeMeta: () => ({}), + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the fake only supplies the meta/query members these cases drive. + getRepos: () => [] as never[], + removeWorktreeMeta: () => {}, + getRepo: () => undefined, + getSettings: () => ({}) + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the service constructor only reads the store members the fake supplies. + const runtime = new OrcaRuntimeService(store as never) + const purgeSpy = vi + .spyOn( + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the suite spies one private member by its exact declared shape. + runtime as unknown as { + removeWorktreeMetadataAndHistory: (store: unknown, worktreeId: string) => Promise + }, + 'removeWorktreeMetadataAndHistory' + ) + .mockReturnValue(purgeParked) + const cleanupRemovedFolderWorkspaceState = + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the suite reads one private member by its exact declared shape. + ( + runtime as unknown as { + projectGroups: { + deps: { + cleanupRemovedFolderWorkspaceState: (worktreeId: string) => void | Promise + } + } + } + ).projectGroups.deps.cleanupRemovedFolderWorkspaceState + + const cleanup = cleanupRemovedFolderWorkspaceState('folder:ws-1') + expect(purgeSpy).toHaveBeenCalledWith(expect.anything(), 'folder:ws-1') + + // Voiding the promise made this resolve instantly; the dep must hand the + // caller the still-pending purge to await. + let settledEarly = false + void Promise.resolve(cleanup).then(() => { + settledEarly = true + }) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(settledEarly).toBe(false) + releasePurge() + await cleanup + }) }) diff --git a/src/main/runtime/worktree-removal-codex-pretrust-cleanup.ts b/src/main/runtime/worktree-removal-codex-pretrust-cleanup.ts index b405275c6de..d0275df3c21 100644 --- a/src/main/runtime/worktree-removal-codex-pretrust-cleanup.ts +++ b/src/main/runtime/worktree-removal-codex-pretrust-cleanup.ts @@ -9,10 +9,20 @@ import { removeOrcaCreatedProjectTrustEntries } from '../codex/config-toml-trust * pretrust entry Orca wrote for the path is the same hazard: drop it so a * recreated worktree is not born pre-trusted (#24697). The grant keyed the bare * path, so a folder session's ::workspace: suffix must not hide it. + * + * Resolves only once every recorded entry has been written out, so a removal + * can await it and never report the worktree gone while the path would still + * inherit `trust_level = "trusted"` on rebuild. Await is for ordering, not + * failure propagation: a write that cannot land (read-only config.toml and + * alike) must not turn an accepted removal into a failed one, so errors + * degrade to this warn — the ownership record stays for a later pass. */ -export function dropOrcaCreatedCodexPretrustForRemovedWorktree(worktreeId: string): void { +export function dropOrcaCreatedCodexPretrustForRemovedWorktree(worktreeId: string): Promise { const worktreePath = splitWorktreeIdForFilesystem(worktreeId)?.worktreePath - if (worktreePath) { - void removeOrcaCreatedProjectTrustEntries(worktreePath) + if (!worktreePath) { + return Promise.resolve() } + return removeOrcaCreatedProjectTrustEntries(worktreePath).catch((error) => { + console.warn('[codex-config] Failed to drop an Orca-created project trust entry:', error) + }) } diff --git a/src/main/workspaces/removal/worktree-removal-process-cleanup.ts b/src/main/workspaces/removal/worktree-removal-process-cleanup.ts index a28fb17ee01..4abc682b8d1 100644 --- a/src/main/workspaces/removal/worktree-removal-process-cleanup.ts +++ b/src/main/workspaces/removal/worktree-removal-process-cleanup.ts @@ -10,7 +10,15 @@ import { pruneWorkspaceSpaceAnalysisSnapshot } from '../space-analysis/workspace export function dropRemovedWorktreeProcessState(worktreeId: string): void { advertisedUrlWatcher.forgetWorktree(worktreeId) localhostWorktreeLabelProxy.unregisterWorktree(worktreeId) - deleteWorktreeHistoryDir(worktreeId) + // Guarded here: removal purges must degrade, not reject, when AppEnvironment + // is uninitialized and the history root cannot be resolved. + try { + deleteWorktreeHistoryDir(worktreeId) + } catch (err) { + console.warn( + `[pty:history] Failed to delete history dir: ${err instanceof Error ? err.message : String(err)}` + ) + } } /** Removed workspaces must never resurrect from the persisted cleanup/space scan snapshots. */