Skip to content

Commit af03038

Browse files
AbirAbbassantoshkumarradhaclaude
authored
fix: honor explicit fallbacks and preserve finished shell calls (#2020)
* fix(exec): keep completed calls safe from late cancellation Carry forward the completed-call cancellation fix and regression from santoshkumarradha in #1738 (original commit b11e3d7). Pairing and moving chats already shipped; preserve only this missing cancellation behavior on current dev. Co-authored-by: santoshkumarradha <instrument.santosh@gmail.com> * fix(provider): respect explicit fallback lists before catalog guesses Carry forward santoshkumarradha PR #1738 commit a8039c5: 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 <instrument.santosh@gmail.com> * docs(changes): number the explicit-fallbacks entry for #2020 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: santoshkumarradha <instrument.santosh@gmail.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 3bf6072 commit af03038

7 files changed

Lines changed: 122 additions & 1 deletion

File tree

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
kind: fixed
3+
title: Explicit fallback lists hold, and finished shell calls keep background processes
4+
pr: 2020
5+
surface: [chat, engine]
6+
invalidates:
7+
- "A cancellation watcher could kill background processes after their shell call had already finished. A completed call now takes precedence over a late cancellation."
8+
- "A fallback list containing only the failing model could trigger catalog guesses. Only a list with no named model now allows a catalog suggestion."
9+
---
10+
11+
When a shell call's completion and cancellation are both ready, the cancellation
12+
watcher checks completion before signalling the process group. Commands that are
13+
still running retain their existing cancellation behavior.
14+
15+
A fallback list that names only the current model leaves no model to try after
16+
it. Duplicates and surrounding blanks keep that choice; a list of only blanks
17+
still allows a catalog suggestion.
18+
19+
Carries forward santoshkumarradha's completed-call cancellation and explicit-list
20+
fixes and regressions from #1738. Pairing and moving chats already shipped
21+
separately. The draft's broader task/checker pin and skill import policies remain
22+
separate design decisions.

‎internal/exec/bare/promote.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,13 @@ func watchCancel(ctx context.Context, call *BashCall, done <-chan struct{}) {
263263
guard.Go("exec/bare cancel watch", func() {
264264
select {
265265
case <-ctx.Done():
266+
// Both cases can be ready after a successful call returns. Its
267+
// background processes no longer belong to this cancellation.
268+
select {
269+
case <-done:
270+
return
271+
default:
272+
}
266273
if _, _, adopted := call.close(); !adopted {
267274
killProcessGroup(call.cmd)
268275
}
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
//go:build !windows
2+
3+
package bare
4+
5+
import (
6+
"context"
7+
"os/exec"
8+
"syscall"
9+
"testing"
10+
"time"
11+
12+
"github.com/Agent-Field/codeaf/internal/processgroup"
13+
)
14+
15+
// A call that has already returned is not killed by the cancellation that
16+
// follows it. Both of the watcher's cases are ready when the call's own context
17+
// is cancelled just behind its return; the group it left running must survive
18+
// whichever the watcher is handed.
19+
func TestACallThatReturnedLeavesItsBackgroundProcessAlone(t *testing.T) {
20+
cmd := exec.Command("sleep", "30")
21+
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
22+
if err := cmd.Start(); err != nil {
23+
t.Fatal(err)
24+
}
25+
pgid := cmd.Process.Pid
26+
// The process is reaped as soon as it dies, so a killed group reads as gone.
27+
reaped := make(chan struct{})
28+
go func() { _ = cmd.Wait(); close(reaped) }()
29+
t.Cleanup(func() { _ = syscall.Kill(-pgid, syscall.SIGKILL); <-reaped })
30+
31+
for i := 0; i < 200; i++ {
32+
ctx, cancel := context.WithCancel(context.Background())
33+
over := make(chan struct{})
34+
cancel()
35+
close(over)
36+
watchCancel(ctx, newBashCall("sleep 30", cmd, nil, ctx), over)
37+
}
38+
time.Sleep(50 * time.Millisecond)
39+
if !processgroup.Alive(pgid) {
40+
t.Fatal("the cancellation after a finished call killed the group it left running")
41+
}
42+
}

‎internal/manual/chat/models-and-cost.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,13 @@
11
# Models, context, and what it costs
22

3+
## A fallback list naming only the current model — keep the answer on that model
4+
5+
The `fallback models` row is your list of other models a failed request may try.
6+
If it names only the current model, there is no other model to try: codeaf does
7+
not replace it with a suggestion from the catalog.
8+
Repeating the same model or adding blank rows does not change that choice.
9+
A list containing only blanks leaves the catalog free to suggest another model.
10+
311
## Lost internet, Wi-Fi disconnected, DNS errors, and waiting for connection
412

513
When DNS or a connection attempt fails before the request is accepted, codeaf

‎internal/manual/chat/running-from-the-terminal.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1233,6 +1233,10 @@ own keypair, and `CODEAF_MODEL_POOL_RELAY_URL` with `models.pool.public_key`
12331233
server, a watcher — one per line, tab separated: the name, the status, how long it has been
12341234
up, what is watched for health, and where its log is.
12351235

1236+
A shell command that has already finished keeps any background server or watcher
1237+
it left running when the turn ends. Interrupting a command that is still running
1238+
stops the command and its child processes.
1239+
12361240
```
12371241
dev-server running 2h port:5173 /tmp/dev.log
12381242
```

‎internal/provider/endpoints.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -890,6 +890,18 @@ func (c *Client) FallbackModels(model string) []string {
890890
return carried
891891
}
892892

893+
// fallbacksWritten reports whether the configured list names any model. A list
894+
// naming only the failing model is still an explicit answer, even though that
895+
// model is removed from the chain; blank rows leave the catalog free to help.
896+
func (c *Client) fallbacksWritten() bool {
897+
for _, model := range c.config.Fallbacks {
898+
if strings.TrimSpace(model) != "" {
899+
return true
900+
}
901+
}
902+
return false
903+
}
904+
893905
// fallbackChain is the models to try after the ladder, in order.
894906
//
895907
// 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 {
913925
}
914926
}
915927
add(c.config.Fallbacks)
916-
if len(chain) == 0 && c.config.NearestModels != nil {
928+
if !c.fallbacksWritten() && c.config.NearestModels != nil {
917929
add(c.config.NearestModels(model))
918930
}
919931
if len(chain) > maxFallbackModels {
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package provider
2+
3+
import "testing"
4+
5+
func TestAFallbackListNamingOnlyTheFailingModelIsAPin(t *testing.T) {
6+
guessed := func(string) []string { return []string{"guessed/one", "guessed/two"} }
7+
for name, written := range map[string][]string{
8+
"the pinned model alone": {"sim/model"},
9+
"the pinned model repeated": {"sim/model", " SIM/model "},
10+
"blank rows around the pin": {"", "sim/model"},
11+
} {
12+
client, _ := refusingClient(t, func(map[string]any) bool { return false }, Config{
13+
Fallbacks: written, NearestModels: guessed,
14+
})
15+
if got := client.FallbackModels("sim/model"); len(got) != 0 {
16+
t.Fatalf("%s: FallbackModels = %v, want none: a pin sends no call to another model", name, got)
17+
}
18+
}
19+
// A row of blanks is no answer at all, and the catalog still helps.
20+
client, _ := refusingClient(t, func(map[string]any) bool { return false }, Config{
21+
Fallbacks: []string{" ", ""}, NearestModels: guessed,
22+
})
23+
if got := client.FallbackModels("sim/model"); len(got) != 2 {
24+
t.Fatalf("FallbackModels = %v, want the catalog's two for a row that said nothing", got)
25+
}
26+
}

0 commit comments

Comments
 (0)