Advisor: Fix retry behavior for missing item (#105608)

apps/advisor/pkg/app/checks/authchecks/check.go

@@ -2,6 +2,7 @@ package authchecks
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/grafana/grafana/apps/advisor/pkg/app/checks"
@@ -53,6 +54,9 @@ func (c *check) Items(ctx context.Context) ([]any, error) {
 func (c *check) Item(ctx context.Context, id string) (any, error) {
 	ssoSetting, err := c.ssoSettingsService.GetForProviderWithRedactedSecrets(ctx, id)
 	if err != nil {
+		if errors.Is(err, ssosettings.ErrNotFound) {
+			return nil, nil
+		}
 		return nil, err
 	}
 	return ssoSetting, nil

apps/advisor/pkg/app/checks/authchecks/check_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/grafana/grafana/pkg/services/ssosettings"
 	"github.com/grafana/grafana/pkg/services/ssosettings/models"
 	"github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" // Correct import path for the mock
 	"github.com/stretchr/testify/require"
@@ -91,4 +92,14 @@ func TestCheck_Item(t *testing.T) {
 		require.Nil(t, item)
 		require.ErrorIs(t, err, expectedErr)
 	})
+
+	t.Run("should return nil when sso settings are not found", func(t *testing.T) {
+		mockService := ssosettingstests.NewMockService(t)
+		mockService.On("GetForProviderWithRedactedSecrets", ctx, providerID).Return(nil, ssosettings.ErrNotFound)
+
+		c := New(mockService)
+		item, err := c.Item(ctx, providerID)
+		require.NoError(t, err)
+		require.Nil(t, item)
+	})
 }

apps/advisor/pkg/app/checks/datasourcecheck/check.go

@@ -69,10 +69,18 @@ func (c *check) Item(ctx context.Context, id string) (any, error) {
 	if err != nil {
 		return nil, err
 	}
-	return c.DatasourceSvc.GetDataSource(ctx, &datasources.GetDataSourceQuery{
+	ds, err := c.DatasourceSvc.GetDataSource(ctx, &datasources.GetDataSourceQuery{
 		UID:   id,
 		OrgID: requester.GetOrgID(),
 	})
+	if err != nil {
+		if errors.Is(err, datasources.ErrDataSourceNotFound) {
+			// The data source does not exist, skip the check
+			return nil, nil
+		}
+		return nil, err
+	}
+	return ds, nil
 }
 
 func (c *check) ID() string {

apps/advisor/pkg/app/checks/datasourcecheck/check_test.go

@@ -231,6 +231,19 @@ func TestCheck_Run(t *testing.T) {
 	})
 }
 
+func TestCheck_Item(t *testing.T) {
+	t.Run("should return nil when datasource is not found", func(t *testing.T) {
+		mockDatasourceSvc := &MockDatasourceSvc{dss: []*datasources.DataSource{}}
+		check := &check{
+			DatasourceSvc: mockDatasourceSvc,
+		}
+		ctx := identity.WithRequester(context.Background(), &user.SignedInUser{})
+		item, err := check.Item(ctx, "invalid-uid")
+		assert.NoError(t, err)
+		assert.Nil(t, item)
+	})
+}
+
 type MockDatasourceSvc struct {
 	datasources.DataSourceService
 
@@ -241,6 +254,13 @@ func (m *MockDatasourceSvc) GetAllDataSources(context.Context, *datasources.GetA
 	return m.dss, nil
 }
 
+func (m *MockDatasourceSvc) GetDataSource(context.Context, *datasources.GetDataSourceQuery) (*datasources.DataSource, error) {
+	if len(m.dss) == 0 {
+		return nil, datasources.ErrDataSourceNotFound
+	}
+	return m.dss[0], nil
+}
+
 type MockPluginContextProvider struct {
 	pCtx backend.PluginContext
 }

apps/advisor/pkg/app/checks/plugincheck/check.go

@@ -57,7 +57,7 @@ func (c *check) Items(ctx context.Context) ([]any, error) {
 func (c *check) Item(ctx context.Context, id string) (any, error) {
 	p, exists := c.PluginStore.Plugin(ctx, id)
 	if !exists {
-		return nil, fmt.Errorf("plugin %s not found", id)
+		return nil, nil
 	}
 	return p, nil
 }

apps/advisor/pkg/app/checks/plugincheck/check_test.go

@@ -154,6 +154,18 @@ func TestRun(t *testing.T) {
 	}
 }
 
+func TestCheck_Item(t *testing.T) {
+	t.Run("should return nil when plugin is not found", func(t *testing.T) {
+		pluginStore := &mockPluginStore{plugins: []pluginstore.Plugin{}}
+		check := &check{
+			PluginStore: pluginStore,
+		}
+		item, err := check.Item(context.Background(), "invalid-uid")
+		assert.NoError(t, err)
+		assert.Nil(t, item)
+	})
+}
+
 type mockPluginStore struct {
 	pluginstore.Store
 	plugins []pluginstore.Plugin
@@ -163,6 +175,13 @@ func (m *mockPluginStore) Plugins(ctx context.Context, t ...plugins.Type) []plug
 	return m.plugins
 }
 
+func (m *mockPluginStore) Plugin(ctx context.Context, id string) (pluginstore.Plugin, bool) {
+	if len(m.plugins) == 0 {
+		return pluginstore.Plugin{}, false
+	}
+	return m.plugins[0], true
+}
+
 type mockPluginRepo struct {
 	repo.Service
 	pluginInfo []repo.PluginInfo

apps/advisor/pkg/app/utils.go

@@ -123,6 +123,7 @@ func processCheckRetry(ctx context.Context, log logging.Logger, client resource.
 	if err != nil {
 		return fmt.Errorf("error initializing check: %w", err)
 	}
+	failures := []advisorv0alpha1.CheckReportFailure{}
 	item, err := check.Item(ctx, itemToRetry)
 	if err != nil {
 		setErr := checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationError)
@@ -131,27 +132,29 @@ func processCheckRetry(ctx context.Context, log logging.Logger, client resource.
 		}
 		return fmt.Errorf("error initializing check: %w", err)
 	}
-	// Get the check type
-	var checkType resource.Object
-	checkType, err = typesClient.Get(ctx, resource.Identifier{
-		Namespace: obj.GetNamespace(),
-		Name:      check.ID(),
-	})
-	if err != nil {
-		return err
-	}
-	// Run the steps
-	steps, err := filterSteps(checkType, check.Steps())
-	if err != nil {
-		return err
-	}
-	failures, err := runStepsInParallel(ctx, log, &c.Spec, steps, []any{item})
-	if err != nil {
-		setErr := checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationError)
-		if setErr != nil {
-			return setErr
+	if item != nil {
+		// Get the check type
+		var checkType resource.Object
+		checkType, err = typesClient.Get(ctx, resource.Identifier{
+			Namespace: obj.GetNamespace(),
+			Name:      check.ID(),
+		})
+		if err != nil {
+			return err
+		}
+		// Run the steps
+		steps, err := filterSteps(checkType, check.Steps())
+		if err != nil {
+			return err
+		}
+		failures, err = runStepsInParallel(ctx, log, &c.Spec, steps, []any{item})
+		if err != nil {
+			setErr := checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationError)
+			if setErr != nil {
+				return setErr
+			}
+			return fmt.Errorf("error running steps: %w", err)
 		}
-		return fmt.Errorf("error running steps: %w", err)
 	}
 	// Pull failures from the report for the items to retry
 	c.CheckStatus.Report.Failures = slices.DeleteFunc(c.CheckStatus.Report.Failures, func(f advisorv0alpha1.CheckReportFailure) bool {

apps/advisor/pkg/app/utils_test.go

@@ -226,6 +226,38 @@ func TestProcessCheckRetry_RetryError(t *testing.T) {
 	assert.Equal(t, checks.StatusAnnotationError, obj.GetAnnotations()[checks.StatusAnnotation])
 }
 
+func TestProcessCheckRetry_SkipMissingItem(t *testing.T) {
+	obj := &advisorv0alpha1.Check{}
+	obj.SetAnnotations(map[string]string{
+		checks.RetryAnnotation:  "item",
+		checks.StatusAnnotation: checks.StatusAnnotationProcessed,
+	})
+	obj.CheckStatus.Report.Failures = []advisorv0alpha1.CheckReportFailure{
+		{
+			ItemID: "item",
+			StepID: "step",
+		},
+	}
+	meta, err := utils.MetaAccessor(obj)
+	if err != nil {
+		t.Fatal(err)
+	}
+	meta.SetCreatedBy("user:1")
+	client := &mockClient{}
+	typesClient := &mockTypesClient{}
+	ctx := context.TODO()
+
+	check := &mockCheck{
+		items: []any{nil},
+	}
+
+	err = processCheckRetry(ctx, logging.DefaultLogger, client, typesClient, obj, check)
+	assert.NoError(t, err)
+	assert.Equal(t, checks.StatusAnnotationProcessed, obj.GetAnnotations()[checks.StatusAnnotation])
+	assert.Empty(t, obj.GetAnnotations()[checks.RetryAnnotation])
+	assert.Empty(t, obj.CheckStatus.Report.Failures)
+}
+
 func TestProcessCheckRetry_Success(t *testing.T) {
 	obj := &advisorv0alpha1.Check{}
 	obj.SetAnnotations(map[string]string{