fix(test): stabilize flaky batch goroutine race safety test (issue #810)#934
Conversation
The test launched 100 sequential RunBatchWithProgress calls with a 1ms timeout. When the timeout fired before the batch goroutine was scheduled, RunBatchWithProgress returned but the goroutine hadn't captured its input yet. The final assertions ran before all goroutines finished, seeing fewer captures than expected on loaded CI machines. Fix: add a WaitGroup so the test blocks until every batch goroutine has completed. Switch capture assertions from ordered iteration to a set lookup since goroutine scheduling order is not guaranteed. Refs #810 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for alloyssg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Stabilizes a flaky race-safety regression test in internal/plugin by ensuring all batch goroutines have actually captured their input before assertions run, and by removing ordering assumptions that don’t hold under goroutine scheduling contention.
Changes:
- Add a
sync.WaitGroupto block until all batch goroutines finish capturing inputs before validatingcaptures. - Replace index-ordered assertions with set-based validation of all expected captured items.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Code Review Results
Scope: internal/plugin/hooks_test.go — one test stabilization fix
Intent: Fix flaky batch goroutine receives a stable input snapshot test by adding WaitGroup synchronization and switching to set-based assertions
Reviewers: correctness, testing, maintainability, reliability, project-standards, agent-native, learnings-researcher
P3 — Low
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 1 | hooks_test.go:575 |
wg.Wait() has no timeout guard — if a future RunBatchWithProgress refactor stops calling batchFn, the test hangs silently instead of failing with a diagnostic message. Currently unreachable (batchFn is registered and non-nil). Defense-in-depth: wrap in a channel+select with a generous deadline (e.g. 5s). |
correctness | 75 | advisory → human |
Synthesis Notes
Testing reviewer's "duplicate captures" finding was a false positive. The claim that set-based assertions allow duplicates to pass undetected is incorrect — HaveLen(100) on captures + HaveKey for all 100 items enforces exact-once by the pigeonhole principle. If item-42 is captured twice and item-17 is never captured, HaveKey("item-17") fails. The combination is semantically equivalent to the old ordered assertions for uniqueness.
Maintainability findings discarded:
- Comment "redundancy" — the original comment (lines 525-533) explains the closure-capture race being tested; the new comment (lines 535-539) explains the test infrastructure fix. Different concerns, not duplicative.
- Variable scope for
mu/captures— these are closed over bybatchFnand must be declared before the closure. - Unguarded type assertion
c.input[0].(string)— inputs are always strings; a panic here from a torn slice header IS diagnostic for the bug class being tested.
WaitGroup pattern is correct:
wg.Add(1)in main goroutine beforeRunBatchWithProgressspawns the goroutinedefer wg.Done()inbatchFnfires after capture but before batchFn returns (via<-ctx.Done())wg.Wait()blocks until all captures complete- No deadlock risk: batchFn is always called when registered with non-nil batchFn
Immutable-tests rule: This is a developer PR modifying test code. The change is legitimate — it fixes a test infrastructure bug (missing synchronization + invalid goroutine ordering assumption), not a spec behavior change. Semantic coverage is preserved: torn slice headers, stale variable capture, and exact-once guarantees are all still verified.
Coverage
- Suppressed: 4 findings below confidence threshold or validated as false positives
- Testing gaps: none
- Residual risks: none actionable
- Failed reviewers: none
- All 214 tests pass with
-raceon the PR branch
Verdict: Ready to merge
Reasoning: The fix correctly addresses both root causes of the flaky test: (1) missing synchronization between batch goroutines and assertions, and (2) an invalid assumption about goroutine scheduling order. The WaitGroup pattern is sound, the set-based assertions are semantically equivalent to the old ordered assertions by the pigeonhole principle, and all 214 tests pass with the race detector. The single P3 advisory (timeout guard on
wg.Wait()) is defense-in-depth for an unreachable code path and does not block merge.
Review responseP3 #1 — Timeout guard on wg.Wait()Skipped. Copilot — Unguarded type assertionSkipped. Resolved on thread. A panic from No code changes needed. |
Summary
sync.WaitGroupso the test blocks until all batch goroutines have finished capturing input before running assertions — the root cause was goroutines not yet scheduled when the 1ms timeout fires, leavingcapturesshort at assertion time on loaded CI machinesChanges
internal/plugin/hooks_test.gobatch goroutine receives a stable input snapshottestTest plan
go test -race ./internal/plugin/— 214/214 passRefs #810
🤖 Generated with Claude Code