chore: centralize temp files and prefer streaming IO (#4668)

* chore: centralize temp files and prefer streaming IO Catalogers that create temp files ad-hoc can easily forget cleanup, leaking files on disk. Similarly, io.ReadAll is convenient but risks OOM on large or malicious inputs. Introduce internal/tmpdir to manage all cataloger temp storage under a single root directory with automatic cleanup. Prefer streaming parsers (bufio.Scanner, json/yaml.NewDecoder, io.LimitReader) over buffering entire inputs into memory. Add ruleguard rules to enforce both practices going forward. Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> * chore: go back to old release parsing Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> * simplify to limit reader in version check Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> * chore: regex change postponed Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> * simplify supplement release to limitreader Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> --------- Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com>
2026-10-31 06:26:30 +01:00 · 2026-03-18 10:53:51 -04:00
parent a3dacf5ecd
commit e38851143e
56 changed files with 704 additions and 214 deletions
@@ -0,0 +1,121 @@
+package tmpdir
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"sync"
+	"sync/atomic"
+)
+
+type ctxKey struct{}
+
+// Root creates a new root temp directory with the given prefix and returns a context with the
+// TempDir attached. Callers should defer Cleanup() on the returned TempDir to ensure all
+// temp files are removed.
+func Root(ctx context.Context, prefix string) (context.Context, *TempDir) {
+	td := &TempDir{prefix: prefix}
+	return context.WithValue(ctx, ctxKey{}, td), td
+}
+
+// FromPath creates a TempDir backed by an existing directory. The caller owns the lifecycle of
+// the directory; Cleanup() is a no-op. This is useful for wrapping a directory from t.TempDir()
+// where the test framework handles cleanup automatically.
+func FromPath(dir string) *TempDir {
+	td := &TempDir{}
+	td.root = dir
+	td.initOnce.Do(func() {}) // mark as initialized
+	return td
+}
+
+// WithValue returns a new context with the given TempDir attached. Use this to inject an
+// existing TempDir into a context (e.g., sharing a TempDir across multiple test contexts).
+func WithValue(ctx context.Context, td *TempDir) context.Context {
+	return context.WithValue(ctx, ctxKey{}, td)
+}
+
+// FromContext returns the TempDir from the context, or nil if none is set.
+func FromContext(ctx context.Context) *TempDir {
+	td, _ := ctx.Value(ctxKey{}).(*TempDir)
+	return td
+}
+
+// TempDir manages a tree of temporary directories. All files and child directories live under
+// a single root path that can be removed in one shot via Cleanup(). After initialization, the
+// struct has no mutable state — NewChild and NewFile delegate uniqueness to os.MkdirTemp and
+// os.CreateTemp respectively, so no locking is needed on the hot path.
+type TempDir struct {
+	prefix      string
+	root        string // set exactly once by initOnce
+	initOnce    sync.Once
+	initErr     error
+	cleanupOnce sync.Once
+	cleaned     atomic.Bool
+}
+
+func noop() {}
+
+// path returns the root directory, lazily creating it on the first call.
+func (t *TempDir) path() (string, error) {
+	t.initOnce.Do(func() {
+		t.root, t.initErr = os.MkdirTemp("", t.prefix+"-")
+	})
+	if t.initErr != nil {
+		return "", fmt.Errorf("failed to create root temp dir: %w", t.initErr)
+	}
+	if t.cleaned.Load() {
+		return "", fmt.Errorf("temp dir has been cleaned up")
+	}
+	return t.root, nil
+}
+
+// NewChild creates a named subdirectory under this TempDir. The returned cleanup function removes
+// the subdirectory and all contents; callers should defer it to reclaim space early. The root
+// Cleanup acts as a safety net if the per-child cleanup is missed. The cleanup function is safe
+// to call multiple times and is safe to call after the root has already been cleaned up.
+func (t *TempDir) NewChild(name string) (string, func(), error) {
+	root, err := t.path()
+	if err != nil {
+		return "", noop, err
+	}
+	dir, err := os.MkdirTemp(root, name+"-")
+	if err != nil {
+		return "", noop, fmt.Errorf("failed to create child temp dir: %w", err)
+	}
+	cleanup := func() {
+		_ = os.RemoveAll(dir)
+	}
+	return dir, cleanup, nil
+}
+
+// NewFile creates a new temp file under this TempDir with the given name pattern (as in os.CreateTemp).
+// The caller is responsible for closing the file. The returned cleanup function removes the file;
+// callers should defer it to reclaim space early. The root Cleanup acts as a safety net if the
+// per-file cleanup is missed. The cleanup function is safe to call multiple times.
+func (t *TempDir) NewFile(pattern string) (*os.File, func(), error) {
+	root, err := t.path()
+	if err != nil {
+		return nil, noop, err
+	}
+	f, err := os.CreateTemp(root, pattern)
+	if err != nil {
+		return nil, noop, fmt.Errorf("failed to create temp file: %w", err)
+	}
+	cleanup := func() {
+		_ = os.Remove(f.Name())
+	}
+	return f, cleanup, nil
+}
+
+// Cleanup removes the entire root directory and all contents. Safe to call multiple times.
+func (t *TempDir) Cleanup() error {
+	var err error
+	t.cleanupOnce.Do(func() {
+		t.cleaned.Store(true)
+		if t.root == "" {
+			return
+		}
+		err = os.RemoveAll(t.root)
+	})
+	return err
+}
@@ -0,0 +1,295 @@
+package tmpdir
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestRootAndFromContext(t *testing.T) {
+	ctx := context.Background()
+	assert.Nil(t, FromContext(ctx))
+
+	ctx, td := Root(ctx, "test")
+	require.NotNil(t, FromContext(ctx))
+	assert.Same(t, td, FromContext(ctx))
+}
+
+func TestWithValue(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+	defer td.Cleanup()
+
+	// inject the existing TempDir into a fresh context
+	ctx := WithValue(context.Background(), td)
+	assert.Same(t, td, FromContext(ctx))
+
+	// the injected TempDir is fully functional
+	f, cleanup, err := FromContext(ctx).NewFile("with-value-*.txt")
+	require.NoError(t, err)
+	defer cleanup()
+	require.NoError(t, f.Close())
+}
+
+func TestNewChild(t *testing.T) {
+	ctx, td := Root(context.Background(), "test")
+	defer td.Cleanup()
+	_ = ctx
+
+	child1, cleanup1, err := td.NewChild("sub")
+	require.NoError(t, err)
+	defer cleanup1()
+	child2, cleanup2, err := td.NewChild("sub")
+	require.NoError(t, err)
+	defer cleanup2()
+
+	// children are distinct
+	assert.NotEqual(t, child1, child2)
+
+	// both exist and are under the same root
+	info1, err := os.Stat(child1)
+	require.NoError(t, err)
+	assert.True(t, info1.IsDir())
+
+	info2, err := os.Stat(child2)
+	require.NoError(t, err)
+	assert.True(t, info2.IsDir())
+
+	assert.Equal(t, filepath.Dir(child1), filepath.Dir(child2))
+}
+
+func TestNewFile(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+	defer td.Cleanup()
+
+	f, cleanup, err := td.NewFile("hello-*.txt")
+	require.NoError(t, err)
+	defer cleanup()
+
+	_, err = f.WriteString("hello")
+	require.NoError(t, err)
+	require.NoError(t, f.Close())
+
+	content, err := os.ReadFile(f.Name())
+	require.NoError(t, err)
+	assert.Equal(t, "hello", string(content))
+}
+
+func TestCleanup(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+
+	child, _, err := td.NewChild("sub")
+	require.NoError(t, err)
+
+	f, _, err := td.NewFile("file-*")
+	require.NoError(t, err)
+	fname := f.Name()
+	f.Close()
+
+	// write a file inside the child dir too
+	require.NoError(t, os.WriteFile(filepath.Join(child, "inner.txt"), []byte("x"), 0600))
+
+	// everything exists
+	_, err = os.Stat(child)
+	require.NoError(t, err)
+	_, err = os.Stat(fname)
+	require.NoError(t, err)
+
+	// cleanup
+	require.NoError(t, td.Cleanup())
+
+	// everything is gone
+	_, err = os.Stat(child)
+	assert.True(t, os.IsNotExist(err))
+	_, err = os.Stat(fname)
+	assert.True(t, os.IsNotExist(err))
+
+	// double cleanup is safe
+	require.NoError(t, td.Cleanup())
+}
+
+func TestCleanupPreventsNewAllocation(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+	require.NoError(t, td.Cleanup())
+
+	_, _, err := td.NewChild("nope")
+	assert.Error(t, err)
+
+	_, _, err = td.NewFile("nope-*")
+	assert.Error(t, err)
+}
+
+func TestEarlyCleanupFile(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+	defer td.Cleanup()
+
+	f, cleanup, err := td.NewFile("early-*.txt")
+	require.NoError(t, err)
+
+	fname := f.Name()
+	require.NoError(t, f.Close())
+
+	// file exists before cleanup
+	_, err = os.Stat(fname)
+	require.NoError(t, err)
+
+	// early cleanup removes the file
+	cleanup()
+	_, err = os.Stat(fname)
+	assert.True(t, os.IsNotExist(err))
+
+	// calling cleanup again is safe (idempotent)
+	cleanup()
+}
+
+func TestEarlyCleanupChild(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+	defer td.Cleanup()
+
+	child, cleanup, err := td.NewChild("early")
+	require.NoError(t, err)
+
+	// child dir exists
+	_, err = os.Stat(child)
+	require.NoError(t, err)
+
+	// early cleanup removes it
+	cleanup()
+	_, err = os.Stat(child)
+	assert.True(t, os.IsNotExist(err))
+
+	// calling cleanup again is safe (idempotent)
+	cleanup()
+}
+
+func TestEarlyCleanupThenRootCleanup(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+
+	f, cleanupFile, err := td.NewFile("combo-*.txt")
+	require.NoError(t, err)
+	fname := f.Name()
+	f.Close()
+
+	child, cleanupChild, err := td.NewChild("combo")
+	require.NoError(t, err)
+
+	// early cleanup both
+	cleanupFile()
+	cleanupChild()
+
+	// files are already gone
+	_, err = os.Stat(fname)
+	assert.True(t, os.IsNotExist(err))
+	_, err = os.Stat(child)
+	assert.True(t, os.IsNotExist(err))
+
+	// root cleanup still works (no error on already-removed contents)
+	require.NoError(t, td.Cleanup())
+}
+
+func TestConcurrentNewChildAndNewFile(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+	defer td.Cleanup()
+
+	const goroutines = 20
+	errs := make(chan error, goroutines)
+	paths := make(chan string, goroutines)
+
+	for i := 0; i < goroutines; i++ {
+		go func(i int) {
+			if i%2 == 0 {
+				child, cleanup, err := td.NewChild("concurrent")
+				if err != nil {
+					errs <- err
+					return
+				}
+				defer cleanup()
+				paths <- child
+			} else {
+				f, cleanup, err := td.NewFile("concurrent-*.txt")
+				if err != nil {
+					errs <- err
+					return
+				}
+				defer cleanup()
+				_ = f.Close()
+				paths <- f.Name()
+			}
+			errs <- nil
+		}(i)
+	}
+
+	seen := make(map[string]bool)
+	for i := 0; i < goroutines; i++ {
+		err := <-errs
+		require.NoError(t, err)
+	}
+	close(paths)
+	for p := range paths {
+		assert.False(t, seen[p], "duplicate path: %s", p)
+		seen[p] = true
+	}
+	assert.Len(t, seen, goroutines)
+}
+
+func TestConcurrentNewChildDuringCleanup(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+
+	// trigger root creation
+	_, cleanup, err := td.NewChild("init")
+	require.NoError(t, err)
+	cleanup()
+
+	// cleanup and concurrent NewChild should not panic
+	done := make(chan struct{})
+	go func() {
+		_ = td.Cleanup()
+		close(done)
+	}()
+	// try creating children concurrently with cleanup — should get errors, not panics
+	for i := 0; i < 10; i++ {
+		_, c, _ := td.NewChild("race")
+		if c != nil {
+			c()
+		}
+	}
+	<-done
+}
+
+func TestLazyCreation(t *testing.T) {
+	_, td := Root(context.Background(), "test")
+
+	// root dir is not created until needed
+	assert.Equal(t, "", td.root)
+
+	_, _, err := td.NewFile("trigger-*")
+	require.NoError(t, err)
+
+	assert.NotEmpty(t, td.root)
+
+	require.NoError(t, td.Cleanup())
+}
+
+func TestFromPath(t *testing.T) {
+	dir := t.TempDir()
+	td := FromPath(dir)
+
+	// can create children
+	child, cleanup, err := td.NewChild("sub")
+	require.NoError(t, err)
+	defer cleanup()
+	assert.DirExists(t, child)
+
+	// can create files
+	f, cleanupFile, err := td.NewFile("file-*.txt")
+	require.NoError(t, err)
+	defer cleanupFile()
+	require.NoError(t, f.Close())
+	assert.FileExists(t, f.Name())
+
+	// root is the provided dir
+	assert.Equal(t, dir, filepath.Dir(child))
+}