fix: SSE reconnect followup - reset refs, prevent duplication, sort active requests#7992
fix: SSE reconnect followup - reset refs, prevent duplication, sort active requests#7992jh-block wants to merge 4 commits into
Conversation
… active requests - Clear all request-tracking refs (active, pending, buffer) in the session-load effect cleanup so switching sessions doesn't leak state from the previous session. - Clear active request state in reloadConversation so the SSE reconnect doesn't reattach and replay deltas on top of the freshly-loaded conversation after a buffer overflow. - Sort ActiveRequests IDs lexicographically (uuidv7 is time-ordered) to always pick the newest reply, not an arbitrary HashMap key.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b336664e8d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ch during reload - Extract request-tracking ref cleanup into a separate useEffect so it runs on ALL session changes, including cache hits (previously the cleanup was inside the resumeAgent branch and skipped for cached sessions) - Add reloadingConversationRef flag to suppress ActiveRequests reattach while getSession is in flight during buffer overflow recovery, preventing replayed deltas from being appended to stale messages
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4471aacaad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ActiveRequests is only sent once per SSE connection (before replay). If it arrives while reloadConversation's getSession is in flight, we now buffer events using the same cold-mount pattern instead of discarding the request IDs. Once the reload completes, the deferred reattach runs with the fresh conversation snapshot.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56ed38bb2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const pendingRequestId = pendingReattachRequestIdRef.current; | ||
| if (pendingRequestId) { | ||
| doReattachRef.current?.(pendingRequestId, messages); |
There was a problem hiding this comment.
If the user switches from session A to session B while reloadConversation() for A is still waiting on getSession(), these lines read pendingReattachRequestIdRef and doReattachRef after those shared refs have already been repointed at B by the new render. Unlike the resumeAgent load effect later in this file, reloadConversation has no cancellation/session guard, so A's stale reload can invoke doReattach for B with A's message snapshot and corrupt the newly opened chat.
Useful? React with 👍 / 👎.
| }).catch((e) => { | ||
| console.warn('Failed to reload conversation after buffer overflow:', e); | ||
| reloadingConversationRef.current = false; |
There was a problem hiding this comment.
By the time execution reaches this catch, reloadConversation() has already unsubscribed the active processor and cleared the request refs. If /sessions/{id} transiently fails during buffer-overflow recovery, the replacement listener created by ActiveRequests is still only appending to pendingReattachBufferRef, but this path never calls doReattach or restores the old listener. Because ActiveRequests is only sent once per SSE connection, the in-flight reply can stay permanently stuck until the user remounts the session.
Useful? React with 👍 / 👎.
- Capture sessionId at reload start and guard the .then()/.catch() callbacks so a stale reload from session A can't corrupt session B after a session switch - On getSession failure, attempt best-effort reattach with current messages so a transient error doesn't permanently lose the in-flight reply
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 351e17150b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (messages.length > 0) { | ||
| dispatch({ type: 'SET_MESSAGES', payload: messages }); | ||
| } | ||
| reloadingConversationRef.current = false; |
There was a problem hiding this comment.
Fresh evidence in this follow-up patch: reloadingConversationRef is cleared here before React has committed the SET_MESSAGES dispatch, and stateRef.current is only refreshed on the next render (ui/desktop/src/hooks/useChatStream.ts:377-380). If the reconnected SSE delivers ActiveRequests in that gap, the handler at ui/desktop/src/hooks/useChatStream.ts:593-615 stops buffering and calls doReattach(...) with the stale pre-overflow messages, so replayed deltas get appended to the wrong snapshot again.
Useful? React with 👍 / 👎.
| // Session switched while we were reloading — discard stale result | ||
| if (reloadSessionId !== sessionIdRef.current) return; |
There was a problem hiding this comment.
This guard only compares the stored session ID to sessionIdRef.current, so a getSession() started during an earlier visit to session A is still accepted if the user navigates A → B → A before it resolves. Unlike the resumeAgent effect below, there is no per-load cancellation token, so that late callback can overwrite the newer A state with an outdated snapshot and optionally run a deferred reattach against the wrong generation of the chat.
Useful? React with 👍 / 👎.
|
Closing in favour of a fresh PR that addresses the remaining Codex P1 issues (React commit timing race and A→B→A generation counter). Thanks for the work on this — the session-switch cleanup and overflow guard approach are the right direction. |
2 participants
Summary
Follow-up fixes to #7834 (SSE reconnect):
reloadConversationso the SSE reconnect doesn't reattach and replay deltas on top of the freshly-loaded conversation after a buffer overflow