fix: fix flake in TestExecutorAutostartSkipsWhenNoProvisionersAvailable #19478
+57
−15
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
related to usage of time.Now() in MustWaitForProvisionerAvailable and the fact that UpdateProvisionerLastSeenAt can not use a time that is further in the past than the current LastSeenAt time Signed-off-by: Callum Styan <callumstyan@gmail.com>
Comment on lines
1737
to
1738
| require.NoError(t, err, "Error getting provisioner for workspace") | ||
| require.Equal(t, p.LastSeenAt.Time.UnixNano(), staleTime.UnixNano()) |
There was a problem hiding this comment.
Be mindful of the fact you can't call require methods in a goroutine other than the main test goroutine. They all call FailNow, and from the documentation:
FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test. Calling FailNow does not stop those other goroutines.
https://pkg.go.dev/testing#F.FailNow
You can call assert functions, however!
Signed-off-by: Callum Styan <callumstyan@gmail.com>
provisioner Signed-off-by: Callum Styan <callumstyan@gmail.com>
transaction does not go through immediately Signed-off-by: Callum Styan <callumstyan@gmail.com>
johnstcn
reviewed
Aug 25, 2025
Comment on lines
+1660
to
+1680
| require.Eventually(t, func() bool { | ||
| // Use the same logic as hasValidProvisioner but expect false | ||
| provisionerDaemons, err := db.GetProvisionerDaemonsByOrganization(ctx, database.GetProvisionerDaemonsByOrganizationParams{ | ||
| OrganizationID: workspace.OrganizationID, | ||
| WantTags: tags, | ||
| }) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Check if NO provisioners are active (all are stale or gone) | ||
| for _, pd := range provisionerDaemons { | ||
| if pd.LastSeenAt.Valid { | ||
| age := checkTime.Sub(pd.LastSeenAt.Time) | ||
| if age <= provisionerdserver.StaleInterval { | ||
| return false // Found an active provisioner, keep waiting | ||
| } | ||
| } | ||
| } | ||
| return true // No active provisioners found | ||
| }, testutil.WaitMedium, testutil.IntervalFast) |
There was a problem hiding this comment.
suggest using methods in testutil/chan.go and passing in a parent context in the method signature
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.
The flake here had two causes:
Previously the test here was calling
coderdtest.MustWaitForProvisionersAvailablewhich was usingtime.Nowrather than the next tick time like the realhasProvisionersAvailablefunction does. Additionally, when usingUpdateProvisionerLastSeenAtthe underlying db query enforces that the time we're trying to setLastSeenAtto cannot be older than the current value.I was able to reliably reproduce the flake by executing both the
UpdateProvisionerLastSeenAtcall andtickCh <- nextin their own goroutines, the former with a small sleep to reliably ensure we'd trigger the autobuild before we set theLastSeenAttime. That's when I also noticed thatcoderdtest.MustWaitForProvisionersAvailablewas usingtime.Nowinstead of the tick time. When I updated that function to take in a tick time + added a 2nd call toUpdateProvisionerLastSeenAtto set an original non-stale time, we could then never get the test to pass because the later call to set the stale time would not actually modifyLastSeenAt. On top of that, calling the provisioner daemons closer in the middle of the function doesn't really do anything of value in this test.The fix for the flake is to keep the go routines, ensuring there would be a flake if there was not a relevant fix, but to include the fix which is to ensure that we explicitly wait for the provisioner to be stale before passing the time to
tickCh.