fix: hooks should respect maxConcurrency (#9653)

Author: hi-ogawa

docs/config/maxconcurrency.md

@@ -9,6 +9,6 @@ outline: deep
 - **Default**: `5`
 - **CLI**: `--max-concurrency=10`, `--maxConcurrency=10`
 
-A number of tests that are allowed to run at the same time marked with `test.concurrent`.
+The maximum number of tests and hooks that can run at the same time when using `test.concurrent` or `describe.concurrent`.
 
-Test above this limit will be queued to run when available slot appears.
+The hook execution order within a single group is also controlled by [`sequence.hooks`](/config/sequence#sequence-hooks). With `sequence.hooks: 'parallel'`, the execution is bounded by the same limit of [`maxConcurrency`](/config/maxconcurrency).

docs/config/sequence.md

@@ -145,7 +145,7 @@ Changes the order in which hooks are executed.
 
 - `stack` will order "after" hooks in reverse order, "before" hooks will run in the order they were defined
 - `list` will order all hooks in the order they are defined
-- `parallel` will run hooks in a single group in parallel (hooks in parent suites will still run before the current suite's hooks)
+- `parallel` runs hooks in a single group in parallel (hooks in parent suites still run before the current suite's hooks). The actual number of simultaneously running hooks is limited by [`maxConcurrency`](/config/maxconcurrency).
 
 ::: tip
 This option doesn't affect [`onTestFinished`](/api/hooks#ontestfinished). It is always called in reverse order.

docs/guide/cli-generated.md

@@ -781,7 +781,7 @@ Default timeout of a teardown function in milliseconds (default: `10000`)
 - **CLI:** `--maxConcurrency <number>`
 - **Config:** [maxConcurrency](/config/maxconcurrency)
 
-Maximum number of concurrent tests in a suite (default: `5`)
+Maximum number of concurrent tests and suites during test file execution (default: `5`)
 
 ### expect.requireAssertions
 

docs/guide/parallelism.md

@@ -22,6 +22,8 @@ Unlike _test files_, Vitest runs _tests_ in sequence. This means that tests insi
 
 Vitest supports the [`concurrent`](/api/test#test-concurrent) option to run tests together. If this option is set, Vitest will group concurrent tests in the same _file_ (the number of simultaneously running tests depends on the [`maxConcurrency`](/config/maxconcurrency) option) and run them with [`Promise.all`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all).
 
+The hook execution order within a single group is also controlled by [`sequence.hooks`](/config/sequence#sequence-hooks). With `sequence.hooks: 'parallel'`, the execution is bounded by the same limit of [`maxConcurrency`](/config/maxconcurrency).
+
 Vitest doesn't perform any smart analysis and doesn't create additional workers to run these tests. This means that the performance of your tests will improve only if you rely heavily on asynchronous operations. For example, these tests will still run one after another even though the `concurrent` option is specified. This is because they are synchronous:
 
 ```ts

packages/runner/src/run.ts

@@ -18,6 +18,7 @@ import type {
   TestContext,
   WriteableTestContext,
 } from './types/tasks'
+import type { ConcurrencyLimiter } from './utils/limit-concurrency'
 import { processError } from '@vitest/utils/error' // TODO: load dynamically
 import { shuffle } from '@vitest/utils/helpers'
 import { getSafeTimers } from '@vitest/utils/timers'
@@ -35,6 +36,7 @@ import { hasFailed, hasTests } from './utils/tasks'
 const now = globalThis.performance ? globalThis.performance.now.bind(globalThis.performance) : Date.now
 const unixNow = Date.now
 const { clearTimeout, setTimeout } = getSafeTimers()
+let limitMaxConcurrency: ConcurrencyLimiter
 
 /**
  * Normalizes retry configuration to extract individual values.
@@ -141,7 +143,7 @@ async function callTestHooks(
 
   if (sequence === 'parallel') {
     try {
-      await Promise.all(hooks.map(fn => fn(test.context)))
+      await Promise.all(hooks.map(fn => limitMaxConcurrency(() => fn(test.context))))
     }
     catch (e) {
       failTask(test.result!, e, runner.config.diffOptions)
@@ -150,7 +152,7 @@ async function callTestHooks(
   else {
     for (const fn of hooks) {
       try {
-        await fn(test.context)
+        await limitMaxConcurrency(() => fn(test.context))
       }
       catch (e) {
         failTask(test.result!, e, runner.config.diffOptions)
@@ -188,11 +190,13 @@ export async function callSuiteHook<T extends keyof SuiteHooks>(
   }
 
   async function runHook(hook: Function) {
-    return getBeforeHookCleanupCallback(
-      hook,
-      await hook(...args),
-      name === 'beforeEach' ? args[0] as TestContext : undefined,
-    )
+    return limitMaxConcurrency(async () => {
+      return getBeforeHookCleanupCallback(
+        hook,
+        await hook(...args),
+        name === 'beforeEach' ? args[0] as TestContext : undefined,
+      )
+    })
   }
 
   if (sequence === 'parallel') {
@@ -311,6 +315,8 @@ async function callAroundHooks<THook extends Function>(
     let useCalled = false
     let setupTimeout: ReturnType<typeof createTimeoutPromise>
     let teardownTimeout: ReturnType<typeof createTimeoutPromise> | undefined
+    let setupLimitConcurrencyRelease: (() => void) | undefined
+    let teardownLimitConcurrencyRelease: (() => void) | undefined
 
     // Promise that resolves when use() is called (setup phase complete)
     let resolveUseCalled!: () => void
@@ -352,17 +358,22 @@ async function callAroundHooks<THook extends Function>(
 
       // Setup phase completed - clear setup timer
       setupTimeout.clear()
+      setupLimitConcurrencyRelease?.()
 
       // Run inner hooks - don't time this against our teardown timeout
       await runNextHook(index + 1).catch(e => hookErrors.push(e))
 
+      teardownLimitConcurrencyRelease = await limitMaxConcurrency.acquire()
+
       // Start teardown timer after inner hooks complete - only times this hook's teardown code
       teardownTimeout = createTimeoutPromise(timeout, 'teardown', stackTraceError)
 
       // Signal that use() is returning (teardown phase starting)
       resolveUseReturned()
     }
 
+    setupLimitConcurrencyRelease = await limitMaxConcurrency.acquire()
+
     // Start setup timeout
     setupTimeout = createTimeoutPromise(timeout, 'setup', stackTraceError)
 
@@ -381,6 +392,10 @@ async function callAroundHooks<THook extends Function>(
       catch (error) {
         rejectHookComplete(error as Error)
       }
+      finally {
+        setupLimitConcurrencyRelease?.()
+        teardownLimitConcurrencyRelease?.()
+      }
     })()
 
     // Wait for either: use() to be called OR hook to complete (error) OR setup timeout
@@ -392,6 +407,7 @@ async function callAroundHooks<THook extends Function>(
       ])
     }
     finally {
+      setupLimitConcurrencyRelease?.()
       setupTimeout.clear()
     }
 
@@ -410,6 +426,7 @@ async function callAroundHooks<THook extends Function>(
       ])
     }
     finally {
+      teardownLimitConcurrencyRelease?.()
       teardownTimeout?.clear()
     }
   }
@@ -524,7 +541,7 @@ async function callCleanupHooks(runner: VitestRunner, cleanups: unknown[]) {
         if (typeof fn !== 'function') {
           return
         }
-        await fn()
+        await limitMaxConcurrency(() => fn())
       }),
     )
   }
@@ -533,7 +550,7 @@ async function callCleanupHooks(runner: VitestRunner, cleanups: unknown[]) {
       if (typeof fn !== 'function') {
         continue
       }
-      await fn()
+      await limitMaxConcurrency(() => fn())
     }
   }
 }
@@ -623,7 +640,7 @@ export async function runTest(test: Test, runner: VitestRunner): Promise<void> {
           ))
 
           if (runner.runTask) {
-            await $('test.callback', () => runner.runTask!(test))
+            await $('test.callback', () => limitMaxConcurrency(() => runner.runTask!(test)))
           }
           else {
             const fn = getFn(test)
@@ -632,7 +649,7 @@ export async function runTest(test: Test, runner: VitestRunner): Promise<void> {
                 'Test function is not found. Did you add it using `setFn`?',
               )
             }
-            await $('test.callback', () => fn())
+            await $('test.callback', () => limitMaxConcurrency(() => fn()))
           }
 
           await runner.onAfterTryTask?.(test, {
@@ -940,12 +957,10 @@ export async function runSuite(suite: Suite, runner: VitestRunner): Promise<void
   }
 }
 
-let limitMaxConcurrency: ReturnType<typeof limitConcurrency>
-
 async function runSuiteChild(c: Task, runner: VitestRunner) {
   const $ = runner.trace!
   if (c.type === 'test') {
-    return limitMaxConcurrency(() => $(
+    return $(
       'run.test',
       {
         'vitest.test.id': c.id,
@@ -957,7 +972,7 @@ async function runSuiteChild(c: Task, runner: VitestRunner) {
         'code.column.number': c.location?.column,
       },
       () => runTest(c, runner),
-    ))
+    )
   }
   else if (c.type === 'suite') {
     return $(

packages/runner/src/utils/limit-concurrency.ts

@@ -1,10 +1,16 @@
 // A compact (code-wise, probably not memory-wise) singly linked list node.
 type QueueNode<T> = [value: T, next?: QueueNode<T>]
 
+export interface ConcurrencyLimiter extends ConcurrencyLimiterFn {
+  acquire: () => (() => void) | Promise<() => void>
+}
+
+type ConcurrencyLimiterFn = <Args extends unknown[], T>(func: (...args: Args) => PromiseLike<T> | T, ...args: Args) => Promise<T>
+
 /**
  * Return a function for running multiple async operations with limited concurrency.
  */
-export function limitConcurrency(concurrency: number = Infinity): <Args extends unknown[], T>(func: (...args: Args) => PromiseLike<T> | T, ...args: Args) => Promise<T> {
+export function limitConcurrency(concurrency: number = Infinity): ConcurrencyLimiter {
   // The number of currently active + pending tasks.
   let count = 0
 
@@ -30,28 +36,50 @@ export function limitConcurrency(concurrency: number = Infinity): <Args extends
     }
   }
 
-  return (func, ...args) => {
-    // Create a promise chain that:
-    //  1. Waits for its turn in the task queue (if necessary).
-    //  2. Runs the task.
-    //  3. Allows the next pending task (if any) to run.
-    return new Promise<void>((resolve) => {
-      if (count++ < concurrency) {
-        // No need to queue if fewer than maxConcurrency tasks are running.
-        resolve()
+  const acquire = () => {
+    let released = false
+    const release = () => {
+      if (!released) {
+        released = true
+        finish()
       }
-      else if (tail) {
+    }
+
+    if (count++ < concurrency) {
+      return release
+    }
+
+    return new Promise<() => void>((resolve) => {
+      if (tail) {
         // There are pending tasks, so append to the queue.
-        tail = tail[1] = [resolve]
+        tail = tail[1] = [() => resolve(release)]
       }
       else {
         // No other pending tasks, initialize the queue with a new tail and head.
-        head = tail = [resolve]
+        head = tail = [() => resolve(release)]
       }
-    }).then(() => {
-      // Running func here ensures that even a non-thenable result or an
-      // immediately thrown error gets wrapped into a Promise.
-      return func(...args)
-    }).finally(finish)
+    })
   }
+
+  const limiterFn: ConcurrencyLimiterFn = (func, ...args) => {
+    function run(release: () => void) {
+      try {
+        const result = func(...args)
+        if (result instanceof Promise) {
+          return result.finally(release)
+        }
+        release()
+        return Promise.resolve(result)
+      }
+      catch (error) {
+        release()
+        return Promise.reject(error)
+      }
+    }
+
+    const release = acquire()
+    return release instanceof Promise ? release.then(run) : run(release)
+  }
+
+  return Object.assign(limiterFn, { acquire })
 }

packages/vitest/src/node/cli/cli-config.ts

@@ -733,7 +733,7 @@ export const cliOptionsConfig: VitestCLIOptions = {
     },
   },
   maxConcurrency: {
-    description: 'Maximum number of concurrent tests in a suite (default: `5`)',
+    description: 'Maximum number of concurrent tests and suites during test file execution (default: `5`)',
     argument: '<number>',
   },
   expect: {