fix(jetbrains): stop CLI on app close - #12105
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Fix these issues in Kilo Cloud Files Reviewed (14 files)
Previous Review Summaries (3 snapshots, latest commit 8afc161)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8afc161)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (4 files)
Previous review (commit a4a66b0)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Fix these issues in Kilo Cloud Files Reviewed (7 files)
Previous review (commit 12f510f)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Fix these issues in Kilo Cloud Files Reviewed (7 files)
Reviewed by gpt-5.6-sol-20260709 · Input: 174.6K · Output: 47.2K · Cached: 2.1M Review guidance: REVIEW.md from base branch |
Address PR review: honor wait on the Windows tree-kill path so a reported success is verified against actual process exit; re-enumerate descendants before SIGKILL so children forked during the grace period are escalated too. Rework the process-tree test to use a parent that does not clean up its children so the assertions prove the tree kill. Add a test for the disposal terminal-state guard.
Confirm process-tree exit after SIGKILL on the non-Windows kill path, and escalate SIGTERM to SIGKILL on the shutdown-hook (no-wait) path so a SIGTERM-ignoring tree is not orphaned on JVM exit. Deflake the process-tree kill test by capturing both children deterministically and add coverage for the no-wait escalation. Make JetBrains IDE app-close teardown non-blocking: send SIGTERM and return, letting the JVM shutdown hook confirm exit, so quitting the IDE no longer stalls the EDT for up to several seconds. Stop plugin-unload teardown from waiting on the lifecycle mutex behind an in-flight download. Add a parent-death watchdog to `kilo serve`: the VS Code extension and JetBrains plugin pass their PID via KILO_PARENT_PID and the server exits when that process disappears, covering hard client kills where no signal or shutdown hook runs.
Locks in the disposal terminal-state guarantee (R5): after dispose(), a late SSE onClosed/onFailure with a stale source must not resurrect the connection or schedule a reconnect. Deterministic — drives the listener directly, no sleeps.
The process-tree kill test flaked on Linux CI ("child still alive"): a
SIGKILLed orphan reparents to init and lingers as an unreaped zombie that
ProcessHandle still reports alive (onExit never fires for a non-child), so the
kill worked but was unobservable. The test now treats a zombie ('Z' in /proc)
or already-reaped process as exited, with an isAlive fallback off Linux.
Also simplify confirmKilled to wait only on the tracked parent (our real
child, reliably reaped) and report descendants best-effort, so wait=true
shutdown no longer blocks on an orphan exit it cannot observe.
| ) | ||
| if (state is CliServer.State.Error && process == proc) { | ||
| val current = synchronized(lock) { | ||
| if (state !is CliServer.State.Error || process != proc) return@synchronized null |
There was a problem hiding this comment.
[WARNING]: App close can publish a stale Ready result
This branch only cleans up an Error. If awaitReady completes with Ready just before closeForShutdown() marks the manager closed and terminates the process, spawn() resumes here afterward and returns that stale Ready state. The connection layer can then recreate clients, SSE, and background jobs after disposal has completed. Please re-check closed/process for every result and reject a late Ready result.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } ?: return | ||
| closing = proc | ||
| close(proc) | ||
| descendants(proc).forEach { it.destroy() } |
There was a problem hiding this comment.
[WARNING]: Fast close discards descendants before hook escalation
The descendant handles receiving SIGTERM here are immediately discarded. If the CLI parent exits but a child ignores SIGTERM, that child is reparented before the JVM shutdown hook calls descendants(proc) again, so the hook can no longer find it to send SIGKILL. This can still leave an orphan on normal app close. Please retain this snapshot for the hook or otherwise escalate the same process group/tree.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // Re-enumerate before SIGKILL: a tool/shell can fork new descendants during the grace | ||
| // period, and killing the known processes can reparent them. Union the fresh scan with | ||
| // the original handles so late children are escalated too. | ||
| val kids = (original + descendants(proc)).distinctBy { it.pid() } |
There was a problem hiding this comment.
[WARNING]: The rescan still misses reparented late children
A process can fork from a TERM handler after original is captured and then exit before this second descendants(proc) call. Its new child is already reparented at that point, so it appears in neither snapshot and survives the following SIGKILL pass. The current test creates all children before invoking the kill path, so it does not cover this race. Please use process-group/job semantics or another strategy that cannot lose descendants when the parent exits.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| () => | ||
| new Promise<void>((resolve) => { | ||
| // Exit if the editor client that spawned us is hard-killed (no signal reaches us). | ||
| const stopWatchdog = startParentWatchdog(() => process.kill(process.pid, "SIGTERM")) |
There was a problem hiding this comment.
[WARNING]: Self-signaling bypasses graceful cleanup on Windows
Windows emulates process.kill(..., "SIGTERM") as forceful process termination rather than delivering a POSIX signal to this JavaScript listener. The watchdog can therefore terminate the server before disposeAllInstances() and server.stop(true) run, leaving MCP/LSP descendants behind. Please invoke an idempotent shutdown routine directly from the watchdog and retain a bounded force-exit fallback.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * Returns a function that stops the watchdog. | ||
| */ | ||
| export function startParentWatchdog(onOrphan: () => void, intervalMs = 1000): () => void { | ||
| const configured = Number(process.env["KILO_PARENT_PID"]) |
There was a problem hiding this comment.
[WARNING]: An inherited PID activates the watchdog for nested servers
The managed CLI passes KILO_PARENT_PID into process.env, and its shell, PTY, MCP, and LSP subprocesses inherit that environment. A kilo serve launched from one of those subprocesses therefore watches the editor PID even though the editor is not its direct parent; it can shut down when the launching shell reparents it or when the editor exits, contrary to the documented no-op behavior for manually launched servers. Please validate the launcher identity/direct-parent relationship or scrub this variable from descendant environments.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| val log = TestLog() | ||
| // The parent ignores SIGTERM, so only SIGKILL can stop it. This is the shutdown-hook path | ||
| // (wait=false); it must still escalate rather than orphan a tree that survives SIGTERM. | ||
| val proc = process("sh", "-c", "trap '' TERM; sleep 30") |
There was a problem hiding this comment.
[SUGGESTION]: The test can signal before the TERM trap is installed
ProcessBuilder.start() only confirms that sh was created; the test immediately invokes the kill path without proving that trap '' TERM has executed. A regression that sends only SIGTERM could therefore pass by terminating the shell before the ignore disposition is active. Consider adding a deterministic readiness handshake after trap installation before calling killCliProcessTree.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try { | ||
| await Promise.race([ | ||
| orphaned, | ||
| new Promise((_, reject) => setTimeout(() => reject(new Error("watchdog did not fire")), 5000)), |
There was a problem hiding this comment.
[SUGGESTION]: The successful path leaves a five-second timer running
Promise.race does not cancel its losing promise. When orphaned resolves, this referenced timeout remains active for five seconds, leaking asynchronous work across tests and potentially delaying a targeted test process. Please retain the timeout handle and clear it in finally alongside stop().
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
fix(jetbrains): stop CLI on app close
2 participants
Issue
Fixes #12048
Context
Closing a JetBrains IDE (originally reported on Windows) can leave a stale
kilo serveprocess behind. The orphan keeps locks on the extracted CLI binary and can block clean IDE shutdown or the next CLI launch. While hardening this, we also addressed adjacent shutdown-robustness gaps found across the CLI lifecycle (download → spawn → connect → shutdown).Implementation
Process-tree kill is now confirmed, not fire-and-forget. The non-Windows kill path confirms the whole tree has exited after SIGKILL instead of returning immediately, and the JVM shutdown-hook (no-wait) path now escalates SIGTERM→SIGKILL so a binary that ignores SIGTERM (or a stuck sandbox child) is never orphaned on JVM exit. This also removes flakiness in the process-tree kill test, which previously asserted an exit the production code never waited for.
App close no longer blocks the IDE.
appWillBeClosedruns on the EDT; it now uses a fast, non-blocking teardown (send SIGTERM and return) with the JVM shutdown hook as the guaranteed backstop, instead of synchronously waiting up to several seconds for the kill + reader-thread joins.Plugin unload no longer stalls behind an in-flight download. The unload teardown no longer waits on the lifecycle mutex; the CLI manager's own disposal guards handle any concurrent spawn.
kilo serveself-exits when its spawner dies (defense in depth). A hard kill of the IDE (SIGKILL/crash) delivers no signal and runs no shutdown hook. The extension/plugin now pass their PID viaKILO_PARENT_PID, and a new parent-death watchdog inkilo serveexits the server when that process disappears.Rollout note
The parent-death watchdog lives in the CLI core (
packages/opencode/). JetBrains runs the pinned CLI release, so the watchdog is dormant for JetBrains until a CLI release is published and the pin is bumped (packages/kilo-jetbrains/package.json). It is a no-op until then — the extraKILO_PARENT_PIDenv var is simply ignored by an older CLI, so there is no failure or startup impact. The VS Code extension bundles its own CLI binary and picks it up on the next build. All JetBrains plugin-side fixes (kill confirmation, non-blocking app close, mutex-free unload) take effect immediately.Tests
parent-watchdogunit tests (spawn → kill → assert the watchdog fires; no-op when unset/invalid).Verification
./gradlew typecheckand thecli.*/app.*backend suites pass frompackages/kilo-jetbrains/; the process-tree kill test is green across repeated reruns.bun run typecheckpasses forpackages/opencode/; the parent-watchdog tests pass.bun run typecheckpasses forpackages/kilo-vscode/.Reviewer test steps
kilo servestarts.kilo serve/kilo.exeremains and reopening starts Kilo cleanly. Repeat a few times (the original orphaning was intermittent).Checklist