AUT-1477 Close the re-claim race flagged in smartling-express PR #59 review - #32
Open
DmitryMasley wants to merge 7 commits into
Open
AUT-1477 Close the re-claim race flagged in smartling-express PR #59 review#32DmitryMasley wants to merge 7 commits into
DmitryMasley wants to merge 7 commits into
Conversation
…xchange Keycloak's single-use enforcement for both the authorization_code grant and refresh_token grant is correct, but this adapter had no way to prevent two concurrent requests for the same session (e.g. two browser tabs, or a duplicate/racing callback request) from independently calling Keycloak at the same time. `_pendingRefreshes` only deduplicated within a single Node process, so with multiple app instances behind a load balancer, concurrent requests routed to different instances would each hit Keycloak independently - producing "Code already used for userSession" at login and transient "Token is missing or session invalid" failures during normal use as sessions aged toward their access-token refresh cycle. Add an optional `refreshCoordinator` config (claim/publish/await, backed by a shared store such as Redis) so only one instance performs the actual Keycloak call while others share its result. Falls back to today's per-process-only behavior when not configured.
RefreshCoordinator.claim() now returns a per-claim token (or null) instead of a boolean, and publish() takes that token as a 4th argument so it can CAS-guard the write against a stale, previously-expired claim holder. Matches the updated RedisRefreshCoordinator contract in smartling-express. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tract publish() gained a required token argument and claim() now resolves to an ownership token (or null) rather than a boolean, per cadd96d. The public-facing contract doc here still described the old 3-arg shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r itself fails If refreshCoordinator.claim() or await() rejects (e.g. Redis unreachable), _coordinated() previously propagated that rejection and failed the caller even when Keycloak itself was healthy. The coordinator is an optional dedup optimization, so a coordinator failure now degrades to calling Keycloak directly instead of breaking auth. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…expires mid-wait Addresses smartling-express PR keycloak#59 review comment (discussion_r3664941181): when two followers both see await() resolve null - e.g. the leader's claim TTL expires while they're waiting - _coordinated used to fall through to doFetch() unconditionally, so both followers could independently hit Keycloak at once, reproducing the exact race the coordinator exists to prevent. _coordinated now re-attempts claim() (bounded by COORDINATION_RECURSION_LIMIT) before falling back to a direct fetch. Redis's atomic SET...NX still arbitrates a single winner among the re-claiming followers, so only one of them ever reaches Keycloak. Added a regression test that reproduces the scenario with an expiring-lock fake coordinator - confirmed it fails against the pre-fix naive fallback (second follower's real Keycloak call has no matching interceptor left) and passes with the re-claim loop in place.
DmitryMasley
requested review from
dnetrebenko-smartling,
shoffman-smartling and
steplov
July 28, 2026 15:47
…newal The previous commit's bounded retry couldn't distinguish "the leader crashed" from "the leader is just slow" - a claim's TTL expiring meant either, and a follower could only guess by retrying a fixed number of times, at the cost of up to ~15-20s of added latency on a real Keycloak slowdown. Replace it with heartbeating: the claim holder now renews its own claim (coordinator.renew, gated by the same CAS-on-token check publish() already uses) on a timer while doFetch is in flight, via the new GrantManager#_leadWithHeartbeat. A live leader's claim can no longer lapse mid-operation - it can only expire once the leader has genuinely stopped renewing (crashed, or finished without publishing). A follower that sees the claim disappear now has one unambiguous meaning to act on: re-run the whole claim/await cycle. Redis's atomic claim still arbitrates a single new leader among any followers re-claiming at once. The recursive re-run keeps a small depth cap (COORDINATION_RECLAIM_DEPTH_LIMIT) and jitter, but now purely as a defensive bound against pathological coordinator flakiness (e.g. claim() never succeeding) - not as a "how many times do we guess wrong" budget, since the semantics are no longer ambiguous. Adds tests for the heartbeat mechanism itself (renews while pending, stops on settle/preemption, tolerates a transient renew() rejection as a skipped beat) using a monkey-patched setInterval/clearInterval harness, plus a depth-cap self-heal test. The existing re-claim-race test continues to pass unchanged - it already modeled a leader that claims and disappears without renewing, which is exactly the (now well-defined, not ambiguous) trigger this design expects. Companion change: smartling-express adds RedisRefreshCoordinator#renew (CAS Lua PEXPIRE, same pattern as the existing CAS publish).
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
1 participant
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
Follow-up to AUT-1477 / #27 (merged), addressing a review comment from smartling-express PR #59 on the
refreshCoordinatormechanism that PR introduced downstream.The original gap:
_coordinated()'s fallback, when a follower'scoordinator.await()resolved with no result (e.g. the leader's claim TTL expired while the follower was waiting), fell straight through to calling Keycloak directly - unconditionally. Two followers waiting on the same expired lock could both independently call Keycloak at once, reproducing the exact race the coordinator exists to prevent.First fix (superseded within this PR): a bounded retry - re-attempt
claim()a few times before falling back. This closed the race, but a claim's TTL expiring is inherently ambiguous: it can mean "the leader crashed" or "the leader is just slow," and bounded retry can only guess, at the cost of up to ~15-20s of added latency on a real Keycloak slowdown.Current fix - heartbeat-based lease renewal: the claim holder now renews its own claim (
coordinator.renew, gated by the same CAS-on-token checkpublish()already uses) on a timer whiledoFetchis in flight, viaGrantManager#_leadWithHeartbeat. A live leader's claim can no longer lapse mid-operation - it only expires once the leader has genuinely stopped renewing (crashed, or finished without publishing). A follower seeing the claim disappear now has one unambiguous meaning to act on: re-run the claim/await cycle. Redis's atomic claim still arbitrates a single new leader among any followers re-claiming at once. The recursive re-run keeps a small depth cap + jitter, now purely as a defensive bound against pathological coordinator flakiness, not a "how many times do we guess wrong" budget.Companion change in smartling-express:
RedisRefreshCoordinator.renew(), same CAS-Lua pattern as the existingpublish().Test plan
test/unit/grant-manager-unit-test.js): renews on each tick whiledoFetchis pending, stops on success/failure, stops the moment a renewal reports preemption, tolerates a transientrenew()rejection as a skipped beat rather than a lost lease - using a monkey-patchedsetInterval/clearIntervalharness (no real timers, no flaky wall-clock waits).claim()/await()never resolve to a claim still gives up and calls Keycloak directly after the bounded depth limit.node test/unit/grant-manager-unit-test.js).npm run lintclean.🤖 Generated with Claude Code