fix(FN-2054): harden agent loop stuck-detection and self-healing

- Add defensive cleanup for in-memory task tracking when tasks move state or agents pause
- Improve stuck-detection and self-healing flow to reduce leaked state and missed recovery paths
- Add logging around previously swallowed errors and tighten executor cleanup behavior
- Expand restart and self-healing reliability tests to cover regression scenarios
This commit is contained in:
Fusion
2026-04-18 11:08:41 -07:00
committed by gsxdsm
parent eb04fd148d
commit 3977b38895
8 changed files with 290 additions and 91 deletions

View File

@@ -700,6 +700,15 @@ describe("mission-interview module", () => {
expect(agentConfig).toHaveProperty("onThinking"); expect(agentConfig).toHaveProperty("onThinking");
expect(agentConfig).toHaveProperty("onText"); expect(agentConfig).toHaveProperty("onText");
// Verify stream callbacks use the active session id
const broadcastSpy = vi.spyOn(missionInterviewStreamManager, "broadcast").mockReturnValue(1);
agentConfig.onText("Hello");
expect(broadcastSpy).toHaveBeenCalledWith(sessionId, { type: "text", data: "Hello" });
agentConfig.onThinking("thinking...");
expect(broadcastSpy).toHaveBeenCalledWith(sessionId, { type: "thinking", data: "thinking..." });
broadcastSpy.mockRestore();
// Verify no unexpected model override fields // Verify no unexpected model override fields
expect(agentConfig).not.toHaveProperty("modelProvider"); expect(agentConfig).not.toHaveProperty("modelProvider");
expect(agentConfig).not.toHaveProperty("modelId"); expect(agentConfig).not.toHaveProperty("modelId");

View File

@@ -171,6 +171,7 @@ export type MissionInterviewResponse =
/** SSE event types for mission interview streaming */ /** SSE event types for mission interview streaming */
export type MissionInterviewStreamEvent = export type MissionInterviewStreamEvent =
| { type: "thinking"; data: string } | { type: "thinking"; data: string }
| { type: "text"; data: string }
| { type: "question"; data: PlanningQuestion } | { type: "question"; data: PlanningQuestion }
| { type: "summary"; data: MissionPlanSummary } | { type: "summary"; data: MissionPlanSummary }
| { type: "error"; data: string } | { type: "error"; data: string }
@@ -777,6 +778,10 @@ async function createMissionInterviewAgent(
}, },
onText: (delta: string) => { onText: (delta: string) => {
session.thinkingOutput += delta; session.thinkingOutput += delta;
missionInterviewStreamManager.broadcast(session.id, {
type: "text",
data: delta,
});
}, },
}); });
} }

View File

@@ -297,7 +297,27 @@ export class HeartbeatMonitor {
*/ */
async withAgentStartLock<T>(agentId: string, fn: () => Promise<T>): Promise<T> { async withAgentStartLock<T>(agentId: string, fn: () => Promise<T>): Promise<T> {
const existing = this.agentStartLocks.get(agentId) ?? Promise.resolve(); const existing = this.agentStartLocks.get(agentId) ?? Promise.resolve();
const operation = existing.then(fn, fn); const operation = existing.then(
async () => {
try {
return await fn();
} finally {
// Clean up accumulated run state for this agent at end of each serialized run.
// This guarantees cleanup even when the run path throws without calling completeRun
// (e.g., execution error before completeRun is reached, or completeRun itself throws).
// Because withAgentStartLock serializes runs per agent, the finally runs after each
// run completes but before the next concurrent call's callback starts.
this.clearRunState(agentId);
}
},
async (err) => {
try {
throw err;
} finally {
this.clearRunState(agentId);
}
},
);
this.agentStartLocks.set(agentId, operation); this.agentStartLocks.set(agentId, operation);
return operation as Promise<T>; return operation as Promise<T>;
} }
@@ -310,6 +330,39 @@ export class HeartbeatMonitor {
* @returns The created run * @returns The created run
*/ */
async startRun(agentId: string, options?: WakeupOptions): Promise<AgentHeartbeatRun> { async startRun(agentId: string, options?: WakeupOptions): Promise<AgentHeartbeatRun> {
// Safety net: fail any existing active runs for this agent before creating a new one.
// This prevents accumulation of zombie runs when startRun is called multiple times
// (e.g., concurrent timer + on-demand triggers, or retries after crashes).
try {
const existingRun = await this.store.getActiveHeartbeatRun(agentId);
if (existingRun) {
heartbeatLog.warn(
`Agent ${agentId} has active run ${existingRun.id} — marking failed before starting new run`,
);
try {
const existingDetail = await this.store.getRunDetail(agentId, existingRun.id);
if (existingDetail) {
await this.store.saveRun({
...existingDetail,
endedAt: new Date().toISOString(),
status: "terminated",
stderrExcerpt: "Superseded by new heartbeat run (previous run was stale)",
});
}
await this.store.endHeartbeatRun(existingRun.id, "terminated");
this.clearRunState(agentId);
} catch (failErr) {
const failErrMessage = failErr instanceof Error ? failErr.message : String(failErr);
heartbeatLog.warn(
`Failed to terminate stale active run ${existingRun.id} for ${agentId}: ${failErrMessage} — continuing anyway`,
);
}
}
} catch (activeRunCheckErr) {
const msg = activeRunCheckErr instanceof Error ? activeRunCheckErr.message : String(activeRunCheckErr);
heartbeatLog.warn(`Failed to check for existing active run for ${agentId}: ${msg} — continuing with new run`);
}
const run = await this.store.startHeartbeatRun(agentId); const run = await this.store.startHeartbeatRun(agentId);
// Enrich with execution context // Enrich with execution context
@@ -327,8 +380,8 @@ export class HeartbeatMonitor {
// Transition agent to running state // Transition agent to running state
try { try {
await this.store.updateAgentState(agentId, "running"); await this.store.updateAgentState(agentId, "running");
} catch { } catch (startRunErr) {
// May fail if already in running state - that's ok heartbeatLog.warn(`updateAgentState(running) failed for ${agentId}: ${startRunErr instanceof Error ? startRunErr.message : String(startRunErr)} — continuing`);
} }
this.onRunStarted?.(agentId, enrichedRun); this.onRunStarted?.(agentId, enrichedRun);
@@ -384,7 +437,10 @@ export class HeartbeatMonitor {
await this.store.saveRun(completedRun); await this.store.saveRun(completedRun);
// Clear accumulated run state for this agent // Clear accumulated run state for this agent.
// Safe to call even when runCreatedTasks was already cleared by withAgentStartLock's
// finally block (idempotent Map.delete), and necessary for direct completeRun calls
// that bypass the lock (e.g., test scenarios, edge-case error paths).
this.clearRunState(agentId); this.clearRunState(agentId);
// Update cumulative usage on agent // Update cumulative usage on agent
@@ -397,8 +453,8 @@ export class HeartbeatMonitor {
totalOutputTokens: (agent.totalOutputTokens ?? 0) + completionResult.usageJson.outputTokens, totalOutputTokens: (agent.totalOutputTokens ?? 0) + completionResult.usageJson.outputTokens,
}); });
} }
} catch { } catch (usageUpdateErr) {
// Non-critical, skip heartbeatLog.warn(`Agent ${agentId} usage update failed: ${usageUpdateErr instanceof Error ? usageUpdateErr.message : String(usageUpdateErr)} — continuing`);
} }
} }
@@ -413,8 +469,8 @@ export class HeartbeatMonitor {
// Skip the normal state transition below since we already set the correct state // Skip the normal state transition below since we already set the correct state
completionResult = { ...completionResult, skipStateTransition: true }; completionResult = { ...completionResult, skipStateTransition: true };
} }
} catch { } catch (budgetCheckErr) {
// If budget check fails, proceed with normal state transition heartbeatLog.warn(`Agent ${agentId} budget check failed: ${budgetCheckErr instanceof Error ? budgetCheckErr.message : String(budgetCheckErr)} — proceeding with normal state transition`);
} }
} }
@@ -430,8 +486,8 @@ export class HeartbeatMonitor {
// Completed successfully - back to active // Completed successfully - back to active
await this.store.updateAgentState(agentId, "active"); await this.store.updateAgentState(agentId, "active");
} }
} catch { } catch (stateTransErr) {
// State transition may fail if already in target state heartbeatLog.warn(`Agent ${agentId} state transition failed: ${stateTransErr instanceof Error ? stateTransErr.message : String(stateTransErr)} — continuing`);
} }
} }
@@ -470,8 +526,8 @@ export class HeartbeatMonitor {
try { try {
await this.store.updateAgentState(agentId, "active"); await this.store.updateAgentState(agentId, "active");
} catch { } catch (stopStateErr) {
// Best effort — if already active or transition is currently invalid, ignore. heartbeatLog.warn(`Agent ${agentId} updateAgentState(active) failed during stop: ${stopStateErr instanceof Error ? stopStateErr.message : String(stopStateErr)}`);
} }
this.clearRunState(agentId); this.clearRunState(agentId);
@@ -500,8 +556,8 @@ export class HeartbeatMonitor {
try { try {
await this.store.updateAgentState(agentId, "active"); await this.store.updateAgentState(agentId, "active");
} catch { } catch (stopPersistErr) {
// Best effort — if the state cannot be transitioned right now, don't fail stop semantics. heartbeatLog.warn(`Agent ${agentId} updateAgentState(active) failed during persisted-run stop: ${stopPersistErr instanceof Error ? stopPersistErr.message : String(stopPersistErr)}`);
} }
this.clearRunState(agentId); this.clearRunState(agentId);
@@ -649,8 +705,8 @@ export class HeartbeatMonitor {
let preloadedAgent: Agent | null = null; let preloadedAgent: Agent | null = null;
try { try {
preloadedAgent = await this.store.getAgent(agentId); preloadedAgent = await this.store.getAgent(agentId);
} catch { } catch (preloadErr) {
// If preloading fails, resolve again in the execution path below. heartbeatLog.warn(`Agent ${agentId} agent preloading failed: ${preloadErr instanceof Error ? preloadErr.message : String(preloadErr)} — will resolve in execution path`);
} }
const resolvedTaskId = explicitTaskId ?? preloadedAgent?.taskId; const resolvedTaskId = explicitTaskId ?? preloadedAgent?.taskId;
@@ -736,8 +792,8 @@ export class HeartbeatMonitor {
}); });
return (await this.store.getRunDetail(agentId, run.id))!; return (await this.store.getRunDetail(agentId, run.id))!;
} }
} catch { } catch (budgetErr) {
// If getBudgetStatus fails (e.g., method not available), proceed without budget check heartbeatLog.warn(`Agent ${agentId} budget status check failed: ${budgetErr instanceof Error ? budgetErr.message : String(budgetErr)} — proceeding without budget check`);
} }
// Resolve agent // Resolve agent
@@ -782,8 +838,8 @@ export class HeartbeatMonitor {
await checkoutTask.call(taskStore, taskId, agentId, runContext); await checkoutTask.call(taskStore, taskId, agentId, runContext);
// Audit trail: record checkout mutation (FN-1404) // Audit trail: record checkout mutation (FN-1404)
await audit.database({ type: "task:checkout", target: taskId }); await audit.database({ type: "task:checkout", target: taskId });
} catch { } catch (checkoutErr) {
heartbeatLog.log(`Task ${taskId} already checked out — skipping`); heartbeatLog.warn(`Task ${taskId} checkout failed: ${checkoutErr instanceof Error ? checkoutErr.message : String(checkoutErr)} — skipping`);
taskId = undefined; taskId = undefined;
inboxSelection = null; inboxSelection = null;
} }
@@ -843,8 +899,8 @@ export class HeartbeatMonitor {
const resolvedTaskId = taskId!; const resolvedTaskId = taskId!;
try { try {
taskDetail = await taskStore.getTask(resolvedTaskId); taskDetail = await taskStore.getTask(resolvedTaskId);
} catch { } catch (taskDetailErr) {
heartbeatLog.warn(`Task ${resolvedTaskId} not found — graceful exit`); heartbeatLog.warn(`Task ${resolvedTaskId} fetch failed: ${taskDetailErr instanceof Error ? taskDetailErr.message : String(taskDetailErr)} — graceful exit`);
await this.completeRun(agentId, run.id, { await this.completeRun(agentId, run.id, {
status: "completed", status: "completed",
resultJson: { reason: "task_not_found", taskId: resolvedTaskId }, resultJson: { reason: "task_not_found", taskId: resolvedTaskId },
@@ -1069,8 +1125,8 @@ export class HeartbeatMonitor {
if (triggerDetail === "wake-on-message" && this.messageStore) { if (triggerDetail === "wake-on-message" && this.messageStore) {
try { try {
pendingMessages = this.messageStore.getInbox(agentId, "agent", { read: false, limit: 10 }); pendingMessages = this.messageStore.getInbox(agentId, "agent", { read: false, limit: 10 });
} catch { } catch (inboxErr) {
heartbeatLog.warn(`Failed to fetch inbox messages for ${agentId} during wake-on-message`); heartbeatLog.warn(`Failed to fetch inbox messages for ${agentId} during wake-on-message: ${inboxErr instanceof Error ? inboxErr.message : String(inboxErr)}`);
} }
} }
@@ -1124,8 +1180,8 @@ export class HeartbeatMonitor {
if (triggerDetail === "wake-on-message" && this.messageStore) { if (triggerDetail === "wake-on-message" && this.messageStore) {
try { try {
pendingMessages = this.messageStore.getInbox(agentId, "agent", { read: false, limit: 10 }); pendingMessages = this.messageStore.getInbox(agentId, "agent", { read: false, limit: 10 });
} catch { } catch (inboxErr) {
heartbeatLog.warn(`Failed to fetch inbox messages for ${agentId} during wake-on-message`); heartbeatLog.warn(`Failed to fetch inbox messages for ${agentId} during wake-on-message: ${inboxErr instanceof Error ? inboxErr.message : String(inboxErr)}`);
} }
} }
@@ -1198,9 +1254,8 @@ export class HeartbeatMonitor {
if (pendingMessages.length > 0 && this.messageStore) { if (pendingMessages.length > 0 && this.messageStore) {
try { try {
this.messageStore.markAllAsRead(agentId, "agent"); this.messageStore.markAllAsRead(agentId, "agent");
} catch { } catch (markReadErr) {
// Non-critical — mark as read failed, messages remain unread heartbeatLog.warn(`Failed to mark messages as read for ${agentId}: ${markReadErr instanceof Error ? markReadErr.message : String(markReadErr)}`);
heartbeatLog.warn(`Failed to mark messages as read for ${agentId}`);
} }
} }
@@ -1237,7 +1292,11 @@ export class HeartbeatMonitor {
}); });
} finally { } finally {
await flushAgentLogger(); await flushAgentLogger();
this.untrackAgent(agentId); // Defensively untrack the agent — wrap in try/catch to guarantee cleanup
// can't be blocked by an exception in untrackAgent itself.
try { this.untrackAgent(agentId); } catch (untrackErr) {
heartbeatLog.warn(`untrackAgent failed for ${agentId}: ${untrackErr instanceof Error ? untrackErr.message : String(untrackErr)}`);
}
try { session.dispose(); } catch { /* ignore */ } try { session.dispose(); } catch { /* ignore */ }
} }
@@ -1247,14 +1306,39 @@ export class HeartbeatMonitor {
heartbeatLog.error(`Heartbeat execution error for ${agentId}: ${errorMessage}`); heartbeatLog.error(`Heartbeat execution error for ${agentId}: ${errorMessage}`);
await flushAgentLogger(); await flushAgentLogger();
// Attempt to complete the run as failed if it's still active // Attempt to complete the run as failed if it's still active.
// If completeRun also fails, fall back to a direct DB update to ensure
// the run is not permanently stuck in "active" state.
try { try {
await this.completeRun(agentId, run.id, { await this.completeRun(agentId, run.id, {
status: "failed", status: "failed",
stderrExcerpt: errorMessage, stderrExcerpt: errorMessage,
}); });
} catch { } catch (completeRunErr) {
// If completeRun also fails, the run remains active — nothing more we can do const completeRunErrMsg = completeRunErr instanceof Error ? completeRunErr.message : String(completeRunErr);
heartbeatLog.error(`completeRun failed for ${agentId}/${run.id}: ${completeRunErrMsg} — attempting safety-net completion`);
// Safety net: directly update the run record to prevent zombie run state.
// This runs only when completeRun itself threw, guaranteeing the run
// doesn't remain permanently stuck in "active" state.
try {
const runDetail = await this.store.getRunDetail(agentId, run.id);
if (runDetail && runDetail.status !== "completed" && runDetail.status !== "failed" && runDetail.status !== "terminated") {
await this.store.saveRun({
...runDetail,
endedAt: new Date().toISOString(),
status: "failed",
stderrExcerpt: `Heartbeat execution failed: ${errorMessage}. Run completion also failed: ${completeRunErrMsg}`,
});
await this.store.endHeartbeatRun(run.id, "terminated");
// Also clean up run state accumulator
this.clearRunState(agentId);
heartbeatLog.log(`Safety-net run completion for ${agentId}/${run.id} — run terminated`);
}
} catch (safetyNetErr) {
const safetyNetErrMsg = safetyNetErr instanceof Error ? safetyNetErr.message : String(safetyNetErr);
heartbeatLog.error(`Safety-net run completion also failed for ${agentId}/${run.id}: ${safetyNetErrMsg} — run may be stuck permanently`);
}
} }
return (await this.store.getRunDetail(agentId, run.id))!; return (await this.store.getRunDetail(agentId, run.id))!;
@@ -1306,8 +1390,8 @@ export class HeartbeatMonitor {
// Log agent link on the created task with run context for correlation // Log agent link on the created task with run context for correlation
try { try {
await taskStore.logEntry(createdTaskId, `Created by agent ${agentId} during heartbeat run`, undefined, runContext); await taskStore.logEntry(createdTaskId, `Created by agent ${agentId} during heartbeat run`, undefined, runContext);
} catch { } catch (taskCreateLogErr) {
// Non-critical — task was created, just the log failed heartbeatLog.warn(`Task ${createdTaskId} agent-link log failed: ${taskCreateLogErr instanceof Error ? taskCreateLogErr.message : String(taskCreateLogErr)}`);
} }
// Audit trail: record task creation (FN-1404) // Audit trail: record task creation (FN-1404)
@@ -1395,8 +1479,8 @@ export class HeartbeatMonitor {
result.maxConcurrentRuns = Math.max(1, Math.round(rc.maxConcurrentRuns)); result.maxConcurrentRuns = Math.max(1, Math.round(rc.maxConcurrentRuns));
} }
} }
} catch { } catch (agentLookupErr) {
// If agent lookup fails, use monitor defaults heartbeatLog.warn(`getAgentConfig(${agentId}) agent lookup failed: ${agentLookupErr instanceof Error ? agentLookupErr.message : String(agentLookupErr)} — using monitor defaults`);
} }
return result; return result;
@@ -1660,8 +1744,8 @@ export class HeartbeatTriggerScheduler {
heartbeatLog.log(`Agent ${agent.id} budget exhausted — assignment trigger skipped`); heartbeatLog.log(`Agent ${agent.id} budget exhausted — assignment trigger skipped`);
return; return;
} }
} catch { } catch (budgetErr) {
// If getBudgetStatus fails, proceed without budget check heartbeatLog.warn(`Assignment trigger budget check failed for ${agent.id}: ${budgetErr instanceof Error ? budgetErr.message : String(budgetErr)} — proceeding without budget check`);
} }
let triggeringCommentIds: string[] | undefined; let triggeringCommentIds: string[] | undefined;
@@ -1745,8 +1829,8 @@ export class HeartbeatTriggerScheduler {
heartbeatLog.log(`Agent ${agentId} over budget threshold (${budgetStatus.usagePercent}%) — timer tick skipped`); heartbeatLog.log(`Agent ${agentId} over budget threshold (${budgetStatus.usagePercent}%) — timer tick skipped`);
return; return;
} }
} catch { } catch (budgetErr) {
// If getBudgetStatus fails, proceed without budget check heartbeatLog.warn(`Timer tick budget check failed for ${agentId}: ${budgetErr instanceof Error ? budgetErr.message : String(budgetErr)} — proceeding without budget check`);
} }
await this.callback(agentId, "timer", { await this.callback(agentId, "timer", {

View File

@@ -513,6 +513,13 @@ export class TaskExecutor {
); );
this.activeStepExecutors.delete(task.id); this.activeStepExecutors.delete(task.id);
} }
// Clean up all in-memory state for this task so nothing leaks across runs.
// This prevents zombie state from persisting when a task moves away from
// in-progress while execute() is still unwinding, or when the scheduler
// moves a task out before the executor's task:moved fires.
this.loopRecoveryState.delete(task.id);
this.spawnedAgents.delete(task.id);
this.stuckAborted.delete(task.id);
} }
}); });
@@ -535,6 +542,10 @@ export class TaskExecutor {
this.options.stuckTaskDetector?.untrackTask(task.id); this.options.stuckTaskDetector?.untrackTask(task.id);
const { session } = this.activeSessions.get(task.id)!; const { session } = this.activeSessions.get(task.id)!;
session.dispose(); session.dispose();
// Also clean up in-memory state to prevent leaks if the task is later unpaused
this.loopRecoveryState.delete(task.id);
this.spawnedAgents.delete(task.id);
this.stuckAborted.delete(task.id);
return; return;
} }
if (task.paused && this.activeStepExecutors.has(task.id)) { if (task.paused && this.activeStepExecutors.has(task.id)) {
@@ -543,6 +554,10 @@ export class TaskExecutor {
this.options.stuckTaskDetector?.untrackTask(task.id); this.options.stuckTaskDetector?.untrackTask(task.id);
const stepExecutor = this.activeStepExecutors.get(task.id)!; const stepExecutor = this.activeStepExecutors.get(task.id)!;
await stepExecutor.terminateAllSessions(); await stepExecutor.terminateAllSessions();
// Also clean up in-memory state to prevent leaks if the task is later unpaused
this.loopRecoveryState.delete(task.id);
this.spawnedAgents.delete(task.id);
this.stuckAborted.delete(task.id);
return; return;
} }
@@ -556,7 +571,9 @@ export class TaskExecutor {
try { try {
await this.clearResumeFailureState(task); await this.clearResumeFailureState(task);
await this.store.logEntry(task.id, "Resuming execution after unpause", undefined, this.currentRunContext); await this.store.logEntry(task.id, "Resuming execution after unpause", undefined, this.currentRunContext);
} catch { /* non-critical */ } } catch (clearErr) {
executorLog.warn(`${task.id} clearResumeFailureState failed during unpause: ${clearErr instanceof Error ? clearErr.message : String(clearErr)}`);
}
this.execute(task).catch((err) => this.execute(task).catch((err) =>
executorLog.error(`Failed to resume unpaused ${task.id}:`, err), executorLog.error(`Failed to resume unpaused ${task.id}:`, err),
); );
@@ -677,6 +694,10 @@ export class TaskExecutor {
this.pausedAborted.add(taskId); this.pausedAborted.add(taskId);
this.options.stuckTaskDetector?.untrackTask(taskId); this.options.stuckTaskDetector?.untrackTask(taskId);
session.dispose(); session.dispose();
// Clean up all in-memory state so nothing leaks when tasks are later unpaused
this.loopRecoveryState.delete(taskId);
this.spawnedAgents.delete(taskId);
this.stuckAborted.delete(taskId);
} }
for (const [taskId, stepExecutor] of this.activeStepExecutors) { for (const [taskId, stepExecutor] of this.activeStepExecutors) {
executorLog.log(`Global pause — terminating step sessions for ${taskId}`); executorLog.log(`Global pause — terminating step sessions for ${taskId}`);
@@ -685,6 +706,10 @@ export class TaskExecutor {
stepExecutor.terminateAllSessions().catch(err => stepExecutor.terminateAllSessions().catch(err =>
executorLog.warn(`Failed to terminate step sessions for global pause ${taskId}: ${err}`) executorLog.warn(`Failed to terminate step sessions for global pause ${taskId}: ${err}`)
); );
// Clean up all in-memory state so nothing leaks when tasks are later unpaused
this.loopRecoveryState.delete(taskId);
this.spawnedAgents.delete(taskId);
this.stuckAborted.delete(taskId);
} }
} }
}); });
@@ -1470,9 +1495,14 @@ export class TaskExecutor {
} finally { } finally {
this.executing.delete(task.id); this.executing.delete(task.id);
this.loopRecoveryState.delete(task.id); this.loopRecoveryState.delete(task.id);
await stepExecutor.cleanup().catch(cleanupErr => // Wrap cleanup in try/catch so activeStepExecutors.delete() always runs.
executorLog.warn(`StepSessionExecutor cleanup failed for ${task.id}: ${cleanupErr}`) // If cleanup() throws, the executor continues to clean up the in-memory map
); // and requeue logic without leaking the reference.
try {
await stepExecutor.cleanup();
} catch (cleanupErr) {
executorLog.warn(`StepSessionExecutor cleanup failed for ${task.id}: ${cleanupErr instanceof Error ? cleanupErr.message : String(cleanupErr)}`);
}
this.activeStepExecutors.delete(task.id); this.activeStepExecutors.delete(task.id);
// Stuck-requeue: clean up worktree and move to todo // Stuck-requeue: clean up worktree and move to todo
@@ -2137,6 +2167,16 @@ export class TaskExecutor {
// Clear run context at end of execute() lifecycle // Clear run context at end of execute() lifecycle
this.currentRunContext = undefined; this.currentRunContext = undefined;
// Terminate all spawned child agents on ALL exit paths.
// This must run here (in the outer finally) rather than only in agentWork's
// finally block, because failures during worktree creation or before
// agentWork is entered leave children orphaned with no other cleanup path.
try {
await this.terminateAllChildren(task.id);
} catch (err) {
executorLog.warn(`terminateAllChildren failed for ${task.id}: ${err instanceof Error ? err.message : String(err)}`);
}
// Reset loop recovery state at end of execute() lifecycle. // Reset loop recovery state at end of execute() lifecycle.
// State is in-memory and per-run — should not persist across attempts. // State is in-memory and per-run — should not persist across attempts.
this.loopRecoveryState.delete(task.id); this.loopRecoveryState.delete(task.id);
@@ -4131,12 +4171,16 @@ and show an appropriate message to the user.\`
// Normal completion — mark as active (available) // Normal completion — mark as active (available)
try { try {
await this.options.agentStore?.updateAgentState(agentId, "active"); await this.options.agentStore?.updateAgentState(agentId, "active");
} catch { /* non-critical */ } } catch (markActiveErr) {
executorLog.warn(`Child agent ${agentId} updateAgentState(active) failed: ${markActiveErr instanceof Error ? markActiveErr.message : String(markActiveErr)}`);
}
} catch (err: unknown) { } catch (err: unknown) {
// Error during execution — mark as error // Error during execution — mark as error
try { try {
await this.options.agentStore?.updateAgentState(agentId, "error"); await this.options.agentStore?.updateAgentState(agentId, "error");
} catch { /* non-critical */ } } catch (markErrorErr) {
executorLog.warn(`Child agent ${agentId} updateAgentState(error) failed: ${markErrorErr instanceof Error ? markErrorErr.message : String(markErrorErr)}`);
}
const errorMessage = err instanceof Error ? err.message : String(err); const errorMessage = err instanceof Error ? err.message : String(err);
executorLog.warn(`Child agent ${agentId} failed: ${errorMessage}`); executorLog.warn(`Child agent ${agentId} failed: ${errorMessage}`);
} finally { } finally {

View File

@@ -738,6 +738,8 @@ describe("Scheduler after restart", () => {
const todoTask = makeTask("FN-070", "todo"); const todoTask = makeTask("FN-070", "todo");
store.listTasks.mockResolvedValue([todoTask]); store.listTasks.mockResolvedValue([todoTask]);
store.getSettings.mockResolvedValue({ ...DEFAULT_SETTINGS }); store.getSettings.mockResolvedValue({ ...DEFAULT_SETTINGS });
// Mock getTask for compare-and-swap verification in schedule()
store.getTask.mockResolvedValue(todoTask);
// Needs parseFileScopeFromPrompt for overlap checks // Needs parseFileScopeFromPrompt for overlap checks
store.parseFileScopeFromPrompt.mockResolvedValue([]); store.parseFileScopeFromPrompt.mockResolvedValue([]);
@@ -1320,6 +1322,8 @@ describe("Engine pause/unpause cycle", () => {
store.listTasks.mockResolvedValue([todoTask]); store.listTasks.mockResolvedValue([todoTask]);
store.getSettings.mockResolvedValue({ ...DEFAULT_SETTINGS, enginePaused: false }); store.getSettings.mockResolvedValue({ ...DEFAULT_SETTINGS, enginePaused: false });
store.parseFileScopeFromPrompt.mockResolvedValue([]); store.parseFileScopeFromPrompt.mockResolvedValue([]);
// Mock getTask for compare-and-swap verification in schedule()
store.getTask.mockResolvedValue(todoTask);
const onSchedule = vi.fn(); const onSchedule = vi.fn();
const scheduler = new Scheduler(store, { const scheduler = new Scheduler(store, {

View File

@@ -670,6 +670,21 @@ export class Scheduler {
reservedWorktreeNames, reservedWorktreeNames,
); );
// Compare-and-swap: re-read the task to verify it's still in "todo" before dispatching.
// This prevents dispatching a task twice if another schedule() call or user action
// moved it away from "todo" between our initial snapshot and this dispatch attempt.
// The re-entrance guard prevents overlapping schedule() passes, but external events
// (user moves, API calls) can still trigger concurrent state changes.
const freshTask = await this.store.getTask(task.id);
if (!freshTask || freshTask.column !== "todo") {
schedulerLog.log(`Task ${task.id} no longer in "todo" (column=${freshTask?.column ?? "N/A"}) — skipping dispatch`);
continue;
}
if (freshTask.paused) {
schedulerLog.log(`Task ${task.id} is paused — skipping dispatch`);
continue;
}
// Clear status, reserve worktree path, and then move to in-progress // Clear status, reserve worktree path, and then move to in-progress
schedulerLog.log(`Starting ${task.id}: ${task.title || task.id} (deps satisfied)`); schedulerLog.log(`Starting ${task.id}: ${task.title || task.id} (deps satisfied)`);
await this.store.updateTask(task.id, { await this.store.updateTask(task.id, {

View File

@@ -2094,12 +2094,14 @@ describe("maintenance cycle concurrency", () => {
expect((manager as any).maintenanceRunning).toBe(false); expect((manager as any).maintenanceRunning).toBe(false);
}); });
it("runs batch 1 operations in parallel", async () => { it("runs batch 1 operations in sequence (with isolation — one failure doesn't block others)", async () => {
let runningCount = 0; let runningCount = 0;
let maxConcurrent = 0; let maxConcurrent = 0;
let executionOrder: string[] = [];
const makeSlow = (label: string) => const makeSlow = (label: string) =>
(vi.spyOn(manager as any, label).mockImplementation(async () => { (vi.spyOn(manager as any, label).mockImplementation(async () => {
executionOrder.push(label);
runningCount++; runningCount++;
maxConcurrent = Math.max(maxConcurrent, runningCount); maxConcurrent = Math.max(maxConcurrent, runningCount);
await vi.advanceTimersByTimeAsync(10); await vi.advanceTimersByTimeAsync(10);
@@ -2115,16 +2117,25 @@ describe("maintenance cycle concurrency", () => {
await (manager as any).runMaintenance(); await (manager as any).runMaintenance();
// If they ran in parallel, maxConcurrent should be > 1 // Operations run sequentially (one at a time), not in parallel.
expect(maxConcurrent).toBeGreaterThan(1); // This is intentional — each step is isolated so one failure doesn't
// block or race with the others.
expect(maxConcurrent).toBe(1);
// All operations should have run
expect(executionOrder).toContain("pruneWorktrees");
expect(executionOrder).toContain("cleanupOrphans");
expect(executionOrder).toContain("cleanupOrphanedBranches");
expect(executionOrder).toContain("enforceWorktreeCap");
}); });
it("runs batch 2 operations in parallel", async () => { it("runs batch 2 operations in sequence (with isolation — one failure doesn't block others)", async () => {
let runningCount = 0; let runningCount = 0;
let maxConcurrent = 0; let maxConcurrent = 0;
let executionOrder: string[] = [];
const makeSlow = (label: string) => const makeSlow = (label: string) =>
(vi.spyOn(manager as any, label).mockImplementation(async () => { (vi.spyOn(manager as any, label).mockImplementation(async () => {
executionOrder.push(label);
runningCount++; runningCount++;
maxConcurrent = Math.max(maxConcurrent, runningCount); maxConcurrent = Math.max(maxConcurrent, runningCount);
await vi.advanceTimersByTimeAsync(10); await vi.advanceTimersByTimeAsync(10);
@@ -2145,7 +2156,10 @@ describe("maintenance cycle concurrency", () => {
await (manager as any).runMaintenance(); await (manager as any).runMaintenance();
expect(maxConcurrent).toBeGreaterThan(1); // Operations run sequentially (one at a time), not in parallel.
expect(maxConcurrent).toBe(1);
// All operations should have run (including last one)
expect(executionOrder[executionOrder.length - 1]).toBe("recoverOrphanedSpecifyingTasks");
}); });
it("one failing batch 2 operation does not abort the batch", async () => { it("one failing batch 2 operation does not abort the batch", async () => {

View File

@@ -148,15 +148,28 @@ export class SelfHealingManager {
* stale in-progress/specifying tasks that no longer have a live worker. * stale in-progress/specifying tasks that no longer have a live worker.
*/ */
async runStartupRecovery(): Promise<void> { async runStartupRecovery(): Promise<void> {
await this.recoverNoProgressNoTaskDoneFailures(); // Each recovery step is isolated — one failure doesn't prevent subsequent steps.
await this.recoverCompletedTasks(); const steps: Array<{ name: string; fn: () => Promise<unknown> }> = [
await this.recoverStaleIncompleteReviewTasks(); { name: "no-progress-no-task-done", fn: () => this.recoverNoProgressNoTaskDoneFailures().then(() => undefined) },
await this.recoverReviewTasksWithFailedPreMergeSteps(); { name: "completed-tasks", fn: () => this.recoverCompletedTasks().then(() => undefined) },
await this.recoverInterruptedMergingTasks(); { name: "stale-incomplete-review", fn: () => this.recoverStaleIncompleteReviewTasks().then(() => undefined) },
await this.recoverMisclassifiedFailures(); { name: "failed-pre-merge-steps", fn: () => this.recoverReviewTasksWithFailedPreMergeSteps().then(() => undefined) },
await this.recoverOrphanedExecutions(); { name: "interrupted-merging", fn: () => this.recoverInterruptedMergingTasks().then(() => undefined) },
await this.recoverApprovedTriageTasks(); { name: "misclassified-failures", fn: () => this.recoverMisclassifiedFailures().then(() => undefined) },
await this.recoverOrphanedSpecifyingTasks(); { name: "orphaned-executions", fn: () => this.recoverOrphanedExecutions().then(() => undefined) },
{ name: "approved-triage", fn: () => this.recoverApprovedTriageTasks().then(() => undefined) },
{ name: "orphaned-specifying", fn: () => this.recoverOrphanedSpecifyingTasks().then(() => undefined) },
];
for (const step of steps) {
try {
await step.fn();
log.log(`Startup recovery step "${step.name}" completed`);
} catch (stepErr) {
const stepErrMessage = stepErr instanceof Error ? stepErr.message : String(stepErr);
log.error(`Startup recovery step "${step.name}" failed: ${stepErrMessage} — continuing with remaining steps`);
}
}
} }
stop(): void { stop(): void {
@@ -469,44 +482,55 @@ export class SelfHealingManager {
try { try {
// Batch 1 — Git/filesystem cleanup // Batch 1 — Git/filesystem cleanup
const batch1Results = await Promise.allSettled([ const batch1Fns: Array<{ name: string; fn: () => Promise<unknown> }> = [
this.pruneWorktrees(), { name: "prune-worktrees", fn: () => this.pruneWorktrees() },
this.cleanupOrphans(), { name: "cleanup-orphans", fn: () => this.cleanupOrphans() },
this.cleanupOrphanedBranches(), { name: "cleanup-orphaned-branches", fn: () => this.cleanupOrphanedBranches() },
Promise.resolve(this.checkpointWal()), { name: "checkpoint-wal", fn: () => Promise.resolve(this.checkpointWal()) },
this.enforceWorktreeCap(), { name: "enforce-worktree-cap", fn: () => this.enforceWorktreeCap() },
]); ];
for (const result of batch1Results) { for (const fn of batch1Fns) {
if (result.status === "rejected") { try {
log.error(`Batch 1 cleanup failed: ${result.reason}`); await fn.fn();
log.log(`Maintenance batch 1 step "${fn.name}" succeeded`);
} catch (stepErr) {
log.error(`Maintenance batch 1 step "${fn.name}" failed: ${stepErr instanceof Error ? stepErr.message : String(stepErr)}`);
} }
} }
// Batch 2 — Task recovery (operations are independent of each other) // Batch 2 — Task recovery (operations are independent of each other)
const batch2Results = await Promise.allSettled([ const batch2Fns: Array<{ name: string; fn: () => Promise<unknown> }> = [
this.recoverCompletedTasks(), { name: "recover-completed-tasks", fn: () => this.recoverCompletedTasks() },
this.recoverStaleIncompleteReviewTasks(), { name: "recover-stale-incomplete-review", fn: () => this.recoverStaleIncompleteReviewTasks() },
this.recoverReviewTasksWithFailedPreMergeSteps(), { name: "recover-failed-pre-merge-steps", fn: () => this.recoverReviewTasksWithFailedPreMergeSteps() },
this.recoverInterruptedMergingTasks(), { name: "recover-interrupted-merging", fn: () => this.recoverInterruptedMergingTasks() },
this.recoverMergeableReviewTasks(), { name: "recover-mergeable-review", fn: () => this.recoverMergeableReviewTasks() },
this.recoverMergedReviewTasks(), { name: "recover-merged-review", fn: () => this.recoverMergedReviewTasks() },
this.recoverMisclassifiedFailures(), { name: "recover-misclassified-failures", fn: () => this.recoverMisclassifiedFailures() },
this.recoverNoProgressNoTaskDoneFailures(), { name: "recover-no-progress-no-task-done", fn: () => this.recoverNoProgressNoTaskDoneFailures() },
this.recoverOrphanedExecutions(), { name: "recover-orphaned-executions", fn: () => this.recoverOrphanedExecutions() },
this.recoverApprovedTriageTasks(), { name: "recover-approved-triage", fn: () => this.recoverApprovedTriageTasks() },
this.recoverOrphanedSpecifyingTasks(), { name: "recover-orphaned-specifying", fn: () => this.recoverOrphanedSpecifyingTasks() },
]); ];
for (const result of batch2Results) { for (const fn of batch2Fns) {
if (result.status === "rejected") { try {
log.error(`Batch 2 recovery failed: ${result.reason}`); await fn.fn();
log.log(`Maintenance batch 2 step "${fn.name}" succeeded`);
} catch (stepErr) {
log.error(`Maintenance batch 2 step "${fn.name}" failed: ${stepErr instanceof Error ? stepErr.message : String(stepErr)}`);
} }
} }
// Batch 3 — Archive (runs after recovery so we don't archive recoverable tasks) // Batch 3 — Archive (runs after recovery so we don't archive recoverable tasks)
const batch3Results = await Promise.allSettled([this.archiveStaleDoneTasks()]); const batch3Fns: Array<{ name: string; fn: () => Promise<unknown> }> = [
for (const result of batch3Results) { { name: "archive-stale-done", fn: () => this.archiveStaleDoneTasks() },
if (result.status === "rejected") { ];
log.error(`Batch 3 archive failed: ${result.reason}`); for (const fn of batch3Fns) {
try {
await fn.fn();
log.log(`Maintenance batch 3 step "${fn.name}" succeeded`);
} catch (stepErr) {
log.error(`Maintenance batch 3 step "${fn.name}" failed: ${stepErr instanceof Error ? stepErr.message : String(stepErr)}`);
} }
} }