From cb2c29e290b8934591898328423c289d9f160c9a Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Tue, 17 Mar 2026 22:51:41 +0100 Subject: [PATCH 1/4] Implement smart clean --- .../command/generate/generate_test.go | 4 +- private/buf/bufgen/generator.go | 48 ++--- .../bufprotopluginos/bufprotopluginos.go | 21 +- .../bufprotopluginos/cleaner.go | 124 ------------ .../bufprotopluginos/response_writer.go | 191 +++++++++++++++++- .../bufprotopluginos/response_writer_test.go | 184 ++++++++++++++++- 6 files changed, 377 insertions(+), 195 deletions(-) delete mode 100644 private/bufpkg/bufprotoplugin/bufprotopluginos/cleaner.go diff --git a/cmd/buf/internal/command/generate/generate_test.go b/cmd/buf/internal/command/generate/generate_test.go index 5a8cc15538..b699dccc1d 100644 --- a/cmd/buf/internal/command/generate/generate_test.go +++ b/cmd/buf/internal/command/generate/generate_test.go @@ -372,7 +372,7 @@ func TestOutputFlag(t *testing.T) { } } -func TestSkipWriteWhenUnchanged(t *testing.T) { +func TestSmartCleanPreservesMtime(t *testing.T) { t.Parallel() tempDirPath := t.TempDir() template := filepath.Join("testdata", "simple", "buf.gen.yaml") @@ -384,7 +384,7 @@ func TestSkipWriteWhenUnchanged(t *testing.T) { past := time.Now().Add(-time.Hour) require.NoError(t, os.Chtimes(outFile, past, past)) - testRunSuccess(t, "--output", tempDirPath, "--template", template, input) + testRunSuccess(t, "--output", tempDirPath, "--template", template, "--clean", input) info, err := os.Stat(outFile) require.NoError(t, err) diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 769d452010..71935540ef 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -104,15 +104,17 @@ func (g *generator) Generate( if generateOptions.deleteOuts != nil { shouldDeleteOuts = *generateOptions.deleteOuts } + responseWriterOptions := []bufprotopluginos.ResponseWriterOption{ + bufprotopluginos.ResponseWriterWithCreateOutDirIfNotExists(), + } if shouldDeleteOuts { - if err := g.deleteOuts( - ctx, - generateOptions.baseOutDirPath, - config.GeneratePluginConfigs(), - ); err != nil { - return err - } + responseWriterOptions = append(responseWriterOptions, bufprotopluginos.ResponseWriterWithDeleteOuts()) } + responseWriter := bufprotopluginos.NewResponseWriter( + g.logger, + g.storageosProvider, + responseWriterOptions..., + ) for _, image := range images { if err := g.generateCode( ctx, @@ -120,33 +122,14 @@ func (g *generator) Generate( image, generateOptions.baseOutDirPath, config.GeneratePluginConfigs(), + responseWriter, generateOptions.includeImportsOverride, generateOptions.includeWellKnownTypesOverride, ); err != nil { return err } } - return nil -} - -func (g *generator) deleteOuts( - ctx context.Context, - baseOutDir string, - pluginConfigs []bufconfig.GeneratePluginConfig, -) error { - return bufprotopluginos.NewCleaner(g.storageosProvider).DeleteOuts( - ctx, - xslices.Map( - pluginConfigs, - func(pluginConfig bufconfig.GeneratePluginConfig) string { - out := pluginConfig.Out() - if baseOutDir != "" && baseOutDir != "." { - return filepath.Join(baseOutDir, out) - } - return out - }, - ), - ) + return responseWriter.Close() } func (g *generator) generateCode( @@ -155,6 +138,7 @@ func (g *generator) generateCode( inputImage bufimage.Image, baseOutDir string, pluginConfigs []bufconfig.GeneratePluginConfig, + responseWriter bufprotopluginos.ResponseWriter, includeImportsOverride *bool, includeWellKnownTypesOverride *bool, ) error { @@ -170,11 +154,6 @@ func (g *generator) generateCode( return err } // Apply the CodeGeneratorResponses in the order they were specified. - responseWriter := bufprotopluginos.NewResponseWriter( - g.logger, - g.storageosProvider, - bufprotopluginos.ResponseWriterWithCreateOutDirIfNotExists(), - ) for i, pluginConfig := range pluginConfigs { out := pluginConfig.Out() if baseOutDir != "" && baseOutDir != "." { @@ -192,9 +171,6 @@ func (g *generator) generateCode( return fmt.Errorf("plugin %s: %v", pluginConfig.Name(), err) } } - if err := responseWriter.Close(); err != nil { - return err - } return nil } diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/bufprotopluginos.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/bufprotopluginos.go index 4d4092caef..4e07881a0c 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/bufprotopluginos.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/bufprotopluginos.go @@ -67,15 +67,14 @@ func ResponseWriterWithCreateOutDirIfNotExists() ResponseWriterOption { } } -// Cleaner deletes output locations prior to generation. -// -// This must be done before any interaction with ResponseWriters, as multiple plugins may output to a single -// location. -type Cleaner interface { - DeleteOuts(ctx context.Context, pluginOuts []string) error -} - -// NewCleaner returns a new Cleaner. -func NewCleaner(storageosProvider storageos.Provider) Cleaner { - return newCleaner(storageosProvider) +// ResponseWriterWithDeleteOuts returns a ResponseWriterOption that deletes files +// on Close that were not written during generation. For directory outputs, any +// file on disk that was not part of the generated output is deleted after all +// new content is written, and empty directories are removed. For zip/jar outputs, +// the file is only rewritten when the generated content differs from what is +// already on disk. +func ResponseWriterWithDeleteOuts() ResponseWriterOption { + return func(responseWriterOptions *responseWriterOptions) { + responseWriterOptions.deleteOuts = true + } } diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/cleaner.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/cleaner.go deleted file mode 100644 index dda9b09bd0..0000000000 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/cleaner.go +++ /dev/null @@ -1,124 +0,0 @@ -// Copyright 2020-2026 Buf Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package bufprotopluginos - -import ( - "context" - "errors" - "io/fs" - "path/filepath" - - "buf.build/go/standard/xpath/xfilepath" - "github.com/bufbuild/buf/private/pkg/normalpath" - "github.com/bufbuild/buf/private/pkg/osext" - "github.com/bufbuild/buf/private/pkg/storage/storageos" - "github.com/bufbuild/buf/private/pkg/syserror" -) - -type cleaner struct { - storageosProvider storageos.Provider -} - -func newCleaner( - storageosProvider storageos.Provider, -) *cleaner { - return &cleaner{ - storageosProvider: storageosProvider, - } -} - -func (c *cleaner) DeleteOuts( - ctx context.Context, - pluginOuts []string, -) error { - pwd, err := osext.Getwd() - if err != nil { - return err - } - pwd, err = reallyCleanPath(pwd) - if err != nil { - return err - } - for _, pluginOut := range pluginOuts { - if err := validatePluginOut(pwd, pluginOut); err != nil { - return err - } - } - for _, pluginOut := range pluginOuts { - if err := c.deleteOut(ctx, pluginOut); err != nil { - return err - } - } - return nil -} - -func (c *cleaner) deleteOut( - ctx context.Context, - pluginOut string, -) error { - dirPath := pluginOut - removePath := "." - switch filepath.Ext(pluginOut) { - case ".jar", ".zip": - dirPath = normalpath.Dir(pluginOut) - removePath = normalpath.Base(pluginOut) - default: - // Assume output is a directory. - } - bucket, err := c.storageosProvider.NewReadWriteBucket( - dirPath, - storageos.ReadWriteBucketWithSymlinksIfSupported(), - ) - if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil - } - return err - } - return bucket.DeleteAll(ctx, removePath) -} - -func validatePluginOut(pwd string, pluginOut string) error { - if pluginOut == "" { - // This is just triple-making sure. - return syserror.New("got empty pluginOut in bufprotopluginos.Cleaner") - } - if pluginOut == "." { - // This is just a really defensive safety check. We can't see a reason you'd want to delete - // your current working directory other than something like a (cd proto && buf generate), so - // until and unless someone complains, we're just going to outlaw this. - return errors.New("cannot use --clean if your plugin will output to the current directory") - } - cleanedPluginOut, err := reallyCleanPath(pluginOut) - if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil - } - return err - } - if cleanedPluginOut == pwd { - // Same thing, more defense for now. - return errors.New("cannot use --clean if your plugin will output to the current directory") - } - return nil -} - -func reallyCleanPath(path string) (string, error) { - path, err := xfilepath.RealClean(path) - if err != nil { - return "", err - } - return filepath.EvalSymlinks(path) -} diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go index ec4ff29360..766d9d70d0 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go @@ -19,17 +19,21 @@ import ( "context" "errors" "fmt" + "io/fs" "log/slog" "os" "path/filepath" "sync" + "buf.build/go/standard/xpath/xfilepath" "github.com/bufbuild/buf/private/bufpkg/bufprotoplugin" "github.com/bufbuild/buf/private/pkg/normalpath" + "github.com/bufbuild/buf/private/pkg/osext" "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/storage/storagearchive" "github.com/bufbuild/buf/private/pkg/storage/storagemem" "github.com/bufbuild/buf/private/pkg/storage/storageos" + "github.com/bufbuild/buf/private/pkg/syserror" "github.com/bufbuild/buf/private/pkg/thread" "google.golang.org/protobuf/types/pluginpb" ) @@ -49,6 +53,9 @@ type responseWriter struct { responseWriter bufprotoplugin.ResponseWriter // If set, create directories if they don't already exist. createOutDirIfNotExists bool + // If set, delete files from output directories that were not written + // during generation. + deleteOuts bool // Cache the readWriteBuckets by their respective output paths. // These builders are transformed to storage.ReadBuckets and written // to disk once the responseWriter is flushed. @@ -86,6 +93,7 @@ func newResponseWriter( storageosProvider: storageosProvider, responseWriter: bufprotoplugin.NewResponseWriter(logger), createOutDirIfNotExists: responseWriterOptions.createOutDirIfNotExists, + deleteOuts: responseWriterOptions.deleteOuts, readWriteBuckets: make(map[string]storage.ReadWriteBucket), } } @@ -121,6 +129,27 @@ func (w *responseWriter) AddResponse( func (w *responseWriter) Close() error { w.lock.Lock() defer w.lock.Unlock() + // When deleteOuts is enabled, collect the set of generated paths per + // directory output before writing anything to disk. We do this first so + // that the delete phase (after writes) knows exactly which files to keep. + var dirOutputPaths map[string]map[string]struct{} + if w.deleteOuts { + dirOutputPaths = make(map[string]map[string]struct{}, len(w.readWriteBuckets)) + for outPath, readWriteBucket := range w.readWriteBuckets { + if isArchivePath(outPath) { + continue + } + paths, err := storage.AllPaths(context.Background(), readWriteBucket, "") + if err != nil { + return err + } + pathSet := make(map[string]struct{}, len(paths)) + for _, path := range paths { + pathSet[path] = struct{}{} + } + dirOutputPaths[outPath] = pathSet + } + } for _, closeFunc := range w.closers { if err := closeFunc(); err != nil { // Although unlikely, if an error happens here, @@ -135,6 +164,15 @@ func (w *responseWriter) Close() error { // Re-initialize the cached values to be safe. w.readWriteBuckets = make(map[string]storage.ReadWriteBucket) w.closers = nil + if !w.deleteOuts { + return nil + } + // Run cleanup. Delete stale files and remove empty directories. + for outDirPath, retainPaths := range dirOutputPaths { + if err := w.deleteStaleFilesAndEmptyDirs(outDirPath, retainPaths); err != nil { + return err + } + } return nil } @@ -144,6 +182,15 @@ func (w *responseWriter) addResponse( pluginOut string, createOutDirIfNotExists bool, ) error { + // Validate on the first time we see each output path when deleteOuts is + // enabled, before committing to any destructive operations. + if w.deleteOuts { + if _, seen := w.readWriteBuckets[pluginOut]; !seen { + if err := w.validateDeleteOutPath(pluginOut); err != nil { + return err + } + } + } switch filepath.Ext(pluginOut) { case ".jar": return w.writeZip( @@ -177,7 +224,7 @@ func (w *responseWriter) writeZip( outFilePath string, includeManifest bool, createOutDirIfNotExists bool, -) (retErr error) { +) error { outDirPath := filepath.Dir(outFilePath) if readWriteBucket, ok := w.readWriteBuckets[outFilePath]; ok { // We already have a readWriteBucket for this outFilePath, so @@ -225,18 +272,21 @@ func (w *responseWriter) writeZip( // Add this readWriteBucket to the set so that other plugins // can write to the same files (re: insertion points). w.readWriteBuckets[outFilePath] = readWriteBucket - w.closers = append(w.closers, func() (retErr error) { - // We're done writing all of the content into this - // readWriteBucket, so we zip it when we flush. - file, err := os.Create(outFilePath) - if err != nil { + w.closers = append(w.closers, func() error { + // Zip the generated content into a buffer so we can compare it with + // the existing file before deciding whether to write. This preserves + // the modification time when the output is unchanged. + var buf bytes.Buffer + // protoc does not compress. + if err := storagearchive.Zip(ctx, readWriteBucket, &buf, false); err != nil { return err } - defer func() { - retErr = errors.Join(retErr, file.Close()) - }() - // protoc does not compress. - return storagearchive.Zip(ctx, readWriteBucket, file, false) + newContent := buf.Bytes() + existingContent, err := os.ReadFile(outFilePath) + if err == nil && bytes.Equal(existingContent, newContent) { + return nil + } + return os.WriteFile(outFilePath, newContent, 0600) }) return nil } @@ -325,10 +375,129 @@ func (w *responseWriter) copySkipUnchanged( return thread.Parallelize(ctx, jobs) } +// deleteStaleFilesAndEmptyDirs deletes files present in outDirPath that are +// not in retainPaths, then removes any directories that are now empty. +func (w *responseWriter) deleteStaleFilesAndEmptyDirs( + outDirPath string, + retainPaths map[string]struct{}, +) error { + ctx := context.Background() + osReadWriteBucket, err := w.storageosProvider.NewReadWriteBucket( + outDirPath, + storageos.ReadWriteBucketWithSymlinksIfSupported(), + ) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + // Output directory doesn't exist; nothing to delete. + return nil + } + return err + } + existingPaths, err := storage.AllPaths(ctx, osReadWriteBucket, "") + if err != nil { + return err + } + var deleteJobs []func(context.Context) error + for _, existingPath := range existingPaths { + if _, ok := retainPaths[existingPath]; !ok { + deleteJobs = append(deleteJobs, func(ctx context.Context) error { + w.logger.DebugContext(ctx, "deleting stale generated file", slog.String("path", existingPath)) + if err := osReadWriteBucket.Delete(ctx, existingPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + return nil + }) + } + } + if err := thread.Parallelize(ctx, deleteJobs); err != nil { + return err + } + return removeEmptyDirs(outDirPath) +} + +// removeEmptyDirs recursively removes all empty directories under rootDir. +// It processes children before parents so that a chain of directories that +// are empty after their children are removed will be fully cleaned up. +// The rootDir itself is never removed. +// +// This operates directly on the filesystem because the storage abstraction +// only models files, not directories. +func removeEmptyDirs(rootDir string) error { + entries, err := os.ReadDir(rootDir) + if err != nil { + return err + } + for _, entry := range entries { + if entry.IsDir() { + childDir := filepath.Join(rootDir, entry.Name()) + if err := removeEmptyDirs(childDir); err != nil { + return err + } + // Re-check after recursing into children: the child directory + // may now be empty if all its contents were removed. + childEntries, err := os.ReadDir(childDir) + if err != nil { + // Directory was already removed or is inaccessible. + continue + } + if len(childEntries) == 0 { + if err := os.Remove(childDir); err != nil && !os.IsNotExist(err) { + return err + } + } + } + } + return nil +} + +// validateDeleteOutPath checks that absOutPath is safe to delete from. +// It prevents accidentally deleting files from the current working directory, +// which could happen if a user points --out at ".". +func (w *responseWriter) validateDeleteOutPath(absOutPath string) error { + if absOutPath == "" { + return syserror.New("got empty pluginOut in bufprotopluginos.ResponseWriter") + } + pwd, err := osext.Getwd() + if err != nil { + return err + } + resolvedPwd, err := resolveCleanPath(pwd) + if err != nil { + return err + } + resolvedOut, err := resolveCleanPath(absOutPath) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err + } + if resolvedOut == resolvedPwd { + return errors.New("cannot use --clean if your plugin will output to the current directory") + } + return nil +} + type responseWriterOptions struct { createOutDirIfNotExists bool + deleteOuts bool } func newResponseWriterOptions() *responseWriterOptions { return &responseWriterOptions{} } + +// resolveCleanPath returns the real, cleaned absolute path, following symlinks. +func resolveCleanPath(path string) (string, error) { + path, err := xfilepath.RealClean(path) + if err != nil { + return "", err + } + return filepath.EvalSymlinks(path) +} + +// isArchivePath returns true if the given path has a .zip or .jar extension. +func isArchivePath(path string) bool { + ext := filepath.Ext(path) + return ext == ".zip" || ext == ".jar" +} diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go index 6126c9ac2a..2c8dd3325c 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go @@ -35,7 +35,7 @@ func TestResponseWriterSkipsUnchangedFile(t *testing.T) { past := time.Now().Add(-time.Hour) require.NoError(t, os.Chtimes(filePath, past, past)) - runResponseWriter(t, outDir, newResponseFile("foo.go", content)) + runResponseWriter(t, outDir, false, newResponseFile("foo.go", content)) info, err := os.Stat(filePath) require.NoError(t, err) @@ -51,7 +51,7 @@ func TestResponseWriterWritesChangedFile(t *testing.T) { require.NoError(t, os.Chtimes(filePath, past, past)) newContent := "package new\n" - runResponseWriter(t, outDir, newResponseFile("foo.go", newContent)) + runResponseWriter(t, outDir, false, newResponseFile("foo.go", newContent)) data, err := os.ReadFile(filePath) require.NoError(t, err) @@ -64,13 +64,12 @@ func TestResponseWriterWritesChangedFile(t *testing.T) { func TestResponseWriterWritesNewFile(t *testing.T) { t.Parallel() outDir := t.TempDir() - content := "package foo\n" - runResponseWriter(t, outDir, newResponseFile("foo.go", content)) + runResponseWriter(t, outDir, false, newResponseFile("foo.go", "package foo\n")) data, err := os.ReadFile(filepath.Join(outDir, "foo.go")) require.NoError(t, err) - require.Equal(t, content, string(data)) + require.Equal(t, "package foo\n", string(data)) } func TestResponseWriterMixedFiles(t *testing.T) { @@ -79,23 +78,92 @@ func TestResponseWriterMixedFiles(t *testing.T) { unchangedContent := "package unchanged\n" unchangedPath := filepath.Join(outDir, "unchanged.go") changedPath := filepath.Join(outDir, "changed.go") - newPath := filepath.Join(outDir, "new.go") require.NoError(t, os.WriteFile(unchangedPath, []byte(unchangedContent), 0600)) require.NoError(t, os.WriteFile(changedPath, []byte("package old\n"), 0600)) past := time.Now().Add(-time.Hour) require.NoError(t, os.Chtimes(unchangedPath, past, past)) require.NoError(t, os.Chtimes(changedPath, past, past)) - runResponseWriter(t, outDir, + runResponseWriter(t, outDir, false, + newResponseFile("unchanged.go", unchangedContent), + newResponseFile("changed.go", "package changed\n"), + newResponseFile("new.go", "package new\n"), + ) + + unchangedInfo, err := os.Stat(unchangedPath) + require.NoError(t, err) + require.Equal(t, past.Truncate(time.Second), unchangedInfo.ModTime().Truncate(time.Second)) + + changedData, err := os.ReadFile(changedPath) + require.NoError(t, err) + require.Equal(t, "package changed\n", string(changedData)) + changedInfo, err := os.Stat(changedPath) + require.NoError(t, err) + require.Greater(t, changedInfo.ModTime(), past) + + newData, err := os.ReadFile(filepath.Join(outDir, "new.go")) + require.NoError(t, err) + require.Equal(t, "package new\n", string(newData)) +} + +func TestResponseWriterSmartCleanDeletesStaleFile(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + stalePath := filepath.Join(outDir, "stale.go") + require.NoError(t, os.WriteFile(stalePath, []byte("package stale\n"), 0600)) + + // Generate only foo.go; stale.go should be deleted. + runResponseWriter(t, outDir, true, newResponseFile("foo.go", "package foo\n")) + + _, err := os.Stat(stalePath) + require.ErrorIs(t, err, os.ErrNotExist) + data, err := os.ReadFile(filepath.Join(outDir, "foo.go")) + require.NoError(t, err) + require.Equal(t, "package foo\n", string(data)) +} + +func TestResponseWriterSmartCleanPreservesMtimeForUnchanged(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + content := "package foo\n" + filePath := filepath.Join(outDir, "foo.go") + require.NoError(t, os.WriteFile(filePath, []byte(content), 0600)) + past := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(filePath, past, past)) + + runResponseWriter(t, outDir, true, newResponseFile("foo.go", content)) + + info, err := os.Stat(filePath) + require.NoError(t, err) + require.Equal(t, past.Truncate(time.Second), info.ModTime().Truncate(time.Second)) +} + +func TestResponseWriterSmartCleanMixedFiles(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + unchangedContent := "package unchanged\n" + unchangedPath := filepath.Join(outDir, "unchanged.go") + changedPath := filepath.Join(outDir, "changed.go") + stalePath := filepath.Join(outDir, "stale.go") + require.NoError(t, os.WriteFile(unchangedPath, []byte(unchangedContent), 0600)) + require.NoError(t, os.WriteFile(changedPath, []byte("package old\n"), 0600)) + require.NoError(t, os.WriteFile(stalePath, []byte("package stale\n"), 0600)) + past := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(unchangedPath, past, past)) + require.NoError(t, os.Chtimes(changedPath, past, past)) + + runResponseWriter(t, outDir, true, newResponseFile("unchanged.go", unchangedContent), newResponseFile("changed.go", "package changed\n"), newResponseFile("new.go", "package new\n"), ) + // Unchanged: mtime preserved. unchangedInfo, err := os.Stat(unchangedPath) require.NoError(t, err) require.Equal(t, past.Truncate(time.Second), unchangedInfo.ModTime().Truncate(time.Second)) + // Changed: new content, updated mtime. changedData, err := os.ReadFile(changedPath) require.NoError(t, err) require.Equal(t, "package changed\n", string(changedData)) @@ -103,22 +171,116 @@ func TestResponseWriterMixedFiles(t *testing.T) { require.NoError(t, err) require.Greater(t, changedInfo.ModTime(), past) - newData, err := os.ReadFile(newPath) + // New: created. + newData, err := os.ReadFile(filepath.Join(outDir, "new.go")) require.NoError(t, err) require.Equal(t, "package new\n", string(newData)) + + // Stale: deleted. + _, err = os.Stat(stalePath) + require.ErrorIs(t, err, os.ErrNotExist) +} + +func TestResponseWriterZipPreservesMtimeWhenUnchanged(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + outFile := filepath.Join(outDir, "output.zip") + + // First run creates the zip. + runResponseWriter(t, outFile, false, newResponseFile("foo.go", "package foo\n")) + require.FileExists(t, outFile) + + past := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(outFile, past, past)) + + // Second run with identical content should not rewrite the zip. + runResponseWriter(t, outFile, false, newResponseFile("foo.go", "package foo\n")) + + info, err := os.Stat(outFile) + require.NoError(t, err) + require.Equal(t, past.Truncate(time.Second), info.ModTime().Truncate(time.Second)) } -func runResponseWriter(t *testing.T, outDir string, files ...*pluginpb.CodeGeneratorResponse_File) { +func TestResponseWriterZipUpdatesWhenChanged(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + outFile := filepath.Join(outDir, "output.zip") + + runResponseWriter(t, outFile, false, newResponseFile("foo.go", "package foo\n")) + + past := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(outFile, past, past)) + + // Second run with different content should rewrite. + runResponseWriter(t, outFile, false, newResponseFile("foo.go", "package bar\n")) + + info, err := os.Stat(outFile) + require.NoError(t, err) + require.Greater(t, info.ModTime(), past) +} + +func TestResponseWriterSmartCleanRemovesEmptyDirs(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + subDir := filepath.Join(outDir, "subpkg") + require.NoError(t, os.MkdirAll(subDir, 0755)) + // Pre-existing file in a subdirectory that will become stale. + require.NoError(t, os.WriteFile(filepath.Join(subDir, "stale.go"), []byte("package stale\n"), 0600)) + + // Generate only to the root dir; nothing goes into subpkg. + runResponseWriter(t, outDir, true, newResponseFile("foo.go", "package foo\n")) + + // stale.go deleted, subpkg now empty and also removed. + _, err := os.Stat(subDir) + require.ErrorIs(t, err, os.ErrNotExist) +} + +func TestResponseWriterSmartCleanRemovesNestedEmptyDirs(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + // Create a/b/c/stale.go - all three directories should be removed once + // stale.go is deleted, because each parent becomes empty after its child + // is removed. + require.NoError(t, os.MkdirAll(filepath.Join(outDir, "a", "b", "c"), 0755)) + require.NoError(t, os.WriteFile(filepath.Join(outDir, "a", "b", "c", "stale.go"), []byte("package stale\n"), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(outDir, "a", "b", "kept.go"), []byte("package kept\n"), 0600)) + // a/d is a pre-existing empty directory with no files. + require.NoError(t, os.MkdirAll(filepath.Join(outDir, "a", "d"), 0755)) + + runResponseWriter(t, outDir, true, + newResponseFile("foo.go", "package foo\n"), + newResponseFile("a/b/kept.go", "package kept\n"), + ) + + // a/b/c removed (stale file deleted, dir now empty). + _, err := os.Stat(filepath.Join(outDir, "a", "b", "c")) + require.ErrorIs(t, err, os.ErrNotExist) + // a/d removed (pre-existing empty directory). + _, err = os.Stat(filepath.Join(outDir, "a", "d")) + require.ErrorIs(t, err, os.ErrNotExist) + // a/b still present because a/b/kept.go is generated output. + require.FileExists(t, filepath.Join(outDir, "a", "b", "kept.go")) + require.DirExists(t, filepath.Join(outDir, "a", "b")) + require.DirExists(t, filepath.Join(outDir, "a")) +} + +func runResponseWriter(t *testing.T, outPath string, deleteOuts bool, files ...*pluginpb.CodeGeneratorResponse_File) { t.Helper() + opts := []ResponseWriterOption{ + ResponseWriterWithCreateOutDirIfNotExists(), + } + if deleteOuts { + opts = append(opts, ResponseWriterWithDeleteOuts()) + } writer := NewResponseWriter( slogtestext.NewLogger(t), storageos.NewProvider(), - ResponseWriterWithCreateOutDirIfNotExists(), + opts..., ) require.NoError(t, writer.AddResponse( t.Context(), &pluginpb.CodeGeneratorResponse{File: files}, - outDir, + outPath, )) require.NoError(t, writer.Close()) } From 78ace1f13b7ab49fa57ab859e8b9a92a8ed9c5e0 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Tue, 24 Mar 2026 21:29:20 +0100 Subject: [PATCH 2/4] Test and cleanup --- .../command/generate/generate_test.go | 2 +- .../bufprotopluginos/response_writer.go | 15 +++---- .../bufprotopluginos/response_writer_test.go | 43 +++++++++++++++++++ 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/cmd/buf/internal/command/generate/generate_test.go b/cmd/buf/internal/command/generate/generate_test.go index b699dccc1d..4a88e931c9 100644 --- a/cmd/buf/internal/command/generate/generate_test.go +++ b/cmd/buf/internal/command/generate/generate_test.go @@ -384,7 +384,7 @@ func TestSmartCleanPreservesMtime(t *testing.T) { past := time.Now().Add(-time.Hour) require.NoError(t, os.Chtimes(outFile, past, past)) - testRunSuccess(t, "--output", tempDirPath, "--template", template, "--clean", input) + testRunSuccess(t, "--output", tempDirPath, "--template", template, input) info, err := os.Stat(outFile) require.NoError(t, err) diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go index 766d9d70d0..970f07b39d 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go @@ -33,7 +33,6 @@ import ( "github.com/bufbuild/buf/private/pkg/storage/storagearchive" "github.com/bufbuild/buf/private/pkg/storage/storagemem" "github.com/bufbuild/buf/private/pkg/storage/storageos" - "github.com/bufbuild/buf/private/pkg/syserror" "github.com/bufbuild/buf/private/pkg/thread" "google.golang.org/protobuf/types/pluginpb" ) @@ -437,8 +436,10 @@ func removeEmptyDirs(rootDir string) error { // may now be empty if all its contents were removed. childEntries, err := os.ReadDir(childDir) if err != nil { - // Directory was already removed or is inaccessible. - continue + if errors.Is(err, fs.ErrNotExist) { + continue + } + return err } if len(childEntries) == 0 { if err := os.Remove(childDir); err != nil && !os.IsNotExist(err) { @@ -450,13 +451,11 @@ func removeEmptyDirs(rootDir string) error { return nil } -// validateDeleteOutPath checks that absOutPath is safe to delete from. +// validateDeleteOutPath checks that the output path is safe to delete from. // It prevents accidentally deleting files from the current working directory, -// which could happen if a user points --out at ".". +// which could happen if a user configures out as ".". +// The path is already absolute (via filepath.Abs in AddResponse). func (w *responseWriter) validateDeleteOutPath(absOutPath string) error { - if absOutPath == "" { - return syserror.New("got empty pluginOut in bufprotopluginos.ResponseWriter") - } pwd, err := osext.Getwd() if err != nil { return err diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go index 2c8dd3325c..a4ddbe6f40 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go @@ -264,6 +264,49 @@ func TestResponseWriterSmartCleanRemovesNestedEmptyDirs(t *testing.T) { require.DirExists(t, filepath.Join(outDir, "a")) } +func TestResponseWriterSmartCleanMultiplePluginsSameOutDir(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + // Pre-populate the output directory with a stale file that neither plugin will write. + stalePath := filepath.Join(outDir, "stale.go") + require.NoError(t, os.WriteFile(stalePath, []byte("package stale\n"), 0600)) + + writer := NewResponseWriter( + slogtestext.NewLogger(t), + storageos.NewProvider(), + ResponseWriterWithCreateOutDirIfNotExists(), + ResponseWriterWithDeleteOuts(), + ) + // First plugin writes foo.go. + require.NoError(t, writer.AddResponse( + t.Context(), + &pluginpb.CodeGeneratorResponse{File: []*pluginpb.CodeGeneratorResponse_File{ + newResponseFile("foo.go", "package foo\n"), + }}, + outDir, + )) + // Second plugin writes bar.go to the same directory. + require.NoError(t, writer.AddResponse( + t.Context(), + &pluginpb.CodeGeneratorResponse{File: []*pluginpb.CodeGeneratorResponse_File{ + newResponseFile("bar.go", "package bar\n"), + }}, + outDir, + )) + require.NoError(t, writer.Close()) + + // Stale file must be deleted. + _, err := os.Stat(stalePath) + require.ErrorIs(t, err, os.ErrNotExist) + // Both plugin outputs must exist. + fooData, err := os.ReadFile(filepath.Join(outDir, "foo.go")) + require.NoError(t, err) + require.Equal(t, "package foo\n", string(fooData)) + barData, err := os.ReadFile(filepath.Join(outDir, "bar.go")) + require.NoError(t, err) + require.Equal(t, "package bar\n", string(barData)) +} + func runResponseWriter(t *testing.T, outPath string, deleteOuts bool, files ...*pluginpb.CodeGeneratorResponse_File) { t.Helper() opts := []ResponseWriterOption{ From 4924a1424aca837fb9363eb6f5795b15e91d2384 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Wed, 25 Mar 2026 19:53:23 +0100 Subject: [PATCH 3/4] Close ctx, file perm, ext consts --- .../internal/command/alpha/protoc/protoc.go | 2 +- private/buf/bufgen/generator.go | 2 +- .../bufprotopluginos/bufprotopluginos.go | 9 +-- .../bufprotopluginos/response_writer.go | 56 ++++++++++--------- .../bufprotopluginos/response_writer_test.go | 4 +- 5 files changed, 39 insertions(+), 34 deletions(-) diff --git a/cmd/buf/internal/command/alpha/protoc/protoc.go b/cmd/buf/internal/command/alpha/protoc/protoc.go index 8c5daea9bb..27321487ed 100644 --- a/cmd/buf/internal/command/alpha/protoc/protoc.go +++ b/cmd/buf/internal/command/alpha/protoc/protoc.go @@ -219,7 +219,7 @@ func run( return err } } - if err := responseWriter.Close(); err != nil { + if err := responseWriter.Close(ctx); err != nil { return err } return nil diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 71935540ef..283f85da5f 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -129,7 +129,7 @@ func (g *generator) Generate( return err } } - return responseWriter.Close() + return responseWriter.Close(ctx) } func (g *generator) generateCode( diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/bufprotopluginos.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/bufprotopluginos.go index 4e07881a0c..362ec10180 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/bufprotopluginos.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/bufprotopluginos.go @@ -17,7 +17,6 @@ package bufprotopluginos import ( "context" - "io" "log/slog" "github.com/bufbuild/buf/private/pkg/storage/storageos" @@ -26,9 +25,11 @@ import ( // ResponseWriter writes CodeGeneratorResponses to the OS filesystem. type ResponseWriter interface { - // Close writes all of the responses to disk. No further calls can be - // made to the ResponseWriter after this call. - io.Closer + // Close writes all of the responses to disk and, when + // ResponseWriterWithDeleteOuts is enabled, removes stale files from + // output directories. No further calls can be made to the + // ResponseWriter after this call. + Close(ctx context.Context) error // AddResponse adds the response to the writer, switching on the file extension. // If there is a .jar extension, this generates a jar. If there is a .zip diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go index 970f07b39d..0b65976e77 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go @@ -37,6 +37,11 @@ import ( "google.golang.org/protobuf/types/pluginpb" ) +const ( + jarExt = ".jar" + zipExt = ".zip" +) + // Constants used to create .jar files. var ( manifestPath = normalpath.Join("META-INF", "MANIFEST.MF") @@ -74,7 +79,7 @@ type responseWriter struct { // Cache the functions used to flush all of the responses to disk. // This holds all of the buckets in-memory so that we only write // the results to disk if all of the responses are successful. - closers []func() error + closers []func(ctx context.Context) error lock sync.RWMutex } @@ -125,12 +130,22 @@ func (w *responseWriter) AddResponse( ) } -func (w *responseWriter) Close() error { +func (w *responseWriter) Close(ctx context.Context) error { w.lock.Lock() defer w.lock.Unlock() - // When deleteOuts is enabled, collect the set of generated paths per - // directory output before writing anything to disk. We do this first so - // that the delete phase (after writes) knows exactly which files to keep. + for _, closeFunc := range w.closers { + if err := closeFunc(ctx); err != nil { + // Although unlikely, if an error happens here, + // some generated files could be written to disk, + // whereas others aren't. + // + // Regardless, we stop at the first error so that + // we don't unnecessarily write more results. + return err + } + } + // Collect the set of generated paths per directory output before + // clearing state, so the delete phase knows which files to keep. var dirOutputPaths map[string]map[string]struct{} if w.deleteOuts { dirOutputPaths = make(map[string]map[string]struct{}, len(w.readWriteBuckets)) @@ -138,7 +153,7 @@ func (w *responseWriter) Close() error { if isArchivePath(outPath) { continue } - paths, err := storage.AllPaths(context.Background(), readWriteBucket, "") + paths, err := storage.AllPaths(ctx, readWriteBucket, "") if err != nil { return err } @@ -149,26 +164,15 @@ func (w *responseWriter) Close() error { dirOutputPaths[outPath] = pathSet } } - for _, closeFunc := range w.closers { - if err := closeFunc(); err != nil { - // Although unlikely, if an error happens here, - // some generated files could be written to disk, - // whereas others aren't. - // - // Regardless, we stop at the first error so that - // we don't unnecessarily write more results. - return err - } - } // Re-initialize the cached values to be safe. w.readWriteBuckets = make(map[string]storage.ReadWriteBucket) w.closers = nil if !w.deleteOuts { return nil } - // Run cleanup. Delete stale files and remove empty directories. + // Delete stale files and remove empty directories. for outDirPath, retainPaths := range dirOutputPaths { - if err := w.deleteStaleFilesAndEmptyDirs(outDirPath, retainPaths); err != nil { + if err := w.deleteStaleFilesAndEmptyDirs(ctx, outDirPath, retainPaths); err != nil { return err } } @@ -191,7 +195,7 @@ func (w *responseWriter) addResponse( } } switch filepath.Ext(pluginOut) { - case ".jar": + case jarExt: return w.writeZip( ctx, response, @@ -199,7 +203,7 @@ func (w *responseWriter) addResponse( true, createOutDirIfNotExists, ) - case ".zip": + case zipExt: return w.writeZip( ctx, response, @@ -271,7 +275,7 @@ func (w *responseWriter) writeZip( // Add this readWriteBucket to the set so that other plugins // can write to the same files (re: insertion points). w.readWriteBuckets[outFilePath] = readWriteBucket - w.closers = append(w.closers, func() error { + w.closers = append(w.closers, func(ctx context.Context) error { // Zip the generated content into a buffer so we can compare it with // the existing file before deciding whether to write. This preserves // the modification time when the output is unchanged. @@ -285,7 +289,7 @@ func (w *responseWriter) writeZip( if err == nil && bytes.Equal(existingContent, newContent) { return nil } - return os.WriteFile(outFilePath, newContent, 0600) + return os.WriteFile(outFilePath, newContent, 0666) }) return nil } @@ -321,7 +325,7 @@ func (w *responseWriter) writeDirectory( // Add this readWriteBucket to the set so that other plugins // can write to the same files (re: insertion points). w.readWriteBuckets[outDirPath] = readWriteBucket - w.closers = append(w.closers, func() error { + w.closers = append(w.closers, func(ctx context.Context) error { if createOutDirIfNotExists { if err := os.MkdirAll(outDirPath, 0755); err != nil { return err @@ -377,10 +381,10 @@ func (w *responseWriter) copySkipUnchanged( // deleteStaleFilesAndEmptyDirs deletes files present in outDirPath that are // not in retainPaths, then removes any directories that are now empty. func (w *responseWriter) deleteStaleFilesAndEmptyDirs( + ctx context.Context, outDirPath string, retainPaths map[string]struct{}, ) error { - ctx := context.Background() osReadWriteBucket, err := w.storageosProvider.NewReadWriteBucket( outDirPath, storageos.ReadWriteBucketWithSymlinksIfSupported(), @@ -498,5 +502,5 @@ func resolveCleanPath(path string) (string, error) { // isArchivePath returns true if the given path has a .zip or .jar extension. func isArchivePath(path string) bool { ext := filepath.Ext(path) - return ext == ".zip" || ext == ".jar" + return ext == zipExt || ext == jarExt } diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go index a4ddbe6f40..bffdfc08ea 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer_test.go @@ -293,7 +293,7 @@ func TestResponseWriterSmartCleanMultiplePluginsSameOutDir(t *testing.T) { }}, outDir, )) - require.NoError(t, writer.Close()) + require.NoError(t, writer.Close(t.Context())) // Stale file must be deleted. _, err := os.Stat(stalePath) @@ -325,7 +325,7 @@ func runResponseWriter(t *testing.T, outPath string, deleteOuts bool, files ...* &pluginpb.CodeGeneratorResponse{File: files}, outPath, )) - require.NoError(t, writer.Close()) + require.NoError(t, writer.Close(t.Context())) } func newResponseFile(name, content string) *pluginpb.CodeGeneratorResponse_File { From bb3d4907f108f6c8cc712eaacf3d8af367eb76b0 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Wed, 25 Mar 2026 20:15:19 +0100 Subject: [PATCH 4/4] Use os.Create --- .../bufprotoplugin/bufprotopluginos/response_writer.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go index 0b65976e77..1bdf0c5ddc 100644 --- a/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go +++ b/private/bufpkg/bufprotoplugin/bufprotopluginos/response_writer.go @@ -289,7 +289,12 @@ func (w *responseWriter) writeZip( if err == nil && bytes.Equal(existingContent, newContent) { return nil } - return os.WriteFile(outFilePath, newContent, 0666) + file, err := os.Create(outFilePath) + if err != nil { + return err + } + _, writeErr := file.Write(newContent) + return errors.Join(writeErr, file.Close()) }) return nil }