Make shopify theme dev more resilient to HTTP errors with the Admin API
#4606
Conversation
|
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Test suite run success1943 tests passing in 876 suites. Report generated by 🧪jest coverage report action from 707b901 |
|
🚀 Thank you! This looks great. I left a quick question about the retry time |
|
|
||
| outputDebug(`[${retries}] Retrying '${path}' request...`) | ||
|
|
||
| await sleep(0.2) |
There was a problem hiding this comment.
Was there a decision that led you to using 0.2 seconds as our retry "cooldown"?
There was a problem hiding this comment.
Maybe we should use different cooldowns depending on the error type... 0.2 seems sensible for something that is likely to keep failing, like a 401, so that we fail fast.
But perhaps a 503 could recover if we give it more time? Not sure really 🤔
At the same time, we are also adding another type of error handling in #4599 , so maybe it's good to fail fast here and let the consumer handle the situation...
| case status === 401: | ||
| throw new AbortError(`[${status}] API request unauthorized error`) | ||
| // Retry 401 errors to be resilient to authentication errors. | ||
| return handleRetriableError({ |
There was a problem hiding this comment.
If we don't refresh the token, I'm not sure if this will do anything? I guess it doesn't hurt much but the token will be the same so I assume the API will keep returning 401?
Perhaps we should explore the option of "refresh on demand" when auth errors happen in addition to refreshing periodically in an interval? For example, something like session.refresh(). Or perhaps a job for AsyncLocalStorage if it's too hard to pass that logic down here 🤔
|
|
||
| outputDebug(`[${retries}] Retrying '${path}' request...`) | ||
|
|
||
| await sleep(0.2) |
There was a problem hiding this comment.
Maybe we should use different cooldowns depending on the error type... 0.2 seems sensible for something that is likely to keep failing, like a 401, so that we fail fast.
But perhaps a 503 could recover if we give it more time? Not sure really 🤔
At the same time, we are also adding another type of error handling in #4599 , so maybe it's good to fail fast here and let the consumer handle the situation...
WHY are these changes introduced?
Related to #4576, #4598, and #4599
Retry requests for some HTTP errors. This PR doesn't propose to refresh the session because we're already refreshing it every 30 minutes, the Admin session token expires in 2 hours, and we're getting a fresh token when we start the session.
WHAT is this pull request doing?
This PR updates the Admin REST API client to retry requests when they encounter certain HTTP errors. The Ruby CLI was more permissive regarding execution abortion, so we're adopting this approach as a middle-term to: fail when errors really happen but also be resilient and retry in some scenarios.
How to test your changes?
shopify theme push -de806941bd7538a0325593da4c02663c599f18e64in your local CLI (it will trigger an error every time we update a file)layout/theme.liquidfileshopify theme push -d --verbosePost-release steps
N/A
Measuring impact
How do we know this change was effective? Please choose one:
Checklist