Repository navigation
fix(browser): move a finished download off the CDP listener - #961
Merged
Merged
Conversation
A completed download was moved into Downloads under the downloads lock,
inside Chrome's browser event listener. Across filesystems the move is a
copy as long as the file, so every browser event, and every other
download, waited behind it.
progress now only keeps the name reserved; place moves the file outside
the lock, on its own goroutine, and the agent hears completed once the
file is in place. A move that fails is reported as a new failed state
("download failed: <path>") instead of the staged path, which goes with
the profile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #960, closing the two limits it left.
Why
downloadslock, inside Chrome's browser event listener. Across filesystems the move is a copy as long as the file, so every browser event, and every other download'sbegin, waited behind it.What changed
progressno longer moves: a completed download keeps its name reserved.placemoves it outside the lock, on its own goroutine from the listener, then frees the name. Since the session only hearscompletedfromplace, the agent's click still waits for the file to be in place, and the window's toast still names a file that exists.api.DownloadFailed("failed", lich's own, not Chrome's), with the path it would have taken; the CLI page text saysdownload failed: <path>. The window emits no toast for it, as for a canceled one.docs/ceilings.mdanddocs/browser-tab.mdupdated.Tests
TestDownloadsKeepTheNameAndTheLockFreeWhileTheFileMovesbegins another download from inside the move. It fails if the lock is held during the move (checked by moving the lock back), and the new download getsa (1).txtbecause the name is still reserved. New:TestDownloadLineSaysAFailedMove.progressno longer moves the file and returns no error;placedoes the move): the existing cases go through acompletehelper that calls both, as the listener does.TestDownloadsReportTheWrittenFileWhenTheRenameFailsbecameTestDownloadsReportFailedWhenTheMoveFails: a failed move now reportsfailedwith the reserved path, not the staged path, and frees the name.-race -count=50on the download unit tests is green. The downloads live tests ran-count=20on Linux, 20/20. The fullgo test ./...,go vetand the linux, darwin and windows cross-compile loop are clean.