fix: fix TestPendingUpdatesMetric flaky assertion #14534
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
dannykopping
approved these changes
Sep 3, 2024
There was a problem hiding this comment.
LGTM, thanks for fixing this @spikecurtis!
There's a minor change regarding dbtime to be made before merging.
| func newNotifier(cfg codersdk.NotificationsConfig, id uuid.UUID, log slog.Logger, db Store, | ||
| hr map[database.NotificationMethod]Handler, metrics *Metrics, clock quartz.Clock, | ||
| ) *notifier { | ||
| tick := clock.NewTicker(cfg.FetchInterval.Value(), "notifier", "fetchInterval") |
There was a problem hiding this comment.
I love these ticker tags ❤️
0747bb8 to
3ae3fbb
Compare
Merge activity
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes flake seen here:
https://github.com/coder/coder/actions/runs/10677025332/job/29591518118
The original test waits for DB calls to update success and failure of notifications, and blocks on a
pausechannel. However, it uses a single pause channel for both DB calls, and so implicitly assumes that both success and failure updates occur during an update sync.However, the update sync timer isn't synchronized to anything, and so there is a race where the update sync only has either the success or the failure result but not both. This blocks the test, which waits for both, and deadlocks before unpausing.
It appears the unpause mechanism is there to test that the
PendingUpdatesmetric updates accordingly.This fixes the flake by:
Managersyncs updates.PendingUpdatesmetric to reach 2, so that we know that both success and failure have been queued upThis has the nice property that a "pause" function is no longer required: we know the manager is in our desired state before we trigger the update, and can assert on the Metrics before and after.