ParallelExecutions of TransferUtility.DownloadAsync appear to append to the same file

[David-Jacobsen]

Describe the bug

We have compute instances running in parallel that each download files from S3 to on-premises samba share. Due to a bug in the scheduler, both instances were processing the same set of files and downloaded the same files at approximately the same time.

My expectation is that one instance would have a lock on the file and the other instance would either Fail due to the file being locked or the later execution would overwrite the first. Instead the resulting file was nearly twice the original file size listed on S3.

Regression Issue

• Select this option if this issue appears to be a regression.

Expected Behavior

I expected whichever compute instance first obtained a file lock on the destination would then write the file to the destination. I expected the subsequent instance to either fail to download the file due to the destination being inaccessible or wait and retry and ultimately overwrite the originally downloaded file with a clone.

Current Behavior

The file in the destination ended up appearing to be the same file appended to itself. In this case it was an MPEG file that was roughly 2x time the original file size from S3. I was able to play it out in windows media player, however MediaInfo identified the bitrate as being significantly higher than was expected. I don't know enough to verify the actual bytes of the resultant file.

No errors were logged or thrown.

Reproduction Steps

Provided that the source S3 bucket has enough files of sufficient size, I expect this to reproduce the underlying error, though in our case it was one application running on two machines instead of 1 application running executions in parallel.

using Amazon.S3;
using Amazon.S3.Model;
using Amazon.S3.Transfer;
namespace ReproduceTest;

class Program
{
    private static readonly string BucketName = "your-bucket-name";
    private static readonly string DownloadPath = @"\\ServerName\FolderName";
    
    static async Task Main(string[] args)
    {
        var s3Client = new AmazonS3Client();
        
        // List S3 objects
        var objects = await ListS3Objects(s3Client);
        Console.WriteLine($"Found {objects.Count} objects");
        
        // Both tasks download entire list in parallel - simulates two compute instances
        var task1 = Task.Run(() => DownloadObjects(s3Client, objects, 1));
        var task2 = Task.Run(() => DownloadObjects(s3Client, objects, 2));
        
        await Task.WhenAll(task1, task2);
        Console.WriteLine("All downloads completed");
    }
    
    static async Task<List<S3Object>> ListS3Objects(AmazonS3Client s3Client)
    {
        var request = new ListObjectsV2Request { BucketName = BucketName };
        var response = await s3Client.ListObjectsV2Async(request);
        return response.S3Objects;
    }
    
    static async Task DownloadObjects(AmazonS3Client s3Client, List<S3Object> objects, int threadId)
    {
        var transferUtility = new TransferUtility(s3Client);
        
        foreach (var obj in objects)
        {
            var fileName = Path.GetFileName(obj.Key);
            var filePath = Path.Combine(DownloadPath, fileName);
            
            var request = new TransferUtilityDownloadRequest
            {
                BucketName = BucketName,
                Key = obj.Key,
                FilePath = filePath
            };
            
            await transferUtility.DownloadAsync(request);
            Console.WriteLine($"Thread {threadId}: Downloaded {fileName}");
        }
    }
}

Possible Solution

When inspecting the code in the SDK, I do see this comment:

                         /* 
                         * Wipe the local file, if it exists, to handle edge case where:
                         * 
                         * 1. File foo exists
                         * 2. We start trying to download, but unsuccesfully write any data
                         * 3. We retry the download, with retires > 0, thus hitting the else statement below
                         * 4. We will append to file foo, instead of overwriting it
                         * 
                         * We counter it with the call below because it's the same call that would be hit
                         * in WriteResponseStreamToFile. If any exceptions are thrown, they will be the same as before
                         * to avoid any breaking changes to customers who handle that specific exception in a
                         * particular manor.
                         */

It appears to describe the issue we experienced, however the code block immediately following this is ignored with a pre-compilation directive of #if BCL

One instance could write out the bytes, while it was writing out the bytes the second instance got an error and retried, when the first instance finished the second instance then appended the entire file to itself.

I created a sample application that creates this merged file and it matched the file that was generated by transferUtility.

namespace AppendTwoFiles;

class Program
{
    static void Main(string[] args)
    {
        string sourceFile = "good.mpg";
        string outputFile = "bad.mpg";

        // Read source file bytes
        byte[] fileBytes = File.ReadAllBytes(sourceFile);
        
        // Write to output file
        File.WriteAllBytes(outputFile, fileBytes);
        
        // Read source file again and append to output
        byte[] fileBytesAgain = File.ReadAllBytes(sourceFile);
        using var stream = new FileStream(outputFile, FileMode.Append);
        stream.Write(fileBytesAgain, 0, fileBytesAgain.Length);
        
        Console.WriteLine($"Created {outputFile} with duplicated MPEG content");
    }
}

This 'bad.mpg' was playable by Windows Media Player despite being two copies of the same file appended to itself.

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

AWSSDK.Core 4.0.0.25
AWSSDK.S3 4.0.6.10

Targeted .NET Platform

.NET Core 8

Operating System and version

Windows Server 2022

[muhammad-othman]

Thank you @David-Jacobsen for reporting this issue and providing detailed information about the behavior you're experiencing. You've identified an important edge case in our concurrent download handling that could lead to incorrect file contents when multiple downloads of the same file occur simultaneously.

The issue occurs in our retry logic where we handle file operations differently between the first attempt and subsequent retries:

1. On the first attempt (retries = 0), we try to create/overwrite the file using FileMode.Create
2. On subsequent retries (retries > 0), we switch to append mode using FileMode.Append

This logic was originally implemented to handle certain edge cases (as noted in the comment you found) where a failed first attempt might leave a partially written file. However, this implementation can cause problems in concurrent scenarios:

• If the first attempt fails due to file access conflicts with another concurrent download
• The retry will then use append mode (FileMode.Append)
• This results in appending the new download to any existing content, potentially causing duplicate data and incorrect file size

We'll need to investigate this thoroughly to ensure we fix this while maintaining compatibility with other download scenarios. We've created an internal ticket to track this issue and will update this thread once we have a proper fix ready.