feat(storage): add progress stall timeout for S3 uploads#3306
feat(storage): add progress stall timeout for S3 uploads#3306burak-initialcode wants to merge 4 commits into
Conversation
Add a configurable progress stall timeout to S3 storage uploads. When upload progress does not advance for the specified interval (e.g. due to network issues), the upload is automatically cancelled and the operation's onError consumer receives a typed StorageException whose cause is ProgressStallTimeoutException, instead of hanging indefinitely. ProgressStallTimeout (sealed: Disabled / Interval(seconds)) is exposed on AWSS3StoragePluginConfiguration as the plugin-wide default and can be overridden per upload via AWSS3StorageUploadFileOptions and AWSS3StorageUploadInputStreamOptions. The resolved interval is threaded through the upload pipeline into WorkData so SinglePartUploadWorker and PartUploadTransferWorker can decorate their ProgressListeners with a StallDetectingProgressListener; on stall the worker cancels the upload job, marks the transfer FAILED (no retry), and surfaces the typed cause. The default remains Disabled (opt-in), preserving existing behavior. fixes aws-amplify#3302
Cover the happy path for both single-part and multipart S3 uploads when a ProgressStallTimeout is set on the upload options. The stall timer must not break a successful upload; these tests assert that the option flows end-to-end through the worker pipeline without affecting normal completion. Mirrors the unit-tested integration coverage retained in the iOS PR.
96a423c to
c2ae0e3
Compare
|
@harsh62 any progress on this one? |
|
@burak-initialcode I am currently looking into the PR. Will let you know once I have an update. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3306 +/- ##
==========================================
+ Coverage 55.91% 56.03% +0.12%
==========================================
Files 1083 1087 +4
Lines 31932 32126 +194
Branches 4760 4803 +43
==========================================
+ Hits 17854 18003 +149
- Misses 12222 12241 +19
- Partials 1856 1882 +26 🚀 New features to boost your workflow:
|
- Add ProgressStallTimeoutTest covering Disabled/Interval semantics, factory methods, equality, and the negative/zero seconds normalization branches. - Add ProgressStallTimeoutExceptionTest covering message and recovery suggestion round-trip and AmplifyException inheritance. - Add AWSS3StoragePluginProgressStallTimeoutTest covering the plugin's resolveProgressStallTimeoutSeconds branches: plugin-level Disabled and Interval defaults, per-upload Interval override, per-upload Disabled override, and the non-AWS-options instanceof fallback path for both uploadFile (key + StoragePath) and uploadInputStream (key + StoragePath). - Extend AWSS3StorageUploadFileOperationTest and AWSS3StorageUploadInputStreamOperationTest to exercise the new Java-friendly long-arg constructor and verify that the resolved progressStallTimeoutSeconds is forwarded to StorageService. - Extend AWSS3StoragePathUploadFileOperationTest and AWSS3StoragePathUploadInputStreamOperationTest to verify that the progressStallTimeoutSeconds carried on the upload request is forwarded to StorageService. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@jvh-aws, any progress? |
jvh-aws
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I am still testing this on my end. Otherwise, there are two small comments I had
Single-part uploads and multi-part chunks restarted via TransferOperations.resume() previously dropped the configured progress-stall timeout, forcing the worker to treat the resumed transfer as if stall detection was disabled. This matched neither the original upload's behavior nor the iOS Amplify Storage parity expectations. Thread the plugin-level default progressStallTimeoutSeconds resolved from AWSS3StoragePluginConfiguration through the storage service, the per-bucket service container, TransferManager, and TransferOperations.resume() so the worker re-arms stall detection with the plugin default whenever a transfer is resumed. Per-upload overrides remain unchanged: the initial start() path still honors the per-upload progressStallTimeoutSeconds threaded from the upload operation options. Only resume - which has no surviving per-upload context - falls back to the plugin default rather than 0. Migrate AWSS3StoragePluginProgressStallTimeoutTest to MockK per the project convention for new Kotlin tests, and add TransferOperationsResumeTest covering the resume work-data propagation in both the configured-default and legacy zero-fallback cases. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@jvh-aws Thanks for the review — both comments are addressed in 1. Resume path + plugin default (Swift #4162 parity)
The plugin-level default from Added 2. MockK for Migrated Verification Locally: |
Issue
#3302
Description
Adds a configurable progress stall timeout to S3 storage uploads. When upload progress does not advance for the configured interval (e.g. due to network issues), the upload is automatically cancelled and the operation's
onErrorconsumer receives an error instead of hanging indefinitely.Problem: On poor or unstable networks, S3 uploads can stall indefinitely without the client receiving any progress updates.
Solution:
ProgressStallTimeoutsealed class in:core(Disabled/Interval(seconds)), mirroring the iOS API.ProgressStallTimeoutException(AmplifyExceptionsubclass) in:coreso consumers can pattern-match on thecauseof the surfacedStorageException.AWSS3StoragePluginConfiguration.progressStallTimeoutand can be overridden per upload viaAWSS3StorageUploadFileOptions/AWSS3StorageUploadInputStreamOptions(nulldefers to the plugin default;Disabledexplicitly opts out for that operation).WorkData.SinglePartUploadWorkerandPartUploadTransferWorkerdecorate theirProgressListeners with a coroutine-basedStallDetectingProgressListener. On stall, the worker cancels the upload job, marks the transferFAILED(workers explicitly skip retry forProgressStallTimeoutException), and surfaces the typed cause viaThrowable.toStorageUploadException(...).Disabled(opt-in). Existing behavior is preserved.Configuration:
How did you test these changes?
StallDetectingProgressListenerTest— coroutine-based timer logic (no progress, progress resets timer, close cancels, disabled threshold no-op, etc.) usingkotlinx-coroutines-test(StandardTestDispatcher+advanceTimeBy).StorageExceptionExtensionsTest— verifiestoStorageUploadExceptionpreserves the typedProgressStallTimeoutExceptioncause (direct and nested) and falls back to the default message for unrelated throwables.AWSS3StoragePluginConfigurationTest— defaults, builder, and override semantics forprogressStallTimeout.AWSS3StorageUploadFileOptionsTest/AWSS3StorageUploadInputStreamOptionsTest— option propagation,null-defers-to-plugin semantics, builder + Java interop.StorageService.uploadFile/uploadInputStreamoverloads.AWSS3StoragePathUploadTest(mirrors the iOS PR coverage retained after reviewer feedback):testUploadSmallFileWithProgressStallTimeoutOptionCompletesSuccessfully— single-part happy path with the stall timer set.testUploadLargeFileWithProgressStallTimeoutOptionCompletesSuccessfully— multipart happy path with the stall timer set../gradlew :core:test :aws-storage-s3:test— all passing locally../gradlew ktlintCheck checkstyle apiDump— all passing; API dump shows additive-only changes (minor bump).Documentation update required?
General Checklist
fix(storage): message,feat(auth): message,chore(all): message)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.