Skip to content

fix: prevent duplicate groups via race condition in GetOrCreateRepoGroup #638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PureWeen merged 2 commits into from
Apr 20, 2026
Merged

fix: prevent duplicate groups via race condition in GetOrCreateRepoGroup #638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbca382
fix: prevent duplicate groups via race condition in GetOrCreateRepoGroup
PureWeen Apr 20, 2026
c942150
fix: move side effects outside lock, protect PromoteOrCreateLocalFold…
PureWeen Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions PolyPilot.Tests/SessionOrganizationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4912,3 +4912,82 @@ public void HealMultiAgentGroups_MultipleOrchestrators_NoDuplicateGroups()

#endregion
}

/// <summary>
/// Concurrent stress tests validating that GetOrCreateRepoGroup and GetOrCreateLocalFolderGroup
/// produce exactly one group under concurrent access (race condition fix from PR #638).
/// </summary>
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 { }
}
}
}
232 changes: 130 additions & 102 deletions PolyPilot/Services/CopilotService.Organization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1428,48 +1428,57 @@ internal bool IsWorkerInMultiAgentGroup(string sessionName)
/// </summary>
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;
}

/// <summary>
Expand All @@ -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;
}

/// <summary>
Expand All @@ -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.
Expand Down