fix(kill): do not mark non-stop signals as an explicit stop

cmd/nerdctl/container/container_run_restart_linux_test.go

@@ -382,3 +382,86 @@ func TestRunRestartStatusLabel(t *testing.T) {
 
 	testCase.Run(t)
 }
+
+func TestRunRestartAlwaysKillSignal(t *testing.T) {
+	testCase := nerdtest.Setup()
+	// This asserts the label nerdctl itself writes, so it does not depend on the
+	// containerd restart monitor actually acting on it. It is containerd-only
+	// because docker has no equivalent label.
+	testCase.Require = require.Not(nerdtest.Docker)
+
+	testCase.SubTests = []*test.Case{
+		{
+			Description: "a non-stop signal does not mark the container explicitly stopped",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("run", "-d", "--restart=always", "--name", data.Identifier(),
+					testutil.CommonImage, "sleep", "infinity")
+				helpers.Ensure("kill", "--signal", "HUP", data.Identifier())
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				return helpers.Command("inspect", data.Identifier())
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: expect.ExitCodeSuccess,
+					Output: expect.JSON([]dockercompat.Container{}, func(dc []dockercompat.Container, t tig.T) {
+						assert.Equal(t, 1, len(dc))
+						assert.Assert(t, dc[0].Config.Labels[restart.ExplicitlyStoppedLabel] != "true",
+							"a container killed with SIGHUP must stay eligible for restart")
+					}),
+				}
+			},
+		},
+		{
+			Description: "the container's own stop signal marks it explicitly stopped",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("run", "-d", "--restart=always", "--stop-signal", "SIGUSR1",
+					"--name", data.Identifier(), testutil.CommonImage, "sleep", "infinity")
+				helpers.Ensure("kill", "--signal", "USR1", data.Identifier())
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				return helpers.Command("inspect", data.Identifier())
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: expect.ExitCodeSuccess,
+					Output: expect.JSON([]dockercompat.Container{}, func(dc []dockercompat.Container, t tig.T) {
+						assert.Equal(t, 1, len(dc))
+						assert.Equal(t, dc[0].Config.Labels[restart.ExplicitlyStoppedLabel], "true")
+					}),
+				}
+			},
+		},
+		{
+			Description: "the default signal marks the container explicitly stopped",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("run", "-d", "--restart=always", "--name", data.Identifier(),
+					testutil.CommonImage, "sleep", "infinity")
+				helpers.Ensure("kill", data.Identifier())
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				return helpers.Command("inspect", data.Identifier())
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: expect.ExitCodeSuccess,
+					Output: expect.JSON([]dockercompat.Container{}, func(dc []dockercompat.Container, t tig.T) {
+						assert.Equal(t, 1, len(dc))
+						assert.Equal(t, dc[0].Config.Labels[restart.ExplicitlyStoppedLabel], "true")
+					}),
+				}
+			},
+		},
+	}
+
+	testCase.Run(t)
+}

pkg/cmd/container/kill.go

@@ -85,9 +85,22 @@ func killContainer(ctx context.Context, container containerd.Container, signal s
 			containerutil.UpdateErrorLabel(ctx, container, err)
 		}
 	}()
-	if err := containerutil.UpdateExplicitlyStoppedLabel(ctx, container, true); err != nil {
+	containerLabels, err := container.Labels(ctx)
+	if err != nil {
+		return err
+	}
+	stops, err := containerutil.IsStopSignal(signal, containerLabels)
+	if err != nil {
 		return err
 	}
+	// Recording a non-stopping signal as an explicit stop would permanently
+	// suppress the restart policy of a container that is still running.
+	if stops {
+		if err := containerutil.UpdateExplicitlyStoppedLabel(ctx, container, true); err != nil {
+			return err
+		}
+	}
+
 	task, err := container.Task(ctx, cio.Load)
 	if err != nil {
 		return err

pkg/containerutil/containerutil.go

@@ -482,6 +482,22 @@ func getSignal(signalValue string, containerLabels map[string]string) (syscall.S
 	return signal.ParseSignal("SIGTERM")
 }
 
+// IsStopSignal reports whether sig is expected to stop the container, as
+// opposed to being merely delivered to the running process (e.g. SIGHUP).
+// Signals other than SIGKILL must match the container's own stop signal.
+func IsStopSignal(sig syscall.Signal, containerLabels map[string]string) (bool, error) {
+	if sig == syscall.SIGKILL {
+		return true, nil
+	}
+
+	stopSignal, err := getSignal("", containerLabels)
+	if err != nil {
+		return false, err
+	}
+
+	return sig == stopSignal, nil
+}
+
 func waitContainerStop(ctx context.Context, task containerd.Task, exitCh <-chan containerd.ExitStatus, id string) error {
 	select {
 	case <-ctx.Done():

pkg/containerutil/containerutil_test.go

@@ -18,8 +18,11 @@ package containerutil
 
 import (
 	"reflect"
+	"syscall"
 	"testing"
 
+	containerd "github.com/containerd/containerd/v2/client"
+
 	"github.com/containerd/nerdctl/v2/pkg/labels"
 )
 
@@ -120,3 +123,65 @@ func TestGetContainerVolumes_Indexed(t *testing.T) {
 		t.Errorf("Expected third volume to be named 'vol-2', got '%s'", indexedResult[2].Name)
 	}
 }
+
+func TestIsStopSignal(t *testing.T) {
+	tests := []struct {
+		name            string
+		sig             syscall.Signal
+		containerLabels map[string]string
+		expected        bool
+		expectedErr     bool
+	}{
+		{
+			name:     "SIGKILL always stops the container",
+			sig:      syscall.SIGKILL,
+			expected: true,
+		},
+		{
+			name:     "without a stopSignal label the stop signal is SIGTERM",
+			sig:      syscall.SIGTERM,
+			expected: true,
+		},
+		{
+			name:     "SIGHUP does not stop the container",
+			sig:      syscall.SIGHUP,
+			expected: false,
+		},
+		{
+			name:            "a signal matching the stopSignal label stops the container",
+			sig:             syscall.SIGQUIT,
+			containerLabels: map[string]string{containerd.StopSignalLabel: "SIGQUIT"},
+			expected:        true,
+		},
+		{
+			name:            "SIGTERM does not stop the container when stopSignal is overridden",
+			sig:             syscall.SIGTERM,
+			containerLabels: map[string]string{containerd.StopSignalLabel: "SIGQUIT"},
+			expected:        false,
+		},
+		{
+			name:            "an unparsable stopSignal label is an error",
+			sig:             syscall.SIGHUP,
+			containerLabels: map[string]string{containerd.StopSignalLabel: "NOT_A_SIGNAL"},
+			expectedErr:     true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := IsStopSignal(tt.sig, tt.containerLabels)
+			if tt.expectedErr {
+				if err == nil {
+					t.Fatalf("expected an error, got none")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if got != tt.expected {
+				t.Errorf("expected %v, got %v", tt.expected, got)
+			}
+		})
+	}
+}