hooks: harden timeout kill and add platform split
- Move runHook into build-tagged implementations (unix/windows) to keep unix syscalls off Windows builds. - In unix implementation, guard nil Process, return wrapped kill errors except ESRCH, and document linkage to TestRunSync_KillsDescendants. - On Windows, best-effort kill on timeout retains prior behavior. - In tests, move testing.Short earlier and keep descendant-kill coverage on Linux.
This commit is contained in:
@@ -3,13 +3,8 @@
|
|||||||
package hooks
|
package hooks
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"bytes"
|
|
||||||
"context"
|
|
||||||
"encoding/json"
|
|
||||||
"os"
|
"os"
|
||||||
"os/exec"
|
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"syscall"
|
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
"github.com/steveyegge/beads/internal/types"
|
"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)
|
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
|
|
||||||
|
|
||||||
// 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
|
// HookExists checks if a hook exists for an event
|
||||||
func (r *Runner) HookExists(event string) bool {
|
func (r *Runner) HookExists(event string) bool {
|
||||||
hookName := eventToHook(event)
|
hookName := eventToHook(event)
|
||||||
|
|||||||
@@ -260,6 +260,10 @@ func TestRunSync_KillsDescendants(t *testing.T) {
|
|||||||
t.Skip("TestRunSync_KillsDescendants requires Linux /proc")
|
t.Skip("TestRunSync_KillsDescendants requires Linux /proc")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if testing.Short() {
|
||||||
|
t.Skip("Skipping long-running descendant kill test in short mode")
|
||||||
|
}
|
||||||
|
|
||||||
tmpDir := t.TempDir()
|
tmpDir := t.TempDir()
|
||||||
hookPath := filepath.Join(tmpDir, HookOnCreate)
|
hookPath := filepath.Join(tmpDir, HookOnCreate)
|
||||||
pidFile := filepath.Join(tmpDir, "child.pid")
|
pidFile := filepath.Join(tmpDir, "child.pid")
|
||||||
@@ -279,10 +283,6 @@ func TestRunSync_KillsDescendants(t *testing.T) {
|
|||||||
}
|
}
|
||||||
issue := &types.Issue{ID: "bd-test", Title: "Test"}
|
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)
|
err := runner.RunSync(EventCreate, issue)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Fatal("Expected RunSync to return an error on timeout")
|
t.Fatal("Expected RunSync to return an error on timeout")
|
||||||
|
|||||||
74
internal/hooks/hooks_unix.go
Normal file
74
internal/hooks/hooks_unix.go
Normal file
@@ -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
|
||||||
|
}
|
||||||
|
}
|
||||||
53
internal/hooks/hooks_windows.go
Normal file
53
internal/hooks/hooks_windows.go
Normal file
@@ -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
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user