fix(desktop): resync active chat after gateway reconnect - #44443
Conversation
|
Verified this fix — the |
alt-glitch
added
type/bug
|
@liuhao1024 What do we need to merge this? This would be a huge help, I've lost work enough to this already 🫠 |
After a successful reconnect, rehydrate the active transcript in addition to the sidebar/config so deltas emitted while the WebSocket was down do not require an app restart.
2efd125 to
53af4f5
Compare
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real Desktop recovery gap: current main refreshes config and session rows after the boot reconnect, but not the active transcript.
Problems
- The callback is only reached from
useGatewayBoot's reconnect loop.apps/desktop/src/app/gateway/hooks/use-gateway-request.ts:54-78can reconnect the same primary socket after an RPC failure; itsopenstate clears the boot loop timer atapps/desktop/src/app/gateway/hooks/use-gateway-boot.ts:338-343, so this path never invokes the new refresh. hydrateFromStoredSession()replacesmessagesafter its REST request (apps/desktop/src/app/desktop-controller.tsx:508-515), while the socket event handler is already subscribed (apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts:359). A live event received while that request is pending can be overwritten by the older snapshot.
Suggested changes
- Centralize the post-primary-reconnect reconciliation so both reconnect paths use it.
- Guard the REST snapshot against newer live session updates, and cover both the request-reconnect path and the live-event-during-hydrate race.
Automated hermes-sweeper review.
| // otherwise missing from the renderer until a full app restart. | ||
| await callbacksRef.current.refreshHermesConfig().catch(() => undefined) | ||
| await callbacksRef.current.refreshSessions().catch(() => undefined) | ||
| await callbacksRef.current.refreshActiveSession?.().catch(() => undefined) |
There was a problem hiding this comment.
This hook is not the only primary reconnect path: useGatewayRequest calls existing.connect() directly after a failed RPC (apps/desktop/src/app/gateway/hooks/use-gateway-request.ts:54-78). That successful open clears this hook's scheduled timer, so this callback never runs for that reconnect. Please move the reconciliation to a shared primary-reconnect transition or cover the request-triggered path too.
| onGatewayReady: g => { | ||
| gatewayRef.current = g | ||
| }, | ||
| refreshActiveSession: () => hydrateFromStoredSession(3), |
There was a problem hiding this comment.
hydrateFromStoredSession() awaits REST and then replaces messages (desktop-controller.tsx:508-515), but the WebSocket listener is already live after reconnect. A delta received while that request is pending can be overwritten by the older snapshot. Please make the hydrate response generation-aware or otherwise preserve newer live updates.
teknium1
added
sweeper:risk-session-state
GottZ
left a comment
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Three PRs address distinct Desktop gateway-state failure modes: #44443 rehydrates the active transcript after boot-loop reconnects, #50529 seeds the saved profile before cold-boot connection discovery, and #62624 deduplicates concurrent primary soft re-homes and renderer soft switches. Their diffs touch a shared gateway-boot area but do not implement the same fix.
Related pull requests
- #44443
related— (+34/-1) — keep open for revision: the diff adds active-session rehydration only to the boot reconnect loop, so it misses primary-socket reconnects initiated byuse-gateway-request; it also allows an in-flight REST snapshot to overwrite newer live events. This agrees with the keep_open review on #44443, whose two correctness concerns must be addressed before merge. - #50529
related— (+30/-4) — keep open for revision: the diff seeds the saved profile beforedesktop.getConnection(), addressing the cold-boot routing cause, but duplicates profile-adoption logic instead of using the current helper and its test asserts only after all asynchronous work has flushed. This agrees with the keep_open review on #50529: rebase ontoadoptPrimaryProfile()and prove the ordering with a deferredgetConnection(). - #62624
related— (+81/-4) — merge: the diff serializes concurrent global/primary applies through one in-flight re-home promise and independently guards renderersoftSwitch()reentrancy; its regression test verifies that duplicate applied events cause only one additional connection lookup. The contributor keep_open review on #62624 found no substantive correctness issue and rated the change highly salvageable.
Suggested consolidation
Merge #62624 after bringing it current and passing the applicable checks; it directly and comprehensively fixes the concurrent soft-rehome race. Keep #44443 and #50529 open for the specific revisions identified in their contributor reviews; none of #44443, #50529, or #62624 should be closed as a duplicate because they address separate causes.
Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 15 kB of PR diffs, 7 kB of issue/PR text, 5 kB of discussion (9 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
5 participants
Summary
Test Plan