fix: Wait directly in fakeroot MonitorContainer

When the user's container exits, fakeroot cleanup runs `rm` though the
fakeroot engine to remove a temporary container directory, extracted
from a SIF image, where necessary.

Under high load / parallelism, on some distros / kernels, execution of
fakeroot tempdir cleanup can become stuck. See #1109 for discussion of
a reproducer.

The fakeroot engine MonitorContainer code sits in a loop waiting for
signals. It requires a SIGCHLD to be received before it will wait4 on
the child process, to confirm it has exited.

Debug logging showed that sometimes a SIGCHLD was not received by
this code. It is not clear why, as `signal.Notify` is set early, and
the signals channel buffer is not being filled. The behavior varies
between distributions / kernels. It is sometimes easy to trigger, and
sometimes impossible.

As a workaround, re-structure the MonitorContainer function so that it
performs a blocking `wait4` in a go routine, separate from signal
handling. This ensures that receiving a `SIGCHLD` is not necessary to
identify that the child process has exited.

This fix has been verified in the CircleCI ssh environment. A reproducer
script that previously will freeze within a few iterations does not
freeze at all with this change.

Fixes #1109

Author: dtrudg

CHANGELOG.md

@@ -6,6 +6,8 @@
 
 - Ensure `make dist` doesn't include conmon binary or intermediate files.
 - Do not hang on pull from http(s) source that doesn't provide a content-length.
+- Avoid hang on fakeroot cleanup under high load seen on some
+  distributions / kernels.
 
 ## 3.10.3 \[2022-10-06\]
 

internal/pkg/runtime/engine/fakeroot/engine_linux.go

@@ -257,26 +257,42 @@ func (e *EngineOperations) StartProcess(masterConn net.Conn) error {
 // not need them for wait4 and kill syscalls.
 func (e *EngineOperations) MonitorContainer(pid int, signals chan os.Signal) (syscall.WaitStatus, error) {
 	var status syscall.WaitStatus
+	waitStatus := make(chan syscall.WaitStatus, 1)
+	waitError := make(chan error, 1)
+
+	go func() {
+		sylog.Debugf("Waiting for container process %d", pid)
+		_, err := syscall.Wait4(pid, &status, 0, nil)
+		sylog.Debugf("Wait for process %d complete with status %v, error %v", pid, status, err)
+		waitStatus <- status
+		waitError <- err
+	}()
 
 	for {
-		s := <-signals
-		switch s {
-		case syscall.SIGCHLD:
-			if wpid, err := syscall.Wait4(pid, &status, syscall.WNOHANG, nil); err != nil {
-				return status, fmt.Errorf("error while waiting child: %s", err)
-			} else if wpid != pid {
-				continue
+		select {
+		case s := <-signals:
+			// Signal received
+			switch s {
+			case syscall.SIGCHLD:
+				// Our go routine waiting for the container pid will handle container exit.
+				break
+			case syscall.SIGURG:
+				// Ignore SIGURG, which is used for non-cooperative goroutine
+				// preemption starting with Go 1.14. For more information, see
+				// https://github.com/golang/go/issues/24543.
+				break
+			default:
+				if err := syscall.Kill(pid, s.(syscall.Signal)); err != nil {
+					return status, fmt.Errorf("interrupted by signal %s", s.String())
+				}
 			}
-			return status, nil
-		case syscall.SIGURG:
-			// Ignore SIGURG, which is used for non-cooperative goroutine
-			// preemption starting with Go 1.14. For more information, see
-			// https://github.com/golang/go/issues/24543.
-			break
-		default:
-			if err := syscall.Kill(pid, s.(syscall.Signal)); err != nil {
-				return status, fmt.Errorf("interrupted by signal %s", s.String())
+		case ws := <-waitStatus:
+			// Container process exited
+			we := <-waitError
+			if we != nil {
+				return ws, fmt.Errorf("error while waiting for child: %w", we)
 			}
+			return ws, we
 		}
 	}
 }