fix(reconciler): unblock queue on provider failure

pkg/reconciler/event.go

@@ -2,6 +2,7 @@ package reconciler
 
 import (
 	"context"
+	stderrors "errors"
 	"fmt"
 	"strconv"
 
@@ -18,6 +19,13 @@ import (
 	"go.uber.org/zap"
 )
 
+// ErrProviderNotConfigured marks a detectProvider failure as permanent: the
+// PipelineRun's git-provider annotation is missing or names a provider PAC
+// does not know. Retrying detectProvider can never succeed in that case,
+// unlike e.g. a GitHub App client failing to initialize, which is worth a
+// retry.
+var ErrProviderNotConfigured = stderrors.New("provider not configured")
+
 // detectProvider detects the git provider for the given PipelineRun and
 // initializes the corresponding provider interface. It returns the provider
 // interface, event information, and an error if any occurs during detection or
@@ -28,7 +36,7 @@ import (
 func (r *Reconciler) detectProvider(ctx context.Context, logger *zap.SugaredLogger, pr *tektonv1.PipelineRun) (provider.Interface, *info.Event, error) {
 	gitProvider, ok := pr.GetAnnotations()[keys.GitProvider]
 	if !ok {
-		return nil, nil, fmt.Errorf("failed to detect git provider for pipleinerun %s : git-provider label not found", pr.GetName())
+		return nil, nil, fmt.Errorf("%w: git-provider label not found on pipelinerun %s", ErrProviderNotConfigured, pr.GetName())
 	}
 
 	event := buildEventFromPipelineRun(pr)
@@ -54,7 +62,7 @@ func (r *Reconciler) detectProvider(ctx context.Context, logger *zap.SugaredLogg
 	case "gitea", "forgejo":
 		provider = &gitea.Provider{}
 	default:
-		return nil, nil, fmt.Errorf("failed to detect provider for pipelinerun: %s : unknown provider", pr.GetName())
+		return nil, nil, fmt.Errorf("%w: unknown provider %q for pipelinerun %s", ErrProviderNotConfigured, gitProvider, pr.GetName())
 	}
 	provider.SetLogger(logger)
 	return provider, event, nil

pkg/reconciler/event_test.go

@@ -2,6 +2,7 @@ package reconciler
 
 import (
 	"context"
+	stderrors "errors"
 	"testing"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
@@ -109,12 +110,12 @@ func TestDetectProvider(t *testing.T) {
 		{
 			name:       "unknown provider",
 			annotation: "batman",
-			errStr:     "failed to detect provider for pipelinerun: test : unknown provider",
+			errStr:     `provider not configured: unknown provider "batman" for pipelinerun test`,
 		},
 		{
 			name:         "no label",
 			missTheLabel: true,
-			errStr:       "failed to detect git provider for pipleinerun test : git-provider label not found",
+			errStr:       "provider not configured: git-provider label not found on pipelinerun test",
 		},
 	}
 	for _, tt := range tests {
@@ -140,6 +141,8 @@ func TestDetectProvider(t *testing.T) {
 				return
 			}
 			assert.Error(t, err, tt.errStr)
+			assert.Assert(t, stderrors.Is(err, ErrProviderNotConfigured),
+				"a missing or unknown provider annotation must be classified as permanent")
 		})
 	}
 }

pkg/reconciler/reconciler.go

@@ -268,7 +268,18 @@ func (r *Reconciler) reconcileKind(ctx context.Context, pr *tektonv1.PipelineRun
 	if err != nil {
 		msg := fmt.Sprintf("detectProvider: %v", err)
 		r.eventEmitter.EmitMessage(nil, zap.ErrorLevel, "RepositoryDetectProvider", msg)
-		return nil
+
+		if stderrors.Is(err, ErrProviderNotConfigured) {
+			// Nothing to retry here: the annotation is either missing or names
+			// a provider we don't support, and neither fixes itself. Free the
+			// slot and mark the run failed rather than spin on it forever.
+			return r.abandonFinishedWithoutProvider(ctx, logger, repo, pr, err)
+		}
+		// This one might be temporary, a GitHub App client failing to reach the
+		// API for instance, so let it retry. Returning nil here was the
+		// original bug: the key gets dropped, the final status is never
+		// reported and the slot is never freed.
+		return fmt.Errorf("detect provider: %w", err)
 	}
 	detectedProvider.SetPacInfo(&pacInfo)
 
@@ -280,6 +291,38 @@ func (r *Reconciler) reconcileKind(ctx context.Context, pr *tektonv1.PipelineRun
 	return nil
 }
 
+// abandonFinishedWithoutProvider handles a PipelineRun we can never report on,
+// because its git-provider annotation is missing or names something we don't
+// support. With no provider to post a status to, the most we can do is free the
+// concurrency slot it holds and mark it failed. Leave it be and it blocks the
+// repository's queue forever.
+//
+// Only call this for a finished PipelineRun, meaning IsDone() or IsCancelled().
+// Those two are not the same: a run cancelled before its Succeeded condition
+// settles is only the latter. Called on a run that is still going, this hands
+// its slot to somebody else and marks it failed, which stops reconcileKind ever
+// looking at it again.
+//
+// Free the queue before writing the state, not after. Releasing a slot twice is
+// harmless, so a retry after a failed patch still works out. In the other order,
+// dying between the two writes leaves a slot held by a run that already looks
+// failed, and nothing ever comes back for it.
+//
+// Returns nil because there is nothing here worth retrying.
+func (r *Reconciler) abandonFinishedWithoutProvider(ctx context.Context, logger *zap.SugaredLogger, repo *v1alpha1.Repository, pr *tektonv1.PipelineRun, cause error) error {
+	r.startNextPipelineRunInQueue(ctx, logger, repo, pr)
+	if _, err := r.updatePipelineRunState(ctx, logger, pr, kubeinteraction.StateFailed); err != nil {
+		return fmt.Errorf("abandon pipelinerun without provider %s/%s (cause: %w): cannot update state: %w", pr.Namespace, pr.Name, cause, err)
+	}
+	// Not emitMetrics: it labels the count metric by git provider, which is the
+	// one thing we don't have. Record the duration on its own so these runs
+	// still show up somewhere per repository.
+	if err := r.calculatePRDuration(ctx, pr); err != nil {
+		logger.Error("failed to emit duration metric: ", err)
+	}
+	return nil
+}
+
 func (r *Reconciler) createSecretForPipelineRun(ctx context.Context, logger *zap.SugaredLogger, pr *tektonv1.PipelineRun, repo *v1alpha1.Repository) error {
 	var gitAuthSecretName string
 	// as GitAuthSecret annotation is added to the PipelineRun in getPipelineRunsFromRepo function