NO-JIRA: harden watch loop to prevent thread exhaustion#30956
NO-JIRA: harden watch loop to prevent thread exhaustion#30956neisw wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@neisw: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughImplements retry with exponential backoff and ±25% jitter for resource watches, refines Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/resourcewatch/observe/observe.go`:
- Around line 73-74: The code in ObserveResource is resetting retryAttempt to 0
when the watch returns nil/ends, which causes immediate reconnects if the watch
stream was closed (ResultChan() closed) rather than a clean end; modify the
logic in the watch loop (observe.go, functions handling the watch and the block
that currently sets retryAttempt = 0) to only reset retryAttempt when the watch
truly ended cleanly (e.g., explicit stop signal or terminal condition), and
treat a closed ResultChan() (receive with ok == false) as a retryable error path
that does NOT reset retryAttempt but increments backoff and returns an error so
backoff applies; apply the same change to the analogous code paths around the
other reset occurrences (the block referenced by lines ~150-153) and add a small
regression test that simulates an already-closed ResultChan() to assert backoff
is not reset.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66e3ac20-b4e0-4ad4-935e-ed3260c92694
📒 Files selected for processing (2)
pkg/resourcewatch/observe/observe.gopkg/resourcewatch/observe/observe_test.go
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-gcp-ovn-rt |
|
@neisw: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3a7af320-30eb-11f1-8069-b05fa5978c62-0 |
|
Scheduling required tests: |
|
@neisw: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: ba94dc5
|
Continuation of #30944
Handle watch error/bookmark events safely, always stop watch streams, and add context-aware retry backoff so reconnects do not spin and accumulate threads.
Made-with: Cursor