NO-JIRA: Fix data race in TestControllerScheduled#2201
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tchap: 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: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors a unit test in ChangesTest Concurrency Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/factory/factory_test.go (1)
241-251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let shutdown strand a blocked
syncCalledsender.Once
cancel()closesctx.Done(), this observer can exit even if a sync callback is already blocked on the unbufferedsyncCalled <- struct{}{}send. In that casecontroller.Runnever returns andwg.Wait()can hang the test.Suggested fix
controller := New().ResyncSchedule("@every 1s").WithSync(func(ctx context.Context, controllerContext SyncContext) error { - syncCalled <- struct{}{} + select { + case syncCalled <- struct{}{}: + case <-ctx.Done(): + } return nil }).ToController("test", events.NewInMemoryRecorder("fake-controller", clocktesting.NewFakePassiveClock(time.Now())))Also applies to: 270-271
🤖 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 `@pkg/controller/factory/factory_test.go` around lines 241 - 251, The test's goroutine reading from syncCalled can exit on ctx.Done() while a blocked sender (the sync callback that does syncCalled <- struct{}{}) remains blocked, causing controller.Run and wg.Wait() to hang; fix by making the send non-blocking or cancel-aware: update the sync callback (the place that sends to the syncCalled channel) to use a select that attempts to send to syncCalled but also selects on ctx.Done() (or use a buffered syncCalled) so the send never blocks indefinitely; adjust both occurrences referenced (the send near lines 241-251 and the similar send at 270-271) to use this cancel-aware non-blocking send.
🤖 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 `@pkg/controller/factory/factory_test.go`:
- Around line 262-265: The wait condition in the PollUntilContextCancel call is
off-by-one: change the callback used in wait.PollUntilContextCancel (the closure
checking syncCounter) to return syncCounter >= 3 instead of syncCounter > 3 so
the wait completes after observing three syncs as the test and error message
expect; update the closure that references syncCounter and ensure the
surrounding call to wait.PollUntilContextCancel remains unchanged.
---
Outside diff comments:
In `@pkg/controller/factory/factory_test.go`:
- Around line 241-251: The test's goroutine reading from syncCalled can exit on
ctx.Done() while a blocked sender (the sync callback that does syncCalled <-
struct{}{}) remains blocked, causing controller.Run and wg.Wait() to hang; fix
by making the send non-blocking or cancel-aware: update the sync callback (the
place that sends to the syncCalled channel) to use a select that attempts to
send to syncCalled but also selects on ctx.Done() (or use a buffered syncCalled)
so the send never blocks indefinitely; adjust both occurrences referenced (the
send near lines 241-251 and the similar send at 270-271) to use this
cancel-aware non-blocking send.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f50f69de-4948-4af2-9dfb-a72550290c8a
📒 Files selected for processing (1)
pkg/controller/factory/factory_test.go
tchap
force-pushed
the
fix-flaky-test-controller-scheduled
branch
from
193e638 to
4a9a875
Compare
The observer goroutine could call t.Logf after the test returned, racing with testing cleanup. Fix by using sync.WaitGroup to ensure all goroutines finish before the test exits, making the sync callback cancel-aware to prevent deadlock, using atomic.Int32 for the sync counter, and replacing deprecated wait.PollImmediateUntil with wait.PollUntilContextCancel.
tchap
force-pushed
the
fix-flaky-test-controller-scheduled
branch
from
4a9a875 to
10bd453
Compare
|
@tchap: all tests passed! 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. |
2 participants
The observer goroutine could call t.Logf after the test returned,
racing with testing cleanup. Fix by using sync.WaitGroup to ensure
all goroutines finish before the test exits, making the sync callback
cancel-aware to prevent deadlock, using atomic.Int32 for the sync
counter, and replacing deprecated wait.PollImmediateUntil with
wait.PollUntilContextCancel.
Summary by CodeRabbit
Note: These are internal testing improvements with no user-visible changes.