Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,16 @@ export class VitestExecutor implements TestExecutor {

async [Symbol.asyncDispose](): Promise<void> {
this.debugLog(DebugLogLevel.Info, 'Disposing VitestExecutor: Closing Vitest instance.');
await this.vitest?.close();
if (this.vitest && !this.options.watch) {
// In run (non-watch) mode, mirror the `vitest run` CLI by using `exit()`, which arms an
// unref'd `teardownTimeout` safety net (`process.exit()`) before closing. `close()` alone
// can wait indefinitely when a worker or service keeps the event loop alive, leaving the
// process hanging after all tests have completed.
// See https://github.com/angular/angular-cli/issues/32832
await this.vitest.exit();
} else {
await this.vitest?.close();
}
Comment on lines +208 to +217
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

1. Defensive Programming Check

Since vitest is a dependency that might be resolved to different versions in user workspaces, it is safer to defensively check if exit is a function before calling it to prevent runtime TypeError exceptions.

2. Programmatic Execution & process.exit() Side-Effects

Calling vitest.exit() arms a teardown timeout that eventually calls process.exit(). While this successfully prevents hanging in standard CLI runs, it can be problematic when the builder is executed programmatically (for example, via the Architect API in custom scripts, monorepo tools like Nx, or IDE integrations). In those cases, abruptly calling process.exit() will terminate the entire host process, preventing subsequent tasks or cleanup from running.

Consider adding a defensive check for typeof this.vitest.exit === 'function', and potentially exposing a way to opt-out of the force-exit behavior if run programmatically.

Suggested change
if (this.vitest && !this.options.watch) {
// In run (non-watch) mode, mirror the `vitest run` CLI by using `exit()`, which arms an
// unref'd `teardownTimeout` safety net (`process.exit()`) before closing. `close()` alone
// can wait indefinitely when a worker or service keeps the event loop alive, leaving the
// process hanging after all tests have completed.
// See https://github.com/angular/angular-cli/issues/32832
await this.vitest.exit();
} else {
await this.vitest?.close();
}
if (this.vitest && !this.options.watch && typeof this.vitest.exit === 'function') {
// In run (non-watch) mode, mirror the `vitest run` CLI by using `exit()`, which arms an
// unref'd `teardownTimeout` safety net (`process.exit()`) before closing. `close()` alone
// can wait indefinitely when a worker or service keeps the event loop alive, leaving the
// process hanging after all tests have completed.
// See https://github.com/angular/angular-cli/issues/32832
await this.vitest.exit();
} else {
await this.vitest?.close();
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. Responding to both points:

1. Defensive typeof check — declined for consistency: this file already calls other Vitest instance methods (close(), start(), invalidateFile()) without runtime guards. exit() is long-standing public API on the Vitest class — it is what the vitest run CLI itself calls on shutdown — covered by the package's vitest peer range and type-checked via the Vitest type from vitest/node.

2. process.exit() side-effects in programmatic execution — this trade-off was weighed when preparing the change. The key property of exit() is that its teardownTimeout safety net only fires in the pathological case where close() would never settle; in every healthy teardown, exit() and close() behave identically and process.exit() is never invoked. Comparing the scenarios:

  • CLI runs (the common case, and where @angular/build:unit-test vitest executor hangs indefinitely — uses close() instead of exit() #32832 is reported): infinite hang → bounded exit.
  • One-shot programmatic hosts hitting the hang: the host currently hangs forever as well; with this change it terminates after completing its work.
  • Long-lived in-process hosts (daemons) hitting the hang: the only scenario that regresses — the daemon previously survived with leaked handles and would now be terminated after teardownTimeout. Runners like Nx and IDE integrations typically execute builders in dedicated forked processes, which narrows this further.

If the maintainers consider the daemon scenario blocking, happy to gate this (e.g. only arm the safety net when the builder owns the process, or behind an option) — though that would give up protection precisely where most users hit #32832.

this.debugLog(DebugLogLevel.Info, 'Vitest instance closed.');
}

Expand Down
Loading