fix: truncate ttl to time.Minute

coder · greyscaled · May 19, 2022 · May 18, 2022 · May 19, 2022 · May 19, 2022
commit 3ea3e903f0a2ce087311821048bdfbefd88c1d77
diff --git a/agent/usershell/usershell_darwin.go b/agent/usershell/usershell_darwin.go
@@ -3,6 +3,6 @@ package usershell
 import "os"
 
 // Get returns the $SHELL environment variable.
-func Get(username string) (string, error) {
+func Get(_ string) (string, error) {
 	return os.Getenv("SHELL"), nil
 }
diff --git a/cli/ssh.go b/cli/ssh.go
@@ -269,11 +269,11 @@ func notifyCondition(ctx context.Context, client *codersdk.Client, workspaceID u
 			return time.Time{}, nil
 		}
 
-		if ws.TTL == nil {
+		if ws.TTL == nil || *ws.TTL == 0 {
 			return time.Time{}, nil
 		}
 
-		deadline = now.Add(*ws.TTL)
+		deadline = ws.LatestBuild.UpdatedAt.Add(*ws.TTL)
 		callback = func() {
 			ttl := deadline.Sub(now)
 			var title, body string

diff --git a/cli/ttl.go b/cli/ttl.go
@@ -5,11 +5,14 @@ import (
 	"time"
 
 	"github.com/spf13/cobra"
+	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/codersdk"
 )
 
-const ttlDescriptionLong = `To have your workspace stop automatically after a configurable interval has passed.`
+const ttlDescriptionLong = `To have your workspace stop automatically after a configurable interval has passed.
+Minimum TTL is 1 minute.
+`
 
 func ttl() *cobra.Command {
 	ttlCmd := &cobra.Command{
@@ -83,8 +86,18 @@ func ttlEnable() *cobra.Command {
 				return err
 			}
 
+			truncated := ttl.Truncate(time.Minute)
+
+			if truncated == 0 {
+				return xerrors.Errorf("ttl must be at least 1m")
+			}
+
+			if truncated != ttl {
+				_, _ = fmt.Fprintf(cmd.OutOrStdout(), "warning: ttl rounded down to %s", truncated)
+			}
+
 			err = client.UpdateWorkspaceTTL(cmd.Context(), workspace.ID, codersdk.UpdateWorkspaceTTLRequest{
-				TTL: &ttl,
+				TTL: &truncated,
 			})
 			if err != nil {
 				return err

diff --git a/cli/ttl_test.go b/cli/ttl_test.go
@@ -30,7 +30,7 @@ func TestTTL(t *testing.T) {
 			project   = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
 			workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID)
 			cmdArgs   = []string{"ttl", "show", workspace.Name}
-			ttl       = 8 * time.Hour
+			ttl       = 8*time.Hour + 30*time.Minute + 30*time.Second
 			stdoutBuf = &bytes.Buffer{}
 		)
 
@@ -45,7 +45,7 @@ func TestTTL(t *testing.T) {
 
 		err = cmd.Execute()
 		require.NoError(t, err, "unexpected error")
-		require.Equal(t, strings.TrimSpace(stdoutBuf.String()), ttl.String())
+		require.Equal(t, ttl.Truncate(time.Minute).String(), strings.TrimSpace(stdoutBuf.String()))
 	})
 
 	t.Run("EnableDisableOK", func(t *testing.T) {
@@ -60,7 +60,7 @@ func TestTTL(t *testing.T) {
 			_         = coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
 			project   = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
 			workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID)
-			ttl       = 8*time.Hour + 30*time.Minute
+			ttl       = 8*time.Hour + 30*time.Minute + 30*time.Second
 			cmdArgs   = []string{"ttl", "enable", workspace.Name, ttl.String()}
 			stdoutBuf = &bytes.Buffer{}
 		)
@@ -75,7 +75,8 @@ func TestTTL(t *testing.T) {
 		// Ensure autostop schedule updated
 		updated, err := client.Workspace(ctx, workspace.ID)
 		require.NoError(t, err, "fetch updated workspace")
-		require.Equal(t, *updated.TTL, ttl)
+		require.Equal(t, ttl.Truncate(time.Minute), *updated.TTL)
+		require.Contains(t, stdoutBuf.String(), "warning: ttl rounded down")
 
 		// Disable schedule
 		cmd, root = clitest.New(t, "ttl", "disable", workspace.Name)
@@ -91,6 +92,51 @@ func TestTTL(t *testing.T) {
 		require.Nil(t, updated.TTL, "expected ttl to not be set")
 	})
 
+	t.Run("ZeroInvalid", func(t *testing.T) {
+		t.Parallel()
+
+		var (
+			ctx       = context.Background()
+			client    = coderdtest.New(t, nil)
+			_         = coderdtest.NewProvisionerDaemon(t, client)
+			user      = coderdtest.CreateFirstUser(t, client)
+			version   = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
+			_         = coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
+			project   = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
+			workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID)
+			ttl       = 8*time.Hour + 30*time.Minute + 30*time.Second
+			cmdArgs   = []string{"ttl", "enable", workspace.Name, ttl.String()}
+			stdoutBuf = &bytes.Buffer{}
+		)
+
+		cmd, root := clitest.New(t, cmdArgs...)
+		clitest.SetupConfig(t, client, root)
+		cmd.SetOut(stdoutBuf)
+
+		err := cmd.Execute()
+		require.NoError(t, err, "unexpected error")
+
+		// Ensure ttl updated
+		updated, err := client.Workspace(ctx, workspace.ID)
+		require.NoError(t, err, "fetch updated workspace")
+		require.Equal(t, ttl.Truncate(time.Minute), *updated.TTL)
+		require.Contains(t, stdoutBuf.String(), "warning: ttl rounded down")
+
+		// A TTL of zero is not considered valid.
+		stdoutBuf.Reset()
+		cmd, root = clitest.New(t, "ttl", "enable", workspace.Name, "0s")
+		clitest.SetupConfig(t, client, root)
+		cmd.SetOut(stdoutBuf)
+
+		err = cmd.Execute()
+		require.EqualError(t, err, "ttl must be at least 1m", "unexpected error")
+
+		// Ensure ttl remains as before
+		updated, err = client.Workspace(ctx, workspace.ID)
+		require.NoError(t, err, "fetch updated workspace")
+		require.Equal(t, ttl.Truncate(time.Minute), *updated.TTL)
+	})
+
 	t.Run("Enable_NotFound", func(t *testing.T) {
 		t.Parallel()
 

diff --git a/coderd/autobuild/executor/lifecycle_executor.go b/coderd/autobuild/executor/lifecycle_executor.go
@@ -84,7 +84,7 @@ func (e *Executor) runOnce(t time.Time) error {
 			}
 
 			var validTransition database.WorkspaceTransition
-			var nextTransitionAt time.Time
+			var nextTransition time.Time
 			switch priorHistory.Transition {
 			case database.WorkspaceTransitionStart:
 				validTransition = database.WorkspaceTransitionStop
@@ -97,8 +97,9 @@ func (e *Executor) runOnce(t time.Time) error {
 				}
 				ttl := time.Duration(ws.Ttl.Int64)
 				// Measure TTL from the time the workspace finished building.
-				// This can be finer granularity than 1 minute.
-				nextTransitionAt = priorHistory.UpdatedAt.Add(ttl)
+				// Truncate to nearest minute for consistency with autostart
+				// behavior, and add one minute for padding.
+				nextTransition = priorHistory.UpdatedAt.Truncate(time.Minute).Add(ttl + time.Minute)
 			case database.WorkspaceTransitionStop:
 				validTransition = database.WorkspaceTransitionStart
 				sched, err := schedule.Weekly(ws.AutostartSchedule.String)
@@ -111,7 +112,7 @@ func (e *Executor) runOnce(t time.Time) error {
 				}
 				// Round down to the nearest minute, as this is the finest granularity cron supports.
 				// Truncate is probably not necessary here, but doing it anyway to be sure.
-				nextTransitionAt = sched.Next(priorHistory.CreatedAt).Truncate(time.Minute)
+				nextTransition = sched.Next(priorHistory.CreatedAt).Truncate(time.Minute)
 			default:
 				e.log.Debug(e.ctx, "last transition not valid for autostart or autostop",
 					slog.F("workspace_id", ws.ID),
@@ -120,10 +121,10 @@ func (e *Executor) runOnce(t time.Time) error {
 				continue
 			}
 
-			if currentTick.Before(nextTransitionAt) {
+			if currentTick.Before(nextTransition) {
 				e.log.Debug(e.ctx, "skipping workspace: too early",
 					slog.F("workspace_id", ws.ID),
-					slog.F("next_transition_at", nextTransitionAt),
+					slog.F("next_transition_at", nextTransition),
 					slog.F("transition", validTransition),
 					slog.F("current_tick", currentTick),
 				)

diff --git a/coderd/autobuild/executor/lifecycle_executor_test.go b/coderd/autobuild/executor/lifecycle_executor_test.go
@@ -208,13 +208,13 @@ func TestExecutorAutostopOK(t *testing.T) {
 		TTL: &ttl,
 	}))
 
-	// When: the autobuild executor ticks
+	// When: the autobuild executor ticks *after* the TTL:
 	go func() {
 		tickCh <- time.Now().UTC().Add(ttl + time.Minute)
 		close(tickCh)
 	}()
 
-	// Then: the workspace should be started
+	// Then: the workspace should be stopped
 	<-time.After(5 * time.Second)
 	ws := mustWorkspace(t, client, workspace.ID)
 	require.NotEqual(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected a workspace build to occur")
@@ -333,7 +333,7 @@ func TestExecutorWorkspaceDeleted(t *testing.T) {
 	require.Equal(t, codersdk.WorkspaceTransitionDelete, ws.LatestBuild.Transition, "expected workspace to be deleted")
 }
 
-func TestExecutorWorkspaceTooEarly(t *testing.T) {
+func TestExecutorWorkspaceAutostartTooEarly(t *testing.T) {
 	t.Parallel()
 
 	var (
@@ -372,6 +372,41 @@ func TestExecutorWorkspaceTooEarly(t *testing.T) {
 	require.Equal(t, codersdk.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace to be running")
 }
 
+func TestExecutorWorkspaceTTLTooEarly(t *testing.T) {
+	t.Parallel()
+
+	var (
+		ctx    = context.Background()
+		tickCh = make(chan time.Time)
+		client = coderdtest.New(t, &coderdtest.Options{
+			AutobuildTicker: tickCh,
+		})
+		// Given: we have a user with a workspace
+		workspace = mustProvisionWorkspace(t, client)
+		ttl       = time.Hour
+	)
+
+	// Given: the workspace initially has TTL unset
+	require.Nil(t, workspace.TTL)
+
+	// When: we set the TTL to some time in the distant future
+	require.NoError(t, client.UpdateWorkspaceTTL(ctx, workspace.ID, codersdk.UpdateWorkspaceTTLRequest{
+		TTL: &ttl,
+	}))
+
+	// When: the autobuild executor ticks
+	go func() {
+		tickCh <- time.Now().UTC()
+		close(tickCh)
+	}()
+
+	// Then: nothing should happen
+	<-time.After(5 * time.Second)
+	ws := mustWorkspace(t, client, workspace.ID)
+	require.Equal(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected no further workspace builds to occur")
+	require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected workspace to be running")
+}
+
 func TestExecutorAutostartMultipleOK(t *testing.T) {
 	if os.Getenv("DB") == "" {
 		t.Skip(`This test only really works when using a "real" database, similar to a HA setup`)

diff --git a/coderd/workspaces.go b/coderd/workspaces.go
@@ -560,8 +560,9 @@ func (api *api) putWorkspaceTTL(rw http.ResponseWriter, r *http.Request) {
 	}
 
 	var dbTTL sql.NullInt64
-	if req.TTL != nil {
-		dbTTL.Int64 = int64(*req.TTL)
+	if req.TTL != nil && *req.TTL > 0 {
+		truncated := req.TTL.Truncate(time.Minute)
+		dbTTL.Int64 = int64(truncated)
 		dbTTL.Valid = true
 	}