src: fix kill signal 0 on Windows by StefanStojanovic · Pull Request #57695 · nodejs/node

StefanStojanovic · 2025-03-31T09:10:44Z

The previous changes to this file missed the special case of process.kill(0), thus breaking it on Windows. This PR fixes that.

Refs: #55514
Refs: #42923
Fixes: #57669

codecov · 2025-03-31T11:39:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (657f818) to head (cd81beb).
Report is 538 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #57695 +/- ##
=======================================
Coverage 90.23% 90.24% =======================================
Files 630 630 Lines 185017 185017 Branches 36207 36216 +9 =======================================
+ Hits 166948 166966 +18 - Misses 11020 11025 +5 + Partials 7049 7026 -23

Files with missing lines	Coverage Δ
src/process_wrap.cc	`66.66% <ø> (ø)`

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

targos · 2025-03-31T13:04:50Z

Is it possible to add a test?

StefanStojanovic · 2025-03-31T16:23:56Z

Is it possible to add a test?

Good idea. I've added it. Hopefully, the 1 second I gave it will be enough for the CI machines.

lpinca · 2025-04-01T06:45:00Z

Just a suggestion (that can be ignored) to not rely on timers and not wait a second for the test to end.
Suggested change
constcheckProcess=spawn(process.execPath,['-e','setTimeout(() => {}, 1000)']);
checkProcess.on('exit',(code,signal)=>{
assert.strictEqual(code,0);
assert.strictEqual(signal,null);
});
checkProcess.kill(0);
constcheckProcess=spawn(process.execPath,[
'-e',
'setInterval(() => {}, 1000)'
]);
checkProcess.on('exit',(code,signal)=>{
assert.strictEqual(code,null);
assert.strictEqual(signal,'SIGTERM');
});
checkProcess.kill(0);// 'SIGKILL' is not sent to the child process.
checkProcess.kill('SIGTERM');

I like changing timeout to interval and then killing it manually to have complete control. However, the new asserts change the nature of the test itself. We want to test checkProcess.kill(0); and see if the code and signal will be as expected. This change checks if sending 0 will kill the process, but not if it'll return the expected values. Correct?

Yes, correct, the assumption is that checkProcess.kill(0) is a no-op because the received signal is 'SIGTERM'. If it wasn't the received signal would be 'SIGKILL'. I think this is sufficient to assert that it doesn't change the signal. However, as you said, there is no check on the exit code.

Another option can be something like this:
constcode=`const interval = setInterval(() => {}, 1000);process.stdin.on('data', () => { clearInterval(interval); });process.stdout.write('x');`;constcheckProcess=spawn(process.execPath,['-e',code]);checkProcess.on('exit',(code,signal)=>{assert.strictEqual(code,0);assert.strictEqual(signal,null);});checkProcess.stdout.on('data',common.mustCall((chunk)=>{assert.strictEqual(chunk.toString(),'x');checkProcess.kill(0);checkProcess.stdin.write('x');checkProcess.stdin.end();}));
What do you think?

I like that one. It's more elegant IMHO. You can submit it as a new suggestion if you want.

Feel free to apply it yourself.

Done, please reapprove when you get a chance. Thanks.

mcollina

lgtm

nodejs-github-bot · 2025-04-01T11:46:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/65926/

This special case was missed in the previous changes to this file. Refs: nodejs#55514 Refs: nodejs#42923Fixes: nodejs#57669

github-actions · 2025-04-03T09:36:18Z

Failed to start CI

 ⚠ Commits were pushed since the last approving review:
⚠ - src: fix kill signal 0 on Windows
✘ Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/14239782978

nodejs-github-bot · 2025-04-03T12:25:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/65996/

nodejs-github-bot · 2025-04-03T15:30:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66005/

nodejs-github-bot · 2025-04-03T18:55:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66011/

nodejs-github-bot · 2025-04-04T05:29:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66017/

nodejs-github-bot · 2025-04-04T06:52:54Z

Landed in 32e5e81

This special case was missed in the previous changes to this file. Refs: #55514 Refs: #42923Fixes: #57669 PR-URL: #57695 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

StefanStojanovic added windows Issues and PRs related to the Windows platform. process Issues and PRs related to the process subsystem. labels Mar 31, 2025

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Mar 31, 2025

lpinca approved these changes Mar 31, 2025
View reviewed changes

StefanStojanovic force-pushed the mefi-kill-fix branch from b84c0d0 to d33c9e9Compare March 31, 2025 16:22

StefanStojanovic force-pushed the mefi-kill-fix branch from d33c9e9 to 389bf3fCompare March 31, 2025 19:48

jakecastelli approved these changes Apr 1, 2025
View reviewed changes

lpinca reviewed Apr 1, 2025
View reviewed changes

mcollina approved these changes Apr 1, 2025
View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025

src: fix kill signal 0 on Windows
cd81beb
This special case was missed in the previous changes to this file. Refs: nodejs#55514 Refs: nodejs#42923Fixes: nodejs#57669

StefanStojanovic force-pushed the mefi-kill-fix branch from 389bf3f to cd81bebCompare April 3, 2025 09:27

StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

github-actionsBot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 3, 2025

StefanStojanovic removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 3, 2025

richardlau approved these changes Apr 3, 2025
View reviewed changes

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

lpinca approved these changes Apr 3, 2025
View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Apr 4, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2025

nodejs-github-bot merged commit 32e5e81 into nodejs:mainApr 4, 2025

richardlau added lts-watch-v20.x lts-watch-v22.x PRs that may need to be released in v22.x labels Apr 4, 2025

ehmicky mentioned this pull request Apr 4, 2025
subprocess.kill(0) is broken on Windows since Node 23.4.0 sindresorhus/execa#1193
Closed

RafaelGSS mentioned this pull request May 1, 2025
2025-05-06, Version 24.0.0 (Current) #57609
Merged

aduh95 removed the lts-watch-v22.x PRs that may need to be released in v22.x label May 6, 2025

github-actionsBot mentioned this pull request May 19, 2025
2025-05-21, Version 22.16.0 'Jod' (LTS) #58388
Merged

marco-ippolito added backported-to-v20.x and removed lts-watch-v20.x labels Jun 5, 2025

marco-ippolito mentioned this pull request Jun 5, 2025
v20.19.3 proposal #58588
Merged

ghost mentioned this pull request Jun 8, 2025
### Notable Changes autofix-ci/action#28
Closed

coderabbitaiBot mentioned this pull request Jul 27, 2026
fix(runtime): reclaim orca-runtime.json when it stops describing this runtime stablyai/orca#10840
Merged

-constcheckProcess=spawn(process.execPath,['-e','setTimeout(() => {}, 1000)']);
-checkProcess.on('exit',(code,signal)=>{
-assert.strictEqual(code,0);
-assert.strictEqual(signal,null);
-});
-checkProcess.kill(0);
+constcheckProcess=spawn(process.execPath,[
+'-e',
+'setInterval(() => {}, 1000)'
+]);
+checkProcess.on('exit',(code,signal)=>{
+assert.strictEqual(code,null);
+assert.strictEqual(signal,'SIGTERM');
+});
+checkProcess.kill(0);// 'SIGKILL' is not sent to the child process.
+checkProcess.kill('SIGTERM');

Uh oh!

Uh oh!

Conversation

StefanStojanovic commented Mar 31, 2025

Uh oh!

codecovBot commented Mar 31, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

targos commented Mar 31, 2025

Uh oh!

StefanStojanovic commented Mar 31, 2025

Uh oh!

lpincaApr 1, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanStojanovicApr 2, 2025

Choose a reason for hiding this comment

Uh oh!

lpincaApr 2, 2025

Choose a reason for hiding this comment

Uh oh!

lpincaApr 2, 2025

Choose a reason for hiding this comment

Uh oh!

StefanStojanovicApr 3, 2025

Choose a reason for hiding this comment

Uh oh!

lpincaApr 3, 2025

Choose a reason for hiding this comment

Uh oh!

StefanStojanovicApr 3, 2025

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 1, 2025

Uh oh!

github-actionsBot commented Apr 3, 2025

Uh oh!

nodejs-github-bot commented Apr 3, 2025

Uh oh!

nodejs-github-bot commented Apr 3, 2025

Uh oh!

nodejs-github-bot commented Apr 3, 2025

Uh oh!

nodejs-github-bot commented Apr 4, 2025

Uh oh!

nodejs-github-bot commented Apr 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecovBot commented Mar 31, 2025•
edited
Loading

lpincaApr 1, 2025•
edited
Loading