Core: Reset WebsocketTransport heartbeat on every message, not just ping#35422
Conversation
Fixes the "Server timed out" manager disconnect on large addon-vitest runs (see #35400) at its root, in the shared channel transport rather than only reordering ping handling: any incoming message proves the connection is alive, so the heartbeat now resets before the (possibly slow) channel handler runs for every message, not only for a literal ping. This also closes a gap the narrower ping-only fix leaves open - if a backlog is deep enough that the next ping itself is stuck behind many other queued messages, resetting only on ping doesn't help, since nothing about processing those other messages pushes the deadline out.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesWebSocket heartbeat and ping handling
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/channels/websocket/index.test.ts`:
- Line 50: The WebSocket test is stubbing a global with vi.stubGlobal but never
restoring it, which can leak MockWebSocket into other tests. Update the
websocket test setup in index.test.ts so the stubbed WebSocket global is cleaned
up by adding vi.unstubAllGlobals() in the existing afterEach alongside
vi.useRealTimers() and vi.clearAllMocks(), and keep the stub usage centralized
around MockWebSocket so the suite leaves no ambient globals behind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75bc0610-feeb-49a0-8bb1-523bffa36a63
📒 Files selected for processing (2)
code/core/src/channels/websocket/index.test.tscode/core/src/channels/websocket/index.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
@valentinpalkovic, this replaces my PR, right? Shall I close it? |
|
Yes, though it needs some testing. Since you have replicated the error, can you try out the canary release (which I am triggering now)? |
Move the `WebSocket` global stub out of module scope into `beforeEach` and add `vi.unstubAllGlobals()` to `afterEach`, so the suite no longer leaves an ambient global behind for other test files in the same worker. CodeRabbit's proposed fix (only adding `vi.unstubAllGlobals()` while keeping the stub at module scope) would break the suite: after the first test's cleanup the stub is gone and subsequent tests construct a real WebSocket, so `socketRef` is never populated. Re-stubbing per test keeps all 7 tests passing while complying with the repo's global-stubbing guideline. Co-authored-by: Cursor <cursoragent@cursor.com>
…artbeat-reset-on-any-message
|
I was still testing this @Sidnioulz, I'll need to do a follow-up, as this PR alone did not resolve the problem fully |
Manual testing
N?A
What I did
Related to #35400, which fixes a "Server timed out" manager disconnect during large
@storybook/addon-vitestruns. That PR's core-channel fix reordersWebsocketTransport.onmessageto check forpingand reset the heartbeat before calling the (potentially slow, when the channel is busy) channel handler, instead of after.This PR implements that same underlying fix directly against
next(code/core/src/channels/websocket/index.ts, which doesn't yet have #35400's change), but generalizes it one step further: the heartbeat now resets on every incoming message, not only onping.Why: any message arriving over the socket is itself proof the connection is alive - the server is actively sending data and it's reaching this tab. Gating the reset on the literal
pingtype leaves a gap: if the message backlog in front of the next ping is deep enough (many status-update events, each requiring real synchronous work to handle), the ping can still be delayed past the timeout window even while the connection is constantly, verifiably receiving traffic. Resetting on every message closes that gap directly, since forward progress through the backlog - of any kind - is what actually matters, not whether apinghappens to be interspersed in it.Notes for reviewers
websocket/index.test.ts(which doesn't exist onnextyet): timeout-close, ping reset + pong reply, ping not forwarded to the handler, non-ping forwarded, heartbeat reset survives a throwing handler - plus two new tests specifically for the generalized behavior: a non-ping message also resets the heartbeat, and does so even if the handler throws while processing it.STORYBOOK_TEST_STATUS_FLUSH_INTERVALenv var or anything from Addon-Vitest: Prevent manager "Server timed out" during large test runs #35400/Generic heartbeat hardening on top of the Vitest addon timeout fix #35419 - this is meant to be a minimal, independently mergeable improvement to the shared transport.Testing
websocket/index.test.ts, all passing)yarn nx run core:checkpasses with no type errors. Formatted withoxfmt; lint has no new warnings beyond what already existed on this file.Summary by CodeRabbit
Summary by CodeRabbit
pingmessages are now auto-answered withpongand are no longer delivered to the message handler.pingno longer triggers handler errors; non-pingmessages still forward normally (including error propagation).ping/pongbehavior, forwarding rules, and handler error handling.