From 78456caf466811af53d3b712af6504cbc64284e2 Mon Sep 17 00:00:00 2001 From: Oded Porat Date: Wed, 16 Apr 2025 10:30:20 +0300 Subject: [PATCH 1/3] 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 --- registry/storage/driver/filesystem/driver.go | 93 ++++++++++++++----- .../storage/driver/testsuites/testsuites.go | 3 + 2 files changed, 72 insertions(+), 24 deletions(-) diff --git a/registry/storage/driver/filesystem/driver.go b/registry/storage/driver/filesystem/driver.go index 72f80e906..b15745e62 100644 --- a/registry/storage/driver/filesystem/driver.go +++ b/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 } diff --git a/registry/storage/driver/testsuites/testsuites.go b/registry/storage/driver/testsuites/testsuites.go index c61ecc6cd..7a3ee06aa 100644 --- a/registry/storage/driver/testsuites/testsuites.go +++ b/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) From a5a6f1ba3d9047914c43d6ea0de4c55258a4a322 Mon Sep 17 00:00:00 2001 From: Oded Porat Date: Wed, 23 Apr 2025 11:37:55 +0300 Subject: [PATCH 2/3] To address the issue where empty files are created when the write process is interrupted, the solution involves writing to a temporary file first and then atomically renaming it to the target file. This ensures that the target file is only updated if the write completes successfully, preventing empty or partially written files. **Explanation:** 1. **Temporary File Creation:** The content is first written to a temporary file (appending `.tmp` to the original path). This ensures that the original file remains intact until the write is complete. 2. **Write to Temporary File:** Using the existing `Writer` with truncation (`false`), the content is written to the temporary file. If the write fails, the temporary file is closed and deleted. 3. **Commit and Rename:** After successfully writing to the temporary file, it is committed. Then, the temporary file is atomically renamed to the target path using `Move`, which is handled by the filesystem's rename operation (atomic on most systems). 4. **Cleanup on Failure:** If any step fails, the temporary file is cleaned up to avoid leaving orphaned files. Signed-off-by: Oded Porat --- registry/storage/driver/filesystem/driver.go | 123 +++++++----------- .../storage/driver/testsuites/testsuites.go | 3 - 2 files changed, 48 insertions(+), 78 deletions(-) diff --git a/registry/storage/driver/filesystem/driver.go b/registry/storage/driver/filesystem/driver.go index b15745e62..8c3c467da 100644 --- a/registry/storage/driver/filesystem/driver.go +++ b/registry/storage/driver/filesystem/driver.go @@ -134,19 +134,37 @@ func (d *driver) GetContent(ctx context.Context, path string) ([]byte, error) { // PutContent stores the []byte content at a location designated by "path". func (d *driver) PutContent(ctx context.Context, subPath string, contents []byte) error { - writer, err := d.Writer(ctx, subPath, false) + tempPath := subPath + ".tmp" + + // Write to a temporary file to prevent partial writes. + writer, err := d.Writer(ctx, tempPath, false) if err != nil { return err } defer writer.Close() + _, err = io.Copy(writer, bytes.NewReader(contents)) if err != nil { if cErr := writer.Cancel(ctx); cErr != nil { return errors.Join(err, cErr) } + // Attempt to clean up the temporary file on error. + _ = d.Delete(ctx, tempPath) return err } - return writer.Commit(ctx) + + if err := writer.Commit(ctx); err != nil { + return err + } + + // Atomically replace the target file with the temporary file. + if err := d.Move(ctx, tempPath, subPath); err != nil { + // Clean up the temporary file if rename fails. + _ = d.Delete(ctx, tempPath) + return err + } + + return nil } // Reader retrieves an io.ReadCloser for the content stored at "path" with a @@ -180,43 +198,29 @@ func (d *driver) Writer(ctx context.Context, subPath string, append bool) (stora return nil, err } - var ( - fp *os.File - err error - offset int64 - tempFilePath string - ) + fp, err := os.OpenFile(fullPath, os.O_WRONLY|os.O_CREATE, 0o666) + if err != nil { + return nil, err + } + + var offset int64 if !append { - // Create temporary file in target directory - tempFile, err := os.CreateTemp(parentDir, ".tmp-") - if err != nil { - 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 { - 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) + err := fp.Truncate(0) if err != nil { fp.Close() return nil, err } + } else { + n, err := fp.Seek(0, io.SeekEnd) + if err != nil { + fp.Close() + return nil, err + } + offset = n } - return newFileWriter(fp, offset, tempFilePath, fullPath), nil + return newFileWriter(fp, offset), nil } // Stat retrieves the FileInfo for the given path, including the current size @@ -351,23 +355,19 @@ func (fi fileInfo) IsDir() bool { } type fileWriter struct { - 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 + file *os.File + size int64 + bw *bufio.Writer + closed bool + committed bool + cancelled bool } -func newFileWriter(file *os.File, size int64, tempFilePath, targetPath string) *fileWriter { +func newFileWriter(file *os.File, size int64) *fileWriter { return &fileWriter{ - file: file, - size: size, - bw: bufio.NewWriter(file), - tempFilePath: tempFilePath, - targetPath: targetPath, + file: file, + size: size, + bw: bufio.NewWriter(file), } } @@ -390,7 +390,7 @@ func (fw *fileWriter) Size() int64 { func (fw *fileWriter) Close() error { if fw.closed { - return nil // Allow multiple Close calls without error + return fmt.Errorf("already closed") } if err := fw.bw.Flush(); err != nil { @@ -415,15 +415,7 @@ func (fw *fileWriter) Cancel(ctx context.Context) error { fw.cancelled = true fw.file.Close() - - // Remove temporary file if it exists - if fw.tempFilePath != "" { - os.Remove(fw.tempFilePath) - } else { - os.Remove(fw.targetPath) - } - - return nil + return os.Remove(fw.file.Name()) } func (fw *fileWriter) Commit(ctx context.Context) error { @@ -438,30 +430,11 @@ 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 } diff --git a/registry/storage/driver/testsuites/testsuites.go b/registry/storage/driver/testsuites/testsuites.go index 7a3ee06aa..c61ecc6cd 100644 --- a/registry/storage/driver/testsuites/testsuites.go +++ b/registry/storage/driver/testsuites/testsuites.go @@ -410,9 +410,6 @@ 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) From dde1e49f2368d46471adb9488b01611afdebeb85 Mon Sep 17 00:00:00 2001 From: Oded Porat Date: Sun, 4 May 2025 10:43:19 +0300 Subject: [PATCH 3/3] Changes: Append a UUID to ensure uniqueness Join delete error Signed-off-by: Oded Porat --- registry/storage/driver/filesystem/driver.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/registry/storage/driver/filesystem/driver.go b/registry/storage/driver/filesystem/driver.go index 8c3c467da..6d1c15e6e 100644 --- a/registry/storage/driver/filesystem/driver.go +++ b/registry/storage/driver/filesystem/driver.go @@ -15,6 +15,7 @@ import ( storagedriver "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/distribution/distribution/v3/registry/storage/driver/base" "github.com/distribution/distribution/v3/registry/storage/driver/factory" + "github.com/google/uuid" ) const ( @@ -134,7 +135,7 @@ func (d *driver) GetContent(ctx context.Context, path string) ([]byte, error) { // PutContent stores the []byte content at a location designated by "path". func (d *driver) PutContent(ctx context.Context, subPath string, contents []byte) error { - tempPath := subPath + ".tmp" + tempPath := fmt.Sprintf("%s.%s.tmp", subPath, uuid.NewString()) // Write to a temporary file to prevent partial writes. writer, err := d.Writer(ctx, tempPath, false) @@ -149,8 +150,8 @@ func (d *driver) PutContent(ctx context.Context, subPath string, contents []byte return errors.Join(err, cErr) } // Attempt to clean up the temporary file on error. - _ = d.Delete(ctx, tempPath) - return err + dErr := d.Delete(ctx, tempPath) + return errors.Join(err, dErr) } if err := writer.Commit(ctx); err != nil { @@ -160,8 +161,8 @@ func (d *driver) PutContent(ctx context.Context, subPath string, contents []byte // Atomically replace the target file with the temporary file. if err := d.Move(ctx, tempPath, subPath); err != nil { // Clean up the temporary file if rename fails. - _ = d.Delete(ctx, tempPath) - return err + dErr := d.Delete(ctx, tempPath) + return errors.Join(err, dErr) } return nil