Fix data races (#697)

This PR adds `go vet` call, including
[`checklocks`](https://pkg.go.dev/gvisor.dev/gvisor/tools/checklocks),
in `make lint`. It also fixes data races found using `RACE=true make
test/execute`.

Retrying reverted #694.

.github/workflows/CI.md

@@ -16,7 +16,7 @@ Run all tests with coverage reports.
 unset RUNME_SESSION_STRATEGY RUNME_TLS_DIR RUNME_SERVER_ADDR
 export SHELL="/bin/bash"
 export TZ="UTC"
-export TAGS="test_with_docker"
+export TAGS="docker_enabled"
 make test/coverage
 make test/coverage/func
 ```

.gitignore

@@ -42,13 +42,18 @@ dist/
 # Editor
 .idea
 
+# macOS
 .DS_Store
 
 # VSCode debugger
 __debug_bin*
 
+# Cover coverage
 cover.out
 
+# Ignore .env
+.env
+
 # Prevent git from interpreting internal/project/testdata/git-project as a submodule.
 internal/project/testdata/git-project/**/.git
 internal/project/testdata/git-project/**/.gitignore

.vscode/settings.json

@@ -13,7 +13,7 @@
             "--proto_path=/usr/local/include/protoc"
         ]
     },
-    "go.buildTags": "test_with_docker,test_with_txtar",
+    "go.buildTags": "docker_enabled,test_with_txtar",
     "makefile.configureOnOpen": false
     // Uncomment if you want to work on files in ./web.
     // "go.buildTags": "js,wasm",

Makefile

@@ -31,7 +31,7 @@ wasm:
 test/execute: PKGS ?= "./..."
 test/execute: RUN ?= .*
 test/execute: RACE ?= false
-test/execute: TAGS ?= "" # e.g. TAGS="test_with_docker"
+test/execute: TAGS ?= "" # e.g. TAGS="docker_enabled"
 # It depends on the build target because the runme binary
 # is used for tests, for example, "runme env dump".
 test/execute: build
@@ -41,7 +41,7 @@ test/execute: build
 test/coverage: PKGS ?= "./..."
 test/coverage: RUN ?= .*
 test/coverage: GOCOVERDIR ?= "."
-test/coverage: TAGS ?= "" # e.g. TAGS="test_with_docker"
+test/coverage: TAGS ?= "" # e.g. TAGS="docker_enabled"
 # It depends on the build target because the runme binary
 # is used for tests, for example, "runme env dump".
 test/coverage: build
@@ -71,6 +71,7 @@ test-docker/cleanup:
 .PHONY: test-docker/run
 test-docker/run:
 	docker run --rm \
+		-e RUNME_TEST_ENV=docker \
 		-v $(shell pwd):/workspace \
 		-v dev.runme.test-env-gocache:/root/.cache/go-build \
 		runme-test-env:latest
@@ -106,6 +107,8 @@ lint:
 		./...
 	@staticcheck ./...
 	@gosec -quiet -exclude=G110,G204,G304,G404 -exclude-generated ./...
+	@go vet -stdmethods=false ./...
+	@go vet -vettool=$(shell go env GOPATH)/bin/checklocks ./...
 
 .PHONY: pre-commit
 pre-commit: build wasm test lint
@@ -120,6 +123,7 @@ install/dev:
 	go install github.com/icholy/gomajor@v0.13.1
 	go install github.com/stateful/go-proto-gql/protoc-gen-gql@latest
 	go install github.com/atombender/go-jsonschema@v0.16.0
+	go install gvisor.dev/gvisor/tools/checklocks/cmd/checklocks@go
 
 .PHONY: install/goreleaser
 install/goreleaser:

internal/auth/auth.go

@@ -63,6 +63,7 @@ type Auth struct {
 	env Env
 
 	// loginInProgress is a mutex to prevent concurrent `Login` calls.
+	// +checkatomic
 	loginInProgress uint32
 
 	// loginSession contains details about the current login session.
@@ -149,7 +150,6 @@ func (a *Auth) Login(ctx context.Context) error {
 	if !atomic.CompareAndSwapUint32(&a.loginInProgress, 0, 1) {
 		return errors.New("login session already in progress")
 	}
-	defer atomic.StoreUint32(&a.loginInProgress, 0)
 
 	a.log.Debug("start logging in")
 
@@ -159,10 +159,14 @@ func (a *Auth) Login(ctx context.Context) error {
 		a.env = &desktopEnv{Session: a.loginSession}
 	}
 
+	var err error
 	if a.env.IsAutonomous() {
-		return a.loginAuto(ctx)
+		err = a.loginAuto(ctx)
+	} else {
+		err = a.loginManual(ctx)
 	}
-	return a.loginManual(ctx)
+	atomic.StoreUint32(&a.loginInProgress, 0)
+	return err
 }
 
 func (a *Auth) loginAuto(ctx context.Context) error {

internal/cmd/code_server_test.go

@@ -1,4 +1,4 @@
-//go:build test_with_docker
+//go:build docker_enabled
 
 package cmd
 

internal/command/command_docker_test.go

@@ -1,4 +1,4 @@
-//go:build test_with_docker
+//go:build docker_enabled
 
 package command
 

internal/command/command_inline_shell_test.go

@@ -24,8 +24,6 @@ import (
 )
 
 func TestInlineShellCommand_CollectEnv(t *testing.T) {
-	t.Parallel()
-
 	t.Run("Fifo", func(t *testing.T) {
 		envCollectorUseFifo = true
 		testInlineShellCommandCollectEnv(t)

internal/command/command_terminal_test.go

@@ -14,9 +14,12 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
 	"go.uber.org/zap/zaptest"
 
+	"github.com/stateful/runme/v3/internal/sbuffer"
 	"github.com/stateful/runme/v3/internal/session"
+	"github.com/stateful/runme/v3/internal/testutils"
 	runnerv2 "github.com/stateful/runme/v3/pkg/api/gen/proto/go/runme/runner/v2"
 )
 
@@ -26,9 +29,9 @@ func TestTerminalCommand_EnvPropagation(t *testing.T) {
 	session, err := session.New()
 	require.NoError(t, err)
 	stdinR, stdinW := io.Pipe()
-	stdout := bytes.NewBuffer(nil)
+	stdout := sbuffer.New(nil)
 
-	factory := NewFactory(WithLogger(zaptest.NewLogger(t)))
+	factory := NewFactory(WithLogger(zap.NewNop()))
 
 	cmd, err := factory.Build(
 		&ProgramConfig{
@@ -56,8 +59,16 @@ func TestTerminalCommand_EnvPropagation(t *testing.T) {
 
 	_, err = stdinW.Write([]byte("export TEST_ENV=1\n"))
 	require.NoError(t, err)
+	// Give the shell some time to process the command.
+	time.Sleep(time.Second)
+
 	// Wait for the prompt before sending the next command.
-	expectContainLines(ctx, t, stdout, []string{"$"})
+	prompt := "$"
+	if testutils.IsDockerTestEnv() {
+		prompt = "#"
+	}
+	expectContainLines(ctx, t, stdout, []string{prompt})
+
 	_, err = stdinW.Write([]byte("exit\n"))
 	require.NoError(t, err)
 
@@ -66,13 +77,11 @@ func TestTerminalCommand_EnvPropagation(t *testing.T) {
 }
 
 func TestTerminalCommand_Intro(t *testing.T) {
-	t.Parallel()
-
 	session, err := session.New(session.WithSeedEnv(os.Environ()))
 	require.NoError(t, err)
 
 	stdinR, stdinW := io.Pipe()
-	stdout := bytes.NewBuffer(nil)
+	stdout := sbuffer.New(nil)
 
 	factory := NewFactory(WithLogger(zaptest.NewLogger(t)))
 
@@ -103,32 +112,37 @@ func expectContainLines(ctx context.Context, t *testing.T, r io.Reader, expected
 	t.Helper()
 
 	hits := make(map[string]bool, len(expected))
+	buf := new(bytes.Buffer)
 	output := new(strings.Builder)
 
-	for {
-		buf := new(bytes.Buffer)
-		r := io.TeeReader(r, buf)
+	r = io.TeeReader(r, buf)
 
+	for {
 		scanner := bufio.NewScanner(r)
 		for scanner.Scan() {
-			_, _ = output.WriteString(scanner.Text())
-		}
-		require.NoError(t, scanner.Err())
+			line := scanner.Text()
+			if line == "" {
+				continue
+			}
 
-		for _, e := range expected {
-			if strings.Contains(output.String(), e) {
-				hits[e] = true
+			_, _ = output.WriteString(line)
+
+			for _, e := range expected {
+				if strings.Contains(line, e) {
+					hits[e] = true
+				}
 			}
-		}
 
-		if len(hits) == len(expected) {
-			return
+			if len(hits) == len(expected) {
+				return
+			}
 		}
+		require.NoError(t, scanner.Err())
 
 		select {
 		case <-time.After(100 * time.Millisecond):
 		case <-ctx.Done():
-			t.Fatalf("error waiting for line %q, instead read %q: %s", expected, buf.Bytes(), ctx.Err())
+			t.Fatalf("error waiting for line %q, instead read %q: %s", expected, buf.String(), ctx.Err())
 		}
 	}
 }

internal/command/command_unix_test.go

@@ -7,12 +7,15 @@ import (
 	"context"
 	"io"
 	"os"
+	"strings"
 	"testing"
 	"time"
+	"unicode"
 
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zaptest"
 
+	"github.com/stateful/runme/v3/internal/sbuffer"
 	"github.com/stateful/runme/v3/internal/session"
 	runnerv2 "github.com/stateful/runme/v3/pkg/api/gen/proto/go/runme/runner/v2"
 	"github.com/stateful/runme/v3/pkg/document"
@@ -326,7 +329,7 @@ func TestCommand_SetWinsize(t *testing.T) {
 	t.Run("InlineInteractive", func(t *testing.T) {
 		t.Parallel()
 
-		stdout := bytes.NewBuffer(nil)
+		stdout := sbuffer.New(nil)
 
 		cmd, err := factory.Build(
 			&ProgramConfig{
@@ -355,7 +358,7 @@ func TestCommand_SetWinsize(t *testing.T) {
 	t.Run("Terminal", func(t *testing.T) {
 		t.Parallel()
 
-		stdout := bytes.NewBuffer(nil)
+		stdout := sbuffer.New(nil)
 		stdinR, stdinW := io.Pipe()
 
 		// Even if the [ProgramConfig] specifies that the command is non-interactive,
@@ -383,7 +386,7 @@ func TestCommand_SetWinsize(t *testing.T) {
 
 		// TODO(adamb): on macOS is is not necessary, but on Linux
 		// we need to wait for the shell to start before we start sending commands.
-		time.Sleep(time.Second)
+		time.Sleep(time.Second * 3)
 
 		err = SetWinsize(cmd, &Winsize{Rows: 45, Cols: 56, X: 0, Y: 0})
 		require.NoError(t, err)
@@ -392,8 +395,19 @@ func TestCommand_SetWinsize(t *testing.T) {
 		_, err = stdinW.Write([]byte{0x04}) // EOT
 		require.NoError(t, err)
 		err = cmd.Wait(context.Background())
-		require.NoError(t, err, "command failed due to: %s", stdout.String())
-		require.Contains(t, stdout.String(), "56\r\n45\r\n")
+		stdoutStr := stdout.String()
+		require.NoError(
+			t,
+			err,
+			"command failed due to: %s", strings.Map(func(r rune) rune {
+				if unicode.IsGraphic(r) {
+					return r
+				}
+				return -1
+			}, stdoutStr),
+		)
+		require.Contains(t, stdoutStr, "56")
+		require.Contains(t, stdoutStr, "45")
 	})
 }
 

internal/command/command_virtual.go

@@ -29,7 +29,8 @@ type virtualCommand struct {
 
 	wg sync.WaitGroup // watch goroutines copying I/O
 
-	mu  sync.Mutex // protect err
+	mu sync.Mutex // protect err
+	// +checklocks:mu
 	err error
 }
 

internal/dockerexec/docker_test.go

@@ -1,4 +1,4 @@
-//go:build test_with_docker
+//go:build docker_enabled
 
 package dockerexec
 

internal/runner/command.go

@@ -39,7 +39,8 @@ type command struct {
 	Stdout      io.Writer
 	Stderr      io.Writer
 
-	cmd     *exec.Cmd
+	cmd *exec.Cmd
+	// +checkatomic
 	cmdDone uint32
 
 	// pty and tty as pseud-terminal primary and secondary.
@@ -53,8 +54,10 @@ type command struct {
 
 	context context.Context
 	wg      sync.WaitGroup
-	mu      sync.Mutex
-	err     error
+
+	mu sync.Mutex
+	// +checklocks:mu
+	err error
 
 	logger *zap.Logger
 }

internal/runnerv2service/execution.go

@@ -35,7 +35,8 @@ const (
 var opininatedEnvVarNamingRegexp = regexp.MustCompile(`^[A-Z_][A-Z0-9_]{1}[A-Z0-9_]*[A-Z][A-Z0-9_]*$`)
 
 type buffer struct {
-	mu     *sync.Mutex
+	mu *sync.Mutex
+	// +checklocks:mu
 	b      *bytes.Buffer
 	closed *atomic.Bool
 	close  chan struct{}

internal/runnerv2service/service_execute_test.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
 	"go.uber.org/zap/zaptest"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/test/bufconn"
@@ -1092,7 +1093,8 @@ func testStartRunnerServiceServer(t *testing.T) (
 
 	server := grpc.NewServer()
 
-	runnerService, err := NewRunnerService(factory, logger)
+	// Using nop logger to avoid data race.
+	runnerService, err := NewRunnerService(factory, zap.NewNop())
 	require.NoError(t, err)
 	runnerv2.RegisterRunnerServiceServer(server, runnerService)