[management] Remove store locks #4038
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR removes explicit per-account and per-entity locking (AcquireWriteLockByUID/AcquireReadLockByUID) across the management server, streamlining store interface and default account manager methods.
- Eliminated calls to
AcquireWriteLockByUID/AcquireReadLockByUIDinDefaultAccountManagerand router/resource/peer/network managers. - Removed these lock methods from the
Storeinterface. - Retained only
AcquireGlobalLockfor coarse-grained locking.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| management/server/user.go | Dropped per-account write locks from user operations |
| management/server/store/store.go | Removed lock methods from Store interface |
| management/server/setupkey.go | Deleted write-lock wrappers around setup keys |
| management/server/route.go | Removed per-account locks from route CRUD |
| management/server/posture_checks.go | Dropped locks in posture check save/delete |
| management/server/policy.go | Removed write locks around policy save/delete |
| management/server/peer.go | Eliminated read/write locks in peer flows |
| management/server/networks/… | Deleted per-account locks in routers/resources/managers |
| management/server/nameserver.go | Removed write locks in nameserver group operations |
| management/server/integrated_validator.go | Dropped write lock around validator update |
| management/server/group.go | Deleted locks in group management methods |
| management/server/account.go | Removed locks in account settings/jobs and sync routines |
Comments suppressed due to low confidence (2)
management/server/user.go:28
- Concurrent operations on account-level data now lack explicit locks. It's advisable to add tests that simulate concurrent creation/invitation of users to ensure data consistency.
func (am *DefaultAccountManager) createServiceUser(ctx context.Context, accountID string, initiatorUserID string, role types.UserRole, serviceUserName string, nonDeletable bool, autoGroups []string) (*types.UserInfo, error) {
| @@ -26,9 +26,6 @@ import ( | |||
|
|
|||
| // createServiceUser creates a new service user under the given account. | |||
| func (am *DefaultAccountManager) createServiceUser(ctx context.Context, accountID string, initiatorUserID string, role types.UserRole, serviceUserName string, nonDeletable bool, autoGroups []string) (*types.UserInfo, error) { | |||
There was a problem hiding this comment.
Removing per-account write locks in critical sections like createServiceUser may introduce race conditions. Consider using transaction-level locking or AcquireGlobalLock to preserve atomicity of account modifications.
Copilot uses AI. Check for mistakes.
| @@ -640,8 +632,6 @@ func (am *DefaultAccountManager) isCacheCold(ctx context.Context, store cacheSto | |||
|
|
|||
| // DeleteAccount deletes an account and all its users from local store and from the remote IDP if the requester is an admin and account owner | |||
| func (am *DefaultAccountManager) DeleteAccount(ctx context.Context, accountID, userID string) error { | |||
| unlock := am.Store.AcquireWriteLockByUID(ctx, accountID) | |||
| defer unlock() | |||
| account, err := am.Store.GetAccount(ctx, accountID) | |||
| if err != nil { | |||
There was a problem hiding this comment.
I think that here we can have possible race. somewhere between user are being removed and actual removal of an account, still possible that user could be created
| unlockAccount := am.Store.AcquireWriteLockByUID(ctx, accountID) | ||
| defer unlockAccount() | ||
|
|
There was a problem hiding this comment.
I think that here better to add a transaction
| unlock := m.store.AcquireWriteLockByUID(ctx, network.AccountID) | ||
| defer unlock() | ||
|
|
There was a problem hiding this comment.
I think that we need to add a transaction here, as some changes could be made between select and actual update
| @@ -55,8 +55,6 @@ type SetupKeyUpdateOperation struct { | |||
| // and adds it to the specified account. A list of autoGroups IDs can be empty. | |||
| func (am *DefaultAccountManager) CreateSetupKey(ctx context.Context, accountID string, keyName string, keyType types.SetupKeyType, | |||
| expiresIn time.Duration, autoGroups []string, usageLimit int, userID string, ephemeral bool, allowExtraDNSLabels bool) (*types.SetupKey, error) { | |||
| unlock := am.Store.AcquireWriteLockByUID(ctx, accountID) | |||
| defer unlock() | |||
|
|
|||
There was a problem hiding this comment.
seems that here we have a transaction which runs another transaction (SaveSetupKey). this is not about the removal of the lock, but maybe worth it to change
| @@ -227,9 +221,6 @@ func (am *DefaultAccountManager) DeleteUser(ctx context.Context, accountID, init | |||
| return status.Errorf(status.InvalidArgument, "self deletion is not allowed") | |||
| } | |||
|
|
|||
| unlock := am.Store.AcquireWriteLockByUID(ctx, accountID) | |||
| defer unlock() | |||
|
|
|||
| initiatorUser, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthShare, initiatorUserID) | |||
There was a problem hiding this comment.
I propose to add a transaction here
| unlock := am.Store.AcquireWriteLockByUID(ctx, accountID) | ||
| defer unlock() | ||
|
|
There was a problem hiding this comment.
I think that better to add here too
| unlock := am.Store.AcquireWriteLockByUID(ctx, accountID) | ||
| defer unlock() | ||
|
|
There was a problem hiding this comment.
and here. we can have a race if these methods will be called concurrently
Describe your changes
Issue ticket number and link
Stack
Checklist