Skip to content

Commit e72c98f

Browse files
mafredrimafredri
committed
rewrite
1 parent 1286898 commit e72c98f

File tree

2 files changed

+42
-44
lines changed

2 files changed

+42
-44
lines changed

‎agent/agentcontainers/api.go

Lines changed: 41 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,7 +1042,7 @@ func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) {
10421042
logger.Info(api.ctx, "marking devcontainer as dirty")
10431043
dc.Dirty = true
10441044
}
1045-
if api.ignoredDevcontainers[dc.WorkspaceFolder] {
1045+
if _, ok := api.ignoredDevcontainers[dc.WorkspaceFolder]; ok {
10461046
logger.Debug(api.ctx, "clearing devcontainer ignored state")
10471047
delete(api.ignoredDevcontainers, dc.WorkspaceFolder) // Allow re-reading config.
10481048
}
@@ -1051,37 +1051,6 @@ func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) {
10511051
}
10521052
}
10531053

1054-
// markDevcontainerIgnored marks a devcontainer as ignored. Must not be called
1055-
// with the API lock held.
1056-
func (api *API) markDevcontainerIgnored(ctx context.Context, dc codersdk.WorkspaceAgentDevcontainer, proc subAgentProcess) subAgentProcess {
1057-
logger := api.logger.With(
1058-
slog.F("devcontainer_id", dc.ID),
1059-
slog.F("devcontainer_name", dc.Name),
1060-
slog.F("workspace_folder", dc.WorkspaceFolder),
1061-
slog.F("config_path", dc.ConfigPath),
1062-
)
1063-
1064-
// We only allow ignore to be set in the root customization layer to
1065-
// prevent weird interactions with devcontainer features.
1066-
logger.Debug(ctx, "marking devcontainer as ignored")
1067-
proc.stop()
1068-
if proc.agent.ID != uuid.Nil {
1069-
// If we stop the subagent, we also need to delete it.
1070-
client := *api.subAgentClient.Load()
1071-
if err := client.Delete(ctx, proc.agent.ID); err != nil {
1072-
api.logger.Error(ctx, "delete subagent failed", slog.Error(err), slog.F("subagent_id", proc.agent.ID))
1073-
}
1074-
}
1075-
// Reset agent and containerID to force config re-reading if ignore is toggled.
1076-
proc.agent = SubAgent{}
1077-
proc.containerID = ""
1078-
api.mu.Lock()
1079-
api.ignoredDevcontainers[dc.WorkspaceFolder] = true
1080-
api.mu.Unlock()
1081-
1082-
return proc
1083-
}
1084-
10851054
// cleanupSubAgents removes subagents that are no longer managed by
10861055
// this agent. This is usually only run at startup to ensure a clean
10871056
// slate. This method has an internal timeout to prevent blocking
@@ -1172,11 +1141,24 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
11721141
return xerrors.Errorf("read devcontainer config: %w", err)
11731142
}
11741143

1175-
if config.Configuration.Customizations.Coder.Ignore {
1176-
api.mu.Unlock()
1177-
proc = api.markDevcontainerIgnored(ctx, dc, proc)
1178-
api.mu.Lock()
1144+
ignore := config.Configuration.Customizations.Coder.Ignore
1145+
if ignore {
1146+
proc.stop()
1147+
if proc.agent.ID != uuid.Nil {
1148+
// Unlock while doing the delete operation.
1149+
api.mu.Unlock()
1150+
client := *api.subAgentClient.Load()
1151+
if err := client.Delete(ctx, proc.agent.ID); err != nil {
1152+
api.mu.Lock()
1153+
return xerrors.Errorf("delete subagent: %w", err)
1154+
}
1155+
api.mu.Lock()
1156+
}
1157+
// Reset agent and containerID to force config re-reading if ignore is toggled.
1158+
proc.agent = SubAgent{}
1159+
proc.containerID = ""
11791160
api.injectedSubAgentProcs[dc.WorkspaceFolder] = proc
1161+
api.ignoredDevcontainers[dc.WorkspaceFolder] = ignore
11801162
return nil
11811163
}
11821164
}
@@ -1219,7 +1201,11 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
12191201
ranSubAgent := false
12201202

12211203
// Clean up if injection fails.
1204+
var ignored, setIgnored bool
12221205
defer func() {
1206+
if setIgnored {
1207+
api.ignoredDevcontainers[dc.WorkspaceFolder] = ignored
1208+
}
12231209
if !ranSubAgent {
12241210
proc.stop()
12251211
if !api.closed {
@@ -1273,7 +1259,6 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
12731259
}
12741260

12751261
var (
1276-
ignore bool
12771262
appsWithPossibleDuplicates []SubAgentApp
12781263
workspaceFolder = DevcontainerDefaultContainerWorkspaceFolder
12791264
)
@@ -1297,13 +1282,15 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
12971282
return err
12981283
}
12991284

1300-
ignore = config.Configuration.Customizations.Coder.Ignore
1301-
workspaceFolder = config.Workspace.WorkspaceFolder
1302-
1303-
if ignore {
1285+
// We only allow ignore to be set in the root customization layer to
1286+
// prevent weird interactions with devcontainer features.
1287+
ignored, setIgnored = config.Configuration.Customizations.Coder.Ignore, true
1288+
if ignored {
13041289
return nil
13051290
}
13061291

1292+
workspaceFolder = config.Workspace.WorkspaceFolder
1293+
13071294
// NOTE(DanielleMaywood):
13081295
// We only want to take an agent name specified in the root customization layer.
13091296
// This restricts the ability for a feature to specify the agent name. We may revisit
@@ -1350,8 +1337,19 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
13501337
api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err))
13511338
}
13521339

1353-
if ignore {
1354-
proc = api.markDevcontainerIgnored(ctx, dc, proc)
1340+
if ignored {
1341+
proc.stop()
1342+
if proc.agent.ID != uuid.Nil {
1343+
// If we stop the subagent, we also need to delete it.
1344+
client := *api.subAgentClient.Load()
1345+
if err := client.Delete(ctx, proc.agent.ID); err != nil {
1346+
return xerrors.Errorf("delete subagent: %w", err)
1347+
}
1348+
}
1349+
// Reset agent and containerID to force config re-reading if
1350+
// ignore is toggled.
1351+
proc.agent = SubAgent{}
1352+
proc.containerID = ""
13551353
return nil
13561354
}
13571355

‎agent/agentcontainers/api_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2234,7 +2234,7 @@ func TestAPI(t *testing.T) {
22342234

22352235
assert.Empty(t, response.Devcontainers, "devcontainer should be filtered out when ignore=true again")
22362236
assert.Len(t, response.Containers, 1, "regular container should still be listed")
2237-
assert.Len(t, fakeSAC.deleted, 1, "sub agent should be deleted when ignore=true")
2237+
require.Len(t, fakeSAC.deleted, 1, "sub agent should be deleted when ignore=true")
22382238
assert.Equal(t, createdAgentID, fakeSAC.deleted[0], "the same sub agent that was created should be deleted")
22392239
})
22402240
}

0 commit comments

Comments
 (0)