inspect: detect timeouts from inspector context fallback · Entire
inspect: detect timeouts from inspector context fallback
3440616→main·
dipree·4w ago·3 files·+98 added/-11 removed
Some Process.Wait implementations may format ctx.Err() without preserving the context.DeadlineExceeded sentinel. Keep waitErr as the primary classification source, but fall back to the per-inspector context when Wait returned an error and that context's own deadline fired before any parent deadline. Nil Wait still means natural completion, preserving the existing race-safe behavior.
Covers both Run and RunMulti with regressions for string-wrapped context errors.
Sessions
2578476c3de8View transcript
[?
Checkout the hand off doc that I just added.Pi·Opus 4.8·3 steps](/content/gh/entireio/cli/session/019eca64-8c2c-7b00-90c6-3aa49738c497#timeline-2578476c3de8/index.html)
Changes
3
cmd/entire/cli/review
Mrun.go+23/-4
Mrun_multi.go+6/-7
Mrun_test.go+69
53 unmodified lines
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
92 unmodified lines
170
171
172
155
156
157
158
173
174
175
176
177
178
179
180
53 unmodified lines
}
}
func inspectorDeadlineFired(parentCtx, agentCtx context.Context, waitErr error) bool {
if waitErr == nil {
return false
}
if errors.Is(waitErr, context.DeadlineExceeded) {
return true
}
if !errors.Is(agentCtx.Err(), context.DeadlineExceeded) {
return false
}
agentDeadline, ok := agentCtx.Deadline()
if !ok {
return false
}
parentDeadline, parentHasDeadline := parentCtx.Deadline()
return !parentHasDeadline || agentDeadline.Before(parentDeadline)
}
// timedOutError reports the per-inspector timeout as a user-facing error.
func timedOutError(agent string, timeout time.Duration) error {
return fmt.Errorf("review agent %s timed out after %s", agent, timeout)
}
92 unmodified lines
// Classify from waitErr, the termination cause captured when Wait returned:
// the Process contract returns DeadlineExceeded when the process was killed
// by this inspector's deadline and Canceled on a parent cancellation (user
// Ctrl+C). Using waitErr instead of re-sampling agentCtx avoids a deadline
// firing in the gap after a natural completion (waitErr == nil) and producing
// a false timeout.
timedOut := errors.Is(waitErr, context.DeadlineExceeded)
// Ctrl+C). If an implementation formats ctx.Err() without preserving the
// sentinel, fall back to the per-agent context only when Wait returned an
// error; this avoids a deadline firing after a natural completion (waitErr ==
// nil) and producing a false timeout.
timedOut := inspectorDeadlineFired(ctx, agentCtx, waitErr)
if shouldEmitSyntheticRunError(ctx, waitErr) {
synthEvent := reviewtypes.RunError{Err: waitErr}
buffer = append(buffer, synthEvent)
}
Mcmd/entire/cli/review/run.go+23/-4
24 unmodified lines
25
26
27
28
28
29
30
126 unmodified lines
157
158
159
161
160
161
162
163
14 unmodified lines
178
179
180
182
183
184
181
182
183
184
185
186
188
187
188
190
189
190
191
192
24 unmodified lines
import (
"context"
"errors"
"log/slog"
"sync"
"time"
126 unmodified lines
}
states[i].proc = proc
wg.Add(1)
go func(idx int, p reviewtypes.Process, cancel context.CancelFunc) {
go func(idx int, p reviewtypes.Process, runCtx context.Context, cancel context.CancelFunc) {
defer wg.Done()
defer cancel()
for ev := range p.Events() {
14 unmodified lines
})
// Hand the terminal result to the dispatch loop so perAgentState has a
// single writer. Classify the timeout from waitErr (the cause the
// Process captured at Wait): DeadlineExceeded means this agent's deadline
// fired; a parent cancel is Canceled; a natural completion is nil even if
// the deadline elapses a moment later.
// Process captured at Wait). If an implementation formats ctx.Err()
// without preserving the sentinel, fall back to the per-agent context only
// when Wait returned an error; nil Wait still means natural completion.
fanIn <- taggedEvent{agentIdx: idx, terminal: &agentTerminal{
waitErr: waitErr,
finishedAt: finishedAt,
timedOut: errors.Is(waitErr, context.DeadlineExceeded),
timedOut: inspectorDeadlineFired(ctx, runCtx, waitErr),
}}
}(i, proc, cancelAgent)
}(i, proc, agentCtx, cancelAgent)
}
// Close fanIn after all forwarding goroutines finish. This goroutine
Mcmd/entire/cli/review/run_multi.go+6/-7
35 unmodified lines
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
515 unmodified lines
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
33 unmodified lines
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
35 unmodified lines
return p.ctx.Err()
}
type stringWrappedCtxReviewer struct{ name string }
func (r *stringWrappedCtxReviewer) Name() string { return r.name }
func (r *stringWrappedCtxReviewer) Start(ctx context.Context, _ reviewtypes.RunConfig) (reviewtypes.Process, error) {
return &stringWrappedCtxProcess{ctx: ctx}, nil
}
type stringWrappedCtxProcess struct{ ctx context.Context }
func (p *stringWrappedCtxProcess) Events() <-chan reviewtypes.Event {
out := make(chan reviewtypes.Event)
go func() {
<-p.ctx.Done()
close(out)
}()
return out
}
func (p *stringWrappedCtxProcess) Wait() error {
<-p.ctx.Done()
// Deliberately do NOT wrap with %w. This models an adapter that formats the
// context error and loses the context.DeadlineExceeded sentinel.
return errors.New("agent failed: " + p.ctx.Err().Error())
}
// stubReviewer is a test double for reviewtypes.AgentReviewer.
type stubReviewer struct {
name string
515 unmodified lines
}
func TestRun_InspectorTimeoutWithStringWrappedContextError(t *testing.T) {
t.Parallel()
summary, err := Run(
context.Background(),
&stringWrappedCtxReviewer{name: "claude-code"},
reviewtypes.RunConfig{InspectorTimeout: 30 * time.Millisecond},
nil,
)
if err == nil || !strings.Contains(err.Error(), "timed out") {
t.Fatalf("err = %v, want a 'timed out' error", err)
}
if summary.Cancelled {
t.Error("Cancelled should be false for a per-inspector timeout")
}
if len(summary.AgentRuns) != 1 {
t.Fatalf("expected 1 AgentRun, got %d", len(summary.AgentRuns))
}
if run := summary.AgentRuns[0]; run.Status != reviewtypes.AgentStatusFailed || run.Err == nil || !strings.Contains(run.Err.Error(), "timed out") {
t.Fatalf("run = {Status:%v Err:%v}, want Failed with timed-out error", run.Status, run.Err)
}
}
func TestRunMulti_InspectorTimeoutIsolated(t *testing.T) {
t.Parallel()
// One inspector hangs (times out); a sibling finishes cleanly. The run is
33 unmodified lines
// the parent context is cancelled (user Ctrl+C) before an inspector's deadline
// can fire, the inspector is classified Cancelled, not failed-by-timeout. The
// detection reads only the agent context, whose Err() is immutable once set.
func TestRunMulti_InspectorTimeoutWithStringWrappedContextError(t *testing.T) {
t.Parallel()
summary, err := RunMulti(
context.Background(),
[]reviewtypes.AgentReviewer{&stringWrappedCtxReviewer{name: "slow"}},
reviewtypes.RunConfig{InspectorTimeout: 30 * time.Millisecond},
nil,
)
if err == nil || !strings.Contains(err.Error(), "timed out") {
t.Fatalf("err = %v, want a 'timed out' error", err)
}
if summary.Cancelled {
t.Error("Cancelled should be false for a per-inspector timeout")
}
if len(summary.AgentRuns) != 1 {
t.Fatalf("expected 1 AgentRun, got %d", len(summary.AgentRuns))
}
if run := summary.AgentRuns[0]; run.Status != reviewtypes.AgentStatusFailed || run.Err == nil || !strings.Contains(run.Err.Error(), "timed out") {
t.Fatalf("run = {Status:%v Err:%v}, want Failed with timed-out error", run.Status, run.Err)
}
}
func TestRun_ParentCancelIsNotTimeout(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithCancel(context.Background())