review: classify start-phase timeouts consistently · Entire

review: classify start-phase timeouts consistently

26e3e56→main·

dipree·3w ago·3 files·+96 added/-9 removed

Sessions

46e83ec7c92eView transcript

Changes

3

143 unmodified lines

144
145
146
147
148
149
150
151
152
153
154
3 unmodified lines

158
159
160
156
161
162
163
164
1 unmodified line

166
167
168
164
169
170
171
172

143 unmodified lines

// No event-stream signals available since Start failed before producing any.
        finished := time.Now()
        status := classifyStatus(ctx, err, eventOutcome{})
        runErr := err
        if reviewerDeadlineFired(ctx, agentCtx, err) {
            status = reviewtypes.AgentStatusFailed
            runErr = timedOutError(displayName, timeout)
        }
        summary := reviewtypes.RunSummary{
            StartedAt:  started,
            FinishedAt: finished,
3 unmodified lines

AgentName: agentName,
                Model:     modelName,
                Status:    status,
                Err:       err,
                Err:       runErr,
                StartedAt: started,
                Duration:  finished.Sub(started),
            }},
1 unmodified line

for _, sink := range sinks {
            sink.RunFinished(summary)
        }
        return summary, err //nolint:wrapcheck // interface-boundary passthrough; wrapping breaks classifyStatus's ctx.Err() identity check for cancelled-during-Start scenarios
        return summary, runErr
    }

var (

Mcmd/entire/cli/review/run.go+7/-2

154 unmodified lines

155
156
157
158
159
160
158
159
160
161
162
163
162
163
164
165
164
165
166
132 unmodified lines

299
300
301
302
303
304
305
306
307
308
309
310
311
312

154 unmodified lines

proc, err := r.Start(agentCtx, cfg)
        if err != nil {
            // No Process exists, so there is no Events/Wait lifecycle to preserve.
            // Cancel immediately to release the per-agent timeout timer while
            // siblings continue running. Queue the terminal marker so the dispatch
            // loop remains the single writer of perAgentState terminal fields.
            // Build the terminal marker before cancelAgent so a Start call that blocked
            // until the per-reviewer deadline keeps the reviewer-timeout cause visible.
            // Then cancel immediately to release the per-agent timeout timer while
            // siblings continue running.
            startTerminals = append(startTerminals, startFailureTerminal(ctx, agentCtx, i, err))
            cancelAgent()
            startTerminals = append(startTerminals, taggedEvent{agentIdx: i, terminal: &agentTerminal{
                startErr:   err,
                finishedAt: time.Now(),
            }})
            continue
        }
        states[i].proc = proc
132 unmodified lines

return summary, firstErr
}

func startFailureTerminal(parentCtx, agentCtx context.Context, agentIdx int, startErr error) taggedEvent {
    return taggedEvent{agentIdx: agentIdx, terminal: &agentTerminal{
        startErr:   startErr,
        finishedAt: time.Now(),
        timedOut:   reviewerDeadlineFired(parentCtx, agentCtx, startErr),
    }}
}

func emitEnrichedAgentTokens(
    ctx context.Context,
    cfg reviewtypes.RunConfig,

Mcmd/entire/cli/review/run_multi.go+13/-7

39 unmodified lines

40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
618 unmodified lines

678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
67 unmodified lines

782
783
784
785
786
787
788
789
790
791
792
793
794
795
796
797
798
799
800
801
802
803
804
805
806
807
808
809
810
811
812
813
814
815
816
817
818

39 unmodified lines

return p.ctx.Err()
}

type startBlockingReviewer struct {
    name       string
    stringWrap bool
}

func (r *startBlockingReviewer) Name() string { return r.name }
func (r *startBlockingReviewer) Start(ctx context.Context, _ reviewtypes.RunConfig) (reviewtypes.Process, error) {
    <-ctx.Done()
    if r.stringWrap {
        return nil, errors.New("start failed: " + ctx.Err().Error())
    }
    return nil, ctx.Err()
}

type stringWrappedCtxReviewer struct{ name string }

func (r *stringWrappedCtxReviewer) Name() string { return r.name }
618 unmodified lines

}
}

func TestRun_ReviewerTimeoutDuringStart(t *testing.T) {
    t.Parallel()
    rec := &stubSinkRecorder{}
    summary, err := Run(
        context.Background(),
        &startBlockingReviewer{name: "slow-start"},
        reviewtypes.RunConfig{ReviewerTimeout: 20 * time.Millisecond},
        []reviewtypes.Sink{rec},
    )

if err == nil || !strings.Contains(err.Error(), "timed out") {
        t.Fatalf("err = %v, want a 'timed out' error", err)
    }
    if summary.Cancelled {
                                                
                                            
                                                
                                            
    }
    if len(summary.AgentRuns) != 1 {
        t.Fatalf("AgentRuns = %d, want 1", len(summary.AgentRuns))
    }
    run := summary.AgentRuns[0]
    if run.Status != reviewtypes.AgentStatusFailed {
        t.Fatalf("status = %v, want Failed", run.Status)
    }
    if run.Err == nil || !strings.Contains(run.Err.Error(), "timed out") {
        t.Fatalf("run.Err = %v, want 'timed out'", run.Err)
    }
    if len(rec.finishedCalls) != 1 {
        t.Fatalf("RunFinished calls = %d, want 1", len(rec.finishedCalls))
    }
}

func TestRun_ReviewerTimeout(t *testing.T) {
    t.Parallel()
    rec := &stubSinkRecorder{}
67 unmodified lines

}

func TestRunMulti_ReviewerTimeoutDuringStart(t *testing.T) {
    t.Parallel()
    rec := &stubSinkRecorder{}
    summary, err := RunMulti(
        context.Background(),
        []reviewtypes.AgentReviewer{&startBlockingReviewer{name: "slow-start", stringWrap: true}},
        reviewtypes.RunConfig{ReviewerTimeout: 20 * time.Millisecond},
        []reviewtypes.Sink{rec},
    )

func TestRunMulti_ReviewerTimeoutIsolated(t *testing.T) {
    t.Parallel()
    // One reviewer hangs (times out); a sibling finishes cleanly. The run is