Skip to content

Commit fcf9371

Browse files
DanielleMaywoodDanielleMaywood
authored
feat(agent/agentcontainers): retry with longer name on failure (#18513)
Closes coder/internal#732 We now try (up to 5 times) when attempting to create an agent using the workspace folder as the name. It is important to note this flow is only ever ran when attempting to create an agent using the workspace folder as the name. If a deployment uses terraform or the devcontainer customization, we do not fall back to this approach.
1 parent 4fd0312 commit fcf9371

File tree

3 files changed

+403
-22
lines changed

3 files changed

+403
-22
lines changed

‎agent/agentcontainers/api.go

Lines changed: 111 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ const (
4242
// read-write, which seems sensible for devcontainers.
4343
coderPathInsideContainer = "/.coder-agent/coder"
4444

45-
maxAgentNameLength = 64
45+
maxAgentNameLength = 64
46+
maxAttemptsToNameAgent = 5
4647
)
4748

4849
// API is responsible for container-related operations in the agent.
@@ -71,17 +72,18 @@ type API struct {
7172
ownerName string
7273
workspaceName string
7374

74-
mu sync.RWMutex
75-
closed bool
76-
containers codersdk.WorkspaceAgentListContainersResponse // Output from the last list operation.
77-
containersErr error // Error from the last list operation.
78-
devcontainerNames map[string]bool // By devcontainer name.
79-
knownDevcontainers map[string]codersdk.WorkspaceAgentDevcontainer // By workspace folder.
80-
configFileModifiedTimes map[string]time.Time // By config file path.
81-
recreateSuccessTimes map[string]time.Time // By workspace folder.
82-
recreateErrorTimes map[string]time.Time // By workspace folder.
83-
injectedSubAgentProcs map[string]subAgentProcess // By workspace folder.
84-
asyncWg sync.WaitGroup
75+
mu sync.RWMutex
76+
closed bool
77+
containers codersdk.WorkspaceAgentListContainersResponse // Output from the last list operation.
78+
containersErr error // Error from the last list operation.
79+
devcontainerNames map[string]bool // By devcontainer name.
80+
knownDevcontainers map[string]codersdk.WorkspaceAgentDevcontainer // By workspace folder.
81+
configFileModifiedTimes map[string]time.Time // By config file path.
82+
recreateSuccessTimes map[string]time.Time // By workspace folder.
83+
recreateErrorTimes map[string]time.Time // By workspace folder.
84+
injectedSubAgentProcs map[string]subAgentProcess // By workspace folder.
85+
usingWorkspaceFolderName map[string]bool // By workspace folder.
86+
asyncWg sync.WaitGroup
8587

8688
devcontainerLogSourceIDs map[string]uuid.UUID // By workspace folder.
8789
}
@@ -278,6 +280,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API {
278280
recreateErrorTimes: make(map[string]time.Time),
279281
scriptLogger: func(uuid.UUID) ScriptLogger { return noopScriptLogger{} },
280282
injectedSubAgentProcs: make(map[string]subAgentProcess),
283+
usingWorkspaceFolderName: make(map[string]bool),
281284
}
282285
// The ctx and logger must be set before applying options to avoid
283286
// nil pointer dereference.
@@ -630,7 +633,7 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code
630633
// folder's name. If it is not possible to generate a valid
631634
// agent name based off of the folder name (i.e. no valid characters),
632635
// we will instead fall back to using the container's friendly name.
633-
dc.Name = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName)
636+
dc.Name, api.usingWorkspaceFolderName[dc.WorkspaceFolder] = api.makeAgentName(dc.WorkspaceFolder, dc.Container.FriendlyName)
634637
}
635638
}
636639

@@ -678,8 +681,10 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code
678681
var consecutiveHyphenRegex = regexp.MustCompile("-+")
679682

680683
// `safeAgentName` returns a safe agent name derived from a folder name,
681-
// falling back to the container’s friendly name if needed.
682-
func safeAgentName(name string, friendlyName string) string {
684+
// falling back to the container’s friendly name if needed. The second
685+
// return value will be `true` if it succeeded and `false` if it had
686+
// to fallback to the friendly name.
687+
func safeAgentName(name string, friendlyName string) (string, bool) {
683688
// Keep only ASCII letters and digits, replacing everything
684689
// else with a hyphen.
685690
var sb strings.Builder
@@ -701,10 +706,10 @@ func safeAgentName(name string, friendlyName string) string {
701706
name = name[:min(len(name), maxAgentNameLength)]
702707

703708
if provisioner.AgentNameRegex.Match([]byte(name)) {
704-
return name
709+
return name, true
705710
}
706711

707-
return safeFriendlyName(friendlyName)
712+
return safeFriendlyName(friendlyName), false
708713
}
709714

710715
// safeFriendlyName returns a API safe version of the container's
@@ -719,6 +724,47 @@ func safeFriendlyName(name string) string {
719724
return name
720725
}
721726

727+
// expandedAgentName creates an agent name by including parent directories
728+
// from the workspace folder path to avoid name collisions. Like `safeAgentName`,
729+
// the second returned value will be true if using the workspace folder name,
730+
// and false if it fell back to the friendly name.
731+
func expandedAgentName(workspaceFolder string, friendlyName string, depth int) (string, bool) {
732+
var parts []string
733+
for part := range strings.SplitSeq(filepath.ToSlash(workspaceFolder), "/") {
734+
if part = strings.TrimSpace(part); part != "" {
735+
parts = append(parts, part)
736+
}
737+
}
738+
if len(parts) == 0 {
739+
return safeFriendlyName(friendlyName), false
740+
}
741+
742+
components := parts[max(0, len(parts)-depth-1):]
743+
expanded := strings.Join(components, "-")
744+
745+
return safeAgentName(expanded, friendlyName)
746+
}
747+
748+
// makeAgentName attempts to create an agent name. It will first attempt to create an
749+
// agent name based off of the workspace folder, and will eventually fallback to a
750+
// friendly name. Like `safeAgentName`, the second returned value will be true if the
751+
// agent name utilizes the workspace folder, and false if it falls back to the
752+
// friendly name.
753+
func (api *API) makeAgentName(workspaceFolder string, friendlyName string) (string, bool) {
754+
for attempt := 0; attempt <= maxAttemptsToNameAgent; attempt++ {
755+
agentName, usingWorkspaceFolder := expandedAgentName(workspaceFolder, friendlyName, attempt)
756+
if !usingWorkspaceFolder {
757+
return agentName, false
758+
}
759+
760+
if !api.devcontainerNames[agentName] {
761+
return agentName, true
762+
}
763+
}
764+
765+
return safeFriendlyName(friendlyName), false
766+
}
767+
722768
// RefreshContainers triggers an immediate update of the container list
723769
// and waits for it to complete.
724770
func (api *API) RefreshContainers(ctx context.Context) (err error) {
@@ -1234,6 +1280,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
12341280
if provisioner.AgentNameRegex.Match([]byte(name)) {
12351281
subAgentConfig.Name = name
12361282
configOutdated = true
1283+
delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder)
12371284
} else {
12381285
logger.Warn(ctx, "invalid name in devcontainer customization, ignoring",
12391286
slog.F("name", name),
@@ -1320,12 +1367,55 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
13201367
)
13211368

13221369
// Create new subagent record in the database to receive the auth token.
1370+
// If we get a unique constraint violation, try with expanded names that
1371+
// include parent directories to avoid collisions.
13231372
client := *api.subAgentClient.Load()
1324-
newSubAgent, err := client.Create(ctx, subAgentConfig)
1325-
if err != nil {
1326-
return xerrors.Errorf("create subagent failed: %w", err)
1373+
1374+
originalName := subAgentConfig.Name
1375+
1376+
for attempt := 1; attempt <= maxAttemptsToNameAgent; attempt++ {
1377+
if proc.agent, err = client.Create(ctx, subAgentConfig); err == nil {
1378+
if api.usingWorkspaceFolderName[dc.WorkspaceFolder] {
1379+
api.devcontainerNames[dc.Name] = true
1380+
delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder)
1381+
}
1382+
1383+
break
1384+
}
1385+
1386+
// NOTE(DanielleMaywood):
1387+
// Ordinarily we'd use `errors.As` here, but it didn't appear to work. Not
1388+
// sure if this is because of the communication protocol? Instead I've opted
1389+
// for a slightly more janky string contains approach.
1390+
//
1391+
// We only care if sub agent creation has failed due to a unique constraint
1392+
// violation on the agent name, as we can _possibly_ rectify this.
1393+
if !strings.Contains(err.Error(), "workspace agent name") {
1394+
return xerrors.Errorf("create subagent failed: %w", err)
1395+
}
1396+
1397+
// If there has been a unique constraint violation but the user is *not*
1398+
// using an auto-generated name, then we should error. This is because
1399+
// we do not want to surprise the user with a name they did not ask for.
1400+
if usingFolderName := api.usingWorkspaceFolderName[dc.WorkspaceFolder]; !usingFolderName {
1401+
return xerrors.Errorf("create subagent failed: %w", err)
1402+
}
1403+
1404+
if attempt == maxAttemptsToNameAgent {
1405+
return xerrors.Errorf("create subagent failed after %d attempts: %w", attempt, err)
1406+
}
1407+
1408+
// We increase how much of the workspace folder is used for generating
1409+
// the agent name. With each iteration there is greater chance of this
1410+
// being successful.
1411+
subAgentConfig.Name, api.usingWorkspaceFolderName[dc.WorkspaceFolder] = expandedAgentName(dc.WorkspaceFolder, dc.Container.FriendlyName, attempt)
1412+
1413+
logger.Debug(ctx, "retrying subagent creation with expanded name",
1414+
slog.F("original_name", originalName),
1415+
slog.F("expanded_name", subAgentConfig.Name),
1416+
slog.F("attempt", attempt+1),
1417+
)
13271418
}
1328-
proc.agent = newSubAgent
13291419

13301420
logger.Info(ctx, "created new subagent", slog.F("agent_id", proc.agent.ID))
13311421
} else {

‎agent/agentcontainers/api_internal_test.go

Lines changed: 158 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ func TestSafeAgentName(t *testing.T) {
1515
name string
1616
folderName string
1717
expected string
18+
fallback bool
1819
}{
1920
// Basic valid names
2021
{
@@ -110,18 +111,22 @@ func TestSafeAgentName(t *testing.T) {
110111
{
111112
folderName: "",
112113
expected: "friendly-fallback",
114+
fallback: true,
113115
},
114116
{
115117
folderName: "---",
116118
expected: "friendly-fallback",
119+
fallback: true,
117120
},
118121
{
119122
folderName: "___",
120123
expected: "friendly-fallback",
124+
fallback: true,
121125
},
122126
{
123127
folderName: "@#$",
124128
expected: "friendly-fallback",
129+
fallback: true,
125130
},
126131

127132
// Additional edge cases
@@ -192,10 +197,162 @@ func TestSafeAgentName(t *testing.T) {
192197
for _, tt := range tests {
193198
t.Run(tt.folderName, func(t *testing.T) {
194199
t.Parallel()
195-
name := safeAgentName(tt.folderName, "friendly-fallback")
200+
name, usingWorkspaceFolder := safeAgentName(tt.folderName, "friendly-fallback")
196201

197202
assert.Equal(t, tt.expected, name)
198203
assert.True(t, provisioner.AgentNameRegex.Match([]byte(name)))
204+
assert.Equal(t, tt.fallback, !usingWorkspaceFolder)
205+
})
206+
}
207+
}
208+
209+
func TestExpandedAgentName(t *testing.T) {
210+
t.Parallel()
211+
212+
tests := []struct {
213+
name string
214+
workspaceFolder string
215+
friendlyName string
216+
depth int
217+
expected string
218+
fallback bool
219+
}{
220+
{
221+
name: "simple path depth 1",
222+
workspaceFolder: "/home/coder/project",
223+
friendlyName: "friendly-fallback",
224+
depth: 0,
225+
expected: "project",
226+
},
227+
{
228+
name: "simple path depth 2",
229+
workspaceFolder: "/home/coder/project",
230+
friendlyName: "friendly-fallback",
231+
depth: 1,
232+
expected: "coder-project",
233+
},
234+
{
235+
name: "simple path depth 3",
236+
workspaceFolder: "/home/coder/project",
237+
friendlyName: "friendly-fallback",
238+
depth: 2,
239+
expected: "home-coder-project",
240+
},
241+
{
242+
name: "simple path depth exceeds available",
243+
workspaceFolder: "/home/coder/project",
244+
friendlyName: "friendly-fallback",
245+
depth: 9,
246+
expected: "home-coder-project",
247+
},
248+
// Cases with special characters that need sanitization
249+
{
250+
name: "path with spaces and special chars",
251+
workspaceFolder: "/home/coder/My Project_v2",
252+
friendlyName: "friendly-fallback",
253+
depth: 1,
254+
expected: "coder-my-project-v2",
255+
},
256+
{
257+
name: "path with dots and underscores",
258+
workspaceFolder: "/home/user.name/project_folder.git",
259+
friendlyName: "friendly-fallback",
260+
depth: 1,
261+
expected: "user-name-project-folder-git",
262+
},
263+
// Edge cases
264+
{
265+
name: "empty path",
266+
workspaceFolder: "",
267+
friendlyName: "friendly-fallback",
268+
depth: 0,
269+
expected: "friendly-fallback",
270+
fallback: true,
271+
},
272+
{
273+
name: "root path",
274+
workspaceFolder: "/",
275+
friendlyName: "friendly-fallback",
276+
depth: 0,
277+
expected: "friendly-fallback",
278+
fallback: true,
279+
},
280+
{
281+
name: "single component",
282+
workspaceFolder: "project",
283+
friendlyName: "friendly-fallback",
284+
depth: 0,
285+
expected: "project",
286+
},
287+
{
288+
name: "single component with depth 2",
289+
workspaceFolder: "project",
290+
friendlyName: "friendly-fallback",
291+
depth: 1,
292+
expected: "project",
293+
},
294+
// Collision simulation cases
295+
{
296+
name: "foo/project depth 1",
297+
workspaceFolder: "/home/coder/foo/project",
298+
friendlyName: "friendly-fallback",
299+
depth: 0,
300+
expected: "project",
301+
},
302+
{
303+
name: "foo/project depth 2",
304+
workspaceFolder: "/home/coder/foo/project",
305+
friendlyName: "friendly-fallback",
306+
depth: 1,
307+
expected: "foo-project",
308+
},
309+
{
310+
name: "bar/project depth 1",
311+
workspaceFolder: "/home/coder/bar/project",
312+
friendlyName: "friendly-fallback",
313+
depth: 0,
314+
expected: "project",
315+
},
316+
{
317+
name: "bar/project depth 2",
318+
workspaceFolder: "/home/coder/bar/project",
319+
friendlyName: "friendly-fallback",
320+
depth: 1,
321+
expected: "bar-project",
322+
},
323+
// Path with trailing slashes
324+
{
325+
name: "path with trailing slash",
326+
workspaceFolder: "/home/coder/project/",
327+
friendlyName: "friendly-fallback",
328+
depth: 1,
329+
expected: "coder-project",
330+
},
331+
{
332+
name: "path with multiple trailing slashes",
333+
workspaceFolder: "/home/coder/project///",
334+
friendlyName: "friendly-fallback",
335+
depth: 1,
336+
expected: "coder-project",
337+
},
338+
// Path with leading slashes
339+
{
340+
name: "path with multiple leading slashes",
341+
workspaceFolder: "///home/coder/project",
342+
friendlyName: "friendly-fallback",
343+
depth: 1,
344+
expected: "coder-project",
345+
},
346+
}
347+
348+
for _, tt := range tests {
349+
t.Run(tt.name, func(t *testing.T) {
350+
t.Parallel()
351+
name, usingWorkspaceFolder := expandedAgentName(tt.workspaceFolder, tt.friendlyName, tt.depth)
352+
353+
assert.Equal(t, tt.expected, name)
354+
assert.True(t, provisioner.AgentNameRegex.Match([]byte(name)))
355+
assert.Equal(t, tt.fallback, !usingWorkspaceFolder)
199356
})
200357
}
201358
}

0 commit comments

Comments
 (0)