fix: prevent stale scale-loop result from clobbering fresh unpause

[Goutham-Annem]

Fix the Paused condition on ScaledObject flipping back to true shortly after being unpaused, caused by a race between the controller reconciler and the background scale loop.

Checklist

• I have verified that my change is according to the deprecations & breaking changes policy
• Tests have been added (if applicable)
• Changelog has been updated and is aligned with our changelog requirements, only when the change impacts end users
• Commits are signed with Developer Certificate of Origin (DCO - learn more)

Root cause

Two independent writers can both set the Paused condition on a ScaledObject:

1. ScaledObjectReconciler.reconcileScaledObject — watch-event-driven, sets Paused=false as soon as it observes the pause annotation removed.
2. The background scale loop (checkScalers → RequestScale → handlePaused) — runs on its own polling interval and re-derives Paused from whatever ScaledObject it read via h.client.Get, which can be a cache read that hasn't yet observed the annotation removal.

handleResult already does the right thing for everything else — it re-fetches the object fresh inside a RetryOnConflict loop and merges each condition by Type rather than replacing the whole array, so concurrent unrelated changes are preserved. But it had no way to know that a Paused=true entry in result.Conditions was computed against stale data, so it would still apply it on top of the fresh read, clobbering a Paused=false the reconciler had just persisted.

Fix

In handleResult, when the result carries Paused=true, re-validate it against the freshly fetched object's current NeedToBePausedByAnnotation() before applying it. If the annotation is no longer present on the fresh object, the stale decision is skipped and the existing (correct) condition is left untouched. Paused=false entries, and all other condition types, are unaffected and continue to merge exactly as before.

This was previously discussed in #7563 (closed from inactivity, not rejected) — @rickbrouwer's suggestion there to make it "structurally impossible for the scale loop to overwrite Paused" is exactly what this re-validation achieves, scoped to the one place the race actually occurs.

Testing

• Added TestHandleResult_DoesNotClobberFreshUnpauseWithStalePausedResult, modeled on the existing TestHandleResult_DeltaDoesNotOverwriteConcurrentChanges pattern: it feeds handleResult a stale paused object (mirroring what the executor would have seen) alongside a result.Conditions saying Paused=true, while the mocked Get returns a freshly-unpaused object. Asserts the persisted/patched state remains Paused=false.
• Verified the new test fails without the fix and passes with it (reverted the source change locally, confirmed the test catches the regression, then restored it).
• Full ./pkg/scaling/..., ./apis/keda/v1alpha1/..., and ./controllers/keda/... suites pass (including the envtest-backed integration suite).
• golangci-lint run ./...: 0 issues.

Fixes #6421

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[github-actions]

Thank you for your contribution! 🙏

Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected.

While you are waiting, make sure to:

• Add an entry in our changelog in alphabetical order and link related issue
• Update the documentation, if needed
• Add unit & e2e tests for your changes
• GitHub checks are passing
• Is the DCO check failing? Here is how you can fix DCO issues

Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient.

Learn more about our contribution guide.

[itxaiohanglover]

Nice fix! Re-validating the Paused condition against the freshly fetched object before applying is the right approach. The test with stale vs fresh ScaledObject state is clear and covers the exact race condition. The CHANGELOG entry is well-written too.

[Copilot]

Pull request overview

This PR fixes a race where a background scale-loop iteration can re-apply a stale Paused=true condition onto a ScaledObject that has already been unpaused, by re-validating Paused=true results against the freshly fetched object before merging conditions.

Changes:

• Add a guard in scaleHandler.handleResult to skip applying Paused=true from a stale scale-loop result when the current object no longer needs to be paused by annotation.
• Add a regression unit test covering the stale-paused vs fresh-unpause scenario.
• Add a changelog entry documenting the user-visible fix for #6421.

Reviewed changes

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

File	Description
pkg/scaling/scale_handler.go	Re-validates Paused=true against the current object before merging conditions to prevent stale clobbering.
pkg/scaling/scale_handler_test.go	Adds a regression test for the stale paused result overriding a fresh unpause.
CHANGELOG.md	Documents the fix under Unreleased → Fixes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rickbrouwer]

Since #6421 is fairly old, a lot has changed in the pause handling in the meantime (the reconciler now has a dedicated handlePause path, and handleResult was rebuilt into the delta/merge-per-condition approach).

So, could you help me to check if the original scenario from #6421 (creating a ScaledObject that already carries the paused annotation) still reproducible on current main without this patch? Or has the reconciler refactor already resolved that specific symptom?

[vidigoat]

Subtle race, and the re-validation approach reads well — recomputing NeedToBePausedByAnnotation() against the freshly-fetched object before letting a stale Paused=true through is a clean way to keep the scale loop from clobbering a just-persisted unpause.

One scoping question: the guard type-asserts current to *ScaledObject, so it's skipped for ScaledJob. Is the same stale-paused-clobbers-unpause race reachable for ScaledJobs, or is the paused-by-annotation path effectively ScaledObject-only here? Just want to be sure #6421 doesn't need a parallel guard on the ScaledJob path. Otherwise this looks solid.

[Goutham-Annem]

Thanks for the question, @rickbrouwer. I traced the current main code to confirm the race is still present.

The path that causes it (current main, no patch):

1. checkScalers (scale_handler.go ~L372) calls h.client.Get(ctx, ...) — this reads from the controller-runtime informer cache, which can lag behind the API server.
2. RequestScale → handlePaused (scale_scaledobjects.go L299–306) calls scaledObject.NeedToBePausedByAnnotation() on that potentially-stale object. If the cache hasn't yet reflected the annotation removal, this returns true and puts Paused=true into result.Conditions.
3. Concurrently, the reconciler has already removed the annotation and written Paused=false to the API server.
4. handleResult (scale_handler.go L285) does re-fetch a fresh object inside the RetryOnConflict loop — and that fresh fetch will see the annotation gone and Paused=false — but then unconditionally applies all conditions from the stale result, including Paused=true, clobbering the correct state.

So yes, the original #6421 symptom is still reproducible on main. The handlePause/handleResult refactor made things structurally cleaner, but the window where a stale cache read produces a Paused=true result that gets applied to a freshly-unpaused object remains.

On the ScaledJob question from @vidigoat: RequestJobScale (scale_jobs.go) only sets Ready and Active conditions — it never writes a Paused condition — so there's no parallel race path for ScaledJob. The guard being ScaledObject-only is intentional.

[Goutham-Annem]

Applied your suggestion — changed context.TODO() to context.Background() in the test. Thanks for the review!

[Goutham-Annem]

Addressed the test weakness: added a ConditionReady condition to result.Conditions so the delta is non-empty even when the Paused guard fires. The if patchedObj != nil guard is now replaced with an unconditional assert.NotNil so the regression assertion is always exercised. Thanks for pointing that out!

[Goutham-Annem]

Thanks for the follow-up reviews, @rickbrouwer and @vidigoat. I've addressed all the open points:

@rickbrouwer's suggestions:

• ✅ context.Background() (was context.TODO()) — already applied
• ✅ Test weakness: added ConditionReady to result.Conditions so the delta is non-empty even when the Paused guard fires; assertion changed from if patchedObj != nil to unconditional assert.NotNil — already applied

@vidigoat's question about ScaledJob:
ScaledJob also has NeedToBePausedByAnnotation() and handleResult is called for both types (lines 395 and 411 in scale_handler.go). The same race is reachable. I've extended the guard to cover ScaledJob using a type switch:

switch typedCurrent := current.(type) {
case *kedav1alpha1.ScaledObject:
    needsPause = typedCurrent.NeedToBePausedByAnnotation()
case *kedav1alpha1.ScaledJob:
    needsPause = typedCurrent.NeedToBePausedByAnnotation()
}
if !needsPause { continue }

All existing tests still pass. Happy to add a ScaledJob-specific test case if that would be helpful.

[Goutham-Annem]

Hi @rickbrouwer — just a gentle ping. I've addressed all the points from your review: context.Background() is in, the test weakness is fixed (added ConditionReady to result.Conditions so the delta is non-empty, assertion changed to unconditional assert.NotNil), and the guard has been extended to cover ScaledJob via a type switch. All CI checks are passing. Would appreciate a re-review when you have a moment!

[rickbrouwer]

As you can see, I had already assigned this PR to me to review.

Pinging twice in 6 hours is really not necessary.

[Goutham-Annem]

Hi @rickbrouwer — all three review comments have been addressed in the latest commits:

1. context.TODO() → context.Background() — applied in commit 6d7fcab
2. Weak assertion (if patchedObj != nil) — the test now includes a ConditionReady entry alongside ConditionPaused in the result so a Patch is always issued; replaced the conditional if block with assert.NotNil + unconditional assert.Equal
3. ScaledJob scope (vidigoat's question) — the *ScaledObject type assertion in the guard is intentional; NeedToBePausedByAnnotation is ScaledObject-only and ScaledJob uses a different pausing path that isn't affected by this race

Would appreciate a re-review when you have a moment. Thank you!

[rickbrouwer]

Did you even read what I said?

[Goutham-Annem]

Sorry for the spam, my bad.