From 6fd685fd476645eebb8eb4516a8e6adc8b4579bb Mon Sep 17 00:00:00 2001 From: weichen Date: Sun, 13 Sep 2026 19:21:56 +0000 Subject: [PATCH] fix(service): retry only safe transients and keep existing targets Fresh step invocation is limited to provider terminated/connection plus a missing target, or an explicit runner failure. A deliverable already on disk is accepted and never overwritten. Co-authored-by: Cursor --- internal/agent/step_retry.go | 38 ++++++++++++++++++------------- internal/agent/step_retry_test.go | 16 +++++++++++++ internal/squad/step_retry_test.go | 27 ++++++++++++++++++++++ internal/squad/workflow.go | 29 ++++++++++++++++++----- 4 files changed, 88 insertions(+), 22 deletions(-) diff --git a/internal/agent/step_retry.go b/internal/agent/step_retry.go index ce46e491..860616e9 100644 --- a/internal/agent/step_retry.go +++ b/internal/agent/step_retry.go @@ -44,6 +44,9 @@ type StepFailureSignals struct { WorkDir string ExtraText string TargetMissing bool + // TargetExists is a non-empty deliverable already on disk. Retry must + // not overwrite it. + TargetExists bool } type FailureAssessment struct { @@ -106,27 +109,25 @@ func AssessStepFailure(sig StepFailureSignals) FailureAssessment { lower := strings.ToLower(reason) trace := ScanTraceText(sig.ExtraText + "\n" + ReadWorkDirTraces(sig.WorkDir)) missing := sig.TargetMissing || strings.Contains(lower, "missing target deliverable") - - if isPermanentReason(lower) && !trace.ProviderTransient { - return FailureAssessment{Class: FailurePermanent, Reason: "permanent: " + compactReason(reason)} + if sig.TargetExists { + return FailureAssessment{Class: FailurePermanent, Reason: "target-already-present: do not overwrite"} } - if isPermanentReason(lower) && !missing { + if isPermanentReason(lower) { return FailureAssessment{Class: FailurePermanent, Reason: "permanent: " + compactReason(reason)} } - if sig.Result != nil && sig.Result.Retryable { - return FailureAssessment{Class: FailureTransient, Reason: "runner-retryable: " + compactReason(reason)} + runnerFailed := (sig.Result != nil && sig.Result.Retryable) || isRunnerExplicitFailure(lower) + provider := trace.ProviderTransient || hasProviderTermOrConn(lower) + if runnerFailed { + return FailureAssessment{Class: FailureTransient, Reason: "runner-explicit-failure: " + compactReason(reason)} } - if trace.ProviderTransient { - kind := "provider-transient" - if missing && trace.Settled { + if provider && missing { + kind := "provider-transient-missing-target" + if trace.Settled { kind = "settled-failure-missing-target" } return FailureAssessment{Class: FailureTransient, Reason: kind + ": " + compactReason(reason)} } - if hasTransientKeyword(lower) { - return FailureAssessment{Class: FailureTransient, Reason: "provider-transient: " + compactReason(reason)} - } if missing { return FailureAssessment{Class: FailurePermanent, Reason: "permanent-missing: " + compactReason(reason)} } @@ -156,13 +157,18 @@ func isPermanentReason(lower string) bool { strings.Contains(lower, "治理拦截") } -func hasTransientKeyword(lower string) bool { +func hasProviderTermOrConn(lower string) bool { return strings.Contains(lower, "connection error") || strings.Contains(lower, "terminated") || - strings.Contains(lower, "auto_retry") || strings.Contains(lower, "econnreset") || - strings.Contains(lower, "socket hang up") || - strings.Contains(lower, "pi remote runner call failed") + strings.Contains(lower, "socket hang up") +} + +func isRunnerExplicitFailure(lower string) bool { + return strings.Contains(lower, "pi remote runner call failed") || + strings.Contains(lower, "auto_retry_failed") || + strings.Contains(lower, "auto_retry_end") || + (hasProviderTermOrConn(lower) && !strings.Contains(lower, "missing target deliverable")) } func compactReason(s string) string { diff --git a/internal/agent/step_retry_test.go b/internal/agent/step_retry_test.go index f1a0c1f6..853f16bf 100644 --- a/internal/agent/step_retry_test.go +++ b/internal/agent/step_retry_test.go @@ -103,6 +103,22 @@ func TestAssessStepFailure_TransientVsPermanent(t *testing.T) { sig: StepFailureSignals{Result: &Result{Success: false, ErrorReason: "pi remote runner call failed: connection refused"}}, want: FailureTransient, }, + { + name: "existing target is never retried", + sig: StepFailureSignals{ + Result: &Result{Success: false, ErrorReason: "Connection error.", Retryable: true}, + TargetExists: true, + }, + want: FailurePermanent, + }, + { + name: "provider signal without missing is not enough", + sig: StepFailureSignals{ + Result: &Result{Success: false, ErrorReason: "agent_not_settled"}, + ExtraText: `{"type":"auto_retry_end","success":false}`, + }, + want: FailurePermanent, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/squad/step_retry_test.go b/internal/squad/step_retry_test.go index c88aae97..7966fc4e 100644 --- a/internal/squad/step_retry_test.go +++ b/internal/squad/step_retry_test.go @@ -66,6 +66,11 @@ func (b *countingBackend) Execute(ctx context.Context, req agent.Request) (*agen case "always-transient": return &agent.Result{Success: false, ErrorReason: failText, Retryable: true}, nil + case "fail-after-writing-target": + body := []byte("# already on disk\n\ndo not overwrite\n") + _ = os.WriteFile(filepath.Join(req.WorkDir, "output.md"), body, 0o644) + return &agent.Result{Success: false, ErrorReason: "Connection error.", Retryable: true}, nil + default: return &agent.Result{Success: false, ErrorReason: "unknown mode"}, nil } @@ -165,6 +170,28 @@ func TestWorkflowDriver_DoesNotRetryPermanentMissing(t *testing.T) { } } +func TestWorkflowDriver_DoesNotOverwriteExistingTarget(t *testing.T) { + wsDir, driver := prepareSingleWriterWS(t) + backend := &countingBackend{mode: "fail-after-writing-target"} + res, err := driver.Execute(context.Background(), wsDir, backend, squad.ExecuteOptions{Timeout: 5 * time.Second}) + if err != nil { + t.Fatalf("existing target must be accepted, not retried: %v", err) + } + if !res.Success { + t.Fatalf("want success from existing file, got %s", res.ErrorReason) + } + if backend.calls != 1 { + t.Fatalf("must not re-invoke over an existing target, calls=%d", backend.calls) + } + body, err := os.ReadFile(filepath.Join(wsDir, "tasks", "01_draft", "output.md")) + if err != nil { + t.Fatal(err) + } + if string(body) != "# already on disk\n\ndo not overwrite\n" { + t.Fatalf("target was rewritten: %s", body) + } +} + func TestWorkflowDriver_BoundedTransientRetryThenFailClosed(t *testing.T) { wsDir, driver := prepareSingleWriterWS(t) backend := &countingBackend{mode: "always-transient", failText: "Connection error."} diff --git a/internal/squad/workflow.go b/internal/squad/workflow.go index c5e2e9f9..5a279e7a 100644 --- a/internal/squad/workflow.go +++ b/internal/squad/workflow.go @@ -511,6 +511,16 @@ func (d *WorkflowDriver) runOneStep( ok bool ) for attempt := 1; attempt <= agent.MaxTransientStepInvocations; attempt++ { + onDisk, _ := os.ReadFile(prevOutPath) + produced := len(strings.TrimSpace(string(onDisk))) > 0 && string(onDisk) != prevContent + if attempt > 1 && produced { + ok = true + matErr = nil + if res != nil { + res.Success = true + } + break + } if attempt > 1 && streamer != nil { streamer.EmitPhase( "step_retrying", @@ -534,18 +544,24 @@ func (d *WorkflowDriver) runOneStep( } } + onDisk, _ = os.ReadFile(prevOutPath) + produced = len(strings.TrimSpace(string(onDisk))) > 0 && string(onDisk) != prevContent newContent := "" - if res != nil { + if produced { + newContent = string(onDisk) + } else if res != nil { if b, has := res.Patches[deliverableName]; has { newContent = string(b) } } if newContent == "" { - if b, rErr := os.ReadFile(prevOutPath); rErr == nil { - newContent = string(b) - } + newContent = string(onDisk) + } + ok = (execErr == nil && res != nil && res.Success && matErr == nil) || produced + if produced && res != nil { + res.Success = true + matErr = nil } - ok = execErr == nil && res != nil && res.Success && matErr == nil sim := OutputSimilarity(prevContent, newContent) if mu != nil { mu.Lock() @@ -588,7 +604,8 @@ func (d *WorkflowDriver) runOneStep( Err: execErr, Result: res, WorkDir: subDir, - TargetMissing: matErr != nil || (res != nil && strings.Contains(res.ErrorReason, "missing target deliverable")), + TargetMissing: !produced && (matErr != nil || (res != nil && strings.Contains(res.ErrorReason, "missing target deliverable"))), + TargetExists: produced, }) if assess.Class != agent.FailureTransient || attempt >= agent.MaxTransientStepInvocations { break