Note for reviewers

On 4 August 2022 I reverted all changes in this PR so I could reimplement and apply all your review feedback. This happened in fa1fd50. Every subsequent commit encapsulates one logical change to the code. Working through them one-at-a-time should be quite easy.

1. Isolate delay bounding bf65b81
2. Move error details scope up one level a0d340e (H/T @moderakh )
3. Retry GetOperationStatus for OSError a55cf9d (thanks @sander-goos and @benfleis )
4. Log when we do it 38411a8 (H/T @benfleis )

The final four commits are simple code cleanup and fixing one warning in the test suite un-related to this change.

Since our e2e tests are not enabled via Github Actions yet I ran them locally and all passed.

Description

Currently, we only retry attempts that returned a code 429 or 503 and include a Retry-After header. This pull request also allows GetOperationStatus requests to be retried if the request fails with an OSError exception. The configured retry policy is still honoured with regard to maximum attempts and max retry duration.

Background

The reason we only retry 429 and 503 responses today is because retries must be idempotent. Otherwise the connector could cause unexpected or harmful consequences (data loss, excess resource utilisation etc.)

We know that 429/503 responses are idempotent because the attempt was halted before the server could execute it, regardless if the attempted operation was itself idempotent.

We also know that GetOperationStatus requests are idempotent because they do not modify data on the server. Therefore we can add an extra case to our retry allow list:

• If a request is a GetOperationStatus command
• If it fails due to an OSError such as TimeoutError or ConnectionResetError.

Previously we attempted this same behaviour by retrying GetOperationStatus requests, regardless the nature of the exception. But this change could not pass our e2e tests because there are valid cases when GetOperationStatus will raise an exception from within our own library code: for example, if an operation is canceled in a separate thread GetOperationStatus will raise a "DOES NOT EXIST" exception.

Logging Behaviour

The connector will log whenever it retries an attempt because of an OSError. It will use log level INFO if the OSError is one we consider normal. It will use log level WARNING if the OSError seems unusual. The codes we consider normal are:

                                            # | Debian | Darwin |
                    info_errs = [           # |--------|--------|         
                        errno.ESHUTDOWN,    # |   32   |   32   |
                        errno.EAFNOSUPPORT, # |   97   |   47   |
                        errno.ECONNRESET,   # |   104  |   54   |
                        errno.ETIMEDOUT,    # |   110  |   60   |
                    ]

The full set of OSError codes is platform specific. I wrote this patch to target a specific customer scenario when GetOperationStatus requests were retried after an operating system socket timeout exception. In this customer scenario the error code was Errno 110: Connection timed out. However that error code is specific to Linux. On a Darwin/Macos host the code would be 65 and on Windows it would be 10060.

Rather than catch these specifically, I use Python's errno built-in to check for errno.ETIMEDOUT which will resolve to the platform-specific code at runtime. I tested this manually on Linux and MacOS (but not on Windows). @benfleis helped me pick what we consider "normal".

We log all other OSError codes with WARNING is because it would be pretty unusual for a request to fail because of a "FileNotFound" or a system fault.

References

I found this article extremely helpful while formulating this fix. The author is solving a very similar problem across platforms.