Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

qtangs
Copy link
Contributor

@qtangs qtangs commented May 15, 2024

Resolve #828 by updating websocket's thread id header with currentThreadId to ensure session continuation after backend restart #996

Changes:

  • Update websocket headers to use currentThreadId when it's updated
  • Modify data_layer test to save thread history for reload after backend restarts
  • Add new test to ensure the thread is resumed properly with correct state and also ensure that new threads after restart work as before.

…th currentThreadId to ensure session continuation after backend restart
@willydouhard
Copy link
Collaborator

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

@qvalentin
Copy link
Contributor

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.

@qtangs
Copy link
Contributor Author

qtangs commented Jun 25, 2024

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
Copy link
Contributor Author

qtangs commented Jul 18, 2024

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

@dokterbob
Copy link
Collaborator

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

@qtangs
Copy link
Contributor Author

qtangs commented Aug 23, 2024

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:

setCurrentThreadId(event.thread_id);

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.

@kgeekInCominty
Copy link

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?

@dokterbob
Copy link
Collaborator

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

Copy link
Collaborator

@dokterbob dokterbob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replicated the issue and the solution works.

@dokterbob dokterbob merged commit 86798bc into Chainlit:main Aug 28, 2024
5 checks passed
@dokterbob
Copy link
Collaborator

@qtangs Thanks for your patience and contrib! ❤️

@qvalentin
Copy link
Contributor

Thanks for getting this over the finish line.

@qtangs
Copy link
Contributor Author

qtangs commented Aug 30, 2024

@qtangs Thanks for your patience and contrib! ❤️

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend/react-client does not resume session after backend connection loss
5 participants