From 3a8fcae7a7ae110bb21445f6aad3f449df20a4cc Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Mon, 7 Sep 2026 13:20:45 -0700 Subject: [PATCH] refactor(codex): move session teardown out of the structured adapter Merging main crossed the 300-line cap on `codex-structured-session-adapter.ts`: the rewind backend (#19235) and this branch's close-time strip clear both landed in it. The four close paths move verbatim into `codex-structured-session-teardown.ts`, where they funnel through one `settled` helper instead of repeating the notification-retry and background-task cleanup at each call site. No ratchet bump. Also normalize a background task's description once at receipt rather than on every projection; the roster is re-projected on each observed frame. --- .../codex/codex-background-task-tracker.ts | 34 +++---- .../codex/codex-structured-session-adapter.ts | 85 ++++------------- .../codex-structured-session-teardown.ts | 94 +++++++++++++++++++ 3 files changed, 129 insertions(+), 84 deletions(-) create mode 100644 src/main/codex/codex-structured-session-teardown.ts diff --git a/src/main/codex/codex-background-task-tracker.ts b/src/main/codex/codex-background-task-tracker.ts index 9676d51ebd6..555b6233e91 100644 --- a/src/main/codex/codex-background-task-tracker.ts +++ b/src/main/codex/codex-background-task-tracker.ts @@ -46,18 +46,20 @@ const AGENT_TASK_PREFIX = 'codex-agent:' const COMMAND_TASK_PREFIX = 'codex-command:' type TrackedChild = { - label: string | null + label: string | undefined state: NativeChatSubagentState /** Parent turn the child was spawned in; null when Codex named none. */ turnId: string | null } type TrackedCommand = { - label: string | null + label: string | undefined running: boolean turnId: string | null } +/** Normalized once at receipt, not per projection: the roster is re-projected on + * every observed frame, and the bound is a property of the stored value. */ function description(value: string | null): string | undefined { if (value === null) { return undefined @@ -66,6 +68,14 @@ function description(value: string | null): string | undefined { return collapsed.length > 0 ? collapsed.slice(0, MAX_TASK_DESCRIPTION_CHARS) : undefined } +function task( + id: string, + kind: AgentSessionBackgroundTask['kind'], + label: string | undefined +): AgentSessionBackgroundTask { + return { id, kind, ...(label === undefined ? {} : { description: label }) } +} + export class CodexBackgroundTaskTracker { private readonly children = new Map() private readonly commands = new Map() @@ -129,7 +139,7 @@ export class CodexBackgroundTaskTracker { // transition (`item/started` and `item/completed`), so a settled child must // not be resurrected by the duplicate. this.children.set(agentThreadId, { - label: existing.label ?? label, + label: existing.label ?? description(label), state: isTerminalSubagentState(existing.state) ? existing.state : state, turnId: existing.turnId ?? turnId }) @@ -142,7 +152,7 @@ export class CodexBackgroundTaskTracker { ) { return } - this.children.set(agentThreadId, { label, state, turnId }) + this.children.set(agentThreadId, { label: description(label), state, turnId }) } private upsertCommand( @@ -154,7 +164,7 @@ export class CodexBackgroundTaskTracker { const existing = this.commands.get(itemId) if (existing) { this.commands.set(itemId, { - label: label ?? existing.label, + label: description(label) ?? existing.label, running, turnId: existing.turnId ?? turnId }) @@ -163,7 +173,7 @@ export class CodexBackgroundTaskTracker { if (!this.makeRoom(this.commands, MAX_TRACKED_COMMANDS, (command) => !command.running)) { return } - this.commands.set(itemId, { label, running, turnId }) + this.commands.set(itemId, { label: description(label), running, turnId }) } /** Frees a slot by dropping the oldest settled entry. Refuses to evict a live @@ -210,21 +220,13 @@ export class CodexBackgroundTaskTracker { if (isTerminalSubagentState(child.state) || !this.outlivedItsTurn(child.turnId)) { continue } - tasks.push({ - id: `${AGENT_TASK_PREFIX}${agentThreadId}`, - kind: 'agent', - ...(description(child.label) ? { description: description(child.label) } : {}) - }) + tasks.push(task(`${AGENT_TASK_PREFIX}${agentThreadId}`, 'agent', child.label)) } for (const [itemId, command] of this.commands) { if (!command.running || !this.outlivedItsTurn(command.turnId)) { continue } - tasks.push({ - id: `${COMMAND_TASK_PREFIX}${itemId}`, - kind: 'command', - ...(description(command.label) ? { description: description(command.label) } : {}) - }) + tasks.push(task(`${COMMAND_TASK_PREFIX}${itemId}`, 'command', command.label)) } return tasks } diff --git a/src/main/codex/codex-structured-session-adapter.ts b/src/main/codex/codex-structured-session-adapter.ts index 3acc46bf26a..55d809f0b1b 100644 --- a/src/main/codex/codex-structured-session-adapter.ts +++ b/src/main/codex/codex-structured-session-adapter.ts @@ -16,11 +16,7 @@ import type { CodexJournalTranslationAdmission } from './codex-structured-journa import { answerCodexPrompt } from './codex-structured-prompt-replies' import { dispatchCodexTurn, isCodexTurnOptionKey } from './codex-structured-turn-start' import { supportsCodexStructuredLocation } from './codex-structured-location-support' -import { - closeAllCodexSessions, - closeCodexPublishedSession, - closeCodexSession -} from './codex-structured-session-close' +import { CodexStructuredSessionTeardown } from './codex-structured-session-teardown' import { applyCodexStructuredSessionOption, readLiveCodexSessionOptions @@ -54,6 +50,7 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap private readonly acquisitions = new CodexAcquisitionRegistry() private readonly turnCancellation: CodexStructuredTurnCancellation private readonly notificationRetries: ReturnType + private readonly teardown: CodexStructuredSessionTeardown constructor(private readonly deps: CodexStructuredSessionAdapterDeps) { this.notificationRetries = createCodexStructuredNotificationRetry({ @@ -61,6 +58,15 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap translate: (sessionId, session, method, params) => this.translateNotification(sessionId, session, method, params) }) + this.teardown = new CodexStructuredSessionTeardown({ + sessions: this.sessions, + acquisitions: this.acquisitions, + ...(deps.onEvent ? { onEvent: deps.onEvent } : {}), + ...(deps.onBackgroundTasksChanged + ? { onBackgroundTasksChanged: deps.onBackgroundTasksChanged } + : {}), + forgetNotificationRetries: (sessionId) => this.notificationRetries.clear(sessionId, null) + }) this.turnCancellation = new CodexStructuredTurnCancellation({ captureTurnProcesses: deps.captureTurnProcesses, terminateTurnProcesses: deps.terminateTurnProcesses, @@ -92,7 +98,7 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap handleUnhandledFrame: (sessionId, kind, payload) => this.handleUnhandledFrame(sessionId, kind, payload), forceCloseUnexpected: (sessionId, fence, acquisitionGeneration, reason) => - this.forceCloseUnexpected(sessionId, fence, acquisitionGeneration, reason) + this.teardown.forceCloseUnexpected(sessionId, fence, acquisitionGeneration, reason) }) /** Buffers pre-publication events and drops events from superseded children. */ @@ -176,16 +182,6 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap sessionId ) => this.sessions.get(sessionId)?.backgroundTasks.state - /** A closed session monitors nothing. Published explicitly because the state - * reader answers `undefined` once the session leaves the map, which every - * channel reads as "unchanged" and would leave the last roster on screen. */ - private clearBackgroundTasks(sessionId: string, closed: boolean): boolean { - if (closed) { - this.deps.onBackgroundTasksChanged?.(sessionId, null) - } - return closed - } - bindPromptItemId = (sessionId: string, journalItemId: string, promptKey: string): void => this.sessions .get(sessionId) @@ -286,59 +282,12 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap identity: AgentSessionJournalIdentity }): Promise => this.sessions.get(input.identity.sessionId)?.historyPath ?? null - closeSession = async (sessionId: string): Promise => { - const closed = await closeCodexSession( - sessionId, - this.sessions, - this.acquisitions, - this.deps.onEvent - ) - if (closed) { - this.notificationRetries.clear(sessionId, null) - } - return this.clearBackgroundTasks(sessionId, closed) - } - forceCloseSession = async (sessionId: string): Promise => { - const closed = await closeCodexPublishedSession(this.sessions, sessionId, this.deps.onEvent, { - allowFailedSettlement: true, - requestedClose: false - }) - if (closed) { - this.notificationRetries.clear(sessionId, null) - } - return this.clearBackgroundTasks(sessionId, closed) - } - - private forceCloseUnexpected( - sessionId: string, - fence: number, - acquisitionGeneration: string, - reason: Error - ): Promise { - const session = this.sessions.get(sessionId) - if ( - !session || - session.ended || - session.fence !== fence || - session.acquisitionGeneration !== acquisitionGeneration - ) { - return Promise.resolve(false) - } - return closeCodexPublishedSession(this.sessions, sessionId, this.deps.onEvent, { - allowFailedSettlement: true, - requestedClose: false, - expectedFence: fence, - expectedAcquisitionGeneration: acquisitionGeneration, - unexpectedReason: reason - }).then((closed) => this.clearBackgroundTasks(sessionId, closed)) - } - disposeSession = (sessionId: string): Promise => this.closeSession(sessionId) - closeAll = (): Promise => - closeAllCodexSessions(this.sessions, this.acquisitions, (sessionId) => - this.disposeSession(sessionId) - ) + closeSession = (sessionId: string): Promise => this.teardown.close(sessionId) + forceCloseSession = (sessionId: string): Promise => this.teardown.forceClose(sessionId) + disposeSession = (sessionId: string): Promise => this.teardown.close(sessionId) + closeAll = (): Promise => this.teardown.closeAll() releaseAcquisition = (input: { sessionId: string }): Promise => - this.closeSession(input.sessionId) + this.teardown.close(input.sessionId) private session(sessionId: string): CodexSession { return requireLiveCodexSession(this.sessions, sessionId) diff --git a/src/main/codex/codex-structured-session-teardown.ts b/src/main/codex/codex-structured-session-teardown.ts new file mode 100644 index 00000000000..dcd38eb82fe --- /dev/null +++ b/src/main/codex/codex-structured-session-teardown.ts @@ -0,0 +1,94 @@ +// Stopping one Codex app-server child, in the four ways the host asks for it. +// +// Every path funnels through `settled` so the ephemeral surfaces a closed +// session owns are cleared exactly once, and only when the child was actually +// proven stopped — a refused close leaves the session indexed for a retry. + +import type { AgentSessionBackgroundTaskState } from '../../shared/agent-session-wire' +import { + closeAllCodexSessions, + closeCodexPublishedSession, + closeCodexSession +} from './codex-structured-session-close' +import type { + CodexAcquisitionRegistry, + CodexSession, + CodexStructuredSessionEvent +} from './codex-structured-session-state' + +export type CodexStructuredSessionTeardownDeps = { + sessions: Map + acquisitions: CodexAcquisitionRegistry + onEvent?: (event: CodexStructuredSessionEvent) => void + onBackgroundTasksChanged?: ( + sessionId: string, + state: AgentSessionBackgroundTaskState | null + ) => void + forgetNotificationRetries: (sessionId: string) => void +} + +export class CodexStructuredSessionTeardown { + constructor(private readonly deps: CodexStructuredSessionTeardownDeps) {} + + close = async (sessionId: string): Promise => { + const closed = await closeCodexSession( + sessionId, + this.deps.sessions, + this.deps.acquisitions, + this.deps.onEvent + ) + return this.settled(sessionId, closed) + } + + forceClose = async (sessionId: string): Promise => { + const closed = await closeCodexPublishedSession( + this.deps.sessions, + sessionId, + this.deps.onEvent, + { allowFailedSettlement: true, requestedClose: false } + ) + return this.settled(sessionId, closed) + } + + /** Terminates this exact child as an unexpected death. Every ownership check + * stays here so a stale caller cannot close a replacement child. */ + forceCloseUnexpected = ( + sessionId: string, + fence: number, + acquisitionGeneration: string, + reason: Error + ): Promise => { + const session = this.deps.sessions.get(sessionId) + if ( + !session || + session.ended || + session.fence !== fence || + session.acquisitionGeneration !== acquisitionGeneration + ) { + return Promise.resolve(false) + } + return closeCodexPublishedSession(this.deps.sessions, sessionId, this.deps.onEvent, { + allowFailedSettlement: true, + requestedClose: false, + expectedFence: fence, + expectedAcquisitionGeneration: acquisitionGeneration, + unexpectedReason: reason + }).then((closed) => this.settled(sessionId, closed)) + } + + closeAll = (): Promise => + closeAllCodexSessions(this.deps.sessions, this.deps.acquisitions, (sessionId) => + this.close(sessionId) + ) + + private settled(sessionId: string, closed: boolean): boolean { + if (closed) { + this.deps.forgetNotificationRetries(sessionId) + // Explicit null, not silence: the state reader answers `undefined` once + // the session leaves the map, which every channel reads as "unchanged" + // and would leave the last roster on screen. + this.deps.onBackgroundTasksChanged?.(sessionId, null) + } + return closed + } +}