diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index f3d7acdba..11460457f 100644 --- a/internal/sandbox/sandbox.go +++ b/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 { diff --git a/internal/sandbox/sandbox_test.go b/internal/sandbox/sandbox_test.go index 5b64528b2..0b5b1388b 100644 --- a/internal/sandbox/sandbox_test.go +++ b/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) {