fix: coderd: decouple ttl and deadline (#2282)

This commit makes the following changes:

- Partially reverts the changes of feat: update workspace deadline when workspace ttl updated #2165, making the deadline of a running workspace build independant of TTL, once started.
- CLI: updating a workspace TTL no longer updates the deadline of the workspace.
- UI: updating a workspace TTL no longer updates the deadline of the workspace.
- Drive-by: API: When creating a workspace, default TTL to min(12 hours, template max_ttl) if not instructed otherwise.
- Drive-by: CLI: list: measure workspace extension correctly (+X in last column) from the time the provisioner job was completed
- Drive-by: WorkspaceSchedule: show timezone of schedule if it is set, defaulting to dayjs guess otherwise.
- Drive-by: WorkspaceScheduleForm: fixed an issue where deleting the "TTL" value in the form would show the text "Your workspace will shut down a few seconds after start".

coderd/autobuild/executor/lifecycle_executor.go

@@ -85,11 +85,9 @@ func (e *Executor) runOnce(t time.Time) Stats {
 		// is what we compare against when performing autostop operations, rounded down
 		// to the minute.
 		//
-		// NOTE: Currently, if a workspace build is created with a given TTL and then
-		//       the user either changes or unsets the TTL, the deadline for the workspace
-		//       build will not have changed. So, autostop will still happen at the
-		//       original TTL value from when the workspace build was created.
-		//       Whether this is expected behavior from a user's perspective is not yet known.
+		// NOTE: If a workspace build is created with a given TTL and then the user either
+		//       changes or unsets the TTL, the deadline for the workspace build will not
+		//       have changed. This behavior is as expected per #2229.
 		eligibleWorkspaces, err := db.GetWorkspacesAutostart(e.ctx)
 		if err != nil {
 			return xerrors.Errorf("get eligible workspaces for autostart or autostop: %w", err)

coderd/autobuild/executor/lifecycle_executor_test.go

@@ -308,8 +308,7 @@ func TestExecutorAutostopNotEnabled(t *testing.T) {
 	require.NoError(t, err)
 	require.Nil(t, workspace.TTLMillis)
 
-	// TODO(cian): need to stop and start the workspace as we do not update the deadline yet
-	//             see: https://github.com/coder/coder/issues/1783
+	// TODO(cian): need to stop and start the workspace as we do not update the deadline. See: #2229
 	coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop)
 	coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStop, database.WorkspaceTransitionStart)
 
@@ -440,29 +439,36 @@ func TestExecutorWorkspaceAutostopNoWaitChangedMyMind(t *testing.T) {
 	err := client.UpdateWorkspaceTTL(ctx, workspace.ID, codersdk.UpdateWorkspaceTTLRequest{TTLMillis: nil})
 	require.NoError(t, err)
 
-	// Then: the deadline should be the zero value
+	// Then: the deadline should still be the original value
 	updated := coderdtest.MustWorkspace(t, client, workspace.ID)
-	assert.Zero(t, updated.LatestBuild.Deadline)
+	assert.WithinDuration(t, workspace.LatestBuild.Deadline, updated.LatestBuild.Deadline, time.Minute)
 
 	// When: the autobuild executor ticks after the original deadline
 	go func() {
 		tickCh <- workspace.LatestBuild.Deadline.Add(time.Minute)
 	}()
 
-	// Then: the workspace should not stop
+	// Then: the workspace should stop
 	stats := <-statsCh
 	assert.NoError(t, stats.Error)
-	assert.Len(t, stats.Transitions, 0)
+	assert.Len(t, stats.Transitions, 1)
+	assert.Equal(t, stats.Transitions[workspace.ID], database.WorkspaceTransitionStop)
+
+	// Wait for stop to complete
+	updated = coderdtest.MustWorkspace(t, client, workspace.ID)
+	_ = coderdtest.AwaitWorkspaceBuildJob(t, client, updated.LatestBuild.ID)
+
+	// Start the workspace again
+	workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStop, database.WorkspaceTransitionStart)
 
 	// Given: the user changes their mind again and wants to enable auto-stop
 	newTTL := 8 * time.Hour
-	expectedDeadline := workspace.LatestBuild.UpdatedAt.Add(newTTL)
 	err = client.UpdateWorkspaceTTL(ctx, workspace.ID, codersdk.UpdateWorkspaceTTLRequest{TTLMillis: ptr.Ref(newTTL.Milliseconds())})
 	require.NoError(t, err)
 
-	// Then: the deadline should be updated based on the TTL
+	// Then: the deadline should remain at the zero value
 	updated = coderdtest.MustWorkspace(t, client, workspace.ID)
-	assert.WithinDuration(t, expectedDeadline, updated.LatestBuild.Deadline, time.Minute)
+	assert.Zero(t, updated.LatestBuild.Deadline)
 
 	// When: the relentless onward march of time continues
 	go func() {
@@ -470,11 +476,10 @@ func TestExecutorWorkspaceAutostopNoWaitChangedMyMind(t *testing.T) {
 		close(tickCh)
 	}()
 
-	// Then: the workspace should stop
+	// Then: the workspace should not stop
 	stats = <-statsCh
 	assert.NoError(t, stats.Error)
-	assert.Len(t, stats.Transitions, 1)
-	assert.Equal(t, stats.Transitions[workspace.ID], database.WorkspaceTransitionStop)
+	assert.Len(t, stats.Transitions, 0)
 }
 
 func TestExecutorAutostartMultipleOK(t *testing.T) {

coderd/workspaces.go

@@ -31,6 +31,8 @@ import (
 	"github.com/coder/coder/codersdk"
 )
 
+const workspaceDefaultTTL = 12 * time.Hour
+
 func (api *API) workspace(rw http.ResponseWriter, r *http.Request) {
 	workspace := httpmw.WorkspaceParam(r)
 	if !api.Authorize(r, rbac.ActionRead, workspace) {
@@ -291,8 +293,8 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
 	}
 
 	if !dbTTL.Valid {
-		// Default to template maximum when creating a new workspace
-		dbTTL = sql.NullInt64{Valid: true, Int64: template.MaxTtl}
+		// Default to min(12 hours, template maximum). Just defaulting to template maximum can be surprising.
+		dbTTL = sql.NullInt64{Valid: true, Int64: min(template.MaxTtl, int64(workspaceDefaultTTL))}
 	}
 
 	workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{
@@ -513,30 +515,22 @@ func (api *API) putWorkspaceTTL(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	template, err := api.Database.GetTemplateByID(r.Context(), workspace.TemplateID)
-	if err != nil {
-		httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
-			Message: "Error fetching workspace template!",
-		})
-		return
-	}
+	var validErrs []httpapi.Error
 
-	dbTTL, err := validWorkspaceTTLMillis(req.TTLMillis, time.Duration(template.MaxTtl))
-	if err != nil {
-		httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
-			Message: "Invalid workspace TTL.",
-			Detail:  err.Error(),
-			Validations: []httpapi.Error{
-				{
-					Field:  "ttl_ms",
-					Detail: err.Error(),
-				},
-			},
-		})
-		return
-	}
+	err := api.Database.InTx(func(s database.Store) error {
+		template, err := s.GetTemplateByID(r.Context(), workspace.TemplateID)
+		if err != nil {
+			httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
+				Message: "Error fetching workspace template!",
+			})
+			return xerrors.Errorf("fetch workspace template: %w", err)
+		}
 
-	err = api.Database.InTx(func(s database.Store) error {
+		dbTTL, err := validWorkspaceTTLMillis(req.TTLMillis, time.Duration(template.MaxTtl))
+		if err != nil {
+			validErrs = append(validErrs, httpapi.Error{Field: "ttl_ms", Detail: err.Error()})
+			return err
+		}
 		if err := s.UpdateWorkspaceTTL(r.Context(), database.UpdateWorkspaceTTLParams{
 			ID:  workspace.ID,
 			Ttl: dbTTL,
@@ -544,44 +538,18 @@ func (api *API) putWorkspaceTTL(rw http.ResponseWriter, r *http.Request) {
 			return xerrors.Errorf("update workspace TTL: %w", err)
 		}
 
-		// Also extend the workspace deadline if the workspace is running
-		latestBuild, err := s.GetLatestWorkspaceBuildByWorkspaceID(r.Context(), workspace.ID)
-		if err != nil {
-			return xerrors.Errorf("get latest workspace build: %w", err)
-		}
-
-		if latestBuild.Transition != database.WorkspaceTransitionStart {
-			return nil // nothing to do
-		}
-
-		if latestBuild.UpdatedAt.IsZero() {
-			// Build in progress; provisionerd should update with the new TTL.
-			return nil
-		}
-
-		var newDeadline time.Time
-		if dbTTL.Valid {
-			newDeadline = latestBuild.UpdatedAt.Add(time.Duration(dbTTL.Int64))
-		}
-
-		if err := s.UpdateWorkspaceBuildByID(
-			r.Context(),
-			database.UpdateWorkspaceBuildByIDParams{
-				ID:               latestBuild.ID,
-				UpdatedAt:        latestBuild.UpdatedAt,
-				ProvisionerState: latestBuild.ProvisionerState,
-				Deadline:         newDeadline,
-			},
-		); err != nil {
-			return xerrors.Errorf("update workspace deadline: %w", err)
-		}
 		return nil
 	})
 
 	if err != nil {
-		httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
-			Message: "Error updating workspace time until shutdown!",
-			Detail:  err.Error(),
+		code := http.StatusInternalServerError
+		if len(validErrs) > 0 {
+			code = http.StatusBadRequest
+		}
+		httpapi.Write(rw, code, httpapi.Response{
+			Message:     "Error updating workspace time until shutdown!",
+			Validations: validErrs,
+			Detail:      err.Error(),
 		})
 		return
 	}
@@ -1028,3 +996,10 @@ func splitQueryParameterByDelimiter(query string, delimiter rune, maintainQuotes
 
 	return parts
 }
+
+func min(x, y int64) int64 {
+	if x < y {
+		return x
+	}
+	return y
+}

coderd/workspaces_test.go

@@ -867,23 +867,20 @@ func TestWorkspaceUpdateTTL(t *testing.T) {
 	t.Parallel()
 
 	testCases := []struct {
-		name             string
-		ttlMillis        *int64
-		expectedError    string
-		expectedDeadline *time.Time
-		modifyTemplate   func(*codersdk.CreateTemplateRequest)
+		name           string
+		ttlMillis      *int64
+		expectedError  string
+		modifyTemplate func(*codersdk.CreateTemplateRequest)
 	}{
 		{
-			name:             "disable ttl",
-			ttlMillis:        nil,
-			expectedError:    "",
-			expectedDeadline: ptr.Ref(time.Time{}),
+			name:          "disable ttl",
+			ttlMillis:     nil,
+			expectedError: "",
 		},
 		{
-			name:             "update ttl",
-			ttlMillis:        ptr.Ref(12 * time.Hour.Milliseconds()),
-			expectedError:    "",
-			expectedDeadline: ptr.Ref(time.Now().Add(12*time.Hour + time.Minute)),
+			name:          "update ttl",
+			ttlMillis:     ptr.Ref(12 * time.Hour.Milliseconds()),
+			expectedError: "",
 		},
 		{
 			name:          "below minimum ttl",
@@ -891,16 +888,14 @@ func TestWorkspaceUpdateTTL(t *testing.T) {
 			expectedError: "ttl must be at least one minute",
 		},
 		{
-			name:             "minimum ttl",
-			ttlMillis:        ptr.Ref(time.Minute.Milliseconds()),
-			expectedError:    "",
-			expectedDeadline: ptr.Ref(time.Now().Add(2 * time.Minute)),
+			name:          "minimum ttl",
+			ttlMillis:     ptr.Ref(time.Minute.Milliseconds()),
+			expectedError: "",
 		},
 		{
-			name:             "maximum ttl",
-			ttlMillis:        ptr.Ref((24 * 7 * time.Hour).Milliseconds()),
-			expectedError:    "",
-			expectedDeadline: ptr.Ref(time.Now().Add(24*7*time.Hour + time.Minute)),
+			name:          "maximum ttl",
+			ttlMillis:     ptr.Ref((24 * 7 * time.Hour).Milliseconds()),
+			expectedError: "",
 		},
 		{
 			name:          "above maximum ttl",
@@ -953,9 +948,6 @@ func TestWorkspaceUpdateTTL(t *testing.T) {
 			require.NoError(t, err, "fetch updated workspace")
 
 			require.Equal(t, testCase.ttlMillis, updated.TTLMillis, "expected autostop ttl to equal requested")
-			if testCase.expectedDeadline != nil {
-				require.WithinDuration(t, *testCase.expectedDeadline, updated.LatestBuild.Deadline, time.Minute, "expected autostop deadline to be equal expected")
-			}
 		})
 	}