fix(#6420): make ImportProfile concurrency-safe with hash-based caching

internal/sandbox/sandbox.go

@@ -149,9 +149,29 @@ func inGitDir(path, root string) bool {
 
 // ImportProfile imports a single openshell provider profile from a YAML
 // file. The profile defines a provider type schema (credentials, endpoints).
-// To ensure content changes propagate on persistent gateways, the profile
-// is deleted by id before re-importing (mirroring the ImportProfiles flow).
+//
+// Idempotency is hash-based: the function computes a SHA-256 digest of the
+// profile file and compares it against a cached value in a temp file keyed
+// by the profile id. When the hash matches (content unchanged), the import
+// is skipped entirely. This makes parallel fullsend run invocations safe —
+// only the first process imports, and subsequent processes see the cache hit.
+//
+// When content has changed (hash mismatch or no cache), the existing profile
+// is deleted and reimported. If the reimport fails because a parallel process
+// already imported it, the error is treated as success.
 func ImportProfile(ctx context.Context, id, profilePath string) error {
+	currentHash, err := hashProfileFile(profilePath)
+	if err != nil {
+		return fmt.Errorf("hashing profile %q: %w", filepath.Base(profilePath), err)
+	}
+
+	cachePath := profileFileCachePath(id)
+	if cached, readErr := os.ReadFile(cachePath); readErr == nil {
+		if strings.TrimSpace(string(cached)) == currentHash {
+			return nil
+		}
+	}
+
 	// Best-effort delete so content changes propagate (same pattern as ImportProfiles).
 	delCtx, delCancel := context.WithTimeout(ctx, providerTimeout)
 	exec.CommandContext(delCtx, "openshell", "provider", "profile", "delete", id).CombinedOutput() //nolint:errcheck
@@ -164,10 +184,13 @@ func ImportProfile(ctx context.Context, id, profilePath string) error {
 	if err != nil {
 		outStr := strings.ToLower(string(out))
 		if strings.Contains(outStr, "already exists") {
+			// A parallel process imported the profile — safe to continue.
+			os.WriteFile(cachePath, []byte(currentHash), 0o600) //nolint:errcheck
 			return nil
 		}
 		return fmt.Errorf("profile import %q failed: openshell: %w\noutput: %s", filepath.Base(profilePath), err, bytes.TrimSpace(out))
 	}
+	os.WriteFile(cachePath, []byte(currentHash), 0o600) //nolint:errcheck
 	return nil
 }
 
@@ -480,6 +503,25 @@ func profileCachePath(dir string) string {
 	return filepath.Join(os.TempDir(), "fullsend-profiles-"+hex.EncodeToString(dirHash[:8])+".sha256")
 }
 
+// hashProfileFile computes a SHA-256 digest of a single profile file's
+// contents. This is the single-file analog of hashProfileDir.
+func hashProfileFile(path string) (string, error) {
+	data, err := os.ReadFile(path)
+	if err != nil {
+		return "", err
+	}
+	sum := sha256.Sum256(data)
+	return hex.EncodeToString(sum[:]), nil
+}
+
+// profileFileCachePath returns a temp file path for caching the hash of a
+// single profile file. The path is keyed to the profile id so that
+// different profiles get separate caches.
+func profileFileCachePath(id string) string {
+	idHash := sha256.Sum256([]byte(id))
+	return filepath.Join(os.TempDir(), "fullsend-profile-"+hex.EncodeToString(idHash[:8])+".sha256")
+}
+
 // EnableProvidersV2 enables the providers_v2_enabled setting globally in the
 // openshell gateway. This is idempotent and can be called multiple times.
 func EnableProvidersV2() error {

internal/sandbox/sandbox_test.go

@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -1047,15 +1048,23 @@ func TestBuildProviderUpdateArgs(t *testing.T) {
 }
 
 func TestImportProfile_OpenshellNotInPath(t *testing.T) {
+	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "profile.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: test-profile"), 0o644))
 	t.Setenv("PATH", t.TempDir())
 
-	err := ImportProfile(context.Background(), "test-profile", "/some/profile.yaml")
+	cachePath := profileFileCachePath("test-profile")
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	err := ImportProfile(context.Background(), "test-profile", profilePath)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "openshell")
 }
 
 func TestImportProfile_Success(t *testing.T) {
 	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "my-profile.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: my-profile"), 0o644))
 
 	script := `#!/bin/sh
 exit 0
@@ -1064,12 +1073,17 @@ exit 0
 	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
 	t.Setenv("PATH", dir)
 
-	err := ImportProfile(context.Background(), "my-profile", "/some/my-profile.yaml")
+	cachePath := profileFileCachePath("my-profile")
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	err := ImportProfile(context.Background(), "my-profile", profilePath)
 	assert.NoError(t, err)
 }
 
 func TestImportProfile_UsesFileFlag(t *testing.T) {
 	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "my-profile.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: my-profile"), 0o644))
 	argsFile := filepath.Join(dir, "args.log")
 
 	// Fake openshell that logs args on "import" invocations and exits 0.
@@ -1083,17 +1097,22 @@ exit 0
 	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
 	t.Setenv("PATH", dir)
 
-	err := ImportProfile(context.Background(), "my-profile", "/some/my-profile.yaml")
+	cachePath := profileFileCachePath("my-profile")
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	err := ImportProfile(context.Background(), "my-profile", profilePath)
 	require.NoError(t, err)
 
 	logged, err := os.ReadFile(argsFile)
 	require.NoError(t, err)
-	assert.Contains(t, string(logged), "--file /some/my-profile.yaml",
+	assert.Contains(t, string(logged), "--file "+profilePath,
 		"ImportProfile must pass --file flag to openshell provider profile import")
 }
 
 func TestImportProfile_AlreadyExists(t *testing.T) {
 	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "my-profile.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: my-profile"), 0o644))
 
 	script := `#!/bin/sh
 echo "profile already exists" >&2
@@ -1103,12 +1122,17 @@ exit 1
 	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
 	t.Setenv("PATH", dir)
 
-	err := ImportProfile(context.Background(), "my-profile", "/some/my-profile.yaml")
+	cachePath := profileFileCachePath("my-profile")
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	err := ImportProfile(context.Background(), "my-profile", profilePath)
 	assert.NoError(t, err, "idempotent import should not return an error")
 }
 
 func TestImportProfile_OtherError(t *testing.T) {
 	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "my-profile.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: my-profile"), 0o644))
 
 	script := `#!/bin/sh
 echo "connection refused" >&2
@@ -1118,12 +1142,167 @@ exit 1
 	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
 	t.Setenv("PATH", dir)
 
-	err := ImportProfile(context.Background(), "my-profile", "/some/my-profile.yaml")
+	cachePath := profileFileCachePath("my-profile")
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	err := ImportProfile(context.Background(), "my-profile", profilePath)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "my-profile.yaml")
 	assert.Contains(t, err.Error(), "connection refused")
 }
 
+func TestImportProfile_SkipsWhenCacheMatches(t *testing.T) {
+	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "test.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: test-profile\nname: test"), 0o644))
+
+	hash, err := hashProfileFile(profilePath)
+	require.NoError(t, err)
+
+	cachePath := profileFileCachePath("test-profile")
+	require.NoError(t, os.WriteFile(cachePath, []byte(hash), 0o600))
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	// openshell is not in PATH — if ImportProfile tries to run it, it will fail.
+	// A successful return means the cache short-circuited the import.
+	t.Setenv("PATH", "")
+	err = ImportProfile(context.Background(), "test-profile", profilePath)
+	assert.NoError(t, err, "should skip import when cache hash matches")
+}
+
+func TestImportProfile_ReimportsWhenCacheDiffers(t *testing.T) {
+	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "test.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: test-profile\nname: test"), 0o644))
+
+	cachePath := profileFileCachePath("test-profile")
+	require.NoError(t, os.WriteFile(cachePath, []byte("stale-hash"), 0o600))
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	// With openshell missing, reimport will fail — proving the cache miss path runs.
+	t.Setenv("PATH", t.TempDir())
+	err := ImportProfile(context.Background(), "test-profile", profilePath)
+	assert.Error(t, err, "should attempt reimport when cache hash differs")
+}
+
+func TestImportProfile_WritesCacheOnSuccess(t *testing.T) {
+	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "test.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: cached\nname: test"), 0o644))
+
+	script := "#!/bin/sh\nexit 0\n"
+	fakePath := filepath.Join(dir, "openshell")
+	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
+	t.Setenv("PATH", dir)
+
+	cachePath := profileFileCachePath("cached")
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	err := ImportProfile(context.Background(), "cached", profilePath)
+	require.NoError(t, err)
+
+	// Cache file should now contain the profile hash.
+	cached, readErr := os.ReadFile(cachePath)
+	require.NoError(t, readErr, "cache file should exist after successful import")
+
+	expectedHash, _ := hashProfileFile(profilePath)
+	assert.Equal(t, expectedHash, string(cached))
+}
+
+func TestImportProfile_WritesCacheOnAlreadyExists(t *testing.T) {
+	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "test.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: exists\nname: test"), 0o644))
+
+	script := `#!/bin/sh
+echo "profile already exists" >&2
+exit 1
+`
+	fakePath := filepath.Join(dir, "openshell")
+	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
+	t.Setenv("PATH", dir)
+
+	cachePath := profileFileCachePath("exists")
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	err := ImportProfile(context.Background(), "exists", profilePath)
+	require.NoError(t, err, "already-exists should not be an error")
+
+	// Cache should be written even on already-exists (parallel import).
+	cached, readErr := os.ReadFile(cachePath)
+	require.NoError(t, readErr, "cache file should exist after already-exists import")
+
+	expectedHash, _ := hashProfileFile(profilePath)
+	assert.Equal(t, expectedHash, string(cached))
+}
+
+func TestImportProfile_ConcurrentAccess(t *testing.T) {
+	dir := t.TempDir()
+	profilePath := filepath.Join(dir, "concurrent.yaml")
+	require.NoError(t, os.WriteFile(profilePath, []byte("id: concurrent\nname: test"), 0o644))
+
+	script := "#!/bin/sh\nexit 0\n"
+	fakePath := filepath.Join(dir, "openshell")
+	require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755))
+	t.Setenv("PATH", dir)
+
+	cachePath := profileFileCachePath("concurrent")
+	t.Cleanup(func() { os.Remove(cachePath) })
+
+	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] = ImportProfile(context.Background(), "concurrent", profilePath)
+		}(i)
+	}
+	wg.Wait()
+
+	for i, err := range errs {
+		assert.NoError(t, err, "goroutine %d should succeed", i)
+	}
+}
+
+func TestHashProfileFile_Deterministic(t *testing.T) {
+	dir := t.TempDir()
+	f := filepath.Join(dir, "profile.yaml")
+	require.NoError(t, os.WriteFile(f, []byte("id: test\nname: profile"), 0o644))
+
+	h1, err := hashProfileFile(f)
+	require.NoError(t, err)
+	h2, err := hashProfileFile(f)
+	require.NoError(t, err)
+	assert.Equal(t, h1, h2, "hash must be deterministic for same content")
+}
+
+func TestHashProfileFile_ChangesOnContentChange(t *testing.T) {
+	dir := t.TempDir()
+	f := filepath.Join(dir, "profile.yaml")
+	require.NoError(t, os.WriteFile(f, []byte("id: test"), 0o644))
+
+	h1, err := hashProfileFile(f)
+	require.NoError(t, err)
+
+	require.NoError(t, os.WriteFile(f, []byte("id: test-modified"), 0o644))
+
+	h2, err := hashProfileFile(f)
+	require.NoError(t, err)
+	assert.NotEqual(t, h1, h2, "hash must change when file content changes")
+}
+
+func TestProfileFileCachePath_DeterministicAndUnique(t *testing.T) {
+	p1 := profileFileCachePath("profile-a")
+	p2 := profileFileCachePath("profile-a")
+	p3 := profileFileCachePath("profile-b")
+
+	assert.Equal(t, p1, p2, "same id must produce same cache path")
+	assert.NotEqual(t, p1, p3, "different ids must produce different cache paths")
+	assert.True(t, strings.HasPrefix(p1, os.TempDir()), "cache path must be in temp dir")
+}
+
 // TestEnsureProvider_AlreadyExists_FallsBackToUpdate uses a fake openshell
 // script: first invocation exits 1 with AlreadyExists, second exits 0.
 func TestEnsureProvider_AlreadyExists_FallsBackToUpdate(t *testing.T) {