Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

node-daily-v14.x-staging is failing on smartos #3154

Closed
richardlau opened this issue Jan 19, 2023 · 3 comments · Fixed by nodejs/node#46276
Closed

node-daily-v14.x-staging is failing on smartos #3154

richardlau opened this issue Jan 19, 2023 · 3 comments · Fixed by nodejs/node#46276
Labels
platform:smartos

Comments

@richardlau
Copy link
Member

Extracted out of #3131:

https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-v14.x-staging/ has been failing since 6 December. Cursory glance appears to be:

On macOS
(Fixed in #3131.)

On smartos there's one or more stray Node.js process being left behind:

11:29:41 ps awwx | grep Release/node | grep -v grep | cat
11:29:42  574381 ?        S  0:01 /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos20-64/out/Release/node -e setInterval(()=>{}, 99)
11:29:42  574413 ?        S  0:01 /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos20-64/out/Release/node -e setInterval(()=>{}, 99)
11:29:42 make[1]: *** [Makefile:515: test-ci] Error 1

Needs further investigation. The CI jobs are configured to fail if leftover Node.js processes are detected. As far as I can tell the tests themselves have passed. This may have started after the recent migration of the smartos machines (cc https://github.com/orgs/nodejs/teams/platform-smartos @bahamat @sxa ) -- I had clean CI's for Node.js 14.21.2 on December 8: https://ci.nodejs.org/job/node-test-commit-smartos/46911/nodes=smartos20-64/
@richardlau richardlau mentioned this issue Jan 19, 2023
Closed
@richardlau
Copy link
Member Author

I've narrowed down node -e setInterval(()=>{}, 99) to coming from test-child-process-exec-abortcontroller-promisified. This test is passing but leaving behind Node.js child processes.

Curiously this test doesn't run the node child process on Linux and instead uses sleep 2m. This was done in nodejs/node#37572 -- prior to that PR all platforms used sleep 2m apart from Windows (which was flaky). If I switch the test to use sleep 2m on SmartOS the test still passes but doesn't leave behind any node child processes (no surprise since it's not creating any) but also is not leaving behind any sleep processes.

I'm not entirely sure why we didn't have the child processes being left behind on smartos before the migration -- perhaps this is something timing related? We're not seeing the leftover child processes on main or current but are consistently seeing them on Node.js 14 and 16 SmartOS CI runs when we were not prior to the migration.

I'm going to open a PR to switch back to sleep 2m for the test on POSIX platforms to unblock the Node.js 14 and 16 CI.

This was referenced Jan 19, 2023
Closed
Merged
@richardlau
Copy link
Member Author

I'm going to open a PR to switch back to sleep 2m for the test on POSIX platforms to unblock the Node.js 14 and 16 CI.

PR: nodejs/node#46276

@bahamat
Copy link

bahamat commented Jan 19, 2023

Looking at the issue, and the proposed fix, I think this is the correct approach. I can't mark approval on the PR, so I'm noting it here instead.

richardlau added a commit to richardlau/node-1 that referenced this issue Jan 22, 2023
@richardlau
test: avoid left behind child processes
a5f8798 
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs#46276
Fixes: nodejs/build#3154
Refs: nodejs#37518
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau added a commit to nodejs/node that referenced this issue Jan 22, 2023
@richardlau
test: avoid left behind child processes
d7f83c4 
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit to nodejs/node that referenced this issue Feb 1, 2023
@richardlau @ruyadorno
test: avoid left behind child processes
747f368 
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to nodejs/node that referenced this issue Mar 3, 2023
@richardlau @juanarbol
test: avoid left behind child processes
c5fedac 
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to nodejs/node that referenced this issue Mar 5, 2023
@richardlau @juanarbol
test: avoid left behind child processes
041aac3 
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
oraluben pushed a commit to oraluben/node that referenced this issue Mar 14, 2023
@richardlau @oraluben
test: avoid left behind child processes
0bcc192 
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs/node#46276
Fixes: nodejs/build#3154
Refs: nodejs/node#37518
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
oraluben pushed a commit to oraluben/node that referenced this issue Mar 17, 2023
@richardlau @oraluben
test: avoid left behind child processes
f1a36e1 
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs/node#46276
Fixes: nodejs/build#3154
Refs: nodejs/node#37518
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
platform:smartos
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: avoid left behind child processes richardlau/node-1
2 participants
@bahamat @richardlau