Repository navigation
fix(codex): settle dispatch after unrecoverable reconnect #24928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,13 @@ import { | |
| classifyWorkerTerminalProcessIncarnation, | ||
| parseWorkerTerminalHostScope | ||
| } from './orchestration/worker-terminal-process-liveness' | ||
| import { | ||
| CODEX_RECONNECT_SCAN_CONTEXT_CHARS, | ||
| codexReconnectOutputScans, | ||
| createCodexReconnectOutputScan, | ||
| findCodexReconnectFailureBanner, | ||
| stripTerminalControlSequences | ||
| } from './orca-runtime-on-pty-data' | ||
| import { getRepoIdFromWorktreeId } from '../../shared/worktree/id' | ||
| import { buildOrchestrationTaskDisplayMetadata } from '../../shared/orchestration-task-display' | ||
|
|
||
|
|
@@ -92,12 +99,122 @@ export class OrcaRuntimeWithSubscribeToTerminalResize extends OrcaRuntimeWithApp | |
| return | ||
| } | ||
|
|
||
| // Why: create an escalation message so the coordinator is notified about | ||
| // the unexpected exit on its next check cycle, even if the circuit breaker | ||
| // hasn't tripped yet. | ||
| this.notifyWorkerDispatchFailure({ | ||
| dispatch, | ||
| handle, | ||
| failureLogLabel: 'worker exit', | ||
| subject: `Agent exited unexpectedly (${errorContext})`, | ||
| reason: errorContext, | ||
| bodyPrefix: `Worker ${handle} stopped while running task`, | ||
| payload: { | ||
| taskId: dispatch.task_id, | ||
| dispatchId: dispatch.id, | ||
| exitCode, | ||
| exitCause: cause, | ||
| handle | ||
| }, | ||
| settledStatus: settled?.status | ||
| }) | ||
| } | ||
|
|
||
| protected observeCodexReconnectFailureOutput( | ||
| handle: string, | ||
| paneKey: string | null, | ||
| pty: object, | ||
| output: string | ||
| ): void { | ||
| const db = this._orchestrationDb | ||
| const scan = codexReconnectOutputScans.get(pty) ?? createCodexReconnectOutputScan() | ||
| const dispatch = db?.getActiveDispatchForTerminal(handle, paneKey ?? undefined) | ||
| const dispatchId = dispatch?.id ?? null | ||
| if (dispatchId !== scan.dispatchId) { | ||
| scan.dispatchId = dispatchId | ||
| scan.tail = '' | ||
| scan.pendingDispatchId = null | ||
| scan.handledDispatchId = null | ||
| } | ||
| const combinedOutput = scan.tail + stripTerminalControlSequences(output) | ||
| if ( | ||
| dispatch && | ||
| scan.pendingDispatchId !== dispatchId && | ||
| scan.handledDispatchId !== dispatchId | ||
| ) { | ||
| const match = findCodexReconnectFailureBanner(combinedOutput) | ||
| if (match && match.end > scan.tail.length) { | ||
| scan.pendingDispatchId = dispatchId | ||
| } | ||
| } | ||
| if (dispatch && scan.pendingDispatchId === dispatchId) { | ||
| const result = this.failCodexSessionUnrecoverableDispatch(dispatch, handle) | ||
| if (result === 'settled' || result === 'ignored') { | ||
| scan.handledDispatchId = dispatchId | ||
| scan.pendingDispatchId = null | ||
| } | ||
| } | ||
|
Comment on lines
+147
to
+153
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When Technical details# Latched pending settlement without a readiness trigger
## Affected sites
- `src/main/runtime/orca-runtime-subscribe-to-terminal-resize.ts:147-153` — `'retry'` leaves `pendingDispatchId` set and does not mark handled; re-run happens only on the next chunk.
- `src/main/runtime/orchestration/db/dispatch-context/dispatch-completion.ts:261-263` — returns `'retry'` for `starting`/`start_unknown`.
## Required outcome
- A banner observed while the worker is not yet `ready` must still settle the dispatch once the worker reaches `ready`/`input_accepted`, even if no further PTY output arrives.
## Open questions for the human (optional)
- Is the pre-`ready` case reachable in practice (for example after a runtime restart re-adopts a live Codex PTY)? If not, `'retry'` may be dead code and could be simplified to `'ignored'`. |
||
| scan.tail = dispatch ? combinedOutput.slice(-CODEX_RECONNECT_SCAN_CONTEXT_CHARS) : '' | ||
| codexReconnectOutputScans.set(pty, scan) | ||
| } | ||
|
|
||
| protected failCodexSessionUnrecoverableDispatch( | ||
| dispatch: { id: string; run_id: string; task_id: string }, | ||
| handle: string | ||
| ): 'settled' | 'retry' | 'ignored' { | ||
| const db = this._orchestrationDb | ||
| if (!db) { | ||
| return 'ignored' | ||
| } | ||
| const reason = 'Codex could not restore its app-server session' | ||
| const settledStatus = db.failCodexSessionUnrecoverableDispatch(dispatch.id, reason) | ||
| if (settledStatus === 'retry') { | ||
| return 'retry' | ||
| } | ||
| if (settledStatus !== 'failed' && settledStatus !== 'circuit_broken') { | ||
| return 'ignored' | ||
| } | ||
| this.notifyWorkerDispatchFailure({ | ||
| dispatch, | ||
| handle, | ||
| failureLogLabel: 'worker failure', | ||
| taskDispositionNote: | ||
| ' The task was marked failed pending coordinator review and will not be retried automatically.', | ||
| subject: `Agent session failed (${reason})`, | ||
| reason, | ||
| bodyPrefix: `Worker ${handle} could not continue task`, | ||
| bodySuffix: ' The terminal and worktree were kept for diagnosis.', | ||
| payload: { | ||
| taskId: dispatch.task_id, | ||
| dispatchId: dispatch.id, | ||
| handle, | ||
| failureKind: 'codex_session_unrecoverable', | ||
| terminalPreserved: true, | ||
| worktreePreserved: true | ||
| }, | ||
| settledStatus | ||
| }) | ||
| return 'settled' | ||
| } | ||
|
|
||
| private notifyWorkerDispatchFailure(params: { | ||
| dispatch: { id: string; run_id: string; task_id: string } | ||
| handle: string | ||
| failureLogLabel: 'worker exit' | 'worker failure' | ||
| subject: string | ||
| reason: string | ||
| bodyPrefix: string | ||
| bodySuffix?: string | ||
| taskDispositionNote?: string | ||
| payload: Record<string, unknown> | ||
| settledStatus?: string | ||
| }): void { | ||
| const db = this._orchestrationDb | ||
| if (!db) { | ||
| return | ||
| } | ||
| try { | ||
| const owningRun = this._orchestrationDb.getRun?.(dispatch.run_id) | ||
| const active = this._orchestrationDb.getActiveCoordinatorRun?.() | ||
| // Why: a lightweight Run owns its own mailbox; legacy dispatches still fall back to the | ||
| // active coordinator address read by `orchestration check`. | ||
| const owningRun = db.getRun?.(params.dispatch.run_id) | ||
| const active = db.getActiveCoordinatorRun?.() | ||
| const recipient = | ||
| owningRun && owningRun.legacy !== 1 | ||
| ? { to: `run:${owningRun.id}`, runId: owningRun.id } | ||
|
|
@@ -107,7 +224,7 @@ export class OrcaRuntimeWithSubscribeToTerminalResize extends OrcaRuntimeWithApp | |
| if (!recipient) { | ||
| return | ||
| } | ||
| const task = this._orchestrationDb.getTask?.(dispatch.task_id, dispatch.run_id) | ||
| const task = db.getTask?.(params.dispatch.task_id, params.dispatch.run_id) | ||
| // Why: prefer the explicit task title and keep the derived one single-line and bounded; | ||
| // a raw multi-paragraph spec inlined here breaks the coordinator's escalation banner. | ||
| const title = | ||
|
|
@@ -118,28 +235,29 @@ export class OrcaRuntimeWithSubscribeToTerminalResize extends OrcaRuntimeWithApp | |
| displayName: task.display_name | ||
| }).taskTitle | ||
| : '' | ||
| const named = title ? `"${title}" (${dispatch.task_id})` : dispatch.task_id | ||
| const escalation = this._orchestrationDb.insertMessage({ | ||
| from: handle, | ||
| const named = title ? `"${title}" (${params.dispatch.task_id})` : params.dispatch.task_id | ||
| const settlementNote = | ||
| params.taskDispositionNote ?? | ||
| (params.settledStatus === 'circuit_broken' | ||
| ? ' This task has now failed too many times, so it will not be retried automatically.' | ||
| : params.settledStatus === 'failed' | ||
| ? ' The task is ready to be dispatched again.' | ||
| : '') | ||
| const escalation = db.insertMessage({ | ||
| from: params.handle, | ||
| to: recipient.to, | ||
| subject: `Agent exited unexpectedly (${errorContext})`, | ||
| body: `Worker ${handle} stopped while running task ${named}. ${errorContext}.${settled?.status === 'circuit_broken' ? ' This task has now failed too many times, so it will not be retried automatically.' : settled?.status === 'failed' ? ' The task is ready to be dispatched again.' : ''}`, | ||
| subject: params.subject, | ||
| body: `${params.bodyPrefix} ${named}. ${params.reason}.${settlementNote}${params.bodySuffix ?? ''}`, | ||
| type: 'escalation', | ||
| priority: 'high', | ||
| payload: JSON.stringify({ | ||
| taskId: dispatch.task_id, | ||
| dispatchId: dispatch.id, | ||
| exitCode, | ||
| exitCause: cause, | ||
| handle | ||
| }), | ||
| runId: dispatch.run_id | ||
| payload: JSON.stringify(params.payload), | ||
| runId: params.dispatch.run_id | ||
| }) | ||
| this.notifyMessageArrived(escalation.to_handle, escalation.type) | ||
| } catch (error) { | ||
| console.warn('[orchestration] failed to escalate worker exit', { | ||
| dispatchId: dispatch.id, | ||
| runId: dispatch.run_id, | ||
| console.warn(`[orchestration] failed to escalate ${params.failureLogLabel}`, { | ||
| dispatchId: params.dispatch.id, | ||
| runId: params.dispatch.run_id, | ||
| error | ||
| }) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getActiveDispatchForTerminalnow runs once per PTY output chunk for every terminal, becauseobserveCodexReconnectFailureOutputis invoked unconditionally fromonPtyData(orca-runtime-on-pty-data.ts:189) before any banner check. In the previous revision this same lookup only ran afterhasCodexReconnectFailureBanner(pty.preview)matched, so the rework adds a DB query (up to three prepareddispatch_contextsstatements) to the hot terminal path — and contradicts the PR's "database work runs only when the complete banner matches" note.Technical details