diff --git a/pkg/proxy/client.go b/pkg/proxy/client.go index 9f2e317e..53c783e2 100644 --- a/pkg/proxy/client.go +++ b/pkg/proxy/client.go @@ -596,9 +596,7 @@ func (c *proxyClient) discoverOnce(ctx context.Context) error { c.datasources = &datasources c.mu.Unlock() - if c.cfg.OnDiscover != nil { - c.cfg.OnDiscover() - } + c.runOnDiscover() c.log.WithFields(logrus.Fields{ "clickhouse": len(datasources.ClickHouseInfo), @@ -609,6 +607,24 @@ func (c *proxyClient) discoverOnce(ctx context.Context) error { return nil } +// runOnDiscover invokes the configured OnDiscover hook, recovering from any +// panic so a bad discovery response or a bug in module activation cannot take +// down the whole process. This runs on the background discovery goroutine, +// which has no other supervisor. +func (c *proxyClient) runOnDiscover() { + if c.cfg.OnDiscover == nil { + return + } + + defer func() { + if r := recover(); r != nil { + c.log.WithField("panic", r).Error("Recovered from panic in OnDiscover hook") + } + }() + + c.cfg.OnDiscover() +} + // EnsureAuthenticated checks if the user has valid credentials. func (c *proxyClient) EnsureAuthenticated(ctx context.Context) error { if c.tokenSource == nil { diff --git a/pkg/proxy/client_test.go b/pkg/proxy/client_test.go index b7e63798..e90de80c 100644 --- a/pkg/proxy/client_test.go +++ b/pkg/proxy/client_test.go @@ -96,6 +96,49 @@ func TestDiscoverNilOnDiscoverIsSafe(t *testing.T) { } } +// TestDiscoverRecoversFromOnDiscoverPanic verifies a panic inside the +// OnDiscover hook (e.g. a bug in module activation reacting to a newly +// discovered datasource) does not escape Discover and crash the process. The +// client must stay usable for subsequent calls afterward. +func TestDiscoverRecoversFromOnDiscoverPanic(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(DatasourcesResponse{}) + })) + t.Cleanup(srv.Close) + + log := logrus.New() + log.SetOutput(io.Discard) + + var hookCalls atomic.Int32 + + client := NewClient(log, ClientConfig{ + URL: srv.URL, + OnDiscover: func() { + hookCalls.Add(1) + panic("boom") + }, + }).(*proxyClient) + + if err := client.Discover(context.Background()); err != nil { + t.Fatalf("Discover error = %v", err) + } + + if got := hookCalls.Load(); got != 1 { + t.Fatalf("hookCalls after first Discover = %d, want 1", got) + } + + // A second Discover proves the client is still usable after the panic. + if err := client.Discover(context.Background()); err != nil { + t.Fatalf("second Discover error = %v", err) + } + + if got := hookCalls.Load(); got != 2 { + t.Fatalf("hookCalls after second Discover = %d, want 2", got) + } +} + func TestDatasourceInfoIncludesProxyName(t *testing.T) { t.Parallel()