diff --git a/docs/changes/unreleased/2020-explicit-fallbacks-and-finished-calls.md b/docs/changes/unreleased/2020-explicit-fallbacks-and-finished-calls.md new file mode 100644 index 0000000000..1ddca05043 --- /dev/null +++ b/docs/changes/unreleased/2020-explicit-fallbacks-and-finished-calls.md @@ -0,0 +1,22 @@ +--- +kind: fixed +title: Explicit fallback lists hold, and finished shell calls keep background processes +pr: 2020 +surface: [chat, engine] +invalidates: + - "A cancellation watcher could kill background processes after their shell call had already finished. A completed call now takes precedence over a late cancellation." + - "A fallback list containing only the failing model could trigger catalog guesses. Only a list with no named model now allows a catalog suggestion." +--- + +When a shell call's completion and cancellation are both ready, the cancellation +watcher checks completion before signalling the process group. Commands that are +still running retain their existing cancellation behavior. + +A fallback list that names only the current model leaves no model to try after +it. Duplicates and surrounding blanks keep that choice; a list of only blanks +still allows a catalog suggestion. + +Carries forward santoshkumarradha's completed-call cancellation and explicit-list +fixes and regressions from #1738. Pairing and moving chats already shipped +separately. The draft's broader task/checker pin and skill import policies remain +separate design decisions. diff --git a/internal/exec/bare/promote.go b/internal/exec/bare/promote.go index abe88e5b8b..c4fd79c856 100644 --- a/internal/exec/bare/promote.go +++ b/internal/exec/bare/promote.go @@ -263,6 +263,13 @@ func watchCancel(ctx context.Context, call *BashCall, done <-chan struct{}) { guard.Go("exec/bare cancel watch", func() { select { case <-ctx.Done(): + // Both cases can be ready after a successful call returns. Its + // background processes no longer belong to this cancellation. + select { + case <-done: + return + default: + } if _, _, adopted := call.close(); !adopted { killProcessGroup(call.cmd) } diff --git a/internal/exec/bare/watchcancel_test.go b/internal/exec/bare/watchcancel_test.go new file mode 100644 index 0000000000..913ed43472 --- /dev/null +++ b/internal/exec/bare/watchcancel_test.go @@ -0,0 +1,42 @@ +//go:build !windows + +package bare + +import ( + "context" + "os/exec" + "syscall" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/processgroup" +) + +// A call that has already returned is not killed by the cancellation that +// follows it. Both of the watcher's cases are ready when the call's own context +// is cancelled just behind its return; the group it left running must survive +// whichever the watcher is handed. +func TestACallThatReturnedLeavesItsBackgroundProcessAlone(t *testing.T) { + cmd := exec.Command("sleep", "30") + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + pgid := cmd.Process.Pid + // The process is reaped as soon as it dies, so a killed group reads as gone. + reaped := make(chan struct{}) + go func() { _ = cmd.Wait(); close(reaped) }() + t.Cleanup(func() { _ = syscall.Kill(-pgid, syscall.SIGKILL); <-reaped }) + + for i := 0; i < 200; i++ { + ctx, cancel := context.WithCancel(context.Background()) + over := make(chan struct{}) + cancel() + close(over) + watchCancel(ctx, newBashCall("sleep 30", cmd, nil, ctx), over) + } + time.Sleep(50 * time.Millisecond) + if !processgroup.Alive(pgid) { + t.Fatal("the cancellation after a finished call killed the group it left running") + } +} diff --git a/internal/manual/chat/models-and-cost.md b/internal/manual/chat/models-and-cost.md index 77b8110d8b..465b99a659 100644 --- a/internal/manual/chat/models-and-cost.md +++ b/internal/manual/chat/models-and-cost.md @@ -1,5 +1,13 @@ # Models, context, and what it costs +## A fallback list naming only the current model — keep the answer on that model + +The `fallback models` row is your list of other models a failed request may try. +If it names only the current model, there is no other model to try: codeaf does +not replace it with a suggestion from the catalog. +Repeating the same model or adding blank rows does not change that choice. +A list containing only blanks leaves the catalog free to suggest another model. + ## Lost internet, Wi-Fi disconnected, DNS errors, and waiting for connection When DNS or a connection attempt fails before the request is accepted, codeaf diff --git a/internal/manual/chat/running-from-the-terminal.md b/internal/manual/chat/running-from-the-terminal.md index adc299c1fd..26825e9607 100644 --- a/internal/manual/chat/running-from-the-terminal.md +++ b/internal/manual/chat/running-from-the-terminal.md @@ -1233,6 +1233,10 @@ own keypair, and `CODEAF_MODEL_POOL_RELAY_URL` with `models.pool.public_key` server, a watcher — one per line, tab separated: the name, the status, how long it has been up, what is watched for health, and where its log is. +A shell command that has already finished keeps any background server or watcher +it left running when the turn ends. Interrupting a command that is still running +stops the command and its child processes. + ``` dev-server running 2h port:5173 /tmp/dev.log ``` diff --git a/internal/provider/endpoints.go b/internal/provider/endpoints.go index 6a45120bd3..978e84a02f 100644 --- a/internal/provider/endpoints.go +++ b/internal/provider/endpoints.go @@ -890,6 +890,18 @@ func (c *Client) FallbackModels(model string) []string { return carried } +// fallbacksWritten reports whether the configured list names any model. A list +// naming only the failing model is still an explicit answer, even though that +// model is removed from the chain; blank rows leave the catalog free to help. +func (c *Client) fallbacksWritten() bool { + for _, model := range c.config.Fallbacks { + if strings.TrimSpace(model) != "" { + return true + } + } + return false +} + // fallbackChain is the models to try after the ladder, in order. // // The operator's own list wins outright, and the catalog is only asked when @@ -913,7 +925,7 @@ func (c *Client) fallbackChain(model string) []string { } } add(c.config.Fallbacks) - if len(chain) == 0 && c.config.NearestModels != nil { + if !c.fallbacksWritten() && c.config.NearestModels != nil { add(c.config.NearestModels(model)) } if len(chain) > maxFallbackModels { diff --git a/internal/provider/fallback_self_test.go b/internal/provider/fallback_self_test.go new file mode 100644 index 0000000000..40d5dc1357 --- /dev/null +++ b/internal/provider/fallback_self_test.go @@ -0,0 +1,26 @@ +package provider + +import "testing" + +func TestAFallbackListNamingOnlyTheFailingModelIsAPin(t *testing.T) { + guessed := func(string) []string { return []string{"guessed/one", "guessed/two"} } + for name, written := range map[string][]string{ + "the pinned model alone": {"sim/model"}, + "the pinned model repeated": {"sim/model", " SIM/model "}, + "blank rows around the pin": {"", "sim/model"}, + } { + client, _ := refusingClient(t, func(map[string]any) bool { return false }, Config{ + Fallbacks: written, NearestModels: guessed, + }) + if got := client.FallbackModels("sim/model"); len(got) != 0 { + t.Fatalf("%s: FallbackModels = %v, want none: a pin sends no call to another model", name, got) + } + } + // A row of blanks is no answer at all, and the catalog still helps. + client, _ := refusingClient(t, func(map[string]any) bool { return false }, Config{ + Fallbacks: []string{" ", ""}, NearestModels: guessed, + }) + if got := client.FallbackModels("sim/model"); len(got) != 2 { + t.Fatalf("FallbackModels = %v, want the catalog's two for a row that said nothing", got) + } +}