From 7317b63c1ff89435f486c70435afacdf5e462a2a Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 12 Mar 2026 15:26:13 -0500 Subject: [PATCH 1/6] Fix: Restore fallback preserves conversation history When ResumeSessionAsync fails with 'Session not found' / 'corrupt' / 'session file' errors, the fallback path was calling CreateSessionAsync without recovering history from the old session's events.jsonl. This caused sessions to appear empty after app relaunch. The fix adds history recovery to the fallback path: 1. Load history from old session via LoadHistoryFromDisk(entry.SessionId) 2. Inject recovered messages into the new session's Info.History 3. Set MessageCount and LastReadMessageCount 4. Call RestoreUsageStats(entry) to preserve CreatedAt, token counts 5. Sync recovered history to chat DB under the new session ID 6. Add a system message indicating the session was recreated Bug introduced in PR #225 (commit 19219f1), worsened by PR #308 (commit 72886a2) which expanded the catch conditions without adding history recovery. Adds 5 structural regression tests guarding the fallback path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/SessionPersistenceTests.cs | 95 +++++++++++++++++++ .../Services/CopilotService.Persistence.cs | 27 +++++- 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/PolyPilot.Tests/SessionPersistenceTests.cs b/PolyPilot.Tests/SessionPersistenceTests.cs index a292e92a98..008f8685b7 100644 --- a/PolyPilot.Tests/SessionPersistenceTests.cs +++ b/PolyPilot.Tests/SessionPersistenceTests.cs @@ -449,4 +449,99 @@ public void Merge_DeletedMultiAgentSessions_InClosedIds_Excluded() Assert.Single(result); Assert.Equal("regular-session", result[0].SessionId); } + + // --- Restore fallback: structural regression guards --- + // These verify the fallback path in RestorePreviousSessionsAsync preserves + // history and usage stats when creating a fresh session (PR #225 regression). + + [Fact] + public void RestoreFallback_LoadsHistoryFromOldSession() + { + // STRUCTURAL REGRESSION GUARD: The "Session not found" fallback in + // RestorePreviousSessionsAsync must call LoadHistoryFromDisk(entry.SessionId) + // before CreateSessionAsync so conversation history is recovered. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); + Assert.True(fallbackIdx > 0, "Could not find fallback path in RestorePreviousSessionsAsync"); + + // LoadHistoryFromDisk must appear BEFORE CreateSessionAsync in the fallback block + var beforeFallback = source.Substring( + Math.Max(0, fallbackIdx - 500), + Math.Min(500, fallbackIdx)); + Assert.Contains("LoadHistoryFromDisk", beforeFallback); + } + + [Fact] + public void RestoreFallback_InjectsHistoryIntoRecreatedSession() + { + // STRUCTURAL REGRESSION GUARD: After CreateSessionAsync, the fallback must + // inject the recovered history into the new session's Info.History. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); + Assert.True(fallbackIdx > 0); + + var afterFallback = source.Substring(fallbackIdx, Math.Min(1500, source.Length - fallbackIdx)); + Assert.Contains("History.Add", afterFallback); + Assert.Contains("MessageCount", afterFallback); + Assert.Contains("LastReadMessageCount", afterFallback); + } + + [Fact] + public void RestoreFallback_RestoresUsageStats() + { + // STRUCTURAL REGRESSION GUARD: The fallback must call RestoreUsageStats(entry) + // to preserve token counts, CreatedAt, and other stats from the old session. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); + Assert.True(fallbackIdx > 0); + + var afterFallback = source.Substring(fallbackIdx, Math.Min(2500, source.Length - fallbackIdx)); + Assert.Contains("RestoreUsageStats", afterFallback); + } + + [Fact] + public void RestoreFallback_SyncsHistoryToDatabase() + { + // STRUCTURAL REGRESSION GUARD: The fallback must sync recovered history + // to the chat database under the new session ID so it persists. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); + Assert.True(fallbackIdx > 0); + + var afterFallback = source.Substring(fallbackIdx, Math.Min(1500, source.Length - fallbackIdx)); + Assert.Contains("BulkInsertAsync", afterFallback); + } + + [Fact] + public void RestoreFallback_AddsReconnectionIndicator() + { + // STRUCTURAL REGRESSION GUARD: The fallback must add a system message + // indicating the session was recreated with recovered history, so the + // user knows the session state was reconstructed. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); + Assert.True(fallbackIdx > 0); + + var afterFallback = source.Substring(fallbackIdx, Math.Min(1500, source.Length - fallbackIdx)); + Assert.Contains("Session recreated", afterFallback); + Assert.Contains("SystemMessage", afterFallback); + } + + private static string GetRepoRoot() + { + var dir = AppContext.BaseDirectory; + while (dir != null && !File.Exists(Path.Combine(dir, "PolyPilot.slnx"))) + dir = Path.GetDirectoryName(dir); + return dir ?? throw new InvalidOperationException("Could not find repo root"); + } } diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index efb9917536..d15c3b33e4 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -395,15 +395,38 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok // "corrupted" / "session file" errors mean the events.jsonl is locked or // unreadable (e.g., another copilot process owns the session). // Fall back to creating a fresh session so multi-agent workers don't vanish. + // Load history from the old session's events.jsonl so messages aren't lost. if (ex.Message.Contains("Session not found", StringComparison.OrdinalIgnoreCase) || ex.Message.Contains("corrupt", StringComparison.OrdinalIgnoreCase) || ex.Message.Contains("session file", StringComparison.OrdinalIgnoreCase)) { try { - Debug($"Falling back to CreateSessionAsync for '{entry.DisplayName}'"); + // Recover history from the old session before creating a new one + var oldHistory = LoadHistoryFromDisk(entry.SessionId); + Debug($"Falling back to CreateSessionAsync for '{entry.DisplayName}' (recovered {oldHistory.Count} messages from old session)"); + await CreateSessionAsync(entry.DisplayName, entry.Model, entry.WorkingDirectory, cancellationToken, entry.GroupId); - Debug($"Recreated session: {entry.DisplayName}"); + + // Inject recovered history into the newly created session + if (_sessions.TryGetValue(entry.DisplayName, out var recreatedState) && oldHistory.Count > 0) + { + foreach (var msg in oldHistory) + recreatedState.Info.History.Add(msg); + recreatedState.Info.MessageCount = recreatedState.Info.History.Count; + recreatedState.Info.LastReadMessageCount = recreatedState.Info.History.Count; + + // Sync recovered history to DB under the new session ID + if (recreatedState.Info.SessionId != null) + SafeFireAndForget(_chatDb.BulkInsertAsync(recreatedState.Info.SessionId, oldHistory)); + + recreatedState.Info.History.Add(ChatMessage.SystemMessage("🔄 Session recreated — conversation history recovered from previous session.")); + } + + // Restore usage stats (token counts, CreatedAt, etc.) + RestoreUsageStats(entry); + + Debug($"Recreated session with {oldHistory.Count} recovered messages: {entry.DisplayName}"); continue; } catch (Exception createEx) From 452b85c16079a04bb7e8f5b751cb6f50a9f957fe Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 12 Mar 2026 15:54:14 -0500 Subject: [PATCH 2/6] Fix session ID staleness after RECONNECT, merge ghost dedup, events.jsonl checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three additional bugs causing session history loss on restore: 1. RECONNECT handler didn't call SaveActiveSessionsToDisk() after updating SessionId, so the debounced save wrote stale IDs. On next restart, the stale ID's directory exists but has no events.jsonl. 2. MergeSessionEntries only deduped by SessionId (case-insensitive). After RECONNECT changes the ID, old persisted entries with the stale ID pass the check (stale != new), accumulating N+1 ghost entries per restart cycle. 3. Directory existence checks (Directory.Exists) were too lenient — the SDK creates empty session directories during ResumeSessionAsync. Changed to require events.jsonl file presence in both WriteActiveSessionsFile and RestorePreviousSessionsAsync. Tests: 9 new regression tests (display name dedup, reconnect save guard, events.jsonl structural checks). Entry helper default changed from shared name to id-based to avoid false dedup in existing tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/SessionPersistenceTests.cs | 118 +++++++++++++++++- .../Services/CopilotService.Persistence.cs | 23 +++- PolyPilot/Services/CopilotService.cs | 5 + 3 files changed, 140 insertions(+), 6 deletions(-) diff --git a/PolyPilot.Tests/SessionPersistenceTests.cs b/PolyPilot.Tests/SessionPersistenceTests.cs index 008f8685b7..c0541aa890 100644 --- a/PolyPilot.Tests/SessionPersistenceTests.cs +++ b/PolyPilot.Tests/SessionPersistenceTests.cs @@ -9,8 +9,8 @@ namespace PolyPilot.Tests; /// public class SessionPersistenceTests { - private static ActiveSessionEntry Entry(string id, string name = "s") => - new() { SessionId = id, DisplayName = name, Model = "m", WorkingDirectory = "/w" }; + private static ActiveSessionEntry Entry(string id, string? name = null) => + new() { SessionId = id, DisplayName = name ?? id, Model = "m", WorkingDirectory = "/w" }; // --- MergeSessionEntries: basic behavior --- @@ -202,6 +202,61 @@ public void Merge_SomeDirsExist_OnlyThoseKept() Assert.DoesNotContain(result, e => e.SessionId == "gone"); } + // --- MergeSessionEntries: display name deduplication --- + + [Fact] + public void Merge_DuplicateDisplayName_ActiveWins_PersistedDropped() + { + // 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. + 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("new-id", result[0].SessionId); + } + + [Fact] + public void Merge_MultipleGhostEntriesSameDisplayName_OnlyOneKept() + { + // Simulates the "28 MEssagePierce entries" bug: multiple persisted entries + // with different session IDs but the same display name. Only one should survive. + var active = new List(); + var persisted = new List + { + Entry("ghost-1", "MEssagePierce"), + Entry("ghost-2", "MEssagePierce"), + Entry("ghost-3", "MEssagePierce"), + Entry("real-1", "OtherSession"), + }; + var closed = new HashSet(); + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), _ => true); + + Assert.Equal(2, result.Count); + // First MEssagePierce entry wins, others are deduped + Assert.Single(result, e => e.DisplayName == "MEssagePierce"); + Assert.Equal("ghost-1", result.First(e => e.DisplayName == "MEssagePierce").SessionId); + Assert.Single(result, e => e.DisplayName == "OtherSession"); + } + + [Fact] + public void Merge_ActiveAndPersistedDifferentNames_BothKept() + { + // Entries with different display names should both be kept. + var active = new List { Entry("id-1", "Alpha") }; + var persisted = new List { Entry("id-2", "Beta") }; + var closed = new HashSet(); + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), _ => true); + + Assert.Equal(2, result.Count); + } + // --- MergeSessionEntries: mode switch simulation --- [Fact] @@ -544,4 +599,63 @@ private static string GetRepoRoot() dir = Path.GetDirectoryName(dir); return dir ?? throw new InvalidOperationException("Could not find repo root"); } + + // --- RECONNECT handler: structural regression guards --- + // These verify the RECONNECT path in SendPromptAsync persists the new session ID. + // Without this, the debounced save captures a stale pre-reconnect session ID, + // causing the next restore to find an empty directory with no events.jsonl. + + [Fact] + public void Reconnect_CallsSaveActiveSessionsToDisk_AfterUpdatingSessionId() + { + // STRUCTURAL REGRESSION GUARD: After RECONNECT updates state.Info.SessionId + // and _sessions[sessionName] = newState, SaveActiveSessionsToDisk() must be + // called so the new session ID is persisted immediately. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.cs")); + + // Find the specific assignment where the new state replaces the old one + var sessionsAssign = source.IndexOf("_sessions[sessionName] = newState", StringComparison.Ordinal); + Assert.True(sessionsAssign > 0, "Could not find _sessions assignment in RECONNECT handler"); + + // SaveActiveSessionsToDisk must appear within the next 500 chars (before StartProcessingWatchdog) + var afterAssign = source.Substring(sessionsAssign, Math.Min(500, source.Length - sessionsAssign)); + Assert.Contains("SaveActiveSessionsToDisk()", afterAssign); + } + + // --- Restore: events.jsonl existence check --- + + [Fact] + public void Restore_SkipsSessionsWithoutEventsJsonl() + { + // STRUCTURAL REGRESSION GUARD: The restore loop must check for events.jsonl + // existence, not just directory existence. Empty directories (created by SDK + // during ResumeSessionAsync) should be skipped to prevent ghost sessions. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var restoreIdx = source.IndexOf("RestorePreviousSessionsAsync", StringComparison.Ordinal); + Assert.True(restoreIdx > 0); + + var restoreBlock = source.Substring(restoreIdx, Math.Min(5000, source.Length - restoreIdx)); + // Must check events.jsonl, not just Directory.Exists + Assert.Contains("events.jsonl", restoreBlock); + } + + [Fact] + public void SaveActiveSessionsToDisk_ChecksEventsJsonlNotJustDirectory() + { + // STRUCTURAL REGRESSION GUARD: The sessionDirExists callback in + // WriteActiveSessionsFile/SaveActiveSessionsToDisk must check for + // events.jsonl file existence, not just the directory. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var mergeCallIdx = source.IndexOf("MergeSessionEntries(entries", StringComparison.Ordinal); + Assert.True(mergeCallIdx > 0); + + // The callback passed to MergeSessionEntries must reference events.jsonl + var mergeCall = source.Substring(mergeCallIdx, Math.Min(500, source.Length - mergeCallIdx)); + Assert.Contains("events.jsonl", mergeCall); + } } diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index d15c3b33e4..1b01cc84f7 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -113,7 +113,14 @@ private void WriteActiveSessionsFile(List entries) var closedIds = new HashSet(_closedSessionIds.Keys, StringComparer.OrdinalIgnoreCase); var closedNames = new HashSet(_closedSessionNames.Keys, StringComparer.OrdinalIgnoreCase); entries = MergeSessionEntries(entries, existingEntries, closedIds, closedNames, - sessionId => Directory.Exists(Path.Combine(SessionStatePath, sessionId))); + sessionId => + { + var dir = Path.Combine(SessionStatePath, sessionId); + // Require events.jsonl, not just the directory. + // The SDK creates empty directories during ResumeSessionAsync + // even when the session is recreated with a new ID. + return File.Exists(Path.Combine(dir, "events.jsonl")); + }); } } } @@ -148,16 +155,22 @@ internal static List MergeSessionEntries( { var merged = new List(active); var activeIds = new HashSet(active.Select(e => e.SessionId), StringComparer.OrdinalIgnoreCase); + // Track display names already in the merged list to prevent ghost duplicates. + // Without this, a session that reconnected (new ID) accumulates old entries + // because the old IDs pass the session-ID check but share the same display name. + var activeNames = new HashSet(active.Select(e => e.DisplayName), 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; merged.Add(existing); activeIds.Add(existing.SessionId); + activeNames.Add(existing.DisplayName); } return merged; @@ -336,11 +349,13 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok continue; } - // Check the session still exists on disk + // Check the session still exists on disk with actual event data. + // Empty directories (created by SDK during ResumeSessionAsync for + // sessions that were later reconnected with a new ID) are skipped. var sessionDir = Path.Combine(SessionStatePath, entry.SessionId); - if (!Directory.Exists(sessionDir)) + if (!File.Exists(Path.Combine(sessionDir, "events.jsonl"))) { - Debug($"Skipping '{entry.DisplayName}' — session dir not found: {sessionDir}"); + Debug($"Skipping '{entry.DisplayName}' — no events.jsonl in: {sessionDir}"); continue; } diff --git a/PolyPilot/Services/CopilotService.cs b/PolyPilot/Services/CopilotService.cs index d72da3fd34..613ad59298 100644 --- a/PolyPilot/Services/CopilotService.cs +++ b/PolyPilot/Services/CopilotService.cs @@ -2719,6 +2719,11 @@ public async Task SendPromptAsync(string sessionName, string prompt, Lis // 120s watchdog tier instead of the inflated 600s from stale tool state. state.HasUsedToolsThisTurn = false; + // Persist the new session ID immediately so it survives app restart. + // Without this, the debounced save captures the pre-reconnect snapshot + // and the stale session ID is written to active-sessions.json. + SaveActiveSessionsToDisk(); + // Start fresh watchdog for the new connection StartProcessingWatchdog(state, sessionName); From c9763fe37943b2f995757dfc87e2080e18d07933 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 12 Mar 2026 16:13:51 -0500 Subject: [PATCH 3/6] Address review: dedup, events check, history durability, tool normalize - MergeSessionEntries: only dedup persisted entries against active names, not against each other (fixes legitimate same-name session loss) - WriteActiveSessionsFile: accept sessions with events.jsonl OR recently created dirs (< 5 min), drop stale ghost directories - RestorePreviousSessionsAsync: remove early events.jsonl skip so never-used sessions reach the existing fallback path - Fallback path: copy old events.jsonl to new session dir for durability across future restarts - Fallback path: normalize stale incomplete ToolCall/Reasoning entries (matching ResumeSessionAsync behavior) - Add 5 behavioral filesystem tests for sessionDirExists callback logic Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/SessionPersistenceTests.cs | 265 ++++++++++++++++-- .../Services/CopilotService.Persistence.cs | 58 ++-- 2 files changed, 281 insertions(+), 42 deletions(-) diff --git a/PolyPilot.Tests/SessionPersistenceTests.cs b/PolyPilot.Tests/SessionPersistenceTests.cs index c0541aa890..44b0ad50f4 100644 --- a/PolyPilot.Tests/SessionPersistenceTests.cs +++ b/PolyPilot.Tests/SessionPersistenceTests.cs @@ -221,10 +221,11 @@ public void Merge_DuplicateDisplayName_ActiveWins_PersistedDropped() } [Fact] - public void Merge_MultipleGhostEntriesSameDisplayName_OnlyOneKept() + public void Merge_PersistedEntriesWithSameDisplayName_AllKeptWhenNoActiveNameConflict() { - // Simulates the "28 MEssagePierce entries" bug: multiple persisted entries - // with different session IDs but the same display name. Only one should survive. + // Persisted entries should only be deduped against active session names. + // Legitimate persisted sessions that share a display name are restored and + // collision renaming is handled later in the restore flow. var active = new List(); var persisted = new List { @@ -237,10 +238,11 @@ public void Merge_MultipleGhostEntriesSameDisplayName_OnlyOneKept() var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), _ => true); - Assert.Equal(2, result.Count); - // First MEssagePierce entry wins, others are deduped - Assert.Single(result, e => e.DisplayName == "MEssagePierce"); - Assert.Equal("ghost-1", result.First(e => e.DisplayName == "MEssagePierce").SessionId); + Assert.Equal(4, result.Count); + Assert.Equal(3, result.Count(e => e.DisplayName == "MEssagePierce")); + Assert.Contains(result, e => e.SessionId == "ghost-1"); + Assert.Contains(result, e => e.SessionId == "ghost-2"); + Assert.Contains(result, e => e.SessionId == "ghost-3"); Assert.Single(result, e => e.DisplayName == "OtherSession"); } @@ -539,7 +541,7 @@ public void RestoreFallback_InjectsHistoryIntoRecreatedSession() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(1500, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); Assert.Contains("History.Add", afterFallback); Assert.Contains("MessageCount", afterFallback); Assert.Contains("LastReadMessageCount", afterFallback); @@ -556,7 +558,7 @@ public void RestoreFallback_RestoresUsageStats() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(2500, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); Assert.Contains("RestoreUsageStats", afterFallback); } @@ -571,10 +573,45 @@ public void RestoreFallback_SyncsHistoryToDatabase() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(1500, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); Assert.Contains("BulkInsertAsync", afterFallback); } + [Fact] + public void RestoreFallback_CopiesEventsJsonlToNewSessionDirectory() + { + // STRUCTURAL REGRESSION GUARD: The fallback must copy the old + // events.jsonl into the recreated session directory so a later restart + // can reload history from the new session ID. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); + Assert.True(fallbackIdx > 0); + + var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); + Assert.Contains("events.jsonl", afterFallback); + Assert.Contains("File.Copy(oldEvents, newEvents)", afterFallback); + Assert.Contains("Copied events.jsonl", afterFallback); + } + + [Fact] + public void RestoreFallback_NormalizesIncompleteToolAndReasoningEntries() + { + // STRUCTURAL REGRESSION GUARD: The fallback must mark stale incomplete + // tool-call and reasoning entries complete, matching ResumeSessionAsync. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); + Assert.True(fallbackIdx > 0); + + var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); + Assert.Contains("ChatMessageType.ToolCall", afterFallback); + Assert.Contains("ChatMessageType.Reasoning", afterFallback); + Assert.Contains("msg.IsComplete = true", afterFallback); + } + [Fact] public void RestoreFallback_AddsReconnectionIndicator() { @@ -587,7 +624,7 @@ public void RestoreFallback_AddsReconnectionIndicator() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(1500, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); Assert.Contains("Session recreated", afterFallback); Assert.Contains("SystemMessage", afterFallback); } @@ -623,39 +660,219 @@ public void Reconnect_CallsSaveActiveSessionsToDisk_AfterUpdatingSessionId() Assert.Contains("SaveActiveSessionsToDisk()", afterAssign); } - // --- Restore: events.jsonl existence check --- + // --- Restore/save: events.jsonl handling --- [Fact] - public void Restore_SkipsSessionsWithoutEventsJsonl() + public void Restore_DoesNotSkipSessionsBeforeFallbackCanHandleMissingEvents() { - // STRUCTURAL REGRESSION GUARD: The restore loop must check for events.jsonl - // existence, not just directory existence. Empty directories (created by SDK - // during ResumeSessionAsync) should be skipped to prevent ghost sessions. + // STRUCTURAL REGRESSION GUARD: RestorePreviousSessionsAsync must not + // short-circuit on missing events.jsonl before the existing fallback path + // can recreate legitimate never-used sessions. var source = File.ReadAllText( Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); var restoreIdx = source.IndexOf("RestorePreviousSessionsAsync", StringComparison.Ordinal); Assert.True(restoreIdx > 0); - var restoreBlock = source.Substring(restoreIdx, Math.Min(5000, source.Length - restoreIdx)); - // Must check events.jsonl, not just Directory.Exists - Assert.Contains("events.jsonl", restoreBlock); + Assert.DoesNotContain("Skipping '{entry.DisplayName}' — no events.jsonl", source); + Assert.Contains("Session not found", source); } [Fact] - public void SaveActiveSessionsToDisk_ChecksEventsJsonlNotJustDirectory() + public void SaveActiveSessionsToDisk_AcceptsEventsOrRecentDirectories() { // STRUCTURAL REGRESSION GUARD: The sessionDirExists callback in - // WriteActiveSessionsFile/SaveActiveSessionsToDisk must check for - // events.jsonl file existence, not just the directory. + // WriteActiveSessionsFile/SaveActiveSessionsToDisk must keep used sessions + // via events.jsonl and also preserve newly created directories briefly. var source = File.ReadAllText( Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); var mergeCallIdx = source.IndexOf("MergeSessionEntries(entries", StringComparison.Ordinal); Assert.True(mergeCallIdx > 0); - // The callback passed to MergeSessionEntries must reference events.jsonl - var mergeCall = source.Substring(mergeCallIdx, Math.Min(500, source.Length - mergeCallIdx)); + var mergeCall = source.Substring(mergeCallIdx, Math.Min(1000, source.Length - mergeCallIdx)); + Assert.Contains("Directory.Exists(dir)", mergeCall); Assert.Contains("events.jsonl", mergeCall); + Assert.Contains("Directory.GetCreationTimeUtc", mergeCall); + Assert.Contains("TotalMinutes < 5", mergeCall); + } + + // --- Behavioral tests: sessionDirExists with real filesystem --- + // These verify the actual directory/events.jsonl logic that the WriteActiveSessionsFile + // callback implements, using real temp directories instead of source-text assertions. + + [Fact] + public void Merge_WithEventsJsonl_SessionKept() + { + // A persisted session whose directory has events.jsonl should survive merge. + var tempBase = Path.Combine(Path.GetTempPath(), $"polypilot-test-{Guid.NewGuid():N}"); + try + { + var sessionDir = Path.Combine(tempBase, "sess-good"); + Directory.CreateDirectory(sessionDir); + File.WriteAllText(Path.Combine(sessionDir, "events.jsonl"), "{}"); + + var active = new List(); + var persisted = new List { Entry("sess-good", "Good") }; + var closed = new HashSet(); + + Func dirExists = id => + { + var dir = Path.Combine(tempBase, id); + if (!Directory.Exists(dir)) return false; + if (File.Exists(Path.Combine(dir, "events.jsonl"))) return true; + try { return (DateTime.UtcNow - Directory.GetCreationTimeUtc(dir)).TotalMinutes < 5; } + catch { return false; } + }; + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), dirExists); + Assert.Single(result); + Assert.Equal("sess-good", result[0].SessionId); + } + finally { try { Directory.Delete(tempBase, true); } catch { } } + } + + [Fact] + public void Merge_WithEmptyDir_RecentlyCreated_SessionKept() + { + // A session directory without events.jsonl but created recently (< 5 min) + // should be kept — it's a brand-new session that hasn't received events yet. + var tempBase = Path.Combine(Path.GetTempPath(), $"polypilot-test-{Guid.NewGuid():N}"); + try + { + var sessionDir = Path.Combine(tempBase, "sess-new"); + Directory.CreateDirectory(sessionDir); + // No events.jsonl — simulates a just-created session + + var active = new List(); + var persisted = new List { Entry("sess-new", "New") }; + var closed = new HashSet(); + + Func dirExists = id => + { + var dir = Path.Combine(tempBase, id); + if (!Directory.Exists(dir)) return false; + if (File.Exists(Path.Combine(dir, "events.jsonl"))) return true; + try { return (DateTime.UtcNow - Directory.GetCreationTimeUtc(dir)).TotalMinutes < 5; } + catch { return false; } + }; + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), dirExists); + Assert.Single(result); + Assert.Equal("sess-new", result[0].SessionId); + } + finally { try { Directory.Delete(tempBase, true); } catch { } } + } + + [Fact] + public void Merge_WithEmptyDir_NoEvents_GhostDropped() + { + // A session directory without events.jsonl that is NOT recently created + // should be dropped — it's a ghost from a reconnected session. + var tempBase = Path.Combine(Path.GetTempPath(), $"polypilot-test-{Guid.NewGuid():N}"); + try + { + var sessionDir = Path.Combine(tempBase, "sess-ghost"); + Directory.CreateDirectory(sessionDir); + // Backdate the directory creation time to simulate a stale ghost + Directory.SetCreationTimeUtc(sessionDir, DateTime.UtcNow.AddHours(-1)); + + var active = new List(); + var persisted = new List { Entry("sess-ghost", "Ghost") }; + var closed = new HashSet(); + + Func dirExists = id => + { + var dir = Path.Combine(tempBase, id); + if (!Directory.Exists(dir)) return false; + if (File.Exists(Path.Combine(dir, "events.jsonl"))) return true; + try { return (DateTime.UtcNow - Directory.GetCreationTimeUtc(dir)).TotalMinutes < 5; } + catch { return false; } + }; + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), dirExists); + Assert.Empty(result); + } + finally { try { Directory.Delete(tempBase, true); } catch { } } + } + + [Fact] + public void Merge_NoDirectory_SessionDropped() + { + // A persisted session with no directory at all should be dropped. + var tempBase = Path.Combine(Path.GetTempPath(), $"polypilot-test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempBase); + try + { + var active = new List(); + var persisted = new List { Entry("nonexistent", "Gone") }; + var closed = new HashSet(); + + Func dirExists = id => + { + var dir = Path.Combine(tempBase, id); + if (!Directory.Exists(dir)) return false; + if (File.Exists(Path.Combine(dir, "events.jsonl"))) return true; + try { return (DateTime.UtcNow - Directory.GetCreationTimeUtc(dir)).TotalMinutes < 5; } + catch { return false; } + }; + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), dirExists); + Assert.Empty(result); + } + finally { try { Directory.Delete(tempBase, true); } catch { } } + } + + [Fact] + public void Merge_MixedSessions_OnlyValidOnesKept() + { + // End-to-end scenario: mix of valid, ghost, and missing sessions. + // Only sessions with events.jsonl or recently-created dirs should survive. + var tempBase = Path.Combine(Path.GetTempPath(), $"polypilot-test-{Guid.NewGuid():N}"); + try + { + // Session with events.jsonl — kept + var goodDir = Path.Combine(tempBase, "good"); + Directory.CreateDirectory(goodDir); + File.WriteAllText(Path.Combine(goodDir, "events.jsonl"), "{}"); + + // New session (no events, recent dir) — kept + var newDir = Path.Combine(tempBase, "brand-new"); + Directory.CreateDirectory(newDir); + + // Ghost session (empty dir, old) — dropped + var ghostDir = Path.Combine(tempBase, "ghost"); + Directory.CreateDirectory(ghostDir); + Directory.SetCreationTimeUtc(ghostDir, DateTime.UtcNow.AddHours(-2)); + + // "missing" — no directory at all — dropped + + var active = new List(); + var persisted = new List + { + Entry("good", "Good"), + Entry("brand-new", "BrandNew"), + Entry("ghost", "Ghost"), + Entry("missing", "Missing"), + }; + var closed = new HashSet(); + + Func dirExists = id => + { + var dir = Path.Combine(tempBase, id); + if (!Directory.Exists(dir)) return false; + if (File.Exists(Path.Combine(dir, "events.jsonl"))) return true; + try { return (DateTime.UtcNow - Directory.GetCreationTimeUtc(dir)).TotalMinutes < 5; } + catch { return false; } + }; + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), dirExists); + Assert.Equal(2, result.Count); + Assert.Contains(result, e => e.SessionId == "good"); + Assert.Contains(result, e => e.SessionId == "brand-new"); + Assert.DoesNotContain(result, e => e.SessionId == "ghost"); + Assert.DoesNotContain(result, e => e.SessionId == "missing"); + } + finally { try { Directory.Delete(tempBase, true); } catch { } } } } diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index 1b01cc84f7..f569012206 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -116,10 +116,14 @@ private void WriteActiveSessionsFile(List entries) sessionId => { var dir = Path.Combine(SessionStatePath, sessionId); - // Require events.jsonl, not just the directory. - // The SDK creates empty directories during ResumeSessionAsync - // even when the session is recreated with a new ID. - return File.Exists(Path.Combine(dir, "events.jsonl")); + if (!Directory.Exists(dir)) return false; + // Accept if events.jsonl exists (session has been used) + if (File.Exists(Path.Combine(dir, "events.jsonl"))) return true; + // Also accept recently-created directories (new sessions + // that haven't received their first event yet). Ghost + // directories from old reconnects will be stale. + try { return (DateTime.UtcNow - Directory.GetCreationTimeUtc(dir)).TotalMinutes < 5; } + catch { return false; } }); } } @@ -155,9 +159,9 @@ internal static List MergeSessionEntries( { var merged = new List(active); var activeIds = new HashSet(active.Select(e => e.SessionId), StringComparer.OrdinalIgnoreCase); - // Track display names already in the merged list to prevent ghost duplicates. - // Without this, a session that reconnected (new ID) accumulates old entries - // because the old IDs pass the session-ID check but share the same display name. + // Track active display names only. + // 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), StringComparer.OrdinalIgnoreCase); foreach (var existing in persisted) @@ -170,7 +174,6 @@ internal static List MergeSessionEntries( merged.Add(existing); activeIds.Add(existing.SessionId); - activeNames.Add(existing.DisplayName); } return merged; @@ -349,16 +352,6 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok continue; } - // Check the session still exists on disk with actual event data. - // Empty directories (created by SDK during ResumeSessionAsync for - // sessions that were later reconnected with a new ID) are skipped. - var sessionDir = Path.Combine(SessionStatePath, entry.SessionId); - if (!File.Exists(Path.Combine(sessionDir, "events.jsonl"))) - { - Debug($"Skipping '{entry.DisplayName}' — no events.jsonl in: {sessionDir}"); - continue; - } - // Codespace sessions: create placeholder state (client not yet connected). // Health check will resume them after the codespace tunnel is established. // When toggle is off, skip entirely — don't create null-session placeholders. @@ -426,11 +419,40 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok // Inject recovered history into the newly created session if (_sessions.TryGetValue(entry.DisplayName, out var recreatedState) && oldHistory.Count > 0) { + // Copy the old events.jsonl to the new session directory so history + // survives future restarts (LoadHistoryFromDisk reads events.jsonl). + if (recreatedState.Info.SessionId != null && recreatedState.Info.SessionId != entry.SessionId) + { + try + { + var oldEvents = Path.Combine(SessionStatePath, entry.SessionId, "events.jsonl"); + var newEventsDir = Path.Combine(SessionStatePath, recreatedState.Info.SessionId); + var newEvents = Path.Combine(newEventsDir, "events.jsonl"); + if (File.Exists(oldEvents) && !File.Exists(newEvents)) + { + Directory.CreateDirectory(newEventsDir); + File.Copy(oldEvents, newEvents); + Debug($"Copied events.jsonl from {entry.SessionId} to {recreatedState.Info.SessionId}"); + } + } + catch (Exception copyEx) + { + Debug($"Failed to copy events.jsonl: {copyEx.Message}"); + } + } + foreach (var msg in oldHistory) recreatedState.Info.History.Add(msg); recreatedState.Info.MessageCount = recreatedState.Info.History.Count; recreatedState.Info.LastReadMessageCount = recreatedState.Info.History.Count; + // Normalize stale incomplete entries (same as ResumeSessionAsync) + foreach (var msg in recreatedState.Info.History.Where(m => + (m.MessageType == ChatMessageType.ToolCall || m.MessageType == ChatMessageType.Reasoning) && !m.IsComplete)) + { + msg.IsComplete = true; + } + // Sync recovered history to DB under the new session ID if (recreatedState.Info.SessionId != null) SafeFireAndForget(_chatDb.BulkInsertAsync(recreatedState.Info.SessionId, oldHistory)); From 9766e9786c0016a9a9af5ec1fc9aba1b3d7d1498 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 12 Mar 2026 16:26:05 -0500 Subject: [PATCH 4/6] Fix flaky FireAndForget test: use deterministic task assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace GC-based UnobservedTaskException detection with direct task verification. The old approach used TaskScheduler.UnobservedTaskException (a process-global handler) + forced GC, which was susceptible to cross-test pollution — other tests' leaked exceptions could surface during this test's GC cycles. New approach: await the fire-and-forget tasks, assert they completed without faulting (proving internal exception handling works), and verify the error return value (-1). Deterministic, no GC timing dependency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ChatDatabaseResilienceTests.cs | 63 ++++++++----------- 1 file changed, 25 insertions(+), 38 deletions(-) diff --git a/PolyPilot.Tests/ChatDatabaseResilienceTests.cs b/PolyPilot.Tests/ChatDatabaseResilienceTests.cs index 951b54e39a..f8cde82c74 100644 --- a/PolyPilot.Tests/ChatDatabaseResilienceTests.cs +++ b/PolyPilot.Tests/ChatDatabaseResilienceTests.cs @@ -240,44 +240,31 @@ public async Task FireAndForget_AddMessageAsync_DoesNotCauseUnobservedTaskExcept // AddMessageAsync with a broken DB must NOT produce unobserved task exceptions. // Before the fix, AggregateException from SQLite async internals would escape // the narrow catch filter and become unobserved. - var unobservedException = false; - void handler(object? s, UnobservedTaskExceptionEventArgs e) - { - unobservedException = true; - e.SetObserved(); - } - - TaskScheduler.UnobservedTaskException += handler; - try - { - ChatDatabase.SetDbPathForTesting(_impossibleDbPath); - var db = new ChatDatabase(); - db.ResetConnection(); - - var msg = ChatMessage.UserMessage("test"); - - // Fire-and-forget — same pattern as CopilotService.Events.cs - _ = db.AddMessageAsync("session-1", msg); - _ = db.UpdateToolCompleteAsync("session-1", "tool-1", "result", true); - _ = db.UpdateReasoningContentAsync("session-1", "reason-1", "content", true); - - // Give tasks time to complete and finalize — multiple GC cycles - // needed to reliably trigger UnobservedTaskException under load - await Task.Delay(500); - for (int i = 0; i < 3; i++) - { - GC.Collect(2, GCCollectionMode.Forced, blocking: true); - GC.WaitForPendingFinalizers(); - } - await Task.Delay(200); - - Assert.False(unobservedException, - "Fire-and-forget ChatDatabase calls must not produce unobserved task exceptions"); - } - finally - { - TaskScheduler.UnobservedTaskException -= handler; - } + // + // Two-pronged verification: + // 1. Await the tasks — they must complete without throwing (internal catch) + // 2. Verify faulted tasks don't exist (no unobserved exception source) + ChatDatabase.SetDbPathForTesting(_impossibleDbPath); + var db = new ChatDatabase(); + db.ResetConnection(); + + var msg = ChatMessage.UserMessage("test"); + + // Fire-and-forget — same pattern as CopilotService.Events.cs + var t1 = db.AddMessageAsync("session-1", msg); + var t2 = db.UpdateToolCompleteAsync("session-1", "tool-1", "result", true); + var t3 = db.UpdateReasoningContentAsync("session-1", "reason-1", "content", true); + + // All tasks should complete without throwing — internal catch handles errors + await Task.WhenAll(t1, t2, t3); + + // Verify none faulted (faulted tasks are the source of unobserved exceptions) + Assert.False(t1.IsFaulted, "AddMessageAsync should catch internally, not fault"); + Assert.False(t2.IsFaulted, "UpdateToolCompleteAsync should catch internally, not fault"); + Assert.False(t3.IsFaulted, "UpdateReasoningContentAsync should catch internally, not fault"); + + // AddMessageAsync returns -1 on error (not an exception) + Assert.Equal(-1, t1.Result); } // ----------------------------------------------------------------------- From bb3ef9a12ecc2ace9c1951f982d71ce1a179b4fa Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 12 Mar 2026 16:37:55 -0500 Subject: [PATCH 5/6] Sanitize events.jsonl copy + guard null DisplayName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL: Replace File.Copy with sanitized line-by-line copy during fallback recovery. If events.jsonl caused ResumeSessionAsync to fail (corrupt JSON), a raw copy propagates corruption to the new session directory, creating a retry loop on every restart. Now each line is validated via JsonDocument.Parse — only valid JSON lines are written. MODERATE: Guard against null DisplayName in MergeSessionEntries' activeNames HashSet. StringComparer.OrdinalIgnoreCase.GetHashCode(null) throws ArgumentNullException. JSON deserialization can produce null DisplayName even though the default is empty string. Added .Where(n => n != null) filter. Tests: - Merge_NullDisplayNameInActive_DoesNotThrow - Merge_NullDisplayNameInPersisted_DoesNotThrow - WriteActiveSessionsFile_SanitizedCopy_Concept - SanitizedCopy_WritesOnlyValidJsonLines - Updated structural tests: widened search window (5000→7000 chars), updated File.Copy assertion to match sanitized copy comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/SessionPersistenceTests.cs | 130 +++++++++++++++++- .../Services/CopilotService.Persistence.cs | 30 +++- 2 files changed, 151 insertions(+), 9 deletions(-) diff --git a/PolyPilot.Tests/SessionPersistenceTests.cs b/PolyPilot.Tests/SessionPersistenceTests.cs index 44b0ad50f4..fb26048c7d 100644 --- a/PolyPilot.Tests/SessionPersistenceTests.cs +++ b/PolyPilot.Tests/SessionPersistenceTests.cs @@ -558,7 +558,7 @@ public void RestoreFallback_RestoresUsageStats() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(7000, source.Length - fallbackIdx)); Assert.Contains("RestoreUsageStats", afterFallback); } @@ -573,7 +573,7 @@ public void RestoreFallback_SyncsHistoryToDatabase() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(7000, source.Length - fallbackIdx)); Assert.Contains("BulkInsertAsync", afterFallback); } @@ -589,9 +589,9 @@ public void RestoreFallback_CopiesEventsJsonlToNewSessionDirectory() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(7000, source.Length - fallbackIdx)); Assert.Contains("events.jsonl", afterFallback); - Assert.Contains("File.Copy(oldEvents, newEvents)", afterFallback); + Assert.Contains("Sanitized copy: only write lines that parse as valid JSON", afterFallback); Assert.Contains("Copied events.jsonl", afterFallback); } @@ -606,7 +606,7 @@ public void RestoreFallback_NormalizesIncompleteToolAndReasoningEntries() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(7000, source.Length - fallbackIdx)); Assert.Contains("ChatMessageType.ToolCall", afterFallback); Assert.Contains("ChatMessageType.Reasoning", afterFallback); Assert.Contains("msg.IsComplete = true", afterFallback); @@ -624,7 +624,7 @@ public void RestoreFallback_AddsReconnectionIndicator() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); + var afterFallback = source.Substring(fallbackIdx, Math.Min(7000, source.Length - fallbackIdx)); Assert.Contains("Session recreated", afterFallback); Assert.Contains("SystemMessage", afterFallback); } @@ -875,4 +875,122 @@ public void Merge_MixedSessions_OnlyValidOnesKept() } finally { try { Directory.Delete(tempBase, true); } catch { } } } + + // --- Null DisplayName guard --- + + [Fact] + public void Merge_NullDisplayNameInActive_DoesNotThrow() + { + var active = new List + { + new() { SessionId = "a1", DisplayName = null!, Model = "m", WorkingDirectory = "/w" }, + Entry("a2", "Session2"), + }; + var persisted = new List { Entry("p1", "Persisted1") }; + var closed = new HashSet(); + + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), _ => true); + Assert.Equal(3, result.Count); + } + + [Fact] + public void Merge_NullDisplayNameInPersisted_DoesNotThrow() + { + var active = new List { Entry("a1", "Session1") }; + var persisted = new List + { + new() { SessionId = "p1", DisplayName = null!, Model = "m", WorkingDirectory = "/w" }, + }; + var closed = new HashSet(); + + // Null display name should not crash; entry kept if dir exists + var result = CopilotService.MergeSessionEntries(active, persisted, closed, new HashSet(), _ => true); + Assert.Equal(2, result.Count); + } + + // --- Sanitized events.jsonl copy --- + + [Fact] + public void WriteActiveSessionsFile_SanitizedCopy_Concept() + { + // Validates that corrupt JSON lines are detectable via JsonDocument.Parse, + // which is the same mechanism used in the sanitized copy during fallback. + var validLine = "{\"type\":\"user.message\",\"data\":{\"content\":\"hello\"}}"; + var corruptLine = "{\"type\":\"user.message\",\"data\":{\"content\":\"broken"; + var emptyLine = ""; + + var lines = new[] { validLine, corruptLine, emptyLine }; + var validLines = new List(); + int skipped = 0; + foreach (var line in lines) + { + if (string.IsNullOrWhiteSpace(line)) continue; + try + { + using var doc = System.Text.Json.JsonDocument.Parse(line); + validLines.Add(line); + } + catch (System.Text.Json.JsonException) { skipped++; } + } + + Assert.Single(validLines); + Assert.Equal(validLine, validLines[0]); + Assert.Equal(1, skipped); + } + + [Fact] + public void SanitizedCopy_WritesOnlyValidJsonLines() + { + var tempBase = Path.Combine(Path.GetTempPath(), $"polypilot-test-sanitize-{Guid.NewGuid():N}"); + try + { + Directory.CreateDirectory(tempBase); + var sourceDir = Path.Combine(tempBase, "old-session"); + var destDir = Path.Combine(tempBase, "new-session"); + Directory.CreateDirectory(sourceDir); + Directory.CreateDirectory(destDir); + + var sourcePath = Path.Combine(sourceDir, "events.jsonl"); + var destPath = Path.Combine(destDir, "events.jsonl"); + + // Write a mix of valid, corrupt, and empty lines + var lines = new[] + { + "{\"type\":\"session.start\",\"data\":{}}", + "THIS IS CORRUPT", + "{\"type\":\"user.message\",\"data\":{\"content\":\"hello\"}}", + "", + "{\"type\":\"assistant.message\",\"data\":{\"content\":\"world\"}", // missing closing brace + "{\"type\":\"tool.execution_start\",\"data\":{\"toolName\":\"grep\"}}", + }; + File.WriteAllLines(sourcePath, lines); + + // Replicate the sanitized copy logic from CopilotService.Persistence.cs + int validCount = 0, skippedCount = 0; + using (var writer = new StreamWriter(destPath)) + { + foreach (var line in File.ReadLines(sourcePath)) + { + if (string.IsNullOrWhiteSpace(line)) continue; + try + { + using var doc = System.Text.Json.JsonDocument.Parse(line); + writer.WriteLine(line); + validCount++; + } + catch (System.Text.Json.JsonException) { skippedCount++; } + } + } + + Assert.Equal(3, validCount); // session.start, user.message, tool.execution_start + Assert.Equal(2, skippedCount); // "THIS IS CORRUPT" and truncated assistant.message + + var writtenLines = File.ReadAllLines(destPath).Where(l => !string.IsNullOrWhiteSpace(l)).ToArray(); + Assert.Equal(3, writtenLines.Length); + Assert.Contains("session.start", writtenLines[0]); + Assert.Contains("user.message", writtenLines[1]); + Assert.Contains("tool.execution_start", writtenLines[2]); + } + finally { try { Directory.Delete(tempBase, true); } catch { } } + } } diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index f569012206..e1fc2329eb 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -162,7 +162,7 @@ internal static List MergeSessionEntries( // Track active display names only. // 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), StringComparer.OrdinalIgnoreCase); + var activeNames = new HashSet(active.Select(e => e.DisplayName).Where(n => n != null), StringComparer.OrdinalIgnoreCase); foreach (var existing in persisted) { @@ -431,8 +431,32 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok if (File.Exists(oldEvents) && !File.Exists(newEvents)) { Directory.CreateDirectory(newEventsDir); - File.Copy(oldEvents, newEvents); - Debug($"Copied events.jsonl from {entry.SessionId} to {recreatedState.Info.SessionId}"); + // Sanitized copy: only write lines that parse as valid JSON. + // If ResumeSessionAsync failed due to corrupt events.jsonl, + // a raw File.Copy would propagate corruption and cause a + // retry loop on every restart. + int validLines = 0, skippedLines = 0; + using (var writer = new StreamWriter(newEvents)) + { + foreach (var line in File.ReadLines(oldEvents)) + { + if (string.IsNullOrWhiteSpace(line)) continue; + try + { + using var doc = JsonDocument.Parse(line); + writer.WriteLine(line); + validLines++; + } + catch (JsonException) + { + skippedLines++; + } + } + } + if (skippedLines > 0) + Debug($"Sanitized events.jsonl copy from {entry.SessionId} to {recreatedState.Info.SessionId}: {validLines} valid, {skippedLines} corrupt lines skipped"); + else + Debug($"Copied events.jsonl from {entry.SessionId} to {recreatedState.Info.SessionId}: {validLines} lines"); } } catch (Exception copyEx) From 5d194940cc3a9bdd5f8fe88d06c1ef7d89b47a41 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 12 Mar 2026 18:31:47 -0500 Subject: [PATCH 6/6] Fix MessageCount off-by-one and clarify debounce comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move MessageCount/LastReadMessageCount assignment AFTER system message injection so the '🔄 Session recreated' message is included in the count and triggers the unread indicator - Remove duplicate pre-system-message MessageCount assignment - Clarify SaveActiveSessionsToDisk comment: it's debounced (2s), not immediate. The fallback path handles the crash-within-window case. - Add structural regression test verifying MessageCount is set after the system message, not before Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/SessionPersistenceTests.cs | 26 +++++++++++++++++-- .../Services/CopilotService.Persistence.cs | 4 +-- PolyPilot/Services/CopilotService.cs | 4 ++- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/PolyPilot.Tests/SessionPersistenceTests.cs b/PolyPilot.Tests/SessionPersistenceTests.cs index fb26048c7d..1f70047030 100644 --- a/PolyPilot.Tests/SessionPersistenceTests.cs +++ b/PolyPilot.Tests/SessionPersistenceTests.cs @@ -541,8 +541,7 @@ public void RestoreFallback_InjectsHistoryIntoRecreatedSession() var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); Assert.True(fallbackIdx > 0); - var afterFallback = source.Substring(fallbackIdx, Math.Min(5000, source.Length - fallbackIdx)); - Assert.Contains("History.Add", afterFallback); + var afterFallback = source.Substring(fallbackIdx, Math.Min(7000, source.Length - fallbackIdx)); Assert.Contains("History.Add", afterFallback); Assert.Contains("MessageCount", afterFallback); Assert.Contains("LastReadMessageCount", afterFallback); } @@ -629,6 +628,29 @@ public void RestoreFallback_AddsReconnectionIndicator() Assert.Contains("SystemMessage", afterFallback); } + [Fact] + public void RestoreFallback_MessageCount_SetAfterSystemMessage() + { + // STRUCTURAL REGRESSION GUARD: MessageCount and LastReadMessageCount must be + // set AFTER the system message is added, not before. Otherwise the system + // message ("🔄 Session recreated") isn't counted, and the unread indicator + // doesn't trigger for it. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + + var fallbackIdx = source.IndexOf("Falling back to CreateSessionAsync", StringComparison.Ordinal); + Assert.True(fallbackIdx > 0); + + var afterFallback = source.Substring(fallbackIdx, Math.Min(7000, source.Length - fallbackIdx)); + var systemMsgIdx = afterFallback.IndexOf("Session recreated", StringComparison.Ordinal); + var messageCountIdx = afterFallback.IndexOf("MessageCount = recreatedState.Info.History.Count", StringComparison.Ordinal); + + Assert.True(systemMsgIdx > 0, "System message not found in fallback path"); + Assert.True(messageCountIdx > 0, "MessageCount assignment not found in fallback path"); + Assert.True(messageCountIdx > systemMsgIdx, + "MessageCount must be set AFTER the system message is added to History"); + } + private static string GetRepoRoot() { var dir = AppContext.BaseDirectory; diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index e1fc2329eb..e4838690c6 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -467,8 +467,6 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok foreach (var msg in oldHistory) recreatedState.Info.History.Add(msg); - recreatedState.Info.MessageCount = recreatedState.Info.History.Count; - recreatedState.Info.LastReadMessageCount = recreatedState.Info.History.Count; // Normalize stale incomplete entries (same as ResumeSessionAsync) foreach (var msg in recreatedState.Info.History.Where(m => @@ -482,6 +480,8 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok SafeFireAndForget(_chatDb.BulkInsertAsync(recreatedState.Info.SessionId, oldHistory)); recreatedState.Info.History.Add(ChatMessage.SystemMessage("🔄 Session recreated — conversation history recovered from previous session.")); + recreatedState.Info.MessageCount = recreatedState.Info.History.Count; + recreatedState.Info.LastReadMessageCount = recreatedState.Info.History.Count; } // Restore usage stats (token counts, CreatedAt, etc.) diff --git a/PolyPilot/Services/CopilotService.cs b/PolyPilot/Services/CopilotService.cs index 613ad59298..119e975c77 100644 --- a/PolyPilot/Services/CopilotService.cs +++ b/PolyPilot/Services/CopilotService.cs @@ -2719,9 +2719,11 @@ public async Task SendPromptAsync(string sessionName, string prompt, Lis // 120s watchdog tier instead of the inflated 600s from stale tool state. state.HasUsedToolsThisTurn = false; - // Persist the new session ID immediately so it survives app restart. + // Schedule persistence of the new session ID so it survives app restart. // Without this, the debounced save captures the pre-reconnect snapshot // and the stale session ID is written to active-sessions.json. + // Note: This is debounced (2s). If the app crashes within that window, + // the fallback path in RestorePreviousSessionsAsync handles it gracefully. SaveActiveSessionsToDisk(); // Start fresh watchdog for the new connection