fix: kill other running tasks on failure

[s-h-a-d-o-w]

This kills already running tasks as soon as an error is emitted by another task. Making it possible to interrupt concurrently running tasks on first failure.

Aside from the included unmocked test, you can also try it out by staging a linting error and changing the lint-staged configuration in this repo to .lintstagedrc.mjs with the following content:

export default {
  "*.js": "eslint",
  "*.{js,ts}": () => "npm run test",
}

... and running:
node ./bin/lint-staged.js

I have changed the precedence of the tags, since I think it would be confusing to users to see the signals when we consciously kill tasks. (With the previous order, they would have seen SIGTERM. This way, it is KILLED.)

There are two minor caveats:

• It doesn't work with shell: true.
• For this to work with npm scripts (e.g. vanilla tsc, jest and probably anything that deals properly with its children is just fine), we need to kill all children ourselves. Which execa currently doesn't do for a good reason. However, I would argue that while this makes sense for an all purpose tool like execa, given lint-staged's use case, I can't imagine danger from killing child processes within milliseconds.

Fixes #594

[iiroj]

Hello,

thanks for the PR. Currently we use listr2 for handling task lifecycle — I wonder if it has a built-in way to cancel parallel tasks on failure. Also, execa can use an AbortController signal, would it possible to use that somehow?

[iiroj]

I believe we are intentionally disabling this error for parallel tasks, and only enabling it for subtasks (that run serially): https://github.com/okonet/lint-staged/blob/531275cbdb40fa9ec7ee972ee129e6323b9ab9ee/lib/runAll.js#L143-L149

[s-h-a-d-o-w]

I believe we are intentionally disabling this error for parallel tasks

That was the first thing I tried. It just marks the task as interrupted in the terminal but doesn't actually exit.

I wonder if it has a built-in way to cancel parallel tasks on failure

Like you mention - it's just the task life cycle. We manage what those tasks actually do in resolveTaskFn.js with execa.

Also, execa can use an AbortController signal, would it possible to use that somehow?

That would work as well but...

• It is only supported starting from node 15. They claim that it is available in 14 too but using 14.19.0, I get: ReferenceError: AbortController is not defined
• I'm not sure whether it is reliable when it comes to killing children. While it does seem to work with both npm and yarn at the moment, given that neither of them used to pass along signals to their children, I don't know why this works all of a sudden and whether it will continue to work in the future.

It is a more elegant solution though, so if you were to argue that I should check for the existence of AbortController and we will gamble on the fact that it will continue to work with children, I'm happy to make those changes.

[iiroj]

Thanks for the explanation! Just to make sure, does this kill all tasks or just the subtasks that we currently set to fail on error? We should still sync those settings.

[codecov]

Codecov Report

Merging #1117 (54b151d) into master (517235d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1117   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          678       691   +13     
  Branches       179       180    +1     
=========================================
+ Hits           678       691   +13     

Impacted Files	Coverage Δ
lib/resolveTaskFn.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 517235d...54b151d. Read the comment docs.

[iiroj]

Looks like Node.js 12 Windows tests failed, random?

[s-h-a-d-o-w]

I happen to be on windows and so tried to reproduce this. I'm seeing more, different errors even on master though.
A colleague who is also on windows said master fails for him too. But I still need to sync with him on details.

[s-h-a-d-o-w]

That said, I believe this might have to do with the fact that I didn't mock pidtree and maybe it tries to get something like pid 0 and that returns something on nix systems but not on windows (just a theory).
I'll obviously mock that asap, wouldn't want to try killing random things when running tests. 😅

[iiroj]

Our Windows tests could always use more love and attention, thanks.

[s-h-a-d-o-w]

Just ran this with #1118 merged in locally in my Windows 11 environment and at least here, everything passes now. 👍

[iiroj]

It's probably not related to this, but I'm getting double-output from terminal:

❯ node bin/lint-staged.js
✔ Preparing lint-staged...
✔ Hiding unstaged changes to partially staged files...
⚠ Running tasks for staged files...
  ❯ .lintstagedrc.mjs — 1 file
    ❯ *.mjs — 1 file
      ✖ prettier --list-different [FAILED]
    ❯ *.{mjs,js,ts} — 1 file
      ✖ npm run test
↓ Skipped because of errors from tasks. [SKIPPED]
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up temporary files...

✖ prettier --list-different:
.lintstagedrc.mjs
✔ Preparing lint-staged...
✔ Hiding unstaged changes to partially staged files...
⚠ Running tasks for staged files...
  ❯ .lintstagedrc.mjs — 1 file
    ❯ *.mjs — 1 file
      ✖ prettier --list-different [FAILED]
    ❯ *.{mjs,js,ts} — 1 file
      ✖ npm run test [KILLED]
↓ Skipped because of errors from tasks. [SKIPPED]
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up temporary files...

[iiroj]

Great work!

[github-actions]

🎉 This PR is included in version 12.3.6 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[s-h-a-d-o-w]

It's probably not related to this, but I'm getting double-output from terminal

I noticed that too and double-checked with previous master, where I saw it happen as well (no fancy options - just running default concurrently and waiting for tests to finish running). I figured that you might do this on purpose, so that the final output is always the list of tasks, even when there are potentially long error logs.

Great work!

Thanks! It's been a pleasure working with you on this. 🙂

[iiroj]

It might be that we print the task error output too soon, and listr2 does some final update after that, producing the duplicate.