coder · johnstcn · Sep 15, 2023 · Sep 13, 2023 · Sep 14, 2023 · Sep 14, 2023
diff --git a/cli/clitest/golden.go b/cli/clitest/golden.go
@@ -28,7 +28,7 @@ import (
 // make update-golden-files
 var UpdateGoldenFiles = flag.Bool("update", false, "update .golden files")
 
-var timestampRegex = regexp.MustCompile(`(?i)\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(.\d+)?Z`)
+var timestampRegex = regexp.MustCompile(`(?i)\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(.\d+)?(Z|[+-]\d+:\d+)`)
 
 type CommandHelpCase struct {
 	Name string

diff --git a/coderd/activitybump_internal_test.go b/coderd/activitybump_internal_test.go
@@ -2,7 +2,6 @@ package coderd
 
 import (
 	"database/sql"
-	"runtime"
 	"testing"
 	"time"
 
@@ -22,6 +21,16 @@ import (
 func Test_ActivityBumpWorkspace(t *testing.T) {
 	t.Parallel()
 
+	// We test the below in multiple timezones specifically
+	// chosen to trigger timezone-related bugs.
+	timezones := []string{
+		"Asia/Kolkata",        // No DST, positive fractional offset
+		"Canada/Newfoundland", // DST, negative fractional offset
+		"Europe/Paris",        // DST, positive offset
+		"US/Arizona",          // No DST, negative offset
+		"UTC",                 // Baseline
+	}
+
 	for _, tt := range []struct {
 		name                string
 		transition          database.WorkspaceTransition
@@ -92,117 +101,124 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
 		},
 	} {
 		tt := tt
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			var (
-				now   = dbtime.Now()
-				ctx   = testutil.Context(t, testutil.WaitShort)
-				log   = slogtest.Make(t, nil)
-				db, _ = dbtestutil.NewDB(t)
-				org   = dbgen.Organization(t, db, database.Organization{})
-				user  = dbgen.User(t, db, database.User{
-					Status: database.UserStatusActive,
-				})
-				_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
-					UserID:         user.ID,
-					OrganizationID: org.ID,
-				})
-				templateVersion = dbgen.TemplateVersion(t, db, database.TemplateVersion{
-					OrganizationID: org.ID,
-					CreatedBy:      user.ID,
-				})
-				template = dbgen.Template(t, db, database.Template{
-					OrganizationID:  org.ID,
-					ActiveVersionID: templateVersion.ID,
-					CreatedBy:       user.ID,
-				})
-				ws = dbgen.Workspace(t, db, database.Workspace{
-					OwnerID:        user.ID,
-					OrganizationID: org.ID,
-					TemplateID:     template.ID,
-					Ttl:            sql.NullInt64{Valid: true, Int64: int64(tt.workspaceTTL)},
-				})
-				job = dbgen.ProvisionerJob(t, db, database.ProvisionerJob{
-					OrganizationID: org.ID,
-					CompletedAt:    tt.jobCompletedAt,
+		for _, tz := range timezones {
+			tz := tz
+			t.Run(tt.name+"/"+tz, func(t *testing.T) {
+				t.Parallel()
+
+				var (
+					now   = dbtime.Now()
+					ctx   = testutil.Context(t, testutil.WaitShort)
+					log   = slogtest.Make(t, nil)
+					db, _ = dbtestutil.NewDB(t, dbtestutil.WithTimezone(tz))
+					org   = dbgen.Organization(t, db, database.Organization{})
+					user  = dbgen.User(t, db, database.User{
+						Status: database.UserStatusActive,
+					})
+					_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
+						UserID:         user.ID,
+						OrganizationID: org.ID,
+					})
+					templateVersion = dbgen.TemplateVersion(t, db, database.TemplateVersion{
+						OrganizationID: org.ID,
+						CreatedBy:      user.ID,
+					})
+					template = dbgen.Template(t, db, database.Template{
+						OrganizationID:  org.ID,
+						ActiveVersionID: templateVersion.ID,
+						CreatedBy:       user.ID,
+					})
+					ws = dbgen.Workspace(t, db, database.Workspace{
+						OwnerID:        user.ID,
+						OrganizationID: org.ID,
+						TemplateID:     template.ID,
+						Ttl:            sql.NullInt64{Valid: true, Int64: int64(tt.workspaceTTL)},
+					})
+					job = dbgen.ProvisionerJob(t, db, database.ProvisionerJob{
+						OrganizationID: org.ID,
+						CompletedAt:    tt.jobCompletedAt,
+					})
+					_ = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
+						JobID: job.ID,
+					})
+					buildID = uuid.New()
+				)
+
+				var buildNumber int32 = 1
+				// Insert a number of previous workspace builds.
+				for i := 0; i < 5; i++ {
+					insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStart, buildNumber)
+					buildNumber++
+					insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStop, buildNumber)
+					buildNumber++
+				}
+
+				// dbgen.WorkspaceBuild automatically sets deadline to now+1 hour if not set
+				var buildDeadline time.Time
+				if tt.buildDeadlineOffset != nil {
+					buildDeadline = now.Add(*tt.buildDeadlineOffset)
+				}
+				var maxDeadline time.Time
+				if tt.maxDeadlineOffset != nil {
+					maxDeadline = now.Add(*tt.maxDeadlineOffset)
+				}
+				err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{
+					ID:                buildID,
+					CreatedAt:         dbtime.Now(),
+					UpdatedAt:         dbtime.Now(),
+					BuildNumber:       buildNumber,
+					InitiatorID:       user.ID,
+					Reason:            database.BuildReasonInitiator,
+					WorkspaceID:       ws.ID,
+					JobID:             job.ID,
+					TemplateVersionID: templateVersion.ID,
+					Transition:        tt.transition,
+					Deadline:          buildDeadline,
+					MaxDeadline:       maxDeadline,
 				})
-				_ = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
-					JobID: job.ID,
-				})
-				buildID = uuid.New()
-			)
-
-			var buildNumber int32 = 1
-			// Insert a number of previous workspace builds.
-			for i := 0; i < 5; i++ {
-				insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStart, buildNumber)
-				buildNumber++
-				insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStop, buildNumber)
-				buildNumber++
-			}
-
-			// dbgen.WorkspaceBuild automatically sets deadline to now+1 hour if not set
-			var buildDeadline time.Time
-			if tt.buildDeadlineOffset != nil {
-				buildDeadline = now.Add(*tt.buildDeadlineOffset)
-			}
-			var maxDeadline time.Time
-			if tt.maxDeadlineOffset != nil {
-				maxDeadline = now.Add(*tt.maxDeadlineOffset)
-			}
-			err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{
-				ID:                buildID,
-				CreatedAt:         dbtime.Now(),
-				UpdatedAt:         dbtime.Now(),
-				BuildNumber:       buildNumber,
-				InitiatorID:       user.ID,
-				Reason:            database.BuildReasonInitiator,
-				WorkspaceID:       ws.ID,
-				JobID:             job.ID,
-				TemplateVersionID: templateVersion.ID,
-				Transition:        tt.transition,
-				Deadline:          buildDeadline,
-				MaxDeadline:       maxDeadline,
-			})
-			require.NoError(t, err, "unexpected error inserting workspace build")
-			bld, err := db.GetWorkspaceBuildByID(ctx, buildID)
-			require.NoError(t, err, "unexpected error fetching inserted workspace build")
-
-			// Validate our initial state before bump
-			require.Equal(t, tt.transition, bld.Transition, "unexpected transition before bump")
-			require.Equal(t, tt.jobCompletedAt.Time.UTC(), job.CompletedAt.Time.UTC(), "unexpected job completed at before bump")
-			require.Equal(t, buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump")
-			require.Equal(t, maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump")
-			require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump")
-
-			workaroundWindowsTimeResolution(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)
-			elapsed := time.Since(start)
-			if elapsed > 15*time.Second {
-				t.Logf("warning: activityBumpWorkspace took longer than 15 seconds: %s", elapsed)
-			}
-			// The actual bump could have happened anywhere in the elapsed time, so we
-			// guess at the approximate time of the bump.
-			approxBumpTime := start.Add(elapsed / 2)
-
-			// Validate our state after bump
-			updatedBuild, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, bld.WorkspaceID)
-			require.NoError(t, err, "unexpected error getting latest workspace build")
-			if tt.expectedBump == 0 {
-				require.Equal(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should not have bumped updated_at")
-				require.Equal(t, bld.Deadline.UTC(), updatedBuild.Deadline.UTC(), "should not have bumped deadline")
-			} else {
+				require.NoError(t, err, "unexpected error inserting workspace build")
+				bld, err := db.GetWorkspaceBuildByID(ctx, buildID)
+				require.NoError(t, err, "unexpected error fetching inserted workspace build")
+
+				// Validate our initial state before bump
+				require.Equal(t, tt.transition, bld.Transition, "unexpected transition before bump")
+				require.Equal(t, tt.jobCompletedAt.Time.UTC(), job.CompletedAt.Time.UTC(), "unexpected job completed at before bump")
+				require.Equal(t, buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump")
+				require.Equal(t, maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump")
+				require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump")
+
+				// Wait a bit before bumping as dbtime is rounded to the nearest millisecond.
+				// This should also hopefully be enough for Windows time resolution to register
+				// a tick (win32 max timer resolution is apparently between 0.5 and 15.6ms)
+				<-time.After(testutil.IntervalFast)
+
+				// Bump duration is measured from the time of the bump, so we measure from here.
+				start := dbtime.Now()
+				activityBumpWorkspace(ctx, log, db, bld.WorkspaceID)
+				end := dbtime.Now()
+
+				// Validate our state after bump
+				updatedBuild, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, bld.WorkspaceID)
+				require.NoError(t, err, "unexpected error getting latest workspace build")
+				require.Equal(t, bld.MaxDeadline.UTC(), updatedBuild.MaxDeadline.UTC(), "max_deadline should not have changed")
+				if tt.expectedBump == 0 {
+					require.Equal(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should not have bumped updated_at")
+					require.Equal(t, bld.Deadline.UTC(), updatedBuild.Deadline.UTC(), "should not have bumped deadline")
+					return
+				}
 				require.NotEqual(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should have bumped updated_at")
-				expectedDeadline := approxBumpTime.Add(tt.expectedBump).UTC()
-				// Note: if CI is especially slow, this test may fail. There is an internal 15-second
-				// deadline in activityBumpWorkspace, so we allow the same window here.
-				require.WithinDuration(t, expectedDeadline, updatedBuild.Deadline.UTC(), 15*time.Second, "unexpected deadline after bump")
-			}
-		})
+				if tt.maxDeadlineOffset != nil {
+					require.Equal(t, bld.MaxDeadline.UTC(), updatedBuild.MaxDeadline.UTC(), "new deadline must equal original max deadline")
+					return
+				}
+
+				// Assert that the bump occurred between start and end.
+				expectedDeadlineStart := start.Add(tt.expectedBump)
+				expectedDeadlineEnd := end.Add(tt.expectedBump)
+				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")
+			})
+		}
 	}
 }
 
@@ -223,11 +239,3 @@ func insertPrevWorkspaceBuild(t *testing.T, db database.Store, orgID, tvID, work
 		Transition:        transition,
 	})
 }
-
-func workaroundWindowsTimeResolution(t *testing.T) {
-	t.Helper()
-	if runtime.GOOS == "windows" {
-		t.Logf("workaround: sleeping for a short time to avoid time resolution issues on Windows")
-		<-time.After(testutil.IntervalSlow)
-	}
-}
diff --git a/coderd/activitybump_test.go b/coderd/activitybump_test.go
@@ -30,6 +30,7 @@ func TestWorkspaceActivityBump(t *testing.T) {
 	// doesn't use template autostop requirements and instead edits the
 	// 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
 		maxTTL := time.Duration(0)
 		if len(deadline) > 0 {
@@ -120,6 +121,7 @@ func TestWorkspaceActivityBump(t *testing.T) {
 		_ = coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID)
 
 		return client, workspace, func(want bool) {
+			t.Helper()
 			if !want {
 				// It is difficult to test the absence of a call in a non-racey
 				// way. In general, it is difficult for the API to generate
@@ -134,24 +136,32 @@ func TestWorkspaceActivityBump(t *testing.T) {
 				return
 			}
 
+			var updatedAfter time.Time
 			// The Deadline bump occurs asynchronously.
 			require.Eventuallyf(t,
 				func() bool {
 					workspace, err = client.Workspace(ctx, workspace.ID)
 					require.NoError(t, err)
-					return workspace.LatestBuild.Deadline.Time != firstDeadline
+					updatedAfter = dbtime.Now()
+					if workspace.LatestBuild.Deadline.Time == firstDeadline {
+						updatedAfter = time.Now()
+						return false
+					}
+					return true
 				},
 				testutil.WaitLong, testutil.IntervalFast,
 				"deadline %v never updated", firstDeadline,
 			)
 
+			require.Greater(t, workspace.LatestBuild.Deadline.Time, updatedAfter)
+
 			// If the workspace has a max deadline, the deadline must not exceed
 			// it.
-			if maxTTL != 0 && dbtime.Now().Add(ttl).After(workspace.LatestBuild.MaxDeadline.Time) {
-				require.Equal(t, workspace.LatestBuild.Deadline.Time, workspace.LatestBuild.MaxDeadline.Time)
+			if workspace.LatestBuild.MaxDeadline.Valid {
+				require.LessOrEqual(t, workspace.LatestBuild.Deadline.Time, workspace.LatestBuild.MaxDeadline.Time)
 				return
 			}
-			require.WithinDuration(t, dbtime.Now().Add(ttl), workspace.LatestBuild.Deadline.Time, 3*time.Second)
+			require.WithinDuration(t, dbtime.Now().Add(ttl), workspace.LatestBuild.Deadline.Time, testutil.WaitShort)
 		}
 	}
 
@@ -210,12 +220,6 @@ func TestWorkspaceActivityBump(t *testing.T) {
 		require.NoError(t, err)
 		_ = sshConn.Close()
 
-		assertBumped(true)
-
-		// Double check that the workspace build's deadline is equal to the
-		// max deadline.
-		workspace, err = client.Workspace(ctx, workspace.ID)
-		require.NoError(t, err)
-		require.Equal(t, workspace.LatestBuild.Deadline.Time, workspace.LatestBuild.MaxDeadline.Time)
+		assertBumped(true) // also asserts max ttl not exceeded
 	})
 }