Skip to content

Fix order-dependent test failures from shared JWKS cache - #246

Open
pouwerkerk wants to merge 1 commit into
clerk:mainfrom
pouwerkerk:fix-jwk-cache-test-isolation
Open

Fix order-dependent test failures from shared JWKS cache#246
pouwerkerk wants to merge 1 commit into
clerk:mainfrom
pouwerkerk:fix-jwk-cache-test-isolation

Conversation

@pouwerkerk

Copy link
Copy Markdown
Contributor

The credentialed test suite has one test that fails or passes depending on execution order. This PR fixes it by clearing the JWKS cache between tests. It touches only tests/conftest.py, which is hand-maintained (not listed in .speakeasy/gen.lock). No generated or production code changes.

Steps to reproduce

With CLERK_SECRET_KEY and a fresh CLERK_SESSION_TOKEN set, run these two tests in the same pytest process, in this order (the same order the full suite uses):

    pytest "tests/test_verify_token.py::TestJwtVerification::test_verify_token_remote_ok" \
           "tests/test_verify_token.py::TestJwtVerificationAsync::test_verify_token_invalid_secret_key"

Expected and actual behavior

Expected: test_verify_token_invalid_secret_key calls verify_token_async with secret_key='sk_test_invalid', the JWKS fetch gets a 401, and the test sees JWK_FAILED_TO_LOAD.

Actual: the test fails with DID NOT RAISE (or, if the session token has expired, with TOKEN_EXPIRED). Run alone, the same test passes.

Root cause

src/clerk_backend_api/security/verifytoken.py keeps a module-global __jwkcache shared by the sync and async paths and keyed only by JWT kid. The sync test_verify_token_remote_ok runs earlier in the suite, fetches the JWKS with the valid secret key, and caches the PEM. When the async invalid-secret-key test runs, _get_remote_jwt_key_async finds the cached PEM and returns it without contacting Clerk, so the invalid key is never used and the expected error never happens.

This is the cache working as designed in production. The problem is test isolation: nothing resets the cache between tests, so the error-path tests only pass from a cold start. The sync twin of this test passes today only because it happens to run before remote_ok.

Fix

An autouse fixture in tests/conftest.py that clears the cache before each test. Results with credentials: 71 passed, 2 skipped (was 1 failed, 70 passed, 2 skipped). Without credentials: 59 passed, 14 skipped, same as before, so CI and uncredentialed runs are unaffected. Verified identical under cryptography 48.0.0 and 50.0.0; the failure predates any dependency changes and is unrelated to #245.

Environment

Python 3.12.12, macOS (darwin arm64), pytest 8.x, pytest-asyncio 0.24.0,
PyJWT 2.13.0. Reproduced with cryptography 48.0.0 and 50.0.0.

The module-global __jwkcache in clerk_backend_api.security.verifytoken
is keyed only by JWT kid, so once any test verifies a session token with
a valid secret key, the cached PEM lets every later test skip the JWKS
fetch entirely. This made the credentialed error-path tests
order-dependent: TestJwtVerificationAsync::test_verify_token_invalid_secret_key
expects JWK_FAILED_TO_LOAD from a network round-trip that never happens
after the sync TestJwtVerification::test_verify_token_remote_ok has
warmed the cache.

Deterministic repro (requires CLERK_SECRET_KEY and CLERK_SESSION_TOKEN):

    pytest "tests/test_verify_token.py::TestJwtVerification::test_verify_token_remote_ok" \
           "tests/test_verify_token.py::TestJwtVerificationAsync::test_verify_token_invalid_secret_key"

Add an autouse fixture that clears the cache before each test so every
test starts cold. No production code is touched; the uncredentialed run
is unchanged (59 passed, 14 skipped before and after).
@dominic-clerk

Copy link
Copy Markdown
Contributor

Thank you for your contribution!

This is the cache working as designed in production.

I think it's a bug and we already had a ticket for it. I'll have the PR up shortly. We can then see if your PR is still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants