Write manifest file updates to a temp file first (#1442)

Author: dhurley

internal/file/file_operator.go

@@ -155,7 +155,7 @@ func (fo *FileOperator) ReadChunk(
 
 func (fo *FileOperator) WriteManifestFile(
 	ctx context.Context, updatedFiles map[string]*model.ManifestFile, manifestDir, manifestPath string,
-) (writeError error) {
+) error {
 	slog.DebugContext(ctx, "Writing manifest file", "updated_files", updatedFiles)
 	manifestJSON, err := json.MarshalIndent(updatedFiles, "", "  ")
 	if err != nil {
@@ -164,28 +164,50 @@ func (fo *FileOperator) WriteManifestFile(
 
 	fo.manifestLock.Lock()
 	defer fo.manifestLock.Unlock()
+
 	// 0755 allows read/execute for all, write for owner
 	if err = os.MkdirAll(manifestDir, dirPerm); err != nil {
 		return fmt.Errorf("unable to create directory %s: %w", manifestDir, err)
 	}
 
-	// 0600 ensures only root can read/write
-	newFile, err := os.OpenFile(manifestPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, filePerm)
+	// Write to a temporary file first to ensure atomicity
+	tempManifestFilePath := manifestPath + ".tmp"
+	tempFile, err := os.OpenFile(tempManifestFilePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, filePerm)
 	if err != nil {
-		return fmt.Errorf("failed to read manifest file: %w", err)
+		return fmt.Errorf("failed to open file %s: %w", tempManifestFilePath, err)
 	}
-	defer func() {
-		if closeErr := newFile.Close(); closeErr != nil {
-			writeError = closeErr
-		}
-	}()
 
-	_, err = newFile.Write(manifestJSON)
+	if _, err = tempFile.Write(manifestJSON); err != nil {
+		closeFile(ctx, tempFile)
+
+		return fmt.Errorf("failed to write to file %s: %w", tempManifestFilePath, err)
+	}
+
+	closeFile(ctx, tempFile)
+
+	// Verify the contents of the temporary file is JSON
+	file, err := os.ReadFile(tempManifestFilePath)
 	if err != nil {
-		return fmt.Errorf("failed to write manifest file: %w", err)
+		return fmt.Errorf("failed to read file %s: %w", tempManifestFilePath, err)
 	}
 
-	return writeError
+	var manifestFiles map[string]*model.ManifestFile
+
+	err = json.Unmarshal(file, &manifestFiles)
+	if err != nil {
+		if len(file) == 0 {
+			return fmt.Errorf("file %s is empty: %w", tempManifestFilePath, err)
+		}
+
+		return fmt.Errorf("failed to parse file %s: %w", tempManifestFilePath, err)
+	}
+
+	// Rename the temporary file to the actual manifest file path
+	if renameError := os.Rename(tempManifestFilePath, manifestPath); renameError != nil {
+		return fmt.Errorf("failed to file %s to %s: %w", tempManifestFilePath, manifestPath, renameError)
+	}
+
+	return nil
 }
 
 func (fo *FileOperator) MoveFile(ctx context.Context, sourcePath, destPath string) error {

internal/file/file_operator_test.go

@@ -7,6 +7,7 @@ package file
 
 import (
 	"context"
+	"encoding/json"
 	"os"
 	"path"
 	"path/filepath"
@@ -44,13 +45,89 @@ func TestFileOperator_Write(t *testing.T) {
 	assert.Equal(t, fileContent, data)
 }
 
+func TestFileOperator_WriteManifestFile(t *testing.T) {
+	tempDir := t.TempDir()
+	manifestPath := path.Join(tempDir, "manifest.json")
+
+	manifestFiles := map[string]*model.ManifestFile{
+		"/etc/nginx/nginx.conf": {
+			ManifestFileMeta: &model.ManifestFileMeta{
+				Name:       "/etc/nginx/nginx.conf",
+				Size:       1024,
+				Hash:       "6d232d32d44",
+				Referenced: true,
+				Unmanaged:  false,
+			},
+		},
+		"/etc/nginx/conf.d/default.conf": {
+			ManifestFileMeta: &model.ManifestFileMeta{
+				Name:       "/etc/nginx/conf.d/default.conf",
+				Size:       32342,
+				Hash:       "1eh32hd3792hd329",
+				Referenced: true,
+				Unmanaged:  false,
+			},
+		},
+	}
+
+	fileOperator := NewFileOperator(&sync.RWMutex{})
+	err := fileOperator.WriteManifestFile(t.Context(), manifestFiles, tempDir, manifestPath)
+	require.NoError(t, err)
+
+	assert.FileExists(t, manifestPath)
+	assert.NoFileExists(t, manifestPath+".tmp")
+
+	// Verify the contents can be read back
+	data, err := os.ReadFile(manifestPath)
+	require.NoError(t, err)
+
+	var readBack map[string]*model.ManifestFile
+	err = json.Unmarshal(data, &readBack)
+	require.NoError(t, err)
+	assert.Equal(t, manifestFiles, readBack)
+}
+
+func TestFileOperator_WriteManifestFile_directoryCreationError(t *testing.T) {
+	manifestPath := "/unknown/manifest.json"
+	manifestDir := "/unknown"
+
+	fileOperator := NewFileOperator(&sync.RWMutex{})
+	err := fileOperator.WriteManifestFile(t.Context(), make(map[string]*model.ManifestFile), manifestDir, manifestPath)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "unable to create directory")
+}
+
+func TestFileOperator_WriteManifestFile_tempFileCreationError(t *testing.T) {
+	tempDir := t.TempDir()
+
+	// Create a file where we want to write the manifest
+	manifestPath := path.Join(tempDir, "manifest.json")
+	err := os.WriteFile(manifestPath, []byte("existing"), 0o400) // readonly
+	require.NoError(t, err)
+
+	// Create readonly directory to prevent temp file creation
+	err = os.Chmod(tempDir, 0o444)
+	require.NoError(t, err)
+	defer func() {
+		revertPermissionsError := os.Chmod(tempDir, 0o755)
+		require.NoError(t, revertPermissionsError)
+	}()
+
+	fileOperator := NewFileOperator(&sync.RWMutex{})
+	err = fileOperator.WriteManifestFile(t.Context(), make(map[string]*model.ManifestFile), tempDir, manifestPath)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to open file")
+}
+
 func TestFileOperator_WriteManifestFile_fileMissing(t *testing.T) {
 	tempDir := t.TempDir()
 	manifestPath := "/unknown/manifest.json"
 
 	fileOperator := NewFileOperator(&sync.RWMutex{})
 	err := fileOperator.WriteManifestFile(t.Context(), make(map[string]*model.ManifestFile), tempDir, manifestPath)
-	assert.Error(t, err)
+	require.Error(t, err)
+
+	assert.NoFileExists(t, manifestPath+".tmp")
 }
 
 func TestFileOperator_MoveFile_fileExists(t *testing.T) {