From fbca382977f8bd3071e040b4d3f3a53c81dc5ca8 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sun, 19 Apr 2026 19:53:32 -0500 Subject: [PATCH 1/2] fix: prevent duplicate groups via race condition in GetOrCreateRepoGroup GetOrCreateRepoGroup and GetOrCreateLocalFolderGroup had a check-then-create pattern without holding _organizationLock. During app startup, concurrent callers (session restore on ThreadPool + ReconcileOrganization) could both see 'no existing group' and create duplicates. This caused the '4 maui groups' bug where manual cleanup was undone by every restart. Fix: wrap the entire check-then-create in lock(_organizationLock). Monitor is reentrant so nested AddGroup() calls are safe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Services/CopilotService.Organization.cs | 144 ++++++++++-------- 1 file changed, 77 insertions(+), 67 deletions(-) diff --git a/PolyPilot/Services/CopilotService.Organization.cs b/PolyPilot/Services/CopilotService.Organization.cs index fb45599fa9..2818961d23 100644 --- a/PolyPilot/Services/CopilotService.Organization.cs +++ b/PolyPilot/Services/CopilotService.Organization.cs @@ -1428,48 +1428,54 @@ 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. + 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); - SaveOrganization(); - OnStateChanged?.Invoke(); - return group; + var group = new SessionGroup + { + Id = Guid.NewGuid().ToString(), + Name = repoName, + RepoId = repoId, + SortOrder = Organization.Groups.Max(g => g.SortOrder) + 1 + }; + AddGroup(group); + SaveOrganization(); + OnStateChanged?.Invoke(); + return group; + } } /// @@ -1483,36 +1489,40 @@ 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. + 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) + { + 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 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; + 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; + } } /// From c942150bae2852d38c5ee8b54fd4dec34922fd5e Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sun, 19 Apr 2026 21:37:25 -0500 Subject: [PATCH 2/2] fix: move side effects outside lock, protect PromoteOrCreateLocalFolderGroup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings: 1. Move SaveOrganization() + OnStateChanged?.Invoke() outside _organizationLock in GetOrCreateRepoGroup and GetOrCreateLocalFolderGroup — matches the file-wide convention (lock → mutate → release → notify) and eliminates latent deadlock risk from event subscribers. 2. Wrap PromoteOrCreateLocalFolderGroup in _organizationLock to fix the same TOCTOU race (concurrent callers could race on candidate promotion). 3. Add 3 concurrent stress tests validating that each method produces exactly one group under 20 parallel Task.Run callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/SessionOrganizationTests.cs | 79 ++++++++++ .../Services/CopilotService.Organization.cs | 138 ++++++++++-------- 2 files changed, 157 insertions(+), 60 deletions(-) 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 2818961d23..205a433d9c 100644 --- a/PolyPilot/Services/CopilotService.Organization.cs +++ b/PolyPilot/Services/CopilotService.Organization.cs @@ -1431,6 +1431,9 @@ internal bool IsWorkerInMultiAgentGroup(string sessionName) // 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) { // Skip multi-agent groups — they have a RepoId for worktree context but are @@ -1464,18 +1467,18 @@ internal bool IsWorkerInMultiAgentGroup(string sessionName) // Clear the deleted flag when explicitly re-adding Organization.DeletedRepoGroupRepoIds.Remove(repoId); - var group = new SessionGroup + created = new SessionGroup { Id = Guid.NewGuid().ToString(), Name = repoName, RepoId = repoId, SortOrder = Organization.Groups.Max(g => g.SortOrder) + 1 }; - AddGroup(group); - SaveOrganization(); - OnStateChanged?.Invoke(); - return group; + AddGroup(created); } + SaveOrganization(); + OnStateChanged?.Invoke(); + return created; } /// @@ -1490,6 +1493,10 @@ public SessionGroup GetOrCreateLocalFolderGroup(string localPath, string? repoId .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); // 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) { var existing = Organization.Groups.FirstOrDefault(g => @@ -1501,28 +1508,28 @@ public SessionGroup GetOrCreateLocalFolderGroup(string localPath, string? repoId StringComparison.OrdinalIgnoreCase)); if (existing != null) { - bool changed = false; - if (existing.IsCollapsed) { existing.IsCollapsed = false; changed = true; } + 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; changed = true; } - if (changed) { SaveOrganization(); OnStateChanged?.Invoke(); } - return existing; + if (repoId != null && existing.RepoId == null) { existing.RepoId = repoId; notify = true; } + result = existing; } - - var folderName = Path.GetFileName(normalized); - var group = new SessionGroup + else { - 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; + 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; + } } + if (notify) { SaveOrganization(); OnStateChanged?.Invoke(); } + return result; } /// @@ -1538,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.