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.

Now I wrote automated test for both podman run and exec detach but this
uncovered several larger issues:
 - detach sequence parsing is broken[1]
 - podman-remote exec detach is broken[2]
 - detach in general seems to be buggy/racy, seeing lot of flakes that
   fail to restore the terminal and get an EIO instead, i.e.
   "Unable to restore terminal: input/output error"

Thus I cannot add tests for now but this commit should at least fix the
obvoius case as reported by the user so I like to get this in regardless
and I will work through the other issues once I have more time.

Fixes #24895

[1] https://github.com/containers/common/pull/2302
[2] https://github.com/containers/podman/issues/25089

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

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
 	}
 

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/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
 	}