From dc452738d58ead01fb6b7a3f2f913949c2175893 Mon Sep 17 00:00:00 2001 From: Francisco Rodrigues Date: Wed, 15 Jul 2026 19:52:21 -0300 Subject: [PATCH] fix: orphaned agent processes when deleting a worktree in the TUI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a worktree and its session from the TUI left the agent's tmux session and the whole process tree under it alive, consuming resources and pointed at a directory that had just been removed. The session stayed invisible to the dashboard, so the only way to notice was to run tmux ls or watch the machine get slower. The delete path killed its two sessions by different mechanisms. The shell session was killed by its tmux session ID, which always works. The agent session went through sesSvc.Stop, which rebuilds the session name from (project, branch) and looks it up in tmux list-sessions. When that lookup missed, Stop returned ErrSessionNotFound, the caller discarded it with `_ =`, and the worktree was force-deleted anyway. The rebuilt name misses for two independent reasons. Stop derives it from Item.Branch, which carries the display branch — a value items.go deliberately lets diverge from the identity branch the session was named after. Stop also hardcodes an empty profile, so under an active profile it searches for myapp-feat while the session is really work-myapp-feat. Both sessions are now killed by the tmux session ID the item already carries, which is immune to both hazards because it never reconstructs a name, and a failed kill surfaces as an error instead of letting the worktree be deleted out from under a live process. This matches what confirmDeleteAgent and confirmDeleteShell already did. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/tui/actions.go | 20 +++-- internal/tui/delete_teardown_test.go | 114 +++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 7 deletions(-) create mode 100644 internal/tui/delete_teardown_test.go diff --git a/internal/tui/actions.go b/internal/tui/actions.go index 098ea15..ac24dda 100644 --- a/internal/tui/actions.go +++ b/internal/tui/actions.go @@ -322,20 +322,26 @@ func (m Model) confirmDeleteAll() (tea.Model, tea.Cmd) { m.confirm = nil m.screen = screenList - sesSvc := m.sesSvc wtSvc := m.wtSvc tmuxClient := m.tmuxClient project := target.Project branch := target.Branch + agentID := target.AgentSessionID shellID := target.ShellSessionID return m, func() tea.Msg { - if target.AgentSessionID != "" { - _ = sesSvc.Stop(project, branch, semconv.SessionTypeAgent) - } - - if shellID != "" { - _ = tmuxClient.KillSession(shellID) + // Kill by tmux session ID, never by a rebuilt session name: the name + // depends on the active profile and on the identity branch, either of + // which can differ from what this item displays. A lookup miss here + // would leave the session — and the agent process under it — alive + // while the worktree is force-deleted out from under it. + for _, id := range []string{agentID, shellID} { + if id == "" { + continue + } + if err := tmuxClient.KillSession(id); err != nil { + return errMsg{err: fmt.Errorf("killing session %s: %w", id, err)} + } } err := wtSvc.Delete(worktree.DeleteRequest{ diff --git a/internal/tui/delete_teardown_test.go b/internal/tui/delete_teardown_test.go new file mode 100644 index 0000000..eda6007 --- /dev/null +++ b/internal/tui/delete_teardown_test.go @@ -0,0 +1,114 @@ +package tui + +import ( + "strings" + "testing" + + "github.com/xico42/codeherd/internal/config" + "github.com/xico42/codeherd/internal/hooks" + "github.com/xico42/codeherd/internal/session" + "github.com/xico42/codeherd/internal/tmux" + "github.com/xico42/codeherd/internal/worktree" +) + +// recordingRunner serves a fixed list-sessions table and records kill-session targets. +type recordingRunner struct { + sessions string + killed []string +} + +func (r *recordingRunner) Run(args ...string) (string, string, int, error) { + switch args[0] { + case "list-sessions": + return r.sessions, "", 0, nil + case "kill-session": + // args: kill-session -t + r.killed = append(r.killed, args[2]) + return "", "", 0, nil + } + return "", "", 0, nil +} + +// sessionRow builds one tab-separated list-sessions record. +func sessionRow(id, name, canonical, sessType, profile, branch string) string { + return strings.Join([]string{id, name, canonical, sessType, "running", "", "", profile, branch}, "\t") +} + +// A worktree whose HEAD has diverged displays the checked-out branch, which is +// not the identity branch its session was named after. Teardown must not depend +// on that display value. No profile is involved here. +func TestConfirmDeleteAll_divergedHeadSessionIsKilled(t *testing.T) { + runner := &recordingRunner{ + sessions: sessionRow("$1", "myapp-feat", "myapp-feat", "agent", "", "feat"), + } + client := tmux.NewClient(runner) + cfg := &config.Config{Projects: map[string]config.ProjectConfig{}} + + m := Model{ + sesSvc: session.NewService(client, &hooks.NoOp{}), + wtSvc: worktree.NewService(cfg, worktree.NewRealWorktreeRunner(), client, &hooks.NoOp{}), + tmuxClient: client, + confirm: newConfirmModel(Item{ + Project: "myapp", + Branch: "other", // displayed branch after divergence + AgentSessionID: "$1", + HasAgent: true, + HeadHint: "on other", + }), + } + + _, cmd := m.confirmDeleteAll() + cmd() + + if len(runner.killed) != 1 || runner.killed[0] != "$1" { + t.Errorf("agent session $1 not killed; killed=%v", runner.killed) + } +} + +// deleteAll on a profile-scoped worktree must kill both tmux sessions. The +// worktree is force-deleted regardless, so a missed kill leaves the agent +// process running against a directory that no longer exists. +func TestConfirmDeleteAll_profileScopedSessionsAreKilled(t *testing.T) { + runner := &recordingRunner{ + sessions: strings.Join([]string{ + sessionRow("$1", "work-myapp-feat", "work-myapp-feat", "agent", "work", "feat"), + sessionRow("$2", "work-myapp-feat~sh", "work-myapp-feat", "shell", "work", "feat"), + }, "\n"), + } + client := tmux.NewClient(runner) + // No projects configured, so worktree.Delete returns an error instead of + // touching the filesystem — teardown of the tmux sessions must still happen. + cfg := &config.Config{Projects: map[string]config.ProjectConfig{}} + + m := Model{ + sesSvc: session.NewService(client, &hooks.NoOp{}), + wtSvc: worktree.NewService(cfg, worktree.NewRealWorktreeRunner(), client, &hooks.NoOp{}), + tmuxClient: client, + confirm: newConfirmModel(Item{ + Project: "myapp", + Branch: "feat", + AgentSessionID: "$1", + ShellSessionID: "$2", + HasAgent: true, + HasShell: true, + }), + } + + _, cmd := m.confirmDeleteAll() + if cmd == nil { + t.Fatal("confirmDeleteAll returned no command") + } + cmd() // executes the teardown closure; worktree.Delete fails harmlessly (no repo) + + for _, want := range []string{"$1", "$2"} { + found := false + for _, got := range runner.killed { + if got == want { + found = true + } + } + if !found { + t.Errorf("session %s was never killed; killed=%v (dangling tmux session + processes)", want, runner.killed) + } + } +}