From 1187d94d9463a3d9fc76614475cc59244d45f1fd Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:58:16 -0700 Subject: [PATCH] fix(native-chat): require a proven exit before handing off the thread (#15162) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Native-to-TUI handoff called closeSession, but connection close stopped waiting after a fixed forced-exit deadline even when the child never emitted 'exit' and the kill request itself had failed. Handoff then durably recorded `exit-observed`, advanced the fence, and launched a TUI against the same provider thread while the app-server could still be alive and streaming — two live writers on one Codex session. `close()` now reports whether exit was actually observed, and the forward handoff refuses on an unproven one rather than advancing the fence. The gate lives at the forward call site, where the fence decision is made, so a caller cannot bypass it by ignoring the returned proof. Two supporting changes: on POSIX the kill reaped only the direct child, missing the npm-shim grandchild the Windows branch's own comment warns about; and recovered-owner termination sent SIGTERM without escalating. SIGTERM is a request — it now escalates once and requires an independent absence probe before the lease transition proceeds. Verified: 2148 tests across ai-vault/codex/native-chat, node typecheck, oxlint clean. Mutation: stubbing out the forward gate turns the exit-proof test red. --- .../codex/codex-app-server-client.test.ts | 11 +++++++--- .../codex-app-server-connection-types.ts | 3 ++- .../codex/codex-app-server-connection.test.ts | 16 +++++++++++++++ src/main/codex/codex-app-server-connection.ts | 9 +++++---- src/main/codex/codex-app-server-session.ts | 15 ++++++++++++++ .../codex/codex-structured-session-adapter.ts | 9 +++++---- .../codex/codex-structured-session-close.ts | 14 +++++++++---- .../structured-agent-session-adapter.ts | 2 +- ...tructured-agent-session-handoff-forward.ts | 7 ++++++- ...ured-agent-session-handoff-options.test.ts | 17 ++++++++++++++++ .../structured-agent-session-handoff-types.ts | 2 +- .../structured-agent-session-host-handoff.ts | 7 ++++++- src/main/runtime/orca-runtime.ts | 20 ++++++++++++++++++- 13 files changed, 111 insertions(+), 21 deletions(-) diff --git a/src/main/codex/codex-app-server-client.test.ts b/src/main/codex/codex-app-server-client.test.ts index 382f9494dd4..7d1c23ffab4 100644 --- a/src/main/codex/codex-app-server-client.test.ts +++ b/src/main/codex/codex-app-server-client.test.ts @@ -195,16 +195,21 @@ describe('killCodexAppServerProcessTree', () => { expect(child.kill).toHaveBeenCalledWith('SIGKILL') }) - it('kills the direct app-server process on non-Windows hosts', () => { + it('kills the launcher descendants before the direct process on non-Windows hosts', () => { const child = { pid: 1234, kill: vi.fn(() => true) as ChildProcess['kill'] } - const spawnImpl = vi.fn() as unknown as typeof spawn + const descendants = { unref: vi.fn(), on: vi.fn() } + const spawnImpl = vi.fn(() => descendants) as unknown as typeof spawn killCodexAppServerProcessTree(child, { platform: 'linux', spawnImpl }) - expect(spawnImpl).not.toHaveBeenCalled() + expect(spawnImpl).toHaveBeenCalledWith('pkill', ['-KILL', '-P', '1234'], { stdio: 'ignore' }) + // A missing pkill arrives as an async 'error' event; unhandled, it would + // take down the main process. + expect(descendants.on).toHaveBeenCalledWith('error', expect.any(Function)) + expect(descendants.unref).toHaveBeenCalledOnce() expect(child.kill).toHaveBeenCalledWith('SIGKILL') }) }) diff --git a/src/main/codex/codex-app-server-connection-types.ts b/src/main/codex/codex-app-server-connection-types.ts index 6e291740436..0b02b74f6b2 100644 --- a/src/main/codex/codex-app-server-connection-types.ts +++ b/src/main/codex/codex-app-server-connection-types.ts @@ -22,5 +22,6 @@ export type CodexAppServerConnection = { notify: (method: string, params?: Record) => void respond: (id: number | string, result: unknown) => void respondWithError: (id: number | string, code: number, message: string) => void - close: () => Promise + /** Resolves true only after the child emitted `exit`; false is unproven. */ + close: () => Promise } diff --git a/src/main/codex/codex-app-server-connection.test.ts b/src/main/codex/codex-app-server-connection.test.ts index fc83b4edb32..e226e22f518 100644 --- a/src/main/codex/codex-app-server-connection.test.ts +++ b/src/main/codex/codex-app-server-connection.test.ts @@ -307,6 +307,22 @@ describe('openCodexAppServerConnection', () => { expect(child.kill).toHaveBeenCalledWith('SIGKILL') }) + it('reports unproven close when forced termination did not produce an exit event', async () => { + vi.useFakeTimers() + const { child, spawnImpl } = stubChild({ exitOnStdinEnd: false }) + answerInitialize(child) + const connection = await openCodexAppServerConnection( + { command: 'codex', args: ['app-server'] }, + {}, + spawnImpl + ) + + const closing = connection.close() + await vi.advanceTimersByTimeAsync(2_600) + + await expect(closing).resolves.toBe(false) + }) + it('ends the connection rather than buffering an oversized line', async () => { const { child, spawnImpl } = stubChild({ exitOnStdinEnd: false }) answerInitialize(child) diff --git a/src/main/codex/codex-app-server-connection.ts b/src/main/codex/codex-app-server-connection.ts index 3587c04ad39..8058ed95f2e 100644 --- a/src/main/codex/codex-app-server-connection.ts +++ b/src/main/codex/codex-app-server-connection.ts @@ -86,6 +86,7 @@ export async function openCodexAppServerConnection( let stderrTail = '' let nextRequestId = 1 let exited = false + let exitObserved = false let closing = false /** First terminal cause, or null while the transport is still usable. Set once: * a child that dies reaches us through several listeners, and the specific @@ -95,6 +96,7 @@ export async function openCodexAppServerConnection( const exitPromise = new Promise((resolve) => { child.on('exit', () => { exited = true + exitObserved = true resolve() }) }) @@ -127,7 +129,6 @@ export async function openCodexAppServerConnection( } child.on('error', (error) => { - exited = true handleUnexpectedEnd(error) }) // Why: 'close' rather than 'exit' guarantees the stderr tail is complete, so @@ -291,10 +292,9 @@ export async function openCodexAppServerConnection( } } - async function close(): Promise { + async function close(): Promise { if (closing) { - await exitPromise - return + return exitObserved } closing = true try { @@ -310,6 +310,7 @@ export async function openCodexAppServerConnection( } } failPending(new Error('codex app-server connection closed')) + return exitObserved } const connection: CodexAppServerConnection = { diff --git a/src/main/codex/codex-app-server-session.ts b/src/main/codex/codex-app-server-session.ts index ad7fb18a313..e559d0d8467 100644 --- a/src/main/codex/codex-app-server-session.ts +++ b/src/main/codex/codex-app-server-session.ts @@ -88,6 +88,21 @@ export function killCodexAppServerProcessTree( // Fall through to the direct-child best effort when taskkill cannot start. } } + if (child.pid) { + try { + // npm/package-manager launchers insert a shim child on POSIX. Reap its + // direct descendants before signalling the wrapper itself. + const descendants = spawnImpl('pkill', ['-KILL', '-P', String(child.pid)], { + stdio: 'ignore' + }) + // A missing pkill surfaces as an async 'error' event, and an unhandled one + // takes down the main process. + descendants.on('error', () => undefined) + descendants.unref() + } catch { + // The direct kill below remains the fallback when pkill is unavailable. + } + } child.kill('SIGKILL') } diff --git a/src/main/codex/codex-structured-session-adapter.ts b/src/main/codex/codex-structured-session-adapter.ts index 35fa0b17666..a9112230525 100644 --- a/src/main/codex/codex-structured-session-adapter.ts +++ b/src/main/codex/codex-structured-session-adapter.ts @@ -291,14 +291,14 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap /** Reaps one session's child. The proven handle chain is already durable, so * a graceful close loses nothing. */ - async closeSession(sessionId: string): Promise { + async closeSession(sessionId: string): Promise { const attempt = this.acquisitions.get(sessionId) if (attempt) { attempt.cancelled = true await attempt.window.connection?.close() await attempt.finished } - await closeCodexPublishedSession(this.sessions, sessionId, this.deps.onEvent) + return closeCodexPublishedSession(this.sessions, sessionId, this.deps.onEvent) } async closeAll(): Promise { @@ -309,8 +309,9 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap } } - releaseAcquisition = (input: { sessionId: string }): Promise => - this.closeSession(input.sessionId) + releaseAcquisition = async (input: { sessionId: string }): Promise => { + await this.closeSession(input.sessionId) + } private session(sessionId: string): CodexSession { const session = this.sessions.get(sessionId) diff --git a/src/main/codex/codex-structured-session-close.ts b/src/main/codex/codex-structured-session-close.ts index 422074159c2..86e3b58b08b 100644 --- a/src/main/codex/codex-structured-session-close.ts +++ b/src/main/codex/codex-structured-session-close.ts @@ -4,13 +4,19 @@ export async function closeCodexPublishedSession( sessions: Map, sessionId: string, onEvent?: (event: CodexStructuredSessionEvent) => void -): Promise { +): Promise { const session = sessions.get(sessionId) if (!session) { - return + return true + } + session.prompts.clear() + // Keep the session indexed until the child exit is observed. A timeout or + // failed kill must leave the live connection available for a safe retry. + const exited = await session.connection.close() + if (exited === false) { + return false } sessions.delete(sessionId) - session.prompts.clear() const event: CodexStructuredSessionEvent = { type: 'ended', sessionId, @@ -20,5 +26,5 @@ export async function closeCodexPublishedSession( onEvent?.(event) session.translator?.flush() session.translator?.dispose() - await session.connection.close() + return true } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts index ca336467f02..4decb76ad34 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts @@ -102,5 +102,5 @@ export type StructuredAgentSessionAdapter = { * resolver discover it from the provider session id. */ historyFilePath?(input: { identity: AgentSessionJournalIdentity }): Promise /** Gracefully stops the structured owner after its event stream is drained. */ - closeSession?(sessionId: string): Promise + closeSession?(sessionId: string): Promise } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-forward.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-forward.ts index d99a36699bf..5784dc3424d 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-forward.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-forward.ts @@ -33,7 +33,12 @@ export async function handoffStructuredSessionToTui( throw new Error('agent_session_operation_conflict') } record = context.requireRecord(sessionId) - await deps.suspendNative(sessionId) + // A kill is a request. Everything below advances the fence and hands the + // provider session to a TUI, so an unproven exit must stop here rather than + // create a second live writer on the same thread. + if ((await deps.suspendNative(sessionId)) === false) { + throw new Error('agent_session_owner_exit_unproven') + } record = await stopStoredAgentSessionOwnerForHandoff(deps.store, { sessionId, expectedFence: record.lease.runtimeFence, diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-options.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-options.test.ts index 2b6447a1a14..d0adf884b89 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-options.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-options.test.ts @@ -39,6 +39,8 @@ let activeModel: string let activeEffort: string | null let transcriptPath: string let optionFailure: Error | null +/** What the adapter's closeSession reports about the child's exit. */ +let closeSessionExit: boolean | undefined const dispatchedModels: string[] = [] const launchedOptions: (Readonly> | undefined)[] = [] const closedTuiOwners: StructuredTuiOwner[] = [] @@ -158,6 +160,7 @@ function adapter(): StructuredAgentSessionAdapter { })), closeSession: vi.fn(async () => { activeModel = DEFAULT_MODEL + return closeSessionExit }) } } @@ -168,6 +171,7 @@ beforeEach(async () => { activeModel = DEFAULT_MODEL activeEffort = null optionFailure = null + closeSessionExit = undefined dispatchedModels.length = 0 launchedOptions.length = 0 closedTuiOwners.length = 0 @@ -282,4 +286,17 @@ describe('structured session handoff options', () => { ).toMatchObject({ ok: true }) expect(dispatchedModels).toEqual([PICKED_MODEL]) }) + + // A kill is a request. If the app-server never proved it exited, handing the + // thread to a TUI would put two live writers on one provider session. + it('refuses the handoff when closing the native owner cannot prove it exited', async () => { + closeSessionExit = false + + expect(await host.requestHandoff(CALLER, handoff('to-tui'))).toMatchObject({ ok: true }) + await vi.waitFor(async () => + expect(await host.handoffStatus(SESSION)).toMatchObject({ phase: 'failed' }) + ) + expect(launchedOptions).toEqual([]) + expect(store.getRecord(SESSION)?.lease.runtimeKind).toBe('native') + }) }) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-types.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-types.ts index afe398a54d1..e3d125f7e04 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-types.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-handoff-types.ts @@ -55,7 +55,7 @@ export type StructuredAgentSessionHandoffDeps = { claimKeyId: string transport?: StructuredAgentSessionHandoffTransport session: (sessionId: string) => { journal: AgentSessionJournal; fence: number } - suspendNative: (sessionId: string) => Promise + suspendNative: (sessionId: string) => Promise acquireNative: (input: { sessionId: string fence: number diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-host-handoff.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-host-handoff.ts index 313d5d591b2..c4e3890a960 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-host-handoff.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-host-handoff.ts @@ -68,9 +68,14 @@ export function createStructuredAgentSessionHostHandoff( ...(deps.handoffTransport ? { transport: deps.handoffTransport } : {}), session: host.session, suspendNative: async (sessionId) => { - await deps.adapter.closeSession?.(sessionId) + const exited = await deps.adapter.closeSession?.(sessionId) + if (exited === false) { + // Report the unproven exit; the forward handoff refuses on it. + return false + } await host.flush(sessionId) host.eventSink(sessionId).unbind() + return true }, acquireNative: (input) => acquireNativeHandoffOwner(deps, host, input), acquireNativeStop: async (sessionId, turnId, fence) => diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index e0ae303b658..e4b01ae9251 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -10800,7 +10800,25 @@ export class OrcaRuntimeService { } await new Promise((resolve) => setTimeout(resolve, 100)) } - throw new Error('The recovered owner process did not exit.') + // SIGTERM is only a request. Escalate once, then require an independent + // absence probe before allowing the lease transition to proceed. + try { + process.kill(identity.pid, 'SIGKILL') + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ESRCH') { + throw error + } + return + } + const forcedDeadline = Date.now() + 5_000 + while (Date.now() < forcedDeadline) { + const current = await probeAgentSessionProcessIdentity({ identity }) + if (current.outcome === 'pid-absent' || current.outcome === 'identity-mismatch') { + return + } + await new Promise((resolve) => setTimeout(resolve, 100)) + } + throw new Error('The recovered owner process did not exit after forced termination.') } private structuredTuiStatus(owner: StructuredTuiOwner): 'idle' | 'busy' {