fix: close terminal removal races

This commit is contained in:
Brennan Benson
2026-07-13 03:36:31 -07:00
parent 9f38d7da18
commit a18b7fda26
5 changed files with 78 additions and 7 deletions
+3 -1
View File
@@ -3606,6 +3606,7 @@
"duplicate repo and worktree ids isolate admissions and PTY liveness by execution host while whole-project removal still drains every host scope",
"host-scoped terminal stop succeeds when an unrelated provider is unavailable without disconnecting that host's cached PTYs",
"workspace resolution on an unrelated host proceeds while the removed host remains admission-fenced",
"a matching host removal that starts and finishes during unresolved workspace selection still rejects the stale create",
"worktree removal rejects a late renderer terminal admission while destructive preflight is still pending",
"already-missing exceptional removal verifies terminal stop before metadata purge",
"failed and concurrent admissions release without deadlocking project removal",
@@ -3629,7 +3630,8 @@
"file": "src/main/providers/local-pty-provider.test.ts",
"assertions": [
"a synchronous Windows native kill throw retains PTY ownership and listeners so retry can succeed",
"a stale pane identity is rejected before the local PTY can be killed"
"a stale pane identity is rejected before the local PTY can be killed",
"immediate POSIX local shutdown uses SIGKILL while Windows remains signal-free"
]
},
{
@@ -967,7 +967,7 @@ describe('LocalPtyProvider', () => {
})
})
it('kills the PTY process', async () => {
it('uses SIGKILL for immediate POSIX shutdown', async () => {
// Why: capture the spy reference before shutdown triggers onExit →
// POSIX kill neutralization. After neutralization, mockProc.kill is
// replaced with a non-spy no-op to close the UnixTerminal.destroy() →
@@ -975,7 +975,7 @@ describe('LocalPtyProvider', () => {
const killSpy = mockProc.kill
const { id } = await provider.spawn({ cols: 80, rows: 24 })
await provider.shutdown(id, { immediate: true })
expect(killSpy).toHaveBeenCalled()
expect(killSpy).toHaveBeenCalledWith('SIGKILL')
})
it('invokes onExit callback via the node-pty exit handler', async () => {
+7 -1
View File
@@ -917,7 +917,13 @@ export class LocalPtyProvider implements IPtyProvider {
ptyShutdownInProgress.add(id)
let exitCode = -1
try {
proc.kill()
// Why: a renderer or paired/mobile close is destructive. POSIX's
// default SIGHUP can be ignored; Windows node-pty requires no argument.
if (opts.immediate && process.platform !== 'win32') {
proc.kill('SIGKILL')
} else {
proc.kill()
}
} catch (error) {
if (!ptyExitDuringShutdown.has(id)) {
throw error
+54
View File
@@ -23352,6 +23352,60 @@ describe('OrcaRuntimeService', () => {
await removal
})
it('rejects a resolved create when its host removal starts and finishes during resolution', async () => {
const runtime = new OrcaRuntimeService(store)
const resolution = deferred<{
id: string
path: string
hostId: 'ssh:target-1'
connectionId: 'target-1'
repo: ReturnType<typeof store.getRepo>
folderWorkspace: null
}>()
const runtimeWithResolver = runtime as unknown as {
resolveTerminalWorkspaceLaunchScope: () => typeof resolution.promise
}
const resolveSpy = vi
.spyOn(runtimeWithResolver, 'resolveTerminalWorkspaceLaunchScope')
.mockReturnValue(resolution.promise)
const spawn = vi.fn().mockResolvedValue({ id: 'ssh-pty-too-late' })
runtime.setPtyController({
spawn,
write: () => true,
kill: () => false,
getForegroundProcess: async () => null,
listProcesses: async () => []
})
const holdLocalRemoval = deferred<void>()
const localRemoval = runtime.runWithWorktreeRemovalAdmission(
TEST_WORKTREE_ID,
() => holdLocalRemoval.promise,
'local'
)
await Promise.resolve()
const create = runtime.createTerminal(`id:${TEST_WORKTREE_ID}`)
await vi.waitFor(() => expect(resolveSpy).toHaveBeenCalledOnce())
await runtime.runWithWorktreeRemovalAdmission(
TEST_WORKTREE_ID,
async () => undefined,
'ssh:target-1'
)
resolution.resolve({
id: TEST_WORKTREE_ID,
path: '/home/dev/orca',
hostId: 'ssh:target-1',
connectionId: 'target-1',
repo: store.getRepo(TEST_REPO_ID),
folderWorkspace: null
})
await expect(create).rejects.toThrow('terminal_admission_blocked_for_project_removal')
expect(spawn).not.toHaveBeenCalled()
holdLocalRemoval.resolve()
await localRemoval
})
it('drains a create-first managed worktree before project removal snapshots ownership', async () => {
const removeProject = vi.fn()
const runtime = new OrcaRuntimeService({ ...store, removeProject } as never)
+12 -3
View File
@@ -2262,6 +2262,7 @@ export class OrcaRuntimeService {
// Why: repo removal must make verified stop + metadata removal atomic with
// respect to paired/mobile terminal creation, or a late spawn is orphaned.
private terminalAdmissionBlockedWorktreeIds = new Set<string>()
private terminalRemovalGeneration = 0
private terminalAdmissionsByWorktreeId = new Map<string, Set<Promise<void>>>()
private projectRemovalBlockedRepoIds = new Set<string>()
private worktreeCreateAdmissionsByRepoId = new Map<string, Set<Promise<void>>>()
@@ -12418,6 +12419,11 @@ export class OrcaRuntimeService {
}
}
private blockTerminalAdmissionScope(scopeKey: string): void {
this.terminalRemovalGeneration += 1
this.terminalAdmissionBlockedWorktreeIds.add(scopeKey)
}
private async drainTerminalAdmissions(
worktreeIds: readonly string[],
hostId?: ExecutionHostId | null,
@@ -12438,6 +12444,7 @@ export class OrcaRuntimeService {
// aware of the unresolved create, then transfer ownership without a gap.
let releaseResolutionAdmission: (() => void) | null = null
let removalsActiveAtResolutionStart: ReadonlySet<string> | null = null
let removalGenerationAtResolutionStart: number | null = null
try {
releaseResolutionAdmission = this.beginTerminalAdmission(GLOBAL_TERMINAL_ADMISSION_KEY)
} catch (error) {
@@ -12451,12 +12458,14 @@ export class OrcaRuntimeService {
// Resolve it, but remember the active removals so a fast completion
// cannot reopen the exact stale scope before the atomic transfer.
removalsActiveAtResolutionStart = new Set(this.terminalAdmissionBlockedWorktreeIds)
removalGenerationAtResolutionStart = this.terminalRemovalGeneration
}
try {
const workspace = await this.resolveTerminalWorkspaceLaunchScope(selector, hostId)
if (
removalsActiveAtResolutionStart &&
this.isAdmissionBlocked(removalsActiveAtResolutionStart, workspace.id, workspace.hostId)
(removalGenerationAtResolutionStart !== this.terminalRemovalGeneration ||
this.isAdmissionBlocked(removalsActiveAtResolutionStart, workspace.id, workspace.hostId))
) {
throw new Error('terminal_admission_blocked_for_project_removal')
}
@@ -12556,7 +12565,7 @@ export class OrcaRuntimeService {
throw new Error('worktree_removal_in_progress')
}
const scopeKey = this.admissionScopeKey(worktreeId, hostId)
this.terminalAdmissionBlockedWorktreeIds.add(scopeKey)
this.blockTerminalAdmissionScope(scopeKey)
try {
await Promise.all([
this.drainTerminalAdmissions([GLOBAL_TERMINAL_ADMISSION_KEY]),
@@ -12626,7 +12635,7 @@ export class OrcaRuntimeService {
throw new Error('project_removal_in_progress')
}
for (const scope of worktreeScopes) {
this.terminalAdmissionBlockedWorktreeIds.add(
this.blockTerminalAdmissionScope(
this.admissionScopeKey(scope.id, hostId === null ? null : scope.hostId)
)
}