Repository navigation
Core: Remove client-side WebSocket timeout detection - #36244
Conversation
A 20s receive watchdog on the manager main thread measured browser busyness, not a dead server, and closed healthy connections with 3008. Server crashes already close the socket. Keep ping/pong for proxy keepalive. Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe WebSocket transport no longer implements heartbeat timers or timeout closures. Related exports, tests, Node transport configuration, and manager disconnect handling now use generic connection-loss behavior. ChangesWebSocket heartbeat removal
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The manager no longer closes healthy WebSocket connections based on delayed browser timer execution, while retaining generic connection-loss reporting and ping/pong keepalive behavior. No concrete current-head merge risk remains. ✨ Finishing Touches📝 Generate docstrings
Comment |
kasperpeulen
left a comment
There was a problem hiding this comment.
LGTM. This is the right fix: the 20s watchdog measured how busy the client was, not whether the server was alive. Keeping the server ping and client pong for proxies is fine.
Closes #
Related: #36229 (perf fixes stay there; this is the timeout-detection follow-up from review).
What I did
Removed the client-side WebSocket receive watchdog that closed
/storybook-server-channelwith code 3008 and showed Server timed out.That 20s
setTimeoutran on the manager main thread. A busy UI (large Component Testing run, webpack compile, CLI docgen) made the timer fire late, so a healthy server looked dead. The original user report this code was added for (#28258, shipped in #30288) was a false disconnect during webpack, not a hung TCP connection.A crashed or killed server already closes the socket. The manager still shows Connection lost on
CHANNEL_WS_DISCONNECT. Server JSONpingevery 15s and clientpongstay in place so idle proxies and NATs do not drop a quiet tab.Also removed:
enableHeartbeat,pauseHeartbeat, andresumeHeartbeat(the CLI already had to disable the watchdog because the same event loop blocked past 20s)ServerTimedOutstoryHEARTBEAT_MAX_LATENCYexportThe leftover gap is a process that is frozen (
SIGSTOP, deadlock) with TCP still open. That case was never the filed bug, and this detector could not tell it apart from a busy manager.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
yarnthenyarn nx compile core.cd code && yarn storybook:ui.storybook-server-channelWebSocket. Confirm{type:"ping"}still arrives from the server about every 15s and the client replies{type:"pong"}.Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.