Fix/auth flow transient recovery - #145
Merged
Merged
Conversation
FOLIO's /authn/login-with-expiry returns accessTokenExpiration as ISO-8601 with a trailing 'Z' (verified against live Okapi and Eureka instances, e.g. '2026-08-07T18:13:36Z'). datetime.fromisoformat cannot parse a 'Z' suffix before Python 3.11, so on Python 3.10 -- a supported version per requires-python -- FolioAuth.__init__ raised ValueError and the client could not authenticate at all. Parse with dateutil.parser.isoparse instead, which handles 'Z' and offsets written without a colon on every supported version. python-dateutil is already a declared dependency. Also: - Coerce naive timestamps to UTC so _token_is_expiring never compares naive to aware datetimes, which would raise TypeError on every request. - Degrade unparseable values to None rather than raising, so a future format change costs the proactive expiry check rather than all authentication. - Extract _token_from_response so the sync and async auth methods share one parse, replacing two near-identical blocks that each called response.json() up to four times.
httpx hands the auth flow an unread response: Client._send_handling_auth only calls response.read() if the flow yields again, and calls response.close() when the flow raises. The 401 path raised HTTPStatusError without reading first, so callers inspecting exception.response.text got ResponseNotRead. The 403 path already did this (b579e7b); this applies the same fix to 401 and covers both with tests. Also change the 403 check from `if` to `elif`. This is not a behavior change -- both branches test the same original response, so they were already mutually exclusive -- but it makes that explicit so a later edit cannot accidentally run the 403 retry after the 401 path has already yielded.
The async auth flow guarded its token refresh with a threading.RLock. An RLock is reentrant per *thread*, and every coroutine on an event loop shares one thread, so when a coroutine held it across an await the next coroutine re-acquired it immediately. It provided no mutual exclusion at all: 10 concurrent flows produced 10 logins. Switching to a plain threading.Lock is worse -- it blocks the loop's own thread, which can then never resume the holder, deadlocking outright with no traceback. Use an asyncio.Lock, which suspends the coroutine rather than the thread, and keep one per event loop in a WeakKeyDictionary: - asyncio.Lock binds to a loop the first time it must actually wait (see _LoopBoundMixin._get_loop), and that binding is permanent. Because an uncontended acquire returns before _get_loop() is reached, a single cached lock passes light testing and then fails under real concurrency. - The permanence means even sequential reuse breaks: one cached lock raises "is bound to a different event loop" when a client is reused across two asyncio.run() calls. That is easy to hit by accident -- asyncio.run() called twice, a session-scoped client fixture under pytest-asyncio, a re-run notebook cell. - Rebuilding a cached lock on loop change fixes that but is worse overall: two live loops replace each other's lock continuously (39 rebuilds, 16 distinct lock objects per loop), destroying exclusion within each loop as well as between them. - Weak keys let finished loops drop out so long-lived clients do not accumulate locks. The registry is guarded by its own threading.Lock rather than the existing _lock. _lock is deliberately held across network I/O by the sync paths, so reusing it here would let a sync login in a worker thread stall the entire event loop for the duration of that login -- unbounded when timeout is None. Also fix FolioClient.async_login, which held _lock across its await and so had the same absence of exclusion, plus the ability to overwrite a newer token with an older one. The residual gap -- sync and async paths using unrelated locks, so two loops or a loop plus worker threads can each log in once concurrently -- is documented in _get_async_lock and accepted: assigning self._token is a single atomic attribute store, and closing the gap would require holding a cross-thread lock across an await. Tests cover the core guarantee (concurrent flows share one login), lock stability within a loop, isolation between concurrent loops, reuse across sequential loops under contention, registry cleanup, the running-loop precondition, and that the registry guard cannot be stalled by a sync login. All four ways of regressing this were verified to fail the intended test.
The 401 handler refreshed only if _token_is_expiring() said the token was old. That is the wrong predicate: a 401 means the server rejected the token we actually sent, and a token can be rejected while still looking locally valid -- clock skew, Keycloak signing-key rotation, session revocation, or a module with a stale JWKS cache. In all of those the old code replayed the identical request, got a second 401, and raised. The recovery path failed to fire in exactly the cases needing recovery; the only 401 it recovered from was a race in the last 60 seconds, which the proactive check would have caught anyway. De-duplicate on token identity instead. Each flow captures the token it sent, and on a 401 refreshes unless self._token is no longer that object -- i.e. unless another thread or coroutine already replaced it, in which case retrying with the newer token is enough. This keeps the "N callers see a 401, one login happens" property while never suppressing a genuinely needed refresh. _token_is_expiring is retained purely as a proactive optimization that saves a wasted round trip on long-running jobs, and its docstring now says so explicitly. The captured token is also passed into _set_auth_cookies_on_request rather than having it re-read self._token, so a concurrent refresh cannot substitute a different token between the identity check and the cookies actually being applied. Adds _ensure_sync_token / _ensure_async_token / _require_token, which removes the duplicated check-and-refresh preamble and lets the two token properties share it. Tests cover: refresh happens on a 401 even when expiry is far off; refresh is skipped when the token was concurrently replaced; the retry carries the new cookies rather than the stale ones; and 8 concurrent threads holding one token produce exactly one login. Each of the three ways to regress this was verified to fail the intended test.
Two problems with the 403 retry, which matters because FOLIO's Keycloak integration currently translates every backend error that is not a 200/401/403 into a 403. In practice a 403 is therefore frequently a masked transient failure -- a timeout, a 502/503, a reset connection -- rather than a real permission denial, which makes this the library's primary transient-recovery path rather than overhead. 1. Multipart uploads were losing the retry. Add _request_is_replayable, which admits multipart. httpx only sets request._content for in-memory bodies, so testing that alone classifies every files= upload as non-replayable even though MultipartStream *is* replayable -- FileField.render_data seeks each field back to 0 on every render, and two sends produce byte-identical bodies (asserted in test_multipart_bodies_really_do_replay_identically). File uploads are exactly where recovering from a masked transient matters most. The check is an allowlist on purpose: if an httpx internal changes and we get it wrong, we skip a retry we could have done rather than replaying a generator (StreamConsumed) or an exhausted file handle, where .read() at EOF yields b"" and the retry silently becomes a zero-length POST/PUT. 2. The retry was immediate, which is poorly matched to a masked transient -- the condition has rarely cleared microseconds later. Wait briefly first, tunable via FOLIOCLIENT_FORBIDDEN_RETRY_DELAY (default 1.0s, 0 restores the previous behavior). Kept short deliberately: a genuine permission denial is indistinguishable today and pays the same cost, and heavier recovery already belongs to folio_retry_on_auth_error, which retries 403 with exponential backoff and a fresh login. Once FOLIO passes real status codes through, 0 becomes the better default and this can be retired. Also, in the 401 branch, refresh before testing replayability. Returning early without refreshing left the rejected token installed, so every later request reused a token the server had already rejected -- and with an unknown expiry the proactive check never fires either, so the client never recovered on its own. A module-scoped autouse fixture zeroes the delay so unrelated tests stay fast; the default is asserted separately so the fixture cannot mask a change to it. Each of the three regressions above was verified to fail the intended test.
FolioAuth._lock is an RLock, but nothing nests and nothing should: every holder calls only _do_sync_auth, which builds a bare httpx.Client with no auth attached and so cannot re-enter the flow, and every `yield request` sits outside the locked region so no caller-supplied code runs while it is held. Instrumenting the lock and running the full unit and integration suites showed zero reentrant acquisitions. That makes threading.Lock viable, but RLock is kept deliberately. The lock spans network I/O with a possibly unlimited timeout, so an accidental nested acquire under a plain Lock would hang the caller's process with no traceback, whereas under RLock it degrades to a redundant login. For a client library that is the better failure mode. To keep the safety net from silently excusing real nesting, assert the invariant in tests instead of relying on the primitive: ReentrancyDetectingLock raises on a nested acquire, and test_sync_lock_is_never_acquired_reentrantly drives the proactive refresh, both token properties, the 401 re-authentication and the 403 retry through it. test_reentrancy_detector_actually_detects guards against the detector silently never firing. Verified by adding a folio_auth_token read inside the locked region, the most plausible accidental nesting: the new test fails with "acquired reentrantly (depth 2)", and test_concurrent_threads_401_trigger_one_reauth also fails with 2 logins instead of 1 -- confirming nesting causes real duplicate work rather than being merely theoretical. The __init__ comment now records why RLock is used and warns against putting an access_token / folio_auth_token read inside a locked region.
Adds FOLIOCLIENT_FORBIDDEN_RETRY_DELAY to the retry configuration reference, in its own "Authentication Flow Retries" section, since it governs the retry FolioAuth performs internally rather than the tenacity decorators the rest of the page covers. Explains why a 403 is replayed at all: FOLIO's Keycloak integration currently translates every backend error that is not a 200/401/403 into a 403, so a 403 is frequently a masked transient failure rather than a genuine permission denial. Documents setting it to 0 for deployments where 403 means only what it says, and notes that 0 becomes the better default once FOLIO passes real status codes through. Also corrects the Overview and Quick Start, which claimed no retries happen by default. That was already inaccurate before this change: the authentication flow has always made one automatic 401/403 retry attempt regardless of configuration, and only the decorator-based retries are opt-in. The two layers are now described separately so it is clear which one a given variable configures. Adds a troubleshooting entry for the most likely surprise -- 403 responses taking about a second longer than before.
|
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
2 participants
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.
Description
Fixes a set of correctness bugs in
FolioAuthand makes the retry paths actuallyeffective at recovering from the transient failures FOLIO produces in practice.
The headline item is that folioclient cannot authenticate at all on Python 3.10, a
version we claim to support: FOLIO returns
accessTokenExpirationwith a trailingZ(verified against live Okapi and Eureka instances, e.g.
2026-08-07T18:13:36Z), anddatetime.fromisoformatcannot parse that before 3.11, soFolioAuth.__init__raisedValueError.Focused commits, each independently reviewable:
b519434dateutil.isoparse— unbreaks Python 3.103f0aee6exc.response.textworksb5bce6easyncio.Lockf3dd1196bddc05944570243611bba3bdf0cWhy each matters
Async authentication was never serialized. The async flow guarded its refresh with a
threading.RLock, which is reentrant per thread. Every coroutine on an event loopshares one thread, so when one held it across an
awaitthe next re-acquired itimmediately — 10 concurrent flows produced 10 logins. Switching to a plain
threading.Lockis worse: it blocks the loop's own thread, which then cannot resume theholder, deadlocking with no traceback. Now uses one
asyncio.Lockper event loop, keptin a
WeakKeyDictionary.FolioClient.async_loginhad the same bug and is fixed too.A 401 was not treated as authoritative. The handler refreshed only if
_token_is_expiring()agreed, but a token can be rejected while still looking locallyvalid — clock skew, Keycloak key rotation, revocation, a module with a stale JWKS cache.
In all of those the old code replayed the identical request, got a second 401, and
raised. De-duplication is now by token identity, which keeps the "N callers, one login"
property without ever suppressing a needed refresh.
403 retries were ineffective, and 403 is the transient channel. FOLIO's Keycloak
integration currently collapses every backend error that is not a 200/401/403 into a 403,
so a 403 is frequently a masked timeout/502/503 rather than a real permission denial.
Two problems: multipart uploads silently lost their retry (httpx only sets
request._contentfor in-memory bodies, butMultipartStreamis replayable because itseeks each field back to 0), and the retry was immediate, which rarely outlasts a blip.
Now multipart is retried and there is a short configurable delay.
Type of Change
No public API changes. Three behavior changes worth a reviewer's attention:
FOLIOCLIENT_FORBIDDEN_RETRY_DELAYseconds (default1.0)before replaying. Set it to
0to restore the previous immediate replay. A genuinepermission denial pays this too, since the two are indistinguishable until FOLIO
passes real status codes through.
files=) requests are now retried on 401/403 where they previously werenot.
Testing
Integration matrix run against all seven server configs:
5 failed, 186 passed, 38 skipped, 24 errors. Every failure and error is thesnapshot-2-eurekaconfig,which has a single root cause —
httpx.ConnectError: [Errno 8] nodename nor servname provided— becausefolio-etesting-snapshot2-kong.ci.folio.orgno longer resolves.An identical run on
masterproduces the identical set of failing test IDs, so nothinghere is a regression.
snapshot,snapshot2,eureka,eureka-ecs,bugfestandbugfest-nextall pass, including thefolio_post/folio_put/folio_deleteuserlifecycle test.
test_get_large_dataset_id_offsetwas deselected: it ignores theserver_configfixture and hardcodes
BUGFEST_CONFIG, so--integration-servercannot filter it, andit pages 1.5M instances. Worth fixing separately.
Every behavioral claim was mutation-tested. For the three riskiest commits we
deliberately reintroduced each regression — RLock in the async flow, a single cached
lock with and without rebuild-on-loop-change, sharing the sync lock as the registry
guard, expiry-gated 401, unconditional 401 re-auth,
_content-only replayability,immediate 403 replay, replayability-checked-before-refresh, and a nested lock acquire —
and confirmed each fails the test written for it. Without that, the async and multipart
tests in particular would have passed against broken code.
Checklist
ruff checkandruff formatclean)docs/retry_configuration.mdgains an "Authentication Flow Retries" section covering theone new environment variable,
FOLIOCLIENT_FORBIDDEN_RETRY_DELAY, plus a troubleshootingentry for the most likely surprise (403 responses taking about a second longer). The page
now also distinguishes the two retry layers, correcting an existing claim that no retries
happen by default — the authentication flow has always made one automatic 401/403 attempt
regardless of configuration, and only the decorator-based retries are opt-in. Verified
with a clean
sphinx-build.docs/authentication.mdneeded no change: it already stated that the flow re-authenticates"whenever it receives a
401", which is whatf3dd119finally makes true. The code wasthe thing out of step with the documentation, not the reverse.
No changelog entry was added because the project only records entries at minor/major
boundaries — there are no v1.0.1–v1.0.12 sections.
Notes for reviewers
The most unusual code in this PR is
_get_async_lock, so it carries a long docstringexplaining why each alternative was rejected. The short version:
asyncio.Lockbinds toa loop the first time it actually has to wait, and that binding is permanent. Because
an uncontended acquire returns before the binding happens, a single cached lock passes
light testing and then fails under real concurrency — including on strictly sequential
reuse, e.g. one client across two
asyncio.run()calls or a session-scoped fixture underpytest-asyncio. Rebuilding one cached lock on loop change is worse still: two liveloops replace each other's lock continuously (measured: 39 rebuilds, 16 distinct lock
objects per loop), destroying exclusion within each loop as well as between them.
self._lockremains anRLockeven though nothing nests, because it is held acrossnetwork I/O with a possibly unlimited timeout: an accidental nested acquire under a plain
Lockwould hang the caller's process, whereasRLockdegrades to a redundant login.That safety net is prevented from hiding real nesting by
test_sync_lock_is_never_acquired_reentrantlyrather than by the primitive._request_is_replayableimportshttpx._multipart.MultipartStream, a private path,wrapped in
try/except ImportErrorthat degrades to "not replayable". It is written asan allowlist on purpose: if an httpx internal changes, the failure is a skipped retry
rather than a replayed generator or an exhausted file handle silently becoming a
zero-length POST/PUT.
Known follow-ups, deliberately not in scope
folio_retry_on_auth_erroralready retries 403 with a fresh login, but ships disabled(
FOLIOCLIENT_MAX_AUTH_ERROR_RETRIES=0) and itsmultiplier=10.0/exp_base=3.0defaults would cost ~100s per permission denial if enabled. Given 403 is the transient
channel, that combination deserves revisiting.
auth_refresh_callbackre-logs in. One ofthose is wrong; worth resolving deliberately.
_token_is_expiring()still treats an unknown expiry as expiring, so a parse failuremeans one login per request. Left alone because changing it introduces staleness for
access_token/folio_headers, which have no 401 fallback._cleanup_folio_auth'sdel self.folio_auth._tokenyieldsAttributeErrorratherthan
FolioClientClosedafter close.and real 403s are distinguishable by body shape, which would let the retry be narrowed
precisely instead of retrying all 403s.