Skip to content

[POSIX] Clean up temp files in error cases #1020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mhutchinson wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[POSIX] Clean up temp files in error cases #1020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions storage/posix/file_ops.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,19 @@ func overwrite(name string, d []byte) error {
if err != nil {
return fmt.Errorf("failed to create temp file: %w", err)
}
success := false

@AlCutter AlCutter Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this would be cleaner if you named the error in the func signature and just check for err != nil in the deferred func - that's a pattern I've seen used for this sort of thing.

defer func() {
if !success {
if err := os.Remove(tmpName); err != nil {
slog.WarnContext(context.Background(), "Failed to remove temporary file", slog.String("tmpname", tmpName), slog.Any("error", err))
}
}
}()

if err := os.Rename(tmpName, name); err != nil {
return fmt.Errorf("failed to rename temporary file to target %q: %w", name, err)
}
success = true
return nil
})
}
Expand Down Expand Up @@ -186,17 +195,31 @@ func createTemp(prefix string, d []byte) (name string, err error) {
}
}

tmpName := name
success := false
defer func() {
if !success {
if err := os.Remove(tmpName); err != nil {
slog.WarnContext(context.Background(), "Failed to remove temporary file", slog.String("tmpname", tmpName), slog.Any("error", err))
}
name = ""
}
}()
defer func() {
if errC := f.Close(); errC != nil && err == nil {
err = errC
if errC := f.Close(); errC != nil {
if err == nil {
err = errC
}
success = false
}
}()
Comment on lines +198 to 215

@AlCutter AlCutter Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sorta looks like it's going to defeat the point of the createTemp function by deleting the file it just created?

Note that the comment on the func says it's on the caller to remove the temporary file once they've finished with it.

Maybe you could have createTemp return a "cleanup" func which the caller can use?

@mhutchinson mhutchinson Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only deletes files that would otherwise get leaked when we return an error. I agree it's hard to read though. I think dropping the success variable and using a named error would help. But would it completely mitigate your concern?

e.g. before we tried to create a temp file and write to it. The create worked, but the write didn't work (perhaps threw an error, or the len check didn't work out). We don't give the filename to the user, but before this change the created file stays around. Whatever the problem was on disk is probably now getting worse.

@AlCutter AlCutter Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, ok - yeah, I think named returns vars would help, I think you could eliminate tmpName too that way?


if n, err := f.Write(d); err != nil {
return "", fmt.Errorf("failed to write to temporary file %q: %w", name, err)
if n, writeErr := f.Write(d); writeErr != nil {
return "", fmt.Errorf("failed to write to temporary file %q: %w", name, writeErr)
} else if l := len(d); n < l {
return "", fmt.Errorf("short write on %q, %d < %d", name, n, l)
}

success = true
return name, nil
}
21 changes: 21 additions & 0 deletions storage/posix/files_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,3 +655,24 @@ func TestFlockLockingCorrectness(t *testing.T) {
t.Fatalf("expected output file to contain %q, got %q (serialization failed)", expected, string(content))
}
}

func TestOverwrite_CleanupOnRenameFailure(t *testing.T) {
tmpDir := t.TempDir()
targetDir := filepath.Join(tmpDir, "target_dir")
if err := os.Mkdir(targetDir, 0755); err != nil {
t.Fatal(err)
}
err := overwrite(targetDir, []byte("some data"))
if err == nil {
t.Fatal("expected overwrite to fail when target is a directory")
}
entries, err := os.ReadDir(tmpDir)
if err != nil {
t.Fatal(err)
}
for _, entry := range entries {
if entry.Name() != "target_dir" {
t.Errorf("found unexpected file/dir after failure: %s", entry.Name())
}
}
}
Loading