fix: ensure server process tree is fully terminated on macOS/Linux #596
Conversation
Previously, the Electron app only sent SIGTERM to the server process on Unix systems, which could leave child processes running and cause port conflicts when restarting the app. This change uses pkill to terminate all child processes and SIGKILL for reliable termination. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @sikbrad, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue in the Electron application where the server process and its child processes were not reliably terminated on macOS/Linux, leading to port conflicts and preventing successful app restarts. The changes introduce a more aggressive and comprehensive process termination strategy, ensuring that all related processes are properly shut down, thereby improving the application's stability and restart reliability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCentralized and consolidated server shutdown logic in the Electron main process by adding Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as "Electron App"
participant Cleanup as "cleanupServerProcess"
participant Server as "serverProcess"
participant OS as "OS (taskkill/pkill)"
participant Static as "staticServer"
App->>Cleanup: on 'before-quit' / 'window-all-closed' (reason)
Cleanup->>Server: if Windows -> taskkill /t <pid>
Cleanup->>OS: run: taskkill /t /pid <pid> (Windows) (rgba(0,128,0,0.5))
Cleanup->>Server: if non-Windows -> pkill -P <pid>
Cleanup->>OS: run: pkill -P <pid> (ignore errors) (rgba(0,0,255,0.5))
Cleanup->>OS: run: kill -9 <pid> (force) (rgba(255,0,0,0.5))
Cleanup->>Static: if staticServer -> close()
Cleanup->>App: set serverProcess = null
App->>App: if not macOS and from 'window-all-closed' -> quit()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves an issue with orphaned server processes on macOS and Linux by implementing a more robust termination strategy using pkill -P and SIGKILL. The changes are applied to both relevant application lifecycle events. My main feedback is to address the significant code duplication between the window-all-closed and before-quit event handlers by extracting the process termination logic into a shared helper function. This will improve the code's maintainability and prevent future inconsistencies.
apps/ui/src/main.ts
Outdated
| // Unix/Linux: kill entire process tree | ||
| try { | ||
| execSync(`pkill -P ${serverProcess.pid}`, { stdio: 'ignore' }); | ||
| } catch { | ||
| // pkill returns non-zero if no processes found, ignore | ||
| } | ||
| try { | ||
| process.kill(serverProcess.pid, 'SIGKILL'); | ||
| } catch { | ||
| // Process may already be dead | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for terminating the server process tree is nearly identical to the logic in the before-quit event handler on lines 815-828. Duplicating this code makes it harder to maintain and increases the risk of introducing inconsistencies in the future.
To improve this, I recommend extracting the entire server termination logic (for both Windows and Unix-like systems) into a single helper function. This function can then be called from both window-all-closed and before-quit handlers.
Here's an example of how you could structure it:
function cleanupServerProcess(reason: 'window-closed' | 'quitting'): void {
if (!serverProcess || !serverProcess.pid) {
return;
}
const logMessage = reason === 'window-closed'
? 'All windows closed, stopping server...'
: 'Stopping server...';
logger.info(logMessage);
if (process.platform === 'win32') {
try {
execSync(`taskkill /f /t /pid ${serverProcess.pid}`, { stdio: 'ignore' });
} catch (error) {
logger.error('Failed to kill server process:', (error as Error).message);
}
} else {
// Unix/macOS: kill entire process tree
try {
execSync(`pkill -P ${serverProcess.pid}`, { stdio: 'ignore' });
} catch {
// pkill returns non-zero if no processes found, ignore
}
try {
process.kill(serverProcess.pid, 'SIGKILL');
} catch {
// Process may already be dead
}
}
serverProcess = null;
}
// Then call it from the handlers:
app.on('window-all-closed', () => {
if (process.platform !== 'darwin') {
cleanupServerProcess('window-closed');
// ... rest of the handler
}
});
app.on('before-quit', () => {
cleanupServerProcess('quitting');
// ... rest of the handler
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/ui/src/main.ts`:
- Around line 815-828: The comment incorrectly states "SIGTERM first, then
SIGKILL" while the code only sends SIGKILL to the server process; update the
comment near the shutdown block that references serverProcess, execSync and
process.kill to accurately describe behavior (e.g., "Use pkill to terminate
child processes, then send SIGKILL to the server process; SIGTERM is not issued
here") or implement the intended SIGTERM-then-SIGKILL sequence by sending
SIGTERM to serverProcess, waiting briefly for exit, and only then sending
SIGKILL if still alive; adjust comments to match whichever approach you choose
so execSync(`pkill -P ${serverProcess.pid}`), process.kill(serverProcess.pid,
'SIGTERM')/SIGKILL and related try/catch blocks are consistent.
Address PR review feedback: - Extract duplicate process termination logic into cleanupServerProcess() - Fix misleading comment about SIGTERM (code only uses SIGKILL) - Consolidate static server cleanup into the same helper Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/main.ts (1)
764-802: Add SIGKILL flag to pkill to guarantee child process termination.
pkill -Pdefaults to SIGTERM, which child processes can ignore. This contradicts the stated goal of preventing orphaned processes and port conflicts on restart. Align with the Windows strategy by usingpkill -KILLto guarantee termination of child processes that may not respond to SIGTERM.🔧 Proposed hard-kill for child processes
- // Unix/macOS: kill child processes with pkill, then SIGKILL the server + // Unix/macOS: SIGKILL child processes with pkill, then SIGKILL the server try { - execSync(`pkill -P ${serverProcess.pid}`, { stdio: 'ignore' }); + execSync(`pkill -KILL -P ${serverProcess.pid}`, { stdio: 'ignore' }); } catch { // pkill returns non-zero if no processes found, ignore }
|
/gemini review |
|
✅ Actions performedReview triggered.
|
|
/gemini review |
|
Hello @sikbrad, I've received your request for a code review and will begin processing it shortly. Please note that the review will be posted as a separate comment once completed. |
|
✅ Actions performedReview triggered.
|
1 participant
Summary
pkill -Pto kill all child processes of the serverSIGKILLinstead ofSIGTERMfor reliable process terminationProblem
When restarting the built Electron app (
build:electron:mac:dir), the previous server process and its children were not properly terminated. This caused port 3008/3007 to remain occupied, preventing the app from restarting correctly.Solution
pkill -P <pid>before killing the parent processSIGKILLfor guaranteed termination (SIGTERM can be ignored by processes)window-all-closedandbefore-quitevent handlersTest plan
npm run build:electron:mac:dir🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.