coder · Emyrk · Nov 15, 2023 · Nov 14, 2023 · Nov 14, 2023 · Nov 15, 2023
diff --git a/coderd/activitybump.go b/coderd/activitybump.go
@@ -12,13 +12,39 @@ import (
 )
 
 // activityBumpWorkspace automatically bumps the workspace's auto-off timer
-// if it is set to expire soon.
-func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Store, workspaceID uuid.UUID) {
+// if it is set to expire soon. The deadline will be bumped by 1 hour*.
+// If the bump crosses over an autostart time, the workspace will be
+// bumped by the workspace ttl instead.
+//
+// If nextAutostart is the zero value or in the past, the workspace
+// will be bumped by 1 hour.
+// It handles the edge case in the example:
+//  1. Autostart is set to 9am.
+//  2. User works all day, and leaves a terminal open to the workspace overnight.
+//  3. The open terminal continually bumps the workspace deadline.
+//  4. 9am the next day, the activity bump pushes to 10am.
+//  5. If the user goes inactive for 1 hour during the day, the workspace will
+//     now stop, because it has been extended by 1 hour durations. Despite the TTL
+//     being set to 8hrs from the autostart time.
+//
+// So the issue is that when the workspace is bumped across an autostart
+// deadline, we should treat the workspace as being "started" again and
+// extend the deadline by the autostart time + workspace ttl instead.
+//
+// The issue still remains with build_max_deadline. We need to respect the original
+// maximum deadline, so that will need to be handled separately.
+// A way to avoid this is to configure the max deadline to something that will not
+// span more than 1 day. This will force the workspace to restart and reset the deadline
+// each morning when it autostarts.
+func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Store, workspaceID uuid.UUID, nextAutostart time.Time) {
 	// We set a short timeout so if the app is under load, these
 	// low priority operations fail first.
 	ctx, cancel := context.WithTimeout(ctx, time.Second*15)
 	defer cancel()
-	if err := db.ActivityBumpWorkspace(ctx, workspaceID); err != nil {
+	if err := db.ActivityBumpWorkspace(ctx, database.ActivityBumpWorkspaceParams{
+		NextAutostart: nextAutostart.UTC(),
+		WorkspaceID:   workspaceID,
+	}); err != nil {
 		if !xerrors.Is(err, context.Canceled) && !database.IsQueryCanceledError(err) {
 			// Bump will fail if the context is canceled, but this is ok.
 			log.Error(ctx, "bump failed", slog.Error(err),

diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go
@@ -42,6 +42,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
 		templateTTL                   time.Duration
 		templateDisallowsUserAutostop bool
 		expectedBump                  time.Duration
+		nextAutostart                 time.Time
 	}{
 		{
 			name:                "NotFinishedYet",
@@ -66,22 +67,41 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
 			workspaceTTL:        8 * time.Hour,
 			expectedBump:        0,
 		},
+		{
+			// Expected bump is 0 because the original deadline is more than 1 hour
+			// out, so a bump would decrease the deadline.
+			name:                "BumpLessThanDeadline",
+			transition:          database.WorkspaceTransitionStart,
+			jobCompletedAt:      sql.NullTime{Valid: true, Time: dbtime.Now().Add(-30 * time.Minute)},
+			buildDeadlineOffset: ptr.Ref(8*time.Hour - 30*time.Minute),
+			workspaceTTL:        8 * time.Hour,
+			expectedBump:        0,
+		},
 		{
 			name:                "TimeToBump",
 			transition:          database.WorkspaceTransitionStart,
-			jobCompletedAt:      sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
-			buildDeadlineOffset: ptr.Ref(8*time.Hour - 24*time.Minute),
+			jobCompletedAt:      sql.NullTime{Valid: true, Time: dbtime.Now().Add(-30 * time.Minute)},
+			buildDeadlineOffset: ptr.Ref(-30 * time.Minute),
+			workspaceTTL:        8 * time.Hour,
+			expectedBump:        time.Hour,
+		},
+		{
+			name:                "TimeToBumpNextAutostart",
+			transition:          database.WorkspaceTransitionStart,
+			jobCompletedAt:      sql.NullTime{Valid: true, Time: dbtime.Now().Add(-30 * time.Minute)},
+			buildDeadlineOffset: ptr.Ref(-30 * time.Minute),
 			workspaceTTL:        8 * time.Hour,
-			expectedBump:        8 * time.Hour,
+			expectedBump:        8*time.Hour + 30*time.Minute,
+			nextAutostart:       time.Now().Add(time.Minute * 30),
 		},
 		{
 			name:                "MaxDeadline",
 			transition:          database.WorkspaceTransitionStart,
 			jobCompletedAt:      sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
 			buildDeadlineOffset: ptr.Ref(time.Minute), // last chance to bump!
-			maxDeadlineOffset:   ptr.Ref(time.Hour),
+			maxDeadlineOffset:   ptr.Ref(time.Minute * 30),
 			workspaceTTL:        8 * time.Hour,
-			expectedBump:        1 * time.Hour,
+			expectedBump:        time.Minute * 30,
 		},
 		{
 			// A workspace that is still running, has passed its deadline, but has not
@@ -91,7 +111,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
 			jobCompletedAt:      sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
 			buildDeadlineOffset: ptr.Ref(-time.Minute),
 			workspaceTTL:        8 * time.Hour,
-			expectedBump:        8 * time.Hour,
+			expectedBump:        time.Hour,
 		},
 		{
 			// A stopped workspace should never bump.
@@ -106,12 +126,13 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
 			// by the template TTL instead.
 			name:                          "TemplateDisallowsUserAutostop",
 			transition:                    database.WorkspaceTransitionStart,
-			jobCompletedAt:                sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
-			buildDeadlineOffset:           ptr.Ref(8*time.Hour - 24*time.Minute),
-			workspaceTTL:                  6 * time.Hour,
-			templateTTL:                   8 * time.Hour,
+			jobCompletedAt:                sql.NullTime{Valid: true, Time: dbtime.Now().Add(-7 * time.Hour)},
+			buildDeadlineOffset:           ptr.Ref(-30 * time.Minute),
+			workspaceTTL:                  2 * time.Hour,
+			templateTTL:                   10 * time.Hour,
 			templateDisallowsUserAutostop: true,
-			expectedBump:                  8 * time.Hour,
+			expectedBump:                  10*time.Hour + (time.Minute * 30),
+			nextAutostart:                 time.Now().Add(time.Minute * 30),
 		},
 	} {
 		tt := tt
@@ -215,7 +236,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
 
 				// Bump duration is measured from the time of the bump, so we measure from here.
 				start := dbtime.Now()
-				activityBumpWorkspace(ctx, log, db, bld.WorkspaceID)
+				activityBumpWorkspace(ctx, log, db, bld.WorkspaceID, tt.nextAutostart)
 				end := dbtime.Now()
 
 				// Validate our state after bump
@@ -233,9 +254,9 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
 					return
 				}
 
-				// Assert that the bump occurred between start and end.
-				expectedDeadlineStart := start.Add(tt.expectedBump)
-				expectedDeadlineEnd := end.Add(tt.expectedBump)
+				// Assert that the bump occurred between start and end. 1min buffer on either side.
+				expectedDeadlineStart := start.Add(tt.expectedBump).Add(time.Minute * -1)
+				expectedDeadlineEnd := end.Add(tt.expectedBump).Add(time.Minute)
 				require.GreaterOrEqual(t, updatedBuild.Deadline, expectedDeadlineStart, "new deadline should be greater than or equal to start")
 				require.LessOrEqual(t, updatedBuild.Deadline, expectedDeadlineEnd, "new deadline should be lesser than or equal to end")
 			})

diff --git a/coderd/activitybump_test.go b/coderd/activitybump_test.go
@@ -30,7 +30,7 @@ func TestWorkspaceActivityBump(t *testing.T) {
 	// max_deadline on the build directly in the database.
 	setupActivityTest := func(t *testing.T, deadline ...time.Duration) (client *codersdk.Client, workspace codersdk.Workspace, assertBumped func(want bool)) {
 		t.Helper()
-		const ttl = time.Minute
+		const ttl = time.Hour
 		maxTTL := time.Duration(0)
 		if len(deadline) > 0 {
 			maxTTL = deadline[0]
@@ -71,28 +71,29 @@ func TestWorkspaceActivityBump(t *testing.T) {
 		})
 		coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
 
+		var maxDeadline time.Time
 		// Update the max deadline.
 		if maxTTL != 0 {
-			dbBuild, err := db.GetWorkspaceBuildByID(ctx, workspace.LatestBuild.ID)
-			require.NoError(t, err)
-
-			err = db.UpdateWorkspaceBuildDeadlineByID(ctx, database.UpdateWorkspaceBuildDeadlineByIDParams{
-				ID:          workspace.LatestBuild.ID,
-				UpdatedAt:   dbtime.Now(),
-				Deadline:    dbBuild.Deadline,
-				MaxDeadline: dbtime.Now().Add(maxTTL),
-			})
-			require.NoError(t, err)
+			maxDeadline = dbtime.Now().Add(maxTTL)
 		}
 
+		err := db.UpdateWorkspaceBuildDeadlineByID(ctx, database.UpdateWorkspaceBuildDeadlineByIDParams{
+			ID:        workspace.LatestBuild.ID,
+			UpdatedAt: dbtime.Now(),
+			// Make the deadline really close so it needs to be bumped immediately.
+			Deadline:    dbtime.Now().Add(time.Minute),
+			MaxDeadline: maxDeadline,
+		})
+		require.NoError(t, err)
+
 		_ = agenttest.New(t, client.URL, agentToken)
 		coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID)
 
-		// Sanity-check that deadline is near.
-		workspace, err := client.Workspace(ctx, workspace.ID)
+		// Sanity-check that deadline is nearing requiring a bump.
+		workspace, err = client.Workspace(ctx, workspace.ID)
 		require.NoError(t, err)
 		require.WithinDuration(t,
-			time.Now().Add(time.Duration(ttlMillis)*time.Millisecond),
+			time.Now().Add(time.Minute),
 			workspace.LatestBuild.Deadline.Time,
 			testutil.WaitMedium,
 		)
@@ -133,7 +134,7 @@ func TestWorkspaceActivityBump(t *testing.T) {
 					workspace, err = client.Workspace(ctx, workspace.ID)
 					require.NoError(t, err)
 					updatedAfter = dbtime.Now()
-					if workspace.LatestBuild.Deadline.Time == firstDeadline {
+					if workspace.LatestBuild.Deadline.Time.Equal(firstDeadline) {
 						updatedAfter = time.Now()
 						return false
 					}
@@ -151,6 +152,13 @@ func TestWorkspaceActivityBump(t *testing.T) {
 				require.LessOrEqual(t, workspace.LatestBuild.Deadline.Time, workspace.LatestBuild.MaxDeadline.Time)
 				return
 			}
+			now := dbtime.Now()
+			zone, offset := time.Now().Zone()
+			t.Logf("[Zone=%s %d] originDeadline: %s, deadline: %s, now %s, (now-deadline)=%s",
+				zone, offset,
+				firstDeadline, workspace.LatestBuild.Deadline.Time, now,
+				now.Sub(workspace.LatestBuild.Deadline.Time),
+			)
 			require.WithinDuration(t, dbtime.Now().Add(ttl), workspace.LatestBuild.Deadline.Time, testutil.WaitShort)
 		}
 	}
@@ -192,9 +200,9 @@ func TestWorkspaceActivityBump(t *testing.T) {
 	t.Run("NotExceedMaxDeadline", func(t *testing.T) {
 		t.Parallel()
 
-		// Set the max deadline to be in 61 seconds. We bump by 1 minute, so we
+		// Set the max deadline to be in 30min. We bump by 1 hour, so we
 		// should expect the deadline to match the max deadline exactly.
-		client, workspace, assertBumped := setupActivityTest(t, 61*time.Second)
+		client, workspace, assertBumped := setupActivityTest(t, time.Minute*30)
 
 		// Bump by dialing the workspace and sending traffic.
 		resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID)

diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go
@@ -357,26 +357,36 @@ func isEligibleForAutostart(ws database.Workspace, build database.WorkspaceBuild
 		return false
 	}
 
-	sched, err := cron.Weekly(ws.AutostartSchedule.String)
-	if err != nil {
+	nextTransition, allowed := NextAutostartSchedule(build.CreatedAt, ws.AutostartSchedule.String, templateSchedule)
+	if !allowed {
 		return false
 	}
+
+	// Must use '.Before' vs '.After' so equal times are considered "valid for autostart".
+	return !currentTick.Before(nextTransition)
+}
+
+// NextAutostartSchedule takes the workspace and template schedule and returns the next autostart schedule
+// after "at". The boolean returned is if the autostart should be allowed to start based on the template
+// schedule.
+func NextAutostartSchedule(at time.Time, wsSchedule string, templateSchedule schedule.TemplateScheduleOptions) (time.Time, bool) {
+	sched, err := cron.Weekly(wsSchedule)
+	if err != nil {
+		return time.Time{}, false
+	}
+
 	// 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.
-	nextTransition := sched.Next(build.CreatedAt).Truncate(time.Minute)
+	nextTransition := sched.Next(at).Truncate(time.Minute)
 
 	// The nextTransition is when the auto start should kick off. If it lands on a
 	// forbidden day, do not allow the auto start. We use the time location of the
 	// schedule to determine the weekday. So if "Saturday" is disallowed, the
 	// definition of "Saturday" depends on the location of the schedule.
 	zonedTransition := nextTransition.In(sched.Location())
 	allowed := templateSchedule.AutostartRequirement.DaysMap()[zonedTransition.Weekday()]
-	if !allowed {
-		return false
-	}
 
-	// Must used '.Before' vs '.After' so equal times are considered "valid for autostart".
-	return !currentTick.Before(nextTransition)
+	return zonedTransition, allowed
 }
 
 // isEligibleForAutostart returns true if the workspace should be autostopped.

diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -660,9 +660,9 @@ func (q *querier) AcquireProvisionerJob(ctx context.Context, arg database.Acquir
 	return q.db.AcquireProvisionerJob(ctx, arg)
 }
 
-func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) error {
-	fetch := func(ctx context.Context, arg uuid.UUID) (database.Workspace, error) {
-		return q.db.GetWorkspaceByID(ctx, arg)
+func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error {
+	fetch := func(ctx context.Context, arg database.ActivityBumpWorkspaceParams) (database.Workspace, error) {
+		return q.db.GetWorkspaceByID(ctx, arg.WorkspaceID)
 	}
 	return update(q.log, q.auth, fetch, q.db.ActivityBumpWorkspace)(ctx, arg)
 }

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -678,6 +678,13 @@ func (q *FakeQuerier) GetActiveDBCryptKeys(_ context.Context) ([]database.DBCryp
 	return ks, nil
 }
 
+func maxTime(t, u time.Time) time.Time {
+	if t.After(u) {
+		return t
+	}
+	return u
+}
+
 func minTime(t, u time.Time) time.Time {
 	if t.Before(u) {
 		return t
@@ -775,20 +782,20 @@ func (q *FakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu
 	return database.ProvisionerJob{}, sql.ErrNoRows
 }
 
-func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uuid.UUID) error {
-	err := validateDatabaseType(workspaceID)
+func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error {
+	err := validateDatabaseType(arg)
 	if err != nil {
 		return err
 	}
 
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
 
-	workspace, err := q.getWorkspaceByIDNoLock(ctx, workspaceID)
+	workspace, err := q.getWorkspaceByIDNoLock(ctx, arg.WorkspaceID)
 	if err != nil {
 		return err
 	}
-	latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, workspaceID)
+	latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, arg.WorkspaceID)
 	if err != nil {
 		return err
 	}
@@ -822,15 +829,17 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uui
 		}
 
 		var ttlDur time.Duration
-		if workspace.Ttl.Valid {
-			ttlDur = time.Duration(workspace.Ttl.Int64)
-		}
-		if !template.AllowUserAutostop {
-			ttlDur = time.Duration(template.DefaultTTL)
-		}
-		if ttlDur <= 0 {
-			// There's no TTL set anymore, so we don't know the bump duration.
-			return nil
+		if now.Add(time.Hour).After(arg.NextAutostart) && arg.NextAutostart.After(now) {
+			// Extend to TTL
+			add := arg.NextAutostart.Sub(now)
+			if workspace.Ttl.Valid && template.AllowUserAutostop {
+				add += time.Duration(workspace.Ttl.Int64)
+			} else {
+				add += time.Duration(template.DefaultTTL)
+			}
+			ttlDur = add
+		} else {
+			ttlDur = time.Hour
 		}
 
 		// Only bump if 5% of the deadline has passed.
@@ -842,6 +851,8 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uui
 
 		// Bump.
 		newDeadline := now.Add(ttlDur)
+		// Never decrease deadlines from a bump
+		newDeadline = maxTime(newDeadline, q.workspaceBuilds[i].Deadline)
 		q.workspaceBuilds[i].UpdatedAt = now
 		if !q.workspaceBuilds[i].MaxDeadline.IsZero() {
 			q.workspaceBuilds[i].Deadline = minTime(newDeadline, q.workspaceBuilds[i].MaxDeadline)

diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go
diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go