Generic heartbeat hardening on top of the Vitest addon timeout fix#35419
Generic heartbeat hardening on top of the Vitest addon timeout fix#35419valentinpalkovic wants to merge 1 commit into
Conversation
… flush interval Builds on #35400: bounds how many test-case results are applied per flush so a backlog can't turn into one oversized synchronous burst regardless of how STORYBOOK_TEST_STATUS_FLUSH_INTERVAL is tuned, and makes the dev-server's ServerChannelTransport opportunistically top up its heartbeat ping on any outgoing broadcast once one is overdue, so a busy relay path (or any other provider flooding the channel) can no longer starve ping delivery the way the ping-sending setInterval alone could.
|
📝 WalkthroughWalkthroughThis PR adds a configurable cap ( ChangesVitest Bounded Flush Batching
Estimated code review effort: 3 (Moderate) | ~25 minutes Server Channel Heartbeat Priority
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Reporter
participant TestManager
participant StatusStore
Reporter->>TestManager: await onTestRunEnd(endResult)
TestManager->>TestManager: cancel throttled flush
TestManager->>TestManager: flushTestCaseResults()
TestManager->>StatusStore: set(bounded batch)
loop while backlog remains
TestManager->>TestManager: await setImmediate
TestManager->>StatusStore: set(next bounded batch)
end
TestManager-->>Reporter: resolved
Reporter->>Reporter: clearVitestState()
sequenceDiagram
participant Caller
participant ServerChannelTransport
participant Client
Caller->>ServerChannelTransport: send(event)
alt event is ping
ServerChannelTransport->>Client: broadcast ping
else heartbeat overdue
ServerChannelTransport->>Client: broadcast ping
ServerChannelTransport->>Client: broadcast event
else not overdue
ServerChannelTransport->>Client: broadcast event
end
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/addons/vitest/src/node/test-manager.ts`:
- Around line 261-269: The backlog drain in flushTestCaseResults can stall
forever if an exception is thrown after items are spliced but before the next
setImmediate reschedule happens. Update TestManager.flushTestCaseResults (and
the setImmediate continuation around the drain loop) so errors are caught and
surfaced while still guaranteeing the drain chain completes or stops cleanly;
use the existing batchedTestCaseResults / maxTestCaseResultsPerFlush flow and
ensure the while loop in onTestRunEnd cannot spin indefinitely when a flush
fails.
🪄 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: ce7c35e8-d6bb-4e2c-b1a6-48c889680537
📒 Files selected for processing (6)
code/addons/vitest/src/constants.tscode/addons/vitest/src/node/reporter.tscode/addons/vitest/src/node/test-manager.test.tscode/addons/vitest/src/node/test-manager.tscode/core/src/core-server/utils/__tests__/server-channel.test.tscode/core/src/core-server/utils/get-server-channel.ts
| private flushTestCaseResults = () => { | ||
| const testCaseResultsToFlush = this.batchedTestCaseResults; | ||
| this.batchedTestCaseResults = []; | ||
| const testCaseResultsToFlush = this.batchedTestCaseResults.splice( | ||
| 0, | ||
| this.maxTestCaseResultsPerFlush | ||
| ); | ||
|
|
||
| if (testCaseResultsToFlush.length === 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Exception during backlog drain permanently stalls onTestRunEnd.
flushTestCaseResults splices items from the backlog first (Lines 262-265), then does status-store/store updates that can throw. If a self-rescheduled continuation (Line 371, setImmediate(this.flushTestCaseResults)) throws mid-drain, the if (batchedTestCaseResults.length > 0) reschedule check is never reached, so nothing will ever drain the remaining backlog again. Since the exception occurs inside a bare setImmediate callback (outside any promise onTestRunEnd awaits), it's never surfaced — instead the polling loop at Lines 383-385 (while (this.batchedTestCaseResults.length > 0) await ...) spins indefinitely, hanging the run-end flow (and, transitively, reporter.ts's await this.testManager.onTestRunEnd(...) and clearVitestState()).
Previously, an error during the single-shot flush would simply throw synchronously out of onTestRunEnd; this change silently converts that into a permanent hang, which is a more severe failure mode.
🛡️ Suggested guard to ensure the drain chain always terminates or surfaces
private flushTestCaseResults = () => {
const testCaseResultsToFlush = this.batchedTestCaseResults.splice(
0,
this.maxTestCaseResultsPerFlush
);
if (testCaseResultsToFlush.length === 0) {
return;
}
-
- const componentTestStatuses = testCaseResultsToFlush.map(({ storyId, testResult }) => ({
- ...
- }));
- ...
- if (this.batchedTestCaseResults.length > 0) {
- setImmediate(this.flushTestCaseResults);
- }
+ try {
+ const componentTestStatuses = testCaseResultsToFlush.map(({ storyId, testResult }) => ({
+ ...
+ }));
+ ...
+ } catch (err) {
+ this.reportFatalError('Failed to flush test case results', err as Error);
+ } finally {
+ if (this.batchedTestCaseResults.length > 0) {
+ setImmediate(this.flushTestCaseResults);
+ }
+ }
};Also applies to: 369-385
🤖 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/addons/vitest/src/node/test-manager.ts` around lines 261 - 269, The
backlog drain in flushTestCaseResults can stall forever if an exception is
thrown after items are spliced but before the next setImmediate reschedule
happens. Update TestManager.flushTestCaseResults (and the setImmediate
continuation around the drain loop) so errors are caught and surfaced while
still guaranteeing the drain chain completes or stops cleanly; use the existing
batchedTestCaseResults / maxTestCaseResultsPerFlush flow and ensure the while
loop in onTestRunEnd cannot spin indefinitely when a flush fails.
1 participant
What I did
Stacked on top of #35400 (
norbert/fix-timeout-vitest-addon), which fixed the "Server timed out" disconnect on large@storybook/addon-vitestruns by reorderingWebsocketTransport.onmessageto handlepingbefore the channel handler. That fix, plus aSTORYBOOK_TEST_STATUS_FLUSH_INTERVALenv var to reduce flush frequency, is a solid, low-risk improvement on its own.This PR adds two more generic changes on top, so the heartbeat is resilient to any provider flooding the channel, not just addon-vitest, and without needing per-project tuning:
1. Bound how many test-case results are applied per flush (
code/addons/vitest/src/node/test-manager.ts,constants.ts)Previously a single flush applied the entire accumulated batch.
flushTestCaseResultsnow takes at mostmaxTestCaseResultsPerFlush(default 200) per call, draining any remainder viasetImmediatein bounded slices instead of one unbounded burst.onTestRunEndis nowasyncand waits for the drain to fully complete (also in bounded slices) before the run is considered finished, instead of flushing everything synchronously at the end.This makes
STORYBOOK_TEST_STATUS_FLUSH_INTERVALoptional rather than load-bearing: raising the interval no longer trades "queue depth risk" for "bigger single-burst risk," because no single flush can grow unbounded regardless of how much backlog accumulates.2. Give the dev-server's heartbeat ping priority over relay traffic (
code/core/src/core-server/utils/get-server-channel.ts)ServerChannelTransportsends its keep-alive ping via a baresetInterval, in the same Node process that relays every event from a Vitest child process (or any other provider) to connected clients. If that relay work is heavy enough for long enough, the scheduled ping can slip on the server side too - the mirror image of the client-side bug #35400 fixes, just unaddressed until now.send()now opportunistically broadcasts a ping first (and updates its own "last ping sent" bookkeeping) whenever it's asked to relay a non-ping event and a fullHEARTBEAT_INTERVALhas elapsed since the last one. Sincesend()is called far more often than the interval ticks during a busy relay, heartbeat delivery no longer depends on that interval callback getting a turn on a busy event loop - it piggybacks on whatever traffic is already flowing. The existingsetIntervalis left untouched as the baseline driver for idle periods.Notes for reviewers
this.throttledFlushTestCaseResults()) - doing so can synchronously re-enterflushTestCaseResultswhen the throttle's interval has already elapsed, draining multiple chunks in one burst instead of yielding between them. Caught this empirically during development; there's a dedicated regression test for it (test-manager.test.ts: "drains at most one bounded chunk per elapsed interval...").code/addons/vitest/src/node/test-manager.test.ts(bounded flush + run-end drain),code/core/src/core-server/utils/__tests__/server-channel.test.ts(opportunistic heartbeat priority, 3 new tests usingvi.setSystemTime()to jump the clock without advancing timers, isolating the new code path from the pre-existingsetInterval).maxTestCaseResultsPerFlushis a constructor option (for testability) but deliberately has no env var - the point is it shouldn't need tuning.Testing
test-manager.test.ts, 3 new tests inserver-channel.test.ts, all passing)yarn nx run addon-vitest:checkandyarn nx run core:checkboth pass with no type errors. Formatted withoxfmt; lint has no new warnings beyond what already existed on these files.Summary by CodeRabbit
New Features
Bug Fixes