feat: add an endpoint to manually pause a coder task by SasSwart · Pull Request #21889 · coder/coder

SasSwart · 2026-02-03T14:48:25Z

This pull request adds an endpoint to pause coder tasks by stopping the underlying workspace.

Instead of POST /api/v2/tasks/{user}/{task}/pause, the endpoint is currently experimental.

coderd/aitasks.go

mafredri · 2026-02-06T09:12:32Z

coderd/aitasks_test.go

+			getWorkspaceByID: func(ctx context.Context, id uuid.UUID) (database.Workspace, error) {
+				if id == workspaceID && id != uuid.Nil {
+					return database.Workspace{}, sql.ErrNoRows
+				}


I guess the idea here is to provide 100% coverage? Even so, I think we could simplify and skip the wrapper entirely by using dbgen to insert the data in a way that produces the same or similar result, or skip cases that can't happen. (Same for the other uses too.)

For instance, our data model already enforces that when a WorkspaceID is non-null, the workspace must exist (FK).

Main motivator here is to keep the tests aligned with existing tests for endpoints.

I've looked into this. I've removed a few spots where we were using the wrapper with no benefit. That leaves it only in-use in two tests.

One of these tests is technically currently redundant because of the FK, as you stated. I see some value in keeping it simply because it defines behaviour defensively in case anything ever changes to make it possible. Nevertheless, I'm happy to remove it.

For the last remaining use of the wrapper in t.Run("Workspace lookup internal error",..., simulating this using a contrived db interaction would be less clear in my opinion than faking the behaviour this way.

What do you think?

This is one of those things that it'd make sense to test for all endpoints if we're going to, rather than just one. I'd be OK with cutting it but won't enforce it.

Alternatively perhaps it could be a shared method like func (*API) getTaskWorkspace that can be used by all handlers so we only need to test it once. I'm not particularly fond of that suggestion, however.

(For context, this aitasks_test.go file already has many testing patterns, and I'm a bit concerned with each endpoint having bespoke testing arbitrarily enforcing some conditions while allowing others to go unchecked.)

coderd/aitasks_test.go

mafredri · 2026-02-06T09:18:59Z

coderd/aitasks_test.go

+				expectedStatus: http.StatusAccepted,
+			},
+		}
+


I'm a bit conflicted about testing this (I don't think we have it for other endpoints). On the one hand, it's very illustrative. But on another, this should already be enforced by rbac/middleware that's in use.

At the very least, this should not be repeated per-endpoint, but rather testing all task endpoints for the same roles.

I wrote this test to prove to myself that the following requirement is implicitly met: Authorization checks task ownership

You're right that it should already be enforced in another abstraction layer architecturally. But this test makes a domain level promise based on the product requirement.

I'm happy to rework this. We could just move it to the package where this is enforced. But then its no longer as clear that we have a test for this specific requirement on this specific endpoint.

What do you think?

I can understand how that might seem like a distinction that requires testing, however, this endpoint does not differ in authorization compared to other task endpoints. Essentially what that requirement asks for is the presence of the task id/name handling middleware which enforces auth.

One way to stress-test this addition is: Would you duplicate/repeat these tests for the resume endpoint? If yes, that's a signal that we're either going too granular or should generalize.

On (some) other endpoints we're testing auth by creating another user and making sure they don't have access. This is already duplication, but it's not taking roles to this level of granularity. To me it would make sense to take this level of granularity to all endpoints (one test/subtests, tests all) and get rid of the duplication. Or, to duplicate the more simplistic test. I'd prefer the former but up to you as either approach is OK for the PR.

One way to stress-test this addition is: Would you duplicate/repeat these tests for the resume endpoint? If yes, that's a signal that we're either going too granular or should generalize.

This is a great point. And in response I tend to fall on the "should generalize" side of things. Perhaps we can start by generalizing this test to work for both Pause and Resume in the next PR. And then after that generalize it further to all of tasks or beyond?

mafredri · 2026-02-06T09:20:45Z

coderd/aitasks_test.go

+		require.Equal(t, http.StatusNotFound, apiErr.StatusCode())
+	})
+
+	t.Run("Task lookup forbidden", func(t *testing.T) {


This repeats the role test above?

In this test the task actually exists and we test that the reason it can't be found is authz. its part of testing the Authorization checks task ownership requirement.

The test above checks what happens when there is just simply no such task.

It'd be nice to generalize this as well for all tasks endpoints tbh. Perhaps we should create a follow-up task to tests auth/roles for all endpoints and remove duplication?

codersdk/aitasks.go

coderd/aitasks_test.go

rename storeWrapper to aiTaskStoreWrapper. return workspace builds instead of just the build id when a task is paused eliminate unnecessary use of the aiTaskStoreWrapper

SasSwart · 2026-02-06T14:02:40Z

@mafredri I'll work on generalizing the tests over in the next PR where I am already implementing the Task resume tests. It makes sense to do it there since we have both endpoints to generalize on. Is there anything else you'd like to address on this PR or can we move forward?

mafredri

I'm happy with the suggestions for test refactoring/generalization in follow-up PR. Implementation looks solid, nice work. 👍🏻

mafredri · 2026-02-06T15:06:49Z

coderd/aitasks_test.go

+			getWorkspaceByID: func(ctx context.Context, id uuid.UUID) (database.Workspace, error) {
+				if id == workspaceID && id != uuid.Nil {
+					return database.Workspace{}, sql.ErrNoRows
+				}


This is one of those things that it'd make sense to test for all endpoints if we're going to, rather than just one. I'd be OK with cutting it but won't enforce it.

Alternatively perhaps it could be a shared method like func (*API) getTaskWorkspace that can be used by all handlers so we only need to test it once. I'm not particularly fond of that suggestion, however.

(For context, this aitasks_test.go file already has many testing patterns, and I'm a bit concerned with each endpoint having bespoke testing arbitrarily enforcing some conditions while allowing others to go unchecked.)

github-actions bot assigned SasSwart Feb 3, 2026

mafredri mentioned this pull request Feb 4, 2026

feat(site): add pause/resume action buttons to tasks table #21728

Merged

SasSwart requested a review from mafredri February 5, 2026 14:27

SasSwart marked this pull request as ready for review February 5, 2026 14:27

SasSwart mentioned this pull request Feb 5, 2026

Tasks: Manual pause endpoint coder/internal#1261

Closed

5 tasks

mafredri reviewed Feb 6, 2026

View reviewed changes

SasSwart added 10 commits February 6, 2026 13:28

feat: add an endpoint to manually pause a coder task

db7a87c

make gen

71f742b

Implement "Task not found" test case

e3ee8f0

implement test cases

697456f

Use a response DTO for the task pause endpoint

0163b86

make lint and make gen

41c0fe1

test for ownership authorization when pausing a task

b3d807d

Set the build reason for the stop build when a task is paused

333f1c3

review notes

cbfd48d

rename storeWrapper to aiTaskStoreWrapper. return workspace builds instead of just the build id when a task is paused eliminate unnecessary use of the aiTaskStoreWrapper

make gen

c16f1c1

SasSwart force-pushed the jjs/internal-1261 branch from d9ee82b to c16f1c1 Compare February 6, 2026 13:31

SasSwart requested a review from mafredri February 6, 2026 14:02

mafredri approved these changes Feb 6, 2026

View reviewed changes

SasSwart merged commit e6fbf50 into main Feb 9, 2026
32 checks passed

SasSwart deleted the jjs/internal-1261 branch February 9, 2026 06:56

github-actions bot locked and limited conversation to collaborators Feb 9, 2026

Conversation

SasSwart commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SasSwart Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SasSwart Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SasSwart commented Feb 6, 2026

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SasSwart commented Feb 3, 2026 •

edited

Loading

SasSwart Feb 6, 2026 •

edited

Loading

mafredri Feb 6, 2026 •

edited

Loading

SasSwart Feb 6, 2026 •

edited

Loading