Preserve partial downloads on network failure

[FranciscoTGouveia]

Closes #4448.

If resume_from_partial is set to true, it short-circuits, ensuring the remove_file(path) is not called.

[rami3l]

@FranciscoTGouveia Looks easy enough! I'll give it a test drive when I'm back with my machine :)

[rami3l]

Many thanks for investigating into this!

OTOH I do think we need to be more careful about filtering the right kind of error since we do want to redownload if the cache is proven to be corrupted for example 🙏

[rami3l]

Question: How did you perform the test and/or is there a way to stably demonstrate the effectiveness of this change?

[FranciscoTGouveia]

I performed it locally: during an installation, I dropped my internet connection, and after rustup toolchain install failed, I looked in .rustup/downloads for .partial files.
Where, before this patch, there were none, they were now left untouched.
To ensure this only happens when we are certain, I whitelisted the situations in which the removal should not be allowed (e.is_timeout() || e.is_connect()).

Would it be preferable to add a unit test for this?

[rami3l]

@FranciscoTGouveia A test for this would be cool if that's fairly easy to do. That said, I'm not completely sure about the feasibility, so feel free to tell me if it turns out too difficult :)

[rami3l]

@FranciscoTGouveia I have one remaining concern with this change.

When trying to reproduce your scenario above on my machine:

info: rolling back changes
error: component download failed for cargo-aarch64-apple-darwin: partially downloaded file may have been damaged and was removed, please try again: [..] client error (Connect) [..]

... the .partial is actually saved which is okay. The transaction system is rolling back changes which is also okay. However the partially downloaded file may have been damaged and was removed part might be misleading. Have you encountered something similar and, do you have an idea of fixing that, by adjusting the wording or by adding a flag to be passed over? Many thanks again! 🙏

[FranciscoTGouveia]

I was thinking of extracting the is_network_failure to an external function and using it to determine which error message to throw, implying that I will create a new error message.
However, I can also simply delete the ...and was removed part from the current BrokenPartialFile error.

[FranciscoTGouveia]

Is IncompletePartialFile a good error name?

I used a match statement here because I think it is the most readable approach.
Please let me know if you think I should change it.

[rami3l]

LGTM modulo a minor wording change and some nits, nice work :)

When you are done, just squash the first 3 commits into one (they include some back-and-forth) and we are good to go.

[rami3l]

I think we can make this even easier to read: "partially downloaded file was kept for resumption, please try again"

[rami3l]

Question: What is the point of this listener here? You don't seem to be using it anyway?

If it's just for the address, maybe just construct it directly, and probably use an explicitly invalid address such as 240.0.0.0:1080?

- let listener = TcpListener::bind("127.0.0.1:1080").unwrap();
- let addr = listener.local_addr().unwrap();
- drop(listener);
- let from_url = format!("http://{addr}").parse().unwrap();
-
+ let from_url = format!("http://240.0.0.0:1080").parse().unwrap();