review: make per-inspector timeout detection race-free; clarify matcher/defer · Entire

review: make per-inspector timeout detection race-free; clarify matcher/defer

f8b130a→main·

dipree·1mo ago·5 files·+64 added/-12 removed

Timeout detection (run.go + run_multi.go) sampled both the agent context and the parent context (ac.Err()==DeadlineExceeded && ctx.Err()==nil), which races: a parent cancel landing just after a real timeout would misclassify it as a cancellation. context.Err() is immutable once set, so the agent context alone is authoritative — DeadlineExceeded means this inspector's deadline fired first; a parent cancel propagates as Canceled. Drop the parent-ctx read entirely.

Also address two review notes that were false positives, with comments + tests rather than behavior changes:

Added TestRun_ParentCancelIsNotTimeout (parent cancel -> Cancelled, not timed out).

Sessions

0b9319108cb5View transcript

Changes

5

446 unmodified lines

// modelComponentsMatch reports whether the shorter component list `short`
// identifies the same model as the longer `long`: `short` must appear as a
// contiguous run of whole components in `long`, and the component immediately
// after that run must be purely numeric (a version or date). Requiring a
// numeric boundary is what lets "sonnet"/"claude-sonnet" match
// identifies the same model as the strictly longer `long`: `short` must appear
// as a contiguous run of whole components in `long`, and the component
// immediately after that run must be purely numeric (a version or date).
// Requiring a numeric boundary is what lets "sonnet"/"claude-sonnet" match
// "claude-sonnet-4-5" while rejecting variant suffixes like "gpt-4o-mini" and
// bare version fragments like "4-5".
//
// Equal-length cases are intentionally rejected here (`len(short) >= len(long)`):
// identical ids already returned true via reviewRunModelMatches's `want == got`
// short-circuit before this helper runs, and two *distinct* equal-length ids
// (e.g. "claude-sonnet" vs "claude-opus") are different models that must not
// match. Legitimate matches are always strict subsets, so `short` is shorter.
func modelComponentsMatch(short, long []string) bool {
    if len(short) == 0 || len(short) >= len(long) {
        return false
    }
}

Mcmd/entire/cli/review/manifest.go+10/-4

772 unmodified lines

{"version fragment does not match", "4-5", "claude-sonnet-4-5", false},
    {"different families do not match", "gpt-4o-mini", "claude-sonnet-4-5", false},
    {"opus does not match sonnet", "opus", "claude-sonnet-4-5", false},
    // Identical ids match regardless of component count (via the want==got
    // short-circuit), but two distinct equal-length ids must not.
    {"identical multi-component ids match", "claude-sonnet-4-5", "claude-sonnet-4-5", true},
    {"thinking-suffix-only difference matches", "claude-sonnet:high", "claude-sonnet:low", true},
    {"equal-length different family does not match", "claude-sonnet", "claude-opus", false},
    {"equal-length different version does not match", "claude-sonnet-4", "claude-sonnet-5", false},
}
for _, c := range cases {
    t.Run(c.name, func(t *testing.T) {

Mcmd/entire/cli/review/manifest_test.go+6

73 unmodified lines

// Bound the inspector so a stuck agent can't hang the review forever. The
    // deadline applies only to this agent; cancellation kills its process.
    // defer runs at function return (after proc.Wait below), so agentCtx stays
    // live for the whole run; deferring here — not after Start — also releases
    // the timer on the Start-error path instead of leaking it.
    timeout := inspectorTimeout(cfg)
    agentCtx, cancelAgent := context.WithTimeout(ctx, timeout)
    defer cancelAgent()
    53 unmodified lines
    waitErr := proc.Wait()
    finished := time.Now()
    // A per-inspector timeout fired iff this agent's deadline elapsed while the
    // parent run context is still live (a parent cancellation is a user Ctrl+C,
    // classified as Cancelled instead).
timedOut := agentCtx.Err() == context.DeadlineExceeded && ctx.Err() == nil
    // agentCtx.Err() is immutable once set, so DeadlineExceeded means this
    // inspector's own deadline fired first; a parent cancellation (user Ctrl+C)
    // propagates as Canceled instead. Reading only agentCtx is therefore
    // race-free — no need to also sample the parent ctx, which could change
    // between the two reads and misclassify a real timeout.
timedOut := agentCtx.Err() == context.DeadlineExceeded
    if shouldEmitSyntheticRunError(ctx, waitErr) {
        synthEvent := reviewtypes.RunError{Err: waitErr}
        buffer = append(buffer, synthEvent)
    }

Mcmd/entire/cli/review/run.go+9/-4

155 unmodified lines

finishedAt := time.Now()
    states[idx].waitErr = waitErr
    states[idx].finishedAt = finishedAt
    // A per-inspector timeout fired iff this agent's deadline elapsed
    // while the parent run context is still live (a parent cancellation
    // is a user Ctrl+C, classified as Cancelled).
states[idx].timedOut = ac.Err() == context.DeadlineExceeded && ctx.Err() == nil
    // ac.Err() is immutable once set, so DeadlineExceeded means THIS
    // agent's deadline fired first; a parent cancellation (user Ctrl+C)
    // propagates as Canceled instead. Reading only ac is race-free —
    // also sampling the parent ctx could change between the two reads and
    // misclassify a real timeout as a cancellation.
states[idx].timedOut = ac.Err() == context.DeadlineExceeded
    if shouldEmitSyntheticRunError(ctx, waitErr) {
        fanIn <- taggedEvent{agentIdx: idx, ev: reviewtypes.RunError{Err: waitErr}}
    }

Mcmd/entire/cli/review/run_multi.go+6/-4

590 unmodified lines

t.Errorf("fast status = %v, want Succeeded", byName["fast"].Status)
    }
}

// TestRun_ParentCancelIsNotTimeout pins the timeout-vs-cancel distinction: when
// 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 TestRun_ParentCancelIsNotTimeout(t *testing.T) {
    t.Parallel()
    ctx, cancel := context.WithCancel(context.Background())
    cancel() // cancel before the (1h) inspector deadline can elapse
    rec := &stubSinkRecorder{}
    summary, err := Run(
        ctx,
        &ctxReviewer{name: "claude-code"},
        reviewtypes.RunConfig{InspectorTimeout: time.Hour},
        []reviewtypes.Sink{rec},
    )
    if err != nil && strings.Contains(err.Error(), "timed out") {
        t.Errorf("returned err = %v, must not be a timeout for a parent cancel", err)
    }
    if !summary.Cancelled {
        t.Error("expected Cancelled=true for a parent-cancelled run")
    }
    if len(summary.AgentRuns) != 1 {
        t.Fatalf("expected 1 AgentRun, got %d", len(summary.AgentRuns))
    }
    run := summary.AgentRuns[0]
    if run.Status != reviewtypes.AgentStatusCancelled {
        t.Errorf("status = %v, want Cancelled", run.Status)
    }
    if run.Err != nil && strings.Contains(run.Err.Error(), "timed out") {
        t.Errorf("err = %v, must not be a timeout for a parent cancel", run.Err)
    }
}

Mcmd/entire/cli/review/run_test.go+33