fix: clean up all sessions on worktree removal#191
Open
JHK wants to merge 6 commits into
Open
Conversation
…btx) - Add `TerminalMgr.Close(id + RunSessionSuffix)` in RemoveWorktree resolver so run sessions (keyed `<id>__run`) are cleaned up alongside regular sessions - Add TestRemoveWorktreeClosesRunSession covering the cleanup path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… (Refs: beans-4btx) - Stop agent process (AgentMgr.StopSession) when removing a worktree - Extract initTestRepo into internal/testutil.InitTestRepo shared helper - Remove redundant comment restating what the code does Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s: beans-4btx) - Log warning when AgentMgr.StopSession fails instead of silently discarding error - Add AgentMgr to RemoveWorktree test and verify agent session is stopped - Rename test to TestRemoveWorktreeCleansUpSessions to reflect broader scope - Remove trivial initTestRepo wrapper in worktree_test.go, call testutil.InitTestRepo directly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s: beans-4btx) - Stop agent/terminal sessions before removing the worktree directory - Remove dead error handling for StopSession (always returns nil) - Strengthen test by setting status to Running before removal - Add defer agentMgr.Shutdown() for proper test cleanup - Remove extra blank line in worktree_test.go Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JHK
changed the title
- Set Setpgid on commands spawned by CreateWithCommand (Unix only) - Session.Close() sends SIGTERM to entire process group, escalates to SIGKILL after 3s grace period - Interactive shell sessions unchanged (still use direct Process.Kill) - Platform-specific files: process_unix.go, process_windows.go (no-op) - Tests for process group kill and graceful SIGTERM shutdown Refs: beans-3fmc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove redundant Setpgid/pgid field; go-pty's Setsid already makes every spawned PID a process group leader - Close() now unconditionally kills the process group via -pid - SIGTERM with 3s grace, escalating to SIGKILL - Applies to all sessions (interactive and command), not just CreateWithCommand Refs: beans-3fmc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JHK
changed the title
JHK
changed the title
Contributor
Author
|
This code is so far au pair programmed but not tested by me. I will convert this to a proper PR once I've validated the fixes and consider them useful. Background: While working with beans-serve over longer sessions — starting workspaces, finishing work, integrating changes, and closing workspaces — I noticed orphaned claude and watchexec processes accumulating. This eventually escalated to running out of file descriptors. I don't have a consistent reproduction yet, but code analysis confirmed several cleanup gaps. |
Contributor
Author
|
Finally found the time to do some testing and can confirm that this patch does indeed fix the issue of processes piling up and lingering around - thus ready for review. |
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
1 participant
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
RemoveWorktreehad several session cleanup issues, and terminal session teardown didn't kill descendant processes:Session cleanup on worktree removal:
id + "__run") were never closed — the shell process kept runningProcess group kill for terminal sessions:
mise devspawning multiple subprocesses)-pid) to reap all descendants on UnixChanges
idandid + RunSessionSuffixterminal sessions on worktree removalAgentMgr.StopSession(id)killProcessGroup()platform helper (Unix:syscall.Kill(-pid, SIGKILL); Windows:taskkill /F /T)Session.Close()to reap all descendant processestestutil.InitTestRepohelper🤖 Generated with Claude Code