WIP: podman exec: correctly support detaching (part 2)

cmd/podman/containers/exec.go

@@ -1,7 +1,6 @@
 package containers
 
 import (
-	"bufio"
 	"context"
 	"errors"
 	"fmt"
@@ -172,7 +171,7 @@ func exec(cmd *cobra.Command, args []string) error {
 		streams.OutputStream = os.Stdout
 		streams.ErrorStream = os.Stderr
 		if execOpts.Interactive {
-			streams.InputStream = bufio.NewReader(os.Stdin)
+			streams.InputStream = os.Stdin
 			streams.AttachInput = true
 		}
 		streams.AttachOutput = true

go.mod

@@ -231,3 +231,5 @@ require (
 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
 	tags.cncf.io/container-device-interface/specs-go v0.8.0 // indirect
 )
+
+replace github.com/containers/common => github.com/Luap99/common v0.20.3-0.20250123195450-f0fd031b07f8

go.sum

@@ -10,6 +10,8 @@ github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c/go.mod h1:xomTg6
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/BurntSushi/toml v1.4.0 h1:kuoIxZQy2WRRk1pttg9asf+WVv6tWQuBNVmK8+nqPr0=
 github.com/BurntSushi/toml v1.4.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho=
+github.com/Luap99/common v0.20.3-0.20250123195450-f0fd031b07f8 h1:Uh/dSC6OGikcFGsvbzy0a3ma9NISRLGL2KW8XwRknuY=
+github.com/Luap99/common v0.20.3-0.20250123195450-f0fd031b07f8/go.mod h1:mWhwkYaWR5bXeOwq3ruzdmH9gaT2pex00C6pd4VXuvM=
 github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY=
 github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU=
 github.com/Microsoft/hcsshim v0.12.9 h1:2zJy5KA+l0loz1HzEGqyNnjd3fyZA31ZBCGKacp6lLg=
@@ -78,8 +80,6 @@ github.com/containernetworking/plugins v1.5.1 h1:T5ji+LPYjjgW0QM+KyrigZbLsZ8jaX+
 github.com/containernetworking/plugins v1.5.1/go.mod h1:MIQfgMayGuHYs0XdNudf31cLLAC+i242hNm6KuDGqCM=
 github.com/containers/buildah v1.38.1-0.20241119213149-52437ef15d33 h1:Ih6KuyByK7ZGGzkS0M5rVBPLWIyeDvdL5klhsKBo8vA=
 github.com/containers/buildah v1.38.1-0.20241119213149-52437ef15d33/go.mod h1:RxIuKhwTpRl3ma4d4BF6QzSSeg9zNNvo/xhYJOKeDQs=
-github.com/containers/common v0.61.1-0.20250120135258-06628cb958e9 h1:aiup0MIiAi2Xnv15vApAPqgy4/49ZGkYOpevDgGHfxg=
-github.com/containers/common v0.61.1-0.20250120135258-06628cb958e9/go.mod h1:1S+/XhAEOwMGePCUqoYYh1iZo9fU1IpuIwVzCCIdBVU=
 github.com/containers/conmon v2.0.20+incompatible h1:YbCVSFSCqFjjVwHTPINGdMX1F6JXHGTUje2ZYobNrkg=
 github.com/containers/conmon v2.0.20+incompatible/go.mod h1:hgwZ2mtuDrppv78a/cOBNiCm6O0UMWGx1mu7P00nu5I=
 github.com/containers/gvisor-tap-vsock v0.8.2 h1:uQMBCCHlIIj62fPjbvgm6AL5EzsP6TP5eviByOJEsOg=

libpod/container_exec.go

@@ -284,16 +284,17 @@ func (c *Container) ExecStart(sessionID string) error {
 	return c.save()
 }
 
-func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachStreams, newSize *resize.TerminalSize) error {
-	return c.execStartAndAttach(sessionID, streams, newSize, false)
-}
-
-// ExecStartAndAttach starts and attaches to an exec session in a container.
+// execStartAndAttach starts and attaches to an exec session in a container.
 // newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
 func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachStreams, newSize *resize.TerminalSize, isHealthcheck bool) error {
+	unlock := true
 	if !c.batched {
 		c.lock.Lock()
-		defer c.lock.Unlock()
+		defer func() {
+			if unlock {
+				c.lock.Unlock()
+			}
+		}()
 
 		if err := c.syncContainer(); err != nil {
 			return err
@@ -348,6 +349,12 @@ func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachS
 	}
 
 	tmpErr := <-attachChan
+	// user detached
+	if errors.Is(tmpErr, define.ErrDetach) {
+		// ensure we the defer does not unlock as we are not locked here
+		unlock = false
+		return tmpErr
+	}
 	if lastErr != nil {
 		logrus.Errorf("Container %s exec session %s error: %v", c.ID(), session.ID(), lastErr)
 	}
@@ -435,9 +442,14 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
 		close(hijackDone)
 	}()
 
+	unlock := true
 	if !c.batched {
 		c.lock.Lock()
-		defer c.lock.Unlock()
+		defer func() {
+			if unlock {
+				c.lock.Unlock()
+			}
+		}()
 
 		if err := c.syncContainer(); err != nil {
 			return err
@@ -518,6 +530,12 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
 	}
 
 	tmpErr := <-attachChan
+	// user detached
+	if errors.Is(tmpErr, define.ErrDetach) {
+		// ensure we the defer does not unlock as we are not locked here
+		unlock = false
+		return tmpErr
+	}
 	if lastErr != nil {
 		logrus.Errorf("Container %s exec session %s error: %v", c.ID(), session.ID(), lastErr)
 	}
@@ -769,11 +787,14 @@ func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resi
 	if err != nil {
 		return -1, err
 	}
+	cleanup := true
 	defer func() {
-		if err := c.ExecRemove(sessionID, false); err != nil {
-			if retErr == nil && !errors.Is(err, define.ErrNoSuchExecSession) {
-				exitCode = -1
-				retErr = err
+		if cleanup {
+			if err := c.ExecRemove(sessionID, false); err != nil {
+				if retErr == nil && !errors.Is(err, define.ErrNoSuchExecSession) {
+					exitCode = -1
+					retErr = err
+				}
 			}
 		}
 	}()
@@ -807,6 +828,11 @@ func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resi
 	}
 
 	if err := c.execStartAndAttach(sessionID, streams, size, isHealthcheck); err != nil {
+		// user detached, there will be no exit just exit without reporting an error
+		if errors.Is(err, define.ErrDetach) {
+			cleanup = false
+			return 0, nil
+		}
 		return -1, err
 	}
 

libpod/define/config.go

@@ -1,7 +1,6 @@
 package define
 
 import (
-	"bufio"
 	"io"
 
 	"github.com/containers/common/libnetwork/types"
@@ -55,7 +54,7 @@ type AttachStreams struct {
 	// ErrorStream will be attached to container's STDERR
 	ErrorStream io.Writer
 	// InputStream will be attached to container's STDIN
-	InputStream *bufio.Reader
+	InputStream io.Reader
 	// AttachOutput is whether to attach to STDOUT
 	// If false, stdout will not be attached
 	AttachOutput bool

libpod/healthcheck.go

@@ -3,7 +3,6 @@
 package libpod
 
 import (
-	"bufio"
 	"bytes"
 	"context"
 	"errors"
@@ -85,7 +84,7 @@ func (c *Container) runHealthCheck(ctx context.Context, isStartup bool) (define.
 	streams := new(define.AttachStreams)
 	output := &bytes.Buffer{}
 
-	streams.InputStream = bufio.NewReader(os.Stdin)
+	streams.InputStream = os.Stdin
 	streams.OutputStream = output
 	streams.ErrorStream = output
 	streams.AttachOutput = true

libpod/util.go

@@ -149,7 +149,7 @@ func checkDependencyContainer(depCtr, ctr *Container) error {
 
 // hijackWriteError writes an error to a hijacked HTTP session.
 func hijackWriteError(toWrite error, cid string, terminal bool, httpBuf *bufio.ReadWriter) {
-	if toWrite != nil {
+	if toWrite != nil && !errors.Is(toWrite, define.ErrDetach) {
 		errString := []byte(fmt.Sprintf("Error: %v\n", toWrite))
 		if !terminal {
 			// We need a header.

pkg/api/handlers/compat/exec.go

@@ -200,7 +200,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 		t := r.Context().Value(api.IdleTrackerKey).(*idle.Tracker)
 		defer t.Close()
 
-		if err != nil {
+		if err != nil && !errors.Is(err, define.ErrDetach) {
 			// Cannot report error to client as a 500 as the Upgrade set status to 101
 			logErr(err)
 		}

pkg/domain/infra/abi/terminal/terminal_common.go

@@ -3,7 +3,6 @@
 package terminal
 
 import (
-	"bufio"
 	"context"
 	"fmt"
 	"os"
@@ -65,7 +64,7 @@ func StartAttachCtr(ctx context.Context, ctr *libpod.Container, stdout, stderr,
 	streams := new(define.AttachStreams)
 	streams.OutputStream = stdout
 	streams.ErrorStream = stderr
-	streams.InputStream = bufio.NewReader(stdin)
+	streams.InputStream = stdin
 	streams.AttachOutput = true
 	streams.AttachError = true
 	streams.AttachInput = true

pkg/domain/infra/tunnel/containers.go

@@ -1,6 +1,7 @@
 package tunnel
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"errors"
@@ -610,7 +611,7 @@ func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrID string, o
 	startAndAttachOptions := new(containers.ExecStartAndAttachOptions)
 	startAndAttachOptions.WithOutputStream(streams.OutputStream).WithErrorStream(streams.ErrorStream)
 	if streams.InputStream != nil {
-		startAndAttachOptions.WithInputStream(*streams.InputStream)
+		startAndAttachOptions.WithInputStream(*bufio.NewReader(streams.InputStream))
 	}
 	startAndAttachOptions.WithAttachError(streams.AttachError).WithAttachOutput(streams.AttachOutput).WithAttachInput(streams.AttachInput)
 	if err := containers.ExecStartAndAttach(ic.ClientCtx, sessionID, startAndAttachOptions); err != nil {
@@ -666,6 +667,7 @@ func startAndAttach(ic *ContainerEngine, name string, detachKeys *string, sigPro
 	go func() {
 		err := containers.Attach(ic.ClientCtx, name, input, output, errput, attachReady, options)
 		attachErr <- err
+		close(attachErr)
 	}()
 	// Wait for the attach to actually happen before starting
 	// the container.
@@ -683,13 +685,36 @@ func startAndAttach(ic *ContainerEngine, name string, detachKeys *string, sigPro
 			return -1, err
 		}
 
-		// call wait immediately after start to avoid racing against container removal when it was created with --rm
-		exitCode, err := containers.Wait(cancelCtx, name, nil)
-		if err != nil {
-			return -1, err
-		}
-		code = int(exitCode)
+		// Call wait immediately after start to avoid racing against container removal when it was created with --rm.
+		// It must be run in a separate goroutine to so we do not block when attach returns early, i.e. user
+		// detaches in which case wait would not return.
+		waitChan := make(chan error)
+		go func() {
+			defer close(waitChan)
+
+			exitCode, err := containers.Wait(cancelCtx, name, nil)
+			if err != nil {
+				waitChan <- fmt.Errorf("wait for container: %w", err)
+				return
+			}
+			code = int(exitCode)
+		}()
 
+		select {
+		case err := <-waitChan:
+			if err != nil {
+				return -1, err
+			}
+		case err := <-attachErr:
+			if err != nil {
+				return -1, err
+			}
+			// also wait for the wait to be complete in this case
+			err = <-waitChan
+			if err != nil {
+				return -1, err
+			}
+		}
 	case err := <-attachErr:
 		return -1, err
 	}

test/system/075-exec.bats

@@ -84,7 +84,9 @@ load helpers
     dd if=/dev/urandom of=$bigfile bs=1024 count=1500
     expect=$(sha512sum $bigfile | awk '{print $1}')
     # Transfer it to container, via exec, make sure correct #bytes are sent
-    run_podman exec -i $cid dd of=/tmp/bigfile bs=512 <$bigfile
+    # Note using --detach-keys "" to disable the detach sequence, otherwise
+    # the byte sequence may randomly appear in the stream and could cause flakes.
+    run_podman exec -i --detach-keys "" $cid dd of=/tmp/bigfile bs=512 <$bigfile
     is "${lines[0]}" "3000+0 records in"  "dd: number of records in"
     is "${lines[1]}" "3000+0 records out" "dd: number of records out"
     # Verify sha. '% *' strips off the path, keeping only the SHA

test/system/450-interactive.bats

@@ -39,7 +39,7 @@ function teardown() {
         kill $PODMAN_SOCAT_PID
         PODMAN_SOCAT_PID=
     fi
-    rm -f $PODMAN_TEST_PTY $PODMAN_DUMMY_PTY
+    rm -f $PODMAN_TEST_PTY $PODMAN_DUMMY
 
     basic_teardown
 }
@@ -124,4 +124,59 @@ function teardown() {
     is "$output" "$expected_tty" "passthrough-tty: tty matches"
 }
 
+@test "podman run detach keys" {
+    local cname1=c1-$(safename)
+    local cname2=c2-$(safename)
+
+    local msg=$(random_string)
+    # First "type" a command then send CTRL-p,CTRL-q sequence to the run command.
+    # We must send that sequence in two echo commands if I use a single echo it
+    # doesn't work, I don't know why.
+    # If the detach does not work podman run will hang and timeout.
+    # The sleep is needed to ensure the output can be printed before we detach.
+    (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cp' > $PODMAN_DUMMY; echo -n $'\cq' > $PODMAN_DUMMY ) &
+    run_podman run -it --name $cname1 $IMAGE sh <$PODMAN_TEST_PTY
+    # Because we print to a terminal it appends "\r"
+    assert "$output" =~ "$msg"$'\r' "run output contains message"
+
+    # The container should still be running
+    run_podman inspect --format {{.State.Status}} $cname1
+    assert "$output" == "running" "container status"
+
+    # Now a second test with --detach-keys to make sure the cli option works
+    (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cc' > $PODMAN_DUMMY; echo -n $'J' > $PODMAN_DUMMY ) &
+    run_podman run -it --name $cname2 --detach-keys ctrl-c,J $IMAGE sh <$PODMAN_TEST_PTY
+    assert "$output" =~ "$msg"$'\r' "run output with --detach-keys contains message"
+
+    run_podman rm -f -t0 $cname1 $cname2
+}
+
+@test "podman exec detach keys" {
+    skip_if_remote "FIXME #25089: podman-remote exec detach broken"
+
+    local cname=c-$(safename)
+    run_podman run -d --name $cname $IMAGE sleep inf
+
+    local msg=$(random_string)
+    # First "type" a command then send CTRL-p,CTRL-q sequence to the exec command.
+    # If the detach does not work podman exec will hang and timeout.
+    # The sleep is needed to ensure the output can be printed before we detach.
+    (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cp' > $PODMAN_DUMMY; echo -n $'\cq' > $PODMAN_DUMMY ) &
+    run_podman exec -it $cname sh <$PODMAN_TEST_PTY
+    # Because we print to a terminal it appends "\r"
+    assert "$output" =~ "$msg"$'\r' "exec output contains message"
+
+    # The previous exec session/process should still be running
+    run_podman exec $cname ps aux
+    # Match PID=2 USER=root and COMMAND=sh from the ps output
+    assert "$output" =~ "2 root.*sh" "sh exec process still running"
+
+    # Now a second test with --detach-keys to make sure the cli option works
+    (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\ct' > $PODMAN_DUMMY; echo -n $'f' > $PODMAN_DUMMY ) &
+    run_podman exec -it --detach-keys ctrl-t,f $cname sh <$PODMAN_TEST_PTY
+    assert "$output" =~ "$msg"$'\r' "exec output with --detach-keys contains message"
+
+    run_podman rm -f -t0 $cname
+}
+
 # vim: filetype=sh