Handle propogating SIGTERM to spawned child processes

[ganemone]

What's the problem this PR addresses?

Fixes #1741
See https://github.com/albertywu/yarn2-termination-bug#repro for more context
...

How did you fix it?

Add SIGTERM listener to the yarn process and send signal to spawned child process if still running. The current approach will add a new SIGTERM listener to the yarn process for each child process spawned, removing the listener when the child process closes. If this is a concern, we could go with an alternative approach that keeps track of the child process refs in a Set and use a single listener to close all child processes running when the yarn process receives SIGTERM.

NOTE: The current PR only addresses this in the pipevp function in execUtils, not the execvp function. Its unclear to me if the same problem can present itself in usage of the execvp function, but we can easily add the same functionality there.
...

Checklist

• I have read the Contributing Guide.

• I have set the packages that need to be released for my changes to be effective.

• I will check that all automated PR checks pass before the PR gets reviewed.

[andreialecu]

I looked over the repro you linked and it doesn't seem that your changes fix it.

I experimented a bit and moving your changes around here:

berry/packages/yarnpkg-shell/sources/pipe.ts

Lines 50 to 57 in 63a77b5

	const child = crossSpawn(name, args, {...spawnOpts, stdio: [
	stdin,
	stdout,
	stderr,
	]});
	if (sigintRefCount++ === 0)
	process.on(`SIGINT`, sigintHandler);

does help

However, the yarn v1 wrapper seems to have the same bug. SIGTERM on the yarn v1 process will not kill yarn v2.

Made a small screen recording of the behavior here (you can use "Download a Copy")

Some more context from Discord

[andreialecu]

I sent a PR to fix the issue in yarn v1 here: yarnpkg/yarn#8543

[andreialecu]

➜ ./repro.sh                     
2.4.0-git.20210107.hash-f13c07ba
killing yarn process: 66664
got sigterm
[PASS] done sleeping

With above v1 PR and with the below fix at:

berry/packages/yarnpkg-shell/sources/pipe.ts

Lines 54 to 56 in 63a77b5

	]});
	if (sigintRefCount++ === 0)

    const sigtermHandler = () => child.kill(`SIGTERM`);
    process.on(`SIGTERM`, sigtermHandler);

[ganemone]

@andreialecu I'm focused right now on solving the issue for yarn2 not using the yarn1 wrapper. We can follow up with a fix for yarn1 if necessary. I did double check and my changes do fix the issue when running yarn node <script>.

[andreialecu]

Here's a diff with tests:

diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/commands/exec.test.js b/packages/acceptance-tests/pkg-tests-specs/sources/commands/exec.test.js
index 94ba01f5..80f9cad2 100644
--- a/packages/acceptance-tests/pkg-tests-specs/sources/commands/exec.test.js
+++ b/packages/acceptance-tests/pkg-tests-specs/sources/commands/exec.test.js
@@ -26,5 +26,29 @@ describe(`Commands`, () => {
         });
       })
     );
+
+    test(
+      `it should end child process on SIGTERM`,
+      makeTemporaryEnv({}, async ({ path, run, source }) => {
+        await xfs.writeFilePromise(`${path}/test.sh`, ([
+          `yarn node -e "console.log('Testing exec SIGTERM'); setTimeout(() => {}, 10000)" &`,
+          `sleep 1`,
+          `ps | grep -v "grep" | grep -q "Testing exec SIGTERM" || exit 1`, // check if it was started properly
+          `kill $!`,
+          `sleep 1`,
+          `if ps | grep -v "grep" | grep -q "Testing exec SIGTERM"`,
+          `then`,
+          `  echo "[FAIL] still running"; exit 1`,
+          `else`,
+          `  echo "[PASS] ok"; exit 0`,
+          `fi`
+        ]).join('\n'));
+
+        await expect(run(`exec`, `bash`, `test.sh`)).resolves.toMatchObject({
+          code: 0,
+          stdout: expect.stringContaining(`PASS`),
+        });
+      })
+    );
   });
 });
diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/commands/run.test.js b/packages/acceptance-tests/pkg-tests-specs/sources/commands/run.test.js
index 4b031094..389ab8fd 100644
--- a/packages/acceptance-tests/pkg-tests-specs/sources/commands/run.test.js
+++ b/packages/acceptance-tests/pkg-tests-specs/sources/commands/run.test.js
@@ -1,3 +1,5 @@
+const {xfs} = require(`@yarnpkg/fslib`);
+
 describe(`Commands`, () => {
   for (const [description, args] of [[`with prefix`, [`run`]], [`without prefix`, []]]) {
     describe(`run ${description}`, () => {
@@ -123,6 +125,7 @@ describe(`Commands`, () => {
         },
       ),
     );
+
     test(`it should print the list of available scripts if no parameters passed to command`,
       makeTemporaryEnv(
         {
@@ -137,5 +140,35 @@ describe(`Commands`, () => {
         }
       )
     );
+
+    test(
+      `it should end child script on SIGTERM`,
+      makeTemporaryEnv({
+        scripts: {
+          sleep: `node -e "console.log('Testing script SIGTERM'); setTimeout(() => {}, 10000)"`
+        }
+      }, async ({ path, run, source }) => {
+        await run(`install`);
+        
+        await xfs.writeFilePromise(`${path}/test.sh`, ([
+          `yarn sleep &`,
+          `sleep 1`,
+          `ps | grep -v "grep" | grep -q "Testing script SIGTERM" || exit 1`, // check if it was started properly
+          `kill $!`,
+          `sleep 1`,
+          `if ps | grep -v "grep" | grep -q "Testing script SIGTERM"`,
+          `then`,
+          `  echo "[FAIL] still running"; exit 1`,
+          `else`,
+          `  echo "[PASS] ok"; exit 0`,
+          `fi`
+        ]).join('\n'));
+
+        await expect(run(`exec`, `bash`, `test.sh`)).resolves.toMatchObject({
+          code: 0,
+          stdout: expect.stringContaining(`PASS`),
+        });
+      })
+    );
   });
 });

[arcanis]

Nice, thank you both! @ganemone can you add the tests from @andreialecu to your PR? Perhaps we can even make a dedicated file in features/signal.test.ts, we start to have various signal behaviours to check 🤔

[merceyz]

Isn't adding all these SIGTERM handlers going to make node complain?

[arcanis]

It might with yarn workspaces foreach, yep. @ganemone can you use a similar refcount to register a single callback, and keep track of the children manually in a set?

[merceyz]

Do we need to explicitly forward the SIGINT as well? 🤔

[arcanis]

No, iirc SIGINT is purposefully ignored because the shell already sends it to the whole process tree, so we exit once the children have caught it and exited cleanly. I think there are discussions about that (perhaps on Discord).

[andreialecu]

FWIW if you try to run kill -s SIGINT <yarn_pid> nothing happens. Should it not be forwarded to the child process?

[andreialecu]

I'm not sure under which circumstances yarn itself might receive a SIGINT though. I can't think of any, but I'm not sure why forwarding it should be prevented.

Babel for example forwards it to itself: https://github.com/babel/babel/blob/master/packages/babel-node/src/babel-node.js#L99

[arcanis]

The shell sends it to the whole process tree. Using kill doesn't. More details here.

In any case, this is intentional, SIGINT is working as expected. Babel doesn't have to work with many types of binaries, some of them behaving weirdly if they receive the same signal multiple times 🙂

[ganemone]

@arcanis finding a way to test this reliably is pretty difficult. I'm removing the tests for now

[merceyz]

Needs to follow the same refcount logic as the SIGINT event

[arcanis]

I've made a few fixes to unify the SIGINT / SIGTERM handling, and removed the execvp part since it's not clear whether it useful, and in this sort of case I prefer to err on the side of the status quo (at least until we find a practical case where it matters). If the tests pass I think it's good to ship.

[haroldlbrown]

Is there a planned release date for this bugfix?
As long as SIGTERM signals to a Docker container aren't propagated to Yarn 2 child processes, shipping Yarn 2 Docker containers is pretty cumbersome.

[arcanis]

It'll be part of the next major. If you want to use it now, you can run yarn set version from sources (plugins will need to be updated too using yarn plugin import from sources).

[merceyz]

shipping Yarn 2 Docker containers is pretty cumbersome.

Assuming you meant Yarn PnP then you can run it without using the CLI - https://yarnpkg.com/features/pnp#initializing-pnp

[andreialecu]

@BERVOL as a workaround: you can bypass yarn by running your scripts via node directly:

If you use pnp you need to load the pnp hook:

node -r ./.pnp.js server.js

If you use yarn 1 to spawn yarn 2, note that bumping just v2 to sources isn't enough, because v1 has the same problem:

yarnpkg/yarn#8543

[WesCossick]

Passing SIGTERM seems to have been released with Yarn 3.0.0. We're using Yarn 3.0.2 inside Docker, and are still noticing SIGTERM signals aren't propagated to the child process.

In our Dockerfile, we've got:

ENTRYPOINT ["/usr/local/bin/yarn", "workspace", "example-workspace", "run", "start"]

And then in that workspace's package.json file we've got:

"scripts": {
    "start": "node --unhandled-rejections=strict ./entry.js"
}

Should we expect SIGTERM signals to be propagated in this configuration?

[WesCossick]

Using node -r ./.pnp.cjs ./entry.js allows our script to catch SIGTERM signals, so it seems to be the usage of yarn workspace example-workspace run start that stops propagation of signals.

[merceyz]

@WesCossick Did you read the comment right above yours? Specifically this part:

If you use yarn 1 to spawn yarn 2, note that bumping just v2 to sources isn't enough, because v1 has the same problem:

Make sure to update your v1 version as well

[WesCossick]

@merceyz I did see that comment, yes. In many of our company's Docker images we manually install the latest version of Yarn (1.22.15 right now), but in the Docker image I was using for testing, it turns out it was just using the version of Yarn bundled with the node Docker image, which happens to be pretty outdated (1.22.5). I didn't catch that and assumed it was using the latest version like our other images.

Upgrading Yarn v1 from 1.22.5 to 1.22.15 fixed the issue.

There's been some recent discussions and steps taken to upgrade the version of Yarn bundled with Node.js's official Docker images: nodejs/docker-node#1542. Seems like they should be updated in the near future.

In the meantime, for others who find this, you can add the following instruction to update Yarn:

RUN npm install -g [email protected] --force