fix: Prevent backoff bypass in Manifest controller error loops

[LeelaChacha]

Summary

When module resources fail concurrently (e.g. a namespace is Terminating), two issues combined to completely defeat the Manifest controller's exponential backoff:

Root cause 1 — non-deterministic error string (ssa.go)
ConcurrentDefaultSSA.Run collects per-resource errors from goroutines via a channel. The receive order is non-deterministic, so errors.Join produces a different combined error string on every reconcile cycle when ≥2 resources fail. HasStatusDiff compares LastOperation.Operation strings, so it returns true on every cycle, triggering a status patch each time.

Root cause 2 — missing predicate (setup.go)
For(&Manifest{}) had no predicate. Status subresource patches don't increment metadata.generation, so every patch fired a MODIFIED watch event that called queue.Add directly — bypassing AddRateLimited and resetting the backoff timer.

Together, these produced a tight loop (~2–5 reconciles/second per manifest) instead of the intended 5–30s exponential backoff. In a production incident this caused ~700k error log entries in 12h from 7 stuck manifests, 7.5× CPU baseline, and ~2500 KCP API ops/s.

Changes

• ConcurrentDefaultSSA.Run: sort per-resource errors before errors.Join so the combined string is deterministic from cycle 2 onward — HasStatusDiff returns false, no status patch fires, backoff accumulates normally.
• SetupWithManager: add GenerationChangedPredicate to the Manifest self-watch so status-only updates are filtered and never bypass the rate limiter.

Verification

Reproduced locally with a k3d KCP+SKR setup: deployed template-operator, forced template-operator-system into Terminating, then bumped the module version to trigger concurrent SSA failures. With both fixes reverted the manifest_unauthorized requeue counter reached ~120–300 in 60 seconds (backoff bypassed). With both fixes applied it stayed ≤15 (proper 1s→10s backoff schedule).

closes #3523

[hyperspace-pr-bot]

👋 Hi — I'm PR Bot, your SAP code review assistant.

I'll automatically review your pull requests for code quality, security, and SAP compliance. Get an overview of what I do →

What I do

• Summarize your pull request changes
• Review code for quality, correctness, and reliability
• Suggest fixes when a pipeline job fails

Key commands

Command	Description
/review [--all]	Trigger a code review. Add --all to include files excluded by excluded_paths.
/summarize	Generate a PR summary
/ask <question>	Ask about the current changes
/help	See all available commands

Configure me for your team

Create .hyperspace/pull_request_bot.json in your repository:

{
  "$schema": "https://devops-insights-pr-bot.cfapps.eu10-004.hana.ondemand.com/schema/pull_request_bot.json",
  "features": {
    "control_panel": false,
    "summarize": {
      "auto_generate_summary": true,
      "auto_insert_summary": true,
      "auto_run_on_draft_pr": true,
      "use_custom_summarize_prompt": false,
      "use_custom_summarize_output_template": false,
      "excluded_paths": [],
      "auto_exclude_authors": []
    },
    "review": {
      "auto_generate_review": true,
      "auto_run_on_draft_pr": false,
      "use_custom_review_focus": false,
      "excluded_paths": [],
      "auto_exclude_authors": []
    },
    "sonar_fix": {
      "enable": true,
      "excluded_rules": []
    },
    "pipeline_fix": {
      "enable": true
    }
  },
  "excluded_paths": []
}

Full configuration reference →

*This introduction message will be shown to you only once, you will not see it in future PRs.

[LeelaChacha]

Reproduction test

To verify locally, drop these two files into tests/e2e/ and run:

make -f tests/e2e/backoff_bypass_test.mk test

With both fixes reverted the manifest_unauthorized counter hits ~120–300 in 60s (test fails). With the fixes it stays ≤15.

backoff_bypass_test.mk

.DEFAULT_GOAL := test
.PHONY: test $(MAKECMDGOALS)

include $(dir $(abspath $(lastword $(MAKEFILE_LIST))))e2e.common.mk

.PHONY: klm-patch
klm-patch: kustomize-install
	@echo "::group::KLM patch - short backoff delays for backoff-bypass test"
	@export PATH=$(LOCALBIN):$$PATH
	@pushd $(LIFECYCLE_MANAGER_DIR)/config/watcher_local_test > /dev/null
	kustomize edit add patch --kind Deployment --patch \
		'[{"op":"add","path":"/spec/template/spec/containers/0/args/-","value":"--failure-base-delay=1s"},{"op":"add","path":"/spec/template/spec/containers/0/args/-","value":"--failure-max-delay=10s"}]'
	@popd > /dev/null
	@echo "::endgroup::"

.PHONY: module-setup
module-setup: module-setup-latest module-setup-in-newer-version
	@echo "::group::ModuleReleaseMeta setup for backoff-bypass test (initial: regular:$(MODULE_DEPLOYABLE_VERSION))"
	$(SCRIPTS_DIR)/deploy_modulereleasemeta.sh $(MODULE_NAME) regular:$(MODULE_DEPLOYABLE_VERSION)
	@echo "::endgroup::"

.PHONY: test-run
test-run: log-tool-versions
	@echo "::group::Setting kubeconfig variables"
	@export KCP_KUBECONFIG=$(shell k3d kubeconfig write kcp)
	@export SKR_KUBECONFIG=$(shell k3d kubeconfig write skr)
	@echo "::endgroup::"

	@echo "::group::E2E test: Namespace Terminating Backoff Bypass"
	@export PATH=$(LOCALBIN):$$PATH
	@pushd $(E2E_TESTS_DIR) > /dev/null
	set +e; $(GO) test -timeout 20m -ginkgo.v -ginkgo.focus "Namespace Terminating Backoff Bypass"; status=$$?; set -e
	@popd > /dev/null
	@echo "::endgroup::"
	exit $${status}

.PHONY: test
test: create-clusters klm-patch deploy-klm module-setup test-run

backoff_bypass_test.go

package e2e_test

import (
	"fmt"
	"time"

	apicorev1 "k8s.io/api/core/v1"
	"sigs.k8s.io/controller-runtime/pkg/client"

	"github.com/kyma-project/lifecycle-manager/api/shared"
	"github.com/kyma-project/lifecycle-manager/api/v1beta2"
	"github.com/kyma-project/lifecycle-manager/internal/pkg/metrics"
	"github.com/kyma-project/lifecycle-manager/pkg/queue"

	. "github.com/kyma-project/lifecycle-manager/pkg/testutils"
	. "github.com/kyma-project/lifecycle-manager/tests/e2e/commontestutils"
	. "github.com/onsi/ginkgo/v2"
	. "github.com/onsi/gomega"
)

const (
	backoffTestModuleName    = "template-operator"
	backoffBlockingFinalizer = "e2e.test/block-termination"
	moduleNamespace          = "template-operator-system"
	measurementDuration      = 60 * time.Second
	maxExpectedRequeueCount  = 15
)

var _ = Describe("Namespace Terminating Backoff Bypass", Ordered, func() {
	kyma := NewKymaWithNamespaceName("kyma-sample", ControlPlaneNamespace, v1beta2.DefaultChannel)

	InitEmptyKymaBeforeAll(kyma)
	CleanupKymaAfterAll(kyma)

	Context("Given a Kyma CR with template-operator enabled", func() {
		It("When the module is enabled on the SKR cluster", func() {
			Eventually(EnableModule).
				WithContext(ctx).
				WithArguments(skrClient, defaultRemoteKymaName, RemoteNamespace,
					NewTemplateOperator(v1beta2.DefaultChannel)).
				Should(Succeed())
		})

		It("And the Manifest reaches Ready state on KCP", func() {
			Eventually(ManifestExists).
				WithContext(ctx).
				WithArguments(kcpClient, kyma.GetName(), kyma.GetNamespace(), backoffTestModuleName).
				WithTimeout(5 * time.Minute).
				Should(Succeed())

			Eventually(func(g Gomega) {
				manifest, err := GetManifest(ctx, kcpClient,
					kyma.GetName(), kyma.GetNamespace(), backoffTestModuleName)
				g.Expect(err).NotTo(HaveOccurred())
				g.Expect(manifest.Status.State).To(Equal(shared.StateReady))
			}).WithTimeout(5 * time.Minute).Should(Succeed())
		})
	})

	Context("When the module namespace enters Terminating and the module version is bumped", func() {
		It("Adds a blocking finalizer to the module namespace and deletes it", func() {
			ns := &apicorev1.Namespace{}
			Expect(skrClient.Get(ctx, client.ObjectKey{Name: moduleNamespace}, ns)).To(Succeed())
			original := ns.DeepCopy()
			ns.Finalizers = append(ns.Finalizers, backoffBlockingFinalizer)
			Expect(skrClient.Patch(ctx, ns, client.MergeFrom(original))).To(Succeed())
			Expect(skrClient.Delete(ctx, &apicorev1.Namespace{ObjectMeta: ns.ObjectMeta})).To(Succeed())
		})

		It("Then the module namespace is in Terminating state", func() {
			Eventually(func(g Gomega) {
				ns := &apicorev1.Namespace{}
				g.Expect(skrClient.Get(ctx, client.ObjectKey{Name: moduleNamespace}, ns)).To(Succeed())
				g.Expect(ns.DeletionTimestamp).NotTo(BeNil())
			}).WithTimeout(30 * time.Second).Should(Succeed())
		})

		It("Bumps the MRM to the newer version to force a manifest upgrade attempt", func() {
			mrm := &v1beta2.ModuleReleaseMeta{}
			Expect(kcpClient.Get(ctx,
				client.ObjectKey{Name: backoffTestModuleName, Namespace: ControlPlaneNamespace},
				mrm)).To(Succeed())
			original := mrm.DeepCopy()
			for i, ch := range mrm.Spec.Channels {
				if ch.Channel == string(v1beta2.DefaultChannel) {
					mrm.Spec.Channels[i].Version = NewerVersion
					break
				}
			}
			Expect(kcpClient.Patch(ctx, mrm, client.MergeFrom(original))).To(Succeed())
		})
	})

	Context("Then the Manifest controller requeue rate stays within backoff bounds", func() {
		It("Verifies that backoff is not bypassed during repeated 403 Forbidden errors", func() {
			requeueReason := string(metrics.ManifestUnauthorized)
			requeueType := string(queue.UnexpectedRequeue)

			Eventually(func() (int, error) {
				return GetRequeueReasonCount(ctx, requeueReason, requeueType)
			}).WithTimeout(3 * time.Minute).WithPolling(5 * time.Second).
				Should(BeNumerically(">", 0))

			count1, err := GetRequeueReasonCount(ctx, requeueReason, requeueType)
			Expect(err).NotTo(HaveOccurred())
			time.Sleep(measurementDuration)
			count2, err := GetRequeueReasonCount(ctx, requeueReason, requeueType)
			Expect(err).NotTo(HaveOccurred())

			delta := count2 - count1
			GinkgoWriter.Printf("manifest_unauthorized requeues in %s: %d\n", measurementDuration, delta)

			Expect(delta).To(BeNumerically("<=", maxExpectedRequeueCount),
				fmt.Sprintf("backoff bypassed: %d requeues in %s (max expected: %d)",
					delta, measurementDuration, maxExpectedRequeueCount))
		})
	})
})

[Copilot]

Pull request overview

This PR addresses an operational issue in the Manifest controller where exponential backoff could be effectively bypassed during persistent/concurrent failures, leading to tight reconcile loops and excessive load on KLM and the KCP API server.

Changes:

• Make joined SSA errors deterministic by sorting per-resource errors before errors.Join, preventing status “operation” churn across reconcile cycles.
• Filter Manifest self-watch events by generation changes to avoid status-only updates triggering immediate requeues that bypass the rate limiter.
• Add a unit test to ensure the SSA error string remains stable across repeated runs with concurrent failures.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
unit-test-coverage-lifecycle-manager.yaml	Updates expected unit test coverage numbers for internal/manifest/skrresources.
internal/manifest/skrresources/ssa.go	Sorts collected SSA errors prior to joining to stabilize the combined error string.
internal/manifest/skrresources/ssa_test.go	Adds a regression test ensuring SSA error string determinism under concurrent failures.
internal/controller/manifest/setup.go	Adds a generation-based predicate to the Manifest controller’s primary watch to filter status-only updates.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[LeelaChacha]

Follow up issue created: #3563