Core: Fix manager overload and false "Server timed out" disconnects during large test runs#35429
Core: Fix manager overload and false "Server timed out" disconnects during large test runs#35429yannbf wants to merge 3 commits into
Conversation
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds local delivery for universal store events in the Vitest boot path, changes websocket heartbeat timeout handling to tolerate delayed timers, and throttles story index rebuilds triggered by status updates. ChangesUniversal Store Event Routing
WebSocket Heartbeat Starvation Grace
Status-Change Rebuild Throttling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ChildProcess
participant BootTestRunner
participant Channel
participant Transport
ChildProcess->>BootTestRunner: message (UNIVERSAL_STORE event)
BootTestRunner->>BootTestRunner: check UNIVERSAL_STORE_EVENT_NAMES
BootTestRunner->>Channel: channel.receive(event)
Channel->>Channel: handleEvent (local listeners only)
ChildProcess->>BootTestRunner: message (other-event)
BootTestRunner->>Channel: channel.emit(type, args)
Channel->>Transport: transport.send(event)
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
d7a8cbc to
fb2a3e2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager-api/tests/stories.test.ts (1)
2064-2111: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftReal-timer-based throttle assertion risks CI flakiness.
The stress test waits on real wall-clock time (
setTimeout(resolve, 1100)) to let the 1000ms throttle window settle before assertingsetIndexSpy.mock.calls.length <= 4. Under a loaded CI runner, GC pauses or scheduling delays could push the burst's synchronous processing time (50fullStatusStore.setcalls, each triggering real listener/index logic) past the throttle window, causing intermittent extra rebuild cycles and flaky failures — the reverse can also happen if the sleep resolves before the trailing edge fires. Usingvi.useFakeTimers()withvi.advanceTimersByTimeAsync()would make the throttle boundary deterministic instead of relying on generous real-time slack.🤖 Prompt for 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. In `@code/core/src/manager-api/tests/stories.test.ts` around lines 2064 - 2111, The stress test in stories.test.ts is using a real 1100ms sleep to wait for the throttle window, which can be flaky under CI timing jitter. Update the test around initStories, api.setIndex, and the setIndexSpy/assertion block to use vi.useFakeTimers() and advance time deterministically with vi.advanceTimersByTimeAsync() instead of wall-clock waiting. Make sure the burst of fullStatusStore.set updates is followed by explicit timer advancement so the trailing-edge rebuild is observed before asserting the bounded call count.
🤖 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.
Nitpick comments:
In `@code/core/src/manager-api/tests/stories.test.ts`:
- Around line 2064-2111: The stress test in stories.test.ts is using a real
1100ms sleep to wait for the throttle window, which can be flaky under CI timing
jitter. Update the test around initStories, api.setIndex, and the
setIndexSpy/assertion block to use vi.useFakeTimers() and advance time
deterministically with vi.advanceTimersByTimeAsync() instead of wall-clock
waiting. Make sure the burst of fullStatusStore.set updates is followed by
explicit timer advancement so the trailing-edge rebuild is observed before
asserting the bounded call count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfe515fe-5d4f-43bd-afc9-48391b633f5f
📒 Files selected for processing (7)
code/addons/vitest/src/node/boot-test-runner.test.tscode/addons/vitest/src/node/boot-test-runner.tscode/core/src/channels/main.tscode/core/src/channels/websocket/index.test.tscode/core/src/channels/websocket/index.tscode/core/src/manager-api/modules/stories.tscode/core/src/manager-api/tests/stories.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/channels/main.ts
- code/core/src/manager-api/modules/stories.ts
- code/addons/vitest/src/node/boot-test-runner.test.ts
- code/core/src/channels/websocket/index.ts
…eat starvation, and duplicated store broadcasts These tests document the behavior behind the false "Server timed out" disconnect during large addon-vitest test runs. Six of them fail at this commit; the next commit makes them pass: - manager-api: a stream of status updates must trigger a bounded number of full index rebuilds, and must not re-register the status filter (which triggers a second full rebuild per update). - channels/websocket: a heartbeat timer that fires late (main thread starved by a message backlog, GC, or heavy render) must not close a healthy connection with 3008. - addon-vitest: universal store events from the vitest child process must reach browser clients exactly once — not twice (the double broadcast this fixes) and not zero times (the leader-forward test pins the one remaining wire copy). A seventh test (trailing throttle edge applies the last status update) passes here and pins the throttle semantics the fix introduces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…at, dedupe test store events - Manager: remove the redundant recomputeStatusFilter() from the onAllStatusChange handler (the registered status filter reads statuses live, so re-setting the index is enough) and throttle status-driven index rebuilds — during test runs statuses stream in continuously and each rebuild re-transforms the entire index twice. - Channels: make the WebsocketTransport heartbeat starvation-aware — when the watchdog timer fires late because the main thread was blocked (message backlog draining, GC, heavy renders), queued pings may not have been processed yet, so grant one grace window instead of closing a healthy connection with 3008. - Addon-vitest: deliver universal store events from the vitest child process through the channel's receive path instead of emit, so they are broadcast to browser clients exactly once (the store leader's forward) instead of twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fb2a3e2 to
050ea58
Compare
Closes #
What I did
When you run tests on a project with many stories, the manager shows "Server timed out — Please restart your Storybook server and reload the page" partway through the run. The server is actually fine — the manager kills its own connection because it can't keep up. Step by step:
onAllStatusChangehandler, and once more viarecomputeStatusFilter()→experimental_setFilter(), which runs its ownsetIndex(). On a ~3,600-story project the manager can't process events as fast as they arrive, so its UI falls minutes behind the actual run.channel.emit, and once more by the store leader's own event forwarding.Three fixes, one per problem:
Channel.receive()— dispatch to local listeners without re-broadcasting — leaving the store leader's forward as the single copy browsers receive.Measured on a 3,583-test project (coverage on, manager attached):
The canary built from this branch was verified end-to-end against the same project: full run completed, zero disconnects on both the manager and preview sockets.
Commit structure
The first commit adds seven tests, six of which fail on
next— they document the bug (bounded rebuilds under a status-update burst, no filter re-registration, no close on a late-firing heartbeat, and store events reaching clients exactly once: the last one uses a real leader store, so it fails both when events are duplicated and if they ever stop being forwarded at all). The second commit is the fix that makes them pass. The suite was hardened with mutation testing — e.g. removing the throttle's trailing edge (which would silently lose the final second of statuses in every run) fails a test deterministically.Known pre-existing issues in the touched code, left for follow-ups
setIndexcallers can race (a status-driven rebuild reading a not-yet-committed index from a concurrentfetchIndexcan win the commit order). This predates the PR — it used to happen per status event, now at most once per second — and self-heals on the next index invalidation. A generation guard insetIndexwould fix it for all callers.bootTestRunnerregistersprocess.on(exit/SIGINT/SIGTERM)and aFATAL_ERRORsubscription on every child (re)boot without ever removing them, so crash-looping children accumulate listeners.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
storybook-server-channelWS connection — eachUNIVERSAL_STORE:*frame should appear once during a run, not twice back to back.Documentation
MIGRATION.MD
Checklist for Maintainers
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.tsqa:neededorqa:skipThis pull request has been released as version
0.0.0-pr-35429-sha-df3a9be0. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35429-sha-df3a9be0 sandboxor in an existing project withnpx storybook@0.0.0-pr-35429-sha-df3a9be0 upgrade.More information
0.0.0-pr-35429-sha-df3a9be0yann/fix-test-run-status-flooddf3a9be01783951852)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=35429🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests