[AAP-66801] Fix race condition in OAuth2AccessToken.is_valid() causing HTTP 500#907
[AAP-66801] Fix race condition in OAuth2AccessToken.is_valid() causing HTTP 500#907DenisKoleda wants to merge 3 commits intoansible:develfrom
Conversation
|
Soft approval. This is the type of solution I would expect for this type of problem. Still want someone to look at it who was more involved with the oauth2 app development to weigh in. The existing save appears to me to be avoid any other model logic, and if assuming that's the case, this should be good. |
|
john-westcott-iv
left a comment
There was a problem hiding this comment.
Clean fix for a real race condition — QuerySet.update() is the right tool here. Left one suggestion to add a comment explaining why update() is used instead of save(), so future developers don't inadvertently revert this.
BrennanPaciorek
left a comment
There was a problem hiding this comment.
This should not cause any issues I do not think.
BrennanPaciorek
left a comment
There was a problem hiding this comment.
We'll want to check how activitystream handles updates to last used in this case. I think it'll resolve cleanly, but I'm uncertain.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Thanks for this contribution and sorry for this PR getting lost in the shuffle. This should be scheduled to be worked on starting next Thursday and should merge shortly after that. |
Replace save(update_fields=['last_used']) with QuerySet.update() to avoid DatabaseError when concurrent requests try to update the same token's last_used timestamp. The previous implementation could fail with: django.db.utils.DatabaseError: Save with update_fields did not affect any rows. This happened because between exists() check and save() call, another process could modify the row, causing save() to affect 0 rows and raise an exception. QuerySet.update() handles this gracefully by simply returning 0 affected rows without raising an error.
3e62180 to
ce2a95d
Compare
|
5ad6734 to
f4ed60d
Compare
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |



Summary
save(update_fields=['last_used'])withQuerySet.update()to preventDatabaseErrorProblem
When multiple parallel requests authenticate with the same OAuth2 token, the Gateway crashes with:
Root cause: In
is_valid()method, there's a race condition betweenexists()check andsave()call. When concurrent requests try to updatelast_usedtimestamp on the same token:exists()→ Trueexists()→ Truesave()→ Success, row version changessave()→ Fails because Django'ssave(update_fields=...)expects to affect exactly 1 row, but the row was already modifiedSolution
Replace:
With:
QuerySet.update()is atomic and simply returns 0 if no rows match, without raising an exception. This is the expected behavior for updatinglast_used— if the token was deleted between validation and update, we don't need to error out.Test plan
Summary by CodeRabbit