From 978c8719101ddfd44b828c7de6a32e998d5a46d4 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 20 Jun 2025 14:49:19 +0000 Subject: [PATCH 01/23] fix(agent): start devcontainers through agentcontainers package Fixes https://github.com/coder/internal/issues/706 Context for the implementation here https://github.com/coder/internal/issues/706#issuecomment-2990490282 Synchronously starts dev containers defined in terraform with our `DevcontainerCLI` abstraction, instead of piggybacking off of our `agentscripts` package. This gives us more control over logs, instead of being reliant on packages which may or may not exist in the user-provided image. --- agent/agent.go | 15 +- agent/agentcontainers/api.go | 18 +- agent/agentcontainers/devcontainer.go | 57 ----- agent/agentcontainers/devcontainer_test.go | 274 --------------------- agent/agentcontainers/devcontainercli.go | 60 ++--- agent/agentscripts/agentscripts.go | 38 +-- agent/agentscripts/agentscripts_test.go | 26 +- 7 files changed, 64 insertions(+), 424 deletions(-) delete mode 100644 agent/agentcontainers/devcontainer_test.go diff --git a/agent/agent.go b/agent/agent.go index e142f8662f641..0033e7750628a 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1145,13 +1145,6 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, scripts = manifest.Scripts scriptRunnerOpts []agentscripts.InitOption ) - if a.experimentalDevcontainersEnabled { - var dcScripts []codersdk.WorkspaceAgentScript - scripts, dcScripts = agentcontainers.ExtractAndInitializeDevcontainerScripts(manifest.Devcontainers, scripts) - // See ExtractAndInitializeDevcontainerScripts for motivation - // behind running dcScripts as post start scripts. - scriptRunnerOpts = append(scriptRunnerOpts, agentscripts.WithPostStartScripts(dcScripts...)) - } err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted, scriptRunnerOpts...) if err != nil { return xerrors.Errorf("init script runner: %w", err) @@ -1169,7 +1162,13 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // finished (both start and post start). For instance, an // autostarted devcontainer will be included in this time. err := a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecuteStartScripts) - err = errors.Join(err, a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecutePostStartScripts)) + + if cAPI := a.containerAPI.Load(); cAPI != nil { + for _, dc := range manifest.Devcontainers { + err = errors.Join(err, cAPI.CreateDevcontainer(dc)) + } + } + dur := time.Since(start).Seconds() if err != nil { a.logger.Warn(ctx, "startup script(s) failed", slog.Error(err)) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index ddf98e38bdb48..d5e8a10001d0a 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -794,6 +794,17 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques }) } +func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer) error { + api.mu.Lock() + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting + dc.Container = nil + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.asyncWg.Add(1) + api.mu.Unlock() + + return api.recreateDevcontainer(dc, dc.ConfigPath) +} + // recreateDevcontainer should run in its own goroutine and is responsible for // recreating a devcontainer based on the provided devcontainer configuration. // It updates the devcontainer status and logs the process. The configPath is @@ -801,7 +812,7 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques // has a different config file than the one stored in the devcontainer state. // The devcontainer state must be set to starting and the asyncWg must be // incremented before calling this function. -func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string) { +func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string) error { defer api.asyncWg.Done() var ( @@ -857,7 +868,7 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con api.knownDevcontainers[dc.WorkspaceFolder] = dc api.recreateErrorTimes[dc.WorkspaceFolder] = api.clock.Now("agentcontainers", "recreate", "errorTimes") api.mu.Unlock() - return + return err } logger.Info(ctx, "devcontainer recreated successfully") @@ -884,7 +895,10 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con // devcontainer state after recreation. if err := api.RefreshContainers(ctx); err != nil { logger.Error(ctx, "failed to trigger immediate refresh after devcontainer recreation", slog.Error(err)) + return err } + + return nil } // markDevcontainerDirty finds the devcontainer with the given config file path diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index f13963d7b63d7..9ba77e7121b9b 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -2,10 +2,8 @@ package agentcontainers import ( "context" - "fmt" "os" "path/filepath" - "strings" "cdr.dev/slog" "github.com/coder/coder/v2/codersdk" @@ -22,61 +20,6 @@ const ( DevcontainerDefaultContainerWorkspaceFolder = "/workspaces" ) -const devcontainerUpScriptTemplate = ` -if ! which devcontainer > /dev/null 2>&1; then - echo "ERROR: Unable to start devcontainer, @devcontainers/cli is not installed or not found in \$PATH." 1>&2 - echo "Please install @devcontainers/cli by running \"npm install -g @devcontainers/cli\" or by using the \"devcontainers-cli\" Coder module." 1>&2 - exit 1 -fi -devcontainer up %s -` - -// ExtractAndInitializeDevcontainerScripts extracts devcontainer scripts from -// the given scripts and devcontainers. The devcontainer scripts are removed -// from the returned scripts so that they can be run separately. -// -// Dev Containers have an inherent dependency on start scripts, since they -// initialize the workspace (e.g. git clone, npm install, etc). This is -// important if e.g. a Coder module to install @devcontainer/cli is used. -func ExtractAndInitializeDevcontainerScripts( - devcontainers []codersdk.WorkspaceAgentDevcontainer, - scripts []codersdk.WorkspaceAgentScript, -) (filteredScripts []codersdk.WorkspaceAgentScript, devcontainerScripts []codersdk.WorkspaceAgentScript) { -ScriptLoop: - for _, script := range scripts { - for _, dc := range devcontainers { - // The devcontainer scripts match the devcontainer ID for - // identification. - if script.ID == dc.ID { - devcontainerScripts = append(devcontainerScripts, devcontainerStartupScript(dc, script)) - continue ScriptLoop - } - } - - filteredScripts = append(filteredScripts, script) - } - - return filteredScripts, devcontainerScripts -} - -func devcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript) codersdk.WorkspaceAgentScript { - args := []string{ - "--log-format json", - fmt.Sprintf("--workspace-folder %q", dc.WorkspaceFolder), - } - if dc.ConfigPath != "" { - args = append(args, fmt.Sprintf("--config %q", dc.ConfigPath)) - } - cmd := fmt.Sprintf(devcontainerUpScriptTemplate, strings.Join(args, " ")) - // Force the script to run in /bin/sh, since some shells (e.g. fish) - // don't support the script. - script.Script = fmt.Sprintf("/bin/sh -c '%s'", cmd) - // Disable RunOnStart, scripts have this set so that when devcontainers - // have not been enabled, a warning will be surfaced in the agent logs. - script.RunOnStart = false - return script -} - // ExpandAllDevcontainerPaths expands all devcontainer paths in the given // devcontainers. This is required by the devcontainer CLI, which requires // absolute paths for the workspace folder and config path. diff --git a/agent/agentcontainers/devcontainer_test.go b/agent/agentcontainers/devcontainer_test.go deleted file mode 100644 index b20c943175821..0000000000000 --- a/agent/agentcontainers/devcontainer_test.go +++ /dev/null @@ -1,274 +0,0 @@ -package agentcontainers_test - -import ( - "path/filepath" - "strings" - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/google/uuid" - "github.com/stretchr/testify/require" - - "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/v2/agent/agentcontainers" - "github.com/coder/coder/v2/codersdk" -) - -func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { - t.Parallel() - - scriptIDs := []uuid.UUID{uuid.New(), uuid.New()} - devcontainerIDs := []uuid.UUID{uuid.New(), uuid.New()} - - type args struct { - expandPath func(string) (string, error) - devcontainers []codersdk.WorkspaceAgentDevcontainer - scripts []codersdk.WorkspaceAgentScript - } - tests := []struct { - name string - args args - wantFilteredScripts []codersdk.WorkspaceAgentScript - wantDevcontainerScripts []codersdk.WorkspaceAgentScript - - skipOnWindowsDueToPathSeparator bool - }{ - { - name: "no scripts", - args: args{ - expandPath: nil, - devcontainers: nil, - scripts: nil, - }, - wantFilteredScripts: nil, - wantDevcontainerScripts: nil, - }, - { - name: "no devcontainers", - args: args{ - expandPath: nil, - devcontainers: nil, - scripts: []codersdk.WorkspaceAgentScript{ - {ID: scriptIDs[0]}, - {ID: scriptIDs[1]}, - }, - }, - wantFilteredScripts: []codersdk.WorkspaceAgentScript{ - {ID: scriptIDs[0]}, - {ID: scriptIDs[1]}, - }, - wantDevcontainerScripts: nil, - }, - { - name: "no scripts match devcontainers", - args: args{ - expandPath: nil, - devcontainers: []codersdk.WorkspaceAgentDevcontainer{ - {ID: devcontainerIDs[0]}, - {ID: devcontainerIDs[1]}, - }, - scripts: []codersdk.WorkspaceAgentScript{ - {ID: scriptIDs[0]}, - {ID: scriptIDs[1]}, - }, - }, - wantFilteredScripts: []codersdk.WorkspaceAgentScript{ - {ID: scriptIDs[0]}, - {ID: scriptIDs[1]}, - }, - wantDevcontainerScripts: nil, - }, - { - name: "scripts match devcontainers and sets RunOnStart=false", - args: args{ - expandPath: nil, - devcontainers: []codersdk.WorkspaceAgentDevcontainer{ - {ID: devcontainerIDs[0], WorkspaceFolder: "workspace1"}, - {ID: devcontainerIDs[1], WorkspaceFolder: "workspace2"}, - }, - scripts: []codersdk.WorkspaceAgentScript{ - {ID: scriptIDs[0], RunOnStart: true}, - {ID: scriptIDs[1], RunOnStart: true}, - {ID: devcontainerIDs[0], RunOnStart: true}, - {ID: devcontainerIDs[1], RunOnStart: true}, - }, - }, - wantFilteredScripts: []codersdk.WorkspaceAgentScript{ - {ID: scriptIDs[0], RunOnStart: true}, - {ID: scriptIDs[1], RunOnStart: true}, - }, - wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ - { - ID: devcontainerIDs[0], - Script: "devcontainer up --log-format json --workspace-folder \"workspace1\"", - RunOnStart: false, - }, - { - ID: devcontainerIDs[1], - Script: "devcontainer up --log-format json --workspace-folder \"workspace2\"", - RunOnStart: false, - }, - }, - }, - { - name: "scripts match devcontainers with config path", - args: args{ - expandPath: nil, - devcontainers: []codersdk.WorkspaceAgentDevcontainer{ - { - ID: devcontainerIDs[0], - WorkspaceFolder: "workspace1", - ConfigPath: "config1", - }, - { - ID: devcontainerIDs[1], - WorkspaceFolder: "workspace2", - ConfigPath: "config2", - }, - }, - scripts: []codersdk.WorkspaceAgentScript{ - {ID: devcontainerIDs[0]}, - {ID: devcontainerIDs[1]}, - }, - }, - wantFilteredScripts: []codersdk.WorkspaceAgentScript{}, - wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ - { - ID: devcontainerIDs[0], - Script: "devcontainer up --log-format json --workspace-folder \"workspace1\" --config \"workspace1/config1\"", - RunOnStart: false, - }, - { - ID: devcontainerIDs[1], - Script: "devcontainer up --log-format json --workspace-folder \"workspace2\" --config \"workspace2/config2\"", - RunOnStart: false, - }, - }, - skipOnWindowsDueToPathSeparator: true, - }, - { - name: "scripts match devcontainers with expand path", - args: args{ - expandPath: func(s string) (string, error) { - return "/home/" + s, nil - }, - devcontainers: []codersdk.WorkspaceAgentDevcontainer{ - { - ID: devcontainerIDs[0], - WorkspaceFolder: "workspace1", - ConfigPath: "config1", - }, - { - ID: devcontainerIDs[1], - WorkspaceFolder: "workspace2", - ConfigPath: "config2", - }, - }, - scripts: []codersdk.WorkspaceAgentScript{ - {ID: devcontainerIDs[0], RunOnStart: true}, - {ID: devcontainerIDs[1], RunOnStart: true}, - }, - }, - wantFilteredScripts: []codersdk.WorkspaceAgentScript{}, - wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ - { - ID: devcontainerIDs[0], - Script: "devcontainer up --log-format json --workspace-folder \"/home/workspace1\" --config \"/home/workspace1/config1\"", - RunOnStart: false, - }, - { - ID: devcontainerIDs[1], - Script: "devcontainer up --log-format json --workspace-folder \"/home/workspace2\" --config \"/home/workspace2/config2\"", - RunOnStart: false, - }, - }, - skipOnWindowsDueToPathSeparator: true, - }, - { - name: "expand config path when ~", - args: args{ - expandPath: func(s string) (string, error) { - s = strings.Replace(s, "~/", "", 1) - if filepath.IsAbs(s) { - return s, nil - } - return "/home/" + s, nil - }, - devcontainers: []codersdk.WorkspaceAgentDevcontainer{ - { - ID: devcontainerIDs[0], - WorkspaceFolder: "workspace1", - ConfigPath: "~/config1", - }, - { - ID: devcontainerIDs[1], - WorkspaceFolder: "workspace2", - ConfigPath: "/config2", - }, - }, - scripts: []codersdk.WorkspaceAgentScript{ - {ID: devcontainerIDs[0], RunOnStart: true}, - {ID: devcontainerIDs[1], RunOnStart: true}, - }, - }, - wantFilteredScripts: []codersdk.WorkspaceAgentScript{}, - wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ - { - ID: devcontainerIDs[0], - Script: "devcontainer up --log-format json --workspace-folder \"/home/workspace1\" --config \"/home/config1\"", - RunOnStart: false, - }, - { - ID: devcontainerIDs[1], - Script: "devcontainer up --log-format json --workspace-folder \"/home/workspace2\" --config \"/config2\"", - RunOnStart: false, - }, - }, - skipOnWindowsDueToPathSeparator: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - if tt.skipOnWindowsDueToPathSeparator && filepath.Separator == '\\' { - t.Skip("Skipping test on Windows due to path separator difference.") - } - - logger := slogtest.Make(t, nil) - if tt.args.expandPath == nil { - tt.args.expandPath = func(s string) (string, error) { - return s, nil - } - } - gotFilteredScripts, gotDevcontainerScripts := agentcontainers.ExtractAndInitializeDevcontainerScripts( - agentcontainers.ExpandAllDevcontainerPaths(logger, tt.args.expandPath, tt.args.devcontainers), - tt.args.scripts, - ) - - if diff := cmp.Diff(tt.wantFilteredScripts, gotFilteredScripts, cmpopts.EquateEmpty()); diff != "" { - t.Errorf("ExtractAndInitializeDevcontainerScripts() gotFilteredScripts mismatch (-want +got):\n%s", diff) - } - - // Preprocess the devcontainer scripts to remove scripting part. - for i := range gotDevcontainerScripts { - gotDevcontainerScripts[i].Script = textGrep("devcontainer up", gotDevcontainerScripts[i].Script) - require.NotEmpty(t, gotDevcontainerScripts[i].Script, "devcontainer up script not found") - } - if diff := cmp.Diff(tt.wantDevcontainerScripts, gotDevcontainerScripts); diff != "" { - t.Errorf("ExtractAndInitializeDevcontainerScripts() gotDevcontainerScripts mismatch (-want +got):\n%s", diff) - } - }) - } -} - -// textGrep returns matching lines from multiline string. -func textGrep(want, got string) (filtered string) { - var lines []string - for _, line := range strings.Split(got, "\n") { - if strings.Contains(line, want) { - lines = append(lines, line) - } - } - return strings.Join(lines, "\n") -} diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index e302ff07d6dd9..95469b54a515d 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -135,7 +135,7 @@ func WithReadConfigOutput(stdout, stderr io.Writer) DevcontainerCLIReadConfigOpt } func applyDevcontainerCLIUpOptions(opts []DevcontainerCLIUpOptions) devcontainerCLIUpConfig { - conf := devcontainerCLIUpConfig{} + conf := devcontainerCLIUpConfig{stdout: io.Discard, stderr: io.Discard} for _, opt := range opts { if opt != nil { opt(&conf) @@ -145,7 +145,7 @@ func applyDevcontainerCLIUpOptions(opts []DevcontainerCLIUpOptions) devcontainer } func applyDevcontainerCLIExecOptions(opts []DevcontainerCLIExecOptions) devcontainerCLIExecConfig { - conf := devcontainerCLIExecConfig{} + conf := devcontainerCLIExecConfig{stdout: io.Discard, stderr: io.Discard} for _, opt := range opts { if opt != nil { opt(&conf) @@ -155,7 +155,7 @@ func applyDevcontainerCLIExecOptions(opts []DevcontainerCLIExecOptions) devconta } func applyDevcontainerCLIReadConfigOptions(opts []DevcontainerCLIReadConfigOptions) devcontainerCLIReadConfigConfig { - conf := devcontainerCLIReadConfigConfig{} + conf := devcontainerCLIReadConfigConfig{stdout: io.Discard, stderr: io.Discard} for _, opt := range opts { if opt != nil { opt(&conf) @@ -195,17 +195,18 @@ func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath st // Capture stdout for parsing and stream logs for both default and provided writers. var stdoutBuf bytes.Buffer - stdoutWriters := []io.Writer{&stdoutBuf, &devcontainerCLILogWriter{ctx: ctx, logger: logger.With(slog.F("stdout", true))}} - if conf.stdout != nil { - stdoutWriters = append(stdoutWriters, conf.stdout) - } - cmd.Stdout = io.MultiWriter(stdoutWriters...) + cmd.Stdout = io.MultiWriter( + &stdoutBuf, + &devcontainerCLILogWriter{ctx: ctx, + logger: logger.With(slog.F("stdout", true)), + writer: conf.stdout, + }, + ) // Stream stderr logs and provided writer if any. - stderrWriters := []io.Writer{&devcontainerCLILogWriter{ctx: ctx, logger: logger.With(slog.F("stderr", true))}} - if conf.stderr != nil { - stderrWriters = append(stderrWriters, conf.stderr) + cmd.Stderr = &devcontainerCLILogWriter{ctx: ctx, + logger: logger.With(slog.F("stderr", true)), + writer: conf.stderr, } - cmd.Stderr = io.MultiWriter(stderrWriters...) if err := cmd.Run(); err != nil { _, err2 := parseDevcontainerCLILastLine[devcontainerCLIResult](ctx, logger, stdoutBuf.Bytes()) @@ -244,16 +245,14 @@ func (d *devcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath args = append(args, cmdArgs...) c := d.execer.CommandContext(ctx, "devcontainer", args...) - stdoutWriters := []io.Writer{&devcontainerCLILogWriter{ctx: ctx, logger: logger.With(slog.F("stdout", true))}} - if conf.stdout != nil { - stdoutWriters = append(stdoutWriters, conf.stdout) + c.Stdout = &devcontainerCLILogWriter{ctx: ctx, + logger: logger.With(slog.F("stdout", true)), + writer: conf.stdout, } - c.Stdout = io.MultiWriter(stdoutWriters...) - stderrWriters := []io.Writer{&devcontainerCLILogWriter{ctx: ctx, logger: logger.With(slog.F("stderr", true))}} - if conf.stderr != nil { - stderrWriters = append(stderrWriters, conf.stderr) + c.Stderr = &devcontainerCLILogWriter{ctx: ctx, + logger: logger.With(slog.F("stderr", true)), + writer: conf.stderr, } - c.Stderr = io.MultiWriter(stderrWriters...) if err := c.Run(); err != nil { return xerrors.Errorf("devcontainer exec failed: %w", err) @@ -279,16 +278,17 @@ func (d *devcontainerCLI) ReadConfig(ctx context.Context, workspaceFolder, confi c.Env = append(c.Env, env...) var stdoutBuf bytes.Buffer - stdoutWriters := []io.Writer{&stdoutBuf, &devcontainerCLILogWriter{ctx: ctx, logger: logger.With(slog.F("stdout", true))}} - if conf.stdout != nil { - stdoutWriters = append(stdoutWriters, conf.stdout) - } - c.Stdout = io.MultiWriter(stdoutWriters...) - stderrWriters := []io.Writer{&devcontainerCLILogWriter{ctx: ctx, logger: logger.With(slog.F("stderr", true))}} - if conf.stderr != nil { - stderrWriters = append(stderrWriters, conf.stderr) + c.Stdout = io.MultiWriter( + &stdoutBuf, + &devcontainerCLILogWriter{ctx: ctx, + logger: logger.With(slog.F("stdout", true)), + writer: conf.stdout, + }, + ) + c.Stderr = &devcontainerCLILogWriter{ctx: ctx, + logger: logger.With(slog.F("stderr", true)), + writer: conf.stderr, } - c.Stderr = io.MultiWriter(stderrWriters...) if err := c.Run(); err != nil { return DevcontainerConfig{}, xerrors.Errorf("devcontainer read-configuration failed: %w", err) @@ -381,6 +381,7 @@ type devcontainerCLIJSONLogLine struct { type devcontainerCLILogWriter struct { ctx context.Context logger slog.Logger + writer io.Writer } func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { @@ -401,6 +402,7 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { } if logLine.Level >= 3 { l.logger.Info(l.ctx, "@devcontainer/cli", slog.F("line", string(line))) + l.writer.Write([]byte(logLine.Text)) continue } l.logger.Debug(l.ctx, "@devcontainer/cli", slog.F("line", string(line))) diff --git a/agent/agentscripts/agentscripts.go b/agent/agentscripts/agentscripts.go index 79606a80233b9..ac6e3a30f44a1 100644 --- a/agent/agentscripts/agentscripts.go +++ b/agent/agentscripts/agentscripts.go @@ -79,21 +79,6 @@ func New(opts Options) *Runner { type ScriptCompletedFunc func(context.Context, *proto.WorkspaceAgentScriptCompletedRequest) (*proto.WorkspaceAgentScriptCompletedResponse, error) -type runnerScript struct { - runOnPostStart bool - codersdk.WorkspaceAgentScript -} - -func toRunnerScript(scripts ...codersdk.WorkspaceAgentScript) []runnerScript { - var rs []runnerScript - for _, s := range scripts { - rs = append(rs, runnerScript{ - WorkspaceAgentScript: s, - }) - } - return rs -} - type Runner struct { Options @@ -103,7 +88,7 @@ type Runner struct { closed chan struct{} closeMutex sync.Mutex cron *cron.Cron - scripts []runnerScript + scripts []codersdk.WorkspaceAgentScript dataDir string scriptCompleted ScriptCompletedFunc @@ -138,19 +123,6 @@ func (r *Runner) RegisterMetrics(reg prometheus.Registerer) { // InitOption describes an option for the runner initialization. type InitOption func(*Runner) -// WithPostStartScripts adds scripts that should be run after the workspace -// start scripts but before the workspace is marked as started. -func WithPostStartScripts(scripts ...codersdk.WorkspaceAgentScript) InitOption { - return func(r *Runner) { - for _, s := range scripts { - r.scripts = append(r.scripts, runnerScript{ - runOnPostStart: true, - WorkspaceAgentScript: s, - }) - } - } -} - // Init initializes the runner with the provided scripts. // It also schedules any scripts that have a schedule. // This function must be called before Execute. @@ -161,7 +133,7 @@ func (r *Runner) Init(scripts []codersdk.WorkspaceAgentScript, scriptCompleted S return xerrors.New("init: already initialized") } r.initialized = true - r.scripts = toRunnerScript(scripts...) + r.scripts = scripts r.scriptCompleted = scriptCompleted for _, opt := range opts { opt(r) @@ -179,7 +151,7 @@ func (r *Runner) Init(scripts []codersdk.WorkspaceAgentScript, scriptCompleted S } script := script _, err := r.cron.AddFunc(script.Cron, func() { - err := r.trackRun(r.cronCtx, script.WorkspaceAgentScript, ExecuteCronScripts) + err := r.trackRun(r.cronCtx, script, ExecuteCronScripts) if err != nil { r.Logger.Warn(context.Background(), "run agent script on schedule", slog.Error(err)) } @@ -223,7 +195,6 @@ type ExecuteOption int const ( ExecuteAllScripts ExecuteOption = iota ExecuteStartScripts - ExecutePostStartScripts ExecuteStopScripts ExecuteCronScripts ) @@ -246,7 +217,6 @@ func (r *Runner) Execute(ctx context.Context, option ExecuteOption) error { for _, script := range r.scripts { runScript := (option == ExecuteStartScripts && script.RunOnStart) || (option == ExecuteStopScripts && script.RunOnStop) || - (option == ExecutePostStartScripts && script.runOnPostStart) || (option == ExecuteCronScripts && script.Cron != "") || option == ExecuteAllScripts @@ -256,7 +226,7 @@ func (r *Runner) Execute(ctx context.Context, option ExecuteOption) error { script := script eg.Go(func() error { - err := r.trackRun(ctx, script.WorkspaceAgentScript, option) + err := r.trackRun(ctx, script, option) if err != nil { return xerrors.Errorf("run agent script %q: %w", script.LogSourceID, err) } diff --git a/agent/agentscripts/agentscripts_test.go b/agent/agentscripts/agentscripts_test.go index f50a0cc065138..c032ea1f83a1a 100644 --- a/agent/agentscripts/agentscripts_test.go +++ b/agent/agentscripts/agentscripts_test.go @@ -4,7 +4,6 @@ import ( "context" "path/filepath" "runtime" - "slices" "sync" "testing" "time" @@ -177,11 +176,6 @@ func TestExecuteOptions(t *testing.T) { Script: "echo stop", RunOnStop: true, } - postStartScript := codersdk.WorkspaceAgentScript{ - ID: uuid.New(), - LogSourceID: uuid.New(), - Script: "echo poststart", - } regularScript := codersdk.WorkspaceAgentScript{ ID: uuid.New(), LogSourceID: uuid.New(), @@ -193,10 +187,9 @@ func TestExecuteOptions(t *testing.T) { stopScript, regularScript, } - allScripts := append(slices.Clone(scripts), postStartScript) scriptByID := func(t *testing.T, id uuid.UUID) codersdk.WorkspaceAgentScript { - for _, script := range allScripts { + for _, script := range scripts { if script.ID == id { return script } @@ -206,10 +199,9 @@ func TestExecuteOptions(t *testing.T) { } wantOutput := map[uuid.UUID]string{ - startScript.ID: "start", - stopScript.ID: "stop", - postStartScript.ID: "poststart", - regularScript.ID: "regular", + startScript.ID: "start", + stopScript.ID: "stop", + regularScript.ID: "regular", } testCases := []struct { @@ -220,18 +212,13 @@ func TestExecuteOptions(t *testing.T) { { name: "ExecuteAllScripts", option: agentscripts.ExecuteAllScripts, - wantRun: []uuid.UUID{startScript.ID, stopScript.ID, regularScript.ID, postStartScript.ID}, + wantRun: []uuid.UUID{startScript.ID, stopScript.ID, regularScript.ID}, }, { name: "ExecuteStartScripts", option: agentscripts.ExecuteStartScripts, wantRun: []uuid.UUID{startScript.ID}, }, - { - name: "ExecutePostStartScripts", - option: agentscripts.ExecutePostStartScripts, - wantRun: []uuid.UUID{postStartScript.ID}, - }, { name: "ExecuteStopScripts", option: agentscripts.ExecuteStopScripts, @@ -260,7 +247,6 @@ func TestExecuteOptions(t *testing.T) { err := runner.Init( scripts, aAPI.ScriptCompleted, - agentscripts.WithPostStartScripts(postStartScript), ) require.NoError(t, err) @@ -274,7 +260,7 @@ func TestExecuteOptions(t *testing.T) { "script %s should have run when using filter %s", scriptByID(t, id).Script, tc.name) } - for _, script := range allScripts { + for _, script := range scripts { if _, ok := gotRun[script.ID]; ok { continue } From fe99bd6f71da6ae7574c50db3024890863db752f Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 20 Jun 2025 14:52:23 +0000 Subject: [PATCH 02/23] chore: appease formatter --- agent/agentcontainers/devcontainercli.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 95469b54a515d..74e67c73c00aa 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -197,13 +197,15 @@ func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath st var stdoutBuf bytes.Buffer cmd.Stdout = io.MultiWriter( &stdoutBuf, - &devcontainerCLILogWriter{ctx: ctx, + &devcontainerCLILogWriter{ + ctx: ctx, logger: logger.With(slog.F("stdout", true)), writer: conf.stdout, }, ) // Stream stderr logs and provided writer if any. - cmd.Stderr = &devcontainerCLILogWriter{ctx: ctx, + cmd.Stderr = &devcontainerCLILogWriter{ + ctx: ctx, logger: logger.With(slog.F("stderr", true)), writer: conf.stderr, } @@ -245,11 +247,13 @@ func (d *devcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath args = append(args, cmdArgs...) c := d.execer.CommandContext(ctx, "devcontainer", args...) - c.Stdout = &devcontainerCLILogWriter{ctx: ctx, + c.Stdout = &devcontainerCLILogWriter{ + ctx: ctx, logger: logger.With(slog.F("stdout", true)), writer: conf.stdout, } - c.Stderr = &devcontainerCLILogWriter{ctx: ctx, + c.Stderr = &devcontainerCLILogWriter{ + ctx: ctx, logger: logger.With(slog.F("stderr", true)), writer: conf.stderr, } @@ -280,12 +284,14 @@ func (d *devcontainerCLI) ReadConfig(ctx context.Context, workspaceFolder, confi var stdoutBuf bytes.Buffer c.Stdout = io.MultiWriter( &stdoutBuf, - &devcontainerCLILogWriter{ctx: ctx, + &devcontainerCLILogWriter{ + ctx: ctx, logger: logger.With(slog.F("stdout", true)), writer: conf.stdout, }, ) - c.Stderr = &devcontainerCLILogWriter{ctx: ctx, + c.Stderr = &devcontainerCLILogWriter{ + ctx: ctx, logger: logger.With(slog.F("stderr", true)), writer: conf.stderr, } From aeae6e285d4d533735505419750595819e8f2c5d Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 20 Jun 2025 15:18:48 +0000 Subject: [PATCH 03/23] chore: fix test, appease linter --- agent/agentcontainers/api.go | 4 +- agent/agentcontainers/devcontainercli.go | 14 ++-- agent/agentcontainers/devcontainercli_test.go | 2 +- .../devcontainercli/parse/up.expected | 76 +++++++++++++++++++ 4 files changed, 87 insertions(+), 9 deletions(-) create mode 100644 agent/agentcontainers/testdata/devcontainercli/parse/up.expected diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index d5e8a10001d0a..3cc89dee76b49 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -784,7 +784,9 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques dc.Container = nil api.knownDevcontainers[dc.WorkspaceFolder] = dc api.asyncWg.Add(1) - go api.recreateDevcontainer(dc, configPath) + go func() { + _ = api.recreateDevcontainer(dc, configPath) + }() api.mu.Unlock() diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 74e67c73c00aa..9673fd4c2d799 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -247,16 +247,16 @@ func (d *devcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath args = append(args, cmdArgs...) c := d.execer.CommandContext(ctx, "devcontainer", args...) - c.Stdout = &devcontainerCLILogWriter{ + c.Stdout = io.MultiWriter(conf.stdout, &devcontainerCLILogWriter{ ctx: ctx, logger: logger.With(slog.F("stdout", true)), - writer: conf.stdout, - } - c.Stderr = &devcontainerCLILogWriter{ + writer: io.Discard, + }) + c.Stderr = io.MultiWriter(conf.stderr, &devcontainerCLILogWriter{ ctx: ctx, logger: logger.With(slog.F("stderr", true)), - writer: conf.stderr, - } + writer: io.Discard, + }) if err := c.Run(); err != nil { return xerrors.Errorf("devcontainer exec failed: %w", err) @@ -408,7 +408,7 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { } if logLine.Level >= 3 { l.logger.Info(l.ctx, "@devcontainer/cli", slog.F("line", string(line))) - l.writer.Write([]byte(logLine.Text)) + _, _ = l.writer.Write([]byte(logLine.Text + "\n")) continue } l.logger.Debug(l.ctx, "@devcontainer/cli", slog.F("line", string(line))) diff --git a/agent/agentcontainers/devcontainercli_test.go b/agent/agentcontainers/devcontainercli_test.go index 821e6e8f95e76..28a39a4b47ab9 100644 --- a/agent/agentcontainers/devcontainercli_test.go +++ b/agent/agentcontainers/devcontainercli_test.go @@ -363,7 +363,7 @@ func TestDevcontainerCLI_WithOutput(t *testing.T) { require.NotEmpty(t, containerID, "expected non-empty container ID") // Read expected log content. - expLog, err := os.ReadFile(filepath.Join("testdata", "devcontainercli", "parse", "up.log")) + expLog, err := os.ReadFile(filepath.Join("testdata", "devcontainercli", "parse", "up.expected")) require.NoError(t, err, "reading expected log file") // Verify stdout buffer contains the CLI logs and stderr is empty. diff --git a/agent/agentcontainers/testdata/devcontainercli/parse/up.expected b/agent/agentcontainers/testdata/devcontainercli/parse/up.expected new file mode 100644 index 0000000000000..83a2bdcc78f68 --- /dev/null +++ b/agent/agentcontainers/testdata/devcontainercli/parse/up.expected @@ -0,0 +1,76 @@ +@devcontainers/cli 0.75.0. Node.js v23.9.0. darwin 24.4.0 arm64. +Resolving Feature dependencies for 'ghcr.io/devcontainers/features/docker-in-docker:2'... +Soft-dependency 'ghcr.io/devcontainers/features/common-utils' is not required. Removing from installation order... +Files to omit: '' +Run: docker buildx build --load --build-context dev_containers_feature_content_source=/var/folders/1y/cm8mblxd7_x9cljwl_jvfprh0000gn/T/devcontainercli/container-features/0.75.0-1744102171193 --build-arg _DEV_CONTAINERS_BASE_IMAGE=mcr.microsoft.com/devcontainers/javascript-node:1-18-bullseye --build-arg _DEV_CONTAINERS_IMAGE_USER=root --build-arg _DEV_CONTAINERS_FEATURE_CONTENT_SOURCE=dev_container_feature_content_temp --target dev_containers_target_stage -f /var/folders/1y/cm8mblxd7_x9cljwl_jvfprh0000gn/T/devcontainercli/container-features/0.75.0-1744102171193/Dockerfile.extended -t vsc-devcontainers-template-starter-81d8f17e32abef6d434cbb5a37fe05e5c8a6f8ccede47a61197f002dcbf60566-features /var/folders/1y/cm8mblxd7_x9cljwl_jvfprh0000gn/T/devcontainercli/empty-folder +#0 building with "orbstack" instance using docker driver + +#1 [internal] load build definition from Dockerfile.extended + +#1 transferring dockerfile: 3.09kB done +#1 DONE 0.0s + +#2 resolve image config for docker-image://docker.io/docker/dockerfile:1.4 + +#2 DONE 1.3s + + +#3 docker-image://docker.io/docker/dockerfile:1.4@sha256:9ba7531bd80fb0a858632727cf7a112fbfd19b17e94c4e84ced81e24ef1a0dbc +#3 CACHED + + + +#4 [internal] load .dockerignore +#4 transferring context: 2B done +#4 DONE 0.0s + +#5 [internal] load metadata for mcr.microsoft.com/devcontainers/javascript-node:1-18-bullseye +#5 DONE 0.0s + +#6 [context dev_containers_feature_content_source] load .dockerignore +#6 transferring dev_containers_feature_content_source: 2B done +#6 DONE 0.0s + +#7 [dev_containers_feature_content_normalize 1/3] FROM mcr.microsoft.com/devcontainers/javascript-node:1-18-bullseye +#7 DONE 0.0s + +#8 [context dev_containers_feature_content_source] load from client +#8 transferring dev_containers_feature_content_source: 82.11kB 0.0s done +#8 DONE 0.0s + +#9 [dev_containers_feature_content_normalize 2/3] COPY --from=dev_containers_feature_content_source devcontainer-features.builtin.env /tmp/build-features/ +#9 CACHED + +#10 [dev_containers_target_stage 2/5] RUN mkdir -p /tmp/dev-container-features +#10 CACHED + +#11 [dev_containers_target_stage 3/5] COPY --from=dev_containers_feature_content_normalize /tmp/build-features/ /tmp/dev-container-features +#11 CACHED + +#12 [dev_containers_target_stage 4/5] RUN echo "_CONTAINER_USER_HOME=$( (command -v getent >/dev/null 2>&1 && getent passwd 'root' || grep -E '^root|^[^:]*:[^:]*:root:' /etc/passwd || true) | cut -d: -f6)" >> /tmp/dev-container-features/devcontainer-features.builtin.env && echo "_REMOTE_USER_HOME=$( (command -v getent >/dev/null 2>&1 && getent passwd 'node' || grep -E '^node|^[^:]*:[^:]*:node:' /etc/passwd || true) | cut -d: -f6)" >> /tmp/dev-container-features/devcontainer-features.builtin.env +#12 CACHED + +#13 [dev_containers_feature_content_normalize 3/3] RUN chmod -R 0755 /tmp/build-features/ +#13 CACHED + +#14 [dev_containers_target_stage 5/5] RUN --mount=type=bind,from=dev_containers_feature_content_source,source=docker-in-docker_0,target=/tmp/build-features-src/docker-in-docker_0 cp -ar /tmp/build-features-src/docker-in-docker_0 /tmp/dev-container-features && chmod -R 0755 /tmp/dev-container-features/docker-in-docker_0 && cd /tmp/dev-container-features/docker-in-docker_0 && chmod +x ./devcontainer-features-install.sh && ./devcontainer-features-install.sh && rm -rf /tmp/dev-container-features/docker-in-docker_0 +#14 CACHED + +#15 exporting to image +#15 exporting layers done +#15 writing image sha256:275dc193c905d448ef3945e3fc86220cc315fe0cb41013988d6ff9f8d6ef2357 done +#15 naming to docker.io/library/vsc-devcontainers-template-starter-81d8f17e32abef6d434cbb5a37fe05e5c8a6f8ccede47a61197f002dcbf60566-features done +#15 DONE 0.0s + +Run: docker buildx build --load --build-context dev_containers_feature_content_source=/var/folders/1y/cm8mblxd7_x9cljwl_jvfprh0000gn/T/devcontainercli/container-features/0.75.0-1744102171193 --build-arg _DEV_CONTAINERS_BASE_IMAGE=mcr.microsoft.com/devcontainers/javascript-node:1-18-bullseye --build-arg _DEV_CONTAINERS_IMAGE_USER=root --build-arg _DEV_CONTAINERS_FEATURE_CONTENT_SOURCE=dev_container_feature_content_temp --target dev_containers_target_stage -f /var/folders/1y/cm8mblxd7_x9cljwl_jvfprh0000gn/T/devcontainercli/container-features/0.75.0-1744102171193/Dockerfile.extended -t vsc-devcontainers-template-starter-81d8f17e32abef6d434cbb5a37fe05e5c8a6f8ccede47a61197f002dcbf60566-features /var/folders/1y/cm8mblxd7_x9cljwl_jvfprh0000gn/T/devcontainercli/empty-folder +Run: docker run --sig-proxy=false -a STDOUT -a STDERR --mount type=bind,source=/code/devcontainers-template-starter,target=/workspaces/devcontainers-template-starter,consistency=cached --mount type=volume,src=dind-var-lib-docker-0pctifo8bbg3pd06g3j5s9ae8j7lp5qfcd67m25kuahurel7v7jm,dst=/var/lib/docker -l devcontainer.local_folder=/code/devcontainers-template-starter -l devcontainer.config_file=/code/devcontainers-template-starter/.devcontainer/devcontainer.json --privileged --entrypoint /bin/sh vsc-devcontainers-template-starter-81d8f17e32abef6d434cbb5a37fe05e5c8a6f8ccede47a61197f002dcbf60566-features -c echo Container started +Container started + +Not setting dockerd DNS manually. + +Running the postCreateCommand from devcontainer.json... + + + +added 1 package in 784ms + From 916f7e8c675377f207f8d177683bfe62487f82d7 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 13:43:12 +0000 Subject: [PATCH 04/23] chore: feedback --- agent/agentcontainers/api.go | 5 +++-- agent/agentcontainers/devcontainercli_test.go | 2 +- .../devcontainercli/parse/{up.expected => up.golden} | 0 3 files changed, 4 insertions(+), 3 deletions(-) rename agent/agentcontainers/testdata/devcontainercli/parse/{up.expected => up.golden} (100%) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 3cc89dee76b49..dcb02c79d4670 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -870,7 +870,8 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con api.knownDevcontainers[dc.WorkspaceFolder] = dc api.recreateErrorTimes[dc.WorkspaceFolder] = api.clock.Now("agentcontainers", "recreate", "errorTimes") api.mu.Unlock() - return err + + return fmt.Errorf("start devcontainer: %w", err) } logger.Info(ctx, "devcontainer recreated successfully") @@ -897,7 +898,7 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con // devcontainer state after recreation. if err := api.RefreshContainers(ctx); err != nil { logger.Error(ctx, "failed to trigger immediate refresh after devcontainer recreation", slog.Error(err)) - return err + return fmt.Errorf("refresh containers: %w", err) } return nil diff --git a/agent/agentcontainers/devcontainercli_test.go b/agent/agentcontainers/devcontainercli_test.go index 28a39a4b47ab9..fe64b04abb884 100644 --- a/agent/agentcontainers/devcontainercli_test.go +++ b/agent/agentcontainers/devcontainercli_test.go @@ -363,7 +363,7 @@ func TestDevcontainerCLI_WithOutput(t *testing.T) { require.NotEmpty(t, containerID, "expected non-empty container ID") // Read expected log content. - expLog, err := os.ReadFile(filepath.Join("testdata", "devcontainercli", "parse", "up.expected")) + expLog, err := os.ReadFile(filepath.Join("testdata", "devcontainercli", "parse", "up.golden")) require.NoError(t, err, "reading expected log file") // Verify stdout buffer contains the CLI logs and stderr is empty. diff --git a/agent/agentcontainers/testdata/devcontainercli/parse/up.expected b/agent/agentcontainers/testdata/devcontainercli/parse/up.golden similarity index 100% rename from agent/agentcontainers/testdata/devcontainercli/parse/up.expected rename to agent/agentcontainers/testdata/devcontainercli/parse/up.golden From 91bb43af35d52a9052f0bb08dedcfe1772377b8c Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 16:17:59 +0000 Subject: [PATCH 05/23] chore: re-add script timings --- agent/agent.go | 37 ++++++++++++++++++++++++--- agent/agentcontainers/devcontainer.go | 23 +++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 0033e7750628a..e6f54eb8a8a75 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -89,6 +89,7 @@ type Options struct { ServiceBannerRefreshInterval time.Duration BlockFileTransfer bool Execer agentexec.Execer + Clock quartz.Clock ExperimentalDevcontainersEnabled bool ContainerAPIOptions []agentcontainers.Option // Enable ExperimentalDevcontainersEnabled for these to be effective. @@ -145,6 +146,9 @@ func New(options Options) Agent { if options.PortCacheDuration == 0 { options.PortCacheDuration = 1 * time.Second } + if options.Clock == nil { + options.Clock = quartz.NewReal() + } prometheusRegistry := options.PrometheusRegistry if prometheusRegistry == nil { @@ -158,6 +162,7 @@ func New(options Options) Agent { hardCtx, hardCancel := context.WithCancel(context.Background()) gracefulCtx, gracefulCancel := context.WithCancel(hardCtx) a := &agent{ + clock: options.Clock, tailnetListenPort: options.TailnetListenPort, reconnectingPTYTimeout: options.ReconnectingPTYTimeout, logger: options.Logger, @@ -205,6 +210,7 @@ func New(options Options) Agent { } type agent struct { + clock quartz.Clock logger slog.Logger client Client exchangeToken func(ctx context.Context) (string, error) @@ -1142,9 +1148,13 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } var ( - scripts = manifest.Scripts - scriptRunnerOpts []agentscripts.InitOption + scripts = manifest.Scripts + scriptRunnerOpts []agentscripts.InitOption + devcontainerScripts map[uuid.UUID]codersdk.WorkspaceAgentScript ) + if a.experimentalDevcontainersEnabled { + scripts, devcontainerScripts = agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, scripts) + } err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted, scriptRunnerOpts...) if err != nil { return xerrors.Errorf("init script runner: %w", err) @@ -1165,7 +1175,28 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, if cAPI := a.containerAPI.Load(); cAPI != nil { for _, dc := range manifest.Devcontainers { - err = errors.Join(err, cAPI.CreateDevcontainer(dc)) + var ( + script = devcontainerScripts[dc.ID] + exitCode = int32(0) + startTime = a.clock.Now() + ) + if cErr := cAPI.CreateDevcontainer(dc); cErr != nil { + exitCode = 1 + err = errors.Join(err, cErr) + } + endTime := a.clock.Now() + + if _, scriptErr := aAPI.ScriptCompleted(ctx, &proto.WorkspaceAgentScriptCompletedRequest{ + Timing: &proto.Timing{ + ScriptId: script.ID[:], + Start: timestamppb.New(startTime), + End: timestamppb.New(endTime), + ExitCode: exitCode, + Stage: proto.Timing_START, + }, + }); scriptErr != nil { + a.logger.Warn(ctx, "reporting script completed failed", slog.Error(scriptErr)) + } } } diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index 9ba77e7121b9b..e5786d7074321 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -7,6 +7,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" ) const ( @@ -20,6 +21,28 @@ const ( DevcontainerDefaultContainerWorkspaceFolder = "/workspaces" ) +func ExtractDevcontainerScripts( + devcontainers []codersdk.WorkspaceAgentDevcontainer, + scripts []codersdk.WorkspaceAgentScript, +) (filteredScripts []codersdk.WorkspaceAgentScript, devcontainerScripts map[uuid.UUID]codersdk.WorkspaceAgentScript) { + devcontainerScripts = make(map[uuid.UUID]codersdk.WorkspaceAgentScript) +ScriptLoop: + for _, script := range scripts { + for _, dc := range devcontainers { + // The devcontainer scripts match the devcontainer ID for + // identification. + if script.ID == dc.ID { + devcontainerScripts[dc.ID] = script + continue ScriptLoop + } + } + + filteredScripts = append(filteredScripts, script) + } + + return filteredScripts, devcontainerScripts +} + // ExpandAllDevcontainerPaths expands all devcontainer paths in the given // devcontainers. This is required by the devcontainer CLI, which requires // absolute paths for the workspace folder and config path. From 81fe11d05c74f6784b4a4343bbb69b89711a3a00 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 17:05:56 +0000 Subject: [PATCH 06/23] fix: change how containerAPI is stored --- agent/agent.go | 16 +++++++++++++++- agent/api.go | 32 ++------------------------------ 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index e6f54eb8a8a75..324a01581fbb6 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -280,7 +280,7 @@ type agent struct { experimentalDevcontainersEnabled bool containerAPIOptions []agentcontainers.Option - containerAPI atomic.Pointer[agentcontainers.API] // Set by apiHandler. + containerAPI atomic.Pointer[agentcontainers.API] } func (a *agent) TailnetConn() *tailnet.Conn { @@ -1153,6 +1153,20 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, devcontainerScripts map[uuid.UUID]codersdk.WorkspaceAgentScript ) if a.experimentalDevcontainersEnabled { + containerAPIOpts := []agentcontainers.Option{ + agentcontainers.WithExecer(a.execer), + agentcontainers.WithCommandEnv(a.sshServer.CommandEnv), + agentcontainers.WithScriptLogger(func(logSourceID uuid.UUID) agentcontainers.ScriptLogger { + return a.logSender.GetScriptLogger(logSourceID) + }), + agentcontainers.WithManifestInfo(manifest.OwnerName, manifest.WorkspaceName), + agentcontainers.WithDevcontainers(manifest.Devcontainers, scripts), + agentcontainers.WithSubAgentClient(agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI)), + } + containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...) + + a.containerAPI.Store(agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...)) + scripts, devcontainerScripts = agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, scripts) } err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted, scriptRunnerOpts...) diff --git a/agent/api.go b/agent/api.go index fa761988b2f21..6905f52206888 100644 --- a/agent/api.go +++ b/agent/api.go @@ -7,9 +7,6 @@ import ( "github.com/go-chi/chi/v5" - "github.com/google/uuid" - - "github.com/coder/coder/v2/agent/agentcontainers" "github.com/coder/coder/v2/agent/proto" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" @@ -41,34 +38,9 @@ func (a *agent) apiHandler(aAPI proto.DRPCAgentClient26) (http.Handler, func() e } if a.experimentalDevcontainersEnabled { - containerAPIOpts := []agentcontainers.Option{ - agentcontainers.WithExecer(a.execer), - agentcontainers.WithCommandEnv(a.sshServer.CommandEnv), - agentcontainers.WithScriptLogger(func(logSourceID uuid.UUID) agentcontainers.ScriptLogger { - return a.logSender.GetScriptLogger(logSourceID) - }), - agentcontainers.WithSubAgentClient(agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI)), - } - manifest := a.manifest.Load() - if manifest != nil { - containerAPIOpts = append(containerAPIOpts, - agentcontainers.WithManifestInfo(manifest.OwnerName, manifest.WorkspaceName), - ) - - if len(manifest.Devcontainers) > 0 { - containerAPIOpts = append( - containerAPIOpts, - agentcontainers.WithDevcontainers(manifest.Devcontainers, manifest.Scripts), - ) - } + if cAPI := a.containerAPI.Load(); cAPI != nil { + r.Mount("/api/v0/containers", cAPI.Routes()) } - - // 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{ From 8437ca4bacc6a7b3c7e1fb73e8758f6549b9d45f Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 19:38:04 +0000 Subject: [PATCH 07/23] chore: appease linter --- agent/agent.go | 58 ++++++++++++++++++++++++++++---------------------- agent/api.go | 3 +-- 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 312a444cf4ed9..3bf83091a94b9 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1188,28 +1188,8 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, if cAPI := a.containerAPI.Load(); cAPI != nil { for _, dc := range manifest.Devcontainers { - var ( - script = devcontainerScripts[dc.ID] - exitCode = int32(0) - startTime = a.clock.Now() - ) - if cErr := cAPI.CreateDevcontainer(dc); cErr != nil { - exitCode = 1 - err = errors.Join(err, cErr) - } - endTime := a.clock.Now() - - if _, scriptErr := aAPI.ScriptCompleted(ctx, &proto.WorkspaceAgentScriptCompletedRequest{ - Timing: &proto.Timing{ - ScriptId: script.ID[:], - Start: timestamppb.New(startTime), - End: timestamppb.New(endTime), - ExitCode: exitCode, - Stage: proto.Timing_START, - }, - }); scriptErr != nil { - a.logger.Warn(ctx, "reporting script completed failed", slog.Error(scriptErr)) - } + cErr := a.createDevcontainer(ctx, cAPI, aAPI, dc, devcontainerScripts[dc.ID]) + err = errors.Join(err, cErr) } } @@ -1248,6 +1228,36 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } } +func (a *agent) createDevcontainer( + ctx context.Context, + cAPI *agentcontainers.API, + aAPI proto.DRPCAgentClient26, + dc codersdk.WorkspaceAgentDevcontainer, + script codersdk.WorkspaceAgentScript, +) (err error) { + var ( + exitCode = int32(0) + startTime = a.clock.Now() + ) + if err = cAPI.CreateDevcontainer(dc); err != nil { + exitCode = 1 + } + endTime := a.clock.Now() + + if _, scriptErr := aAPI.ScriptCompleted(ctx, &proto.WorkspaceAgentScriptCompletedRequest{ + Timing: &proto.Timing{ + ScriptId: script.ID[:], + Start: timestamppb.New(startTime), + End: timestamppb.New(endTime), + ExitCode: exitCode, + Stage: proto.Timing_START, + }, + }); scriptErr != nil { + a.logger.Warn(ctx, "reporting script completed failed", slog.Error(scriptErr)) + } + return err +} + // createOrUpdateNetwork waits for the manifest to be set using manifestOK, then creates or updates // the tailnet using the information in the manifest func (a *agent) createOrUpdateNetwork(manifestOK, networkOK *checkpoint) func(context.Context, proto.DRPCAgentClient26) error { @@ -1271,7 +1281,6 @@ func (a *agent) createOrUpdateNetwork(manifestOK, networkOK *checkpoint) func(co // agent API. network, err = a.createTailnet( a.gracefulCtx, - aAPI, manifest.AgentID, manifest.DERPMap, manifest.DERPForceWebSockets, @@ -1426,7 +1435,6 @@ func (a *agent) trackGoroutine(fn func()) error { func (a *agent) createTailnet( ctx context.Context, - aAPI proto.DRPCAgentClient26, agentID uuid.UUID, derpMap *tailcfg.DERPMap, derpForceWebSockets, disableDirectConnections bool, @@ -1559,7 +1567,7 @@ func (a *agent) createTailnet( }() if err = a.trackGoroutine(func() { defer apiListener.Close() - apiHandler, closeAPIHAndler := a.apiHandler(aAPI) + apiHandler, closeAPIHAndler := a.apiHandler() defer func() { _ = closeAPIHAndler() }() diff --git a/agent/api.go b/agent/api.go index 8621ffb327e8b..ea3b990c7908a 100644 --- a/agent/api.go +++ b/agent/api.go @@ -7,12 +7,11 @@ import ( "github.com/go-chi/chi/v5" - "github.com/coder/coder/v2/agent/proto" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" ) -func (a *agent) apiHandler(aAPI proto.DRPCAgentClient26) (http.Handler, func() error) { +func (a *agent) apiHandler() (http.Handler, func() error) { r := chi.NewRouter() r.Get("/", func(rw http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{ From 2c6a2b19fd5c0a43f590540d18c5d0154bd75e57 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 20:08:27 +0000 Subject: [PATCH 08/23] chore: ensure the last log line is printed --- agent/agentcontainers/devcontainercli.go | 11 +++++++++++ .../testdata/devcontainercli/parse/up.golden | 1 + 2 files changed, 12 insertions(+) diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index b61d9a7869a93..305275fd15185 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -415,6 +415,17 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { _, _ = l.writer.Write([]byte(logLine.Text + "\n")) continue } + // If we've successfully parsed the final log line, it will succesfully parse + // but will not fill out any of the fields for `logLine`. In this scenario we + // assume it is the final log line, unmarshal it as that, and check if the + // outcome is a non-empty string. + if logLine.Level == 0 { + var lastLine devcontainerCLIResult + if json.Unmarshal(line, &lastLine); err == nil && lastLine.Outcome != "" { + _, _ = l.writer.Write(line) + _, _ = l.writer.Write([]byte{'\n'}) + } + } l.logger.Debug(l.ctx, "@devcontainer/cli", slog.F("line", string(line))) } if err := s.Err(); err != nil { diff --git a/agent/agentcontainers/testdata/devcontainercli/parse/up.golden b/agent/agentcontainers/testdata/devcontainercli/parse/up.golden index 83a2bdcc78f68..cec57262f790b 100644 --- a/agent/agentcontainers/testdata/devcontainercli/parse/up.golden +++ b/agent/agentcontainers/testdata/devcontainercli/parse/up.golden @@ -74,3 +74,4 @@ Not setting dockerd DNS manually. added 1 package in 784ms +{"outcome":"success","containerId":"bc72db8d0c4c4e941bd9ffc341aee64a18d3397fd45b87cd93d4746150967ba8","remoteUser":"node","remoteWorkspaceFolder":"/workspaces/devcontainers-template-starter"} From a512ad41975df4820c2fcad94e4f0e0595d62b28 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 20:10:53 +0000 Subject: [PATCH 09/23] chore: fix typo --- agent/agentcontainers/devcontainercli.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 305275fd15185..1de40401da693 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -415,7 +415,7 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { _, _ = l.writer.Write([]byte(logLine.Text + "\n")) continue } - // If we've successfully parsed the final log line, it will succesfully parse + // If we've successfully parsed the final log line, it will successfully parse // but will not fill out any of the fields for `logLine`. In this scenario we // assume it is the final log line, unmarshal it as that, and check if the // outcome is a non-empty string. From c50dc6e521e3688f0e02ec9ec5eb82230d956a74 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 20:14:47 +0000 Subject: [PATCH 10/23] chore: OOPS --- agent/agentcontainers/devcontainercli.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 1de40401da693..5a9460c945187 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -421,7 +421,7 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { // outcome is a non-empty string. if logLine.Level == 0 { var lastLine devcontainerCLIResult - if json.Unmarshal(line, &lastLine); err == nil && lastLine.Outcome != "" { + if err := json.Unmarshal(line, &lastLine); err == nil && lastLine.Outcome != "" { _, _ = l.writer.Write(line) _, _ = l.writer.Write([]byte{'\n'}) } From 4d40ef226424b8ad78cff1ee31d8218cb98c3978 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 20:30:00 +0000 Subject: [PATCH 11/23] chore: 1 -> 2 --- coderd/workspaceagents_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 67bd6ce06b23a..766e069c89a7f 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1432,7 +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().ReadConfig(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return(agentcontainers.DevcontainerConfig{}, nil).Times(2) mdccli.EXPECT().Up(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return("someid", nil).Times(1) return 0 }, From ce32e2e2d9c06651936311359916cc7623b9c656 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 21:22:43 +0000 Subject: [PATCH 12/23] chore: add a status to the timings --- agent/agent.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/agent/agent.go b/agent/agent.go index 3bf83091a94b9..b99e7be075adf 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1238,9 +1238,11 @@ func (a *agent) createDevcontainer( var ( exitCode = int32(0) startTime = a.clock.Now() + status = proto.Timing_OK ) if err = cAPI.CreateDevcontainer(dc); err != nil { exitCode = 1 + status = proto.Timing_EXIT_FAILURE } endTime := a.clock.Now() @@ -1251,6 +1253,7 @@ func (a *agent) createDevcontainer( End: timestamppb.New(endTime), ExitCode: exitCode, Stage: proto.Timing_START, + Status: status, }, }); scriptErr != nil { a.logger.Warn(ctx, "reporting script completed failed", slog.Error(scriptErr)) From 32ac48a4eb3623dcfe5ca0a083730c6098eaedcb Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 21:47:20 +0000 Subject: [PATCH 13/23] chore: initialize containerapi even earlier --- agent/agent.go | 60 +++++++++++++++--------------------- agent/agentcontainers/api.go | 20 ++++++++++-- agent/api.go | 13 ++------ 3 files changed, 46 insertions(+), 47 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index b99e7be075adf..147958a04ce86 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -279,7 +279,7 @@ type agent struct { devcontainers bool containerAPIOptions []agentcontainers.Option - containerAPI atomic.Pointer[agentcontainers.API] + containerAPI *agentcontainers.API } func (a *agent) TailnetConn() *tailnet.Conn { @@ -336,6 +336,17 @@ func (a *agent) init() { // will not report anywhere. a.scriptRunner.RegisterMetrics(a.prometheusRegistry) + containerAPIOpts := []agentcontainers.Option{ + agentcontainers.WithExecer(a.execer), + agentcontainers.WithCommandEnv(a.sshServer.CommandEnv), + agentcontainers.WithScriptLogger(func(logSourceID uuid.UUID) agentcontainers.ScriptLogger { + return a.logSender.GetScriptLogger(logSourceID) + }), + } + containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...) + + a.containerAPI = agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) + a.reconnectingPTYServer = reconnectingpty.NewServer( a.logger.Named("reconnecting-pty"), a.sshServer, @@ -1152,19 +1163,11 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, devcontainerScripts map[uuid.UUID]codersdk.WorkspaceAgentScript ) if a.devcontainers { - containerAPIOpts := []agentcontainers.Option{ - agentcontainers.WithExecer(a.execer), - agentcontainers.WithCommandEnv(a.sshServer.CommandEnv), - agentcontainers.WithScriptLogger(func(logSourceID uuid.UUID) agentcontainers.ScriptLogger { - return a.logSender.GetScriptLogger(logSourceID) - }), + a.containerAPI.Init( agentcontainers.WithManifestInfo(manifest.OwnerName, manifest.WorkspaceName), agentcontainers.WithDevcontainers(manifest.Devcontainers, scripts), agentcontainers.WithSubAgentClient(agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI)), - } - containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...) - - a.containerAPI.Store(agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...)) + ) scripts, devcontainerScripts = agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, scripts) } @@ -1186,11 +1189,9 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // autostarted devcontainer will be included in this time. err := a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecuteStartScripts) - if cAPI := a.containerAPI.Load(); cAPI != nil { - for _, dc := range manifest.Devcontainers { - cErr := a.createDevcontainer(ctx, cAPI, aAPI, dc, devcontainerScripts[dc.ID]) - err = errors.Join(err, cErr) - } + for _, dc := range manifest.Devcontainers { + cErr := a.createDevcontainer(ctx, aAPI, dc, devcontainerScripts[dc.ID]) + err = errors.Join(err, cErr) } dur := time.Since(start).Seconds() @@ -1211,14 +1212,6 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } a.metrics.startupScriptSeconds.WithLabelValues(label).Set(dur) a.scriptRunner.StartCron() - - // If the container API is enabled, trigger an immediate refresh - // for quick sub agent injection. - if cAPI := a.containerAPI.Load(); cAPI != nil { - if err := cAPI.RefreshContainers(ctx); err != nil { - a.logger.Error(ctx, "failed to refresh containers", slog.Error(err)) - } - } }) if err != nil { return xerrors.Errorf("track conn goroutine: %w", err) @@ -1230,7 +1223,6 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, func (a *agent) createDevcontainer( ctx context.Context, - cAPI *agentcontainers.API, aAPI proto.DRPCAgentClient26, dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript, @@ -1240,7 +1232,7 @@ func (a *agent) createDevcontainer( startTime = a.clock.Now() status = proto.Timing_OK ) - if err = cAPI.CreateDevcontainer(dc); err != nil { + if err = a.containerAPI.CreateDevcontainer(dc); err != nil { exitCode = 1 status = proto.Timing_EXIT_FAILURE } @@ -1318,10 +1310,8 @@ func (a *agent) createOrUpdateNetwork(manifestOK, networkOK *checkpoint) func(co network.SetBlockEndpoints(manifest.DisableDirectConnections) // Update the subagent client if the container API is available. - if cAPI := a.containerAPI.Load(); cAPI != nil { - client := agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI) - cAPI.UpdateSubAgentClient(client) - } + client := agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI) + a.containerAPI.UpdateSubAgentClient(client) } return nil } @@ -1570,10 +1560,7 @@ func (a *agent) createTailnet( }() if err = a.trackGoroutine(func() { defer apiListener.Close() - apiHandler, closeAPIHAndler := a.apiHandler() - defer func() { - _ = closeAPIHAndler() - }() + apiHandler := a.apiHandler() server := &http.Server{ BaseContext: func(net.Listener) context.Context { return ctx }, Handler: apiHandler, @@ -1587,7 +1574,6 @@ func (a *agent) createTailnet( case <-ctx.Done(): case <-a.hardCtx.Done(): } - _ = closeAPIHAndler() _ = server.Close() }() @@ -1926,6 +1912,10 @@ func (a *agent) Close() error { a.logger.Error(a.hardCtx, "script runner close", slog.Error(err)) } + if err := a.containerAPI.Close(); err != nil { + a.logger.Error(a.hardCtx, "container API close", slog.Error(err)) + } + // Wait for the graceful shutdown to complete, but don't wait forever so // that we don't break user expectations. go func() { diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index f120ceb16594a..a89ea85d042b2 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -51,6 +51,7 @@ const ( type API struct { ctx context.Context cancel context.CancelFunc + initialized chan struct{} watcherDone chan struct{} updaterDone chan struct{} initialUpdateDone chan struct{} // Closed after first update in updaterLoop. @@ -265,6 +266,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API { api := &API{ ctx: ctx, cancel: cancel, + initialized: make(chan struct{}), watcherDone: make(chan struct{}), updaterDone: make(chan struct{}), initialUpdateDone: make(chan struct{}), @@ -315,12 +317,26 @@ func NewAPI(logger slog.Logger, options ...Option) *API { api.subAgentClient.Store(&c) } - go api.watcherLoop() - go api.updaterLoop() + go func() { + select { + case <-api.ctx.Done(): + break + case <-api.initialized: + go api.watcherLoop() + go api.updaterLoop() + } + }() return api } +func (api *API) Init(opts ...Option) { + for _, opt := range opts { + opt(api) + } + close(api.initialized) +} + func (api *API) watcherLoop() { defer close(api.watcherDone) defer api.logger.Debug(api.ctx, "watcher loop stopped") diff --git a/agent/api.go b/agent/api.go index ea3b990c7908a..c6b1af7347bcd 100644 --- a/agent/api.go +++ b/agent/api.go @@ -11,7 +11,7 @@ import ( "github.com/coder/coder/v2/codersdk" ) -func (a *agent) apiHandler() (http.Handler, func() error) { +func (a *agent) apiHandler() http.Handler { r := chi.NewRouter() r.Get("/", func(rw http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{ @@ -37,9 +37,7 @@ func (a *agent) apiHandler() (http.Handler, func() error) { } if a.devcontainers { - if cAPI := a.containerAPI.Load(); cAPI != nil { - r.Mount("/api/v0/containers", cAPI.Routes()) - } + r.Mount("/api/v0/containers", a.containerAPI.Routes()) } else { r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusForbidden, codersdk.Response{ @@ -60,12 +58,7 @@ func (a *agent) apiHandler() (http.Handler, func() error) { r.Get("/debug/manifest", a.HandleHTTPDebugManifest) r.Get("/debug/prometheus", promHandler.ServeHTTP) - return r, func() error { - if containerAPI := a.containerAPI.Load(); containerAPI != nil { - return containerAPI.Close() - } - return nil - } + return r } type listeningPortsHandler struct { From 738b755bb329e3dc264a86b30ddfbf05b0611de1 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 22:17:48 +0000 Subject: [PATCH 14/23] chore: only enable when devcontainers are enabled --- agent/agent.go | 35 ++++++++++++++++++++++------------- agent/agentcontainers/api.go | 2 ++ 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 147958a04ce86..2830e6d51d527 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -336,16 +336,18 @@ func (a *agent) init() { // will not report anywhere. a.scriptRunner.RegisterMetrics(a.prometheusRegistry) - containerAPIOpts := []agentcontainers.Option{ - agentcontainers.WithExecer(a.execer), - agentcontainers.WithCommandEnv(a.sshServer.CommandEnv), - agentcontainers.WithScriptLogger(func(logSourceID uuid.UUID) agentcontainers.ScriptLogger { - return a.logSender.GetScriptLogger(logSourceID) - }), - } - containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...) + if a.devcontainers { + containerAPIOpts := []agentcontainers.Option{ + agentcontainers.WithExecer(a.execer), + agentcontainers.WithCommandEnv(a.sshServer.CommandEnv), + agentcontainers.WithScriptLogger(func(logSourceID uuid.UUID) agentcontainers.ScriptLogger { + return a.logSender.GetScriptLogger(logSourceID) + }), + } + containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...) - a.containerAPI = agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) + a.containerAPI = agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) + } a.reconnectingPTYServer = reconnectingpty.NewServer( a.logger.Named("reconnecting-pty"), @@ -1106,6 +1108,9 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, if a.devcontainers { a.logger.Info(ctx, "devcontainers are not supported on sub agents, disabling feature") a.devcontainers = false + if err := a.containerAPI.Close(); err != nil { + a.logger.Error(ctx, "disable container API", slog.Error(err)) + } } } a.client.RewriteDERPMap(manifest.DERPMap) @@ -1310,8 +1315,10 @@ func (a *agent) createOrUpdateNetwork(manifestOK, networkOK *checkpoint) func(co network.SetBlockEndpoints(manifest.DisableDirectConnections) // Update the subagent client if the container API is available. - client := agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI) - a.containerAPI.UpdateSubAgentClient(client) + if a.containerAPI != nil { + client := agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI) + a.containerAPI.UpdateSubAgentClient(client) + } } return nil } @@ -1912,8 +1919,10 @@ func (a *agent) Close() error { a.logger.Error(a.hardCtx, "script runner close", slog.Error(err)) } - if err := a.containerAPI.Close(); err != nil { - a.logger.Error(a.hardCtx, "container API close", slog.Error(err)) + if a.containerAPI != nil { + if err := a.containerAPI.Close(); err != nil { + a.logger.Error(a.hardCtx, "container API close", slog.Error(err)) + } } // Wait for the graceful shutdown to complete, but don't wait forever so diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index a89ea85d042b2..f5586b966dcd3 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1605,6 +1605,8 @@ func (api *API) Close() error { api.logger.Debug(api.ctx, "closing API") api.closed = true + close(api.initialized) + // Stop all running subagent processes and clean up. subAgentIDs := make([]uuid.UUID, 0, len(api.injectedSubAgentProcs)) for workspaceFolder, proc := range api.injectedSubAgentProcs { From 3714fec62efa5e092aca885a254d71bae1ff4847 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 22:30:42 +0000 Subject: [PATCH 15/23] chore: simplify things a little --- agent/agent.go | 3 --- agent/agentcontainers/api.go | 21 ++++++--------------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 2830e6d51d527..1c5a06af08f9d 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1108,9 +1108,6 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, if a.devcontainers { a.logger.Info(ctx, "devcontainers are not supported on sub agents, disabling feature") a.devcontainers = false - if err := a.containerAPI.Close(); err != nil { - a.logger.Error(ctx, "disable container API", slog.Error(err)) - } } } a.client.RewriteDERPMap(manifest.DERPMap) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index f5586b966dcd3..30fc0594d0fa1 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -51,7 +51,6 @@ const ( type API struct { ctx context.Context cancel context.CancelFunc - initialized chan struct{} watcherDone chan struct{} updaterDone chan struct{} initialUpdateDone chan struct{} // Closed after first update in updaterLoop. @@ -266,7 +265,6 @@ func NewAPI(logger slog.Logger, options ...Option) *API { api := &API{ ctx: ctx, cancel: cancel, - initialized: make(chan struct{}), watcherDone: make(chan struct{}), updaterDone: make(chan struct{}), initialUpdateDone: make(chan struct{}), @@ -317,24 +315,19 @@ func NewAPI(logger slog.Logger, options ...Option) *API { api.subAgentClient.Store(&c) } - go func() { - select { - case <-api.ctx.Done(): - break - case <-api.initialized: - go api.watcherLoop() - go api.updaterLoop() - } - }() - return api } +// Init applies a final set of options to the API and then +// begins the watcherLoop and updaterLoop. This function +// must only be called once. func (api *API) Init(opts ...Option) { for _, opt := range opts { opt(api) } - close(api.initialized) + + go api.watcherLoop() + go api.updaterLoop() } func (api *API) watcherLoop() { @@ -1605,8 +1598,6 @@ func (api *API) Close() error { api.logger.Debug(api.ctx, "closing API") api.closed = true - close(api.initialized) - // Stop all running subagent processes and clean up. subAgentIDs := make([]uuid.UUID, 0, len(api.injectedSubAgentProcs)) for workspaceFolder, proc := range api.injectedSubAgentProcs { From 996d440a98f4ac4b21bffdffbb7b9d7f8d8dd78c Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 22:33:26 +0000 Subject: [PATCH 16/23] chore: recreate -> create with argument --- agent/agentcontainers/api.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 30fc0594d0fa1..e9b17ef70e4ca 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -920,7 +920,7 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques api.knownDevcontainers[dc.WorkspaceFolder] = dc api.asyncWg.Add(1) go func() { - _ = api.recreateDevcontainer(dc, configPath) + _ = api.createDevcontainer(dc, configPath, true) }() api.mu.Unlock() @@ -939,17 +939,17 @@ func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer) error api.asyncWg.Add(1) api.mu.Unlock() - return api.recreateDevcontainer(dc, dc.ConfigPath) + return api.createDevcontainer(dc, dc.ConfigPath, false) } -// recreateDevcontainer should run in its own goroutine and is responsible for +// createDevcontainer should run in its own goroutine and is responsible for // recreating a devcontainer based on the provided devcontainer configuration. // It updates the devcontainer status and logs the process. The configPath is // passed as a parameter for the odd chance that the container being recreated // has a different config file than the one stored in the devcontainer state. // The devcontainer state must be set to starting and the asyncWg must be // incremented before calling this function. -func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string) error { +func (api *API) createDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string, restart bool) error { defer api.asyncWg.Done() var ( @@ -991,12 +991,17 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con logger.Debug(ctx, "starting devcontainer recreation") - _, err = api.dccli.Up(ctx, dc.WorkspaceFolder, configPath, WithUpOutput(infoW, errW), WithRemoveExistingContainer()) + upOptions := []DevcontainerCLIUpOptions{WithUpOutput(infoW, errW)} + if restart { + upOptions = append(upOptions, WithRemoveExistingContainer()) + } + + _, err = api.dccli.Up(ctx, dc.WorkspaceFolder, configPath, upOptions...) if err != nil { // No need to log if the API is closing (context canceled), as this // is expected behavior when the API is shutting down. if !errors.Is(err, context.Canceled) { - logger.Error(ctx, "devcontainer recreation failed", slog.Error(err)) + logger.Error(ctx, "devcontainer creation failed", slog.Error(err)) } api.mu.Lock() @@ -1009,7 +1014,7 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con return xerrors.Errorf("start devcontainer: %w", err) } - logger.Info(ctx, "devcontainer recreated successfully") + logger.Info(ctx, "devcontainer created successfully") api.mu.Lock() dc = api.knownDevcontainers[dc.WorkspaceFolder] @@ -1032,7 +1037,7 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con // Ensure an immediate refresh to accurately reflect the // devcontainer state after recreation. if err := api.RefreshContainers(ctx); err != nil { - logger.Error(ctx, "failed to trigger immediate refresh after devcontainer recreation", slog.Error(err)) + logger.Error(ctx, "failed to trigger immediate refresh after devcontainer creation", slog.Error(err)) return xerrors.Errorf("refresh containers: %w", err) } From ae5dd1e086908232e940beb5bc00396267c13fd6 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 22:43:46 +0000 Subject: [PATCH 17/23] chore: ensure we close and init --- agent/agentcontainers/api.go | 13 +++++++++---- agent/agentcontainers/api_test.go | 12 ++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index e9b17ef70e4ca..8a5f9cb89e386 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -265,8 +265,6 @@ func NewAPI(logger slog.Logger, options ...Option) *API { api := &API{ ctx: ctx, cancel: cancel, - watcherDone: make(chan struct{}), - updaterDone: make(chan struct{}), initialUpdateDone: make(chan struct{}), updateTrigger: make(chan chan error), updateInterval: defaultUpdateInterval, @@ -326,6 +324,9 @@ func (api *API) Init(opts ...Option) { opt(api) } + api.watcherDone = make(chan struct{}) + api.updaterDone = make(chan struct{}) + go api.watcherLoop() go api.updaterLoop() } @@ -1640,8 +1641,12 @@ func (api *API) Close() error { err := api.watcher.Close() // Wait for loops to finish. - <-api.watcherDone - <-api.updaterDone + if api.watcherDone != nil { + <-api.watcherDone + } + if api.updaterDone != nil { + <-api.updaterDone + } // Wait for all async tasks to complete. api.asyncWg.Wait() diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index b6bae46c835c9..889600ba0280d 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -437,6 +437,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithContainerCLI(mLister), agentcontainers.WithContainerLabelIncludeFilter("this.label.does.not.exist.ignore.devcontainers", "true"), ) + api.Init() defer api.Close() r.Mount("/", api.Routes()) @@ -614,6 +615,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI), agentcontainers.WithWatcher(watcher.NewNoop()), ) + api.Init() defer api.Close() r.Mount("/", api.Routes()) @@ -1027,6 +1029,7 @@ func TestAPI(t *testing.T) { } api := agentcontainers.NewAPI(logger, apiOptions...) + api.Init() defer api.Close() r.Mount("/", api.Routes()) @@ -1111,6 +1114,7 @@ func TestAPI(t *testing.T) { []codersdk.WorkspaceAgentScript{{LogSourceID: uuid.New(), ID: dc.ID}}, ), ) + api.Init() defer api.Close() // Make sure the ticker function has been registered @@ -1206,6 +1210,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithWatcher(fWatcher), agentcontainers.WithClock(mClock), ) + api.Init() defer api.Close() r := chi.NewRouter() @@ -1358,6 +1363,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithDevcontainerCLI(fakeDCCLI), agentcontainers.WithManifestInfo("test-user", "test-workspace"), ) + api.Init() apiClose := func() { closeOnce.Do(func() { // Close before api.Close() defer to avoid deadlock after test. @@ -1578,6 +1584,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithSubAgentClient(fakeSAC), agentcontainers.WithDevcontainerCLI(&fakeDevcontainerCLI{}), ) + api.Init() defer api.Close() tickerTrap.MustWait(ctx).MustRelease(ctx) @@ -1899,6 +1906,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithSubAgentURL("test-subagent-url"), agentcontainers.WithWatcher(watcher.NewNoop()), ) + api.Init() defer api.Close() // Close before api.Close() defer to avoid deadlock after test. @@ -1991,6 +1999,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithSubAgentURL("test-subagent-url"), agentcontainers.WithWatcher(watcher.NewNoop()), ) + api.Init() defer api.Close() // Close before api.Close() defer to avoid deadlock after test. @@ -2045,6 +2054,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithExecer(fakeExec), agentcontainers.WithCommandEnv(commandEnv), ) + api.Init() defer api.Close() // Call RefreshContainers directly to trigger CommandEnv usage. @@ -2134,6 +2144,7 @@ func TestAPI(t *testing.T) { agentcontainers.WithWatcher(fWatcher), agentcontainers.WithClock(mClock), ) + api.Init() defer func() { close(fakeSAC.createErrC) close(fakeSAC.deleteErrC) @@ -2334,6 +2345,7 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { agentcontainers.WithSubAgentClient(fSAC), agentcontainers.WithWatcher(watcher.NewNoop()), ) + api.Init() defer api.Close() tickerTrap.MustWait(ctx).MustRelease(ctx) From 9d76cf6053020a0339ce162394a3b045e4358ca6 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 22:53:35 +0000 Subject: [PATCH 18/23] chore: appease linter --- agent/agent.go | 2 +- agent/agentcontainers/api.go | 29 +++++++++++++++++------------ 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 1c5a06af08f9d..b0d1d113736c7 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1234,7 +1234,7 @@ func (a *agent) createDevcontainer( startTime = a.clock.Now() status = proto.Timing_OK ) - if err = a.containerAPI.CreateDevcontainer(dc); err != nil { + if err = a.containerAPI.CreateDevcontainer(dc, dc.ConfigPath, agentcontainers.CreateBehaviorStart); err != nil { exitCode = 1 status = proto.Timing_EXIT_FAILURE } diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 8a5f9cb89e386..c8fdc990579ae 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -921,7 +921,7 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques api.knownDevcontainers[dc.WorkspaceFolder] = dc api.asyncWg.Add(1) go func() { - _ = api.createDevcontainer(dc, configPath, true) + _ = api.CreateDevcontainer(dc, configPath, CreateBehaviorRestart) }() api.mu.Unlock() @@ -932,16 +932,12 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques }) } -func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer) error { - api.mu.Lock() - dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting - dc.Container = nil - api.knownDevcontainers[dc.WorkspaceFolder] = dc - api.asyncWg.Add(1) - api.mu.Unlock() +type CreateBehavior bool - return api.createDevcontainer(dc, dc.ConfigPath, false) -} +const ( + CreateBehaviorStart CreateBehavior = false + CreateBehaviorRestart CreateBehavior = true +) // createDevcontainer should run in its own goroutine and is responsible for // recreating a devcontainer based on the provided devcontainer configuration. @@ -950,9 +946,18 @@ func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer) error // has a different config file than the one stored in the devcontainer state. // The devcontainer state must be set to starting and the asyncWg must be // incremented before calling this function. -func (api *API) createDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string, restart bool) error { +func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string, behavior CreateBehavior) error { defer api.asyncWg.Done() + if behavior == CreateBehaviorStart { + api.mu.Lock() + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting + dc.Container = nil + api.knownDevcontainers[dc.WorkspaceFolder] = dc + api.asyncWg.Add(1) + api.mu.Unlock() + } + var ( err error ctx = api.ctx @@ -993,7 +998,7 @@ func (api *API) createDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, confi logger.Debug(ctx, "starting devcontainer recreation") upOptions := []DevcontainerCLIUpOptions{WithUpOutput(infoW, errW)} - if restart { + if behavior == CreateBehaviorRestart { upOptions = append(upOptions, WithRemoveExistingContainer()) } From 7285c39f5dcebf2353175a0dd3755a01c6ba80ae Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 23:05:03 +0000 Subject: [PATCH 19/23] chore: mock ReadConfig any time --- coderd/workspaceagents_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 766e069c89a7f..4559131e98d0a 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1432,7 +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(2) + mdccli.EXPECT().ReadConfig(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return(agentcontainers.DevcontainerConfig{}, nil).AnyTimes() mdccli.EXPECT().Up(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return("someid", nil).Times(1) return 0 }, From 3fce51ca9360a337ce4484b8605a396ab70d1efb Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 25 Jun 2025 08:27:41 +0000 Subject: [PATCH 20/23] chore: feedback --- agent/agent.go | 2 +- agent/agentcontainers/api.go | 37 +++++++++++++++++------------------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index b0d1d113736c7..8d3766dae89ad 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1234,7 +1234,7 @@ func (a *agent) createDevcontainer( startTime = a.clock.Now() status = proto.Timing_OK ) - if err = a.containerAPI.CreateDevcontainer(dc, dc.ConfigPath, agentcontainers.CreateBehaviorStart); err != nil { + if err = a.containerAPI.CreateDevcontainer(dc.WorkspaceFolder, dc.ConfigPath); err != nil { exitCode = 1 status = proto.Timing_EXIT_FAILURE } diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index c8fdc990579ae..9eaaef54d9b9e 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -207,6 +207,10 @@ func WithDevcontainers(devcontainers []codersdk.WorkspaceAgentDevcontainer, scri api.devcontainerNames = make(map[string]bool, len(devcontainers)) api.devcontainerLogSourceIDs = make(map[string]uuid.UUID) for _, dc := range devcontainers { + if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopped { + dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting + } + api.knownDevcontainers[dc.WorkspaceFolder] = dc api.devcontainerNames[dc.Name] = true for _, script := range scripts { @@ -320,6 +324,12 @@ func NewAPI(logger slog.Logger, options ...Option) *API { // begins the watcherLoop and updaterLoop. This function // must only be called once. func (api *API) Init(opts ...Option) { + api.mu.Lock() + defer api.mu.Unlock() + if api.closed { + return + } + for _, opt := range opts { opt(api) } @@ -919,9 +929,8 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting dc.Container = nil api.knownDevcontainers[dc.WorkspaceFolder] = dc - api.asyncWg.Add(1) go func() { - _ = api.CreateDevcontainer(dc, configPath, CreateBehaviorRestart) + _ = api.CreateDevcontainer(dc.WorkspaceFolder, configPath, WithRemoveExistingContainer()) }() api.mu.Unlock() @@ -932,13 +941,6 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques }) } -type CreateBehavior bool - -const ( - CreateBehaviorStart CreateBehavior = false - CreateBehaviorRestart CreateBehavior = true -) - // createDevcontainer should run in its own goroutine and is responsible for // recreating a devcontainer based on the provided devcontainer configuration. // It updates the devcontainer status and logs the process. The configPath is @@ -946,16 +948,13 @@ const ( // has a different config file than the one stored in the devcontainer state. // The devcontainer state must be set to starting and the asyncWg must be // incremented before calling this function. -func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string, behavior CreateBehavior) error { +func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...DevcontainerCLIUpOptions) error { + api.asyncWg.Add(1) defer api.asyncWg.Done() - if behavior == CreateBehaviorStart { - api.mu.Lock() - dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting - dc.Container = nil - api.knownDevcontainers[dc.WorkspaceFolder] = dc - api.asyncWg.Add(1) - api.mu.Unlock() + dc, found := api.knownDevcontainers[workspaceFolder] + if !found { + return xerrors.Errorf("no devcontainer found") } var ( @@ -998,9 +997,7 @@ func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, confi logger.Debug(ctx, "starting devcontainer recreation") upOptions := []DevcontainerCLIUpOptions{WithUpOutput(infoW, errW)} - if behavior == CreateBehaviorRestart { - upOptions = append(upOptions, WithRemoveExistingContainer()) - } + upOptions = append(upOptions, opts...) _, err = api.dccli.Up(ctx, dc.WorkspaceFolder, configPath, upOptions...) if err != nil { From 54aa84a67a57391af85806b339852cbecfc54dc0 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 25 Jun 2025 08:49:00 +0000 Subject: [PATCH 21/23] chore: feedback --- agent/agentcontainers/api.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 9eaaef54d9b9e..d01275f690280 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -949,14 +949,22 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques // The devcontainer state must be set to starting and the asyncWg must be // incremented before calling this function. func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...DevcontainerCLIUpOptions) error { - api.asyncWg.Add(1) - defer api.asyncWg.Done() + api.mu.Lock() + if api.closed { + api.mu.Unlock() + return nil + } dc, found := api.knownDevcontainers[workspaceFolder] if !found { + api.mu.Unlock() return xerrors.Errorf("no devcontainer found") } + api.asyncWg.Add(1) + defer api.asyncWg.Done() + api.mu.Unlock() + var ( err error ctx = api.ctx From 3d08d0e01ea2a196565781a8d784dffa856ff71b Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 25 Jun 2025 10:27:16 +0000 Subject: [PATCH 22/23] chore: only set status if not set, and run create in test --- agent/agentcontainers/api.go | 2 +- agent/agentcontainers/api_test.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index d01275f690280..c0adba84f0799 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -207,7 +207,7 @@ func WithDevcontainers(devcontainers []codersdk.WorkspaceAgentDevcontainer, scri api.devcontainerNames = make(map[string]bool, len(devcontainers)) api.devcontainerLogSourceIDs = make(map[string]uuid.UUID) for _, dc := range devcontainers { - if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopped { + if dc.Status == "" { dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting } diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 889600ba0280d..446bb472edefd 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -1012,6 +1012,7 @@ func TestAPI(t *testing.T) { apiOptions := []agentcontainers.Option{ agentcontainers.WithClock(mClock), agentcontainers.WithContainerCLI(tt.lister), + agentcontainers.WithDevcontainerCLI(&fakeDevcontainerCLI{}), agentcontainers.WithWatcher(watcher.NewNoop()), } @@ -1041,6 +1042,11 @@ func TestAPI(t *testing.T) { tickerTrap.MustWait(ctx).MustRelease(ctx) tickerTrap.Close() + for _, dc := range tt.knownDevcontainers { + err := api.CreateDevcontainer(dc.WorkspaceFolder, dc.ConfigPath) + require.NoError(t, err) + } + // Advance the clock to run the updater loop. _, aw := mClock.AdvanceNext() aw.MustWait(ctx) From fa479fc48bdcfdfa5545cea598b2f4b365cee544 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 25 Jun 2025 10:37:23 +0000 Subject: [PATCH 23/23] chore: feedback on poor error message --- 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 c0adba84f0799..27a29bd535185 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -958,7 +958,7 @@ func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...D dc, found := api.knownDevcontainers[workspaceFolder] if !found { api.mu.Unlock() - return xerrors.Errorf("no devcontainer found") + return xerrors.Errorf("devcontainer not found") } api.asyncWg.Add(1)