Automatically remove incomplete file downloads

[KoBeWi]

When HTTPRequest fails to download a file (either by download error or manual cancel), the file is saved anyway and is incomplete (corrupted). There is no way to resume the download AFAIK, so there is no reason to keep the file.

This PR makes incomplete files automatically deleted. I don't know much about HTTPRequest, so not sure if there isn't another code path for successful downloads, but I tested that it works correctly with simple requests.

This change is needed for #117072. I tried manually removing the file after failed/canceled request, but it does not work for whatever reason 🤷‍♂️

[Alex2782]

There is no way to resume the download AFAIK

That should make it possible:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/Range_requests

verified with python and gdscript (HTTPRequest)

[KoBeWi]

Well, it still needs to be fixed until that is implemented. And even then, this could be optional behavior (with a property auto_remove_failed_downloads or something).

EDIT:
Also resuming is useless if the user cancels downloading. Currently this case also leaves a corrupted file.

[akien-mga]

Thanks!

[Naros]

Hi @KoBeWi I found locally this behavior breaks specific download scenarios, e.g. when fetching release information from a repositories requests endpoint and saving the json as a file.

Error OrchestratorUpdaterButton::_send_http_request(const String& p_url, const String& p_filename, const Callable& p_callable) {
  HTTPRequest* request = memnew(HTTPRequest);
  request->set_download_file(p_filename);
  add_child(request);

  request->connect(
    "request_completed",
    callable_mp_lambda(this, [=](int result, int code, const PackedStringArray& headers, const PackedByteArray& data) {
      if (result == HTTPRequest::RESULT_SUCCESS && code == 200) {
        p_callback.call();
      }
    }),
    CONNECT_ONE_SHOT);

  const Error error = request->request(url);
  if (error != OK) {
    request->queue_free();
  }

  return OK;
}

In this example, the expectation is that we initiate a request to download the file, it gets downloaded and the callback then fires. However, by the time the callback fires, the file no longer exists, and cannot be parsed.

In the HTTPRequest code this goes into the STATUS_CONNECTED block, we received a body but the body length is less than 0, chunked transfer is done. AFAICT, this breaks chunked downloads?

				if (body_len < 0) {
					// Chunked transfer is done.
					_defer_done(RESULT_SUCCESS, response_code, response_headers, body); // <- this is whats called
					return true;
				}

This allows us to retain the old JSON cache files for release metadata across timer calls to this code, so that in the event of rate limits, we have stale data to work with.

We can work around this by using the PackedByteArray input argument to write the bytes to a file in the callback, but this does introduce a change in behavior that did not previously exist.

[Naros]

I'm also concerned with the description of download_file on the HTTPRequest that says

The file to download into. Will output any received file into it.

Nothing here expresses that the location is simply temporary and will be removed.

[KoBeWi]

// <- this is whats called

Looks like download_complete should also be true there, as the download technically completes, no? The file is supposed to be deleted only if the download fails (or gets canceled) and is incomplete.