-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(windows): prevent zombie process accumulation on app close #1258
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
Changes from all commits
864535f
791e661
5a0875f
edf0b94
0f9512c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,9 @@ const mockProcess = Object.assign(new EventEmitter(), { | |
| killed: false, | ||
| kill: vi.fn(() => { | ||
| mockProcess.killed = true; | ||
| // Emit exit event synchronously to simulate process termination | ||
| // (needed for killAllProcesses wait - using nextTick for more predictable timing) | ||
| process.nextTick(() => mockProcess.emit('exit', 0, null)); | ||
| return true; | ||
| }) | ||
| }); | ||
|
|
@@ -290,7 +293,12 @@ describe('Subprocess Spawn Integration', () => { | |
| const result = manager.killTask('task-1'); | ||
|
|
||
| expect(result).toBe(true); | ||
| expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM'); | ||
| // On Windows, kill() is called without arguments; on Unix, kill('SIGTERM') is used | ||
| if (process.platform === 'win32') { | ||
| expect(mockProcess.kill).toHaveBeenCalled(); | ||
| } else { | ||
| expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM'); | ||
| } | ||
|
Comment on lines
+296
to
+301
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion | π Major π§© Analysis chainπ Script executed: # Find platform-related utility files
fd -t f -i platform . --extension ts --extension tsx --extension js | head -20Repository: AndyMik90/Auto-Claude Length of output: 164 π Script executed: # Check the test file to understand its structure and imports
head -50 apps/frontend/src/__tests__/integration/subprocess-spawn.test.tsRepository: AndyMik90/Auto-Claude Length of output: 1804 π Script executed: # Search for platform detection functions in the codebase
rg -t ts -t tsx "isWindows|isMacOS|isLinux" --max-count 10Repository: AndyMik90/Auto-Claude Length of output: 92 π Script executed: # Check if process.platform is used elsewhere in tests
rg -t ts "process\.platform" apps/frontend/src/__tests__/ -B 2 -A 2Repository: AndyMik90/Auto-Claude Length of output: 622 π Script executed: # Check the platform abstraction module
cat -n apps/frontend/src/shared/platform.tsRepository: AndyMik90/Auto-Claude Length of output: 2080 π Script executed: # Check the platform tests to see how they handle multi-platform testing
cat -n apps/frontend/src/main/platform/__tests__/platform.test.ts | head -100Repository: AndyMik90/Auto-Claude Length of output: 3234 π Script executed: # Look at the full test file context around the problematic code
sed -n '280,310p' apps/frontend/src/__tests__/integration/subprocess-spawn.test.tsRepository: AndyMik90/Auto-Claude Length of output: 1170 π Script executed: # Check imports in the test file
grep -n "^import\|^from" apps/frontend/src/__tests__/integration/subprocess-spawn.test.tsRepository: AndyMik90/Auto-Claude Length of output: 363 Parameterize and mock platform detection to test all three platforms. This test uses π€ Prompt for AI Agents |
||
| expect(manager.isRunning('task-1')).toBe(false); | ||
| }); | ||
|
|
||
|
|
||
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.
These two loops for copying DLLs to different destinations contain duplicated logic. To improve maintainability and reduce redundancy, you can refactor this into a single loop that iterates over a list of target destinations.