Revert "Fix data races (#694)"

This reverts commit 002fb9f.

Makefile

@@ -106,8 +106,6 @@ 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
@@ -122,7 +120,6 @@ 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,7 +63,6 @@ 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.
@@ -150,6 +149,7 @@ 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,14 +159,10 @@ func (a *Auth) Login(ctx context.Context) error {
 		a.env = &desktopEnv{Session: a.loginSession}
 	}
 
-	var err error
 	if a.env.IsAutonomous() {
-		err = a.loginAuto(ctx)
-	} else {
-		err = a.loginManual(ctx)
+		return a.loginAuto(ctx)
 	}
-	atomic.StoreUint32(&a.loginInProgress, 0)
-	return err
+	return a.loginManual(ctx)
 }
 
 func (a *Auth) loginAuto(ctx context.Context) error {

internal/command/command_inline_shell_test.go

@@ -24,6 +24,8 @@ 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

@@ -4,6 +4,7 @@ package command
 
 import (
 	"bufio"
+	"bytes"
 	"context"
 	"io"
 	"os"
@@ -15,7 +16,6 @@ import (
 	"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"
 )
@@ -26,7 +26,7 @@ func TestTerminalCommand_EnvPropagation(t *testing.T) {
 	session, err := session.New()
 	require.NoError(t, err)
 	stdinR, stdinW := io.Pipe()
-	stdout := sbuffer.New(nil)
+	stdout := bytes.NewBuffer(nil)
 
 	factory := NewFactory(WithLogger(zaptest.NewLogger(t)))
 
@@ -52,12 +52,12 @@ func TestTerminalCommand_EnvPropagation(t *testing.T) {
 
 	// Terminal command sets up a trap on EXIT.
 	// Wait for it before starting to send commands.
-	expectContainLines(t, stdout, []string{"trap -- \"__cleanup\" EXIT"})
+	expectContainLines(ctx, t, stdout, []string{"trap -- \"__cleanup\" EXIT"})
 
 	_, err = stdinW.Write([]byte("export TEST_ENV=1\n"))
 	require.NoError(t, err)
 	// Wait for the prompt before sending the next command.
-	expectContainLines(t, stdout, []string{"$"})
+	expectContainLines(ctx, t, stdout, []string{"$"})
 	_, err = stdinW.Write([]byte("exit\n"))
 	require.NoError(t, err)
 
@@ -66,11 +66,13 @@ 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 := sbuffer.New(nil)
+	stdout := bytes.NewBuffer(nil)
 
 	factory := NewFactory(WithLogger(zaptest.NewLogger(t)))
 
@@ -94,15 +96,19 @@ func TestTerminalCommand_Intro(t *testing.T) {
 
 	require.NoError(t, cmd.Start(ctx))
 
-	expectContainLines(t, stdout, []string{envSourceCmd, introSecondLine})
+	expectContainLines(ctx, t, stdout, []string{envSourceCmd, introSecondLine})
 }
 
-func expectContainLines(t *testing.T, r io.Reader, expected []string) {
+func expectContainLines(ctx context.Context, t *testing.T, r io.Reader, expected []string) {
 	t.Helper()
-	var output strings.Builder
+
 	hits := make(map[string]bool, len(expected))
+	output := new(strings.Builder)
 
 	for {
+		buf := new(bytes.Buffer)
+		r := io.TeeReader(r, buf)
+
 		scanner := bufio.NewScanner(r)
 		for scanner.Scan() {
 			_, _ = output.WriteString(scanner.Text())
@@ -119,7 +125,11 @@ func expectContainLines(t *testing.T, r io.Reader, expected []string) {
 			return
 		}
 
-		time.Sleep(time.Millisecond * 400)
+		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())
+		}
 	}
 }
 

internal/command/command_unix_test.go

@@ -383,7 +383,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 * 3)
+		time.Sleep(time.Second)
 
 		err = SetWinsize(cmd, &Winsize{Rows: 45, Cols: 56, X: 0, Y: 0})
 		require.NoError(t, err)
@@ -393,8 +393,7 @@ func TestCommand_SetWinsize(t *testing.T) {
 		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\n")
-		require.Contains(t, stdout.String(), "45\r\n")
+		require.Contains(t, stdout.String(), "56\r\n45\r\n")
 	})
 }
 

internal/command/command_virtual.go

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

internal/runner/command.go

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

internal/runnerv2service/execution.go

@@ -35,8 +35,7 @@ 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
-	// +checklocks:mu
+	mu     *sync.Mutex
 	b      *bytes.Buffer
 	closed *atomic.Bool
 	close  chan struct{}

internal/runnerv2service/service_execute_test.go

@@ -16,7 +16,6 @@ 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"
@@ -1093,8 +1092,7 @@ func testStartRunnerServiceServer(t *testing.T) (
 
 	server := grpc.NewServer()
 
-	// Using nop logger to avoid data race.
-	runnerService, err := NewRunnerService(factory, zap.NewNop())
+	runnerService, err := NewRunnerService(factory, logger)
 	require.NoError(t, err)
 	runnerv2.RegisterRunnerServiceServer(server, runnerService)
 

internal/session/env_store_map.go

@@ -7,8 +7,7 @@ import (
 )
 
 type EnvStoreMap struct {
-	mu sync.RWMutex
-	// +checklocks:mu
+	mu    sync.RWMutex
 	items map[string]string
 }