Vitest should not eat stdin while running non-interactively

[silverwind]

Describe the bug

While vitest is running non-interactively, e.g. with watch: false, anything typed into the terminal is needlessly consumed by the process but not actually used. I find this disturbing as I often like to type the next command while the current one is still running.

I assume the reason for this is because vitest sets the stdin stream into raw mode despite having no reason to do so when watch is not enabled. The only thing is effectively does is prevent echo and this is only useful on a interactive prompt.

System Info

System:
    OS: macOS 13.5
    CPU: (8) x64 Intel(R) Core(TM) i7-1060NG7 CPU @ 1.20GHz
    Memory: 115.56 MB / 16.00 GB
    Shell: 5.9 - /usr/local/bin/zsh
  Binaries:
    Node: 20.5.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.8.1 - ~/.npm-global/bin/npm
    pnpm: 8.6.11 - /usr/local/bin/pnpm
  Browsers:
    Chrome: 114.0.5735.133
    Safari: 16.6
  npmPackages:
    @vitejs/plugin-react-swc: 3.3.2 => 3.3.2
    vite: 4.4.9 => 4.4.9
    vitest: 0.34.1 => 0.34.1

Used Package Manager

npm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.

[silverwind]

I assume the fix for this is

diff --git a/packages/vitest/src/node/stdin.ts b/packages/vitest/src/node/stdin.ts
index f4b3b82d..69c734f1 100644
--- a/packages/vitest/src/node/stdin.ts
+++ b/packages/vitest/src/node/stdin.ts
@@ -125,18 +125,18 @@ export function registerConsoleShortcuts(ctx: Vitest) {
   function on() {
     off()
     rl = readline.createInterface({ input: process.stdin, escapeCodeTimeout: 50 })
     readline.emitKeypressEvents(process.stdin, rl)
-    if (process.stdin.isTTY)
+    if (process.stdin.isTTY && ctx.config.watch)
       process.stdin.setRawMode(true)
     process.stdin.on('keypress', keypressHandler)
   }

   function off() {
     rl?.close()
     rl = undefined
     process.stdin.removeListener('keypress', keypressHandler)
-    if (process.stdin.isTTY)
+    if (process.stdin.isTTY && ctx.config.watch)
       process.stdin.setRawMode(false)
   }

   on()

Unfortunately, I can not get vitest to build correctly on my machine. After pnpm install && pnpm run build and symlinking the module into my node_modules via npm i vitest@../vitest/packages/vitest and running it, it seems to find no tests in my package any more.

[AriPerkkio]

I assume the reason for this is because vitest sets the stdin stream into raw mode despite having no reason to do so when watch is not enabled.

This is done for test interruption via CTRL + c:

• Print errors when test runner is interrupted #3373
• CTRL+C does not immediately stop running tests #3569

It used to be done by relying on process.on('SIGINT') but wasn't stable across package managers and operating systems:

• CTRL+C does not immediately stop running tests #3569 (comment)

So I guess we should not count on catching SIGINT at all. Maybe we should instead capture keys with process.stdin.on('keypress') even when in run mode, and simply call process.exit when second key press is caught.

[silverwind]

I see, so as per this, raw mode is used to get node to emit keypress events (which it doesn't when not raw). The removal of the watch condition in #3642 made the raw mode unconditional which then brought up this new issue.

What I don't quite get is why SIGINT is not working universally, I thought it did and that's what I was using in many applications so far. Never once had I resort to keypress events.

[silverwind]

Another problem introduced by not using SIGINT is that I'm pretty sure other things than CTRL-C can invoke SIGINT, for example when the OS is shutting down, the process manager will sind SIGINT to all processes followed by SIGTERM.

By not catching SIGINT any more, you force the OS to SIGTERM the process eventually, which is not really the fine way to gracefully shut down a process.

So I think what should be done is:

• Restore the SIGINT handler, find out why it does not work in some cases (blocked event loop?)
• Never set stdin into raw mode in non-watch mode

Interesting tidbit about setRawMode from node docs:

Ctrl+C will no longer cause a SIGINT when in this mode.

[AriPerkkio]

When SIGINT is not caught, the default Nodejs behaviour will kick in. It will forcefully stop everything. So the current feature is that you can use CTRL + c to gracefully stop tests - signals and other mechanisms are not handled at all.

So I think what should be done is:

• Restore the SIGINT handler, find out why it does not work in some cases (blocked event loop?)

I have no idea how to fix this. Any help is appreciated. Here is the minimal reproduction case for studying this. It requires no dependencies: #3569 (comment).

[silverwind]

What's worse, setRawMode also clears any data queued in the terminal buffer before vitest ran, so if I pre-type something before vitest runs in a chain of commands, vitest will unexpectedly flush that buffer.

Imho setRawMode should never be used when non-interactive, it's far too intrusive.

[AriPerkkio]

Sounds like we should revert it then. Any ideas how to support #3373 when not listening for CTRL + c?

[silverwind]

Not directly revert, but find a cleaner solution to SIGINT handling that does not involve setting terminal to raw mode would be all I'm asking for.

I think some of the code may assume that SIGINT only comes via CTRL-c which is not true. One can for example generate the same signal via kill -SIGINT <pid> which of course would not be caught by keyboard handlers.

[AriPerkkio]

I think some of the code may assume that SIGINT only comes via CTRL-c which is not true. One can for example generate the same signal via kill -SIGINT <pid> which of course would not be caught by keyboard handlers.

Vitest is not listening for SIGINT at all. You can kill the process with kill -s SIGINT <pid>. If OS, other program or user by terminal sends SIGINT, Node process will die forcefully.

[silverwind]

Vitest is not listening for SIGINT at all. You can kill the process with kill -s SIGINT . If OS, other program or user by terminal sends SIGINT, Node process will die forcefully.

Interesting, does node install a default SIGINT handler in case the app does not register one? The docs do not mention it, they only say:

'SIGINT' from the terminal is supported on all platforms, and can usually be generated with Ctrl+C (though this may be configurable). It is not generated when terminal raw mode is enabled and Ctrl+C is used.

I think vitest should always install this handler. Only after switching into raw mode for watch mode, fall back to CTRL-c.

[AriPerkkio]

I think vitest should always install this handler. Only after switching into raw mode for watch mode, fall back to CTRL-c.

Vitest used to do that but it did not work on all platforms and package managers: #3569 (comment).
It was changed to listen for CTRL + c keys in #3642.

[silverwind]

It does sound like bugs in those package managers, but maybe I'll set up a test repo for this which could then run in GH Actions on the various package managers/os combos to really narrow it down.

[silverwind]

vitejs/vite#14342 is related for vite. It outlines a few more caveats that raw mode brings. Likely vitest could use a similar implementation.

[AriPerkkio]

vitejs/vite#14342 is related for vite.

That would not work for Vitest as that completely removes reacting to CTRL + C. Now Node handles that and simply terminates all execution immediately.

I think the only way to fix this is to revert changes of https://github.com/vitest-dev/vitest/pull/3642/files#diff-9c6031e2e4327db659ca4d448e18039217e6bf3dadfd843e1a5c0cb06297edcaL90-L93.

To fulfil #3373 we would need #3549 and it's successor #3732.

[AriPerkkio]

Enabling raw mode in non-interactive run-mode seems to also cause issues when running multiple Vitest commands in parallel using command & command in Unix systems, e.g.:

vitest run --shard=1/4 & \
vitest run --shard=2/4 & \
vitest run --shard=3/4 & \
vitest run --shard=4/4 & \
wait # https://man7.org/linux/man-pages/man2/waitpid.2.html

It doesn't run the commands before fg is executed. Disabling raw mode seems to fix this.

Time to revert this change and keep raw mode disabled in run-mode.