Repository navigation
π fix(dashboard): drain review-question autodecide sweep before WS test cleanup - #17452
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Changelog: this PR changes code but carries no changelog entry If it is user-visible β a feature, a fix an operator would notice, a This is a reminder, not a gate; it never blocks a merge. |
Generated by changelog-fragment-guard from the PR title (#5675). Edit the wording freely, or replace this file with the no-changelog label if the change is not user-visible. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: hivecommons-hive[bot] <280983584+hivecommons-hive[bot]@users.noreply.github.com>
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
Hive status β updated in place; edits do not notify. Full history is in the Activity Monitor. hive/awaiting-ci: CI pending β sweep will merge when green. π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 Evidence bundle `#1745278b72de` sha256:24dd86962879ad162664ddb521f865ac510c02e85a765f69e3a126a7242e0085 β download from the dashboard or `hivectl review evidence #17452`. |
There was a problem hiding this comment.
Read the tree, not just the diff. Looks correct to me.
reviewQuestionAutois a value field on*Server(src/pkg/dashboard/server.go:257), sot.Cleanup(s.reviewQuestionAuto.wg.Wait)is a method value on an addressable field β no nil deref, no copy of theWaitGroup.wg.Add(1)runs afterst.running = trueis set underst.mu(review_question_autodecide.go:286-297), and therunningflag already guarantees at most one sweep in flight, soAddcan't raceWaitfrom a second sweep.- Ordering:
setupWSTestregisters the wait afterredirectContributeWSDiskand before the hubClose/handlers.Waitcleanups (contribute_ws_test.go:22-40), so LIFO gives hub Close β HandleWS unwind β sweep drain β globals restored β TempDir removal. That's the order the race inTestStandbyAutoDispatchUsesAcceptedFreshStatusQueueneeded.
Open note, not a defect: only setupWSTest callers get the drain. Any other test that pairs redirectContributeWSDisk with UpdateStatusIfFresh on a server that has a GHClient would still be exposed; none in the current tree that I could see.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
UpdateStatusIfFreshstarts a backgroundmaybeAutoDecideReviewQuestionsgoroutine. InTestStandbyAutoDispatchUsesAcceptedFreshStatusQueueit outlived the test body, soredirectContributeWSDisk's Cleanup restoredauditLogPath(api_contribute_test.go:55) while the sweep was still reading it viaAuditLog.HasOnDiskLog(audit.go:454, frombuildStageTrackerβsupersessionAuditEntries), tripping the race detector (run 42700566451, job 129444134549).The sweep is now tracked by a
WaitGroupon the autodecide state, andsetupWSTestregisters a Cleanup that waits on it; being registered after the redirect, it runs first (LIFO). No runtime behaviour change.Closes #17445
β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88