Fix download directory concurrency issue and refactor #4180

GarrettBeatty · 2025-12-02T15:45:10Z

Description

Fixes the issue where in the previous code this happened

 // Creates semaphore with the right limit
fileOperationThrottler = DownloadFilesConcurrently ?
    new SemaphoreSlim(this._config.ConcurrentServiceRequests) :  // e.g., 4
    new SemaphoreSlim(1);

foreach (S3Object s3o in objs) // Say we have 100 files
{
    await fileOperationThrottler.WaitAsync(); // Wait for slot (max 4 waiting)
    
    try
    {
        var task = _failurePolicy.ExecuteAsync(/* download logic */);
        pendingTasks.Add(task); // Queue the task
    }
    finally
    {
        fileOperationThrottler.Release(); // ⚠️ RELEASE IMMEDIATELY!
    }
}

// ⚠️ ALL 100 DOWNLOADS RUN SIMULTANEOUSLY!
await TaskHelpers.WhenAllOrFirstExceptionAsync(pendingTasks, cancellationToken);

this was accidentally introduced in #4151

I also refactored download directory to be easier to read

Motivation and Context

#3806

Testing

made unit tests which pass on development branch but failed on feature/transfermanager branch before this change. Then after i made the fix they pass

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed

License

I confirm that this pull request can be released under the Apache 2 license

GarrettBeatty · 2025-12-02T22:35:33Z

sdk/src/Services/S3/Custom/Transfer/Internal/DownloadDirectoryCommand.cs

        long _transferredBytes;        
        string _currentFile;

-        internal DownloadDirectoryCommand(IAmazonS3 s3Client, TransferUtilityDownloadDirectoryRequest request)


removing unused constructors

GarrettBeatty · 2025-12-02T22:35:55Z

sdk/src/Services/S3/Custom/Transfer/Internal/DownloadDirectoryCommand.cs

            }

-            if (File.Exists(this._request.S3Directory))
+            if (File.Exists(this._request.LocalDirectory))


i think this was a bug. we were checking if s3 directory existed on machine and not local directory???

do we need a dedicated dev config because of this line? @philasmar @normj ?

Since we are fixing an unrelated bug to the feature we should have a change log entry for it.

I'm curious if this should have been Directory.Exists instead of File.Exists. Also was the validation basically broken before and users have been successfully working when the directory already exists. This is fixing a bug but might be introducing a breaking change that might not be okay.

in that case ill remove this line change from this PR and we can update later if we want

ive removed the change from this pr for now since i didnt feel like checking,testing more that scenario. will recheck and create a new PR if required

GarrettBeatty · 2025-12-02T22:37:30Z

sdk/test/Services/S3/UnitTests/Custom/DownloadDirectoryCommandTests.cs

+        /// Actual (broken): 5 concurrent downloads (all files download simultaneously)
+        /// </summary>
+        [TestMethod]
+        public async Task ExecuteAsync_ConcurrentServiceRequests_RespectsLimit()


these were failing before this fix

normj

The main point of the PR looks good. Not sure if it is safe to make the unrelated bug fix given it changes existing behavior.

normj · 2025-12-03T00:05:59Z

sdk/src/Services/S3/Custom/Transfer/Internal/DownloadDirectoryCommand.cs

            }

-            if (File.Exists(this._request.S3Directory))
+            if (File.Exists(this._request.LocalDirectory))


Since we are fixing an unrelated bug to the feature we should have a change log entry for it.

I'm curious if this should have been Directory.Exists instead of File.Exists. Also was the validation basically broken before and users have been successfully working when the directory already exists. This is fixing a bug but might be introducing a breaking change that might not be okay.

normj

Approve but we should continue the discussion about what to do with the bug in a separate thread/pr.

Copilot

Pull request overview

This PR fixes a critical concurrency control bug in the S3 Transfer Utility's DownloadDirectoryCommand where the semaphore was being released immediately after acquiring it, causing all files to download simultaneously instead of respecting the configured concurrency limits. The fix refactors the download logic to use a task pool pattern that properly manages concurrency throughout the download lifecycle.

Key Changes:

Fixed semaphore release timing to occur after download completion rather than immediately after acquiring
Refactored download logic into smaller, focused methods for better readability and maintainability
Introduced ForEachWithConcurrencyAsync helper to properly implement the task pool pattern

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
sdk/src/Services/S3/Custom/Transfer/Internal/_bcl+netstandard/DownloadDirectoryCommand.cs	Refactored ExecuteAsync into smaller methods using task pool pattern; added logging; fixed concurrency control
sdk/src/Services/S3/Custom/Transfer/Internal/DownloadDirectoryCommand.cs	Consolidated constructors to require TransferUtilityConfig parameter
sdk/src/Services/S3/Custom/Transfer/Internal/TaskHelpers.cs	Added ForEachWithConcurrencyAsync method for task pool pattern implementation; added debug logging
sdk/test/Services/S3/UnitTests/Custom/DownloadDirectoryCommandTests.cs	Updated test constructor calls to include config parameter; added concurrency control tests
sdk/test/Services/S3/UnitTests/Custom/FailurePolicyTests.cs	Added comprehensive tests for path validation, sequential mode, and failure handling scenarios

Copilot · 2025-12-03T01:10:27Z

sdk/src/Services/S3/Custom/Transfer/Internal/_bcl+netstandard/DownloadDirectoryCommand.cs


        public bool DownloadFilesConcurrently { get; set; }

+        private Logger Logger
+        {
+            get { return Logger.GetLogger(typeof(DownloadDirectoryCommand)); }


Recursive property definition: the Logger property calls Logger.GetLogger() which references itself. This should be _logger field or use a different pattern.

Suggested change

public bool DownloadFilesConcurrently { get; set; }

private Logger Logger

{

get { return Logger.GetLogger(typeof(DownloadDirectoryCommand)); }

private readonly Logger _logger = Logger.GetLogger(typeof(DownloadDirectoryCommand));

public bool DownloadFilesConcurrently { get; set; }

private Logger Logger

{

get { return _logger; }

i will fix this before merging

Copilot · 2025-12-03T01:10:27Z

sdk/src/Services/S3/Custom/Transfer/Internal/TaskHelpers.cs

+        private static Logger Logger
+        {
+            get { return Logger.GetLogger(typeof(TaskHelpers)); }
+        }


Recursive property definition: the Logger property calls Logger.GetLogger() which references itself. This should use a static field or a different pattern.

GarrettBeatty force-pushed the gcbeatty/downloadfix branch from d19e194 to 1e922b6 Compare December 2, 2025 22:27

GarrettBeatty commented Dec 2, 2025

View reviewed changes

GarrettBeatty force-pushed the gcbeatty/downloadfix branch from 57a8525 to d52eaf5 Compare December 2, 2025 23:00

GarrettBeatty changed the title ~~Fix download directory concurrency issue~~ Fix download directory concurrency issue and refactor Dec 2, 2025

GarrettBeatty marked this pull request as ready for review December 2, 2025 23:59

GarrettBeatty requested review from normj and philasmar December 2, 2025 23:59

normj reviewed Dec 3, 2025

View reviewed changes

GarrettBeatty requested a review from normj December 3, 2025 00:26

normj approved these changes Dec 3, 2025

View reviewed changes

GarrettBeatty mentioned this pull request Dec 3, 2025

refactor upload directory and fix concurrency bug #4186

Merged

GarrettBeatty requested a review from Copilot December 3, 2025 01:00

Copilot started reviewing on behalf of GarrettBeatty December 3, 2025 01:01 View session

Copilot finished reviewing on behalf of GarrettBeatty December 3, 2025 01:03

Copilot AI reviewed Dec 3, 2025

View reviewed changes

philasmar approved these changes Dec 3, 2025

View reviewed changes

GarrettBeatty added 3 commits December 3, 2025 11:09

Fix download directory concurrency issue

ecd578a

refactor

126802a

remove s3directory change

b1c9123

GarrettBeatty force-pushed the gcbeatty/downloadfix branch from 0c0d54d to b1c9123 Compare December 3, 2025 16:11

GarrettBeatty merged commit 6a31b25 into feature/transfermanager Dec 3, 2025
1 check passed

GarrettBeatty deleted the gcbeatty/downloadfix branch December 3, 2025 16:11

Fix download directory concurrency issue and refactor #4180

Fix download directory concurrency issue and refactor #4180

Uh oh!

Conversation

GarrettBeatty commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Testing

Types of changes

Checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normj left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

GarrettBeatty Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GarrettBeatty commented Dec 2, 2025 •

edited

Loading

GarrettBeatty Dec 3, 2025 •

edited

Loading