command: fix target file is created despite the download failure (#477)

Before downloading a file from s3 a local target file is created. If the download fails the created file should've been deleted. But since the file was not closed, the delete operation fails in Windows.

Fixes #348

CHANGELOG.md

@@ -23,6 +23,7 @@
 - Fixed a bug where some part of the destination path is removed by `cp` and `sync` subcommands ([#360](https://github.com/peak/s5cmd/issues/360))
 - Fixed a bug where proxy is not being used when `--no-verify-ssl` flag is used. ([#445](https://github.com/peak/s5cmd/issues/445))
 - Fixed `unknown url format` error when object key also includes `s3://` e.g. `s5cmd ls s3://foo/bar/s3://baz` ([#449](https://github.com/peak/s5cmd/issues/449))
+- Fixed a bug where the local file created for the download operation was not deleted if the download fails in Windows. ([#348](https://github.com/peak/s5cmd/issues/348))
 
 ## v2.0.0 - 4 Jul 2022
 

command/cp.go

@@ -517,13 +517,18 @@ func (c Copy) doDownload(ctx context.Context, srcurl *url.URL, dsturl *url.URL)
 	if err != nil {
 		return err
 	}
-	defer file.Close()
 
 	size, err := srcClient.Get(ctx, srcurl, file, c.concurrency, c.partSize)
 	if err != nil {
-		_ = dstClient.Delete(ctx, dsturl)
+		// file must be closed before deletion
+		file.Close()
+		dErr := dstClient.Delete(ctx, dsturl)
+		if dErr != nil {
+			printDebug(c.op, dErr, srcurl, dsturl)
+		}
 		return err
 	}
+	defer file.Close()
 
 	if c.deleteSource {
 		_ = srcClient.Delete(ctx, srcurl)

e2e/cp_test.go

@@ -4185,3 +4185,27 @@ func TestCopySingleFileToS3WithNoSuchUploadRetryCount(t *testing.T) {
 	// assert S3
 	assert.Assert(t, ensureS3Object(s3client, bucket, filename, content))
 }
+
+// Before downloading a file from s3 a local target file is created. If download
+// fails the created file should be deleted.
+func TestDeleteFileWhenDownloadFailed(t *testing.T) {
+	t.Parallel()
+
+	s3client, s5cmd, cleanup := setup(t)
+	defer cleanup()
+
+	bucket := s3BucketFromTestName(t)
+	filename := "testfile1.txt"
+	createBucket(t, s3client, bucket)
+
+	// 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 with the name `filename`.
+	cmd := s5cmd("cp", "s3://"+bucket+"/"+filename, filename)
+	result := icmd.RunCmd(cmd)
+
+	result.Assert(t, icmd.Expected{ExitCode: 1})
+
+	// assert local filesystem does not have any (such) file
+	expected := fs.Expected(t)
+	assert.Assert(t, fs.Equal(cmd.Dir, expected))
+}