From 5648b61075676c8c081938c8a7ed5f64697e68b9 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Tue, 2 Jun 2026 05:29:32 +0900 Subject: [PATCH] fix: stop Codex pool retry loops --- src/agent-error-detection.test.ts | 13 +++++ src/agent-error-detection.ts | 30 +++++++++++ src/agent-runner-environment.test.ts | 2 +- src/agent-runner-environment.ts | 2 +- src/codex-token-rotation.test.ts | 34 ++++++++++++ src/codex-token-rotation.ts | 17 ++++++ src/db-paired-turn-attempt-recovery.test.ts | 37 ++++++++++++++ src/db/paired-turn-attempts.ts | 18 +++++++ src/message-agent-executor-rules.test.ts | 13 +++++ src/message-agent-executor-rules.ts | 12 +---- src/paired-execution-context-arbiter.ts | 12 +---- src/paired-execution-context-owner.ts | 26 ++++++++++ src/paired-execution-context-reviewer.ts | 26 ++++++++++ src/paired-execution-context-routing.test.ts | 54 ++++++++++++++++++++ src/task-scheduler.test.ts | 53 ++++++++++++++++++- src/task-scheduler.ts | 44 ++++++++++++++++ 16 files changed, 369 insertions(+), 24 deletions(-) diff --git a/src/agent-error-detection.test.ts b/src/agent-error-detection.test.ts index 79cbd1b..53a3e2c 100644 --- a/src/agent-error-detection.test.ts +++ b/src/agent-error-detection.test.ts @@ -85,6 +85,19 @@ describe('agent-error-detection', () => { }); }); + it('does not classify the internal Codex pool-unavailable sentinel as an auth failure', () => { + expect( + classifyCodexAuthError( + 'auth-expired: All Codex rotation accounts unavailable; re-auth required before launching Codex', + ), + ).toEqual({ category: 'none', reason: '' }); + expect( + classifyCodexAuthError( + 'Codex rotation pool unavailable: all rotation accounts are currently dead, rate-limited, or locked', + ), + ).toEqual({ category: 'none', reason: '' }); + }); + it('classifies Codex workspace credit exhaustion as rate-limit', () => { expect(classifyAgentError('Workspace out of credits')).toEqual({ category: 'rate-limit', diff --git a/src/agent-error-detection.ts b/src/agent-error-detection.ts index 9664e1e..8f855bc 100644 --- a/src/agent-error-detection.ts +++ b/src/agent-error-detection.ts @@ -231,6 +231,35 @@ const NONE: AgentErrorClassification = { reason: '', }; +export function isCodexPoolUnavailableError( + error: string | null | undefined, +): boolean { + if (!error) return false; + return ( + /all\s+codex(?:\s+rotation)?\s+accounts(?:\s+are)?\s+unavailable/i.test( + error, + ) || /codex\s+rotation\s+pool\s+unavailable/i.test(error) + ); +} + +export function isTerminalCodexAccountFailure( + error: string | null | undefined, +): boolean { + if (!error) return false; + if (isCodexPoolUnavailableError(error)) return true; + if (classifyCodexAuthError(error).category !== 'none') return true; + const lower = error.toLowerCase(); + if ( + classifyAgentError(error).category === 'rate-limit' && + (lower.includes('workspace out of credits') || + lower.includes('out of credits') || + lower.includes('codex')) + ) { + return true; + } + return false; +} + /** * Classify an agent error string into a category. * Handles patterns common to both Claude and Codex: 429, 503, network. @@ -334,6 +363,7 @@ export function classifyCodexAuthError( error: string | null | undefined, ): AgentErrorClassification { if (!error) return NONE; + if (isCodexPoolUnavailableError(error)) return NONE; const lower = error.toLowerCase(); if ( diff --git a/src/agent-runner-environment.test.ts b/src/agent-runner-environment.test.ts index c944a05..e70b5db 100644 --- a/src/agent-runner-environment.test.ts +++ b/src/agent-runner-environment.test.ts @@ -273,7 +273,7 @@ describe('prepareGroupEnvironment codex auth handling', () => { mockReadEnvFile.mockReturnValue({}); expect(() => prepareGroupEnvironment(group, false, 'dc:test')).toThrow( - /auth-expired: All Codex rotation accounts unavailable/, + /Codex rotation pool unavailable/, ); const authPath = path.join( diff --git a/src/agent-runner-environment.ts b/src/agent-runner-environment.ts index e5bebf9..0422071 100644 --- a/src/agent-runner-environment.ts +++ b/src/agent-runner-environment.ts @@ -193,7 +193,7 @@ function syncHostCodexSessionFiles( lease?.release(); if (hasRotationAccounts) { throw new Error( - 'auth-expired: All Codex rotation accounts unavailable; re-auth required before launching Codex', + 'Codex rotation pool unavailable: all rotation accounts are currently dead, rate-limited, or locked; re-auth or clear stale state before launching Codex', ); } return null; diff --git a/src/codex-token-rotation.test.ts b/src/codex-token-rotation.test.ts index be4ec7b..1647112 100644 --- a/src/codex-token-rotation.test.ts +++ b/src/codex-token-rotation.test.ts @@ -6,6 +6,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; vi.mock('./agent-error-detection.js', () => ({ classifyAgentError: vi.fn(() => ({ category: 'none', reason: '' })), classifyCodexAuthError: vi.fn(() => ({ category: 'none', reason: '' })), + isCodexPoolUnavailableError: vi.fn( + (error: string | null | undefined) => + /all\s+codex(?:\s+rotation)?\s+accounts(?:\s+are)?\s+unavailable/i.test( + error ?? '', + ) || /codex\s+rotation\s+pool\s+unavailable/i.test(error ?? ''), + ), })); vi.mock('./config.js', () => ({ @@ -166,6 +172,34 @@ describe('codex-token-rotation d7 ≥ 100% auto-skip', () => { expect(mod.getActiveCodexAuthPath()).not.toBe(fallbackAuthPath); }); + it('does not mutate account health for the internal all-accounts-unavailable sentinel', async () => { + const agentErrors = await import('./agent-error-detection.js'); + vi.mocked(agentErrors.classifyCodexAuthError).mockReturnValueOnce({ + category: 'auth-expired', + reason: 'auth-expired', + }); + + const mod = await import('./codex-token-rotation.js'); + mod.initCodexTokenRotation(); + + expect( + mod.rotateCodexToken( + 'auth-expired: All Codex rotation accounts unavailable; re-auth required before launching Codex', + ), + ).toBe(false); + + const accounts = mod.getAllCodexAccounts(); + expect(accounts[0]).toEqual( + expect.objectContaining({ + isActive: true, + isAuthDead: false, + authStatus: 'healthy', + isRateLimited: false, + }), + ); + expect(accounts[1].isActive).toBe(false); + }); + it('marks refresh-token reuse as dead auth instead of a recoverable cooldown', async () => { const agentErrors = await import('./agent-error-detection.js'); vi.mocked(agentErrors.classifyCodexAuthError).mockReturnValueOnce({ diff --git a/src/codex-token-rotation.ts b/src/codex-token-rotation.ts index 1ed0f93..3b8066c 100644 --- a/src/codex-token-rotation.ts +++ b/src/codex-token-rotation.ts @@ -17,6 +17,7 @@ import path from 'path'; import { classifyAgentError, classifyCodexAuthError, + isCodexPoolUnavailableError, type CodexRotationReason, } from './agent-error-detection.js'; import { DATA_DIR } from './config.js'; @@ -638,6 +639,9 @@ export function detectCodexRotationTrigger( error?: string | null, ): CodexRotationTriggerResult { if (!error) return { shouldRotate: false, reason: '' }; + if (isCodexPoolUnavailableError(error)) { + return { shouldRotate: false, reason: '' }; + } // Common patterns (429, 503, network) — delegated to SSOT const common = classifyAgentError(error); @@ -664,6 +668,19 @@ export function rotateCodexToken( ): boolean { if (accounts.length <= 1) return false; + if (isCodexPoolUnavailableError(errorMessage)) { + logger.warn( + { + transition: 'rotation:skip-pool-unavailable-sentinel', + currentIndex, + totalAccounts: accounts.length, + reason: errorMessage ?? null, + }, + 'Refusing to mark Codex accounts unhealthy from internal pool-unavailable sentinel', + ); + return false; + } + const previousIndex = currentIndex; const acct = accounts[currentIndex]; const authFailure = classifyCodexAuthError(errorMessage); diff --git a/src/db-paired-turn-attempt-recovery.test.ts b/src/db-paired-turn-attempt-recovery.test.ts index 9e87c6c..245f053 100644 --- a/src/db-paired-turn-attempt-recovery.test.ts +++ b/src/db-paired-turn-attempt-recovery.test.ts @@ -143,6 +143,43 @@ describe('paired turn attempt restart recovery', () => { ]); }); + it('clears stale running attempts for already completed tasks without replaying them', () => { + const task = makeTask({ + id: 'completed-stale-running-task', + status: 'completed', + completion_reason: 'done', + }); + createPairedTask(task); + const turnIdentity = buildPairedTurnIdentity({ + taskId: task.id, + taskUpdatedAt: task.updated_at, + intentKind: 'owner-turn', + role: 'owner', + }); + + markPairedTurnRunning({ + turnIdentity, + executorServiceId: CODEX_MAIN_SERVICE_ID, + executorAgentType: 'codex', + runId: 'completed-run-before-restart', + }); + + expect( + recoverInterruptedPairedTurnAttemptsForService({ + serviceIds: [CODEX_MAIN_SERVICE_ID], + now: '2026-05-27T00:11:00.000Z', + }), + ).toEqual([]); + expect(getPairedTurnAttempts(turnIdentity.turnId)).toEqual([ + expect.objectContaining({ + state: 'failed', + active_run_id: null, + completed_at: '2026-05-27T00:11:00.000Z', + last_error: 'Interrupted by service restart before completion.', + }), + ]); + }); + it('recovers codex executor attempts even when the orchestration lease used the claude service id', () => { const task = makeTask({ id: 'cross-service-lease-task' }); createPairedTask(task); diff --git a/src/db/paired-turn-attempts.ts b/src/db/paired-turn-attempts.ts index b9f3f33..10220d0 100644 --- a/src/db/paired-turn-attempts.ts +++ b/src/db/paired-turn-attempts.ts @@ -462,6 +462,24 @@ export function recoverInterruptedPairedTurnAttemptsForServiceInDatabase( const error = args.error ?? 'Interrupted by service restart before completion.'; return database.transaction(() => { + database + .prepare( + ` + UPDATE paired_turn_attempts + SET state = 'failed', + active_run_id = NULL, + updated_at = ?, + completed_at = ?, + last_error = COALESCE(last_error, ?) + WHERE state = 'running' + AND executor_service_id IN (${servicePlaceholders}) + AND task_id IN ( + SELECT id FROM paired_tasks WHERE status = 'completed' + ) + `, + ) + .run(now, now, error, ...serviceIds); + const rows = database .prepare( ` diff --git a/src/message-agent-executor-rules.test.ts b/src/message-agent-executor-rules.test.ts index 1d20c34..84db5ec 100644 --- a/src/message-agent-executor-rules.test.ts +++ b/src/message-agent-executor-rules.test.ts @@ -207,6 +207,19 @@ describe('message-agent-executor-rules', () => { ).toBe('pending'); }); + it('does not requeue a silent owner failure caused by Codex pool unavailability', () => { + expect( + resolvePairedFollowUpQueueAction({ + completedRole: 'owner', + executionStatus: 'failed', + sawOutput: false, + taskStatus: 'active', + outputSummary: + 'auth-expired: All Codex rotation accounts unavailable; re-auth required before launching Codex\nExecution completed without a visible terminal verdict.', + }), + ).toBe('none'); + }); + it('does not request an executor-side follow-up after successful owner output moved the task to review_ready', () => { expect( resolvePairedFollowUpQueueAction({ diff --git a/src/message-agent-executor-rules.ts b/src/message-agent-executor-rules.ts index e405d02..4af85ad 100644 --- a/src/message-agent-executor-rules.ts +++ b/src/message-agent-executor-rules.ts @@ -3,7 +3,7 @@ import { resolveNextTurnAction, shouldRetrySilentOwnerExecution, } from './message-runtime-rules.js'; -import { classifyCodexAuthError } from './agent-error-detection.js'; +import { isTerminalCodexAccountFailure } from './agent-error-detection.js'; import { parseVisibleVerdict } from './paired-verdict.js'; import type { PairedRoomRole, PairedTaskStatus } from './types.js'; export { @@ -19,14 +19,7 @@ export { export type PairedFollowUpQueueAction = 'pending' | 'none'; function isSilentCodexAccountFailure(summary?: string | null): boolean { - if (!summary) return false; - if (classifyCodexAuthError(summary).category !== 'none') return true; - const lower = summary.toLowerCase(); - return ( - lower.includes('workspace out of credits') || - lower.includes('out of credits') || - /all\s+codex(?:\s+rotation)?\s+accounts/i.test(summary) - ); + return isTerminalCodexAccountFailure(summary); } export function resolvePairedFollowUpQueueAction(args: { @@ -39,7 +32,6 @@ export function resolvePairedFollowUpQueueAction(args: { if ( args.executionStatus === 'failed' && args.sawOutput === false && - (args.completedRole === 'reviewer' || args.completedRole === 'arbiter') && isSilentCodexAccountFailure(args.outputSummary) ) { return 'none'; diff --git a/src/paired-execution-context-arbiter.ts b/src/paired-execution-context-arbiter.ts index 83214c0..95541a1 100644 --- a/src/paired-execution-context-arbiter.ts +++ b/src/paired-execution-context-arbiter.ts @@ -1,21 +1,11 @@ import { logger } from './logger.js'; -import { - classifyAgentError, - classifyCodexAuthError, -} from './agent-error-detection.js'; +import { isTerminalCodexAccountFailure } from './agent-error-detection.js'; import { transitionPairedTaskStatus } from './paired-task-status.js'; import { classifyArbiterVerdict } from './paired-verdict.js'; import type { PairedTask } from './types.js'; const ARBITER_RESOLUTION_ROUND_TRIP_COUNT = 0; -function isTerminalCodexAccountFailure(summary?: string | null): boolean { - if (!summary) return false; - if (classifyCodexAuthError(summary).category !== 'none') return true; - if (classifyAgentError(summary).category === 'rate-limit') return true; - return /all\s+codex(?:\s+rotation)?\s+accounts/i.test(summary); -} - export function handleFailedArbiterExecution(args: { task: PairedTask; taskId: string; diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index 2d28c96..0ccc06b 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -7,6 +7,7 @@ import { getPairedWorkspace, hasActiveCiWatcherForChat, } from './db.js'; +import { isTerminalCodexAccountFailure } from './agent-error-detection.js'; import { logger } from './logger.js'; import { markPairedTaskReviewReady } from './paired-workspace-manager.js'; import { requestArbiterOrEscalate } from './paired-arbiter-request.js'; @@ -30,6 +31,31 @@ export function handleFailedOwnerExecution(args: { }): void { const { task, taskId, summary } = args; const now = new Date().toISOString(); + if (isTerminalCodexAccountFailure(summary)) { + transitionPairedTaskStatus({ + taskId, + currentStatus: task.status, + nextStatus: 'completed', + expectedUpdatedAt: task.updated_at, + updatedAt: now, + patch: { + arbiter_verdict: 'escalate', + arbiter_requested_at: null, + completion_reason: 'owner_codex_unavailable', + }, + }); + logger.warn( + { + taskId, + role: 'owner', + status: task.status, + summary: summary?.slice(0, 200), + }, + 'Completed owner task after terminal Codex account failure instead of retrying owner loop', + ); + return; + } + const nextFailureCount = (task.owner_failure_count ?? 0) + 1; if (nextFailureCount >= OWNER_FAILURE_ESCALATION_THRESHOLD) { diff --git a/src/paired-execution-context-reviewer.ts b/src/paired-execution-context-reviewer.ts index 8dc0d58..119a1aa 100644 --- a/src/paired-execution-context-reviewer.ts +++ b/src/paired-execution-context-reviewer.ts @@ -1,5 +1,6 @@ import { ARBITER_DEADLOCK_THRESHOLD } from './config.js'; import { getPairedWorkspace } from './db.js'; +import { isTerminalCodexAccountFailure } from './agent-error-detection.js'; import { logger } from './logger.js'; import { requestArbiterOrEscalate } from './paired-arbiter-request.js'; import { transitionPairedTaskStatus } from './paired-task-status.js'; @@ -19,6 +20,31 @@ export function handleFailedReviewerExecution(args: { const { task, taskId, summary } = args; const now = new Date().toISOString(); + if (isTerminalCodexAccountFailure(summary)) { + transitionPairedTaskStatus({ + taskId, + currentStatus: task.status, + nextStatus: 'completed', + expectedUpdatedAt: task.updated_at, + updatedAt: now, + patch: { + arbiter_verdict: 'escalate', + arbiter_requested_at: null, + completion_reason: 'reviewer_codex_unavailable', + }, + }); + logger.warn( + { + taskId, + role: 'reviewer', + status: task.status, + summary: summary?.slice(0, 200), + }, + 'Completed reviewer task after terminal Codex account failure instead of preserving review loop', + ); + return; + } + if (summary) { const verdict = parseVisibleVerdict(summary); const signal = resolveReviewerFailureSignal({ diff --git a/src/paired-execution-context-routing.test.ts b/src/paired-execution-context-routing.test.ts index 023c0c6..2b80ed5 100644 --- a/src/paired-execution-context-routing.test.ts +++ b/src/paired-execution-context-routing.test.ts @@ -224,6 +224,60 @@ describe('paired execution routing loop guards', () => { ); }); + it('completes reviewer task after terminal Codex account failure instead of preserving review_ready loop', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_review', + updated_at: '2026-03-28T00:00:05.000Z', + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'reviewer', + status: 'failed', + summary: + 'auth-expired: All Codex rotation accounts unavailable; re-auth required before launching Codex\nExecution completed without a visible terminal verdict.', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'completed', + arbiter_verdict: 'escalate', + arbiter_requested_at: null, + completion_reason: 'reviewer_codex_unavailable', + }), + ); + }); + + it('completes owner task after terminal Codex account failure instead of retrying owner forever', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'active', + updated_at: '2026-03-28T00:00:05.000Z', + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'owner', + status: 'failed', + summary: + 'auth-expired: All Codex rotation accounts unavailable; re-auth required before launching Codex\nExecution completed without a visible terminal verdict.', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'completed', + arbiter_verdict: 'escalate', + arbiter_requested_at: null, + completion_reason: 'owner_codex_unavailable', + }), + ); + }); + it('keeps arbiter REVISE on owner flow while clearing stale loop counters', () => { vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({ diff --git a/src/task-scheduler.test.ts b/src/task-scheduler.test.ts index 298b727..ca8a1fd 100644 --- a/src/task-scheduler.test.ts +++ b/src/task-scheduler.test.ts @@ -143,7 +143,12 @@ vi.mock('./service-routing.js', () => ({ hasReviewerLease: vi.fn(() => false), })); -import { _initTestDatabase, createTask, getTaskById } from './db.js'; +import { + _initTestDatabase, + createPairedTask, + createTask, + getTaskById, +} from './db.js'; import * as codexTokenRotation from './codex-token-rotation.js'; import { TIMEZONE } from './config.js'; import * as serviceRouting from './service-routing.js'; @@ -304,6 +309,52 @@ describe('task scheduler', () => { ); }); + it('requeues an orphaned review_ready paired task once its CI watcher is gone', async () => { + createPairedTask({ + id: 'paired-review-ready-orphan', + chat_jid: 'review@g.us', + group_folder: 'review-group', + owner_service_id: 'codex-main', + reviewer_service_id: 'claude', + owner_agent_type: 'codex', + reviewer_agent_type: 'claude-code', + arbiter_agent_type: null, + title: null, + source_ref: 'HEAD', + plan_notes: null, + review_requested_at: null, + round_trip_count: 1, + status: 'review_ready', + arbiter_verdict: null, + arbiter_requested_at: null, + completion_reason: null, + owner_failure_count: 0, + created_at: '2026-02-22T00:00:00.000Z', + updated_at: '2026-02-22T00:00:01.000Z', + }); + + const enqueueMessageCheck = vi.fn(); + const deps = { + serviceAgentType: 'codex' as const, + roomBindings: () => ({}), + getSessions: () => ({}), + queue: { enqueueTask: vi.fn(), enqueueMessageCheck } as any, + onProcess: () => {}, + sendMessage: async () => {}, + }; + + await runSchedulerTickOnce(deps); + + expect(enqueueMessageCheck).toHaveBeenCalledTimes(1); + expect(enqueueMessageCheck).toHaveBeenCalledWith( + 'review@g.us', + expect.stringContaining('review-group'), + ); + + await runSchedulerTickOnce(deps); + expect(enqueueMessageCheck).toHaveBeenCalledTimes(1); + }); + it('can execute one scheduler tick without starting the timer loop', async () => { const dueAt = new Date(Date.now() - 60_000).toISOString(); createTask({ diff --git a/src/task-scheduler.ts b/src/task-scheduler.ts index b79dedd..0d54daa 100644 --- a/src/task-scheduler.ts +++ b/src/task-scheduler.ts @@ -12,8 +12,10 @@ import { ASSISTANT_NAME, SCHEDULER_POLL_INTERVAL, TIMEZONE } from './config.js'; import { AgentOutput, runAgentProcess } from './agent-runner.js'; import { getAllTasks, + getAllOpenPairedTasks, deleteTask, getDueTasks, + hasActiveCiWatcherForChat, getTaskById, logTaskRun, updateTask, @@ -44,6 +46,8 @@ import { } from './task-suspension.js'; import { getTaskQueueJid, isGitHubCiTask } from './task-watch-status.js'; import { ScheduledTask } from './types.js'; +import { resolveGroupIpcPath } from './group-folder.js'; +import { schedulePairedFollowUpOnce } from './paired-follow-up-scheduler.js'; import { hasTaskExceededMaxDuration, resolveTaskExecutionContext, @@ -506,6 +510,7 @@ export async function runSchedulerTickOnce( ): Promise { // Unified service: process all agent types, not just the service default. const nowMs = Date.now(); + reconcileOrphanedPairedReviewReadyTasks(deps); const activeTasks = getAllTasks().filter((task) => task.status === 'active'); for (const task of activeTasks) { @@ -550,6 +555,45 @@ export async function runSchedulerTickOnce( } } +function reconcileOrphanedPairedReviewReadyTasks( + deps: SchedulerDependencies, +): void { + for (const task of getAllOpenPairedTasks()) { + if (task.status !== 'review_ready') { + continue; + } + if (hasActiveCiWatcherForChat(task.chat_jid)) { + continue; + } + + const scheduled = schedulePairedFollowUpOnce({ + chatJid: task.chat_jid, + runId: `scheduler-review-ready-${Date.now().toString(36)}`, + task, + intentKind: 'reviewer-turn', + enqueue: () => + deps.queue.enqueueMessageCheck( + task.chat_jid, + resolveGroupIpcPath(task.group_folder), + ), + }); + + if (!scheduled) { + continue; + } + + logger.info( + { + taskId: task.id, + chatJid: task.chat_jid, + groupFolder: task.group_folder, + status: task.status, + }, + 'Re-queued orphaned review_ready paired task without active CI watcher', + ); + } +} + let schedulerRunning = false; let schedulerTimer: ReturnType | null = null; let schedulerLoopFn: (() => Promise) | null = null;