From 9c5f11502e17221f074a1413c1a81c2269aed7ba Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Wed, 28 Oct 2026 18:29:48 -0400 Subject: [PATCH 1/3] fix(exec): keep completed calls safe from late cancellation Carry forward the completed-call cancellation fix and regression from santoshkumarradha in #1738 (original commit b11e3d7a1b523464490e6b5cf87ccbb00da695cd). Pairing and moving chats already shipped; preserve only this missing cancellation behavior on current dev. Co-authored-by: santoshkumarradha --- .../0-finished-shell-call-cancellation.md | 16 +++++++ internal/exec/bare/promote.go | 7 ++++ internal/exec/bare/watchcancel_test.go | 42 +++++++++++++++++++ .../manual/chat/running-from-the-terminal.md | 4 ++ 4 files changed, 69 insertions(+) create mode 100644 docs/changes/unreleased/0-finished-shell-call-cancellation.md create mode 100644 internal/exec/bare/watchcancel_test.go diff --git a/docs/changes/unreleased/0-finished-shell-call-cancellation.md b/docs/changes/unreleased/0-finished-shell-call-cancellation.md new file mode 100644 index 0000000000..42a9c1814a --- /dev/null +++ b/docs/changes/unreleased/0-finished-shell-call-cancellation.md @@ -0,0 +1,16 @@ +--- +kind: fixed +title: Finished shell calls keep their background processes after a late cancellation +pr: 0 +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." +--- + +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. + +Carries forward santoshkumarradha's completed-call cancellation fix and regression +test from #1738. Pairing and moving chats already shipped separately; this change +does not reintroduce the old relay or its model fallback and skill import policies. 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/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 ``` From ccfbd81b1571b75570e7e783e30d40897323b6cd Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Wed, 28 Oct 2026 18:38:37 -0400 Subject: [PATCH 2/3] fix(provider): respect explicit fallback lists before catalog guesses Carry forward santoshkumarradha PR #1738 commit a8039c5cf387fdb1a90cfa2e1273a254bf7b8ab3: naming only the failing model must not permit a catalog guess, while a list of blanks still permits inference. Retain the original regression and document the behavior. Co-authored-by: santoshkumarradha --- ...0-explicit-fallbacks-and-finished-calls.md | 22 ++++++++++++++++ .../0-finished-shell-call-cancellation.md | 16 ------------ internal/manual/chat/models-and-cost.md | 8 ++++++ internal/provider/endpoints.go | 14 +++++++++- internal/provider/fallback_self_test.go | 26 +++++++++++++++++++ 5 files changed, 69 insertions(+), 17 deletions(-) create mode 100644 docs/changes/unreleased/0-explicit-fallbacks-and-finished-calls.md delete mode 100644 docs/changes/unreleased/0-finished-shell-call-cancellation.md create mode 100644 internal/provider/fallback_self_test.go diff --git a/docs/changes/unreleased/0-explicit-fallbacks-and-finished-calls.md b/docs/changes/unreleased/0-explicit-fallbacks-and-finished-calls.md new file mode 100644 index 0000000000..a63c653a0f --- /dev/null +++ b/docs/changes/unreleased/0-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: 0 +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/docs/changes/unreleased/0-finished-shell-call-cancellation.md b/docs/changes/unreleased/0-finished-shell-call-cancellation.md deleted file mode 100644 index 42a9c1814a..0000000000 --- a/docs/changes/unreleased/0-finished-shell-call-cancellation.md +++ /dev/null @@ -1,16 +0,0 @@ ---- -kind: fixed -title: Finished shell calls keep their background processes after a late cancellation -pr: 0 -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." ---- - -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. - -Carries forward santoshkumarradha's completed-call cancellation fix and regression -test from #1738. Pairing and moving chats already shipped separately; this change -does not reintroduce the old relay or its model fallback and skill import policies. 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/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) + } +} From cbf8963c4c95125d6f7e581b4441d831230a17e7 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Wed, 28 Oct 2026 18:41:59 -0400 Subject: [PATCH 3/3] docs(changes): number the explicit-fallbacks entry for #2020 Co-Authored-By: Claude Opus 5.5 (1M context) --- ...d-calls.md => 2020-explicit-fallbacks-and-finished-calls.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename docs/changes/unreleased/{0-explicit-fallbacks-and-finished-calls.md => 2020-explicit-fallbacks-and-finished-calls.md} (99%) diff --git a/docs/changes/unreleased/0-explicit-fallbacks-and-finished-calls.md b/docs/changes/unreleased/2020-explicit-fallbacks-and-finished-calls.md similarity index 99% rename from docs/changes/unreleased/0-explicit-fallbacks-and-finished-calls.md rename to docs/changes/unreleased/2020-explicit-fallbacks-and-finished-calls.md index a63c653a0f..1ddca05043 100644 --- a/docs/changes/unreleased/0-explicit-fallbacks-and-finished-calls.md +++ b/docs/changes/unreleased/2020-explicit-fallbacks-and-finished-calls.md @@ -1,7 +1,7 @@ --- kind: fixed title: Explicit fallback lists hold, and finished shell calls keep background processes -pr: 0 +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."