fix(config): race condition in createWorkRoot() (#2338)

coryan · web-flow · commit 46428ca2a397 · 2025-09-24T10:07:23.000-04:00
Creating a temporary directory based on a timestamp is inherently racy.
Use the standard functions to create temporary directories, and relax
the tests to check for what matters.
diff --git a/internal/config/config.go b/internal/config/config.go
@@ -24,7 +24,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
-	"time"
 )
 
 const (
@@ -60,7 +59,6 @@ const (
 
 // are variables so it can be replaced during testing.
 var (
-	now         = time.Now
 	tempDir     = os.TempDir
 	currentUser = user.Current
 )
@@ -272,19 +270,9 @@ func (c *Config) createWorkRoot() error {
 		slog.Info("Using specified working directory", "dir", c.WorkRoot)
 		return nil
 	}
-	t := now()
-	path := filepath.Join(tempDir(), fmt.Sprintf("librarian-%s", formatTimestamp(t)))
-
-	_, err := os.Stat(path)
-	switch {
-	case os.IsNotExist(err):
-		if err := os.Mkdir(path, 0755); err != nil {
-			return fmt.Errorf("unable to create temporary working directory '%s': %w", path, err)
-		}
-	case err == nil:
-		return fmt.Errorf("temporary working directory already exists: %s", path)
-	default:
-		return fmt.Errorf("unable to check directory '%s': %w", path, err)
+	path, err := os.MkdirTemp(tempDir(), "librarian-*")
+	if err != nil {
+		return err
 	}
 
 	slog.Info("Temporary working directory", "dir", path)
@@ -364,8 +352,3 @@ func validateHostMount(hostMount, defaultValue string) (bool, error) {
 
 	return true, nil
 }
-
-func formatTimestamp(t time.Time) string {
-	const yyyyMMddHHmmss = "20060102T150405Z" // Expected format by time library
-	return t.Format(yyyyMMddHHmmss)
-}
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
@@ -16,13 +16,11 @@ package config
 
 import (
 	"errors"
-	"fmt"
 	"os"
 	"os/user"
 	"path/filepath"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/google/go-cmp/cmp"
 )
@@ -253,23 +251,17 @@ func TestIsValid(t *testing.T) {
 }
 
 func TestCreateWorkRoot(t *testing.T) {
-	timestamp := time.Now()
 	localTempDir := t.TempDir()
-	now = func() time.Time {
-		return timestamp
-	}
 	tempDir = func() string {
 		return localTempDir
 	}
-	defer func() {
-		now = time.Now
+	t.Cleanup(func() {
 		tempDir = os.TempDir
-	}()
+	})
 	for _, test := range []struct {
 		name   string
 		config *Config
 		setup  func(t *testing.T) (string, func())
-		errMsg string
 	}{
 		{
 			name: "configured root",
@@ -294,53 +286,44 @@ func TestCreateWorkRoot(t *testing.T) {
 			name:   "without override, new dir",
 			config: &Config{},
 			setup: func(t *testing.T) (string, func()) {
-				expectedPath := filepath.Join(localTempDir, fmt.Sprintf("librarian-%s", formatTimestamp(timestamp)))
+				expectedPath := filepath.Join(localTempDir, "librarian-")
 				return expectedPath, func() {
 					if err := os.RemoveAll(expectedPath); err != nil {
 						t.Errorf("os.RemoveAll(%q) = %v; want nil", expectedPath, err)
 					}
 				}
 			},
 		},
-		{
-			name:   "without override, dir exists",
-			config: &Config{},
-			setup: func(t *testing.T) (string, func()) {
-				expectedPath := filepath.Join(localTempDir, fmt.Sprintf("librarian-%s", formatTimestamp(timestamp)))
-				if err := os.Mkdir(expectedPath, 0755); err != nil {
-					t.Fatalf("failed to create test dir: %v", err)
-				}
-				return expectedPath, func() {
-					if err := os.RemoveAll(expectedPath); err != nil {
-						t.Errorf("os.RemoveAll(%q) = %v; want nil", expectedPath, err)
-					}
-				}
-			},
-			errMsg: "working directory already exists",
-		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			want, cleanup := test.setup(t)
 			defer cleanup()
 
-			err := test.config.createWorkRoot()
-			if test.errMsg != "" {
-				if !strings.Contains(err.Error(), test.errMsg) {
-					t.Errorf("createWorkRoot() = %q, want contains %q", err, test.errMsg)
-				}
-				return
-			} else if err != nil {
+			if err := test.config.createWorkRoot(); err != nil {
 				t.Errorf("createWorkRoot() got unexpected error: %v", err)
 				return
 			}
 
-			if test.config.WorkRoot != want {
+			if !strings.HasPrefix(test.config.WorkRoot, want) {
 				t.Errorf("createWorkRoot() = %v, want %v", test.config.WorkRoot, want)
 			}
 		})
 	}
 }
 
+func TestCreateWorkRootError(t *testing.T) {
+	tempDir = func() string {
+		return filepath.Join("--invalid--", "--not-a-directory--")
+	}
+	t.Cleanup(func() {
+		tempDir = os.TempDir
+	})
+	config := &Config{}
+	if err := config.createWorkRoot(); err == nil {
+		t.Errorf("createWorkRoot() expected an error got: %v", config.WorkRoot)
+	}
+}
+
 func TestDeriveRepo(t *testing.T) {
 	for _, test := range []struct {
 		name         string
@@ -417,12 +400,7 @@ func TestSetDefaults(t *testing.T) {
 		}, nil
 	}
 
-	timestamp := time.Now()
-	now = func() time.Time {
-		return timestamp
-	}
 	t.Cleanup(func() {
-		now = time.Now
 		currentUser = user.Current
 	})
 	for _, test := range []struct {
