fix(cli)!: protect client Logger and refactor cli scaletest tests (#8317)

- (breaking) Protects Logger and LogBodies fields of codersdk.Client with its mutex. This addresses a data race in cli/scaletest.
- Fillets the existing cli/createworkspaces unit test and moves the testing logic there into the tests under scaletest/createworkspaces.
- Adds testutil.RaceEnabled bool const and conditionaly skips previously-skipped tests under scaletest/ if the race detector is enabled. This is unfortunate and sad, but I would prefer to have these tests at least running without the race detector than not running at all.
- Adds IgnoreErrors option to fake in-memory agent loggers; having the agents fail the test immediately when they encounter any sort of error isn't really helpful.

cli/agent.go

@@ -167,7 +167,7 @@ func (r *RootCmd) workspaceAgent() *clibase.Cmd {
 				slog.F("version", version),
 			)
 			client := agentsdk.New(r.agentURL)
-			client.SDK.Logger = logger
+			client.SDK.SetLogger(logger)
 			// Set a reasonable timeout so requests can't hang forever!
 			// The timeout needs to be reasonably long, because requests
 			// with large payloads can take a bit. e.g. startup scripts

cli/root.go

@@ -543,7 +543,7 @@ func (r *RootCmd) InitClient(client *codersdk.Client) clibase.MiddlewareFunc {
 
 			if r.debugHTTP {
 				client.PlainLogger = os.Stderr
-				client.LogBodies = true
+				client.SetLogBodies(true)
 			}
 			client.DisableDirectConnections = r.disableDirect
 

cli/scaletest_test.go

@@ -3,201 +3,53 @@ package cli_test
 import (
 	"bytes"
 	"context"
-	"encoding/json"
-	"os"
 	"path/filepath"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/cli/clitest"
 	"github.com/coder/coder/coderd/coderdtest"
-	"github.com/coder/coder/codersdk"
 	"github.com/coder/coder/pty/ptytest"
-	"github.com/coder/coder/scaletest/harness"
 	"github.com/coder/coder/testutil"
 )
 
 func TestScaleTestCreateWorkspaces(t *testing.T) {
-	t.Skipf("This test is flakey. See https://github.com/coder/coder/issues/4942")
 	t.Parallel()
 
-	// This test does a create-workspaces scale test with --no-cleanup, checks
-	// that the created resources are OK, and then runs a cleanup.
-	t.Run("WorkspaceBuildNoCleanup", func(t *testing.T) {
-		t.Parallel()
+	// This test only validates that the CLI command accepts known arguments.
+	// More thorough testing is done in scaletest/createworkspaces/run_test.go.
+	ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitLong)
+	defer cancelFunc()
 
-		ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitLong)
-		defer cancelFunc()
+	client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
+	_ = coderdtest.CreateFirstUser(t, client)
 
-		client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
-		user := coderdtest.CreateFirstUser(t, client)
+	// Write a parameters file.
+	tDir := t.TempDir()
+	outputFile := filepath.Join(tDir, "output.json")
 
-		version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
-		coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
-		template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
+	inv, root := clitest.New(t, "scaletest", "create-workspaces",
+		"--count", "2",
+		"--template", "doesnotexist",
+		"--no-cleanup",
+		"--no-wait-for-agents",
+		"--concurrency", "2",
+		"--timeout", "30s",
+		"--job-timeout", "15s",
+		"--cleanup-concurrency", "1",
+		"--cleanup-timeout", "30s",
+		"--cleanup-job-timeout", "15s",
+		"--output", "text",
+		"--output", "json:"+outputFile,
+	)
+	clitest.SetupConfig(t, client, root)
+	pty := ptytest.New(t)
+	inv.Stdout = pty.Output()
+	inv.Stderr = pty.Output()
 
-		// Write a parameters file.
-		tDir := t.TempDir()
-		paramsFile := filepath.Join(tDir, "params.yaml")
-		outputFile := filepath.Join(tDir, "output.json")
-
-		f, err := os.Create(paramsFile)
-		require.NoError(t, err)
-		defer f.Close()
-		_, err = f.WriteString(`---
-param1: foo
-param2: true
-param3: 1
-`)
-		require.NoError(t, err)
-		err = f.Close()
-		require.NoError(t, err)
-
-		inv, root := clitest.New(t, "scaletest", "create-workspaces",
-			"--count", "2",
-			"--template", template.Name,
-			"--parameters-file", paramsFile,
-			"--parameter", "param1=bar",
-			"--parameter", "param4=baz",
-			"--no-cleanup",
-			// This flag is important for tests because agents will never be
-			// started.
-			"--no-wait-for-agents",
-			// Run and connect flags cannot be tested because they require an
-			// agent.
-			"--concurrency", "2",
-			"--timeout", "30s",
-			"--job-timeout", "15s",
-			"--cleanup-concurrency", "1",
-			"--cleanup-timeout", "30s",
-			"--cleanup-job-timeout", "15s",
-			"--output", "text",
-			"--output", "json:"+outputFile,
-		)
-		clitest.SetupConfig(t, client, root)
-		pty := ptytest.New(t)
-		inv.Stdout = pty.Output()
-		inv.Stderr = pty.Output()
-
-		done := make(chan any)
-		go func() {
-			err := inv.WithContext(ctx).Run()
-			assert.NoError(t, err)
-			close(done)
-		}()
-		pty.ExpectMatch("Test results:")
-		pty.ExpectMatch("Pass:  2")
-		select {
-		case <-done:
-		case <-ctx.Done():
-		}
-		cancelFunc()
-		<-done
-
-		// Recreate the context.
-		ctx, cancelFunc = context.WithTimeout(context.Background(), testutil.WaitLong)
-		defer cancelFunc()
-
-		// Verify the output file.
-		f, err = os.Open(outputFile)
-		require.NoError(t, err)
-		defer f.Close()
-		var res harness.Results
-		err = json.NewDecoder(f).Decode(&res)
-		require.NoError(t, err)
-
-		require.EqualValues(t, 2, res.TotalRuns)
-		require.EqualValues(t, 2, res.TotalPass)
-
-		// Find the workspaces and users and check that they are what we expect.
-		workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{
-			Offset: 0,
-			Limit:  100,
-		})
-		require.NoError(t, err)
-		require.Len(t, workspaces.Workspaces, 2)
-
-		seenUsers := map[string]struct{}{}
-		for _, w := range workspaces.Workspaces {
-			// Sadly we can't verify params as the API doesn't seem to return
-			// them.
-
-			// Verify that the user is a unique scaletest user.
-			u, err := client.User(ctx, w.OwnerID.String())
-			require.NoError(t, err)
-
-			_, ok := seenUsers[u.ID.String()]
-			require.False(t, ok, "user has more than one workspace")
-			seenUsers[u.ID.String()] = struct{}{}
-
-			require.Contains(t, u.Username, "scaletest-")
-			require.Contains(t, u.Email, "scaletest")
-		}
-
-		require.Len(t, seenUsers, len(workspaces.Workspaces))
-
-		// Check that there are exactly 3 users.
-		users, err := client.Users(ctx, codersdk.UsersRequest{
-			Pagination: codersdk.Pagination{
-				Offset: 0,
-				Limit:  100,
-			},
-		})
-		require.NoError(t, err)
-		require.Len(t, users.Users, len(seenUsers)+1)
-
-		// Cleanup.
-		inv, root = clitest.New(t, "scaletest", "cleanup",
-			"--cleanup-concurrency", "1",
-			"--cleanup-timeout", "30s",
-			"--cleanup-job-timeout", "15s",
-		)
-		clitest.SetupConfig(t, client, root)
-		pty = ptytest.New(t)
-		inv.Stdout = pty.Output()
-		inv.Stderr = pty.Output()
-
-		done = make(chan any)
-		go func() {
-			err := inv.WithContext(ctx).Run()
-			assert.NoError(t, err)
-			close(done)
-		}()
-		pty.ExpectMatch("Test results:")
-		pty.ExpectMatch("Pass:  2")
-		pty.ExpectMatch("Test results:")
-		pty.ExpectMatch("Pass:  2")
-		select {
-		case <-done:
-		case <-ctx.Done():
-		}
-		cancelFunc()
-		<-done
-
-		// Recreate the context (again).
-		ctx, cancelFunc = context.WithTimeout(context.Background(), testutil.WaitLong)
-		defer cancelFunc()
-
-		// Verify that the workspaces are gone.
-		workspaces, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
-			Offset: 0,
-			Limit:  100,
-		})
-		require.NoError(t, err)
-		require.Len(t, workspaces.Workspaces, 0)
-
-		// Verify that the users are gone.
-		users, err = client.Users(ctx, codersdk.UsersRequest{
-			Pagination: codersdk.Pagination{
-				Offset: 0,
-				Limit:  100,
-			},
-		})
-		require.NoError(t, err)
-		require.Len(t, users.Users, 1)
-	})
+	err := inv.WithContext(ctx).Run()
+	require.ErrorContains(t, err, "could not find template \"doesnotexist\" in any organization")
 }
 
 // This test just validates that the CLI command accepts its known arguments.

cli/ssh.go

@@ -119,7 +119,7 @@ func (r *RootCmd) ssh() *clibase.Cmd {
 				}
 
 				// log HTTP requests
-				client.Logger = logger
+				client.SetLogger(logger)
 			}
 
 			workspace, workspaceAgent, err := getWorkspaceAndAgent(ctx, inv, client, codersdk.Me, inv.Args[0])

cli/ssh_test.go

@@ -49,7 +49,7 @@ func setupWorkspaceForAgent(t *testing.T, mutate func([]*proto.Agent) []*proto.A
 		}
 	}
 	client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
-	client.Logger = slogtest.Make(t, nil).Named("client").Leveled(slog.LevelDebug)
+	client.SetLogger(slogtest.Make(t, nil).Named("client").Leveled(slog.LevelDebug))
 	user := coderdtest.CreateFirstUser(t, client)
 	agentToken := uuid.NewString()
 	version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{