use reqwest_retry to automatically retry failed requests

[jubrad]

supersedes #12

but non forked.

attempts to fix #10

Adds retries via reqwest_retry / reqwest_middlware.

Caution!!!
I'm bad at rust

[evanharmon]

Thank you for tackling this issue! 😄

[benesch]

Thanks for working on this! I want to call out an important consideration: the API client today intentionally doesn't retry requests because retrying a mutating operation is not guaranteed to be idempotent. E.g., retrying a create user request might cause it to create duplicate users.

I think it'd be much safer to only retry read requests (e.g., get tenant, get user), since those can be retried without concern.

I'm not sure how you'd make reqwest_retry work in this world. You may find that it's easier to implement conditional retries using Materialize's in-house Retry library. It doesn't have any support for HTTP/reqwests specifically—it operates on the level of a fallible async function—but lets you classify errors as transient/fatal as you like.

That said, the logic that reqwest_retry uses to determine whether to retry a request looks really solid: https://github.com/TrueLayer/reqwest-middleware/blob/b8b9400858f469c438bcf00fae6cd4b980dc1350/reqwest-retry/src/retryable_strategy.rs#L106-L196

Would be a real shame to reimplement that in house.

(Aside: the logic is really geared for idempotent API calls. You don't want to retry a write API request if you see a network error or server error because you don't know what happened—the request may or may not have succeeded—but it is technically safe to retry a write API call on a 429 error, because that's the server explicitly telling you that it didn't process your API call. )

Maybe the answer is for the frontegg::Client to hold two reqwest clients? One configured for retries and one not? And then each method on the client could determine which client to use, based on whether it was making a read or write API call.

[jubrad]

Yeah I had the same concern about create retries. I also dug into the conditions they use for retries. I think it's probably good, but agree there's some risk.

I see some options:

1. Providing an easier interface to get a non retrying client so that downstream users can more easily use 2 clients 1 for create 1 for other.
2. Create a custom retryable strategy that's more protective.
3. A multi-client wrapper that allows you to create two clients and choose which methods you want to use for each.

Questions I have:
If the server is providing an error message denying the request due to rate limits, it should definitely be hitting the server middleware before getting to actual application code that could create the resource, this is true about many server http responses (definitely not internal server error). How much do we trust this?

If we trust the server, I think we probably just don't want to trust any network related errors, for create.
Would it be enough to just make this always return Some(Retryable::Fatal) on the reqwest failure here

[benesch]

How much do we trust this?

I think I'd trust it for a 429 but nothing else.

I think to keep things simple at the start, my preference would be for this crate to be opinionated and apply exactly the code you have here for read API requests only. So list_users, get_user, etc would all use the default retry policy, and create_user etc would do zero retry handling and say that it's for the user of this library to handle. That should totally solve our needs (our production code doesn't use create_user IIRC; it's here literally just to facilitate this crate's own tests) and lets us punt on the hard questions of idempotency for now.

[jubrad]

Issues compiling wiremock, had no issues locally.
pin became stable in 1.68.0, this was using 1.65.0, I've updated rust to 1.68.2
rust-lang/rust#103800

error[E0658]: use of unstable library feature 'pin_macro'
   --> /home/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/wiremock-0.5.19/src/mock_server/bare_server.rs:247:32

[benesch]

Thanks very much for this! I pushed up fixes for a few formatting nits and will merge in a second.