Correctly handle sending signal to child process (#7738)

Calling proc.Kill causes the process to exit immediately, which
prevents components from executing some clean up functions.

To gracefully terminate the components, on Linux and MacOS SIGTERM
is sent.

On Windows signals are sent to process groups, child process are created
in the same group as the parent, so sending a signal to a child process
causes the Elastic-Agent to receive the signal as well. Therefore child
process need to be created in their own process group.

To do so, the child process needs to be created setting the creation flag
CREATE_NEW_PROCESS_GROUP, which has the side effect of disbling
the CTRL+C signal.

Then to correctly terminate the child process `windows.GenerateConsoleCtrlEvent`
is called to send CTRL+BREAK signal that is translated by go Go runtime into a
`syscall.SIGINT`, which is already correctly handled by Beats.

Author: belimawr

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

@@ -432,6 +432,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

@@ -45,10 +45,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_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,
+	}
+}