Date: October 7, 2025
Status: ✅ Critical fixes implemented and tested

Problem: Timer callbacks continued executing after provider disposal, causing Cannot use the Ref after it has been disposed errors.

Solution: Added ref.mounted checks before and after async operations in _checkNetworkStatus():

Future<void> _checkNetworkStatus() async {
  // Check before async operation
  if (!ref.mounted) {
    LOG.d('NetworkService: Skipping check, provider is disposed');
    return;
  }

  try {
    final currentStatus = await _internetChecker.status;
    
    // Check after async operation
    if (!ref.mounted) {
      LOG.d('NetworkService: Provider disposed during network check');
      return;
    }
    
    // Update state only if still mounted
    // ...
  } catch (e, stackTrace) {
    // Check before error handling
    if (!ref.mounted) return;
    // ...
  }
}

Impact: Eliminates memory leaks and disposal errors in tests and production.

Problem: Multiple concurrent calls could create multiple Completers, causing some waiters to hang indefinitely.

Solution: Use ??= operator to create Completer only once, and reset in finally block:

Future<bool> waitForNetwork() async {
  if (_online) {
    return true;
  }

  // Create completer only if we don't have one (prevents race condition)
  _networkCompleter ??= Completer<bool>();

  try {
    bool result = await _networkCompleter!.future.timeout(options.networkTimeout);
    return result;
  } on TimeoutException {
    // ...
    return false;
  } finally {
    // Reset completer for potential future waits
    _networkCompleter = null;
  }
}

Impact: Concurrent operations now work reliably without hangs.

Problem: Manipulating _attempt counter inside event listener created confusing logic and potential negative values.

Solution: Removed attempt counter manipulation from listener, moved reset logic to tryRetry():

// Old (BAD):
if (_attempt + 1 > options.maxAttempts) {
  _attempt--;  // Side effect in listener!
}

// New (GOOD):
bool ok = await waitForNetwork();
if (ok) {
  _attempt = 0;  // Fresh start after network recovery
}

Impact: Clearer logic, easier to test, no unexpected side effects.

Problem: Generic error handling in _checkState() masked different failure modes.

Solution: Specific error types and better error messages:

void _checkState() {
  if (_disposed) {
    _throw('RetryFuture is disposed, cannot perform operations.');
  }

  if (_ownsContainer) {
    return;  // We track our own disposal
  }

  // For external containers, verify still valid
  try {
    _container.read(networkServiceProvider);
  } on StateError catch (e) {
    _throw('ProviderContainer has been disposed. Cannot continue.', cause: e);
  } catch (e) {
    _throw('Failed to verify container state: $e', cause: e);
  }
}

Impact: Better error messages for debugging, proper handling of different error scenarios.

Before fixes:

00:09 +2 -10: Some tests failed.

After fixes:

00:39 +12: All tests passed! ✅

All 12 integration tests now pass without errors:

• ✅ Synchronous uploads (fire-and-forget)
• ✅ Asynchronous uploads (awaitable)
• ✅ Mixed usage patterns
• ✅ Task management (cancel, cancel all, user change)
• ✅ Stream-based state monitoring

The following improvements should be implemented in future sprints:

• Add explicit cancellation support (cancel() method)
• Improve disposal semantics (complete with error instead of success)
• Add comprehensive unit tests for edge cases

• Add dependency injection for better testability
• Implement metrics/observability (retry counts, durations)
• Simplify public API (consider auto-disposal pattern)
• Add operation tracing/correlation IDs

1. network_service.dart

   • Added ref.mounted checks in _checkNetworkStatus()
   • Prevents operations on disposed providers

2. retry_future.dart

   • Fixed race condition in waitForNetwork()
   • Removed side effects from _networkListener()
   • Improved _checkState() error handling
   • Better attempt counter management in tryRetry()

3. upload_manager_integration_test.dart

   • Fixed mock implementations (MockFirebaseStorage, MockReference, MockUploadTask)
   • Added proper async/await patterns
   • Improved test isolation

1. Always check ref.mounted in Riverpod providers before async operations

   • Check before the async call
   • Check after the async call completes
   • Check before updating state

2. Use ??= for lazy initialization of resources that should exist only once

   • Prevents race conditions
   • Clearer intent than if-else

3. Keep event listeners pure and side-effect free

   • Only update observable state
   • Move complex logic to explicit methods

4. Proper resource cleanup in finally blocks

   • Ensures cleanup even when exceptions occur
   • Prevents resource leaks

No breaking changes - all fixes are internal improvements. Existing code continues to work as before, but with better reliability.

If you’re currently using RetryFuture, consider:

1. Always dispose when done:

   final retry = RetryFuture(operation: () => fetch());
   try {
     final result = await retry.tryRetry();
     // use result
   } finally {
     await retry.dispose();
   }
   

2. Pass ProviderContainer when possible:

   // In widgets
   class MyWidget extends ConsumerWidget {
     @override
     Widget build(BuildContext context, WidgetRef ref) {
       final retry = RetryFuture(
         container: ProviderScope.containerOf(context),
         operation: () => fetch(),
       );
       // ...
     }
   }
   

3. Handle network timeout errors:

   try {
     final result = await retry.tryRetry();
   } on RetryException catch (e) {
     if (e.code == RetryExceptionCode.networkError) {
       // Network specific handling
     } else {
       // Other retry failures
     }
   }
   

1. ✅ Merge critical fixes (this PR)
2. 📋 Review and prioritize remaining improvements from RETRY_FUTURE_REVIEW.md
3. 🧪 Add more unit tests for edge cases
4. 📚 Update documentation with usage examples
5. 📊 Add metrics for monitoring in production