fix(server): prevent Windows memory leak from orphaned Node.js processes #666
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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. 📝 WalkthroughWalkthroughAdds cross-platform process-tree utilities and integrates Windows-aware process cleanup across platform libs, subprocess helpers, server services (auto-mode, dev-server, codex-app), a stop-feature route parameter, startup initialization of AutoModeService, and related unit test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Server as Server
participant AMS as AutoModeService
participant Util as ProcessUtils
participant OS as OS
Server->>AMS: initialize()
activate AMS
AMS->>Util: cleanupOrphanedProcessesOnStartup()
activate Util
Util->>OS: detect platform (IS_WINDOWS)
alt Windows
Util->>OS: killProcessTree(orphaned_pids)
else Non-Windows
Note over Util: no tree-kill (skip)
end
deactivate Util
AMS-->>Server: initialization complete
deactivate AMS
sequenceDiagram
participant Client as Client
participant Route as StopFeature Route
participant AMS as AutoModeService
participant RF as RunningFeature
participant Util as ProcessUtils
Client->>Route: POST /stop-feature { featureId, waitForCleanup? }
activate Route
Route->>AMS: stopFeature(featureId, shouldWait)
activate AMS
AMS->>RF: signalCompletion() / set completionPromise
activate RF
RF->>Util: trigger cleanup (killProcessTree / SIGTERM)
activate Util
Util-->>RF: cleanup done
deactivate Util
RF-->>AMS: completionPromise resolves
deactivate RF
alt shouldWait == true
AMS->>AMS: await completionPromise (with timeout)
end
AMS-->>Route: result (true/false)
deactivate AMS
Route-->>Client: 200 OK / error
deactivate Route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
![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).
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 |
Shironex
added
the
Performance
|
if u could resolve merge conflicts before i review the pr |
Shironex
added
the
waiting-on-author
ScotTFO
force-pushed
the
fix/windows-memory-leak-auto-mode
branch
from
44edfb4 to
1923a3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/server/src/services/auto-mode-service.ts`:
- Around line 344-347: The RunningFeature's completionPromise/signalCompletion
fields are never initialized or resolved, so waitForCleanup in stopFeature
doesn't actually wait; initialize completionPromise and signalCompletion when a
RunningFeature is created (set completionPromise = new Promise<void>(resolve =>
signalCompletion = resolve) and attach both to the RunningFeature instance), and
then call signalCompletion() in the finalizers/finally blocks of executeFeature,
resumePipelineFeature, and followUpFeature (where execution ends on success,
failure, or abort) so completionPromise is resolved; ensure stopFeature uses
await running.completionPromise when the abort-and-wait path is taken so cleanup
truly waits.
d141852 to
bd7e039
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 completion tracking for graceful feature cleanup on stop - Add pid to mock process in tests for tree-kill condition Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/server/src/services/auto-mode-service.ts`:
- Around line 79-83: The file fails Prettier formatting; run Prettier to fix
formatting inconsistencies in this file (e.g., align whitespace around the
top-level constant IS_WINDOWS and the surrounding JSDoc/comment blocks). Run
your formatter (prettier --write) on
apps/server/src/services/auto-mode-service.ts or apply equivalent lint/format
fixes so the declaration const IS_WINDOWS = process.platform === 'win32'; and
the following comment block match the project's Prettier rules.
♻️ Duplicate comments (3)
libs/platform/src/subprocess.ts (1)
7-10: Align import with shared-package rule.
Consider reorganizing exports (e.g., movekillProcessTreeinto a lower-level module) so this can import via@automaker/platforminstead of a relative path, avoiding circular dependency while staying guideline‑compliant. As per coding guidelines, ...apps/server/src/services/auto-mode-service.ts (2)
101-118: Escape PowerShell-likepatterns to avoid wildcard mis-matches.
Paths containing*,?,[,], or'can break matching and skip cleanup. Consider escaping those characters before interpolation.🛠️ Suggested escape helper
- 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, '/'); + .replace(/\\/g, '/') + );PowerShell -like operator: how to escape wildcard characters (* ? [ ]) and single quotes inside single-quoted strings?
344-347:waitForCleanupwon’t wait because completionPromise is never wired.
stopFeature(..., true)relies oncompletionPromise, but it’s never created or resolved. As-is, the wait path is a no‑op.🛠️ Wire completionPromise to execution lifecycle
@@ - const entry: RunningFeature = { + let signalCompletion!: () => void; + const completionPromise = new Promise<void>((resolve) => { + signalCompletion = resolve; + }); + const entry: RunningFeature = { featureId: params.featureId, projectPath: params.projectPath, worktreePath: null, branchName: null, abortController, isAutoMode: params.isAutoMode, startTime: Date.now(), leaseCount: 1, + completionPromise, + signalCompletion, }; @@ - this.releaseRunningFeature(featureId); + tempRunningFeature.signalCompletion?.(); + this.releaseRunningFeature(featureId); @@ - } finally { - this.releaseRunningFeature(featureId); + } finally { + runningEntry.signalCompletion?.(); + this.releaseRunningFeature(featureId); } @@ - } finally { - this.releaseRunningFeature(featureId); + } finally { + runningEntry.signalCompletion?.(); + this.releaseRunningFeature(featureId); }Also applies to: 463-475, 1663-1674, 1838-1867, 2246-2248, 2501-2503
Merge Conflict Resolved ✅Rebased onto
All CI checks are now passing:
|
- Create completionPromise and signalCompletion in acquireRunningFeature - Call signalCompletion in finally blocks of all execute methods - Fixes CodeRabbit review: completionPromise was defined but never wired Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addressed CodeRabbit Review ✅Fixed the issue raised by CodeRabbit:
Fix Applied (commit 72a080a):
This ensures |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/server/src/services/auto-mode-service.ts`:
- Line 52: The PowerShell output "null" is not handled in
cleanupOrphanedProcesses(), causing JSON.parse to produce null and then access
.ProcessId on null; update cleanupOrphanedProcesses() to mirror
cleanupOrphanedProcessesOnStartup() by rejecting outputs that are falsy, empty
when trimmed, or equal to the literal string "null" before calling JSON.parse
(or alternatively parse and verify the result is non-null and an array/object),
and ensure you normalize single-object results into an array before filtering so
access to .ProcessId is safe.
- Around line 524-533: The current cleanupOrphanedProcessesOnStartup builds a
broad psScript that can kill unrelated dev servers; update it to fetch known
project paths via settingsService.getGlobalSettings(), skip the cleanup entirely
if no Automaker project paths are found, and restrict the PowerShell filter
(psScript) to only match processes whose CommandLine contains one of those
project paths (build a safe escaped regex or explicit contains checks for each
path). Modify the method cleanupOrphanedProcessesOnStartup to early-return when
settingsService reports no paths, and to dynamically construct psScript using
the retrieved paths so only processes launched from those project directories
(not any node.exe/sh.exe/cmd.exe) are considered orphaned.
PowerShell returns the literal string "null" when no processes match, which JSON.parse accepts as valid JSON (returning null). Added check for this case to prevent null reference errors. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addressing CodeRabbit Review Comments✅ Fixed Issues
ℹ️ Issues That Don't Need Fixing
|
2 participants
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.