feat: bump workspace deadline on user activity #4119
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| package coderd | ||
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "errors" | ||
| "time" | ||
|
|
||
| "golang.org/x/xerrors" | ||
|
|
||
| "cdr.dev/slog" | ||
| "github.com/coder/coder/coderd/database" | ||
| ) | ||
|
|
||
| // activityBumpWorkspace automatically bumps the workspace's auto-off timer | ||
| // if it is set to expire soon. | ||
| func activityBumpWorkspace(log slog.Logger, db database.Store, workspace database.Workspace) { | ||
| // We set a short timeout so if the app is under load, these | ||
| // low priority operations fail first. | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second*15) | ||
| defer cancel() | ||
|
|
||
| err := db.InTx(func(s database.Store) error { | ||
| build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil | ||
| } else if err != nil { | ||
| return xerrors.Errorf("get latest workspace build: %w", err) | ||
| } | ||
|
|
||
| job, err := s.GetProvisionerJobByID(ctx, build.JobID) | ||
| if err != nil { | ||
| return xerrors.Errorf("get provisioner job: %w", err) | ||
| } | ||
|
|
||
| if build.Transition != database.WorkspaceTransitionStart || !job.CompletedAt.Valid { | ||
| return nil | ||
| } | ||
|
|
||
| if build.Deadline.IsZero() { | ||
| // Workspace shutdown is manual | ||
| return nil | ||
| } | ||
|
|
||
| // We sent bumpThreshold slightly under bumpAmount to minimize DB writes. | ||
| const ( | ||
| bumpAmount = time.Hour | ||
| bumpThreshold = time.Hour - (time.Minute * 10) | ||
| ) | ||
ammario marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if !build.Deadline.Before(time.Now().Add(bumpThreshold)) { | ||
| return nil | ||
| } | ||
|
|
||
| newDeadline := database.Now().Add(bumpAmount) | ||
|
|
||
| if err := s.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ | ||
| ID: build.ID, | ||
| UpdatedAt: database.Now(), | ||
| ProvisionerState: build.ProvisionerState, | ||
| Deadline: newDeadline, | ||
| }); err != nil { | ||
| return xerrors.Errorf("update workspace build: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| log.Error( | ||
| ctx, "bump failed", | ||
| slog.Error(err), | ||
| slog.F("workspace_id", workspace.ID), | ||
| ) | ||
| } else { | ||
| log.Debug( | ||
| ctx, "bumped deadline from activity", | ||
| slog.F("workspace_id", workspace.ID), | ||
| ) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| package coderd_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "cdr.dev/slog/sloggers/slogtest" | ||
|
|
||
| "github.com/coder/coder/coderd/coderdtest" | ||
| "github.com/coder/coder/coderd/database" | ||
| "github.com/coder/coder/codersdk" | ||
| "github.com/coder/coder/testutil" | ||
| ) | ||
|
|
||
| func TestWorkspaceActivityBump(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| setupActivityTest := func(t *testing.T) (client *codersdk.Client, workspace codersdk.Workspace, assertBumped func(want bool)) { | ||
| var ttlMillis int64 = 60 * 1000 | ||
|
|
||
| client, _, workspace, _ = setupProxyTest(t, func(cwr *codersdk.CreateWorkspaceRequest) { | ||
| cwr.TTLMillis = &ttlMillis | ||
| }) | ||
|
|
||
| // Sanity-check that deadline is near. | ||
| workspace, err := client.Workspace(ctx, workspace.ID) | ||
| require.NoError(t, err) | ||
| require.WithinDuration(t, | ||
| time.Now().Add(time.Duration(ttlMillis)*time.Millisecond), | ||
| workspace.LatestBuild.Deadline.Time, testutil.WaitShort, | ||
| ) | ||
| firstDeadline := workspace.LatestBuild.Deadline.Time | ||
|
|
||
| _ = coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) | ||
|
|
||
| return client, workspace, func(want bool) { | ||
| 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 | ||
| // false positive activity since Agent networking event | ||
| // is required. The Activity Bump behavior is also coupled with | ||
| // Last Used, so it would be obvious to the user if we | ||
| // are falsely recognizing activity. | ||
| time.Sleep(testutil.IntervalMedium) | ||
|
There was a problem hiding this comment. This might not trigger a CI flake currently, but its behavior is racy. We can't guarantee that the goroutine has run to completion here so we also can't guarantee we're testing the correct thing. There was a problem hiding this comment. Yeah, it sucks, but I couldn't think of a clean alternative. There was a problem hiding this comment. One option would be dependency injection, e.g. store the activity bumping method on This would be a bit cleaner as an internal test so that nothing needs to be exported. Another idea: I'm not sure what we're sending on the websocket, but could we write a payload informing the other side about what we did (e.g. event: bumped workspace)? There was a problem hiding this comment.
In this particular test we're looking for an absence of a call. So, it's not as simple as replacing the activity bump method. We should also strive to test behavior from the user's perspective. The more we dependency inject and replace internals in our tests the further away we're getting from the user. We may be better off accepting a little bit of raciness for more test accuracy here.
I, too, enjoy internal tests, but that is a codebase decision larger than this PR. There was a problem hiding this comment. I added an explanatory comment to the code. Let me know your thoughts. There was a problem hiding this comment.
I thought we would be testing for the presence of a call, but the absence of modification? What’s the purpose of testing for the absence of a call? The only case that the call wouldn’t happen is if the In my personal opinion we should avoid test racyness as much as possible because those lead to CI flakes that in turn result in loss of developer productivity. There was a problem hiding this comment. In the current implementation, since |
||
| workspace, err = client.Workspace(ctx, workspace.ID) | ||
| require.NoError(t, err) | ||
| require.Equal(t, workspace.LatestBuild.Deadline.Time, firstDeadline) | ||
| return | ||
| } | ||
|
|
||
| // 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 | ||
| }, | ||
| testutil.WaitShort, testutil.IntervalFast, | ||
| "deadline %v never updated", firstDeadline, | ||
| ) | ||
|
|
||
| require.WithinDuration(t, database.Now().Add(time.Hour), workspace.LatestBuild.Deadline.Time, time.Second) | ||
| } | ||
| } | ||
|
|
||
| t.Run("Dial", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| client, workspace, assertBumped := setupActivityTest(t) | ||
|
|
||
| resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) | ||
| conn, err := client.DialWorkspaceAgentTailnet(ctx, slogtest.Make(t, nil), resources[0].Agents[0].ID) | ||
| require.NoError(t, err) | ||
| defer conn.Close() | ||
|
|
||
| sshConn, err := conn.SSHClient() | ||
| require.NoError(t, err) | ||
| _ = sshConn.Close() | ||
|
|
||
| assertBumped(true) | ||
| }) | ||
|
|
||
| t.Run("NoBump", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| client, workspace, assertBumped := setupActivityTest(t) | ||
|
|
||
| // Benign operations like retrieving resources must not | ||
| // bump the deadline. | ||
| _, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID) | ||
| require.NoError(t, err) | ||
|
|
||
| assertBumped(false) | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we don't want to surface an error? It seems like an inconsistency to me that a workspace is being bumped but doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the bump occurs asynchronously, there is no guarantee the workspace exists when this line executes. But, that is not an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense 👍🏻. Should we btw consider moving the log above to blow the error handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was an artifact from testing so I will remove.