Skip to content

Commit 290180b

Browse files
EmyrkEmyrk
authored
feat!: bump workspace activity by 1 hour (#10704)
Marked as a breaking change as the previous activity bump was always the TTL duration of the workspace/template. This change is more cost conservative, only bumping by 1 hour for workspace activity. To accommodate wrap around, eg bumping a workspace into the next autostart, the deadline is bumped by the TTL if the workspace crosses the autostart threshold. This is a niche case that is likely caused by an idle terminal making a workspace survive through a night. The next morning, the workspace will get activity bumped the default TTL on the autostart, being similar to as if the workspace was autostarted again. In practice, a good way to avoid this is to set a max_deadline of <24hrs to avoid wrap around entirely.
1 parent 6085b92 commit 290180b

12 files changed

+235
-88
lines changed

‎coderd/activitybump.go

+29-3
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,39 @@ import (
1212
)
1313

1414
// activityBumpWorkspace automatically bumps the workspace's auto-off timer
15-
// if it is set to expire soon.
16-
func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Store, workspaceID uuid.UUID) {
15+
// if it is set to expire soon. The deadline will be bumped by 1 hour*.
16+
// If the bump crosses over an autostart time, the workspace will be
17+
// bumped by the workspace ttl instead.
18+
//
19+
// If nextAutostart is the zero value or in the past, the workspace
20+
// will be bumped by 1 hour.
21+
// It handles the edge case in the example:
22+
// 1. Autostart is set to 9am.
23+
// 2. User works all day, and leaves a terminal open to the workspace overnight.
24+
// 3. The open terminal continually bumps the workspace deadline.
25+
// 4. 9am the next day, the activity bump pushes to 10am.
26+
// 5. If the user goes inactive for 1 hour during the day, the workspace will
27+
// now stop, because it has been extended by 1 hour durations. Despite the TTL
28+
// being set to 8hrs from the autostart time.
29+
//
30+
// So the issue is that when the workspace is bumped across an autostart
31+
// deadline, we should treat the workspace as being "started" again and
32+
// extend the deadline by the autostart time + workspace ttl instead.
33+
//
34+
// The issue still remains with build_max_deadline. We need to respect the original
35+
// maximum deadline, so that will need to be handled separately.
36+
// A way to avoid this is to configure the max deadline to something that will not
37+
// span more than 1 day. This will force the workspace to restart and reset the deadline
38+
// each morning when it autostarts.
39+
func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Store, workspaceID uuid.UUID, nextAutostart time.Time) {
1740
// We set a short timeout so if the app is under load, these
1841
// low priority operations fail first.
1942
ctx, cancel := context.WithTimeout(ctx, time.Second*15)
2043
defer cancel()
21-
if err := db.ActivityBumpWorkspace(ctx, workspaceID); err != nil {
44+
if err := db.ActivityBumpWorkspace(ctx, database.ActivityBumpWorkspaceParams{
45+
NextAutostart: nextAutostart.UTC(),
46+
WorkspaceID: workspaceID,
47+
}); err != nil {
2248
if !xerrors.Is(err, context.Canceled) && !database.IsQueryCanceledError(err) {
2349
// Bump will fail if the context is canceled, but this is ok.
2450
log.Error(ctx, "bump failed", slog.Error(err),

‎coderd/activitybump_internal_test.go

+36-15
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
4242
templateTTL time.Duration
4343
templateDisallowsUserAutostop bool
4444
expectedBump time.Duration
45+
nextAutostart time.Time
4546
}{
4647
{
4748
name: "NotFinishedYet",
@@ -66,22 +67,41 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
6667
workspaceTTL: 8 * time.Hour,
6768
expectedBump: 0,
6869
},
70+
{
71+
// Expected bump is 0 because the original deadline is more than 1 hour
72+
// out, so a bump would decrease the deadline.
73+
name: "BumpLessThanDeadline",
74+
transition: database.WorkspaceTransitionStart,
75+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-30 * time.Minute)},
76+
buildDeadlineOffset: ptr.Ref(8*time.Hour - 30*time.Minute),
77+
workspaceTTL: 8 * time.Hour,
78+
expectedBump: 0,
79+
},
6980
{
7081
name: "TimeToBump",
7182
transition: database.WorkspaceTransitionStart,
72-
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
73-
buildDeadlineOffset: ptr.Ref(8*time.Hour - 24*time.Minute),
83+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-30 * time.Minute)},
84+
buildDeadlineOffset: ptr.Ref(-30 * time.Minute),
85+
workspaceTTL: 8 * time.Hour,
86+
expectedBump: time.Hour,
87+
},
88+
{
89+
name: "TimeToBumpNextAutostart",
90+
transition: database.WorkspaceTransitionStart,
91+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-30 * time.Minute)},
92+
buildDeadlineOffset: ptr.Ref(-30 * time.Minute),
7493
workspaceTTL: 8 * time.Hour,
75-
expectedBump: 8 * time.Hour,
94+
expectedBump: 8*time.Hour + 30*time.Minute,
95+
nextAutostart: time.Now().Add(time.Minute * 30),
7696
},
7797
{
7898
name: "MaxDeadline",
7999
transition: database.WorkspaceTransitionStart,
80100
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
81101
buildDeadlineOffset: ptr.Ref(time.Minute), // last chance to bump!
82-
maxDeadlineOffset: ptr.Ref(time.Hour),
102+
maxDeadlineOffset: ptr.Ref(time.Minute * 30),
83103
workspaceTTL: 8 * time.Hour,
84-
expectedBump: 1 * time.Hour,
104+
expectedBump: time.Minute * 30,
85105
},
86106
{
87107
// A workspace that is still running, has passed its deadline, but has not
@@ -91,7 +111,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
91111
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
92112
buildDeadlineOffset: ptr.Ref(-time.Minute),
93113
workspaceTTL: 8 * time.Hour,
94-
expectedBump: 8 * time.Hour,
114+
expectedBump: time.Hour,
95115
},
96116
{
97117
// A stopped workspace should never bump.
@@ -106,12 +126,13 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
106126
// by the template TTL instead.
107127
name: "TemplateDisallowsUserAutostop",
108128
transition: database.WorkspaceTransitionStart,
109-
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
110-
buildDeadlineOffset: ptr.Ref(8*time.Hour - 24*time.Minute),
111-
workspaceTTL: 6 * time.Hour,
112-
templateTTL: 8 * time.Hour,
129+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-7 * time.Hour)},
130+
buildDeadlineOffset: ptr.Ref(-30 * time.Minute),
131+
workspaceTTL: 2 * time.Hour,
132+
templateTTL: 10 * time.Hour,
113133
templateDisallowsUserAutostop: true,
114-
expectedBump: 8 * time.Hour,
134+
expectedBump: 10*time.Hour + (time.Minute * 30),
135+
nextAutostart: time.Now().Add(time.Minute * 30),
115136
},
116137
} {
117138
tt := tt
@@ -215,7 +236,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
215236

216237
// Bump duration is measured from the time of the bump, so we measure from here.
217238
start := dbtime.Now()
218-
activityBumpWorkspace(ctx, log, db, bld.WorkspaceID)
239+
activityBumpWorkspace(ctx, log, db, bld.WorkspaceID, tt.nextAutostart)
219240
end := dbtime.Now()
220241

221242
// Validate our state after bump
@@ -233,9 +254,9 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
233254
return
234255
}
235256

236-
// Assert that the bump occurred between start and end.
237-
expectedDeadlineStart := start.Add(tt.expectedBump)
238-
expectedDeadlineEnd := end.Add(tt.expectedBump)
257+
// Assert that the bump occurred between start and end. 1min buffer on either side.
258+
expectedDeadlineStart := start.Add(tt.expectedBump).Add(time.Minute * -1)
259+
expectedDeadlineEnd := end.Add(tt.expectedBump).Add(time.Minute)
239260
require.GreaterOrEqual(t, updatedBuild.Deadline, expectedDeadlineStart, "new deadline should be greater than or equal to start")
240261
require.LessOrEqual(t, updatedBuild.Deadline, expectedDeadlineEnd, "new deadline should be lesser than or equal to end")
241262
})

‎coderd/activitybump_test.go

+25-17
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func TestWorkspaceActivityBump(t *testing.T) {
3030
// max_deadline on the build directly in the database.
3131
setupActivityTest := func(t *testing.T, deadline ...time.Duration) (client *codersdk.Client, workspace codersdk.Workspace, assertBumped func(want bool)) {
3232
t.Helper()
33-
const ttl = time.Minute
33+
const ttl = time.Hour
3434
maxTTL := time.Duration(0)
3535
if len(deadline) > 0 {
3636
maxTTL = deadline[0]
@@ -71,28 +71,29 @@ func TestWorkspaceActivityBump(t *testing.T) {
7171
})
7272
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
7373

74+
var maxDeadline time.Time
7475
// Update the max deadline.
7576
if maxTTL != 0 {
76-
dbBuild, err := db.GetWorkspaceBuildByID(ctx, workspace.LatestBuild.ID)
77-
require.NoError(t, err)
78-
79-
err = db.UpdateWorkspaceBuildDeadlineByID(ctx, database.UpdateWorkspaceBuildDeadlineByIDParams{
80-
ID: workspace.LatestBuild.ID,
81-
UpdatedAt: dbtime.Now(),
82-
Deadline: dbBuild.Deadline,
83-
MaxDeadline: dbtime.Now().Add(maxTTL),
84-
})
85-
require.NoError(t, err)
77+
maxDeadline = dbtime.Now().Add(maxTTL)
8678
}
8779

80+
err := db.UpdateWorkspaceBuildDeadlineByID(ctx, database.UpdateWorkspaceBuildDeadlineByIDParams{
81+
ID: workspace.LatestBuild.ID,
82+
UpdatedAt: dbtime.Now(),
83+
// Make the deadline really close so it needs to be bumped immediately.
84+
Deadline: dbtime.Now().Add(time.Minute),
85+
MaxDeadline: maxDeadline,
86+
})
87+
require.NoError(t, err)
88+
8889
_ = agenttest.New(t, client.URL, agentToken)
8990
coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID)
9091

91-
// Sanity-check that deadline is near.
92-
workspace, err := client.Workspace(ctx, workspace.ID)
92+
// Sanity-check that deadline is nearing requiring a bump.
93+
workspace, err = client.Workspace(ctx, workspace.ID)
9394
require.NoError(t, err)
9495
require.WithinDuration(t,
95-
time.Now().Add(time.Duration(ttlMillis)*time.Millisecond),
96+
time.Now().Add(time.Minute),
9697
workspace.LatestBuild.Deadline.Time,
9798
testutil.WaitMedium,
9899
)
@@ -133,7 +134,7 @@ func TestWorkspaceActivityBump(t *testing.T) {
133134
workspace, err = client.Workspace(ctx, workspace.ID)
134135
require.NoError(t, err)
135136
updatedAfter = dbtime.Now()
136-
if workspace.LatestBuild.Deadline.Time == firstDeadline {
137+
if workspace.LatestBuild.Deadline.Time.Equal(firstDeadline) {
137138
updatedAfter = time.Now()
138139
return false
139140
}
@@ -151,6 +152,13 @@ func TestWorkspaceActivityBump(t *testing.T) {
151152
require.LessOrEqual(t, workspace.LatestBuild.Deadline.Time, workspace.LatestBuild.MaxDeadline.Time)
152153
return
153154
}
155+
now := dbtime.Now()
156+
zone, offset := time.Now().Zone()
157+
t.Logf("[Zone=%s %d] originDeadline: %s, deadline: %s, now %s, (now-deadline)=%s",
158+
zone, offset,
159+
firstDeadline, workspace.LatestBuild.Deadline.Time, now,
160+
now.Sub(workspace.LatestBuild.Deadline.Time),
161+
)
154162
require.WithinDuration(t, dbtime.Now().Add(ttl), workspace.LatestBuild.Deadline.Time, testutil.WaitShort)
155163
}
156164
}
@@ -192,9 +200,9 @@ func TestWorkspaceActivityBump(t *testing.T) {
192200
t.Run("NotExceedMaxDeadline", func(t *testing.T) {
193201
t.Parallel()
194202

195-
// Set the max deadline to be in 61 seconds. We bump by 1 minute, so we
203+
// Set the max deadline to be in 30min. We bump by 1 hour, so we
196204
// should expect the deadline to match the max deadline exactly.
197-
client, workspace, assertBumped := setupActivityTest(t, 61*time.Second)
205+
client, workspace, assertBumped := setupActivityTest(t, time.Minute*30)
198206

199207
// Bump by dialing the workspace and sending traffic.
200208
resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID)

‎coderd/autobuild/lifecycle_executor.go

+18-8
Original file line numberDiff line numberDiff line change
@@ -357,26 +357,36 @@ func isEligibleForAutostart(ws database.Workspace, build database.WorkspaceBuild
357357
return false
358358
}
359359

360-
sched, err := cron.Weekly(ws.AutostartSchedule.String)
361-
if err != nil {
360+
nextTransition, allowed := NextAutostartSchedule(build.CreatedAt, ws.AutostartSchedule.String, templateSchedule)
361+
if !allowed {
362362
return false
363363
}
364+
365+
// Must use '.Before' vs '.After' so equal times are considered "valid for autostart".
366+
return !currentTick.Before(nextTransition)
367+
}
368+
369+
// NextAutostartSchedule takes the workspace and template schedule and returns the next autostart schedule
370+
// after "at". The boolean returned is if the autostart should be allowed to start based on the template
371+
// schedule.
372+
func NextAutostartSchedule(at time.Time, wsSchedule string, templateSchedule schedule.TemplateScheduleOptions) (time.Time, bool) {
373+
sched, err := cron.Weekly(wsSchedule)
374+
if err != nil {
375+
return time.Time{}, false
376+
}
377+
364378
// Round down to the nearest minute, as this is the finest granularity cron supports.
365379
// Truncate is probably not necessary here, but doing it anyway to be sure.
366-
nextTransition := sched.Next(build.CreatedAt).Truncate(time.Minute)
380+
nextTransition := sched.Next(at).Truncate(time.Minute)
367381

368382
// The nextTransition is when the auto start should kick off. If it lands on a
369383
// forbidden day, do not allow the auto start. We use the time location of the
370384
// schedule to determine the weekday. So if "Saturday" is disallowed, the
371385
// definition of "Saturday" depends on the location of the schedule.
372386
zonedTransition := nextTransition.In(sched.Location())
373387
allowed := templateSchedule.AutostartRequirement.DaysMap()[zonedTransition.Weekday()]
374-
if !allowed {
375-
return false
376-
}
377388

378-
// Must used '.Before' vs '.After' so equal times are considered "valid for autostart".
379-
return !currentTick.Before(nextTransition)
389+
return zonedTransition, allowed
380390
}
381391

382392
// isEligibleForAutostart returns true if the workspace should be autostopped.

‎coderd/database/dbauthz/dbauthz.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -660,9 +660,9 @@ func (q *querier) AcquireProvisionerJob(ctx context.Context, arg database.Acquir
660660
return q.db.AcquireProvisionerJob(ctx, arg)
661661
}
662662

663-
func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) error {
664-
fetch := func(ctx context.Context, arg uuid.UUID) (database.Workspace, error) {
665-
return q.db.GetWorkspaceByID(ctx, arg)
663+
func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error {
664+
fetch := func(ctx context.Context, arg database.ActivityBumpWorkspaceParams) (database.Workspace, error) {
665+
return q.db.GetWorkspaceByID(ctx, arg.WorkspaceID)
666666
}
667667
return update(q.log, q.auth, fetch, q.db.ActivityBumpWorkspace)(ctx, arg)
668668
}

‎coderd/database/dbmem/dbmem.go

+24-13
Original file line numberDiff line numberDiff line change
@@ -678,6 +678,13 @@ func (q *FakeQuerier) GetActiveDBCryptKeys(_ context.Context) ([]database.DBCryp
678678
return ks, nil
679679
}
680680

681+
func maxTime(t, u time.Time) time.Time {
682+
if t.After(u) {
683+
return t
684+
}
685+
return u
686+
}
687+
681688
func minTime(t, u time.Time) time.Time {
682689
if t.Before(u) {
683690
return t
@@ -775,20 +782,20 @@ func (q *FakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu
775782
return database.ProvisionerJob{}, sql.ErrNoRows
776783
}
777784

778-
func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uuid.UUID) error {
779-
err := validateDatabaseType(workspaceID)
785+
func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, arg database.ActivityBumpWorkspaceParams) error {
786+
err := validateDatabaseType(arg)
780787
if err != nil {
781788
return err
782789
}
783790

784791
q.mutex.Lock()
785792
defer q.mutex.Unlock()
786793

787-
workspace, err := q.getWorkspaceByIDNoLock(ctx, workspaceID)
794+
workspace, err := q.getWorkspaceByIDNoLock(ctx, arg.WorkspaceID)
788795
if err != nil {
789796
return err
790797
}
791-
latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, workspaceID)
798+
latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, arg.WorkspaceID)
792799
if err != nil {
793800
return err
794801
}
@@ -822,15 +829,17 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uui
822829
}
823830

824831
var ttlDur time.Duration
825-
if workspace.Ttl.Valid {
826-
ttlDur = time.Duration(workspace.Ttl.Int64)
827-
}
828-
if !template.AllowUserAutostop {
829-
ttlDur = time.Duration(template.DefaultTTL)
830-
}
831-
if ttlDur <= 0 {
832-
// There's no TTL set anymore, so we don't know the bump duration.
833-
return nil
832+
if now.Add(time.Hour).After(arg.NextAutostart) && arg.NextAutostart.After(now) {
833+
// Extend to TTL
834+
add := arg.NextAutostart.Sub(now)
835+
if workspace.Ttl.Valid && template.AllowUserAutostop {
836+
add += time.Duration(workspace.Ttl.Int64)
837+
} else {
838+
add += time.Duration(template.DefaultTTL)
839+
}
840+
ttlDur = add
841+
} else {
842+
ttlDur = time.Hour
834843
}
835844

836845
// Only bump if 5% of the deadline has passed.
@@ -842,6 +851,8 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, workspaceID uui
842851

843852
// Bump.
844853
newDeadline := now.Add(ttlDur)
854+
// Never decrease deadlines from a bump
855+
newDeadline = maxTime(newDeadline, q.workspaceBuilds[i].Deadline)
845856
q.workspaceBuilds[i].UpdatedAt = now
846857
if !q.workspaceBuilds[i].MaxDeadline.IsZero() {
847858
q.workspaceBuilds[i].Deadline = minTime(newDeadline, q.workspaceBuilds[i].MaxDeadline)

‎coderd/database/dbmetrics/dbmetrics.go

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)