Skip to content

Fix DownloadManager sessions racing conditions#4080

Merged
SergioEstevao merged 3 commits intotrunkfrom
fix/lazy_download_sessions
Mar 19, 2026
Merged

Fix DownloadManager sessions racing conditions#4080
SergioEstevao merged 3 commits intotrunkfrom
fix/lazy_download_sessions

Conversation

@SergioEstevao
Copy link
Contributor

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

Fixes PCIOS-

Ensure lazy sessions are created on init so that no racing condition happen to avoid racing conditions on creation.

Related issues: https://a8c.sentry.io/issues/6791357208/?project=4503921846583296&query=is%3Aunresolved&referrer=issue-stream&sort=freq

To test

  1. Start the app
  2. Start different downloads for episode
  3. Check downloads work 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.

@SergioEstevao SergioEstevao added this to the 8.9 milestone Mar 18, 2026
@SergioEstevao SergioEstevao requested a review from a team as a code owner March 18, 2026 19:43
@SergioEstevao SergioEstevao requested review from bjtitus and Copilot and removed request for a team and Copilot March 18, 2026 19:43
@SergioEstevao SergioEstevao added [Type] Bug Used for issues where something is not functioning as intended. crash Crash related issues labels Mar 18, 2026
@SergioEstevao
Copy link
Contributor Author

@claude can you review this

@claude
Copy link

claude bot commented Mar 18, 2026

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


Code Review

  • Gathering context
  • Reading changed files
  • Analyzing code
  • Posting review

Summary

This is a small, focused fix that correctly addresses a real threading issue. Swift's lazy stored properties are not thread-safe — if two threads access an uninitialized lazy property simultaneously, the initializer can run more than once, potentially creating duplicate URLSessions with the same background session identifier (which iOS does not allow). By calling setupSessions() from init(), all three sessions are eagerly initialized on a single thread before any concurrent access can occur.

What's Good ✅

  • Correct diagnosis and fix. Eagerly initializing lazy vars in init is the canonical pattern for avoiding lazy-init races in Swift without adding locks.
  • Minimal blast radius. Only 7 lines added; no unrelated changes.
  • Preserves readability. The lazy closures remain self-contained and easy to read.

Suggestions

Missing explanatory comment (main concern)

setupSessions() body looks like dead code at first glance — three let _ = ... statements that appear to do nothing. Without a comment, the next developer may remove it thinking it's a no-op, re-introducing the race condition. I'd recommend:

/// Eagerly initializes all URLSessions to avoid race conditions.
/// Swift lazy properties are not thread-safe: if multiple threads access an
/// uninitialized lazy var concurrently, the initializer can run more than once.
/// Calling this from `init()` guarantees single-threaded first access.
private func setupSessions() {
    _ = wifiOnlyBackgroundSession
    _ = cellularBackgroundSession
    _ = cellularForegroundSession
}

Fix this →

let _ = vs _ = (minor / nit)

The more idiomatic Swift spelling is _ = property rather than let _ = property. Both compile identically and SwiftLint's redundant_discardable_let rule is not enabled in this repo, so this is a pure style nit.

Consider making sessions non-lazy (optional / medium)

Since setupSessions() forces every lazy var to initialize synchronously during init, the lazy modifier no longer defers work — initialization always happens at construction time. An alternative would be to convert them to regular stored properties initialized directly in init. That said, the current approach keeps each session's configuration self-contained, which is arguably more readable, so this is a trade-off rather than a clear improvement.

Testing

Race conditions are inherently hard to unit-test deterministically. The PR checklist correctly notes this was considered. The fix itself is sound; no test gap here.


Overall: Approachable fix, correct approach. The main actionable item is adding the explanatory comment to prevent the fix from being accidentally reverted later.

Copy link
Contributor

@bjtitus bjtitus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good and I tested downloads over various network conditions 👍

Copilot AI review requested due to automatic review settings March 19, 2026 10:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to prevent DownloadManager URLSession creation race conditions by eagerly initializing the lazy session properties during initialization, and documents the fix in the changelog.

Changes:

  • Add setupSessions() to force initialization of the three URLSession lazy properties.
  • Call setupSessions() from DownloadManager.init(dataManager:).
  • Add an entry to CHANGELOG.md for the fix.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
podcasts/DownloadManager.swift Forces early creation of the background/foreground URLSessions to avoid lazy-init races.
CHANGELOG.md Adds a release note entry for the DownloadManager session race fix.
Comments suppressed due to low confidence (1)

podcasts/DownloadManager.swift:172

  • setupSessions() is invoked before tempDownloadFolder / download directories are set up. Creating a background URLSession with an existing identifier can immediately start delivering delegate callbacks for pending tasks; those callbacks call pathForEpisode/streamingBufferPathForEpisode (and may need the directories to exist). Consider moving setupSessions() to the end of init (after directory creation) so the filesystem state is ready before any session events can be handled.
    init(dataManager: DataManager) {
        self.dataManager = dataManager
        super.init()

        setupSessions()
        // setup the temp download folder, in caches where iOS can purge it if need be
        let paths = NSSearchPathForDirectoriesInDomains(.cachesDirectory, .userDomainMask, true)
        if let cachePath = paths.first as NSString? {
            tempDownloadFolder = cachePath.appendingPathComponent("temp_download")

@SergioEstevao SergioEstevao merged commit eb2b7e5 into trunk Mar 19, 2026
8 checks passed
@SergioEstevao SergioEstevao deleted the fix/lazy_download_sessions branch March 19, 2026 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crash Crash related issues [Type] Bug Used for issues where something is not functioning as intended.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants