Skip to content

Harden legacy Downloader background behavior and progress reporting#325

Closed
mattt wants to merge 4 commits intomainfrom
mattt/background-hardening
Closed

Harden legacy Downloader background behavior and progress reporting#325
mattt wants to merge 4 commits intomainfrom
mattt/background-hardening

Conversation

@mattt
Copy link
Collaborator

@mattt mattt commented Mar 3, 2026

Most of the changes in #305 were superseded by our migration to swift-huggingface in #315. However, there were some useful changes there that are worth pulling in, even if Downloader is only kept around for backward compatibility — specifically:

  • Keeping background session identifiers collision-safe
  • Guarding progress with unknown content-length
  • Preserving temp files safely before move

mattt and others added 4 commits March 3, 2026 02:48
…collision

Co-authored-by: Musa Deniev <45773032+musamuss@users.noreply.github.com>
Co-authored-by: Musa Deniev <45773032+musamuss@users.noreply.github.com>
…ngTo

Co-authored-by: Musa Deniev <45773032+musamuss@users.noreply.github.com>
Copy link

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

Hardens the legacy Downloader (kept for backward compatibility post swift-huggingface migration) by improving background-session behavior, progress reporting, and download file finalization safety.

Changes:

  • Makes the background URLSession identifier vary per destination to reduce collisions.
  • Guards progress calculation when totalBytesExpectedToWrite is unknown/non-positive.
  • Copies the downloaded file to a “safe” temporary location before moving it to the final destination.
Comments suppressed due to low confidence (1)

Sources/Hub/Downloader.swift:420

  • Copying the downloaded file (copyItem) into safeTempURL can double disk usage and add significant I/O for large downloads. Since the file at location is only ephemeral after this callback returns, you can moveItem to safeTempURL (or directly to destination) inside the callback to persist it without an extra copy; also consider cleaning up safeTempURL on failure (e.g., defer + conditional remove) to avoid leaving orphaned temp files when moveDownloadedFile throws.
        // Persist to a safe temporary path before leaving this delegate callback
        let tempDir = fileManager.temporaryDirectory
        let safeTempURL = tempDir.appendingPathComponent(UUID().uuidString + "_" + location.lastPathComponent)

        do {
            try fileManager.copyItem(at: location, to: safeTempURL)
            // If the downloaded file already exists on the filesystem, overwrite it
            try fileManager.moveDownloadedFile(from: safeTempURL, to: destination)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

self.chunkSize = chunkSize

let sessionIdentifier = "swift-transformers.hub.downloader"
let sessionIdentifier = "swift-transformers.hub.downloader.\(destination.lastPathComponent.hashValue)"
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

destination.lastPathComponent.hashValue is not stable across process launches (Swift hashes are intentionally randomized) and can collide. For background URLSessionConfiguration.background(withIdentifier:), the identifier must be deterministic to allow reconnecting to existing tasks after relaunch. Consider using a stable identifier derived from the full destination URL/path (e.g., a deterministic cryptographic hash or a sanitized/truncated absoluteString) instead of hashValue.

Suggested change
let sessionIdentifier = "swift-transformers.hub.downloader.\(destination.lastPathComponent.hashValue)"
let sessionIdentifier = "swift-transformers.hub.downloader.\(destination.absoluteString)"

Copilot uses AI. Check for mistakes.
@mattt
Copy link
Collaborator Author

mattt commented Mar 3, 2026

On second thought, Downloader is an internal type that's no longer used. I thought it still had some purchase in the public API, but I was mistaken.

Closing this.

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