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:
@@ -700,6 +700,15 @@ describe("mission-interview module", () => {
|
||||
expect(agentConfig).toHaveProperty("onThinking");
|
||||
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
|
||||
expect(agentConfig).not.toHaveProperty("modelProvider");
|
||||
expect(agentConfig).not.toHaveProperty("modelId");
|
||||
|
||||
@@ -171,6 +171,7 @@ export type MissionInterviewResponse =
|
||||
/** SSE event types for mission interview streaming */
|
||||
export type MissionInterviewStreamEvent =
|
||||
| { type: "thinking"; data: string }
|
||||
| { type: "text"; data: string }
|
||||
| { type: "question"; data: PlanningQuestion }
|
||||
| { type: "summary"; data: MissionPlanSummary }
|
||||
| { type: "error"; data: string }
|
||||
@@ -777,6 +778,10 @@ async function createMissionInterviewAgent(
|
||||
},
|
||||
onText: (delta: string) => {
|
||||
session.thinkingOutput += delta;
|
||||
missionInterviewStreamManager.broadcast(session.id, {
|
||||
type: "text",
|
||||
data: delta,
|
||||
});
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
@@ -297,7 +297,27 @@ export class HeartbeatMonitor {
|
||||
*/
|
||||
async withAgentStartLock<T>(agentId: string, fn: () => Promise<T>): Promise<T> {
|
||||
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);
|
||||
return operation as Promise<T>;
|
||||
}
|
||||
@@ -310,6 +330,39 @@ export class HeartbeatMonitor {
|
||||
* @returns The created run
|
||||
*/
|
||||
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);
|
||||
|
||||
// Enrich with execution context
|
||||
@@ -327,8 +380,8 @@ export class HeartbeatMonitor {
|
||||
// Transition agent to running state
|
||||
try {
|
||||
await this.store.updateAgentState(agentId, "running");
|
||||
} catch {
|
||||
// May fail if already in running state - that's ok
|
||||
} catch (startRunErr) {
|
||||
heartbeatLog.warn(`updateAgentState(running) failed for ${agentId}: ${startRunErr instanceof Error ? startRunErr.message : String(startRunErr)} — continuing`);
|
||||
}
|
||||
|
||||
this.onRunStarted?.(agentId, enrichedRun);
|
||||
@@ -384,7 +437,10 @@ export class HeartbeatMonitor {
|
||||
|
||||
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);
|
||||
|
||||
// Update cumulative usage on agent
|
||||
@@ -397,8 +453,8 @@ export class HeartbeatMonitor {
|
||||
totalOutputTokens: (agent.totalOutputTokens ?? 0) + completionResult.usageJson.outputTokens,
|
||||
});
|
||||
}
|
||||
} catch {
|
||||
// Non-critical, skip
|
||||
} catch (usageUpdateErr) {
|
||||
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
|
||||
completionResult = { ...completionResult, skipStateTransition: true };
|
||||
}
|
||||
} catch {
|
||||
// If budget check fails, proceed with normal state transition
|
||||
} catch (budgetCheckErr) {
|
||||
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
|
||||
await this.store.updateAgentState(agentId, "active");
|
||||
}
|
||||
} catch {
|
||||
// State transition may fail if already in target state
|
||||
} catch (stateTransErr) {
|
||||
heartbeatLog.warn(`Agent ${agentId} state transition failed: ${stateTransErr instanceof Error ? stateTransErr.message : String(stateTransErr)} — continuing`);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -470,8 +526,8 @@ export class HeartbeatMonitor {
|
||||
|
||||
try {
|
||||
await this.store.updateAgentState(agentId, "active");
|
||||
} catch {
|
||||
// Best effort — if already active or transition is currently invalid, ignore.
|
||||
} catch (stopStateErr) {
|
||||
heartbeatLog.warn(`Agent ${agentId} updateAgentState(active) failed during stop: ${stopStateErr instanceof Error ? stopStateErr.message : String(stopStateErr)}`);
|
||||
}
|
||||
|
||||
this.clearRunState(agentId);
|
||||
@@ -500,8 +556,8 @@ export class HeartbeatMonitor {
|
||||
|
||||
try {
|
||||
await this.store.updateAgentState(agentId, "active");
|
||||
} catch {
|
||||
// Best effort — if the state cannot be transitioned right now, don't fail stop semantics.
|
||||
} catch (stopPersistErr) {
|
||||
heartbeatLog.warn(`Agent ${agentId} updateAgentState(active) failed during persisted-run stop: ${stopPersistErr instanceof Error ? stopPersistErr.message : String(stopPersistErr)}`);
|
||||
}
|
||||
|
||||
this.clearRunState(agentId);
|
||||
@@ -649,8 +705,8 @@ export class HeartbeatMonitor {
|
||||
let preloadedAgent: Agent | null = null;
|
||||
try {
|
||||
preloadedAgent = await this.store.getAgent(agentId);
|
||||
} catch {
|
||||
// If preloading fails, resolve again in the execution path below.
|
||||
} catch (preloadErr) {
|
||||
heartbeatLog.warn(`Agent ${agentId} agent preloading failed: ${preloadErr instanceof Error ? preloadErr.message : String(preloadErr)} — will resolve in execution path`);
|
||||
}
|
||||
|
||||
const resolvedTaskId = explicitTaskId ?? preloadedAgent?.taskId;
|
||||
@@ -736,8 +792,8 @@ export class HeartbeatMonitor {
|
||||
});
|
||||
return (await this.store.getRunDetail(agentId, run.id))!;
|
||||
}
|
||||
} catch {
|
||||
// If getBudgetStatus fails (e.g., method not available), proceed without budget check
|
||||
} catch (budgetErr) {
|
||||
heartbeatLog.warn(`Agent ${agentId} budget status check failed: ${budgetErr instanceof Error ? budgetErr.message : String(budgetErr)} — proceeding without budget check`);
|
||||
}
|
||||
|
||||
// Resolve agent
|
||||
@@ -782,8 +838,8 @@ export class HeartbeatMonitor {
|
||||
await checkoutTask.call(taskStore, taskId, agentId, runContext);
|
||||
// Audit trail: record checkout mutation (FN-1404)
|
||||
await audit.database({ type: "task:checkout", target: taskId });
|
||||
} catch {
|
||||
heartbeatLog.log(`Task ${taskId} already checked out — skipping`);
|
||||
} catch (checkoutErr) {
|
||||
heartbeatLog.warn(`Task ${taskId} checkout failed: ${checkoutErr instanceof Error ? checkoutErr.message : String(checkoutErr)} — skipping`);
|
||||
taskId = undefined;
|
||||
inboxSelection = null;
|
||||
}
|
||||
@@ -843,8 +899,8 @@ export class HeartbeatMonitor {
|
||||
const resolvedTaskId = taskId!;
|
||||
try {
|
||||
taskDetail = await taskStore.getTask(resolvedTaskId);
|
||||
} catch {
|
||||
heartbeatLog.warn(`Task ${resolvedTaskId} not found — graceful exit`);
|
||||
} catch (taskDetailErr) {
|
||||
heartbeatLog.warn(`Task ${resolvedTaskId} fetch failed: ${taskDetailErr instanceof Error ? taskDetailErr.message : String(taskDetailErr)} — graceful exit`);
|
||||
await this.completeRun(agentId, run.id, {
|
||||
status: "completed",
|
||||
resultJson: { reason: "task_not_found", taskId: resolvedTaskId },
|
||||
@@ -1069,8 +1125,8 @@ export class HeartbeatMonitor {
|
||||
if (triggerDetail === "wake-on-message" && this.messageStore) {
|
||||
try {
|
||||
pendingMessages = this.messageStore.getInbox(agentId, "agent", { read: false, limit: 10 });
|
||||
} catch {
|
||||
heartbeatLog.warn(`Failed to fetch inbox messages for ${agentId} during wake-on-message`);
|
||||
} catch (inboxErr) {
|
||||
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) {
|
||||
try {
|
||||
pendingMessages = this.messageStore.getInbox(agentId, "agent", { read: false, limit: 10 });
|
||||
} catch {
|
||||
heartbeatLog.warn(`Failed to fetch inbox messages for ${agentId} during wake-on-message`);
|
||||
} catch (inboxErr) {
|
||||
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) {
|
||||
try {
|
||||
this.messageStore.markAllAsRead(agentId, "agent");
|
||||
} catch {
|
||||
// Non-critical — mark as read failed, messages remain unread
|
||||
heartbeatLog.warn(`Failed to mark messages as read for ${agentId}`);
|
||||
} catch (markReadErr) {
|
||||
heartbeatLog.warn(`Failed to mark messages as read for ${agentId}: ${markReadErr instanceof Error ? markReadErr.message : String(markReadErr)}`);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1237,7 +1292,11 @@ export class HeartbeatMonitor {
|
||||
});
|
||||
} finally {
|
||||
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 */ }
|
||||
}
|
||||
|
||||
@@ -1247,14 +1306,39 @@ export class HeartbeatMonitor {
|
||||
heartbeatLog.error(`Heartbeat execution error for ${agentId}: ${errorMessage}`);
|
||||
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 {
|
||||
await this.completeRun(agentId, run.id, {
|
||||
status: "failed",
|
||||
stderrExcerpt: errorMessage,
|
||||
});
|
||||
} catch {
|
||||
// If completeRun also fails, the run remains active — nothing more we can do
|
||||
} catch (completeRunErr) {
|
||||
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))!;
|
||||
@@ -1306,8 +1390,8 @@ export class HeartbeatMonitor {
|
||||
// Log agent link on the created task with run context for correlation
|
||||
try {
|
||||
await taskStore.logEntry(createdTaskId, `Created by agent ${agentId} during heartbeat run`, undefined, runContext);
|
||||
} catch {
|
||||
// Non-critical — task was created, just the log failed
|
||||
} catch (taskCreateLogErr) {
|
||||
heartbeatLog.warn(`Task ${createdTaskId} agent-link log failed: ${taskCreateLogErr instanceof Error ? taskCreateLogErr.message : String(taskCreateLogErr)}`);
|
||||
}
|
||||
|
||||
// Audit trail: record task creation (FN-1404)
|
||||
@@ -1395,8 +1479,8 @@ export class HeartbeatMonitor {
|
||||
result.maxConcurrentRuns = Math.max(1, Math.round(rc.maxConcurrentRuns));
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
// If agent lookup fails, use monitor defaults
|
||||
} catch (agentLookupErr) {
|
||||
heartbeatLog.warn(`getAgentConfig(${agentId}) agent lookup failed: ${agentLookupErr instanceof Error ? agentLookupErr.message : String(agentLookupErr)} — using monitor defaults`);
|
||||
}
|
||||
|
||||
return result;
|
||||
@@ -1660,8 +1744,8 @@ export class HeartbeatTriggerScheduler {
|
||||
heartbeatLog.log(`Agent ${agent.id} budget exhausted — assignment trigger skipped`);
|
||||
return;
|
||||
}
|
||||
} catch {
|
||||
// If getBudgetStatus fails, proceed without budget check
|
||||
} catch (budgetErr) {
|
||||
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;
|
||||
@@ -1745,8 +1829,8 @@ export class HeartbeatTriggerScheduler {
|
||||
heartbeatLog.log(`Agent ${agentId} over budget threshold (${budgetStatus.usagePercent}%) — timer tick skipped`);
|
||||
return;
|
||||
}
|
||||
} catch {
|
||||
// If getBudgetStatus fails, proceed without budget check
|
||||
} catch (budgetErr) {
|
||||
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", {
|
||||
|
||||
@@ -513,6 +513,13 @@ export class TaskExecutor {
|
||||
);
|
||||
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);
|
||||
const { session } = this.activeSessions.get(task.id)!;
|
||||
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;
|
||||
}
|
||||
if (task.paused && this.activeStepExecutors.has(task.id)) {
|
||||
@@ -543,6 +554,10 @@ export class TaskExecutor {
|
||||
this.options.stuckTaskDetector?.untrackTask(task.id);
|
||||
const stepExecutor = this.activeStepExecutors.get(task.id)!;
|
||||
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;
|
||||
}
|
||||
|
||||
@@ -556,7 +571,9 @@ export class TaskExecutor {
|
||||
try {
|
||||
await this.clearResumeFailureState(task);
|
||||
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) =>
|
||||
executorLog.error(`Failed to resume unpaused ${task.id}:`, err),
|
||||
);
|
||||
@@ -677,6 +694,10 @@ export class TaskExecutor {
|
||||
this.pausedAborted.add(taskId);
|
||||
this.options.stuckTaskDetector?.untrackTask(taskId);
|
||||
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) {
|
||||
executorLog.log(`Global pause — terminating step sessions for ${taskId}`);
|
||||
@@ -685,6 +706,10 @@ export class TaskExecutor {
|
||||
stepExecutor.terminateAllSessions().catch(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 {
|
||||
this.executing.delete(task.id);
|
||||
this.loopRecoveryState.delete(task.id);
|
||||
await stepExecutor.cleanup().catch(cleanupErr =>
|
||||
executorLog.warn(`StepSessionExecutor cleanup failed for ${task.id}: ${cleanupErr}`)
|
||||
);
|
||||
// Wrap cleanup in try/catch so activeStepExecutors.delete() always runs.
|
||||
// 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);
|
||||
|
||||
// Stuck-requeue: clean up worktree and move to todo
|
||||
@@ -2137,6 +2167,16 @@ export class TaskExecutor {
|
||||
// Clear run context at end of execute() lifecycle
|
||||
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.
|
||||
// State is in-memory and per-run — should not persist across attempts.
|
||||
this.loopRecoveryState.delete(task.id);
|
||||
@@ -4131,12 +4171,16 @@ and show an appropriate message to the user.\`
|
||||
// Normal completion — mark as active (available)
|
||||
try {
|
||||
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) {
|
||||
// Error during execution — mark as error
|
||||
try {
|
||||
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);
|
||||
executorLog.warn(`Child agent ${agentId} failed: ${errorMessage}`);
|
||||
} finally {
|
||||
|
||||
@@ -738,6 +738,8 @@ describe("Scheduler after restart", () => {
|
||||
const todoTask = makeTask("FN-070", "todo");
|
||||
store.listTasks.mockResolvedValue([todoTask]);
|
||||
store.getSettings.mockResolvedValue({ ...DEFAULT_SETTINGS });
|
||||
// Mock getTask for compare-and-swap verification in schedule()
|
||||
store.getTask.mockResolvedValue(todoTask);
|
||||
|
||||
// Needs parseFileScopeFromPrompt for overlap checks
|
||||
store.parseFileScopeFromPrompt.mockResolvedValue([]);
|
||||
@@ -1320,6 +1322,8 @@ describe("Engine pause/unpause cycle", () => {
|
||||
store.listTasks.mockResolvedValue([todoTask]);
|
||||
store.getSettings.mockResolvedValue({ ...DEFAULT_SETTINGS, enginePaused: false });
|
||||
store.parseFileScopeFromPrompt.mockResolvedValue([]);
|
||||
// Mock getTask for compare-and-swap verification in schedule()
|
||||
store.getTask.mockResolvedValue(todoTask);
|
||||
|
||||
const onSchedule = vi.fn();
|
||||
const scheduler = new Scheduler(store, {
|
||||
|
||||
@@ -670,6 +670,21 @@ export class Scheduler {
|
||||
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
|
||||
schedulerLog.log(`Starting ${task.id}: ${task.title || task.id} (deps satisfied)`);
|
||||
await this.store.updateTask(task.id, {
|
||||
|
||||
@@ -2094,12 +2094,14 @@ describe("maintenance cycle concurrency", () => {
|
||||
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 maxConcurrent = 0;
|
||||
let executionOrder: string[] = [];
|
||||
|
||||
const makeSlow = (label: string) =>
|
||||
(vi.spyOn(manager as any, label).mockImplementation(async () => {
|
||||
executionOrder.push(label);
|
||||
runningCount++;
|
||||
maxConcurrent = Math.max(maxConcurrent, runningCount);
|
||||
await vi.advanceTimersByTimeAsync(10);
|
||||
@@ -2115,16 +2117,25 @@ describe("maintenance cycle concurrency", () => {
|
||||
|
||||
await (manager as any).runMaintenance();
|
||||
|
||||
// If they ran in parallel, maxConcurrent should be > 1
|
||||
expect(maxConcurrent).toBeGreaterThan(1);
|
||||
// Operations run sequentially (one at a time), not in parallel.
|
||||
// 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 maxConcurrent = 0;
|
||||
let executionOrder: string[] = [];
|
||||
|
||||
const makeSlow = (label: string) =>
|
||||
(vi.spyOn(manager as any, label).mockImplementation(async () => {
|
||||
executionOrder.push(label);
|
||||
runningCount++;
|
||||
maxConcurrent = Math.max(maxConcurrent, runningCount);
|
||||
await vi.advanceTimersByTimeAsync(10);
|
||||
@@ -2145,7 +2156,10 @@ describe("maintenance cycle concurrency", () => {
|
||||
|
||||
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 () => {
|
||||
|
||||
@@ -148,15 +148,28 @@ export class SelfHealingManager {
|
||||
* stale in-progress/specifying tasks that no longer have a live worker.
|
||||
*/
|
||||
async runStartupRecovery(): Promise<void> {
|
||||
await this.recoverNoProgressNoTaskDoneFailures();
|
||||
await this.recoverCompletedTasks();
|
||||
await this.recoverStaleIncompleteReviewTasks();
|
||||
await this.recoverReviewTasksWithFailedPreMergeSteps();
|
||||
await this.recoverInterruptedMergingTasks();
|
||||
await this.recoverMisclassifiedFailures();
|
||||
await this.recoverOrphanedExecutions();
|
||||
await this.recoverApprovedTriageTasks();
|
||||
await this.recoverOrphanedSpecifyingTasks();
|
||||
// Each recovery step is isolated — one failure doesn't prevent subsequent steps.
|
||||
const steps: Array<{ name: string; fn: () => Promise<unknown> }> = [
|
||||
{ name: "no-progress-no-task-done", fn: () => this.recoverNoProgressNoTaskDoneFailures().then(() => undefined) },
|
||||
{ name: "completed-tasks", fn: () => this.recoverCompletedTasks().then(() => undefined) },
|
||||
{ name: "stale-incomplete-review", fn: () => this.recoverStaleIncompleteReviewTasks().then(() => undefined) },
|
||||
{ name: "failed-pre-merge-steps", fn: () => this.recoverReviewTasksWithFailedPreMergeSteps().then(() => undefined) },
|
||||
{ name: "interrupted-merging", fn: () => this.recoverInterruptedMergingTasks().then(() => undefined) },
|
||||
{ name: "misclassified-failures", fn: () => this.recoverMisclassifiedFailures().then(() => undefined) },
|
||||
{ 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 {
|
||||
@@ -469,44 +482,55 @@ export class SelfHealingManager {
|
||||
|
||||
try {
|
||||
// Batch 1 — Git/filesystem cleanup
|
||||
const batch1Results = await Promise.allSettled([
|
||||
this.pruneWorktrees(),
|
||||
this.cleanupOrphans(),
|
||||
this.cleanupOrphanedBranches(),
|
||||
Promise.resolve(this.checkpointWal()),
|
||||
this.enforceWorktreeCap(),
|
||||
]);
|
||||
for (const result of batch1Results) {
|
||||
if (result.status === "rejected") {
|
||||
log.error(`Batch 1 cleanup failed: ${result.reason}`);
|
||||
const batch1Fns: Array<{ name: string; fn: () => Promise<unknown> }> = [
|
||||
{ name: "prune-worktrees", fn: () => this.pruneWorktrees() },
|
||||
{ name: "cleanup-orphans", fn: () => this.cleanupOrphans() },
|
||||
{ name: "cleanup-orphaned-branches", fn: () => this.cleanupOrphanedBranches() },
|
||||
{ name: "checkpoint-wal", fn: () => Promise.resolve(this.checkpointWal()) },
|
||||
{ name: "enforce-worktree-cap", fn: () => this.enforceWorktreeCap() },
|
||||
];
|
||||
for (const fn of batch1Fns) {
|
||||
try {
|
||||
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)
|
||||
const batch2Results = await Promise.allSettled([
|
||||
this.recoverCompletedTasks(),
|
||||
this.recoverStaleIncompleteReviewTasks(),
|
||||
this.recoverReviewTasksWithFailedPreMergeSteps(),
|
||||
this.recoverInterruptedMergingTasks(),
|
||||
this.recoverMergeableReviewTasks(),
|
||||
this.recoverMergedReviewTasks(),
|
||||
this.recoverMisclassifiedFailures(),
|
||||
this.recoverNoProgressNoTaskDoneFailures(),
|
||||
this.recoverOrphanedExecutions(),
|
||||
this.recoverApprovedTriageTasks(),
|
||||
this.recoverOrphanedSpecifyingTasks(),
|
||||
]);
|
||||
for (const result of batch2Results) {
|
||||
if (result.status === "rejected") {
|
||||
log.error(`Batch 2 recovery failed: ${result.reason}`);
|
||||
const batch2Fns: Array<{ name: string; fn: () => Promise<unknown> }> = [
|
||||
{ name: "recover-completed-tasks", fn: () => this.recoverCompletedTasks() },
|
||||
{ name: "recover-stale-incomplete-review", fn: () => this.recoverStaleIncompleteReviewTasks() },
|
||||
{ name: "recover-failed-pre-merge-steps", fn: () => this.recoverReviewTasksWithFailedPreMergeSteps() },
|
||||
{ name: "recover-interrupted-merging", fn: () => this.recoverInterruptedMergingTasks() },
|
||||
{ name: "recover-mergeable-review", fn: () => this.recoverMergeableReviewTasks() },
|
||||
{ name: "recover-merged-review", fn: () => this.recoverMergedReviewTasks() },
|
||||
{ name: "recover-misclassified-failures", fn: () => this.recoverMisclassifiedFailures() },
|
||||
{ name: "recover-no-progress-no-task-done", fn: () => this.recoverNoProgressNoTaskDoneFailures() },
|
||||
{ name: "recover-orphaned-executions", fn: () => this.recoverOrphanedExecutions() },
|
||||
{ name: "recover-approved-triage", fn: () => this.recoverApprovedTriageTasks() },
|
||||
{ name: "recover-orphaned-specifying", fn: () => this.recoverOrphanedSpecifyingTasks() },
|
||||
];
|
||||
for (const fn of batch2Fns) {
|
||||
try {
|
||||
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)
|
||||
const batch3Results = await Promise.allSettled([this.archiveStaleDoneTasks()]);
|
||||
for (const result of batch3Results) {
|
||||
if (result.status === "rejected") {
|
||||
log.error(`Batch 3 archive failed: ${result.reason}`);
|
||||
const batch3Fns: Array<{ name: string; fn: () => Promise<unknown> }> = [
|
||||
{ name: "archive-stale-done", fn: () => this.archiveStaleDoneTasks() },
|
||||
];
|
||||
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)}`);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user