diff --git a/internal/agent/loop.go b/internal/agent/loop.go index 3e0804e0..83c46ed0 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -20,6 +20,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/i18n" "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/safego" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/tools" "github.com/nextlevelbuilder/goclaw/pkg/protocol" @@ -39,10 +40,13 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) l.emit(event) } - // Inject agent UUID into context for tool routing + // Inject agent UUID + key into context for tool routing if l.agentUUID != uuid.Nil { ctx = store.WithAgentID(ctx, l.agentUUID) } + if l.id != "" { + ctx = store.WithAgentKey(ctx, l.id) + } // Inject tenant into context for tool-level tenant scoping (spawn, MCP, etc.) if l.tenantID != uuid.Nil { ctx = store.WithTenantID(ctx, l.tenantID) @@ -1140,6 +1144,14 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) wg.Add(1) go func(idx int, tc providers.ToolCall) { defer wg.Done() + defer safego.Recover(func(v any) { + resultCh <- indexedResult{ + idx: idx, + tc: tc, + registryName: tc.Name, + result: tools.ErrorResult(fmt.Sprintf("tool %q panicked: %v", tc.Name, v)), + } + }, "agent", l.id, "tool", tc.Name) argsJSON, _ := json.Marshal(tc.Arguments) slog.Info("tool call", "agent", l.id, "tool", tc.Name, "args_len", len(argsJSON), "parallel", true) spanStart := time.Now().UTC() diff --git a/internal/agent/loop_history.go b/internal/agent/loop_history.go index bbbdd3c2..9b6f002a 100644 --- a/internal/agent/loop_history.go +++ b/internal/agent/loop_history.go @@ -14,6 +14,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/bootstrap" "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/safego" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/tools" ) @@ -518,6 +519,7 @@ func (l *Loop) maybeSummarize(ctx context.Context, sessionKey string) { // Summarize in background (holds the per-session lock until done) go func() { defer sessionMu.Unlock() + defer safego.Recover(nil, "session", sessionKey) // Re-check: history may have been truncated by a concurrent summarize // that finished between our threshold check and acquiring the lock. diff --git a/internal/cron/service_execution.go b/internal/cron/service_execution.go index 7f01754d..cd50e72c 100644 --- a/internal/cron/service_execution.go +++ b/internal/cron/service_execution.go @@ -10,6 +10,8 @@ import ( "time" "github.com/adhocore/gronx" + + "github.com/nextlevelbuilder/goclaw/internal/safego" ) // RunJob manually triggers a job execution. @@ -182,6 +184,7 @@ func (cs *Service) checkJobs() { wg.Add(1) go func(id string) { defer wg.Done() + defer safego.Recover(nil, "job_id", id) cs.executeJobByID(id) }(jobID) } diff --git a/internal/safego/recover.go b/internal/safego/recover.go new file mode 100644 index 00000000..7eef2a14 --- /dev/null +++ b/internal/safego/recover.go @@ -0,0 +1,28 @@ +// Package safego provides panic-recovery helpers for goroutines. +package safego + +import ( + "fmt" + "log/slog" + "runtime" +) + +// Recover catches panics, logs an error with stack trace, and optionally +// invokes onPanic. Must be called via defer: +// +// defer safego.Recover(nil, "job_id", id) // log-only +// defer safego.Recover(func(v any) { ... }, "tool", n) // log + callback +func Recover(onPanic func(v any), attrs ...any) { + r := recover() + if r == nil { + return + } + buf := make([]byte, 8192) + n := runtime.Stack(buf, false) + slog.Error("goroutine panicked", + append(attrs, "panic", fmt.Sprint(r), "stack", string(buf[:n]))..., + ) + if onPanic != nil { + onPanic(r) + } +} diff --git a/internal/safego/recover_test.go b/internal/safego/recover_test.go new file mode 100644 index 00000000..12609726 --- /dev/null +++ b/internal/safego/recover_test.go @@ -0,0 +1,97 @@ +package safego + +import ( + "fmt" + "strings" + "sync" + "testing" +) + +func TestRecover_LogOnly(t *testing.T) { + // Should not crash — panic is caught and logged. + done := make(chan struct{}) + go func() { + defer close(done) + defer Recover(nil, "test", "log_only") + panic("boom") + }() + <-done +} + +func TestRecover_WithCallback(t *testing.T) { + var captured string + done := make(chan struct{}) + go func() { + defer close(done) + defer Recover(func(v any) { + captured = v.(string) + }, "test", "callback") + panic("caught me") + }() + <-done + + if captured != "caught me" { + t.Fatalf("expected callback to capture panic value, got: %q", captured) + } +} + +func TestRecover_IndexOutOfRange(t *testing.T) { + var got string + done := make(chan struct{}) + go func() { + defer close(done) + defer Recover(func(v any) { + got = fmt.Sprint(v) + }, "test", "oob") + var s []int + _ = s[10] // index out of range + }() + <-done + + if !strings.Contains(got, "index out of range") { + t.Fatalf("expected index-out-of-range panic, got: %q", got) + } +} + +func TestRecover_Concurrent(t *testing.T) { + const n = 20 + var wg sync.WaitGroup + errors := make(chan string, n) + + for range n { + wg.Add(1) + go func() { + defer wg.Done() + defer Recover(func(v any) { + errors <- fmt.Sprint(v) + }, "test", "concurrent") + panic("concurrent boom") + }() + } + wg.Wait() + close(errors) + + count := 0 + for e := range errors { + if e != "concurrent boom" { + t.Errorf("unexpected panic value: %q", e) + } + count++ + } + if count != n { + t.Errorf("expected %d recovered panics, got %d", n, count) + } +} + +func TestRecover_NoPanic(t *testing.T) { + called := false + func() { + defer Recover(func(v any) { + called = true + }) + // No panic — callback should not fire. + }() + if called { + t.Fatal("onPanic should not be called when there is no panic") + } +} diff --git a/internal/tools/registry.go b/internal/tools/registry.go index 3bb8305a..8611a327 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -10,6 +10,7 @@ import ( "time" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/safego" ) // Registry manages tool registration and execution. @@ -178,7 +179,7 @@ func (r *Registry) ExecuteWithContext(ctx context.Context, name string, args map } start := time.Now() - result := tool.Execute(ctx, args) + result := safeExecute(tool, ctx, args) duration := time.Since(start) // Scrub credentials from tool output before returning to LLM @@ -201,6 +202,15 @@ func (r *Registry) ExecuteWithContext(ctx context.Context, name string, args map return result } +// safeExecute runs tool.Execute with panic recovery. A panicking tool returns +// an error result instead of crashing the process. +func safeExecute(tool Tool, ctx context.Context, args map[string]any) (result *Result) { + defer safego.Recover(func(v any) { + result = ErrorResult(fmt.Sprintf("tool %q panicked: %v", tool.Name(), v)) + }, "tool", tool.Name()) + return tool.Execute(ctx, args) +} + // ProviderDefs returns tool definitions for LLM provider APIs. // Includes alias definitions (same params/description, alias name). func (r *Registry) ProviderDefs() []providers.ToolDefinition { diff --git a/internal/tools/registry_test.go b/internal/tools/registry_test.go index 42c5aaf8..1daeb397 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -2,6 +2,7 @@ package tools import ( "context" + "strings" "testing" ) @@ -207,6 +208,75 @@ func TestRegistry_ExecuteWithContext_EmptyContextValues(t *testing.T) { } } +// --- Panic recovery tests --- + +func TestRegistry_ExecuteWithContext_PanicRecovery(t *testing.T) { + reg := NewRegistry() + reg.Register(&mockTool{ + name: "panicking_tool", + execFn: func(ctx context.Context, args map[string]any) *Result { + panic("unexpected nil pointer") + }, + }) + + // Without panic recovery this would crash the test process. + result := reg.ExecuteWithContext(context.Background(), "panicking_tool", nil, + "", "", "", "", nil) + + if !result.IsError { + t.Fatal("expected error result from panicking tool") + } + if !strings.Contains(result.ForLLM, "panicked") { + t.Errorf("error should mention panic, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "panicking_tool") { + t.Errorf("error should mention tool name, got: %s", result.ForLLM) + } +} + +func TestRegistry_Execute_PanicRecovery(t *testing.T) { + reg := NewRegistry() + reg.Register(&mockTool{ + name: "panic_via_execute", + execFn: func(ctx context.Context, args map[string]any) *Result { + var s []int + _ = s[10] // index out of range panic + return NewResult("unreachable") + }, + }) + + result := reg.Execute(context.Background(), "panic_via_execute", nil) + if !result.IsError { + t.Fatal("expected error result from panicking tool") + } +} + +func TestRegistry_ExecuteWithContext_PanicRecovery_Concurrent(t *testing.T) { + reg := NewRegistry() + reg.Register(&mockTool{ + name: "concurrent_panic", + execFn: func(ctx context.Context, args map[string]any) *Result { + panic("boom") + }, + }) + + const goroutines = 20 + results := make(chan *Result, goroutines) + for range goroutines { + go func() { + results <- reg.ExecuteWithContext(context.Background(), "concurrent_panic", nil, + "", "", "", "session-1", nil) + }() + } + + for range goroutines { + r := <-results + if !r.IsError { + t.Error("expected error result from panicking tool in concurrent execution") + } + } +} + // --- TryActivateDeferred / SetDeferredActivator tests --- func TestRegistry_TryActivateDeferred_NoActivator(t *testing.T) {