Update close stdin logic

go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/containerd/log v0.1.1-0.20260403072107-cb1839ebf76b
 	github.com/containerd/otelttrpc v0.1.0
 	github.com/containerd/plugin v1.1.0
-	github.com/containerd/shimtest v0.3.0
+	github.com/containerd/shimtest v0.3.3
 	github.com/containerd/ttrpc v1.2.9
 	github.com/containerd/typeurl/v2 v2.3.0
 	github.com/docker/go-events v0.0.0-20190806004212-e31b211e4f1c

go.sum

@@ -37,8 +37,8 @@ github.com/containerd/platforms v1.0.0-rc.4 h1:M42JrUT4zfZTqtkUwkr0GzmUWbfyO5VO0
 github.com/containerd/platforms v1.0.0-rc.4/go.mod h1:lKlMXyLybmBedS/JJm11uDofzI8L2v0J2ZbYvNsbq1A=
 github.com/containerd/plugin v1.1.0 h1:O+7lczNJVMy8rz0YNx3xGB8tTf5qY4i5abF041Ew19U=
 github.com/containerd/plugin v1.1.0/go.mod h1:qBTum+A8lJ6lO44A19Eo7y1OlcLj4OWFH1DA/vnHmcc=
-github.com/containerd/shimtest v0.3.0 h1:oTtRnAEA20cqqxU68Jpg6fajUVXtUYI38Q1Z/shv70k=
-github.com/containerd/shimtest v0.3.0/go.mod h1:v9b7phlmKrfn9zKHqhDyoe0kv24mxDEYyJlXFcFhjnI=
+github.com/containerd/shimtest v0.3.3 h1:n0bG1i5baAjrtFWtPySjIiDICq7Gmh50hhjvVkYUktU=
+github.com/containerd/shimtest v0.3.3/go.mod h1:vT0DHiGsMJ6Hi56uGZLWS3o8gzRd0wSCC17Wen3XjP4=
 github.com/containerd/ttrpc v1.2.9 h1:ha0ak962T0s3CA/RoZ6S6xiWZQF24GrBaEpiGX1uihg=
 github.com/containerd/ttrpc v1.2.9/go.mod h1:jjtQRwXm4DL3KsHKW8vDiUOV6wO0hi6IPhmJhxU7aEs=
 github.com/containerd/typeurl/v2 v2.3.0 h1:HZHPhRWo5XMy3QGQoPrUzbW/2ckwjfweHmOwlkIrPAQ=

internal/shim/task/io.go

@@ -32,6 +32,7 @@ import (
 
 	"github.com/containerd/containerd/v2/pkg/stdio"
 	"github.com/containerd/errdefs"
+	"github.com/containerd/log"
 )
 
 type streamCreator interface {
@@ -105,11 +106,25 @@ func (s *service) forwardIO(ctx context.Context, ss streamCreator, idPrefix stri
 		}
 	}()
 	ioDone := make(chan struct{})
-	stdinEOF, err := copyStreams(ctx, streams, sio.Stdin, sio.Stdout, sio.Stderr, ioDone)
+	stdinEOF, stdinDone, err := copyStreams(ctx, streams, sio.Stdin, sio.Stdout, sio.Stderr, ioDone)
 	if err != nil {
 		return stdio.Stdio{}, nil, nil, nil, err
 	}
 	return pio, func(ctx context.Context) error {
+		// Release our stdin FIFO write reference (if any) unconditionally,
+		// as a safety net for callers that tear down the process without
+		// ever issuing CloseIO (e.g. Kill+Delete). This is idempotent: it
+		// is a no-op if CloseIO already released it. Dropping the
+		// reference here only allows the host-side stdin copy to reach a
+		// real EOF once the external client has also closed its own
+		// write end; it does not force-close anything the client still
+		// holds open.
+		if stdinEOF != nil {
+			if err := stdinEOF(); err != nil {
+				log.G(ctx).WithError(err).Warn("error releasing stdin during io shutdown")
+			}
+		}
+
 		// ioDone is expected to already be closed by the time ioShutdown
 		// is called: the host Wait handler blocks until ioDone fires before
 		// returning to the caller, ensuring all buffered bytes have been
@@ -127,6 +142,21 @@ func (s *service) forwardIO(ctx context.Context, ss streamCreator, idPrefix stri
 		case <-ctx.Done():
 			err = ctx.Err()
 		}
+		// Wait for the stdin copy goroutine to finish draining and send
+		// its in-band CloseWrite before we close the underlying stream
+		// connection out from under it. Bounded by the same deadline as
+		// ioDone above; if the external client never closes its own FIFO
+		// write end, this times out and we force-close everything below,
+		// same as the ioDone safety net.
+		if stdinDone != nil {
+			select {
+			case <-stdinDone:
+			case <-ctx.Done():
+				if err == nil {
+					err = ctx.Err()
+				}
+			}
+		}
 		for i, c := range streams {
 			if c != nil && (i != 2 || c != streams[1]) {
 				c.Close()

internal/shim/task/io_copystreams_unix.go

@@ -41,10 +41,23 @@ type stdinStreamWriteCloser interface {
 	CloseWrite() error
 }
 
-// copyStreams returns a stdinEOF function that, when called (by CloseIO),
-// signals the stdin goroutine to stop reading the FIFO and send the
-// OP_SHUTDOWN(SEND) in-band EOF to the guest. It is nil when stdin is empty.
-func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdout, stderr string, done chan struct{}) (stdinEOF func() error, err error) {
+// copyStreams returns a stdinEOF function and a stdinDone channel for the
+// stdin FIFO (both nil when stdin is empty).
+//
+// stdinEOF, when called (by CloseIO or container teardown), drops the
+// host's own O_WRONLY reference on the stdin FIFO -- mirroring the
+// reference containerd runc shim's stdin FIFO handling. Holding that
+// reference is what lets the external client close its own FIFO write end
+// to detach without delivering EOF to the process: the FIFO can only reach
+// a real EOF once every writer, including this one, is closed. Dropping it
+// does not force anything; the copy goroutine still fully drains whatever
+// is already buffered in the FIFO (and whatever the client still writes,
+// if it hasn't closed its own end yet) before the guest sees EOF.
+//
+// stdinDone is closed once the copy goroutine has drained the FIFO to a
+// real EOF and delivered the in-band CloseWrite, so callers can wait for
+// it before tearing down the underlying stream connection.
+func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdout, stderr string, done chan struct{}) (stdinEOF func() error, stdinDone <-chan struct{}, err error) {
 	var cwg sync.WaitGroup
 	var copying atomic.Int32
 	copying.Store(2)
@@ -103,18 +116,18 @@ func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdo
 		}
 		ok, err := fifo.IsFifo(i.name)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		var (
 			fw io.WriteCloser
 			fr io.Closer
 		)
 		if ok {
 			if fw, err = fifo.OpenFifo(ctx, i.name, syscall.O_WRONLY, 0); err != nil {
-				return nil, fmt.Errorf("containerd-shim: opening w/o fifo %q failed: %w", i.name, err)
+				return nil, nil, fmt.Errorf("containerd-shim: opening w/o fifo %q failed: %w", i.name, err)
 			}
 			if fr, err = fifo.OpenFifo(ctx, i.name, syscall.O_RDONLY, 0); err != nil {
-				return nil, fmt.Errorf("containerd-shim: opening r/o fifo %q failed: %w", i.name, err)
+				return nil, nil, fmt.Errorf("containerd-shim: opening r/o fifo %q failed: %w", i.name, err)
 			}
 		} else {
 			if sameFile != nil {
@@ -123,7 +136,7 @@ func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdo
 				continue
 			}
 			if fw, err = os.OpenFile(i.name, syscall.O_WRONLY|syscall.O_APPEND, 0); err != nil {
-				return nil, fmt.Errorf("containerd-shim: opening file %q failed: %w", i.name, err)
+				return nil, nil, fmt.Errorf("containerd-shim: opening file %q failed: %w", i.name, err)
 			}
 			if stdout == stderr {
 				sameFile = newCountingWriteCloser(fw, 1)
@@ -133,36 +146,63 @@ func copyStreams(ctx context.Context, streams [3]io.ReadWriteCloser, stdin, stdo
 	}
 	if stdin != "" {
 		// Assert early: the stdin vsock stream must implement CloseWrite so
-		// we can send OP_SHUTDOWN(SEND) in-order when CloseIO fires, rather
-		// than forwarding an out-of-band RPC that races in-flight bytes.
+		// we can send OP_SHUTDOWN(SEND) in-order once the FIFO reaches a
+		// real EOF, rather than forwarding an out-of-band RPC that races
+		// in-flight bytes.
 		sc, ok := streams[0].(stdinStreamWriteCloser)
 		if !ok {
-			return nil, fmt.Errorf("stdin stream connection does not implement CloseWrite; vsock conn required")
+			return nil, nil, fmt.Errorf("stdin stream connection does not implement CloseWrite; vsock conn required")
+		}
+
+		// Hold our own O_WRONLY reference on the stdin FIFO, mirroring the
+		// reference containerd runc shim's stdin handling (it opens the
+		// FIFO write end itself to unblock its own O_RDONLY open and to
+		// decouple client detach from process EOF). As long as this
+		// reference is open, the FIFO cannot reach EOF even if the
+		// external client closes its own write end -- that just means
+		// detach. EOF is only delivered once this reference is dropped by
+		// stdinEOF below (CloseIO or container teardown).
+		fw, err := fifo.OpenFifo(context.Background(), stdin, syscall.O_WRONLY|syscall.O_NONBLOCK, 0)
+		if err != nil {
+			return nil, nil, fmt.Errorf("containerd-shim: opening w/o stdin fifo %q failed: %w", stdin, err)
 		}
 		f, err := fifo.OpenFifo(context.Background(), stdin, syscall.O_RDONLY|syscall.O_NONBLOCK, 0)
 		if err != nil {
-			return nil, fmt.Errorf("containerd-shim: opening %s failed: %s", stdin, err)
+			fw.Close()
+			return nil, nil, fmt.Errorf("containerd-shim: opening %s failed: %s", stdin, err)
 		}
-		// closeCh is closed by the stdinEOF function (triggered by CloseIO).
-		closeCh := make(chan struct{})
+		stdinDoneCh := make(chan struct{})
+		stdinDone = stdinDoneCh
 		cwg.Add(1)
 		go func() {
 			cwg.Done()
+			defer close(stdinDoneCh)
 			p := bufPool.Get().(*[]byte)
 			defer bufPool.Put(p)
-			copyStdinUntilClose(ctx, sc, f, *p, closeCh)
+			// Drain to a real EOF: io.CopyBuffer only returns once every
+			// writer of the FIFO -- including our own reference above --
+			// has closed, guaranteeing every byte buffered before EOF has
+			// already been forwarded to the guest via sc.Write.
+			if _, err := io.CopyBuffer(sc, f, *p); err != nil {
+				log.G(ctx).WithError(err).Warn("error copying stdin")
+			}
+			// All buffered bytes are now on the wire; deliver EOF in-band.
+			if err := sc.CloseWrite(); err != nil {
+				log.G(ctx).WithError(err).Warn("error sending stdin EOF via CloseWrite")
+			}
 			// Do NOT Close sc here; deferred to ioShutdown/forwardIO cleanup
 			// so the transport outlives the in-band EOF and the host can
 			// close its end cleanly after the guest drains.
 			f.Close()
 		}()
 		stdinEOF = func() error {
-			// Signal the goroutine to stop reading the FIFO and send
-			// OP_SHUTDOWN(SEND) in-order on the stdin stream.
-			close(closeCh)
-			return nil
+			// Drop our write reference. Idempotent: fifo.Close is safe to
+			// call more than once (e.g. once from an explicit CloseIO
+			// call and again from container teardown as a safety net for
+			// callers that never issue CloseIO).
+			return fw.Close()
 		}
 	}
 	cwg.Wait()
-	return stdinEOF, nil
+	return stdinEOF, stdinDone, nil
 }