fix: prevent phantom (previous) sessions from lazy-resume fallback

[PureWeen]

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.

[PureWeen]

Code Review — PR #584: fix: prevent phantom (previous) sessions from lazy-resume fallback

Reviewed by: 3 independent reviewers | CI: ⚠️ No checks reported on branch

---

Final Re-Review (4 commits)

Previous Finding Status

#	Finding	Status
1	🟡 ??= should be = on RecoveredFromSessionId	✅ FIXED — All 3 sites now use = (commit acab0f62f)
2	🟡 Missing behavioral test for lazy-resume fallback	✅ FIXED — 3 behavioral MergeSessionEntries tests + 1 structural guard added (commit c459819a0)
3	🟢 TryAdd vs indexer inconsistency	✅ FIXED — All 3 sites now use _closedSessionIds[id] = 0
4	🟢 Defensive null guard on validated input	➖ ACCEPTED — Harmless defensive guard, kept as-is
5	🟢 Code duplication across 3 sites	➖ ACCEPTED — Helper extraction deferred; pattern is small and clear

Test Quality Assessment

4 well-constructed tests:

• Merge_LazyResumeFallback_RecoveredFromSessionId_DropsOldEntry — behavioral: verifies merge drops old entry when both RecoveredFromSessionId and _closedSessionIds are set
• Merge_DoubleRecovery_UsesImmediatePredecessor — behavioral: proves immediate predecessor (B) is correctly linked and dropped after restart (empty _closedSessionIds)
• Merge_DoubleRecovery_StaleAncestor_CreatesPhantom — behavioral: documents why ??= was wrong — stale ancestor creates phantom
• LazyResumeFallback_SetsRecoveredFromSessionIdAndClosedIds — structural guard ensuring the production code uses = (not ??=) and the indexer

The 3 behavioral tests cover the merge logic regardless of which call site triggers it. The structural test serves as a supplementary invariant guard against regression — consistent with the project's convention of using structural tests as supplements to behavioral coverage.

No New Issues Found

All 3 reviewers confirmed no new bugs, regressions, security issues, or race conditions.

---

Recommended Action: ✅ Approve

All prior findings addressed. Code is correct, consistent, and well-tested. Ship it.