fix: return deep copies from ListPodsInfo to prevent data race - #2055
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesPod listing isolation
Estimated code review effort: 2 (Simple) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies ListPodsInfo in pkg/device/pods.go to return deep copies of PodInfo objects instead of direct references, preventing external mutation of the internal state. It also adds a corresponding unit test TestListPodsInfoReturnsDeepCopy in pkg/device/pod_test.go to verify this behavior. There are no review comments to address, so no further feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ListPodsInfo was returning raw *PodInfo pointers from the internal map. Callers access fields after the read lock is released, which races with concurrent AddPod or UpdatePod calls. Use the existing DeepCopy method so callers get an independent snapshot. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
c6449b6 to
b307d79
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, mesutoezdil 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 |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
ListPodsInforeturned raw*PodInfopointers from the internal map. The read lock is released before callers ingetNodesUsagereadp.NodeID,p.Devices, and store the pointer in device state. A concurrentAddPodorUpdatePodcan write through the same pointer at the same time, causing a data race.PodInfo.DeepCopy()already existed. This PR uses it so callers get an independent snapshot. One line change.Same class of race fixed in #1967.
Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
This change is scoped to the scheduler. Unit test added:
TestListPodsInfoReturnsDeepCopymutates the returned slice and checks the internal map is unchanged. Passes with-race.Does this PR introduce a user-facing change?:
No
Summary by CodeRabbit