-
Notifications
You must be signed in to change notification settings - Fork 8
feat(main): trying to always kill thv process #1159
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the cleanup of the ToolHive child process to ensure it's always terminated when the application exits, regardless of the exit scenario. The changes add multiple layers of protection across different exit paths and improve the graceful shutdown process.
Key changes:
- Enhanced
stopToolhive()with graceful termination (SIGTERM) followed by forced kill (SIGKILL) after 2s timeout - Added
windowsHide: truespawn option to leverage Windows job objects for automatic child process cleanup - Implemented comprehensive exit handlers:
quit,session-end(Windows-specific),exit,SIGTERM, andSIGINT
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| main/src/toolhive-manager.ts | Enhanced process spawning with Windows job object support and improved stopToolhive() with graceful-then-forced termination |
| main/src/main.ts | Added multiple exit event handlers for comprehensive cleanup coverage across different quit scenarios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧
Ensures the ToolHive child process always gets cleaned up when the app exits, no matter how it quits. We've added multiple layers of protection to catch every exit scenario:
quit,will-quit) - normal user quitssession-endhandler on BrowserWindow - catches Windows shutdown/restart/logout (where normal quit events don't fire!)exit,SIGTERM,SIGINT) - force kills and Docker stopsstopToolhive()- tries graceful SIGTERM first, then force SIGKILL after 2s timeoutdetached: false+windowsHide: trueso Windows job objects auto-kill child processesI tried multiple way including the upgrade, and it seems consistent
I hope the quit and exit callbacks will cover what we were missing