Skip to content

fix(client): tolerate slow event connections - #44301

Open
Brendonovich wants to merge 1 commit into
v2from
tolerate-slow-events
Open

fix(client): tolerate slow event connections#44301
Brendonovich wants to merge 1 commit into
v2from
tolerate-slow-events

Conversation

@Brendonovich

Copy link
Copy Markdown
Member

Summary

  • extend the initial SSE handshake timeout from 2 seconds to 30 seconds
  • prevent slow or high-latency connections from repeatedly cancelling /api/event before server.connected arrives
  • retain the existing reconnect behavior for failed streams

Testing

  • bun typecheck (packages/client)
  • bun typecheck (packages/app)
  • pre-push workspace typecheck
  • bun test (packages/client): 61 passed; one unrelated generated-client surface assertion fails because question is absent

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

Correct and minimal fix. I traced the timer lifecycle: the timeout is armed per attempt (connection.ts:67), cleared on server.connected (:82–85), and again in the finally block (:109), so there's no leak and a successful slow handshake is unaffected afterwards. Retrying with the existing 1s reconnectDelay still works because the timeout only bounds "TCP open but no server.connected", not fast network-refused failures.

Two things to consider:

  1. The new bound also applies to reconnect attempts. connectTimeout gates every attempt in runStream, not just the initial handshake. When a server hangs (accepts TCP but never sends) mid-session, the status now sits in connecting/reconnecting for up to ~31s per attempt instead of ~3s before the first retry. That's the right trade-off for slow links, but worth confirming the UI communicates that state well over 30 seconds — users may assume the app is frozen rather than connecting.

  2. Consider making it tunable. ClientConnectionOptions already carries knobs like flushInterval. Exposing connectTimeoutMs there (defaulting to 30_000) would let desktop/remote-SSH deployments tune it without another code change — this is the second latency-related tweak to this file, suggesting the value is environment-dependent.

Minor: a one-line comment on the constant explaining the rationale ("high-latency / remote backends can exceed 2s before server.connected") would prevent a future reader from "tightening" it back.

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