{{ message }}
Resolve RBAC deadlocks via sequential locking - #1685
Open
ygndotgg wants to merge 1 commit into
Open
Conversation
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rbac/mod.rs (1)
336-344: 💤 Low valueNarrow race window between session insertion and snapshot retrieval.
After
track_newcompletes and the write lock is dropped (line 335), there's a brief window before the read lock is acquired (line 337) where another thread could theoretically remove the session. The.expect()on line 344 would panic in this case.While this window is extremely narrow and would require a concurrent
remove_usercall for the same user being authorized, consider using.unwrap_or(Response::ReloadRequired)instead of.expect()for defensive resilience.🛡️ Defensive alternative
- return snapshot - .map(|snap| { - map::check_auth_snapshot(snap, action, context_stream, context_user) - }) - .expect("entry for this key just added"); + return snapshot + .map(|snap| { + map::check_auth_snapshot(snap, action, context_stream, context_user) + }) + .unwrap_or(Response::ReloadRequired);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rbac/mod.rs` around lines 336 - 344, Replace the `.expect("entry for this key just added")` call on the snapshot map chain with `.unwrap_or(Response::ReloadRequired)` to defensively handle the rare race condition where another thread might remove the session entry between the write lock being dropped after track_new and the read lock being acquired in sessions(). This prevents a panic if the session snapshot is no longer available when retrieved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/rbac/mod.rs`:
- Around line 336-344: Replace the `.expect("entry for this key just added")`
call on the snapshot map chain with `.unwrap_or(Response::ReloadRequired)` to
defensively handle the rare race condition where another thread might remove the
session entry between the write lock being dropped after track_new and the read
lock being acquired in sessions(). This prevents a panic if the session snapshot
is no longer available when retrieved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2e5ee68-0278-47f6-b02f-f9658734d92c
📒 Files selected for processing (2)
src/rbac/map.rssrc/rbac/mod.rs
Eliminated nested `RwLock` acquisitions by introducing `AuthSnapShot` for read paths and sequential lock-dropping for user mutations. Server remains deadlock-free under `quest-parallel` concurrent loads.
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
1 participant
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.
Description
Resolves critical
RwLockdeadlocks in the in-memory RBAC maps (USERS,SESSIONS,ROLES,USER_GROUPS) that caused the server to freeze under concurrent user management and authorization requests.Possible solutions and chosen one
A global lock-ordering hierarchy could have been implemented, but the chosen solution enforces a strict Core Invariant: a map lock may only read or mutate its own map. Cross-map logic is achieved by cloning lightweight data (e.g., the
AuthSnapShotstruct), dropping the current lock, and acquiring the next map sequentially. This minimizes lock hold times and completely eliminates circular lock dependencies.Key changes made in the patch
Sessions::check_authwith anAuthSnapShotpattern to drop theSESSIONSread lock before evaluating cross-map permissions.add_roles,remove_roles,change_password_hash,put_user, anddelete_userto ensureUSERSis fully unlocked before touchingSESSIONSorROLES.aggregate_group_permissionsandroles_to_permissionto explicitly dropUSER_GROUPSandROLESread locks between loop iterations.This PR has:
Summary by CodeRabbit