From af3fe2fe08db5f576cf61bd37708b4eac724524d Mon Sep 17 00:00:00 2001 From: Duc Nguyen Date: Tue, 24 Mar 2026 21:09:16 +0700 Subject: [PATCH] fix: add panic recovery to tool, cron, and summarization goroutines (#420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add panic recovery to tool, cron, and summarization goroutines A panicking tool, cron job, or auto-summarization goroutine crashes the entire server process — disconnecting all clients and aborting all running agents. Add defer/recover at three levels: - safeExecute() in tools/registry.go: catches panics from any tool's Execute method and returns an error result to the LLM - Agent loop goroutine (loop.go): defense-in-depth for non-tool code in the parallel tool execution goroutine - Cron execution goroutine (service_execution.go): prevents a single panicking cron job from taking down the server - Summarization goroutine (loop_history.go): prevents background auto-summarization panics from crashing the process All recovery points log the panic value and stack trace at ERROR level for debugging. Tool execution panics return actionable error messages to the LLM so the agent can recover gracefully. * refactor: extract panic recovery into safego.Recover helper DRY the duplicated recover+stack+log boilerplate across 4 goroutine sites into a shared safego.Recover(onPanic, attrs...) function. Stack buffer bumped from 4096 to 8192 bytes. --------- Co-authored-by: viettranx --- internal/agent/loop.go | 14 ++++- internal/agent/loop_history.go | 2 + internal/cron/service_execution.go | 3 + internal/safego/recover.go | 28 +++++++++ internal/safego/recover_test.go | 97 ++++++++++++++++++++++++++++++ internal/tools/registry.go | 12 +++- internal/tools/registry_test.go | 70 +++++++++++++++++++++ 7 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 internal/safego/recover.go create mode 100644 internal/safego/recover_test.go 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) {