Core: Ensure process termination on SIGINT when telemetry is disabled#34585
Conversation
|
View your CI Pipeline Execution ↗ for commit 1d5f0b7
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull request overview
Fixes a dev-server shutdown bug where Storybook could leave the Node.js process running after SIGINT/SIGTERM when telemetry is disabled, by ensuring the signal handler always terminates the process.
Changes:
- Refactors the
cancelTelemetrysignal handler soprocess.exit(0)runs in afinallyblock. - Keeps sending the
canceledtelemetry event (with story index/stats) when telemetry is enabled, but still exits when it’s disabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughThe Changes
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 the current code and only fix it if needed.
Inline comments:
In `@code/core/src/core-server/dev-server.ts`:
- Around line 211-223: The signal shutdown currently awaits potentially-hung
operations (storyIndexGeneratorPromise / generator.getIndexAndStats() and
telemetry(...)), so change both awaits to bounded best-effort waits: wrap
awaiting storyIndexGeneratorPromise and generator.getIndexAndStats() in a
Promise.race with a small timeout (e.g., 200–500ms) so missing index data is
skipped if slow, and call telemetry('canceled', payload, { immediate: true })
via a Promise.race with a short timeout (or fire-and-forget while still
attempting a short await) to ensure the signal handler does not block
indefinitely; update references to storyIndexGeneratorPromise, getIndexAndStats,
summarizeIndex, and telemetry accordingly.
🪄 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: f0a0e4cb-9196-437b-a684-4289bc756925
📒 Files selected for processing (1)
code/core/src/core-server/dev-server.ts
What I did
Storybook was leaving behind lingering Node.js processes after being terminated, but only when telemetry was disabled. This PR fixes that bug.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Start Storybook with telemetry disabled. Kill it (Cmd+C), then use Activity Monitor to verify no
nodeprocesses are left behind.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.tsMake 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.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit