From a93968bf9361a3240228d8e35bd43bfa1f7f7799 Mon Sep 17 00:00:00 2001 From: pku-xht Date: Tue, 4 Aug 2026 22:36:31 +0800 Subject: [PATCH] fix(subagent-codex): await managed tree exit --- docs/config-catalog.md | 4 +- .../subagent/subagent-codex/README.i18n.yaml | 4 +- packages/subagent/subagent-codex/README.md | 2 +- packages/subagent/subagent-codex/README.zh.md | 2 +- packages/subagent/subagent-codex/src/run.ts | 59 +------------- .../tests/subagent-codex.spec.ts | 81 +++++-------------- packages/subagent/tool-subagent/src/index.ts | 4 +- 7 files changed, 32 insertions(+), 124 deletions(-) diff --git a/docs/config-catalog.md b/docs/config-catalog.md index d063d793e6..008d4ee2dd 100644 --- a/docs/config-catalog.md +++ b/docs/config-catalog.md @@ -1949,8 +1949,8 @@ export interface Config { * requires the provider's `depthLimit` capability (mount fails loud * otherwise). The provider checks the calling agent's current depth at every * start; the tool remains model-visible so runtime policy owns rejection. - * `'provider-managed'` is for an out-of-process provider (ACP) whose - * recursion budget belongs to the child harness's own deployment. + * `'provider-managed'` is for an out-of-process provider whose recursion + * budget belongs to the child runtime or its own deployment. */ maxDepth?: number | 'provider-managed' } diff --git a/packages/subagent/subagent-codex/README.i18n.yaml b/packages/subagent/subagent-codex/README.i18n.yaml index 3e8e805c88..c3d4da77bf 100644 --- a/packages/subagent/subagent-codex/README.i18n.yaml +++ b/packages/subagent/subagent-codex/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-codex/README.md -README.md: ce1c66427b562c08af06320f012f28b9e125ac45 -README.zh.md: bef47586db77c70bec741629d8579ba0e2efba1e +README.md: d7293a0ef37e4ec0f0cf983c254f9e22f830fcd8 +README.zh.md: 110953312162e146f01ef037a40d2f70b136850c diff --git a/packages/subagent/subagent-codex/README.md b/packages/subagent/subagent-codex/README.md index ce1c66427b..d7293a0ef3 100644 --- a/packages/subagent/subagent-codex/README.md +++ b/packages/subagent/subagent-codex/README.md @@ -23,7 +23,7 @@ The provider advertises no optional start-time capabilities and reports `inherit | Key | Default | Meaning | |---|---|---| | `env` | `{}` | Explicit child environment layered over the subprocess seam's credential-scrubbed parent environment. | -| `disposeGraceMs` | `3000` | Positive finite process-tree termination grace in milliseconds; the final exit proof is bounded at twice this value. | +| `disposeGraceMs` | `3000` | Positive finite grace in milliseconds between the shared process-tree owner's termination tiers; disposal then waits for whole-tree exit. | Production resolves `codex` from `PATH` and uses the host's native Codex configuration and authentication. The plugin does not install Codex, select a model, create `CODEX_HOME`, log in, or probe a version. Credential-shaped ambient variables are removed by the subprocess seam, so an API key intended for the child must be supplied explicitly in `env`; ordinary ambient values such as `PATH` and `HOME` remain available unless overridden. diff --git a/packages/subagent/subagent-codex/README.zh.md b/packages/subagent/subagent-codex/README.zh.md index bef47586db..1109533121 100644 --- a/packages/subagent/subagent-codex/README.zh.md +++ b/packages/subagent/subagent-codex/README.zh.md @@ -23,7 +23,7 @@ | 配置键 | 默认值 | 含义 | |---|---|---| | `env` | `{}` | 显式指定的子进程环境，叠加在由子进程 seam 清除凭证后的父环境之上。 | -| `disposeGraceMs` | `3000` | 进程树终止宽限期，须为正有限值，单位为毫秒；最终退出确认的等待时间上限为该值的两倍。 | +| `disposeGraceMs` | `3000` | 共享进程树责任方各终止层级之间的宽限期，单位为毫秒且须为正有限值；随后资源释放会等待整棵进程树退出。 | 生产环境会从 `PATH` 中解析 `codex`，并使用宿主机原生的 Codex 配置与身份验证。本插件不安装 Codex、不选择模型、不创建 `CODEX_HOME`、不执行登录，也不探测版本。子进程 seam 会移除具有凭证特征的环境变量，因此供子进程使用的 API 密钥必须在 `env` 中显式提供；除非被覆盖，`PATH` 和 `HOME` 等普通环境变量值仍然可用。 diff --git a/packages/subagent/subagent-codex/src/run.ts b/packages/subagent/subagent-codex/src/run.ts index 811f7c8f98..9f52e18f12 100644 --- a/packages/subagent/subagent-codex/src/run.ts +++ b/packages/subagent/subagent-codex/src/run.ts @@ -24,54 +24,13 @@ import { CodexAppServerWire } from './wire.ts' /** Default POSIX grace between subprocess termination tiers. */ export const DEFAULT_DISPOSE_GRACE_MS = 3_000 -/** Largest delay Node schedules without collapsing it to one millisecond. */ -const MAX_TIMER_DELAY_MS = 2_147_483_647n - -/** - * Bound final exit observation at twice a positive finite grace without - * narrowing the public config to Node's single-timer integer range. - */ -function doubledGraceWindow(graceMs: number): { - readonly signal: AbortSignal - readonly cancel: () => void -} { - const whole = Math.floor(graceMs) - let remaining = BigInt(whole) * 2n - + BigInt(Math.ceil((graceMs - whole) * 2)) - const controller = new AbortController() - let timer: ReturnType | undefined - const arm = (): void => { - const chunk = remaining > MAX_TIMER_DELAY_MS - ? MAX_TIMER_DELAY_MS - : remaining - remaining -= chunk - timer = setTimeout(() => { - timer = undefined - if (remaining === 0n) { - controller.abort() - } else { - arm() - } - }, Number(chunk)) - } - arm() - return { - signal: controller.signal, - cancel: () => { - if (timer === undefined) return - clearTimeout(timer) - timer = undefined - }, - } -} - /** Fully resolved inputs for one Codex app-server run. */ export interface CodexRunSpec { /** Parent Session workspace, also supplied to `thread/start`. */ readonly cwd: string /** Explicit deployment/test environment layered after the shared scrub. */ readonly env: Record - /** Subprocess termination grace and final tree-exit bound. */ + /** Subprocess termination grace passed to the shared process-tree owner. */ readonly disposeGraceMs: number /** Shared subprocess service spawn operation. */ readonly spawn: (spec: SubprocessSpawnSpec) => SubprocessHandle @@ -111,12 +70,10 @@ export function textTask(prompt: readonly ContentBlock[]): string[] { * subprocess owner to prove it is gone. * @param wire - private app-server protocol connection. * @param child - shared-service handle that owns the process tree. - * @param graceMs - termination grace used to bound final exit observation. */ export async function disposeCodexChild( wire: CodexAppServerWire, child: SubprocessHandle, - graceMs: number, ): Promise { wire.close() if (child.pid <= 0) { @@ -129,14 +86,7 @@ export async function disposeCodexChild( // A concurrently closed stdin does not change tree ownership below. } child.terminate() - const exitWindow = doubledGraceWindow(graceMs) - try { - if (!(await child.waitForExit(exitWindow.signal))) { - throw new Error('subagent-codex: app-server process tree did not exit within its dispose window') - } - } finally { - exitWindow.cancel() - } + await child.waitForExit() await child.done } @@ -167,8 +117,7 @@ export async function startCodexRun( child.stdout as NonNullable, child.stdin as NonNullable, ) - const disposeProcess = (): Promise => - disposeCodexChild(wire, child, spec.disposeGraceMs) + const disposeProcess = (): Promise => disposeCodexChild(wire, child) const processFailure: Promise = child.done.then( outcome => Promise.reject(new Error( @@ -205,7 +154,7 @@ export async function startCodexRun( ) } if (runAbort.signal.aborted) { - throw new Error('subagent-codex: request was aborted before app-server startup') + throw new Error('subagent-codex: request was aborted before run publication') } throw thrown(error) } diff --git a/packages/subagent/subagent-codex/tests/subagent-codex.spec.ts b/packages/subagent/subagent-codex/tests/subagent-codex.spec.ts index 8e6c7ebd51..181ddb919e 100644 --- a/packages/subagent/subagent-codex/tests/subagent-codex.spec.ts +++ b/packages/subagent/subagent-codex/tests/subagent-codex.spec.ts @@ -91,7 +91,6 @@ class ProtocolPeer { interface FakeChildOptions { readonly pid?: number readonly exitOnTerminate?: boolean - readonly waitForExitResult?: boolean readonly doneError?: Error } @@ -134,9 +133,6 @@ function fakeChild(options: FakeChildOptions = {}): FakeChild { if (options.exitOnTerminate !== false) settle() }) const waitForExit = vi.fn(async (signal?: AbortSignal) => { - if (options.waitForExitResult !== undefined) { - return options.waitForExitResult - } if (exited) return true if (signal === undefined) { await done.catch(() => {}) @@ -943,7 +939,7 @@ describe('run lifecycle and quiescence', () => { const threadStart = await child.peer.nextMethod('thread/start') child.peer.respond(threadStart, { thread: { id: 'thread-1', ephemeral: true } }) controller.abort('startup race') - await expect(starting).rejects.toThrow('aborted before app-server startup') + await expect(starting).rejects.toThrow('aborted before run publication') expect(child.terminate).toHaveBeenCalledTimes(1) }) @@ -964,19 +960,6 @@ describe('run lifecycle and quiescence', () => { expect(child.terminate).toHaveBeenCalledTimes(1) }) - it('reports both startup and rollback failures', async () => { - const child = fakeChild({ waitForExitResult: false, exitOnTerminate: false }) - const starting = startCodexRun( - request(), - runSpec(child, { disposeGraceMs: 1 }), - ) - const initialize = await child.peer.nextMethod('initialize') - child.peer.respond(initialize, { userAgent: '' }) - await expect(starting).rejects.toThrow( - 'startup failed and app-server cleanup also failed', - ) - }) - it('keeps overlapping runs isolated', async () => { const first = fakeChild() const second = fakeChild() @@ -1047,41 +1030,25 @@ describe('disposeCodexChild', () => { const child = fakeChild() const wire = new CodexAppServerWire(child.handle.stdout!, child.handle.stdin!) const end = vi.spyOn(child.toChild, 'end') - await disposeCodexChild(wire, child.handle, 100) + await disposeCodexChild(wire, child.handle) expect(end).toHaveBeenCalled() expect(child.terminate).toHaveBeenCalledTimes(1) expect(child.waitForExit).toHaveBeenCalledTimes(1) + expect(child.waitForExit).toHaveBeenCalledWith() }) - it('accepts fractional and larger-than-Node grace windows', async () => { - for (const graceMs of [0.25, Number.MAX_VALUE]) { - const child = fakeChild() - const wire = new CodexAppServerWire(child.handle.stdout!, child.handle.stdin!) - await expect(disposeCodexChild(wire, child.handle, graceMs)) - .resolves.toBeUndefined() - const signal = vi.mocked(child.waitForExit).mock.calls[0]?.[0] - expect(signal?.aborted).toBe(false) - } - }) - - it('chains a doubled grace window beyond one Node timer segment', async () => { - vi.useFakeTimers() - try { - const child = fakeChild({ exitOnTerminate: false }) - const wire = new CodexAppServerWire(child.handle.stdout!, child.handle.stdin!) - const disposal = disposeCodexChild( - wire, - child.handle, - 1_073_741_823.75, - ) - const rejected = expect(disposal) - .rejects.toThrow('did not exit within its dispose window') - await vi.advanceTimersByTimeAsync(2_147_483_647) - await vi.advanceTimersByTimeAsync(1) - await rejected - } finally { - vi.useRealTimers() - } + it('does not finish disposal before the managed tree exits', async () => { + const child = fakeChild({ exitOnTerminate: false }) + const wire = new CodexAppServerWire(child.handle.stdout!, child.handle.stdin!) + let disposed = false + const disposal = disposeCodexChild(wire, child.handle).then(() => { + disposed = true + }) + await new Promise((resolve) => { setImmediate(resolve) }) + expect(disposed).toBe(false) + child.settle() + await disposal + expect(disposed).toBe(true) }) it('contains a concurrently closed stdin error', async () => { @@ -1090,7 +1057,7 @@ describe('disposeCodexChild', () => { vi.spyOn(child.toChild, 'end').mockImplementation(() => { throw new Error('already closed') }) - await expect(disposeCodexChild(wire, child.handle, 100)) + await expect(disposeCodexChild(wire, child.handle)) .resolves.toBeUndefined() }) @@ -1100,34 +1067,26 @@ describe('disposeCodexChild', () => { doneError: new Error('spawn failed'), }) const wire = new CodexAppServerWire(child.handle.stdout!, child.handle.stdin!) - await expect(disposeCodexChild(wire, child.handle, 100)) + await expect(disposeCodexChild(wire, child.handle)) .resolves.toBeUndefined() expect(child.terminate).not.toHaveBeenCalled() expect(child.waitForExit).not.toHaveBeenCalled() }) - it('fails when the tree misses the release window or done rejects', async () => { - { - const child = fakeChild({ - exitOnTerminate: false, - }) - const wire = new CodexAppServerWire(child.handle.stdout!, child.handle.stdin!) - await expect(disposeCodexChild(wire, child.handle, 1)) - .rejects.toThrow('did not exit within its dispose window') - } + it('reports direct-child observer failure and accepts absent stdin', async () => { { const child = fakeChild({ doneError: new Error('close observer failed'), }) const wire = new CodexAppServerWire(child.handle.stdout!, child.handle.stdin!) - await expect(disposeCodexChild(wire, child.handle, 1)) + await expect(disposeCodexChild(wire, child.handle)) .rejects.toThrow('close observer failed') } { const child = fakeChild() const handle = { ...child.handle, stdin: undefined } const wire = new CodexAppServerWire(child.handle.stdout!, child.handle.stdin!) - await expect(disposeCodexChild(wire, handle, 1)).resolves.toBeUndefined() + await expect(disposeCodexChild(wire, handle)).resolves.toBeUndefined() } }) }) diff --git a/packages/subagent/tool-subagent/src/index.ts b/packages/subagent/tool-subagent/src/index.ts index f95c5ad09d..67894c32cb 100644 --- a/packages/subagent/tool-subagent/src/index.ts +++ b/packages/subagent/tool-subagent/src/index.ts @@ -67,8 +67,8 @@ export interface Config { * requires the provider's `depthLimit` capability (mount fails loud * otherwise). The provider checks the calling agent's current depth at every * start; the tool remains model-visible so runtime policy owns rejection. - * `'provider-managed'` is for an out-of-process provider (ACP) whose - * recursion budget belongs to the child harness's own deployment. + * `'provider-managed'` is for an out-of-process provider whose recursion + * budget belongs to the child runtime or its own deployment. */ maxDepth?: number | 'provider-managed' }