diff --git a/internal/graph/schema.resolvers.go b/internal/graph/schema.resolvers.go index 54b4886c..ed63e0db 100644 --- a/internal/graph/schema.resolvers.go +++ b/internal/graph/schema.resolvers.go @@ -243,20 +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) } - // Close any terminal session associated with this worktree - if r.TerminalMgr != nil { - r.TerminalMgr.Close(id) - } - return true, nil } diff --git a/internal/graph/schema.resolvers_test.go b/internal/graph/schema.resolvers_test.go index ed91bfb6..4605b221 100644 --- a/internal/graph/schema.resolvers_test.go +++ b/internal/graph/schema.resolvers_test.go @@ -12,6 +12,9 @@ import ( "time" "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" "github.com/hmans/beans/pkg/bean" @@ -3252,3 +3255,71 @@ func TestListFiles(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) + defer agentMgr.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, + AgentMgr: agentMgr, + } + + // Create a worktree + wt, err := wtMgr.Create("test-wt") + if err != nil { + t.Fatalf("Create worktree: %v", err) + } + + // 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") + agentMgr.GetSession(wt.ID).Status = agent.StatusRunning + + // 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() + if _, err := mr.RemoveWorktree(context.Background(), wt.ID); err != nil { + t.Fatalf("RemoveWorktree: %v", err) + } + + // 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 { + 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/terminal/process_unix.go b/internal/terminal/process_unix.go new file mode 100644 index 00000000..84af029f --- /dev/null +++ b/internal/terminal/process_unix.go @@ -0,0 +1,24 @@ +//go:build !windows + +package terminal + +import ( + "syscall" + "time" +) + +const processGroupGrace = 3 * time.Second + +// 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(-pid, 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..a878f9dd --- /dev/null +++ b/internal/terminal/process_windows.go @@ -0,0 +1,11 @@ +//go:build windows + +package terminal + +import "os" + +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 38b1efc0..9f8c3afa 100644 --- a/internal/terminal/terminal.go +++ b/internal/terminal/terminal.go @@ -177,12 +177,13 @@ func (s *Session) readLoop() { } } -// Close kills the process 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 { - _ = s.cmd.Process.Kill() + killProcessGroup(s.cmd.Process.Pid, s.done) } _ = s.pty.Close() _ = s.cmd.Wait() 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..15b78e6e 100644 --- a/internal/worktree/worktree_test.go +++ b/internal/worktree/worktree_test.go @@ -7,39 +7,9 @@ import ( "strings" "testing" "time" -) - -// 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 -} + "github.com/hmans/beans/internal/testutil" +) func TestParsePorcelain(t *testing.T) { tests := []struct { @@ -170,7 +140,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 @@ -225,7 +195,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("") @@ -235,7 +205,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") @@ -263,7 +233,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") @@ -289,7 +259,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 @@ -308,7 +278,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 @@ -322,7 +292,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{ @@ -383,7 +353,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)) @@ -399,7 +369,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. @@ -432,7 +402,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() @@ -465,7 +435,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 { @@ -540,7 +510,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 @@ -556,7 +526,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 { @@ -593,7 +563,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 { @@ -626,7 +596,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 @@ -670,7 +640,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") @@ -720,7 +690,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, "", "") @@ -737,7 +707,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 @@ -781,7 +751,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 @@ -829,7 +799,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)