Harden bridge History thread-safety with comprehensive HistoryLock coverage#215
Closed
Harden bridge History thread-safety with comprehensive HistoryLock coverage#215
Conversation
62625b0 to
1cd7b30
Compare
905018b to
0083ce7
Compare
1. Thread-safe History access:
- Add HistoryLock + GetHistorySnapshot() to AgentSessionInfo
- Lock all background-thread writes in Bridge.cs event handlers
(OnContentReceived, OnToolStarted, OnToolCompleted, OnImageReceived,
OnReasoningReceived, OnReasoningComplete, SyncRemoteSessions Clear+AddRange)
- WsBridgeServer uses GetHistorySnapshot() instead of bare ToArray()
- UnreadCount uses GetHistorySnapshot() instead of try/catch
2. Server-side history cap (MaxHistoryPayloadMessages = 500):
- SendSessionHistoryToClient enforces max regardless of client-requested limit
- Prevents any caller (including LoadFullRemoteHistory) from sending unbounded payloads
3. Retry on failed history requests:
- SyncRemoteSessions now re-requests history for sessions that were previously
requested but still have no local history (server snapshot may have failed)
- Prevents sessions from being permanently stuck with no history
4. Use MessageCount instead of History.Count in WsBridgeServer to avoid
cross-thread reads of the list for simple count checks.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s mutations, reset retry cap - Lock ApplyReasoningUpdate to protect History reads and Add under HistoryLock - Lock CompleteReasoningMessages History.Where query under HistoryLock - Lock tool dedup read (FirstOrDefault) and History.Add for tool messages - Lock ToolExecutionCompleteEvent History.LastOrDefault lookup - Lock History.Add in remote-mode SendPromptAsync - Lock History.Add in AbortSessionAsync with MessageCount update - Lock History.Clear/MessageCount in ClearHistory - Clear _requestedHistorySessions entry on successful history sync (fix retry cap never resetting) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…MessageCount - ToolExecutionCompleteEvent: extend lock to cover all histToolMsg mutations (was: reference escaped lock scope leaving all property writes unprotected) - ApplyReasoningUpdate: extend lock to cover all reasoningMsg property mutations - CompleteReasoningMessages: move property mutations inside lock with DB content snapshot - SyncRemoteSessions: add retry cap (max 5) — was retrying indefinitely every cycle - OnContentReceived/OnToolStarted: update MessageCount after History.Add Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d, SyncRemoteSessions - FlushCurrentResponse: wrap History.LastOrDefault + History.Add + MessageCount in HistoryLock - CompleteResponse: wrap History.Add + MessageCount + LastReadMessageCount in HistoryLock - Bridge OnTurnEnd: wrap History.LastOrDefault + IsComplete/Model mutations in HistoryLock - SyncRemoteSessions: move messages.Count >= History.Count check inside HistoryLock to close TOCTOU These paths ran on UI/InvokeOnUI thread but accessed History without the lock that background bridge callbacks (OnContentReceived, OnToolStarted, etc.) hold — allowing concurrent List<T> corruption. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9255aad to
76ef9c6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #214. Addresses review findings around History thread-safety, server-side enforcement, retry resilience, and lock scope correctness.
Changes
1. Thread-safe History access via
HistoryLockHistoryLock+GetHistorySnapshot()toAgentSessionInfo— a single lock object and snapshot method for all callersCopilotService.Bridge.cs:OnContentReceived,OnToolStarted,OnToolCompleted,OnImageReceived,OnReasoningReceived,OnReasoningComplete, andSyncRemoteSessionsClear+AddRangeWsBridgeServer.SendSessionHistoryToClientusesGetHistorySnapshot()instead of bareToArray()UnreadCountusesGetHistorySnapshot()instead of try/catch2. Widened lock scopes to cover all mutations (Review Fix)
ToolExecutionCompleteEvent: Extended lock to cover allhistToolMsgproperty mutations (MessageType,ImagePath,Caption,IsComplete,IsSuccess,Content) — previously the reference escaped the lock scopeApplyReasoningUpdate: Extended lock to cover allreasoningMsgproperty mutations (ReasoningId,IsComplete,IsCollapsed,Timestamp,MergeReasoningContent)CompleteReasoningMessages: Moved property mutations (IsComplete,IsCollapsed,Timestamp) inside the lock block — previously captured reference list was mutated outside lock3. Server-side history cap (
MaxHistoryPayloadMessages = 500)SendSessionHistoryToClientenforces a server-side max regardless of client-requested limitLoadFullRemoteHistory(limit: null) is capped to 500 messages server-side4. Retry with cap for failed history requests (Review Fix)
SyncRemoteSessionsre-requests history for sessions that were previously requested but still have no local history5. MessageCount consistency (Review Fix)
OnContentReceivedandOnToolStartednow updatesession.MessageCountafterHistory.Addinside the lock — previously onlyOnReasoningReceivedupdated itBuildSessionsListPayloadand initial connect check useMessageCount(int field) instead ofHistory.Count(list enumeration)6. Avoid cross-thread History.Count reads
MessageCountfield instead ofHistory.Countto avoid racing with background writesReview History
Fix 5 thread-safety issues