fix(render): cancel on SIGINT/SIGTERM so deferred cleanup runs

cmd/crossplane/main.go

@@ -18,12 +18,16 @@ limitations under the License.
 package main
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"os"
+	"os/signal"
 	"strings"
+	"syscall"
 
 	"github.com/alecthomas/kong"
+	"github.com/charmbracelet/x/ansi"
 	"github.com/charmbracelet/x/term"
 	"github.com/spf13/afero"
 	"github.com/willabides/kongplete"
@@ -159,13 +163,45 @@ func main() {
 
 	// Set up a spinner printer for commands to use. This helps ensure output
 	// consistency across commands.
-	sp := terminal.NewSpinnerPrinter(os.Stderr, term.IsTerminal(os.Stderr.Fd()))
+	tty := term.IsTerminal(os.Stderr.Fd())
+	sp := terminal.NewSpinnerPrinter(os.Stderr, tty)
 	ctx.BindTo(sp, (*terminal.SpinnerPrinter)(nil))
 
+	// Cancel the context on the first SIGINT or SIGTERM so in-flight work
+	// unwinds and deferred cleanup (e.g. render's containers and network)
+	// runs. A second signal exits immediately without waiting for cleanup.
+	sigCtx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+	stopExitOnSignal := context.AfterFunc(sigCtx, func() { exitOnSignal(tty) })
+	ctx.BindTo(sigCtx, (*context.Context)(nil))
+
 	err = ctx.Run()
+	interrupted := sigCtx.Err() != nil
+	stopExitOnSignal()
+	stop()
+	if interrupted {
+		// Exit with the conventional 128+SIGINT status.
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "crossplane: %v\n", err)
+		}
+		os.Exit(130)
+	}
 	ctx.FatalIfErrorf(err)
 }
 
+// exitOnSignal waits for SIGINT or SIGTERM, then exits with status 130
+// without waiting for cleanup. A spinner may still own the terminal, so when
+// stderr is a terminal it first shows the cursor and disables bracketed
+// paste, which would otherwise be left hidden and enabled.
+func exitOnSignal(tty bool) {
+	sig := make(chan os.Signal, 1)
+	signal.Notify(sig, os.Interrupt, syscall.SIGTERM)
+	<-sig
+	if tty {
+		_, _ = fmt.Fprint(os.Stderr, ansi.ShowCursor+ansi.ResetModeBracketedPaste)
+	}
+	os.Exit(130)
+}
+
 // configFlag scans argv for the --config flag and returns its value or "" if
 // the config flag is not present.
 func configFlag(args []string) (string, error) {

cmd/crossplane/render/engine_docker.go

@@ -63,6 +63,13 @@ type dockerRenderEngine struct {
 	// (exit-3 partial output, *docker.ContainerExitError vs non-exit errors)
 	// without a real Docker daemon.
 	runner containerRunner
+
+	// networks creates and removes the temporary Docker network Setup owns.
+	// Production callers leave it nil and Setup builds a real client from the
+	// environment only when it needs to create a network. Tests substitute a
+	// fake to exercise the create-network branch without a real Docker
+	// daemon.
+	networks networkClient
 }
 
 func (e *dockerRenderEngine) CheckContextSupport() error {
@@ -95,7 +102,16 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f
 		return func() {}, nil
 	}
 
-	networkID, networkName, err := createRenderNetwork(ctx)
+	cli := e.networks
+	if cli == nil {
+		c, err := newNetworkClient()
+		if err != nil {
+			return func() {}, errors.Wrap(err, "cannot create Docker network for rendering")
+		}
+		cli = c
+	}
+
+	networkID, networkName, err := createRenderNetwork(ctx, cli)
 	if err != nil {
 		return func() {}, errors.Wrap(err, "cannot create Docker network for rendering")
 	}
@@ -104,7 +120,7 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f
 	injectNetworkAnnotation(fns, networkName)
 
 	cleanup := func() { //nolint:contextcheck // Detached context for cleanup.
-		_ = removeRenderNetwork(context.Background(), networkID)
+		_ = removeRenderNetwork(context.Background(), cli, networkID)
 	}
 
 	return cleanup, nil

cmd/crossplane/render/engine_docker_test.go

@@ -23,10 +23,13 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/moby/moby/client"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/testing/protocmp"
 	"google.golang.org/protobuf/types/known/structpb"
 
+	"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
 	"github.com/crossplane/crossplane-runtime/v2/pkg/logging"
 
 	pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1"
@@ -222,9 +225,8 @@ func TestDockerRenderEngineSetup(t *testing.T) {
 	// call on the same engine stored its created network there. The branch
 	// must annotate the supplied functions so their containers join the
 	// network, never create a second network, and always return a no-op
-	// cleanup. The create-new-network branch is not covered here because it
-	// depends on a live Docker daemon; the broader render command tests
-	// exercise it integration-style.
+	// cleanup. The create-new-network branch is covered separately by
+	// TestDockerRenderEngineSetupCreatesNetwork.
 	//
 	// The MultiBatchAnnotatesAdditionalFunctions case simulates the
 	// in-process multi-composition use case from crossplane/cli#96: a
@@ -346,6 +348,95 @@ func TestDockerRenderEngineSetup(t *testing.T) {
 	}
 }
 
+func TestDockerRenderEngineSetupCreatesNetwork(t *testing.T) {
+	// When e.network is unset, Setup must create a temporary network through
+	// the engine's network client, record its name, annotate the supplied
+	// functions to join it, and return a cleanup that removes it through the
+	// same client.
+	errBoom := errors.New("boom")
+
+	type args struct {
+		create func(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error)
+	}
+	type want struct {
+		err error
+		// created is whether Setup should create a network, annotate the
+		// functions to join it, and return a cleanup that removes it.
+		created bool
+	}
+
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"CreatesNetwork": {
+			reason: "Setup should create a network, annotate the functions to join it, and return a cleanup that removes it.",
+			args: args{
+				create: createRenderNetworkReturns("network-id", nil),
+			},
+			want: want{
+				created: true,
+			},
+		},
+		"NetworkCreateError": {
+			reason: "Setup should return an error and a no-op cleanup, leaving the functions unannotated, when it cannot create the network.",
+			args: args{
+				create: createRenderNetworkReturns("", errBoom),
+			},
+			want: want{
+				err: errBoom,
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			// Cleanup discards NetworkRemove's result, so record whether it
+			// was called. A cleanup that shouldn't remove anything leaves
+			// MockNetworkRemove nil.
+			removed := false
+			cli := &mockNetworkClient{MockNetworkCreate: tc.args.create}
+			if tc.want.created {
+				cli.MockNetworkRemove = func(_ context.Context, networkID string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) {
+					if diff := cmp.Diff("network-id", networkID); diff != "" {
+						t.Errorf("\n%s\nNetworkRemove(...): -want network ID, +got network ID:\n%s", tc.reason, diff)
+					}
+					removed = true
+					return client.NetworkRemoveResult{}, nil
+				}
+			}
+			e := &dockerRenderEngine{log: logging.NewNopLogger(), networks: cli}
+			fns := []pkgv1.Function{functionWithAnnotations(nil)}
+
+			cleanup, err := e.Setup(t.Context(), fns)
+
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\nSetup(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+
+			wantFns := []pkgv1.Function{functionWithAnnotations(nil)}
+			if tc.want.created {
+				if !strings.HasPrefix(e.network, renderNetworkPrefix) {
+					t.Errorf("\n%s\nSetup(...): e.network = %q, want a network with prefix %q", tc.reason, e.network, renderNetworkPrefix)
+				}
+				wantFns = []pkgv1.Function{functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: e.network})}
+			} else if e.network != "" {
+				t.Errorf("\n%s\nSetup(...): e.network = %q, want it unset", tc.reason, e.network)
+			}
+			if diff := cmp.Diff(wantFns, fns); diff != "" {
+				t.Errorf("\n%s\nSetup(...): -want fns, +got fns:\n%s", tc.reason, diff)
+			}
+
+			cleanup()
+
+			if diff := cmp.Diff(tc.want.created, removed); diff != "" {
+				t.Errorf("\n%s\nSetup(...) cleanup: -want network removed, +got network removed:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}
+
 // nonExitError is a stand-in for non-*ContainerExitError failures (e.g. image
 // pull errors) returned by docker.RunContainer.
 type nonExitError struct{ msg string }

cmd/crossplane/render/network.go

@@ -46,15 +46,28 @@ func (f *EngineFlags) SetDefaultCrossplaneDockerNetwork(fns []pkgv1.Function) {
 	}
 }
 
-// createRenderNetwork creates a temporary Docker bridge network for render.
-// Function containers and the Crossplane render container join this network so
-// they can reach each other. Returns the network ID and name.
-func createRenderNetwork(ctx context.Context) (string, string, error) {
+// networkClient is the subset of the Docker client the render engine uses to
+// manage its temporary Docker network.
+type networkClient interface {
+	NetworkCreate(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error)
+	NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error)
+}
+
+var _ networkClient = (*client.Client)(nil)
+
+// newNetworkClient returns a real Docker client built from the environment.
+func newNetworkClient() (networkClient, error) {
 	cli, err := docker.NewClient()
 	if err != nil {
-		return "", "", errors.Wrap(err, "cannot create Docker client")
+		return nil, errors.Wrap(err, "cannot create Docker client")
 	}
+	return cli, nil
+}
 
+// createRenderNetwork creates a temporary Docker bridge network for render.
+// Function containers and the Crossplane render container join this network so
+// they can reach each other. Returns the network ID and name.
+func createRenderNetwork(ctx context.Context, cli networkClient) (string, string, error) {
 	name := fmt.Sprintf("crossplane-render-%s", rand.String(8))
 
 	resp, err := cli.NetworkCreate(ctx, name, client.NetworkCreateOptions{
@@ -68,11 +81,7 @@ func createRenderNetwork(ctx context.Context) (string, string, error) {
 }
 
 // removeRenderNetwork removes a temporary Docker network.
-func removeRenderNetwork(ctx context.Context, networkID string) error {
-	cli, err := docker.NewClient()
-	if err != nil {
-		return errors.Wrap(err, "cannot create Docker client")
-	}
-	_, err = cli.NetworkRemove(ctx, networkID, client.NetworkRemoveOptions{})
+func removeRenderNetwork(ctx context.Context, cli networkClient, networkID string) error {
+	_, err := cli.NetworkRemove(ctx, networkID, client.NetworkRemoveOptions{})
 	return errors.Wrap(err, "cannot remove Docker network")
 }