From bdfc781044e58fd699d46a35c3d2b8a960dbe0ec Mon Sep 17 00:00:00 2001
From: Lukasz Raczylo <lukasz@raczylo.com>
Date: Fri, 2 Oct 2026 03:59:44 +0100
Subject: [PATCH] fix(controller): report Failed when a group fails

checkOverallStatus overwrote the Failed status with Running. A failed workflow
stayed Running and kept requeueing. It now stays Failed and the failure event
fires once. Also remove err shadowing in the visualization client.
---
 controllers/crd_scrapper.go      | 20 ++++++++++++--------
 controllers/crd_scrapper_test.go |  5 +----
 pkg/visualization/client.go      |  4 ++--
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/controllers/crd_scrapper.go b/controllers/crd_scrapper.go
index b087186..633f8fc 100644
--- a/controllers/crd_scrapper.go
+++ b/controllers/crd_scrapper.go
@@ -311,24 +311,28 @@ func (cp *connPackage) executeJob(j *jobsmanagerv1beta1.ManagedJobDefinition, g
 
 func (cp *connPackage) checkOverallStatus() {
 	groupsCompleted := 0
+	failedGroup := ""
 	negativeStatuses := []string{ExecutionStatusFailed, ExecutionStatusAborted}
 	for _, group := range cp.mj.Spec.Groups {
 		if group.Status == ExecutionStatusSucceeded {
 			groupsCompleted++
-		} else if pandati.ExistsInSlice(negativeStatuses, group.Status) {
-			cp.mj.Status = ExecutionStatusFailed
-			cp.r.Recorder.Eventf(cp.mj, corev1.EventTypeWarning, "Failure", "Run failed in group %s", group.Name)
-		} else {
-			continue
+		} else if failedGroup == "" && pandati.ExistsInSlice(negativeStatuses, group.Status) {
+			failedGroup = group.Name
 		}
 	}
 
-	if groupsCompleted == len(cp.mj.Spec.Groups) {
+	switch {
+	case failedGroup != "":
+		if cp.mj.Status != ExecutionStatusFailed {
+			cp.r.Recorder.Eventf(cp.mj, corev1.EventTypeWarning, "Failure", "Run failed in group %s", failedGroup)
+		}
+		cp.mj.Status = ExecutionStatusFailed
+	case groupsCompleted == len(cp.mj.Spec.Groups):
 		if cp.mj.Status != ExecutionStatusSucceeded {
-			cp.r.Recorder.Eventf(cp.mj, corev1.EventTypeNormal, "Success", "Run completed successfuly")
+			cp.r.Recorder.Eventf(cp.mj, corev1.EventTypeNormal, "Success", "Run completed successfully")
 		}
 		cp.mj.Status = ExecutionStatusSucceeded
-	} else {
+	default:
 		cp.mj.Status = ExecutionStatusRunning
 	}
 	if err := cp.r.Status().Update(cp.ctx, cp.mj); err != nil {
diff --git a/controllers/crd_scrapper_test.go b/controllers/crd_scrapper_test.go
index 5b7c5c8..ab80525 100644
--- a/controllers/crd_scrapper_test.go
+++ b/controllers/crd_scrapper_test.go
@@ -315,10 +315,7 @@ func (s *ScrapperTestSuite) TestCheckOverallStatus_GroupFailed() {
 	cp := s.newConnPackage(mj)
 	cp.checkOverallStatus()
 
-	// Note: Current implementation sets running status after checking failed
-	// This is the actual behavior - the status goes through the failed branch
-	// but then gets overwritten by the final else block
-	s.Equal(ExecutionStatusRunning, mj.Status)
+	s.Equal(ExecutionStatusFailed, mj.Status)
 }
 
 func (s *ScrapperTestSuite) TestCheckOverallStatus_GroupRunning() {
diff --git a/pkg/visualization/client.go b/pkg/visualization/client.go
index 7cc0e01..ee2af87 100644
--- a/pkg/visualization/client.go
+++ b/pkg/visualization/client.go
@@ -25,10 +25,10 @@ func NewClient() (*Client, error) {
 	}
 
 	scheme := runtime.NewScheme()
-	if err := clientgoscheme.AddToScheme(scheme); err != nil {
+	if err = clientgoscheme.AddToScheme(scheme); err != nil {
 		return nil, fmt.Errorf("failed to add client-go scheme: %w", err)
 	}
-	if err := jobsmanagerv1beta1.AddToScheme(scheme); err != nil {
+	if err = jobsmanagerv1beta1.AddToScheme(scheme); err != nil {
 		return nil, fmt.Errorf("failed to add jobsmanager scheme: %w", err)
 	}