Skip to content

fix(sdk): ensure process exits after SIGTERM signal handling#1435

Merged
yifancong merged 3 commits intoweb-infra-dev:mainfrom
deepcoldy:fix/sigterm-process-exit
Dec 9, 2025
Merged

fix(sdk): ensure process exits after SIGTERM signal handling#1435
yifancong merged 3 commits intoweb-infra-dev:mainfrom
deepcoldy:fix/sigterm-process-exit

Conversation

@deepcoldy
Copy link
Copy Markdown
Contributor

When SIGTERM is received, the dispose method was called but the process did not exit, causing rstest to wait indefinitely and eventually kill the process with SIGKILL, preventing proper test result collection.

Changes:

  • Add shouldExit parameter to dispose method (default: false)
  • Call process.exit(0) after cleanup when shouldExit is true
  • Update signal handlers (SIGTERM, SIGINT, unhandledRejection, uncaughtException) to pass shouldExit: true
  • Keep exit event handler with shouldExit: false to avoid double exit
  • Manual dispose() calls in tests remain unaffected (default shouldExit: false)

This ensures that when rstest sends SIGTERM, the process properly exits after cleanup, allowing rstest to collect test results normally.

When SIGTERM is received, the dispose method was called but the process
did not exit, causing rstest to wait indefinitely and eventually kill
the process with SIGKILL, preventing proper test result collection.

Changes:
- Add shouldExit parameter to dispose method (default: false)
- Call process.exit(0) after cleanup when shouldExit is true
- Update signal handlers (SIGTERM, SIGINT, unhandledRejection, uncaughtException) to pass shouldExit: true
- Keep exit event handler with shouldExit: false to avoid double exit
- Manual dispose() calls in tests remain unaffected (default shouldExit: false)

This ensures that when rstest sends SIGTERM, the process properly exits
after cleanup, allowing rstest to collect test results normally.
@netlify
Copy link
Copy Markdown

netlify Bot commented Dec 8, 2025

Deploy Preview for rsdoctor ready!

Name Link
🔨 Latest commit 6b2377c
🔍 Latest deploy log https://app.netlify.com/projects/rsdoctor/deploys/69378defb87db3000800caf1
😎 Deploy Preview https://deploy-preview-1435--rsdoctor.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/sdk/src/sdk/server/index.ts Outdated
deepcoldy and others added 2 commits December 8, 2025 20:48
Address review feedback: unhandledRejection and uncaughtException should
exit with non-zero status code (1) instead of 0, so CI/rstest can properly
detect failures and collect error information.

Changes:
- Change dispose parameter from shouldExit: boolean to exitCode?: number
- SIGTERM and SIGINT exit with code 0 (normal termination)
- unhandledRejection and uncaughtException exit with code 1 (error)
- exit event handler doesn't pass exitCode (process already exiting)
@yifancong yifancong self-requested a review December 9, 2025 02:48
@yifancong yifancong merged commit 1206e73 into web-infra-dev:main Dec 9, 2025
8 checks passed
@Knagis
Copy link
Copy Markdown

Knagis commented Jan 21, 2026

This now silently kills the webpack build process (when @rsdoctor/webpack-plugin) is used on any uncaught exception.
Stopping the server would be fine, though in webpack case those uncaught exceptions could be completely unrelated - however stopping the process seems wrong when it isn't a process completely owned by rsdoctor

@Knagis
Copy link
Copy Markdown

Knagis commented Jan 21, 2026

https://nodejs.org/api/process.html#event-uncaughtexception

by default node already kills the process, however, the default also prints the error stack. so the handler in rsdoctor should simply be removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants