fix: do not close caller-owned writer in container DefaultHandler

internal/container/run.go

@@ -9,6 +9,8 @@ import (
 	dcontainer "github.com/moby/moby/api/types/container"
 	dockerClient "github.com/moby/moby/client"
 	"github.com/pkg/errors"
+
+	"github.com/buildpacks/pack/pkg/logging"
 )
 
 type Handler func(bodyChan <-chan dcontainer.WaitResponse, errChan <-chan error, reader io.Reader) error
@@ -87,9 +89,17 @@ func DefaultHandler(out, errOut io.Writer) Handler {
 	}
 }
 
+// optionallyCloseWriter flushes writers that pack created for a phase and that
+// buffer output, namely *logging.PrefixWriter, whose Close flushes any pending
+// partial line without closing the writer it wraps.
+//
+// Writers owned by the caller must not be closed here. When a builder is
+// trusted the logger's writer (often os.Stdout) is passed through unwrapped, and
+// closing an arbitrary io.Closer would close the caller's stream. That is
+// surprising and breaks consumers using pack as a library. See #1214.
 func optionallyCloseWriter(writer io.Writer) error {
-	if closer, ok := writer.(io.Closer); ok {
-		return closer.Close()
+	if prefixWriter, ok := writer.(*logging.PrefixWriter); ok {
+		return prefixWriter.Close()
 	}
 
 	return nil

internal/container/run_test.go

@@ -0,0 +1,74 @@
+package container
+
+import (
+	"bytes"
+	"io"
+	"testing"
+
+	"github.com/heroku/color"
+	"github.com/sclevine/spec"
+	"github.com/sclevine/spec/report"
+
+	"github.com/buildpacks/pack/pkg/logging"
+)
+
+func TestContainerRun(t *testing.T) {
+	color.Disable(true)
+	defer color.Disable(false)
+
+	spec.Run(t, "container/run", testContainerRun, spec.Report(report.Terminal{}), spec.Parallel())
+}
+
+// recordingWriteCloser models a writer owned by the caller that also happens to
+// implement io.Closer, such as os.Stdout passed to pack via the logger.
+type recordingWriteCloser struct {
+	bytes.Buffer
+	closed bool
+}
+
+func (w *recordingWriteCloser) Close() error {
+	w.closed = true
+	return nil
+}
+
+var _ io.WriteCloser = &recordingWriteCloser{}
+
+func testContainerRun(t *testing.T, when spec.G, it spec.S) {
+	when("#optionallyCloseWriter", func() {
+		it("does not close writers owned by the caller", func() {
+			writer := &recordingWriteCloser{}
+
+			if err := optionallyCloseWriter(writer); err != nil {
+				t.Fatalf("expected no error, got: %v", err)
+			}
+
+			// Closing a caller-owned writer (e.g. os.Stdout) is surprising and
+			// breaks consumers using pack as a library. See #1214.
+			if writer.closed {
+				t.Fatal("expected caller-owned writer to remain open, but it was closed")
+			}
+		})
+
+		it("flushes buffered output from a PrefixWriter", func() {
+			var buf bytes.Buffer
+			prefixWriter := logging.NewPrefixWriter(&buf, "some-prefix")
+
+			// A line without a trailing newline stays buffered until the writer
+			// is flushed.
+			if _, err := prefixWriter.Write([]byte("buffered output")); err != nil {
+				t.Fatalf("writing to prefix writer: %v", err)
+			}
+			if got := buf.String(); got != "" {
+				t.Fatalf("expected partial line to stay buffered, got: %q", got)
+			}
+
+			if err := optionallyCloseWriter(prefixWriter); err != nil {
+				t.Fatalf("expected no error, got: %v", err)
+			}
+
+			if got := buf.String(); !bytes.Contains([]byte(got), []byte("buffered output")) {
+				t.Fatalf("expected buffered output to be flushed, got: %q", got)
+			}
+		})
+	})
+}