fix(subprocess): honor Windows termination semantics
This commit is contained in:
8 files changed
+48
-38
No files matched your search
@@ -853,7 +853,7 @@ export interface Config {
|
||||
* before the parent escalates to a signal.
|
||||
*/
|
||||
disposeEofGraceMs?: number
|
||||
/** Grace period (ms) between `SIGTERM` and the `SIGKILL` escalation on dispose. */
|
||||
/** POSIX grace period (ms) between `SIGTERM` and `SIGKILL`; unused on Windows. */
|
||||
disposeGraceMs?: number
|
||||
}
|
||||
|
||||
|
||||
@@ -8,7 +8,7 @@ The ACP provider runs each subagent in a fresh subprocess and drives it as an Ag
|
||||
|
||||
After publication, the provider sends the prompt and collects streamed `agent_message_chunk` text into `SubagentResult.output`. A prompt/transport failure resolves with `stopReason: 'error'`, or `aborted` when the required request signal or disposal requested cancellation.
|
||||
|
||||
`dispose()` is idempotent. It removes the signal listener, requests ACP cancellation when possible, closes stdin, waits `disposeEofGraceMs`, escalates to SIGTERM, waits `disposeGraceMs`, and finally uses SIGKILL if necessary. Every run uses a fresh process; process pooling is not implemented.
|
||||
`dispose()` is idempotent. It removes the signal listener, requests ACP cancellation when possible, closes stdin, and waits `disposeEofGraceMs`. POSIX then escalates through SIGTERM and `disposeGraceMs` before SIGKILL; Windows force-terminates directly because Node maps both signals to `TerminateProcess`. Disposal resolves only after child exit. Every run uses a fresh process; process pooling is not implemented.
|
||||
|
||||
## Capabilities and context
|
||||
|
||||
@@ -24,8 +24,8 @@ ACP advertises no start-time capabilities because this process cannot enforce th
|
||||
| `cwd` | process cwd | Child process and ACP session working directory. |
|
||||
| `permission` | `reject` | Auto-answer permission requests by rejecting or choosing the first allow-shaped option. |
|
||||
| `env` | `{}` | Explicit child environment layered over a credential-scrubbed parent environment. |
|
||||
| `disposeEofGraceMs` | `6000` | Grace after stdin EOF before SIGTERM. |
|
||||
| `disposeGraceMs` | `3000` | Grace after SIGTERM before SIGKILL. |
|
||||
| `disposeEofGraceMs` | `6000` | Grace after stdin EOF before platform termination. |
|
||||
| `disposeGraceMs` | `3000` | POSIX grace after SIGTERM before SIGKILL; unused on Windows. |
|
||||
|
||||
```yaml
|
||||
- id: subagent-acp
|
||||
|
||||
@@ -46,7 +46,7 @@ export interface Config {
|
||||
* before the parent escalates to a signal.
|
||||
*/
|
||||
disposeEofGraceMs?: number
|
||||
/** Grace period (ms) between `SIGTERM` and the `SIGKILL` escalation on dispose. */
|
||||
/** POSIX grace period (ms) between `SIGTERM` and `SIGKILL`; unused on Windows. */
|
||||
disposeGraceMs?: number
|
||||
}
|
||||
|
||||
|
||||
@@ -56,9 +56,9 @@ export interface AcpRunSpec {
|
||||
*/
|
||||
disposeEofGraceMs: number
|
||||
/**
|
||||
* Grace period (ms) between `SIGTERM` and the `SIGKILL` escalation in
|
||||
* {@link SubagentRun.dispose}. The plugin fills this from its
|
||||
* `disposeGraceMs` config.
|
||||
* POSIX grace period (ms) between `SIGTERM` and `SIGKILL` in
|
||||
* {@link SubagentRun.dispose}; unused on Windows. The plugin fills this from
|
||||
* its `disposeGraceMs` config.
|
||||
*/
|
||||
disposeGraceMs: number
|
||||
/**
|
||||
@@ -75,7 +75,7 @@ export interface AcpRunSpec {
|
||||
/** EOF grace for child flush and nested-process teardown; wider than the signal grace below. */
|
||||
export const DEFAULT_DISPOSE_EOF_GRACE_MS = 6_000
|
||||
|
||||
/** Default grace between SIGTERM and SIGKILL on dispose (the `disposeGraceMs` config; mirrors the bash executor). */
|
||||
/** Default POSIX grace between SIGTERM and SIGKILL on dispose (the `disposeGraceMs` config). */
|
||||
export const DEFAULT_DISPOSE_GRACE_MS = 3_000
|
||||
|
||||
/**
|
||||
@@ -290,9 +290,9 @@ export async function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpe
|
||||
if (disposal !== undefined) return disposal
|
||||
request.signal.removeEventListener('abort', onAbort)
|
||||
requestCancel()
|
||||
// The shared EOF → TERM → KILL ladder awaits exit. ACP normally quiesces
|
||||
// from stdin EOF, including the final flush, so this backend uses a wider
|
||||
// EOF grace before signals escalate.
|
||||
// The shared platform-aware ladder awaits exit. ACP normally quiesces from
|
||||
// stdin EOF, including the final flush, so this backend uses a wider EOF
|
||||
// grace before process termination escalates.
|
||||
disposal = disposeProcess()
|
||||
return disposal
|
||||
},
|
||||
|
||||
@@ -263,13 +263,9 @@ describe('dsh-subagent-acp', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it.skipIf(process.platform === 'win32')('escalates to SIGTERM for a child that ignores EOF but is not SIGTERM-trapping', async () => {
|
||||
// A child that keeps its loop alive past stdin EOF (so the graceful window
|
||||
// times out) but exits cooperatively on SIGTERM must die on the SIGTERM tier
|
||||
// — dispose returns there, never reaching the SIGKILL tier. The child touches
|
||||
// a SIGTERM marker from its signal handler: SIGKILL is uncatchable, so if
|
||||
// dispose had skipped the middle rung (EOF→SIGKILL) the handler would never
|
||||
// run and the marker would be absent — making this a GENUINE middle-tier guard.
|
||||
it('terminates a child that ignores EOF using the host platform semantics', async () => {
|
||||
// POSIX uses the catchable SIGTERM tier and records the marker. Windows has
|
||||
// no distinct graceful signal, so disposal skips directly to forced exit.
|
||||
const tmp = mkdtempSync(join(tmpdir(), 'acp-ignore-eof-'))
|
||||
const ready = join(tmp, 'ready')
|
||||
const sigterm = join(tmp, 'sigterm')
|
||||
@@ -283,7 +279,7 @@ describe('dsh-subagent-acp', () => {
|
||||
MOCK_HANG: '1', MOCK_IGNORE_EOF: '1', MOCK_TEXT: 'x',
|
||||
MOCK_READY_FILE: ready, MOCK_SIGTERM_FILE: sigterm,
|
||||
},
|
||||
// Tiny EOF grace so the ignored-EOF window elapses fast, then SIGTERM.
|
||||
// Tiny EOF grace so the ignored-EOF window elapses quickly.
|
||||
disposeEofGraceMs: 150,
|
||||
disposeGraceMs: 2000,
|
||||
}
|
||||
@@ -294,9 +290,7 @@ describe('dsh-subagent-acp', () => {
|
||||
run.dispose(),
|
||||
new Promise((_r, reject) => { setTimeout(() => { reject(new Error('dispose did not return')) }, 5000) }),
|
||||
])).resolves.toBeUndefined()
|
||||
// The child caught SIGTERM and exited — proof the middle rung fired (not a
|
||||
// jump straight to the uncatchable SIGKILL).
|
||||
expect(existsSync(sigterm)).toBe(true)
|
||||
expect(existsSync(sigterm)).toBe(process.platform !== 'win32')
|
||||
} finally {
|
||||
rmSync(tmp, { recursive: true, force: true })
|
||||
}
|
||||
|
||||
@@ -20,13 +20,13 @@ Exit waits over a `ChildProcess`: resolve once the child exits by any code or si
|
||||
|
||||
### `disposeChildProcess(child, graces)`
|
||||
|
||||
The three-tier dispose ladder. Resolves only once the child has ACTUALLY exited — quiescence reached, not merely requested (see [defensive patterns](../../../docs/defensive-patterns.md)):
|
||||
The platform-aware dispose ladder resolves only once the child has ACTUALLY exited — quiescence reached, not merely requested (see [defensive patterns](../../../docs/defensive-patterns.md)):
|
||||
|
||||
1. stdin EOF (when stdin is piped), then wait `graces.disposeEofGraceMs` — a cooperative child quiesces on its own, its flushes and nested-subprocess teardown intact;
|
||||
2. `SIGTERM`, then wait `graces.disposeGraceMs`;
|
||||
3. `SIGKILL`, then await the now-certain exit — a child that ignores EOF and traps `SIGTERM` cannot wedge dispose forever.
|
||||
2. on POSIX, `SIGTERM`, then wait `graces.disposeGraceMs`;
|
||||
3. force termination and await exit — `SIGKILL` on POSIX and Node's `TerminateProcess` mapping on Windows.
|
||||
|
||||
The two graces (`DisposeLadderGraces`) come from the consuming plugin's `disposeEofGraceMs`/`disposeGraceMs` Config fields; the EOF window is deliberately a separate — usually wider — grace than the signal tier, since a cooperative child's EOF teardown may itself await a signal-trapping grandchild plus a final flush.
|
||||
The two graces (`DisposeLadderGraces`) come from the consuming plugin's `disposeEofGraceMs`/`disposeGraceMs` Config fields; `disposeGraceMs` is unused on Windows because Node maps `SIGTERM` and `SIGKILL` to the same forced termination. The EOF window is deliberately separate and usually wider, since cooperative teardown may await a signal-trapping grandchild plus a final flush.
|
||||
|
||||
### `createIsolatedConfigDir(prefix, pinnedPath?)`
|
||||
|
||||
@@ -37,7 +37,7 @@ A per-run isolated config directory for an external CLI child (the target of `CL
|
||||
|
||||
## Testing
|
||||
|
||||
`tests/subagent-subprocess.spec.ts`: the env scrub and config-dir helpers run against the real process env and real filesystem (the rm-failure path injects its rejection at the fs boundary — a real recursive-rm failure is not portably provokable, and root ignores permission bits); the exit waits and the dispose ladder run against a scriptable fake child, driving each escalation tier deterministically. The [ACP backend suite](../subagent-acp/README.md) exercises the same ladder against real subprocesses (EOF-cooperative, EOF-ignoring, and SIGTERM-trapping children) end to end.
|
||||
`tests/subagent-subprocess.spec.ts`: the env scrub and config-dir helpers run against the real process env and real filesystem (the rm-failure path injects its rejection at the fs boundary — a real recursive-rm failure is not portably provokable, and root ignores permission bits); the exit waits and platform termination paths run against a scriptable fake child. The [ACP backend suite](../subagent-acp/README.md) exercises them against real subprocesses end to end.
|
||||
|
||||
## Model Experience
|
||||
|
||||
|
||||
@@ -98,33 +98,42 @@ export interface DisposeLadderGraces {
|
||||
/**
|
||||
* Tier-1 window (ms): after stdin EOF, how long the child gets to quiesce
|
||||
* ON ITS OWN — flush durable state, tear down its own nested subprocesses —
|
||||
* before the parent escalates to `SIGTERM`. A separate (usually WIDER)
|
||||
* before the parent escalates to platform termination. A separate (usually WIDER)
|
||||
* grace than {@link DisposeLadderGraces.disposeGraceMs}: a cooperative
|
||||
* child's EOF-driven teardown may itself be waiting on a signal-trapping
|
||||
* grandchild plus a final flush, needing more than one signal-grace of
|
||||
* headroom.
|
||||
*/
|
||||
disposeEofGraceMs: number
|
||||
/** Tier-2 window (ms): between `SIGTERM` and the `SIGKILL` escalation. */
|
||||
/** POSIX tier-2 window (ms): between `SIGTERM` and the `SIGKILL` escalation. */
|
||||
disposeGraceMs: number
|
||||
}
|
||||
|
||||
/**
|
||||
* Tear a child process down to quiescence, resolving only after exit: close stdin and allow
|
||||
* cooperative flush, then send `SIGTERM`, then `SIGKILL` and await the forced exit.
|
||||
* cooperative flush, then use the host's graceful and forced termination semantics. POSIX
|
||||
* sends `SIGTERM` before `SIGKILL`; Windows skips directly to forced termination because Node
|
||||
* maps both signals to `TerminateProcess`.
|
||||
*
|
||||
* @param child - the child process to tear down.
|
||||
* @param graces - the two grace periods, from the consuming plugin's Config.
|
||||
* @param platform - the host platform, injectable for unit coverage.
|
||||
*/
|
||||
export async function disposeChildProcess(child: ChildProcess, graces: DisposeLadderGraces): Promise<void> {
|
||||
export async function disposeChildProcess(
|
||||
child: ChildProcess,
|
||||
graces: DisposeLadderGraces,
|
||||
platform: NodeJS.Platform = process.platform,
|
||||
): Promise<void> {
|
||||
// Already gone: nothing to reap.
|
||||
if (child.exitCode !== null || child.signalCode !== null) return
|
||||
// 1. Close stdin and allow cooperative teardown and durable-state flush.
|
||||
child.stdin?.end()
|
||||
if (await exitsWithin(child, graces.disposeEofGraceMs)) return
|
||||
// 2. SIGTERM, escalating if the child still does not exit within the grace.
|
||||
child.kill('SIGTERM')
|
||||
if (await exitsWithin(child, graces.disposeGraceMs)) return
|
||||
// 2. POSIX gets a catchable graceful signal; Windows signals all force-terminate.
|
||||
if (platform !== 'win32') {
|
||||
child.kill('SIGTERM')
|
||||
if (await exitsWithin(child, graces.disposeGraceMs)) return
|
||||
}
|
||||
// 3. Force-kill and await the (now-certain) exit.
|
||||
child.kill('SIGKILL')
|
||||
await waitForExit(child)
|
||||
|
||||
@@ -229,7 +229,7 @@ describe('disposeChildProcess', () => {
|
||||
|
||||
it('tier 2: a child that ignores EOF but honors SIGTERM dies on the middle rung', async () => {
|
||||
const fake = new FakeChild({ diesOn: 'SIGTERM', delayMs: 5 })
|
||||
await disposeChildProcess(asChild(fake), { disposeEofGraceMs: 20, disposeGraceMs: 1000 })
|
||||
await disposeChildProcess(asChild(fake), { disposeEofGraceMs: 20, disposeGraceMs: 1000 }, 'linux')
|
||||
expect(fake.stdinEnded).toBe(true)
|
||||
expect(fake.kills).toEqual(['SIGTERM'])
|
||||
expect(fake.signalCode).toBe('SIGTERM')
|
||||
@@ -237,7 +237,7 @@ describe('disposeChildProcess', () => {
|
||||
|
||||
it('tier 3: a SIGTERM-trapping child is SIGKILLed, and dispose resolves only after the exit', async () => {
|
||||
const fake = new FakeChild({ delayMs: 5 }) // only SIGKILL fells it
|
||||
await disposeChildProcess(asChild(fake), { disposeEofGraceMs: 20, disposeGraceMs: 20 })
|
||||
await disposeChildProcess(asChild(fake), { disposeEofGraceMs: 20, disposeGraceMs: 20 }, 'linux')
|
||||
expect(fake.kills).toEqual(['SIGTERM', 'SIGKILL'])
|
||||
// Quiescence, not a request: at resolution the child has ACTUALLY exited
|
||||
// (the exit event landed, despite the scripted post-SIGKILL delay).
|
||||
@@ -246,9 +246,16 @@ describe('disposeChildProcess', () => {
|
||||
|
||||
it('walks the ladder for a child spawned without a stdin pipe', async () => {
|
||||
const fake = new FakeChild({ stdin: false, diesOn: 'SIGTERM', delayMs: 5 })
|
||||
await disposeChildProcess(asChild(fake), { disposeEofGraceMs: 20, disposeGraceMs: 1000 })
|
||||
await disposeChildProcess(asChild(fake), { disposeEofGraceMs: 20, disposeGraceMs: 1000 }, 'linux')
|
||||
expect(fake.kills).toEqual(['SIGTERM'])
|
||||
})
|
||||
|
||||
it('skips the redundant SIGTERM tier on Windows and awaits forced exit', async () => {
|
||||
const fake = new FakeChild({ diesOn: 'SIGTERM', delayMs: 5 })
|
||||
await disposeChildProcess(asChild(fake), { disposeEofGraceMs: 20, disposeGraceMs: 1000 }, 'win32')
|
||||
expect(fake.kills).toEqual(['SIGKILL'])
|
||||
expect(fake.signalCode).toBe('SIGKILL')
|
||||
})
|
||||
})
|
||||
|
||||
describe('createIsolatedConfigDir', () => {
|
||||
|
||||
Reference in New Issue
Block a user