diff --git a/Makefile b/Makefile index 9886e4072..3cb26410e 100644 --- a/Makefile +++ b/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: diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 1164446b2..01ee28d7b 100644 --- a/internal/auth/auth.go +++ b/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 { diff --git a/internal/command/command_inline_shell_test.go b/internal/command/command_inline_shell_test.go index 499fee997..0998aa5cb 100644 --- a/internal/command/command_inline_shell_test.go +++ b/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) diff --git a/internal/command/command_terminal_test.go b/internal/command/command_terminal_test.go index 8f16d8fd7..80278581e 100644 --- a/internal/command/command_terminal_test.go +++ b/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()) + } } } diff --git a/internal/command/command_unix_test.go b/internal/command/command_unix_test.go index 5e18556b4..4767ff8be 100644 --- a/internal/command/command_unix_test.go +++ b/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") }) } diff --git a/internal/command/command_virtual.go b/internal/command/command_virtual.go index b9ed6bedc..5b726cfd7 100644 --- a/internal/command/command_virtual.go +++ b/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 } diff --git a/internal/runner/command.go b/internal/runner/command.go index c049e34dd..c0ca18d30 100644 --- a/internal/runner/command.go +++ b/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 } diff --git a/internal/runnerv2service/execution.go b/internal/runnerv2service/execution.go index 38123b10e..d3100622b 100644 --- a/internal/runnerv2service/execution.go +++ b/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{} diff --git a/internal/runnerv2service/service_execute_test.go b/internal/runnerv2service/service_execute_test.go index 30bd25c79..20ad89a34 100644 --- a/internal/runnerv2service/service_execute_test.go +++ b/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) diff --git a/internal/sbuffer/safe_buffer.go b/internal/sbuffer/safe_buffer.go deleted file mode 100644 index cc6d5e6f4..000000000 --- a/internal/sbuffer/safe_buffer.go +++ /dev/null @@ -1,43 +0,0 @@ -package sbuffer - -import ( - "bytes" - "sync" -) - -type Buffer struct { - // +checklocks:mu - b *bytes.Buffer - mu *sync.RWMutex -} - -func New(buf []byte) *Buffer { - return &Buffer{ - b: bytes.NewBuffer(buf), - mu: &sync.RWMutex{}, - } -} - -func (b *Buffer) Bytes() []byte { - b.mu.RLock() - defer b.mu.RUnlock() - return b.b.Bytes() -} - -func (b *Buffer) Write(p []byte) (int, error) { - b.mu.Lock() - defer b.mu.Unlock() - return b.b.Write(p) -} - -func (b *Buffer) WriteString(s string) (int, error) { - b.mu.Lock() - defer b.mu.Unlock() - return b.b.WriteString(s) -} - -func (b *Buffer) Read(p []byte) (int, error) { - b.mu.Lock() - defer b.mu.Unlock() - return b.b.Read(p) -} diff --git a/internal/session/env_store_map.go b/internal/session/env_store_map.go index 563f9bf42..855174d37 100644 --- a/internal/session/env_store_map.go +++ b/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 }