Merged
Conversation
Contributor
|
You quit a CLI and the process is still alive in memory? Because I checked that issue on OC and there people are reporting huge memory usage while CLI is running. So I see your claim is different and therefor that first question I have for you. |
The shutdown RPC call to the worker thread never resolved because the worker's event loop drained after cleanup, preventing the postMessage result from being delivered back to the main thread. This left process.exit() unreachable and the process hanging indefinitely. Replace the blocking await with a fire-and-forget shutdown RPC that waits for the worker's close event (up to 5s timeout) before calling worker.terminate().
This comment has been minimized.
This comment has been minimized.
Contributor
Author
@mikij We uncovered more problems than we initially suspected. Aside from the memory leak while running the CLI, we uncovered that |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (2 files)
|
terminateWorker is registered on SIGHUP, SIGTERM, and used in onExit.
If a signal fires and then onExit triggers (or two signals arrive),
worker.terminate() and client.call('shutdown') would fire redundantly.
Cache the pending promise so subsequent calls return the same operation.
Add a comment noting that (transport as any)._process relies on a private implementation detail of StdioClientTransport from @modelcontextprotocol/sdk@1.25.2. Future SDK upgrades should verify this field still exists.
Previously, connect() called create() (which spawns a new child process and writes processes[key]) before closing the existing client. The old client's close handler would then delete processes[key], removing the *new* process entry from the map — losing track of the new child. Move the existing client close to before create(), matching the pattern already used in add().
…into fix/memory-leak
…ocess map corruption
…rySendSignal utilities
The setTimeout fallback in terminateWorker can keep the event loop open even when the worker exits promptly, delaying shutdown paths triggered by signals. Unref the timer so normal fast shutdown is not blocked by this fallback.
Wrap originalClose() and child-process cleanup in try/finally so that delete processes[key] always executes. Previously, if originalClose() threw, the process map entry would be left stale, risking incorrect PID references in later lifecycle operations.
Handle tab-close paths where the shell dies without forwarding SIGHUP/SIGTERM, which left `bun run ...` alive as an orphan. Add a parent-liveness watchdog in the TUI thread and route shutdown through a single idempotent `shutdownAndExit` path. Also guard the normal `process.exit(0)` path so signal/parent-exit codes are not overridden.
marius-kilocode
requested changes
Feb 26, 2026
|
|
||
| while (queue.length > 0) { | ||
| const pid = queue.shift()! | ||
| const proc = Bun.spawn(["pgrep", "-P", String(pid)], { |
Collaborator
There was a problem hiding this comment.
What about Windows?
Contributor
Author
There was a problem hiding this comment.
I have removed the MCP related fix for now, but I left the fix for closing the terminal tab (see PR summary recordings).
What’s left should work on Windows, and the only potential limitation is reduced reliability of the parent-watchdog in restricted-permission cases (not a crash/compat break).
- No type/runtime API mismatches introduced.
- The changes are runtime-API based (Worker, RPC shutdown, timers, process handlers) and do not use Unix-only commands (pgrep, ps, etc.).
- Existing Windows guard logic is still in place (
win32InstallCtrlCGuard,win32DisableProcessedInput) and is untouched functionally. - Caveat: parent-death detection uses
process.kill(parentPid, 0). On some Windows setups this can fail with permission-related errors (EPERM), in which case the watchdog intentionally does not force-exit; normal shutdown paths still work.
marius-kilocode
approved these changes
Feb 26, 2026
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
3 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.
Context
Multiple issues:
/exitdoesn't close all processesStdioClientTransport.close()from the MCP SDK closes pipes but does not kill the spawned child process, leaving orphan processes after every MCP server lifecycleImplementation
packages/opencode/src/cli/cmd/tui/thread.tsso worker teardown is idempotent, does not hang on missing RPC responses, and exits cleanly onSIGHUP/SIGTERMor when the parent terminal process disappears.packages/opencode/src/mcp/index.tsto track spawned child processes, close existing clients before reconnect/recreate, and force-kill surviving MCP children when normal close/dispose does not fully terminate them.packages/opencode/test/memory/pluspackages/opencode/test/fixture/mcp/fake-mcp-server.jsto validate disposal behavior, heap growth bounds, and orphan-process cleanup across MCP/LSP/session flows.How to Test
Manually: Ask the CLI to plan anything, then try closing the terminal tab while watching the Activity Monitor.
From
packages/opencode:Before
Screen.Recording.2026-02-26.at.09.07.22.mov
After
Screen.Recording.2026-02-26.at.08.43.20.mov
Get in Touch
We'd love to have a way to chat with you about your changes if necessary. If you're in the Kilo Code Discord, please share your handle here.