From 7c7b9554bdb967b7b65727e96fb0df59342a1ed1 Mon Sep 17 00:00:00 2001 From: Julian Knocke <352838+JHK@users.noreply.github.com> Date: Mon, 13 Apr 2026 14:19:48 +0200 Subject: [PATCH 1/6] fix: close run terminal session when removing worktree (Refs: beans-4btx) - Add `TerminalMgr.Close(id + RunSessionSuffix)` in RemoveWorktree resolver so run sessions (keyed `__run`) are cleaned up alongside regular sessions - Add TestRemoveWorktreeClosesRunSession covering the cleanup path Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/graph/schema.resolvers.go | 3 +- internal/graph/schema.resolvers_test.go | 77 +++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/internal/graph/schema.resolvers.go b/internal/graph/schema.resolvers.go index 54b4886c..f9f34a01 100644 --- a/internal/graph/schema.resolvers.go +++ b/internal/graph/schema.resolvers.go @@ -252,9 +252,10 @@ func (r *mutationResolver) RemoveWorktree(ctx context.Context, id string) (bool, r.PortAlloc.Free(id) } - // Close any terminal session associated with this worktree + // Close any terminal sessions associated with this worktree if r.TerminalMgr != nil { r.TerminalMgr.Close(id) + r.TerminalMgr.Close(id + RunSessionSuffix) } return true, nil diff --git a/internal/graph/schema.resolvers_test.go b/internal/graph/schema.resolvers_test.go index ed91bfb6..723b6070 100644 --- a/internal/graph/schema.resolvers_test.go +++ b/internal/graph/schema.resolvers_test.go @@ -12,6 +12,8 @@ import ( "time" "github.com/hmans/beans/internal/agent" + "github.com/hmans/beans/internal/terminal" + "github.com/hmans/beans/internal/worktree" "github.com/hmans/beans/pkg/beangraph" "github.com/hmans/beans/pkg/beangraph/model" "github.com/hmans/beans/pkg/bean" @@ -3252,3 +3254,78 @@ func TestListFiles(t *testing.T) { }) } +func TestRemoveWorktreeClosesRunSession(t *testing.T) { + // Set up a real git repo so the worktree manager works + repoDir := t.TempDir() + for _, args := range [][]string{ + {"git", "init", "-b", "main"}, + {"git", "config", "user.email", "test@test.com"}, + {"git", "config", "user.name", "Test"}, + {"git", "commit", "--allow-empty", "-m", "initial"}, + } { + cmd := exec.Command(args[0], args[1:]...) + cmd.Dir = repoDir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("%v failed: %s: %v", args, out, err) + } + } + + beansDir := filepath.Join(repoDir, ".beans") + if err := os.MkdirAll(beansDir, 0755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + wtRoot := t.TempDir() + wtMgr := worktree.NewManager(repoDir, wtRoot, "main", "") + termMgr := terminal.NewManager(nil) + defer termMgr.Shutdown() + + cfg := config.Default() + core := beancore.New(beansDir, cfg) + if err := core.Load(); err != nil { + t.Fatalf("core.Load: %v", err) + } + + resolver := &Resolver{ + CoreResolver: &beangraph.CoreResolver{Core: core}, + WorktreeMgr: wtMgr, + TerminalMgr: termMgr, + } + + // Create a worktree + wt, err := wtMgr.Create("test-wt") + if err != nil { + t.Fatalf("Create worktree: %v", err) + } + + // Create both a regular terminal session and a run session for this worktree + if _, err := termMgr.Create(wt.ID, os.TempDir(), 80, 24); err != nil { + t.Fatalf("Create terminal session: %v", err) + } + if _, err := termMgr.Create(wt.ID+RunSessionSuffix, os.TempDir(), 80, 24); err != nil { + t.Fatalf("Create run session: %v", err) + } + + // Verify both sessions exist + if termMgr.Get(wt.ID) == nil { + t.Fatal("terminal session should exist before removal") + } + if termMgr.Get(wt.ID+RunSessionSuffix) == nil { + t.Fatal("run session should exist before removal") + } + + // Remove the worktree + mr := resolver.Mutation() + if _, err := mr.RemoveWorktree(context.Background(), wt.ID); err != nil { + t.Fatalf("RemoveWorktree: %v", err) + } + + // Both sessions should be closed + if termMgr.Get(wt.ID) != nil { + t.Error("terminal session should be closed after worktree removal") + } + if termMgr.Get(wt.ID+RunSessionSuffix) != nil { + t.Error("run session should be closed after worktree removal") + } +} + From e0095352c292862f80b41fbee880abc91a497bd1 Mon Sep 17 00:00:00 2001 From: Julian Knocke <352838+JHK@users.noreply.github.com> Date: Mon, 13 Apr 2026 14:33:21 +0200 Subject: [PATCH 2/6] fix: also stop agent session on worktree removal, extract test helper (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) --- internal/graph/schema.resolvers.go | 5 +++- internal/graph/schema.resolvers_test.go | 23 ++------------- internal/testutil/git.go | 37 +++++++++++++++++++++++++ internal/worktree/worktree_test.go | 31 ++------------------- 4 files changed, 46 insertions(+), 50 deletions(-) create mode 100644 internal/testutil/git.go diff --git a/internal/graph/schema.resolvers.go b/internal/graph/schema.resolvers.go index f9f34a01..c01f2ddf 100644 --- a/internal/graph/schema.resolvers.go +++ b/internal/graph/schema.resolvers.go @@ -252,7 +252,10 @@ func (r *mutationResolver) RemoveWorktree(ctx context.Context, id string) (bool, r.PortAlloc.Free(id) } - // Close any terminal sessions associated with this worktree + if r.AgentMgr != nil { + r.AgentMgr.StopSession(id) + } + if r.TerminalMgr != nil { r.TerminalMgr.Close(id) r.TerminalMgr.Close(id + RunSessionSuffix) diff --git a/internal/graph/schema.resolvers_test.go b/internal/graph/schema.resolvers_test.go index 723b6070..1888032d 100644 --- a/internal/graph/schema.resolvers_test.go +++ b/internal/graph/schema.resolvers_test.go @@ -13,6 +13,7 @@ import ( "github.com/hmans/beans/internal/agent" "github.com/hmans/beans/internal/terminal" + "github.com/hmans/beans/internal/testutil" "github.com/hmans/beans/internal/worktree" "github.com/hmans/beans/pkg/beangraph" "github.com/hmans/beans/pkg/beangraph/model" @@ -3255,27 +3256,7 @@ func TestListFiles(t *testing.T) { } func TestRemoveWorktreeClosesRunSession(t *testing.T) { - // Set up a real git repo so the worktree manager works - repoDir := t.TempDir() - for _, args := range [][]string{ - {"git", "init", "-b", "main"}, - {"git", "config", "user.email", "test@test.com"}, - {"git", "config", "user.name", "Test"}, - {"git", "commit", "--allow-empty", "-m", "initial"}, - } { - cmd := exec.Command(args[0], args[1:]...) - cmd.Dir = repoDir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("%v failed: %s: %v", args, out, err) - } - } - - beansDir := filepath.Join(repoDir, ".beans") - if err := os.MkdirAll(beansDir, 0755); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - - wtRoot := t.TempDir() + repoDir, beansDir, wtRoot := testutil.InitTestRepo(t) wtMgr := worktree.NewManager(repoDir, wtRoot, "main", "") termMgr := terminal.NewManager(nil) defer termMgr.Shutdown() diff --git a/internal/testutil/git.go b/internal/testutil/git.go new file mode 100644 index 00000000..d34c5639 --- /dev/null +++ b/internal/testutil/git.go @@ -0,0 +1,37 @@ +package testutil + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +// InitTestRepo creates a temporary git repo with an initial commit, +// a .beans directory inside it, and a separate worktree root directory. +func InitTestRepo(t *testing.T) (repoDir, beansDir, worktreeRoot string) { + t.Helper() + dir := t.TempDir() + + for _, args := range [][]string{ + {"git", "init", "-b", "main"}, + {"git", "config", "user.email", "test@test.com"}, + {"git", "config", "user.name", "Test"}, + {"git", "commit", "--allow-empty", "-m", "initial"}, + } { + cmd := exec.Command(args[0], args[1:]...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("%v failed: %s: %v", args, out, err) + } + } + + bd := filepath.Join(dir, ".beans") + if err := os.MkdirAll(bd, 0755); err != nil { + t.Fatalf("MkdirAll .beans: %v", err) + } + + wtRoot := t.TempDir() + + return dir, bd, wtRoot +} diff --git a/internal/worktree/worktree_test.go b/internal/worktree/worktree_test.go index 518aa43e..64699dca 100644 --- a/internal/worktree/worktree_test.go +++ b/internal/worktree/worktree_test.go @@ -7,38 +7,13 @@ import ( "strings" "testing" "time" + + "github.com/hmans/beans/internal/testutil" ) -// initTestRepo creates a temporary git repo with an initial commit, -// a .beans directory inside it, and a separate worktree root directory. func initTestRepo(t *testing.T) (repoDir, beansDir, worktreeRoot string) { t.Helper() - dir := t.TempDir() - - commands := [][]string{ - {"git", "init", "-b", "main"}, - {"git", "config", "user.email", "test@test.com"}, - {"git", "config", "user.name", "Test"}, - {"git", "commit", "--allow-empty", "-m", "initial"}, - } - - for _, args := range commands { - cmd := exec.Command(args[0], args[1:]...) - cmd.Dir = dir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("%v failed: %s: %v", args, out, err) - } - } - - bd := filepath.Join(dir, ".beans") - if err := os.MkdirAll(bd, 0755); err != nil { - t.Fatalf("MkdirAll .beans: %v", err) - } - - // Create a separate worktree root directory (outside the repo) - wtRoot := t.TempDir() - - return dir, bd, wtRoot + return testutil.InitTestRepo(t) } func TestParsePorcelain(t *testing.T) { From c30db5c1f65d5dead636a3b1a16553fe2856122e Mon Sep 17 00:00:00 2001 From: Julian Knocke <352838+JHK@users.noreply.github.com> Date: Mon, 13 Apr 2026 14:43:20 +0200 Subject: [PATCH 3/6] fix: handle StopSession error, improve test coverage and cleanup (Refs: 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) --- internal/graph/schema.resolvers.go | 6 ++-- internal/graph/schema.resolvers_test.go | 17 +++++++--- internal/worktree/worktree_test.go | 44 +++++++++++-------------- 3 files changed, 37 insertions(+), 30 deletions(-) diff --git a/internal/graph/schema.resolvers.go b/internal/graph/schema.resolvers.go index c01f2ddf..0ae63e85 100644 --- a/internal/graph/schema.resolvers.go +++ b/internal/graph/schema.resolvers.go @@ -252,10 +252,12 @@ func (r *mutationResolver) RemoveWorktree(ctx context.Context, id string) (bool, r.PortAlloc.Free(id) } + // Clean up sessions associated with this worktree if r.AgentMgr != nil { - r.AgentMgr.StopSession(id) + if err := r.AgentMgr.StopSession(id); err != nil { + fmt.Printf("[beans] warning: failed to stop agent session for worktree %s: %v\n", id, err) + } } - if r.TerminalMgr != nil { r.TerminalMgr.Close(id) r.TerminalMgr.Close(id + RunSessionSuffix) diff --git a/internal/graph/schema.resolvers_test.go b/internal/graph/schema.resolvers_test.go index 1888032d..3c55f697 100644 --- a/internal/graph/schema.resolvers_test.go +++ b/internal/graph/schema.resolvers_test.go @@ -3255,11 +3255,12 @@ func TestListFiles(t *testing.T) { }) } -func TestRemoveWorktreeClosesRunSession(t *testing.T) { +func TestRemoveWorktreeCleansUpSessions(t *testing.T) { repoDir, beansDir, wtRoot := testutil.InitTestRepo(t) wtMgr := worktree.NewManager(repoDir, wtRoot, "main", "") termMgr := terminal.NewManager(nil) defer termMgr.Shutdown() + agentMgr := agent.NewManager("", nil) cfg := config.Default() core := beancore.New(beansDir, cfg) @@ -3271,6 +3272,7 @@ func TestRemoveWorktreeClosesRunSession(t *testing.T) { CoreResolver: &beangraph.CoreResolver{Core: core}, WorktreeMgr: wtMgr, TerminalMgr: termMgr, + AgentMgr: agentMgr, } // Create a worktree @@ -3279,21 +3281,25 @@ func TestRemoveWorktreeClosesRunSession(t *testing.T) { t.Fatalf("Create worktree: %v", err) } - // Create both a regular terminal session and a run session for this worktree + // Create terminal sessions and an agent session for this worktree if _, err := termMgr.Create(wt.ID, os.TempDir(), 80, 24); err != nil { t.Fatalf("Create terminal session: %v", err) } if _, err := termMgr.Create(wt.ID+RunSessionSuffix, os.TempDir(), 80, 24); err != nil { t.Fatalf("Create run session: %v", err) } + agentMgr.AddInfoMessage(wt.ID, "test") - // Verify both sessions exist + // Verify all sessions exist if termMgr.Get(wt.ID) == nil { t.Fatal("terminal session should exist before removal") } if termMgr.Get(wt.ID+RunSessionSuffix) == nil { t.Fatal("run session should exist before removal") } + if agentMgr.GetSession(wt.ID) == nil { + t.Fatal("agent session should exist before removal") + } // Remove the worktree mr := resolver.Mutation() @@ -3301,12 +3307,15 @@ func TestRemoveWorktreeClosesRunSession(t *testing.T) { t.Fatalf("RemoveWorktree: %v", err) } - // Both sessions should be closed + // All sessions should be closed/stopped if termMgr.Get(wt.ID) != nil { t.Error("terminal session should be closed after worktree removal") } if termMgr.Get(wt.ID+RunSessionSuffix) != nil { t.Error("run session should be closed after worktree removal") } + if s := agentMgr.GetSession(wt.ID); s != nil && s.Status != agent.StatusIdle { + t.Errorf("agent session status = %q, want %q", s.Status, agent.StatusIdle) + } } diff --git a/internal/worktree/worktree_test.go b/internal/worktree/worktree_test.go index 64699dca..b6d248de 100644 --- a/internal/worktree/worktree_test.go +++ b/internal/worktree/worktree_test.go @@ -11,10 +11,6 @@ import ( "github.com/hmans/beans/internal/testutil" ) -func initTestRepo(t *testing.T) (repoDir, beansDir, worktreeRoot string) { - t.Helper() - return testutil.InitTestRepo(t) -} func TestParsePorcelain(t *testing.T) { tests := []struct { @@ -145,7 +141,7 @@ branch refs/heads/beans/beans-good } func TestCreateAndList(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") // List should be empty initially @@ -200,7 +196,7 @@ func TestCreateAndList(t *testing.T) { } func TestCreateEmptyName(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") _, err := mgr.Create("") @@ -210,7 +206,7 @@ func TestCreateEmptyName(t *testing.T) { } func TestRemove(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") wt, err := mgr.Create("to-remove") @@ -238,7 +234,7 @@ func TestRemove(t *testing.T) { } func TestRemoveDirtyWorktree(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") wt, err := mgr.Create("dirty-wt") @@ -264,7 +260,7 @@ func TestRemoveDirtyWorktree(t *testing.T) { } func TestRemoveStaleWorktree(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") // Create a worktree, then delete its directory out from under git @@ -283,7 +279,7 @@ func TestRemoveStaleWorktree(t *testing.T) { } func TestRemoveNonexistent(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") // Remove a worktree that doesn't exist should return an error @@ -297,7 +293,7 @@ func TestRemoveNonexistent(t *testing.T) { } func TestCreateUsesBaseRef(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) // Create a second commit on a new branch so we have a distinct ref to branch from commands := [][]string{ @@ -358,7 +354,7 @@ func TestFetchTimeoutCustom(t *testing.T) { } func TestFetchTimeoutZeroSkipsFetch(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) // Create a manager with fetch timeout = 0 (disables fetch) mgr := NewManager(repoDir, wtRoot, "main", "", WithFetchTimeout(0)) @@ -374,7 +370,7 @@ func TestFetchTimeoutZeroSkipsFetch(t *testing.T) { } func TestFetchBaseRefTimesOut(t *testing.T) { - repoDir, _, _ := initTestRepo(t) + repoDir, _, _ := testutil.InitTestRepo(t) // Configure git to use a "fetch" command that just sleeps, simulating a hanging remote. // GIT_SSH_COMMAND is used by git when fetching over SSH. @@ -407,7 +403,7 @@ func TestFetchBaseRefTimesOut(t *testing.T) { } func TestSubscription(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") ch := mgr.Subscribe() @@ -440,7 +436,7 @@ func TestSubscription(t *testing.T) { } func TestDetectBeanIDs(t *testing.T) { - repoDir, beansDir, wtRoot := initTestRepo(t) + repoDir, beansDir, wtRoot := testutil.InitTestRepo(t) // Commit a file in .beans so the directory exists on main if err := os.WriteFile(filepath.Join(beansDir, ".gitkeep"), []byte(""), 0644); err != nil { @@ -515,7 +511,7 @@ func TestDetectBeanIDs(t *testing.T) { } func TestDetectBeanIDs_NoChanges(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "main", "") // Create a worktree with no bean changes @@ -531,7 +527,7 @@ func TestDetectBeanIDs_NoChanges(t *testing.T) { } func TestDetectBeanIDs_UncommittedChanges(t *testing.T) { - repoDir, beansDir, wtRoot := initTestRepo(t) + repoDir, beansDir, wtRoot := testutil.InitTestRepo(t) // Commit a file in .beans so the directory exists on main if err := os.WriteFile(filepath.Join(beansDir, ".gitkeep"), []byte(""), 0644); err != nil { @@ -568,7 +564,7 @@ func TestDetectBeanIDs_UncommittedChanges(t *testing.T) { } func TestDetectBeanIDs_DeletedFile(t *testing.T) { - repoDir, beansDir, wtRoot := initTestRepo(t) + repoDir, beansDir, wtRoot := testutil.InitTestRepo(t) // Create a bean on main if err := os.WriteFile(filepath.Join(beansDir, "beans-del1--to-delete.md"), []byte("---\ntitle: To Delete\nstatus: todo\ntype: task\n---\n"), 0644); err != nil { @@ -601,7 +597,7 @@ func TestDetectBeanIDs_DeletedFile(t *testing.T) { } func TestUpdateDescription(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") // Create a worktree @@ -645,7 +641,7 @@ func TestUpdateDescription(t *testing.T) { } func TestCreateRunsSetupCommand(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) // Use a setup command that creates a marker file mgr := NewManager(repoDir, wtRoot, "", "touch .setup-done") @@ -695,7 +691,7 @@ func TestCreateRunsSetupCommand(t *testing.T) { } func TestCreateNoSetupCommand(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) // No setup command — should still create fine mgr := NewManager(repoDir, wtRoot, "", "") @@ -712,7 +708,7 @@ func TestCreateNoSetupCommand(t *testing.T) { } func TestTouchLastActive(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") // Create a worktree @@ -756,7 +752,7 @@ func TestTouchLastActive(t *testing.T) { } func TestListKeepsCreationOrder(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") // Create three worktrees in a specific order @@ -804,7 +800,7 @@ func TestListKeepsCreationOrder(t *testing.T) { } func TestCreateSetsLastActiveAt(t *testing.T) { - repoDir, _, wtRoot := initTestRepo(t) + repoDir, _, wtRoot := testutil.InitTestRepo(t) mgr := NewManager(repoDir, wtRoot, "", "") before := time.Now().UTC().Add(-time.Second) From 361e21214f967bea12ae15614fd9db0ebc32df2f Mon Sep 17 00:00:00 2001 From: Julian Knocke <352838+JHK@users.noreply.github.com> Date: Mon, 13 Apr 2026 15:07:12 +0200 Subject: [PATCH 4/6] fix: improve worktree session cleanup ordering and test coverage (Refs: 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) --- internal/graph/schema.resolvers.go | 21 +++++++++------------ internal/graph/schema.resolvers_test.go | 6 +++++- internal/worktree/worktree_test.go | 1 - 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/graph/schema.resolvers.go b/internal/graph/schema.resolvers.go index 0ae63e85..ed63e0db 100644 --- a/internal/graph/schema.resolvers.go +++ b/internal/graph/schema.resolvers.go @@ -243,26 +243,23 @@ func (r *mutationResolver) RemoveWorktree(ctx context.Context, id string) (bool, } } + // Clean up sessions before removing the worktree directory + if r.AgentMgr != nil { + r.AgentMgr.StopSession(id) + } + if r.TerminalMgr != nil { + r.TerminalMgr.Close(id) + r.TerminalMgr.Close(id + RunSessionSuffix) + } + if err := r.WorktreeMgr.Remove(id); err != nil { return false, err } - // Free the workspace port if r.PortAlloc != nil { r.PortAlloc.Free(id) } - // Clean up sessions associated with this worktree - if r.AgentMgr != nil { - if err := r.AgentMgr.StopSession(id); err != nil { - fmt.Printf("[beans] warning: failed to stop agent session for worktree %s: %v\n", id, err) - } - } - if r.TerminalMgr != nil { - r.TerminalMgr.Close(id) - r.TerminalMgr.Close(id + RunSessionSuffix) - } - return true, nil } diff --git a/internal/graph/schema.resolvers_test.go b/internal/graph/schema.resolvers_test.go index 3c55f697..4605b221 100644 --- a/internal/graph/schema.resolvers_test.go +++ b/internal/graph/schema.resolvers_test.go @@ -3261,6 +3261,7 @@ func TestRemoveWorktreeCleansUpSessions(t *testing.T) { termMgr := terminal.NewManager(nil) defer termMgr.Shutdown() agentMgr := agent.NewManager("", nil) + defer agentMgr.Shutdown() cfg := config.Default() core := beancore.New(beansDir, cfg) @@ -3289,6 +3290,7 @@ func TestRemoveWorktreeCleansUpSessions(t *testing.T) { t.Fatalf("Create run session: %v", err) } agentMgr.AddInfoMessage(wt.ID, "test") + agentMgr.GetSession(wt.ID).Status = agent.StatusRunning // Verify all sessions exist if termMgr.Get(wt.ID) == nil { @@ -3314,7 +3316,9 @@ func TestRemoveWorktreeCleansUpSessions(t *testing.T) { if termMgr.Get(wt.ID+RunSessionSuffix) != nil { t.Error("run session should be closed after worktree removal") } - if s := agentMgr.GetSession(wt.ID); s != nil && s.Status != agent.StatusIdle { + if s := agentMgr.GetSession(wt.ID); s == nil { + t.Error("agent session should still exist after worktree removal") + } else if s.Status != agent.StatusIdle { t.Errorf("agent session status = %q, want %q", s.Status, agent.StatusIdle) } } diff --git a/internal/worktree/worktree_test.go b/internal/worktree/worktree_test.go index b6d248de..15b78e6e 100644 --- a/internal/worktree/worktree_test.go +++ b/internal/worktree/worktree_test.go @@ -11,7 +11,6 @@ import ( "github.com/hmans/beans/internal/testutil" ) - func TestParsePorcelain(t *testing.T) { tests := []struct { name string From caf78f2c74a690973d042ad09dc2c9a2798ea3fc Mon Sep 17 00:00:00 2001 From: Julian Knocke <352838+JHK@users.noreply.github.com> Date: Mon, 13 Apr 2026 15:18:03 +0200 Subject: [PATCH 5/6] fix: reap descendants of run-command shells via process group isolation - 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) --- internal/terminal/process_unix.go | 29 +++++++++ internal/terminal/process_unix_test.go | 83 ++++++++++++++++++++++++++ internal/terminal/process_windows.go | 9 +++ internal/terminal/terminal.go | 19 ++++-- 4 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 internal/terminal/process_unix.go create mode 100644 internal/terminal/process_unix_test.go create mode 100644 internal/terminal/process_windows.go diff --git a/internal/terminal/process_unix.go b/internal/terminal/process_unix.go new file mode 100644 index 00000000..a0113ff3 --- /dev/null +++ b/internal/terminal/process_unix.go @@ -0,0 +1,29 @@ +//go:build !windows + +package terminal + +import ( + "syscall" + "time" + + gopty "github.com/aymanbagabas/go-pty" +) + +const processGroupGrace = 3 * time.Second + +func setProcessGroup(cmd *gopty.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} +} + +// closeProcessGroup sends SIGTERM to the process group, waits for it to exit, +// and escalates to SIGKILL after the grace period. +func closeProcessGroup(pgid int, done <-chan struct{}) { + _ = syscall.Kill(-pgid, syscall.SIGTERM) + + select { + case <-done: + return + case <-time.After(processGroupGrace): + _ = syscall.Kill(-pgid, syscall.SIGKILL) + } +} diff --git a/internal/terminal/process_unix_test.go b/internal/terminal/process_unix_test.go new file mode 100644 index 00000000..1addbd7b --- /dev/null +++ b/internal/terminal/process_unix_test.go @@ -0,0 +1,83 @@ +//go:build !windows + +package terminal + +import ( + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +func TestCloseKillsProcessGroup(t *testing.T) { + mgr := NewManager(nil) + defer mgr.Shutdown() + + pidFile := filepath.Join(t.TempDir(), "child.pid") + + command := fmt.Sprintf(`sh -c 'echo $$ > %s; sleep 300' & wait`, pidFile) + _, err := mgr.CreateWithCommand("test-pgkill", os.TempDir(), 80, 24, command) + if err != nil { + t.Fatalf("CreateWithCommand failed: %v", err) + } + + var childPID int + deadline := time.After(5 * time.Second) + for { + data, readErr := os.ReadFile(pidFile) + if readErr == nil { + trimmed := strings.TrimSpace(string(data)) + if trimmed != "" { + childPID, err = strconv.Atoi(trimmed) + if err == nil { + break + } + } + } + select { + case <-deadline: + t.Fatal("timed out waiting for child PID file") + default: + time.Sleep(50 * time.Millisecond) + } + } + + if err := syscall.Kill(childPID, 0); err != nil { + t.Fatalf("child process %d not alive before close: %v", childPID, err) + } + + mgr.Close("test-pgkill") + + time.Sleep(500 * time.Millisecond) + + if err := syscall.Kill(childPID, 0); err == nil { + t.Fatalf("child process %d still alive after session close", childPID) + } +} + +func TestCloseProcessGroupGracefulShutdown(t *testing.T) { + mgr := NewManager(nil) + defer mgr.Shutdown() + + // trap SIGTERM so the process exits cleanly on SIGTERM (not SIGKILL) + sess, err := mgr.CreateWithCommand("test-pg-graceful", os.TempDir(), 80, 24, + `trap 'exit 0' TERM; sleep 300`) + if err != nil { + t.Fatalf("CreateWithCommand failed: %v", err) + } + + // Give the shell time to set up the trap + time.Sleep(200 * time.Millisecond) + + mgr.Close("test-pg-graceful") + + select { + case <-sess.Done(): + case <-time.After(5 * time.Second): + t.Fatal("session did not exit after SIGTERM") + } +} diff --git a/internal/terminal/process_windows.go b/internal/terminal/process_windows.go new file mode 100644 index 00000000..4c986f02 --- /dev/null +++ b/internal/terminal/process_windows.go @@ -0,0 +1,9 @@ +//go:build windows + +package terminal + +import gopty "github.com/aymanbagabas/go-pty" + +func setProcessGroup(_ *gopty.Cmd) {} + +func closeProcessGroup(_ int, _ <-chan struct{}) {} diff --git a/internal/terminal/terminal.go b/internal/terminal/terminal.go index 38b1efc0..5ca8e17c 100644 --- a/internal/terminal/terminal.go +++ b/internal/terminal/terminal.go @@ -68,10 +68,11 @@ func (r *RingBuffer) Bytes() []byte { // Session represents an active PTY session with scrollback buffering. type Session struct { - id string - pty gopty.Pty - cmd *gopty.Cmd - mu sync.Mutex + id string + pty gopty.Pty + cmd *gopty.Cmd + pgid int // non-zero when using process group isolation + mu sync.Mutex scrollback *RingBuffer scrollMu sync.Mutex @@ -177,12 +178,16 @@ func (s *Session) readLoop() { } } -// Close kills the process and closes the PTY. +// Close kills the process (and its process group, if isolated) and closes the PTY. func (s *Session) Close() { s.mu.Lock() defer s.mu.Unlock() if s.cmd.Process != nil { - _ = s.cmd.Process.Kill() + if s.pgid != 0 { + closeProcessGroup(s.pgid, s.done) + } else { + _ = s.cmd.Process.Kill() + } } _ = s.pty.Close() _ = s.cmd.Wait() @@ -339,6 +344,7 @@ func (m *Manager) CreateWithCommand(sessionID, workDir string, cols, rows uint16 cmd := p.Command(shell, "-l", "-c", command) cmd.Dir = workDir cmd.Env = env + setProcessGroup(cmd) if err := cmd.Start(); err != nil { p.Close() @@ -351,6 +357,7 @@ func (m *Manager) CreateWithCommand(sessionID, workDir string, cols, rows uint16 id: sessionID, pty: p, cmd: cmd, + pgid: cmd.Process.Pid, scrollback: NewRingBuffer(scrollbackSize), done: make(chan struct{}), } From daabae648b41755373d79a0217653ad00926126b Mon Sep 17 00:00:00 2001 From: Julian Knocke <352838+JHK@users.noreply.github.com> Date: Mon, 13 Apr 2026 15:32:01 +0200 Subject: [PATCH 6/6] =?UTF-8?q?fix:=20simplify=20process=20group=20kill=20?= =?UTF-8?q?=E2=80=94=20go-pty=20already=20sets=20Setsid?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- internal/terminal/process_unix.go | 17 ++++++----------- internal/terminal/process_windows.go | 10 ++++++---- internal/terminal/terminal.go | 20 +++++++------------- 3 files changed, 19 insertions(+), 28 deletions(-) diff --git a/internal/terminal/process_unix.go b/internal/terminal/process_unix.go index a0113ff3..84af029f 100644 --- a/internal/terminal/process_unix.go +++ b/internal/terminal/process_unix.go @@ -5,25 +5,20 @@ package terminal import ( "syscall" "time" - - gopty "github.com/aymanbagabas/go-pty" ) const processGroupGrace = 3 * time.Second -func setProcessGroup(cmd *gopty.Cmd) { - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} -} - -// closeProcessGroup sends SIGTERM to the process group, waits for it to exit, -// and escalates to SIGKILL after the grace period. -func closeProcessGroup(pgid int, done <-chan struct{}) { - _ = syscall.Kill(-pgid, syscall.SIGTERM) +// killProcessGroup sends SIGTERM to the process group, then escalates to +// SIGKILL after the grace period. go-pty sets Setsid on every spawned command, +// so the PID is always the process group leader. +func killProcessGroup(pid int, done <-chan struct{}) { + _ = syscall.Kill(-pid, syscall.SIGTERM) select { case <-done: return case <-time.After(processGroupGrace): - _ = syscall.Kill(-pgid, syscall.SIGKILL) + _ = syscall.Kill(-pid, syscall.SIGKILL) } } diff --git a/internal/terminal/process_windows.go b/internal/terminal/process_windows.go index 4c986f02..a878f9dd 100644 --- a/internal/terminal/process_windows.go +++ b/internal/terminal/process_windows.go @@ -2,8 +2,10 @@ package terminal -import gopty "github.com/aymanbagabas/go-pty" +import "os" -func setProcessGroup(_ *gopty.Cmd) {} - -func closeProcessGroup(_ int, _ <-chan struct{}) {} +func killProcessGroup(pid int, _ <-chan struct{}) { + if p, err := os.FindProcess(pid); err == nil { + _ = p.Kill() + } +} diff --git a/internal/terminal/terminal.go b/internal/terminal/terminal.go index 5ca8e17c..9f8c3afa 100644 --- a/internal/terminal/terminal.go +++ b/internal/terminal/terminal.go @@ -68,11 +68,10 @@ func (r *RingBuffer) Bytes() []byte { // Session represents an active PTY session with scrollback buffering. type Session struct { - id string - pty gopty.Pty - cmd *gopty.Cmd - pgid int // non-zero when using process group isolation - mu sync.Mutex + id string + pty gopty.Pty + cmd *gopty.Cmd + mu sync.Mutex scrollback *RingBuffer scrollMu sync.Mutex @@ -178,16 +177,13 @@ func (s *Session) readLoop() { } } -// Close kills the process (and its process group, if isolated) and closes the PTY. +// Close kills the process group and closes the PTY. go-pty sets Setsid on +// every spawned command, so the PID is always the process group leader. func (s *Session) Close() { s.mu.Lock() defer s.mu.Unlock() if s.cmd.Process != nil { - if s.pgid != 0 { - closeProcessGroup(s.pgid, s.done) - } else { - _ = s.cmd.Process.Kill() - } + killProcessGroup(s.cmd.Process.Pid, s.done) } _ = s.pty.Close() _ = s.cmd.Wait() @@ -344,7 +340,6 @@ func (m *Manager) CreateWithCommand(sessionID, workDir string, cols, rows uint16 cmd := p.Command(shell, "-l", "-c", command) cmd.Dir = workDir cmd.Env = env - setProcessGroup(cmd) if err := cmd.Start(); err != nil { p.Close() @@ -357,7 +352,6 @@ func (m *Manager) CreateWithCommand(sessionID, workDir string, cols, rows uint16 id: sessionID, pty: p, cmd: cmd, - pgid: cmd.Process.Pid, scrollback: NewRingBuffer(scrollbackSize), done: make(chan struct{}), }