Skip to content

Commit 6d3306b

Browse files
PureWeenCopilot
andauthored
fix: prevent phantom (previous) sessions from lazy-resume fallback (#584)
## What When lazy-resume fails (CLI returns "Session not found" / corrupt / shutdown) and creates a fresh session, the old persisted entry was not being marked as superseded. On the next `SaveActiveSessionsToDisk`, `MergeSessionEntries` found both old and new entries with the same display name and renamed the old one to `"(previous)"`. ## Root cause The lazy-resume fallback in `EnsureSessionConnectedAsync` creates a fresh session but didn't: 1. Set `RecoveredFromSessionId` — so `CanSafelyDropSupersededGroupMoveEntry` returned false 2. Add the old session ID to `_closedSessionIds` — so the merge didn't filter it out ## Fix Two lines after the fresh session is created: - `state.Info.RecoveredFromSessionId ??= sessionId` — marks the new session as the successor - `_closedSessionIds.TryAdd(sessionId, 0)` — ensures the merge drops the old entry ## Real-world trigger Observed on session "34678": the CLI shut down the original session (`session.shutdown` at 02:57 UTC). After app restart, lazy-resume got "Session not found", created a fresh session, but left the old entry on disk → "34678 (previous)" appeared. ## Testing 3335/3335 tests pass. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent fa06232 commit 6d3306b

3 files changed

Lines changed: 116 additions & 0 deletions

File tree

PolyPilot.Tests/SessionPersistenceTests.cs

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1790,4 +1790,102 @@ public void Merge_NameCollision_NullGroupIds_StillCreatesPrevious()
17901790
Assert.Equal(2, result.Count);
17911791
Assert.Equal("MyWorker (previous)", result[1].DisplayName);
17921792
}
1793+
1794+
[Fact]
1795+
public void Merge_LazyResumeFallback_RecoveredFromSessionId_DropsOldEntry()
1796+
{
1797+
// Simulates: lazy-resume fails ("Session not found") → fresh session created →
1798+
// RecoveredFromSessionId set to old ID → _closedSessionIds contains old ID →
1799+
// merge must drop the old entry, not rename it "(previous)".
1800+
var active = new List<ActiveSessionEntry>
1801+
{
1802+
new() { SessionId = "fresh-id", DisplayName = "34678", Model = "claude-opus-4.6",
1803+
WorkingDirectory = "/w", GroupId = "group-1", RecoveredFromSessionId = "dead-id" }
1804+
};
1805+
var persisted = new List<ActiveSessionEntry>
1806+
{
1807+
new() { SessionId = "dead-id", DisplayName = "34678", Model = "claude-opus-4.6",
1808+
WorkingDirectory = "/w", GroupId = "group-1" }
1809+
};
1810+
var closedIds = new HashSet<string> { "dead-id" };
1811+
1812+
var result = CopilotService.MergeSessionEntries(active, persisted, closedIds, new HashSet<string>(), _ => true);
1813+
1814+
Assert.Single(result);
1815+
Assert.Equal("fresh-id", result[0].SessionId);
1816+
Assert.Equal("34678", result[0].DisplayName);
1817+
}
1818+
1819+
[Fact]
1820+
public void Merge_DoubleRecovery_UsesImmediatePredecessor()
1821+
{
1822+
// Simulates double recovery: A → B (first recovery) → C (second recovery).
1823+
// C.RecoveredFromSessionId must be B (the immediate predecessor), not A.
1824+
// After restart, _closedSessionIds is empty, so only RecoveredFromSessionId
1825+
// can prevent B from appearing as "(previous)".
1826+
var active = new List<ActiveSessionEntry>
1827+
{
1828+
new() { SessionId = "session-C", DisplayName = "MySession", Model = "m",
1829+
WorkingDirectory = "/w", GroupId = "g1", RecoveredFromSessionId = "session-B" }
1830+
};
1831+
var persisted = new List<ActiveSessionEntry>
1832+
{
1833+
new() { SessionId = "session-B", DisplayName = "MySession", Model = "m",
1834+
WorkingDirectory = "/w", GroupId = "g1" }
1835+
};
1836+
// _closedSessionIds is empty (simulates post-restart)
1837+
var closedIds = new HashSet<string>();
1838+
1839+
var result = CopilotService.MergeSessionEntries(active, persisted, closedIds, new HashSet<string>(), _ => true);
1840+
1841+
// B must be dropped because C.RecoveredFromSessionId == B
1842+
Assert.Single(result);
1843+
Assert.Equal("session-C", result[0].SessionId);
1844+
Assert.Equal("MySession", result[0].DisplayName);
1845+
}
1846+
1847+
[Fact]
1848+
public void Merge_DoubleRecovery_StaleAncestor_CreatesPhantom()
1849+
{
1850+
// Proves why ??= was wrong: if C.RecoveredFromSessionId == A (stale ancestor
1851+
// from first recovery, not B), the merge can't link C→B and creates "(previous)".
1852+
var active = new List<ActiveSessionEntry>
1853+
{
1854+
new() { SessionId = "session-C", DisplayName = "MySession", Model = "m",
1855+
WorkingDirectory = "/w", GroupId = "g1", RecoveredFromSessionId = "session-A" }
1856+
};
1857+
var persisted = new List<ActiveSessionEntry>
1858+
{
1859+
new() { SessionId = "session-B", DisplayName = "MySession", Model = "m",
1860+
WorkingDirectory = "/w", GroupId = "g1" }
1861+
};
1862+
var closedIds = new HashSet<string>();
1863+
1864+
var result = CopilotService.MergeSessionEntries(active, persisted, closedIds, new HashSet<string>(), _ => true);
1865+
1866+
// B survives as "(previous)" because RecoveredFromSessionId=A doesn't match B
1867+
Assert.Equal(2, result.Count);
1868+
Assert.Equal("MySession (previous)", result[1].DisplayName);
1869+
}
1870+
1871+
[Fact]
1872+
public void LazyResumeFallback_SetsRecoveredFromSessionIdAndClosedIds()
1873+
{
1874+
// Structural guard: the lazy-resume fallback in EnsureSessionConnectedAsync
1875+
// must set RecoveredFromSessionId (=, not ??=) and add old ID to _closedSessionIds
1876+
// so MergeSessionEntries drops the old entry.
1877+
var source = File.ReadAllText(
1878+
Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs"));
1879+
1880+
// Find the fallback block — search for the full method containing the fallback
1881+
var methodIdx = source.IndexOf("EnsureSessionConnectedAsync", StringComparison.Ordinal);
1882+
Assert.True(methodIdx > 0, "Could not find EnsureSessionConnectedAsync");
1883+
var methodBlock = source.Substring(methodIdx, Math.Min(5000, source.Length - methodIdx));
1884+
1885+
// Must use = (not ??=) for double-recovery correctness
1886+
Assert.Contains("RecoveredFromSessionId = sessionId;", methodBlock);
1887+
Assert.DoesNotContain("RecoveredFromSessionId ??= sessionId", methodBlock);
1888+
// Must add to _closedSessionIds for same-process protection
1889+
Assert.Contains("_closedSessionIds[sessionId] = 0;", methodBlock);
1890+
}
17931891
}

PolyPilot/Services/CopilotService.Persistence.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -442,7 +442,14 @@ private async Task EnsureSessionConnectedAsync(string sessionName, SessionState
442442
// BuildFreshSessionConfig doesn't set OnEvent on the SessionConfig, and
443443
// the old post-resume .On() was removed, so we must register explicitly here.
444444
copilotSession.On(evt => HandleSessionEvent(state, evt));
445+
// Record that this fresh session replaced the old one so MergeSessionEntries
446+
// drops the old persisted entry instead of renaming it "(previous)".
447+
state.Info.RecoveredFromSessionId = sessionId;
445448
state.Info.SessionId = copilotSession.SessionId;
449+
// Add the old session ID to the closed set so SaveActiveSessionsToDisk's
450+
// merge logic drops it instead of creating a "(previous)" phantom entry.
451+
if (!string.IsNullOrEmpty(sessionId))
452+
_closedSessionIds[sessionId] = 0;
446453
FlushSaveActiveSessionsToDisk();
447454
}
448455
catch (Exception ex) when (IsAuthError(ex))

PolyPilot/Services/CopilotService.cs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3858,6 +3858,7 @@ public async Task<string> SendPromptAsync(string sessionName, string prompt, Lis
38583858
reconnectConfig.WorkingDirectory = state.Info.WorkingDirectory;
38593859

38603860
CopilotSession newSession;
3861+
var oldSessionId = state.Info.SessionId; // Capture before resume may change it
38613862
try
38623863
{
38633864
newSession = await client.ResumeSessionAsync(state.Info.SessionId, reconnectConfig, cancellationToken);
@@ -3887,7 +3888,12 @@ public async Task<string> SendPromptAsync(string sessionName, string prompt, Lis
38873888
{
38883889
var freshConfig = BuildFreshSessionConfig(state);
38893890
newSession = await client.CreateSessionAsync(freshConfig, cancellationToken);
3891+
// Mark the old session as superseded so MergeSessionEntries drops it
3892+
// instead of renaming it to "(previous)".
3893+
state.Info.RecoveredFromSessionId = oldSessionId;
38903894
state.Info.SessionId = newSession.SessionId;
3895+
if (!string.IsNullOrEmpty(oldSessionId))
3896+
_closedSessionIds[oldSessionId] = 0;
38913897
FlushSaveActiveSessionsToDisk();
38923898
}
38933899
catch (Exception createEx)
@@ -3914,7 +3920,12 @@ public async Task<string> SendPromptAsync(string sessionName, string prompt, Lis
39143920
{
39153921
var freshConfig = BuildFreshSessionConfig(state);
39163922
newSession = await client.CreateSessionAsync(freshConfig, cancellationToken);
3923+
// Mark the old session as superseded so MergeSessionEntries drops it
3924+
// instead of renaming it to "(previous)".
3925+
state.Info.RecoveredFromSessionId = oldSessionId;
39173926
state.Info.SessionId = newSession.SessionId;
3927+
if (!string.IsNullOrEmpty(oldSessionId))
3928+
_closedSessionIds[oldSessionId] = 0;
39183929
FlushSaveActiveSessionsToDisk();
39193930
}
39203931
catch (Exception createEx)

0 commit comments

Comments
 (0)