Distributed locks to prevent multiple pods executing Reload and Refresh#7311
Distributed locks to prevent multiple pods executing Reload and Refresh#7311sfmskywalker merged 23 commits intorelease/3.6.0from
Conversation
Greptile SummaryAdds distributed locking decorators to prevent concurrent workflow definition reloads and refreshes across multiple pods.
Confidence Score: 1/5
|
| Filename | Overview |
|---|---|
| src/modules/Elsa.Workflows.Runtime/Services/WorkflowDefinitionsRefresherDistributedLocking.cs | Adds distributed locking decorator, but has critical issues: abstract keyword prevents instantiation, and null/empty check changes behavior |
| src/modules/Elsa.Workflows.Runtime/Services/WorkflowDefinitionsReloaderDistributedLocking.cs | Adds distributed locking decorator, but abstract keyword prevents instantiation |
| src/modules/Elsa.Workflows.Runtime/Features/WorkflowRuntimeFeature.cs | Correctly registers decorators for distributed locking using standard decorator pattern |
Sequence Diagram
sequenceDiagram
participant Client
participant Decorator as WorkflowDefinitionsRefresher<br/>DistributedLocking
participant LockProvider as IDistributedLockProvider
participant Inner as WorkflowDefinitionsRefresher
participant Store as IWorkflowDefinitionStore
Client->>Decorator: RefreshWorkflowDefinitionsAsync(request)
Decorator->>Decorator: Generate lock key from DefinitionIds
Decorator->>LockProvider: TryAcquireLockAsync(key, 1min)
alt Lock acquired
LockProvider-->>Decorator: Lock handle
Decorator->>Inner: RefreshWorkflowDefinitionsAsync(request)
Inner->>Store: FindManyAsync(filter)
Store-->>Inner: Workflow definitions
Inner->>Inner: Index triggers
Inner-->>Decorator: Response (refreshed, notFound)
Decorator-->>Client: Response
Decorator->>LockProvider: Release lock (via await using)
else Lock not acquired
LockProvider-->>Decorator: null
Decorator-->>Client: Empty response (skip refresh)
end
Last reviewed commit: a0e6a4a
src/modules/Elsa.Workflows.Runtime/Services/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Services/WorkflowDefinitionsReloaderDistributedLocking.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Services/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...dules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsRefresher.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request adds distributed locking decorators to prevent race conditions when multiple pods concurrently execute workflow definition reload or refresh operations. The implementation uses Medallion.Threading's IDistributedLockProvider to ensure only one pod can execute these operations at a time.
Changes:
- Added
WorkflowDefinitionsReloaderDistributedLockingdecorator with a global lock to serialize reload operations across all pods - Added
WorkflowDefinitionsRefresherDistributedLockingdecorator with per-definition-set locks to allow concurrent refreshes of different workflow sets - Updated
WorkflowRuntimeFeatureto register these decorators in the dependency injection container
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
WorkflowDefinitionsReloaderDistributedLocking.cs |
New decorator that adds distributed locking to workflow definition reload operations using a single global lock key |
WorkflowDefinitionsRefresherDistributedLocking.cs |
New decorator that adds distributed locking to workflow definition refresh operations using lock keys based on the set of definition IDs being refreshed |
WorkflowRuntimeFeature.cs |
Registers the new distributed locking decorators using the Decorate<> pattern after the base service registrations |
src/modules/Elsa.Workflows.Runtime/Services/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...dules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsRefresher.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Services/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...dules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsRefresher.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Services/WorkflowDefinitionsReloaderDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...odules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsReloader.cs
Show resolved
Hide resolved
sfmskywalker
left a comment
There was a problem hiding this comment.
Please move the distributed decorators to Elsa.Workflows.Runtime.Distributed.
…sRefresherDistributedLocking.cs Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…sReloaderDistributedLocking.cs Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…sRefresherDistributedLocking.cs Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
...modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...dules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsRefresher.cs
Show resolved
Hide resolved
...dules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsRefresher.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsReloaderDistributedLocking.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsReloaderDistributedLocking.cs
Outdated
Show resolved
Hide resolved
...modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsRefresherDistributedLocking.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsReloaderDistributedLocking.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Distributed/WorkflowDefinitionsReloaderDistributedLocking.cs
Outdated
Show resolved
Hide resolved
…ionsReloaderDistributedLocking.cs Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
…ionsRefresherDistributedLocking.cs Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
…ionsRefresherDistributedLocking.cs Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
…ionsReloaderDistributedLocking.cs Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
…ionsRefresherDistributedLocking.cs Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
…ionsReloaderDistributedLocking.cs Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
src/modules/Elsa.Workflows.Runtime/Elsa.Workflows.Runtime.csproj
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Features/WorkflowRuntimeFeature.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Runtime/Features/WorkflowRuntimeFeature.cs
Outdated
Show resolved
Hide resolved
sfmskywalker
left a comment
There was a problem hiding this comment.
This is the remaining issue to look into.
...dules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsRefresher.cs
Outdated
Show resolved
Hide resolved
sfmskywalker
left a comment
There was a problem hiding this comment.
Looks good in principle, just one question and one code quality change request.
src/modules/Elsa.Workflows.Api/Endpoints/WorkflowDefinitions/Refresh/Endpoint.cs
Show resolved
Hide resolved
...dules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsRefresher.cs
Outdated
Show resolved
Hide resolved
...dules/Elsa.Workflows.Runtime.Distributed/Services/DistributedWorkflowDefinitionsRefresher.cs
Outdated
Show resolved
Hide resolved
…butedWorkflowDefinitionsRefresher.cs Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
… simplification.
Purpose
Addition of Distributed locks to prevent multiple pods from triggering Reload or Refresh at the same time.
Scope
Select one primary concern:
Description
Distributed lock with generic lock has been added to
IWorkflowDefinitionsReloaderinterface as a decorator. The goal is to prevent multiple pods from ever executing this action at the same time.The same was done for
IWorkflowDefinitionsRefresher, with the exception that the key to the distributed lock uses a combination of definition Ids from the request to allow it to be called by different pods with with different Requests.Problem
Double or multiple triggering of change notifications and updates/publishing of workflows.
Solution
Distributed locks ensure no race between pods and only one action that should be singleton accross pods is taken.