From 00011b011323e6da969a195fd064c8e55cc941d8 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 26 Jul 2026 12:08:03 -0700 Subject: [PATCH] fix(engine): recover restart-orphaned review steps in one cycle, raise fix budget FN-8603 sat in-review for ~36 minutes after an engine restart killed its Code Review session 34 seconds in. It did recover on its own; the cost was latency, not a terminal park. Sweep ordering. reconcile-orphaned-pending-step-results PRODUCES the failed results that recover-failed-pre-merge-steps CONSUMES, but in the periodic maintenance list it ran ~15 entries after it. A step orphaned in cycle N was therefore rewritten to failed only after recovery had already scanned, so nothing re-ran it until cycle N+1. Moved it immediately before its consumer and removed the now-duplicated later entry. Startup recovery already ordered the two correctly. Post-review fix budget. Default raised 3 -> 10 per operator request. Three passes is below the observed convergence length for the gates this fallback actually governs -- Browser Verification and custom optional gates -- since Plan Review and Code Review already resolve to "unbounded" when unset, and exhausting the budget parks the card for a human. The declaration default and five inline `settings.maxPostReviewFixes ?? 3` call sites in executor.ts/self-healing.ts had drifted into separate literals, so raising one alone would have left every unset-settings path on the old value; they now share the exported DEFAULT_MAX_POST_REVIEW_FIXES. Not done, and why. Re-dispatching a restart-orphaned lease immediately at startup is the change that would close the remaining ~14-minute wait, but it is unsound as specified: liveness is judged by a 15-minute lease-staleness floor because leases carry no node attribution, so treating a pre-boot lease as dead would let one node orphan another node's genuinely running review. Needs a node id on the lease record first. Left the floor intact. Verified: tsc clean on core and engine, pnpm lint clean, pnpm test:gate green, self-healing orphaned-pending-step-results and optional-step-revision suites green. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/review-restart-recovery-latency.md | 7 +++++++ .../core/src/builtin-workflow-settings.ts | 18 +++++++++++++++- packages/core/src/index.ts | 1 + packages/engine/src/executor.ts | 12 +++++------ packages/engine/src/self-healing.ts | 21 ++++++++++++++++--- 5 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 .changeset/review-restart-recovery-latency.md diff --git a/.changeset/review-restart-recovery-latency.md b/.changeset/review-restart-recovery-latency.md new file mode 100644 index 0000000000..7c2bd1514e --- /dev/null +++ b/.changeset/review-restart-recovery-latency.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Reviews stalled by an engine restart now recover in one self-healing cycle instead of ~36 minutes. +category: fix +dev: Moves `reconcile-orphaned-pending-step-results` ahead of `recover-failed-pre-merge-steps` in the periodic maintenance list (it produces the `failed` results that step consumes; it previously ran ~15 entries later, so an orphan found in cycle N was not re-dispatched until cycle N+1) and removes the now-duplicated later entry. Raises the `maxPostReviewFixes` default 3 -> 10 and routes the five inline `?? 3` fallbacks in executor.ts/self-healing.ts through the new exported `DEFAULT_MAX_POST_REVIEW_FIXES` so the declaration default and the unset-settings paths cannot drift again. Plan Review and Code Review are unaffected — they already resolve to "unbounded" when unset. diff --git a/packages/core/src/builtin-workflow-settings.ts b/packages/core/src/builtin-workflow-settings.ts index 75303d884d..2704fe7f4c 100644 --- a/packages/core/src/builtin-workflow-settings.ts +++ b/packages/core/src/builtin-workflow-settings.ts @@ -140,8 +140,15 @@ export const BUILTIN_MOVED_WORKFLOW_SETTINGS: WorkflowSettingDefinition[] = [ /* * FNXC:WorkflowOptionalStepCycle 2026-06-29-17:55: * This global budget remains the fallback for custom optional gates and explicitly capped built-in gates. Built-in Code Review now sets `maxRevisions: "unbounded"` so ordinary reviewer feedback keeps recovering instead of terminal-failing after three passes. + * + * FNXC:WorkflowOptionalStepCycle 2026-07-26-19:35: + * Raised 3 -> 10 (operator request). Three passes is below the observed convergence length for + * Browser Verification and custom gates — the gates this fallback actually governs, since + * Plan Review and Code Review resolve to "unbounded" when unset. Exhausting the budget parks + * the card for a human, so a too-low cap converts "needs another pass" into operator toil. + * This is a fallback, not a ceiling: an explicit workflow value or node `maxRevisions` still wins. */ - default: 3, + default: 10, description: "Maximum automatic fix passes after review/optional-step feedback; the step re-runs each pass until it passes or this budget is exhausted.", }, @@ -539,6 +546,15 @@ export const BUILTIN_REVIEW_REVISION_SETTINGS: WorkflowSettingDefinition[] = [ * convention `metaTaskStallAutoCloseMs` already uses for a comparable stall * judgment call elsewhere in this codebase. */ +/* +FNXC:WorkflowOptionalStepCycle 2026-07-26-19:38: +Single source for the post-review fix fallback. The `maxPostReviewFixes` declaration default above and +the engine's inline `settings.maxPostReviewFixes ?? N` call sites had drifted apart as two separate +literal 3s, so raising the declaration alone would have left every unset-settings path on the old +value. Import this rather than re-inlining a number. +*/ +export const DEFAULT_MAX_POST_REVIEW_FIXES = 10; + export const DEFAULT_PLANNER_OVERSEER_EXECUTOR_STUCK_AFTER_MS = 2 * 60 * 60 * 1000; export const PLANNER_HEARTBEAT_PATROL_ENABLED_SETTING_ID = "plannerHeartbeatPatrolEnabled"; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index fc1c56f507..04bede7f9e 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -305,6 +305,7 @@ export { BUILTIN_MOVED_WORKFLOW_SETTINGS, BUILTIN_TRIAGE_POLICY_SETTINGS, BUILTIN_OVERSIGHT_SETTINGS, + DEFAULT_MAX_POST_REVIEW_FIXES, DEFAULT_PLANNER_OVERSEER_EXECUTOR_STUCK_AFTER_MS, PLANNER_HEARTBEAT_PATROL_ENABLED_SETTING_ID, renderTriagePolicyPlaceholders, diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 54308550b2..547dad7e57 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -14,7 +14,7 @@ import { existsSync, lstatSync, realpathSync } from "node:fs"; import { readFile, rm, writeFile } from "node:fs/promises"; import type { TaskStore, Task, TaskDetail, TaskTokenUsage, StepStatus, Settings, WorkflowStep, MissionStore, AsyncMissionStore, Slice, AgentState, AgentCapability, RunMutationContext, AgentHeartbeatConfig, Agent, AgentMemoryInclusionMode, ProjectSettings, MergeResult, WorkflowIrNode, WorkflowIrNodeKind, WorkflowStepResult as CoreWorkflowStepResult, ThinkingLevel } from "@fusion/core"; import { getUnmetSchedulingDependencies } from "./scheduler.js"; -import { RetryStormError, serializeRetryStormError, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, resolveWorkflowIrForTask, evaluateForeachMergeProof, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveReboundTarget, resolveColumnAgentBinding, resolveEffectiveAgent, instanceNodeId, getWorkflowExtensionRegistry, getBuiltinWorkflow, parseNoOpCompletionMarker, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, isLiveSharedBranchGroupMemberIntegration, resolveMaxAutoMergeRetries, resolveMaxConsecutiveToolFailureRetries, resolveConsecutiveToolFailureRetryBackoffMs, resolveConsecutiveToolFailureThreshold, resolveExecutorEscalationTarget, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, COMPLETION_SUMMARY_NODE_ID, upsertWorkflowStepResult, AWAITING_APPROVAL_PAUSE_REASON, THINKING_LEVELS, ACTIVE_WORKFLOW_WORK_ITEM_STATES, AgentStore, resolveExecutorFallbackModel } from "@fusion/core"; +import { RetryStormError, serializeRetryStormError, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, resolveWorkflowIrForTask, evaluateForeachMergeProof, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveReboundTarget, resolveColumnAgentBinding, resolveEffectiveAgent, instanceNodeId, getWorkflowExtensionRegistry, getBuiltinWorkflow, parseNoOpCompletionMarker, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, isLiveSharedBranchGroupMemberIntegration, resolveMaxAutoMergeRetries, resolveMaxConsecutiveToolFailureRetries, resolveConsecutiveToolFailureRetryBackoffMs, resolveConsecutiveToolFailureThreshold, resolveExecutorEscalationTarget, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, DEFAULT_MAX_POST_REVIEW_FIXES, COMPLETION_SUMMARY_NODE_ID, upsertWorkflowStepResult, AWAITING_APPROVAL_PAUSE_REASON, THINKING_LEVELS, ACTIVE_WORKFLOW_WORK_ITEM_STATES, AgentStore, resolveExecutorFallbackModel } from "@fusion/core"; import { finalizeProvenAutoMergeTask } from "./auto-merge-finalization.js"; import { mergeEffectiveSettings } from "./effective-settings.js"; import { generateFeatureVideo, type GenerateFeatureVideoOptions } from "./review-artifacts/feature-video.js"; @@ -5038,9 +5038,9 @@ export class TaskExecutor { optionalGroupId: info.nodeId ?? "plan-review", workflowSettings: settings as Record, nodeMaxRevisions: info.maxRevisions, - fallbackMaxRevisions: settings.maxPostReviewFixes ?? 3, + fallbackMaxRevisions: settings.maxPostReviewFixes ?? DEFAULT_MAX_POST_REVIEW_FIXES, }); - const budget = resolveOptionalStepRevisionBudget(maxRevisions, settings.maxPostReviewFixes ?? 3); + const budget = resolveOptionalStepRevisionBudget(maxRevisions, settings.maxPostReviewFixes ?? DEFAULT_MAX_POST_REVIEW_FIXES); if (!budget.unbounded && (!Number.isFinite(budget.max) || budget.max <= 0)) { // FNXC:RemediationVisibility 2026-07-26-19:20 (FN-8596 follow-up): returning false here // makes the graph's plan-replan node fail with `remediation-not-scheduled` and leaves the @@ -5130,9 +5130,9 @@ export class TaskExecutor { optionalGroupId: info.nodeId ?? "", workflowSettings: settings as Record, nodeMaxRevisions: info.maxRevisions, - fallbackMaxRevisions: settings.maxPostReviewFixes ?? 3, + fallbackMaxRevisions: settings.maxPostReviewFixes ?? DEFAULT_MAX_POST_REVIEW_FIXES, }); - const budget = resolveOptionalStepRevisionBudget(maxRevisions, settings.maxPostReviewFixes ?? 3); + const budget = resolveOptionalStepRevisionBudget(maxRevisions, settings.maxPostReviewFixes ?? DEFAULT_MAX_POST_REVIEW_FIXES); if (!budget.unbounded && (!Number.isFinite(budget.max) || budget.max <= 0)) { executorLog.warn( `${taskId}: pre-merge remediation NOT scheduled for step "${info.stepName}" — revision budget is zero/invalid (max=${String(budget.max)}). Card left parked.`, @@ -9694,7 +9694,7 @@ export class TaskExecutor { target: CoreWorkflowStepResult, ): Promise<{ unbounded: boolean; max: number; label: string; key: string; stepName?: string; attempts: number }> { const settings = await mergeEffectiveSettings(this.store, task, await this.store.getSettings()); - const fallback = settings.maxPostReviewFixes ?? 3; + const fallback = settings.maxPostReviewFixes ?? DEFAULT_MAX_POST_REVIEW_FIXES; let rawMaxRevisions: unknown; try { const ir = await resolveWorkflowIrForTask(this.store, task.id); diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 1d30f4dd5e..c839149db2 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -31,7 +31,7 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmSync, import { readFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { isAbsolute, join, relative, resolve } from "node:path"; -import { resolveColumnFlags, IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, IN_REVIEW_STALL_TERMINAL_LOG_PREFIX, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, evaluateNoCommitsNoOpFinalize, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkflowColumnsEnabled, isWorkspaceTask, isSharedBranchGroupMemberIntegration, isNearDuplicateCanonicalInactive, parseExplicitDuplicateMarker, flagTriageDuplicate, isTriageDuplicateKeepAcknowledged, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, resolveWorkflowIrForTask, resolveReboundTarget, planLegacyAdoption, resolveOrphanedPendingStepResults, classifyReviewLease, PLAN_REVIEW_LEASE_STALENESS_MS, ACTIVE_WORKFLOW_WORK_ITEM_STATES, AWAITING_APPROVAL_PAUSE_REASON, type Agent, type AgentStore, type ChatStore, type MessageStore, type TaskStore, type Settings, type Task, type MergeDetails, type TaskPriority, type MergeResult, type WorkflowStepResult } from "@fusion/core"; +import { resolveColumnFlags, IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, IN_REVIEW_STALL_TERMINAL_LOG_PREFIX, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, evaluateNoCommitsNoOpFinalize, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkflowColumnsEnabled, isWorkspaceTask, isSharedBranchGroupMemberIntegration, isNearDuplicateCanonicalInactive, parseExplicitDuplicateMarker, flagTriageDuplicate, isTriageDuplicateKeepAcknowledged, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, resolveWorkflowIrForTask, resolveReboundTarget, planLegacyAdoption, resolveOrphanedPendingStepResults, classifyReviewLease, PLAN_REVIEW_LEASE_STALENESS_MS, DEFAULT_MAX_POST_REVIEW_FIXES, ACTIVE_WORKFLOW_WORK_ITEM_STATES, AWAITING_APPROVAL_PAUSE_REASON, type Agent, type AgentStore, type ChatStore, type MessageStore, type TaskStore, type Settings, type Task, type MergeDetails, type TaskPriority, type MergeResult, type WorkflowStepResult } from "@fusion/core"; import { finalizePlanningSegment } from "@fusion/core"; import type { MeshLeaseManager } from "./mesh-lease-manager.js"; import { createLogger, schedulerLog } from "./logger.js"; @@ -2882,6 +2882,18 @@ export class SelfHealingManager { { name: "recover-stranded-completed-todo", fn: () => this.recoverStrandedCompletedTodoTasks() }, { name: "recover-advanced-triage", fn: () => this.recoverAdvancedTriageTasks() }, { name: "recover-stale-incomplete-review", fn: () => this.recoverStaleIncompleteReviewTasks() }, + /* + FNXC:OrphanedPendingSteps 2026-07-26-19:20: + Ordering is load-bearing: this sweep PRODUCES the `failed` results that + `recover-failed-pre-merge-steps` CONSUMES, so it must run first. It used to sit ~15 + entries later in this list, which meant a step orphaned in cycle N was rewritten to + `failed` only after recovery had already scanned — nothing re-ran it until cycle N+1. + Combined with the lease-staleness wait that is a multi-cycle hole for a card whose + review session died (observed: FN-8603 sat ~36 min after an engine restart killed its + Code Review session 34s in). Startup recovery already orders these two correctly. + Keep them adjacent and in this order. + */ + { name: "reconcile-orphaned-pending-step-results", fn: () => this.reconcileOrphanedPendingStepResults() }, { name: "recover-failed-pre-merge-steps", fn: () => this.recoverReviewTasksWithFailedPreMergeSteps() }, { name: "recover-missing-worktree-review-failures", fn: () => this.recoverMissingWorktreeReviewFailures() }, { name: "recover-interrupted-merging", fn: () => this.recoverInterruptedMergingTasks() }, @@ -2897,7 +2909,10 @@ export class SelfHealingManager { // steady-state — a step session can die without an engine restart, and startup-only // cadence left that case riding the 3×30-min stall escalator to a deadlock park. // Live sessions register their worktree path, so the liveness veto holds here. - { name: "reconcile-orphaned-pending-step-results", fn: () => this.reconcileOrphanedPendingStepResults() }, + // FNXC:OrphanedPendingSteps 2026-07-26-19:22: this entry MOVED up to immediately + // before `recover-failed-pre-merge-steps` (see the ordering note there). It is not + // duplicated here — running it twice per cycle would re-scan every in-review task for + // no benefit. { name: "reconcile-stranded-hold-continuations", fn: () => this.reconcileStrandedHoldContinuations() }, { name: "recover-mergeable-review", fn: () => this.recoverMergeableReviewTasks() }, // FNXC:Workspace 2026-06-22-09:30 (Phase D U1) — workspace-mode reconcilers. @@ -7770,7 +7785,7 @@ export class SelfHealingManager { }; for (const task of tasks) { const eff = await mergeEffectiveSettings(this.store, task, settings); - const fallback = eff.maxPostReviewFixes ?? 3; + const fallback = eff.maxPostReviewFixes ?? DEFAULT_MAX_POST_REVIEW_FIXES; let rawMaxRevisions: unknown; const target = latestFailedPreMergeStep(task); if (target?.workflowStepId) {