[pmaciel]

Description

eckit_test_geo_cache failed on some networks (ecmwf/eckit#370). The test downloads https://does.not/exist and expects an error, but no error was thrown.

In this case, a proxy answers on behalf of the unreachable host with its own small error page (HTTP 503, 515 bytes). cache::Download::to_path never looked at the HTTP status (it isn't available usign a DataHandle). It treated a download as successful if the body was non-empty and didn’t contain the exact text . The proxy’s page passed both checks, so it was saved as if it were the requested file. On a direct connection the host just doesn’t resolve, which is why the failure only showed up behind a proxy.

The same weakness could put proxy, captive-portal or login pages into the download cache, and those would then be read as grid or shapefile data.

Solution

to_path now requires positive evidence that a download succeeded, instead of guessing from the content:

Transport errors fail the download. DNS, connection, SSL and partial-transfer errors from curl are reported, not swallowed.
For http(s), the status must be 2xx. Code 0 (no HTTP response at all) also fails.
HTML is rejected unless the caller allows it (html=true). This is based on the Content-Type header (text/html, application/xhtml+xml), not on sniffing the body.
The status and headers are checked twice:
before the body is read, so a 404 or 503 fails without downloading anything;
after the transfer, against the final response, because behind a tunnelling proxy the status known before the body can belong to the proxy’s CONNECT reply.
If the server sent Content-Length, the number of bytes received must match it.
Empty responses still fail.
Other schemes such as file:// have no status or headers, so only transport errors and empty files count as failures there.

The status and header rules are in a new static function, Download::validate_response(url, code, headers, html), so they can be tested without a network. to_path uses EasyCURL directly instead of URLHandle, because URLHandle doesn’t expose the response status or headers.

On failure, the partial .part file is removed and a new exception::DownloadError is thrown. It follows the existing geo exception pattern and includes the URL, the target path and the reason (for example HTTP status 503).

Without curl, to_path now throws DownloadError (“eckit built without CURL support”). It used to return silently with no file, which then tripped an assertion in to_cached_path. Files already in the cache are still used without curl.

Contributor Declaration

By opening this pull request, I affirm the following:

• All authors agree to the Contributor License Agreement.
• The code follows the project's coding standards.
• I have performed self-review and added comments where needed.
• I have added or updated tests to verify that my changes are effective and functional.
• I have run all existing tests and confirmed they pass.

🌦️ >> Documentation << 🌦️
https://sites.ecmwf.int/docs/dev-section/eckit/pull-requests/PR-376

[pmaciel]

@danovaro can you review this quickly? It's about solving downloading issue by directly using CURL (instead of teh DataHandle) because it's much better to access the HTTP code to determine failure/success.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 99.11504% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.82%. Comparing base (c55f47e) to head (02da1e0).

Files with missing lines	Patch %	Lines
src/eckit/geo/cache/Download.cc	95.34%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #376      +/-   ##
===========================================
+ Coverage    68.76%   68.82%   +0.05%     
===========================================
  Files         1189     1190       +1     
  Lines        63067    63233     +166     
  Branches      4746     4758      +12     
===========================================
+ Hits         43368    43518     +150     
- Misses       19699    19715      +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[danovaro]

lgtm, thanks @pmaciel

one question though: why DiskCache (and subclasses) are part of eckit geo?
It might be useful to move them in a more visible part of eckit

[pmaciel]

lgtm, thanks @pmaciel

one question though: why DiskCache (and subclasses) are part of eckit geo? It might be useful to move them in a more visible part of eckit

Yes, this is a point that will need revisiting; There is an amount of in-memory caching mechanisms currently in eckit::geo for basic functionality, but a much more complex one is coming soon separately (#374), to act in mir-like environments - multi-thread, multi-process parallelism with limited disk and memory cache resources. So I'm factoring as I can, It might be moved out the really required functionality is known and/or implemented.