Download destination#129
Conversation
📝 WalkthroughWalkthroughThis PR introduces file destination management for downloads. A new Changes
Sequence DiagramsequenceDiagram
participant Caller
participant FileDownloader
participant DownloadTask
participant DownloadDestination
participant FileManager
Caller->>FileDownloader: init(url, destination, session)
FileDownloader->>DownloadTask: start download
DownloadTask->>DownloadTask: receive file at temp location
DownloadTask->>FileDownloader: notify completion(tempURL)
FileDownloader->>DownloadDestination: moveFile(from: tempURL)
alt Move Success
DownloadDestination->>FileManager: copy/move file
FileManager-->>DownloadDestination: finalURL
DownloadDestination-->>FileDownloader: finalURL
FileDownloader->>Caller: .completed(finalURL)
else Move Failure
DownloadDestination->>FileDownloader: throw error
FileDownloader->>Caller: .failed(fileMoveFailed error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/EZNetworking/Util/Downloader/FileDownloader.swift (1)
182-186:⚠️ Potential issue | 🔴 CriticalRoot cause of race condition: async dispatch in event handler.
The
Task { ... }wrapper causeshandleDownloadInterceptorEventto execute asynchronously, afterURLSessionDownloadDelegate.didFinishDownloadingToreturns. Since URLSession deletes the temporary file immediately after the delegate method returns, the file move at lines 200-208 will likely fail because the source file no longer exists.Consider making the file copy/move synchronous within the delegate callback chain, or restructuring so the interceptor itself performs the copy before returning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/EZNetworking/Util/Downloader/FileDownloader.swift` around lines 182 - 186, The delegate closure currently dispatches handleDownloadInterceptorEvent inside Task { } which defers execution until after URLSessionDownloadDelegate.didFinishDownloadingTo returns (causing the temp file to be removed); instead, make the interceptor invocation synchronous so file operations happen before the delegate returns: either remove the Task wrapper and invoke handleDownloadInterceptorEvent directly on the delegate thread (or change the onEvent signature to be async so the delegate can await handleDownloadInterceptorEvent inline), or move the responsibility for copying/moving the temp file into the downloadTaskInterceptor implementation itself so the copy happens before the interceptor returns; update the closure around session.delegate.downloadTaskInterceptor?.onEvent and the handleDownloadInterceptorEvent flow accordingly to ensure the file move occurs synchronously within the delegate callback chain.
🧹 Nitpick comments (1)
Tests/EZNetworkingTests/Util/Downloader/Helpers/DownloadDestinationTests.swift (1)
137-143: Minor: Force unwraps in test helper.The
try!and!are acceptable here since UTF-8 encoding a string literal will never fail. For improved robustness, you could usefatalErrorwith a message, but this is fine for test code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/EZNetworkingTests/Util/Downloader/Helpers/DownloadDestinationTests.swift` around lines 137 - 143, The helper function createTempFile currently uses force unwraps (try! and !); update it to handle failures explicitly by using do/catch for the write operation and optional binding for content.data(using: .utf8), calling fatalError with a clear message on failure so tests fail with useful diagnostics; keep the same return type and behavior but replace the force unwraps in createTempFile to provide explicit error handling and messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/EZNetworking/Util/Downloader/Helpers/DownloadDestination.swift`:
- Around line 13-39: The download temporary file is being moved asynchronously
which races with URLSession deleting it when the delegate returns; update the
flow so the file is copied/moved synchronously inside the URLSession download
delegate before returning. Concretely, in DefaultDownloadTaskInterceptor's
delegate path where it calls onEvent(.onDownloadCompleted(location)) (and where
FileDownloader currently wraps handlers in Task { ... }), perform the move/copy
using DownloadDestination.moveFile(from:) inline (or call a new synchronous
helper that copies the temp URL to a stable location) and then call onEvent with
the resulting stable URL (or pass the moved URL into the async Task) so the
system temporary file is preserved before the delegate method returns. Ensure
moveFile(from:) is invoked on the delegate thread synchronously and only after a
successful move/copy trigger further asynchronous processing.
---
Outside diff comments:
In `@Sources/EZNetworking/Util/Downloader/FileDownloader.swift`:
- Around line 182-186: The delegate closure currently dispatches
handleDownloadInterceptorEvent inside Task { } which defers execution until
after URLSessionDownloadDelegate.didFinishDownloadingTo returns (causing the
temp file to be removed); instead, make the interceptor invocation synchronous
so file operations happen before the delegate returns: either remove the Task
wrapper and invoke handleDownloadInterceptorEvent directly on the delegate
thread (or change the onEvent signature to be async so the delegate can await
handleDownloadInterceptorEvent inline), or move the responsibility for
copying/moving the temp file into the downloadTaskInterceptor implementation
itself so the copy happens before the interceptor returns; update the closure
around session.delegate.downloadTaskInterceptor?.onEvent and the
handleDownloadInterceptorEvent flow accordingly to ensure the file move occurs
synchronously within the delegate callback chain.
---
Nitpick comments:
In
`@Tests/EZNetworkingTests/Util/Downloader/Helpers/DownloadDestinationTests.swift`:
- Around line 137-143: The helper function createTempFile currently uses force
unwraps (try! and !); update it to handle failures explicitly by using do/catch
for the write operation and optional binding for content.data(using: .utf8),
calling fatalError with a clear message on failure so tests fail with useful
diagnostics; keep the same return type and behavior but replace the force
unwraps in createTempFile to provide explicit error handling and messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a332a9cb-e99c-4c91-ae97-d20324faf270
📒 Files selected for processing (8)
Sources/EZNetworking/Error/NetworkFailureReason/DownloadFailureReason.swiftSources/EZNetworking/Util/Downloader/FileDownloader.swiftSources/EZNetworking/Util/Downloader/Helpers/DownloadDestination.swiftTests/EZNetworkingTests/Error/NetworkFailureReason/DownloadFailureReasonTests.swiftTests/EZNetworkingTests/Util/Downloader/FileDownloaderCoreFunctionalityTests.swiftTests/EZNetworkingTests/Util/Downloader/FileDownloaderInvalidStateTests.swiftTests/EZNetworkingTests/Util/Downloader/FileDownloaderPauseResumeTests.swiftTests/EZNetworkingTests/Util/Downloader/Helpers/DownloadDestinationTests.swift
| func moveFile(from tempURL: URL, fileManager: FileManager = .default) throws -> URL { | ||
| switch self { | ||
| case .temporary: | ||
| let destination = fileManager.temporaryDirectory | ||
| .appendingPathComponent(UUID().uuidString) | ||
| .appendingPathExtension(tempURL.pathExtension) | ||
| try fileManager.copyItem(at: tempURL, to: destination) | ||
| return destination | ||
|
|
||
| case let .documents(filename): | ||
| let documentsURL = try fileManager.url( | ||
| for: .documentDirectory, | ||
| in: .userDomainMask, | ||
| appropriateFor: nil, | ||
| create: true | ||
| ) | ||
| let destination = documentsURL.appendingPathComponent(filename) | ||
| if fileManager.fileExists(atPath: destination.path) { | ||
| try fileManager.removeItem(at: destination) | ||
| } | ||
| try fileManager.moveItem(at: tempURL, to: destination) | ||
| return destination | ||
|
|
||
| case let .custom(handler): | ||
| return try handler(tempURL) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the async dispatch in FileDownloader's event handler setup
ast-grep --pattern $'onEvent = { $_ in
Task { $$$
$$$
}
}'Repository: Aldo10012/EZNetworking
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check how the interceptor calls onEvent in the delegate method
rg -n -A5 'didFinishDownloadingTo.*location' --type swiftRepository: Aldo10012/EZNetworking
Length of output: 6627
🏁 Script executed:
# Find and examine the FileDownloader class
fd -t f 'FileDownloader' --exec wc -l {} \; | head -20Repository: Aldo10012/EZNetworking
Length of output: 558
🏁 Script executed:
# Search for the setupDownloadEventHandler method mentioned in the review
rg -n 'setupDownloadEventHandler' --type swiftRepository: Aldo10012/EZNetworking
Length of output: 322
🏁 Script executed:
# Search for where onEvent is assigned/used
rg -n 'onEvent\s*=' --type swift -B2 -A5Repository: Aldo10012/EZNetworking
Length of output: 7330
🏁 Script executed:
# Check the actual implementation of handleDownloadInterceptorEvent
rg -n 'handleDownloadInterceptorEvent' --type swift -A10Repository: Aldo10012/EZNetworking
Length of output: 1585
🏁 Script executed:
# Get the complete handleDownloadInterceptorEvent method
rg -n 'case let .onDownloadCompleted' --type swift -A30 Sources/EZNetworking/Util/Downloader/FileDownloader.swiftRepository: Aldo10012/EZNetworking
Length of output: 1453
🏁 Script executed:
# Search for any moveFile calls in FileDownloader
rg -n 'moveFile' --type swift Sources/EZNetworking/Util/Downloader/Repository: Aldo10012/EZNetworking
Length of output: 363
🏁 Script executed:
# Get more context around the handleDownloadInterceptorEvent implementation
sed -n '189,237p' Sources/EZNetworking/Util/Downloader/FileDownloader.swiftRepository: Aldo10012/EZNetworking
Length of output: 2005
🏁 Script executed:
# Verify URLSession's temporary file deletion behavior from Apple docs
web_searchRepository: Aldo10012/EZNetworking
Length of output: 116
🌐 Web query:
URLSessionDownloadDelegate didFinishDownloadingTo temporary file deletion behavior
💡 Result:
The temporary file provided to URLSessionDownloadDelegate's didFinishDownloadingTo(location:) method is deleted by the system after the delegate method returns if the app does not move it to a permanent location or open it for reading within the method. The official Apple documentation explicitly states: "Because the file is temporary, you must either open the file for reading or move it to a permanent location in your app's sandbox container directory before returning from this delegate method." Multiple developer reports confirm that delaying access (e.g., via async dispatch) results in "No such file or directory" errors because the file is removed promptly after the method exits.
Citations:
- 1: https://developer.apple.com/documentation/foundation/urlsessiondownloaddelegate/urlsession(_:downloadtask:didfinishdownloadingto:)
- 2: https://stackoverflow.com/questions/66416160/urlsessiondownloaddelegate-doesnt-appear-to-play-well-with-different-threads
- 3: https://developer.apple.com/documentation/foundation/urlsessiondownloadtask
- 4: https://developer.apple.com/documentation/foundation/url_loading_system/downloading_files_from_websites
Critical: Race condition with URLSession's temporary file lifecycle.
The moveFile(from:) method is called asynchronously (via Task { ... } wrapping at lines 182-186 of FileDownloader.swift), but URLSession deletes the temporary file immediately when the didFinishDownloadingTo delegate method returns. According to Apple's documentation, the temporary file must be moved or accessed synchronously within the delegate method, before returning.
Since DefaultDownloadTaskInterceptor calls onEvent(.onDownloadCompleted(location)) and the event handler wraps execution in Task { @sendable [weak self] in }, the actual moveFile call (line 201) happens asynchronously after the delegate method returns and the file is deleted by the system. This will consistently fail with file-not-found errors.
Fix: Move the file synchronously within the delegate callback, or copy the temporary file to a stable location before returning from the delegate method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/EZNetworking/Util/Downloader/Helpers/DownloadDestination.swift`
around lines 13 - 39, The download temporary file is being moved asynchronously
which races with URLSession deleting it when the delegate returns; update the
flow so the file is copied/moved synchronously inside the URLSession download
delegate before returning. Concretely, in DefaultDownloadTaskInterceptor's
delegate path where it calls onEvent(.onDownloadCompleted(location)) (and where
FileDownloader currently wraps handlers in Task { ... }), perform the move/copy
using DownloadDestination.moveFile(from:) inline (or call a new synchronous
helper that copies the temp URL to a stable location) and then call onEvent with
the resulting stable URL (or pass the moved URL into the async Task) so the
system temporary file is preserved before the delegate method returns. Ensure
moveFile(from:) is invoked on the delegate thread synchronously and only after a
successful move/copy trigger further asynchronous processing.
1 participant
Summary by CodeRabbit
New Features
Improvements