Release media resource loader if not being used

[SergioEstevao]

| 📘 Part of: # |
|:---:|

Fixes PCIOS-

While debugging the media resource loader file handle I found out that our custom resource loader was never been released.
After some investigation I found that the reason is that the our loader and the session used to download the media file where in a strong reference cycle chain.
This happening because the loader is the delegate for the session and the session holds a strong reference for it.

MediaExportResourceLoader <-> URLSession

My initial thought was to break this cycle by invalidating the session when the download was complete.
When I tried that I found out that breaking that reference chain when the download completed made the loader to be free while it was still being used by the AVURLAsset while in play. This is because the AVAssetResourceLoader does not retain the custom delegate we provided to it.

So if release the session reference, by invalidating the session at the end of the download we end up releasing the loader, and the asset stop responding for data requests.

So I looked at an alternative method, and end up implementing on the DownloadManager a reference to the last provided Resource Loader Delegate. This way when the DownloadManager starts new one, we can say to the old to be released when/if it completed the download.

In order to be safe I implemented this code around a FF

To test

1. Start the app
2. Open a Podcast with episodes that are not downloaded
3. Tap play on one episode
4. Check in the console if you see a message like: MediaExporterResourceLoaderDelegate: Start data request for XXX
5. Tap on the option to download the episode
6. Now wait a bit, where you can play around seeking forward and back on the episode
7. Now start playing another episode
8. Check if you see in the console a message like: MediaExporterResourceLoaderDelegate: Releasing loader for and another message of the type: MediaExporterResourceLoaderDelegate: Start data request for XXX
9. Now test it again but you start playing another episode before the download of the first one is finished
10. Check that when start the new episode you don't see the release message, but after a while if the donwload finish you see the release message
11. Now please test again with the FF releaseMediaExporterWhenNoLongerActive disabled to see if all works correctly

Checklist

• I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
• I have considered adding unit tests for my changes.
• I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

[dangermattic]

	1 Warning
⚠️	This PR is assigned to the milestone 8.3 ❄️. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

[SergioEstevao]

@claude can you review this PR

[claude]

Claude finished @SergioEstevao's task in 2m 9s —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of this PR that addresses the MediaExporterResourceLoaderDelegate memory leak.

---

Summary

The PR correctly identifies and fixes a retain cycle between MediaExporterResourceLoaderDelegate and URLSession. The solution tracks the active loader and releases previous loaders when they're no longer needed, protected by a feature flag for safe rollout.

✅ Strengths

1. Root cause correctly identified - The PR accurately diagnoses the MediaExporterResourceLoaderDelegate <-> URLSession retain cycle
2. Good safety approach - Wrapping the fix behind a feature flag (releaseMediaExporterWhenNoLongerActive) allows for safe rollout
3. Thoughtful implementation - The solution correctly handles the case where AVAssetResourceLoader doesn't retain the delegate
4. Clear logging - The deinit message helps verify the fix is working

🔍 Key Issues Found

I've left inline comments on specific code locations. Here are the main concerns:

1. Thread Safety Issue with activeLoaderDelegate ⚠️ High Priority

DownloadManager.swift:328 - The activeLoaderDelegate property is accessed from multiple threads without synchronization. This could lead to race conditions.

Recommendation: Add thread safety using a lock or actor pattern.

2. Potential Premature Release ⚠️ Medium Priority

DownloadManager.swift:354-356 - When reusing an existing loader, there's no check for whether a different active loader should be released first, or whether the reused loader was previously marked for release.

3. Missing Cleanup on Edge Cases ⚠️ Medium Priority

DownloadManager.swift:421-425 - The activeLoaderDelegate is never cleared in error cases or when downloads are cancelled. This could hold references longer than necessary.

4. Naming Could Be Clearer ℹ️ Low Priority

MediaExporterResourceLoaderDelegate.swift:307-311 - The markToRelease() method name doesn't convey its conditional behavior (only releases if download is complete).

🎯 Testing Recommendations

1. Concurrency testing: Rapidly switch between multiple episodes to test race conditions
2. Memory testing: Use Xcode's Memory Graph Debugger to verify loaders are actually released
3. Edge cases: Test with network failures, app backgrounding, and rapid start/stop scenarios
4. Feature flag: Verify behavior works correctly with flag both enabled and disabled

---

Verdict

The approach is sound and correctly addresses the memory leak. However, the thread safety issue should be addressed before merging to prevent potential race conditions in production. The other issues are less critical but would improve robustness.

Recommended Actions Before Merge

1. ✅ Must fix: Add thread safety for activeLoaderDelegate
2. 🔄 Should consider: Add cleanup logic for edge cases (cancellation, errors)
3. 💡 Nice to have: Improve naming and add defensive checks for reuse scenarios

---

| Branch: sergio/release_media_resource_loader

[claude]

Thread Safety Concern: This property is accessed from multiple threads without synchronization:

• Written on lines 355, 413 (potentially from background queues)
• Read/compared on line 423 (from async Task)

Consider making this thread-safe using a lock or making DownloadManager an actor.

private let activeLoaderLock = NSLock()
private var _activeLoaderDelegate: AVAssetResourceLoaderDelegate?
private var activeLoaderDelegate: AVAssetResourceLoaderDelegate? {
    get { 
        activeLoaderLock.lock()
        defer { activeLoaderLock.unlock() }
        return _activeLoaderDelegate 
    }
    set { 
        activeLoaderLock.lock()
        _activeLoaderDelegate = newValue
        activeLoaderLock.unlock()
    }
}

[claude]

Potential Issue: When reusing an existing loader, we don't check if there's a different active loader that should be released first.

Scenario:

1. Episode A playing (loader A active)
2. Start episode B (loader B active, A marked to release)
3. Resume episode A (loader A becomes active again)

At step 3, loader A might still be marked for release from step 2. Consider clearing the release flag or checking loader state before reuse.

[claude]

Cleanup Question: What happens to activeLoaderDelegate if this loader is the active one and it just completed with an error? Should we clear activeLoaderDelegate = nil in that case?

Also, consider simplifying the nested feature flag checks by extracting to a helper method or checking once at the start.

[claude]

Naming suggestion: The method name markToRelease() suggests it unconditionally marks for release, but it only releases if isDownloadComplete. Consider renaming to releaseIfComplete() or adding documentation to clarify the conditional behavior.