From 12868988ea89eafd8e44c41a1d74c561965ce991 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 24 Jun 2025 11:00:52 +0000 Subject: [PATCH 1/5] feat(agent/agentcontainers): implement ignore customization for devcontainers Fixes coder/internal#737 --- agent/agentcontainers/api.go | 166 ++++++++++++++++------ agent/agentcontainers/api_test.go | 167 +++++++++++++++++++++++ agent/agentcontainers/devcontainercli.go | 1 + 3 files changed, 291 insertions(+), 43 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 6b6f391d238f2..ff1c277bfdb3d 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -83,6 +83,7 @@ type API struct { recreateErrorTimes map[string]time.Time // By workspace folder. injectedSubAgentProcs map[string]subAgentProcess // By workspace folder. usingWorkspaceFolderName map[string]bool // By workspace folder. + ignoredDevcontainers map[string]bool // By workspace folder. Tracks three states (true, false and not checked). asyncWg sync.WaitGroup devcontainerLogSourceIDs map[string]uuid.UUID // By workspace folder. @@ -276,6 +277,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API { devcontainerNames: make(map[string]bool), knownDevcontainers: make(map[string]codersdk.WorkspaceAgentDevcontainer), configFileModifiedTimes: make(map[string]time.Time), + ignoredDevcontainers: make(map[string]bool), recreateSuccessTimes: make(map[string]time.Time), recreateErrorTimes: make(map[string]time.Time), scriptLogger: func(uuid.UUID) ScriptLogger { return noopScriptLogger{} }, @@ -804,6 +806,10 @@ func (api *API) getContainers() (codersdk.WorkspaceAgentListContainersResponse, if len(api.knownDevcontainers) > 0 { devcontainers = make([]codersdk.WorkspaceAgentDevcontainer, 0, len(api.knownDevcontainers)) for _, dc := range api.knownDevcontainers { + if api.ignoredDevcontainers[dc.WorkspaceFolder] { + continue + } + // Include the agent if it's running (we're iterating over // copies, so mutating is fine). if proc := api.injectedSubAgentProcs[dc.WorkspaceFolder]; proc.agent.ID != uuid.Nil { @@ -1036,11 +1042,46 @@ func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) { logger.Info(api.ctx, "marking devcontainer as dirty") dc.Dirty = true } + if api.ignoredDevcontainers[dc.WorkspaceFolder] { + logger.Debug(api.ctx, "clearing devcontainer ignored state") + delete(api.ignoredDevcontainers, dc.WorkspaceFolder) // Allow re-reading config. + } api.knownDevcontainers[dc.WorkspaceFolder] = dc } } +// markDevcontainerIgnored marks a devcontainer as ignored. Must not be called +// with the API lock held. +func (api *API) markDevcontainerIgnored(ctx context.Context, dc codersdk.WorkspaceAgentDevcontainer, proc subAgentProcess) subAgentProcess { + logger := api.logger.With( + slog.F("devcontainer_id", dc.ID), + slog.F("devcontainer_name", dc.Name), + slog.F("workspace_folder", dc.WorkspaceFolder), + slog.F("config_path", dc.ConfigPath), + ) + + // We only allow ignore to be set in the root customization layer to + // prevent weird interactions with devcontainer features. + logger.Debug(ctx, "marking devcontainer as ignored") + proc.stop() + if proc.agent.ID != uuid.Nil { + // If we stop the subagent, we also need to delete it. + client := *api.subAgentClient.Load() + if err := client.Delete(ctx, proc.agent.ID); err != nil { + api.logger.Error(ctx, "delete subagent failed", slog.Error(err), slog.F("subagent_id", proc.agent.ID)) + } + } + // Reset agent and containerID to force config re-reading if ignore is toggled. + proc.agent = SubAgent{} + proc.containerID = "" + api.mu.Lock() + api.ignoredDevcontainers[dc.WorkspaceFolder] = true + api.mu.Unlock() + + return proc +} + // cleanupSubAgents removes subagents that are no longer managed by // this agent. This is usually only run at startup to ensure a clean // slate. This method has an internal timeout to prevent blocking @@ -1092,6 +1133,10 @@ func (api *API) cleanupSubAgents(ctx context.Context) error { // This method uses an internal timeout to prevent blocking indefinitely // if something goes wrong with the injection. func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc codersdk.WorkspaceAgentDevcontainer) (err error) { + if api.ignoredDevcontainers[dc.WorkspaceFolder] { + return nil + } + ctx, cancel := context.WithTimeout(ctx, defaultOperationTimeout) defer cancel() @@ -1113,6 +1158,29 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c maybeRecreateSubAgent := false proc, injected := api.injectedSubAgentProcs[dc.WorkspaceFolder] if injected { + if _, ignoreChecked := api.ignoredDevcontainers[dc.WorkspaceFolder]; !ignoreChecked { + // If ignore status has not yet been checked, or cleared by + // modifications to the devcontainer.json, we must read it + // to determine the current status. This can happen while + // the devcontainer subagent is already running or before + // we've had a chance to inject it. + // + // Note, for simplicity, we do not try to optimize to reduce + // ReadConfig calls here. + config, err := api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath, nil) + if err != nil { + return xerrors.Errorf("read devcontainer config: %w", err) + } + + if config.Configuration.Customizations.Coder.Ignore { + api.mu.Unlock() + proc = api.markDevcontainerIgnored(ctx, dc, proc) + api.mu.Lock() + api.injectedSubAgentProcs[dc.WorkspaceFolder] = proc + return nil + } + } + if proc.containerID == container.ID && proc.ctx.Err() == nil { // Same container and running, no need to reinject. return nil @@ -1131,7 +1199,8 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c // Container ID changed or the subagent process is not running, // stop the existing subagent context to replace it. proc.stop() - } else { + } + if proc.agent.OperatingSystem == "" { // Set SubAgent defaults. proc.agent.OperatingSystem = "linux" // Assuming Linux for devcontainers. } @@ -1188,48 +1257,6 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c proc.agent.Architecture = arch } - agentBinaryPath, err := os.Executable() - if err != nil { - return xerrors.Errorf("get agent binary path: %w", err) - } - agentBinaryPath, err = filepath.EvalSymlinks(agentBinaryPath) - if err != nil { - return xerrors.Errorf("resolve agent binary path: %w", err) - } - - // If we scripted this as a `/bin/sh` script, we could reduce these - // steps to one instruction, speeding up the injection process. - // - // Note: We use `path` instead of `filepath` here because we are - // working with Unix-style paths inside the container. - if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "mkdir", "-p", path.Dir(coderPathInsideContainer)); err != nil { - return xerrors.Errorf("create agent directory in container: %w", err) - } - - if err := api.ccli.Copy(ctx, container.ID, agentBinaryPath, coderPathInsideContainer); err != nil { - return xerrors.Errorf("copy agent binary: %w", err) - } - - logger.Info(ctx, "copied agent binary to container") - - // Make sure the agent binary is executable so we can run it (the - // user doesn't matter since we're making it executable for all). - if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "chmod", "0755", path.Dir(coderPathInsideContainer), coderPathInsideContainer); err != nil { - return xerrors.Errorf("set agent binary executable: %w", err) - } - - // Attempt to add CAP_NET_ADMIN to the binary to improve network - // performance (optional, allow to fail). See `bootstrap_linux.sh`. - // TODO(mafredri): Disable for now until we can figure out why this - // causes the following error on some images: - // - // Image: mcr.microsoft.com/devcontainers/base:ubuntu - // Error: /.coder-agent/coder: Operation not permitted - // - // if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "setcap", "cap_net_admin+ep", coderPathInsideContainer); err != nil { - // logger.Warn(ctx, "set CAP_NET_ADMIN on agent binary failed", slog.Error(err)) - // } - subAgentConfig := proc.agent.CloneConfig(dc) if proc.agent.ID == uuid.Nil || maybeRecreateSubAgent { subAgentConfig.Architecture = arch @@ -1246,6 +1273,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c } var ( + ignore bool appsWithPossibleDuplicates []SubAgentApp workspaceFolder = DevcontainerDefaultContainerWorkspaceFolder ) @@ -1269,8 +1297,13 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c return err } + ignore = config.Configuration.Customizations.Coder.Ignore workspaceFolder = config.Workspace.WorkspaceFolder + if ignore { + return nil + } + // NOTE(DanielleMaywood): // We only want to take an agent name specified in the root customization layer. // This restricts the ability for a feature to specify the agent name. We may revisit @@ -1317,6 +1350,11 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) } + if ignore { + proc = api.markDevcontainerIgnored(ctx, dc, proc) + return nil + } + displayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap)) for app, enabled := range displayAppsMap { if enabled { @@ -1349,6 +1387,48 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c subAgentConfig.Directory = workspaceFolder } + agentBinaryPath, err := os.Executable() + if err != nil { + return xerrors.Errorf("get agent binary path: %w", err) + } + agentBinaryPath, err = filepath.EvalSymlinks(agentBinaryPath) + if err != nil { + return xerrors.Errorf("resolve agent binary path: %w", err) + } + + // If we scripted this as a `/bin/sh` script, we could reduce these + // steps to one instruction, speeding up the injection process. + // + // Note: We use `path` instead of `filepath` here because we are + // working with Unix-style paths inside the container. + if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "mkdir", "-p", path.Dir(coderPathInsideContainer)); err != nil { + return xerrors.Errorf("create agent directory in container: %w", err) + } + + if err := api.ccli.Copy(ctx, container.ID, agentBinaryPath, coderPathInsideContainer); err != nil { + return xerrors.Errorf("copy agent binary: %w", err) + } + + logger.Info(ctx, "copied agent binary to container") + + // Make sure the agent binary is executable so we can run it (the + // user doesn't matter since we're making it executable for all). + if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "chmod", "0755", path.Dir(coderPathInsideContainer), coderPathInsideContainer); err != nil { + return xerrors.Errorf("set agent binary executable: %w", err) + } + + // Attempt to add CAP_NET_ADMIN to the binary to improve network + // performance (optional, allow to fail). See `bootstrap_linux.sh`. + // TODO(mafredri): Disable for now until we can figure out why this + // causes the following error on some images: + // + // Image: mcr.microsoft.com/devcontainers/base:ubuntu + // Error: /.coder-agent/coder: Operation not permitted + // + // if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "setcap", "cap_net_admin+ep", coderPathInsideContainer); err != nil { + // logger.Warn(ctx, "set CAP_NET_ADMIN on agent binary failed", slog.Error(err)) + // } + deleteSubAgent := proc.agent.ID != uuid.Nil && maybeRecreateSubAgent && !proc.agent.EqualConfig(subAgentConfig) if deleteSubAgent { logger.Debug(ctx, "deleting existing subagent for recreation", slog.F("agent_id", proc.agent.ID)) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 3ee1c775087c2..7b4bbadc79711 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -2070,6 +2070,173 @@ func TestAPI(t *testing.T) { require.Equal(t, testDir, lastCmd.Dir, "custom directory should be used") require.Equal(t, testEnv, lastCmd.Env, "custom environment should be used") }) + + t.Run("IgnoreCustomization", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + startTime := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) + configPath := "/workspace/project/.devcontainer/devcontainer.json" + + container := codersdk.WorkspaceAgentContainer{ + ID: "container-id", + FriendlyName: "container-name", + Running: true, + CreatedAt: startTime.Add(-1 * time.Hour), + Labels: map[string]string{ + agentcontainers.DevcontainerLocalFolderLabel: "/workspace/project", + agentcontainers.DevcontainerConfigFileLabel: configPath, + }, + } + + fLister := &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{container}, + }, + arch: runtime.GOARCH, + } + + // Start with ignore=true + fDCCLI := &fakeDevcontainerCLI{ + execErrC: make(chan func(string, ...string) error, 1), + readConfig: agentcontainers.DevcontainerConfig{ + Configuration: agentcontainers.DevcontainerConfiguration{ + Customizations: agentcontainers.DevcontainerCustomizations{ + Coder: agentcontainers.CoderCustomization{Ignore: true}, + }, + }, + Workspace: agentcontainers.DevcontainerWorkspace{WorkspaceFolder: "/workspace/project"}, + }, + } + + 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), + } + + mClock := quartz.NewMock(t) + mClock.Set(startTime) + fWatcher := newFakeWatcher(t) + + logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + api := agentcontainers.NewAPI( + logger, + agentcontainers.WithDevcontainerCLI(fDCCLI), + agentcontainers.WithContainerCLI(fLister), + agentcontainers.WithSubAgentClient(fakeSAC), + agentcontainers.WithWatcher(fWatcher), + agentcontainers.WithClock(mClock), + ) + defer func() { + close(fakeSAC.createErrC) + close(fakeSAC.deleteErrC) + api.Close() + }() + + r := chi.NewRouter() + r.Mount("/", api.Routes()) + + t.Log("Phase 1: Test ignore=true filters out devcontainer") + req := httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + var response codersdk.WorkspaceAgentListContainersResponse + err := json.NewDecoder(rec.Body).Decode(&response) + require.NoError(t, err) + + assert.Empty(t, response.Devcontainers, "ignored devcontainer should not be in response when ignore=true") + assert.Len(t, response.Containers, 1, "regular container should still be listed") + + t.Log("Phase 2: Change to ignore=false") + fDCCLI.readConfig.Configuration.Customizations.Coder.Ignore = false + var ( + exitSubAgent = make(chan struct{}) + subAgentExited = make(chan struct{}) + exitSubAgentOnce sync.Once + ) + defer func() { + exitSubAgentOnce.Do(func() { + close(exitSubAgent) + }) + }() + execSubAgent := func(cmd string, args ...string) error { + if len(args) != 1 || args[0] != "agent" { + t.Log("execSubAgent called with unexpected arguments", cmd, args) + return nil + } + defer close(subAgentExited) + select { + case <-exitSubAgent: + case <-ctx.Done(): + return ctx.Err() + } + return nil + } + testutil.RequireSend(ctx, t, fDCCLI.execErrC, execSubAgent) + testutil.RequireSend(ctx, t, fakeSAC.createErrC, nil) + + fWatcher.sendEventWaitNextCalled(ctx, fsnotify.Event{ + Name: configPath, + Op: fsnotify.Write, + }) + + err = api.RefreshContainers(ctx) + require.NoError(t, err) + + req = httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) + rec = httptest.NewRecorder() + r.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + err = json.NewDecoder(rec.Body).Decode(&response) + require.NoError(t, err) + + assert.Len(t, response.Devcontainers, 1, "devcontainer should be in response when ignore=false") + assert.Len(t, response.Containers, 1, "regular container should still be listed") + assert.Equal(t, "/workspace/project", response.Devcontainers[0].WorkspaceFolder) + assert.Len(t, fakeSAC.created, 1, "sub agent should be created when ignore=false") + createdAgentID := fakeSAC.created[0].ID + + t.Log("Phase 2: Done, waiting for sub agent to exit") + exitSubAgentOnce.Do(func() { + close(exitSubAgent) + }) + select { + case <-subAgentExited: + case <-ctx.Done(): + t.Fatal("timeout waiting for sub agent to exit") + } + + 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) + + fWatcher.sendEventWaitNextCalled(ctx, fsnotify.Event{ + Name: configPath, + Op: fsnotify.Write, + }) + + err = api.RefreshContainers(ctx) + require.NoError(t, err) + + req = httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) + rec = httptest.NewRecorder() + r.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + err = json.NewDecoder(rec.Body).Decode(&response) + require.NoError(t, err) + + assert.Empty(t, response.Devcontainers, "devcontainer should be filtered out when ignore=true again") + assert.Len(t, response.Containers, 1, "regular container should still be listed") + assert.Len(t, fakeSAC.deleted, 1, "sub agent should be deleted when ignore=true") + assert.Equal(t, createdAgentID, fakeSAC.deleted[0], "the same sub agent that was created should be deleted") + }) } // mustFindDevcontainerByPath returns the devcontainer with the given workspace diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 850dc84d5ac7d..e49c6900facdb 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -44,6 +44,7 @@ type CoderCustomization struct { DisplayApps map[codersdk.DisplayApp]bool `json:"displayApps,omitempty"` Apps []SubAgentApp `json:"apps,omitempty"` Name string `json:"name,omitempty"` + Ignore bool `json:"ignore,omitempty"` } type DevcontainerWorkspace struct { From e72c98ffa9811acaac3b53d94da85dbc8cf670bf Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 24 Jun 2025 17:36:54 +0000 Subject: [PATCH 2/5] rewrite --- agent/agentcontainers/api.go | 84 +++++++++++++++---------------- agent/agentcontainers/api_test.go | 2 +- 2 files changed, 42 insertions(+), 44 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index ff1c277bfdb3d..be65a764c6eda 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1042,7 +1042,7 @@ func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) { logger.Info(api.ctx, "marking devcontainer as dirty") dc.Dirty = true } - if api.ignoredDevcontainers[dc.WorkspaceFolder] { + if _, ok := api.ignoredDevcontainers[dc.WorkspaceFolder]; ok { logger.Debug(api.ctx, "clearing devcontainer ignored state") delete(api.ignoredDevcontainers, dc.WorkspaceFolder) // Allow re-reading config. } @@ -1051,37 +1051,6 @@ func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) { } } -// markDevcontainerIgnored marks a devcontainer as ignored. Must not be called -// with the API lock held. -func (api *API) markDevcontainerIgnored(ctx context.Context, dc codersdk.WorkspaceAgentDevcontainer, proc subAgentProcess) subAgentProcess { - logger := api.logger.With( - slog.F("devcontainer_id", dc.ID), - slog.F("devcontainer_name", dc.Name), - slog.F("workspace_folder", dc.WorkspaceFolder), - slog.F("config_path", dc.ConfigPath), - ) - - // We only allow ignore to be set in the root customization layer to - // prevent weird interactions with devcontainer features. - logger.Debug(ctx, "marking devcontainer as ignored") - proc.stop() - if proc.agent.ID != uuid.Nil { - // If we stop the subagent, we also need to delete it. - client := *api.subAgentClient.Load() - if err := client.Delete(ctx, proc.agent.ID); err != nil { - api.logger.Error(ctx, "delete subagent failed", slog.Error(err), slog.F("subagent_id", proc.agent.ID)) - } - } - // Reset agent and containerID to force config re-reading if ignore is toggled. - proc.agent = SubAgent{} - proc.containerID = "" - api.mu.Lock() - api.ignoredDevcontainers[dc.WorkspaceFolder] = true - api.mu.Unlock() - - return proc -} - // cleanupSubAgents removes subagents that are no longer managed by // this agent. This is usually only run at startup to ensure a clean // slate. This method has an internal timeout to prevent blocking @@ -1172,11 +1141,24 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c return xerrors.Errorf("read devcontainer config: %w", err) } - if config.Configuration.Customizations.Coder.Ignore { - api.mu.Unlock() - proc = api.markDevcontainerIgnored(ctx, dc, proc) - api.mu.Lock() + ignore := config.Configuration.Customizations.Coder.Ignore + if ignore { + proc.stop() + if proc.agent.ID != uuid.Nil { + // Unlock while doing the delete operation. + api.mu.Unlock() + client := *api.subAgentClient.Load() + if err := client.Delete(ctx, proc.agent.ID); err != nil { + api.mu.Lock() + return xerrors.Errorf("delete subagent: %w", err) + } + api.mu.Lock() + } + // Reset agent and containerID to force config re-reading if ignore is toggled. + proc.agent = SubAgent{} + proc.containerID = "" api.injectedSubAgentProcs[dc.WorkspaceFolder] = proc + api.ignoredDevcontainers[dc.WorkspaceFolder] = ignore return nil } } @@ -1219,7 +1201,11 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c ranSubAgent := false // Clean up if injection fails. + var ignored, setIgnored bool defer func() { + if setIgnored { + api.ignoredDevcontainers[dc.WorkspaceFolder] = ignored + } if !ranSubAgent { proc.stop() if !api.closed { @@ -1273,7 +1259,6 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c } var ( - ignore bool appsWithPossibleDuplicates []SubAgentApp workspaceFolder = DevcontainerDefaultContainerWorkspaceFolder ) @@ -1297,13 +1282,15 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c return err } - ignore = config.Configuration.Customizations.Coder.Ignore - workspaceFolder = config.Workspace.WorkspaceFolder - - if ignore { + // We only allow ignore to be set in the root customization layer to + // prevent weird interactions with devcontainer features. + ignored, setIgnored = config.Configuration.Customizations.Coder.Ignore, true + if ignored { return nil } + workspaceFolder = config.Workspace.WorkspaceFolder + // NOTE(DanielleMaywood): // We only want to take an agent name specified in the root customization layer. // This restricts the ability for a feature to specify the agent name. We may revisit @@ -1350,8 +1337,19 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) } - if ignore { - proc = api.markDevcontainerIgnored(ctx, dc, proc) + if ignored { + proc.stop() + if proc.agent.ID != uuid.Nil { + // If we stop the subagent, we also need to delete it. + client := *api.subAgentClient.Load() + if err := client.Delete(ctx, proc.agent.ID); err != nil { + return xerrors.Errorf("delete subagent: %w", err) + } + } + // Reset agent and containerID to force config re-reading if + // ignore is toggled. + proc.agent = SubAgent{} + proc.containerID = "" return nil } diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 7b4bbadc79711..d92819d589197 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -2234,7 +2234,7 @@ func TestAPI(t *testing.T) { assert.Empty(t, response.Devcontainers, "devcontainer should be filtered out when ignore=true again") assert.Len(t, response.Containers, 1, "regular container should still be listed") - assert.Len(t, fakeSAC.deleted, 1, "sub agent should be deleted when ignore=true") + require.Len(t, fakeSAC.deleted, 1, "sub agent should be deleted when ignore=true") assert.Equal(t, createdAgentID, fakeSAC.deleted[0], "the same sub agent that was created should be deleted") }) } From 2807a9d55683f96f90d3a1069e6571426e12a6bc Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 24 Jun 2025 17:39:09 +0000 Subject: [PATCH 3/5] consistency --- agent/agentcontainers/api.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index be65a764c6eda..6d2c46b961122 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1141,8 +1141,8 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c return xerrors.Errorf("read devcontainer config: %w", err) } - ignore := config.Configuration.Customizations.Coder.Ignore - if ignore { + dcIgnored := config.Configuration.Customizations.Coder.Ignore + if dcIgnored { proc.stop() if proc.agent.ID != uuid.Nil { // Unlock while doing the delete operation. @@ -1158,7 +1158,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c proc.agent = SubAgent{} proc.containerID = "" api.injectedSubAgentProcs[dc.WorkspaceFolder] = proc - api.ignoredDevcontainers[dc.WorkspaceFolder] = ignore + api.ignoredDevcontainers[dc.WorkspaceFolder] = dcIgnored return nil } } @@ -1201,10 +1201,10 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c ranSubAgent := false // Clean up if injection fails. - var ignored, setIgnored bool + var dcIgnored, setDCIgnored bool defer func() { - if setIgnored { - api.ignoredDevcontainers[dc.WorkspaceFolder] = ignored + if setDCIgnored { + api.ignoredDevcontainers[dc.WorkspaceFolder] = dcIgnored } if !ranSubAgent { proc.stop() @@ -1284,8 +1284,8 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c // We only allow ignore to be set in the root customization layer to // prevent weird interactions with devcontainer features. - ignored, setIgnored = config.Configuration.Customizations.Coder.Ignore, true - if ignored { + dcIgnored, setDCIgnored = config.Configuration.Customizations.Coder.Ignore, true + if dcIgnored { return nil } @@ -1337,7 +1337,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) } - if ignored { + if dcIgnored { proc.stop() if proc.agent.ID != uuid.Nil { // If we stop the subagent, we also need to delete it. From 5f1d7fbcfe8b7339ad88de1f6e78a7448fca3667 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 24 Jun 2025 18:10:29 +0000 Subject: [PATCH 4/5] fix test --- coderd/workspaceagents_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index b3fb53c228ef8..3154fb32bb3d2 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1432,6 +1432,7 @@ func TestWorkspaceAgentRecreateDevcontainer(t *testing.T) { }, nil).AnyTimes() // DetectArchitecture always returns "" for this test to disable agent injection. mccli.EXPECT().DetectArchitecture(gomock.Any(), devContainer.ID).Return("", nil).AnyTimes() + mdccli.EXPECT().ReadConfig(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return(agentcontainers.DevcontainerConfig{}, nil).Times(1) mdccli.EXPECT().Up(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return("someid", nil).Times(1) return 0 }, From f01711aaa1bb24ae173aaca9ee334415e7160f57 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 24 Jun 2025 18:37:21 +0000 Subject: [PATCH 5/5] nowindows --- agent/agentcontainers/api_test.go | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index d92819d589197..b6bae46c835c9 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -2074,6 +2074,10 @@ func TestAPI(t *testing.T) { t.Run("IgnoreCustomization", 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)") + } + ctx := testutil.Context(t, testutil.WaitShort) startTime := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) @@ -2188,6 +2192,16 @@ func TestAPI(t *testing.T) { err = api.RefreshContainers(ctx) require.NoError(t, err) + t.Log("Phase 2: Cont, waiting for sub agent to exit") + exitSubAgentOnce.Do(func() { + close(exitSubAgent) + }) + select { + case <-subAgentExited: + case <-ctx.Done(): + t.Fatal("timeout waiting for sub agent to exit") + } + req = httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) rec = httptest.NewRecorder() r.ServeHTTP(rec, req) @@ -2199,19 +2213,9 @@ func TestAPI(t *testing.T) { assert.Len(t, response.Devcontainers, 1, "devcontainer should be in response when ignore=false") assert.Len(t, response.Containers, 1, "regular container should still be listed") assert.Equal(t, "/workspace/project", response.Devcontainers[0].WorkspaceFolder) - assert.Len(t, fakeSAC.created, 1, "sub agent should be created when ignore=false") + require.Len(t, fakeSAC.created, 1, "sub agent should be created when ignore=false") createdAgentID := fakeSAC.created[0].ID - t.Log("Phase 2: Done, waiting for sub agent to exit") - exitSubAgentOnce.Do(func() { - close(exitSubAgent) - }) - select { - case <-subAgentExited: - case <-ctx.Done(): - t.Fatal("timeout waiting for sub agent to exit") - } - 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)