Skip to content

Commit 4dc1487

Browse files
jkyberneeesclaude
andauthored
fix: tool resilience & crash-durable persistence (5 fixes) (#34)
* fix(shell): bound command execution with timeout + context cancellation shell ran cmd.Run() on a plain exec.Command with no timeout and no context, so a stuck command (network read that never returns, interactive prompt, infinite loop) wedged the agent forever — and Ctrl-C could not recover because the loop's drain blocks on the tool goroutine. The sibling parallel_shell already had a timeout; plain shell, the most-used tool, did not. - SetContext ties execution to the agent context (the loop already calls it on context-aware tools), so Ctrl-C / turn timeout kills the command now. - exec.CommandContext + a generous 30m backstop timeout bounds genuinely stuck commands for unattended runs (serve, telegram) with no human to interrupt. WaitDelay guarantees Run() returns even if the killed process leaves children holding the pipes. - Cancellation/timeout surface as clear errors, not opaque "signal: killed". Tests: a sleeping command now returns promptly via both the timeout and context-cancellation paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tools): propagate agent context so Ctrl-C/timeout interrupts tools Only delegate_tasks honored the loop's SetContext, so a turn timeout or Ctrl-C could not interrupt the other long-running tools — their HTTP requests and subprocesses ran to completion regardless, and the loop's drain blocked on them. Add a small race-safe ctxTool embeddable (SetContext + toolCtx) and wire it through the tools that do unbounded network or subprocess work: - http_batch, browser, web_search → http.NewRequestWithContext - vision (ffprobe/ffmpeg/llama-mtmd-cli), transcribe (ffmpeg/whisper) → exec.CommandContext - shell now uses the shared embed too (replacing its bare ctx field). The mutex in ctxTool matters: when the LLM emits two calls to the same tool in one turn, the loop sets the context on the shared instance from parallel goroutines — an unsynchronised field would race. Tests: ctxTool default/set/concurrent behavior, and http_batch returning a cancellation error when its context is already cancelled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(llm): give SimpleCall the same retry/backoff as the main loop SimpleCall did a single http.Do with no retry, so any transient 429/5xx or network blip aborted the best-effort secondary features that use it (skill matching, memory summaries, episode extraction, session titles), while the main agent Call retried. Extract the retry loop into postChatWithRetry and route both through it. Test: SimpleCall now succeeds after two 429s (3 attempts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(llm): honor Retry-After on rate-limited responses The retry loop used a fixed 1/2/4s exponential backoff and ignored the server's Retry-After header, so a real rate limit (Retry-After: 20-60s) burned all three retries in ~7s and failed the turn even though the server said exactly when to come back. Parse Retry-After (integer seconds or HTTP-date) on retryable statuses and use it for the next wait, capped at 60s so a pathological value can't wedge a turn (ctx still breaks the wait). Tests: parseRetryAfter unit cases (seconds, blank, garbage, zero, cap) and a 429+Retry-After call that retries and succeeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(persist): fsync before rename so a crash can't lose the latest write Session and memory writes did WriteFile(tmp)+Rename — atomic against torn reads, but NOT durable: without an fsync a power loss / kernel crash can land the rename while the data is still in the page cache, leaving an empty or truncated file and silently losing the latest conversation turn or extracted memory. session.go also used a fixed "<target>.tmp" name, so two concurrent saves of the same target could clobber each other's temp file. Add internal/fsatomic.WriteFile (unique temp → fsync data → rename → fsync dir) and route the irreplaceable writes through it: session save + index, episode index + summaries, and the facts store. Tests: fsatomic content/perm/overwrite, no temp litter, and concurrent same-target writers never producing a torn file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(shell): note docker-exec cancellation semantics vprotocol: in sandbox mode the timeout/ctx kills the host-side docker exec client (unblocking the agent) but Docker does not forward the signal to the in-container process, which lingers until container teardown. Document the sharp edge so it isn't a future debugging mystery. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(shell): kill the whole process group on cancel/timeout CI caught it: the two shell timeout tests passed locally but hung ~5s on CI. exec.CommandContext's default Cancel SIGKILLs only the `sh` leader. On a shell that forks the command (`sh -c "sleep 30"` → child sleep, or any pipeline), the child survives and holds the output pipe, so Run() blocks until WaitDelay (5s) — over the test's deadline, and a real 30m-timeout command would leave a lingering process. Run the command in its own process group (Setpgid) and override Cancel to SIGKILL the whole group (negative pid). The repo is Unix-only and already uses syscall.Kill directly. WaitDelay drops to 3s as a backstop. Tests now use a forking pipeline (`sleep 30 | cat`) so they reproduce the CI failure locally; both return in <0.2s with the group kill. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 6a2055c commit 4dc1487

16 files changed

Lines changed: 597 additions & 98 deletions

cmd/odek/browser_tool.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ type browserState struct {
5555
// ── Browser Tool ──────────────────────────────────────────────────────
5656

5757
type browserTool struct {
58+
ctxTool
5859
state *browserState
5960
client *http.Client
6061
dangerousConfig danger.DangerousConfig
@@ -196,7 +197,7 @@ func (t *browserTool) doNavigate(rawURL string) (string, error) {
196197
return jsonError(fmt.Sprintf("invalid URL %q: must start with http:// or https://", rawURL))
197198
}
198199

199-
req, err := http.NewRequest("GET", rawURL, nil)
200+
req, err := http.NewRequestWithContext(t.toolCtx(), "GET", rawURL, nil)
200201
if err != nil {
201202
return jsonError(fmt.Sprintf("cannot create request: %v", err))
202203
}

cmd/odek/perf_tools.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,7 @@ func (t *parallelShellTool) runOne(cmd parallelShellCmd) parallelShellEntry {
412412
const maxHTTPBatchURLs = 10
413413

414414
type httpBatchTool struct {
415+
ctxTool
415416
dangerousConfig danger.DangerousConfig
416417
client *http.Client
417418
}
@@ -556,7 +557,7 @@ func (t *httpBatchTool) fetchOne(r httpBatchReq) httpBatchEntry {
556557
}
557558

558559
entry := httpBatchEntry{URL: r.URL}
559-
httpReq, err := http.NewRequest(method, r.URL, nil)
560+
httpReq, err := http.NewRequestWithContext(t.toolCtx(), method, r.URL, nil)
560561
if err != nil {
561562
entry.Error = err.Error()
562563
return entry

cmd/odek/shell.go

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,26 @@ package main
22

33
import (
44
"bytes"
5+
"context"
56
"encoding/json"
67
"fmt"
78
"os/exec"
89
"strings"
910
"sync"
11+
"syscall"
12+
"time"
1013

1114
"github.com/BackendStack21/odek/internal/danger"
1215
)
1316

17+
// defaultShellTimeout bounds a single shell command. It is deliberately
18+
// generous — the goal is to stop a genuinely stuck command (a network read
19+
// that never returns, an interactive prompt, an infinite loop) from wedging
20+
// the agent forever, NOT to kill legitimate long builds or test suites. When
21+
// the agent context is cancelled (Ctrl-C, turn timeout) the command is killed
22+
// immediately regardless of this backstop.
23+
const defaultShellTimeout = 30 * time.Minute
24+
1425
// shellTool is odek's built-in tool that lets the agent run shell commands.
1526
//
1627
// This is the only built-in tool — it's enough for reading files, running
@@ -61,6 +72,13 @@ type shellTool struct {
6172
// ttyPath is the path to the terminal device for approval prompts.
6273
// Overridden in tests to mock user input. Only used when approver is nil.
6374
ttyPath string
75+
76+
// ctxTool provides SetContext/toolCtx so cancelling the agent context
77+
// (Ctrl-C, turn timeout) kills the running command.
78+
ctxTool
79+
80+
// timeout bounds a single command. Zero falls back to defaultShellTimeout.
81+
timeout time.Duration
6482
}
6583

6684
func (t *shellTool) Name() string { return "shell" }
@@ -113,13 +131,52 @@ func (t *shellTool) Call(args string) (string, error) {
113131
return "", err
114132
}
115133

116-
cmd := t.buildCmd(input.Command)
134+
// Bound execution: cancel with the agent context (Ctrl-C / turn timeout)
135+
// and a generous backstop timeout so a stuck command can never wedge the
136+
// agent forever. Note: in sandbox mode this kills the host-side
137+
// `docker exec` client, which unblocks the agent, but Docker does not
138+
// propagate the signal to the in-container process — that lingers until the
139+
// container is torn down at session end.
140+
base := t.toolCtx()
141+
timeout := t.timeout
142+
if timeout <= 0 {
143+
timeout = defaultShellTimeout
144+
}
145+
ctx, cancel := context.WithTimeout(base, timeout)
146+
defer cancel()
147+
148+
cmd := t.buildCmd(ctx, input.Command)
149+
// Run the command in its own process group and, on cancel/timeout, kill the
150+
// WHOLE group — not just the `sh` leader. `sh -c "<cmd>"` may fork children
151+
// (e.g. `sleep`); killing only the leader leaves them alive holding the
152+
// output pipes, so Run() would block until WaitDelay. Signalling the group
153+
// (negative pid) tears the whole tree down at once.
154+
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
155+
cmd.Cancel = func() error {
156+
if cmd.Process != nil {
157+
// Best-effort group kill; ignore ESRCH if it already exited.
158+
_ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
159+
}
160+
return nil
161+
}
162+
// WaitDelay is a backstop in case a process somehow outlives the group kill.
163+
cmd.WaitDelay = 3 * time.Second
117164

118165
var outBuf, errBuf bytes.Buffer
119166
cmd.Stdout = &outBuf
120167
cmd.Stderr = &errBuf
121168

122169
err := cmd.Run()
170+
171+
// Surface cancellation/timeout as a clear, actionable error rather than an
172+
// opaque "signal: killed".
173+
if ctxErr := ctx.Err(); ctxErr != nil {
174+
if ctxErr == context.DeadlineExceeded {
175+
return "", fmt.Errorf("shell: command timed out after %s (still running? it was killed): %s", timeout, input.Command)
176+
}
177+
return "", fmt.Errorf("shell: command cancelled: %s", input.Command)
178+
}
179+
123180
output := strings.TrimSpace(outBuf.String())
124181
stderrStr := strings.TrimSpace(errBuf.String())
125182
if stderrStr != "" {
@@ -200,9 +257,9 @@ func (t *shellTool) promptUser(cmd, description string) error {
200257
//
201258
// When running on the host (default), the command executes via "sh -c"
202259
// in odek's current working directory.
203-
func (t *shellTool) buildCmd(command string) *exec.Cmd {
260+
func (t *shellTool) buildCmd(ctx context.Context, command string) *exec.Cmd {
204261
if t.containerName != "" {
205-
return exec.Command("docker", "exec", "-w", "/workspace", t.containerName, "sh", "-c", command)
262+
return exec.CommandContext(ctx, "docker", "exec", "-w", "/workspace", t.containerName, "sh", "-c", command)
206263
}
207-
return exec.Command("sh", "-c", command)
264+
return exec.CommandContext(ctx, "sh", "-c", command)
208265
}

cmd/odek/shell_test.go

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
package main
22

33
import (
4+
"context"
45
"encoding/json"
56
"os"
67
"os/exec"
78
"strings"
89
"testing"
10+
"time"
911
)
1012

1113
func TestShellTool_Name(t *testing.T) {
@@ -15,6 +17,60 @@ func TestShellTool_Name(t *testing.T) {
1517
}
1618
}
1719

20+
// TestShellTool_Timeout verifies a stuck command can no longer wedge the agent:
21+
// a tiny per-tool timeout kills the command and Call returns promptly with a
22+
// clear timeout error instead of blocking forever.
23+
func TestShellTool_Timeout(t *testing.T) {
24+
st := &shellTool{timeout: 200 * time.Millisecond}
25+
done := make(chan struct{})
26+
var out string
27+
var err error
28+
go func() {
29+
out, err = st.Call(`{"command":"sleep 30 | cat"}`)
30+
close(done)
31+
}()
32+
select {
33+
case <-done:
34+
case <-time.After(5 * time.Second):
35+
t.Fatal("Call did not return after the command timeout — agent would hang")
36+
}
37+
if err == nil {
38+
t.Fatalf("expected a timeout error, got output %q", out)
39+
}
40+
if !strings.Contains(err.Error(), "timed out") {
41+
t.Errorf("error should mention the timeout, got: %v", err)
42+
}
43+
}
44+
45+
// TestShellTool_ContextCancellation verifies Ctrl-C / turn cancellation kills a
46+
// running command immediately via the agent context.
47+
func TestShellTool_ContextCancellation(t *testing.T) {
48+
st := &shellTool{}
49+
ctx, cancel := context.WithCancel(context.Background())
50+
st.SetContext(ctx)
51+
52+
done := make(chan struct{})
53+
var err error
54+
go func() {
55+
_, err = st.Call(`{"command":"sleep 30 | cat"}`)
56+
close(done)
57+
}()
58+
time.Sleep(100 * time.Millisecond)
59+
cancel()
60+
61+
select {
62+
case <-done:
63+
case <-time.After(5 * time.Second):
64+
t.Fatal("Call did not return after context cancellation — Ctrl-C would not work")
65+
}
66+
if err == nil {
67+
t.Fatal("expected a cancellation error")
68+
}
69+
if !strings.Contains(err.Error(), "cancelled") {
70+
t.Errorf("error should mention cancellation, got: %v", err)
71+
}
72+
}
73+
1874
func TestShellTool_Description(t *testing.T) {
1975
st := &shellTool{}
2076
desc := st.Description()
@@ -130,7 +186,7 @@ func TestShellTool_Call_StdoutAndStderr(t *testing.T) {
130186

131187
func TestShellTool_BuildCmd_Local(t *testing.T) {
132188
st := &shellTool{}
133-
cmd := st.buildCmd("echo test")
189+
cmd := st.buildCmd(context.Background(), "echo test")
134190
args := cmd.Args
135191
if args[0] != "sh" || args[1] != "-c" || args[2] != "echo test" {
136192
t.Errorf("local cmd args = %v, want [sh -c 'echo test']", args)
@@ -139,7 +195,7 @@ func TestShellTool_BuildCmd_Local(t *testing.T) {
139195

140196
func TestShellTool_BuildCmd_Docker(t *testing.T) {
141197
st := &shellTool{containerName: "odek-12345"}
142-
cmd := st.buildCmd("echo test")
198+
cmd := st.buildCmd(context.Background(), "echo test")
143199
args := cmd.Args
144200
expected := []string{"docker", "exec", "-w", "/workspace", "odek-12345", "sh", "-c", "echo test"}
145201
if !stringSlicesEqual(args, expected) {
@@ -303,7 +359,7 @@ func TestShellTool_CheckApproval(t *testing.T) {
303359

304360
func TestShellTool_BuildCmd_Default(t *testing.T) {
305361
st := &shellTool{}
306-
cmd := st.buildCmd("echo hello")
362+
cmd := st.buildCmd(context.Background(), "echo hello")
307363
if cmd.Args[0] != "sh" {
308364
t.Errorf("expected sh, got %s", cmd.Args[0])
309365
}

cmd/odek/toolctx.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
package main
2+
3+
import (
4+
"context"
5+
"sync"
6+
)
7+
8+
// ctxTool is embedded by tools that support agent-context cancellation. The
9+
// agent loop calls SetContext on any tool implementing it (see internal/loop)
10+
// right before invoking the tool, so cancelling the agent context — Ctrl-C, a
11+
// turn timeout — interrupts the tool's in-flight network request or subprocess
12+
// instead of letting it run to completion (or hang) unsupervised.
13+
//
14+
// The mutex matters: when the LLM emits two calls to the SAME tool in one
15+
// turn, the loop runs them in parallel goroutines and calls SetContext on the
16+
// shared instance from each. Without synchronisation that is a data race on the
17+
// context field even though the value is identical for the turn.
18+
type ctxTool struct {
19+
mu sync.Mutex
20+
ctx context.Context
21+
}
22+
23+
// SetContext records the agent context for the next Call. Safe for concurrent
24+
// use by parallel invocations of the same tool instance.
25+
func (c *ctxTool) SetContext(ctx context.Context) {
26+
c.mu.Lock()
27+
c.ctx = ctx
28+
c.mu.Unlock()
29+
}
30+
31+
// toolCtx returns the recorded agent context, or context.Background() if none
32+
// was set (e.g. tools invoked directly in tests or outside the agent loop).
33+
func (c *ctxTool) toolCtx() context.Context {
34+
c.mu.Lock()
35+
defer c.mu.Unlock()
36+
if c.ctx == nil {
37+
return context.Background()
38+
}
39+
return c.ctx
40+
}

cmd/odek/toolctx_test.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package main
2+
3+
import (
4+
"context"
5+
"strings"
6+
"sync"
7+
"testing"
8+
)
9+
10+
func TestCtxTool_DefaultsToBackground(t *testing.T) {
11+
var c ctxTool
12+
if c.toolCtx() != context.Background() {
13+
t.Error("unset ctxTool should return context.Background()")
14+
}
15+
}
16+
17+
func TestCtxTool_SetAndGet(t *testing.T) {
18+
var c ctxTool
19+
ctx, cancel := context.WithCancel(context.Background())
20+
defer cancel()
21+
c.SetContext(ctx)
22+
if c.toolCtx() != ctx {
23+
t.Error("toolCtx should return the context set via SetContext")
24+
}
25+
}
26+
27+
// TestCtxTool_ConcurrentSetContext mirrors the loop calling SetContext on a
28+
// shared tool instance from parallel goroutines — it must be race-free.
29+
func TestCtxTool_ConcurrentSetContext(t *testing.T) {
30+
var c ctxTool
31+
ctx := context.Background()
32+
var wg sync.WaitGroup
33+
for i := 0; i < 100; i++ {
34+
wg.Add(1)
35+
go func() {
36+
defer wg.Done()
37+
c.SetContext(ctx)
38+
_ = c.toolCtx()
39+
}()
40+
}
41+
wg.Wait()
42+
}
43+
44+
// TestHTTPBatch_ContextCancelled verifies the propagated context aborts the
45+
// fetch: a cancelled context yields an error entry instead of a real request.
46+
func TestHTTPBatch_ContextCancelled(t *testing.T) {
47+
tool := newHTTPBatchTool(allowAllDanger())
48+
ctx, cancel := context.WithCancel(context.Background())
49+
cancel() // already cancelled
50+
tool.SetContext(ctx)
51+
52+
result := callJSON(t, tool, `{"requests":[{"url":"http://example.com/"}]}`)
53+
var r struct {
54+
Results []struct {
55+
Error string `json:"error"`
56+
} `json:"results"`
57+
}
58+
mustUnmarshal(t, result, &r)
59+
if len(r.Results) != 1 {
60+
t.Fatalf("Results = %d, want 1", len(r.Results))
61+
}
62+
if r.Results[0].Error == "" {
63+
t.Fatal("expected an error from the cancelled context")
64+
}
65+
if !strings.Contains(r.Results[0].Error, "context canceled") {
66+
t.Errorf("error should reflect cancellation, got: %s", r.Results[0].Error)
67+
}
68+
}

0 commit comments

Comments
 (0)