fix(events): synchronize drainer status reads - #6850
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new TestDrainer_ConcurrentStatus uses sync.WaitGroup.Go, which doesn’t exist and will not compile.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the events.Drainer concurrency model by ensuring status snapshots are read under the same mutex used for updates, and it updates tests to better exercise shutdown and concurrent status polling.
Changes:
- Synchronize
Drainer.GetStatus()by locking the existing mutex before returning the status snapshot. - Replace the shutdown test’s timing-based coordination with
testing/synctestto deterministically wait for shutdown blocking. - Add a concurrent status polling test intended to catch races between operation updates and status reads.
File summaries
| File | Description |
|---|---|
| server/events/drainer.go | Locks around GetStatus() to prevent concurrent reads of status racing with updates. |
| server/events/drainer_test.go | Updates shutdown test to use synctest; adds concurrent status polling coverage (currently has a compile issue). |
Review details
Suppressed comments (1)
server/events/drainer_test.go:92
- sync.WaitGroup does not have a Go method; this goroutine should be started with go + wg.Add/Done, otherwise the test won't compile.
wg.Go(func() {
<-start
for range 1000 {
status := d.GetStatus()
if status.ShuttingDown || status.InProgressOps < 0 || status.InProgressOps > workers {
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall line coverage in commit b302208 in the Show a line coverage summary of the most covered files.
Updated |
7796857 to
b1328ca
Compare
b1328ca to
438d1e5
Compare
Protect status snapshots with the existing mutex and cover concurrent status polling. Synchronize shutdown tests with the standard-library test scheduler. Assisted-by: OpenAI Signed-off-by: Rui Chen <rui@chenrui.dev>
438d1e5 to
b302208
Compare
What
Protect drainer status snapshots with the existing mutex and add concurrent polling coverage.
Why
Status requests and shutdown logging can read operation counts while commands update them.
Testing
Drainer regression fails under -race before the fix; 20 repetitions and the events package pass.