- Notifications
You must be signed in to change notification settings - Fork17
Zg/resume on reconnect#330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…AttemptReconnectHook to its options, update handle close to reconnect based on shouldAttemptReconnectHook if provided
…ting the resumed_chat_group_id query param when receiving a chat_metadata message for resume on reconnect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. We should add a test or manually test both paths before release though
| } | ||
| } | ||
| constshouldResumeChat=args.shouldResumeChat??true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO defaulting to true is "breaking" enough that we should highlight it by releasing minor instead of patch wdyt?
zgreathouseApr 11, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@twitchard I agree! Although, I am now leaning toward defaulting tofalse.
It is likely true, in nearly all cases, that users would want to resume a Chat when reconnecting after an unexpected disconnect. However, this current implementation actually breaks when attempting to resume on reconnect if zero data retention is enabled.
There is currently no field in the API which we can leverage to determine whether or not zero data retention has been enabled on the account. Since we are always adding theresumed_chat_group_id query param toReconnectingWebSocket._queryParamOverrides upon receiving thechat_metadata message, when anew chat unexpectedly disconnects and we try to reconnect it will fail with aE0720 error instead of reconnecting. When receiving this error we need to remove the query param from theReconnectingWebSocket._queryParamOverrides so when it tries to reconnect it won't fail.
Apart from that fix, defaulting tofalse would make the changes here purely additive, while giving users the option to enable resuming on reconnect.
Uh oh!
There was an error while loading.Please reload this page.
…ault value to false.
…tReconnectEvi logic to only reconnect with abnormal close code
volskaya commentedApr 12, 2025
I feel like this might suffer from a server restriction, where it doesn't let you resume the group ID if it didn't detect a close, because a socket closing client side doesn't guarantee that it closed server side too. Like if there's a network drop or switch, or on mobile if the app running this code was suspended or some other factor broke it, leaving the server hanging waiting for response, until its own networking library times it out or the EVI's inactivity timeout gets triggered. 🙇In my opinion the server should allow the client to resume when ever it wants, the behavior should be that if it sees an attempt to establish a new websocket with an already used group id, the old connection should be disposed and it should proceed with the new one. It should let a test case like this pass to fully support resumes: it("Chat group ID can take over an older connection",async()=>{consthume=newHumeClient({accessToken:"<>",});letresolveNewerSocket:(socket:ChatSocket)=>void|undefined;letresolveSuccess:()=>void|undefined;letrejectSuccess:()=>void|undefined;constpromiseNewerSocket=newPromise<ChatSocket>((res)=>(resolveNewerSocket=res));constpromiseSuccess=newPromise<void>((res,rej)=>((resolveSuccess=res),(rejectSuccess=rej)));constolderSocket=hume.empathicVoice.chat.connect({debug:false,reconnectAttempts:30,});awaitolderSocket.tillSocketOpen();olderSocket.on("message",(message)=>{if(message.type==="chat_metadata"){resolveNewerSocket(hume.empathicVoice.chat.connect({debug:false,reconnectAttempts:30,resumedChatGroupId:message.chatGroupId,}),);}});constnewerSocket=awaitpromiseNewerSocket;newerSocket.on("close",()=>rejectSuccess());newerSocket.on("message",(message)=>{if(message.type==="chat_metadata")resolveSuccess();});awaitnewerSocket.tillSocketOpen();awaitpromiseSuccess;olderSocket.close();newerSocket.close();},100000); |
Uh oh!
There was an error while loading.Please reload this page.
Summary of Changes Made
EmpathicVoice Client
Adds built-in support for resuming a Chats when reconnecting
shouldAttemptReconnectcallback argument inReconnectingWebSocketfor implementing custom logic for when to try to reconnect.resumed_chat_group_idquery param to the handshake url to support resume on reconnect.)ChatSocketto extractchatGroupIdfrom chat_metadata message and update query param overrides onReconnectingWebSocketby default to support resume on reconnect.shouldResumeChatoption forChatClientChatclient to accept optionalshouldResumeChatargument (escape hatch for default resume on reconnect behavior)Chatclient for reconnect logic, only attempting a reconnect when WebSocket was closed with one of the followingWebSocket close codes:1006(Abnormal Closure)1011(Internal Error)1012(Service Restart)1013(Try Again Later)1014(Bad Gateway)