Repository navigation
fix(cli): Destroy the worker pool after the initial compilation - #130
你好怪凡 (higuaifan) wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 35c843e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment was marked as outdated.
This comment was marked as outdated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35c843e186
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| } | ||
| }); | ||
| }) | ||
| .finally(() => workers.destroy()); |
There was a problem hiding this comment.
This package's build script runs tsc, but packages/cli/tsconfig.json only includes ES2019.Array, ES2020.Promise, and ES2015 in lib; that combination declares Promise.allSettled but not Promise.prototype.finally. With this new call the package no longer type-checks (Property 'finally' does not exist on type 'Promise<...>'), so the CLI build will fail until the lib is updated or the cleanup is expressed without finally.
Useful? React with 👍 / 👎.
## Problem `initialCompilation()` in `packages/cli/src/swc/dir.ts` creates a Piscina worker pool and does not destroy it. A one-shot CLI run exits, so the pool does not stay. But long-lived processes call `swcDir()` as a library. For example, the `@nestjs/cli` swc builder calls `swcDir()` on each file-add event in watch mode. Each call leaks a pool with `minThreads` idle worker threads. ## Change - Wrap the pool usage in `try`/`finally` and call `await workers.destroy()` in the `finally` block. The pool is released when the compilation succeeds and when it fails. - Use `try`/`finally` instead of `Promise.prototype.finally`. The `lib` setting in `packages/cli/tsconfig.json` does not declare `Promise.prototype.finally`. This was the review finding on #130. - Add `packages/cli/src/swc/__tests__/dir.test.ts`. The test mocks `piscina` and checks that `destroy()` is called once after a successful compilation and once after a failed compilation. - Add a patch changeset for `@swc/cli`. The watch path compiles with `dirWorker` directly and does not use this pool, so watch behavior does not change. This change is based on the investigation and the approach in #130 by the issue author. ## Validation All local checks below ran on commit e661703: - `pnpm exec tsc --noEmit` in `packages/cli`: passed. - `pnpm exec jest` in `packages/cli`: 8 suites, 65 tests passed. - Without the `dir.ts` change, the 2 new tests in `dir.test.ts` failed. - `prettier --check` on the changed source and test files: passed. Closes #126 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: 你好怪凡 <9988024+higuaifan@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Closes #126.
initialCompilation()never destroys thePiscinapool it creates on the async path, so everyswcDir()call permanently pinsminThreads(= cores / 2) idle workers. Root cause, measurements and a nest-free repro are in the issue..finally(() => workers.destroy())on the pool's task chain. The watch path compiles viadirWorkerdirectly and never touches this pool, so behavior is unchanged.src/swc/__tests__/dir.test.tscovering pool teardown after both a successful and a failed compilation. Both assertions fail without the fix..then()block is re-indented by the added chain link — the functional change is one line.Investigated and engineered by Claude; I'm following up on this PR.