Fix: resolve issue #4478 by using a temporary file for non-append writes

To address the issue where a failed write operation results in an empty file, we can use a temporary file for non-append writes. This ensures that the original file is only replaced once the new content is fully written and committed.

**Key Changes:**

1. **Temporary File Handling:**
   - For non-append writes, a temporary file is created in the same directory as the target file.
   - All write operations are performed on the temporary file first.

2. **Atomic Commit:**
   - The temporary file is only renamed to the target path during `Commit()`, ensuring atomic replacement.
   - If `Commit()` fails, the temporary file is cleaned up.

3. **Error Handling:**
   - `Cancel()` properly removes temporary files if the operation is aborted.
   - `Close()` is made idempotent to handle multiple calls safely.

4. **Data Integrity:**
   - Directory sync after rename ensures metadata persistence.
   - Proper file flushing and syncing before rename operations.

Signed-off-by: Oded Porat <onporat@gmail.com>

registry/storage/driver/filesystem/driver.go

@@ -180,29 +180,43 @@ func (d *driver) Writer(ctx context.Context, subPath string, append bool) (stora
 		return nil, err
 	}
 
-	fp, err := os.OpenFile(fullPath, os.O_WRONLY|os.O_CREATE, 0o666)
-	if err != nil {
-		return nil, err
-	}
-
-	var offset int64
+	var (
+		fp           *os.File
+		err          error
+		offset       int64
+		tempFilePath string
+	)
 
 	if !append {
-		err := fp.Truncate(0)
+		// Create temporary file in target directory
+		tempFile, err := os.CreateTemp(parentDir, ".tmp-")
 		if err != nil {
-			fp.Close()
 			return nil, err
 		}
+		tempFilePath = tempFile.Name()
+		tempFile.Close()
+
+		// Open temp file with truncation
+		fp, err = os.OpenFile(tempFilePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o666)
+		if err != nil {
+			os.Remove(tempFilePath)
+			return nil, err
+		}
+		offset = 0
 	} else {
-		n, err := fp.Seek(0, io.SeekEnd)
+		fp, err = os.OpenFile(fullPath, os.O_WRONLY|os.O_CREATE, 0o666)
+		if err != nil {
+			return nil, err
+		}
+
+		offset, err = fp.Seek(0, io.SeekEnd)
 		if err != nil {
 			fp.Close()
 			return nil, err
 		}
-		offset = n
 	}
 
-	return newFileWriter(fp, offset), nil
+	return newFileWriter(fp, offset, tempFilePath, fullPath), nil
 }
 
 // Stat retrieves the FileInfo for the given path, including the current size
@@ -337,19 +351,23 @@ func (fi fileInfo) IsDir() bool {
 }
 
 type fileWriter struct {
-	file      *os.File
-	size      int64
-	bw        *bufio.Writer
-	closed    bool
-	committed bool
-	cancelled bool
+	file         *os.File
+	size         int64
+	bw           *bufio.Writer
+	closed       bool
+	committed    bool
+	cancelled    bool
+	tempFilePath string // Path to the temporary file (non-empty for non-append writes)
+	targetPath   string // Target path for final file
 }
 
-func newFileWriter(file *os.File, size int64) *fileWriter {
+func newFileWriter(file *os.File, size int64, tempFilePath, targetPath string) *fileWriter {
 	return &fileWriter{
-		file: file,
-		size: size,
-		bw:   bufio.NewWriter(file),
+		file:         file,
+		size:         size,
+		bw:           bufio.NewWriter(file),
+		tempFilePath: tempFilePath,
+		targetPath:   targetPath,
 	}
 }
 
@@ -372,7 +390,7 @@ func (fw *fileWriter) Size() int64 {
 
 func (fw *fileWriter) Close() error {
 	if fw.closed {
-		return fmt.Errorf("already closed")
+		return nil // Allow multiple Close calls without error
 	}
 
 	if err := fw.bw.Flush(); err != nil {
@@ -397,7 +415,15 @@ func (fw *fileWriter) Cancel(ctx context.Context) error {
 
 	fw.cancelled = true
 	fw.file.Close()
-	return os.Remove(fw.file.Name())
+
+	// Remove temporary file if it exists
+	if fw.tempFilePath != "" {
+		os.Remove(fw.tempFilePath)
+	} else {
+		os.Remove(fw.targetPath)
+	}
+
+	return nil
 }
 
 func (fw *fileWriter) Commit(ctx context.Context) error {
@@ -412,11 +438,30 @@ func (fw *fileWriter) Commit(ctx context.Context) error {
 	if err := fw.bw.Flush(); err != nil {
 		return err
 	}
-
 	if err := fw.file.Sync(); err != nil {
 		return err
 	}
 
+	// Close the file before renaming (required on some systems)
+	if err := fw.Close(); err != nil {
+		return err
+	}
+
+	// Handle temporary file replacement
+	if fw.tempFilePath != "" {
+		// Atomically rename temp file to target path
+		if err := os.Rename(fw.tempFilePath, fw.targetPath); err != nil {
+			os.Remove(fw.tempFilePath)
+			return err
+		}
+
+		// Sync directory to ensure rename persistence
+		if dir, err := os.Open(path.Dir(fw.targetPath)); err == nil {
+			defer dir.Close()
+			dir.Sync()
+		}
+	}
+
 	fw.committed = true
 	return nil
 }

registry/storage/driver/testsuites/testsuites.go

@@ -410,6 +410,9 @@ func (suite *DriverSuite) testContinueStreamAppend(chunkSize int64) {
 	suite.Require().NoError(err)
 	suite.Require().Equal(chunkSize, nn)
 
+	err = writer.Commit(suite.ctx)
+	suite.Require().NoError(err)
+
 	err = writer.Close()
 	suite.Require().NoError(err)