diff --git a/src/main/daemon/terminal-host.test.ts b/src/main/daemon/terminal-host.test.ts index 8d69d455b07..2ad5cf90ccf 100644 --- a/src/main/daemon/terminal-host.test.ts +++ b/src/main/daemon/terminal-host.test.ts @@ -383,6 +383,29 @@ describe('TerminalHost', () => { expect(host.isKilled('session-1')).toBe(true) }) + it('does not tombstone a session when graceful kill admission fails', async () => { + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + lastSubprocess.kill = vi.fn(() => { + throw new Error('signal rejected') + }) + + expect(() => host.kill('session-1')).toThrow('signal rejected') + expect(host.isKilled('session-1')).toBe(false) + await expect( + host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + ).resolves.toMatchObject({ isNew: false }) + }) + it('force-kills immediately when requested', async () => { await host.createOrAttach({ sessionId: 'session-1', diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index 26dd4d7bdfa..526676050b3 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -208,11 +208,11 @@ export class TerminalHost { kill(sessionId: string, opts: { immediate?: boolean } = {}): Promise { const session = this.getAliveSession(sessionId) + const killed = opts.immediate + ? session.forceKillAndWaitForExit() + : Promise.resolve(session.kill()) this.recordTombstone(sessionId) - if (opts.immediate) { - return session.forceKillAndWaitForExit() - } - return Promise.resolve(session.kill()) + return killed } // Why: dispose a dead session's headless emulator and drop it from the map so diff --git a/src/main/ipc/watcher-removal-gate.test.ts b/src/main/ipc/watcher-removal-gate.test.ts index 5bb7bc50f7f..37eed327379 100644 --- a/src/main/ipc/watcher-removal-gate.test.ts +++ b/src/main/ipc/watcher-removal-gate.test.ts @@ -88,6 +88,16 @@ describe('watcher removal gate', () => { removal.release() }) + it('rejects enclosing installs while a nested root is being removed', async () => { + const removal = acquireWatcherRemovalGate('/repo/nested') + await removal.ready + + expect(() => beginWatcherInstall('/repo')).toThrow(WatcherRemovalInProgressError) + expect(() => beginTerminalInstall('/repo')).toThrow(TerminalRemovalInProgressError) + + removal.release() + }) + it('does not fence a distinct POSIX root containing a literal backslash', async () => { const removal = acquireWatcherRemovalGate('/srv/team\\repo') await removal.ready diff --git a/src/main/ipc/watcher-removal-gate.ts b/src/main/ipc/watcher-removal-gate.ts index accf279961b..5df7b7e098a 100644 --- a/src/main/ipc/watcher-removal-gate.ts +++ b/src/main/ipc/watcher-removal-gate.ts @@ -58,9 +58,14 @@ function beginRemovalSensitiveInstall( createRemovalError: () => Error ): () => void { const normalizedRoot = normalizeRuntimePathForComparison(rootPath) + // Why: PTY admission can fence both worktree identity and cwd, which may be + // parent/child roots; neither side may overlap an active removal. if ( matchingHostStates(connectionId).some( - (state) => state.removalCount > 0 && isPathInsideOrEqual(state.rootPath, normalizedRoot) + (state) => + state.removalCount > 0 && + (isPathInsideOrEqual(state.rootPath, normalizedRoot) || + isPathInsideOrEqual(normalizedRoot, state.rootPath)) ) ) { throw createRemovalError() diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index e300c594620..cd393f56d32 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -1417,5 +1417,21 @@ describe('LocalPtyProvider', () => { expect(killSpy).toHaveBeenCalledTimes(1) expect(destroySpy).not.toHaveBeenCalled() }) + + it('settles an overlapping shutdown when app quit takes final ownership', async () => { + mockProc.kill.mockImplementation(() => undefined) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + const shutdown = provider.shutdown(id, { immediate: true }) + let settled = false + void shutdown.then(() => { + settled = true + }) + await Promise.resolve() + expect(settled).toBe(false) + + provider.killAll() + + await expect(shutdown).resolves.toBeUndefined() + }) }) }) diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index e68799834e4..f44cae2ea08 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -1244,6 +1244,9 @@ export class LocalPtyProvider implements IPtyProvider { // Why: app quit cannot retain NAPI callbacks into FreeEnvironment; the // process exit itself is the final physical handle boundary here. destroyPtyProcess(proc, { alreadyKilled: true }) + // Why: app quit replaces node-pty's onExit callback as the final owner; + // overlapping shutdown waiters must join that same terminal boundary. + ptyPhysicalExits.get(id)?.markExited() clearPtyState(id) } } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 2a23c27077e..c5cc0780010 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -28725,6 +28725,28 @@ describe('OrcaRuntimeService', () => { expect(deleteWorktreeHistoryDirMock).not.toHaveBeenCalled() }) + it('stops worktree PTYs before removing linked paths', async () => { + const repo = { ...store.getRepos()[0], symlinkPaths: ['node_modules'] } + const runtimeStore = { ...store, getRepos: () => [repo], getRepo: () => repo } + const runtime = createWorktreeRemovalRuntime(runtimeStore) + const stopAndWait = vi.fn().mockResolvedValue(true) + runtime.setPtyController({ + write: () => true, + kill: () => true, + stopAndWait, + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, 'pty-1') + vi.mocked(removeWorktree).mockResolvedValue({}) + removeWorktreeLinkedPathsMock.mockImplementationOnce(async () => { + expect(stopAndWait).toHaveBeenCalledWith('pty-1') + }) + + await runtime.removeManagedWorktree(TEST_WORKTREE_ID) + + expect(removeWorktreeLinkedPathsMock).toHaveBeenCalledWith(TEST_WORKTREE_PATH, ['node_modules']) + }) + it('waits for every watcher layer to settle before restoring after teardown failure', async () => { const runtime = createRuntime() let finishRuntimeClose: () => void = () => {} diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 895c0e5db37..75e964f8d03 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -17711,14 +17711,14 @@ export class OrcaRuntimeService { const removalGate = await this.acquireFileWatcherRemoval(canonicalWorktreePath) let removalCompleted = false try { + // Why: linked-path deletion is destructive too; PTYs must release every + // handle before Windows or WSL filesystem cleanup starts. + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id) + if (linkedPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalWorktreePath, linkedPaths) } - // Why: the gate spans terminal shutdown through Git and any recursive - // fallback so a late headless spawn cannot recreate a native handle. - await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id) - try { const removeOptions = { ...(!deleteBranch ? { deleteBranch } : {}), diff --git a/src/main/runtime/worktree-teardown.test.ts b/src/main/runtime/worktree-teardown.test.ts index c2f18cec03a..fe008a3cfee 100644 --- a/src/main/runtime/worktree-teardown.test.ts +++ b/src/main/runtime/worktree-teardown.test.ts @@ -111,6 +111,26 @@ describe('killAllProcessesForWorktree', () => { expect(result.providerStopped).toBe(2) }) + it('does not use cwd fallback against a different authoritative worktree id', async () => { + const localProvider = createProviderStub(async () => [ + { + id: 'owned-by-sibling', + cwd: '/repo/app/nested', + title: 'shell', + worktreeId: 'repo-1::/repo/sibling' + } + ]) + listRegisteredPtysMock.mockReturnValue([]) + + const result = await killAllProcessesForWorktree('repo-1::/repo/app', { + localProvider, + requirePhysicalStop: true + }) + + expect(localProvider.shutdown).not.toHaveBeenCalled() + expect(result.providerStopped).toBe(0) + }) + it('uses authoritative remote worktree ownership without sweeping the local registry', async () => { const remoteProvider = createProviderStub(async () => [ { id: 'pty-remote', cwd: '/remote/w1', title: 'shell', worktreeId: 'w1' }, @@ -338,7 +358,7 @@ describe('killAllProcessesForWorktree', () => { }) it('tolerates runtime.stopTerminalsForWorktree throwing (headless assertGraphReady reject)', async () => { - const stopTerminalsForWorktree = vi.fn().mockRejectedValue(new Error('graph not ready')) + const stopTerminalsForWorktree = vi.fn().mockRejectedValue(new Error('runtime_unavailable')) const runtime = { stopTerminalsForWorktree } as unknown as Parameters[1]['runtime'] @@ -351,6 +371,23 @@ describe('killAllProcessesForWorktree', () => { expect(result.runtimeStopped).toBe(0) }) + it('fails destructive teardown closed when the runtime sweep rejects', async () => { + const stopTerminalsForWorktree = vi.fn().mockRejectedValue(new Error('runtime sweep failed')) + const runtime = { + stopTerminalsForWorktree + } as unknown as Parameters[1]['runtime'] + const localProvider = createProviderStub(async () => []) + listRegisteredPtysMock.mockReturnValue([]) + + await expect( + killAllProcessesForWorktree('w1', { + runtime, + localProvider, + requirePhysicalStop: true + }) + ).rejects.toThrow('runtime sweep failed') + }) + it('bounds the entire process sweep when a provider never settles', async () => { vi.useFakeTimers() try { diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index f89c340739d..8f4cf8d4fb3 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -76,13 +76,15 @@ export async function killAllProcessesForWorktree( return current } + // Why: headless CLI has no ready renderer graph; only that known sentinel + // may fall through to the provider and registry physical-owner sweeps. const runtimeSweep = deps.runtime ? settleBeforeDeadline( () => deps.runtime!.stopTerminalsForWorktree(worktreeId, { deadline, stopPty }), { stopped: 0 }, deadline, deps.requirePhysicalStop ? deadlineError : undefined, - false + (error) => !(error instanceof Error && error.message === 'runtime_unavailable') ) : Promise.resolve({ stopped: 0 }) const providerSweep = settleBeforeDeadline( @@ -139,7 +141,7 @@ async function settleBeforeDeadline( fallback: T, deadline: number, failClosedError?: Error, - failClosedOnRunError = true + failClosedOnRunError: (error: unknown) => boolean = () => true ): Promise { const remaining = deadline - Date.now() if (remaining <= 0) { @@ -172,7 +174,7 @@ async function settleBeforeDeadline( ) timer.unref?.() void run().then(finish, (error: unknown) => - failClosedError && failClosedOnRunError ? fail(error) : finish(fallback) + failClosedError && failClosedOnRunError(error) ? fail(error) : finish(fallback) ) }) } @@ -202,6 +204,7 @@ async function sweepProviderByPrefix( // and authoritative worktree ownership must remain usable during teardown. const cwdOwned = worktreePath !== undefined && + s.worktreeId === undefined && typeof s.cwd === 'string' && s.cwd.length > 0 && isPathInsideOrEqual(worktreePath, s.cwd) diff --git a/src/relay/relay-filesystem-watch-registry.test.ts b/src/relay/relay-filesystem-watch-registry.test.ts index fd6d2ca37d1..9a21fb9eace 100644 --- a/src/relay/relay-filesystem-watch-registry.test.ts +++ b/src/relay/relay-filesystem-watch-registry.test.ts @@ -199,6 +199,16 @@ describe('RelayFilesystemWatchRegistry', () => { expect(rejectSubscribe).toBeDefined() }) + it('propagates unexpected non-Error setup failures', async () => { + vi.spyOn(pool, 'subscribe').mockRejectedValueOnce('native setup failed') + const stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + try { + await expect(registry.watch('/repo', context(1))).rejects.toBe('native setup failed') + } finally { + stderr.mockRestore() + } + }) + it('rejects acknowledged teardown while another client still owns the watch', async () => { await registry.watch('/repo', context(1)) await registry.watch('/repo', context(2)) diff --git a/src/relay/relay-filesystem-watch-registry.ts b/src/relay/relay-filesystem-watch-registry.ts index 2e9c3809916..4acb786fa23 100644 --- a/src/relay/relay-filesystem-watch-registry.ts +++ b/src/relay/relay-filesystem-watch-registry.ts @@ -284,13 +284,12 @@ export class RelayFilesystemWatchRegistry { const expectedAbort = (error instanceof Error && error.name === 'AbortError') || (isWatcherProcessFailure(error) && error.code === 'subscribe_aborted') - if (!expectedAbort && error instanceof Error) { - process.stderr.write( - `[relay] File watcher not available for ${state.rootPath}: ${error.message}\n` - ) - throw error + if (expectedAbort) { + return } - return + const message = error instanceof Error ? error.message : String(error) + process.stderr.write(`[relay] File watcher not available for ${state.rootPath}: ${message}\n`) + throw error } if (context?.isStale()) { this.releaseWatchClient(state, clientId)