From 82ed80761f3265ad20dcf68b8b709b9aeb234a80 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 23 Jun 2025 18:43:23 +0000 Subject: [PATCH 01/14] feat(agent/agentcontainers): retry with longer name on failure Closes https://github.com/coder/internal/issues/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. --- agent/agentcontainers/api.go | 89 +++++++++++-- agent/agentcontainers/api_internal_test.go | 147 +++++++++++++++++++++ agent/agentcontainers/api_test.go | 134 +++++++++++++++++++ 3 files changed, 356 insertions(+), 14 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 8896c3217558f..4a1275ef95299 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -19,6 +19,7 @@ import ( "github.com/fsnotify/fsnotify" "github.com/go-chi/chi/v5" "github.com/google/uuid" + "github.com/lib/pq" "golang.org/x/xerrors" "cdr.dev/slog" @@ -69,17 +70,18 @@ type API struct { ownerName string workspaceName string - mu sync.RWMutex - closed bool - containers codersdk.WorkspaceAgentListContainersResponse // Output from the last list operation. - containersErr error // Error from the last list operation. - devcontainerNames map[string]bool // By devcontainer name. - knownDevcontainers map[string]codersdk.WorkspaceAgentDevcontainer // 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. - asyncWg sync.WaitGroup + mu sync.RWMutex + closed bool + containers codersdk.WorkspaceAgentListContainersResponse // Output from the last list operation. + containersErr error // Error from the last list operation. + devcontainerNames map[string]bool // By devcontainer name. + knownDevcontainers map[string]codersdk.WorkspaceAgentDevcontainer // 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]struct{} // By workspace folder. + asyncWg sync.WaitGroup devcontainerLogSourceIDs map[string]uuid.UUID // By workspace folder. } @@ -253,6 +255,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API { recreateErrorTimes: make(map[string]time.Time), scriptLogger: func(uuid.UUID) ScriptLogger { return noopScriptLogger{} }, injectedSubAgentProcs: make(map[string]subAgentProcess), + usingWorkspaceFolderName: make(map[string]struct{}), } // The ctx and logger must be set before applying options to avoid // nil pointer dereference. @@ -591,6 +594,7 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code // agent name based off of the folder name (i.e. no valid characters), // we will instead fall back to using the container's friendly name. dc.Name = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName) + api.usingWorkspaceFolderName[dc.WorkspaceFolder] = struct{}{} } } @@ -679,6 +683,25 @@ func safeFriendlyName(name string) string { return name } +// expandedAgentName creates an agent name by including parent directories +// from the workspace folder path to avoid name collisions. +func expandedAgentName(workspaceFolder string, friendlyName string, depth int) string { + var parts []string + for part := range strings.SplitSeq(filepath.ToSlash(workspaceFolder), "/") { + if part = strings.TrimSpace(part); part != "" { + parts = append(parts, part) + } + } + if len(parts) == 0 { + return safeFriendlyName(friendlyName) + } + + components := parts[max(0, len(parts)-depth-1):] + expanded := strings.Join(components, "-") + + return safeAgentName(expanded, friendlyName) +} + // RefreshContainers triggers an immediate update of the container list // and waits for it to complete. func (api *API) RefreshContainers(ctx context.Context) (err error) { @@ -1194,6 +1217,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c if provisioner.AgentNameRegex.Match([]byte(name)) { subAgentConfig.Name = name configOutdated = true + delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) } else { logger.Warn(ctx, "invalid name in devcontainer customization, ignoring", slog.F("name", name), @@ -1280,10 +1304,47 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c ) // Create new subagent record in the database to receive the auth token. + // If we get a unique constraint violation, try with expanded names that + // include parent directories to avoid collisions. client := *api.subAgentClient.Load() - newSubAgent, err := client.Create(ctx, subAgentConfig) - if err != nil { - return xerrors.Errorf("create subagent failed: %w", err) + + var newSubAgent SubAgent + originalName := subAgentConfig.Name + maxAttempts := 5 // Maximum number of parent directories to include + + for attempt := 1; attempt <= maxAttempts; attempt++ { + if proc.agent, err = client.Create(ctx, subAgentConfig); err == nil { + break + } + + // We only care if sub agent creation has failed due to a unique constraint + // violation on the agent name, as we can _possibly_ rectify this. + var pgErr *pq.Error + if !errors.As(err, &pgErr) || (pgErr.Code != "23505" && pgErr.Column == "name") { + return xerrors.Errorf("create subagent failed: %w", err) + } + + // If there has been a unique constraint violation but the user is *not* + // using an auto-generated name, then we should error. This is because + // we do not want to surprise the user with a name they did not ask for. + if _, usingFolderName := api.usingWorkspaceFolderName[dc.WorkspaceFolder]; !usingFolderName { + return xerrors.Errorf("create subagent failed: %w", err) + } + + if attempt == maxAttempts { + return xerrors.Errorf("create subagent failed after %d attempts: %w", attempt, err) + } + + // We increase how much of the workspace folder is used for generating + // the agent name. With each iteration there is greater chance of this + // being successful. + subAgentConfig.Name = expandedAgentName(dc.WorkspaceFolder, dc.Container.FriendlyName, attempt) + + logger.Debug(ctx, "retrying subagent creation with expanded name", + slog.F("original_name", originalName), + slog.F("expanded_name", subAgentConfig.Name), + slog.F("attempt", attempt+1), + ) } proc.agent = newSubAgent diff --git a/agent/agentcontainers/api_internal_test.go b/agent/agentcontainers/api_internal_test.go index bda6371f63e5e..641234a40185d 100644 --- a/agent/agentcontainers/api_internal_test.go +++ b/agent/agentcontainers/api_internal_test.go @@ -199,3 +199,150 @@ func TestSafeAgentName(t *testing.T) { }) } } + +func TestExpandedAgentName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + workspaceFolder string + friendlyName string + depth int + expected string + }{ + { + name: "simple path depth 1", + workspaceFolder: "/home/coder/project", + friendlyName: "friendly-fallback", + depth: 0, + expected: "project", + }, + { + name: "simple path depth 2", + workspaceFolder: "/home/coder/project", + friendlyName: "friendly-fallback", + depth: 1, + expected: "coder-project", + }, + { + name: "simple path depth 3", + workspaceFolder: "/home/coder/project", + friendlyName: "friendly-fallback", + depth: 2, + expected: "home-coder-project", + }, + { + name: "simple path depth exceeds available", + workspaceFolder: "/home/coder/project", + friendlyName: "friendly-fallback", + depth: 9, + expected: "home-coder-project", + }, + // Cases with special characters that need sanitization + { + name: "path with spaces and special chars", + workspaceFolder: "/home/coder/My Project_v2", + friendlyName: "friendly-fallback", + depth: 1, + expected: "coder-my-project-v2", + }, + { + name: "path with dots and underscores", + workspaceFolder: "/home/user.name/project_folder.git", + friendlyName: "friendly-fallback", + depth: 1, + expected: "user-name-project-folder-git", + }, + // Edge cases + { + name: "empty path", + workspaceFolder: "", + friendlyName: "friendly-fallback", + depth: 0, + expected: "friendly-fallback", + }, + { + name: "root path", + workspaceFolder: "/", + friendlyName: "friendly-fallback", + depth: 0, + expected: "friendly-fallback", + }, + { + name: "single component", + workspaceFolder: "project", + friendlyName: "friendly-fallback", + depth: 0, + expected: "project", + }, + { + name: "single component with depth 2", + workspaceFolder: "project", + friendlyName: "friendly-fallback", + depth: 1, + expected: "project", + }, + // Collision simulation cases + { + name: "foo/project depth 1", + workspaceFolder: "/home/coder/foo/project", + friendlyName: "friendly-fallback", + depth: 0, + expected: "project", + }, + { + name: "foo/project depth 2", + workspaceFolder: "/home/coder/foo/project", + friendlyName: "friendly-fallback", + depth: 1, + expected: "foo-project", + }, + { + name: "bar/project depth 1", + workspaceFolder: "/home/coder/bar/project", + friendlyName: "friendly-fallback", + depth: 0, + expected: "project", + }, + { + name: "bar/project depth 2", + workspaceFolder: "/home/coder/bar/project", + friendlyName: "friendly-fallback", + depth: 1, + expected: "bar-project", + }, + // Path with trailing slashes + { + name: "path with trailing slash", + workspaceFolder: "/home/coder/project/", + friendlyName: "friendly-fallback", + depth: 1, + expected: "coder-project", + }, + { + name: "path with multiple trailing slashes", + workspaceFolder: "/home/coder/project///", + friendlyName: "friendly-fallback", + depth: 1, + expected: "coder-project", + }, + // Path with leading slashes + { + name: "path with multiple leading slashes", + workspaceFolder: "///home/coder/project", + friendlyName: "friendly-fallback", + depth: 1, + expected: "coder-project", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + name := expandedAgentName(tt.workspaceFolder, tt.friendlyName, tt.depth) + + assert.Equal(t, tt.expected, name) + assert.True(t, provisioner.AgentNameRegex.Match([]byte(name))) + }) + } +} diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index a59a3bfd6731e..f26ce763b7c70 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -3,6 +3,7 @@ package agentcontainers_test import ( "context" "encoding/json" + "fmt" "math/rand" "net/http" "net/http/httptest" @@ -16,6 +17,7 @@ import ( "github.com/fsnotify/fsnotify" "github.com/go-chi/chi/v5" "github.com/google/uuid" + "github.com/lib/pq" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -26,6 +28,7 @@ import ( "github.com/coder/coder/v2/agent/agentcontainers" "github.com/coder/coder/v2/agent/agentcontainers/acmock" "github.com/coder/coder/v2/agent/agentcontainers/watcher" + "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" "github.com/coder/quartz" @@ -220,6 +223,9 @@ type fakeSubAgentClient struct { createErrC chan error // If set, send to return error, close to return nil. deleted []uuid.UUID deleteErrC chan error // If set, send to return error, close to return nil. + + // For testing unique constraint violations + nameConflictCount map[string]int // Track how many times each name has been attempted } func (m *fakeSubAgentClient) List(ctx context.Context) ([]agentcontainers.SubAgent, error) { @@ -261,6 +267,22 @@ func (m *fakeSubAgentClient) Create(ctx context.Context, agent agentcontainers.S if agent.OperatingSystem == "" { return agentcontainers.SubAgent{}, xerrors.New("operating system must be set") } + + // Check for simulated unique constraint violations + if m.nameConflictCount == nil { + m.nameConflictCount = make(map[string]int) + } + for _, a := range m.agents { + if a.Name == agent.Name { + m.nameConflictCount[agent.Name]++ + return agentcontainers.SubAgent{}, &pq.Error{ + Code: "23505", + Column: "name", + Message: "duplicate key value violates unique constraint", + } + } + } + agent.ID = uuid.New() agent.AuthToken = uuid.New() if m.agents == nil { @@ -1987,6 +2009,118 @@ func mustFindDevcontainerByPath(t *testing.T, devcontainers []codersdk.Workspace return codersdk.WorkspaceAgentDevcontainer{} // Unreachable, but required for compilation } +// TestSubAgentCreationWithNameRetry tests the retry logic when unique constraint violations occur +func TestSubAgentCreationWithNameRetry(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + workspaceFolders []string + expectedConflicts map[string]int + expectedFinalNames []string + }{ + { + name: "SingleCollision", + workspaceFolders: []string{ + "/home/coder/foo/project", + "/home/coder/bar/project", + }, + expectedConflicts: map[string]int{ + "project": 1, // First "project" fails once, then succeeds + }, + expectedFinalNames: []string{"project", "bar-project"}, + }, + { + name: "MultipleCollisions", + workspaceFolders: []string{ + "/home/coder/foo/project", + "/home/coder/bar/foo/project", + "/home/coder/baz/foo/project", + "/home/coder/wibble/baz/foo/project", + "/home/coder/wobble/wibble/baz/foo/project", + }, + expectedConflicts: map[string]int{ + "project": 4, + "foo-project": 3, + "baz-foo-project": 2, + "wibble-baz-foo-project": 1, + }, + expectedFinalNames: []string{ + "project", + "foo-project", + "baz-foo-project", + "wibble-baz-foo-project", + "wobble-wibble-baz-foo-project", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Create fake clients and API + var containers []codersdk.WorkspaceAgentContainer + for i, workspaceFolder := range tt.workspaceFolders { + containers = append(containers, newFakeContainer( + fmt.Sprintf("container%d", i+1), + fmt.Sprintf("/.devcontainer/devcontainer%d.json", i+1), + workspaceFolder, + )) + } + + ccli := &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{Containers: containers}, + arch: "amd64", + } + + logger := slogtest.Make(t, &slogtest.Options{}) + subAgentClient := &fakeSubAgentClient{logger: logger} + + watcher := newFakeWatcher(t) + + ctx := context.Background() + api := agentcontainers.NewAPI(logger, + agentcontainers.WithExecer(agentexec.DefaultExecer), + agentcontainers.WithContainerCLI(ccli), + agentcontainers.WithDevcontainerCLI(&fakeDevcontainerCLI{}), + agentcontainers.WithSubAgentClient(subAgentClient), + agentcontainers.WithWatcher(watcher), + agentcontainers.WithManifestInfo("owner", "workspace"), + ) + defer api.Close() + + // Trigger container updates to create sub agents + err := api.RefreshContainers(ctx) + require.NoError(t, err) + + // Verify that both agents were created with expected names + require.Len(t, subAgentClient.created, len(tt.workspaceFolders)) + + actualNames := make([]string, len(subAgentClient.created)) + for i, agent := range subAgentClient.created { + actualNames[i] = agent.Name + } + + assert.Equal(t, tt.expectedConflicts, subAgentClient.nameConflictCount) + assert.Equal(t, tt.expectedFinalNames, actualNames) + }) + } +} + +func newFakeContainer(id, configPath, workspaceFolder string) codersdk.WorkspaceAgentContainer { + return codersdk.WorkspaceAgentContainer{ + ID: id, + FriendlyName: "test-friendly", + Image: "test-image:latest", + Labels: map[string]string{ + agentcontainers.DevcontainerLocalFolderLabel: workspaceFolder, + agentcontainers.DevcontainerConfigFileLabel: configPath, + }, + Running: true, + } +} + func fakeContainer(t *testing.T, mut ...func(*codersdk.WorkspaceAgentContainer)) codersdk.WorkspaceAgentContainer { t.Helper() ct := codersdk.WorkspaceAgentContainer{ From 7cf8181246d1110c2be605ba8590227a340a58c8 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 23 Jun 2025 18:50:45 +0000 Subject: [PATCH 02/14] chore: listen to feedback --- agent/agentcontainers/api.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 4a1275ef95299..4f374cad66511 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1308,9 +1308,8 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c // include parent directories to avoid collisions. client := *api.subAgentClient.Load() - var newSubAgent SubAgent originalName := subAgentConfig.Name - maxAttempts := 5 // Maximum number of parent directories to include + maxAttempts := 5 for attempt := 1; attempt <= maxAttempts; attempt++ { if proc.agent, err = client.Create(ctx, subAgentConfig); err == nil { @@ -1346,7 +1345,6 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c slog.F("attempt", attempt+1), ) } - proc.agent = newSubAgent logger.Info(ctx, "created new subagent", slog.F("agent_id", proc.agent.ID)) } else { From 5873a4780f868820fc5a26ed3a343187c77897ce Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 23 Jun 2025 19:07:54 +0000 Subject: [PATCH 03/14] fix: remove `arch: "amd64"` --- agent/agentcontainers/api_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index f26ce763b7c70..eca868828bc41 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -2069,10 +2069,7 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { )) } - ccli := &fakeContainerCLI{ - containers: codersdk.WorkspaceAgentListContainersResponse{Containers: containers}, - arch: "amd64", - } + ccli := &fakeContainerCLI{containers: codersdk.WorkspaceAgentListContainersResponse{Containers: containers}} logger := slogtest.Make(t, &slogtest.Options{}) subAgentClient := &fakeSubAgentClient{logger: logger} From fb5079ca2b0265f1cf4880d1d5603f07701b4da8 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 23 Jun 2025 19:22:12 +0000 Subject: [PATCH 04/14] chore: maybe use runtime.GOARCH? --- agent/agentcontainers/api_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index eca868828bc41..c468593d293c3 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -2069,7 +2069,10 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { )) } - ccli := &fakeContainerCLI{containers: codersdk.WorkspaceAgentListContainersResponse{Containers: containers}} + ccli := &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{Containers: containers}, + arch: runtime.GOARCH, + } logger := slogtest.Make(t, &slogtest.Options{}) subAgentClient := &fakeSubAgentClient{logger: logger} From b6b9cea2582478aa5234348e691070c84358111b Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 23 Jun 2025 19:30:35 +0000 Subject: [PATCH 05/14] chore: skip test on windows --- agent/agentcontainers/api_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index c468593d293c3..14d81fb657103 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -2013,6 +2013,10 @@ func mustFindDevcontainerByPath(t *testing.T, devcontainers []codersdk.Workspace func TestSubAgentCreationWithNameRetry(t *testing.T) { t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("Dev Container tests are not supported on Windows") + } + tests := []struct { name string workspaceFolders []string From 057360a701989f5f4cba8f386ad99ce917374478 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 08:54:49 +0000 Subject: [PATCH 06/14] chore: fix logic and reduce flakiness of test --- agent/agentcontainers/api.go | 9 +++++--- agent/agentcontainers/api_test.go | 35 +++++++++---------------------- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 4f374cad66511..789ce84cbe386 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -19,7 +19,6 @@ import ( "github.com/fsnotify/fsnotify" "github.com/go-chi/chi/v5" "github.com/google/uuid" - "github.com/lib/pq" "golang.org/x/xerrors" "cdr.dev/slog" @@ -1316,10 +1315,14 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c break } + // NOTE(DanielleMaywood): + // Ordinarily we'd use `errors.As` here, but it didn't appear to work. Not + // sure if this is because of the communication protocol? Instead I've opted + // for a slightly more janky string contains approach. + // // We only care if sub agent creation has failed due to a unique constraint // violation on the agent name, as we can _possibly_ rectify this. - var pgErr *pq.Error - if !errors.As(err, &pgErr) || (pgErr.Code != "23505" && pgErr.Column == "name") { + if !strings.Contains(err.Error(), "workspace agent name") { return xerrors.Errorf("create subagent failed: %w", err) } diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 14d81fb657103..66fbc21bf6d4a 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -277,8 +277,7 @@ func (m *fakeSubAgentClient) Create(ctx context.Context, agent agentcontainers.S m.nameConflictCount[agent.Name]++ return agentcontainers.SubAgent{}, &pq.Error{ Code: "23505", - Column: "name", - Message: "duplicate key value violates unique constraint", + Message: fmt.Sprintf("workspace agent name %q already exists in this workspace build", agent.Name), } } } @@ -2018,10 +2017,9 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { } tests := []struct { - name string - workspaceFolders []string - expectedConflicts map[string]int - expectedFinalNames []string + name string + workspaceFolders []string + expectedConflicts map[string]int }{ { name: "SingleCollision", @@ -2030,31 +2028,19 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { "/home/coder/bar/project", }, expectedConflicts: map[string]int{ - "project": 1, // First "project" fails once, then succeeds + "project": 1, }, - expectedFinalNames: []string{"project", "bar-project"}, }, { name: "MultipleCollisions", workspaceFolders: []string{ - "/home/coder/foo/project", - "/home/coder/bar/foo/project", - "/home/coder/baz/foo/project", - "/home/coder/wibble/baz/foo/project", - "/home/coder/wobble/wibble/baz/foo/project", + "/home/coder/foo/x/project", + "/home/coder/bar/x/project", + "/home/coder/baz/x/project", }, expectedConflicts: map[string]int{ - "project": 4, - "foo-project": 3, - "baz-foo-project": 2, - "wibble-baz-foo-project": 1, - }, - expectedFinalNames: []string{ - "project", - "foo-project", - "baz-foo-project", - "wibble-baz-foo-project", - "wobble-wibble-baz-foo-project", + "project": 2, + "x-project": 1, }, }, } @@ -2107,7 +2093,6 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { } assert.Equal(t, tt.expectedConflicts, subAgentClient.nameConflictCount) - assert.Equal(t, tt.expectedFinalNames, actualNames) }) } } From df43868fa97945af6c2097a42e22e59253b9eac2 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 10:53:00 +0000 Subject: [PATCH 07/14] chore: `map[string]struct{}` to `map[string]bool` --- agent/agentcontainers/api.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 789ce84cbe386..0a8488df6d240 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -79,7 +79,7 @@ type API struct { 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]struct{} // By workspace folder. + usingWorkspaceFolderName map[string]bool // By workspace folder. asyncWg sync.WaitGroup devcontainerLogSourceIDs map[string]uuid.UUID // By workspace folder. @@ -254,7 +254,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API { recreateErrorTimes: make(map[string]time.Time), scriptLogger: func(uuid.UUID) ScriptLogger { return noopScriptLogger{} }, injectedSubAgentProcs: make(map[string]subAgentProcess), - usingWorkspaceFolderName: make(map[string]struct{}), + usingWorkspaceFolderName: make(map[string]bool), } // The ctx and logger must be set before applying options to avoid // nil pointer dereference. @@ -593,7 +593,7 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code // agent name based off of the folder name (i.e. no valid characters), // we will instead fall back to using the container's friendly name. dc.Name = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName) - api.usingWorkspaceFolderName[dc.WorkspaceFolder] = struct{}{} + api.usingWorkspaceFolderName[dc.WorkspaceFolder] = true } } @@ -1216,7 +1216,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c if provisioner.AgentNameRegex.Match([]byte(name)) { subAgentConfig.Name = name configOutdated = true - delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) + api.usingWorkspaceFolderName[dc.WorkspaceFolder] = false } else { logger.Warn(ctx, "invalid name in devcontainer customization, ignoring", slog.F("name", name), @@ -1329,7 +1329,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c // If there has been a unique constraint violation but the user is *not* // using an auto-generated name, then we should error. This is because // we do not want to surprise the user with a name they did not ask for. - if _, usingFolderName := api.usingWorkspaceFolderName[dc.WorkspaceFolder]; !usingFolderName { + if usingFolderName, _ := api.usingWorkspaceFolderName[dc.WorkspaceFolder]; !usingFolderName { return xerrors.Errorf("create subagent failed: %w", err) } From c25f6517fb518506e7cead285cfd9fd3f89b7553 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 11:01:46 +0000 Subject: [PATCH 08/14] chore: delete from another place --- agent/agentcontainers/api.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 0a8488df6d240..8f993fa683aeb 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1216,7 +1216,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c if provisioner.AgentNameRegex.Match([]byte(name)) { subAgentConfig.Name = name configOutdated = true - api.usingWorkspaceFolderName[dc.WorkspaceFolder] = false + delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) } else { logger.Warn(ctx, "invalid name in devcontainer customization, ignoring", slog.F("name", name), @@ -1312,6 +1312,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c for attempt := 1; attempt <= maxAttempts; attempt++ { if proc.agent, err = client.Create(ctx, subAgentConfig); err == nil { + delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) break } From 4c79563fb343968df58b165faa53050d4ba16fe7 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 11:02:09 +0000 Subject: [PATCH 09/14] chore: mark as using folder name when not falling back to friendly name --- agent/agentcontainers/api.go | 16 +++++++++------- agent/agentcontainers/api_internal_test.go | 8 +++++++- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 8f993fa683aeb..b22ff48af597d 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -592,8 +592,7 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code // folder's name. If it is not possible to generate a valid // agent name based off of the folder name (i.e. no valid characters), // we will instead fall back to using the container's friendly name. - dc.Name = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName) - api.usingWorkspaceFolderName[dc.WorkspaceFolder] = true + dc.Name, api.usingWorkspaceFolderName[dc.WorkspaceFolder] = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName) } } @@ -641,8 +640,10 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code var consecutiveHyphenRegex = regexp.MustCompile("-+") // `safeAgentName` returns a safe agent name derived from a folder name, -// falling back to the container’s friendly name if needed. -func safeAgentName(name string, friendlyName string) string { +// falling back to the container’s friendly name if needed. The second +// return value will be `true` if it succeeded and `false` if it had +// to fallback to the friendly name. +func safeAgentName(name string, friendlyName string) (string, bool) { // Keep only ASCII letters and digits, replacing everything // else with a hyphen. var sb strings.Builder @@ -664,10 +665,10 @@ func safeAgentName(name string, friendlyName string) string { name = name[:min(len(name), maxAgentNameLength)] if provisioner.AgentNameRegex.Match([]byte(name)) { - return name + return name, true } - return safeFriendlyName(friendlyName) + return safeFriendlyName(friendlyName), false } // safeFriendlyName returns a API safe version of the container's @@ -697,8 +698,9 @@ func expandedAgentName(workspaceFolder string, friendlyName string, depth int) s components := parts[max(0, len(parts)-depth-1):] expanded := strings.Join(components, "-") + agentName, _ := safeAgentName(expanded, friendlyName) - return safeAgentName(expanded, friendlyName) + return agentName } // RefreshContainers triggers an immediate update of the container list diff --git a/agent/agentcontainers/api_internal_test.go b/agent/agentcontainers/api_internal_test.go index 641234a40185d..11e6a2aa5e827 100644 --- a/agent/agentcontainers/api_internal_test.go +++ b/agent/agentcontainers/api_internal_test.go @@ -15,6 +15,7 @@ func TestSafeAgentName(t *testing.T) { name string folderName string expected string + fallback bool }{ // Basic valid names { @@ -110,18 +111,22 @@ func TestSafeAgentName(t *testing.T) { { folderName: "", expected: "friendly-fallback", + fallback: true, }, { folderName: "---", expected: "friendly-fallback", + fallback: true, }, { folderName: "___", expected: "friendly-fallback", + fallback: true, }, { folderName: "@#$", expected: "friendly-fallback", + fallback: true, }, // Additional edge cases @@ -192,10 +197,11 @@ func TestSafeAgentName(t *testing.T) { for _, tt := range tests { t.Run(tt.folderName, func(t *testing.T) { t.Parallel() - name := safeAgentName(tt.folderName, "friendly-fallback") + name, usingWorkspaceFolder := safeAgentName(tt.folderName, "friendly-fallback") assert.Equal(t, tt.expected, name) assert.True(t, provisioner.AgentNameRegex.Match([]byte(name))) + assert.Equal(t, tt.fallback, !usingWorkspaceFolder) }) } } From 15dafc71eb4234912d81236b50eab52fdee4772e Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 11:06:55 +0000 Subject: [PATCH 10/14] chore: fallback if we're using a name already defined in terraform --- agent/agentcontainers/api.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index b22ff48af597d..f66ad2ba2e3ba 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -593,6 +593,15 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code // agent name based off of the folder name (i.e. no valid characters), // we will instead fall back to using the container's friendly name. dc.Name, api.usingWorkspaceFolderName[dc.WorkspaceFolder] = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName) + + // If we have accidentally picked a name that is already + // defined by terraform, we'll need to force a fallback + // to the friendly name. The odds of a collision here are + // incredibly slim so this should be fine. + if api.devcontainerNames[dc.Name] { + dc.Name = safeFriendlyName(dc.Container.FriendlyName) + delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) + } } } From 08969cea240f5b61b611927b3b579c16a765a125 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 11:23:28 +0000 Subject: [PATCH 11/14] chore: use map of known names to help create an agent name --- agent/agentcontainers/api.go | 58 +++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index f66ad2ba2e3ba..a3bacd22d85f6 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -41,7 +41,8 @@ const ( // read-write, which seems sensible for devcontainers. coderPathInsideContainer = "/.coder-agent/coder" - maxAgentNameLength = 64 + maxAgentNameLength = 64 + maxAttemptsToNameAgent = 5 ) // API is responsible for container-related operations in the agent. @@ -592,16 +593,7 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code // folder's name. If it is not possible to generate a valid // agent name based off of the folder name (i.e. no valid characters), // we will instead fall back to using the container's friendly name. - dc.Name, api.usingWorkspaceFolderName[dc.WorkspaceFolder] = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName) - - // If we have accidentally picked a name that is already - // defined by terraform, we'll need to force a fallback - // to the friendly name. The odds of a collision here are - // incredibly slim so this should be fine. - if api.devcontainerNames[dc.Name] { - dc.Name = safeFriendlyName(dc.Container.FriendlyName) - delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) - } + dc.Name, api.usingWorkspaceFolderName[dc.WorkspaceFolder] = api.makeAgentName(dc.WorkspaceFolder, dc.Container.FriendlyName) } } @@ -693,8 +685,10 @@ func safeFriendlyName(name string) string { } // expandedAgentName creates an agent name by including parent directories -// from the workspace folder path to avoid name collisions. -func expandedAgentName(workspaceFolder string, friendlyName string, depth int) string { +// from the workspace folder path to avoid name collisions. Like `safeAgentName`, +// the second returned value will be true if using the workspace folder name, +// and false if it fell back to the friendly name. +func expandedAgentName(workspaceFolder string, friendlyName string, depth int) (string, bool) { var parts []string for part := range strings.SplitSeq(filepath.ToSlash(workspaceFolder), "/") { if part = strings.TrimSpace(part); part != "" { @@ -702,14 +696,33 @@ func expandedAgentName(workspaceFolder string, friendlyName string, depth int) s } } if len(parts) == 0 { - return safeFriendlyName(friendlyName) + return safeFriendlyName(friendlyName), false } components := parts[max(0, len(parts)-depth-1):] expanded := strings.Join(components, "-") - agentName, _ := safeAgentName(expanded, friendlyName) - return agentName + return safeAgentName(expanded, friendlyName) +} + +// makeAgentName attempts to create an agent name. It will first attempt to create an +// agent name based off of the workspace folder, and will eventually fallback to a +// friendly name. Like `safeAgentName`, the second returned value will be true if the +// agent name utilises the workspace folder, and false if it falls back to the +// friendly name. +func (api *API) makeAgentName(workspaceFolder string, friendlyName string) (string, bool) { + for attempt := 0; attempt <= maxAttemptsToNameAgent; attempt++ { + agentName, usingWorkspaceFolder := expandedAgentName(workspaceFolder, friendlyName, attempt) + if !usingWorkspaceFolder { + return agentName, false + } + + if !api.devcontainerNames[agentName] { + return agentName, true + } + } + + return safeFriendlyName(friendlyName), false } // RefreshContainers triggers an immediate update of the container list @@ -1319,11 +1332,14 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c client := *api.subAgentClient.Load() originalName := subAgentConfig.Name - maxAttempts := 5 - for attempt := 1; attempt <= maxAttempts; attempt++ { + for attempt := 1; attempt <= maxAttemptsToNameAgent; attempt++ { if proc.agent, err = client.Create(ctx, subAgentConfig); err == nil { - delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) + if api.usingWorkspaceFolderName[dc.WorkspaceFolder] { + api.devcontainerNames[dc.Name] = true + delete(api.usingWorkspaceFolderName, dc.WorkspaceFolder) + } + break } @@ -1345,14 +1361,14 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c return xerrors.Errorf("create subagent failed: %w", err) } - if attempt == maxAttempts { + if attempt == maxAttemptsToNameAgent { return xerrors.Errorf("create subagent failed after %d attempts: %w", attempt, err) } // We increase how much of the workspace folder is used for generating // the agent name. With each iteration there is greater chance of this // being successful. - subAgentConfig.Name = expandedAgentName(dc.WorkspaceFolder, dc.Container.FriendlyName, attempt) + subAgentConfig.Name, api.usingWorkspaceFolderName[dc.WorkspaceFolder] = expandedAgentName(dc.WorkspaceFolder, dc.Container.FriendlyName, attempt) logger.Debug(ctx, "retrying subagent creation with expanded name", slog.F("original_name", originalName), From 14bbb1ddb8bfe10edfee582a966fab6c1e7f58b4 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 11:37:41 +0000 Subject: [PATCH 12/14] chore: testing --- agent/agentcontainers/api_internal_test.go | 6 +- agent/agentcontainers/api_test.go | 77 ++++++++++++---------- 2 files changed, 47 insertions(+), 36 deletions(-) diff --git a/agent/agentcontainers/api_internal_test.go b/agent/agentcontainers/api_internal_test.go index 11e6a2aa5e827..2e049640d74b8 100644 --- a/agent/agentcontainers/api_internal_test.go +++ b/agent/agentcontainers/api_internal_test.go @@ -215,6 +215,7 @@ func TestExpandedAgentName(t *testing.T) { friendlyName string depth int expected string + fallback bool }{ { name: "simple path depth 1", @@ -266,6 +267,7 @@ func TestExpandedAgentName(t *testing.T) { friendlyName: "friendly-fallback", depth: 0, expected: "friendly-fallback", + fallback: true, }, { name: "root path", @@ -273,6 +275,7 @@ func TestExpandedAgentName(t *testing.T) { friendlyName: "friendly-fallback", depth: 0, expected: "friendly-fallback", + fallback: true, }, { name: "single component", @@ -345,10 +348,11 @@ func TestExpandedAgentName(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - name := expandedAgentName(tt.workspaceFolder, tt.friendlyName, tt.depth) + name, usingWorkspaceFolder := expandedAgentName(tt.workspaceFolder, tt.friendlyName, tt.depth) assert.Equal(t, tt.expected, name) assert.True(t, provisioner.AgentNameRegex.Match([]byte(name))) + assert.Equal(t, tt.fallback, !usingWorkspaceFolder) }) } } diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 66fbc21bf6d4a..ac6856825459b 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "runtime" + "slices" "strings" "sync" "testing" @@ -223,9 +224,6 @@ type fakeSubAgentClient struct { createErrC chan error // If set, send to return error, close to return nil. deleted []uuid.UUID deleteErrC chan error // If set, send to return error, close to return nil. - - // For testing unique constraint violations - nameConflictCount map[string]int // Track how many times each name has been attempted } func (m *fakeSubAgentClient) List(ctx context.Context) ([]agentcontainers.SubAgent, error) { @@ -268,13 +266,8 @@ func (m *fakeSubAgentClient) Create(ctx context.Context, agent agentcontainers.S return agentcontainers.SubAgent{}, xerrors.New("operating system must be set") } - // Check for simulated unique constraint violations - if m.nameConflictCount == nil { - m.nameConflictCount = make(map[string]int) - } for _, a := range m.agents { if a.Name == agent.Name { - m.nameConflictCount[agent.Name]++ return agentcontainers.SubAgent{}, &pq.Error{ Code: "23505", Message: fmt.Sprintf("workspace agent name %q already exists in this workspace build", agent.Name), @@ -2017,9 +2010,10 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { } tests := []struct { - name string - workspaceFolders []string - expectedConflicts map[string]int + name string + workspaceFolders []string + expectedNames []string + takenNames []string }{ { name: "SingleCollision", @@ -2027,8 +2021,9 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { "/home/coder/foo/project", "/home/coder/bar/project", }, - expectedConflicts: map[string]int{ - "project": 1, + expectedNames: []string{ + "project", + "bar-project", }, }, { @@ -2038,9 +2033,20 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { "/home/coder/bar/x/project", "/home/coder/baz/x/project", }, - expectedConflicts: map[string]int{ - "project": 2, - "x-project": 1, + expectedNames: []string{ + "project", + "x-project", + "baz-x-project", + }, + }, + { + name: "NameAlreadyTaken", + takenNames: []string{"project", "x-project"}, + workspaceFolders: []string{ + "/home/coder/foo/x/project", + }, + expectedNames: []string{ + "foo-x-project", }, }, } @@ -2049,23 +2055,14 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - // Create fake clients and API - var containers []codersdk.WorkspaceAgentContainer - for i, workspaceFolder := range tt.workspaceFolders { - containers = append(containers, newFakeContainer( - fmt.Sprintf("container%d", i+1), - fmt.Sprintf("/.devcontainer/devcontainer%d.json", i+1), - workspaceFolder, - )) - } - - ccli := &fakeContainerCLI{ - containers: codersdk.WorkspaceAgentListContainersResponse{Containers: containers}, - arch: runtime.GOARCH, - } + ccli := &fakeContainerCLI{arch: runtime.GOARCH} logger := slogtest.Make(t, &slogtest.Options{}) - subAgentClient := &fakeSubAgentClient{logger: logger} + subAgentClient := &fakeSubAgentClient{logger: logger, agents: make(map[uuid.UUID]agentcontainers.SubAgent)} + + for _, name := range tt.takenNames { + subAgentClient.agents[uuid.New()] = agentcontainers.SubAgent{Name: name} + } watcher := newFakeWatcher(t) @@ -2080,9 +2077,16 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { ) defer api.Close() - // Trigger container updates to create sub agents - err := api.RefreshContainers(ctx) - require.NoError(t, err) + for i, workspaceFolder := range tt.workspaceFolders { + ccli.containers.Containers = append(ccli.containers.Containers, newFakeContainer( + fmt.Sprintf("container%d", i+1), + fmt.Sprintf("/.devcontainer/devcontainer%d.json", i+1), + workspaceFolder, + )) + + err := api.RefreshContainers(ctx) + require.NoError(t, err) + } // Verify that both agents were created with expected names require.Len(t, subAgentClient.created, len(tt.workspaceFolders)) @@ -2092,7 +2096,10 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { actualNames[i] = agent.Name } - assert.Equal(t, tt.expectedConflicts, subAgentClient.nameConflictCount) + slices.Sort(tt.expectedNames) + slices.Sort(actualNames) + + assert.Equal(t, tt.expectedNames, actualNames) }) } } From 257c62dc329bbcfdb612eaf542171f3deef2f959 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 11:54:45 +0000 Subject: [PATCH 13/14] chore: appease linter --- agent/agentcontainers/api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index b7e406c213ebf..ef9f58ad88fa8 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -740,7 +740,7 @@ func expandedAgentName(workspaceFolder string, friendlyName string, depth int) ( // makeAgentName attempts to create an agent name. It will first attempt to create an // agent name based off of the workspace folder, and will eventually fallback to a // friendly name. Like `safeAgentName`, the second returned value will be true if the -// agent name utilises the workspace folder, and false if it falls back to the +// agent name utilizes the workspace folder, and false if it falls back to the // friendly name. func (api *API) makeAgentName(workspaceFolder string, friendlyName string) (string, bool) { for attempt := 0; attempt <= maxAttemptsToNameAgent; attempt++ { @@ -1389,7 +1389,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c // If there has been a unique constraint violation but the user is *not* // using an auto-generated name, then we should error. This is because // we do not want to surprise the user with a name they did not ask for. - if usingFolderName, _ := api.usingWorkspaceFolderName[dc.WorkspaceFolder]; !usingFolderName { + if usingFolderName := api.usingWorkspaceFolderName[dc.WorkspaceFolder]; !usingFolderName { return xerrors.Errorf("create subagent failed: %w", err) } From 627fe9f2175e1d81cfddaca80ae52ea8ac27e88b Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 24 Jun 2025 12:40:24 +0000 Subject: [PATCH 14/14] fix: maybe fix race condition? --- agent/agentcontainers/api_test.go | 34 +++++++++++++++++-------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index f8a2ba7cfdcc9..f79ae7a493993 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -30,7 +30,6 @@ import ( "github.com/coder/coder/v2/agent/agentcontainers" "github.com/coder/coder/v2/agent/agentcontainers/acmock" "github.com/coder/coder/v2/agent/agentcontainers/watcher" - "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/agent/usershell" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/pty" @@ -2141,28 +2140,33 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - ccli := &fakeContainerCLI{arch: runtime.GOARCH} - - logger := slogtest.Make(t, &slogtest.Options{}) - subAgentClient := &fakeSubAgentClient{logger: logger, agents: make(map[uuid.UUID]agentcontainers.SubAgent)} + var ( + ctx = testutil.Context(t, testutil.WaitMedium) + logger = testutil.Logger(t) + mClock = quartz.NewMock(t) + fSAC = &fakeSubAgentClient{logger: logger, agents: make(map[uuid.UUID]agentcontainers.SubAgent)} + ccli = &fakeContainerCLI{arch: runtime.GOARCH} + ) for _, name := range tt.takenNames { - subAgentClient.agents[uuid.New()] = agentcontainers.SubAgent{Name: name} + fSAC.agents[uuid.New()] = agentcontainers.SubAgent{Name: name} } - watcher := newFakeWatcher(t) + mClock.Set(time.Now()).MustWait(ctx) + tickerTrap := mClock.Trap().TickerFunc("updaterLoop") - ctx := context.Background() api := agentcontainers.NewAPI(logger, - agentcontainers.WithExecer(agentexec.DefaultExecer), + agentcontainers.WithClock(mClock), agentcontainers.WithContainerCLI(ccli), agentcontainers.WithDevcontainerCLI(&fakeDevcontainerCLI{}), - agentcontainers.WithSubAgentClient(subAgentClient), - agentcontainers.WithWatcher(watcher), - agentcontainers.WithManifestInfo("owner", "workspace"), + agentcontainers.WithSubAgentClient(fSAC), + agentcontainers.WithWatcher(watcher.NewNoop()), ) defer api.Close() + tickerTrap.MustWait(ctx).MustRelease(ctx) + tickerTrap.Close() + for i, workspaceFolder := range tt.workspaceFolders { ccli.containers.Containers = append(ccli.containers.Containers, newFakeContainer( fmt.Sprintf("container%d", i+1), @@ -2175,10 +2179,10 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) { } // Verify that both agents were created with expected names - require.Len(t, subAgentClient.created, len(tt.workspaceFolders)) + require.Len(t, fSAC.created, len(tt.workspaceFolders)) - actualNames := make([]string, len(subAgentClient.created)) - for i, agent := range subAgentClient.created { + actualNames := make([]string, len(fSAC.created)) + for i, agent := range fSAC.created { actualNames[i] = agent.Name }