fix: cap scalers-cache reader at a per-reader budget derived from globalHTTPTimeout#7780
Merged
wozniakjan merged 2 commits intoMay 29, 2026
Conversation
|
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. |
5 tasks
Member
Author
|
if we conclude this is worth having, I will spin up a PR to charts and docs |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a configurable timeout for draining in-flight scaler cache readers during cache rebuild/close, preventing a single slow or stuck scaler read from stalling cache replacement indefinitely (follow-up to the reader-tracking fix added in #7737).
Changes:
- Add
KEDA_SCALERS_CACHE_READERS_DRAIN_TIMEOUT(default 5s) and plumb it from operator startup into the scale handler and scalers cache. - Update
ScalersCache.Close()to wait for readers with a bounded timeout before proceeding to close underlying scalers. - Add unit tests validating timeout and “disabled timeout” behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/env_resolver.go | Adds env var constant for the new drain-timeout configuration. |
| cmd/operator/main.go | Resolves the drain timeout from env (default 5s) and passes it into controllers/handlers. |
| pkg/scaling/scale_handler.go | Extends NewScaleHandler to carry the drain timeout and sets it on newly built caches. |
| pkg/scaling/cache/scalers_cache.go | Implements timeboxed waiting for active readers during cache close. |
| pkg/scaling/cache/scalers_cache_test.go | Adds tests covering timeout honoring and disabled-timeout behavior. |
| controllers/keda/scaledjob_controller.go | Plumbs the timeout into the ScaledJob controller’s scale handler. |
| controllers/keda/suite_test.go | Updates the scale handler constructor call in controller test setup. |
| CHANGELOG.md | Documents the new env var behavior in the unreleased changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
I'm struggling a bit with how this interacts with |
wozniakjan
force-pushed
the
agent/timebox-scalers-cache-close-wait
branch
from
ef47d38 to
f337a15
Compare
Member
Author
for scalers that use keda's HTTP client, it would be effectively |
wozniakjan
added
the
nice-to-have:keda-v2.20
zroubalik
approved these changes
May 27, 2026
wozniakjan
force-pushed
the
agent/timebox-scalers-cache-close-wait
branch
from
f337a15 to
09996f8
Compare
wozniakjan
force-pushed
the
agent/timebox-scalers-cache-close-wait
branch
2 times, most recently
from
7c93d2f to
3de48c3
Compare
wozniakjan
commented
May 27, 2026
JorTurFer
approved these changes
May 27, 2026
Member
Author
|
/run-e2e |
Member
|
I'm wondering whether this could be smaller and simpler. Since the rebuild path already detaches the old cache via Would something along those lines be worth considering, or is there a case I'm missing that justifies the configurable approach? |
Member
Author
you bring a good point, there is a very little benefit for timeboxing async cache close when the cache is hotswapped (just a temporary goroutine running doesn't hurt that much).
I guess constant would be ok too, with these types of timeouts I usually default to configurability but the use case in configuring this particular timeout is also rather limited I am trying to explore another alternative, trying to see if I can refactor the synchronous path to also be asynchronous and then this timebox wouldn't be necessary at all |
Member
Great that you're exploring this Jan! This would be a much cleaner outcome than a timebox, so it's well worth a try. |
wozniakjan
removed
the
nice-to-have:keda-v2.20
wozniakjan
force-pushed
the
agent/timebox-scalers-cache-close-wait
branch
from
3de48c3 to
265403e
Compare
wozniakjan
force-pushed
the
agent/timebox-scalers-cache-close-wait
branch
from
265403e to
4a9d3ea
Compare
Member
Author
|
/run-e2e passed tests: 195failed tests: 2 |
Signed-off-by: Jan Wozniak <[email protected]>
wozniakjan
changed the title
globalHTTPTimeout
Signed-off-by: Jan Wozniak <[email protected]>
wozniakjan
force-pushed
the
agent/timebox-scalers-cache-close-wait
branch
2 times, most recently
from
3de48c3 to
955cdf1
Compare
Member
Author
|
/run-e2e azure|scaling_strategies passed tests: 35failed tests: 1 |
Member
Author
|
/run-e2e azure_event_hub_blob_metadata_wi passed tests: 1failed tests: 0 |
shcherbak
pushed a commit
to shcherbak/keda
that referenced
this pull request
Jun 3, 2026
…obalHTTPTimeout` (kedacore#7780) * fix: timebox scaler drain during cache rebuild Signed-off-by: Jan Wozniak <[email protected]> * cap scalers-cache reader budget Signed-off-by: Jan Wozniak <[email protected]> --------- Signed-off-by: Jan Wozniak <[email protected]> Signed-off-by: Yurii Shcherbak <[email protected]>
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.
followup to #7737. The PR got merged before discussion regarding timeboxing the wait for active scalers during cache rebuild got concluded. I think #7737 is a great addition fixing real issue. My only concern was regarding the possible awry scaler taking too long to respond and affecting other scalers in the ScaledObject and their behavior.
Users will be able to set this through
globalHTTPTimeoutto control the drain mechanism to not exceed a configured timeout for this operation.Checklist
Relates to #7737
Relates to #7574