fix(auto-renewal-token): refresh access token on 401 and retry once so long-running syncs survive token expiry (backport #442) - #450
Merged
Conversation
…o long-running syncs survive token expiry (cherry picked from commit a879473)
Author
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Unicommerce API client (
UnicommerceAPIClient) resolves an access token once at construction and reuses it for the entire lifetime of the client. Long-running jobs — most notably the new date-range old-order sync, which can run for hours — outlive the token's TTL. When the token expires mid-run, every subsequent call fails and the job aborts, even though the token was trivially refreshable.This PR makes the client reactively refresh the token on an HTTP
401and retry the failing request once, so in-flight jobs continue transparently.Issue / Problem
File:
ecommerce_integrations/unicommerce/api_client.py__initialize_auth()setsself._auth_headers(theBearertoken) a single time when the client is created.renew_tokens()refreshes the token before a request, and only ifexpires_onhas already passed at construction time.request()had no handling for an auth failure at call time. On any non-2xx response,raise_for_status()raises, the exception is caught, and the method returns(None, False).Consequence: for a job that constructs one client and loops for a long time (e.g. the old-order sync fetching 1000 orders/page and creating Sales Orders one by one), once the access token expires mid-run:
401→ treated as a generic failure →(None, False).Fix
Both changes are scoped to
api_client.py:1. New
_refresh_auth()helpersettings.update_tokens(grant_type="refresh_token")to obtain a fresh access token and rebuildsself._auth_headersin memory.Unicommerce Settings(withignore_permissions/ignore_custom_fields) so other clients/workers can reuse it. The save is wrapped intry/except— a persistence failure must not block the retry, which already holds the new token in memory — and logs viafrappe.log_error(...)so repeated failures stay visible.2. Reactive 401 handling in
request()401, the method calls_refresh_auth(), re-applies the fresh header (headers.update(self._auth_headers)), and re-issues the request once. The retry is a single inline step — no recursion and no public parameter — so the guard cannot be bypassed by a caller, and it can never loop.request()is called withfiles=(onlycreate_import_jobdoes this),requestsstreams the file object into the first request's body, leaving the handle at EOF. Replaying it on the retry would silently send an empty body. Such calls skip the refresh/retry path and fail loudly instead, so the upload can be re-run intact rather than losing data.Behavior change
401→(None, False), job aborts401→ refresh + retry once → job continues401persists(None, False)401401→(None, False)(None, False), no silent empty uploadTests
Added
TestTokenRenewRetryinecommerce_integrations/unicommerce/tests/test_client.py(using theresponsesmock), covering:test_401_refreshes_token_and_retries_once— a401triggers exactly one refresh + retry, and the retry carries the freshly refreshed token.test_persistent_401_gives_up_after_one_retry— a persistent401stops after a single retry (no infinite loop) and returns(None, False).test_no_401_does_not_refresh— a successful response never touches the refresh path.test_file_upload_401_is_not_retried— afiles=request on401is not replayed and returns(None, False).Notes / Considerations for reviewers
401is not auth-TTL related.update_tokens(grant_type="refresh_token")falls back to the password grant via_handle_refresh_token_expiry()if the refresh token is itself expired (30-day rotation), so the refresh path is robust end-to-end.401triggers the path; all other status codes and the existing error-logging behavior are unchanged. This is a backward-compatible change to the request flow.update_tokensrotates the refresh token on each call. Concurrent refreshes from multiple workers could race on the persisted token; in practice the primary consumer (old-order sync) runs under a single-run lock, so this is not a concern for that job, but it's worth noting for other callers.Backport needed in v15
This is an automatic backport of pull request #442 done by [Mergify](https://mergify.com).