Repository navigation
test(swarm): wait for every slot to be released before checking the drain - #1181
Conversation
…rain A finishing member's task reads Done before its slot is freed: the supervisor completes the task, removes the member and only then frees the slot in afterExit. The test waited for Done == 5 and checked the running count straight after, so a busy runner could see one slot still held. It now waits for the drain itself.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Wait for task and team completion internal/swarm/lifecycle_test.go |
The test waits for five completed tasks, zero running members, and an empty queue in one condition. Comments clarify that task completion can precede slot release. |
Priority:
Estimated code review effort: 1 (Trivial) | ~5 minutes
Change: Other
Suggested reviewers: gnanam1990
Merge Risk: 
79e51
-
The updated test waits for the full drain before asserting state, with no merge-blocking risk identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the test change: it waits for every concurrency slot to be released before checking that the queue drains. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
The test now waits for the actual drain boundary while preserving its initial concurrency-cap check, five-task completion requirement, zero running/queued counts, and exactly-five-launch assertion. The added comment matches the supervisor's completion-before-slot-release order.
Reviewed scope
| Surface | Contract and result |
|---|---|
TestConcurrencyCapAndQueueDrains |
Five registered tasks; two running and three queued before release; eventually five done, zero occupied slots and zero queued tasks; five launches. All assertions retained. |
| Completion and cleanup | Traced launcher → supervisor → coordinator completion → member removal → execution barrier → slot release/queue drain. Close joins outstanding lifecycle work and watchers. |
| Changed explanatory comment | Accurate for the production order. No production, persistence, protocol, dependency or operator-documentation change. |
Validation
- Focused test: 100 repetitions under
-racepassed; fullinternal/swarmpackage under-racepassed; scopedgo vetand diff hygiene passed. - A 20 ms delay before member removal reproduces the merge-base test's
running=1 queue=0failure; the changed test passes with the same delay. - A deliberate permanent slot leak makes the changed test fail at its bounded timeout, confirming that the synchronization repair preserves leak detection.
- Completed primary inspection, two waves of two independent blind searches, a missing-contract search, candidate reconciliation, intent/attribution audit and final clean-outcome challenge.
Merge readiness
Reviewed head 79e51c87f2c28cb4de6a6547dc25520964111bd5 against merge-base/live main 4247a07aba0f89c3c2597314a33edaede56be70c. The branch is current and mergeable; all 13 reported GitHub checks pass, including race and Linux/macOS/Windows smoke jobs. No duplicate or superseding drain repair, unresolved actionable reviewer request, or scope drift was identified. Author association is COLLABORATOR, so the approved-community-issue requirement does not apply.
Local execution was on Linux. Native macOS/Windows execution and the repository-wide suite were not repeated locally; the exact-head CI results provide those broader checks. The CodeRabbit docstring coverage notice is advisory and does not identify a defect in this test-only change.
What changed
TestConcurrencyCapAndQueueDrainsnow waits until all five tasks are done and every slot is released, instead of checking the running count the moment the fifth task reads Done. Test-only.Why
It failed the Zero Review job on #835, which doesn't touch
internal/swarm:A finishing member's task reads Done before its slot is freed. The supervisor in
lifecycle.gocallsCompleteWithSession, thenremoveMember, and only thenafterExit, whose drain releases the slot. The test waited for Done == 5 and checkedRunning()straight after, so on a busy runner it could land in that gap. The production order looks deliberate: the barrier is published before the drain on purpose (see the comment aboverun.finish()), and the slot is freed as part of that drain. So the fix is in the test.Linked issue
None. Maintainer fix for a CI flake, opened through the internal development process.
Verification
t.removeMemberinlifecycle.go, the old test fails every run with the same message and the new one passes.-racepass.go vet ./...for windows, linux and darwin,gofmt,git diff --check,go test ./..., the release build and smoke, andgovulncheckfor linux and windows all pass.Summary by CodeRabbit