fix: force sync bypasses streaming guard for active session

[PureWeen]

When the user clicks the sync button on mobile, ForceRefreshRemoteAsync requests full history from the server but SyncRemoteSessions skips applying it because the session is in _remoteStreamingSessions. This caused 'Already up to date' even when messages were missed during a disconnect window.

Fix: Force sync now directly applies the server's authoritative history for the active session, bypassing the streaming guard.

Root cause: Line 556 in CopilotService.Bridge.cs — the streaming guard prevents SyncRemoteSessions from replacing incrementally-built history during active streaming. But when a user explicitly clicks sync after a disconnect, they need the server's full history to fill in missed messages.

[PureWeen]

Review: PR #444 — fix: force sync bypasses streaming guard for active session

Small, focused fix that correctly addresses the root cause described in the PR: SyncRemoteSessions was blocking history replacement for sessions in _remoteStreamingSessions, causing the sync button to always show "Already up to date" even after a disconnect.

---

✅ What's correct

• The lock (forceState.Info.HistoryLock) around Clear+AddRange is correct — consistent with how SyncRemoteSessions does it.
• Using SessionHistories.TryGetValue as the authoritative copy source is the right approach (it holds what the server sent).
• The diagnostic log is helpful for post-mortem analysis.
• Small diff, surgical change.

---

⚠️ Bug: force-apply can corrupt history during an active streaming session

The new block has no guard against sessions that are genuinely mid-stream (not just post-disconnect):

if (activeSessionName != null
    && _bridgeClient.SessionHistories.TryGetValue(activeSessionName, out var serverMessages)
    && _sessions.TryGetValue(activeSessionName, out var forceState))
{
    lock (forceState.Info.HistoryLock)
    {
        forceState.Info.History.Clear();
        forceState.Info.History.AddRange(serverMessages);  // ← could be stale
    }

If the user hits sync while a session is actively streaming (in _remoteStreamingSessions), SessionHistories[name] holds the last pushed history snapshot from the server — which is behind the incremental content_delta events that have already been applied locally. Force-replacing history with this stale snapshot erases content the user can already see.

The streaming guard exists exactly to prevent this. Bypassing it unconditionally is unsafe.

Suggested fix: only bypass the guard when the session is NOT actively streaming, OR when the server snapshot is strictly larger than local history:

var isActivelyStreaming = _remoteStreamingSessions.ContainsKey(activeSessionName);
{
    lock (forceState.Info.HistoryLock)
    {
        forceState.Info.History.Clear();
        forceState.Info.History.AddRange(serverMessages);
    }
    forceState.Info.MessageCount = forceState.Info.History.Count;
}

This mirrors the logic already in SyncRemoteSessions on line 556 — skip the guard only when the server has more messages than local.

---

Minor: MessageCount written outside the lock

lock (forceState.Info.HistoryLock)
{
    forceState.Info.History.Clear();
    forceState.Info.History.AddRange(serverMessages);
}
forceState.Info.MessageCount = forceState.Info.History.Count;  // ← outside lock

SyncRemoteSessions keeps MessageCount inside the lock. This should match for consistency (and to avoid a theoretical race where MessageCount is read between the lock release and this assignment).

---

Pre-existing: Task.Delay(500) still racy

Not introduced by this PR, but worth noting: if the history response from the server takes >500ms, SessionHistories.TryGetValue returns false and the force-apply block is silently skipped. The user still sees "Already up to date" and no error. This PR doesn't make it worse, but it remains the underlying fragility of this approach.

---

Summary

The fix is directionally correct but bypasses the streaming guard unconditionally. Add the "not streaming OR server has more" condition before merging to avoid corrupting live session history.

[PureWeen]

🔍 Squad Review — PR #444

PR: fix: force sync bypasses streaming guard for active session
Size: +16/-0, 1 file (CopilotService.Bridge.cs)
Commit: cd28bfd4

---

Root Cause & Fix

SyncRemoteSessions correctly skips history replacement for sessions in _remoteStreamingSessions (to avoid overwriting incrementally-built content_delta messages with a stale snapshot). But when the user explicitly clicks the sync button via ForceRefreshRemoteAsync, that same guard prevents the server's authoritative history from filling in missed messages — giving a misleading "Already up to date" result.

The fix directly applies SessionHistories[activeSession] after the Task.Delay(500) wait, bypassing the streaming guard:

if (activeSessionName != null
    && _bridgeClient.SessionHistories.TryGetValue(activeSessionName, out var serverMessages)
    && _sessions.TryGetValue(activeSessionName, out var forceState))
{
    lock (forceState.Info.HistoryLock)
    {
        forceState.Info.History.Clear();
        forceState.Info.History.AddRange(serverMessages);
    }
    forceState.Info.MessageCount = forceState.Info.History.Count;
    Debug($"[SYNC] Force-applied {serverMessages.Count} messages ...");
}

Logic is correct. ✅

---

Thread Safety Analysis

SessionHistories value safety: SessionHistories is ConcurrentDictionary<string, List<ChatMessage>>. TryGetValue returns a reference to the specific list at that moment. If a new SessionHistory bridge message arrives concurrently, SessionHistories[name] is atomically replaced with a new list — the old list reference held by serverMessages remains valid and unmodified (the bridge always creates a fresh deserialized list, never mutates the existing one). ✅

HistoryLock usage: The fix holds HistoryLock during Clear() + AddRange(), matching the pattern used by BroadcastSessionHistoryAsync and ContinueInNewSessionAsync. This is stricter than the existing SyncRemoteSessions, which performs the same Clear()/AddRange() without the lock (lines 538–539). The new code sets the bar correctly. ✅

MessageCount outside lock: forceState.Info.MessageCount = forceState.Info.History.Count is read after releasing HistoryLock — identical to the existing SyncRemoteSessions pattern (line 540). MessageCount is a display counter, not used for safety-critical decisions. Consistent with existing behavior. ✅

Post-sync snapshot ordering: The force-apply happens before the post-sync snapshot (preSyncMessageCount / postSyncMessageCount), so messageDelta accurately reflects how many messages were recovered. ✅

---

Potential Edge Case (non-blocking)

If the user clicks sync during active streaming (rare but possible), the force-apply clears and replaces History with the server's last-known snapshot. Content_delta events will continue arriving and appending correctly after the clear — they represent the in-progress message, not messages already in the snapshot. _remoteStreamingSessions remains set and the normal TurnEnd→idle flow eventually clears it. No duplication risk. ✅

---

✅ Verdict: Approve

Small, well-reasoned fix. HistoryLock is correctly held (actually improves on the existing SyncRemoteSessions pattern). The streaming guard bypass is intentional and sound — user-initiated sync should always win. No new issues introduced.

🚢 Good to merge.

[PureWeen]

🤖 PR #444 Review

Title: fix: force sync bypasses streaming guard for active session
Files changed: 1 file, +16 lines
Test results: ✅ All 2987 tests pass

---

✅ Problem Statement — Clear and Well-Defined

The PR description clearly identifies the issue:

Symptom: User clicks sync button on mobile, but sees "Already up to date" even when messages were missed during a disconnect window.

Root cause: Line 556 in CopilotService.Bridge.cs — the streaming guard in SyncRemoteSessions prevents history replacement for sessions in _remoteStreamingSessions, even when the user explicitly requests a force sync.

Why the guard exists: During active streaming (content deltas, tool events), the incrementally-built local history is more up-to-date than the server's cached history. The guard prevents overwriting real-time updates with stale server data.

Why this is a problem for force sync: When a user explicitly clicks the sync button after a disconnect, they need the server's full history to recover missed messages. The automatic SyncRemoteSessions logic is conservative (preserves streaming), but force sync should be aggressive (trust the server).

---

✅ Solution — Surgical and Correct

The fix adds 16 lines to ForceRefreshRemoteAsync (lines 658-672) that directly apply the server's history for the active session, bypassing the streaming guard:

// Force-apply server history for the active session, bypassing the streaming guard.
// SyncRemoteSessions skips sessions in _remoteStreamingSessions, but a user-initiated
// force sync should always replace local history with the server's authoritative copy.
if (activeSessionName != null
    && _bridgeClient.SessionHistories.TryGetValue(activeSessionName, out var serverMessages)
    && _sessions.TryGetValue(activeSessionName, out var forceState))
{
    lock (forceState.Info.HistoryLock)
    {
        forceState.Info.History.Clear();
        forceState.Info.History.AddRange(serverMessages);
    }
    forceState.Info.MessageCount = forceState.Info.History.Count;
    Debug($"[SYNC] Force-applied {serverMessages.Count} messages for '{activeSessionName}' (bypassed streaming guard)");
}

Why this is correct:

1. Proper locking: Uses HistoryLock to prevent race conditions (consistent with rest of codebase)
2. Atomic replace: Clear + AddRange pattern matches SyncRemoteSessions (line 564-565)
3. Only active session: Only forces the session the user is viewing, doesn't affect background sessions
4. Placement: Happens AFTER the 500ms delay (line 656), so server response is available
5. Updates MessageCount: Keeps Info.MessageCount in sync (line 670)
6. Diagnostic logging: Clear [SYNC] log message for post-mortem analysis

---

🟢 Minor Observations

1. Interaction with streaming guard removal

The streaming guard is normally removed on SessionIdleEvent (line 275). If the user force-syncs while a turn is active, the guard is still present in _remoteStreamingSessions. This fix correctly bypasses it for force sync, but the guard will remain until the turn completes.

Impact: None — the fix is specifically designed for this scenario (mid-turn force sync after disconnect).

---

2. Post-sync count may double-count

After the force-apply block (line 672), the code snapshots postSyncMessageCount (line 679-681). But the post-sync snapshot reads from activeInfo.History, which was populated from the pre-sync snapshot (line 643-648). If the force-apply changed the history, the delta calculation could be off.

Wait, let me trace this more carefully:

• Line 643: activeState is captured pre-sync
• Line 645: activeInfo = activeState.Info
• Line 663: forceState is re-fetched from _sessions
• Line 677: Post-sync reads from activeInfo

Are activeInfo and forceState.Info the same object?

Yes! AgentSessionInfo is the .Info property of SessionState, and SessionState is a reference type. So:

• activeInfo points to the same AgentSessionInfo object as forceState.Info
• When line 668 modifies forceState.Info.History, it's modifying the same list that activeInfo.History points to
• The post-sync snapshot (line 680) sees the updated history

Verdict: The count logic is correct. No issue here.

---

3. The 500ms delay race condition (from PR #438 Round 1)

This PR doesn't address the hardcoded 500ms delay at line 656, which is still a race condition. However:

• The force-apply happens AFTER the delay, so it has the same timing issues as before
• The addition doesn't make the race worse
• This is a pre-existing issue that should be tracked separately

---

📊 Test Coverage

Passing: All 2987 tests pass
New tests: None added

Is test coverage needed?

The fix is a 16-line addition to an existing method. Ideally, there would be a test like:

[Fact]
public async Task ForceRefreshRemoteAsync_WhenStreamingGuardActive_StillAppliesServerHistory()
{
    var svc = CreateRemoteService();
    await AddRemoteSession(svc, "test-session");
    
    // Simulate streaming guard active (turn in progress)
    svc._remoteStreamingSessions.TryAdd("test-session", 1);
    
    // Local has 5 messages, server has 10 (some were missed)
    AddLocalMessages(svc, "test-session", 5);
    _bridgeClient.SessionHistories["test-session"] = CreateMessages(10);
    
    var result = await svc.ForceRefreshRemoteAsync("test-session");
    
    // Force sync should bypass guard and apply all 10 messages
    var session = svc.GetSession("test-session");
    Assert.Equal(10, session.History.Count);
    Assert.Equal(5, result.MessageCountBefore);
    Assert.Equal(10, result.MessageCountAfter);
}

Impact of missing test: Low — the fix is straightforward and follows established patterns. The existing 2987 tests provide good coverage of the surrounding code. A targeted test would be nice-to-have for regression protection, but not blocking.

---

🎯 Final Verdict

✅ APPROVE — Clean, focused fix that solves a real user-facing problem.

Strengths:

• Problem statement is clear and root cause is identified
• Solution is surgical (16 lines) and follows codebase patterns
• Proper thread safety with HistoryLock
• Diagnostic logging for debugging
• All tests pass
• Code comment explains the why (bypass streaming guard for user-initiated sync)

Recommendations for follow-up:

1. Add regression test (nice-to-have) — test that force sync works even when streaming guard is active
2. Address 500ms delay (from PR feat: add sync button for mobile + diagnostic logging #438) — this PR doesn't fix it, but it's still a race condition

Recommendation: Ready to merge as-is. Test coverage would be ideal but is not blocking given the simplicity and clarity of the fix.

---

📈 Context

This PR builds on the sync button feature from PR #438 (which added ForceRefreshRemoteAsync). It fixes an edge case where the sync button didn't work as expected when a session was actively streaming.

PR #438 history:

• Round 1: Added sync button + diagnostic logging
• Round 2: Fixed thread safety (HistoryLock), image cloning, name collisions
• Round 3: Added bridge disconnect error handling, auto-send continuation

PR #444: Fixes the streaming guard bypass for force sync (this review)

The incremental fix approach shows good engineering discipline — each PR addresses a specific, well-scoped issue.

[PureWeen]

🔍 Squad Review — PR #444 Round 1

Models: claude-opus-4.6 ×2, claude-sonnet-4.6 ×2, gpt-5.3-codex
CI: No checks reported (1 commit, fresh PR)

---

🟡 Moderate (consensus: 4/5 models)

M1 — Force-apply unconditionally clears History, no count guard

The force-apply does History.Clear() + AddRange(serverMessages) without checking whether the server snapshot is behind local history. SyncRemoteSessions at line 556 has an explicit messages.Count >= s.Info.History.Count guard to prevent truncation. The force-apply skips it entirely.

Scenario: During active streaming, local history has 10 messages (partially built by content_delta). Server snapshot (from the last BroadcastSessionHistoryAsync) has 8 messages (captured before the latest deltas). Force-apply wipes 2 in-progress messages with no recovery path.

The intended scenario (force sync after disconnect) is correct — stuck sessions in _remoteStreamingSessions with missed messages. But the code also fires during active streaming where the server snapshot is stale.

Fix: Add a guard: only force-apply when serverMessages.Count >= forceState.Info.History.Count, or gate on _remoteStreamingSessions.ContainsKey(activeSessionName) so it only fires when the streaming guard is genuinely blocking SyncRemoteSessions.

M2 — MessageCount set outside HistoryLock (consensus: 5/5 models)

lock (forceState.Info.HistoryLock)
{
    forceState.Info.History.Clear();
    forceState.Info.History.AddRange(serverMessages);
}
forceState.Info.MessageCount = forceState.Info.History.Count;  // ← outside lock

A content_delta handler can acquire HistoryLock and append between the lock release and the Count read. MessageCount then reflects a state that never existed atomically. This is a pre-existing pattern (line 567 has the same issue), but both should move inside the lock.

One-line fix: Move forceState.Info.MessageCount = forceState.Info.History.Count; inside the lock block.

M3 — No test coverage (consensus: 5/5 models)

The PR adds a concurrency-sensitive bypass of a safety guard with no test. A unit test verifying that ForceRefreshRemoteAsync applies history when the session is in _remoteStreamingSessions (and does NOT truncate when server has fewer messages) would prevent regressions.

---

🟢 Minor (pre-existing, not blocking)

#	Issue	Models
m1	Task.Delay(500) still sole sync mechanism — if server responds >500ms, SessionHistories has stale data and force-apply does nothing new	3/5

---

⚠️ Request Changes

One functional concern:

M1 — The force-apply needs a count guard or streaming-state gate to prevent truncating history during active streaming. Without it, a user clicking sync while the model is actively responding can lose in-progress content. The fix is a one-line if guard.

M2 is a one-line fix (move inside lock). M3 (tests) is strongly recommended given this code bypasses a safety guard that has required multiple fix cycles.

[PureWeen]

🔄 Re-Review Round 2 (4-Model Consensus)

Tests: ✅ 2990/2990 (1 pre-existing flaky TurnEndFallbackTests.FallbackTimer_NotCancelled_FiresAfterDelay — passes in isolation)
Verdict: ✅ Approve (with one hardening suggestion)

---

R1 Findings Status

Finding	Status	Notes
M1 — Force-apply truncates history during streaming	✅ FIXED	New guard: !isActivelyStreaming || serverMessages.Count > localCount prevents clearing when server snapshot is stale
M2 — MessageCount outside HistoryLock	✅ FIXED	Assignment now inside the lock block
M3 — No test coverage	✅ FIXED	3 new tests in BridgeDisconnectTests.cs covering: guard+server-larger (applies), guard+server-smaller (skips), no-guard (always applies)

---

New Findings

🟡 Minor: Double-lock pattern is a maintenance hazard (3/4 models flagged)

localCount is read under one lock acquisition, then the apply decision and second lock happen separately:

lock (forceState.Info.HistoryLock)
    localCount = forceState.Info.History.Count;   // lock 1 released

// ← a content_delta could append here

if (!isActivelyStreaming || serverMessages.Count > localCount)  // stale localCount
{
    lock (forceState.Info.HistoryLock)            // lock 2
    {
        forceState.Info.History.Clear();           // could wipe the just-appended delta

While the consequence is mild (streaming naturally recovers from the next delta), merging into a single lock region eliminates the race entirely and is cleaner:

lock (forceState.Info.HistoryLock)
{
    var localCount = forceState.Info.History.Count;
    if (!isActivelyStreaming || serverMessages.Count > localCount)
    {
        forceState.Info.History.Clear();
        forceState.Info.History.AddRange(serverMessages);
        forceState.Info.MessageCount = forceState.Info.History.Count;
    }
}

🟢 Nit: Missing test for the original regression scenario (2/4 models)

The new test ForceSync_WhenStreamingGuardActive_AppliesServerHistoryIfLarger uses server=4 > local=2, which the old unconditional apply also handled correctly. The original bug was server.Count == local.Count (same count, different content — missed messages during reconnect that the stale cache hadn't refreshed). A test for this exact case would lock in the fix:

// ForceSync_WhenStreamingGuardActive_SameCount_SkipsApply
// local=3, server=3 (stale snapshot, different content), streaming=true → should NOT replace
Assert.Equal(3, session.History.Count); // local preserved

🟢 Nit: New tests add ~3s each to the test suite (1 model)

The polling loop runs for the full 3s timeout in tests because StubWsBridgeClient.RequestHistoryAsync is a no-op — it doesn't replace the SessionHistories reference, so ReferenceEquals never breaks early. 3 tests × ~3s = ~9s of idle wait added to CI. Stub could simulate a server response by replacing the reference in a background task.

ℹ️ Pre-existing: SyncRemoteSessions has the same MessageCount pattern (2 models noted)

SyncRemoteSessions at line ~567 still writes s.Info.MessageCount = s.Info.History.Count outside HistoryLock. Not introduced by this PR — follow-up cleanup opportunity.

---

Polling loop correctness: ✅

The ReferenceEquals-based detection works correctly in all cases:

• No prior history (preSyncCachedHistory = null): When server responds with a new list, !ReferenceEquals(newList, null) → breaks
• In-place mutation: WsBridgeClient always does full-replace (SessionHistories[name] = history.Messages), not in-place appends — ReferenceEquals reliably detects this

The fix is correct and the R1 blockers are all resolved. The double-lock consolidation is strongly recommended before merge but is a hardening improvement, not a blocking correctness issue.

[PureWeen]

🔄 Re-Review Round 3 (3-Model Consensus)

Tests: ✅ 2991/2991 (clean)
Verdict: ⚠️ Request Changes (one scroll regression)

---

Previous Findings Status

Finding	Status
M1 — Streaming truncation (force-apply without count guard)	✅ FIXED
M2 — MessageCount outside HistoryLock	✅ FIXED
M3 — No test coverage	✅ FIXED (4 tests including same-count case)
R2 — Double-lock TOCTOU	✅ ADDRESSED (single lock block)
R2 — Tests taking ~3s each	✅ ADDRESSED (stub now replaces reference immediately)
R2 — Missing same-count test	✅ FIXED (ForceSync_WhenStreamingGuardActive_SameCount_SkipsApply)

The core bridge fix is complete and correct. All tests pass cleanly.

---

New Findings

🔴 High: __userScrolledUp not cleared on forceScroll — streaming auto-scroll broken after send (3/3 models)

Introduced by this PR (the __userScrolledUp flag is new in this diff). The forceScroll path bypasses the guard but never clears the flag:

// Line 685: guard bypassed on forceScroll
if (el.__userScrolledUp && !forceScroll) return;

// Line 698-701: scroll to bottom, but __userScrolledUp stays true
if (forceScroll || isNearBottom || el.__wasAtBottom) {
    el.__programmaticScroll = true;
    el.scrollTop = el.scrollHeight;
    requestAnimationFrame(function() { el.__programmaticScroll = false; });
    // ← __userScrolledUp is NEVER reset here
}

The scroll listener also can't clear it — it fires during the programmatic scroll but returns early because __programmaticScroll is still true.

Result: user scrolls up to read context → sends message → view jumps to bottom ✓ → response starts streaming → __userScrolledUp && !forceScroll → true && true → returns early → every streaming delta renders off-screen, user can't see the response without manually scrolling down.

Fix: reset the flag when forceScroll causes a scroll:

if (forceScroll || isNearBottom || el.__wasAtBottom) {
    if (forceScroll) {
        el.__userScrolledUp = false;   // ← add this
        el.__wasAtBottom = true;
    }
    el.__programmaticScroll = true;
    el.scrollTop = el.scrollHeight;
    requestAnimationFrame(function() { el.__programmaticScroll = false; });
}

---

🟡 Minor: positionSessionMenu called on every Blazor render while menu is open (2/3 models)

OnAfterRenderAsync fires on every render cycle — including per-streaming-delta renders. When the menu is open, each cycle invokes positionSessionMenu which reads menu.offsetWidth, menu.scrollHeight, and buttonEl.getBoundingClientRect() — all layout-forcing properties.

Fix: track whether the menu was already positioned and skip re-positioning when only content (not layout) changed. A simple _menuPositioned bool reset when IsMenuOpen toggles would suffice.

---

🟢 Nit: Fixed-position menu drifts on session-list scroll (2/3 models)

positionSessionMenu positions the menu once at open time via style.top/left. If the user scrolls the session sidebar (or resizes the window) while the menu is open, the menu floats at its original position detached from the ⋯ button. The .menu-overlay allows click-to-dismiss but there's no scroll or resize handler. A scroll listener on the sidebar container that closes the menu would address this.

---

ℹ️ Note: isActivelyStreaming outside the lock is not a real TOCTOU

_remoteStreamingSessions is a ConcurrentDictionary not governed by HistoryLock — there's no way to check them atomically. The window is microseconds on a user-initiated action, and the localCount guard inside the lock provides a second safety check. This is acceptable.

[PureWeen]

✅ Re-Review Round 4 (Orchestrator Direct)

Tests: ✅ 2991/2991
Commits: 5 (1 new since R3)

---

Previous Findings Status

Finding	Status
M1 — Streaming truncation (force-apply without count guard)	✅ FIXED (R2)
M2 — MessageCount outside HistoryLock	✅ FIXED (R2)
M3 — No test coverage	✅ FIXED (R2, 4 tests)
R2 — Double-lock TOCTOU	✅ ADDRESSED (R3)
R3/N1 — __userScrolledUp not reset on forceScroll	✅ FIXED
R3/N2 — Layout thrashing in OnAfterRenderAsync	✅ FIXED (_menuPositioned flag)
R3/N3 — Menu drifts on sidebar scroll	✅ FIXED (scroll listener auto-closes)

New Findings

None. All R1–R3 findings fully resolved.

Verdict: ✅ Approve

Clean implementation. The bridge force-sync is correct (count guard + single lock), scroll tracking works (forceScroll resets user-scroll-up flag), menu positioning is efficient (one-shot + scroll-close). 4 regression tests cover the key scenarios.