Resolve RBAC deadlocks via sequential locking#1685
Conversation
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.
WalkthroughRefactors RBAC authorization across ChangesRBAC Snapshot Authorization Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant authorize
participant Sessions
participant check_auth_snapshot
participant aggregate_group_permissions
Client->>authorize: request + SessionKey
authorize->>Sessions: auth_snapshot(key)
alt Session exists
Sessions-->>authorize: AuthSnapShot { username, tenant_id, permissions }
authorize->>check_auth_snapshot: snapshot, required_action, resource, user
check_auth_snapshot->>aggregate_group_permissions: username, tenant_id
aggregate_group_permissions-->>check_auth_snapshot: group_perms
check_auth_snapshot-->>authorize: Response::Authorized / UnAuthorized
else BasicAuth
authorize->>authorize: verify password hash
authorize->>Sessions: track_new(key, perms)
authorize->>Sessions: auth_snapshot(key)
Sessions-->>authorize: AuthSnapShot
authorize->>check_auth_snapshot: snapshot, required_action, resource, user
check_auth_snapshot-->>authorize: Response
end
authorize-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
1 participant
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