ACM-35499 avoid blocking incoming requests#6337
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a generic ChangesBatch Promise Utility and Adoption
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/test/lib/batch-promise-all.test.ts (1)
67-82: ⚡ Quick winStrengthen default-batch-size assertion to cover the final partial batch.
Current assertion only verifies two full 100-item groups. It does not fail if the last 50-item batch is accidentally dropped. Add a total-processed assertion (or assert result length) to close that gap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/lib/batch-promise-all.test.ts` around lines 67 - 82, The test `should use default batch size of 100` only asserts that batchCount equals 2, which verifies two full batches but does not catch if the final 50-item partial batch is accidentally dropped. Strengthen the test by adding an assertion that verifies all 250 input items were actually processed. Either capture the result returned by batchPromiseAll and assert its length equals 250, or track the total number of items processed through the callback and assert that total equals 250. This ensures the partial final batch is properly handled and processed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/lib/batch-promise-all.ts`:
- Around line 11-15: The batchPromiseAll function lacks validation for the
batchSize parameter, which can cause an infinite loop when batchSize is 0 (since
i += batchSize would not advance) or move backwards when negative. Add an
upfront guard at the beginning of the function to validate that batchSize is a
positive integer, throwing an appropriate error if the validation fails. This
check should occur before the for loop that uses the i += batchSize increment.
In `@backend/test/lib/batch-promise-all.test.ts`:
- Around line 39-48: The global.setImmediate restoration is not protected by
try/finally, which means if the await batchPromiseAll call throws an exception,
the restoration code never executes and leaves patched globals in place for
subsequent tests. Wrap the batchPromiseAll invocation and associated test logic
in a try/finally block, moving the global.setImmediate restoration statement
into the finally block to ensure it always executes regardless of whether
batchPromiseAll succeeds or throws. This fix applies to all occurrences where
global.setImmediate is temporarily patched in your test file.
---
Nitpick comments:
In `@backend/test/lib/batch-promise-all.test.ts`:
- Around line 67-82: The test `should use default batch size of 100` only
asserts that batchCount equals 2, which verifies two full batches but does not
catch if the final 50-item partial batch is accidentally dropped. Strengthen the
test by adding an assertion that verifies all 250 input items were actually
processed. Either capture the result returned by batchPromiseAll and assert its
length equals 250, or track the total number of items processed through the
callback and assert that total equals 250. This ensures the partial final batch
is properly handled and processed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 34a7344e-4847-45d3-a33d-9ddbcc10ce5f
📒 Files selected for processing (4)
backend/src/lib/batch-promise-all.tsbackend/src/lib/server-side-events.tsbackend/src/routes/events.tsbackend/test/lib/batch-promise-all.test.ts
…oming network requests Assisted-by: Cursor (Claude Opus 4.6 High) Signed-off-by: Kevin Cormier <kcormier@redhat.com>
Generated-by: Cursor (Claude Opus 4.6 High) Signed-off-by: Kevin Cormier <kcormier@redhat.com>
KevinFCormier
force-pushed
the
ACM-35499-avoid-blocking-incoming-requests
branch
from
3a8870d to
60c651e
Compare
|
/cc @fxiang1 |
|
/cherry-pick release-2.17 |
|
@KevinFCormier: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1, KevinFCormier The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test unit-tests-sonarcloud |
|
|
@KevinFCormier: new pull request created: #6357 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
📝 Summary
Ticket Summary (Title):
console-mce pod CrashLoopBackOff due to probe failure with large resource sets
Ticket Link:
https://redhat.atlassian.net/browse/ACM-35499
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Bugfix
🗒️ Notes for Reviewers
Customer was observing CrashLoopBackoff on console-mce pods and had over 20,000 Group resources present. Initially processing these resources may have blocked us from responding to the readiness and liveness probes on time, causing Kubernetes to restart the pod.
This PR batches Promise.all calls so that we call setImmediate after each batch of 100 to yield the event queue so that new requests can be handled. This seems to smooth out memory usage of the pods. Before this change, I observed a spike at startup, then a retreat to stable size.
Summary by CodeRabbit