mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-13 02:14:39 +00:00
fix: add panic recovery to tool, cron, and summarization goroutines (#420)
* 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 <viettranx@gmail.com>
This commit is contained in:
1 parent
ebc82d3265
commit
af3fe2fe08
7 files changed
+224
-2
No files matched your search
+13
-1
@@ -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()
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in new issue
Block a user