Fix failed downloads (file coordination) #2467

mallexxx · 2024-03-21T10:09:39Z

Task/Issue URL: https://app.asana.com/0/1199178362774117/1201330245678289/f
Notarized build: https://github.com/duckduckgo/macos-browser/actions/runs/8388376828/artifacts/1349521698
CC: @ayoy

Description:

File Presenter implementation for file coordination and file moving/removal detection
Downloads refactored to use File Presenters instead of URLs

Steps to test this PR:

Validate both sandboxed and non-sandboxed builds, preferably in RELEASE/REVIEW
Clean app data before testing to reset download settings; Delete the app from /Applications; delete the app rom System Settings -> Files and Folders

Test regular downloads: quick ones (like https://hexfiend.com/) and long (like openoffice.org) - validate both saving in default location (~/Downloads), choosing a custom location (like ~/Desktop; ~/Downloads/DuckDuckGo), or always requesting location;
Validate files are not overwritten when using a set location;
Validate files are correctly overwritten when always requesting location and choosing an existing files
Start a long download (e.g. openoffice); click Download again, choose the 1st downloaded file as a location for the second download, confirm overwrite: fist download should stop and disappear from the Downloads panel, the new one should be added
set a breakpoint in the setupTemporaryDownloadURL method at return, remove the item replacement directory (po itemReplacementDirectory; rm /tmp/path/to/the/dir) manually and continue – this should lead to download failure
test download cancellation by clicking "x" on a .duckload file in Finder/Dock; by clicking "x" in the Downloads panel - download should stay in the panel with "cancelled" state
Validate retrying cancelled downloads - they should resume with the same progress, displaying progress in Finder
test download cancellation by removing a .duckload file in Finder (to Trash or by Cmd+Shift+Del); by rm filename.duckload, rm destinationFileName.zip; by overwriting a download file (.duckload or the target one) from another app - removed downloads should disappear from the downloads panel
Cancel some downloads - .duckload and target files should stay in ~/Downloads; Click "clear" in the Downloads panel - both .duckload and target files should be removed
Validate removing a completed download file - removing the file from the Downloads panel
Turn off wi-fi during a download - downloads should fail showing an error and Retry button for resumable downloads and no retry button for non-resumable downloads (like when Downloading All from a shared google photos album)
Validate retrying failed downloads work
Session restoration should bring back both failed and completed downloads; If a file was removed when the app was not running - the download should be removed from Downloads;
14..duckload and target files should be removed if one of them was removed - for non-complete downloads
Open File Location should highlight the file in Finder both right after download completion or after session restoration; after hiding&showing the Downloads panel
Open File (double click) should not work for non-completed downloads; should work for completed downloads right after download completion and after session restoration
Old downloads should be removed after 3 days including the .duckload and target file for non-completed download, but not remove a completed download file
When renaming/moving a .duckload file, target download file should be also renamed (but not moved), download should continue, the rename should be displayed in the Downloads panel. If there‘s an existing file with the target name (i.e. renaming myload.duckload to test.duckload - myload.zip should be renamed to test.zip but if there‘s already a test.zip file present, the target file should remain myload.zip. This won‘t work when the sandboxed build downloads a file to a user-selected non-Downloads location - in this case the download should go-on with an old target name.
If a target file was renamed during download (mv myload.zip test.zip) - the name change should be reflected in the downloads panel, the download should complete successfully to the new location

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

…alex/fix-failed-downloads

github-actions · 2024-03-21T10:11:26Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against b357e49

tomasstrba

This is a great engineering, @mallexxx! 👏
In major refactoring like this, I’d recommend to use tech designs more. It can help with the review and also later in case someone else has to fix/enhance the functionality.

I tested all kinds of scenarios and it worked very well 👏 :
✔️ Tested simultanous downloads of files
✔️ Tested custom download location
✔️ Tested asking for download destination on every
❔ Couldn’t remove the item replacement directory because: Operation not permitted. Not sure if it's a big deal
✔️ Tested download cancellation
✔️ Tested download resuming after the cancelation
✔️ Tested download cancellation by removing a .duckload file in Finder
✔️ Tested cancelation and resuming with wifi problems
✔️ Validated downloads list is properly restored after the app restart
✔️ Validated downloads list is refreshed according to the file system changes when the app is turned off
✔️ Validated opening files from the download list
✔️ Validated renaming of the .duckload file

There are couple of issues I experienced:

After clicking "clear" in the Downloads panel, .duckload files were not removed from Desktop (custom download location)
Removing a completed download file from Downloads panel didn't remove it from the file system. (Not really sure if this is the requirement, just assuming from testing steps)
It often takes very long to see .duckload being renamed to the final name. Sometimes even 10 seconds
Experienced following assertion when clicking multiple times on the download link

5) Experienced following assertion when choosing a disk as the destination for download

And I have 2 product suggestions we can bring to Asana:

Does it make sense to rephrase “Open Downloads Folder” in case of custom downloads folder is set?
The download list truncates the file name with "...". Should it be able to show the full filename, at least when the item is selected or on hover?

tomasstrba · 2024-03-22T07:26:07Z

DuckDuckGo/FileDownload/Model/DownloadListItem.swift

            modified = Date()
        }
    }

-    var destinationURL: URL? {
+    var destinationBookmarkData: Data? {


I know that it won't be trivial to rename this across the whole PR, but I'd recommend to avoid using 'bookmark' word in this context, because it might be very confusing when looking at the codebase later after the merge. Would destinationPersistedData, tempFilePersistedURLData, etc, work better?

I‘d stick with the API naming (i.e. bookmarkData), we could use URLBookmarkData or FileBookmarkData if it‘s better

DuckDuckGo/FileDownload/Model/FileDownloadManager.swift

tomasstrba · 2024-03-22T07:41:49Z

DuckDuckGo/FileDownload/Model/FilePresenter.swift

+            delegate?.presentedItemDidMove(to: newURL)
+        }
+
+        final func accommodatePresentedItemDeletion() async throws {


Is async necessary here?

tomasstrba · 2024-03-22T07:49:31Z

DuckDuckGo/FileDownload/Model/WebKitDownloadTask.swift

-                                             completionHandler: @escaping (URL?, UTType?) -> Void)
-    func fileDownloadTask(_ task: WebKitDownloadTask, didFinishWith result: Result<URL, FileDownloadError>)
+    func fileDownloadTaskNeedsDestinationURL(_ task: WebKitDownloadTask, suggestedFilename: String, suggestedFileType: UTType?) async -> (URL?, UTType?)
+    func fileDownloadTask(_ task: WebKitDownloadTask, didFinishWith result: Result<Void, FileDownloadError>)
 }

 /// WKDownload wrapper managing Finder File Progress and coordinating file URLs
 final class WebKitDownloadTask: NSObject, ProgressReporting, @unchecked Sendable {


WebKitDownloadTask class is quite complex and covers a wide range of functionalities. Is it possible to break it down into smaller components?

I‘ve moved out the progress/fileProgress from it, otherwise it‘s now mainly related to file coordination

mallexxx · 2024-03-22T12:45:51Z

TBD: test on macOS 11

mallexxx · 2024-03-22T14:14:04Z

Fixed the issues we‘ve discussed including the "duckload" download corner case,

After clicking "clear" in the Downloads panel, .duckload files were not removed from Desktop (custom download location)

actually I couldn‘t reproduce this one, is there some special steps?

Removing a completed download file from Downloads panel didn't remove it from the file system.

it shouldn‘t delete completed files

It often takes very long to see .duckload being renamed to the final name. Sometimes even 10 seconds

can you provide more detailed description for this one?

Experienced following assertion when clicking multiple times on the download link

out of scope for now

Experienced following assertion when choosing a disk as the destination for download

should be fixed now. although it wouldn‘t display any error for it but let‘s handle it separately

Does it make sense to rephrase “Open Downloads Folder” in case of custom downloads folder is set?

Probably it doesn‘t since the behavior of the button may vary - if there‘s only files downloaded to custom directories - it will try to locate the file instead of opening a "downloads" dir

The download list truncates the file name with "...". Should it be able to show the full filename, at least when the item is selected or on hover?

it displays a tooltip with a full name

tomasstrba

> actually I couldn‘t reproduce this one, is there some special steps?
As discussed on MM, the behavior is expected.

> It often takes very long to see .duckload being renamed to the final name. Sometimes even 10 seconds
I don't experience this anymore. In the previous testing, a download finished correctly and I could see it completed in the Download panel. But in Finder, I saw .duckload file and it took ~10 seconds until it changed.

I performed the testing again on macOS 14, both sandboxed and non-sandboxed app. Everything works great! 💯 Job job, Alex 👏

Two last comments from me:

Please don't forget to perform smoke tests on macOS 11
One more product suggestion is to keep Downloads panel button in the stack after the manual activation for at least a minute. It was slightly annoying to activate it through the menu all the time.

…oads

This reverts commit 6ee57a4.

mallexxx added 5 commits March 14, 2024 12:49

add sandbox test tool

e0fdf98

downloads file presenters

f4f2caa

Merge branch 'main' into alex/fix-failed-downloads

1cd3048

commit missing file contents

9df235d

Merge remote-tracking branch 'origin/alex/fix-failed-downloads' into …

e041179

…alex/fix-failed-downloads

fix resuming issues, fixing some tests

ada3b50

mallexxx requested a review from tomasstrba March 22, 2024 05:43

github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Mar 22, 2024

tomasstrba reviewed Mar 22, 2024

View reviewed changes

tomasstrba assigned mallexxx Mar 22, 2024

fix release build, some fixes

45155e3

mallexxx added 2 commits March 22, 2024 19:41

open downloads popup when opening a "duckload" file

0947e9e

fix PR comments, corner cases and tests

ec922ca

mallexxx marked this pull request as ready for review March 22, 2024 13:44

validate destination is accessible before firing the download

09e1d7a

replace breakByRaisingSigInt with raise(SIGINT) and log message

c175e61

github-actions bot removed the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Mar 22, 2024

mallexxx added 3 commits March 22, 2024 21:00

fix non-sandbox file presenter tests

378a48e

disable sandbox tool logger

b1d69f0

fix linter issues

f778d98

tomasstrba approved these changes Mar 31, 2024

View reviewed changes

mallexxx added 6 commits April 1, 2024 16:07

Merge remote-tracking branch 'origin/main' into alex/fix-failed-downl…

38d9321

…oads

fix integration tests

cbeeff2

fix UserDialogRequest leak

915f0ec

extend FilePresenterTests timeout

556574a

add app group entitlement

2d94fe7

fix changing file letter case causing file removal detection

e8bc8dd

mallexxx added 7 commits April 1, 2024 22:20

disable FilePresenterTests on CI

1b78e1a

fix app hanging when cancelling downloads on Quit in RELEASE

8866fdf

extend fileReadPromise timeout

a6d291b

fix flaky test

f7ffaec

fix warnings

0d59476

cleanup

6ac1784

fix linter warning

b357e49

mallexxx merged commit 6ee57a4 into main Apr 2, 2024
17 checks passed

mallexxx deleted the alex/fix-failed-downloads branch April 2, 2024 08:23

mallexxx added a commit that referenced this pull request Apr 4, 2024

Revert "Fix failed downloads (file coordination) (#2467)"

39bd5ba

This reverts commit 6ee57a4.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failed downloads (file coordination) #2467

Fix failed downloads (file coordination) #2467

mallexxx commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024 •

edited

Loading

tomasstrba left a comment

tomasstrba Mar 22, 2024

mallexxx Mar 22, 2024 •

edited

Loading

tomasstrba Mar 22, 2024

mallexxx Mar 22, 2024

tomasstrba Mar 22, 2024

mallexxx Mar 22, 2024

mallexxx commented Mar 22, 2024

mallexxx commented Mar 22, 2024

tomasstrba left a comment

Fix failed downloads (file coordination) #2467

Fix failed downloads (file coordination) #2467

Conversation

mallexxx commented Mar 21, 2024 • edited Loading

Internal references:

github-actions bot commented Mar 21, 2024 • edited Loading

tomasstrba left a comment

Choose a reason for hiding this comment

tomasstrba Mar 22, 2024

Choose a reason for hiding this comment

mallexxx Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

tomasstrba Mar 22, 2024

Choose a reason for hiding this comment

mallexxx Mar 22, 2024

Choose a reason for hiding this comment

tomasstrba Mar 22, 2024

Choose a reason for hiding this comment

mallexxx Mar 22, 2024

Choose a reason for hiding this comment

mallexxx commented Mar 22, 2024

mallexxx commented Mar 22, 2024

tomasstrba left a comment

Choose a reason for hiding this comment

mallexxx commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024 •

edited

Loading

mallexxx Mar 22, 2024 •

edited

Loading