chainlit
chainlit copied to clipboard
fix: resolve #828 by updating websocket's thread id header with currentThreadId to ensure session continuation after backend restart
Resolve #828 by updating websocket's thread id header with currentThreadId to ensure session continuation after backend restart #996
Changes:
- [x] Update websocket headers to use currentThreadId when it's updated
- [x] Modify data_layer test to save thread history for reload after backend restarts
- [x] Add new test to ensure the thread is resumed properly with correct state and also ensure that new threads after restart work as before.
This looks really nice, will run a couple more tests before merging. Thank you for your contribution!
In case anyone is also waiting for this: If you are building a custom frontend, you can implement this change in your react code. Just use the useChatSession
hook to get the socket.io session.
This looks really nice, will run a couple more tests before merging. Thank you for your contribution!
@willydouhard I've resolved the merge conflict with the latest version. Please review soon to avoid another conflict. Thanks.
@willydouhard any chance this can get reviewed soon? :)
@qtangs I just tried to replicate the issue but I was unable to reliably do so.
First, I followed steps in your bug report -- but I guess we need to have history enabled?
So then I used the resume-chat example from the cookbook (https://github.com/Chainlit/cookbook/tree/main/resume-chat).
I first tried with main, with which I did indeed notice that interrupting a session, then writing a message after a reconnect yields in a second conversation.
Then I tried with your PR branch, only to yield the same result.
In addition, in both cases, after the reconnect I am somehow presented with an explicit 'Resume chat' options. Which actually to me seems to be the intended behaviour, albeit quirky UX.
Could you please help me gain clarity, or perhaps even supply a test project, for me to consistently replicate the issue (on main) only to find it resolved on your branch?
Happy to help this (finally!) move forward (I've joined the team in order to offload Willy a bit on maintenance).
Thanks @dokterbob and welcome onboard if I haven't said so :). Glad to know that the team is growing to make chainlit even more awesome.
This PR was done a while ago, at that time the data_layer
test (resuming after a backend restart) were passing. I guess due to some updates in Chainlit, the changes here no longer pass that test, likely due this code not being executed:
https://github.com/Chainlit/chainlit/blob/9d6821558bdd57e1e88180f1b440699605974174/libs/react-client/src/useChatSession.ts#L153
So this code updated the thread id header wasn't executed:
// Use currentThreadId as thread id in websocket header
useEffect(() => {
if (session?.socket) {
session.socket.io.opts.extraHeaders!['X-Chainlit-Thread-Id'] =
currentThreadId || '';
}
}, [currentThreadId]);
I'll try to look into it when I have time. In the meantime, if anyone else can help follow up on this that'd be great. Maybe the correct place to update the header is somewhere else.
In case anyone is also waiting for this: If you are building a custom frontend, you can implement this change in your react code. Just use the
useChatSession
hook to get the socket.io session.
Hey @qvalentin thanks for your message, we are also building a custom frontend would you mind sharing how you managed to solve it directly in your react code, please ? And Thanks.
ps: Did you mean to use the idToResume
check instead the threadId
?
@qvalentin You seem to have a lot of grip on this issue. Any chance you might be able to double check whether it resolves it for you?
@qtangs Thanks for your patience and contrib! ❤️
Thanks for getting this over the finish line.
@qtangs Thanks for your patience and contrib! ❤️
Wonderful. So glad that this still works. Thanks @dokterbob!