fix: don't delete an existing conflist (#2115)

Author: thatmattlong

internal/fs/atomic.go

@@ -2,6 +2,7 @@ package fs
 
 import (
 	"io"
+	"io/fs"
 	"os"
 	"path"
 
@@ -15,19 +16,39 @@ type AtomicWriter struct {
 
 var _ io.WriteCloser = &AtomicWriter{}
 
+// NewAtomicWriter returns an io.WriteCloser that will write contents to a temp file and move that temp file to the destination filename. If the destination
+// filename already exists, this constructor will copy the file to <filename>-old, truncating that file if it already exists.
 func NewAtomicWriter(filename string) (*AtomicWriter, error) {
+	// if a file already exists, copy it to <filname>-old
 	exists := true
-	if _, err := os.Stat(filename); err != nil {
-		if os.IsNotExist(err) {
+	existingFile, err := os.Open(filename)
+	if err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
 			exists = false
 		} else {
-			return nil, errors.Wrap(err, "unable to stat existing file")
+			return nil, errors.Wrap(err, "error opening existing file")
 		}
 	}
 
 	if exists {
-		if err := os.Rename(filename, filename+"-old"); err != nil {
-			return nil, errors.Wrap(err, "unable to move existing file from destination")
+		// os.Create truncates existing files so we'll keep overwriting the <filename>-old and not filling up the disc if the
+		// process calls this over and over again on the same filename (e.g. if CNS uses this for conflist generation and keeps crashing and re-writing)
+		oldFilename := filename + "-old"
+		oldFile, createErr := os.Create(oldFilename)
+		if createErr != nil {
+			if closeErr := existingFile.Close(); closeErr != nil {
+				return nil, errors.Wrapf(createErr, "error closing file: [%v] occurred when handling file creation error", closeErr.Error())
+			}
+			return nil, errors.Wrapf(createErr, "error creating file %s", oldFilename)
+		}
+
+		// copy the existing file to <filename>-old
+		if _, err := io.Copy(oldFile, existingFile); err != nil { //nolint:govet // shadowing err is fine here since its encapsulated in the if block
+			return nil, errors.Wrapf(err, "error copying existing file %s to destination %s", existingFile.Name(), oldFile.Name())
+		}
+
+		if err := existingFile.Close(); err != nil { //nolint:govet // shadowing err is fine here since its encapsulated in the if block
+			return nil, errors.Wrapf(err, "error closing file %s", existingFile.Name())
 		}
 	}
 
@@ -39,18 +60,20 @@ func NewAtomicWriter(filename string) (*AtomicWriter, error) {
 	return &AtomicWriter{filename: filename, tempFile: tempFile}, nil
 }
 
+// Close closes the temp file handle and moves the temp file to the final destination
 func (a *AtomicWriter) Close() error {
 	if err := a.tempFile.Close(); err != nil {
-		return errors.Wrap(err, "unable to close temp file")
+		return errors.Wrapf(err, "unable to close temp file %s", a.tempFile.Name())
 	}
 
 	if err := os.Rename(a.tempFile.Name(), a.filename); err != nil {
-		return errors.Wrap(err, "unable to move temp file to destination")
+		return errors.Wrapf(err, "unable to move temp file %s to destination %s", a.tempFile.Name(), a.filename)
 	}
 
 	return nil
 }
 
+// Write writes the buffer to the temp file. You must call Close() to complete the move from temp file to dest file
 func (a *AtomicWriter) Write(p []byte) (int, error) {
 	bs, err := a.tempFile.Write(p)
 	return bs, errors.Wrap(err, "unable to write to temp file")

internal/fs/atmoic_test.go renamed to internal/fs/atomic_test.go

@@ -14,7 +14,9 @@ func TestAtomicWriterFileExists(t *testing.T) {
 	w, err := fs.NewAtomicWriter(file)
 	require.NoError(t, err, "error creating atomic writer")
 
-	// atomic writer should replace existing file with -old suffix
+	// atomic writer should copy existing file with -old suffix
+	_, err = os.Stat(file)
+	require.NoError(t, err, "error stating existing file")
 	_, err = os.Stat(file + "-old")
 	require.NoError(t, err, "error stating old file")