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' {