test: fix fake informer removal race - #742
Open
outsider987 wants to merge 1 commit into
Open
outsider987 wants to merge 1 commit into
outsider987 wants to merge 1 commit into
Conversation
Signed-off-by: Victor Chang <t7902195204@gmail.com>
Author
|
Could a maintainer approve the pending CI run and review this test-only fix? Local race checks and the full test suite pass. Thanks! |
Member
|
Thanks for the fix... can I ask why you picked this up? Wrangler doesn't see a lot of use outside the Rancher ecosystem and the issue was created by a maintainer mostly just to track a flake, so I'm curious what brought you here. |
Author
|
Thanks for asking! Someone I know in the broader SUSE ecosystem mentioned
that he’s leaving soon and suggested I keep an eye out for potential
opportunities.
That’s what got me looking through the related repositories. I wanted to
get familiar with the ecosystem and find a small, well-scoped issue I could
genuinely contribute to ^_^
Also, just to make sure — does the fix itself look okay, or is there
anything I missed?
I also used AI as a coding assistant while investigating this, but the PR
was based on my understanding of the code and the potential race condition
I found. if it not expect pls let me know it!!
|
Author
|
if that is bad I will cancel this fixed, sry |
Member
|
no, no problem - just curious. |
Author
|
let's go~~~ |
brandond
approved these changes
Sep 18, 2026
Member
|
lgm. someone from @rancher/rancher-squad-frameworks needs to approve as well. |
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.
Summary
Fixes #462.
Synchronize the test
fakeInformer's shared slices, signal completion after removal, and fail explicitly on timeout. Callbacks and channel waits run outside the mutex. Production code and the existing event-count assertions are unchanged.Verification
Local Linux/amd64, Go 1.26.0; baseline
c4b075131e36a1c89b39c89baa7615396d009992.-race -count=100-race -count=100 -cpu=1,2,4pkg/relatedresourcepackage,-racego test -mod=readonly ./...Mutations were tested through temporary Go overlays, not included in this change. This verifies the test helper's synchronization, not a production-cluster failure.
Before / after: synchronization
flowchart LR subgraph Before B1[Signal completion] --> B2[Remove handler] B1 --> B3[Test resumes] B2 -.->|Unsynchronized shared slices| B3 end subgraph After A1[Lock shared state] --> A2[Remove handler] A2 --> A3[Signal completion and unlock] A3 --> A4[Test checks subsequent events] endBefore the fix, the waiter's slice lookup can also race with removal, even before it receives the completion signal. The fix protects that lookup and captures the channel under the same mutex.