Skip to content

fix: remove error log when notification manager is already closed #18264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 6, 2025
Merged

fix: remove error log when notification manager is already closed #18264

merged 3 commits into from
Jun 6, 2025

Conversation

ethanndickson
Copy link
Member

@ethanndickson ethanndickson commented Jun 6, 2025 •
edited
Loading

Closes coder/internal#677

Resolves flakes such as:

$ go test -race -run "TestRunStopRace" github.com/coder/coder/v2/coderd/notifications -count=10000 -parallel $(nproc)
--- FAIL: TestRunStopRace (0.00s)
    t.go:106: 2025-06-06 02:44:39.348 [debu]  notifications-manager: notification manager started
    t.go:106: 2025-06-06 02:44:39.348 [debu]  notifications-manager: graceful stop requested
    t.go:106: 2025-06-06 02:44:39.348 [debu]  notifications-manager: notification manager stopped
    t.go:115: 2025-06-06 02:44:39.348 [erro]  notifications-manager: notification manager stopped with error ...
        error= manager already closed:
                   github.com/coder/coder/v2/coderd/notifications.(*Manager).loop
                       /home/coder/coder/coderd/notifications/manager.go:166
         *** slogtest: log detected at level ERROR; TEST FAILURE ***
--- FAIL: TestRunStopRace (0.00s)
    t.go:106: 2025-06-06 02:44:41.632 [debu]  notifications-manager: notification manager started
    t.go:106: 2025-06-06 02:44:41.632 [debu]  notifications-manager: graceful stop requested
    t.go:106: 2025-06-06 02:44:41.632 [debu]  notifications-manager: notification manager stopped
    t.go:115: 2025-06-06 02:44:41.633 [erro]  notifications-manager: notification manager stopped with error ...
        error= manager already closed:
                   github.com/coder/coder/v2/coderd/notifications.(*Manager).loop
                       /home/coder/coder/coderd/notifications/manager.go:166
         *** slogtest: log detected at level ERROR; TEST FAILURE ***
FAIL
FAIL    github.com/coder/coder/v2/coderd/notifications  6.847s
FAIL

These error logs are caused as a result of the Manager Run start operation being asynchronous. In the flaking test case we immediately call Stop after Run. It's possible for Stop to be scheduled to completion before the goroutine spawned by Run calls loop and checks closed. If this happens, loop returns an error and produces the error log.

We'll address this by replacing this specific error log with a warning log.

$ go test -run "TestRunStopRace" github.com/coder/coder/v2/coderd/notifications -count=10000 -parallel $(nproc)
ok      github.com/coder/coder/v2/coderd/notifications  1.294s

$ go test -race github.com/coder/coder/v2/coderd/notifications -count=100 -parallel $(nproc)
ok      github.com/coder/coder/v2/coderd/notifications  26.525s

@ethanndickson ethanndickson force-pushed the ethan/remove-closed-check branch from bf720c0 to b7e88f0 Compare June 6, 2025 03:53
This was referenced Jun 6, 2025
Merged
Merged
@ethanndickson Graphite App
Copy link
Member Author

ethanndickson commented Jun 6, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ethanndickson ethanndickson changed the base branch from ethan/send-ping-results to main June 6, 2025 03:53
ethanndickson
ethanndickson commented Jun 6, 2025
View reviewed changes
@ethanndickson ethanndickson requested a review from mafredri June 6, 2025 04:13
@ethanndickson ethanndickson marked this pull request as ready for review June 6, 2025 04:14
@ethanndickson ethanndickson force-pushed the ethan/remove-closed-check branch from b7e88f0 to f8f5e68 Compare June 6, 2025 05:41
@ethanndickson ethanndickson changed the title fix: remove closed check when starting notification manager fix: remove error log when notification manager is closed on startup Jun 6, 2025
@ethanndickson ethanndickson changed the title fix: remove error log when notification manager is closed on startup fix: remove error log when notification manager is already closed Jun 6, 2025
@ethanndickson ethanndickson merged commit 533c6dc into main Jun 6, 2025
41 checks passed
@ethanndickson ethanndickson deleted the ethan/remove-closed-check branch June 6, 2025 09:28
@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

Assignees

@ethanndickson ethanndickson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

flake: TestRunStopRace
2 participants
@ethanndickson @mafredri