remove downloadexception field

GarrettBeatty · GarrettBeatty · commit d664bd731e4e · 2025-12-12T16:56:56.000-05:00
diff --git a/sdk/src/Services/S3/Custom/Transfer/Internal/IDownloadManager.cs b/sdk/src/Services/S3/Custom/Transfer/Internal/IDownloadManager.cs
@@ -55,11 +55,6 @@ internal interface IDownloadManager : IDisposable
         /// with the previous two-method API.
         /// </remarks>
         Task<DownloadResult> StartDownloadAsync(EventHandler<WriteObjectProgressArgs> progressCallback, CancellationToken cancellationToken);
-
-        /// <summary>
-        /// Exception that occurred during downloads, if any.
-        /// </summary>
-        Exception DownloadException { get; }
     }
 
     /// <summary>
diff --git a/sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs b/sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
@@ -46,8 +46,6 @@ internal class MultipartDownloadManager : IDownloadManager
         private readonly SemaphoreSlim _httpConcurrencySlots;
         private readonly bool _ownsHttpThrottler;
         private readonly RequestEventHandler _requestEventHandler;
-        
-        private Exception _downloadException;
         private bool _disposed = false;
         private bool _discoveryCompleted = false;
         private Task _downloadCompletionTask;
@@ -166,15 +164,6 @@ public MultipartDownloadManager(IAmazonS3 s3Client, BaseDownloadRequest request,
             }
         }
 
-        /// <inheritdoc/>
-        public Exception DownloadException
-        { 
-            get 
-            { 
-                return _downloadException;
-            }
-        }
-
         /// <summary>
         /// Discovers the download strategy and starts concurrent downloads in a single unified operation.
         /// This eliminates resource leakage by managing HTTP slots and buffer capacity internally.
@@ -259,7 +248,6 @@ private async Task<DownloadResult> PerformDiscoveryAsync(CancellationToken cance
             }
             catch (Exception ex)
             {
-                _downloadException = ex;
                 _logger.Error(ex, "MultipartDownloadManager: Discovery failed");
                 throw;
             }
@@ -336,7 +324,6 @@ private async Task PerformDownloadsAsync(DownloadResult downloadResult, EventHan
             }
             catch (Exception ex)
             {
-                _downloadException = ex;
                 _logger.Error(ex, "MultipartDownloadManager: Download failed");
                 
                 HandleDownloadError(ex, internalCts);
@@ -431,7 +418,6 @@ private async Task StartBackgroundDownloadsAsync(DownloadResult downloadResult,
             #pragma warning disable CA1031 // Do not catch general exception types
             catch (Exception ex)
             {
-                _downloadException = ex;
                 HandleDownloadError(ex, internalCts);
                 throw;
             }
diff --git a/sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs b/sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs
@@ -130,7 +130,6 @@ public void Constructor_WithValidParameters_CreatesCoordinator()
 
             // Assert
             Assert.IsNotNull(coordinator);
-            Assert.IsNull(coordinator.DownloadException);
         }
 
         [DataTestMethod]
@@ -192,26 +191,6 @@ public void Constructor_WithEncryptionClient_ExceptionMessageIsDescriptive()
 
         #endregion
 
-        #region Property Tests
-
-        [TestMethod]
-        public void DownloadException_InitiallyNull()
-        {
-            // Arrange
-            var mockClient = MultipartDownloadTestHelpers.CreateMockS3Client();
-            var request = MultipartDownloadTestHelpers.CreateOpenStreamRequest();
-            var config = MultipartDownloadTestHelpers.CreateBufferedDownloadConfiguration();
-            var coordinator = new MultipartDownloadManager(mockClient.Object, request, config, CreateMockDataHandler().Object);
-
-            // Act
-            var exception = coordinator.DownloadException;
-
-            // Assert
-            Assert.IsNull(exception);
-        }
-
-        #endregion
-
         #region Discovery - PART Strategy - Single Part Tests
 
         [TestMethod]
@@ -1156,8 +1135,6 @@ public async Task StartDownloadsAsync_BackgroundTaskSuccess_DisposesCancellation
               !coordinator.DownloadCompletionTask.IsCanceled, 
             "Background task should complete successfully");
 
-            Assert.IsNull(coordinator.DownloadException, 
-                "No download exception should occur");
         }
 
         [TestMethod]
@@ -1217,10 +1194,8 @@ public async Task StartDownloadsAsync_BackgroundTaskFailure_DisposesCancellation
             // Assert - Background task should have failed but cleanup should be done
             Assert.IsTrue(coordinator.DownloadCompletionTask.IsCompleted, 
                 "Background task should be completed (even with failure)");
-            Assert.IsNotNull(coordinator.DownloadException, 
-                "Download exception should be captured");
-            Assert.IsInstanceOfType(coordinator.DownloadException, typeof(InvalidOperationException), 
-                "Should capture the simulated failure");
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted,
+                "Background task should be faulted");
         }
 
         [TestMethod]
@@ -1270,8 +1245,7 @@ public async Task StartDownloadsAsync_EarlyError_DisposesCancellationTokenSource
                 Assert.AreEqual("Simulated prepare failure", ex.Message);
             }
             
-            // Assert - Exception should be captured and no background task should exist
-            Assert.IsNotNull(coordinator.DownloadException, "Download exception should be captured");
+            // Assert - DownloadCompletionTask should return completed task when no background work exists
             Assert.IsTrue(coordinator.DownloadCompletionTask.IsCompleted, 
                 "DownloadCompletionTask should return completed task when no background work exists");
         }
@@ -1341,8 +1315,8 @@ public async Task StartDownloadsAsync_BackgroundTaskCancellation_HandlesTokenDis
             // Assert - Cancellation should be handled properly with cleanup
             Assert.IsTrue(coordinator.DownloadCompletionTask.IsCompleted, 
                 "Background task should be completed");
-            Assert.IsNotNull(coordinator.DownloadException, 
-                "Cancellation exception should be captured");
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted || coordinator.DownloadCompletionTask.IsCanceled,
+                "Background task should be faulted or canceled");
         }
 
         #endregion
@@ -1424,36 +1398,6 @@ public async Task StartDownloadAsync_SinglePart_WithPreCancelledToken_ThrowsOper
         }
 
 
-        [TestMethod]
-        public async Task DiscoverDownloadStrategyAsync_WhenCancelled_SetsDownloadException()
-        {
-            // Arrange
-            var mockClient = new Mock<IAmazonS3>();
-            var cancelledException = new OperationCanceledException();
-            mockClient.Setup(x => x.GetObjectAsync(It.IsAny<GetObjectRequest>(), It.IsAny<CancellationToken>()))
-                .ThrowsAsync(cancelledException);
-            
-            var request = MultipartDownloadTestHelpers.CreateOpenStreamRequest();
-            var config = MultipartDownloadTestHelpers.CreateBufferedDownloadConfiguration();
-            var coordinator = new MultipartDownloadManager(mockClient.Object, request, config, CreateMockDataHandler().Object);
-            
-            var cts = new CancellationTokenSource();
-            cts.Cancel();
-
-            // Act
-            try
-            {
-                await coordinator.StartDownloadAsync(null, cts.Token);
-            }
-            catch (OperationCanceledException)
-            {
-                // Expected
-            }
-
-            // Assert
-            Assert.IsNotNull(coordinator.DownloadException);
-            Assert.IsInstanceOfType(coordinator.DownloadException, typeof(OperationCanceledException));
-        }
 
         [TestMethod]
         public async Task DiscoverDownloadStrategyAsync_PassesCancellationTokenToS3Client()
@@ -1549,13 +1493,14 @@ public async Task StartDownloadsAsync_WhenCancelledDuringDownloads_NotifiesBuffe
                 // Expected
             }
 
-            // Assert
-            Assert.IsNotNull(coordinator.DownloadException);
-            Assert.IsInstanceOfType(coordinator.DownloadException, typeof(OperationCanceledException));
+            // Assert - Verify DownloadCompletionTask is faulted with the cancellation exception
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsCompleted, "DownloadCompletionTask should be completed");
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted || coordinator.DownloadCompletionTask.IsCanceled,
+                "DownloadCompletionTask should be faulted or canceled");
         }
 
         [TestMethod]
-        public async Task StartDownloadsAsync_WhenCancelled_SetsDownloadException()
+        public async Task StartDownloadsAsync_WhenCancelled_CompletionTaskIsFaulted()
         {
             // Arrange
             var totalParts = 3;
@@ -1594,9 +1539,10 @@ public async Task StartDownloadsAsync_WhenCancelled_SetsDownloadException()
                 // Expected
             }
 
-            // Assert
-            Assert.IsNotNull(coordinator.DownloadException);
-            Assert.IsInstanceOfType(coordinator.DownloadException, typeof(OperationCanceledException));
+            // Assert - Verify DownloadCompletionTask is faulted with the cancellation
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsCompleted, "DownloadCompletionTask should be completed");
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted || coordinator.DownloadCompletionTask.IsCanceled,
+                "DownloadCompletionTask should be faulted or canceled");
         }
 
         [TestMethod]
@@ -1680,8 +1626,9 @@ public async Task StartDownloadsAsync_CancellationPropagatesAcrossConcurrentDown
                 // Expected
             }
 
-            // Assert - Error should be captured
-            Assert.IsNotNull(coordinator.DownloadException);
+            // Assert - DownloadCompletionTask should be faulted
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted || coordinator.DownloadCompletionTask.IsCanceled,
+                "DownloadCompletionTask should be faulted or canceled when errors occur");
         }
 
         [TestMethod]
@@ -3418,10 +3365,8 @@ public async Task StartDownloadsAsync_BackgroundPartFails_CancelsInternalToken()
             Assert.IsTrue(part3SawCancellation, 
                 "Part 3 should have received cancellation via internalCts.Token (deterministic with TaskCompletionSource)");
             
-            Assert.IsNotNull(coordinator.DownloadException, 
-                "Download exception should be captured when background part fails");
-            Assert.IsInstanceOfType(coordinator.DownloadException, typeof(InvalidOperationException),
-                "Download exception should be the Part 2 failure");
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted, 
+                "DownloadCompletionTask should be faulted when background part fails");
         }
 
         [TestMethod]
@@ -3484,7 +3429,7 @@ public async Task StartDownloadsAsync_MultiplePartsFail_HandlesGracefully()
 
             // Assert - Should handle multiple failures gracefully
             Assert.IsTrue(failedParts.Count > 0, "At least one part should have failed");
-            Assert.IsNotNull(coordinator.DownloadException, "Download exception should be captured");
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted, "DownloadCompletionTask should be faulted");
         }
 
         [TestMethod]
@@ -3559,10 +3504,14 @@ public async Task StartDownloadsAsync_CancellationRacesWithDispose_HandlesGracef
             // by checking IsCancellationRequested before calling Cancel()
             Assert.IsFalse(objectDisposedExceptionCaught, 
                 "ObjectDisposedException should not propagate due to IsCancellationRequested check");
-            Assert.IsNotNull(coordinator.DownloadException, 
-                "Download exception should be the original failure, not ObjectDisposedException");
-            Assert.IsInstanceOfType(coordinator.DownloadException, typeof(InvalidOperationException),
-                "Download exception should be the original InvalidOperationException from Part 2 failure");
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted, 
+                "DownloadCompletionTask should be faulted with the original failure");
+            
+            // Verify the exception type via the Task's exception
+            var aggregateException = coordinator.DownloadCompletionTask.Exception;
+            Assert.IsNotNull(aggregateException, "Task should have an exception");
+            Assert.IsInstanceOfType(aggregateException.InnerException, typeof(InvalidOperationException),
+                "Inner exception should be the original InvalidOperationException from Part 2 failure");
         }
 
         [TestMethod]
@@ -3640,10 +3589,8 @@ public async Task StartDownloadsAsync_PartFailsDuringDownload_OriginalExceptionP
             Assert.AreEqual("Simulated Part 3 failure", caughtException.Message,
                 "The original exception message should be preserved");
             
-            // Also verify DownloadException matches
-            Assert.IsNotNull(coordinator.DownloadException, "DownloadException should be set");
-            Assert.IsInstanceOfType(coordinator.DownloadException, typeof(InvalidOperationException),
-                "DownloadException should also be InvalidOperationException");
+            // Also verify DownloadCompletionTask is faulted
+            Assert.IsTrue(coordinator.DownloadCompletionTask.IsFaulted, "DownloadCompletionTask should be faulted");
         }
 
         #endregion

Original file line number	Diff line number	Diff line change
`@@ -46,8 +46,6 @@ internal class MultipartDownloadManager : IDownloadManager`
`46`	`46`	`private readonly SemaphoreSlim _httpConcurrencySlots;`
`47`	`47`	`private readonly bool _ownsHttpThrottler;`
`48`	`48`	`private readonly RequestEventHandler _requestEventHandler;`
`49`		`-`
`50`		`- private Exception _downloadException;`
`51`	`49`	`private bool _disposed = false;`
`52`	`50`	`private bool _discoveryCompleted = false;`
`53`	`51`	`private Task _downloadCompletionTask;`
`@@ -166,15 +164,6 @@ public MultipartDownloadManager(IAmazonS3 s3Client, BaseDownloadRequest request,`
`166`	`164`	`}`
`167`	`165`	`}`
`168`	`166`
`169`		`- /// <inheritdoc/>`
`170`		`- public Exception DownloadException`
`171`		`- {`
`172`		`- get`
`173`		`- {`
`174`		`- return _downloadException;`
`175`		`- }`
`176`		`- }`
`177`		`-`
`178`	`167`	`/// <summary>`
`179`	`168`	`/// Discovers the download strategy and starts concurrent downloads in a single unified operation.`
`180`	`169`	`/// This eliminates resource leakage by managing HTTP slots and buffer capacity internally.`
`@@ -259,7 +248,6 @@ private async Task<DownloadResult> PerformDiscoveryAsync(CancellationToken cance`
`259`	`248`	`}`
`260`	`249`	`catch (Exception ex)`
`261`	`250`	`{`
`262`		`- _downloadException = ex;`
`263`	`251`	`_logger.Error(ex, "MultipartDownloadManager: Discovery failed");`
`264`	`252`	`throw;`
`265`	`253`	`}`
`@@ -336,7 +324,6 @@ private async Task PerformDownloadsAsync(DownloadResult downloadResult, EventHan`
`336`	`324`	`}`
`337`	`325`	`catch (Exception ex)`
`338`	`326`	`{`
`339`		`- _downloadException = ex;`
`340`	`327`	`_logger.Error(ex, "MultipartDownloadManager: Download failed");`
`341`	`328`
`342`	`329`	`HandleDownloadError(ex, internalCts);`
`@@ -431,7 +418,6 @@ private async Task StartBackgroundDownloadsAsync(DownloadResult downloadResult,`
`431`	`418`	`#pragma warning disable CA1031 // Do not catch general exception types`
`432`	`419`	`catch (Exception ex)`
`433`	`420`	`{`
`434`		`- _downloadException = ex;`
`435`	`421`	`HandleDownloadError(ex, internalCts);`
`436`	`422`	`throw;`
`437`	`423`	`}`