Add retry option to AsyncTAPJob.fetch_result for transient failures

[stvoutsin]

Problem

(Reposting from issue)
Queries via Pyvo may occasionally run into intermittent/transient networking issues during result fetching, forcing users to re-run entire queries. We've seen this during testing from our automated bots, but have not narrowed down the cause yet, and it is very likely that this is an issue specific to our setup. However I have seen these one-off behaviors with other TAP services out in the wild as well, so I'm wondering if this is something worth addressing in pyvo.

Solution

Add configurable retry logic specifically for transient network errors during result fetching. This mirrors retry patterns used in other HTTP clients.

Changes

Add max_retries parameter to fetch_result()
Implement exponential backoff with jitter
Perhaps only retry on clearly transient errors? (connection resets, timeouts)
Should be fully backwards compatible
I think something like this has the benefit of improving reliability and reducing the need for users to implement their own retry logic.

Addresses issue #695

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@3fe6e80). Learn more about missing BASE report.
⚠️ Report is 65 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #696   +/-   ##
=======================================
  Coverage        ?   84.09%           
=======================================
  Files           ?       80           
  Lines           ?     8556           
  Branches        ?        0           
=======================================
  Hits            ?     7195           
  Misses          ?     1361           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 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.

[msdemlei]

I'm not quite sure how a ChunkedEncodingError could be transient on a reliable channel like TCP, but I'm happy to take your word for it if you say that's been a reasoned choice.

I'd also say that a bit of randomness is a good idea in the backoff, but random.uniform(0,1) is overdoing things (which, incidentally, I'd write as random.random()). How about random.uniform(0.8, 1)?

The second enumeration of the transient exceptions (line 1078f) should also use the symbolic name.

And I wonder if the code didn't become clearer if you did without last_exception and re-raised or updated as appropriate in the exception handlers. I think in this case I'd prefer a tiny bit of repetition over a variable changed in a fairly complex pattern.

[stvoutsin]

I'm not quite sure how a ChunkedEncodingError could be transient on a reliable channel like TCP, but I'm happy to take your word for it if you say that's been a reasoned choice.

I'd also say that a bit of randomness is a good idea in the backoff, but random.uniform(0,1) is overdoing things (which, incidentally, I'd write as random.random()). How about random.uniform(0.8, 1)?

The second enumeration of the transient exceptions (line 1078f) should also use the symbolic name.

And I wonder if the code didn't become clearer if you did without last_exception and re-raised or updated as appropriate in the exception handlers. I think in this case I'd prefer a tiny bit of repetition over a variable changed in a fairly complex pattern.

Thanks for the review @msdemlei, I've made some changes to hopefully address your comments.
In terms of ChunkedEncodingError, I do think most of the time this indicates a structural issue rather than temporary networking conditions so I've removed it.
I initially had it in there because I think I have encountered it with large downloads being interrupted, but I haven't logged enough information about the occurrences, which are also much rarer than the other two so I've removed it for now, and we can always add it later if we do notice a transient nature to it.

[msdemlei]

I'm happy with that improvement, thanks.

@bsipocz, would you also have a quick look and then merge if you don't find any major snags?

For the record, I am not opposed to classifying ChunkedEncodingError as temporary if it turns out that's a common failure mode; it's just that I couldn't see a probably scenario for that. So, totally feel free to add it back if you find it'd be useful.

[bsipocz]

@bsipocz, would you also have a quick look and then merge if you don't find any major snags?

I don't really have much opinion about ChunkedEncodingError, but the tests should pass before we can merge this.

[stvoutsin]

@bsipocz, would you also have a quick look and then merge if you don't find any major snags?

I don't really have much opinion about ChunkedEncodingError, but the tests should pass before we can merge this.

Yea sorry about that, I forgot to remove the relevant test case for this error. I've removed it now and it looks like the tests all passed.

[bsipocz]

Thank you @stvoutsin!