Skip to content

Conversation

@solracsf
Copy link
Member

@solracsf solracsf commented Dec 18, 2025

Fix #1175

Added locking mechanism to prevent race conditions during token refresh.
Enhanced error handling and logging for token refresh process.

The fix uses ILockingProvider to ensure atomic token refresh:​

  1. Per-session locking (prevents race within same user's requests)
  2. First request acquires lock and refreshes token successfully, or,
  3. a second request hits lock and waits 100ms, retries in loop (for 5 seconds max), then reads the already-refreshed token from session

@solracsf solracsf force-pushed the tokenRefreshLocking branch from 1c543a7 to f8cc3cf Compare December 18, 2025 08:03
@solracsf solracsf marked this pull request as ready for review December 18, 2025 08:03
@solracsf solracsf added bug Something isn't working 3. to review labels Dec 18, 2025
@solracsf solracsf requested a review from julien-nc December 18, 2025 08:04
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much. Code looks good. A few questions then I'll make some tests and we're good to go.

@solracsf solracsf force-pushed the tokenRefreshLocking branch from f8cc3cf to c6da408 Compare December 18, 2025 13:25
@solracsf
Copy link
Member Author

solracsf commented Dec 18, 2025

You're right. The token refresh involves network requests to the OAuth2 provider, which typically takes 200-500ms or more. The 100ms wait was pointless because Process A will still be in the middle of the HTTP call...

I've pushed a retry mechanism to accomplish this :

Process A:

  • Gets lock immediately
  • Refreshes token (network request takes 300ms)
  • Stores new token in session
  • Releases lock

Process B:

  • Tries to get lock, but get LockedException
  • Waits 100ms, retries, waits, retries, acquires lock (A released it)
  • Double-checks session, but Token already fresh
  • Returns cached token without making redundant HTTP request
  • Releases lock

This (should) prevent both the race condition and unnecessary duplicate token refreshes.

@solracsf solracsf force-pushed the tokenRefreshLocking branch from c6da408 to 5955dea Compare December 18, 2025 13:48
@solracsf solracsf self-assigned this Dec 18, 2025
@solracsf
Copy link
Member Author

I've just found out that this is a bit of a duplicate of #1178 but with another approach... sorry! 🙈

@julien-nc
Copy link
Member

Please don't be sorry. I'm really not sure writing a lock in the Php session is a viable solution because I don't exactly know when Symfony reads and writes the session data. So we might get outdated data when reading the lock. Using the lock mechanism like you did + the session ID as identifier seems like a better approach.

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt?

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 💙

@julien-nc julien-nc merged commit 3dceab3 into main Dec 18, 2025
42 checks passed
@julien-nc julien-nc deleted the tokenRefreshLocking branch December 18, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in TokenService causes logouts on token refresh

3 participants