Fix race condition between zombie reaper and RPC command execution

The sidecar container had a race condition between the reapZombies
goroutine and the RPC command handler (CmdExec.Run). Both called
Wait4 on the same child processes:

- reapZombies called Wait4(-1, WNOHANG) to reap any exited child.
- CmdExec.Run called cmd.Run(), which internally calls cmd.Wait(),
  which calls waitid(P_PID, <pid>) on the specific child process.

Since the kernel only stores one exit status per process and the first
Wait4/waitid call to match a PID consumes it, the reaper could steal
the exit status before cmd.Wait() collected it. This caused cmd.Wait()
to fail with "waitid: no child processes" (ECHILD), which surfaced in
kapp-controller logs as:

  Fetching resources: waitid: no child processes

The fix introduces a centralized Reaper that is the sole caller of
Wait4 in the sidecar process. CmdExec.Run now uses cmd.Start()
instead of cmd.Run() and registers the child PID with the Reaper via
a channel. The Reaper's Wait4(-1) loop dispatches exit statuses to
tracked PIDs through their channels, and silently discards statuses
for untracked PIDs (orphaned zombies). A mutex ensures cmd.Start()
and PID registration are atomic with respect to the reaping loop,
preventing the Reaper from consuming an exit status before the
channel is registered.

Made-with: Cursor

ai-assisted=yes

Made-with: Cursor
Signed-off-by: Miroslav Shipkovenski <miroslav.shipkovenski@broadcom.com>

Author: Miroslav Shipkovenski

cmd/controller/sidecarexec.go

@@ -4,22 +4,17 @@
 package main
 
 import (
-	"syscall"
-	"time"
-
-	"carvel.dev/kapp-controller/pkg/exec"
 	"carvel.dev/kapp-controller/pkg/sidecarexec"
-	"github.com/go-logr/logr"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 )
 
 func sidecarexecMain() {
 	mainLog := zap.New(zap.UseDevMode(false)).WithName("kc-sidecarexec")
 	mainLog.Info("start sidecarexec", "version", Version)
 
-	go reapZombies(mainLog)
+	reaper := sidecarexec.NewReaper(mainLog)
+	go reaper.Run()
 
-	localCmdRunner := exec.NewPlainCmdRunner()
 	opts := sidecarexec.ServerOpts{
 		AllowedCmdNames: []string{
 			// Fetch (calls impgkg and others internally)
@@ -29,25 +24,10 @@ func sidecarexecMain() {
 		},
 	}
 
-	server := sidecarexec.NewServer(localCmdRunner, opts, mainLog)
+	server := sidecarexec.NewServer(reaper, opts, mainLog)
 
 	err := server.Serve()
 	if err != nil {
 		mainLog.Error(err, "Serving RPC")
 	}
 }
-
-func reapZombies(log logr.Logger) {
-	log.Info("starting zombie reaper")
-
-	for {
-		var status syscall.WaitStatus
-
-		pid, _ := syscall.Wait4(-1, &status, syscall.WNOHANG, nil)
-		if pid <= 0 {
-			time.Sleep(1 * time.Second)
-		} else {
-			continue
-		}
-	}
-}

pkg/sidecarexec/cmd_exec.go

@@ -6,10 +6,10 @@ package sidecarexec
 import (
 	"bytes"
 	"fmt"
+	"io"
 	"os"
 	goexec "os/exec"
-
-	"carvel.dev/kapp-controller/pkg/exec"
+	"sync"
 )
 
 // CmdInput describes a command to run.
@@ -31,11 +31,13 @@ type CmdOutput struct {
 
 // CmdExec provides RPC interface for command execution.
 type CmdExec struct {
-	local           exec.CmdRunner
+	reaper          *Reaper
 	allowedCmdNames map[string]struct{}
 }
 
 // Run executes a command (out of a set of allowed ones).
+// It uses cmd.Start() and delegates process wait to the centralized Reaper
+// to avoid a race with the zombie reaping loop.
 func (r CmdExec) Run(input CmdInput, output *CmdOutput) error {
 	if _, found := r.allowedCmdNames[input.Command]; !found {
 		return fmt.Errorf("Command '%s' is not allowed", input.Command)
@@ -54,20 +56,57 @@ func (r CmdExec) Run(input CmdInput, output *CmdOutput) error {
 		cmd.Dir = input.Dir
 	}
 
-	var stdout, stderr bytes.Buffer
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
-
-	err := r.local.Run(cmd)
+	stdoutPipe, err := cmd.StdoutPipe()
 	if err != nil {
-		output.Error = err.Error()
-		output.ExitCode = -1
-		if exitError, ok := err.(*goexec.ExitError); ok {
-			output.ExitCode = exitError.ExitCode()
-		}
+		return fmt.Errorf("Creating stdout pipe: %s", err)
 	}
+	stderrPipe, err := cmd.StderrPipe()
+	if err != nil {
+		return fmt.Errorf("Creating stderr pipe: %s", err)
+	}
+
+	// Hold the reaper lock across Start + Track to guarantee the reaping
+	// loop cannot consume this child's exit status before we register it.
+	r.reaper.Lock()
+	if err := cmd.Start(); err != nil {
+		r.reaper.Unlock()
+		return fmt.Errorf("Starting command: %s", err)
+	}
+	statusCh := r.reaper.TrackLocked(cmd.Process.Pid)
+	r.reaper.Unlock()
+
+	var stdout, stderr bytes.Buffer
+	var wg sync.WaitGroup
+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		io.Copy(&stdout, stdoutPipe)
+	}()
+	go func() {
+		defer wg.Done()
+		io.Copy(&stderr, stderrPipe)
+	}()
+
+	// Wait for all output to be read before checking exit status.
+	// The pipes close when the process exits, so this will unblock.
+	wg.Wait()
+
+	// Now wait for the Reaper to deliver the exit status.
+	waitStatus := <-statusCh
+
+	// Tell Go's os/exec we handled the wait ourselves.
+	cmd.Process.Release()
 
 	output.Stdout = stdout.Bytes()
 	output.Stderr = stderr.Bytes()
+
+	if waitStatus.Signaled() {
+		output.ExitCode = -1
+		output.Error = fmt.Sprintf("process killed by signal %d", waitStatus.Signal())
+	} else if waitStatus.Exited() && waitStatus.ExitStatus() != 0 {
+		output.ExitCode = waitStatus.ExitStatus()
+		output.Error = fmt.Sprintf("exit status %d", waitStatus.ExitStatus())
+	}
+
 	return nil
 }

pkg/sidecarexec/export_test.go

@@ -0,0 +1,21 @@
+// Copyright 2024 The Carvel Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package sidecarexec
+
+import "syscall"
+
+// CmdExec_ForTesting creates a CmdExec with the given reaper and allowed
+// command names. This is only available in test builds.
+func CmdExec_ForTesting(reaper *Reaper, allowedCmds []string) CmdExec {
+	allowed := map[string]struct{}{}
+	for _, cmd := range allowedCmds {
+		allowed[cmd] = struct{}{}
+	}
+	return CmdExec{reaper: reaper, allowedCmdNames: allowed}
+}
+
+// Dispatch_ForTesting exposes the reaper's dispatch method for unit tests.
+func Dispatch_ForTesting(r *Reaper, pid int, status syscall.WaitStatus) bool {
+	return r.dispatch(pid, status)
+}

pkg/sidecarexec/reaper.go

@@ -0,0 +1,105 @@
+// Copyright 2024 The Carvel Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package sidecarexec
+
+import (
+	"sync"
+	"syscall"
+	"time"
+
+	"github.com/go-logr/logr"
+)
+
+// Reaper is a centralized zombie process reaper that eliminates the race
+// condition between a blanket Wait4(-1) and Go's os/exec cmd.Wait().
+// It is the sole goroutine calling Wait4, and dispatches exit statuses
+// back to callers that registered interest in specific PIDs.
+type Reaper struct {
+	mu      sync.Mutex
+	tracked map[int]chan syscall.WaitStatus
+	log     logr.Logger
+}
+
+// NewReaper creates a new Reaper. Start the reaping loop by calling Run
+// in a separate goroutine.
+func NewReaper(log logr.Logger) *Reaper {
+	return &Reaper{
+		tracked: make(map[int]chan syscall.WaitStatus),
+		log:     log.WithName("reaper"),
+	}
+}
+
+// Lock acquires the reaper's mutex. This must be held while starting a
+// process and calling TrackLocked, so that the reaping loop cannot
+// consume the process exit status before tracking is registered.
+func (r *Reaper) Lock() { r.mu.Lock() }
+
+// Unlock releases the reaper's mutex.
+func (r *Reaper) Unlock() { r.mu.Unlock() }
+
+// TrackLocked registers a PID so that when Wait4 reaps it, the wait status
+// is delivered to the returned channel. The caller MUST hold r.Lock() and
+// must have started the process while holding the lock, ensuring no race
+// between process start and tracking registration.
+func (r *Reaper) TrackLocked(pid int) <-chan syscall.WaitStatus {
+	ch := make(chan syscall.WaitStatus, 1)
+	r.tracked[pid] = ch
+	return ch
+}
+
+// dispatch delivers a wait status to a tracked PID's channel.
+// Returns true if the PID was being tracked (an RPC-managed process).
+func (r *Reaper) dispatch(pid int, status syscall.WaitStatus) bool {
+	r.mu.Lock()
+	ch, ok := r.tracked[pid]
+	if ok {
+		delete(r.tracked, pid)
+	}
+	r.mu.Unlock()
+
+	if ok {
+		ch <- status
+	}
+	return ok
+}
+
+// Run is the reaping loop and must be the only goroutine calling Wait4
+// in this process. It runs forever.
+func (r *Reaper) Run() {
+	r.log.Info("starting zombie reaper")
+
+	for {
+		var status syscall.WaitStatus
+
+		// The mutex is acquired here so that Wait4 cannot race with
+		// a concurrent cmd.Start() + TrackLocked() sequence.
+		r.mu.Lock()
+		pid, err := syscall.Wait4(-1, &status, syscall.WNOHANG, nil)
+
+		if pid > 0 {
+			ch, ok := r.tracked[pid]
+			if ok {
+				delete(r.tracked, pid)
+			}
+			r.mu.Unlock()
+
+			if ok {
+				ch <- status
+				r.log.V(1).Info("reaped tracked process", "pid", pid)
+			} else {
+				r.log.V(1).Info("reaped orphaned zombie", "pid", pid)
+			}
+			continue
+		}
+		r.mu.Unlock()
+
+		if pid == 0 || (pid == -1 && err == syscall.ECHILD) {
+			time.Sleep(1 * time.Second)
+			continue
+		}
+
+		r.log.V(1).Info("wait4 unexpected error", "err", err)
+		time.Sleep(1 * time.Second)
+	}
+}