Close input files after processing each one (#2796) (#2808)

readStream opened an *os.File but returned a *bufio.Reader, so the
*os.File type switch in streamEvaluator.EvaluateFiles never matched and
no input file was ever closed. allAtOnceEvaluator.EvaluateFiles did not
attempt to close them at all. Descriptors were only released at process
exit or by the os.File finaliser, so a multi-file run could exhaust the
file descriptor limit.

readStream now returns an explicit cleanup function alongside the reader,
which both evaluators call once the file has been processed.

Co-authored-by: Pablo Garcia <pablito@MacBook-Neo-de-Pablo.local>
Co-authored-by: Mike Farah <mikefarah@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

pkg/yqlib/all_at_once_evaluator.go

@@ -49,12 +49,13 @@ func (e *allAtOnceEvaluator) EvaluateFiles(expression string, filenames []string
 
 	var allDocuments = list.New()
 	for _, filename := range filenames {
-		reader, err := readStream(filename)
+		reader, cleanup, err := readStream(filename)
 		if err != nil {
 			return err
 		}
 
 		fileDocuments, err := readDocuments(reader, filename, fileIndex, decoder)
+		cleanup()
 		if err != nil {
 			return err
 		}

pkg/yqlib/front_matter_test.go

@@ -136,10 +136,11 @@ Some content
 	test.AssertResult(t, originalFilename, resolved)
 
 	// Read documents using the temp file, verify they get the original filename
-	reader, err := readStream(tempFilename)
+	reader, cleanup, err := readStream(tempFilename)
 	if err != nil {
 		panic(err)
 	}
+	defer cleanup()
 	decoder := NewYamlDecoder(ConfiguredYamlPreferences)
 	docs, err := readDocuments(reader, tempFilename, 0, decoder)
 	if err != nil {

pkg/yqlib/stream_evaluator.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 )
 
 // A yaml expression evaluator that runs the expression multiple times for each given yaml document.
@@ -50,21 +49,17 @@ func (s *streamEvaluator) EvaluateFiles(expression string, filenames []string, p
 	}
 
 	for _, filename := range filenames {
-		reader, err := readStream(filename)
+		reader, cleanup, err := readStream(filename)
 
 		if err != nil {
 			return err
 		}
 		processedDocs, err := s.Evaluate(filename, reader, node, printer, decoder)
+		cleanup()
 		if err != nil {
 			return err
 		}
 		totalProcessDocs = totalProcessDocs + processedDocs
-
-		switch reader := reader.(type) {
-		case *os.File:
-			safelyCloseFile(reader)
-		}
 	}
 
 	if totalProcessDocs == 0 {

pkg/yqlib/utils.go

@@ -32,21 +32,20 @@ func resolveFilename(filename string) string {
 	return filename
 }
 
-func readStream(filename string) (io.Reader, error) {
-	var reader *bufio.Reader
+// readStream returns a reader for the given file, along with a cleanup function
+// that must be called once the reader is no longer needed. The cleanup is a no-op
+// for stdin.
+func readStream(filename string) (io.Reader, func(), error) {
 	if filename == "-" {
-		reader = bufio.NewReader(os.Stdin)
-	} else {
-		// ignore CWE-22 gosec issue - that's more targeted for http based apps that run in a public directory,
-		// and ensuring that it's not possible to give a path to a file outside that directory.
-		file, err := os.Open(filename) // #nosec
-		if err != nil {
-			return nil, err
-		}
-		reader = bufio.NewReader(file)
+		return bufio.NewReader(os.Stdin), func() {}, nil
 	}
-	return reader, nil
-
+	// ignore CWE-22 gosec issue - that's more targeted for http based apps that run in a public directory,
+	// and ensuring that it's not possible to give a path to a file outside that directory.
+	file, err := os.Open(filename) // #nosec
+	if err != nil {
+		return nil, nil, err
+	}
+	return bufio.NewReader(file), func() { safelyCloseFile(file) }, nil
 }
 
 func writeString(writer io.Writer, txt string) error {

pkg/yqlib/utils_test.go

@@ -4,11 +4,89 @@ import (
 	"bufio"
 	"bytes"
 	"io"
+	"os"
+	"path/filepath"
+	"runtime/debug"
+	"strconv"
 	"testing"
 
 	"github.com/mikefarah/yq/v4/test"
 )
 
+// countOpenFileDescriptors returns the number of file descriptors this process
+// currently holds open, or -1 if the platform does not expose them.
+func countOpenFileDescriptors() int {
+	for _, dir := range []string{"/proc/self/fd", "/dev/fd"} {
+		// Readdirnames avoids stat-ing each entry, which races with descriptors
+		// (including this directory handle) being closed underneath us.
+		handle, err := os.Open(dir)
+		if err != nil {
+			continue
+		}
+		names, err := handle.Readdirnames(-1)
+		safelyCloseFile(handle)
+		if err != nil {
+			continue
+		}
+		// discount the directory handle itself
+		return len(names) - 1
+	}
+	return -1
+}
+
+func writeSampleFiles(t *testing.T, count int) []string {
+	t.Helper()
+	dir := t.TempDir()
+	filenames := make([]string, count)
+	for i := 0; i < count; i++ {
+		filename := filepath.Join(dir, "sample-"+strconv.Itoa(i)+".yml")
+		if err := os.WriteFile(filename, []byte("a: apple\n"), 0600); err != nil {
+			t.Fatalf("failed to write sample file: %v", err)
+		}
+		filenames[i] = filename
+	}
+	return filenames
+}
+
+func discardingPrinter() Printer {
+	return NewPrinter(NewYamlEncoder(ConfiguredYamlPreferences), NewSinglePrinterWriter(bufio.NewWriter(io.Discard)))
+}
+
+func assertNoLeakedFileDescriptors(t *testing.T, evaluate func(filenames []string) error) {
+	t.Helper()
+	InitExpressionParser()
+
+	// os.File finalisers close leaked descriptors on collection, which would
+	// let a genuine leak pass unnoticed.
+	defer debug.SetGCPercent(debug.SetGCPercent(-1))
+
+	before := countOpenFileDescriptors()
+	if before < 0 {
+		t.Skip("file descriptors are not observable on this platform")
+	}
+
+	if err := evaluate(writeSampleFiles(t, 50)); err != nil {
+		t.Fatalf("failed to evaluate files: %v", err)
+	}
+
+	after := countOpenFileDescriptors()
+	if after > before {
+		t.Errorf("expected no additional open file descriptors, had %d before and %d after", before, after)
+	}
+}
+
+func TestStreamEvaluatorClosesInputFiles(t *testing.T) {
+	assertNoLeakedFileDescriptors(t, func(filenames []string) error {
+		return NewStreamEvaluator().EvaluateFiles(".a", filenames, discardingPrinter(), NewYamlDecoder(ConfiguredYamlPreferences))
+	})
+}
+
+func TestAllAtOnceEvaluatorClosesInputFiles(t *testing.T) {
+	assertNoLeakedFileDescriptors(t, func(filenames []string) error {
+		return NewAllAtOnceEvaluator().EvaluateFiles(".a", filenames, discardingPrinter(), NewYamlDecoder(ConfiguredYamlPreferences))
+	})
+}
+
 // plainWriter only implements io.Writer, so io.WriteString must fall back to Write.
 type plainWriter struct {
 	buf bytes.Buffer