fix(#6448): add flock serialization to ImportProfiles

internal/sandbox/sandbox.go

@@ -502,6 +502,13 @@ func CheckGateway() error {
 // entirely. This makes parallel fullsend run invocations safe — only the
 // first process imports, and subsequent processes see the cache hit.
 //
+// Concurrency safety: the delete+reimport critical section is protected by
+// a cross-process file lock (flock) keyed by directory path. This prevents the
+// race where a concurrent process deletes profiles between another process's
+// import and provider creation. Processes that block on the lock re-check the
+// cache after acquiring it (double-check pattern) and skip the import if the
+// winner already wrote the cache.
+//
 // When profiles have changed (hash mismatch or no cache), existing profiles
 // are deleted and reimported. If the reimport fails because a parallel process
 // already imported them, the error is treated as success.
@@ -518,13 +525,36 @@ func ImportProfiles(dir string) error {
 		return fmt.Errorf("hashing profiles directory %s: %w", dir, err)
 	}
 
+	// Fast path: check cache before acquiring the lock.
 	cachePath := profileCachePath(dir)
 	if cached, readErr := os.ReadFile(cachePath); readErr == nil {
 		if strings.TrimSpace(string(cached)) == currentHash {
 			return nil
 		}
 	}
 
+	// Acquire a cross-process file lock so only one process at a time
+	// performs the non-atomic delete+reimport sequence for this directory.
+	lockPath := profileDirLockPath(dir)
+	lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return fmt.Errorf("opening profiles lock for %q: %w", dir, err)
+	}
+	defer lockFile.Close()
+
+	if err := syscall.Flock(int(lockFile.Fd()), syscall.LOCK_EX); err != nil {
+		return fmt.Errorf("acquiring profiles lock for %q: %w", dir, err)
+	}
+	defer syscall.Flock(int(lockFile.Fd()), syscall.LOCK_UN) //nolint:errcheck
+
+	// Double-check: the process that held the lock before us may have
+	// already imported these profiles and written the cache.
+	if cached, readErr := os.ReadFile(cachePath); readErr == nil {
+		if strings.TrimSpace(string(cached)) == currentHash {
+			return nil
+		}
+	}
+
 	ids, err := resolve.CollectProfileIDs(dir)
 	if err != nil {
 		return err
@@ -574,16 +604,31 @@ func hashProfileDir(dir string) (string, error) {
 	return hex.EncodeToString(h.Sum(nil)), nil
 }
 
-// profileCachePath returns a temp file path for caching the profile directory
-// hash. The path is keyed to the absolute directory path so that different
-// fullsend-dir values get separate caches.
-func profileCachePath(dir string) string {
+// profileDirTempPath returns a temp file path keyed by the absolute directory
+// path with the given extension. Used by profileCachePath and profileDirLockPath
+// to derive deterministic, per-directory paths without duplicating the hashing
+// logic. This mirrors profileTempPath for single-profile paths.
+func profileDirTempPath(dir, ext string) string {
 	absDir, err := filepath.Abs(dir)
 	if err != nil {
 		absDir = dir
 	}
 	dirHash := sha256.Sum256([]byte(absDir))
-	return filepath.Join(os.TempDir(), "fullsend-profiles-"+hex.EncodeToString(dirHash[:8])+".sha256")
+	return filepath.Join(os.TempDir(), "fullsend-profiles-"+hex.EncodeToString(dirHash[:8])+"."+ext)
+}
+
+// profileCachePath returns a temp file path for caching the profile directory
+// hash. The path is keyed to the absolute directory path so that different
+// fullsend-dir values get separate caches.
+func profileCachePath(dir string) string {
+	return profileDirTempPath(dir, "sha256")
+}
+
+// profileDirLockPath returns a temp file path used as a cross-process flock
+// for serializing the delete+reimport critical section in ImportProfiles.
+// Keyed by directory path so different profile directories lock independently.
+func profileDirLockPath(dir string) string {
+	return profileDirTempPath(dir, "lock")
 }
 
 // hashProfileFile computes a SHA-256 digest of a single profile file's

internal/sandbox/sandbox_test.go

@@ -10,6 +10,7 @@ import (
 	"runtime"
 	"strings"
 	"sync"
+	"syscall"
 	"testing"
 	"time"
 
@@ -1703,3 +1704,141 @@ func TestResolvedBasename(t *testing.T) {
 		assert.Equal(t, "gone.txt", resolvedBasename("/no/such/gone.txt"))
 	})
 }
+
+func TestProfileDirLockPath_DeterministicAndUnique(t *testing.T) {
+	p1 := profileDirLockPath("/some/profiles")
+	p2 := profileDirLockPath("/some/profiles")
+	p3 := profileDirLockPath("/other/profiles")
+
+	assert.Equal(t, p1, p2, "same dir must produce same lock path")
+	assert.NotEqual(t, p1, p3, "different dirs must produce different lock paths")
+	assert.True(t, strings.HasPrefix(p1, os.TempDir()), "lock path must be in temp dir")
+	assert.True(t, strings.HasSuffix(p1, ".lock"), "lock path must end with .lock")
+}
+
+// TestImportProfiles_FlockSerializesConcurrent verifies that the flock in
+// ImportProfiles serializes concurrent access. The fake openshell import
+// handler uses a marker file to detect overlapping executions: it creates
+// the marker at entry and removes it at exit, failing if the marker
+// already exists. Without the flock, concurrent goroutines would enter
+// the import section simultaneously and the marker-already-present check
+// would trigger a failure, proving the lock is load-bearing.
+func TestImportProfiles_FlockSerializesConcurrent(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "p1.yaml"), []byte("id: p1\nname: profile-one"), 0o644))
+
+	// Marker file used by the fake openshell to detect concurrent imports.
+	markerFile := filepath.Join(dir, "import-active")
+	script := fmt.Sprintf(`#!/bin/sh
+if [ "$3" = "import" ]; then
+  if [ -f "%s" ]; then
+    echo "concurrent import detected" >&2
+    exit 1
+  fi
+  echo $$ > "%s"
+  sleep 0.05
+  rm -f "%s"
+fi
+exit 0
+`, markerFile, markerFile, markerFile)
+	fakePath := filepath.Join(dir, "openshell")
+	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
+	t.Setenv("PATH", dir)
+
+	cachePath := profileCachePath(dir)
+	lockPath := profileDirLockPath(dir)
+	t.Cleanup(func() {
+		os.Remove(cachePath)
+		os.Remove(lockPath)
+	})
+
+	const goroutines = 12
+	errs := make([]error, goroutines)
+	var wg sync.WaitGroup
+	for i := range goroutines {
+		wg.Add(1)
+		go func(idx int) {
+			defer wg.Done()
+			errs[idx] = ImportProfiles(dir)
+		}(i)
+	}
+	wg.Wait()
+
+	for i, err := range errs {
+		assert.NoError(t, err, "goroutine %d should succeed under flock serialization", i)
+	}
+}
+
+// TestImportProfiles_DoubleCheckCacheHit verifies the double-check pattern:
+// after acquiring the lock, ImportProfiles re-reads the cache and returns early
+// if another process already imported the profiles while we were waiting.
+func TestImportProfiles_DoubleCheckCacheHit(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "p1.yaml"), []byte("id: p1\nname: profile-one"), 0o644))
+
+	// Set up a no-op openshell so the test doesn't need a real one.
+	// If the double-check cache hit works, openshell import is never called.
+	script := `#!/bin/sh
+if [ "$3" = "import" ]; then
+  echo "import should not be called" >&2
+  exit 1
+fi
+exit 0
+`
+	fakePath := filepath.Join(dir, "openshell")
+	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
+	t.Setenv("PATH", dir)
+
+	cachePath := profileCachePath(dir)
+	lockPath := profileDirLockPath(dir)
+	t.Cleanup(func() {
+		os.Remove(cachePath)
+		os.Remove(lockPath)
+	})
+
+	// Compute the expected hash so we can pre-populate the cache.
+	currentHash, err := hashProfileDir(dir)
+	require.NoError(t, err)
+
+	// Acquire the lock before starting ImportProfiles so it blocks.
+	lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600)
+	require.NoError(t, err)
+	defer lockFile.Close()
+	require.NoError(t, syscall.Flock(int(lockFile.Fd()), syscall.LOCK_EX))
+
+	// No cache file → fast-path will miss. Start ImportProfiles in a
+	// goroutine; it will block waiting for our lock.
+	var importErr error
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		importErr = ImportProfiles(dir)
+	}()
+
+	// Give ImportProfiles time to pass the fast-path check and block on the lock.
+	time.Sleep(50 * time.Millisecond)
+
+	// Simulate the "other process" having written the cache while we held the lock.
+	require.NoError(t, os.WriteFile(cachePath, []byte(currentHash), 0o600))
+
+	// Release the lock — ImportProfiles should now hit the double-check,
+	// see the cache, and return nil without calling openshell import.
+	require.NoError(t, syscall.Flock(int(lockFile.Fd()), syscall.LOCK_UN))
+
+	<-done
+	assert.NoError(t, importErr, "double-check cache hit should succeed without importing")
+}
+
+// TestImportProfiles_LockOpenFailure verifies that ImportProfiles returns a
+// clear error when the lock file cannot be created (e.g., temp dir missing).
+func TestImportProfiles_LockOpenFailure(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "p1.yaml"), []byte("id: p1\nname: profile-one"), 0o644))
+
+	// Point TMPDIR to a non-existent directory so lock file creation fails.
+	t.Setenv("TMPDIR", filepath.Join(dir, "nonexistent"))
+
+	err := ImportProfiles(dir)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "opening profiles lock")
+}