fix: Prevent excessive reconciliation when timeout disabled

- Stop immediate requeue loops when default-timeout-minutes is "0"
- Remove redundant hasCondition checks (Knative already deduplicates)

This adds an e2e tests that looks at pipeline logs to see how much
reconciler loop there is. If you run it before the fix, it would count
more than 1500 reconciler loop, whereas with the fix, only about 10.

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
Co-Authored-By: Claude <noreply@anthropic.com>

Author: 2 people

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -25,7 +25,6 @@ import (
 	"reflect"
 	"regexp"
 	"strings"
-	"time"
 
 	"github.com/hashicorp/go-multierror"
 	"github.com/tektoncd/pipeline/pkg/apis/config"
@@ -272,31 +271,50 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr
 	}
 
 	if pr.Status.StartTime != nil {
-		// Compute the time since the task started.
+		// Compute the time since the pipeline started.
 		elapsed := c.Clock.Since(pr.Status.StartTime.Time)
 		// Snooze this resource until the appropriate timeout has elapsed.
-		// but if the timeout has been disabled by setting timeout to 0, we
-		// do not want to subtract from 0, because a negative wait time will
-		// result in the requeue happening essentially immediately
 		timeout := pr.PipelineTimeout(ctx)
 		taskTimeout := pr.TasksTimeout()
-		waitTime := timeout - elapsed
-		if timeout == config.NoTimeoutDuration {
-			waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute
-		}
-		if pr.Status.FinallyStartTime == nil && taskTimeout != nil {
-			waitTime = pr.TasksTimeout().Duration - elapsed
-			if taskTimeout.Duration == config.NoTimeoutDuration {
-				waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute
+
+		// If the main pipeline timeout is NoTimeoutDuration (0), it means no timeout is configured.
+		// This can happen in two ways:
+		// 1. User explicitly set pr.Spec.Timeouts.Pipeline to 0 (wants no timeout)
+		// 2. User didn't set pr.Spec.Timeouts.Pipeline (nil) AND default-timeout-minutes config is "0"
+		// In these cases, check if there are specific task or finally timeouts to enforce.
+		// If not, don't requeue - the reconciler will be triggered by watch events.
+		// Check which phase we're in and handle timeout accordingly
+		if pr.Status.FinallyStartTime != nil {
+			// We're in finally phase - check for finally-specific timeout
+			if pr.FinallyTimeout() != nil && pr.FinallyTimeout().Duration != config.NoTimeoutDuration {
+				finallyWaitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time)
+				// If pipeline timeout is also set, use the most restrictive timeout
+				if timeout != config.NoTimeoutDuration {
+					waitTime := timeout - elapsed
+					if finallyWaitTime < waitTime {
+						waitTime = finallyWaitTime
+					}
+					return controller.NewRequeueAfter(waitTime)
+				}
+				return controller.NewRequeueAfter(finallyWaitTime)
 			}
-		} else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil &&
-			pr.FinallyTimeout().Duration != config.NoTimeoutDuration {
-			finallyWaitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time)
-			if finallyWaitTime < waitTime {
-				waitTime = finallyWaitTime
+			// No finally timeout, use pipeline timeout if set
+			if timeout != config.NoTimeoutDuration {
+				return controller.NewRequeueAfter(timeout - elapsed)
 			}
+			return nil
 		}
-		return controller.NewRequeueAfter(waitTime)
+
+		// We're in tasks phase - check for task-specific timeout
+		if taskTimeout != nil && taskTimeout.Duration != config.NoTimeoutDuration {
+			return controller.NewRequeueAfter(taskTimeout.Duration - elapsed)
+		}
+		// No task timeout, use pipeline timeout if set
+		if timeout != config.NoTimeoutDuration {
+			return controller.NewRequeueAfter(timeout - elapsed)
+		}
+		return nil
+
 	}
 	return nil
 }

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -2644,13 +2644,26 @@ spec:
 			c := prt.TestAssets.Controller
 			clients := prt.TestAssets.Clients
 			reconcileError := c.Reconciler.Reconcile(prt.TestAssets.Ctx, fmt.Sprintf("%s/%s", "foo", tc.prs[0].Name))
-			if reconcileError == nil {
-				t.Errorf("expected error, but got nil")
-			}
-			if isRequeueError, requeueDuration := controller.IsRequeueKey(reconcileError); !isRequeueError {
-				t.Errorf("Expected requeue error, but got: %s", reconcileError.Error())
-			} else if requeueDuration < 0 {
-				t.Errorf("Expected a positive requeue duration but got %s", requeueDuration.String())
+
+			// When timeout is explicitly disabled (set to 0), we should NOT requeue
+			// This prevents excessive reconciliation (issue #8495)
+			// Check if the pipeline has timeout disabled
+			pipelineTimeout := tc.prs[0].PipelineTimeout(prt.TestAssets.Ctx)
+			if pipelineTimeout == config.NoTimeoutDuration {
+				// Timeout is disabled - should not requeue
+				if reconcileError != nil {
+					t.Errorf("expected no error when timeout is disabled, but got: %v", reconcileError)
+				}
+			} else {
+				// Timeout is enabled - should requeue
+				if reconcileError == nil {
+					t.Errorf("expected error when timeout is enabled, but got nil")
+				}
+				if isRequeueError, requeueDuration := controller.IsRequeueKey(reconcileError); !isRequeueError {
+					t.Errorf("Expected requeue error, but got: %v", reconcileError)
+				} else if requeueDuration < 0 {
+					t.Errorf("Expected a positive requeue duration but got %s", requeueDuration.String())
+				}
 			}
 			prt.Test.Logf("Getting reconciled run")
 			reconciledRun, err := clients.Pipeline.TektonV1().PipelineRuns("foo").Get(prt.TestAssets.Ctx, tc.prs[0].Name, metav1.GetOptions{})

pkg/reconciler/taskrun/taskrun.go

@@ -231,10 +231,16 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1.TaskRun) pkgrecon
 		elapsed := c.Clock.Since(tr.Status.StartTime.Time)
 		// Snooze this resource until the timeout has elapsed.
 		timeout := tr.GetTimeout(ctx)
-		waitTime := timeout - elapsed
+		// If timeout is NoTimeoutDuration (0), it means no timeout is configured.
+		// This can happen in two ways:
+		// 1. User explicitly set tr.Spec.Timeout.Duration to 0 (wants no timeout)
+		// 2. User didn't set tr.Spec.Timeout (nil) AND default-timeout-minutes config is "0"
+		// In both cases, we should not requeue based on timeout. The reconciler will
+		// still be triggered appropriately by pod watch events when the TaskRun changes.
 		if timeout == config.NoTimeoutDuration {
-			waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute
+			return nil
 		}
+		waitTime := timeout - elapsed
 		return controller.NewRequeueAfter(waitTime)
 	}
 	return nil

pkg/reconciler/taskrun/taskrun_test.go

@@ -2929,13 +2929,25 @@ status:
 			clients := testAssets.Clients
 
 			err = c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tc.taskRun))
-			if err == nil {
-				t.Errorf("expected error when reconciling completed TaskRun : %v", err)
-			}
-			if isRequeueError, requeueDuration := controller.IsRequeueKey(err); !isRequeueError {
-				t.Errorf("Expected requeue error, but got: %s", err.Error())
-			} else if requeueDuration < 0 {
-				t.Errorf("Expected a positive requeue duration but got %s", requeueDuration.String())
+
+			// When timeout is explicitly disabled (set to 0), we should NOT requeue
+			// This prevents excessive reconciliation (issue #8495)
+			taskTimeout := tc.taskRun.GetTimeout(testAssets.Ctx)
+			if taskTimeout == config.NoTimeoutDuration {
+				// Timeout is disabled - should not requeue
+				if err != nil {
+					t.Errorf("expected no error when timeout is disabled, but got: %v", err)
+				}
+			} else {
+				// Timeout is enabled - should requeue
+				if err == nil {
+					t.Errorf("expected error when timeout is enabled, but got nil")
+				}
+				if isRequeueError, requeueDuration := controller.IsRequeueKey(err); !isRequeueError {
+					t.Errorf("Expected requeue error, but got: %v", err)
+				} else if requeueDuration < 0 {
+					t.Errorf("Expected a positive requeue duration but got %s", requeueDuration.String())
+				}
 			}
 			_, err = clients.Pipeline.TektonV1().TaskRuns(tc.taskRun.Namespace).Get(testAssets.Ctx, tc.taskRun.Name, metav1.GetOptions{})
 			if err != nil {