refactor(agent): update agentcontainers api initialization (#17600)

There were too many ways to configure the agentcontainers API resulting
in inconsistent behavior or features not being enabled. This refactor
introduces a control flag for enabling or disabling the containers API.
When disabled, all implementations are no-op and explicit endpoint
behaviors are defined. When enabled, concrete implementations are used
by default but can be overridden by passing options.

agent/agent.go

@@ -89,9 +89,9 @@ type Options struct {
 	ServiceBannerRefreshInterval time.Duration
 	BlockFileTransfer            bool
 	Execer                       agentexec.Execer
-	ContainerLister              agentcontainers.Lister
 
 	ExperimentalDevcontainersEnabled bool
+	ContainerAPIOptions              []agentcontainers.Option // Enable ExperimentalDevcontainersEnabled for these to be effective.
 }
 
 type Client interface {
@@ -154,9 +154,6 @@ func New(options Options) Agent {
 	if options.Execer == nil {
 		options.Execer = agentexec.DefaultExecer
 	}
-	if options.ContainerLister == nil {
-		options.ContainerLister = agentcontainers.NoopLister{}
-	}
 
 	hardCtx, hardCancel := context.WithCancel(context.Background())
 	gracefulCtx, gracefulCancel := context.WithCancel(hardCtx)
@@ -192,9 +189,9 @@ func New(options Options) Agent {
 		prometheusRegistry: prometheusRegistry,
 		metrics:            newAgentMetrics(prometheusRegistry),
 		execer:             options.Execer,
-		lister:             options.ContainerLister,
 
 		experimentalDevcontainersEnabled: options.ExperimentalDevcontainersEnabled,
+		containerAPIOptions:              options.ContainerAPIOptions,
 	}
 	// Initially, we have a closed channel, reflecting the fact that we are not initially connected.
 	// Each time we connect we replace the channel (while holding the closeMutex) with a new one
@@ -274,9 +271,10 @@ type agent struct {
 	// labeled in Coder with the agent + workspace.
 	metrics *agentMetrics
 	execer  agentexec.Execer
-	lister  agentcontainers.Lister
 
 	experimentalDevcontainersEnabled bool
+	containerAPIOptions              []agentcontainers.Option
+	containerAPI                     atomic.Pointer[agentcontainers.API] // Set by apiHandler.
 }
 
 func (a *agent) TailnetConn() *tailnet.Conn {
@@ -1170,6 +1168,12 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
 				}
 				a.metrics.startupScriptSeconds.WithLabelValues(label).Set(dur)
 				a.scriptRunner.StartCron()
+				if containerAPI := a.containerAPI.Load(); containerAPI != nil {
+					// Inform the container API that the agent is ready.
+					// This allows us to start watching for changes to
+					// the devcontainer configuration files.
+					containerAPI.SignalReady()
+				}
 			})
 			if err != nil {
 				return xerrors.Errorf("track conn goroutine: %w", err)

agent/agentcontainers/api.go

@@ -39,6 +39,7 @@ type API struct {
 	watcher watcher.Watcher
 
 	cacheDuration time.Duration
+	execer        agentexec.Execer
 	cl            Lister
 	dccli         DevcontainerCLI
 	clock         quartz.Clock
@@ -56,14 +57,6 @@ type API struct {
 // Option is a functional option for API.
 type Option func(*API)
 
-// WithLister sets the agentcontainers.Lister implementation to use.
-// The default implementation uses the Docker CLI to list containers.
-func WithLister(cl Lister) Option {
-	return func(api *API) {
-		api.cl = cl
-	}
-}
-
 // WithClock sets the quartz.Clock implementation to use.
 // This is primarily used for testing to control time.
 func WithClock(clock quartz.Clock) Option {
@@ -72,6 +65,21 @@ func WithClock(clock quartz.Clock) Option {
 	}
 }
 
+// WithExecer sets the agentexec.Execer implementation to use.
+func WithExecer(execer agentexec.Execer) Option {
+	return func(api *API) {
+		api.execer = execer
+	}
+}
+
+// WithLister sets the agentcontainers.Lister implementation to use.
+// The default implementation uses the Docker CLI to list containers.
+func WithLister(cl Lister) Option {
+	return func(api *API) {
+		api.cl = cl
+	}
+}
+
 // WithDevcontainerCLI sets the DevcontainerCLI implementation to use.
 // This can be used in tests to modify @devcontainer/cli behavior.
 func WithDevcontainerCLI(dccli DevcontainerCLI) Option {
@@ -113,6 +121,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API {
 		done:                    make(chan struct{}),
 		logger:                  logger,
 		clock:                   quartz.NewReal(),
+		execer:                  agentexec.DefaultExecer,
 		cacheDuration:           defaultGetContainersCacheDuration,
 		lockCh:                  make(chan struct{}, 1),
 		devcontainerNames:       make(map[string]struct{}),
@@ -123,30 +132,46 @@ func NewAPI(logger slog.Logger, options ...Option) *API {
 		opt(api)
 	}
 	if api.cl == nil {
-		api.cl = &DockerCLILister{}
+		api.cl = NewDocker(api.execer)
 	}
 	if api.dccli == nil {
-		api.dccli = NewDevcontainerCLI(logger.Named("devcontainer-cli"), agentexec.DefaultExecer)
+		api.dccli = NewDevcontainerCLI(logger.Named("devcontainer-cli"), api.execer)
 	}
 	if api.watcher == nil {
-		api.watcher = watcher.NewNoop()
+		var err error
-	}
+		api.watcher, err = watcher.NewFSNotify()
-
+		if err != nil {
-	// Make sure we watch the devcontainer config files for changes.
+			logger.Error(ctx, "create file watcher service failed", slog.Error(err))
-	for _, devcontainer := range api.knownDevcontainers {
+			api.watcher = watcher.NewNoop()
-		if devcontainer.ConfigPath != "" {
-			if err := api.watcher.Add(devcontainer.ConfigPath); err != nil {
-				api.logger.Error(ctx, "watch devcontainer config file failed", slog.Error(err), slog.F("file", devcontainer.ConfigPath))
-			}
 		}
 	}
 
-	go api.start()
+	go api.loop()
 
 	return api
 }
 
-func (api *API) start() {
+// SignalReady signals the API that we are ready to begin watching for
+// file changes. This is used to prime the cache with the current list
+// of containers and to start watching the devcontainer config files for
+// changes. It should be called after the agent ready.
+func (api *API) SignalReady() {
+	// Prime the cache with the current list of containers.
+	_, _ = api.cl.List(api.ctx)
+
+	// Make sure we watch the devcontainer config files for changes.
+	for _, devcontainer := range api.knownDevcontainers {
+		if devcontainer.ConfigPath == "" {
+			continue
+		}
+
+		if err := api.watcher.Add(devcontainer.ConfigPath); err != nil {
+			api.logger.Error(api.ctx, "watch devcontainer config file failed", slog.Error(err), slog.F("file", devcontainer.ConfigPath))
+		}
+	}
+}
+
+func (api *API) loop() {
 	defer close(api.done)
 
 	for {
@@ -187,9 +212,11 @@ func (api *API) start() {
 // Routes returns the HTTP handler for container-related routes.
 func (api *API) Routes() http.Handler {
 	r := chi.NewRouter()
+
 	r.Get("/", api.handleList)
 	r.Get("/devcontainers", api.handleListDevcontainers)
 	r.Post("/{id}/recreate", api.handleRecreate)
+
 	return r
 }
 

agent/agentcontainers/api_test.go

@@ -18,6 +18,7 @@ import (
 	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/slogtest"
 	"github.com/coder/coder/v2/agent/agentcontainers"
+	"github.com/coder/coder/v2/agent/agentcontainers/watcher"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/testutil"
 	"github.com/coder/quartz"
@@ -253,6 +254,7 @@ func TestAPI(t *testing.T) {
 					logger,
 					agentcontainers.WithLister(tt.lister),
 					agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI),
+					agentcontainers.WithWatcher(watcher.NewNoop()),
 				)
 				defer api.Close()
 				r.Mount("/", api.Routes())
@@ -558,6 +560,7 @@ func TestAPI(t *testing.T) {
 				r := chi.NewRouter()
 				apiOptions := []agentcontainers.Option{
 					agentcontainers.WithLister(tt.lister),
+					agentcontainers.WithWatcher(watcher.NewNoop()),
 				}
 
 				if len(tt.knownDevcontainers) > 0 {
@@ -631,6 +634,8 @@ func TestAPI(t *testing.T) {
 		)
 		defer api.Close()
 
+		api.SignalReady()
+
 		r := chi.NewRouter()
 		r.Mount("/", api.Routes())
 

agent/api.go

@@ -37,10 +37,10 @@ func (a *agent) apiHandler() (http.Handler, func() error) {
 		cacheDuration: cacheDuration,
 	}
 
-	containerAPIOpts := []agentcontainers.Option{
-		agentcontainers.WithLister(a.lister),
-	}
 	if a.experimentalDevcontainersEnabled {
+		containerAPIOpts := []agentcontainers.Option{
+			agentcontainers.WithExecer(a.execer),
+		}
 		manifest := a.manifest.Load()
 		if manifest != nil && len(manifest.Devcontainers) > 0 {
 			containerAPIOpts = append(
@@ -48,12 +48,24 @@ func (a *agent) apiHandler() (http.Handler, func() error) {
 				agentcontainers.WithDevcontainers(manifest.Devcontainers),
 			)
 		}
+
+		// Append after to allow the agent options to override the default options.
+		containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...)
+
+		containerAPI := agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...)
+		r.Mount("/api/v0/containers", containerAPI.Routes())
+		a.containerAPI.Store(containerAPI)
+	} else {
+		r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) {
+			httpapi.Write(r.Context(), w, http.StatusForbidden, codersdk.Response{
+				Message: "The agent dev containers feature is experimental and not enabled by default.",
+				Detail:  "To enable this feature, set CODER_AGENT_DEVCONTAINERS_ENABLE=true in your template.",
+			})
+		})
 	}
-	containerAPI := agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...)
 
 	promHandler := PrometheusMetricsHandler(a.prometheusRegistry, a.logger)
 
-	r.Mount("/api/v0/containers", containerAPI.Routes())
 	r.Get("/api/v0/listening-ports", lp.handler)
 	r.Get("/api/v0/netcheck", a.HandleNetcheck)
 	r.Post("/api/v0/list-directory", a.HandleLS)
@@ -64,7 +76,10 @@ func (a *agent) apiHandler() (http.Handler, func() error) {
 	r.Get("/debug/prometheus", promHandler.ServeHTTP)
 
 	return r, func() error {
-		return containerAPI.Close()
+		if containerAPI := a.containerAPI.Load(); containerAPI != nil {
+			return containerAPI.Close()
+		}
+		return nil
 	}
 }
 

cli/agent.go

@@ -26,7 +26,6 @@ import (
 	"cdr.dev/slog/sloggers/slogjson"
 	"cdr.dev/slog/sloggers/slogstackdriver"
 	"github.com/coder/coder/v2/agent"
-	"github.com/coder/coder/v2/agent/agentcontainers"
 	"github.com/coder/coder/v2/agent/agentexec"
 	"github.com/coder/coder/v2/agent/agentssh"
 	"github.com/coder/coder/v2/agent/reaper"
@@ -319,13 +318,10 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
 				return xerrors.Errorf("create agent execer: %w", err)
 			}
 
-			var containerLister agentcontainers.Lister
+			if experimentalDevcontainersEnabled {
-			if !experimentalDevcontainersEnabled {
-				logger.Info(ctx, "agent devcontainer detection not enabled")
-				containerLister = &agentcontainers.NoopLister{}
-			} else {
 				logger.Info(ctx, "agent devcontainer detection enabled")
-				containerLister = agentcontainers.NewDocker(execer)
+			} else {
+				logger.Info(ctx, "agent devcontainer detection not enabled")
 			}
 
 			agnt := agent.New(agent.Options{
@@ -354,7 +350,6 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
 				PrometheusRegistry: prometheusRegistry,
 				BlockFileTransfer:  blockFileTransfer,
 				Execer:             execer,
-				ContainerLister:    containerLister,
 
 				ExperimentalDevcontainersEnabled: experimentalDevcontainersEnabled,
 			})

cli/exp_rpty_test.go

@@ -9,7 +9,6 @@ import (
 	"github.com/ory/dockertest/v3/docker"
 
 	"github.com/coder/coder/v2/agent"
-	"github.com/coder/coder/v2/agent/agentcontainers"
 	"github.com/coder/coder/v2/agent/agenttest"
 	"github.com/coder/coder/v2/cli/clitest"
 	"github.com/coder/coder/v2/coderd/coderdtest"
@@ -112,7 +111,6 @@ func TestExpRpty(t *testing.T) {
 
 		_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
 			o.ExperimentalDevcontainersEnabled = true
-			o.ContainerLister = agentcontainers.NewDocker(o.Execer)
 		})
 		_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
 

cli/open_test.go

@@ -14,6 +14,7 @@ import (
 	"go.uber.org/mock/gomock"
 
 	"github.com/coder/coder/v2/agent"
+	"github.com/coder/coder/v2/agent/agentcontainers"
 	"github.com/coder/coder/v2/agent/agentcontainers/acmock"
 	"github.com/coder/coder/v2/agent/agenttest"
 	"github.com/coder/coder/v2/cli/clitest"
@@ -335,7 +336,8 @@ func TestOpenVSCodeDevContainer(t *testing.T) {
 	})
 
 	_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
-		o.ContainerLister = mcl
+		o.ExperimentalDevcontainersEnabled = true
+		o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl))
 	})
 	_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
 
@@ -508,7 +510,8 @@ func TestOpenVSCodeDevContainer_NoAgentDirectory(t *testing.T) {
 	})
 
 	_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
-		o.ContainerLister = mcl
+		o.ExperimentalDevcontainersEnabled = true
+		o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl))
 	})
 	_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
 

cli/ssh.go

@@ -299,8 +299,6 @@ func (r *RootCmd) ssh() *serpent.Command {
 				}
 				if len(cts.Containers) == 0 {
 					cliui.Info(inv.Stderr, "No containers found!")
-					cliui.Info(inv.Stderr, "Tip: Agent container integration is experimental and not enabled by default.")
-					cliui.Info(inv.Stderr, "     To enable it, set CODER_AGENT_DEVCONTAINERS_ENABLE=true in your template.")
 					return nil
 				}
 				var found bool

cli/ssh_test.go

@@ -2029,7 +2029,6 @@ func TestSSH_Container(t *testing.T) {
 
 		_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
 			o.ExperimentalDevcontainersEnabled = true
-			o.ContainerLister = agentcontainers.NewDocker(o.Execer)
 		})
 		_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
 
@@ -2058,7 +2057,7 @@ func TestSSH_Container(t *testing.T) {
 		mLister := acmock.NewMockLister(ctrl)
 		_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
 			o.ExperimentalDevcontainersEnabled = true
-			o.ContainerLister = mLister
+			o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mLister))
 		})
 		_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
 
@@ -2097,16 +2096,9 @@ func TestSSH_Container(t *testing.T) {
 
 		inv, root := clitest.New(t, "ssh", workspace.Name, "-c", uuid.NewString())
 		clitest.SetupConfig(t, client, root)
-		ptty := ptytest.New(t).Attach(inv)
 
-		cmdDone := tGo(t, func() {
+		err := inv.WithContext(ctx).Run()
-			err := inv.WithContext(ctx).Run()
+		require.ErrorContains(t, err, "The agent dev containers feature is experimental and not enabled by default.")
-			assert.NoError(t, err)
-		})
-
-		ptty.ExpectMatch("No containers found!")
-		ptty.ExpectMatch("Tip: Agent container integration is experimental and not enabled by default.")
-		<-cmdDone
 	})
 }
 

coderd/workspaceagents.go

@@ -848,6 +848,11 @@ func (api *API) workspaceAgentListContainers(rw http.ResponseWriter, r *http.Req
 			})
 			return
 		}
+		// If the agent returns a codersdk.Error, we can return that directly.
+		if cerr, ok := codersdk.AsError(err); ok {
+			httpapi.Write(ctx, rw, cerr.StatusCode(), cerr.Response)
+			return
+		}
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Internal error fetching containers.",
 			Detail:  err.Error(),

coderd/workspaceagents_test.go

@@ -35,7 +35,6 @@ import (
 	"github.com/coder/coder/v2/agent"
 	"github.com/coder/coder/v2/agent/agentcontainers"
 	"github.com/coder/coder/v2/agent/agentcontainers/acmock"
-	"github.com/coder/coder/v2/agent/agentexec"
 	"github.com/coder/coder/v2/agent/agenttest"
 	agentproto "github.com/coder/coder/v2/agent/proto"
 	"github.com/coder/coder/v2/coderd/coderdtest"
@@ -1171,8 +1170,8 @@ func TestWorkspaceAgentContainers(t *testing.T) {
 		}).WithAgent(func(agents []*proto.Agent) []*proto.Agent {
 			return agents
 		}).Do()
-		_ = agenttest.New(t, client.URL, r.AgentToken, func(opts *agent.Options) {
+		_ = agenttest.New(t, client.URL, r.AgentToken, func(o *agent.Options) {
-			opts.ContainerLister = agentcontainers.NewDocker(agentexec.DefaultExecer)
+			o.ExperimentalDevcontainersEnabled = true
 		})
 		resources := coderdtest.NewWorkspaceAgentWaiter(t, client, r.Workspace.ID).Wait()
 		require.Len(t, resources, 1, "expected one resource")
@@ -1273,8 +1272,9 @@ func TestWorkspaceAgentContainers(t *testing.T) {
 				}).WithAgent(func(agents []*proto.Agent) []*proto.Agent {
 					return agents
 				}).Do()
-				_ = agenttest.New(t, client.URL, r.AgentToken, func(opts *agent.Options) {
+				_ = agenttest.New(t, client.URL, r.AgentToken, func(o *agent.Options) {
-					opts.ContainerLister = mcl
+					o.ExperimentalDevcontainersEnabled = true
+					o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl))
 				})
 				resources := coderdtest.NewWorkspaceAgentWaiter(t, client, r.Workspace.ID).Wait()
 				require.Len(t, resources, 1, "expected one resource")

site/src/api/api.ts

@@ -2447,11 +2447,20 @@ class ApiMethods {
 			labels?.map((label) => ["label", label]),
 		);
 
-		const res =
+		try {
-			await this.axios.get<TypesGen.WorkspaceAgentListContainersResponse>(
+			const res =
-				`/api/v2/workspaceagents/${agentId}/containers?${params.toString()}`,
+				await this.axios.get<TypesGen.WorkspaceAgentListContainersResponse>(
-			);
+					`/api/v2/workspaceagents/${agentId}/containers?${params.toString()}`,
-		return res.data;
+				);
+			return res.data;
+		} catch (err) {
+			// If the error is a 403, it means that experimental
+			// containers are not enabled on the agent.
+			if (isAxiosError(err) && err.response?.status === 403) {
+				return { containers: [] };
+			}
+			throw err;
+		}
 	};
 
 	getInboxNotifications = async (startingBeforeId?: string) => {