fix: stop button not working on Windows for Claude Code agents#556
Open
jSydorowicz21 wants to merge 2 commits intoRunMaestro:mainfrom
Open
fix: stop button not working on Windows for Claude Code agents#556jSydorowicz21 wants to merge 2 commits intoRunMaestro:mainfrom
jSydorowicz21 wants to merge 2 commits intoRunMaestro:mainfrom
Conversation
On Windows, child.kill('SIGINT') is unreliable for shell-spawned processes
(which all agents are on Windows due to PATH resolution requiring shell: true).
The signal is silently ignored, leaving the agent running when users click stop.
Changes:
- interrupt(): Write Ctrl+C (\x03) to stdin on Windows instead of SIGINT,
with 2-second escalation to kill() if the process doesn't exit
- kill(): Use taskkill /pid /t /f on Windows to terminate the entire process
tree instead of SIGTERM, which doesn't reliably kill shell-spawned children
- Use execFile with args array (async, no shell) to avoid command injection
and main thread blocking
- Add logging for Windows interrupt paths for debuggability
Unix/macOS behavior is unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughProcessManager adds Windows-specific termination: attempts Ctrl+C on shell children via stdin, falls back to taskkill to kill process trees. Non-Windows retains signal-based SIGINT/SIGTERM flow; escalation messaging changed to reference "interrupt" before kill. Changes
Sequence DiagramsequenceDiagram
participant PM as ProcessManager
participant Child as Child Process
participant Stdin as stdin
participant Taskkill as taskkill
participant Signal as System Signal
rect rgba(200,150,255,0.5)
Note over PM,Child: Interrupt Phase (attempt graceful stop)
alt Windows & child spawned in shell
PM->>Child: write Ctrl+C to stdin
Stdin-->>PM: unavailable? log warning
else Non-Windows or no stdin
PM->>Signal: send SIGINT
end
end
rect rgba(255,180,100,0.5)
Note over PM: Escalation on timeout
PM->>PM: update message -> "Process did not exit after interrupt, escalating to kill"
end
rect rgba(255,150,150,0.5)
Note over PM,Taskkill: Kill Phase (Windows)
alt Windows & PID available
PM->>Taskkill: execFile taskkill /pid ... /T /F
Taskkill->>Child: terminate process tree
end
end
rect rgba(150,200,150,0.5)
Note over PM,Signal: Kill Phase (Non-Windows or fallback)
PM->>Signal: send SIGTERM
Signal->>Child: terminate process
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
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 |
Greptile SummaryThis PR fixes the stop button not working on Windows for Claude Code agents by replacing unreliable POSIX signal delivery with Windows-native process termination mechanisms. It adds a Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[interrupt called] --> B{isTerminal / ptyProcess?}
B -- yes --> C[ptyProcess.write Ctrl+C\nReturn true]
B -- no --> D{childProcess exists?}
D -- no --> E[Return false]
D -- yes --> F{isWindows?}
F -- no --> G[child.kill SIGINT]
F -- yes --> H{stdin available\nAND not destroyed\nAND not writableEnded?}
H -- yes --> I[stdin.write Ctrl+C\nLog debug]
H -- no --> J[Log warn:\nstdin unavailable]
G --> K[Set 2s escalation timer]
I --> K
J --> K
K --> L{child exits\nbefore timer?}
L -- yes --> M[clearTimeout\nNo further action]
L -- no --> N[kill called]
N --> O{isTerminal / ptyProcess?}
O -- yes --> P[ptyProcess.kill]
O -- no --> Q{isWindows AND pid?}
Q -- yes --> R[killWindowsProcessTree\ntaskkill /pid /t /f\nasync execFile]
Q -- no --> S[child.kill SIGTERM]
R --> T[processes.delete\nReturn true]
S --> T
P --> T
Last reviewed commit: 9567c7f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/process-manager/ProcessManager.ts (1)
233-249: Consider distinguishing genuinetaskkillfailures from expected ones.The debug-level logging for errors is reasonable since
taskkillreturns non-zero when the process is already dead. However, genuine failures (access denied,taskkillnot found on PATH, etc.) will also be logged at debug level and may go unnoticed.Consider checking the error code/message to log genuine failures at warn level:
♻️ Optional: differentiate error types
private killWindowsProcessTree(pid: number, sessionId: string): void { logger.info( '[ProcessManager] Using taskkill to terminate process tree on Windows', 'ProcessManager', { sessionId, pid } ); execFile('taskkill', ['/pid', String(pid), '/t', '/f'], (error) => { if (error) { - // taskkill returns non-zero if the process is already dead, which is fine - logger.debug( - '[ProcessManager] taskkill exited with error (process may already be terminated)', - 'ProcessManager', - { sessionId, pid, error: String(error) } - ); + // taskkill exit code 128 means process not found (already dead), which is expected + const isExpectedError = error.message?.includes('not found') || + (error as NodeJS.ErrnoException).code === 'ENOENT'; + const logFn = isExpectedError ? logger.debug : logger.warn; + logFn( + '[ProcessManager] taskkill exited with error', + 'ProcessManager', + { sessionId, pid, error: String(error), expected: isExpectedError } + ); } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/ProcessManager.ts` around lines 233 - 249, In killWindowsProcessTree, the execFile callback currently logs all taskkill errors at debug level; change it to inspect the Error object returned by execFile (in the callback inside killWindowsProcessTree) and classify expected "process already terminated" responses versus real failures: if error.message or error.code indicates benign conditions (e.g., contains phrases like "not running", "no such process", "not found", or exit/status codes known to mean "already terminated") keep logger.debug, but if error indicates genuine failures (e.g., ENOENT/taskkill not on PATH, "Access is denied", other non-benign exit codes or messages) log with logger.warn and include sessionId, pid and the full error details; keep the existing debug branch for the benign case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/process-manager/ProcessManager.ts`:
- Around line 233-249: In killWindowsProcessTree, the execFile callback
currently logs all taskkill errors at debug level; change it to inspect the
Error object returned by execFile (in the callback inside
killWindowsProcessTree) and classify expected "process already terminated"
responses versus real failures: if error.message or error.code indicates benign
conditions (e.g., contains phrases like "not running", "no such process", "not
found", or exit/status codes known to mean "already terminated") keep
logger.debug, but if error indicates genuine failures (e.g., ENOENT/taskkill not
on PATH, "Access is denied", other non-benign exit codes or messages) log with
logger.warn and include sessionId, pid and the full error details; keep the
existing debug branch for the benign case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 334eb72c-c480-4a5c-bc73-80e1e9409508
📒 Files selected for processing (1)
src/main/process-manager/ProcessManager.ts
- Check writableEnded on stdin before writing Ctrl+C to avoid ERR_STREAM_WRITE_AFTER_END on batch-mode processes where stdin was already ended via .end() - Add warn log when pid is unavailable on Windows, making the SIGTERM fallback explicit rather than silent
Author
|
Greptile concerns addressed, should be good to go |
Author
|
Test Failures addressed in #565 |
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.
On Windows, child.kill('SIGINT') is unreliable for shell-spawned processes (This may be honored for users who specify a different terminal). The signal is silently ignored, leaving the agent running when users click stop.
Changes:
Unix/macOS behavior is unchanged.
Summary by CodeRabbit