From 60960dac6e1230949dbc0cab9298a3fecf302a64 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sun, 5 Apr 2026 11:59:58 -0500 Subject: [PATCH 1/4] fix: Preserve sessions on name collision instead of silently dropping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When MergeSessionEntries encounters a persisted entry with the same display name as an active entry but a different session ID, it now renames the persisted entry to 'Name (previous)' instead of silently dropping it. This prevents data loss when a session is replaced during restore (e.g., CLI resume failure → CreateSessionAsync fallback). Also adds IsLikelyWorkerSession() heuristic for future use in filtering orchestrated worker/evaluator sessions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/SessionPersistenceTests.cs | 108 ++++++++++++++++-- .../Services/CopilotService.Persistence.cs | 43 ++++++- 2 files changed, 143 insertions(+), 8 deletions(-) diff --git a/PolyPilot.Tests/SessionPersistenceTests.cs b/PolyPilot.Tests/SessionPersistenceTests.cs index df30ec812..2d9d86dfe 100644 --- a/PolyPilot.Tests/SessionPersistenceTests.cs +++ b/PolyPilot.Tests/SessionPersistenceTests.cs @@ -206,19 +206,22 @@ public void Merge_SomeDirsExist_OnlyThoseKept() // --- MergeSessionEntries: display name deduplication --- [Fact] - public void Merge_DuplicateDisplayName_ActiveWins_PersistedDropped() + public void Merge_DuplicateDisplayName_ActiveWins_PersistedRecoveredWithNewName() { // Active session "MyChat" has ID "new-id" (from reconnect). // Persisted has old entry with stale ID "old-id" but same display name. - // Only the active entry should survive — no ghost duplicates. + // Both entries should survive — the persisted one gets renamed to prevent data loss. var active = new List { Entry("new-id", "MyChat") }; var persisted = new List { Entry("old-id", "MyChat") }; var closed = new HashSet(); var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), _ => true); - Assert.Single(result); + Assert.Equal(2, result.Count); Assert.Equal("new-id", result[0].SessionId); + Assert.Equal("MyChat", result[0].DisplayName); + Assert.Equal("old-id", result[1].SessionId); + Assert.Equal("MyChat (previous)", result[1].DisplayName); } [Fact] @@ -1114,17 +1117,19 @@ public void Merge_RecreatedSessionWithNewId_OverridesOldEntry() var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), _ => true); - // Only the active (new ID) entry should survive — persisted entry has same display name - Assert.Single(result); + // Active entry keeps original name; persisted entry is recovered with "(previous)" suffix + Assert.Equal(2, result.Count); Assert.Equal("new-id-after-recreate", result[0].SessionId); Assert.Equal("AndroidShellHandler", result[0].DisplayName); + Assert.Equal("old-stale-id", result[1].SessionId); + Assert.Equal("AndroidShellHandler (previous)", result[1].DisplayName); } [Fact] public void Merge_RecreatedSessionNewId_OldIdNotResurrected() { // Even if the old session directory still exists, the persisted entry with a - // matching display name must not be re-added (it has a stale SessionId). + // matching display name is kept under a renamed display name to prevent data loss. var active = new List { Entry("recreated-id", "MySession") }; var persisted = new List { Entry("original-id", "MySession") }; var closed = new HashSet(); @@ -1132,8 +1137,11 @@ public void Merge_RecreatedSessionNewId_OldIdNotResurrected() var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), sessionId => true); // All dirs exist - Assert.Single(result); + Assert.Equal(2, result.Count); Assert.Equal("recreated-id", result[0].SessionId); + Assert.Equal("MySession", result[0].DisplayName); + Assert.Equal("original-id", result[1].SessionId); + Assert.Equal("MySession (previous)", result[1].DisplayName); } // --- Regression: events.jsonl copy when new file already exists --- @@ -1615,4 +1623,90 @@ public void CopyEventsToNewSession_CleansTempFileOnFailure() Assert.Contains("tmpFile = null; // Move succeeded", persistenceFile); Assert.Contains("File.Delete(tmpFile)", persistenceFile); } + + // --- MergeSessionEntries: name collision recovery --- + + [Fact] + public void Merge_NameCollision_MultiplePersisted_AllRecoveredWithUniqueNames() + { + var active = new List { Entry("new-id", "MyChat") }; + var persisted = new List + { + Entry("old-id-1", "MyChat"), + Entry("old-id-2", "MyChat") + }; + var closed = new HashSet(); + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), _ => true); + + Assert.Equal(3, result.Count); + Assert.Equal("MyChat", result[0].DisplayName); + Assert.Contains(result, e => e.SessionId == "old-id-1"); + Assert.Contains(result, e => e.SessionId == "old-id-2"); + var names = result.Select(e => e.DisplayName).ToList(); + Assert.Equal(names.Count, names.Distinct(StringComparer.OrdinalIgnoreCase).Count()); + } + + [Fact] + public void Merge_NameCollision_ClosedPersistedStillExcluded() + { + var active = new List { Entry("new-id", "MyChat") }; + var persisted = new List { Entry("closed-old", "MyChat") }; + var closedIds = new HashSet(StringComparer.OrdinalIgnoreCase) { "closed-old" }; + + var result = CopilotService.MergeSessionEntries(active, persisted, closedIds, new HashSet(), _ => true); + + Assert.Single(result); + Assert.Equal("new-id", result[0].SessionId); + } + + [Fact] + public void Merge_NameCollision_MissingDirStillExcluded() + { + var active = new List { Entry("new-id", "MyChat") }; + var persisted = new List { Entry("old-id", "MyChat") }; + + var result = CopilotService.MergeSessionEntries(active, persisted, new HashSet(), new HashSet(), _ => false); + + Assert.Single(result); + Assert.Equal("new-id", result[0].SessionId); + } + + // --- IsLikelyWorkerSession --- + + [Fact] + public void IsLikelyWorkerSession_WorkerBranch_ReturnsTrue() + { + Assert.True(CopilotService.IsLikelyWorkerSession("Implement---Challenge-worker-1-abc1", null)); + Assert.True(CopilotService.IsLikelyWorkerSession("Implement---Challenge-orchestrator-5b44", null)); + Assert.True(CopilotService.IsLikelyWorkerSession("Skill-Validator-worker-2-9671", null)); + } + + [Fact] + public void IsLikelyWorkerSession_EvaluatorBranch_ReturnsTrue() + { + Assert.True(CopilotService.IsLikelyWorkerSession("evaluator-session-1", null)); + } + + [Fact] + public void IsLikelyWorkerSession_OrchestratorSummary_ReturnsTrue() + { + Assert.True(CopilotService.IsLikelyWorkerSession(null, "You are the Implementer. Your job is to write correct code.")); + Assert.True(CopilotService.IsLikelyWorkerSession(null, "You are the Challenger. Your job is to find bugs.")); + Assert.True(CopilotService.IsLikelyWorkerSession(null, "You are a PR reviewer. When assigned a PR...")); + } + + [Fact] + public void IsLikelyWorkerSession_NormalSession_ReturnsFalse() + { + Assert.False(CopilotService.IsLikelyWorkerSession("fix/model-1m-slug", "fix the PR number display")); + Assert.False(CopilotService.IsLikelyWorkerSession("main", "can you analyze the copilot cli?")); + Assert.False(CopilotService.IsLikelyWorkerSession(null, null)); + } + + [Fact] + public void IsLikelyWorkerSession_SkillValidatorBranch_ReturnsTrue() + { + Assert.True(CopilotService.IsLikelyWorkerSession("Skill-Validator-worker-3-78c9", null)); + } } diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index 18bde439a..d105170f7 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -155,6 +155,9 @@ private void WriteActiveSessionsFile(List entries) /// Merge active (in-memory) session entries with persisted (on-disk) entries. /// Persisted entries are kept if they aren't already active, weren't explicitly /// closed (by ID or display name), and their session directory still exists. + /// When a persisted entry has a name collision with an active entry that has a + /// DIFFERENT session ID, the persisted entry is kept under a renamed display + /// name to prevent data loss from session replacement. /// internal static List MergeSessionEntries( List active, @@ -169,22 +172,60 @@ internal static List MergeSessionEntries( // This stops persisted entries from shadowing active sessions after reconnect. // Persisted entries may still share names with each other. var activeNames = new HashSet(active.Select(e => e.DisplayName).Where(n => n != null), StringComparer.OrdinalIgnoreCase); + // Track all merged display names to avoid collisions when renaming recovered entries + var allMergedNames = new HashSet(activeNames, StringComparer.OrdinalIgnoreCase); foreach (var existing in persisted) { if (activeIds.Contains(existing.SessionId)) continue; - if (activeNames.Contains(existing.DisplayName)) continue; if (closedIds.Contains(existing.SessionId)) continue; if (closedNames.Contains(existing.DisplayName)) continue; if (!sessionDirExists(existing.SessionId)) continue; + // Name collision: an active session has the same display name but a different + // session ID. This happens when a session is replaced (reconnect, lazy-resume + // fallback, etc.). Instead of silently dropping the persisted entry (losing its + // history), keep it under a unique name so the user can find it. + if (activeNames.Contains(existing.DisplayName)) + { + var recoveredName = $"{existing.DisplayName} (previous)"; + if (allMergedNames.Contains(recoveredName)) + recoveredName = $"{existing.DisplayName} ({existing.SessionId[..Math.Min(8, existing.SessionId.Length)]})"; + existing.DisplayName = recoveredName; + } + merged.Add(existing); activeIds.Add(existing.SessionId); + allMergedNames.Add(existing.DisplayName); } return merged; } + /// + /// Heuristic to detect orchestrated worker/evaluator sessions that shouldn't + /// be auto-recovered (they're meaningless without their parent orchestration). + /// + internal static bool IsLikelyWorkerSession(string? branch, string? summary) + { + if (!string.IsNullOrEmpty(branch)) + { + if (branch.Contains("-worker-", StringComparison.OrdinalIgnoreCase) || + branch.Contains("-orchestrator-", StringComparison.OrdinalIgnoreCase) || + branch.Contains("evaluator", StringComparison.OrdinalIgnoreCase) || + branch.StartsWith("Skill-Validator", StringComparison.OrdinalIgnoreCase)) + return true; + } + if (!string.IsNullOrEmpty(summary)) + { + if (summary.StartsWith("You are the Implementer", StringComparison.OrdinalIgnoreCase) || + summary.StartsWith("You are the Challenger", StringComparison.OrdinalIgnoreCase) || + summary.StartsWith("You are a PR reviewer", StringComparison.OrdinalIgnoreCase)) + return true; + } + return false; + } + /// /// Restore persisted usage stats onto the in-memory session after resume. /// Uses Math.Max for accumulative fields to avoid overwriting values the SDK From a55d0aa2a82e79b6358172de406445bac410774f Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sun, 5 Apr 2026 12:16:41 -0500 Subject: [PATCH 2/4] fix: Treat 'abort' as terminal event in IsSessionStillProcessing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the last event in events.jsonl is 'abort' (user cancelled), the session was incorrectly treated as still processing during restore. This caused the session to appear stuck with a spinner, blocking user interaction. During rapid relaunches (where the abort was <600s old and the staleness check didn't save us), this led users to create new sessions — which then triggered the merge name collision that dropped the original session's history. Adding 'abort' to the terminal events list is the root cause fix. The merge rename (previous commit) is the safety net. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/StuckSessionRecoveryTests.cs | 28 +++++++++++++++++++ .../Services/CopilotService.Utilities.cs | 3 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/PolyPilot.Tests/StuckSessionRecoveryTests.cs b/PolyPilot.Tests/StuckSessionRecoveryTests.cs index c3508d1bc..52fb2d1e0 100644 --- a/PolyPilot.Tests/StuckSessionRecoveryTests.cs +++ b/PolyPilot.Tests/StuckSessionRecoveryTests.cs @@ -117,6 +117,34 @@ public void IsSessionStillProcessing_RecentFile_IdleEvent_ReturnsFalse() } } + [Fact] + public void IsSessionStillProcessing_RecentFile_AbortEvent_ReturnsFalse() + { + var svc = CreateService(); + var tmpDir = Path.Combine(Path.GetTempPath(), "polypilot-test-" + Guid.NewGuid().ToString("N")); + var sessionId = Guid.NewGuid().ToString(); + var sessionDir = Path.Combine(tmpDir, sessionId); + Directory.CreateDirectory(sessionDir); + var eventsFile = Path.Combine(sessionDir, "events.jsonl"); + + try + { + File.WriteAllText(eventsFile, + """{"type":"session.resume","data":{}}""" + "\n" + + """{"type":"user.message","data":{"content":"test"}}""" + "\n" + + """{"type":"assistant.turn_start","data":{}}""" + "\n" + + """{"type":"abort","data":{"reason":"user initiated"}}"""); + + var result = svc.IsSessionStillProcessing(sessionId, tmpDir); + + Assert.False(result, "events.jsonl ending with abort should not report still processing — user explicitly cancelled"); + } + finally + { + Directory.Delete(tmpDir, true); + } + } + [Fact] public void IsSessionStillProcessing_MissingFile_ReturnsFalse() { diff --git a/PolyPilot/Services/CopilotService.Utilities.cs b/PolyPilot/Services/CopilotService.Utilities.cs index 181f8e666..749857377 100644 --- a/PolyPilot/Services/CopilotService.Utilities.cs +++ b/PolyPilot/Services/CopilotService.Utilities.cs @@ -138,7 +138,8 @@ internal bool IsSessionStillProcessing(string sessionId, string basePath) // session.idle is ephemeral (never on disk). session.start can also be the only // on-disk event for a never-used session, so treat it as terminal on restore. - var terminalEvents = new[] { "session.idle", "session.error", "session.shutdown", "session.start" }; + // "abort" means the user explicitly cancelled — the session is definitively not processing. + var terminalEvents = new[] { "session.idle", "session.error", "session.shutdown", "session.start", "abort" }; if (terminalEvents.Contains(type)) return false; // Smart completion for assistant.message / assistant.turn_end: session.idle is not From 2fd68cb9a1e51948ac5b4178ceb6e6dd9f4ac811 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sun, 5 Apr 2026 12:55:47 -0500 Subject: [PATCH 3/4] =?UTF-8?q?fix:=20Address=20R3=20review=20=E2=80=94=20?= =?UTF-8?q?clone=20on=20rename,=20loop=20fallback,=20remove=20dead=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Clone ActiveSessionEntry before renaming to avoid mutating caller's persisted list (MergeSessionEntries is a static method) - Use while loop for fallback name generation to guarantee uniqueness even with colliding sessionId prefixes - Remove IsLikelyWorkerSession and its 5 tests — dead code with no production callers. Will re-add when orphan recovery is implemented. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/SessionPersistenceTests.cs | 38 ------------- .../Services/CopilotService.Persistence.cs | 56 +++++++++---------- 2 files changed, 26 insertions(+), 68 deletions(-) diff --git a/PolyPilot.Tests/SessionPersistenceTests.cs b/PolyPilot.Tests/SessionPersistenceTests.cs index 2d9d86dfe..991942ed8 100644 --- a/PolyPilot.Tests/SessionPersistenceTests.cs +++ b/PolyPilot.Tests/SessionPersistenceTests.cs @@ -1671,42 +1671,4 @@ public void Merge_NameCollision_MissingDirStillExcluded() Assert.Single(result); Assert.Equal("new-id", result[0].SessionId); } - - // --- IsLikelyWorkerSession --- - - [Fact] - public void IsLikelyWorkerSession_WorkerBranch_ReturnsTrue() - { - Assert.True(CopilotService.IsLikelyWorkerSession("Implement---Challenge-worker-1-abc1", null)); - Assert.True(CopilotService.IsLikelyWorkerSession("Implement---Challenge-orchestrator-5b44", null)); - Assert.True(CopilotService.IsLikelyWorkerSession("Skill-Validator-worker-2-9671", null)); - } - - [Fact] - public void IsLikelyWorkerSession_EvaluatorBranch_ReturnsTrue() - { - Assert.True(CopilotService.IsLikelyWorkerSession("evaluator-session-1", null)); - } - - [Fact] - public void IsLikelyWorkerSession_OrchestratorSummary_ReturnsTrue() - { - Assert.True(CopilotService.IsLikelyWorkerSession(null, "You are the Implementer. Your job is to write correct code.")); - Assert.True(CopilotService.IsLikelyWorkerSession(null, "You are the Challenger. Your job is to find bugs.")); - Assert.True(CopilotService.IsLikelyWorkerSession(null, "You are a PR reviewer. When assigned a PR...")); - } - - [Fact] - public void IsLikelyWorkerSession_NormalSession_ReturnsFalse() - { - Assert.False(CopilotService.IsLikelyWorkerSession("fix/model-1m-slug", "fix the PR number display")); - Assert.False(CopilotService.IsLikelyWorkerSession("main", "can you analyze the copilot cli?")); - Assert.False(CopilotService.IsLikelyWorkerSession(null, null)); - } - - [Fact] - public void IsLikelyWorkerSession_SkillValidatorBranch_ReturnsTrue() - { - Assert.True(CopilotService.IsLikelyWorkerSession("Skill-Validator-worker-3-78c9", null)); - } } diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index d105170f7..23bfa1b0a 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -186,46 +186,42 @@ internal static List MergeSessionEntries( // session ID. This happens when a session is replaced (reconnect, lazy-resume // fallback, etc.). Instead of silently dropping the persisted entry (losing its // history), keep it under a unique name so the user can find it. + // NOTE: We clone the entry to avoid mutating the caller's persisted list. + var entryToAdd = existing; if (activeNames.Contains(existing.DisplayName)) { + entryToAdd = new ActiveSessionEntry + { + SessionId = existing.SessionId, + DisplayName = existing.DisplayName, + Model = existing.Model, + ReasoningEffort = existing.ReasoningEffort, + WorkingDirectory = existing.WorkingDirectory, + LastPrompt = existing.LastPrompt, + GroupId = existing.GroupId, + TotalInputTokens = existing.TotalInputTokens, + TotalOutputTokens = existing.TotalOutputTokens, + ContextCurrentTokens = existing.ContextCurrentTokens, + ContextTokenLimit = existing.ContextTokenLimit, + PremiumRequestsUsed = existing.PremiumRequestsUsed, + TotalApiTimeSeconds = existing.TotalApiTimeSeconds, + CreatedAt = existing.CreatedAt, + LastUpdatedAt = existing.LastUpdatedAt, + }; var recoveredName = $"{existing.DisplayName} (previous)"; - if (allMergedNames.Contains(recoveredName)) - recoveredName = $"{existing.DisplayName} ({existing.SessionId[..Math.Min(8, existing.SessionId.Length)]})"; - existing.DisplayName = recoveredName; + while (allMergedNames.Contains(recoveredName)) + recoveredName = $"{existing.DisplayName} ({existing.SessionId[..Math.Min(8, existing.SessionId.Length)]}-{allMergedNames.Count})"; + entryToAdd.DisplayName = recoveredName; } - merged.Add(existing); - activeIds.Add(existing.SessionId); - allMergedNames.Add(existing.DisplayName); + merged.Add(entryToAdd); + activeIds.Add(entryToAdd.SessionId); + allMergedNames.Add(entryToAdd.DisplayName); } return merged; } - /// - /// Heuristic to detect orchestrated worker/evaluator sessions that shouldn't - /// be auto-recovered (they're meaningless without their parent orchestration). - /// - internal static bool IsLikelyWorkerSession(string? branch, string? summary) - { - if (!string.IsNullOrEmpty(branch)) - { - if (branch.Contains("-worker-", StringComparison.OrdinalIgnoreCase) || - branch.Contains("-orchestrator-", StringComparison.OrdinalIgnoreCase) || - branch.Contains("evaluator", StringComparison.OrdinalIgnoreCase) || - branch.StartsWith("Skill-Validator", StringComparison.OrdinalIgnoreCase)) - return true; - } - if (!string.IsNullOrEmpty(summary)) - { - if (summary.StartsWith("You are the Implementer", StringComparison.OrdinalIgnoreCase) || - summary.StartsWith("You are the Challenger", StringComparison.OrdinalIgnoreCase) || - summary.StartsWith("You are a PR reviewer", StringComparison.OrdinalIgnoreCase)) - return true; - } - return false; - } - /// /// Restore persisted usage stats onto the in-memory session after resume. /// Uses Math.Max for accumulative fields to avoid overwriting values the SDK From cb707c46123454d04b223d22d6d320d5b3d29827 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sun, 5 Apr 2026 13:02:02 -0500 Subject: [PATCH 4/4] fix: Use local counter in rename loop to prevent infinite loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The while loop used allMergedNames.Count as a suffix, which is invariant inside the loop — if the generated name already existed, the loop would spin forever. Use an incrementing local counter instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot/Services/CopilotService.Persistence.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index 23bfa1b0a..99cd8e078 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -209,8 +209,9 @@ internal static List MergeSessionEntries( LastUpdatedAt = existing.LastUpdatedAt, }; var recoveredName = $"{existing.DisplayName} (previous)"; + var suffix = 2; while (allMergedNames.Contains(recoveredName)) - recoveredName = $"{existing.DisplayName} ({existing.SessionId[..Math.Min(8, existing.SessionId.Length)]}-{allMergedNames.Count})"; + recoveredName = $"{existing.DisplayName} (previous {suffix++})"; entryToAdd.DisplayName = recoveredName; }