fix: PagerBug: Hibernating filter in Devtron Apps list shows incomplete results (pager #2974)

internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go

@@ -21,6 +21,7 @@ import (
 	"github.com/devtron-labs/devtron/util"
 	"github.com/go-pg/pg"
 	"go.uber.org/zap"
+	"golang.org/x/exp/slices"
 	"strings"
 )
 
@@ -34,6 +35,15 @@ const (
 	ExternalChartStoreApp AppType = 3 // external helm app
 )
 
+const (
+	// appStatusHibernating mirrors client/argocdServer/bean.HIBERNATING / appStatus.HealthStatusHibernatingFilter.
+	// Not imported directly to avoid pulling those packages into the query-builder layer.
+	appStatusHibernating = "HIBERNATING"
+	// pipelineDeploymentAppTypeHelm mirrors internal/util.PIPELINE_DEPLOYMENT_TYPE_HELM, kept local for
+	// the same reason (this file already inlines the sibling "manifest_download" literal below).
+	pipelineDeploymentAppTypeHelm = "helm"
+)
+
 type AppListingRepositoryQueryBuilder struct {
 	logger *zap.SugaredLogger
 }
@@ -318,18 +328,34 @@ func (impl AppListingRepositoryQueryBuilder) buildAppListingWhereCondition(appLi
 			}
 		}
 	}
+	// AppStatus is only ever persisted to app_status (aps.status) for pipelines whose status has been
+	// synced there - today that is effectively ArgoCD/GitOps pipelines (see
+	// appStatus.AppStatusServiceImpl.UpdateStatusWithAppIdEnvId). Helm pipelines' status is instead
+	// resolved on the fly at listing time (AppListingServiceImpl.updateAppStatusForHelmTypePipelines),
+	// so a Helm pipeline that hasn't been synced to app_status has aps.status = NULL here, and "aps.status
+	// IN (...)" alone would wrongly exclude it from a HIBERNATING search. In that case only, let such
+	// unsynced rows through instead of excluding them outright, so the caller can resolve their status
+	// on the fly and recheck it against the filter after fetch (see
+	// AppListingServiceImpl.reconcileAppsWithUnresolvedStatus) - without weakening the filter for any
+	// other status, which is still matched purely against the persisted column.
+	statusCondition := " aps.status IN (?) "
+	statusConditionParams := []interface{}{pg.In(appStatusExcludingNotDeployed)}
+	if slices.Contains(appStatusExcludingNotDeployed, appStatusHibernating) {
+		statusCondition = " (aps.status IN (?) or (p.deployment_app_type = ? and aps.status IS NULL)) "
+		statusConditionParams = []interface{}{pg.In(appStatusExcludingNotDeployed), pipelineDeploymentAppTypeHelm}
+	}
 	if isNotDeployedFilterApplied {
 		deploymentAppType := "manifest_download"
 		whereCondition += " and (p.deployment_app_created=? and (p.deployment_app_type <> ? or dc.deployment_app_type <> ? ) or a.id NOT IN (SELECT app_id from pipeline) "
 		queryParams = append(queryParams, false, deploymentAppType, deploymentAppType)
 		if len(appStatusExcludingNotDeployed) > 0 {
-			whereCondition += " or aps.status IN (?) "
-			queryParams = append(queryParams, pg.In(appStatusExcludingNotDeployed))
+			whereCondition += " or " + statusCondition
+			queryParams = append(queryParams, statusConditionParams...)
 		}
 		whereCondition += " ) "
 	} else if len(appStatusExcludingNotDeployed) > 0 {
-		whereCondition += " and aps.status IN (?) "
-		queryParams = append(queryParams, pg.In(appStatusExcludingNotDeployed))
+		whereCondition += " and " + statusCondition
+		queryParams = append(queryParams, statusConditionParams...)
 	}
 	// Tag filters are AND-combined for now as requested by product.
 	// Each row translates to a correlated EXISTS/NOT EXISTS on app_label.

internal/sql/repository/helper/AppListingRepositoryQueryBuilder_app_status_test.go

@@ -0,0 +1,57 @@
+package helper
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+// When HIBERNATING is requested, Helm pipelines - whose status isn't persisted in app_status and is
+// instead resolved on the fly at listing time - must not be excluded outright by the db-side predicate
+// just because they have no app_status row yet (aps.status IS NULL). See
+// AppListingServiceImpl.reconcileAppsWithUnresolvedStatus for how such rows are rechecked after fetch.
+func TestBuildAppListingWhereCondition_HibernatingLetsUnresolvedHelmStatusThrough(t *testing.T) {
+	queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar())
+
+	whereClause, queryParams, err := queryBuilder.buildAppListingWhereCondition(AppListingFilter{
+		AppStatuses: []string{"HIBERNATING"},
+	})
+	require.NoError(t, err)
+
+	require.Contains(t, whereClause, "aps.status IN (?) or (p.deployment_app_type = ? and aps.status IS NULL)")
+	require.Len(t, queryParams, 4)
+	require.Equal(t, true, queryParams[0])
+	require.Equal(t, CustomApp, queryParams[1])
+	require.Equal(t, "helm", queryParams[3])
+}
+
+// A status filter that does not include HIBERNATING keeps matching purely against the persisted
+// app_status column, as before - unresolved (NULL) rows are not specially let through for it.
+func TestBuildAppListingWhereCondition_NonHibernatingStatusOnlyMatchesPersistedStatus(t *testing.T) {
+	queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar())
+
+	whereClause, queryParams, err := queryBuilder.buildAppListingWhereCondition(AppListingFilter{
+		AppStatuses: []string{"Healthy"},
+	})
+	require.NoError(t, err)
+
+	require.Contains(t, whereClause, "aps.status IN (?)")
+	require.NotContains(t, whereClause, "deployment_app_type")
+	require.Len(t, queryParams, 3)
+}
+
+// HIBERNATING combined with NOT DEPLOYED still lets unresolved Helm rows through in the "or aps.status
+// IN (...)" branch of the NOT DEPLOYED condition.
+func TestBuildAppListingWhereCondition_HibernatingWithNotDeployed(t *testing.T) {
+	queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar())
+
+	whereClause, queryParams, err := queryBuilder.buildAppListingWhereCondition(AppListingFilter{
+		AppStatuses: []string{"HIBERNATING", "NOT DEPLOYED"},
+	})
+	require.NoError(t, err)
+
+	require.Contains(t, whereClause, "(aps.status IN (?) or (p.deployment_app_type = ? and aps.status IS NULL))")
+	require.Len(t, queryParams, 7)
+	require.Equal(t, "helm", queryParams[6])
+}

pkg/app/AppListingService.go

@@ -438,16 +438,20 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques
 		return []*AppView.AppEnvironmentContainer{}, 0, nil
 	}
 
-	// Currently AppStatus is available in Db for only ArgoApps
-	// We fetch AppStatus on the fly for Helm Apps from scoop, So AppStatus filter will be applied in last
-	// fun to check if "HIBERNATING" exists in fetchAppListingRequest.AppStatuses
+	// "HIBERNATING" is persisted in app_status (see appStatus.UpdateStatusWithAppIdEnvId) for pipelines
+	// whose status has been synced there - today that is effectively ArgoCD/GitOps pipelines. For those
+	// it is filtered+paginated in the db query itself along with the other statuses (see
+	// buildAppListingWhereCondition), so the count/pagination for that (majority) case is correct.
+	//
+	// Helm pipelines don't have their AppStatus persisted in app_status - it is instead fetched on the
+	// fly at listing time (impl.updateAppStatusForHelmTypePipelines, below), so a Helm pipeline that
+	// hasn't been synced to app_status has no way to be matched by the db-side "aps.status IN (...)"
+	// predicate. To avoid silently dropping such apps from a HIBERNATING search, the db query lets rows
+	// with no persisted app_status through instead of excluding them (buildAppListingWhereCondition),
+	// and reconcileAppsWithUnresolvedStatus below re-checks exactly those rows, once their status is
+	// resolved on the fly, against the requested filter - without touching the count/pagination that
+	// was already correctly computed in sql for the persisted-status rows.
 	isFilteredOnHibernatingStatus := impl.isFilteredOnHibernatingStatus(fetchAppListingRequest)
-	// remove ""HIBERNATING" from fetchAppListingRequest.AppStatuses
-	appStatusesFilter := make([]string, 0)
-	if isFilteredOnHibernatingStatus {
-		appStatusesFilter = fetchAppListingRequest.AppStatuses
-		fetchAppListingRequest.AppStatuses = []string{}
-	}
 
 	appListingFilter := helper.AppListingFilter{
 		Environments:      fetchAppListingRequest.Environments,
@@ -473,6 +477,21 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques
 		return []*AppView.AppEnvironmentContainer{}, appSize, err
 	}
 
+	// A row whose app-status isn't persisted in db comes back with an empty AppStatus here (aps.status
+	// was NULL). Record which rows those are before updateAppStatusForHelmTypePipelines below
+	// potentially resolves an on-the-fly status for them, so that only those rows get rechecked against
+	// the requested filter afterwards - rows whose status was already persisted (and already correctly
+	// matched by the db query) are left exactly as sql returned them.
+	var containersWithUnresolvedStatus map[*AppView.AppEnvironmentContainer]bool
+	if isFilteredOnHibernatingStatus {
+		containersWithUnresolvedStatus = make(map[*AppView.AppEnvironmentContainer]bool)
+		for _, container := range envContainers {
+			if container.AppStatus == "" {
+				containersWithUnresolvedStatus[container] = true
+			}
+		}
+	}
+
 	envContainersMap := make(map[int][]*AppView.AppEnvironmentContainer)
 	envIds := make([]int, 0)
 	envsSet := make(map[int]bool)
@@ -506,20 +525,37 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques
 		impl.Logger.Errorw("error, UpdateAppStatusForHelmTypePipelines", "envIds", envIds, "err", err)
 	}
 
-	// apply filter for "HIBERNATING" status
 	if isFilteredOnHibernatingStatus {
-		filteredContainers := make([]*AppView.AppEnvironmentContainer, 0)
-		for _, container := range envContainers {
-			if slices.Contains(appStatusesFilter, container.AppStatus) {
-				filteredContainers = append(filteredContainers, container)
-			}
-		}
-		envContainers = filteredContainers
-		appSize = len(filteredContainers)
+		envContainers, appSize = impl.reconcileAppsWithUnresolvedStatus(envContainers, appSize, containersWithUnresolvedStatus, fetchAppListingRequest.AppStatuses)
 	}
+
 	return envContainers, appSize, nil
 }
 
+// reconcileAppsWithUnresolvedStatus drops rows whose app-status was not persisted in db (identified by
+// containersWithUnresolvedStatus, captured before the on-the-fly Helm status resolution) and that, once
+// resolved, do not actually match the requested app-status filter (appStatusesFilter). appSize is
+// decremented by exactly the number of rows dropped, so it is not recomputed wholesale - the count
+// contributed by rows whose status IS persisted in db (already correctly filtered and paginated in sql)
+// is left untouched.
+func (impl AppListingServiceImpl) reconcileAppsWithUnresolvedStatus(envContainers []*AppView.AppEnvironmentContainer, appSize int, containersWithUnresolvedStatus map[*AppView.AppEnvironmentContainer]bool, appStatusesFilter []string) ([]*AppView.AppEnvironmentContainer, int) {
+	if len(containersWithUnresolvedStatus) == 0 {
+		return envContainers, appSize
+	}
+	filteredContainers := make([]*AppView.AppEnvironmentContainer, 0, len(envContainers))
+	dropped := 0
+	for _, container := range envContainers {
+		if containersWithUnresolvedStatus[container] && !slices.Contains(appStatusesFilter, container.AppStatus) {
+			// unresolved status (e.g. a Helm pipeline not yet synced to app_status) that, even after the
+			// on-the-fly fetch above, doesn't match what was asked for - exclude it.
+			dropped++
+			continue
+		}
+		filteredContainers = append(filteredContainers, container)
+	}
+	return filteredContainers, appSize - dropped
+}
+
 func (impl AppListingServiceImpl) isFilteredOnHibernatingStatus(fetchAppListingRequest FetchAppListingRequest) bool {
 	if fetchAppListingRequest.AppStatuses != nil && len(fetchAppListingRequest.AppStatuses) > 0 {
 		if slices.Contains(fetchAppListingRequest.AppStatuses, bean2.HIBERNATING) {

pkg/app/AppListingService_hibernating_filter_test.go

@@ -0,0 +1,65 @@
+package app
+
+import (
+	"testing"
+
+	"github.com/devtron-labs/devtron/api/bean/AppView"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIsFilteredOnHibernatingStatus(t *testing.T) {
+	impl := AppListingServiceImpl{}
+
+	require.True(t, impl.isFilteredOnHibernatingStatus(FetchAppListingRequest{AppStatuses: []string{"Healthy", "HIBERNATING"}}))
+	require.False(t, impl.isFilteredOnHibernatingStatus(FetchAppListingRequest{AppStatuses: []string{"Healthy"}}))
+	require.False(t, impl.isFilteredOnHibernatingStatus(FetchAppListingRequest{}))
+}
+
+// A container whose status was already persisted in db (and therefore already correctly matched and
+// counted by the sql query) must never be touched by the in-memory reconciliation pass, even if its
+// resolved AppStatus happens not to be an exact string match (defensive: containersWithUnresolvedStatus
+// is the only signal used to decide whether a row is eligible for the recheck).
+func TestReconcileAppsWithUnresolvedStatus_LeavesResolvedContainersUntouched(t *testing.T) {
+	impl := AppListingServiceImpl{}
+	dbBacked := &AppView.AppEnvironmentContainer{AppId: 1, AppStatus: "HIBERNATING"}
+
+	envContainers := []*AppView.AppEnvironmentContainer{dbBacked}
+	result, appSize := impl.reconcileAppsWithUnresolvedStatus(envContainers, 10, nil, []string{"HIBERNATING"})
+
+	require.Equal(t, envContainers, result)
+	require.Equal(t, 10, appSize)
+}
+
+// A Helm-type row with no persisted app_status (let through by the db query as an unresolved row) is
+// dropped, and appSize is decremented by exactly the number of rows dropped, once its on-the-fly status
+// turns out not to match the requested filter.
+func TestReconcileAppsWithUnresolvedStatus_DropsNonMatchingUnresolvedContainers(t *testing.T) {
+	impl := AppListingServiceImpl{}
+	matching := &AppView.AppEnvironmentContainer{AppId: 1, AppStatus: "HIBERNATING"}        // resolved on the fly, matches
+	nonMatching := &AppView.AppEnvironmentContainer{AppId: 2, AppStatus: "Healthy"}         // resolved on the fly, doesn't match
+	unresolvedStillEmpty := &AppView.AppEnvironmentContainer{AppId: 3, AppStatus: ""}       // on-the-fly fetch was a no-op / found nothing
+	dbBackedArgoApp := &AppView.AppEnvironmentContainer{AppId: 4, AppStatus: "HIBERNATING"} // already matched by sql
+
+	envContainers := []*AppView.AppEnvironmentContainer{matching, nonMatching, unresolvedStillEmpty, dbBackedArgoApp}
+	containersWithUnresolvedStatus := map[*AppView.AppEnvironmentContainer]bool{
+		matching:             true,
+		nonMatching:          true,
+		unresolvedStillEmpty: true,
+	}
+
+	result, appSize := impl.reconcileAppsWithUnresolvedStatus(envContainers, 4, containersWithUnresolvedStatus, []string{"HIBERNATING"})
+
+	require.Equal(t, []*AppView.AppEnvironmentContainer{matching, dbBackedArgoApp}, result)
+	require.Equal(t, 2, appSize)
+}
+
+func TestReconcileAppsWithUnresolvedStatus_NoUnresolvedContainersIsNoOp(t *testing.T) {
+	impl := AppListingServiceImpl{}
+	container := &AppView.AppEnvironmentContainer{AppId: 1, AppStatus: "HIBERNATING"}
+	envContainers := []*AppView.AppEnvironmentContainer{container}
+
+	result, appSize := impl.reconcileAppsWithUnresolvedStatus(envContainers, 1, map[*AppView.AppEnvironmentContainer]bool{}, []string{"HIBERNATING"})
+
+	require.Equal(t, envContainers, result)
+	require.Equal(t, 1, appSize)
+}