fix: solve race in scalers cache rebuild that caused transient scaler errors#7737
Merged
Conversation
… errors Signed-off-by: Rick Brouwer <[email protected]>
|
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:
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. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![semgrep-code-kedacore[bot]](https://avatars.githubusercontent.com/u/49917779?s=60&v=4){kind=small}
Signed-off-by: Rick Brouwer <[email protected]>
Signed-off-by: Rick Brouwer <[email protected]>
Member
Author
|
/run-e2e internals |
Member
Author
|
/run-e2e internals |
rickbrouwer
added
the
nice-to-have:keda-v2.20
wozniakjan
reviewed
May 25, 2026
| c.closed = true | ||
| c.mutex.Unlock() | ||
|
|
||
| c.activeReaders.Wait() |
Member
There was a problem hiding this comment.
just thinking out loud - can this possibly lead to prolonged ErrCacheClosed responses if one trigger just takes too long to finish the active metric call?
wondering if we may want to timebox this with some reasonable default (10 second?) or if that would be unnecessary overkill.
Member
There was a problem hiding this comment.
I think that close is almost instant and closes current active connections. We could ship it as it is right now and in case of feedback about long refreshing, we could cut a hotfix. WDYT?
Member
There was a problem hiding this comment.
but the wait is not waiting for close to finish, it's waiting for the metric call to finish, closes are below after this wait passes. So a prometheus trigger on a server that is taking 30s to respond, all other triggers will get ErrCacheClosed for the whole duration of that overloaded prometheus hanging call. That is the difference from the current behavior, right now nothing waits and rebuilds scalers under the active calls which has it's own set of issues.
Member
There was a problem hiding this comment.
meanwhile the PR merged, I will spin up what I see could be a reasonable mitigation. We can continue the discussion on that PR and see if it has merit in merging or if we are ok with what is in main implemented through this PR
Member
Author
There was a problem hiding this comment.
I saw it. I will also take a look at the code this afternoon and your comment to check if a follow-up is needed.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a concurrency race during scaler cache rebuilds where in-flight scale loops could observe partially-torn-down caches/scalers and surface transient errors (eg redis: client is closed, scaler with id N not found). It introduces reader-tracking in the scalers cache and treats a new ErrCacheClosed sentinel as benign in key call paths to avoid transient failures impacting readiness/behavior.
Changes:
- Add
ErrCacheClosedsentinel + reader tracking (activeReaders) toScalersCache, and makeClose()idempotent while waiting for in-flight readers before closing scalers. - Update scale handler paths (ScaledObject metrics/state + ScaledJob metrics) to ignore
ErrCacheClosedas a transient cache-swap condition. - Add unit tests for cache close semantics and benign handling of
ErrCacheClosed, plus a changelog entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/scaling/cache/scalers_cache.go | Introduces ErrCacheClosed, reader tracking, and a safer Close() that waits for active readers. |
| pkg/scaling/scale_handler.go | Handles ErrCacheClosed in scaler metric/spec reads to avoid treating cache-swap as a scaler failure. |
| pkg/scaling/cache/scalers_cache_test.go | Adds concurrency/idempotency tests for the new cache close + reader-wait behavior. |
| pkg/scaling/scale_handler_test.go | Adds tests asserting ErrCacheClosed is benign for ScaledJob metrics and ScaledObject state paths. |
| CHANGELOG.md | Documents the user-visible fix for transient scaler errors during cache rebuild. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JorTurFer
approved these changes
May 25, 2026
Signed-off-by: Rick Brouwer <[email protected]>
Signed-off-by: Rick Brouwer <[email protected]>
rickbrouwer
added
the
Awaiting/2nd-approval
Signed-off-by: Rick Brouwer <[email protected]>
Member
Author
|
/run-e2e internal passed tests: 34failed tests: 0 |
Signed-off-by: Rick Brouwer <[email protected]>
Member
Author
|
/run-e2e internal passed tests: 34failed tests: 0 |
zroubalik
approved these changes
May 26, 2026
4 tasks
shcherbak
pushed a commit
to shcherbak/keda
that referenced
this pull request
Jun 3, 2026
… errors (kedacore#7737) * fix: solve race in scalers cache rebuild that caused transient scaler errors Signed-off-by: Rick Brouwer <[email protected]> * fix Signed-off-by: Rick Brouwer <[email protected]> * fix Signed-off-by: Rick Brouwer <[email protected]> * adjust tests Signed-off-by: Rick Brouwer <[email protected]> * update Signed-off-by: Rick Brouwer <[email protected]> * update Signed-off-by: Rick Brouwer <[email protected]> --------- Signed-off-by: Rick Brouwer <[email protected]> Co-authored-by: Zbynek Roubalik <[email protected]> Signed-off-by: Yurii Shcherbak <[email protected]>
rickbrouwer
removed
the
Awaiting/2nd-approval
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a ScaledJob or ScaledObject spec change rebuilds the scalers cache, a running scale loop could still read from the old cache while it was being torn down, surfacing errors like
redis: client is closedorscaler with id N not found. Len = 0.The cache now tracks active readers and waits for them to finish before closing the underlying scalers. Callers treat the new
ErrCacheClosedsentinel as benign so a transient race no longer flips Ready to False or invalidates the fresh cache.Checklist
make generate-scalers-schemahas been run to update any outdated generated filesFixes #7574