fix: data race, token file misplacement, and silent auth errors

internal/auth/auth.go

@@ -45,7 +45,10 @@ type Authenticator struct {
 }
 
 func NewAuthenticator() (*Authenticator, error) {
-	cfgDir, _ := os.UserConfigDir()
+	cfgDir, err := os.UserConfigDir()
+	if err != nil {
+		return nil, fmt.Errorf("cannot determine user config directory: %w", err)
+	}
 	authPath := filepath.Join(cfgDir, "tusk", "auth.json")
 
 	cfg, err := config.Get()
@@ -279,7 +282,10 @@ func (a *Authenticator) RequestDeviceCode(ctx context.Context) (DeviceCodeRespon
 	}
 	defer func() { _ = resp.Body.Close() }()
 
-	body, _ := io.ReadAll(resp.Body)
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return dcr, fmt.Errorf("error reading device code response body: %w", err)
+	}
 	if resp.StatusCode != http.StatusOK {
 		return dcr, fmt.Errorf(
 			"error returned by device code endpoint %d: %s",
@@ -318,8 +324,11 @@ func (a *Authenticator) PollForToken(ctx context.Context, dcr DeviceCodeResponse
 		if err != nil {
 			return fmt.Errorf("error making request for token: %w", err)
 		}
-		body, _ := io.ReadAll(resp.Body)
+		body, err := io.ReadAll(resp.Body)
 		_ = resp.Body.Close()
+		if err != nil {
+			return fmt.Errorf("error reading token response body: %w", err)
+		}
 
 		var tr tokenResp
 		if err := json.Unmarshal(body, &tr); err != nil {
@@ -372,7 +381,10 @@ func (a *Authenticator) FetchUserEmail(ctx context.Context) error {
 		return err
 	}
 	defer func() { _ = resp.Body.Close() }()
-	b, _ := io.ReadAll(resp.Body)
+	b, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return fmt.Errorf("error reading userinfo response body: %w", err)
+	}
 
 	if resp.StatusCode != http.StatusOK {
 		return fmt.Errorf("userinfo http %d: %s", resp.StatusCode, string(b))
@@ -408,8 +420,11 @@ func (a *Authenticator) refreshAccessToken(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("error requesting refresh token endpoint %w", err)
 	}
-	body, _ := io.ReadAll(resp.Body)
+	body, err := io.ReadAll(resp.Body)
 	_ = resp.Body.Close()
+	if err != nil {
+		return fmt.Errorf("error reading refresh token response body: %w", err)
+	}
 
 	if resp.StatusCode != http.StatusOK {
 		return fmt.Errorf("refresh http %d: %s", resp.StatusCode, string(body))

internal/auth/auth_test.go

@@ -0,0 +1,218 @@
+package auth
+
+// Regression tests for the three bugs fixed in commit 3d1d0c8:
+//
+//  1. Token file misplacement: os.UserConfigDir() error was silently ignored,
+//     so a failure produced an empty cfgDir and the token was written to a
+//     relative path ("tusk/auth.json") in the current working directory instead
+//     of the proper user-config location.
+//
+//  2. Silent io.ReadAll errors: all four Auth0 HTTP methods (RequestDeviceCode,
+//     PollForToken, FetchUserEmail, refreshAccessToken) discarded io.ReadAll
+//     errors with `body, _ := io.ReadAll(...)`, hiding network failures.
+
+import (
+	"context"
+	"errors"
+	"io"
+	"net/http"
+	"runtime"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// ── helpers ──────────────────────────────────────────────────────────────────
+
+// errorReader returns a fixed error on the first Read call, mimicking a
+// mid-stream network failure that causes io.ReadAll to return an error.
+type errorReader struct{}
+
+func (e *errorReader) Read(_ []byte) (int, error) {
+	return 0, errors.New("simulated network read error")
+}
+
+// errorBodyTransport is an http.RoundTripper that returns a well-formed
+// HTTP 200 response whose body always errors on Read.  This is the minimal
+// setup needed to trigger the io.ReadAll error paths that were previously
+// swallowed with `body, _ := io.ReadAll(resp.Body)`.
+type errorBodyTransport struct{}
+
+func (t *errorBodyTransport) RoundTrip(_ *http.Request) (*http.Response, error) {
+	return &http.Response{
+		StatusCode: http.StatusOK,
+		Header:     make(http.Header),
+		Body:       io.NopCloser(&errorReader{}),
+	}, nil
+}
+
+// newTestAuthenticator builds a minimal Authenticator for unit tests.
+// It bypasses NewAuthenticator (which calls os.UserConfigDir and config.Get)
+// so tests remain hermetic and fast.
+func newTestAuthenticator(transport http.RoundTripper) *Authenticator {
+	return &Authenticator{
+		authFilePath: "/tmp/tusk-test-auth.json",
+		httpClient:   &http.Client{Timeout: 5 * time.Second, Transport: transport},
+		domain:       "test.example.auth0.com",
+		clientID:     "test-client-id",
+		scope:        "openid email offline_access",
+		audience:     "drift-cli",
+	}
+}
+
+// ── Bug 1: os.UserConfigDir() failure ────────────────────────────────────────
+
+// TestNewAuthenticator_UserConfigDirFailure is a minimal repro for the token
+// file misplacement bug.
+//
+// Before the fix, NewAuthenticator used:
+//
+//	cfgDir, _ := os.UserConfigDir()           // error silently dropped
+//	authPath := filepath.Join(cfgDir, ...)    // cfgDir == "" → relative path
+//
+// With cfgDir empty, filepath.Join("", "tusk", "auth.json") returned the
+// relative path "tusk/auth.json", so the token was written to the current
+// working directory – not the user's config dir.
+//
+// After the fix the error is propagated and NewAuthenticator returns an error,
+// preventing the misplaced write entirely.
+//
+// Reproduce:
+//
+//	HOME="" XDG_CONFIG_HOME="" go test ./internal/auth/ -run TestNewAuthenticator_UserConfigDirFailure
+func TestNewAuthenticator_UserConfigDirFailure(t *testing.T) {
+	// Unset every env var that os.UserConfigDir reads on any supported OS.
+	t.Setenv("HOME", "")
+	t.Setenv("XDG_CONFIG_HOME", "") // Linux fallback
+	if runtime.GOOS == "windows" {
+		t.Setenv("AppData", "")
+	}
+
+	auth, err := NewAuthenticator()
+
+	require.Error(t, err, "NewAuthenticator must fail when the config dir cannot be determined")
+	assert.Nil(t, auth)
+	assert.Contains(t, err.Error(), "cannot determine user config directory",
+		"error message should identify the root cause")
+
+	// Before the fix this test would PASS (no error returned) but the
+	// resulting Authenticator would have an authFilePath starting with
+	// "tusk/" – a relative path.  The assertion below is included as
+	// documentation of the old buggy behaviour:
+	//
+	//   assert.True(t, filepath.IsAbs(auth.authFilePath))  // would FAIL
+}
+
+// TestNewAuthenticator_AuthPathIsAbsolute verifies that, when the environment
+// is intact, the resolved auth file path is always absolute.  This would have
+// caught the misplacement silently introduced when cfgDir was empty.
+func TestNewAuthenticator_AuthPathIsAbsolute(t *testing.T) {
+	// NewAuthenticator also needs a minimal config (clientID etc.).
+	// Set the env vars expected by config.Get / the Authenticator constructor
+	// so the call succeeds in CI without a real config file.
+	t.Setenv("TUSK_AUTH0_CLIENT_ID", "test-client-id")
+	t.Setenv("TUSK_AUTH0_AUDIENCE", "drift-cli")
+
+	auth, err := NewAuthenticator()
+	if err != nil {
+		// If the env/config still isn't complete enough (e.g. missing YAML),
+		// skip rather than fail – the important assertion is the path check.
+		t.Skipf("NewAuthenticator setup incomplete in test env: %v", err)
+	}
+
+	assert.True(t, strings.HasPrefix(auth.authFilePath, "/") || (len(auth.authFilePath) > 1 && auth.authFilePath[1] == ':'),
+		"authFilePath %q must be an absolute path, not a relative one", auth.authFilePath)
+	assert.Contains(t, auth.authFilePath, "tusk",
+		"authFilePath should be inside a 'tusk' sub-directory")
+	assert.Contains(t, auth.authFilePath, "auth.json",
+		"authFilePath should point at auth.json")
+}
+
+// ── Bug 2: silent io.ReadAll errors ──────────────────────────────────────────
+
+// TestRequestDeviceCode_PropagatesReadError is a minimal repro for the
+// silent-error bug in RequestDeviceCode.
+//
+// Before the fix:
+//
+//	body, _ := io.ReadAll(resp.Body)   // network error silently dropped
+//	json.Unmarshal(body, &dcr)         // body is empty/partial → silent failure
+//
+// After the fix:
+//
+//	body, err := io.ReadAll(resp.Body)
+//	if err != nil { return dcr, fmt.Errorf("error reading device code response body: %w", err) }
+func TestRequestDeviceCode_PropagatesReadError(t *testing.T) {
+	a := newTestAuthenticator(&errorBodyTransport{})
+
+	_, err := a.RequestDeviceCode(context.Background())
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "error reading device code response body",
+		"ReadAll error must be surfaced with context")
+	assert.Contains(t, err.Error(), "simulated network read error")
+}
+
+// TestPollForToken_PropagatesReadError is a minimal repro for the
+// silent-error bug in PollForToken.
+//
+// Before the fix:
+//
+//	body, _ := io.ReadAll(resp.Body)   // error silently dropped
+func TestPollForToken_PropagatesReadError(t *testing.T) {
+	a := newTestAuthenticator(&errorBodyTransport{})
+
+	dcr := DeviceCodeResponse{
+		DeviceCode: "test-device-code",
+		Interval:   0, // zero → 5 s default, but ctx is cancelled immediately
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	err := a.PollForToken(ctx, dcr)
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "error reading token response body",
+		"ReadAll error must be surfaced with context")
+	assert.Contains(t, err.Error(), "simulated network read error")
+}
+
+// TestFetchUserEmail_PropagatesReadError is a minimal repro for the
+// silent-error bug in FetchUserEmail.
+//
+// Before the fix:
+//
+//	b, _ := io.ReadAll(resp.Body)   // error silently dropped
+func TestFetchUserEmail_PropagatesReadError(t *testing.T) {
+	a := newTestAuthenticator(&errorBodyTransport{})
+	a.AccessToken = "fake-access-token"
+
+	err := a.FetchUserEmail(context.Background())
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "error reading userinfo response body",
+		"ReadAll error must be surfaced with context")
+	assert.Contains(t, err.Error(), "simulated network read error")
+}
+
+// TestRefreshAccessToken_PropagatesReadError is a minimal repro for the
+// silent-error bug in refreshAccessToken.
+//
+// Before the fix:
+//
+//	body, _ := io.ReadAll(resp.Body)   // error silently dropped
+func TestRefreshAccessToken_PropagatesReadError(t *testing.T) {
+	a := newTestAuthenticator(&errorBodyTransport{})
+	a.RefreshToken = "fake-refresh-token"
+
+	err := a.refreshAccessToken(context.Background())
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "error reading refresh token response body",
+		"ReadAll error must be surfaced with context")
+	assert.Contains(t, err.Error(), "simulated network read error")
+}

internal/runner/executor.go

@@ -12,6 +12,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/Use-Tusk/fence/pkg/fence"
@@ -37,6 +38,7 @@ type Executor struct {
 	globalSpans            []*core.Span // Explicitly marked global spans for cross-trace matching
 	allowSuiteWideMatching bool         // When true, allows cross-trace matching from any suite span
 	cancelTests            context.CancelFunc
+	cancelTestsMu          sync.Mutex
 	disableSandbox         bool
 	debug                  bool
 	fenceManager           *fence.Manager
@@ -162,7 +164,9 @@ func (e *Executor) RunTestsConcurrently(tests []Test, maxConcurrency int) ([]Tes
 	defer cancel()
 
 	// Store cancel function so signal handler can call it
+	e.cancelTestsMu.Lock()
 	e.cancelTests = cancel
+	e.cancelTestsMu.Unlock()
 
 	testChan := make(chan Test, len(tests))
 	resultChan := make(chan TestResult, len(tests))
@@ -387,8 +391,11 @@ func (e *Executor) SetAllowSuiteWideMatching(enabled bool) {
 }
 
 func (e *Executor) CancelTests() {
-	if e.cancelTests != nil {
-		e.cancelTests()
+	e.cancelTestsMu.Lock()
+	cancel := e.cancelTests
+	e.cancelTestsMu.Unlock()
+	if cancel != nil {
+		cancel()
 	}
 }
 

internal/runner/executor_test.go

@@ -877,6 +877,68 @@ func BenchmarkExecutor_RunTestsConcurrently(b *testing.B) {
 	}
 }
 
+// TestExecutor_CancelTests_NoDataRace is a minimal repro for the data race that
+// existed between RunTestsConcurrently (which writes e.cancelTests) and CancelTests
+// (which reads and invokes e.cancelTests). The two operations ran in separate
+// goroutines without synchronisation, triggering the Go race detector.
+//
+// Run with: go test -race ./internal/runner/ -run TestExecutor_CancelTests_NoDataRace
+//
+// Before the fix (cancelTestsMu added in commit 3d1d0c8) this test would be
+// flagged as a DATA RACE by the race detector. After the fix it passes cleanly.
+func TestExecutor_CancelTests_NoDataRace(t *testing.T) {
+	// A fast server so the writer goroutine actually enters RunTestsConcurrently
+	// and stores the cancel function before the test finishes.
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// Tiny sleep so the two goroutines overlap in time.
+		time.Sleep(5 * time.Millisecond)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	// Run many iterations: each one races a write (RunTestsConcurrently stores
+	// cancelTests) against a read (CancelTests loads cancelTests).
+	const iterations = 50
+	for i := 0; i < iterations; i++ {
+		executor := NewExecutor()
+		executor.serviceURL = server.URL
+		executor.SetTestTimeout(500 * time.Millisecond)
+
+		tests := []Test{
+			{TraceID: "race-t1", Request: Request{Method: "GET", Path: "/"}},
+			{TraceID: "race-t2", Request: Request{Method: "GET", Path: "/"}},
+		}
+
+		var wg sync.WaitGroup
+		wg.Add(2)
+
+		// Goroutine A: writes e.cancelTests inside RunTestsConcurrently.
+		go func() {
+			defer wg.Done()
+			_, _ = executor.RunTestsConcurrently(tests, 2)
+		}()
+
+		// Goroutine B: reads and invokes e.cancelTests inside CancelTests.
+		// Without the mutex these two goroutines race on the same field.
+		go func() {
+			defer wg.Done()
+			executor.CancelTests()
+		}()
+
+		wg.Wait()
+	}
+}
+
+// TestExecutor_CancelTests_BeforeRunIsNoop verifies that CancelTests is safe to
+// call before any test run has started (e.cancelTests is nil).
+func TestExecutor_CancelTests_BeforeRunIsNoop(t *testing.T) {
+	executor := NewExecutor()
+	// Must not panic when cancelTests has never been set.
+	assert.NotPanics(t, func() {
+		executor.CancelTests()
+	})
+}
+
 func BenchmarkCountPassedTests(b *testing.B) {
 	// Create test data
 	results := make([]TestResult, 1000)