-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: improve windows background process handling and cleanup #1146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📋 Review SummaryThis PR significantly improves Windows background process handling and overall process cleanup reliability. The changes address critical issues with background command execution on Windows by implementing a proper keep-alive mechanism and adding robust cleanup procedures for both PTYs and child processes. Additionally, timeout support has been added to prevent commands from hanging indefinitely. 🔍 General Feedback
🎯 Specific Feedback🔴 CriticalNo specific issues identified in this review. 🟡 HighNo specific issues identified in this review. 🟢 Medium
🔵 Low
✅ Highlights
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
…ackground-terminal-execute-x
PR: Fix Windows Background Terminal Execution & Process Cleanup
📝 Description
This PR addresses critical issues with process management on Windows, specifically focusing on background task execution and reliable process cleanup upon application exit.
Key Changes
Problem: Previously, child processes spawned via child_process (fallback mode) were not tracked, leading to orphaned processes after the main application exited. Additionally, using asynchronous spawn inside the process.on('exit') hook is ineffective because the Node.js event loop terminates immediately, preventing the cleanup command from ever executing.
Solution:
Introduced activeChildProcesses to track all spawned child process PIDs.
Implemented a robust cleanup() method hooked to process.on('exit').
Crucial Fix: Switched to spawnSync for the cleanup command. Since the Node.js event loop stops during the exit phase, asynchronous commands are ignored. spawnSync blocks the main process execution, ensuring the taskkill command is fully sent to the OS before the application terminates.
On Windows, uses taskkill /f /t /pid to forcefully terminate the entire process tree, ensuring no subprocesses are left behind.
Related Issue
Demo / Videos
Before Fix (Incorrect Behavior):
badCut.mp4
After Fix (Correct Behavior):
goodCut.1.mp4