diff --git a/internal/hooks/hooks.go b/internal/hooks/hooks.go index ba72316b..d4f94328 100644 --- a/internal/hooks/hooks.go +++ b/internal/hooks/hooks.go @@ -3,13 +3,8 @@ package hooks import ( - "bytes" - "context" - "encoding/json" "os" - "os/exec" "path/filepath" - "syscall" "time" "github.com/steveyegge/beads/internal/types" @@ -99,62 +94,6 @@ func (r *Runner) RunSync(event string, issue *types.Issue) error { return r.runHook(hookPath, event, issue) } -func (r *Runner) runHook(hookPath, event string, issue *types.Issue) error { - ctx, cancel := context.WithTimeout(context.Background(), r.timeout) - defer cancel() - - // Prepare JSON data for stdin - issueJSON, err := json.Marshal(issue) - if err != nil { - return err - } - - // Create command: hook_script - // #nosec G204 -- hookPath is from controlled .beads/hooks directory - cmd := exec.CommandContext(ctx, hookPath, issue.ID, event) - cmd.Stdin = bytes.NewReader(issueJSON) - - // Capture output for debugging (but don't block on it) - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - // Start the hook so we can manage its process group and kill children on timeout. - // - // Rationale: scripts may spawn child processes (backgrounded or otherwise). - // If we only kill the immediate process, descendants may survive and keep - // the test (or caller) blocked — see TestRunSync_Timeout which previously - // observed a `sleep 60` still running after the parent process was killed. - // Creating a process group (Setpgid) and sending a negative PID to - // `syscall.Kill` ensures the entire group (parent + children) are killed - // reliably on timeout. - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - - if err := cmd.Start(); err != nil { - return err - } - - done := make(chan error, 1) - go func() { - done <- cmd.Wait() - }() - - select { - case <-ctx.Done(): - // Kill the whole process group to ensure any children (e.g., sleep) - // are also terminated. - _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - // Wait for process to exit - <-done - return ctx.Err() - case err := <-done: - if err != nil { - return err - } - return nil - } -} - // HookExists checks if a hook exists for an event func (r *Runner) HookExists(event string) bool { hookName := eventToHook(event) diff --git a/internal/hooks/hooks_test.go b/internal/hooks/hooks_test.go index 09a484a1..aed1439f 100644 --- a/internal/hooks/hooks_test.go +++ b/internal/hooks/hooks_test.go @@ -260,6 +260,10 @@ func TestRunSync_KillsDescendants(t *testing.T) { t.Skip("TestRunSync_KillsDescendants requires Linux /proc") } + if testing.Short() { + t.Skip("Skipping long-running descendant kill test in short mode") + } + tmpDir := t.TempDir() hookPath := filepath.Join(tmpDir, HookOnCreate) pidFile := filepath.Join(tmpDir, "child.pid") @@ -279,10 +283,6 @@ func TestRunSync_KillsDescendants(t *testing.T) { } issue := &types.Issue{ID: "bd-test", Title: "Test"} - if testing.Short() { - t.Skip("Skipping long-running descendant kill test in short mode") - } - err := runner.RunSync(EventCreate, issue) if err == nil { t.Fatal("Expected RunSync to return an error on timeout") diff --git a/internal/hooks/hooks_unix.go b/internal/hooks/hooks_unix.go new file mode 100644 index 00000000..8c424ea2 --- /dev/null +++ b/internal/hooks/hooks_unix.go @@ -0,0 +1,74 @@ +//go:build unix + +package hooks + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "os/exec" + "syscall" + + "github.com/steveyegge/beads/internal/types" +) + +// runHook executes the hook and enforces a timeout, killing the process group +// on expiration to ensure descendant processes are terminated. +func (r *Runner) runHook(hookPath, event string, issue *types.Issue) error { + ctx, cancel := context.WithTimeout(context.Background(), r.timeout) + defer cancel() + + // Prepare JSON data for stdin + issueJSON, err := json.Marshal(issue) + if err != nil { + return err + } + + // Create command: hook_script + // #nosec G204 -- hookPath is from controlled .beads/hooks directory + cmd := exec.CommandContext(ctx, hookPath, issue.ID, event) + cmd.Stdin = bytes.NewReader(issueJSON) + + // Capture output for debugging (but don't block on it) + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + // Start the hook so we can manage its process group and kill children on timeout. + // + // Rationale: scripts may spawn child processes (backgrounded or otherwise). + // If we only kill the immediate process, descendants may survive and keep + // the test (or caller) blocked — this was exposed by TestRunSync_Timeout and + // validated by TestRunSync_KillsDescendants. Creating a process group (Setpgid) + // and sending a negative PID to syscall.Kill ensures the entire group + // (parent + children) are killed reliably on timeout. + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + + if err := cmd.Start(); err != nil { + return err + } + + done := make(chan error, 1) + go func() { + done <- cmd.Wait() + }() + + select { + case <-ctx.Done(): + if cmd.Process != nil { + if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil && !errors.Is(err, syscall.ESRCH) { + return fmt.Errorf("kill process group: %w", err) + } + } + // Wait for process to exit after the kill attempt + <-done + return ctx.Err() + case err := <-done: + if err != nil { + return err + } + return nil + } +} diff --git a/internal/hooks/hooks_windows.go b/internal/hooks/hooks_windows.go new file mode 100644 index 00000000..7bfd8a76 --- /dev/null +++ b/internal/hooks/hooks_windows.go @@ -0,0 +1,53 @@ +//go:build windows + +package hooks + +import ( + "bytes" + "context" + "encoding/json" + "os/exec" + + "github.com/steveyegge/beads/internal/types" +) + +// runHook executes the hook and enforces a timeout on Windows. +// Windows lacks Unix-style process groups; on timeout we best-effort kill +// the started process. Descendant processes may survive if they detach, +// but this preserves previous behavior while keeping tests green on Windows. +func (r *Runner) runHook(hookPath, event string, issue *types.Issue) error { + ctx, cancel := context.WithTimeout(context.Background(), r.timeout) + defer cancel() + + issueJSON, err := json.Marshal(issue) + if err != nil { + return err + } + + cmd := exec.CommandContext(ctx, hookPath, issue.ID, event) + cmd.Stdin = bytes.NewReader(issueJSON) + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Start(); err != nil { + return err + } + + done := make(chan error, 1) + go func() { + done <- cmd.Wait() + }() + + select { + case <-ctx.Done(): + if cmd.Process != nil { + _ = cmd.Process.Kill() + } + <-done + return ctx.Err() + case err := <-done: + return err + } +}