chainlit icon indicating copy to clipboard operation
chainlit copied to clipboard

fix: resolve #828 by updating websocket's thread id header with currentThreadId to ensure session continuation after backend restart

Open qtangs opened this issue 9 months ago • 4 comments

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.

qtangs avatar May 15 '24 11:05 qtangs

This looks really nice, will run a couple more tests before merging. Thank you for your contribution!

willydouhard avatar May 27 '24 15:05 willydouhard

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.

qvalentin avatar Jun 11 '24 14:06 qvalentin

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.

qtangs avatar Jun 25 '24 03:06 qtangs

@willydouhard any chance this can get reviewed soon? :)

qtangs avatar Jul 18 '24 09:07 qtangs

@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).

dokterbob avatar Aug 19 '24 15:08 dokterbob

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.

qtangs avatar Aug 23 '24 10:08 qtangs

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?

kgeekInCominty avatar Aug 27 '24 19:08 kgeekInCominty

@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?

dokterbob avatar Aug 28 '24 09:08 dokterbob

@qtangs Thanks for your patience and contrib! ❤️

dokterbob avatar Aug 28 '24 09:08 dokterbob

Thanks for getting this over the finish line.

qvalentin avatar Aug 30 '24 06:08 qvalentin

@qtangs Thanks for your patience and contrib! ❤️

Wonderful. So glad that this still works. Thanks @dokterbob!

qtangs avatar Aug 30 '24 11:08 qtangs