CLI: Simplify and optimize process existence check for temp dir cleanup#3395
Merged
brandonpayton merged 4 commits intotrunkfrom Mar 15, 2026
Merged
CLI: Simplify and optimize process existence check for temp dir cleanup#3395brandonpayton merged 4 commits intotrunkfrom
brandonpayton merged 4 commits intotrunkfrom
Conversation
The stale temp directory cleanup was using ps-man, which spawns `tasklist /v /FO CSV` on Windows to check if a process is still running. Each call takes ~1-4 seconds on Windows, and they run sequentially — one per stale temp directory. With a few stale dirs, this added 5-7+ seconds to every CLI boot. Replace with process.kill(pid, 0), which is a standard Node.js/POSIX idiom that checks process existence without sending a signal. It completes in ~0.02ms per call (85,000x faster than tasklist). Error handling: only treat ESRCH (no such process) as confirmation that the process is gone. For EPERM (no permission) or any other error, conservatively assume the process still exists to avoid accidentally deleting a temp dir that may still be in use. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
brandonpayton
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the ps-man dependency and switches the Playground CLI’s temp-dir “stale lock” detection to Node’s built-in process.kill(pid, 0) existence check, improving performance (notably on Windows) and simplifying process checks.
Changes:
- Replaced
ps-manprocess listing withprocess.kill(pid, 0)for process existence checks. - Converted
doesProcessExistfrom async to sync and updated callers accordingly. - Removed
ps-manfrom root dependencies.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/playground/cli/src/temp-dir.ts | Reworks process-existence logic to use process.kill(pid, 0) and adjusts stale temp-dir detection accordingly. |
| package.json | Drops ps-man dependency now that it’s no longer used. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Member
Author
|
This is simple and low-risk and has tests. Let's go ahead and merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ps-mannpm package with Node.js built-inprocess.kill(pid, 0)for checking whether a process is still runningprocess.kill(pid, 0)sends signal 0 (no actual signal) which tests process existence without side effectsTest plan