podman exec: correctly support detaching

podman exec support detaching early via the detach key sequence. In that case the podman process should exit successfully but the container exec process keeps running. Given that I could not find any existing test for the detach key functionality not even for exec I added some. This seems to reveal more issues with on podman-remote, podman-remote run detach was broken which I fixed here as well but for podman-remote exec something bigger is needed. While I thought I fixed most problems there there was a strange race condition which caused the process to just hang. Thus I skipped the remote exec test for now and filled #25089 to track that. Fixes #24895 Signed-off-by: Paul Holzinger <[email protected]>
containers · Jan 22, 2025 · 61fd7a4 · 61fd7a4
1 parent 392840d
commit 61fd7a4
Show file tree

Hide file tree

Showing 5 changed files with 124 additions and 15 deletions.
diff --git a/libpod/container_exec.go b/libpod/container_exec.go
@@ -287,9 +287,14 @@ func (c *Container) ExecStart(sessionID string) error {
 // 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
@@ -344,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)
 	}
@@ -431,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
@@ -514,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)
 	}
@@ -765,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
+				}
 			}
 		}
 	}()
@@ -803,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
 	}
 

diff --git a/libpod/util.go b/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.

diff --git a/pkg/api/handlers/compat/exec.go b/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)
 		}

diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
@@ -666,6 +666,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 +684,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
 	}

diff --git a/test/system/450-interactive.bats b/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