FN-8006: terminalize Plan Review retry storms
Plan Review now fails tasks when reviewer fallback retry limits are exceeded. - Detect RetryStormError from Plan Review workflow execution - Serialize the terminal retry error, clear recovery scheduling, and preserve workflow results - Add retry-storm regression coverage, architecture guidance, and a patch changeset Files changed: .changeset/fn-8006-plan-review-retry-storm.md | 7 ++++ docs/architecture.md | 2 +- packages/engine/src/__tests__/triage-plan-review-unavailable-retry.test.ts | 47 +++++++++++++++++++++- packages/engine/src/triage.ts | 33 +++++++++++++++ 4 files changed, 87 insertions(+), 2 deletions(-) Fusion-Task-Id: FN-8006 Fusion-Task-Lineage: 932e7930-2069-4b0c-9cd1-9db39c2de5a3 Co-authored-by: Fusion (runfusion.ai) <noreply@runfusion.ai>
This commit is contained in:
7
.changeset/fn-8006-plan-review-retry-storm.md
Normal file
7
.changeset/fn-8006-plan-review-retry-storm.md
Normal file
@@ -0,0 +1,7 @@
|
|||||||
|
---
|
||||||
|
"@runfusion/fusion": patch
|
||||||
|
---
|
||||||
|
|
||||||
|
summary: Plan Review no longer loops forever on reviewer retry storms — it fails the task with a clear error.
|
||||||
|
category: fix
|
||||||
|
dev: runPlanReviewBeforeExecution now terminalizes RetryStormError (status "failed", serialized error, nextRecoveryAt cleared) instead of re-queuing plan-review-unavailable, which had let reviewerFallbackRetryCount climb unbounded past maxReviewerFallbackRetries.
|
||||||
@@ -2176,7 +2176,7 @@ UI contract boundary:
|
|||||||
|
|
||||||
Fusion derives a per-task `retrySummary` at read time by aggregating retry counters (stuck-kill, recovery, task_done, workflow-step, verification, post-review-fix, merge-conflict bounce, branch-conflict recovery, reviewer context retry, reviewer fallback retry). The engine emits a structured `retry-burned` log channel with `{ taskId, agentId, role, category, attempt, total, breakdown }` so token-cost telemetry can correlate retry burn with spend.
|
Fusion derives a per-task `retrySummary` at read time by aggregating retry counters (stuck-kill, recovery, task_done, workflow-step, verification, post-review-fix, merge-conflict bounce, branch-conflict recovery, reviewer context retry, reviewer fallback retry). The engine emits a structured `retry-burned` log channel with `{ taskId, agentId, role, category, attempt, total, breakdown }` so token-cost telemetry can correlate retry burn with spend.
|
||||||
|
|
||||||
Project settings expose per-category caps (`maxBranchConflictRecoveries`, `maxReviewerContextRetries`, `maxReviewerFallbackRetries`) plus a master cap (`maxTotalRetriesBeforeFail`). When a cap is exceeded, engine code throws `RetryStormError`; executor terminal failure handling serializes this into `task.error` so dashboard surfaces can render structured failure details.
|
Project settings expose per-category caps (`maxBranchConflictRecoveries`, `maxReviewerContextRetries`, `maxReviewerFallbackRetries`) plus a master cap (`maxTotalRetriesBeforeFail`). When a cap is exceeded, engine code throws `RetryStormError`; executor and triage Plan Review terminal failure handling serialize this into `task.error` so dashboard surfaces can render structured failure details. Plan Review must terminalize this guard rather than re-queue `plan-review-unavailable`, which would otherwise continue burning the reviewer-fallback budget.
|
||||||
|
|
||||||
## Lifecycle invariants
|
## Lifecycle invariants
|
||||||
|
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import { describe, it, expect, vi, afterEach } from "vitest";
|
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||||
import type { Settings, Task, TaskStore } from "@fusion/core";
|
import { RetryStormError, type Settings, type Task, type TaskStore } from "@fusion/core";
|
||||||
import { join } from "node:path";
|
import { join } from "node:path";
|
||||||
import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises";
|
import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises";
|
||||||
import { readFileSync } from "node:fs";
|
import { readFileSync } from "node:fs";
|
||||||
@@ -209,6 +209,51 @@ describe("Plan Review unavailable retry", () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("terminalizes a reviewer retry storm instead of scheduling another unavailable retry", async () => {
|
||||||
|
const rootDir = await createFixtureRoot();
|
||||||
|
roots.push(rootDir);
|
||||||
|
const task = createRetryTask({ id: "FN-PLAN-RETRY-STORM", reviewerFallbackRetryCount: 3 });
|
||||||
|
const prompt = `# Task: ${task.id} - Existing draft\n\n## Mission\n\nKeep this exact text.\n`;
|
||||||
|
await writePrompt(rootDir, task.id, prompt);
|
||||||
|
const store = createStore(task);
|
||||||
|
const storm = new RetryStormError({
|
||||||
|
category: "reviewerFallback",
|
||||||
|
total: 3,
|
||||||
|
cap: 2,
|
||||||
|
breakdown: {
|
||||||
|
stuckKill: 0,
|
||||||
|
recovery: 0,
|
||||||
|
taskDone: 0,
|
||||||
|
worktreeSession: 0,
|
||||||
|
workflowStep: 0,
|
||||||
|
verification: 0,
|
||||||
|
postReviewFix: 0,
|
||||||
|
mergeConflict: 0,
|
||||||
|
branchConflict: 0,
|
||||||
|
reviewerContext: 0,
|
||||||
|
reviewerFallback: 3,
|
||||||
|
total: 3,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
mockReviewStep.mockRejectedValue(storm);
|
||||||
|
|
||||||
|
await retryTask(rootDir, task, store);
|
||||||
|
|
||||||
|
expect(store.updateTask).toHaveBeenCalledWith(task.id, expect.objectContaining({
|
||||||
|
status: "failed",
|
||||||
|
error: expect.stringContaining('"type":"RetryStormError"'),
|
||||||
|
nextRecoveryAt: null,
|
||||||
|
}));
|
||||||
|
expect(store.updateTask).not.toHaveBeenCalledWith(task.id, expect.objectContaining({
|
||||||
|
status: "plan-review-unavailable",
|
||||||
|
}));
|
||||||
|
expect(store.logEntry).toHaveBeenCalledWith(
|
||||||
|
task.id,
|
||||||
|
"[pre-merge] Workflow step failed: Plan Review",
|
||||||
|
expect.stringContaining('"type":"RetryStormError"'),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it.each([
|
it.each([
|
||||||
{ name: "missing", contents: null, expectedError: /could not read existing PROMPT\.md/i },
|
{ name: "missing", contents: null, expectedError: /could not read existing PROMPT\.md/i },
|
||||||
{ name: "whitespace-only", contents: " \n\t\n", expectedError: /PROMPT\.md.*(empty|whitespace)/i },
|
{ name: "whitespace-only", contents: " \n\t\n", expectedError: /PROMPT\.md.*(empty|whitespace)/i },
|
||||||
|
|||||||
@@ -12,6 +12,8 @@ import type {
|
|||||||
import {
|
import {
|
||||||
DUPLICATE_OF_METADATA_KEY,
|
DUPLICATE_OF_METADATA_KEY,
|
||||||
PLAN_REVIEW_GROUP_ID,
|
PLAN_REVIEW_GROUP_ID,
|
||||||
|
RetryStormError,
|
||||||
|
serializeRetryStormError,
|
||||||
TaskDeletedError,
|
TaskDeletedError,
|
||||||
buildTriageMemoryInstructions,
|
buildTriageMemoryInstructions,
|
||||||
isUnplannedSeedPrompt,
|
isUnplannedSeedPrompt,
|
||||||
@@ -2339,6 +2341,7 @@ export class TriageProcessor {
|
|||||||
instead of step-checkbox language that does not match this gate. Inline PROMPT.md
|
instead of step-checkbox language that does not match this gate. Inline PROMPT.md
|
||||||
repair remains allowed so the reviewer can fix-and-APPROVE instead of REVISE-looping.
|
repair remains allowed so the reviewer can fix-and-APPROVE instead of REVISE-looping.
|
||||||
*/
|
*/
|
||||||
|
let reviewFailure: unknown;
|
||||||
const review = await reviewStep(
|
const review = await reviewStep(
|
||||||
this.rootDir,
|
this.rootDir,
|
||||||
task.id,
|
task.id,
|
||||||
@@ -2362,6 +2365,7 @@ export class TriageProcessor {
|
|||||||
onSessionEnded: (session) => this.unregisterSubagentSession(task.id, session),
|
onSessionEnded: (session) => this.unregisterSubagentSession(task.id, session),
|
||||||
},
|
},
|
||||||
).catch((error: unknown) => {
|
).catch((error: unknown) => {
|
||||||
|
reviewFailure = error;
|
||||||
const message = error instanceof Error ? error.message : String(error);
|
const message = error instanceof Error ? error.message : String(error);
|
||||||
planLog.warn(`${task.id}: Plan Review unavailable before execution (${message})`);
|
planLog.warn(`${task.id}: Plan Review unavailable before execution (${message})`);
|
||||||
return {
|
return {
|
||||||
@@ -2372,6 +2376,35 @@ export class TriageProcessor {
|
|||||||
});
|
});
|
||||||
|
|
||||||
const completedAt = new Date().toISOString();
|
const completedAt = new Date().toISOString();
|
||||||
|
/*
|
||||||
|
FNXC:PlanReview 2026-07-15-18:00:
|
||||||
|
A reviewer-fallback RetryStormError is a terminal guard, not a transient reviewer
|
||||||
|
outage. Preserve its structured core serialization and stop here so Plan Review does
|
||||||
|
not re-enter `plan-review-unavailable`, where another poll would re-increment
|
||||||
|
reviewerFallbackRetryCount beyond the cap.
|
||||||
|
*/
|
||||||
|
if (reviewFailure instanceof RetryStormError) {
|
||||||
|
const terminalError = JSON.stringify(serializeRetryStormError(reviewFailure));
|
||||||
|
const output = review.review || reviewFailure.message;
|
||||||
|
await this.recordPlanReviewWorkflowResult(task, {
|
||||||
|
workflowStepId: PLAN_REVIEW_GROUP_ID,
|
||||||
|
workflowStepName: "Plan Review",
|
||||||
|
phase: "pre-merge",
|
||||||
|
status: "failed",
|
||||||
|
output,
|
||||||
|
notes: review.summary,
|
||||||
|
startedAt,
|
||||||
|
completedAt,
|
||||||
|
});
|
||||||
|
await this.store.logEntry(task.id, "[pre-merge] Workflow step failed: Plan Review", terminalError);
|
||||||
|
await this.store.updateTask(task.id, {
|
||||||
|
status: "failed",
|
||||||
|
error: terminalError,
|
||||||
|
nextRecoveryAt: null,
|
||||||
|
});
|
||||||
|
return "blocked";
|
||||||
|
}
|
||||||
|
|
||||||
if (review.verdict === "APPROVE") {
|
if (review.verdict === "APPROVE") {
|
||||||
await this.recordPlanReviewWorkflowResult(task, {
|
await this.recordPlanReviewWorkflowResult(task, {
|
||||||
workflowStepId: PLAN_REVIEW_GROUP_ID,
|
workflowStepId: PLAN_REVIEW_GROUP_ID,
|
||||||
|
|||||||
Reference in New Issue
Block a user