diff --git a/PolyPilot.Tests/SessionOrganizationTests.cs b/PolyPilot.Tests/SessionOrganizationTests.cs index 4725e08a69..db7edf884f 100644 --- a/PolyPilot.Tests/SessionOrganizationTests.cs +++ b/PolyPilot.Tests/SessionOrganizationTests.cs @@ -4912,3 +4912,82 @@ public void HealMultiAgentGroups_MultipleOrchestrators_NoDuplicateGroups() #endregion } + +/// +/// Concurrent stress tests validating that GetOrCreateRepoGroup and GetOrCreateLocalFolderGroup +/// produce exactly one group under concurrent access (race condition fix from PR #638). +/// +public class GroupCreationConcurrencyTests +{ + private readonly StubChatDatabase _chatDb = new(); + private readonly StubServerManager _serverManager = new(); + private readonly StubWsBridgeClient _bridgeClient = new(); + private readonly StubDemoService _demoService = new(); + private readonly RepoManager _repoManager = new(); + private readonly IServiceProvider _serviceProvider; + + public GroupCreationConcurrencyTests() + { + var services = new ServiceCollection(); + _serviceProvider = services.BuildServiceProvider(); + } + + private CopilotService CreateService() => + new CopilotService(_chatDb, _serverManager, _bridgeClient, _repoManager, _serviceProvider, _demoService); + + [Fact] + public async Task GetOrCreateRepoGroup_ConcurrentCalls_CreatesExactlyOneGroup() + { + var svc = CreateService(); + var tasks = Enumerable.Range(0, 20).Select(_ => + Task.Run(() => svc.GetOrCreateRepoGroup("repo-1", "MyRepo"))); + var results = await Task.WhenAll(tasks); + + Assert.Single(svc.Organization.Groups.Where(g => g.RepoId == "repo-1" && !g.IsMultiAgent)); + Assert.All(results, g => Assert.Equal(results[0]!.Id, g!.Id)); + } + + [Fact] + public async Task GetOrCreateLocalFolderGroup_ConcurrentCalls_CreatesExactlyOneGroup() + { + var svc = CreateService(); + var tempDir = Path.Combine(Path.GetTempPath(), $"polypilot-test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + try + { + var tasks = Enumerable.Range(0, 20).Select(_ => + Task.Run(() => svc.GetOrCreateLocalFolderGroup(tempDir, "repo-2"))); + var results = await Task.WhenAll(tasks); + + Assert.Single(svc.Organization.Groups.Where(g => g.IsLocalFolder && g.RepoId == "repo-2")); + Assert.All(results, g => Assert.Equal(results[0].Id, g.Id)); + } + finally + { + try { Directory.Delete(tempDir, recursive: true); } catch { } + } + } + + [Fact] + public async Task PromoteOrCreateLocalFolderGroup_ConcurrentCalls_CreatesExactlyOneGroup() + { + var svc = CreateService(); + var tempDir = Path.Combine(Path.GetTempPath(), $"polypilot-test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + try + { + var tasks = Enumerable.Range(0, 20).Select(_ => + Task.Run(() => svc.PromoteOrCreateLocalFolderGroup(tempDir, "repo-3"))); + var results = await Task.WhenAll(tasks); + + var localGroups = svc.Organization.Groups.Where(g => + g.IsLocalFolder && g.RepoId == "repo-3").ToList(); + Assert.Single(localGroups); + Assert.All(results, g => Assert.Equal(results[0].Id, g.Id)); + } + finally + { + try { Directory.Delete(tempDir, recursive: true); } catch { } + } + } +} diff --git a/PolyPilot/Services/CopilotService.Organization.cs b/PolyPilot/Services/CopilotService.Organization.cs index fb45599fa9..205a433d9c 100644 --- a/PolyPilot/Services/CopilotService.Organization.cs +++ b/PolyPilot/Services/CopilotService.Organization.cs @@ -1428,48 +1428,57 @@ internal bool IsWorkerInMultiAgentGroup(string sessionName) /// public SessionGroup? GetOrCreateRepoGroup(string repoId, string repoName, bool explicitly = false) { - // Skip multi-agent groups — they have a RepoId for worktree context but are - // not the "repo group" that regular sessions should auto-join. - // Also skip local folder groups — they are a separate concept from URL-based repo groups, - // and coexist with them when the same repo is added both ways. - // Also skip groups that have orchestrator/worker sessions (defensive: protects against - // IsMultiAgent being lost due to stale writes or serialization issues). - var existing = Organization.Groups.FirstOrDefault(g => g.RepoId == repoId && !g.IsMultiAgent && !g.IsLocalFolder - && !Organization.Sessions.Any(m => m.GroupId == g.Id && m.Role == MultiAgentRole.Orchestrator)); - if (existing != null) return existing; - - // Don't recreate groups the user explicitly deleted (unless re-adding) - if (!explicitly && Organization.DeletedRepoGroupRepoIds.Contains(repoId)) - return null; - - // Don't create a URL-based group when a local folder group already covers this repo - // and no URL-based group exists. This prevents duplicate sidebar entries for local-only - // repos (e.g., "maui" appearing twice — once as local folder, once as URL-based). - // Local-only repos are identified by RepoManager.IsLocalOnlyRepoId (IDs containing the - // "-local-" infix from AddRepositoryFromLocalAsync). For these repos, the local folder - // group IS the repo's group — no URL-based group should be created. - // Exception: repos WITHOUT the local infix (same ID for both URL and local groups) - // are allowed to create URL groups for the heal-stranded-sessions scenario. - if (!explicitly && RepoManager.IsLocalOnlyRepoId(repoId) - && Organization.Groups.Any(g => g.RepoId == repoId && g.IsLocalFolder && !g.IsMultiAgent)) + // Lock the entire check-then-create to prevent concurrent callers (e.g., ReconcileOrganization + // on ThreadPool + session restore) from both seeing "no existing group" and creating duplicates. + // Monitor is reentrant, so nested AddGroup() calls are safe. + // Side effects (SaveOrganization + OnStateChanged) are outside the lock to match the + // file-wide convention and avoid latent deadlock risk from event subscribers. + SessionGroup? created = null; + lock (_organizationLock) { - return null; - } + // Skip multi-agent groups — they have a RepoId for worktree context but are + // not the "repo group" that regular sessions should auto-join. + // Also skip local folder groups — they are a separate concept from URL-based repo groups, + // and coexist with them when the same repo is added both ways. + // Also skip groups that have orchestrator/worker sessions (defensive: protects against + // IsMultiAgent being lost due to stale writes or serialization issues). + var existing = Organization.Groups.FirstOrDefault(g => g.RepoId == repoId && !g.IsMultiAgent && !g.IsLocalFolder + && !Organization.Sessions.Any(m => m.GroupId == g.Id && m.Role == MultiAgentRole.Orchestrator)); + if (existing != null) return existing; + + // Don't recreate groups the user explicitly deleted (unless re-adding) + if (!explicitly && Organization.DeletedRepoGroupRepoIds.Contains(repoId)) + return null; + + // Don't create a URL-based group when a local folder group already covers this repo + // and no URL-based group exists. This prevents duplicate sidebar entries for local-only + // repos (e.g., "maui" appearing twice — once as local folder, once as URL-based). + // Local-only repos are identified by RepoManager.IsLocalOnlyRepoId (IDs containing the + // "-local-" infix from AddRepositoryFromLocalAsync). For these repos, the local folder + // group IS the repo's group — no URL-based group should be created. + // Exception: repos WITHOUT the local infix (same ID for both URL and local groups) + // are allowed to create URL groups for the heal-stranded-sessions scenario. + if (!explicitly && RepoManager.IsLocalOnlyRepoId(repoId) + && Organization.Groups.Any(g => g.RepoId == repoId && g.IsLocalFolder && !g.IsMultiAgent)) + { + return null; + } - // Clear the deleted flag when explicitly re-adding - Organization.DeletedRepoGroupRepoIds.Remove(repoId); + // Clear the deleted flag when explicitly re-adding + Organization.DeletedRepoGroupRepoIds.Remove(repoId); - var group = new SessionGroup - { - Id = Guid.NewGuid().ToString(), - Name = repoName, - RepoId = repoId, - SortOrder = Organization.Groups.Max(g => g.SortOrder) + 1 - }; - AddGroup(group); + created = new SessionGroup + { + Id = Guid.NewGuid().ToString(), + Name = repoName, + RepoId = repoId, + SortOrder = Organization.Groups.Max(g => g.SortOrder) + 1 + }; + AddGroup(created); + } SaveOrganization(); OnStateChanged?.Invoke(); - return group; + return created; } /// @@ -1483,36 +1492,44 @@ public SessionGroup GetOrCreateLocalFolderGroup(string localPath, string? repoId var normalized = Path.GetFullPath(localPath) .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); - var existing = Organization.Groups.FirstOrDefault(g => - g.IsLocalFolder && - !g.IsMultiAgent && - string.Equals( - Path.GetFullPath(g.LocalPath!).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), - normalized, - StringComparison.OrdinalIgnoreCase)); - if (existing != null) + // Lock the entire check-then-create to prevent concurrent callers from creating duplicates. + // Side effects (SaveOrganization + OnStateChanged) are outside the lock to match the + // file-wide convention and avoid latent deadlock risk from event subscribers. + SessionGroup result; + bool notify = false; + lock (_organizationLock) { - bool changed = false; - if (existing.IsCollapsed) { existing.IsCollapsed = false; changed = true; } - // Back-fill RepoId if we now know it - if (repoId != null && existing.RepoId == null) { existing.RepoId = repoId; changed = true; } - if (changed) { SaveOrganization(); OnStateChanged?.Invoke(); } - return existing; + var existing = Organization.Groups.FirstOrDefault(g => + g.IsLocalFolder && + !g.IsMultiAgent && + string.Equals( + Path.GetFullPath(g.LocalPath!).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), + normalized, + StringComparison.OrdinalIgnoreCase)); + if (existing != null) + { + if (existing.IsCollapsed) { existing.IsCollapsed = false; notify = true; } + // Back-fill RepoId if we now know it + if (repoId != null && existing.RepoId == null) { existing.RepoId = repoId; notify = true; } + result = existing; + } + else + { + var folderName = Path.GetFileName(normalized); + result = new SessionGroup + { + Id = Guid.NewGuid().ToString(), + Name = folderName, + LocalPath = normalized, + RepoId = repoId, + SortOrder = Organization.Groups.Any() ? Organization.Groups.Max(g => g.SortOrder) + 1 : 0 + }; + AddGroup(result); + notify = true; + } } - - var folderName = Path.GetFileName(normalized); - var group = new SessionGroup - { - Id = Guid.NewGuid().ToString(), - Name = folderName, - LocalPath = normalized, - RepoId = repoId, - SortOrder = Organization.Groups.Any() ? Organization.Groups.Max(g => g.SortOrder) + 1 : 0 - }; - AddGroup(group); - SaveOrganization(); - OnStateChanged?.Invoke(); - return group; + if (notify) { SaveOrganization(); OnStateChanged?.Invoke(); } + return result; } /// @@ -1528,45 +1545,56 @@ public SessionGroup PromoteOrCreateLocalFolderGroup(string localPath, string? re var normalized = Path.GetFullPath(localPath) .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); - // If a local folder group already exists for this exact path, just update it. - var alreadyLocal = Organization.Groups.FirstOrDefault(g => - g.IsLocalFolder && !g.IsMultiAgent && - string.Equals( - Path.GetFullPath(g.LocalPath!).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), - normalized, StringComparison.OrdinalIgnoreCase)); - if (alreadyLocal != null) - { - bool changed = false; - if (alreadyLocal.IsCollapsed) { alreadyLocal.IsCollapsed = false; changed = true; } - if (repoId != null && alreadyLocal.RepoId == null) { alreadyLocal.RepoId = repoId; changed = true; } - if (changed) { SaveOrganization(); OnStateChanged?.Invoke(); } - return alreadyLocal; - } - - // Look for an existing URL-based group for this repo to promote in-place. - // Pick the most recently created (highest SortOrder) non-multi-agent group. - // This handles migration from older code versions that created URL-based groups - // instead of local folder groups when the user added an existing folder. - if (repoId != null) - { - var candidate = Organization.Groups - .Where(g => g.RepoId == repoId && !g.IsLocalFolder && !g.IsMultiAgent) - .OrderByDescending(g => g.SortOrder) - .FirstOrDefault(); - if (candidate != null) + // Lock the entire check-then-promote/create to prevent concurrent callers from racing + // on the same candidate group (same TOCTOU pattern as GetOrCreateRepoGroup). + // Side effects (SaveOrganization + OnStateChanged) are outside the lock. + SessionGroup? result = null; + bool notify = false; + lock (_organizationLock) + { + // If a local folder group already exists for this exact path, just update it. + var alreadyLocal = Organization.Groups.FirstOrDefault(g => + g.IsLocalFolder && !g.IsMultiAgent && + string.Equals( + Path.GetFullPath(g.LocalPath!).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), + normalized, StringComparison.OrdinalIgnoreCase)); + if (alreadyLocal != null) { - candidate.LocalPath = normalized; - // Preserve the user's group name — don't overwrite with the folder basename. - // The old code did: candidate.Name = Path.GetFileName(normalized) - // which destroyed user-customized names (e.g., "maui" → "maui2"). - // Fallback: if the group somehow has an empty name, use the folder basename. - if (string.IsNullOrWhiteSpace(candidate.Name)) - candidate.Name = Path.GetFileName(normalized); - SaveOrganization(); - OnStateChanged?.Invoke(); - Debug($"PromoteOrCreateLocalFolderGroup: promoted '{candidate.Id}' ('{candidate.Name}') to local folder group for '{normalized}'"); - return candidate; + if (alreadyLocal.IsCollapsed) { alreadyLocal.IsCollapsed = false; notify = true; } + if (repoId != null && alreadyLocal.RepoId == null) { alreadyLocal.RepoId = repoId; notify = true; } + result = alreadyLocal; } + + // Look for an existing URL-based group for this repo to promote in-place. + // Pick the most recently created (highest SortOrder) non-multi-agent group. + // This handles migration from older code versions that created URL-based groups + // instead of local folder groups when the user added an existing folder. + if (result == null && repoId != null) + { + var candidate = Organization.Groups + .Where(g => g.RepoId == repoId && !g.IsLocalFolder && !g.IsMultiAgent) + .OrderByDescending(g => g.SortOrder) + .FirstOrDefault(); + if (candidate != null) + { + candidate.LocalPath = normalized; + // Preserve the user's group name — don't overwrite with the folder basename. + // The old code did: candidate.Name = Path.GetFileName(normalized) + // which destroyed user-customized names (e.g., "maui" → "maui2"). + // Fallback: if the group somehow has an empty name, use the folder basename. + if (string.IsNullOrWhiteSpace(candidate.Name)) + candidate.Name = Path.GetFileName(normalized); + notify = true; + result = candidate; + Debug($"PromoteOrCreateLocalFolderGroup: promoted '{candidate.Id}' ('{candidate.Name}') to local folder group for '{normalized}'"); + } + } + } + + if (result != null) + { + if (notify) { SaveOrganization(); OnStateChanged?.Invoke(); } + return result; } // No existing group to promote — create a fresh local folder group.