fix(client): serialise Keycloak token acquisition #7

2026-09-21T15:12:29-04:00

claude commented

2026-09-21 15:12:29 -04:00

Fixes the concurrency bug noted in #6.

Two concurrent tool calls each found the token stale, each POSTed the password grant, and the appliance rejected one of the simultaneous grants. That call died with Keycloak token failed HTTP 401 — nothing to do with the operation it was running. I hit it firing two zerto_guard_before_mutate calls at once.

The fix

ensure_token double-checks the cache under an asyncio.Lock, so N concurrent callers produce exactly one token request and the rest reuse it.

8 concurrent reads on a COLD client:  8/8 ok
  keycloak token POSTs: 1   (was 8 before the fix)

The 401-retry path had the same shape and was worse: every in-flight request that got a 401 set _token = None and re-authed independently, so one expiry became a thundering herd. Requests now capture a token generation, and _reauth re-fetches only if nothing else has already moved past it.

Token fetch also gets a bounded retry for transient failures (5xx, network) with linear backoff. 401 and 403 are deliberately not retried — those are the credentials themselves, and hammering Keycloak can trip its brute-force lockout on a real service account.

From the Zerto API lessons

I hadn't read zerto_api_lessons before this. It validates the existing design (30s refresh buffer, re-auth once on 401, password grant not the swagger's implicit flow, one client for ZCA and ZVM) and gave one concrete improvement, its lesson 13:

Keycloak realms have varied client_ids across versions... If auth fails with invalid_client, that's the first thing to check.

Auth failures now name what Keycloak reported rather than listing both possibilities: invalid_client means the client_id is wrong for this appliance (10.x zerto-client, 9.x may be zerto-api); invalid_grant means the username or password is.

Verified live

The original failing case, two concurrent guards:

A: ok=True
B: ok=False — Zerto task ... finished as Failed

B now fails for the correct reason: its checkpoint genuinely didn't happen (concurrent InsertTaggedCP on one VPG), so the guard correctly refuses to mutate. The spurious auth error is gone.

pytest: 48 passed (7 new — single-request concurrency, cache reuse, both generation branches, no-retry-on-bad-credentials, the invalid_client message, transient retry).

Also clears the two long-standing lint findings in this file (PIE810, E501) while it's open. ruff check now passes across src/ and tests/. (ruff format still flags protection.py and status.py — pre-existing, untouched here.)

🤖 Generated with Claude Code

https://claude.ai/code/session_016yVfC5nvZowoLFnEGWhLGn

Fixes the concurrency bug noted in #6. Two concurrent tool calls each found the token stale, each POSTed the password grant, and the appliance rejected one of the simultaneous grants. That call died with `Keycloak token failed HTTP 401` — nothing to do with the operation it was running. I hit it firing two `zerto_guard_before_mutate` calls at once. ## The fix `ensure_token` double-checks the cache under an `asyncio.Lock`, so N concurrent callers produce exactly **one** token request and the rest reuse it. ``` 8 concurrent reads on a COLD client: 8/8 ok keycloak token POSTs: 1 (was 8 before the fix) ``` The 401-retry path had the same shape and was worse: every in-flight request that got a 401 set `_token = None` and re-authed independently, so one expiry became a thundering herd. Requests now capture a token **generation**, and `_reauth` re-fetches only if nothing else has already moved past it. Token fetch also gets a bounded retry for transient failures (5xx, network) with linear backoff. **401 and 403 are deliberately not retried** — those are the credentials themselves, and hammering Keycloak can trip its brute-force lockout on a real service account. ## From the Zerto API lessons I hadn't read `zerto_api_lessons` before this. It validates the existing design (30s refresh buffer, re-auth once on 401, password grant not the swagger's implicit flow, one client for ZCA and ZVM) and gave one concrete improvement, its lesson 13: > Keycloak realms have varied client_ids across versions... If auth fails with `invalid_client`, that's the first thing to check. Auth failures now name what Keycloak reported rather than listing both possibilities: `invalid_client` means the client_id is wrong for this appliance (10.x `zerto-client`, 9.x may be `zerto-api`); `invalid_grant` means the username or password is. ## Verified live The original failing case, two concurrent guards: ``` A: ok=True B: ok=False — Zerto task ... finished as Failed ``` B now fails for the *correct* reason: its checkpoint genuinely didn't happen (concurrent `InsertTaggedCP` on one VPG), so the guard correctly refuses to mutate. The spurious auth error is gone. `pytest`: 48 passed (7 new — single-request concurrency, cache reuse, both generation branches, no-retry-on-bad-credentials, the `invalid_client` message, transient retry). Also clears the two long-standing lint findings in this file (PIE810, E501) while it's open. **`ruff check` now passes across `src/` and `tests/`.** (`ruff format` still flags `protection.py` and `status.py` — pre-existing, untouched here.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016yVfC5nvZowoLFnEGWhLGn

claude added 1 commit 2026-09-21 15:12:30 -04:00

fix(client): serialise Keycloak token acquisition 8ac89c7f82

Two concurrent tool calls each found the token stale, each POSTed the
password grant, and the appliance rejected one of the simultaneous
grants. That call died with "Keycloak token failed HTTP 401" -- nothing
to do with the operation it was performing. Hit while testing the guard:
two concurrent zerto_guard_before_mutate calls, one came back 401.

ensure_token now double-checks the cache under an asyncio.Lock, so N
concurrent callers produce exactly one token request and the rest reuse
the result. Measured against the lab ZVM with a cold client: 8 concurrent
reads made 8 token POSTs before, 1 after.

The 401-retry path had the same shape and was worse: every in-flight
request that got a 401 set _token = None and re-authed independently, so
one expiry became a thundering herd. Requests now capture a token
generation, and _reauth re-fetches only if nothing else has already
moved past it.

Token fetch also gets a bounded retry for transient failures (5xx,
network) with linear backoff. 401 and 403 are NOT retried: those are the
credentials themselves, and hammering Keycloak can trip its brute-force
lockout on a real service account.

Per the Zerto API lessons, auth failures now name the cause Keycloak
reported instead of a generic hint -- invalid_client means the client_id
is wrong for this appliance (10.x zerto-client, 9.x may be zerto-api),
invalid_grant means the username or password is. That is the first thing
to check and it was previously guesswork.

Also clears the two long-standing lint findings in this file (PIE810,
E501) while it is open. ruff check now passes across src/ and tests/.

pytest 48 passed (7 new, covering single-request concurrency, cache
reuse, both generation branches, no-retry-on-bad-credentials, the
invalid_client message, and transient retry).

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_016yVfC5nvZowoLFnEGWhLGn

claude merged commit ebf714cc20 into main

2026-09-21 15:15:43 -04:00

claude deleted branch fix/token-race

2026-09-21 15:15:43 -04:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: justin/zerto-ai-rewind#7