Improve terminal server teardown behavior.#12205
Conversation
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds a terminal-server shutdown request/response path, stops PTY event loops before terminal-server teardown, and updates hosted PTY cleanup to signal entire process groups with a bounded SIGKILL fallback. It also adds targeted Unix tests for process-group termination and escalation behavior.
Concerns
- No blocking correctness concerns found in the changed lines.
- No security findings found in the supplemental security pass.
- No approved or repository spec context was available for implementation drift review.
- The behavioral change is a non-visual lifecycle cleanup; the PR includes targeted process-group tests and manual startup/shutdown testing, so screenshots or video would not add meaningful evidence for this review.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds a graceful terminal-server shutdown protocol, group-aware SIGHUP/SIGKILL cleanup for server-owned PTYs, and shutdown ordering changes so PTY event loops stop before the terminal server is torn down.
Concerns
- The new cleanup still loses the PTY process group once the shell leader has already exited and been reaped by the terminal server, so descendants can remain unsignaled during later pane or app shutdown; see the inline comment.
- No approved spec context was provided. This is non-visual teardown behavior, so screenshots or recordings would not add meaningful evidence.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| /// while descendants remain in that group, so this keeps checking group | ||
| /// liveness throughout a grace period and sends `SIGKILL` to survivors. | ||
| fn terminate_child(&mut self, pid: u32) -> Result<()> { | ||
| if !self.0.contains_key(&pid) { |
There was a problem hiding this comment.
terminated_children() has reaped and removed the shell leader, but descendants can still be alive in that PTY process group; keep tracking known pgids until the group is gone and run the group-aware cleanup for already-exited children too.
1 participant
Description
Fix terminal-server teardown so Warp does not leave server-owned PTY shell process groups running after application shutdown.
This adds an explicit terminal-server shutdown request/response, terminates PTY process groups with
SIGHUPfollowed by a boundedSIGKILLfallback, and reuses the same group-aware cleanup when terminating individual children. It also stops pane PTY event loops before tearing down the terminal server so normal shutdown does not produce redundant socket writes or misleading EOF logs.Note
note from dstern: i was seeing (terminal session) shell processes leaking, and impacting performance of my system. i asked agent mode to do a little investigation, it pointed out that we don't have a terribly robust teardown strategy for the terminal server. while i'm not entirely sure whether this will fix the cause, improving this can't hurt.
Linked Issue
N/A
Testing
cargo fmt --manifest-path /Users/david/src/warp/Cargo.toml --package warpcargo nextest run --manifest-path /Users/david/src/warp/Cargo.toml -p warp -E 'test(terminate_child_signals_the_entire_pty_process_group) | test(terminate_child_escalates_when_a_descendant_ignores_sighup_after_the_shell_exits) | test(terminate_all_escalates_when_a_pty_process_group_ignores_sighup)'cargo check --manifest-path /Users/david/src/warp/Cargo.toml -p warp --libgit diff --check./script/runAgent Mode
CHANGELOG-BUG-FIX: Prevent terminal shell processes from being left running after Warp shuts down.
Co-Authored-By: Oz oz-agent@warp.dev