Skip to content

fix(agent): fix script filtering for devcontainers #18635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 27, 2025
Merged

fix(agent): fix script filtering for devcontainers #18635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,15 +1158,13 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
}
}

var (
scripts = manifest.Scripts
scriptRunnerOpts []agentscripts.InitOption
devcontainerScripts map[uuid.UUID]codersdk.WorkspaceAgentScript
)
scripts := manifest.Scripts
if a.containerAPI != nil {
scripts, devcontainerScripts = agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, scripts)
// Since devcontainer are enabled, remove devcontainer scripts
// from the main scripts list to avoid showing an error.
scripts, _ = agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, manifest.Scripts)
}
err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted, scriptRunnerOpts...)
err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted)
if err != nil {
return xerrors.Errorf("init script runner: %w", err)
}
Expand All @@ -1187,10 +1185,11 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
if a.containerAPI != nil {
a.containerAPI.Init(
agentcontainers.WithManifestInfo(manifest.OwnerName, manifest.WorkspaceName, manifest.AgentName),
agentcontainers.WithDevcontainers(manifest.Devcontainers, scripts),
agentcontainers.WithDevcontainers(manifest.Devcontainers, manifest.Scripts),
agentcontainers.WithSubAgentClient(agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI)),
)

_, devcontainerScripts := agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, manifest.Scripts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to call this function twice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly I wanted to reduce variable scope to avoid similar issues as introduced in the previous PR. And make it clear that scripts from earlier shouldn't be used here. It's a lightweight call but I can revert it if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no issue with the duplication just wanted to ensure I knew the reason why, all good!

for _, dc := range manifest.Devcontainers {
cErr := a.createDevcontainer(ctx, aAPI, dc, devcontainerScripts[dc.ID])
err = errors.Join(err, cErr)
Expand Down
28 changes: 13 additions & 15 deletions agent/agentcontainers/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,14 @@ type API struct {
containersErr error // Error from the last list operation.
devcontainerNames map[string]bool // By devcontainer name.
knownDevcontainers map[string]codersdk.WorkspaceAgentDevcontainer // By workspace folder.
devcontainerLogSourceIDs map[string]uuid.UUID // By workspace folder.
configFileModifiedTimes map[string]time.Time // By config file path.
recreateSuccessTimes map[string]time.Time // By workspace folder.
recreateErrorTimes map[string]time.Time // By workspace folder.
injectedSubAgentProcs map[string]subAgentProcess // By workspace folder.
usingWorkspaceFolderName map[string]bool // By workspace folder.
ignoredDevcontainers map[string]bool // By workspace folder. Tracks three states (true, false and not checked).
asyncWg sync.WaitGroup

devcontainerLogSourceIDs map[string]uuid.UUID // By workspace folder.
}

type subAgentProcess struct {
Expand Down Expand Up @@ -935,12 +934,7 @@ func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...D
return xerrors.Errorf("devcontainer not found")
}

api.asyncWg.Add(1)
defer api.asyncWg.Done()
api.mu.Unlock()

var (
err error
ctx = api.ctx
logger = api.logger.With(
slog.F("devcontainer_id", dc.ID),
Expand All @@ -950,19 +944,23 @@ func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...D
)
)

if dc.ConfigPath != configPath {
logger.Warn(ctx, "devcontainer config path mismatch",
slog.F("config_path_param", configPath),
)
}

// Send logs via agent logging facilities.
logSourceID := api.devcontainerLogSourceIDs[dc.WorkspaceFolder]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: We were accessing this outside the mutex so I moved it just in case due to change in how we Init() the API which allows part of the API to be used before Init() is called with input that changes this map.

if logSourceID == uuid.Nil {
// Fallback to the external log source ID if not found.
api.logger.Debug(api.ctx, "devcontainer log source ID not found, falling back to external log source ID")
logSourceID = agentsdk.ExternalLogSourceID
}

api.asyncWg.Add(1)
defer api.asyncWg.Done()
api.mu.Unlock()

if dc.ConfigPath != configPath {
logger.Warn(ctx, "devcontainer config path mismatch",
slog.F("config_path_param", configPath),
)
}

scriptLogger := api.scriptLogger(logSourceID)
defer func() {
flushCtx, cancel := context.WithTimeout(api.ctx, 5*time.Second)
Expand All @@ -981,7 +979,7 @@ func (api *API) CreateDevcontainer(workspaceFolder, configPath string, opts ...D
upOptions := []DevcontainerCLIUpOptions{WithUpOutput(infoW, errW)}
upOptions = append(upOptions, opts...)

_, err = api.dccli.Up(ctx, dc.WorkspaceFolder, configPath, upOptions...)
_, 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.
Expand Down
Loading