shim_load: Consider shim leaked only if we can't find pids

Right now the statement is treating any error from the shim as leaking
(len(pInfo == 0 is true for any error). It seems the intent was to only
treat shims this way if the error was not found, or if there's
legitimately no pids associated with the shim. Let's fix that up to
avoid orphaning shims that just had a small error in reading pids.

Link: https://github.com/containerd/containerd/issues/13784
Signed-off-by: Andrew Halaney <ahalaney@netflix.com>

core/runtime/v2/shim_load.go

@@ -28,6 +28,7 @@ import (
 	"github.com/containerd/log"
 
 	"github.com/containerd/containerd/v2/core/mount"
+	runtimeapi "github.com/containerd/containerd/v2/core/runtime"
 	"github.com/containerd/containerd/v2/pkg/namespaces"
 	"github.com/containerd/containerd/v2/pkg/timeout"
 	"golang.org/x/sync/errgroup"
@@ -188,18 +189,31 @@ func (m *ShimManager) loadShim(ctx context.Context, bundle *Bundle) error {
 
 	_, sgetErr := m.sandboxStore.Get(ctx, id)
 	pInfo, pidErr := shim.Pids(ctx)
-	if sgetErr != nil && errors.Is(sgetErr, errdefs.ErrNotFound) && (len(pInfo) == 0 || errors.Is(pidErr, errdefs.ErrNotFound)) {
-		log.G(ctx).WithField("id", id).Info("cleaning leaked shim process")
-		// We are unable to get Pids from the shim and it's not a sandbox
-		// shim. We should clean it up her.
-		// No need to do anything for removeTask since we never added this shim.
+	if shouldCleanupShim(sgetErr, pidErr, pInfo) {
+		logEntry := log.G(ctx).WithField("id", id)
+		if pidErr != nil {
+			logEntry = logEntry.WithError(pidErr)
+		}
+		logEntry.Info("cleaning leaked shim process")
 		shim.delete(ctx, false, func(ctx context.Context, id string) {})
 	} else {
+		if pidErr != nil {
+			log.G(ctx).WithField("id", id).WithError(pidErr).Warn("failed to query shim pids, keeping shim registered")
+		}
 		m.shims.Add(ctx, shim.ShimInstance)
 	}
 	return nil
 }
 
+// shouldCleanupShim determines whether or not a shim is in such a state that
+// we should reap it. To be reapable we confirm that it is not a sandbox shim
+// and it has no pids running
+func shouldCleanupShim(sgetErr, pidErr error, pInfo []runtimeapi.ProcessInfo) bool {
+	return errors.Is(sgetErr, errdefs.ErrNotFound) &&
+		(errors.Is(pidErr, errdefs.ErrNotFound) ||
+			(pidErr == nil && len(pInfo) == 0))
+}
+
 func loadShimTask(ctx context.Context, bundle *Bundle, onClose func()) (_ *shimTask, retErr error) {
 	shim, err := loadShim(ctx, bundle, onClose)
 	if err != nil {

core/runtime/v2/shim_load_test.go

@@ -0,0 +1,88 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package v2
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/containerd/errdefs"
+	"github.com/stretchr/testify/require"
+
+	runtimeapi "github.com/containerd/containerd/v2/core/runtime"
+)
+
+func TestShouldCleanupShim(t *testing.T) {
+	otherErr := errors.New("some other error")
+
+	testCases := []struct {
+		Name     string
+		SgetErr  error
+		PidErr   error
+		PInfo    []runtimeapi.ProcessInfo
+		Expected bool
+	}{
+		{
+			Name:     "sandbox found",
+			SgetErr:  nil,
+			PidErr:   nil,
+			PInfo:    nil,
+			Expected: false,
+		},
+		{
+			Name:     "sandbox lookup fails with unrelated error",
+			SgetErr:  otherErr,
+			PidErr:   nil,
+			PInfo:    nil,
+			Expected: false,
+		},
+		{
+			Name:     "not a sandbox, no pids running",
+			SgetErr:  errdefs.ErrNotFound,
+			PidErr:   nil,
+			PInfo:    []runtimeapi.ProcessInfo{},
+			Expected: true,
+		},
+		{
+			Name:     "not a sandbox, pids still running",
+			SgetErr:  errdefs.ErrNotFound,
+			PidErr:   nil,
+			PInfo:    []runtimeapi.ProcessInfo{{Pid: 1234}},
+			Expected: false,
+		},
+		{
+			Name:     "not a sandbox, pids lookup returns not found",
+			SgetErr:  errdefs.ErrNotFound,
+			PidErr:   errdefs.ErrNotFound,
+			PInfo:    nil,
+			Expected: true,
+		},
+		{
+			Name:     "not a sandbox, pids lookup fails with other error",
+			SgetErr:  errdefs.ErrNotFound,
+			PidErr:   otherErr,
+			PInfo:    nil,
+			Expected: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			require.Equal(t, tc.Expected, shouldCleanupShim(tc.SgetErr, tc.PidErr, tc.PInfo))
+		})
+	}
+}