Repository: peak/s5cmd (go)

Issue:
Local file is lost even if the download fails
### Issue & steps to reproduce
```
$ echo "Example text." > tmp
$ cat tmp
Example text.
$ s5cmd cp s3://nonexistentbucket/key tmp
ERROR "cp s3://nonexistentbucket/key tmp": InvalidAccessKeyId: The AWS Access Key Id you provided does not exist in our records. status code: 403, request id: PAYWDH7HHVPAHC8G, host id: cnI/cBhYZKKJ159f8mMs4KHOn1+jYtvfpYnXgMuKMRd9pZ10FBdi/cGuVxyr+iwrKbk2kP7Opx4=
$ cat tmp
cat: tmp: No such file or directory
```
We would expect the local file to remain untouched since the download operation did not even started.


### Proposed Solution
Instead of creating the file before the download request a custom [WriterAt](https://pkg.go.dev/io#WriterAt), which will create the file once the first write request came, can be used. Following snippets are going to change:
https://github.com/peak/s5cmd/blob/123b1c7fc9c614aa214a6795468fa140a38ad05e/command/cp.go#L511-L517
https://github.com/peak/s5cmd/blob/0431f503d99953e1809bf0e86d73750c0c1f561e/storage/fs.go#L208-L214

Pull request:
command: fix file overriding when download fails (#479)
Resolves #479 
Local files used to be overwritten even if downloads failed.
Solved it by creating a temporary file and renaming it with the original filename after completing the download successfully.

Patch:
diff --git a/command/cp.go b/command/cp.go
index a81539e..e294d7b 100644
--- a/command/cp.go
+++ b/command/cp.go
@@ -528,27 +528,32 @@ func (c Copy) doDownload(ctx context.Context, srcurl *url.URL, dsturl *url.URL)
 		return err
 	}
 
-	file, err := dstClient.Create(dsturl.Absolute())
+	dstPath := filepath.Dir(dsturl.Absolute())
+	dstFile := filepath.Base(dsturl.Absolute())
+	file, err := dstClient.CreateTemp(dstPath, dstFile)
 	if err != nil {
 		return err
 	}
 
 	size, err := srcClient.Get(ctx, srcurl, file, c.concurrency, c.partSize)
+	file.Close()
 	if err != nil {
-		// file must be closed before deletion
-		file.Close()
-		dErr := dstClient.Delete(ctx, dsturl)
+		dErr := dstClient.Delete(ctx, &url.URL{Path: file.Name(), Type: dsturl.Type})
 		if dErr != nil {
 			printDebug(c.op, dErr, srcurl, dsturl)
 		}
 		return err
 	}
-	defer file.Close()
 
 	if c.deleteSource {
 		_ = srcClient.Delete(ctx, srcurl)
 	}
 
+	err = dstClient.Rename(file, dsturl.Absolute())
+	if err != nil {
+		return err
+	}
+
 	msg := log.InfoMessage{
 		Operation:   c.op,
 		Source:      srcurl,
diff --git a/storage/fs.go b/storage/fs.go
index 9f8dcb5..f707590 100644
--- a/storage/fs.go
+++ b/storage/fs.go
@@ -223,6 +223,30 @@ func (f *Filesystem) Open(path string) (*os.File, error) {
 	return file, nil
 }
 
+// CreateTemp creates a new temporary file
+func (f *Filesystem) CreateTemp(dir, pattern string) (*os.File, error) {
+	if f.dryRun {
+		return &os.File{}, nil
+	}
+
+	file, err := os.CreateTemp(dir, pattern)
+	if err != nil {
+		return nil, err
+	}
+
+	err = file.Chmod(0644)
+	return file, err
+}
+
+// Rename a file
+func (f *Filesystem) Rename(file *os.File, newpath string) error {
+	if f.dryRun {
+		return nil
+	}
+
+	return os.Rename(file.Name(), newpath)
+}
+
 func sendObject(ctx context.Context, obj *Object, ch chan *Object) {
 	select {
 	case <-ctx.Done():


Test patch:
diff --git a/e2e/cp_test.go b/e2e/cp_test.go
index ad4a648..ff2bf49 100644
--- a/e2e/cp_test.go
+++ b/e2e/cp_test.go
@@ -4140,3 +4140,32 @@ func TestDeleteFileWhenDownloadFailed(t *testing.T) {
 	expected := fs.Expected(t)
 	assert.Assert(t, fs.Equal(cmd.Dir, expected))
 }
+
+// Target local file should be overriden only if download completed successfully
+func TestLocalFileOverridenWhenDownloadFailed(t *testing.T) {
+	t.Parallel()
+
+	s3client, s5cmd := setup(t)
+	bucket := s3BucketFromTestName(t)
+	createBucket(t, s3client, bucket)
+
+	const (
+		filename        = "testfile1.txt"
+		content         = "preserved content"
+		expectedContent = "preserved content"
+	)
+
+	workdir := fs.NewDir(t, t.Name(), fs.WithFile(filename, content))
+	defer workdir.Remove()
+
+	// It is going try downloading a nonexistent file from the s3 so it will fail.
+	// In this case we don't expect to have a local file will be overwritten.
+	cmd := s5cmd("cp", "s3://"+bucket+"/"+filename, filename)
+	result := icmd.RunCmd(cmd, withWorkingDir(workdir))
+
+	result.Assert(t, icmd.Expected{ExitCode: 1})
+
+	// assert initial file is untouched
+	expected := fs.Expected(t, fs.WithFile(filename, content))
+	assert.Assert(t, fs.Equal(workdir.Path(), expected))
+}