mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 08:03:13 +00:00
fix(windows): harden agent Job teardown safety
This commit is contained in:
OrcaWin
@@ -26,7 +26,7 @@ on:
|
||||
- 'tests/e2e/terminal-probe-input-sequence.unit.test.ts'
|
||||
- 'tests/e2e/terminal-restart-persistence.spec.ts'
|
||||
- 'src/main/daemon/**'
|
||||
- 'src/main/windows-pty-descendant-termination*.ts'
|
||||
- 'src/main/windows-pty-job-control.ts'
|
||||
- 'src/main/ipc/pty*.ts'
|
||||
- 'src/main/providers/**'
|
||||
- 'src/main/pty/**'
|
||||
|
||||
@@ -1,17 +1,3 @@
|
||||
diff --git a/binding.gyp b/binding.gyp
|
||||
index 5f63978b07ab50aaf7523219a2170ec737a6b5db..c73569f8bac0a79023d26123e10db8a074089cf1 100644
|
||||
--- a/binding.gyp
|
||||
+++ b/binding.gyp
|
||||
@@ -5,9 +5,6 @@
|
||||
],
|
||||
'conditions': [
|
||||
['OS=="win"', {
|
||||
- 'msvs_configuration_attributes': {
|
||||
- 'SpectreMitigation': 'Spectre'
|
||||
- },
|
||||
'msvs_settings': {
|
||||
'VCCLCompilerTool': {
|
||||
'AdditionalOptions': [
|
||||
diff --git a/deps/winpty/src/winpty.gyp b/deps/winpty/src/winpty.gyp
|
||||
index 1ac5758bedd8cf54f32280dea4e4aeb5afdee30d..e619813759c6f14694838bdfbd0ea5f8360130ef 100644
|
||||
--- a/deps/winpty/src/winpty.gyp
|
||||
@@ -34,26 +20,6 @@ index 1ac5758bedd8cf54f32280dea4e4aeb5afdee30d..e619813759c6f14694838bdfbd0ea5f8
|
||||
]
|
||||
},
|
||||
'targets' : [
|
||||
@@ -40,9 +40,6 @@
|
||||
'-lshell32',
|
||||
'-luser32',
|
||||
],
|
||||
- 'msvs_configuration_attributes': {
|
||||
- 'SpectreMitigation': 'Spectre'
|
||||
- },
|
||||
'msvs_settings': {
|
||||
# Specify this setting here to override a setting from somewhere
|
||||
# else, such as node's common.gypi.
|
||||
@@ -142,9 +139,6 @@
|
||||
'-ladvapi32',
|
||||
'-luser32',
|
||||
],
|
||||
- 'msvs_configuration_attributes': {
|
||||
- 'SpectreMitigation': 'Spectre'
|
||||
- },
|
||||
'msvs_settings': {
|
||||
# Specify this setting here to override a setting from somewhere
|
||||
# else, such as node's common.gypi.
|
||||
diff --git a/lib/conpty_console_list_agent.js b/lib/conpty_console_list_agent.js
|
||||
index 8c4fca9022a6d6f015bca87f61625cde2278f428..201fc1d70a907e31b5a2b33b19a9db20803bc652 100644
|
||||
--- a/lib/conpty_console_list_agent.js
|
||||
|
||||
@@ -712,19 +712,20 @@
|
||||
"ssh",
|
||||
"runtime"
|
||||
],
|
||||
"coverageNotes": "Live macOS Electron tests prove exact local PTY disappearance after parked-tab close and detached-pgid descendant death after agent close. A Windows 2022 Actions red/green run proves production store.removeWorktree(id, true) retires an agent PTY and its reparented native lock-holder before deleting the worktree, while a separate live test proves plain terminal close preserves an intentionally detached child. Deterministic tests cover native Windows agent-only Job ownership, atomic spawn failure, fail-closed Job drain, cleanup-only taskkill fallback, daemon-wide shutdown, daemon and SSH routing, local/daemon pending-snapshot ownership across natural exit, stale-root descendant-signal suppression, graceful-to-immediate kill upgrades, duplicate-kill completion sharing, locale-stable bounded/fresh/coalesced process-table reads, deadline-safe successor scans, cycle-safe linear descendant traversal, target-only escalation indexing, 32-wide bulk teardown, source-scan timestamp identity, same-second PID ambiguity, ordinary runtime close ownership, unified-only hydration, split ownership, pane detach transfer, restart alias hydration, and late-hook suppression; live Linux, WSL, SSH, and remote-runtime process evidence remains pending.",
|
||||
"coverageNotes": "Live macOS Electron tests prove exact local PTY disappearance after parked-tab close and detached-pgid descendant death after agent close. A Windows 2022 Actions red/green run proves production store.removeWorktree(id, true) retires an agent PTY and its reparented native lock-holder before deleting the worktree, while a separate live test proves plain terminal close preserves an intentionally detached child. Deterministic tests cover native Windows agent-only Job ownership, atomic spawn failure, fail-closed Job drain and bridge errors, safe failure when Job ownership is unavailable, daemon-wide shutdown, daemon and SSH routing, local/daemon pending-snapshot ownership across natural exit, stale-root descendant-signal suppression, graceful-to-immediate kill upgrades, duplicate-kill completion sharing, locale-stable bounded/fresh/coalesced process-table reads, deadline-safe successor scans, cycle-safe linear descendant traversal, target-only escalation indexing, 32-wide bulk teardown, source-scan timestamp identity, same-second PID ambiguity, ordinary runtime close ownership, unified-only hydration, split ownership, pane detach transfer, restart alias hydration, and late-hook suppression; live Linux, WSL, SSH, and remote-runtime process evidence remains pending.",
|
||||
"motivatingLinks": [
|
||||
"https://github.com/stablyai/orca/pull/8628",
|
||||
"https://github.com/stablyai/orca/pull/8706",
|
||||
"https://github.com/stablyai/orca/issues/9045"
|
||||
],
|
||||
"invariant": "Close permanently removes the owned provider session, agent descendants, and resume authority even when no TerminalPane is mounted; a native Windows agent PTY owns its spawn-time process family in a kill-on-close Job that must drain before destructive worktree deletion succeeds, while plain terminals, WSL, and SSH retain their existing semantics; a terminating id remains reserved through natural exit, duplicate callers await the same completion, and immediate teardown upgrades any graceful request without signalling a recycled PID or a descendant tree after root ownership is lost; process-table work is locale-stable, bounded, fresh for each post-start request, same-turn coalesced, and begins within the requesting caller's deadline, including bulk worktree cleanup; detach and park preserve ownership; aliases prevent a detached agent's immutable physical pane key from being retired with its former tab.",
|
||||
"oracle": "Capture the exact PTY before parking, prove it remains listed while the view is absent, close through the product state boundary, and poll the provider inventory until that exact ID disappears; an agent-marked PTY's detached-pgid child is alive before close and absent afterward. On Windows, launch PowerShell shell -> agent Node process -> exited launcher -> reparented native lock-holder, invoke production store.removeWorktree(id, true), and prove the exact PTY, root, agent, lock-holder, worktree directory, Git registration, and worktree list entry are absent; separately prove a plain terminal's intentionally detached child survives close. Unit tests require agent-only spawn-time Job ownership, Job termination before root close, ActiveProcesses zero before success, fail-closed timeout/rejection behavior, cleanup-only taskkill fallback, daemon-wide routing through the same teardown, naturally exited id reservation, bounded locale-stable process-table reads, duplicate completion sharing, canonical owner dedupe, exact pane tombstones, chained detach transfer, and restart alias restoration.",
|
||||
"oracle": "Capture the exact PTY before parking, prove it remains listed while the view is absent, close through the product state boundary, and poll the provider inventory until that exact ID disappears; an agent-marked PTY's detached-pgid child is alive before close and absent afterward. On Windows, prove the fixture is Git-registered, launch PowerShell shell -> agent Node process -> exited launcher -> reparented native lock-holder, invoke production store.removeWorktree(id, true), and prove the exact PTY, root, agent, lock-holder, worktree directory, Git registration, and worktree list entry are absent; separately prove a plain terminal's intentionally detached child survives close. Unit tests require agent-only spawn-time Job ownership, Job termination before root close, ActiveProcesses zero before success, fail-closed timeout/rejection/synchronous-throw behavior, safe failure without PID-only fallback when Job ownership is unavailable, daemon-wide routing through the same teardown, naturally exited id reservation, bounded locale-stable process-table reads, duplicate completion sharing, canonical owner dedupe, exact pane tombstones, chained detach transfer, and restart alias restoration.",
|
||||
"commands": [
|
||||
"pnpm dlx node@24 ./node_modules/vitest/vitest.mjs run --config config/vitest.config.ts src/main/agent-hooks/server-pane-authority.test.ts src/main/ipc/agent-hooks.test.ts src/main/ipc/agent-pane-authority-ownership.test.ts src/main/ipc/pty-management.test.ts src/main/persistence.test.ts src/renderer/src/store/slices/agent-pane-authority.test.ts src/renderer/src/store/slices/terminal-pane-detach-agent-identity.test.ts src/renderer/src/store/slices/terminal-tab-retirement.test.ts src/renderer/src/store/slices/terminal-tab-retirement-store.test.ts src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts",
|
||||
"pnpm exec vitest run --config config/vitest.config.ts src/main/pty-descendant-termination.test.ts src/main/windows-pty-descendant-termination.test.ts src/main/daemon/session.test.ts src/main/daemon/terminal-host.test.ts src/main/daemon/terminal-session-teardown.test.ts src/main/daemon/pty-subprocess.test.ts src/main/providers/local-pty-provider.test.ts src/main/runtime/worktree-teardown.test.ts",
|
||||
"pnpm exec vitest run --config config/vitest.config.ts src/main/pty-descendant-termination.test.ts src/main/daemon/session.test.ts src/main/daemon/terminal-host.test.ts src/main/daemon/terminal-session-teardown.test.ts src/main/daemon/pty-subprocess.test.ts src/main/providers/local-pty-provider.test.ts src/main/runtime/worktree-teardown.test.ts",
|
||||
"pnpm run test:e2e -- tests/e2e/terminal-parked-close-retirement.spec.ts --workers=1",
|
||||
"pnpm run test:e2e -- tests/e2e/agent-descendant-process-kill.spec.ts --workers=1"
|
||||
"pnpm run test:e2e -- tests/e2e/agent-descendant-process-kill.spec.ts --workers=1",
|
||||
"gh workflow run windows-terminal-restart-e2e.yml --ref OrcaWin/issue-9045-worktree-deletion-fails"
|
||||
],
|
||||
"testFiles": [
|
||||
"src/main/agent-hooks/server-pane-authority.test.ts",
|
||||
@@ -733,7 +734,6 @@
|
||||
"src/main/ipc/pty-management.test.ts",
|
||||
"src/main/persistence.test.ts",
|
||||
"src/main/pty-descendant-termination.test.ts",
|
||||
"src/main/windows-pty-descendant-termination.test.ts",
|
||||
"src/main/daemon/session.test.ts",
|
||||
"src/main/daemon/terminal-host.test.ts",
|
||||
"src/main/daemon/terminal-session-teardown.test.ts",
|
||||
@@ -792,8 +792,8 @@
|
||||
{
|
||||
"file": "src/main/daemon/terminal-session-teardown.test.ts",
|
||||
"assertions": [
|
||||
"native Windows agent Job termination precedes root force-kill and a non-draining or rejected Job fails closed",
|
||||
"plain terminals retain detached descendants, cleanup-only taskkill fallback cannot prove Job drain, and daemon-wide shutdown uses coordinated teardown"
|
||||
"native Windows agent Job termination precedes root force-kill and a non-draining, rejected, or synchronously throwing Job fails closed after root closure",
|
||||
"plain terminals retain detached descendants, missing Job ownership fails without a PID-only fallback, and daemon-wide shutdown uses coordinated teardown"
|
||||
]
|
||||
},
|
||||
{
|
||||
@@ -805,14 +805,8 @@
|
||||
{
|
||||
"file": "src/main/providers/local-pty-provider.test.ts",
|
||||
"assertions": [
|
||||
"native Windows agents terminate their Job before ConPTY close and retain ownership when drain fails",
|
||||
"plain Windows terminals and WSL avoid native Job-tree termination while legacy sessions use taskkill only as cleanup"
|
||||
]
|
||||
},
|
||||
{
|
||||
"file": "src/main/windows-pty-descendant-termination.test.ts",
|
||||
"assertions": [
|
||||
"cleanup-only taskkill uses a bounded System32 tree kill and reports timeout, spawn, and non-zero-exit failures"
|
||||
"native Windows agents terminate their Job before ConPTY close and close the root on drain, rejection, synchronous bridge, or missing-ownership failure",
|
||||
"plain Windows terminals and WSL avoid native Job-tree termination"
|
||||
]
|
||||
},
|
||||
{
|
||||
@@ -863,15 +857,15 @@
|
||||
"date": "2026-07-20",
|
||||
"runner": "ci",
|
||||
"platform": "windows",
|
||||
"command": "pnpm run test:e2e -- tests/e2e/agent-descendant-process-kill.spec.ts --workers=1",
|
||||
"command": "gh workflow run windows-terminal-restart-e2e.yml --ref OrcaWin/issue-9045-worktree-deletion-fails",
|
||||
"result": "passed",
|
||||
"durationSeconds": 471,
|
||||
"summary": "At dea7375dd, all five Windows E2Es passed; the issue #9045 worktree-deletion test completed in 25.2 seconds and proved its PTY, root, agent, reparented native lock-holder, worktree directory, Git registration, and worktree list entry were absent, while the plain-terminal regression test proved its detached child survived close."
|
||||
"summary": "Workflow run 29731617563 checked out dea7375dd, installed dependencies, built Electron, and ran the three registered specs with the workflow's five-test grep under SKIP_BUILD=1. All five Windows E2Es passed; the issue #9045 test completed in 25.2 seconds and proved its PTY, root, agent, reparented native lock-holder, worktree directory, Git registration, and worktree list entry were absent, while the plain-terminal regression test proved its detached child survived close."
|
||||
}
|
||||
],
|
||||
"runtimeBudget": {
|
||||
"p95Seconds": 60,
|
||||
"scope": "fresh E2E build plus isolated local Electron parked-close and descendant-kill tests"
|
||||
"p95Seconds": 600,
|
||||
"scope": "local fresh-build Electron proofs plus the Windows CI job from checkout and native dependency build through five selected Electron E2Es"
|
||||
},
|
||||
"flakeHistory": {
|
||||
"status": "unknown",
|
||||
|
||||
Generated
+3
-3
@@ -21,7 +21,7 @@ patchedDependencies:
|
||||
hash: 9c1de9931d86864923ff53bc9d64474a86478085ac81ea4e432648c7e23d702c
|
||||
path: config/patches/@xterm__xterm@6.1.0-beta.287.patch
|
||||
node-pty@1.1.0:
|
||||
hash: 0b44048c9daec38eee7bca718b1de63f8bf08338c9acee12d257405a246a9161
|
||||
hash: 37444feda4521a62bd76f2abc04efe142a09134f26b1ef0a7f4cf0e0bbbfdedf
|
||||
path: config/patches/node-pty@1.1.0.patch
|
||||
|
||||
importers:
|
||||
@@ -63,7 +63,7 @@ importers:
|
||||
version: 3.3.1
|
||||
node-pty:
|
||||
specifier: ^1.1.0
|
||||
version: 1.1.0(patch_hash=0b44048c9daec38eee7bca718b1de63f8bf08338c9acee12d257405a246a9161)
|
||||
version: 1.1.0(patch_hash=37444feda4521a62bd76f2abc04efe142a09134f26b1ef0a7f4cf0e0bbbfdedf)
|
||||
posthog-node:
|
||||
specifier: ^5.33.3
|
||||
version: 5.33.3
|
||||
@@ -12043,7 +12043,7 @@ snapshots:
|
||||
undici: 6.27.0
|
||||
which: 6.0.1
|
||||
|
||||
node-pty@1.1.0(patch_hash=0b44048c9daec38eee7bca718b1de63f8bf08338c9acee12d257405a246a9161):
|
||||
node-pty@1.1.0(patch_hash=37444feda4521a62bd76f2abc04efe142a09134f26b1ef0a7f4cf0e0bbbfdedf):
|
||||
dependencies:
|
||||
node-addon-api: 7.1.1
|
||||
|
||||
|
||||
@@ -4,15 +4,10 @@ import { TerminalSessionTeardown } from './terminal-session-teardown'
|
||||
import { shutdownTerminalHostSessions } from './terminal-host-session-shutdown'
|
||||
|
||||
const killWithDescendantSweepMock = vi.hoisted(() => vi.fn())
|
||||
const requestWindowsDescendantTreeTerminationMock = vi.hoisted(() => vi.fn())
|
||||
|
||||
vi.mock('../pty-descendant-termination', () => ({
|
||||
killWithDescendantSweep: killWithDescendantSweepMock
|
||||
}))
|
||||
vi.mock('../windows-pty-descendant-termination', () => ({
|
||||
requestWindowsDescendantTreeTermination: requestWindowsDescendantTreeTerminationMock,
|
||||
WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS: 8_000
|
||||
}))
|
||||
|
||||
type MockSession = Session & {
|
||||
setAlive(value: boolean): void
|
||||
@@ -50,8 +45,6 @@ describe('TerminalSessionTeardown Windows descendant termination', () => {
|
||||
beforeEach(() => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
|
||||
killWithDescendantSweepMock.mockReset()
|
||||
requestWindowsDescendantTreeTerminationMock.mockReset()
|
||||
requestWindowsDescendantTreeTerminationMock.mockResolvedValue(undefined)
|
||||
sessions = new Map()
|
||||
teardown = new TerminalSessionTeardown(sessions)
|
||||
})
|
||||
@@ -74,7 +67,6 @@ describe('TerminalSessionTeardown Windows descendant termination', () => {
|
||||
expect(session.terminateJobTree.mock.invocationCallOrder[0]).toBeLessThan(
|
||||
session.forceKillAndWaitForExit.mock.invocationCallOrder[0]!
|
||||
)
|
||||
expect(requestWindowsDescendantTreeTerminationMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('rejects fail-closed after closing the root when the agent Job does not drain', async () => {
|
||||
@@ -107,7 +99,7 @@ describe('TerminalSessionTeardown Windows descendant termination', () => {
|
||||
expect(teardown.get('agent')).toBeUndefined()
|
||||
})
|
||||
|
||||
it('uses taskkill before the agent root when Job assignment was unavailable', async () => {
|
||||
it('closes the root and fails closed when Job assignment was unavailable', async () => {
|
||||
const session = createSession({ agent: true, nativeWindows: true, hasJob: false })
|
||||
sessions.set('agent', session)
|
||||
|
||||
@@ -115,10 +107,22 @@ describe('TerminalSessionTeardown Windows descendant termination', () => {
|
||||
'Windows PTY Job ownership unavailable'
|
||||
)
|
||||
|
||||
expect(requestWindowsDescendantTreeTerminationMock).toHaveBeenCalledWith(99999)
|
||||
expect(requestWindowsDescendantTreeTerminationMock.mock.invocationCallOrder[0]).toBeLessThan(
|
||||
session.forceKillAndWaitForExit.mock.invocationCallOrder[0]!
|
||||
expect(session.forceKillAndWaitForExit).toHaveBeenCalledOnce()
|
||||
})
|
||||
|
||||
it('closes the root when the native Job bridge throws synchronously', async () => {
|
||||
const session = createSession({ agent: true, nativeWindows: true })
|
||||
session.terminateJobTree.mockImplementation(() => {
|
||||
throw new Error('synchronous native Job failure')
|
||||
})
|
||||
sessions.set('agent', session)
|
||||
|
||||
await expect(teardown.killSession('agent', session, true)).rejects.toThrow(
|
||||
'synchronous native Job failure'
|
||||
)
|
||||
|
||||
expect(session.forceKillAndWaitForExit).toHaveBeenCalledOnce()
|
||||
expect(teardown.get('agent')).toBeUndefined()
|
||||
})
|
||||
|
||||
it('preserves native Job descendants for plain terminals', async () => {
|
||||
@@ -129,25 +133,6 @@ describe('TerminalSessionTeardown Windows descendant termination', () => {
|
||||
expect(normal.terminateJobTree).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not signal a root that exits while taskkill is pending', async () => {
|
||||
let finishFallback!: () => void
|
||||
requestWindowsDescendantTreeTerminationMock.mockReturnValue(
|
||||
new Promise<void>((resolve) => {
|
||||
finishFallback = resolve
|
||||
})
|
||||
)
|
||||
const session = createSession({ agent: true, nativeWindows: true, hasJob: false })
|
||||
sessions.set('agent', session)
|
||||
|
||||
const killing = teardown.killSession('agent', session, true)
|
||||
session.setAlive(false)
|
||||
sessions.delete('agent')
|
||||
finishFallback()
|
||||
await expect(killing).rejects.toThrow('Windows PTY Job ownership unavailable')
|
||||
|
||||
expect(session.forceKillAndWaitForExit).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('routes daemon-wide shutdown through coordinated session teardown', async () => {
|
||||
const session = createSession({ agent: true, nativeWindows: true })
|
||||
const detachAllClients = vi.fn()
|
||||
|
||||
@@ -1,8 +1,5 @@
|
||||
import { killWithDescendantSweep } from '../pty-descendant-termination'
|
||||
import {
|
||||
requestWindowsDescendantTreeTermination,
|
||||
WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS
|
||||
} from '../windows-pty-descendant-termination'
|
||||
import { WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS } from '../windows-pty-job-control'
|
||||
import type { Session } from './session'
|
||||
|
||||
type AgentTeardownOperation = {
|
||||
@@ -55,7 +52,7 @@ export class TerminalSessionTeardown {
|
||||
): void | Promise<void> {
|
||||
return this.killCoordinatedSession(sessionId, session, immediate, (killRoot, ownsRoot) => {
|
||||
if (process.platform === 'win32' && session.ownsNativeWindowsPty) {
|
||||
return this.terminateWindowsTree(session, killRoot, ownsRoot)
|
||||
return this.terminateWindowsTree(session, killRoot)
|
||||
}
|
||||
return killWithDescendantSweep(session.pid, killRoot, { ownsRoot })
|
||||
})
|
||||
@@ -138,32 +135,24 @@ export class TerminalSessionTeardown {
|
||||
return operation
|
||||
}
|
||||
|
||||
private async terminateWindowsTree(
|
||||
session: Session,
|
||||
killRoot: () => void,
|
||||
ownsRoot: () => boolean
|
||||
): Promise<void> {
|
||||
const nativeCompletion = session.terminateJobTree(WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS)
|
||||
if (nativeCompletion) {
|
||||
private async terminateWindowsTree(session: Session, killRoot: () => void): Promise<void> {
|
||||
let nativeCompletion: Promise<boolean> | undefined
|
||||
try {
|
||||
nativeCompletion = session.terminateJobTree(WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS)
|
||||
} catch (error) {
|
||||
// Why: even a synchronous native bridge failure must not strand the
|
||||
// still-owned ConPTY root while destructive teardown fails closed.
|
||||
killRoot()
|
||||
if (!(await nativeCompletion)) {
|
||||
throw new Error(`Windows PTY Job did not drain for process ${session.pid}`)
|
||||
}
|
||||
return
|
||||
throw error
|
||||
}
|
||||
if (!ownsRoot()) {
|
||||
throw new Error(`Windows PTY root exited without Job ownership for process ${session.pid}`)
|
||||
if (!nativeCompletion) {
|
||||
killRoot()
|
||||
throw new Error(`Windows PTY Job ownership unavailable for process ${session.pid}`)
|
||||
}
|
||||
|
||||
try {
|
||||
await requestWindowsDescendantTreeTermination(session.pid)
|
||||
// taskkill cannot identify a descendant after its launcher exits, so a
|
||||
// successful request is cleanup only and never authorizes deletion.
|
||||
throw new Error(`Windows PTY Job ownership unavailable for process ${session.pid}`)
|
||||
} finally {
|
||||
// Why: fallback failure must not leave the agent root running, while the
|
||||
// rejected operation still keeps destructive callers fail-closed.
|
||||
killRoot()
|
||||
killRoot()
|
||||
if (!(await nativeCompletion)) {
|
||||
throw new Error(`Windows PTY Job did not drain for process ${session.pid}`)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,8 +14,7 @@ const {
|
||||
resolveAgentForegroundProcessMock,
|
||||
readWindowsConptyProcessIdsMock,
|
||||
captureDescendantSnapshotMock,
|
||||
terminateDescendantSnapshotMock,
|
||||
requestWindowsDescendantTreeTerminationMock
|
||||
terminateDescendantSnapshotMock
|
||||
} = vi.hoisted(() => ({
|
||||
existsSyncMock: vi.fn(),
|
||||
statSyncMock: vi.fn(),
|
||||
@@ -27,8 +26,7 @@ const {
|
||||
resolveAgentForegroundProcessMock: vi.fn(),
|
||||
readWindowsConptyProcessIdsMock: vi.fn(),
|
||||
captureDescendantSnapshotMock: vi.fn(),
|
||||
terminateDescendantSnapshotMock: vi.fn(),
|
||||
requestWindowsDescendantTreeTerminationMock: vi.fn()
|
||||
terminateDescendantSnapshotMock: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('fs', () => ({
|
||||
@@ -61,11 +59,6 @@ vi.mock('../pty-descendant-termination', () => ({
|
||||
terminateDescendantSnapshot: terminateDescendantSnapshotMock
|
||||
}))
|
||||
|
||||
vi.mock('../windows-pty-descendant-termination', () => ({
|
||||
requestWindowsDescendantTreeTermination: requestWindowsDescendantTreeTerminationMock,
|
||||
WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS: 8_000
|
||||
}))
|
||||
|
||||
// Resolve PowerShell family names to deterministic absolute paths (the fs mock
|
||||
// above otherwise makes every probe miss). The real resolver — which skips the
|
||||
// Store App Execution Alias stub — is covered in
|
||||
@@ -163,8 +156,6 @@ describe('LocalPtyProvider', () => {
|
||||
terminateDescendantSnapshotMock.mockReset()
|
||||
prepareMacosTccLoginShellMock.mockReset()
|
||||
prepareMacosTccLoginShellMock.mockResolvedValue(undefined)
|
||||
requestWindowsDescendantTreeTerminationMock.mockReset()
|
||||
requestWindowsDescendantTreeTerminationMock.mockResolvedValue(undefined)
|
||||
resolveAgentForegroundProcessMock.mockReset()
|
||||
resolveAgentForegroundProcessMock.mockImplementation(
|
||||
async (_pid: number, fallbackProcess: string | null) => ({
|
||||
@@ -1177,7 +1168,6 @@ describe('LocalPtyProvider', () => {
|
||||
await provider.shutdown(id, { immediate: true })
|
||||
|
||||
expect(order).toEqual(['job', 'conpty'])
|
||||
expect(requestWindowsDescendantTreeTerminationMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('rejects fail-closed after closing ConPTY when the agent Job does not drain', async () => {
|
||||
@@ -1204,6 +1194,21 @@ describe('LocalPtyProvider', () => {
|
||||
expect(provider.hasPty(id)).toBe(false)
|
||||
})
|
||||
|
||||
it('closes ConPTY when native agent Job termination throws synchronously', async () => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
|
||||
mockProc.terminateJobTree.mockImplementation(() => {
|
||||
throw new Error('synchronous native Job failure')
|
||||
})
|
||||
const { id } = await provider.spawn({ cols: 80, rows: 24, launchAgent: 'claude' })
|
||||
|
||||
await expect(provider.shutdown(id, { immediate: true })).rejects.toThrow(
|
||||
'synchronous native Job failure'
|
||||
)
|
||||
|
||||
expect(mockProc.kill).toHaveBeenCalledOnce()
|
||||
expect(provider.hasPty(id)).toBe(false)
|
||||
})
|
||||
|
||||
it('preserves plain detached-child semantics for normal Windows tab close', async () => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
|
||||
const { id } = await provider.spawn({ cols: 80, rows: 24 })
|
||||
@@ -1211,7 +1216,6 @@ describe('LocalPtyProvider', () => {
|
||||
await provider.shutdown(id, { immediate: true })
|
||||
|
||||
expect(mockProc.terminateJobTree).not.toHaveBeenCalled()
|
||||
expect(requestWindowsDescendantTreeTerminationMock).not.toHaveBeenCalled()
|
||||
expect(spawnMock).toHaveBeenCalledWith(
|
||||
expect.any(String),
|
||||
expect.any(Array),
|
||||
@@ -1219,7 +1223,7 @@ describe('LocalPtyProvider', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('falls back to taskkill when the Windows PTY has no Job Object', async () => {
|
||||
it('closes ConPTY and fails closed when the Windows PTY has no Job Object', async () => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
|
||||
mockProc.terminateJobTree.mockReturnValue(undefined)
|
||||
const { id } = await provider.spawn({ cols: 80, rows: 24, launchAgent: 'codex' })
|
||||
@@ -1228,10 +1232,8 @@ describe('LocalPtyProvider', () => {
|
||||
'Windows PTY Job ownership unavailable'
|
||||
)
|
||||
|
||||
expect(requestWindowsDescendantTreeTerminationMock).toHaveBeenCalledWith(mockProc.pid)
|
||||
expect(requestWindowsDescendantTreeTerminationMock.mock.invocationCallOrder[0]).toBeLessThan(
|
||||
mockProc.kill.mock.invocationCallOrder[0]!
|
||||
)
|
||||
expect(mockProc.kill).toHaveBeenCalledOnce()
|
||||
expect(provider.hasPty(id)).toBe(false)
|
||||
})
|
||||
|
||||
it('never runs native Windows tree termination for a WSL terminal', async () => {
|
||||
@@ -1246,7 +1248,6 @@ describe('LocalPtyProvider', () => {
|
||||
await provider.shutdown(id, { immediate: true })
|
||||
|
||||
expect(mockProc.terminateJobTree).not.toHaveBeenCalled()
|
||||
expect(requestWindowsDescendantTreeTerminationMock).not.toHaveBeenCalled()
|
||||
expect(spawnMock).toHaveBeenCalledWith(
|
||||
expect.any(String),
|
||||
expect.any(Array),
|
||||
|
||||
@@ -60,10 +60,7 @@ import {
|
||||
captureDescendantSnapshot,
|
||||
terminateDescendantSnapshot
|
||||
} from '../pty-descendant-termination'
|
||||
import {
|
||||
requestWindowsDescendantTreeTermination,
|
||||
WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS
|
||||
} from '../windows-pty-descendant-termination'
|
||||
import { WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS } from '../windows-pty-job-control'
|
||||
import { readWindowsConptyProcessIds } from './windows-conpty-process-membership'
|
||||
import { canConfirmAgentFromConsolePresence } from './windows-console-foreground'
|
||||
import { forceKillPosixPtyProcessGroups } from '../pty/posix-pty-process-groups'
|
||||
@@ -291,27 +288,26 @@ type WindowsPtyJobControl = pty.IPty & {
|
||||
}
|
||||
|
||||
async function terminateWindowsPtyTree(proc: pty.IPty, closeRoot: () => void): Promise<void> {
|
||||
const nativeCompletion = (proc as WindowsPtyJobControl).terminateJobTree?.call(
|
||||
proc,
|
||||
WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS
|
||||
)
|
||||
if (nativeCompletion) {
|
||||
let nativeCompletion: Promise<boolean> | undefined
|
||||
try {
|
||||
nativeCompletion = (proc as WindowsPtyJobControl).terminateJobTree?.call(
|
||||
proc,
|
||||
WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS
|
||||
)
|
||||
} catch (error) {
|
||||
// Why: a synchronous native bridge failure must still close ConPTY while
|
||||
// the rejected shutdown keeps destructive deletion fail-closed.
|
||||
closeRoot()
|
||||
if (!(await nativeCompletion)) {
|
||||
throw new Error(`Windows PTY Job did not drain for process ${proc.pid}`)
|
||||
}
|
||||
return
|
||||
throw error
|
||||
}
|
||||
if (!nativeCompletion) {
|
||||
closeRoot()
|
||||
throw new Error(`Windows PTY Job ownership unavailable for process ${proc.pid}`)
|
||||
}
|
||||
|
||||
try {
|
||||
await requestWindowsDescendantTreeTermination(proc.pid)
|
||||
// taskkill cannot identify a descendant after its launcher exits, so a
|
||||
// successful request is cleanup only and never authorizes deletion.
|
||||
throw new Error(`Windows PTY Job ownership unavailable for process ${proc.pid}`)
|
||||
} finally {
|
||||
// Why: fallback failure must not leave the agent root alive, but callers
|
||||
// still receive the failure instead of treating worktree handles as free.
|
||||
closeRoot()
|
||||
closeRoot()
|
||||
if (!(await nativeCompletion)) {
|
||||
throw new Error(`Windows PTY Job did not drain for process ${proc.pid}`)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1,92 +0,0 @@
|
||||
import { EventEmitter } from 'node:events'
|
||||
import type { ChildProcess } from 'node:child_process'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
requestWindowsDescendantTreeTermination,
|
||||
WINDOWS_DESCENDANT_KILL_TIMEOUT_MS,
|
||||
type WindowsTreeKillSpawner
|
||||
} from './windows-pty-descendant-termination'
|
||||
|
||||
function createChild(): Pick<ChildProcess, 'kill' | 'once' | 'unref'> & EventEmitter {
|
||||
const child = new EventEmitter() as Pick<ChildProcess, 'kill' | 'once' | 'unref'> & EventEmitter
|
||||
child.kill = vi.fn(() => true)
|
||||
child.unref = vi.fn()
|
||||
return child
|
||||
}
|
||||
|
||||
describe('requestWindowsDescendantTreeTermination', () => {
|
||||
afterEach(() => {
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
it('runs the System32 taskkill binary with a forceful tree argv', async () => {
|
||||
const child = createChild()
|
||||
const spawnProcess = vi.fn(() => child) as unknown as WindowsTreeKillSpawner
|
||||
const killing = requestWindowsDescendantTreeTermination(4321, {
|
||||
spawn: spawnProcess,
|
||||
env: { SystemRoot: 'C:\\Windows' }
|
||||
})
|
||||
|
||||
expect(spawnProcess).toHaveBeenCalledWith(
|
||||
'C:\\Windows\\System32\\taskkill.exe',
|
||||
['/PID', '4321', '/T', '/F'],
|
||||
{ shell: false, stdio: 'ignore', windowsHide: true }
|
||||
)
|
||||
expect(child.unref).toHaveBeenCalledOnce()
|
||||
|
||||
child.emit('exit', 0, null)
|
||||
await expect(killing).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
it('falls back to the bare binary and reports a non-zero exit', async () => {
|
||||
const child = createChild()
|
||||
const spawnProcess = vi.fn(() => child) as unknown as WindowsTreeKillSpawner
|
||||
const killing = requestWindowsDescendantTreeTermination(77, {
|
||||
spawn: spawnProcess,
|
||||
env: {}
|
||||
})
|
||||
|
||||
expect(spawnProcess).toHaveBeenCalledWith(
|
||||
'taskkill',
|
||||
['/PID', '77', '/T', '/F'],
|
||||
expect.any(Object)
|
||||
)
|
||||
child.emit('exit', 128, null)
|
||||
await expect(killing).rejects.toThrow('taskkill exited 128')
|
||||
})
|
||||
|
||||
it.each([0, -1, 1.5, Number.NaN, undefined])(
|
||||
'does not spawn for invalid pid %s',
|
||||
async (rootPid) => {
|
||||
const spawnProcess = vi.fn() as unknown as WindowsTreeKillSpawner
|
||||
|
||||
await expect(
|
||||
requestWindowsDescendantTreeTermination(rootPid as number, { spawn: spawnProcess })
|
||||
).rejects.toThrow('Invalid Windows PTY root PID')
|
||||
expect(spawnProcess).not.toHaveBeenCalled()
|
||||
}
|
||||
)
|
||||
|
||||
it('bounds a hung taskkill process and cancels it', async () => {
|
||||
vi.useFakeTimers()
|
||||
const child = createChild()
|
||||
const spawnProcess = vi.fn(() => child) as unknown as WindowsTreeKillSpawner
|
||||
const killing = requestWindowsDescendantTreeTermination(99, { spawn: spawnProcess })
|
||||
const rejection = expect(killing).rejects.toThrow('taskkill timed out')
|
||||
|
||||
await vi.advanceTimersByTimeAsync(WINDOWS_DESCENDANT_KILL_TIMEOUT_MS)
|
||||
|
||||
expect(child.kill).toHaveBeenCalledOnce()
|
||||
await rejection
|
||||
})
|
||||
|
||||
it('reports spawn errors instead of treating them as proof', async () => {
|
||||
const child = createChild()
|
||||
const spawnProcess = vi.fn(() => child) as unknown as WindowsTreeKillSpawner
|
||||
const killing = requestWindowsDescendantTreeTermination(123, { spawn: spawnProcess })
|
||||
|
||||
child.emit('error', new Error('blocked'))
|
||||
|
||||
await expect(killing).rejects.toThrow('taskkill failed')
|
||||
})
|
||||
})
|
||||
@@ -1,89 +0,0 @@
|
||||
import { spawn, type ChildProcess, type SpawnOptions } from 'node:child_process'
|
||||
import { win32 as pathWin32 } from 'node:path'
|
||||
|
||||
export const WINDOWS_DESCENDANT_KILL_TIMEOUT_MS = 2_000
|
||||
export const WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS = 8_000
|
||||
|
||||
type WindowsTreeKillProcess = Pick<ChildProcess, 'kill' | 'once' | 'unref'>
|
||||
|
||||
export type WindowsTreeKillSpawner = (
|
||||
file: string,
|
||||
args: string[],
|
||||
options: SpawnOptions
|
||||
) => WindowsTreeKillProcess
|
||||
|
||||
type WindowsTreeKillDeps = {
|
||||
spawn?: WindowsTreeKillSpawner
|
||||
env?: NodeJS.ProcessEnv
|
||||
timeoutMs?: number
|
||||
}
|
||||
|
||||
/**
|
||||
* Best-effort fallback for Windows hosts where node-pty could not assign the
|
||||
* shell to a Job Object. taskkill follows only live PPID links, so it cannot
|
||||
* reach a detached child whose launcher already exited. It also accepts only
|
||||
* a PID, leaving a small reuse window after node-pty closes the shell handle.
|
||||
*/
|
||||
export function requestWindowsDescendantTreeTermination(
|
||||
rootPid: number,
|
||||
deps: WindowsTreeKillDeps = {}
|
||||
): Promise<void> {
|
||||
if (!Number.isInteger(rootPid) || rootPid <= 0) {
|
||||
return Promise.reject(new Error(`Invalid Windows PTY root PID: ${rootPid}`))
|
||||
}
|
||||
|
||||
const systemRoot = (deps.env ?? process.env).SystemRoot
|
||||
const taskkillPath = systemRoot
|
||||
? pathWin32.join(systemRoot, 'System32', 'taskkill.exe')
|
||||
: 'taskkill'
|
||||
const spawnProcess: WindowsTreeKillSpawner = deps.spawn ?? spawn
|
||||
|
||||
return new Promise((resolve, reject) => {
|
||||
let settled = false
|
||||
let child: WindowsTreeKillProcess | undefined
|
||||
const finish = (error?: Error): void => {
|
||||
if (settled) {
|
||||
return
|
||||
}
|
||||
settled = true
|
||||
clearTimeout(timer)
|
||||
if (error) {
|
||||
reject(error)
|
||||
} else {
|
||||
resolve()
|
||||
}
|
||||
}
|
||||
const timer = setTimeout(() => {
|
||||
try {
|
||||
child?.kill()
|
||||
} catch {
|
||||
/* taskkill may have exited between the deadline and cancellation */
|
||||
}
|
||||
finish(new Error(`taskkill timed out for Windows PTY root ${rootPid}`))
|
||||
}, deps.timeoutMs ?? WINDOWS_DESCENDANT_KILL_TIMEOUT_MS)
|
||||
timer.unref?.()
|
||||
|
||||
try {
|
||||
child = spawnProcess(taskkillPath, ['/PID', String(rootPid), '/T', '/F'], {
|
||||
shell: false,
|
||||
stdio: 'ignore',
|
||||
windowsHide: true
|
||||
})
|
||||
child.once('error', (error) =>
|
||||
finish(new Error(`taskkill failed for Windows PTY root ${rootPid}: ${error.message}`))
|
||||
)
|
||||
child.once('exit', (code) =>
|
||||
finish(
|
||||
code === 0
|
||||
? undefined
|
||||
: new Error(
|
||||
`taskkill exited ${code ?? 'without a code'} for Windows PTY root ${rootPid}`
|
||||
)
|
||||
)
|
||||
)
|
||||
child.unref()
|
||||
} catch {
|
||||
finish(new Error(`Could not start taskkill for Windows PTY root ${rootPid}`))
|
||||
}
|
||||
})
|
||||
}
|
||||
@@ -0,0 +1 @@
|
||||
export const WINDOWS_PTY_JOB_DRAIN_TIMEOUT_MS = 8_000
|
||||
@@ -388,6 +388,12 @@ test('Windows worktree deletion releases a reparented agent descendant file lock
|
||||
created = await createIsolatedWorktree(orcaPage)
|
||||
const lockedPath = join(created.path, 'README.md')
|
||||
expect(existsSync(lockedPath), 'fixture worktree must contain tracked README.md').toBe(true)
|
||||
const initialGitRegistration = inspectGitWorktreeRegistration(created.repoPath, created.path)
|
||||
expect(initialGitRegistration.error).toBeUndefined()
|
||||
expect(
|
||||
initialGitRegistration.registered,
|
||||
'fixture worktree must be Git-registered before destructive removal'
|
||||
).toBe(true)
|
||||
|
||||
const command = [
|
||||
`[IO.File]::WriteAllText(${quotePowerShellArg(rootPidPath)}, [string]$PID);`,
|
||||
@@ -464,7 +470,8 @@ test('Windows worktree deletion releases a reparented agent descendant file lock
|
||||
rootAlive: isProcessAlive(rootPid),
|
||||
agentAlive: isProcessAlive(agentPid),
|
||||
launcherAlive: isProcessAlive(launcherPid),
|
||||
lockHolderAlive: isProcessAlive(lockHolderPid)
|
||||
lockHolderAlive: isProcessAlive(lockHolderPid),
|
||||
gitWorktreeRegistered: initialGitRegistration.registered
|
||||
}
|
||||
|
||||
removalResult = await removeWorktreeViaStore(orcaPage, created.id)
|
||||
|
||||
Reference in New Issue
Block a user