feat(agent): support deleting dev containers #21247

DanielleMaywood · 2025-12-12T11:06:00Z

Relates to #19062

Add logic to the agent to allow deleting Dev Containers

PR Stack:

mafredri

I did a quick review, didn't really dig into the tests yet.

mafredri · 2025-12-12T15:42:21Z

agent/agentcontainers/api.go

+ return
+	}
+
+ // Check if the devcontainer is currently starting - if so, we can't delete it.


This should be rewritten and explain why we've decided to have this limitation. Technically there's nothing preventing us from implementing cancellation of create, etc.

PS. I see you -.

Believe it or not, I think I wrote that 😂

mafredri · 2025-12-12T15:44:32Z

agent/agentcontainers/api.go

+	}
+
+ // Similarly, if already stopping, don't allow another delete.
+ if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopping {


Could we make this state deleting instead? I would like to keep stopping as a separate state for the future, one that does not mean delete (i.e. start/stop/delete for the full suite).

Logically a delete could be a stop followed by a delete. I think both are valid use-cases (a user may want to just quickly restart the container to reset the processes, deletion will take potentially much longer).

mafredri · 2025-12-12T15:48:54Z

agent/agentcontainers/api.go

+ Detail:  err.Error(),
+			})
+ return
+		}


Error paths seem to be lacking state handling for the devcontainer. Should probably update dc.Error, perhaps others?

Have gone ahead and implemented some better error handling

mafredri · 2025-12-12T15:51:58Z

agent/agentcontainers/api.go

+ delete(api.recreateSuccessTimes, workspaceFolder)
+ delete(api.recreateErrorTimes, workspaceFolder)
+ delete(api.usingWorkspaceFolderName, workspaceFolder)
+ delete(api.injectedSubAgentProcs, workspaceFolder)


Check: Will this leave the devcontainer available in the dashboard to start, like we do for e.g. detected containers?

Just because the user deleted it, doesn't necessarily mean they won't want to start it again.

The purpose of this PR is to explicitly support removing it, not stopping it (as per #19062)

mafredri · 2025-12-12T15:53:45Z

agent/agentcontainers/api.go

+	}
+
+ // Delete the subagent if it exists.
+ if subAgentID != uuid.Nil {


Should we run this cleanup even if stop/remove fails?

I guess I can't see a reason for not cleaning it up

mafredri · 2025-12-12T15:56:02Z

agent/agentcontainers/api.go

+ httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{
+ Message: "An internal error occurred deleting the agent",
+ Detail:  err.Error(),
+			})


These failures leave the API in a weird state, we've entered a terminal state (stopping), and can't re-invoke delete even if the errors are ephemeral. The container will become immortal.

I've slightly reworked this. On error they now enter WorkspaceAgentDevcontainerStatusError with the Error field filled in.

agent/agentcontainers/api.go

mafredri · 2025-12-12T15:57:53Z

agent/agentcontainers/api_test.go

 return nil, f.execErr
 }

+func (f *fakeContainerCLI) Stop(ctx context.Context, name string) error {


Since we're introducing mutation, we'll need to add a mutex to these.

johnstcn · 2025-12-15T15:40:20Z

agent/agentcontainers/api.go

 	}, nil
 }

+func (api *API) devcontainerByIDLocked(devcontainerID string) (codersdk.WorkspaceAgentDevcontainer, error) {


With methods like this it's nice to add a comment that screams at you to make sure you hold the mutex when you call it.

Also, it would make sense to use this method in handleDevcontainerRecreate().

johnstcn · 2025-12-15T15:52:52Z

agent/agentcontainers/api.go

+ if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusDeleting {
+ api.mu.Unlock()
+ httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{
+ Message: "Devcontainer is already being deleted",
+ Detail:  fmt.Sprintf("Devcontainer %q is already being deleted.", dc.Name),
+		})
+ return
+	}


We should probably also short-circuit this in the recreate handler. If someone tries to delete and then immediately recreate a devcontainer while it's in the "deleting" state, I would imagine a 409 Conflict is the correct response.

johnstcn · 2025-12-15T15:54:15Z

agent/agentcontainers/api.go

+
+ // NOTE(DanielleMaywood):
+ // We currently do not support canceling the startup of a dev container.
+ if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting {


suggestion: Would it make sense to define a method IsTerminal() on the WorkspaceAgentDevcontainerStatus type that returns true for any *-ing state? My understanding is that we don't want to allow any modifications to *-ing devcontainers.

I've added an IsTransitional method for seeing if we're in Starting Stopping or Deleting. I didn't think it made sense to add Running in this since we're wanting to delete from that state.

We don't allow transitioning a Dev Container when it is currently being transitioned

mafredri · 2025-12-16T15:35:34Z

codersdk/workspaceagents.go

 WorkspaceAgentDevcontainerStatusError WorkspaceAgentDevcontainerStatus = "error"
 )

+func (s WorkspaceAgentDevcontainerStatus) IsTransitional() bool {


Nit: Naming, I believe Transitioning better captures the what this is (Is could be dropped to as it's inferred via ing).

mafredri · 2025-12-16T15:42:23Z

agent/agentcontainers/api.go

+ api.mu.Unlock()
+
+ httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{
+ Message: "Unable to delete transitioning Devcontainer",


Suggested change

Message: "Unable to delete transitioning Devcontainer",

Message: "Unable to delete transitioning devcontainer",

mafredri · 2025-12-16T15:43:15Z

agent/agentcontainers/api.go

+ proc.stop()
+	}
+
+ api.mu.Unlock()


We didn't update devcontainer state, re-injection can happen at this point.

Also, why do we unlock just to re-acquire lock 3 lines below?

Suggestion: Drop this Unlock and both next Lock's

Alternatively: Move stopping here, the no-container case will just transition from stopping very quickly which probably fine?

mafredri · 2025-12-16T15:47:44Z

agent/agentcontainers/api.go

+ api.mu.Unlock()
+
+ httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{
+ Message: "An internal error occurred stopping the container",


Suggested change

Message: "An internal error occurred stopping the container",

Message: "An error occurred stopping the container",

Nit: Internal suggests fault in api.

mafredri · 2025-12-16T15:49:10Z

agent/agentcontainers/api.go

+ Detail:  err.Error(),
+			})
+ return
+		}


If following above advice, Lock here.

mafredri · 2025-12-16T15:51:35Z

agent/agentcontainers/api.go

+ api.mu.Unlock()
+
+ httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{
+ Message: "An internal error occurred removing the container",


Suggested change

Message: "An internal error occurred removing the container",

Message: "An error occurred removing the container",

mafredri · 2025-12-16T15:53:42Z

agent/agentcontainers/api.go

 httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{
- Message: "Devcontainer recreation already in progress",
- Detail:  fmt.Sprintf("Recreation for devcontainer %q is already underway.", dc.Name),
+ Message: "Unable to recreate transitioning Devcontainer",


Suggested change

Message: "Unable to recreate transitioning Devcontainer",

Message: "Unable to recreate transitioning devcontainer",

mafredri · 2025-12-16T15:56:04Z

agent/agentcontainers/api_test.go

+ containers: codersdk.WorkspaceAgentListContainersResponse{
+ Containers: []codersdk.WorkspaceAgentContainer{devContainer1},
+					},
+ arch: "<none>",


Nit: Missing comment explaining weirdness, I suppose this also suggests we don't want subagent injection for this test? A few other too.

mafredri · 2025-12-16T15:57:35Z

agent/agentcontainers/api_test.go

+					},
+				},
+ lister: &fakeContainerCLI{
+ arch: "<none>",


None of these tests seem to verify what happens with subagents present?

Seems relevant considering the race I identified with mutex unlocking and injection.

github-actions bot assigned DanielleMaywood Dec 12, 2025

DanielleMaywood force-pushed the danielle/devcontainers/delete/agent branch from 49794cc to 4504755 Compare December 12, 2025 12:12

feat(agent): support deleting dev containers

0fcab3f

DanielleMaywood force-pushed the danielle/devcontainers/delete/agent branch from 4504755 to 0fcab3f Compare December 12, 2025 14:43

This was referenced Dec 12, 2025

feat(coderd): support deleting dev containers #21248

Draft

feat(site): support deleting dev containers #21249

Draft

DanielleMaywood marked this pull request as ready for review December 12, 2025 15:19

DanielleMaywood requested a review from mafredri December 12, 2025 15:19

mafredri reviewed Dec 12, 2025

View reviewed changes

DanielleMaywood added 4 commits December 12, 2025 16:30

chore: Stopping -> Deleting

641f6a2

chore: add stopping phase

a57d4b8

chore: appease linter

973e0ab

chore: remove unneeded variable

3b0b84a

DanielleMaywood requested a review from johnstcn December 15, 2025 15:28

johnstcn reviewed Dec 15, 2025

View reviewed changes

DanielleMaywood added 2 commits December 15, 2025 16:30

refactor: add IsTransitional method

9ae270b

We don't allow transitioning a Dev Container when it is currently being transitioned

chore: some comments

25b7a72

johnstcn approved these changes Dec 15, 2025

View reviewed changes

DanielleMaywood requested a review from mafredri December 16, 2025 09:24

mafredri reviewed Dec 16, 2025

View reviewed changes

	Message: "Unable to delete transitioning Devcontainer",
	Message: "Unable to delete transitioning devcontainer",

	Message: "An internal error occurred stopping the container",
	Message: "An error occurred stopping the container",

	Message: "An internal error occurred removing the container",
	Message: "An error occurred removing the container",

	Message: "Unable to recreate transitioning Devcontainer",
	Message: "Unable to recreate transitioning devcontainer",

feat(agent): support deleting dev containers #21247

Are you sure you want to change the base?

feat(agent): support deleting dev containers #21247

Uh oh!

Conversation

DanielleMaywood commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mafredri left a comment

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!

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!

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!

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!

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!

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DanielleMaywood commented Dec 12, 2025 •

edited

Loading