fix: kill other running tasks on failure (#1117)

Author: s-h-a-d-o-w

lib/resolveTaskFn.js

@@ -2,14 +2,17 @@ import { redBright, dim } from 'colorette'
 import execa from 'execa'
 import debug from 'debug'
 import { parseArgsStringToArgv } from 'string-argv'
+import pidTree from 'pidtree'
 
 import { error, info } from './figures.js'
 import { getInitialState } from './state.js'
 import { TaskError } from './symbols.js'
 
+const ERROR_CHECK_INTERVAL = 200
+
 const debugLog = debug('lint-staged:resolveTaskFn')
 
-const getTag = ({ code, killed, signal }) => signal || (killed && 'KILLED') || code || 'FAILED'
+const getTag = ({ code, killed, signal }) => (killed && 'KILLED') || signal || code || 'FAILED'
 
 /**
  * Handle task console output.
@@ -43,6 +46,34 @@ const handleOutput = (command, result, ctx, isError = false) => {
   }
 }
 
+/**
+ * Interrupts the execution of the execa process that we spawned if
+ * another task adds an error to the context.
+ *
+ * @param {Object} ctx
+ * @param {execa.ExecaChildProcess<string>} execaChildProcess
+ * @returns {function(): void} Function that clears the interval that
+ * checks the context.
+ */
+const interruptExecutionOnError = (ctx, execaChildProcess) => {
+  async function loop() {
+    if (ctx.errors.size > 0) {
+      const ids = await pidTree(execaChildProcess.pid)
+      ids.forEach(process.kill)
+
+      // The execa process is killed separately in order
+      // to get the `KILLED` status.
+      execaChildProcess.kill()
+    }
+  }
+
+  const loopIntervalId = setInterval(loop, ERROR_CHECK_INTERVAL)
+
+  return () => {
+    clearInterval(loopIntervalId)
+  }
+}
+
 /**
  * Create a error output dependding on process result.
  *
@@ -101,9 +132,13 @@ export const resolveTaskFn = ({
   debugLog('execaOptions:', execaOptions)
 
   return async (ctx = getInitialState()) => {
-    const result = await (shell
+    const execaChildProcess = shell
       ? execa.command(isFn ? command : `${command} ${files.join(' ')}`, execaOptions)
-      : execa(cmd, isFn ? args : args.concat(files), execaOptions))
+      : execa(cmd, isFn ? args : args.concat(files), execaOptions)
+
+    const quitInterruptCheck = interruptExecutionOnError(ctx, execaChildProcess)
+    const result = await execaChildProcess
+    quitInterruptCheck()
 
     if (result.failed || result.killed || result.signal != null) {
       throw makeErr(command, result, ctx)

package-lock.json

@@ -42,6 +42,7 @@
     "micromatch": "^4.0.4",
     "normalize-path": "^3.0.0",
     "object-inspect": "^1.12.0",
+    "pidtree": "^0.5.0",
     "string-argv": "^0.3.1",
     "supports-color": "^9.2.1",
     "yaml": "^1.10.2"

package.json

@@ -1,5 +1,7 @@
+import { createExecaReturnValue } from '../utils/createExecaReturnValue'
+
 const execa = jest.fn(() =>
-  Promise.resolve({
+  createExecaReturnValue({
     stdout: 'a-ok',
     stderr: '',
     code: 0,

test/__mocks__/execa.js

@@ -0,0 +1,3 @@
+module.exports = () => {
+  return Promise.resolve([])
+}

test/__mocks__/pidtree.js

@@ -4,8 +4,14 @@ import { resolveTaskFn } from '../lib/resolveTaskFn'
 import { getInitialState } from '../lib/state'
 import { TaskError } from '../lib/symbols'
 
+import { createExecaReturnValue } from './utils/createExecaReturnValue'
+
 const defaultOpts = { files: ['test.js'] }
 
+function mockExecaImplementationOnce(value) {
+  execa.mockImplementationOnce(() => createExecaReturnValue(value))
+}
+
 describe('resolveTaskFn', () => {
   beforeEach(() => {
     execa.mockClear()
@@ -135,7 +141,7 @@ describe('resolveTaskFn', () => {
 
   it('should throw error for failed linters', async () => {
     expect.assertions(1)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock error',
       stderr: '',
       code: 0,
@@ -149,7 +155,7 @@ describe('resolveTaskFn', () => {
 
   it('should throw error for interrupted processes', async () => {
     expect.assertions(1)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock error',
       stderr: '',
       code: 0,
@@ -167,7 +173,7 @@ describe('resolveTaskFn', () => {
 
   it('should throw error for killed processes without signal', async () => {
     expect.assertions(1)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock error',
       stderr: '',
       code: 0,
@@ -192,7 +198,7 @@ describe('resolveTaskFn', () => {
   })
 
   it('should add TaskError on error', async () => {
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock error',
       stderr: '',
       code: 0,
@@ -210,7 +216,7 @@ describe('resolveTaskFn', () => {
 
   it('should not add output when there is none', async () => {
     expect.assertions(2)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: '',
       stderr: '',
       code: 0,
@@ -236,7 +242,7 @@ describe('resolveTaskFn', () => {
 
   it('should add output even when task succeeds if `verbose: true`', async () => {
     expect.assertions(2)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock success',
       stderr: '',
       code: 0,
@@ -266,7 +272,7 @@ describe('resolveTaskFn', () => {
 
   it('should not add title to output when task errors while quiet', async () => {
     expect.assertions(2)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: '',
       stderr: 'stderr',
       code: 1,
@@ -296,7 +302,7 @@ describe('resolveTaskFn', () => {
 
   it('should not print anything when task errors without output while quiet', async () => {
     expect.assertions(2)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: '',
       stderr: '',
       code: 1,
@@ -321,4 +327,29 @@ describe('resolveTaskFn', () => {
       }
     `)
   })
+
+  it('should kill a long running task when an error is added to the context', async () => {
+    execa.mockImplementationOnce(() =>
+      createExecaReturnValue(
+        {
+          stdout: 'a-ok',
+          stderr: '',
+          code: 0,
+          cmd: 'mock cmd',
+          failed: false,
+          killed: false,
+          signal: null,
+        },
+        1000
+      )
+    )
+
+    const context = getInitialState()
+    const taskFn = resolveTaskFn({ command: 'node' })
+    const taskPromise = taskFn(context)
+
+    context.errors.add({})
+
+    await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`)
+  })
 })

test/resolveTaskFn.spec.js

@@ -1,4 +1,5 @@
 import { resolveTaskFn } from '../lib/resolveTaskFn'
+import { getInitialState } from '../lib/state'
 
 jest.unmock('execa')
 
@@ -13,4 +14,25 @@ describe('resolveTaskFn', () => {
 
     await expect(taskFn()).resolves.toMatchInlineSnapshot(`undefined`)
   })
+
+  it('should kill a long running task when another fails', async () => {
+    const context = getInitialState()
+
+    const taskFn = resolveTaskFn({
+      command: 'node',
+      isFn: true,
+    })
+    const taskPromise = taskFn(context)
+
+    const taskFn2 = resolveTaskFn({
+      command: 'node -e "process.exit(1)"',
+      isFn: true,
+    })
+    const task2Promise = taskFn2(context)
+
+    await expect(task2Promise).rejects.toThrowErrorMatchingInlineSnapshot(
+      `"node -e \\"process.exit(1)\\" [FAILED]"`
+    )
+    await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`)
+  })
 })

test/resolveTaskFn.unmocked.spec.js

@@ -12,6 +12,8 @@ import { ConfigNotFoundError, GitError } from '../lib/symbols'
 import * as searchConfigsNS from '../lib/searchConfigs'
 import * as getConfigGroupsNS from '../lib/getConfigGroups'
 
+import { createExecaReturnValue } from './utils/createExecaReturnValue'
+
 jest.mock('../lib/file')
 jest.mock('../lib/getStagedFiles')
 jest.mock('../lib/gitWorkflow')
@@ -192,7 +194,7 @@ describe('runAll', () => {
     expect.assertions(2)
     getStagedFiles.mockImplementationOnce(async () => ['sample.js'])
     execa.mockImplementation(() =>
-      Promise.resolve({
+      createExecaReturnValue({
         stdout: '',
         stderr: 'Linter finished with error',
         code: 1,
@@ -229,7 +231,7 @@ describe('runAll', () => {
     expect.assertions(2)
     getStagedFiles.mockImplementationOnce(async () => ['sample.js'])
     execa.mockImplementation(() =>
-      Promise.resolve({
+      createExecaReturnValue({
         stdout: '',
         stderr: '',
         code: 0,
@@ -252,8 +254,8 @@ describe('runAll', () => {
       LOG [STARTED]  — 1 file
       LOG [STARTED] *.js — 1 file
       LOG [STARTED] echo \\"sample\\"
-      ERROR [FAILED] echo \\"sample\\" [SIGINT]
-      ERROR [FAILED] echo \\"sample\\" [SIGINT]
+      ERROR [FAILED] echo \\"sample\\" [KILLED]
+      ERROR [FAILED] echo \\"sample\\" [KILLED]
       LOG [SUCCESS] Running tasks for staged files...
       LOG [STARTED] Applying modifications from tasks...
       INFO [SKIPPED] Skipped because of errors from tasks.

test/runAll.spec.js

@@ -0,0 +1,20 @@
+export function createExecaReturnValue(value, executionTime) {
+  const returnValue = { ...value }
+  let triggerResolve
+  let resolveTimeout
+
+  const returnedPromise = executionTime
+    ? new Promise((resolve) => {
+        triggerResolve = resolve.bind(null, returnValue)
+        resolveTimeout = setTimeout(triggerResolve, executionTime)
+      })
+    : Promise.resolve(returnValue)
+
+  returnedPromise.kill = () => {
+    returnValue.killed = true
+    clearTimeout(resolveTimeout)
+    triggerResolve()
+  }
+
+  return returnedPromise
+}