fix(render): stop every function runtime gracefully and always remove Remove-policy containers

cmd/crossplane/render/op/cmd.go

@@ -218,7 +218,11 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinte
 	if err != nil {
 		return errors.Wrap(err, "cannot start function runtimes")
 	}
-	defer render.StopFunctionRuntimes(log, fnAddrs)
+	defer func() {
+		if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil {
+			log.Info("Error stopping function runtimes", "error", err)
+		}
+	}()
 
 	// Build and execute the render request.
 	in := render.OperationInputs{

cmd/crossplane/render/render.go

@@ -21,7 +21,6 @@ package render
 import (
 	"context"
 	"strings"
-	"time"
 
 	fnv1 "github.com/crossplane/function-sdk-go/proto/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -105,14 +104,16 @@ func (fa *FunctionAddresses) Addresses() map[string]string {
 	return fa.addrs
 }
 
-// Stop all function runtimes.
+// Stop all function runtimes. Every runtime is stopped even if some fail; the
+// returned error joins all failures. Each runtime bounds its own cleanup.
 func (fa *FunctionAddresses) Stop(ctx context.Context) error {
+	var errs []error
 	for name, rctx := range fa.contexts {
 		if err := rctx.Stop(ctx); err != nil {
-			return errors.Wrapf(err, "cannot stop function %q runtime (target %q)", name, rctx.Target)
+			errs = append(errs, errors.Wrapf(err, "cannot stop function %q runtime (target %q)", name, rctx.Target))
 		}
 	}
-	return nil
+	return errors.Join(errs...)
 }
 
 // StartFunctionRuntimes starts the runtime for each function and returns their
@@ -167,16 +168,14 @@ func injectNetworkAnnotation(fns []pkgv1.Function, networkName string) {
 	}
 }
 
-// StopFunctionRuntimes stops all function runtimes with a timeout.
-func StopFunctionRuntimes(log logging.Logger, fa *FunctionAddresses) {
+// StopFunctionRuntimes stops all function runtimes and returns all failures
+// joined. Cleanup runs even if ctx is already cancelled, so it can follow an
+// interrupted render. Each runtime bounds its own cleanup.
+func StopFunctionRuntimes(ctx context.Context, fa *FunctionAddresses) error {
 	if fa == nil {
-		return
-	}
-	stopCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-	defer cancel()
-	if err := fa.Stop(stopCtx); err != nil {
-		log.Info("Error stopping function runtimes", "error", err)
+		return nil
 	}
+	return fa.Stop(context.WithoutCancel(ctx))
 }
 
 // OverrideFunctionAnnotations applies annotation overrides from flags to

cmd/crossplane/render/render_test.go

@@ -1,12 +1,15 @@
 package render
 
 import (
+	"context"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
+
 	pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1"
 )
 
@@ -91,3 +94,148 @@ func TestOverrideFunctionAnnotations(t *testing.T) {
 func functionWithAnnotations(annotations map[string]string) pkgv1.Function {
 	return pkgv1.Function{ObjectMeta: metav1.ObjectMeta{Annotations: annotations}}
 }
+
+func TestFunctionAddressesStop(t *testing.T) {
+	errA := errors.New("boom a")
+	errB := errors.New("boom b")
+
+	stopReturns := func(err error) RuntimeContext {
+		return RuntimeContext{Target: "fn:9443", Stop: func(context.Context) error { return err }}
+	}
+
+	type args struct {
+		contexts map[string]RuntimeContext
+	}
+	type want struct {
+		// errs are the errors the returned error must wrap. When empty, Stop
+		// must return nil.
+		errs []error
+	}
+
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"AllSucceed": {
+			reason: "Stop should return nil when every runtime stops.",
+			args: args{
+				contexts: map[string]RuntimeContext{
+					"ok-a": stopReturns(nil),
+					"ok-b": stopReturns(nil),
+				},
+			},
+		},
+		"SomeFail": {
+			reason: "Stop should stop every runtime even if some fail, and return every failure.",
+			args: args{
+				contexts: map[string]RuntimeContext{
+					"ok-a":   stopReturns(nil),
+					"fail-a": stopReturns(errA),
+					"ok-b":   stopReturns(nil),
+					"fail-b": stopReturns(errB),
+				},
+			},
+			want: want{
+				errs: []error{errA, errB},
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			// Repeat so Go's randomized map iteration can't hide a regression
+			// to returning on the first error.
+			for range 20 {
+				fa := &FunctionAddresses{contexts: tc.args.contexts}
+
+				err := fa.Stop(t.Context())
+
+				wantErrs := tc.want.errs
+				if len(wantErrs) == 0 {
+					wantErrs = []error{nil}
+				}
+				for _, want := range wantErrs {
+					if diff := cmp.Diff(want, err, cmpopts.EquateErrors()); diff != "" {
+						t.Fatalf("\n%s\nStop(...): -want error, +got error:\n%s", tc.reason, diff)
+					}
+				}
+			}
+		})
+	}
+}
+
+func TestStopFunctionRuntimes(t *testing.T) {
+	errBoom := errors.New("boom")
+
+	type args struct {
+		// stopErr is what the runtime's Stop returns.
+		stopErr error
+		// cancelParent cancels the context passed to StopFunctionRuntimes
+		// before calling it.
+		cancelParent bool
+		// noRuntimes passes nil FunctionAddresses.
+		noRuntimes bool
+	}
+	type want struct {
+		// ctxErr is the error of the context the runtime's Stop was called
+		// with.
+		ctxErr error
+		err    error
+	}
+
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"ParentContextLive": {
+			reason: "StopFunctionRuntimes should stop each runtime with a live context.",
+			want:   want{},
+		},
+		"ParentContextCancelled": {
+			reason: "StopFunctionRuntimes should still stop each runtime, with a live context, when the parent context is already cancelled.",
+			args:   args{cancelParent: true},
+			want:   want{},
+		},
+		"StopError": {
+			reason: "StopFunctionRuntimes should return the error a runtime fails to stop with.",
+			args:   args{stopErr: errBoom},
+			want:   want{err: errBoom},
+		},
+		"NilFunctionAddresses": {
+			reason: "StopFunctionRuntimes should do nothing when there are no runtimes.",
+			args:   args{noRuntimes: true},
+			want:   want{},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			// The context the runtime is stopped with is the property under
+			// test, so record it.
+			var got want
+			var fa *FunctionAddresses
+			if !tc.args.noRuntimes {
+				fa = &FunctionAddresses{contexts: map[string]RuntimeContext{
+					"fn": {Target: "fn:9443", Stop: func(ctx context.Context) error {
+						got.ctxErr = ctx.Err()
+						return tc.args.stopErr
+					}},
+				}}
+			}
+
+			ctx, cancel := context.WithCancel(t.Context())
+			defer cancel()
+			if tc.args.cancelParent {
+				cancel()
+			}
+
+			got.err = StopFunctionRuntimes(ctx, fa)
+
+			if diff := cmp.Diff(tc.want, got, cmp.AllowUnexported(want{}), cmpopts.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\nStopFunctionRuntimes(...): -want, +got:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}

cmd/crossplane/render/runtime_docker.go

@@ -24,6 +24,7 @@ import (
 	"net"
 	"net/netip"
 	"strings"
+	"time"
 
 	"github.com/containerd/errdefs"
 	"github.com/google/go-containerregistry/pkg/authn"
@@ -87,12 +88,15 @@ type DockerCleanup string
 
 // Supported AnnotationKeyRuntimeDockerCleanup values.
 const (
-	// AnnotationValueRuntimeDockerCleanupStop is the default. It stops the
-	// container once rendering is done.
+	// AnnotationValueRuntimeDockerCleanupStop stops the container once
+	// rendering is done, waiting up to containerStopGracePeriod for it to exit
+	// on SIGTERM before Docker kills it.
 	AnnotationValueRuntimeDockerCleanupStop DockerCleanup = "Stop"
 
-	// AnnotationValueRuntimeDockerCleanupRemove stops and removes the
-	// container once rendering is done.
+	// AnnotationValueRuntimeDockerCleanupRemove is the default. It stops the
+	// container once rendering is done, waiting up to containerStopGracePeriod
+	// for it to exit on SIGTERM, then force removes it. The container is
+	// removed even if the graceful stop fails.
 	AnnotationValueRuntimeDockerCleanupRemove DockerCleanup = "Remove"
 
 	// AnnotationValueRuntimeDockerCleanupOrphan leaves the container running
@@ -102,6 +106,22 @@ const (
 	AnnotationValueRuntimeDockerCleanupDefault = AnnotationValueRuntimeDockerCleanupRemove
 )
 
+const (
+	// containerStopGracePeriod is how long the Stop and Remove cleanup
+	// policies wait for a Function container to exit on SIGTERM before Docker
+	// kills it.
+	containerStopGracePeriod = 3 * time.Second
+
+	// containerStopTimeout bounds the ContainerStop request: the grace period
+	// plus time for Docker to kill the container.
+	containerStopTimeout = containerStopGracePeriod + 2*time.Second
+
+	// containerRemoveTimeout bounds the ContainerRemove request. It's separate
+	// from containerStopTimeout so a stop that hangs can't leave removal
+	// without time to run.
+	containerRemoveTimeout = 10 * time.Second
+)
+
 // AnnotationKeyRuntimeDockerPullPolicy can be added to a Function to control how its runtime
 // image is pulled.
 const AnnotationKeyRuntimeDockerPullPolicy = "render.crossplane.io/runtime-docker-pull-policy"
@@ -512,19 +532,27 @@ func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) {
 
 	// Inline stop function
 	stop := func(ctx context.Context) error {
+		grace := int(containerStopGracePeriod / time.Second)
+		stopOpts := client.ContainerStopOptions{Timeout: &grace}
+
 		switch r.Cleanup {
 		case AnnotationValueRuntimeDockerCleanupOrphan:
 			return nil
 		case AnnotationValueRuntimeDockerCleanupStop:
-			if _, err := cli.ContainerStop(ctx, containerID, client.ContainerStopOptions{}); err != nil {
+			if err := stopContainer(ctx, cli, containerID, stopOpts); err != nil {
 				return errors.Wrap(err, "cannot stop Docker container")
 			}
 		case AnnotationValueRuntimeDockerCleanupRemove:
-			if _, err := cli.ContainerStop(ctx, containerID, client.ContainerStopOptions{}); err != nil {
-				return errors.Wrap(err, "cannot stop Docker container")
+			// Give the container a chance to exit gracefully, then force
+			// remove it whether or not the stop succeeded, so a container
+			// that's slow to exit on SIGTERM can't cause removal to be
+			// skipped. A stop failure only matters if removal fails too.
+			var stopErr error
+			if err := stopContainer(ctx, cli, containerID, stopOpts); err != nil {
+				stopErr = errors.Wrap(err, "cannot stop Docker container")
 			}
-			if _, err := cli.ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{}); err != nil {
-				return errors.Wrap(err, "cannot remove Docker container")
+			if err := removeContainer(ctx, cli, containerID); err != nil {
+				return errors.Join(errors.Wrap(err, "cannot remove Docker container"), stopErr)
 			}
 		}
 
@@ -534,6 +562,26 @@ func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) {
 	return RuntimeContext{Target: containerAddr, Stop: stop}, nil
 }
 
+// stopContainer stops a container, bounding the request by
+// containerStopTimeout.
+func stopContainer(ctx context.Context, cli containerCleanupClient, containerID string, opts client.ContainerStopOptions) error {
+	ctx, cancel := context.WithTimeout(ctx, containerStopTimeout)
+	defer cancel()
+
+	_, err := cli.ContainerStop(ctx, containerID, opts)
+	return err
+}
+
+// removeContainer force removes a container, bounding the request by
+// containerRemoveTimeout.
+func removeContainer(ctx context.Context, cli containerCleanupClient, containerID string) error {
+	ctx, cancel := context.WithTimeout(ctx, containerRemoveTimeout)
+	defer cancel()
+
+	_, err := cli.ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{Force: true})
+	return err
+}
+
 type pullClient interface {
 	ImagePull(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error)
 }