From 6859e9c035986b89a1ff2a8fc5e504e8eccfb593 Mon Sep 17 00:00:00 2001 From: Anas Khan Date: Tue, 14 Jul 2026 20:28:07 +0530 Subject: [PATCH] fix: clamp max-concurrency in custom analysis to avoid deadlock and panic (#1700) RunCustomAnalysis built its worker semaphore directly from the unvalidated a.MaxConcurrency field, unlike RunAnalysis which clamps the value first. With the user-supplied --max-concurrency flag this caused two failures: - --max-concurrency 0 makes an unbuffered channel, so the pre-send blocks waiting for a receiver that is only launched on the next line, deadlocking the whole analyze command whenever a custom analyzer is configured. - --max-concurrency -1 makes make(chan struct{}, -1) panic with "makechan: size out of range". Apply the same lower/upper bound clamp RunAnalysis already uses (default 10 for non-positive values, cap at 100). Add regression tests covering both cases. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com> Co-authored-by: Alex Jones <1235925+AlexsJones@users.noreply.github.com> --- pkg/analysis/analysis.go | 11 ++++++++- pkg/analysis/analysis_test.go | 43 +++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/pkg/analysis/analysis.go b/pkg/analysis/analysis.go index e408e0ca..29f7215f 100644 --- a/pkg/analysis/analysis.go +++ b/pkg/analysis/analysis.go @@ -260,7 +260,16 @@ func (a *Analysis) RunCustomAnalysis() { return } - semaphore := make(chan struct{}, a.MaxConcurrency) + // Set a reasonable maximum for concurrency to prevent excessive memory allocation + const maxAllowedConcurrency = 100 + concurrency := a.MaxConcurrency + if concurrency <= 0 { + concurrency = 10 // Default value if not set + } else if concurrency > maxAllowedConcurrency { + concurrency = maxAllowedConcurrency // Cap at a reasonable maximum + } + + semaphore := make(chan struct{}, concurrency) var wg sync.WaitGroup var mutex sync.Mutex verbose := viper.GetBool("verbose") diff --git a/pkg/analysis/analysis_test.go b/pkg/analysis/analysis_test.go index ed399456..e7296ce1 100644 --- a/pkg/analysis/analysis_test.go +++ b/pkg/analysis/analysis_test.go @@ -20,6 +20,7 @@ import ( "reflect" "strings" "testing" + "time" "github.com/agiledragon/gomonkey/v2" "github.com/k8sgpt-ai/k8sgpt/pkg/ai" @@ -634,6 +635,48 @@ func TestVerbose_RunCustomAnalysisWithCustomAnalyzer(t *testing.T) { } } +// Test: RunCustomAnalysis must not deadlock when MaxConcurrency is 0. +// A non-positive value previously produced an unbuffered semaphore whose only +// receiver is launched after the blocking send, hanging the command forever. +func TestRunCustomAnalysisZeroConcurrency(t *testing.T) { + viper.Set("custom_analyzers", []map[string]interface{}{ + { + "name": "TestCustomAnalyzer", + "connection": map[string]interface{}{"url": "127.0.0.1", "port": "2333"}, + }, + }) + + analysisObj := &Analysis{ + MaxConcurrency: 0, + } + + done := make(chan struct{}) + go func() { + analysisObj.RunCustomAnalysis() + close(done) + }() + + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("RunCustomAnalysis deadlocked with MaxConcurrency=0") + } +} + +// Test: RunCustomAnalysis must not panic when MaxConcurrency is negative. +// make(chan struct{}, negative) previously panicked with "makechan: size out of range". +func TestRunCustomAnalysisNegativeConcurrency(t *testing.T) { + viper.Set("custom_analyzers", []interface{}{}) + + analysisObj := &Analysis{ + MaxConcurrency: -1, + } + + require.NotPanics(t, func() { + analysisObj.RunCustomAnalysis() + }) +} + // Test: Verbose output in GetAIResults func TestVerbose_GetAIResults(t *testing.T) { viper.Set("verbose", true)