fix: stop rewriting the user row on every login (the actual cause of connection exhaustion) - #136
Merged
Merged
Conversation
This is the actual cause of the connection exhaustion. My earlier fix
(more uvicorn workers) addressed a real constraint but not this one, and
concurrency did not improve as a result - the bottleneck is a row lock
in Postgres, which no number of application workers can help.
Captured from pg_stat_activity during a burst of concurrent logins:
36 active | Lock | tuple
31 active | Lock | transactionid
16 idle in transaction | Client| ClientRead
67 UPDATE "ds_user" SET "password" = ..., "last_login" = ...
Every login called user.save() unconditionally, rewriting every column
of the same row. Concurrent logins for one user therefore queued on that
row's lock, and each waiting request held a database connection while it
waited - which is what walked the connection count up to max_connections
and produced "sorry, too many clients already", the 504s, and the failed
deploy.
Two changes:
- sync_user_from_keycloak compares against the stored values and saves
only when a field actually changed, with update_fields to keep the
UPDATE narrow. A repeat login of an unchanged user now performs no
write at all, so there is no lock to contend on.
- validate_token no longer calls user.save() on the Django-JWT path. It
rewrote an unchanged row on every authenticated request for no benefit.
New-user creation is untouched: that INSERT is genuine work.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #135. That PR fixed a real constraint but not this one, and concurrency did not improve as a result — because the bottleneck is a row lock in Postgres, which no number of application workers can help.
Evidence
Captured from
pg_stat_activityduring a burst of concurrent logins:Every login called
user.save()unconditionally, rewriting every column of the same row. Concurrent logins for one user queued on that row's lock, and each waiting request held a database connection while it waited — which walked the count tomax_connectionsand producedsorry, too many clients already, the 504s, and the failed deploy.Ruled out along the way, with measurements:
ATOMIC_REQUESTSUser.post_savesignalsAfter #135 deployed, 20 concurrent logins still returned at 4.3s, 8.1s, 12.0s … 60s — the same arithmetic queue, now demonstrably across two worker processes. That is what pointed at the database rather than the app.
Changes
sync_user_from_keycloakcompares against stored values and saves only when a field actually changed, withupdate_fieldsto keep the UPDATE narrow. A repeat login of an unchanged user now performs no write at all, so there is no lock to contend on.validate_tokenno longer callsuser.save()on the Django-JWT path — it rewrote an unchanged row on every authenticated request for no benefit.New-user creation is untouched; that INSERT is genuine work.
keycloak_idis aCharField, so the string comparison is sound (a UUID field would have compared unequal every time and silently defeated this).Verification
Post-deploy: re-run the 20-concurrent burst and confirm latencies stop forming an arithmetic sequence, no 504s, and the connection peak drops well below the 26-for-20 pattern. Numbers to follow on this PR.