Skip to content

Conversation

@tylerjroach
Copy link
Member

@tylerjroach tylerjroach commented Nov 4, 2025

  • PR title and description conform to Pull Request guidelines.

Issue #, if available:
#3150

Fix StoragePath uploads to non-default buckets not triggering onSuccess callbacks

Problem

StoragePath uploads to non-default buckets were not triggering onSuccess callbacks, while progress callbacks worked correctly and uploads to the default bucket succeeded.

Root Cause

Multi-bucket support introduced multiple AWSS3StorageService instances (one per bucket), each creating its own TransferManager with its own TransferStatusUpdater. However, the singleton TransferWorkerObserver only calls completion events on the first TransferStatusUpdater it encountered (from the default bucket).

This created a mismatch:

  • Listener registration: Non-default bucket transfers registered with their own TransferStatusUpdater
  • Completion notification: TransferWorkerObserver sent events to the default bucket's TransferStatusUpdater
  • Result: onSuccess callbacks never triggered for non-default buckets

Solution

Implemented dependency injection to share a single TransferStatusUpdater across all storage services:

  1. AWSS3StoragePlugin creates one shared TransferStatusUpdater instance during configuration
  2. AWSS3StorageServiceContainer receives the shared updater via constructor injection
  3. AWSS3StorageService accepts the shared updater via constructor injection
  4. TransferManager receives the shared updater instead of creating its own
  5. All transfers now register listeners and receive completion events through the same updater

How did you test these changes?
(Please add a line here how the changes were tested)

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Ensure commit message has the appropriate scope (e.g 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.

@tylerjroach tylerjroach requested a review from a team as a code owner November 4, 2025 19:57
mattcreaser
mattcreaser previously approved these changes Nov 5, 2025
@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

❌ Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.74%. Comparing base (75562b5) to head (a3de935).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3154      +/-   ##
==========================================
+ Coverage   54.71%   54.74%   +0.03%     
==========================================
  Files        1046     1046              
  Lines       31251    31255       +4     
  Branches     4690     4690              
==========================================
+ Hits        17099    17111      +12     
+ Misses      12328    12318      -10     
- Partials     1824     1826       +2     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tylerjroach tylerjroach merged commit 320836b into main Nov 6, 2025
17 checks passed
@tylerjroach tylerjroach deleted the tjroach/fix-storage-callbacks branch November 6, 2025 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants