inspect: document immediate cleanup on start errors · Entire

inspect: document immediate cleanup on start errors

79f2992→main

dipree·1mo ago·3 files·+93 added/-3 removed

When a reviewer fails in Start, no Process exists for the orchestrator to drain, so RunMulti cancels the per-agent timeout context immediately to release its timer while siblings continue. Document that Start errors must not retain ctx-bound background work, and add a regression test proving start-error contexts are cancelled before the multi-agent run completes.

Sessions

4cd5b996d828View transcript

?\Checkout the hand off doc that I just added.Pi·Opus 4.8·2 steps

Changes

3

147 unmodified lines

148
149
150
151
152
153
154
155
156

147 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.
            cancelAgent()
            states[i].startErr = err
            states[i].finishedAt = time.Now()

Mcmd/entire/cli/review/run_multi.go+3

623 unmodified lines

624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
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
671
672
673
674
675
676
677
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

623 unmodified lines

t.Errorf("AgentRun.Model = %q, want fallback to cfg.Model %q", got, "claude-sonnet-4-5")
    }
}

type startErrorContextReviewer struct {
    name     string
    startErr error
    ctx      context.Context
    started  chan struct{}
}

func (r *startErrorContextReviewer) Name() string { return r.name }

func (r *startErrorContextReviewer) Start(ctx context.Context, _ reviewtypes.RunConfig) (reviewtypes.Process, error) {
    r.ctx = ctx
    close(r.started)
    return nil, r.startErr
}

type blockingWaitReviewer struct {
    name    string
    release <-chan struct{}
}

func (r blockingWaitReviewer) Name() string { return r.name }

func (r blockingWaitReviewer) Start(context.Context, reviewtypes.RunConfig) (reviewtypes.Process, error) {
    return blockingWaitProcess{release: r.release}, nil
}

type blockingWaitProcess struct {
    release <-chan struct{}
}

func (p blockingWaitProcess) Events() <-chan reviewtypes.Event {
    ch := make(chan reviewtypes.Event)
    close(ch)
    return ch
}

func (p blockingWaitProcess) Wait() error {
    <-p.release
    return nil
}

func TestRunMulti_StartErrorCancelsAgentContextImmediately(t *testing.T) {
    t.Parallel()
    startErr := errors.New("start failed")
    bad := &startErrorContextReviewer{
        name:     "bad-start-agent",
        startErr: startErr,
        started:  make(chan struct{}),
    }
    releaseGood := make(chan struct{})
    good := blockingWaitReviewer{name: "still-running-agent", release: releaseGood}
    done := make(chan error, 1)
    go func() {
        _, err := RunMulti(context.Background(), []reviewtypes.AgentReviewer{bad, good}, reviewtypes.RunConfig{
            InspectorTimeout: time.Hour,
        }, nil)
        done <- err
    }()

select {
    case <-bad.started:
    case <-time.After(time.Second):
        t.Fatal("bad reviewer did not start")
    }
    if bad.ctx == nil {
        t.Fatal("bad reviewer did not capture context")
    }
    select {
    case <-bad.ctx.Done():
        // Correct: no process was returned, so the per-agent context is cleaned up
        // immediately even though another agent is still running.
    case <-time.After(time.Second):
        t.Fatal("start-error agent context was not cancelled while sibling continued running")
    }

close(releaseGood)
    select {
    case err := <-done:
        if !errors.Is(err, startErr) {
            t.Fatalf("RunMulti error = %v, want startErr", err)
        }
    case <-time.After(time.Second):
        t.Fatal("RunMulti did not finish after releasing sibling")
    }
}

Mcmd/entire/cli/review/run_multi_test.go+86

38 unmodified lines

39
40
41
42
43
44
42
43
44
45
46
47
48

38 unmodified lines

// lifecycle hooks adopt the session as a review session.
    //
    // Errors from Start indicate failure to construct or launch the process
    // (e.g., binary not on PATH at exec.Cmd.Start time, invalid argv). Once
    // Start returns nil, errors during the run flow through Process.Events
    // (as RunError) and Process.Wait.
    // (e.g., binary not on PATH at exec.Cmd.Start time, invalid argv). On error,
    // Start must not retain background work that depends on ctx; no Process exists
    // for the orchestrator to drain. Once Start returns nil, errors during the
    // run flow through Process.Events (as RunError) and Process.Wait.
    Start(ctx context.Context, run RunConfig) (Process, error)
}

Mcmd/entire/cli/review/types/reviewer.go+4/-3