From 0fcab3f187020f81032c1888ed82c8cddf8df8e1 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 26 Nov 2025 10:55:27 +0000 Subject: [PATCH 01/10] feat(agent): support deleting dev containers --- agent/agentcontainers/acmock/acmock.go | 28 ++ agent/agentcontainers/api.go | 152 +++++++++- agent/agentcontainers/api_test.go | 264 ++++++++++++++++++ agent/agentcontainers/containers.go | 6 + agent/agentcontainers/containers_dockercli.go | 16 ++ .../containers_dockercli_test.go | 96 +++++++ cli/open_test.go | 8 + coderd/apidoc/docs.go | 2 + coderd/apidoc/swagger.json | 3 +- codersdk/workspaceagents.go | 1 + docs/reference/api/schemas.md | 1 + site/src/api/typesGenerated.ts | 5 +- 12 files changed, 566 insertions(+), 16 deletions(-) diff --git a/agent/agentcontainers/acmock/acmock.go b/agent/agentcontainers/acmock/acmock.go index b6bb4a9523fb6..af18e880459d1 100644 --- a/agent/agentcontainers/acmock/acmock.go +++ b/agent/agentcontainers/acmock/acmock.go @@ -106,6 +106,34 @@ func (mr *MockContainerCLIMockRecorder) List(ctx any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockContainerCLI)(nil).List), ctx) } +// Remove mocks base method. +func (m *MockContainerCLI) Remove(ctx context.Context, containerName string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Remove", ctx, containerName) + ret0, _ := ret[0].(error) + return ret0 +} + +// Remove indicates an expected call of Remove. +func (mr *MockContainerCLIMockRecorder) Remove(ctx, containerName any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Remove", reflect.TypeOf((*MockContainerCLI)(nil).Remove), ctx, containerName) +} + +// Stop mocks base method. +func (m *MockContainerCLI) Stop(ctx context.Context, containerName string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Stop", ctx, containerName) + ret0, _ := ret[0].(error) + return ret0 +} + +// Stop indicates an expected call of Stop. +func (mr *MockContainerCLIMockRecorder) Stop(ctx, containerName any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Stop", reflect.TypeOf((*MockContainerCLI)(nil).Stop), ctx, containerName) +} + // MockDevcontainerCLI is a mock of DevcontainerCLI interface. type MockDevcontainerCLI struct { ctrl *gomock.Controller diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 9838b7b9dc55d..72f0fbe1d63fb 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -32,6 +32,7 @@ import ( "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/agent/usershell" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/httpapi/httperror" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/agentsdk" "github.com/coder/coder/v2/provisioner" @@ -743,6 +744,7 @@ func (api *API) Routes() http.Handler { // /-route was dropped. We can drop the /devcontainers prefix here too. r.Route("/devcontainers/{devcontainer}", func(r chi.Router) { r.Post("/recreate", api.handleDevcontainerRecreate) + r.Delete("/", api.handleDevcontainerDelete) }) return r @@ -1019,6 +1021,9 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting: continue // This state is handled by the recreation routine. + case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopping: + continue // This state is handled by the delete routine. + case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusError && (dc.Container == nil || dc.Container.CreatedAt.Before(api.recreateErrorTimes[dc.WorkspaceFolder])): continue // The devcontainer needs to be recreated. @@ -1224,6 +1229,137 @@ func (api *API) getContainers() (codersdk.WorkspaceAgentListContainersResponse, }, nil } +func (api *API) devcontainerByIDLocked(devcontainerID string) (codersdk.WorkspaceAgentDevcontainer, error) { + for _, knownDC := range api.knownDevcontainers { + if knownDC.ID.String() == devcontainerID { + return knownDC, nil + } + } + + return codersdk.WorkspaceAgentDevcontainer{}, httperror.NewResponseError(http.StatusNotFound, codersdk.Response{ + Message: "Devcontainer not found.", + Detail: fmt.Sprintf("Could not find devcontainer with ID: %q", devcontainerID), + }) +} + +func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) { + var ( + ctx = r.Context() + devcontainerID = chi.URLParam(r, "devcontainer") + ) + + if devcontainerID == "" { + httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{ + Message: "Missing devcontainer ID", + Detail: "Devcontainer ID is required to delete a devcontainer.", + }) + return + } + + api.mu.Lock() + + dc, err := api.devcontainerByIDLocked(devcontainerID) + if err != nil { + api.mu.Unlock() + httperror.WriteResponseError(ctx, w, err) + return + } + + // Check if the devcontainer is currently starting - if so, we can't delete it. + if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting { + api.mu.Unlock() + httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ + Message: "Devcontainer is starting", + Detail: fmt.Sprintf("Devcontainer %q is currently starting and cannot be deleted.", dc.Name), + }) + return + } + + // Similarly, if already stopping, don't allow another delete. + if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopping { + api.mu.Unlock() + httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ + Message: "Devcontainer is stopping", + Detail: fmt.Sprintf("Devcontainer %q is currently stopping.", dc.Name), + }) + return + } + + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStopping + dc.Error = "" + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.broadcastUpdatesLocked() + + // Gather the information we need before unlocking. + workspaceFolder := dc.WorkspaceFolder + dcName := dc.Name + var containerID string + if dc.Container != nil { + containerID = dc.Container.ID + } + proc, hasSubAgent := api.injectedSubAgentProcs[workspaceFolder] + var subAgentID uuid.UUID + if hasSubAgent && proc.agent.ID != uuid.Nil { + subAgentID = proc.agent.ID + // Stop the subagent process context to ensure it stops. + proc.stop() + } + + // Unlock the mutex while we perform potentially slow operations + // (network calls, docker commands) to avoid blocking other operations. + api.mu.Unlock() + + // Stop and remove the container if it exists. + if containerID != "" { + if err := api.ccli.Stop(ctx, containerID); err != nil { + api.logger.Error(ctx, "unable to stop container", slog.Error(err)) + + httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ + Message: "An internal error occurred stopping the container", + Detail: err.Error(), + }) + return + } + + if err := api.ccli.Remove(ctx, containerID); err != nil { + api.logger.Error(ctx, "unable to remove container", slog.Error(err)) + + httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ + Message: "An internal error occurred removing the container", + Detail: err.Error(), + }) + return + } + } + + // Delete the subagent if it exists. + if subAgentID != uuid.Nil { + client := *api.subAgentClient.Load() + if err := client.Delete(ctx, subAgentID); err != nil { + api.logger.Error(ctx, "unable to delete agent", slog.Error(err)) + + httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ + Message: "An internal error occurred deleting the agent", + Detail: err.Error(), + }) + return + } + } + + api.mu.Lock() + delete(api.devcontainerNames, dcName) + delete(api.knownDevcontainers, workspaceFolder) + delete(api.devcontainerLogSourceIDs, workspaceFolder) + delete(api.recreateSuccessTimes, workspaceFolder) + delete(api.recreateErrorTimes, workspaceFolder) + delete(api.usingWorkspaceFolderName, workspaceFolder) + delete(api.injectedSubAgentProcs, workspaceFolder) + api.broadcastUpdatesLocked() + api.mu.Unlock() + + httpapi.Write(ctx, w, http.StatusNoContent, nil) +} + // handleDevcontainerRecreate handles the HTTP request to recreate a // devcontainer by referencing the container. func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Request) { @@ -1240,20 +1376,10 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques api.mu.Lock() - var dc codersdk.WorkspaceAgentDevcontainer - for _, knownDC := range api.knownDevcontainers { - if knownDC.ID.String() == devcontainerID { - dc = knownDC - break - } - } - if dc.ID == uuid.Nil { + dc, err := api.devcontainerByIDLocked(devcontainerID) + if err != nil { api.mu.Unlock() - - httpapi.Write(ctx, w, http.StatusNotFound, codersdk.Response{ - Message: "Devcontainer not found.", - Detail: fmt.Sprintf("Could not find devcontainer with ID: %q", devcontainerID), - }) + httperror.WriteResponseError(ctx, w, err) return } if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting { diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 45a1fa28f015a..2ad63cc869e53 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -34,6 +34,7 @@ import ( "github.com/coder/coder/v2/agent/agentcontainers/acmock" "github.com/coder/coder/v2/agent/agentcontainers/watcher" "github.com/coder/coder/v2/agent/usershell" + "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/pty" "github.com/coder/coder/v2/testutil" @@ -50,6 +51,8 @@ type fakeContainerCLI struct { archErr error copyErr error execErr error + stopErr error + removeErr error } func (f *fakeContainerCLI) List(_ context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) { @@ -68,6 +71,26 @@ func (f *fakeContainerCLI) ExecAs(ctx context.Context, name, user string, args . return nil, f.execErr } +func (f *fakeContainerCLI) Stop(ctx context.Context, name string) error { + f.containers.Devcontainers = slice.Filter(f.containers.Devcontainers, func(dc codersdk.WorkspaceAgentDevcontainer) bool { + return dc.Container.ID == name + }) + for i, container := range f.containers.Containers { + container.Running = false + f.containers.Containers[i] = container + } + + return f.stopErr +} + +func (f *fakeContainerCLI) Remove(ctx context.Context, name string) error { + f.containers.Containers = slice.Filter(f.containers.Containers, func(container codersdk.WorkspaceAgentContainer) bool { + return container.ID == name + }) + + return f.removeErr +} + // fakeDevcontainerCLI implements the agentcontainers.DevcontainerCLI // interface for testing. type fakeDevcontainerCLI struct { @@ -1035,6 +1058,247 @@ func TestAPI(t *testing.T) { } }) + t.Run("Delete", func(t *testing.T) { + t.Parallel() + + devcontainerID1 := uuid.New() + workspaceFolder1 := "/workspace/test1" + configPath1 := "/workspace/test1/.devcontainer/devcontainer.json" + + // Create a container that represents an existing devcontainer. + devContainer1 := codersdk.WorkspaceAgentContainer{ + ID: "container-1", + FriendlyName: "test-container-1", + Running: true, + Labels: map[string]string{ + agentcontainers.DevcontainerLocalFolderLabel: workspaceFolder1, + agentcontainers.DevcontainerConfigFileLabel: configPath1, + }, + } + + tests := []struct { + name string + devcontainerID string + setupDevcontainers []codersdk.WorkspaceAgentDevcontainer + lister *fakeContainerCLI + devcontainerCLI *fakeDevcontainerCLI + wantStatus int + wantBody string + }{ + { + name: "Missing devcontainer ID", + devcontainerID: "", + lister: &fakeContainerCLI{}, + devcontainerCLI: &fakeDevcontainerCLI{}, + wantStatus: http.StatusBadRequest, + wantBody: "Missing devcontainer ID", + }, + { + name: "Devcontainer not found", + devcontainerID: uuid.NewString(), + lister: &fakeContainerCLI{ + arch: "", // Unsupported architecture, don't inject subagent. + }, + devcontainerCLI: &fakeDevcontainerCLI{}, + wantStatus: http.StatusNotFound, + wantBody: "Devcontainer not found", + }, + { + name: "Devcontainer is starting", + devcontainerID: devcontainerID1.String(), + setupDevcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerID1, + Name: "test-devcontainer-1", + WorkspaceFolder: workspaceFolder1, + ConfigPath: configPath1, + Status: codersdk.WorkspaceAgentDevcontainerStatusStarting, + Container: &devContainer1, + }, + }, + lister: &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{devContainer1}, + }, + arch: "", + }, + devcontainerCLI: &fakeDevcontainerCLI{}, + wantStatus: http.StatusConflict, + wantBody: "Devcontainer is starting", + }, + { + name: "Devcontainer is stopping", + devcontainerID: devcontainerID1.String(), + setupDevcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerID1, + Name: "test-devcontainer-1", + WorkspaceFolder: workspaceFolder1, + ConfigPath: configPath1, + Status: codersdk.WorkspaceAgentDevcontainerStatusStopping, + Container: &devContainer1, + }, + }, + lister: &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{devContainer1}, + }, + arch: "", + }, + devcontainerCLI: &fakeDevcontainerCLI{}, + wantStatus: http.StatusConflict, + wantBody: "Devcontainer is stopping", + }, + { + name: "Container stop fails", + devcontainerID: devcontainerID1.String(), + setupDevcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerID1, + Name: "test-devcontainer-1", + WorkspaceFolder: workspaceFolder1, + ConfigPath: configPath1, + Status: codersdk.WorkspaceAgentDevcontainerStatusRunning, + Container: &devContainer1, + }, + }, + lister: &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{devContainer1}, + }, + arch: "", + stopErr: xerrors.New("stop error"), + }, + devcontainerCLI: &fakeDevcontainerCLI{}, + wantStatus: http.StatusInternalServerError, + wantBody: "An internal error occurred stopping the container", + }, + { + name: "Container remove fails", + devcontainerID: devcontainerID1.String(), + setupDevcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerID1, + Name: "test-devcontainer-1", + WorkspaceFolder: workspaceFolder1, + ConfigPath: configPath1, + Status: codersdk.WorkspaceAgentDevcontainerStatusRunning, + Container: &devContainer1, + }, + }, + lister: &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{devContainer1}, + }, + arch: "", + removeErr: xerrors.New("remove error"), + }, + devcontainerCLI: &fakeDevcontainerCLI{}, + wantStatus: http.StatusInternalServerError, + wantBody: "An internal error occurred removing the container", + }, + { + name: "OK with container", + devcontainerID: devcontainerID1.String(), + setupDevcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerID1, + Name: "test-devcontainer-1", + WorkspaceFolder: workspaceFolder1, + ConfigPath: configPath1, + Status: codersdk.WorkspaceAgentDevcontainerStatusRunning, + Container: &devContainer1, + }, + }, + lister: &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{devContainer1}, + }, + arch: "", + }, + devcontainerCLI: &fakeDevcontainerCLI{}, + wantStatus: http.StatusNoContent, + wantBody: "", + }, + { + name: "OK without container", + devcontainerID: devcontainerID1.String(), + setupDevcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerID1, + Name: "test-devcontainer-1", + WorkspaceFolder: workspaceFolder1, + ConfigPath: configPath1, + Status: codersdk.WorkspaceAgentDevcontainerStatusStopped, + Container: nil, // No container. + }, + }, + lister: &fakeContainerCLI{ + arch: "", + }, + devcontainerCLI: &fakeDevcontainerCLI{}, + wantStatus: http.StatusNoContent, + wantBody: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + mClock := quartz.NewMock(t) + mClock.Set(time.Now()).MustWait(ctx) + tickerTrap := mClock.Trap().TickerFunc("updaterLoop") + + // Setup router with the handler under test. + r := chi.NewRouter() + + api := agentcontainers.NewAPI( + logger, + agentcontainers.WithClock(mClock), + agentcontainers.WithContainerCLI(tt.lister), + agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI), + agentcontainers.WithWatcher(watcher.NewNoop()), + agentcontainers.WithDevcontainers(tt.setupDevcontainers, nil), + ) + + api.Start() + defer api.Close() + r.Mount("/", api.Routes()) + + tickerTrap.MustWait(ctx).MustRelease(ctx) + tickerTrap.Close() + + req := httptest.NewRequest(http.MethodDelete, "/devcontainers/"+tt.devcontainerID+"/", nil). + WithContext(ctx) + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + require.Equal(t, tt.wantStatus, rec.Code, "status code mismatch") + if tt.wantBody != "" { + assert.Contains(t, rec.Body.String(), tt.wantBody, "response body mismatch") + } + + // For successful deletes, verify the devcontainer is removed from the list. + if tt.wantStatus == http.StatusNoContent { + req = httptest.NewRequest(http.MethodGet, "/", nil). + WithContext(ctx) + rec = httptest.NewRecorder() + r.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, "status code mismatch on list") + var resp codersdk.WorkspaceAgentListContainersResponse + err := json.NewDecoder(rec.Body).Decode(&resp) + require.NoError(t, err, "unmarshal response failed") + assert.Empty(t, resp.Devcontainers, "devcontainer should be removed after delete") + } + }) + } + }) + t.Run("List devcontainers", func(t *testing.T) { t.Parallel() diff --git a/agent/agentcontainers/containers.go b/agent/agentcontainers/containers.go index e728507e8f394..99226fd2f5a7c 100644 --- a/agent/agentcontainers/containers.go +++ b/agent/agentcontainers/containers.go @@ -17,6 +17,10 @@ type ContainerCLI interface { Copy(ctx context.Context, containerName, src, dst string) error // ExecAs executes a command in a container as a specific user. ExecAs(ctx context.Context, containerName, user string, args ...string) ([]byte, error) + // Stop terminates the container + Stop(ctx context.Context, containerName string) error + // Remove removes the container + Remove(ctx context.Context, containerName string) error } // noopContainerCLI is a ContainerCLI that does nothing. @@ -35,3 +39,5 @@ func (noopContainerCLI) Copy(_ context.Context, _ string, _ string, _ string) er func (noopContainerCLI) ExecAs(_ context.Context, _ string, _ string, _ ...string) ([]byte, error) { return nil, nil } +func (noopContainerCLI) Stop(_ context.Context, _ string) error { return nil } +func (noopContainerCLI) Remove(_ context.Context, _ string) error { return nil } diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index 58ca3901e2f23..ad88b44c06c18 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -583,6 +583,22 @@ func (dcli *dockerCLI) ExecAs(ctx context.Context, containerName, uid string, ar return stdout, nil } +func (dcli *dockerCLI) Stop(ctx context.Context, containerName string) error { + _, stderr, err := runCmd(ctx, dcli.execer, "docker", "stop", containerName) + if err != nil { + return xerrors.Errorf("stop %s: %w: %s", containerName, err, stderr) + } + return nil +} + +func (dcli *dockerCLI) Remove(ctx context.Context, containerName string) error { + _, stderr, err := runCmd(ctx, dcli.execer, "docker", "rm", containerName) + if err != nil { + return xerrors.Errorf("remove %s: %w: %s", containerName, err, stderr) + } + return nil +} + // runCmd is a helper function that runs a command with the given // arguments and returns the stdout and stderr output. func runCmd(ctx context.Context, execer agentexec.Execer, cmd string, args ...string) (stdout, stderr []byte, err error) { diff --git a/agent/agentcontainers/containers_dockercli_test.go b/agent/agentcontainers/containers_dockercli_test.go index 3c299e353858d..6b35b67858bd2 100644 --- a/agent/agentcontainers/containers_dockercli_test.go +++ b/agent/agentcontainers/containers_dockercli_test.go @@ -126,3 +126,99 @@ func TestIntegrationDockerCLI(t *testing.T) { t.Logf("Successfully executed commands in container %s", containerName) }) } + +// TestIntegrationDockerCLIStop tests the Stop method using a real +// Docker container. +// +// Run manually with: CODER_TEST_USE_DOCKER=1 go test ./agent/agentcontainers -run TestIntegrationDockerCLIStop +// +//nolint:tparallel,paralleltest // Docker integration tests don't run in parallel to avoid flakiness. +func TestIntegrationDockerCLIStop(t *testing.T) { + if os.Getenv("CODER_TEST_USE_DOCKER") != "1" { + t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") + } + + ctx := testutil.Context(t, testutil.WaitLong) + + pool, err := dockertest.NewPool("") + require.NoError(t, err, "Could not connect to docker") + + // Given: A simple busybox container + ct, err := pool.RunWithOptions(&dockertest.RunOptions{ + Repository: "busybox", + Tag: "latest", + Cmd: []string{"sleep", "infinity"}, + }, func(config *docker.HostConfig) { + config.RestartPolicy = docker.RestartPolicy{Name: "no"} + }) + require.NoError(t, err, "Could not start test docker container") + t.Logf("Created container %q", ct.Container.Name) + t.Cleanup(func() { + assert.NoError(t, pool.Purge(ct), "Could not purge resource %q", ct.Container.Name) + t.Logf("Purged container %q", ct.Container.Name) + }) + + // Given: The container is running + require.Eventually(t, func() bool { + ct, ok := pool.ContainerByName(ct.Container.Name) + return ok && ct.Container.State.Running + }, testutil.WaitShort, testutil.IntervalSlow, "Container did not start in time") + + dcli := agentcontainers.NewDockerCLI(agentexec.DefaultExecer) + containerName := strings.TrimPrefix(ct.Container.Name, "/") + + // When: We attempt to stop the container + err = dcli.Stop(ctx, containerName) + require.NoError(t, err) + + // Then: We expect the container to be stopped. + ct, ok := pool.ContainerByName(ct.Container.Name) + require.True(t, ok) + require.False(t, ct.Container.State.Running) + require.Equal(t, "exited", ct.Container.State.Status) +} + +// TestIntegrationDockerCLIRemove tests the Remove method using a real +// Docker container. +// +// Run manually with: CODER_TEST_USE_DOCKER=1 go test ./agent/agentcontainers -run TestIntegrationDockerCLIRemove +// +//nolint:tparallel,paralleltest // Docker integration tests don't run in parallel to avoid flakiness. +func TestIntegrationDockerCLIRemove(t *testing.T) { + if os.Getenv("CODER_TEST_USE_DOCKER") != "1" { + t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") + } + + ctx := testutil.Context(t, testutil.WaitLong) + + pool, err := dockertest.NewPool("") + require.NoError(t, err, "Could not connect to docker") + + // Given: A simple busybox container that exits immediately. + ct, err := pool.RunWithOptions(&dockertest.RunOptions{ + Repository: "busybox", + Tag: "latest", + Cmd: []string{"true"}, + }, func(config *docker.HostConfig) { + config.RestartPolicy = docker.RestartPolicy{Name: "no"} + }) + require.NoError(t, err, "Could not start test docker container") + t.Logf("Created container %q", ct.Container.Name) + containerName := strings.TrimPrefix(ct.Container.Name, "/") + + // Wait for the container to exit. + require.Eventually(t, func() bool { + ct, ok := pool.ContainerByName(ct.Container.Name) + return ok && !ct.Container.State.Running + }, testutil.WaitShort, testutil.IntervalSlow, "Container did not stop in time") + + dcli := agentcontainers.NewDockerCLI(agentexec.DefaultExecer) + + // When: We attempt to remove the container. + err = dcli.Remove(ctx, containerName) + require.NoError(t, err) + + // Then: We expect the container to be removed. + _, ok := pool.ContainerByName(ct.Container.Name) + require.False(t, ok, "Container should be removed") +} diff --git a/cli/open_test.go b/cli/open_test.go index 688fc24b5e84d..595bb2f1ceaf5 100644 --- a/cli/open_test.go +++ b/cli/open_test.go @@ -311,6 +311,14 @@ func (*fakeContainerCLI) ExecAs(ctx context.Context, containerID, user string, a return nil, nil } +func (*fakeContainerCLI) Stop(ctx context.Context, containerID string) error { + return nil +} + +func (*fakeContainerCLI) Remove(ctx context.Context, containerID string) error { + return nil +} + type fakeDevcontainerCLI struct { config agentcontainers.DevcontainerConfig execAgent func(ctx context.Context, token string) error diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 800058b759cad..09ba905265379 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -20343,12 +20343,14 @@ const docTemplate = `{ "running", "stopped", "starting", + "stopping", "error" ], "x-enum-varnames": [ "WorkspaceAgentDevcontainerStatusRunning", "WorkspaceAgentDevcontainerStatusStopped", "WorkspaceAgentDevcontainerStatusStarting", + "WorkspaceAgentDevcontainerStatusStopping", "WorkspaceAgentDevcontainerStatusError" ] }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index ea54c1b219a98..c5f12d0685b31 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -18695,11 +18695,12 @@ }, "codersdk.WorkspaceAgentDevcontainerStatus": { "type": "string", - "enum": ["running", "stopped", "starting", "error"], + "enum": ["running", "stopped", "starting", "stopping", "error"], "x-enum-varnames": [ "WorkspaceAgentDevcontainerStatusRunning", "WorkspaceAgentDevcontainerStatusStopped", "WorkspaceAgentDevcontainerStatusStarting", + "WorkspaceAgentDevcontainerStatusStopping", "WorkspaceAgentDevcontainerStatusError" ] }, diff --git a/codersdk/workspaceagents.go b/codersdk/workspaceagents.go index 4f3faedb534fc..6693eb90e4546 100644 --- a/codersdk/workspaceagents.go +++ b/codersdk/workspaceagents.go @@ -401,6 +401,7 @@ const ( WorkspaceAgentDevcontainerStatusRunning WorkspaceAgentDevcontainerStatus = "running" WorkspaceAgentDevcontainerStatusStopped WorkspaceAgentDevcontainerStatus = "stopped" WorkspaceAgentDevcontainerStatusStarting WorkspaceAgentDevcontainerStatus = "starting" + WorkspaceAgentDevcontainerStatusStopping WorkspaceAgentDevcontainerStatus = "stopping" WorkspaceAgentDevcontainerStatusError WorkspaceAgentDevcontainerStatus = "error" ) diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 892e52c4e24fe..b4027961e76df 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -10762,6 +10762,7 @@ If the schedule is empty, the user will be updated to use the default schedule.| | `running` | | `stopped` | | `starting` | +| `stopping` | | `error` | ## codersdk.WorkspaceAgentHealth diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 7311032a4063f..df8c12f3661cb 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -6104,10 +6104,11 @@ export type WorkspaceAgentDevcontainerStatus = | "error" | "running" | "starting" - | "stopped"; + | "stopped" + | "stopping"; export const WorkspaceAgentDevcontainerStatuses: WorkspaceAgentDevcontainerStatus[] = - ["error", "running", "starting", "stopped"]; + ["error", "running", "starting", "stopped", "stopping"]; // From codersdk/workspaceagents.go export interface WorkspaceAgentHealth { From 641f6a29ac95da18e75511a02e410a3297638a4a Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 12 Dec 2025 16:29:38 +0000 Subject: [PATCH 02/10] chore: Stopping -> Deleting --- agent/agentcontainers/api.go | 12 ++++++------ agent/agentcontainers/api_test.go | 2 +- codersdk/workspaceagents.go | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 72f0fbe1d63fb..ca8db161f71db 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1021,7 +1021,7 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting: continue // This state is handled by the recreation routine. - case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopping: + case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusDeleting: continue // This state is handled by the delete routine. case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusError && (dc.Container == nil || dc.Container.CreatedAt.Before(api.recreateErrorTimes[dc.WorkspaceFolder])): @@ -1275,17 +1275,17 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) return } - // Similarly, if already stopping, don't allow another delete. - if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopping { + // Similarly, if already deleting, don't allow another delete. + if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusDeleting { api.mu.Unlock() httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ - Message: "Devcontainer is stopping", - Detail: fmt.Sprintf("Devcontainer %q is currently stopping.", dc.Name), + Message: "Devcontainer is already being deleted", + Detail: fmt.Sprintf("Devcontainer %q is already being deleted.", dc.Name), }) return } - dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStopping + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusDeleting dc.Error = "" api.knownDevcontainers[dc.WorkspaceFolder] = dc api.broadcastUpdatesLocked() diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 2ad63cc869e53..01f21b862f361 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -1135,7 +1135,7 @@ func TestAPI(t *testing.T) { Name: "test-devcontainer-1", WorkspaceFolder: workspaceFolder1, ConfigPath: configPath1, - Status: codersdk.WorkspaceAgentDevcontainerStatusStopping, + Status: codersdk.WorkspaceAgentDevcontainerStatusDeleting, Container: &devContainer1, }, }, diff --git a/codersdk/workspaceagents.go b/codersdk/workspaceagents.go index 6693eb90e4546..8e593623ccfd5 100644 --- a/codersdk/workspaceagents.go +++ b/codersdk/workspaceagents.go @@ -401,7 +401,7 @@ const ( WorkspaceAgentDevcontainerStatusRunning WorkspaceAgentDevcontainerStatus = "running" WorkspaceAgentDevcontainerStatusStopped WorkspaceAgentDevcontainerStatus = "stopped" WorkspaceAgentDevcontainerStatusStarting WorkspaceAgentDevcontainerStatus = "starting" - WorkspaceAgentDevcontainerStatusStopping WorkspaceAgentDevcontainerStatus = "stopping" + WorkspaceAgentDevcontainerStatusDeleting WorkspaceAgentDevcontainerStatus = "deleting" WorkspaceAgentDevcontainerStatusError WorkspaceAgentDevcontainerStatus = "error" ) From a57d4b889e0e5aad62323083e4a4730dfcf30f91 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 12 Dec 2025 16:29:38 +0000 Subject: [PATCH 03/10] chore: add stopping phase --- agent/agentcontainers/api.go | 67 ++++++++++++++++++++++--------- agent/agentcontainers/api_test.go | 9 ++++- coderd/apidoc/docs.go | 2 + coderd/apidoc/swagger.json | 10 ++++- codersdk/workspaceagents.go | 1 + docs/reference/api/schemas.md | 1 + site/src/api/typesGenerated.ts | 3 +- 7 files changed, 72 insertions(+), 21 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index ca8db161f71db..905a6f224062a 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1021,6 +1021,9 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting: continue // This state is handled by the recreation routine. + case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopping: + continue // This state is handled by the stopping routine. + case dc.Status == codersdk.WorkspaceAgentDevcontainerStatusDeleting: continue // This state is handled by the delete routine. @@ -1265,7 +1268,8 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) return } - // Check if the devcontainer is currently starting - if so, we can't delete it. + // NOTE(DanielleMaywood): + // We currently do not support cancelling the startup of a dev container. if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting { api.mu.Unlock() httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ @@ -1275,7 +1279,6 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) return } - // Similarly, if already deleting, don't allow another delete. if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusDeleting { api.mu.Unlock() httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ @@ -1285,45 +1288,66 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) return } - dc.Status = codersdk.WorkspaceAgentDevcontainerStatusDeleting - dc.Error = "" - api.knownDevcontainers[dc.WorkspaceFolder] = dc - api.broadcastUpdatesLocked() - - // Gather the information we need before unlocking. - workspaceFolder := dc.WorkspaceFolder - dcName := dc.Name - var containerID string + var ( + workspaceFolder = dc.WorkspaceFolder + containerID string + subAgentID uuid.UUID + ) if dc.Container != nil { containerID = dc.Container.ID } - proc, hasSubAgent := api.injectedSubAgentProcs[workspaceFolder] - var subAgentID uuid.UUID - if hasSubAgent && proc.agent.ID != uuid.Nil { + if proc, hasSubAgent := api.injectedSubAgentProcs[workspaceFolder]; hasSubAgent && proc.agent.ID != uuid.Nil { subAgentID = proc.agent.ID - // Stop the subagent process context to ensure it stops. proc.stop() } - // Unlock the mutex while we perform potentially slow operations - // (network calls, docker commands) to avoid blocking other operations. api.mu.Unlock() // Stop and remove the container if it exists. if containerID != "" { + api.mu.Lock() + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStopping + dc.Error = "" + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.broadcastUpdatesLocked() + api.mu.Unlock() + if err := api.ccli.Stop(ctx, containerID); err != nil { api.logger.Error(ctx, "unable to stop container", slog.Error(err)) + api.mu.Lock() + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusError + dc.Error = err.Error() + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.broadcastUpdatesLocked() + api.mu.Unlock() + httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ Message: "An internal error occurred stopping the container", Detail: err.Error(), }) return } + } + api.mu.Lock() + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusDeleting + dc.Error = "" + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.broadcastUpdatesLocked() + api.mu.Unlock() + + if containerID != "" { if err := api.ccli.Remove(ctx, containerID); err != nil { api.logger.Error(ctx, "unable to remove container", slog.Error(err)) + api.mu.Lock() + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusError + dc.Error = err.Error() + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.broadcastUpdatesLocked() + api.mu.Unlock() + httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ Message: "An internal error occurred removing the container", Detail: err.Error(), @@ -1338,6 +1362,13 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) if err := client.Delete(ctx, subAgentID); err != nil { api.logger.Error(ctx, "unable to delete agent", slog.Error(err)) + api.mu.Lock() + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusError + dc.Error = err.Error() + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.broadcastUpdatesLocked() + api.mu.Unlock() + httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ Message: "An internal error occurred deleting the agent", Detail: err.Error(), @@ -1347,7 +1378,7 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) } api.mu.Lock() - delete(api.devcontainerNames, dcName) + delete(api.devcontainerNames, dc.Name) delete(api.knownDevcontainers, workspaceFolder) delete(api.devcontainerLogSourceIDs, workspaceFolder) delete(api.recreateSuccessTimes, workspaceFolder) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 01f21b862f361..6420a0e8c13c9 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -45,6 +45,7 @@ import ( // fakeContainerCLI implements the agentcontainers.ContainerCLI interface for // testing. type fakeContainerCLI struct { + mu sync.Mutex containers codersdk.WorkspaceAgentListContainersResponse listErr error arch string @@ -72,6 +73,9 @@ func (f *fakeContainerCLI) ExecAs(ctx context.Context, name, user string, args . } func (f *fakeContainerCLI) Stop(ctx context.Context, name string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.containers.Devcontainers = slice.Filter(f.containers.Devcontainers, func(dc codersdk.WorkspaceAgentDevcontainer) bool { return dc.Container.ID == name }) @@ -84,6 +88,9 @@ func (f *fakeContainerCLI) Stop(ctx context.Context, name string) error { } func (f *fakeContainerCLI) Remove(ctx context.Context, name string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.containers.Containers = slice.Filter(f.containers.Containers, func(container codersdk.WorkspaceAgentContainer) bool { return container.ID == name }) @@ -1147,7 +1154,7 @@ func TestAPI(t *testing.T) { }, devcontainerCLI: &fakeDevcontainerCLI{}, wantStatus: http.StatusConflict, - wantBody: "Devcontainer is stopping", + wantBody: "Devcontainer is already being deleted", }, { name: "Container stop fails", diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 09ba905265379..18361e1d06ff3 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -20344,6 +20344,7 @@ const docTemplate = `{ "stopped", "starting", "stopping", + "deleting", "error" ], "x-enum-varnames": [ @@ -20351,6 +20352,7 @@ const docTemplate = `{ "WorkspaceAgentDevcontainerStatusStopped", "WorkspaceAgentDevcontainerStatusStarting", "WorkspaceAgentDevcontainerStatusStopping", + "WorkspaceAgentDevcontainerStatusDeleting", "WorkspaceAgentDevcontainerStatusError" ] }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index c5f12d0685b31..386d6d14de6f9 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -18695,12 +18695,20 @@ }, "codersdk.WorkspaceAgentDevcontainerStatus": { "type": "string", - "enum": ["running", "stopped", "starting", "stopping", "error"], + "enum": [ + "running", + "stopped", + "starting", + "stopping", + "deleting", + "error" + ], "x-enum-varnames": [ "WorkspaceAgentDevcontainerStatusRunning", "WorkspaceAgentDevcontainerStatusStopped", "WorkspaceAgentDevcontainerStatusStarting", "WorkspaceAgentDevcontainerStatusStopping", + "WorkspaceAgentDevcontainerStatusDeleting", "WorkspaceAgentDevcontainerStatusError" ] }, diff --git a/codersdk/workspaceagents.go b/codersdk/workspaceagents.go index 8e593623ccfd5..4eea34036b48c 100644 --- a/codersdk/workspaceagents.go +++ b/codersdk/workspaceagents.go @@ -401,6 +401,7 @@ const ( WorkspaceAgentDevcontainerStatusRunning WorkspaceAgentDevcontainerStatus = "running" WorkspaceAgentDevcontainerStatusStopped WorkspaceAgentDevcontainerStatus = "stopped" WorkspaceAgentDevcontainerStatusStarting WorkspaceAgentDevcontainerStatus = "starting" + WorkspaceAgentDevcontainerStatusStopping WorkspaceAgentDevcontainerStatus = "stopping" WorkspaceAgentDevcontainerStatusDeleting WorkspaceAgentDevcontainerStatus = "deleting" WorkspaceAgentDevcontainerStatusError WorkspaceAgentDevcontainerStatus = "error" ) diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index b4027961e76df..d3df03705805b 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -10763,6 +10763,7 @@ If the schedule is empty, the user will be updated to use the default schedule.| | `stopped` | | `starting` | | `stopping` | +| `deleting` | | `error` | ## codersdk.WorkspaceAgentHealth diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index df8c12f3661cb..d12ae7ccffcfa 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -6101,6 +6101,7 @@ export interface WorkspaceAgentDevcontainerAgent { // From codersdk/workspaceagents.go export type WorkspaceAgentDevcontainerStatus = + | "deleting" | "error" | "running" | "starting" @@ -6108,7 +6109,7 @@ export type WorkspaceAgentDevcontainerStatus = | "stopping"; export const WorkspaceAgentDevcontainerStatuses: WorkspaceAgentDevcontainerStatus[] = - ["error", "running", "starting", "stopped", "stopping"]; + ["deleting", "error", "running", "starting", "stopped", "stopping"]; // From codersdk/workspaceagents.go export interface WorkspaceAgentHealth { From 973e0ab87ca478979a054be51746854725636d89 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 15 Dec 2025 13:14:02 +0000 Subject: [PATCH 04/10] chore: appease linter --- agent/agentcontainers/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 905a6f224062a..59bb7d24ac989 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1269,7 +1269,7 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) } // NOTE(DanielleMaywood): - // We currently do not support cancelling the startup of a dev container. + // We currently do not support canceling the startup of a dev container. if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting { api.mu.Unlock() httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ From 3b0b84a9f411b3f1e19f0ee2f3ff1656a9bd5511 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 15 Dec 2025 13:59:21 +0000 Subject: [PATCH 05/10] chore: remove unneeded variable --- agent/agentcontainers/api.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 59bb7d24ac989..2cf9213cce5a1 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1289,14 +1289,13 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) } var ( - workspaceFolder = dc.WorkspaceFolder - containerID string - subAgentID uuid.UUID + containerID string + subAgentID uuid.UUID ) if dc.Container != nil { containerID = dc.Container.ID } - if proc, hasSubAgent := api.injectedSubAgentProcs[workspaceFolder]; hasSubAgent && proc.agent.ID != uuid.Nil { + if proc, hasSubAgent := api.injectedSubAgentProcs[dc.WorkspaceFolder]; hasSubAgent && proc.agent.ID != uuid.Nil { subAgentID = proc.agent.ID proc.stop() } @@ -1379,12 +1378,12 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) api.mu.Lock() delete(api.devcontainerNames, dc.Name) - delete(api.knownDevcontainers, workspaceFolder) - delete(api.devcontainerLogSourceIDs, workspaceFolder) - delete(api.recreateSuccessTimes, workspaceFolder) - delete(api.recreateErrorTimes, workspaceFolder) - delete(api.usingWorkspaceFolderName, workspaceFolder) - delete(api.injectedSubAgentProcs, workspaceFolder) + delete(api.knownDevcontainers, dc.WorkspaceFolder) + delete(api.devcontainerLogSourceIDs, dc.WorkspaceFolder) + delete(api.recreateSuccessTimes, dc.WorkspaceFolder) + delete(api.recreateErrorTimes, dc.WorkspaceFolder) + delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) + delete(api.injectedSubAgentProcs, dc.WorkspaceFolder) api.broadcastUpdatesLocked() api.mu.Unlock() From 9ae270b856b8ab5733038debecfe08b4b18076b5 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 15 Dec 2025 16:28:23 +0000 Subject: [PATCH 06/10] refactor: add `IsTransitional` method We don't allow transitioning a Dev Container when it is currently being transitioned --- agent/agentcontainers/api.go | 20 ++++++-------------- agent/agentcontainers/api_test.go | 8 ++++---- codersdk/workspaceagents.go | 11 +++++++++++ 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 2cf9213cce5a1..9e5727721f5e1 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1270,20 +1270,12 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) // NOTE(DanielleMaywood): // We currently do not support canceling the startup of a dev container. - if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting { + if dc.Status.IsTransitional() { api.mu.Unlock() - httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ - Message: "Devcontainer is starting", - Detail: fmt.Sprintf("Devcontainer %q is currently starting and cannot be deleted.", dc.Name), - }) - return - } - 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), + Message: "Unable to delete transitioning Devcontainer", + Detail: fmt.Sprintf("Devcontainer %q is currently %s and cannot be deleted.", dc.Name, dc.Status), }) return } @@ -1412,12 +1404,12 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques httperror.WriteResponseError(ctx, w, err) return } - if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStarting { + if dc.Status.IsTransitional() { api.mu.Unlock() 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", + Detail: fmt.Sprintf("Devcontainer %q is currently %s and cannot be restarted.", dc.Name, dc.Status), }) return } diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 6420a0e8c13c9..671ce9d92c4f7 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -902,7 +902,7 @@ func TestAPI(t *testing.T) { upErr: xerrors.New("devcontainer CLI error"), }, wantStatus: []int{http.StatusAccepted, http.StatusConflict}, - wantBody: []string{"Devcontainer recreation initiated", "Devcontainer recreation already in progress"}, + wantBody: []string{"Devcontainer recreation initiated", "is currently starting and cannot be restarted"}, }, { name: "OK", @@ -925,7 +925,7 @@ func TestAPI(t *testing.T) { }, devcontainerCLI: &fakeDevcontainerCLI{}, wantStatus: []int{http.StatusAccepted, http.StatusConflict}, - wantBody: []string{"Devcontainer recreation initiated", "Devcontainer recreation already in progress"}, + wantBody: []string{"Devcontainer recreation initiated", "is currently starting and cannot be restarted"}, }, } @@ -1131,7 +1131,7 @@ func TestAPI(t *testing.T) { }, devcontainerCLI: &fakeDevcontainerCLI{}, wantStatus: http.StatusConflict, - wantBody: "Devcontainer is starting", + wantBody: "is currently starting and cannot be deleted", }, { name: "Devcontainer is stopping", @@ -1154,7 +1154,7 @@ func TestAPI(t *testing.T) { }, devcontainerCLI: &fakeDevcontainerCLI{}, wantStatus: http.StatusConflict, - wantBody: "Devcontainer is already being deleted", + wantBody: "is currently deleting and cannot be deleted.", }, { name: "Container stop fails", diff --git a/codersdk/workspaceagents.go b/codersdk/workspaceagents.go index 4eea34036b48c..371f9c90083e8 100644 --- a/codersdk/workspaceagents.go +++ b/codersdk/workspaceagents.go @@ -406,6 +406,17 @@ const ( WorkspaceAgentDevcontainerStatusError WorkspaceAgentDevcontainerStatus = "error" ) +func (s WorkspaceAgentDevcontainerStatus) IsTransitional() bool { + switch s { + case WorkspaceAgentDevcontainerStatusStarting, + WorkspaceAgentDevcontainerStatusStopping, + WorkspaceAgentDevcontainerStatusDeleting: + return true + default: + return false + } +} + // WorkspaceAgentDevcontainer defines the location of a devcontainer // configuration in a workspace that is visible to the workspace agent. type WorkspaceAgentDevcontainer struct { From 25b7a7225a0854b38862207769693f5f54c07d1f Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 15 Dec 2025 16:28:23 +0000 Subject: [PATCH 07/10] chore: some comments --- agent/agentcontainers/api.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 9e5727721f5e1..a2f385142a8e2 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -750,6 +750,8 @@ func (api *API) Routes() http.Handler { return r } +// broadcastUpdatesLocked sends the current state to any listening clients. +// This method assumes that api.mu is held. func (api *API) broadcastUpdatesLocked() { // Broadcast state changes to WebSocket listeners. for _, ch := range api.updateChans { @@ -1232,6 +1234,8 @@ func (api *API) getContainers() (codersdk.WorkspaceAgentListContainersResponse, }, nil } +// devcontainerByIDLocked attempts to find a devcontainer by its ID. +// This method assumes that api.mu is held. func (api *API) devcontainerByIDLocked(devcontainerID string) (codersdk.WorkspaceAgentDevcontainer, error) { for _, knownDC := range api.knownDevcontainers { if knownDC.ID.String() == devcontainerID { From 4c0f06a1c1331fc198eaac73856b715f10ef5831 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 16 Dec 2025 15:58:45 +0000 Subject: [PATCH 08/10] chore: IsTransitional -> Transitioning --- agent/agentcontainers/api.go | 4 ++-- codersdk/workspaceagents.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index a2f385142a8e2..736da41996e42 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1274,7 +1274,7 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) // NOTE(DanielleMaywood): // We currently do not support canceling the startup of a dev container. - if dc.Status.IsTransitional() { + if dc.Status.Transitioning() { api.mu.Unlock() httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ @@ -1408,7 +1408,7 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques httperror.WriteResponseError(ctx, w, err) return } - if dc.Status.IsTransitional() { + if dc.Status.Transitioning() { api.mu.Unlock() httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ diff --git a/codersdk/workspaceagents.go b/codersdk/workspaceagents.go index 371f9c90083e8..657493eadf491 100644 --- a/codersdk/workspaceagents.go +++ b/codersdk/workspaceagents.go @@ -406,7 +406,7 @@ const ( WorkspaceAgentDevcontainerStatusError WorkspaceAgentDevcontainerStatus = "error" ) -func (s WorkspaceAgentDevcontainerStatus) IsTransitional() bool { +func (s WorkspaceAgentDevcontainerStatus) Transitioning() bool { switch s { case WorkspaceAgentDevcontainerStatusStarting, WorkspaceAgentDevcontainerStatusStopping, From 476f73992a8f178b8e8a3df70b75461853be93dd Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 16 Dec 2025 15:58:45 +0000 Subject: [PATCH 09/10] chore: modify error messages --- agent/agentcontainers/api.go | 10 +++++----- agent/agentcontainers/api_test.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 736da41996e42..d720bd2692ee4 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1278,7 +1278,7 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) api.mu.Unlock() httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ - Message: "Unable to delete transitioning Devcontainer", + Message: "Unable to delete transitioning devcontainer", Detail: fmt.Sprintf("Devcontainer %q is currently %s and cannot be deleted.", dc.Name, dc.Status), }) return @@ -1318,7 +1318,7 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) api.mu.Unlock() httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ - Message: "An internal error occurred stopping the container", + Message: "An error occurred stopping the container", Detail: err.Error(), }) return @@ -1344,7 +1344,7 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) api.mu.Unlock() httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ - Message: "An internal error occurred removing the container", + Message: "An error occurred removing the container", Detail: err.Error(), }) return @@ -1365,7 +1365,7 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) api.mu.Unlock() httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ - Message: "An internal error occurred deleting the agent", + Message: "An error occurred deleting the agent", Detail: err.Error(), }) return @@ -1412,7 +1412,7 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques api.mu.Unlock() httpapi.Write(ctx, w, http.StatusConflict, codersdk.Response{ - Message: "Unable to recreate transitioning Devcontainer", + Message: "Unable to recreate transitioning devcontainer", Detail: fmt.Sprintf("Devcontainer %q is currently %s and cannot be restarted.", dc.Name, dc.Status), }) return diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 671ce9d92c4f7..54226ee8511f2 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -1178,7 +1178,7 @@ func TestAPI(t *testing.T) { }, devcontainerCLI: &fakeDevcontainerCLI{}, wantStatus: http.StatusInternalServerError, - wantBody: "An internal error occurred stopping the container", + wantBody: "An error occurred stopping the container", }, { name: "Container remove fails", @@ -1202,7 +1202,7 @@ func TestAPI(t *testing.T) { }, devcontainerCLI: &fakeDevcontainerCLI{}, wantStatus: http.StatusInternalServerError, - wantBody: "An internal error occurred removing the container", + wantBody: "An error occurred removing the container", }, { name: "OK with container", From 8754d24a25f7f5b3d2c0e14ca0eea75f0c735366 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 16 Dec 2025 15:58:45 +0000 Subject: [PATCH 10/10] refactor: lock placements, status updates and test helpers --- agent/agentcontainers/api.go | 11 +- agent/agentcontainers/api_test.go | 340 +++++++++++++++++++++++------- 2 files changed, 269 insertions(+), 82 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index d720bd2692ee4..2c6c985ef4615 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1296,17 +1296,14 @@ func (api *API) handleDevcontainerDelete(w http.ResponseWriter, r *http.Request) proc.stop() } + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStopping + dc.Error = "" + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.broadcastUpdatesLocked() api.mu.Unlock() // Stop and remove the container if it exists. if containerID != "" { - api.mu.Lock() - dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStopping - dc.Error = "" - api.knownDevcontainers[dc.WorkspaceFolder] = dc - api.broadcastUpdatesLocked() - api.mu.Unlock() - if err := api.ccli.Stop(ctx, containerID); err != nil { api.logger.Error(ctx, "unable to stop container", slog.Error(err)) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 54226ee8511f2..ebacdd3689937 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -145,6 +145,62 @@ func (f *fakeDevcontainerCLI) Exec(ctx context.Context, _, _ string, cmd string, return f.execErr } +// newFakeDevcontainerCLI returns a `fakeDevcontainerCLI` with the common +// channel-based controls initialized, plus a cleanup function. +func newFakeDevcontainerCLI(t testing.TB, cfg agentcontainers.DevcontainerConfig) (*fakeDevcontainerCLI, func()) { + t.Helper() + + cli := &fakeDevcontainerCLI{ + readConfig: cfg, + execErrC: make(chan func(cmd string, args ...string) error, 1), + readConfigErrC: make(chan func(envs []string) error, 1), + } + + var once sync.Once + cleanup := func() { + once.Do(func() { + close(cli.execErrC) + close(cli.readConfigErrC) + }) + } + + return cli, cleanup +} + +// requireDevcontainerExec ensures the devcontainer CLI Exec behaves like a +// running process: it signals started by closing `started`, then blocks until +// `stop` is closed or ctx is canceled. +func requireDevcontainerExec( + ctx context.Context, + t testing.TB, + cli *fakeDevcontainerCLI, + started chan struct{}, + stop <-chan struct{}, +) { + t.Helper() + + require.NotNil(t, cli, "developer error: devcontainerCLI is nil") + require.NotNil(t, started, "developer error: started channel is nil") + require.NotNil(t, stop, "developer error: stop channel is nil") + + if cli.execErrC == nil { + cli.execErrC = make(chan func(cmd string, args ...string) error, 1) + t.Cleanup(func() { + close(cli.execErrC) + }) + } + + testutil.RequireSend(ctx, t, cli.execErrC, func(_ string, _ ...string) error { + close(started) + select { + case <-stop: + return nil + case <-ctx.Done(): + return ctx.Err() + } + }) +} + func (f *fakeDevcontainerCLI) ReadConfig(ctx context.Context, _, configPath string, envs []string, _ ...agentcontainers.DevcontainerCLIReadConfigOptions) (agentcontainers.DevcontainerConfig, error) { if f.configMap != nil { if v, found := f.configMap[configPath]; found { @@ -261,6 +317,58 @@ func (w *fakeWatcher) sendEventWaitNextCalled(ctx context.Context, event fsnotif w.waitNext(ctx) } +// newFakeSubAgentClient returns a `fakeSubAgentClient` with the common +// channel-based controls initialized, plus a cleanup function. +func newFakeSubAgentClient(t testing.TB, logger slog.Logger) (*fakeSubAgentClient, func()) { + t.Helper() + + sac := &fakeSubAgentClient{ + logger: logger, + agents: make(map[uuid.UUID]agentcontainers.SubAgent), + createErrC: make(chan error, 1), + deleteErrC: make(chan error, 1), + } + + var once sync.Once + cleanup := func() { + once.Do(func() { + close(sac.createErrC) + close(sac.deleteErrC) + }) + } + + return sac, cleanup +} + +func allowSubAgentCreate(ctx context.Context, t testing.TB, sac *fakeSubAgentClient) { + t.Helper() + require.NotNil(t, sac, "developer error: subAgentClient is nil") + require.NotNil(t, sac.createErrC, "developer error: createErrC is nil") + testutil.RequireSend(ctx, t, sac.createErrC, nil) +} + +func allowSubAgentDelete(ctx context.Context, t testing.TB, sac *fakeSubAgentClient) { + t.Helper() + require.NotNil(t, sac, "developer error: subAgentClient is nil") + require.NotNil(t, sac.deleteErrC, "developer error: deleteErrC is nil") + testutil.RequireSend(ctx, t, sac.deleteErrC, nil) +} + +func expectSubAgentInjection( + mCCLI *acmock.MockContainerCLI, + containerID string, + arch string, + coderBin string, +) { + gomock.InOrder( + mCCLI.EXPECT().DetectArchitecture(gomock.Any(), containerID).Return(arch, nil), + mCCLI.EXPECT().ExecAs(gomock.Any(), containerID, "root", "mkdir", "-p", "/.coder-agent").Return(nil, nil), + mCCLI.EXPECT().Copy(gomock.Any(), containerID, coderBin, "/.coder-agent/coder").Return(nil), + mCCLI.EXPECT().ExecAs(gomock.Any(), containerID, "root", "chmod", "0755", "/.coder-agent", "/.coder-agent/coder").Return(nil, nil), + mCCLI.EXPECT().ExecAs(gomock.Any(), containerID, "root", "/bin/sh", "-c", "chown $(id -u):$(id -g) /.coder-agent/coder").Return(nil, nil), + ) +} + // fakeSubAgentClient implements SubAgentClient for testing purposes. type fakeSubAgentClient struct { logger slog.Logger @@ -1068,6 +1176,10 @@ func TestAPI(t *testing.T) { t.Run("Delete", func(t *testing.T) { t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("Dev Container tests are not supported on Windows (this test uses mocks but fails due to Windows paths)") + } + devcontainerID1 := uuid.New() workspaceFolder1 := "/workspace/test1" configPath1 := "/workspace/test1/.devcontainer/devcontainer.json" @@ -1084,13 +1196,14 @@ func TestAPI(t *testing.T) { } tests := []struct { - name string - devcontainerID string - setupDevcontainers []codersdk.WorkspaceAgentDevcontainer - lister *fakeContainerCLI - devcontainerCLI *fakeDevcontainerCLI - wantStatus int - wantBody string + name string + devcontainerID string + setupDevcontainers []codersdk.WorkspaceAgentDevcontainer + lister *fakeContainerCLI + devcontainerCLI *fakeDevcontainerCLI + wantStatus int + wantBody string + wantSubAgentDeleted bool }{ { name: "Missing devcontainer ID", @@ -1104,7 +1217,7 @@ func TestAPI(t *testing.T) { name: "Devcontainer not found", devcontainerID: uuid.NewString(), lister: &fakeContainerCLI{ - arch: "", // Unsupported architecture, don't inject subagent. + arch: "", }, devcontainerCLI: &fakeDevcontainerCLI{}, wantStatus: http.StatusNotFound, @@ -1237,7 +1350,7 @@ func TestAPI(t *testing.T) { WorkspaceFolder: workspaceFolder1, ConfigPath: configPath1, Status: codersdk.WorkspaceAgentDevcontainerStatusStopped, - Container: nil, // No container. + Container: nil, }, }, lister: &fakeContainerCLI{ @@ -1247,38 +1360,138 @@ func TestAPI(t *testing.T) { wantStatus: http.StatusNoContent, wantBody: "", }, + { + name: "OK with container and subagent", + devcontainerID: devcontainerID1.String(), + setupDevcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerID1, + Name: "test-devcontainer-1", + WorkspaceFolder: workspaceFolder1, + ConfigPath: configPath1, + Status: codersdk.WorkspaceAgentDevcontainerStatusStopped, + Container: &devContainer1, + }, + }, + lister: &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{devContainer1}, + }, + arch: "amd64", + }, + devcontainerCLI: &fakeDevcontainerCLI{ + readConfig: agentcontainers.DevcontainerConfig{ + Workspace: agentcontainers.DevcontainerWorkspace{ + WorkspaceFolder: workspaceFolder1, + }, + }, + }, + wantStatus: http.StatusNoContent, + wantBody: "", + wantSubAgentDeleted: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitShort) + var ( + ctx = testutil.Context(t, testutil.WaitShort) + logger = slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + mClock = quartz.NewMock(t) + withSubAgent = tt.wantSubAgentDeleted + ) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) - mClock := quartz.NewMock(t) mClock.Set(time.Now()).MustWait(ctx) tickerTrap := mClock.Trap().TickerFunc("updaterLoop") - // Setup router with the handler under test. - r := chi.NewRouter() + var ( + fakeSAC *fakeSubAgentClient + mCCLI *acmock.MockContainerCLI + containerCLI agentcontainers.ContainerCLI + ) + if withSubAgent { + var cleanupSAC func() + fakeSAC, cleanupSAC = newFakeSubAgentClient(t, logger.Named("fakeSubAgentClient")) + defer cleanupSAC() - api := agentcontainers.NewAPI( - logger, + mCCLI = acmock.NewMockContainerCLI(gomock.NewController(t)) + containerCLI = mCCLI + + coderBin, err := os.Executable() + require.NoError(t, err) + coderBin, err = filepath.EvalSymlinks(coderBin) + require.NoError(t, err) + + mCCLI.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{ + Containers: tt.lister.containers.Containers, + }, nil).AnyTimes() + expectSubAgentInjection(mCCLI, devContainer1.ID, runtime.GOARCH, coderBin) + + mCCLI.EXPECT().Stop(gomock.Any(), devContainer1.ID).Return(nil).Times(1) + mCCLI.EXPECT().Remove(gomock.Any(), devContainer1.ID).Return(nil).Times(1) + } else { + containerCLI = tt.lister + } + + apiOpts := []agentcontainers.Option{ agentcontainers.WithClock(mClock), - agentcontainers.WithContainerCLI(tt.lister), + agentcontainers.WithContainerCLI(containerCLI), agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI), agentcontainers.WithWatcher(watcher.NewNoop()), agentcontainers.WithDevcontainers(tt.setupDevcontainers, nil), - ) + } + if withSubAgent { + apiOpts = append(apiOpts, + agentcontainers.WithSubAgentClient(fakeSAC), + agentcontainers.WithSubAgentURL("test-subagent-url"), + ) + } + + api := agentcontainers.NewAPI(logger, apiOpts...) api.Start() defer api.Close() + + r := chi.NewRouter() r.Mount("/", api.Routes()) + var ( + agentRunningCh chan struct{} + stopAgentCh chan struct{} + ) + if withSubAgent { + agentRunningCh = make(chan struct{}) + stopAgentCh = make(chan struct{}) + defer close(stopAgentCh) + + allowSubAgentCreate(ctx, t, fakeSAC) + + if tt.devcontainerCLI != nil { + requireDevcontainerExec(ctx, t, tt.devcontainerCLI, agentRunningCh, stopAgentCh) + } + } + tickerTrap.MustWait(ctx).MustRelease(ctx) tickerTrap.Close() + if tt.wantSubAgentDeleted { + err := api.RefreshContainers(ctx) + require.NoError(t, err, "refresh containers should not fail") + + select { + case <-agentRunningCh: + case <-ctx.Done(): + t.Fatal("timeout waiting for agent to start") + } + + require.Len(t, fakeSAC.created, 1, "subagent should be created") + require.Empty(t, fakeSAC.deleted, "no subagent should be deleted yet") + + allowSubAgentDelete(ctx, t, fakeSAC) + } + req := httptest.NewRequest(http.MethodDelete, "/devcontainers/"+tt.devcontainerID+"/", nil). WithContext(ctx) rec := httptest.NewRecorder() @@ -1301,6 +1514,11 @@ func TestAPI(t *testing.T) { err := json.NewDecoder(rec.Body).Decode(&resp) require.NoError(t, err, "unmarshal response failed") assert.Empty(t, resp.Devcontainers, "devcontainer should be removed after delete") + + if tt.wantSubAgentDeleted { + require.Len(t, fakeSAC.deleted, 1, "subagent should be deleted") + assert.Equal(t, fakeSAC.created[0].ID, fakeSAC.deleted[0], "correct subagent should be deleted") + } } }) } @@ -1991,25 +2209,17 @@ func TestAPI(t *testing.T) { } var ( - ctx = testutil.Context(t, testutil.WaitMedium) - errTestTermination = xerrors.New("test termination") - logger = slogtest.Make(t, &slogtest.Options{IgnoredErrorIs: []error{errTestTermination}}).Leveled(slog.LevelDebug) - mClock = quartz.NewMock(t) - mCCLI = acmock.NewMockContainerCLI(gomock.NewController(t)) - fakeSAC = &fakeSubAgentClient{ - logger: logger.Named("fakeSubAgentClient"), - createErrC: make(chan error, 1), - deleteErrC: make(chan error, 1), - } - fakeDCCLI = &fakeDevcontainerCLI{ - readConfig: agentcontainers.DevcontainerConfig{ - Workspace: agentcontainers.DevcontainerWorkspace{ - WorkspaceFolder: "/workspaces/coder", - }, + ctx = testutil.Context(t, testutil.WaitMedium) + errTestTermination = xerrors.New("test termination") + logger = slogtest.Make(t, &slogtest.Options{IgnoredErrorIs: []error{errTestTermination}}).Leveled(slog.LevelDebug) + mClock = quartz.NewMock(t) + mCCLI = acmock.NewMockContainerCLI(gomock.NewController(t)) + fakeSAC, cleanupSAC = newFakeSubAgentClient(t, logger.Named("fakeSubAgentClient")) + fakeDCCLI, cleanupDCCLI = newFakeDevcontainerCLI(t, agentcontainers.DevcontainerConfig{ + Workspace: agentcontainers.DevcontainerWorkspace{ + WorkspaceFolder: "/workspaces/coder", }, - execErrC: make(chan func(cmd string, args ...string) error, 1), - readConfigErrC: make(chan func(envs []string) error, 1), - } + }) testContainer = codersdk.WorkspaceAgentContainer{ ID: "test-container-id", @@ -2032,18 +2242,11 @@ func TestAPI(t *testing.T) { mCCLI.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{ Containers: []codersdk.WorkspaceAgentContainer{testContainer}, }, nil).Times(3) // 1 initial call + 2 updates. - gomock.InOrder( - mCCLI.EXPECT().DetectArchitecture(gomock.Any(), "test-container-id").Return(runtime.GOARCH, nil), - mCCLI.EXPECT().ExecAs(gomock.Any(), "test-container-id", "root", "mkdir", "-p", "/.coder-agent").Return(nil, nil), - mCCLI.EXPECT().Copy(gomock.Any(), "test-container-id", coderBin, "/.coder-agent/coder").Return(nil), - mCCLI.EXPECT().ExecAs(gomock.Any(), "test-container-id", "root", "chmod", "0755", "/.coder-agent", "/.coder-agent/coder").Return(nil, nil), - mCCLI.EXPECT().ExecAs(gomock.Any(), "test-container-id", "root", "/bin/sh", "-c", "chown $(id -u):$(id -g) /.coder-agent/coder").Return(nil, nil), - ) + expectSubAgentInjection(mCCLI, "test-container-id", runtime.GOARCH, coderBin) mClock.Set(time.Now()).MustWait(ctx) tickerTrap := mClock.Trap().TickerFunc("updaterLoop") - var closeOnce sync.Once api := agentcontainers.NewAPI(logger, agentcontainers.WithClock(mClock), agentcontainers.WithContainerCLI(mCCLI), @@ -2054,21 +2257,15 @@ func TestAPI(t *testing.T) { agentcontainers.WithManifestInfo("test-user", "test-workspace", "test-parent-agent", "/parent-agent"), ) api.Start() - apiClose := func() { - closeOnce.Do(func() { - // Close before api.Close() defer to avoid deadlock after test. - close(fakeSAC.createErrC) - close(fakeSAC.deleteErrC) - close(fakeDCCLI.execErrC) - close(fakeDCCLI.readConfigErrC) + defer func() { + cleanupSAC() + cleanupDCCLI() - _ = api.Close() - }) - } - defer apiClose() + _ = api.Close() + }() // Allow initial agent creation and injection to succeed. - testutil.RequireSend(ctx, t, fakeSAC.createErrC, nil) + allowSubAgentCreate(ctx, t, fakeSAC) testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=coder") assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace") @@ -2121,13 +2318,7 @@ func TestAPI(t *testing.T) { t.Log("Waiting for agent reinjection...") // Expect the agent to be reinjected. - gomock.InOrder( - mCCLI.EXPECT().DetectArchitecture(gomock.Any(), "test-container-id").Return(runtime.GOARCH, nil), - mCCLI.EXPECT().ExecAs(gomock.Any(), "test-container-id", "root", "mkdir", "-p", "/.coder-agent").Return(nil, nil), - mCCLI.EXPECT().Copy(gomock.Any(), "test-container-id", coderBin, "/.coder-agent/coder").Return(nil), - mCCLI.EXPECT().ExecAs(gomock.Any(), "test-container-id", "root", "chmod", "0755", "/.coder-agent", "/.coder-agent/coder").Return(nil, nil), - mCCLI.EXPECT().ExecAs(gomock.Any(), "test-container-id", "root", "/bin/sh", "-c", "chown $(id -u):$(id -g) /.coder-agent/coder").Return(nil, nil), - ) + expectSubAgentInjection(mCCLI, "test-container-id", runtime.GOARCH, coderBin) // Verify that the agent has started. agentStarted := make(chan struct{}) @@ -2236,7 +2427,12 @@ func TestAPI(t *testing.T) { t.Log("Agent deleted and recreated successfully.") - apiClose() + // Allow API shutdown to delete the currently active agent record. + allowSubAgentDelete(ctx, t, fakeSAC) + + err = api.Close() + require.NoError(t, err) + require.Len(t, fakeSAC.created, 2, "API close should not create more agents") require.Len(t, fakeSAC.deleted, 2, "API close should delete the agent") assert.Equal(t, fakeSAC.created[1].ID, fakeSAC.deleted[1], "the second created agent should be deleted on API close") @@ -3296,12 +3492,8 @@ func TestAPI(t *testing.T) { }, } - fakeSAC := &fakeSubAgentClient{ - logger: slogtest.Make(t, nil).Named("fakeSubAgentClient"), - agents: make(map[uuid.UUID]agentcontainers.SubAgent), - createErrC: make(chan error, 1), - deleteErrC: make(chan error, 1), - } + fakeSAC, cleanupSAC := newFakeSubAgentClient(t, slogtest.Make(t, nil).Named("fakeSubAgentClient")) + defer cleanupSAC() mClock := quartz.NewMock(t) mClock.Set(startTime) @@ -3318,9 +3510,7 @@ func TestAPI(t *testing.T) { ) api.Start() defer func() { - close(fakeSAC.createErrC) - close(fakeSAC.deleteErrC) - api.Close() + _ = api.Close() }() err := api.RefreshContainers(ctx) @@ -3368,7 +3558,7 @@ func TestAPI(t *testing.T) { return nil } testutil.RequireSend(ctx, t, fDCCLI.execErrC, execSubAgent) - testutil.RequireSend(ctx, t, fakeSAC.createErrC, nil) + allowSubAgentCreate(ctx, t, fakeSAC) fWatcher.sendEventWaitNextCalled(ctx, fsnotify.Event{ Name: configPath, @@ -3408,7 +3598,7 @@ func TestAPI(t *testing.T) { t.Log("Phase 3: Change back to ignore=true and test sub agent deletion") fDCCLI.readConfig.Configuration.Customizations.Coder.Ignore = true - testutil.RequireSend(ctx, t, fakeSAC.deleteErrC, nil) + allowSubAgentDelete(ctx, t, fakeSAC) fWatcher.sendEventWaitNextCalled(ctx, fsnotify.Event{ Name: configPath,