Skip to content

Conversation

@AaronVr
Copy link

@AaronVr AaronVr commented Jan 21, 2026

Currently the Transfer.transfer function automatically retries 3 times when a TransferException is raised. This behaviour is not desirable when the cause of the exception can not possibly be resolved by retrying the function. One such case is a 404 error indicating a missing file.

As regards the Transfer.transfer function, the TransferException can be raised in the following places:

  • _fetch_size (lines 126)
  • _prepare_target_transfer (line 219)
  • _transfer_file (lines 242, 252, 263, 269, 281, 287, 301, 312, 321)
function line change note
_fetch_size 126 Y Pertains to a possible 404 error
_prepare_target_transfer 219 N General SSH error
_transfer_file 242 N Checks curl stderr and should does not imply a 404 error
_transfer_file 252 Y Relates to a possible 404 from curl
_transfer_file 263 N Pertains to an IndexError, can be refactored out
_transfer_file 269 N General SSH error
_transfer_file 281 N General size mismatch error
_transfer_file 287 N General OSError
_transfer_file 301 N General OSError
_transfer_file 312 N General SSH error
_transfer_file 321 N General OSError

Some further explanation regarding the two cases where a code change is necessary:

_fetch_size line 126: Here a HEAD request is sent to the source file, defined by a source url, which could possibly return a 404. In that case, there is no file to be transferred and we should not retry.

_transfer_file line 252: Here we notice that our cURL command, which attempted to download the source file, returns a 404. As in the previous case, this implies that there is no file to be transferred and we should not retry.

A small refactor of the _transfer_file function has been done as well to make the logic more legible and reduce nesting.

As the documentation was outdated, I have pushed a commit to ensure that the README setup and testing documentation are correct. I have also updated the Dockerfile so that the user has correct permissions for the workdir so tests work in the service's Docker container.

NOTE: There are probably a range of other status codes that can be used as a heuristic to decide whether to retry or not. Intuitively I assumed this would be true for all 4xx client errors, but @spacid reminded me that 429 can actually be resolved using a retry (though whether our retry method is appropriate in this case, is unclear).

The documentation was outdated and out of sync with the Dockerfile. The
README instructions were changed to make sure they actually work to set up
and test the service and the Dockerfile now sets correct workdir
permissions to enable tests to run in the service's Docker container.
Introduced a `TransferSourceFileNotFoundException` class that will not
trigger the `retry` decorator on `Transfer.transfer`. Modified
`Transfer._fetch_size` and `Transfer._transfer_file` to raise this
exception instead of `TransferException` when a 404 regarding the
source file is concerned. Added unit tests for expected behaviours.
Fixed the name of `TransferSourceFileNotFoundException` in the
`Transfer._fetch_size` docstring. Refactored a list comprehension to a copy
to make the code more idiomatic.
@AaronVr AaronVr requested a review from spacid January 21, 2026 15:32
@AaronVr AaronVr self-assigned this Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants