fix: kill entire process group on quit to prevent stale server processes#656
Open
fix: kill entire process group on quit to prevent stale server processes#656
Conversation
Previously, stopServer() only sent SIGINT to the direct child (uv), leaving the Python server orphaned and holding the port. This caused "No available ports found" errors on next launch. Now spawns with detached:true on Unix and kills the entire process group with SIGKILL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Rafał Leszko <rafal@livepeer.org>
📝 WalkthroughWalkthroughUpdated .gitignore to exclude TypeScript build info files. Refactored Python process spawning and termination logic to create detached process groups on Unix systems, enabling efficient tree-wide process cleanup via negative PID signals instead of per-process termination. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/services/pythonProcess.ts`:
- Around line 197-220: The stop() method currently sets this.intentionalStop =
true and clears this.serverProcess before verifying there is a PID and before
the old child actually emits its exit events, which can leave the flag stuck or
allow a stale close/error handler to clear a new child; move the lifecycle
changes so you first check if (!this.serverProcess?.pid) return, then perform
the platform-specific kill and only after the child has actually exited (or
after successful kill) set this.serverProcess = null and this.intentionalStop =
true as appropriate. Also update the child event handlers (child.on('close') and
child.on('error')) to only clear this.serverProcess if this.serverProcess ===
child and to reset this.intentionalStop (e.g., this.intentionalStop = false) in
close handling so the flag reflects the real lifecycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51193ce6-5b4c-47d4-851e-a4959ece972b
📒 Files selected for processing (2)
.gitignoreapp/src/services/pythonProcess.ts
Comment on lines
197
to
+220
| this.intentionalStop = true; // Mark as intentional to prevent respawn on exit code 42 | ||
| if (this.serverProcess) { | ||
| logger.info('Stopping server...'); | ||
| const pid = this.serverProcess.pid; | ||
|
|
||
| if (process.platform === 'win32' && pid) { | ||
| // On Windows, kill the entire process tree using taskkill | ||
| // This ensures child processes (like the Python server spawned by uv) are also terminated | ||
| try { | ||
| execSync(`taskkill /PID ${pid} /T /F`, { stdio: 'ignore' }); | ||
| logger.info(`Killed process tree for PID ${pid}`); | ||
| } catch (err) { | ||
| logger.warn(`Failed to kill process tree: ${err}`); | ||
| // Fallback to regular kill | ||
| this.serverProcess.kill('SIGINT'); | ||
| } | ||
| } else { | ||
| // On Unix-like systems, SIGINT should propagate to child processes | ||
| this.serverProcess.kill('SIGINT'); | ||
| } | ||
|
|
||
| this.serverProcess = null; | ||
| if (!this.serverProcess?.pid) return; | ||
|
|
||
| logger.info('Stopping server...'); | ||
| const pid = this.serverProcess.pid; | ||
| this.serverProcess = null; | ||
|
|
||
| if (process.platform === 'win32') { | ||
| // On Windows, kill the entire process tree using taskkill | ||
| try { | ||
| execSync(`taskkill /PID ${pid} /T /F`, { stdio: 'ignore' }); | ||
| logger.info(`Killed process tree for PID ${pid}`); | ||
| } catch (err) { | ||
| logger.warn(`Failed to kill process tree: ${err}`); | ||
| } | ||
| } else { | ||
| // On Unix, kill the entire process group (negative PID) since we spawned with detached: true | ||
| try { | ||
| process.kill(-pid, 'SIGKILL'); | ||
| logger.info(`Killed process group ${pid}`); | ||
| } catch (err) { | ||
| logger.warn(`Failed to kill process group: ${err}`); | ||
| } |
There was a problem hiding this comment.
Move the lifecycle state changes after the PID check and actual child exit.
Setting intentionalStop before the PID guard leaves the flag stuck true on a no-op stop, and clearing serverProcess before the old child emits close lets that stale handler wipe out a replacement child reference. Both cases break restart/error handling.
💡 Proposed fix
stopServer(): void {
- this.intentionalStop = true; // Mark as intentional to prevent respawn on exit code 42
-
- if (!this.serverProcess?.pid) return;
+ const child = this.serverProcess;
+ if (!child?.pid) return;
+ this.intentionalStop = true; // Mark as intentional to prevent respawn on exit code 42
logger.info('Stopping server...');
- const pid = this.serverProcess.pid;
- this.serverProcess = null;
+ const pid = child.pid;And in the existing handlers, only clear the field if the event belongs to the currently tracked child:
child.on('close', (code, signal) => {
const wasIntentionalStop = this.intentionalStop;
if (this.serverProcess === child) {
this.serverProcess = null;
}
this.intentionalStop = false;
// ...
});
child.on('error', (err) => {
// ...
if (this.serverProcess === child) {
this.serverProcess = null;
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/services/pythonProcess.ts` around lines 197 - 220, The stop() method
currently sets this.intentionalStop = true and clears this.serverProcess before
verifying there is a PID and before the old child actually emits its exit
events, which can leave the flag stuck or allow a stale close/error handler to
clear a new child; move the lifecycle changes so you first check if
(!this.serverProcess?.pid) return, then perform the platform-specific kill and
only after the child has actually exited (or after successful kill) set
this.serverProcess = null and this.intentionalStop = true as appropriate. Also
update the child event handlers (child.on('close') and child.on('error')) to
only clear this.serverProcess if this.serverProcess === child and to reset
this.intentionalStop (e.g., this.intentionalStop = false) in close handling so
the flag reflects the real lifecycle.
Contributor
🚀 fal.ai Preview Deployment
TestingConnect to this preview deployment by running this on your branch: 🧪 E2E tests will run automatically against this deployment. |
Contributor
✅ E2E Tests passed
Test ArtifactsCheck the workflow run for screenshots. |
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
1 participant
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
detached: trueon Unix to create a dedicated process groupstopServer()now kills the entire process group (process.kill(-pid, 'SIGKILL')) instead of just sending SIGINT to the direct child (uv), ensuring the Python server is also terminated*.tsbuildinfoto.gitignoreProblem
Users were seeing "No available ports found after 5 attempts starting from 52178" because
stopServer()only sent SIGINT to theuvprocess, leaving the Python server orphaned and holding the port across app restarts.Test plan
lsof -i :52178)taskkill /T /Fstill works correctly🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores