fix(tui): terminate cleanly when terminal closes#610
fix(tui): terminate cleanly when terminal closes#610Olusammytee wants to merge 1 commit intoKilo-Org:devfrom
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
| process.on("SIGTERM", shutdown) | ||
| process.once("SIGHUP", shutdown) | ||
| process.once("SIGTERM", shutdown) | ||
| process.once("SIGINT", shutdown) |
There was a problem hiding this comment.
WARNING: Handling SIGINT here may conflict with the TUI's own Ctrl+C handling.
The TUI (ink-based) typically intercepts SIGINT to manage its own exit flow (e.g., prompting to confirm exit, cleaning up the terminal). By registering process.once("SIGINT", shutdown) at this level, the signal handler will call client.call("shutdown") immediately when the user presses Ctrl+C, potentially racing with or bypassing the TUI's normal onExit cleanup path (line 188-190).
Consider removing the SIGINT handler and relying on the TUI's existing onExit callback for user-initiated exits. The other signal handlers (SIGHUP, SIGTERM, disconnect, stdin close) are appropriate for external/non-interactive termination scenarios.
Code Review SummaryStatus: 2 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 (1 files)
|
There was a problem hiding this comment.
Pull request overview
This pull request adds graceful shutdown handling to the TUI command to fix issue #159, where the CLI process remains in memory after closing a VS Code terminal. The changes register shutdown hooks for various termination signals and stdin closure events, with a guard to prevent duplicate shutdown calls.
Changes:
- Added shutdown handlers for SIGINT, SIGHUP, SIGTERM, and parent disconnect signals
- Added stdin close/end event handlers to detect terminal closure
- Implemented a
shuttingDownflag to prevent duplicate RPC shutdown calls
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process.stdin.once("end", shutdown) | ||
| process.stdin.once("close", shutdown) |
There was a problem hiding this comment.
The stdin event listeners may cause premature shutdown when stdin is piped. On line 146, Bun.stdin.text() reads stdin completely when !process.stdin.isTTY, which will trigger the "end" event immediately after reading. This means the TUI would shut down right after starting when input is piped.
Consider registering the stdin event handlers only when process.stdin.isTTY is true, or register them after the stdin read at line 146 completes.
| process.stdin.once("end", shutdown) | |
| process.stdin.once("close", shutdown) | |
| if (process.stdin.isTTY) { | |
| process.stdin.once("end", shutdown) | |
| process.stdin.once("close", shutdown) | |
| } |
| process.once("SIGHUP", shutdown) | ||
| process.once("SIGTERM", shutdown) | ||
| process.once("SIGINT", shutdown) |
There was a problem hiding this comment.
The use of process.once() here is inconsistent with the existing signal handlers in the codebase. The serve and web commands (packages/opencode/src/cli/cmd/serve.ts:28-30 and packages/opencode/src/cli/cmd/web.ts:89-91) use process.on() for SIGTERM, SIGINT, and SIGHUP handlers.
While process.once() is appropriate for this use case due to the shuttingDown guard preventing duplicate shutdowns, this inconsistency may be confusing. Consider documenting why once() is used here versus on() in other commands, or standardizing the approach across all commands.
| process.once("SIGHUP", shutdown) | |
| process.once("SIGTERM", shutdown) | |
| process.once("SIGINT", shutdown) | |
| process.on("SIGHUP", shutdown) | |
| process.on("SIGTERM", shutdown) | |
| process.on("SIGINT", shutdown) |
Summary
Fixes #159