Skip to content

fix(oauth2): use UTC timestamp and FOR UPDATE SKIP LOCKED in JTI blacklist cleanup (#4113) - #4120

Open
waterWang wants to merge 1 commit into
ory:masterfrom
waterWang:fix/jti-delete-lock-contention
Open

fix(oauth2): use UTC timestamp and FOR UPDATE SKIP LOCKED in JTI blacklist cleanup (#4113)#4120
waterWang wants to merge 1 commit into
ory:masterfrom
waterWang:fix/jti-delete-lock-contention

Conversation

@waterWang

@waterWang waterWang commented Aug 6, 2026

Copy link
Copy Markdown

What

Fixes lock contention and a clock-skew bug in the JTI blacklist cleanup path.

Why

SetClientAssertionJWT runs an unbounded inline DELETE WHERE expires_at < CURRENT_TIMESTAMP on every jwt-bearer token request. Under concurrent load, many goroutines contend for row locks on the same expired rows, causing 100-400ms latency spikes on /oauth2/token (#4113). The same statement also causes InnoDB deadlocks on MySQL (#3740).

Two defects in the single statement:

  1. Lock contention / deadlock — every request deletes all expired rows, and concurrent requests all target the same rows.
  2. Clock skew (security)CURRENT_TIMESTAMP is the database server's local time, while expires_at is written in UTC. On any non-UTC database the replay blacklist is pruned against the wrong clock, so JTIs can be deleted before they actually expire, widening the assertion-replay window.

How

  • Bind a UTC timestamp (time.Now().UTC()) instead of CURRENT_TIMESTAMP.
  • Bound the delete with LIMIT 100 ... FOR UPDATE SKIP LOCKED so concurrent requests skip rows already locked by another transaction instead of blocking.

Notes

The derived-table subquery follows the existing batch-delete pattern in this file for MySQL compatibility.

Closes #4113

Summary by CodeRabbit

  • Bug Fixes
    • Improved cleanup of expired OAuth2 client assertion entries.
    • Cleanup now uses bounded processing to improve reliability and reduce database contention.
    • Cleanup errors continue to be handled consistently.

@waterWang
waterWang requested review from a team and aeneasr as code owners August 6, 2026 15:57
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dcb20ced-2500-45e1-84cb-759a0a0fa31c

📥 Commits

Reviewing files that changed from the base of the PR and between 4174065 and 279d954.

📒 Files selected for processing (1)
  • persistence/sql/persister_oauth2.go

📝 Walkthrough

Walkthrough

Changes

OAuth2 JTI blacklist cleanup

Layer / File(s) Summary
Bounded expired-entry deletion
persistence/sql/persister_oauth2.go
SetClientAssertionJWT uses raw SQL to delete up to 100 expired JTI blacklist entries for the current network. The query uses application UTC timestamps and FOR UPDATE SKIP LOCKED. Errors still pass through sqlcon.HandleError.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: aeneasr

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the OAuth2 JTI blacklist cleanup changes for UTC timestamps and lock skipping.
Description check ✅ Passed The description explains the problem, impact, implementation, and linked issue; the omitted checklist is non-critical.
Linked Issues check ✅ Passed The changes address issue #4113 by using UTC timestamps, limiting cleanup to 100 rows, and applying FOR UPDATE SKIP LOCKED.
Out of Scope Changes check ✅ Passed All changes remain within the JTI blacklist cleanup path and support the linked issue objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

SetClientAssertionJWT inline DELETE on hydra_oauth2_jti_blacklist causes lock contention and latency spikes under concurrent load

3 participants