Correctly handle sending signal to child process

changelog/fragments/1744059162-send-correct-signal-on-windows.yaml

@@ -0,0 +1,32 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: bug-fix
+
+# Change summary; a 80ish characters long description of the change.
+summary: Correctly handle sending signal to child process
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+#description:
+
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+component: elastic-agent
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+pr: https://github.com/elastic/elastic-agent/pull/7738
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+issue: https://github.com/elastic/elastic-agent/issues/6875

magefile.go

@@ -430,6 +430,7 @@ func getTestBinariesPath() ([]string, error) {
 testBinaryPkgs := []string{
 filepath.Join(wd, "pkg", "component", "fake", "component"),
 filepath.Join(wd, "internal", "pkg", "agent", "install", "testblocking"),
+filepath.Join(wd, "pkg", "core", "process", "testsignal"),
 }
 return testBinaryPkgs, nil
 }

pkg/core/process/cmd.go

@@ -11,6 +11,9 @@ import (
 "os"
 "os/exec"
 "path/filepath"
+"syscall"
+
+"golang.org/x/sys/windows"
 )
 
 func getCmd(ctx context.Context, path string, env []string, uid, gid int, arg ...string) (*exec.Cmd, error) {
@@ -23,14 +26,33 @@ func getCmd(ctx context.Context, path string, env []string, uid, gid int, arg ..
 cmd.Env = append(cmd.Env, os.Environ()...)
 cmd.Env = append(cmd.Env, env...)
 cmd.Dir = filepath.Dir(path)
+cmd.SysProcAttr = &syscall.SysProcAttr{
+// Signals are sent to process groups, and child process are part of the
+// parent's prcoess group. So to send a signal to a
+// child process and not have it also affect ourselves
+// (the parent process), the child needs to be created in a new
+// process group.
+//
+// Creating a child with CREATE_NEW_PROCESS_GROUP disables CTLR_C_EVENT
+// handling for the child, so the only way to gracefully stop it is with
+// a CTRL_BREAK_EVENT signal.
+// https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
+CreationFlags: windows.CREATE_NEW_PROCESS_GROUP,
+}
 
 return cmd, nil
 }
 
+// killCmd calls Process.Kill
 func killCmd(proc *os.Process) error {
 return proc.Kill()
 }
 
+// terminateCmd sends the CTRL+BREAK (SIGINT) to the process
 func terminateCmd(proc *os.Process) error {
-return proc.Kill()
+// Because we set CREATE_NEW_PROCESS_GROUP when creating the process,
+// it CTLR_C_EVENT is disabled, so the only way to gracefully terminate
+// the child process is to send a CTRL_BREAK_EVENT.
+// https://learn.microsoft.com/en-us/windows/console/generateconsolectrlevent
+return windows.GenerateConsoleCtrlEvent(windows.CTRL_BREAK_EVENT, uint32(proc.Pid))
 }

pkg/core/process/cmd_darwin.go

@@ -29,8 +29,8 @@
 if isInt32(uid) && isInt32(gid) {
 cmd.SysProcAttr = &syscall.SysProcAttr{
 Credential: &syscall.Credential{
 Uid: uint32(uid),
 Gid: uint32(gid),
 NoSetGroups: true,
 },
 }
@@ -45,10 +45,12 @@
 return val >= 0 && val <= math.MaxInt32
 }
 
+// killCmd calls Process.Kill
 func killCmd(proc *os.Process) error {
 return proc.Kill()
 }
 
+// terminateCmd sends SIGTERM to the process
 func terminateCmd(proc *os.Process) error {
 return proc.Signal(syscall.SIGTERM)
 }

pkg/core/process/cmd_linux.go

@@ -48,10 +48,12 @@ func isInt32(val int) bool {
 return val >= 0 && val <= math.MaxInt32
 }
 
+// killCmd calls Process.Kill
 func killCmd(proc *os.Process) error {
 return proc.Kill()
 }
 
+// terminateCmd sends SIGTERM to the process
 func terminateCmd(proc *os.Process) error {
 return proc.Signal(syscall.SIGTERM)
 }

pkg/core/process/cmd_test.go

@@ -0,0 +1,116 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License 2.0;
+// you may not use this file except in compliance with the Elastic License 2.0.
+
+package process
+
+import (
+"bufio"
+"context"
+"io"
+"os"
+"path/filepath"
+"runtime"
+"strings"
+"syscall"
+"testing"
+"time"
+)
+
+// TestGetCmdAndTerminateCmd ensures the process started by this package
+// can be gracefully terminated.
+//
+// This requires two things:
+// 1. getCmd sets the correct SysProcAttr according to the OS
+// 2. terminateCmd sends the correct signal according to the OS
+//
+// On Linux and MacOS, no attribute needs to be set on SysProcAttr
+// and terminateCmd send SIGTERM.
+// On Windows the process needs the CREATE_NEW_PROCESS_GROUP flag
+// and terminateCmd needs to send the CTRL+BREAK event, which the
+// Go runtime translates into a syscall.SIGINT.
+//
+// If this test is failing, run go test with -v so all the output
+// of the child process is sent to stdout
+func TestGetCmdAndTerminateCmd(t *testing.T) {
+wd, err := os.Getwd()
+if err != nil {
+t.Fatalf("cannot get working directory: %s", err)
+}
+testBinary := filepath.Join(wd, "testsignal", "testsignal")
+
+if runtime.GOOS == "windows" {
+testBinary += ".exe"
+}
+
+ctx, cancel := context.WithTimeout(t.Context(), 15*time.Second)
+t.Cleanup(cancel)
+
+cmd, err := getCmd(ctx, testBinary, nil, os.Getuid(), os.Getgid(), t.Name())
+if err != nil {
+t.Fatalf("'getCmd' failed: %s", err)
+}
+
+out, err := cmd.StderrPipe()
+if err != nil {
+t.Fatalf("cannot get stderr pipe for child process: %s", err)
+}
+sc := bufio.NewScanner(out)
+
+if err := cmd.Start(); err != nil {
+t.Fatalf("cannot start child process: %s", err)
+}
+
+for sc.Scan() {
+line := sc.Text()
+if testing.Verbose() {
+t.Log("Child process output:", line)
+}
+if strings.Contains(line, "Wait for signal") {
+break
+}
+}
+
+if err := terminateCmd(cmd.Process); err != nil {
+t.Fatalf("did not expect an error from 'terminateCmd': %s", err)
+}
+
+expectedMsg := "Got signal: "
+if runtime.GOOS == "windows" {
+expectedMsg += syscall.SIGINT.String()
+} else {
+expectedMsg += syscall.SIGTERM.String()
+}
+
+for sc.Scan() {
+line := sc.Text()
+if testing.Verbose() {
+t.Log("Child process output:", line)
+}
+if strings.Contains(line, expectedMsg) {
+break
+}
+}
+
+// Drain the stdout and wait for the child process to exit.
+// Because we called cmd.StderrPipe() we need to drain the
+// pipe before calling cmd.Wait.
+if testing.Verbose() {
+for sc.Scan() {
+line := sc.Text()
+t.Log("Process Output:", line)
+}
+return
+} else {
+_, _ = io.Copy(io.Discard, out)
+}
+
+// Wait to ensure the child process exits successfully
+if err := cmd.Wait(); err != nil {
+t.Fatalf("cmd did not finish successfully: %s", err)
+}
+
+if cmd.ProcessState.ExitCode() != 0 {
+t.Fatalf("process did not finish successfully, exit code: %d", cmd.ProcessState.ExitCode())
+}
+}

pkg/core/process/testsignal/main.go

@@ -0,0 +1,32 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License 2.0;
+// you may not use this file except in compliance with the Elastic License 2.0.
+
+package main
+
+import (
+"log"
+"os"
+"os/signal"
+"syscall"
+)
+
+var name string
+
+func main() {
+if len(os.Args) == 1 {
+log.Println("Usage: ./testsignal [name]")
+os.Exit(1)
+}
+
+name = os.Args[1]
+log.Printf("[%s] Starting, PID %d", name, os.Getpid())
+defer log.Printf("[%s] Done", name)
+
+signals := make(chan os.Signal, 1)
+signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM, syscall.SIGQUIT, syscall.SIGHUP)
+
+log.Printf("[%s] Wait for signal", name)
+s := <-signals
+log.Printf("[%s] Got signal: %s", name, s)
+}

pkg/core/process/testsignal/proc.go

@@ -0,0 +1,20 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License 2.0;
+// you may not use this file except in compliance with the Elastic License 2.0.
+
+//go:build !windows
+
+package main
+
+import (
+"os/exec"
+"syscall"
+)
+
+func stopCmd(cmd *exec.Cmd) error {
+return cmd.Process.Signal(syscall.SIGINT)
+}
+
+func getSysProcAttr() *syscall.SysProcAttr {
+return &syscall.SysProcAttr{}
+}

pkg/core/process/testsignal/proc_win.go

@@ -0,0 +1,26 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License 2.0;
+// you may not use this file except in compliance with the Elastic License 2.0.
+
+//go:build windows
+
+package main
+
+import (
+"os/exec"
+"syscall"
+
+"golang.org/x/sys/windows"
+)
+
+func stopCmd(cmd *exec.Cmd) error {
+return windows.GenerateConsoleCtrlEvent(windows.CTRL_BREAK_EVENT, uint32(cmd.Process.Pid))
+}
+
+func getSysProcAttr() *syscall.SysProcAttr {
+return &syscall.SysProcAttr{
+// This disables the child from receiveing CTRL_C events
+// But isolates other siganls from us, aka the parent
+CreationFlags: windows.CREATE_NEW_PROCESS_GROUP,
+}
+}