Skip to content

Commit 78456ca

Browse files
onporatonporat
committed
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>
1 parent 523791c commit 78456ca

2 files changed

Lines changed: 72 additions & 24 deletions

File tree

‎registry/storage/driver/filesystem/driver.go‎

Lines changed: 69 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -180,29 +180,43 @@ func (d *driver) Writer(ctx context.Context, subPath string, append bool) (stora
180180
return nil, err
181181
}
182182

183-
fp, err := os.OpenFile(fullPath, os.O_WRONLY|os.O_CREATE, 0o666)
184-
if err != nil {
185-
return nil, err
186-
}
187-
188-
var offset int64
183+
var (
184+
fp *os.File
185+
err error
186+
offset int64
187+
tempFilePath string
188+
)
189189

190190
if !append {
191-
err := fp.Truncate(0)
191+
// Create temporary file in target directory
192+
tempFile, err := os.CreateTemp(parentDir, ".tmp-")
192193
if err != nil {
193-
fp.Close()
194194
return nil, err
195195
}
196+
tempFilePath = tempFile.Name()
197+
tempFile.Close()
198+
199+
// Open temp file with truncation
200+
fp, err = os.OpenFile(tempFilePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o666)
201+
if err != nil {
202+
os.Remove(tempFilePath)
203+
return nil, err
204+
}
205+
offset = 0
196206
} else {
197-
n, err := fp.Seek(0, io.SeekEnd)
207+
fp, err = os.OpenFile(fullPath, os.O_WRONLY|os.O_CREATE, 0o666)
208+
if err != nil {
209+
return nil, err
210+
}
211+
212+
offset, err = fp.Seek(0, io.SeekEnd)
198213
if err != nil {
199214
fp.Close()
200215
return nil, err
201216
}
202-
offset = n
203217
}
204218

205-
return newFileWriter(fp, offset), nil
219+
return newFileWriter(fp, offset, tempFilePath, fullPath), nil
206220
}
207221

208222
// Stat retrieves the FileInfo for the given path, including the current size
@@ -337,19 +351,23 @@ func (fi fileInfo) IsDir() bool {
337351
}
338352

339353
type fileWriter struct {
340-
file *os.File
341-
size int64
342-
bw *bufio.Writer
343-
closed bool
344-
committed bool
345-
cancelled bool
354+
file *os.File
355+
size int64
356+
bw *bufio.Writer
357+
closed bool
358+
committed bool
359+
cancelled bool
360+
tempFilePath string // Path to the temporary file (non-empty for non-append writes)
361+
targetPath string // Target path for final file
346362
}
347363

348-
func newFileWriter(file *os.File, size int64) *fileWriter {
364+
func newFileWriter(file *os.File, size int64, tempFilePath, targetPath string) *fileWriter {
349365
return &fileWriter{
350-
file: file,
351-
size: size,
352-
bw: bufio.NewWriter(file),
366+
file: file,
367+
size: size,
368+
bw: bufio.NewWriter(file),
369+
tempFilePath: tempFilePath,
370+
targetPath: targetPath,
353371
}
354372
}
355373

@@ -372,7 +390,7 @@ func (fw *fileWriter) Size() int64 {
372390

373391
func (fw *fileWriter) Close() error {
374392
if fw.closed {
375-
return fmt.Errorf("already closed")
393+
return nil // Allow multiple Close calls without error
376394
}
377395

378396
if err := fw.bw.Flush(); err != nil {
@@ -397,7 +415,15 @@ func (fw *fileWriter) Cancel(ctx context.Context) error {
397415

398416
fw.cancelled = true
399417
fw.file.Close()
400-
return os.Remove(fw.file.Name())
418+
419+
// Remove temporary file if it exists
420+
if fw.tempFilePath != "" {
421+
os.Remove(fw.tempFilePath)
422+
} else {
423+
os.Remove(fw.targetPath)
424+
}
425+
426+
return nil
401427
}
402428

403429
func (fw *fileWriter) Commit(ctx context.Context) error {
@@ -412,11 +438,30 @@ func (fw *fileWriter) Commit(ctx context.Context) error {
412438
if err := fw.bw.Flush(); err != nil {
413439
return err
414440
}
415-
416441
if err := fw.file.Sync(); err != nil {
417442
return err
418443
}
419444

445+
// Close the file before renaming (required on some systems)
446+
if err := fw.Close(); err != nil {
447+
return err
448+
}
449+
450+
// Handle temporary file replacement
451+
if fw.tempFilePath != "" {
452+
// Atomically rename temp file to target path
453+
if err := os.Rename(fw.tempFilePath, fw.targetPath); err != nil {
454+
os.Remove(fw.tempFilePath)
455+
return err
456+
}
457+
458+
// Sync directory to ensure rename persistence
459+
if dir, err := os.Open(path.Dir(fw.targetPath)); err == nil {
460+
defer dir.Close()
461+
dir.Sync()
462+
}
463+
}
464+
420465
fw.committed = true
421466
return nil
422467
}

‎registry/storage/driver/testsuites/testsuites.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,9 @@ func (suite *DriverSuite) testContinueStreamAppend(chunkSize int64) {
410410
suite.Require().NoError(err)
411411
suite.Require().Equal(chunkSize, nn)
412412

413+
err = writer.Commit(suite.ctx)
414+
suite.Require().NoError(err)
415+
413416
err = writer.Close()
414417
suite.Require().NoError(err)
415418

0 commit comments

Comments
 (0)