From 06343fcda9320cafeb226887312d1112dce6ac04 Mon Sep 17 00:00:00 2001
From: Misi <mgyongyosi@users.noreply.github.com>
Date: Fri, 25 Apr 2025 14:14:44 +0200
Subject: [PATCH] Advisor: Recover correctly when step.Run panics (#104521)

* wip

* Add test case for it
---
 apps/advisor/pkg/app/utils.go      | 11 +++++++++-
 apps/advisor/pkg/app/utils_test.go | 35 ++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/apps/advisor/pkg/app/utils.go b/apps/advisor/pkg/app/utils.go
index 82290b24020..2dd4caf3de8 100644
--- a/apps/advisor/pkg/app/utils.go
+++ b/apps/advisor/pkg/app/utils.go
@@ -158,7 +158,16 @@ func runStepsInParallel(ctx context.Context, spec *advisorv0alpha1.CheckSpec, st
 			go func(step checks.Step, item any) {
 				defer wg.Done()
 				defer func() { <-limit }()
-				stepErr, err := step.Run(ctx, spec, item)
+				var stepErr *advisorv0alpha1.CheckReportFailure
+				var err error
+				func() {
+					defer func() {
+						if r := recover(); r != nil {
+							err = fmt.Errorf("panic recovered in step %s: %v", step.ID(), r)
+						}
+					}()
+					stepErr, err = step.Run(ctx, spec, item)
+				}()
 				mu.Lock()
 				defer mu.Unlock()
 				if err != nil {
diff --git a/apps/advisor/pkg/app/utils_test.go b/apps/advisor/pkg/app/utils_test.go
index 830afb5950e..5d8e4441a18 100644
--- a/apps/advisor/pkg/app/utils_test.go
+++ b/apps/advisor/pkg/app/utils_test.go
@@ -129,6 +129,28 @@ func TestProcessCheck_RunError(t *testing.T) {
 	assert.Equal(t, "error", obj.GetAnnotations()[checks.StatusAnnotation])
 }
 
+func TestProcessCheck_RunRecoversFromPanic(t *testing.T) {
+	obj := &advisorv0alpha1.Check{}
+	obj.SetAnnotations(map[string]string{})
+	meta, err := utils.MetaAccessor(obj)
+	if err != nil {
+		t.Fatal(err)
+	}
+	meta.SetCreatedBy("user:1")
+	client := &mockClient{}
+	ctx := context.TODO()
+
+	check := &mockCheck{
+		items:     []any{"item"},
+		runPanics: true,
+	}
+
+	err = processCheck(ctx, client, obj, check)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "panic recovered in step")
+	assert.Equal(t, "error", obj.GetAnnotations()[checks.StatusAnnotation])
+}
+
 func TestProcessCheckRetry_NoRetry(t *testing.T) {
 	obj := &advisorv0alpha1.Check{}
 	obj.SetAnnotations(map[string]string{})
@@ -212,8 +234,9 @@ func (m *mockClient) PatchInto(ctx context.Context, id resource.Identifier, req
 }
 
 type mockCheck struct {
-	err   error
-	items []any
+	err       error
+	items     []any
+	runPanics bool
 }
 
 func (m *mockCheck) ID() string {
@@ -230,15 +253,19 @@ func (m *mockCheck) Item(ctx context.Context, id string) (any, error) {
 
 func (m *mockCheck) Steps() []checks.Step {
 	return []checks.Step{
-		&mockStep{err: m.err},
+		&mockStep{err: m.err, panics: m.runPanics},
 	}
 }
 
 type mockStep struct {
-	err error
+	err    error
+	panics bool
 }
 
 func (m *mockStep) Run(ctx context.Context, obj *advisorv0alpha1.CheckSpec, items any) (*advisorv0alpha1.CheckReportFailure, error) {
+	if m.panics {
+		panic("panic")
+	}
 	if m.err != nil {
 		return nil, m.err
 	}