From dfb8f2155ac39e21bbd03feaa3f2763fb1ff18c5 Mon Sep 17 00:00:00 2001
From: Laura Brehm <laurabrehm@hey.com>
Date: Wed, 24 Jul 2024 19:05:41 +0100
Subject: [PATCH] attach: don't return context cancelled error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In 3f0d90a2a939afa04a784b810dbdb035a0dff669 we introduced a global
signal handler and made sure all the contexts passed into command
execution get (appropriately) cancelled when we get a SIGINT.

Due to that change, and how we use this context during `docker attach`,
we started to return the context cancelation error when a user signals
the running `docker attach`.

Since this is the intended behavior, we shouldn't return an error, so
this commit adds checks to ignore this specific error in this case.

Also adds a regression test.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
(cherry picked from commit 66aa0f672c77490e8b06753b5af9fa1db5510f9c)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
---
 cli/command/container/attach.go      |  6 +++++-
 cli/command/container/attach_test.go |  5 +++++
 e2e/container/attach_test.go         | 29 ++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go
index a2d9838683..ddbaded4b4 100644
--- a/cli/command/container/attach.go
+++ b/cli/command/container/attach.go
@@ -146,7 +146,8 @@ func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, o
 		detachKeys:   options.DetachKeys,
 	}
 
-	if err := streamer.stream(ctx); err != nil {
+	// if the context was canceled, this was likely intentional and we shouldn't return an error
+	if err := streamer.stream(ctx); err != nil && !errors.Is(err, context.Canceled) {
 		return err
 	}
 
@@ -163,6 +164,9 @@ func getExitStatus(errC <-chan error, resultC <-chan container.WaitResponse) err
 			return cli.StatusError{StatusCode: int(result.StatusCode)}
 		}
 	case err := <-errC:
+		if errors.Is(err, context.Canceled) {
+			return nil
+		}
 		return err
 	}
 
diff --git a/cli/command/container/attach_test.go b/cli/command/container/attach_test.go
index 702b6dad31..0ba71ec7de 100644
--- a/cli/command/container/attach_test.go
+++ b/cli/command/container/attach_test.go
@@ -1,6 +1,7 @@
 package container
 
 import (
+	"context"
 	"io"
 	"testing"
 
@@ -113,6 +114,10 @@ func TestGetExitStatus(t *testing.T) {
 			},
 			expectedError: cli.StatusError{StatusCode: 15},
 		},
+		{
+			err:           context.Canceled,
+			expectedError: nil,
+		},
 	}
 
 	for _, testcase := range testcases {
diff --git a/e2e/container/attach_test.go b/e2e/container/attach_test.go
index 9b2ee9cdb8..6822ca60f7 100644
--- a/e2e/container/attach_test.go
+++ b/e2e/container/attach_test.go
@@ -1,11 +1,17 @@
 package container
 
 import (
+	"bytes"
 	"fmt"
+	"os"
+	"os/exec"
 	"strings"
 	"testing"
+	"time"
 
+	"github.com/creack/pty"
 	"github.com/docker/cli/e2e/internal/fixtures"
+	"gotest.tools/v3/assert"
 	"gotest.tools/v3/icmd"
 )
 
@@ -23,3 +29,26 @@ func TestAttachExitCode(t *testing.T) {
 func withStdinNewline(cmd *icmd.Cmd) {
 	cmd.Stdin = strings.NewReader("\n")
 }
+
+// Regression test for https://github.com/docker/cli/issues/5294
+func TestAttachInterrupt(t *testing.T) {
+	result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage, "sh", "-c", "sleep 5")
+	result.Assert(t, icmd.Success)
+	containerID := strings.TrimSpace(result.Stdout())
+
+	// run it as such so we can signal it later
+	c := exec.Command("docker", "attach", containerID)
+	d := bytes.Buffer{}
+	c.Stdout = &d
+	c.Stderr = &d
+	_, err := pty.Start(c)
+	assert.NilError(t, err)
+
+	// have to wait a bit to give time for the command to execute/print
+	time.Sleep(500 * time.Millisecond)
+	c.Process.Signal(os.Interrupt)
+
+	_ = c.Wait()
+	assert.Equal(t, c.ProcessState.ExitCode(), 0)
+	assert.Equal(t, d.String(), "")
+}