Merge pull request #606 from SorraTheOrc/fix/hooks-timeout-kill-descendants

hooks: kill descendant processes on timeout; add test and comment

internal/hooks/hooks.go

@@ -3,11 +3,7 @@
 package hooks
 
 import (
-	"bytes"
-	"context"
-	"encoding/json"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"time"
 
@@ -98,37 +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 <issue_id> <event_type>
-	// #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
-
-	// Run the hook
-	err = cmd.Run()
-	if err != nil {
-		// Log error but don't fail - hooks shouldn't break beads
-		// In production, this could go to a log file
-		return err
-	}
-
-	return nil
-}
-
 // HookExists checks if a hook exists for an event
 func (r *Runner) HookExists(event string) bool {
 	hookName := eventToHook(event)

internal/hooks/hooks_test.go

@@ -3,6 +3,9 @@ package hooks
 import (
 	"os"
 	"path/filepath"
+	"runtime"
+	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -252,6 +255,56 @@ sleep 60`
 	}
 }
 
+func TestRunSync_KillsDescendants(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		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")
+
+	// Hook starts a background sleep, writes its pid, and waits for it.
+	// Parent will remain alive until the child exits, so killing the
+	// process group should terminate both.
+	hookScript := `#!/bin/sh
+(sleep 60 & echo $! > ` + pidFile + ` ; wait)`
+	if err := os.WriteFile(hookPath, []byte(hookScript), 0755); err != nil {
+		t.Fatalf("Failed to create hook file: %v", err)
+	}
+
+	runner := &Runner{
+		hooksDir: tmpDir,
+		timeout:  500 * time.Millisecond,
+	}
+	issue := &types.Issue{ID: "bd-test", Title: "Test"}
+
+	err := runner.RunSync(EventCreate, issue)
+	if err == nil {
+		t.Fatal("Expected RunSync to return an error on timeout")
+	}
+
+	// Read the child PID and ensure it's not running anymore.
+	data, err := os.ReadFile(pidFile)
+	if err != nil {
+		t.Fatalf("Failed to read pid file: %v", err)
+	}
+	pidStr := strings.TrimSpace(string(data))
+	pid, err := strconv.Atoi(pidStr)
+	if err != nil {
+		t.Fatalf("Invalid pid in pid file: %v", err)
+	}
+
+	// Check /proc/<pid> does not exist
+	if _, err := os.Stat(filepath.Join("/proc", strconv.Itoa(pid))); err == nil {
+		t.Fatalf("Child process %d still exists after timeout", pid)
+	}
+}
+
 func TestRunSync_HookFailure(t *testing.T) {
 	tmpDir := t.TempDir()
 	hookPath := filepath.Join(tmpDir, HookOnUpdate)

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 <issue_id> <event_type>
+	// #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
+	}
+}

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
+	}
+}