fix: Windows process cleanup for Cue and tunnel on shutdown#788
Merged
pedramamini merged 3 commits intoRunMaestro:rcfrom Apr 12, 2026
Merged
fix: Windows process cleanup for Cue and tunnel on shutdown#788pedramamini merged 3 commits intoRunMaestro:rcfrom
pedramamini merged 3 commits intoRunMaestro:rcfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds shutdown termination for active Cue runs and platform-aware process-tree termination for Cue/tunnel processes; integrates stopAllCueRuns into quit flow and updates tests and ProcessManager to support synchronous Windows taskkill behavior. Changes
Sequence DiagramsequenceDiagram
participant App as App Lifecycle
participant Quit as Quit Handler
participant CueExec as Cue Executor
participant Tunnel as Tunnel Manager
participant ProcMgr as Process Manager
App->>Quit: performCleanup()
Quit->>Quit: grooming-session cleanup (optional)
Quit->>CueExec: stopAllCueRuns()
rect rgba(100,150,255,0.5)
CueExec->>CueExec: iterate activeProcesses
CueExec->>CueExec: killCueProcess(child, sync?) -- Windows: taskkill /pid /t /f (sync on shutdown) / POSIX: SIGTERM → SIGKILL if needed
CueExec->>CueExec: remove entries from activeProcesses
end
Quit->>Tunnel: stop()
rect rgba(100,200,150,0.5)
Tunnel->>Tunnel: If Windows: execFileSync("taskkill /pid <pid> /t /f")
Tunnel->>Tunnel: Else: send SIGTERM, wait, then SIGKILL on timeout
Tunnel->>Quit: stop() complete
end
Quit->>ProcMgr: killAll() (sync on Windows)
ProcMgr->>ProcMgr: kill(..., { sync: true }) -> ensure taskkill completes
Quit->>App: Cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
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 two Windows-specific process cleanup bugs: (1) Cue-spawned processes were tracked separately from Confidence Score: 5/5Safe to merge — fixes are targeted, correct, and well-tested; no blocking issues found. All three files make narrow, well-scoped changes. The Windows taskkill /t /f path is the established pattern for child-process-tree termination on Windows. POSIX behaviour is unchanged. stopAllCueRuns() is correctly wired into the shutdown sequence before killAll(), and the Map-iteration deletion pattern is safe in JavaScript. All findings are P2 or lower. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant App as Electron App
participant QH as QuitHandler
participant CE as CueExecutor
participant TM as TunnelManager
participant OS as OS / taskkill
User->>App: Cmd+Q / Quit
App->>QH: before-quit event
QH->>QH: performCleanup()
QH->>CE: stopAllCueRuns()
loop Each active Cue process
CE->>OS: taskkill /pid /t /f (Windows) or SIGTERM->SIGKILL (POSIX)
CE->>CE: activeProcesses.delete(runId)
end
QH->>App: processManager.killAll()
QH->>TM: tunnelManager.stop()
TM->>OS: taskkill /pid /t /f (Windows) or SIGTERM->SIGKILL (POSIX)
TM-->>QH: await exit / timeout (3 s)
QH->>App: deleteCliServerInfo(), closeStatsDB()
Reviews (1): Last reviewed commit: "fix: use taskkill on Windows for Cue and..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/cue/cue-executor.ts (1)
442-457: Well-structured platform-aware termination helper.The
killCueProcesshelper correctly:
- Uses
taskkill /t /fon Windows to terminate the entire process tree- Uses SIGTERM with delayed SIGKILL escalation on POSIX
- Checks
exitCode === null && signalCode === nullbefore SIGKILL to avoid signaling an already-exited processSame feedback as
tunnel-manager.ts: consider adding debug logging for unexpectedtaskkillfailures to aid debugging orphaned processes on Windows.,
🔧 Optional: Add debug logging for taskkill errors
function killCueProcess(child: ChildProcess): void { if (isWindows() && child.pid) { - execFile('taskkill', ['/pid', String(child.pid), '/t', '/f'], () => { - // taskkill returns non-zero if the process is already dead, which is fine + execFile('taskkill', ['/pid', String(child.pid), '/t', '/f'], (error) => { + // taskkill returns non-zero if the process is already dead, which is fine. + // Log other errors for debugging. + if (error && !error.message.includes('not found')) { + // Import logger if not already available, or use console.debug for minimal overhead + } }); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-executor.ts` around lines 442 - 457, In killCueProcess, the Windows branch calls execFile('taskkill', ...) but currently ignores errors; modify that callback to log unexpected taskkill failures for debugging orphaned processes by capturing the (err, stdout, stderr) arguments and emitting a debug-level log that includes the error and stderr output (but do not change control flow or treat non-zero exit as fatal). Update the callback inside killCueProcess where execFile is invoked so it logs the details (err and stderr) using the module's logger (e.g., processLogger or the existing logger used elsewhere in this file) while keeping the existing comment about non-zero return codes.src/main/tunnel-manager.ts (1)
109-117: Consider loggingtaskkillfailures for debugging orphaned process issues.The empty callback swallows all
execFileerrors. While ignoring "process already dead" (non-zero exit) is reasonable, unexpected failures (e.g.,taskkillnot found, permission denied) would be silently lost, making it harder to debug orphaned process issues on Windows.🔧 Proposed fix to add error logging
if (isWindows() && proc.pid) { // On Windows, POSIX signals don't terminate process trees. // Use taskkill /t /f to kill the cloudflared process and its children. - execFile('taskkill', ['/pid', String(proc.pid), '/t', '/f'], () => { - // taskkill returns non-zero if the process is already dead, which is fine + execFile('taskkill', ['/pid', String(proc.pid), '/t', '/f'], (error) => { + // taskkill returns non-zero if the process is already dead, which is fine. + // Log other errors for debugging orphaned process issues. + if (error && !error.message.includes('not found')) { + logger.debug(`taskkill for cloudflared: ${error.message}`, 'TunnelManager'); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/tunnel-manager.ts` around lines 109 - 117, The taskkill callback currently swallows all errors; update the execFile('taskkill', ['/pid', String(proc.pid), '/t', '/f'], ...) callback to check for an error and log unexpected failures (e.g., taskkill not found or permission denied) using the module's existing logger instead of ignoring them, while still treating non-zero exit when the process is already dead as benign; keep the else branch that calls proc.kill('SIGTERM') unchanged and reference isWindows(), proc.pid and execFile('taskkill'...) when making the change.
🤖 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/cue/cue-executor.ts`:
- Around line 442-457: In killCueProcess, the Windows branch calls
execFile('taskkill', ...) but currently ignores errors; modify that callback to
log unexpected taskkill failures for debugging orphaned processes by capturing
the (err, stdout, stderr) arguments and emitting a debug-level log that includes
the error and stderr output (but do not change control flow or treat non-zero
exit as fatal). Update the callback inside killCueProcess where execFile is
invoked so it logs the details (err and stderr) using the module's logger (e.g.,
processLogger or the existing logger used elsewhere in this file) while keeping
the existing comment about non-zero return codes.
In `@src/main/tunnel-manager.ts`:
- Around line 109-117: The taskkill callback currently swallows all errors;
update the execFile('taskkill', ['/pid', String(proc.pid), '/t', '/f'], ...)
callback to check for an error and log unexpected failures (e.g., taskkill not
found or permission denied) using the module's existing logger instead of
ignoring them, while still treating non-zero exit when the process is already
dead as benign; keep the else branch that calls proc.kill('SIGTERM') unchanged
and reference isWindows(), proc.pid and execFile('taskkill'...) when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c72d4907-a457-4138-8e00-1318ea8feaee
📒 Files selected for processing (3)
src/main/app-lifecycle/quit-handler.tssrc/main/cue/cue-executor.tssrc/main/tunnel-manager.ts
Cue process lifecycle and TunnelManager used POSIX signals (SIGTERM/SIGKILL) to terminate child processes, which don't work for shell-spawned process trees on Windows — leaving orphaned processes after Maestro closes. Changes: - cue-process-lifecycle: add killCueProcess() helper using taskkill /t /f on Windows, falling back to SIGTERM→SIGKILL on POSIX - cue-process-lifecycle: add stopAllProcesses() to kill all active Cue processes - cue-executor: export stopAllCueRuns() delegating to lifecycle module - tunnel-manager: use taskkill /t /f on Windows for cloudflared cleanup - quit-handler: call stopAllCueRuns() during shutdown (Cue processes are tracked separately from ProcessManager and were never killed on quit) - quit-handler test: mock cue-executor, verify stopAllCueRuns call order Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
chr1syy
force-pushed
the
fix/windows-closing-hang
branch
from
b971b95 to
8740ed5
Compare
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 `@src/main/cue/cue-process-lifecycle.ts`:
- Around line 101-105: The Windows branch of killCueProcess currently swallows
errors from execFile('taskkill', ...), so update the execFile callback in
killCueProcess to explicitly handle errors: if err indicates taskkill failed for
an unexpected reason, call Sentry utilities (captureException or captureMessage)
and do not clear the process tracking entry; if the error is the expected
"process not found"/non-zero exit for already-dead process, treat it as success
and proceed to clear tracking; on success clear tracking as before. Use the
existing execFile call signature and refer to captureException/captureMessage
for reporting and the same process-tracking removal logic currently executed at
the removal point so that removal only happens after confirming success or an
acceptable “already dead” condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11fd6047-5fcc-4478-af4b-abfc16c5b6ea
📒 Files selected for processing (5)
src/__tests__/main/app-lifecycle/quit-handler.test.tssrc/main/app-lifecycle/quit-handler.tssrc/main/cue/cue-executor.tssrc/main/cue/cue-process-lifecycle.tssrc/main/tunnel-manager.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/app-lifecycle/quit-handler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/tunnel-manager.ts
- Add explicit error handling to taskkill callback in killCueProcess: ignore expected "not found" / "no running instance" errors, report unexpected failures to Sentry via captureException. - Add exitCode and signalCode properties to MockChildProcess in cue-executor tests so the SIGKILL escalation guard condition works correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/cue/cue-process-lifecycle.ts (1)
267-271:⚠️ Potential issue | 🟠 MajorDo not untrack processes before kill outcome is known.
stopAllProcesses()deletes entries immediately afterkillCueProcess(). On Windows,taskkillis async; if it fails, the process can survive while being removed from tracking.Suggested fix
export function stopAllProcesses(): void { for (const [runId, entry] of activeProcesses) { + const clear = () => activeProcesses.delete(runId); + entry.child.once('close', clear); + entry.child.once('error', clear); killCueProcess(entry.child); - activeProcesses.delete(runId); } }As per coding guidelines: "Do not silently swallow errors... Handle expected/recoverable errors explicitly... For unexpected errors, re-throw them to allow Sentry to capture them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-process-lifecycle.ts` around lines 267 - 271, stopAllProcesses currently removes entries from activeProcesses before the kill outcome is known, which can leave orphaned processes on Windows; change stopAllProcesses to await the kill outcome and only call activeProcesses.delete(runId) after killCueProcess(entry.child) completes successfully, wrapping the await in a try/catch so failures are either handled (log/record) or re-thrown per guidelines; if killCueProcess is synchronous, convert it to return a Promise (or provide an async wrapper) so stopAllProcesses can await it, and ensure you reference stopAllProcesses, activeProcesses, killCueProcess, entry.child and runId when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/cue/cue-process-lifecycle.ts`:
- Around line 267-271: stopAllProcesses currently removes entries from
activeProcesses before the kill outcome is known, which can leave orphaned
processes on Windows; change stopAllProcesses to await the kill outcome and only
call activeProcesses.delete(runId) after killCueProcess(entry.child) completes
successfully, wrapping the await in a try/catch so failures are either handled
(log/record) or re-thrown per guidelines; if killCueProcess is synchronous,
convert it to return a Promise (or provide an async wrapper) so stopAllProcesses
can await it, and ensure you reference stopAllProcesses, activeProcesses,
killCueProcess, entry.child and runId when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de426df9-9ddd-4bdf-b67e-1c2bd05045b5
📒 Files selected for processing (2)
src/__tests__/main/cue/cue-executor.test.tssrc/main/cue/cue-process-lifecycle.ts
Contributor
Author
|
@pedramamini good to go for your review |
…cesses on Windows
Async execFile('taskkill') calls were fire-and-forget — Electron exited before
taskkill.exe could finish terminating the process trees. Switch to execFileSync
during shutdown (killAll, stopAllProcesses, TunnelManager.stop) so processes are
confirmed dead before the app exits. Normal single-process kills remain async.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
chr1syy
added a commit
to chr1syy/Maestro
that referenced
this pull request
Apr 16, 2026
…ro#788) * fix: use taskkill on Windows for Cue and tunnel process cleanup Cue process lifecycle and TunnelManager used POSIX signals (SIGTERM/SIGKILL) to terminate child processes, which don't work for shell-spawned process trees on Windows — leaving orphaned processes after Maestro closes. Changes: - cue-process-lifecycle: add killCueProcess() helper using taskkill /t /f on Windows, falling back to SIGTERM→SIGKILL on POSIX - cue-process-lifecycle: add stopAllProcesses() to kill all active Cue processes - cue-executor: export stopAllCueRuns() delegating to lifecycle module - tunnel-manager: use taskkill /t /f on Windows for cloudflared cleanup - quit-handler: call stopAllCueRuns() during shutdown (Cue processes are tracked separately from ProcessManager and were never killed on quit) - quit-handler test: mock cue-executor, verify stopAllCueRuns call order Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: handle taskkill errors explicitly and fix SIGKILL escalation test - Add explicit error handling to taskkill callback in killCueProcess: ignore expected "not found" / "no running instance" errors, report unexpected failures to Sentry via captureException. - Add exitCode and signalCode properties to MockChildProcess in cue-executor tests so the SIGKILL escalation guard condition works correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use synchronous taskkill during shutdown to prevent orphaned processes on Windows Async execFile('taskkill') calls were fire-and-forget — Electron exited before taskkill.exe could finish terminating the process trees. Switch to execFileSync during shutdown (killAll, stopAllProcesses, TunnelManager.stop) so processes are confirmed dead before the app exits. Normal single-process kills remain async. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
2 participants
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
performCleanup()callsprocessManager.killAll()but Cue processes are tracked separately incue-executor.ts'sactiveProcessesmap — they were never killed on quit. AddedstopAllCueRuns()and call it during shutdown.stopCueRun()and timeout enforcement sent POSIX signals which don't terminate process trees on Windows. AddedkillCueProcess()helper that usestaskkill /t /fon Windows.TunnelManager.stop()sent POSIX signals. Now usestaskkill /t /fon Windows for thecloudflaredprocess tree.Changes
src/main/cue/cue-executor.tskillCueProcess()(Windows: taskkill, POSIX: SIGTERM→SIGKILL),stopAllCueRuns()export, refactoredstopCueRun()and timeout to use itsrc/main/tunnel-manager.tsstop()now usestaskkill /t /fon Windows instead of SIGTERM/SIGKILLsrc/main/app-lifecycle/quit-handler.tsperformCleanup()now callsstopAllCueRuns()beforekillAll()Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests