feat/make windows ebusy errors less fatal#31
Conversation
yjaaidi
commented
Apr 3, 2026
yjaaidi
commented
- Revert "feat(cook): ✨ move apps to tmp folder before removing them and turn removal errors into warnings (feat(cook): ✨ move apps to tmp folder before removing them and turn removal errors into warnings #26)"
- feat(cook): ✨ make windows EBUSY errors less fatal
There was a problem hiding this comment.
Pull request overview
This PR adjusts the tools/cook cleanup behavior to be more resilient on Windows by changing how app directories are removed, and simplifies the tools Vitest configuration in line with the removal of the prior “wide” filesystem integration test.
Changes:
- Updated
FileSystemAdapter.removeDirto delete directories directly withfs.rmSyncretries and downgrade removal failures to warnings. - Simplified
tools/vitest.config.mtsby removing the narrow/wide multi-project setup and running all*.spec.tstests. - Removed the prior wide filesystem test and added
rimrafto devDependencies.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/vitest.config.mts | Removes narrow/wide Vitest project split; runs all **/*.spec.ts under tools/. |
| tools/cook/infra.wide.spec.ts | Deletes the wide integration test that asserted “move-to-tmp then remove” behavior. |
| tools/cook/infra.ts | Changes directory removal to direct rmSync with retries and warning-on-failure behavior. |
| package.json | Adds rimraf as a devDependency. |
| pnpm-lock.yaml | Lockfile updates reflecting the added rimraf and related dependency graph changes. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "nx": "22.5.4", | ||
| "prettier": "3.2.5", | ||
| "reflect-metadata": "0.2.2", | ||
| "rimraf": "6.1.3", |
There was a problem hiding this comment.
rimraf is added to devDependencies but doesn't appear to be used anywhere in the repo (search only finds it in package.json / lockfile). This increases install surface area unnecessarily; either remove the dependency or switch the directory-removal implementation to use rimraf so the dependency is justified.
| import { readFileSync, writeFileSync } from 'fs'; | ||
| import { execSync } from 'node:child_process'; | ||
| import { mkdirSync, readdirSync, renameSync, rmSync } from 'node:fs'; |
There was a problem hiding this comment.
This file mixes node:fs and fs imports (readdirSync/rmSync vs readFileSync/writeFileSync). Using a single module specifier (preferably node:fs) improves consistency and also makes it easier to mock fs in Vitest (a vi.mock('node:fs') won’t affect imports from 'fs').
| import { readFileSync, writeFileSync } from 'fs'; | |
| import { execSync } from 'node:child_process'; | |
| import { mkdirSync, readdirSync, renameSync, rmSync } from 'node:fs'; | |
| import { execSync } from 'node:child_process'; | |
| import { | |
| mkdirSync, | |
| readFileSync, | |
| readdirSync, | |
| renameSync, | |
| rmSync, | |
| writeFileSync, | |
| } from 'node:fs'; |
| /** | ||
| * Windows antiviruses can be a pain (e.g. EBUSY errors). | ||
| * This method will try to remove the folder multiple times, | ||
| * then if it fails, it will try to move the folder to tmp/trash, | ||
| * and finally if that fails, it will warn the user that they may need to remove it manually. | ||
| * | ||
| * Note that I've tried to use rimraf and its backoff but that wasn't enough. | ||
| */ | ||
| removeDir(path: string): void { | ||
| mkdirSync(TRASH_PATH, { recursive: true }); | ||
| const targetPath = join(TRASH_PATH, `${basename(path)}-${Date.now()}`); | ||
| renameSync(path, targetPath); | ||
| try { |
There was a problem hiding this comment.
FileSystemAdapter.removeDir now relies on fs.rmSync retry behavior and intentionally downgrades failures to warnings. There isn’t test coverage for this behavior (e.g., verifying it doesn’t throw when rmSync fails and that a warning is emitted), and the previous integration-style test for removal behavior was removed. Adding a focused unit test (mocking rmSync) would help prevent regressions, especially on Windows EBUSY scenarios.
