Copy low level retry does not respect pacer.retryAfterError

Suspected Bug
1

What is the problem you are having with rclone?

For the copy operation, if Put() or Update() functions of any backend are called, the backends use CallNoRetry for the actual upload operation since the retry is supposed to be done using the low level retry, not using the pacer so that the file may be re-opened. If a pacer.retryAfterError is returned by the backend, we will not retry the error. Currently, we only handle fserrors.retryError

What is your rclone version (output from rclone version)

v1.54.0

The command you were trying to run (eg rclone copy /tmp remote:tmp)

rclone copy <local_path> <remote_name>:<remote_path>

Is the above understanding correct? If so, what should be the best way to make the Copy() operation respect pacer.retryAfterError - should it just be an explicit sleep, or can we make it more sophisticated and somehow use pacer for the low lever retries?

2

What needs to be done is obey the retry after error in the Copy retry loop - probably about here

So see if the error is a retry error and sleep on it like is done in the pacer code.

Confusingly there seem to be two types of retry after error in the rclone code base!

How did you come across this error?

3

That was my thought as well after creating this post. Please see if these changes look fine:

There are not. One is RetryError, another is RetryAfterError. One of them suggest if an error want to be retried, while the other suggests that an error wants to be retried and specifies the duration after which it wants to be retried.
Of course, both can, and should be condensed into a single error. Do you want me to make such a change within this MR (considering this is a bugfix, and merging both errors is more of a code improvement which will also require changes across multiple backends).

I was looking at the best way to handle the 429 status code and the Retry-After header for a custom backend.

4

See github for review :slight_smile:

Hmm, I suggest we leave as-is for the moment as the pacer RetryAfter should be low-level retried, but the other error shouldn't - it should be high level retried in the cmd/cmd.go loop I think.

5

Both are low-level retried. Below is how the code looks after my changes:

		if fserrors.IsRetryError(err) || fserrors.ShouldRetry(err) {
			retry = true
		} else if t, ok := pacer.IsRetryAfter(err); ok {
			fs.Debugf(src, "Sleeping for %v (as indicated by the server) to obey Retry-After error: %v", t, err)
			time.Sleep(t)
			retry = true
		}

fserrors.IsRetryError already exists in RClone, and checks for fserrors.retryError, and the second check (pacer.IsRetryAfter) I added to retry pacer.retryAfterError.

6

I'm talking about this error

Which isn't retried by IsRetryError or ShouldRetry

It is checked here

It confused me anyway :wink:

7

I missed this RetryAfter error. I am pretty sure this is an unintended duplication. It is used only in the Swift backend, and intention seems to be same as pacer.RetryAfterError.

8

What it is used for in the swift backend is to say attempt all the copies, but if any failed with this error then wait this long before doing a complete retry. What it is for is so that users can get things out of cold storage which takes time, but they do them all at once and wait for all of them at once at the high level retry.

So yes, these are similar, but one should be low level retried and one should be high level retried...

9

This topic was automatically closed 60 days after the last reply. New replies are no longer allowed.