Browse Source

fix(subagent): keep ACP failure observation cancellable

pku-xht 2 months ago
parent
commit
2a060adfa8

+ 2 - 2
packages/subagent/subagent-acp/README.i18n.yaml

@@ -2,5 +2,5 @@
 # side as of the last confirmed-consistent state. Both languages carry equal authority;
 # after editing either side, bring the other along and re-record with:
 #   pnpm run verify-translation-pairing --write packages/subagent/subagent-acp/README.md
-README.md: 6366a224b8f56f0b86466d6946f75ab45fee3997
-README.zh.md: 490c17dd883224bcfe129bd3b29eea973a4e69c7
+README.md: 00f084c8252c001d98d04c8ccc1dff5f14976683
+README.zh.md: 3b820805d23b39757376edc14a3d75860f2e8eae

+ 1 - 1
packages/subagent/subagent-acp/README.md

@@ -31,7 +31,7 @@ ACP advertises no start-time capabilities because this process cannot enforce th
 | `permission` | `reject` | Auto-answer permission requests by rejecting or choosing the first `allow_once` or `allow_always` option. |
 | `env` | `{}` | Explicit child environment layered over a credential-scrubbed parent environment. |
 | `disposeEofGraceMs` | `6000` | Positive grace after stdin EOF before platform termination; it cannot exceed [`MAX_TIMER_DELAY_MS`](../../util/timeout/README.md). |
-| `disposeGraceMs` | `3000` | Positive POSIX grace after SIGTERM before SIGKILL (Windows force-terminates directly); it cannot exceed [`MAX_TIMER_DELAY_MS`](../../util/timeout/README.md). |
+| `disposeGraceMs` | `3000` | Positive bound for observing structured process facts after failure and, on POSIX, the SIGTERM-to-SIGKILL grace (Windows force-terminates directly); it cannot exceed [`MAX_TIMER_DELAY_MS`](../../util/timeout/README.md). |
 
 ```yaml
 - id: subagent-acp

+ 1 - 1
packages/subagent/subagent-acp/README.zh.md

@@ -31,7 +31,7 @@ ACP 不声明任何启动时能力,因为当前进程无法强制执行远程
 | `permission` | `reject` | 自动回答权限请求:拒绝,或选择第一个 `allow_once` 或 `allow_always` 选项。 |
 | `env` | `{}` | 显式子进程环境,叠加到已清理凭据的父进程环境之上。 |
 | `disposeEofGraceMs` | `6000` | stdin EOF 之后、平台终止之前的宽限时间须为正值,且不得大于 [`MAX_TIMER_DELAY_MS`](../../util/timeout/README.zh.md)。 |
-| `disposeGraceMs` | `3000` | POSIX 在 SIGTERM 后、SIGKILL 前的宽限时间(Windows 直接强制终止),须为正值且不得大于 [`MAX_TIMER_DELAY_MS`](../../util/timeout/README.zh.md)。 |
+| `disposeGraceMs` | `3000` | 失败后观测结构化进程事实的正数时限;在 POSIX 上也作为 SIGTERM 到 SIGKILL 的宽限时间(Windows 直接强制终止),且不得大于 [`MAX_TIMER_DELAY_MS`](../../util/timeout/README.zh.md)。 |
 
 ```yaml
 - id: subagent-acp

+ 2 - 2
packages/subagent/subagent-acp/src/index.ts

@@ -59,7 +59,7 @@ export interface Config {
    * `MAX_TIMER_DELAY_MS`.
    */
   disposeEofGraceMs?: number
-  /** Termination-escalation grace (ms); must not exceed `MAX_TIMER_DELAY_MS`. */
+  /** Failure-observation and termination-escalation grace (ms); must not exceed `MAX_TIMER_DELAY_MS`. */
   disposeGraceMs?: number
 }
 
@@ -74,7 +74,7 @@ export const Config: z<Config> = z.object({
   disposeGraceMs: z.number().default(DEFAULT_DISPOSE_GRACE_MS),
 })
 
-/** A dispose grace must fit the single Node timer that owns its teardown tier. */
+/** A process grace must fit every Node timer that observes or terminates the child. */
 function assertPositiveFinite(name: string, value: number): void {
   if (!Number.isFinite(value) || value <= 0 || value > MAX_TIMER_DELAY_MS) {
     throw new Error(`subagent-acp: ${name} must be a positive finite number no greater than ${MAX_TIMER_DELAY_MS}`)

+ 12 - 8
packages/subagent/subagent-acp/src/run.ts

@@ -61,9 +61,11 @@ export interface AcpRunSpec {
    */
   disposeEofGraceMs: number
   /**
-   * Termination-escalation grace (ms) in {@link SubagentRun.dispose}; POSIX
-   * waits this long after `SIGTERM` before `SIGKILL`, while Windows
-   * force-terminates directly. The plugin fills it from `disposeGraceMs`.
+   * Process-observation and termination-escalation grace (ms). Failure
+   * classification waits at most this long for structured exit facts; POSIX
+   * dispose also waits this long after `SIGTERM` before `SIGKILL`, while
+   * Windows force-terminates directly. The plugin fills it from
+   * `disposeGraceMs`.
    */
   disposeGraceMs: number
   /**
@@ -282,7 +284,6 @@ function startupFailure(
   child: SubprocessHandle,
   outcome: SubprocessOutcome | undefined,
 ): AcpRunFailure {
-  if (error instanceof AcpRunFailure) return error
   if (child.pid <= 0) {
     return new AcpRunFailure({ stage: 'process', category: 'process-start' }, error)
   }
@@ -374,11 +375,12 @@ export async function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpe
   )
   spawnFailed.catch(() => { /* observed by the startup race; never unhandled */ })
 
-  const observeProcessOutcome = async (): Promise<SubprocessOutcome | undefined> => {
+  const observeProcessOutcome = async (signal?: AbortSignal): Promise<SubprocessOutcome | undefined> => {
     if (processOutcome !== undefined || child.pid <= 0) return processOutcome
     try {
+      const timeout = AbortSignal.timeout(Math.ceil(spec.disposeGraceMs))
       const exited = await child.waitForExit(
-        AbortSignal.timeout(Math.ceil(spec.disposeGraceMs)),
+        signal === undefined ? timeout : AbortSignal.any([signal, timeout]),
       )
       if (exited) return await processDone
     } catch {
@@ -495,7 +497,9 @@ export async function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpe
       ? { kind: 'cancelled' } as const
       : {
         kind: 'failed',
-        failure: startupFailure(error, startupStage, child, await observeProcessOutcome()),
+        failure: error instanceof AcpRunFailure
+          ? error
+          : startupFailure(error, startupStage, child, await observeProcessOutcome()),
       } as const
     if (startup.kind === 'cancelled') {
       // Local cancellation owns the startup outcome; only cleanup failure is
@@ -550,7 +554,7 @@ export async function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpe
         }
       } catch (error: unknown) {
         if (!flags.cancelled) {
-          const outcome = await observeProcessOutcome()
+          const outcome = await observeProcessOutcome(request.signal)
           const facts = outcome === undefined
             ? { stage: 'prompt', category: 'transport' } as const
             : { stage: 'process', category: 'process-exit', outcome } as const

+ 43 - 1
packages/subagent/subagent-acp/tests/subagent-acp.spec.ts

@@ -105,6 +105,22 @@ function rejectFinalExitWaitAfterExit(child: SubprocessHandle, message: string):
   }
 }
 
+function tapBoundedExitWait(child: SubprocessHandle, onWait: () => void): SubprocessHandle {
+  return {
+    pid: child.pid,
+    stdin: child.stdin,
+    stdout: child.stdout,
+    stderr: child.stderr,
+    collected: child.collected,
+    done: child.done,
+    terminate: () => { child.terminate() },
+    waitForExit: (signal?: AbortSignal) => {
+      if (signal !== undefined) onWait()
+      return child.waitForExit(signal)
+    },
+  }
+}
+
 describe('acpStopReason', () => {
   it('maps each ACP stop reason to the harness vocabulary', () => {
     expect(acpStopReason('end_turn')).toBe('completed')
@@ -593,6 +609,7 @@ describe('dsh-subagent-acp', () => {
   it('reaps a child whose session/new response omits the session id', async () => {
     const tmp = mkdtempSync(join(tmpdir(), 'acp-malformed-session-'))
     const flushed = join(tmp, 'flushed')
+    let boundedWaits = 0
     try {
       await expect(startAcpRun(request(), {
         command: process.execPath,
@@ -606,13 +623,14 @@ describe('dsh-subagent-acp', () => {
         },
         disposeEofGraceMs: 1000,
         disposeGraceMs: 100,
-        spawn: spawnSubprocess,
+        spawn: spec => tapBoundedExitWait(spawnSubprocess(spec), () => { boundedWaits += 1 }),
       })).rejects.toThrow(
         `subagent-acp: ${expectedFailure('stage: new-session; category: protocol')}`,
       )
       // Startup rejects only after its private child reaches quiescence. The
       // marker proves rollback closed stdin and allowed the child's EOF flush.
       expect(existsSync(flushed)).toBe(true)
+      expect(boundedWaits).toBe(1)
     } finally {
       rmSync(tmp, { recursive: true, force: true })
     }
@@ -939,6 +957,30 @@ describe('dsh-subagent-acp', () => {
     await run.dispose()
   })
 
+  it('lets local cancellation interrupt prompt-failure process observation', async () => {
+    const controller = new AbortController()
+    const observing = Promise.withResolvers<undefined>()
+    const run = await startAcpRun(request('p', controller.signal), {
+      command: process.execPath,
+      args: [mockServer],
+      cwd: process.cwd(),
+      permission: 'reject',
+      env: { MOCK_CLOSE_PROTOCOL_ON_PROMPT: '1' },
+      disposeEofGraceMs: 100,
+      disposeGraceMs: 5000,
+      spawn: spec => tapBoundedExitWait(spawnSubprocess(spec), () => { observing.resolve(undefined) }),
+    })
+    await observing.promise
+    controller.abort()
+    await expect(Promise.race([
+      run.result,
+      new Promise<never>((_resolve, reject) => {
+        setTimeout(() => { reject(new Error('cancellation waited for process observation')) }, 500)
+      }),
+    ])).resolves.toEqual({ output: [], stopReason: 'aborted' })
+    await run.dispose()
+  })
+
   it('preserves partial output and structured process facts when the child exits', async () => {
     const ctx = await setup({ MOCK_TEXT: 'partial answer', MOCK_CRASH_AFTER_CHUNK: '1' })
     const run = await ctx.subagents.start('acp', request())