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

Display connection rejection errors passed to client #6101

Conversation

raymyers
Copy link
Contributor

@raymyers raymyers commented Jan 7, 2025

End-user friendly description of the problem this fixes or functionality that this introduces

  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

Give a summary of what the PR does, explaining any non-trivial design decisions

In our backend, we raise ConnectionRefusedError. SocketIO passes that message to the client. The client did ignore it but will now display it to the user.

Open questions

We may need to find a way to prevent the same message from displaying repeatedly, if so we can handle this more generally later, e.g. by showing repeated messages with a count.

This does not give the message an id, which appears to be for i18n. However some of the other uses seem to omit id as well so the type has been updated to more accurately reflect this as an optional field.


Link of any specific issues this addresses

ALL-959

@raymyers raymyers marked this pull request as draft January 7, 2025 02:19
@raymyers raymyers force-pushed the ray/all-959-user-story-1-connection-rejections branch 3 times, most recently from c66baf3 to c26e9fd Compare January 7, 2025 02:39
@raymyers raymyers force-pushed the ray/all-959-user-story-1-connection-rejections branch from c26e9fd to 505f9ed Compare January 7, 2025 02:43
@raymyers raymyers marked this pull request as ready for review January 7, 2025 19:15
@enyst enyst requested a review from amanape January 7, 2025 19:18
@raymyers raymyers force-pushed the ray/all-959-user-story-1-connection-rejections branch from ec900cb to c03344e Compare January 7, 2025 19:20
@raymyers
Copy link
Contributor Author

raymyers commented Jan 7, 2025

To trigger this on demand, your can add this listen_socket.py in connect(). Potentially we could use flags to enable faults like this in integration tests.

    import random
    if random.random() < 0.2:
        raise ConnectionRefusedError('Dev injected random failure')

frontend/src/context/ws-client-provider.tsx Outdated Show resolved Hide resolved
@raymyers
Copy link
Contributor Author

raymyers commented Jan 7, 2025

I think e2e ran successfully before, it might just need to rerun

@amanape
Copy link
Member

amanape commented Jan 7, 2025

Yes it happens sometimes

@raymyers
Copy link
Contributor Author

raymyers commented Jan 7, 2025

Yes it happens sometimes

Does it just retry on its own or is there something we press to restart it?

@amanape
Copy link
Member

amanape commented Jan 7, 2025

Click on the failed job -> scroll up and you'll see something like "Re-run jobs" on the top right. Click it and the "Re-run failed jobs only" option.

I've already done it by the time you've commented 😃

@amanape amanape merged commit 561f308 into All-Hands-AI:main Jan 7, 2025
15 checks passed
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.

2 participants