Skip to content

feat(apps/hermes): add connection timeout for SSE & WebSocket connections #2612

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

Merged
merged 10 commits into from
Apr 28, 2025

Conversation

cctdaniel
Copy link
Contributor

@cctdaniel cctdaniel commented Apr 24, 2025

Summary

  • Added a 24-hour connection timeout for SSE endpoint
  • Added a 24-hour connection timeout for WS endpoint
  • Each client connection will automatically disconnect after 24 hours of continuous connection, with a proper error message sent before closing.

Rationale

  • Prevents resource leaks from indefinite connections
  • Ensures proper cleanup of long-running connections
  • Each client gets their own independent 24h window from their connection time
  • Provides clear error messaging to clients about connection termination

How has this been tested?

  • Current tests cover my changes
  • Added new tests
  • Manually tested the code

Testing steps:

  1. Verified that each new SSE/WS client connection gets its own independent 24h timeout
  2. Confirmed that the timeout message is properly sent before connection close
  3. Tested that the stream properly terminates after the timeout period
  4. Verified that existing functionality (price updates, error handling) continues to work during the connection lifetime
  5. Tested that multiple concurrent clients have independent timeouts based on their individual connection times

Copy link

vercel bot commented Apr 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Apr 24, 2025 11:46am
component-library ⬜️ Ignored (Inspect) Visit Preview Apr 24, 2025 11:46am
entropy-debugger ⬜️ Ignored (Inspect) Visit Preview Apr 24, 2025 11:46am
insights ⬜️ Ignored (Inspect) Visit Preview Apr 24, 2025 11:46am
proposals ⬜️ Ignored (Inspect) Visit Preview Apr 24, 2025 11:46am
staking ⬜️ Ignored (Inspect) Visit Preview Apr 24, 2025 11:46am

@cctdaniel cctdaniel changed the title feat(ws): add connection timeout for WebSocket connections feat(apps/hermes): add connection timeout for WebSocket connections Apr 24, 2025
@cctdaniel cctdaniel changed the title feat(apps/hermes): add connection timeout for WebSocket connections feat(apps/hermes): add connection timeout for SSE & WebSocket connections Apr 24, 2025
Copy link
Collaborator

@ali-behjati ali-behjati left a comment

Choose a reason for hiding this comment

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

Please address my comment before merging and bump the version. Also please update the docs (for SSE at least) to document this behaviour.

Copy link
Contributor

@tejasbadadare tejasbadadare left a comment

Choose a reason for hiding this comment

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

nice! one thing i'm wondering about is that there are many clients out there that maintain open connections to hermes. when we deploy this change, they'll all reconnect and start their connection TTLs at the same time. then every 24h there will be a thundering herd of reconnections. we could add a jitter to the 24h to avoid this (give them a variable extra 1-5m after the 24h).

in reality, many of them will drop their connections naturally due to network conditions before 24h elapses, and hermes can probably handle the thundering herd anyway, but just something to be aware of.

@cctdaniel cctdaniel merged commit 7cd8977 into main Apr 28, 2025
9 checks passed
@cctdaniel cctdaniel deleted the hermes-ws branch April 28, 2025 02:08
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.

3 participants