fix(pipelinerun): resolve issue with PipelineRun not timing out successfully

pkg/apis/pipeline/errors/errors.go

@@ -13,7 +13,12 @@ limitations under the License.
 
 package errors
 
-import "errors"
+import (
+	"errors"
+	"strings"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+)
 
 const UserErrorLabel = "[User error] "
 
@@ -71,3 +76,10 @@ func GetErrorMessage(err error) string {
 	}
 	return err.Error()
 }
+
+// IsImmutableTaskRunSpecError returns true if the error is the taskrun spec is immutable
+func IsImmutableTaskRunSpecError(err error) bool {
+	// The TaskRun may have completed and the spec field is immutable.
+	// validation code: https://github.com/tektoncd/pipeline/blob/v0.62.0/pkg/apis/pipeline/v1/taskrun_validation.go#L136-L138
+	return apierrors.IsBadRequest(err) && strings.Contains(err.Error(), "no updates are allowed")
+}

pkg/reconciler/pipelinerun/cancel.go

@@ -24,6 +24,7 @@ import (
 	"strings"
 	"time"
 
+	pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors"
 	v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned"
@@ -88,6 +89,10 @@ func cancelTaskRun(ctx context.Context, taskRunName string, namespace string, cl
 		// still be able to cancel the PipelineRun
 		return nil
 	}
+	if pipelineErrors.IsImmutableTaskRunSpecError(err) {
+		// The TaskRun may have completed and the spec field is immutable, we should ignore this error.
+		return nil
+	}
 	return err
 }
 

pkg/reconciler/pipelinerun/timeout.go

@@ -21,6 +21,7 @@ import (
 	"strings"
 	"time"
 
+	pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors"
 	v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned"
@@ -125,6 +126,10 @@ func timeoutPipelineTasksForTaskNames(ctx context.Context, logger *zap.SugaredLo
 		logger.Infof("patching TaskRun %s for timeout", taskRunName)
 
 		if err := timeoutTaskRun(ctx, taskRunName, pr.Namespace, clientSet); err != nil {
+			if pipelineErrors.IsImmutableTaskRunSpecError(err) {
+				// The TaskRun may have completed and the spec field is immutable, we should ignore this error.
+				continue
+			}
 			errs = append(errs, fmt.Errorf("failed to patch TaskRun `%s` with timeout: %w", taskRunName, err).Error())
 			continue
 		}

test/timeout_test.go

@@ -560,3 +560,102 @@ spec:
 	}
 	wg.Wait()
 }
+
+// TestPipelineRunTimeoutWithCompletedTaskRuns tests the case where a PipelineRun is timeout and has completed TaskRuns.
+func TestPipelineRunTimeoutWithCompletedTaskRuns(t *testing.T) {
+	t.Parallel()
+	// cancel the context after we have waited a suitable buffer beyond the given deadline.
+	ctx, cancel := context.WithTimeout(context.Background(), timeout+2*time.Minute)
+	defer cancel()
+	c, namespace := setup(ctx, t)
+
+	knativetest.CleanupOnInterrupt(func() { tearDown(context.Background(), t, c, namespace) }, t.Logf)
+	defer tearDown(context.Background(), t, c, namespace)
+
+	t.Logf("Creating Task in namespace %s", namespace)
+	task := parse.MustParseV1Task(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  params:
+  - name: sleep
+    default: "1"
+  steps:
+  - image: mirror.gcr.io/busybox
+    command: ['/bin/sh']
+    args: ['-c', 'sleep $(params.sleep)']
+`, helpers.ObjectNameForTest(t), namespace))
+	if _, err := c.V1TaskClient.Create(ctx, task, metav1.CreateOptions{}); err != nil {
+		t.Fatalf("Failed to create Task `%s`: %s", task.Name, err)
+	}
+
+	pipeline := parse.MustParseV1Pipeline(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  tasks:
+  - name: fast-task
+    params:
+    - name: sleep
+      value: "1"
+    taskRef:
+      name: %s
+  - name: slow-task
+    params:
+    - name: sleep
+      value: "120"
+    taskRef:
+      name: %s
+`, helpers.ObjectNameForTest(t), namespace, task.Name, task.Name))
+	pipelineRun := parse.MustParseV1PipelineRun(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  pipelineRef:
+    name: %s
+  timeouts:
+    pipeline: 30s
+    tasks: 30s
+`, helpers.ObjectNameForTest(t), namespace, pipeline.Name))
+	if _, err := c.V1PipelineClient.Create(ctx, pipeline, metav1.CreateOptions{}); err != nil {
+		t.Fatalf("Failed to create Pipeline `%s`: %s", pipeline.Name, err)
+	}
+	if _, err := c.V1PipelineRunClient.Create(ctx, pipelineRun, metav1.CreateOptions{}); err != nil {
+		t.Fatalf("Failed to create PipelineRun `%s`: %s", pipelineRun.Name, err)
+	}
+
+	t.Logf("Waiting for PipelineRun %s in namespace %s to be timed out", pipelineRun.Name, namespace)
+	if err := WaitForPipelineRunState(ctx, c, pipelineRun.Name, timeout, FailedWithReason(v1.PipelineRunReasonTimedOut.String(), pipelineRun.Name), "PipelineRunTimedOut", v1Version); err != nil {
+		t.Errorf("Error waiting for PipelineRun %s to finish: %s", pipelineRun.Name, err)
+	}
+
+	taskrunList, err := c.V1TaskRunClient.List(ctx, metav1.ListOptions{LabelSelector: "tekton.dev/pipelineRun=" + pipelineRun.Name})
+	if err != nil {
+		t.Fatalf("Error listing TaskRuns for PipelineRun %s: %s", pipelineRun.Name, err)
+	}
+
+	t.Logf("Waiting for TaskRuns from PipelineRun %s in namespace %s to time out and be cancelled", pipelineRun.Name, namespace)
+	var wg sync.WaitGroup
+	for _, taskrunItem := range taskrunList.Items {
+		wg.Add(1)
+		go func(name string) {
+			defer wg.Done()
+			if strings.Contains(name, "fast-task") {
+				// fast-task should have completed, not timed out
+				return
+			}
+			err := WaitForTaskRunState(ctx, c, name, FailedWithReason(v1.TaskRunReasonCancelled.String(), name), v1.TaskRunReasonCancelled.String(), v1Version)
+			if err != nil {
+				t.Errorf("Error waiting for TaskRun %s to timeout: %s", name, err)
+			}
+		}(taskrunItem.Name)
+	}
+	wg.Wait()
+
+	if _, err := c.V1PipelineRunClient.Get(ctx, pipelineRun.Name, metav1.GetOptions{}); err != nil {
+		t.Fatalf("Failed to get PipelineRun `%s`: %s", pipelineRun.Name, err)
+	}
+}