fix(server): prevent Windows memory leak from orphaned Node.js processes #666
Conversation
|
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. 📝 WalkthroughWalkthroughThis PR adds cross-platform process cleanup utilities and integrates them throughout the server. It introduces Windows-specific process tree termination, startup cleanup routines, and feature stop-cleanup coordination with graceful timeout handling. Changes
Sequence DiagramssequenceDiagram
participant Server as Server Startup
participant AMS as AutoModeService
participant Util as ProcessUtils
participant OS as OS/Platform
Server->>AMS: initialize()
activate AMS
AMS->>Util: cleanupOrphanedProcessesOnStartup()
activate Util
Util->>OS: Detect Windows platform
alt Windows
Util->>OS: killProcessTree(orphaned_pids)
else Unix
Note over Util: Skip cleanup
end
deactivate Util
AMS-->>Server: Ready
deactivate AMS
sequenceDiagram
participant Client as Client/Endpoint
participant Route as StopFeature Route
participant AMS as AutoModeService
participant RF as RunningFeature
participant Util as ProcessUtils
Client->>Route: stopFeature(featureId, waitForCleanup=true)
activate Route
Route->>AMS: stopFeature(featureId, true, 10000ms)
activate AMS
AMS->>RF: signalCompletion() invoke
activate RF
RF->>Util: killProcessTree() / cleanup start
activate Util
deactivate Util
RF-->>RF: completionPromise resolves
deactivate RF
alt waitForCleanup = true
AMS->>AMS: await completionPromise (with timeout)
note over AMS: Wait up to 10000ms for cleanup
end
AMS->>AMS: Remove from running features map
AMS-->>Route: true (success)
deactivate AMS
Route-->>Client: 200 OK
deactivate Route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
Summary of ChangesHello @ScotTFO, 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 addresses a critical memory leak issue on Windows where Node.js processes, particularly those spawned by dev servers and other agents, would become orphaned and accumulate over time, leading to significant memory consumption. The core solution involves integrating a cross-platform process tree termination utility ( 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
|
![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 introduces a robust solution to prevent memory leaks from orphaned processes on Windows by integrating the tree-kill package. The changes are well-structured, with a new cross-platform process utility and its application across various services like auto-mode-service, dev-server-service, and subprocess. The addition of logic to wait for feature cleanup is also a significant improvement. My main feedback concerns the use of execSync, which can block the server's event loop. I've suggested replacing it with its asynchronous counterpart to ensure the server remains responsive.
ScotTFO
force-pushed
the
fix/windows-memory-leak-auto-mode
branch
from
0929af2 to
0975042
Compare
On Windows, child processes spawned via cmd.exe or sh.exe don't terminate when the parent process is killed (unlike Unix where SIGTERM propagates). This caused dev servers and MCP processes to accumulate, consuming 30GB+ memory over time. Changes: - Add tree-kill integration for cross-platform process tree termination - Add cleanupOrphanedProcesses() to kill lingering processes when features complete - Add startup cleanup to catch orphans from previous runs - Fix PowerShell pattern matching (no backslash escaping needed for -like) - Fix PowerShell script syntax (semicolons required for single-line execution) - Apply tree-kill to subprocess.ts, dev-server-service, codex-app-server-service - Add pid to mock process in tests for tree-kill condition Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace execSync with execAsync in cleanupOrphanedProcessesOnStartup - Remove unused execSync import - Fix tests to mock killProcessTree for Windows process cleanup - Add HOSTNAME env var to tests for consistent URL assertions - Apply prettier formatting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/server/src/services/auto-mode-service.ts`:
- Around line 146-158: The skip logic in the pidsToKill filter is too broad
because automakerPaths includes 'automaker', causing any command line with
"\automaker\" to be skipped; update the automakerPaths array to only include
specific process indicators (e.g., ['tsx','vite']) and keep the existing check
in the pidsToKill filter (the automakerPaths.some(...) expression) so it only
skips processes when the command line contains both a specific indicator (like
'tsx' or 'vite') and the automaker path marker; ensure you reference the
automakerPaths constant and the pidsToKill filter (and variables serverPid,
process.ppid, cmdLower) when making the change.
- Around line 101-118: The PowerShell -like patterns interpolate normalizedDir
and unixStyleDir directly which breaks when folder names contain wildcard chars
([ ] * ? ) or single quotes; add an escaping helper (e.g.,
escapeForPowerShellLike) and use it before building psCommand: replace all
wildcard characters with a backtick-escaped version
(value.replace(/([*?\[\]])/g, '`$1')) and double any single quotes
(value.replace(/'/g, "''")) so both normalizedDir and unixStyleDir are escaped
prior to interpolation into psCommand (refer to normalizedDir, unixStyleDir, and
psCommand).
In `@libs/platform/src/subprocess.ts`:
- Around line 7-9: Replace the relative import of killProcessTree with the
shared package barrel import; specifically change the import that currently
references './process-utils.js' to the `@automaker` package entry (for example
import { killProcessTree } from '@automaker/process-utils') so the code uses the
canonical shared module export (ensure the imported symbol name killProcessTree
remains unchanged).
| try { | ||
| // For PowerShell's -like operator, we need to escape only special wildcard chars (*, ?, [) | ||
| // Backslashes are NOT special in -like, so don't escape them | ||
| // Just normalize forward slashes to backslashes for Windows paths | ||
| const normalizedDir = workDir.replace(/\//g, '\\'); | ||
|
|
||
| // Also create a Unix-style path variant (Git Bash uses /c/Users/... format) | ||
| const unixStyleDir = workDir | ||
| .replace(/^([A-Z]):/i, (_, drive) => `/${drive.toLowerCase()}`) | ||
| .replace(/\\/g, '/'); | ||
|
|
||
| // Search for multiple process types that might be in the chain | ||
| // sh.exe (Git Bash), cmd.exe (Windows shell), node.exe (Node.js) | ||
| const processTypes = ['node.exe', 'sh.exe', 'cmd.exe']; | ||
| const processFilter = processTypes.map((p) => `$_.Name -eq '${p}'`).join(' -or '); | ||
|
|
||
| // Get processes via PowerShell - match either Windows-style or Unix-style paths | ||
| const psCommand = `Get-CimInstance Win32_Process | Where-Object { (${processFilter}) -and ($_.CommandLine -like '*${normalizedDir}*' -or $_.CommandLine -like '*${unixStyleDir}*') } | Select-Object ProcessId, Name, CommandLine | ConvertTo-Json`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file
cat -n apps/server/src/services/auto-mode-service.ts | sed -n '95,125p'Repository: AutoMaker-Org/automaker
Length of output: 1776
🏁 Script executed:
# Search for calls to cleanupOrphanedProcesses to see what workDir values are passed
rg "cleanupOrphanedProcesses" apps/server/src/ -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 3290
🏁 Script executed:
# Find where cleanupOrphanedProcesses is called with actual workDir
rg "cleanupOrphanedProcesses\(workDir" apps/server/src/ -B 5 -A 1Repository: AutoMaker-Org/automaker
Length of output: 1269
🏁 Script executed:
# Find the context where workDir is defined and cleanupOrphanedProcesses is called
rg -B 30 "await cleanupOrphanedProcesses\(workDir" apps/server/src/services/auto-mode-service.ts | head -80Repository: AutoMaker-Org/automaker
Length of output: 1213
🏁 Script executed:
# Find where workDir is defined in the runAgent function
rg -B 100 "await cleanupOrphanedProcesses\(workDir" apps/server/src/services/auto-mode-service.ts | grep -A 5 "workDir.*=" | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Search for the runAgent function definition and workDir initialization
rg "async.*runAgent\(" apps/server/src/services/auto-mode-service.ts -A 50 | head -100Repository: AutoMaker-Org/automaker
Length of output: 2156
🏁 Script executed:
# Search for where runAgent is called to see the source of workDir
rg "this\.runAgent\(" apps/server/src/services/auto-mode-service.ts -B 3 -A 3 | head -50Repository: AutoMaker-Org/automaker
Length of output: 797
🏁 Script executed:
# Search for where workDir is defined - look for variable assignment
rg "workDir\s*=" apps/server/src/services/auto-mode-service.ts -B 2 -A 2 | head -60Repository: AutoMaker-Org/automaker
Length of output: 1742
🏁 Script executed:
# Create a test to understand PowerShell -like escaping
# Reference PowerShell documentation on wildcard patterns
python3 << 'EOF'
# PowerShell -like operator special characters and escaping
# According to PowerShell docs:
# - * (asterisk) - matches any number of characters
# - ? (question mark) - matches exactly one character
# - [ ] (brackets) - match character range
# - Backtick (`) is the escape character in PowerShell
# - To escape special chars in -like patterns, use backtick prefix
escaping_rules = {
'*': '`*', # literal asterisk
'?': '`?', # literal question mark
'[': '`[', # literal opening bracket
']': '`]', # literal closing bracket
'`': '``', # literal backtick
}
# Single quotes in PowerShell strings are escaped by doubling: ''
# But when already in double quotes, they don't need escaping
test_cases = [
"C:\\Users\\John's Folder", # Contains single quote
"C:\\Path\\[test]", # Contains brackets
"C:\\Path\\*wildcard", # Contains asterisk
]
print("PowerShell -like escaping analysis:")
print("=" * 50)
for path in test_cases:
print(f"\nOriginal: {path}")
# Escape for -like pattern
escaped = path.replace('`', '``').replace('*', '`*').replace('?', '`?').replace('[', '`[').replace(']', '`]')
print(f"Escaped: {escaped}")
EOFRepository: AutoMaker-Org/automaker
Length of output: 153
🏁 Script executed:
# Search for other PowerShell command usage to see if escaping is done elsewhere
rg "powershell.*Command" apps/server/src/ -A 2 | head -40Repository: AutoMaker-Org/automaker
Length of output: 893
🏁 Script executed:
# Look at the other PowerShell command usage in the file
rg "const psScript" apps/server/src/services/auto-mode-service.ts -B 10 -A 10Repository: AutoMaker-Org/automaker
Length of output: 1435
🏁 Script executed:
# Let me check if there are any unit tests or examples of PowerShell escaping in the codebase
rg "escapeFor|escape.*[Pp]ower" apps/server/src/ -iRepository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Search for how workDir values are typically set - look for example paths
rg "projectPath|worktree" apps/server/src/services/auto-mode-service.ts -i | grep -i "path\|folder" | head -20Repository: AutoMaker-Org/automaker
Length of output: 1054
🏁 Script executed:
# Check if there are tests or documentation about special characters in paths
fd -e md -e ts README test | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Look at the complete context of the cleanupOrphanedProcesses function to understand all the concerns
sed -n '95,145p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 2260
🏁 Script executed:
# Check if there are any integration tests or examples that use paths with special chars
rg "John's|'.*Folder|test.*path" apps/server/src/services/ -i | head -10Repository: AutoMaker-Org/automaker
Length of output: 1023
Escape special characters in PowerShell -like patterns.
The normalizedDir and unixStyleDir values are interpolated directly into -like patterns on line 118 without escaping. If a project folder name contains [, ], *, or ?, the PowerShell query will misinterpret them as wildcard patterns and fail or match unintended processes.
🛠️ Safer escaping for PowerShell -like patterns
- const normalizedDir = workDir.replace(/\//g, '\\');
+ const escapeForLike = (value: string) =>
+ value.replace(/'/g, "''").replace(/([`\*\?\[\]])/g, '`$1');
+ const normalizedDir = escapeForLike(workDir.replace(/\//g, '\\'));
- const unixStyleDir = workDir
+ const unixStyleDir = escapeForLike(
+ workDir
.replace(/^([A-Z]):/i, (_, drive) => `/${drive.toLowerCase()}`)
.replace(/\\/g, '/');
+ );🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 101 - 118, The
PowerShell -like patterns interpolate normalizedDir and unixStyleDir directly
which breaks when folder names contain wildcard chars ([ ] * ? ) or single
quotes; add an escaping helper (e.g., escapeForPowerShellLike) and use it before
building psCommand: replace all wildcard characters with a backtick-escaped
version (value.replace(/([*?\[\]])/g, '`$1')) and double any single quotes
(value.replace(/'/g, "''")) so both normalizedDir and unixStyleDir are escaped
prior to interpolation into psCommand (refer to normalizedDir, unixStyleDir, and
psCommand).
| const automakerPaths = ['automaker', 'tsx', 'vite']; | ||
|
|
||
| const pidsToKill = processes | ||
| .filter((p) => { | ||
| // Skip current process and parent | ||
| if (p.ProcessId === serverPid || p.ProcessId === process.ppid) { | ||
| return false; | ||
| } | ||
| // Skip Automaker's own processes (server, vite, tsx) | ||
| const cmdLower = (p.CommandLine || '').toLowerCase(); | ||
| if ( | ||
| automakerPaths.some((ap) => cmdLower.includes(`\\automaker\\`) && cmdLower.includes(ap)) | ||
| ) { |
There was a problem hiding this comment.
Automaker path filter is too broad and can disable cleanup.
Line 146-158: automakerPaths includes 'automaker', so any command line containing \\automaker\\ is skipped. This can prevent orphan cleanup for projects under an “automaker” path (including this repo). Consider narrowing the markers to only the server process indicators (e.g., tsx/vite).
🐛 Tighten the skip filter
- const automakerPaths = ['automaker', 'tsx', 'vite'];
+ const automakerProcessMarkers = ['tsx', 'vite'];
@@
- if (
- automakerPaths.some((ap) => cmdLower.includes(`\\automaker\\`) && cmdLower.includes(ap))
- ) {
+ if (
+ cmdLower.includes('\\automaker\\') &&
+ automakerProcessMarkers.some((ap) => cmdLower.includes(ap))
+ ) {
return false;
}🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 146 - 158, The
skip logic in the pidsToKill filter is too broad because automakerPaths includes
'automaker', causing any command line with "\automaker\" to be skipped; update
the automakerPaths array to only include specific process indicators (e.g.,
['tsx','vite']) and keep the existing check in the pidsToKill filter (the
automakerPaths.some(...) expression) so it only skips processes when the command
line contains both a specific indicator (like 'tsx' or 'vite') and the automaker
path marker; ensure you reference the automakerPaths constant and the pidsToKill
filter (and variables serverPid, process.ppid, cmdLower) when making the change.
| import { killProcessTree } from './process-utils.js'; | ||
|
|
||
| const IS_WINDOWS = process.platform === 'win32'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use @automaker/ import instead of a relative path.*
Line 7 imports killProcessTree via ./process-utils.js; the guidelines require shared package imports. Please switch to the package barrel (or another guideline‑compliant path) to avoid relative module imports. As per coding guidelines, please avoid relative imports across modules.
♻️ Suggested adjustment
-import { killProcessTree } from './process-utils.js';
+import { killProcessTree } from '@automaker/platform';🤖 Prompt for AI Agents
In `@libs/platform/src/subprocess.ts` around lines 7 - 9, Replace the relative
import of killProcessTree with the shared package barrel import; specifically
change the import that currently references './process-utils.js' to the
`@automaker` package entry (for example import { killProcessTree } from
'@automaker/process-utils') so the code uses the canonical shared module export
(ensure the imported symbol name killProcessTree remains unchanged).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Response to CodeRabbit Review Comments1. automakerPaths filter (lines 146-158)The current logic is intentionally broad to protect Automaker's own server processes from being killed. The check requires both:
While this could theoretically skip processes if a user's project is named "automaker", this is an acceptable trade-off to prevent accidentally killing the Automaker server itself. A more precise fix would add significant complexity for an edge case. 2. PowerShell -like pattern escaping (lines 101-118)Windows file system prohibits 3. subprocess.ts relative import (lines 7-9)The suggestion to use Using the barrel import would create a circular dependency:
The relative import |
1 participant
Summary
On Windows, child processes spawned via
cmd.exeorsh.exedon't terminate when the parent process is killed (unlike Unix where SIGTERM propagates). This caused dev servers and MCP processes to accumulate, consuming 30GB+ memory over time during Auto Mode execution.Root Causes
tree-killpackage was installed but never usedChanges
libs/platform/src/process-utils.ts- Cross-platform process tree termination usingtree-killBug Fixes During Implementation
-likepattern: backslashes don't need escaping$_shadowing in nestedWhere-ObjectqueriesTest Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.