With just the test addition the test will fail since the workspace build is triggered, we attempt to move the workspace into thestart state:require.Equal(t, codersdk.WorkspaceTransitionStop, workspace.LatestBuild.Transition)

    lifecycle_executor_test.go:1718:                 Error Trace:    /home/coder/coder/coderd/autobuild/lifecycle_executor_test.go:1718                Error:          Not equal:                                 expected: "stop"                                actual  : "start"                                                            Diff:                                --- Expected                                +++ Actual                                @@ -1,2 +1,2 @@                                -(codersdk.WorkspaceTransition) (len=4) "stop"                                +(codersdk.WorkspaceTransition) (len=5) "start"                                                 Test:           TestExecutorAutostartSkipsWhenNoProvisionersAvailable

With the 2nd commit we skip the calls towsbuilder.New andbuilder.Build and so the transition is never scheduled.

coder@automatic-builds-test:~/coder/coderd/autobuild$ go test -run TestExecutorAutostartSkipsWhenNoProvisionersAvailablePASSok      github.com/coder/coder/v2/coderd/autobuild      1.134scoder@automatic-builds-test:~/coder/coderd/autobuild$ go test -run TestExecutorAutostartSkipsWhenNoProvisionersAvailable -v | grep autostart    t.go:106: 2025-07-28 22:11:09.405 [warn]  coderd.autobuild: skipping autostart - no available provisioners  workspace_id=219f4901-dfd0-4cdc-8cf2-258861adfd18  workspace_name=dazzling-jemison3-89l

However, many tests don't fully setup provisioner or wait for the provisioner + DB to be fully available, so the third commit adds a somewhat smelly way to skip the provisioner check in tests that don't need to test that functionality.

github-actionsbot assignedcstyan

Jul 29, 2025

cstyan changed the title~~fix: don't create autostart workspace builds if there is no available provisioners for the job~~fix: don't create autostart workspace builds with no available provisioners

Jul 29, 2025

deansheather self-requested a review

July 30, 2025 17:04

deansheather reviewed

Jul 31, 2025

View reviewed changes

coderd/autobuild/lifecycle_executor.go OutdatedShow resolvedHide resolved

cstyan added5 commits

August 5, 2025 19:54

add test that fails since we still trigger autobuilds even if a

314bb66

provisioner is not availableSigned-off-by: Callum Styan <callumstyan@gmail.com>

add check for available provisioners before we try to do a workspace

310511e

transition so that we don't accumulate autostart jobs that willeventually have to be cleaned up by the reaper routineSigned-off-by: Callum Styan <callumstyan@gmail.com>

fix provisioner check for tests which don't fully set up/wait for the

1256877

DB/provisioner to be available, so add a way to skip the checkSigned-off-by: Callum Styan <callumstyan@gmail.com>

use the runOnce time

26f5001

Signed-off-by: Callum Styan <callumstyan@gmail.com>

refactor: remove SkipProvisionerCheck and improve test provisioner ha…

78a8f5e

…ndlingThis commit addresses the requirements to:1. Comment out provisionerCloser.Close() in TestExecutorAutostartSkipsWhenNoProvisionersAvailable2. Remove the need for SkipProvisionerCheck by refactoring testsChanges:- Remove SkipProvisionerCheck field and logic from Executor- Remove SkipProvisionerCheck option from coderdtest.Options- Add proper provisioner daemon tags to match template requirements- Add helper functions for waiting for provisioner availability- Update tests to use real provisioner availability checks- Fix timing issues in provisioner staleness tests- Comment out provisioner close call to test proper behaviorThe test now correctly validates that provisioners remain availablewhen not explicitly closed, and uses the real hasAvailableProvisionerslogic instead of bypassing it.Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan force-pushed thecallum-autostart-no-provisioner branch from9971794 to78a8f5eCompare

August 5, 2025 20:01

more fixes for tests; mostly around ensuring that tests which use the

a983dc2

autobuild path have a valid provisioner available and pass anappropriate time.Time to the channel for triggering the build based onwhether the provisioner is supposed to be valid or staleSigned-off-by: Callum Styan <callumstyan@gmail.com>

Copy link

ContributorAuthor

cstyan commentedAug 7, 2025

Okay@deansheather thanks for your patience, I think this is finally ready for another review. Took quite a bit of digging with the help of Blink to find all the bits and pieces that had to be updated to get the tests working without theskipProvisionerCheck flag. I've done my own run through of the changes twice now but I may have still missed some small stuff, nit pick away 👍

The TL; DR of the changes is that we in tests which deal with autobuilds we need:

to create the test in a way where we also return the handle to the database instance
ensure the test creates a provisioner with the relevant tags for the workspace that will be used within that test case (in most cases this is just the defaultowner: "", scope: organization but there is a mechanism for using specific tags in at least one of the tests)
based on whether the autobuild should happen or not, we need to ensure we can retrieve the relevant provisioner and sest the scheduled tick time for the autobuild appropriately (should the provisioner be valid, or should it be stale)

deansheather reviewed

Aug 8, 2025

View reviewed changes

coderd/autobuild/lifecycle_executor.go OutdatedShow resolvedHide resolved

coderd/autobuild/lifecycle_executor.goShow resolvedHide resolved

coderd/autobuild/lifecycle_executor.go OutdatedShow resolvedHide resolved

coderd/autobuild/lifecycle_executor_test.go OutdatedShow resolvedHide resolved

enterprise/coderd/workspaces_test.go OutdatedShow resolvedHide resolved

coderd/coderdtest/coderdtest.go OutdatedShow resolvedHide resolved

cstyan added7 commits

August 11, 2025 19:49

significantly simplify

0bc4cce

TestExecutorAutostartSkipsWhenNoProvisionersAvailableSigned-off-by: Callum Styan <callumstyan@gmail.com>

more refactor and clean up

9a3f534

Signed-off-by: Callum Styan <callumstyan@gmail.com>

fix lint

a98ef1d

Signed-off-by: Callum Styan <callumstyan@gmail.com>

hasValidProvisioner only needs tags

e48c724

Signed-off-by: Callum Styan <callumstyan@gmail.com>

more cleanup of function params

883c748

Signed-off-by: Callum Styan <callumstyan@gmail.com>

already waiting for the workspaces provisioner, don't need to wait for

3c90c6d

any before thatSigned-off-by: Callum Styan <callumstyan@gmail.com>

fix print statement missed in earlier commit

5a6493c

Signed-off-by: Callum Styan <callumstyan@gmail.com>

deansheather reviewed

Aug 12, 2025

View reviewed changes

coderd/autobuild/lifecycle_executor_test.go

Comment on lines +59 to +65

		p,err:=coderdtest.GetProvisionerForTags(db,time.Now(),workspace.OrganizationID,map[string]string{})
		require.NoError(t,err)
		// When: the autobuild executor ticks after the scheduled time
		gofunc() {
		tickCh<-sched.Next(workspace.LatestBuild.CreatedAt)
		tickTime:=sched.Next(workspace.LatestBuild.CreatedAt)
		coderdtest.UpdateProvisionerLastSeenAt(t,db,p.ID,tickTime)
		tickCh<-tickTime

Copy link

Member

deansheatherAug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

W.R.T the last comment from the previous review, this is what I'm referring to. Is this change (and other similar changes) necessary?

Copy link

ContributorAuthor

cstyanAug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, AFAICT it is. Otherwise the test is flakey, it would only pass if you run the test withinStaleInterval seconds of the top of any hour.

When one of these tests starts, the provisioner has aLastSeenAt time oftime.Now() (sometimes + 1 second or so), butsched.Next(workspace.LatestBuild.CreatedAt) will return a value that is the top of the hour.

For example: right now if I comment out theUpdateProvisionerLastSeenAt line here, inhasValidProvisioner thepd.LastSeenAt time is2025-08-12 16:34:02.908042 +0000 UTC butt from thetickCh is2025-08-12 17:00:00 +0000 UTC. So in a real running deployment of Coder that provisioner would be considered stale.

This is the exact thing we were trying to solve for with the addition ofhasValidProvisioner. So any test case which utilized the autobuild functionality but needed to continue passing needed to update theLastSeenAt value. In the test I have added, which should see the provisioner as stale and then assert that there were no autobuild transitions initiated, we ensure theLastSeenAt time is far enough in the past that the provisioner is stale.

Copy link

Member

deansheatherAug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ahhh I get it, because the autostart time is way in the future.

Perhaps a better option for these autostarty tests to set a value on the lifecycle executor likeTestIgnoreNoDaemons: true, and then just set it to true for these tests. Avoids having to get the provisioners and update them right before ticking, which feels like it could be racey

Copy link

ContributorAuthor

cstyanAug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Isn't that essentially just going back to what I had originally?#19067 (comment)

I guess we're doing the configuration in a cleaner way, but we'd still be setting that value to true for essentially all of these tests, which is what we wanted to change.

Copy link

Member

deansheatherAug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah I think I may have looped... sorry. I think what you have now is good then 👍

coderd/autobuild/lifecycle_executor_test.go OutdatedShow resolvedHide resolved

coderd/autobuild/lifecycle_executor_test.goShow resolvedHide resolved

coderd/coderdtest/coderdtest.goShow resolvedHide resolved

coderd/coderdtest/coderdtest.go OutdatedShow resolvedHide resolved

enterprise/coderd/workspaces_test.goShow resolvedHide resolved

cstyan added5 commits

August 12, 2025 17:30

address more review feedback; mostly around provisioner closers in the

c5f0d41

new testSigned-off-by: Callum Styan <callumstyan@gmail.com>

remove MustWaitForAnyProvisionerWithClient

224acfc

Signed-off-by: Callum Styan <callumstyan@gmail.com>

Merge branch 'main' into callum-autostart-no-provisioner

e7d748e

Signed-off-by: Callum Styan <callumstyan@gmail.com>

not sure how this was passing before, the prebuilds NextStartAt time is

3275c57

nil hereSigned-off-by: Callum Styan <callumstyan@gmail.com>

this test also needs to have the provisioner time updated

bae5c80

Signed-off-by: Callum Styan <callumstyan@gmail.com>

deansheather approved these changes

Aug 15, 2025

View reviewed changes

cstyan merged commit6c902a7 intomain

Aug 15, 2025

26 checks passed

cstyan deleted the callum-autostart-no-provisioner branch

August 15, 2025 15:50

github-actionsbot locked and limited conversation to collaborators

Aug 15, 2025

Labels

None yet

2 participants

Movatterモバイル変換

fix: don't create autostart workspace builds with no available provisioners#19067

fix: don't create autostart workspace builds with no available provisioners#19067

Uh oh!

Conversation

cstyan commentedJul 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cstyan commentedAug 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deansheatherAug 12, 2025

Choose a reason for hiding this comment

Uh oh!

cstyanAug 12, 2025

Choose a reason for hiding this comment

Uh oh!

deansheatherAug 13, 2025

Choose a reason for hiding this comment

Uh oh!

cstyanAug 13, 2025

Choose a reason for hiding this comment

Uh oh!

deansheatherAug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!