fix(bail): fix race condition in bail #8971

nicojs · 2025-11-07T14:23:51Z

Description

Resolves #8852

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
Ideally, include a test that fails without this PR but passes with it.
Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.
Please check Allow edits by maintainers to make review process faster. Note that this option is not available for repositories that are owned by Github organizations.

Tests

Run the tests with pnpm test:ci.

Documentation

If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

(no new functionality)

Changesets

Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

netlify · 2025-11-07T14:25:21Z

✅ Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`d017c32`
🔍 Latest deploy log	https://app.netlify.com/projects/vitest-dev/deploys/690e00fa8d631f000870e520
😎 Deploy Preview	https://deploy-preview-8971--vitest-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

netlify · 2025-11-07T14:27:00Z

✅ Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`a791707`
🔍 Latest deploy log	https://app.netlify.com/projects/vitest-dev/deploys/6911aa5a6ddcd600082ba35d
😎 Deploy Preview	https://deploy-preview-8971--vitest-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

nicojs · 2025-11-07T14:47:42Z

I don't know why the tests are failing. @sheremet-va any idea?

sheremet-va · 2025-11-07T14:50:42Z

I don't know why the tests are failing. @sheremet-va any idea?

They are flaky. If you didn't touch them, don't worry.

As a note, your test needs to be inside test/cli, tests are separated in groups, see test/README

nicojs · 2025-11-08T11:21:23Z

Thanks for the review! 🙏

I've moved the test to test/cli.

nicojs · 2025-11-10T07:25:30Z

@sheremet-va I've improved the test, made it silent, assert happy path and clarified with some comments. What do you think?

AriPerkkio · 2025-11-10T07:30:15Z

packages/vitest/src/node/pools/pool.ts

+    // Wait for any ongoing cancellation to complete before accepting new tasks
+    if (this.cancellingPromise) {
+      await this.cancellingPromise


I wonder if this change will start some previous run's tests too. I have no idea why this._isCancelling was required before - @sheremet-va I think that's something you added?

I have no idea why this._isCancelling was required before

The comment explains it:

Prevent new tasks from being queued during cancellation

These kinds of checks exist to catch any unintended behaviours

Now that we just await here and then let it continue, doesn't it mean it's going to continue the run it was intended the cancel? Should it return instead?

No, that was the previous cancellation (because of bail). So it needs to wait for that to finish.

AriPerkkio · 2025-11-10T07:32:56Z

packages/vitest/src/node/pools/pool.ts

-    // Set flag to prevent new tasks from being queued
-    this._isCancelling = true


Pool should already have had all tests queued at this point. Or if there was another group, the ctx.isCancelling in node/pool.ts should have prevented them from being queued.

If this was true, there wouldn't have been an issue #8852

packages/vitest/src/node/pools/pool.ts

test/cli/test/bail-race.test.ts

AriPerkkio

To me this looks more like race condition in core.ts:

We start cancelling here:

vitest/packages/vitest/src/node/core.ts

Line 861 in 26d2b60

this.isCancelling = true

And it's then overwritten by

vitest/packages/vitest/src/node/core.ts

Lines 707 to 710 in 26d2b60

    
           // previous run 
        
           await this.runningPromise 
        
           this._onCancelListeners = [] 
        
           this.isCancelling = false

This is noticable with your reproduction. No idea why that happens.

nicojs · 2025-11-10T09:04:32Z

@AriPerkkio I now use your implementation with rounds >= 2, however the race condition will almost never happen within the first 3 rounds. Should we make it maybe 20 rounds to make sure?

I've implemented the changes requested by @sheremet-va

@AriPerkkio I'm not familiar enough with the code base to know how cancellation in core.ts is supposed to work. Maybe @sheremet-va knows the answer?

AriPerkkio · 2025-11-10T09:54:23Z

test/cli/test/bail-race.test.ts

+test('cancels previous run before starting new one', async () => {
+  const results: Record<string, unknown>[] = []
+
+  const { ctx: vitest, buildTestTree } = await runVitest({


Does this bug reproduce with runVitest? I'm only able to see it via createVitest.

I get reproduce it structurally, as long as I use pool: 'threads'

AriPerkkio · 2025-11-10T10:41:31Z

The underlying bug is rather in core.ts that keeps starting new runs even though it's still cancelling previous ones. I've pushed those fixes in #8991 to avoid overwriting changes in this PR.

nicojs added 3 commits November 7, 2025 15:17

Implement fix for bail race

3c62089

Add e2e test for bail-race

d017c32

Update add.spec.js

168da32

fix lint

4dea735

nicojs added 4 commits November 8, 2025 11:08

move e2e test to fixture

9f1b0ae

Reset lockfile

c3fa105

Remove unused test

add9b44

Merge branch 'main' into fix/bail-race

116c663

Remove unused test output

3fbb1bd

sheremet-va requested a review from AriPerkkio November 9, 2025 12:35

nicojs added 3 commits November 10, 2025 08:21

Silence bail-race test

92c168a

Add some verification of happy path

fe7f953

Clarify test

e175b1e

nicojs and others added 2 commits November 10, 2025 08:25

Merge branch 'main' into fix/bail-race

17181e5

test: simplify test case

cd33b57

AriPerkkio reviewed Nov 10, 2025

View reviewed changes

test: simplify fixtures

ce0d57e

sheremet-va requested changes Nov 10, 2025

View reviewed changes

packages/vitest/src/node/pools/pool.ts Outdated Show resolved Hide resolved

test/cli/test/bail-race.test.ts Outdated Show resolved Hide resolved

test/cli/test/bail-race.test.ts Show resolved Hide resolved

AriPerkkio requested changes Nov 10, 2025

View reviewed changes

nicojs added 4 commits November 10, 2025 09:56

Improve test using runVitest

e9c178f

Use buildTestTree

687f205

Reset to null inside finally

d0fa753

Use buildTestTree

7876307

Merge branch 'main' into fix/bail-race

a791707

AriPerkkio reviewed Nov 10, 2025

View reviewed changes

Improve error message when race condition occurs

6b53104

AriPerkkio mentioned this pull request Nov 10, 2025

fix(core): prevent starting new run when cancelling #8991

Merged

7 tasks

sheremet-va closed this in #8991 Nov 17, 2025

		// Set flag to prevent new tasks from being queued
		this._isCancelling = true

	// previous run
	await this.runningPromise
	this._onCancelListeners = []
	this.isCancelling = false

Uh oh!

fix(bail): fix race condition in bail #8971

fix(bail): fix race condition in bail #8971

Conversation

nicojs commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Documentation

Changesets

Uh oh!

netlify bot commented Nov 7, 2025

✅ Deploy Preview for vitest-dev ready!

Uh oh!

netlify bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for vitest-dev ready!

Uh oh!

nicojs commented Nov 7, 2025

Uh oh!

sheremet-va commented Nov 7, 2025

Uh oh!

nicojs commented Nov 8, 2025

Uh oh!

nicojs commented Nov 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheremet-va Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AriPerkkio left a comment

Choose a reason for hiding this comment

Uh oh!

nicojs commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AriPerkkio commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicojs commented Nov 7, 2025 •

edited

Loading

netlify bot commented Nov 7, 2025 •

edited

Loading

sheremet-va Nov 10, 2025 •

edited

Loading

nicojs commented Nov 10, 2025 •

edited

Loading