cli/command/stack: wait for the tasks to stop before removing the networks

cli/command/stack/remove.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"slices"
+	"time"
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
@@ -81,20 +82,27 @@ func runRemove(ctx context.Context, dockerCli command.Cli, opts removeOptions) e
 
 		// TODO(thaJeztah): change this "hasError" boolean to return a (multi-)error for each of these functions instead.
 		hasError := removeServices(ctx, dockerCli, services.Items)
+
+		if !opts.detach && !hasError {
+			// Wait for the tasks of the services to stop before removing the
+			// networks. The daemon accepts removing a network as soon as every
+			// task attached to it is marked for removal, that is, while the
+			// containers are still stopping; a task that terminates after one
+			// of its networks is gone is never deallocated by the manager, and
+			// the addresses it holds on its remaining networks (the ingress
+			// network in particular) leak until the manager is restarted; see
+			// https://github.com/moby/moby/issues/37338.
+			if err := waitOnTasks(ctx, apiClient, namespace); err != nil {
+				errs = append(errs, fmt.Errorf("failed to wait on tasks of stack: %s: %w", namespace, err))
+			}
+		}
+
 		hasError = removeSecrets(ctx, dockerCli, secrets.Items) || hasError
 		hasError = removeConfigs(ctx, dockerCli, configs.Items) || hasError
 		hasError = removeNetworks(ctx, dockerCli, networks.Items) || hasError
 
 		if hasError {
 			errs = append(errs, errors.New("failed to remove some resources from stack: "+namespace))
-			continue
-		}
-
-		if !opts.detach {
-			err = waitOnTasks(ctx, apiClient, namespace)
-			if err != nil {
-				errs = append(errs, fmt.Errorf("failed to wait on tasks of stack: %s: %w", namespace, err))
-			}
 		}
 	}
 	return errors.Join(errs...)
@@ -171,24 +179,29 @@ func terminalState(state swarm.TaskState) bool {
 	return numberedStates[state] > numberedStates[swarm.TaskStateRunning]
 }
 
+// taskPollInterval is the pause between two polls of the task list while
+// waiting for the tasks of a stack to stop.
+const taskPollInterval = 200 * time.Millisecond
+
+// waitOnTasks blocks until every task of the stack has reached a terminal
+// state or has been removed, or until ctx is done.
 func waitOnTasks(ctx context.Context, apiClient client.APIClient, namespace string) error {
-	terminalStatesReached := 0
 	for {
 		res, err := getStackTasks(ctx, apiClient, namespace)
 		if err != nil {
 			return fmt.Errorf("failed to get tasks: %w", err)
 		}
 
-		for _, task := range res.Items {
-			if terminalState(task.Status.State) {
-				terminalStatesReached++
-				break
-			}
+		if !slices.ContainsFunc(res.Items, func(task swarm.Task) bool {
+			return !terminalState(task.Status.State)
+		}) {
+			return nil
 		}
 
-		if terminalStatesReached == len(res.Items) {
-			break
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		case <-time.After(taskPollInterval):
 		}
 	}
-	return nil
 }

cli/command/stack/remove_test.go

@@ -1,12 +1,14 @@
 package stack
 
 import (
+	"context"
 	"errors"
 	"io"
 	"strings"
 	"testing"
 
 	"github.com/docker/cli/internal/test"
+	"github.com/moby/moby/api/types/swarm"
 	"github.com/moby/moby/client"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
@@ -141,3 +143,85 @@ func TestRemoveContinueAfterError(t *testing.T) {
 	assert.Check(t, is.DeepEqual(allSecretIDs, apiClient.removedSecrets))
 	assert.Check(t, is.DeepEqual(allConfigIDs, apiClient.removedConfigs))
 }
+
+// taskListStoppingAfter returns a TaskList stub reporting two tasks: one that
+// has already stopped, and one that keeps running until the stub has been
+// polled the given number of times. It counts the polls in *calls.
+func taskListStoppingAfter(polls int, calls *int) func(client.TaskListOptions) (client.TaskListResult, error) {
+	return func(client.TaskListOptions) (client.TaskListResult, error) {
+		*calls++
+		last := swarm.TaskStateRunning
+		if *calls >= polls {
+			last = swarm.TaskStateShutdown
+		}
+		return client.TaskListResult{Items: []swarm.Task{
+			{Status: swarm.TaskStatus{State: swarm.TaskStateShutdown}},
+			{Status: swarm.TaskStatus{State: last}},
+		}}, nil
+	}
+}
+
+func TestWaitOnTasksWaitsForAllTasks(t *testing.T) {
+	const pollsUntilStopped = 4
+	var taskListCalls int
+	apiClient := &fakeClient{
+		taskListFunc: taskListStoppingAfter(pollsUntilStopped, &taskListCalls),
+	}
+
+	assert.NilError(t, waitOnTasks(context.Background(), apiClient, "foo"))
+	assert.Check(t, is.Equal(pollsUntilStopped, taskListCalls))
+}
+
+func TestWaitOnTasksReturnsWhenContextIsCancelled(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+	apiClient := &fakeClient{
+		taskListFunc: func(client.TaskListOptions) (client.TaskListResult, error) {
+			cancel()
+			return client.TaskListResult{Items: []swarm.Task{
+				{Status: swarm.TaskStatus{State: swarm.TaskStateRunning}},
+			}}, nil
+		},
+	}
+
+	assert.ErrorIs(t, waitOnTasks(ctx, apiClient, "foo"), context.Canceled)
+}
+
+func TestRemoveStackWaitsForTasksBeforeRemovingNetworks(t *testing.T) {
+	const pollsUntilStopped = 4
+	var taskListCalls, taskListCallsAtNetworkRemoval int
+	apiClient := &fakeClient{
+		services:     []string{objectName("foo", "service1")},
+		networks:     []string{objectName("foo", "network1")},
+		taskListFunc: taskListStoppingAfter(pollsUntilStopped, &taskListCalls),
+	}
+	apiClient.networkRemoveFunc = func(networkID string) error {
+		taskListCallsAtNetworkRemoval = taskListCalls
+		apiClient.removedNetworks = append(apiClient.removedNetworks, networkID)
+		return nil
+	}
+	cmd := newRemoveCommand(test.NewFakeCli(apiClient))
+	cmd.SetArgs([]string{"--detach=false", "foo"})
+	cmd.SetOut(io.Discard)
+
+	assert.NilError(t, cmd.Execute())
+	assert.Check(t, is.DeepEqual(buildObjectIDs(apiClient.services), apiClient.removedServices))
+	assert.Check(t, is.DeepEqual(buildObjectIDs(apiClient.networks), apiClient.removedNetworks))
+	assert.Check(t, is.Equal(pollsUntilStopped, taskListCallsAtNetworkRemoval))
+}
+
+func TestRemoveStackDetachedDoesNotWaitOnTasks(t *testing.T) {
+	apiClient := &fakeClient{
+		services: []string{objectName("foo", "service1")},
+		networks: []string{objectName("foo", "network1")},
+		taskListFunc: func(client.TaskListOptions) (client.TaskListResult, error) {
+			return client.TaskListResult{}, errors.New("tasks must not be listed when detached")
+		},
+	}
+	cmd := newRemoveCommand(test.NewFakeCli(apiClient))
+	cmd.SetArgs([]string{"foo"})
+	cmd.SetOut(io.Discard)
+
+	assert.NilError(t, cmd.Execute())
+	assert.Check(t, is.DeepEqual(buildObjectIDs(apiClient.networks), apiClient.removedNetworks))
+}

docs/reference/commandline/stack_rm.md

@@ -20,6 +20,14 @@ Remove one or more stacks
 
 Remove the stack from the swarm.
 
+By default, the command returns as soon as the removal of the stack's services,
+networks, secrets, and configs has been requested; the tasks of the services
+are stopped in the background. With `--detach=false`, the command waits for the
+tasks to stop before it removes the networks of the stack, and returns once the
+stack is gone. Removing the networks after the tasks have stopped lets the
+swarm manager release every address the tasks held, including their addresses
+on the `ingress` network.
+
 > [!NOTE]
 > This is a cluster management command, and must be executed on a swarm
 > manager node. To learn about managers and workers, refer to the