v1.0.24#64
Conversation
…ased lock lifecycle Replace IAsyncDisposable return with IDistributedLockHandle on TryAcquireLockAsync, enabling TTL extension (Redis) and explicit release. Each acquisition now generates a unique owner (MachineName:Guid) instead of a static client identifier, fixing concurrent lock safety. - Add IDistributedLockHandle interface (ExtendAsync, ReleaseAsync) - Add RedisDistributedLockHandle with atomic Lua-based extend/release - Add DaprDistributedLockHandle (extend unsupported, logs warning) - Mark ReleaseLockAsync as [Obsolete] on both providers - Delete RedisLockHandle, replaced by RedisDistributedLockHandle - Add BBT.Aether.Infrastructure.Tests with unit tests for handles and services - Update distributed-lock docs with handle API and TTL extension patterns - Upgrade Dapr 1.16.1 → 1.17.9, OpenTelemetry 1.14.0 → 1.15.x, Npgsql.OpenTelemetry 8.0.6 → 10.0.2 - Remove deprecated EF Core OTel instruion options (SetDbStatementForText) BREAKING CHANGE: TryAcquireLockAsync returns IDistributedLockHandle? instead of IAsyncDisposable?. ReleaseLockAsync is obsolete — use the handle.
…try-tracing-spans-to-distributed-event-bus-pipeline feat(distributed-lock)!: introduce IDistributedLockHandle for scope-b…
Reviewer's GuideIntroduces a new scope-based distributed lock handle abstraction with TTL extension for Redis, adjusts the distributed lock service interfaces and implementations (Redis and Dapr) to return handles with unique owners, deprecates legacy release methods, updates documentation and examples for handle-based usage vs ExecuteWithLock patterns, adds telemetry for lock extension, simplifies EF Core tracing configuration, and covers the new behavior with unit tests. Sequence diagram for scope-based Redis lock with TTL extensionsequenceDiagram
actor Service
participant LockService as IDistributedLockService
participant RedisService as RedisDistributedLockService
participant Handle as RedisDistributedLockHandle
participant Redis as RedisDatabase
Service->>LockService: TryAcquireLockAsync("pipeline:123", 60, ct)
LockService->>RedisService: TryAcquireLockAsync("pipeline:123", 60, ct)
RedisService->>Redis: StringSetAsync(lockKey, owner, NX, expiry=60s)
Redis-->>RedisService: acquired = true
RedisService-->>LockService: Handle
LockService-->>Service: Handle
Service->>Handle: ExtendAsync(120, ct)
Handle->>Redis: ScriptEvaluateAsync(ExtendScript, lockKey, owner, 120s)
Redis-->>Handle: extended = 1
Handle-->>Service: true
Service->>Handle: DisposeAsync()
Handle->>Redis: ScriptEvaluateAsync(ReleaseScript, lockKey, owner)
Redis-->>Handle: released > 0
Handle-->>Service: completed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a scope-based distributed lock API through the new IDistributedLockHandle interface, enabling explicit lock release and TTL extension (implemented for Redis). The changes include updating Dapr and OpenTelemetry dependencies, adding infrastructure unit tests, and updating documentation. Feedback highlights critical logic errors in the deprecated ReleaseLockAsync methods where random owner IDs prevent successful unlocking. Other recommendations include restoring EF Core SQL capture settings to maintain observability, addressing a sync-over-async anti-pattern in the Redis handle's disposal logic, and removing an unused constructor parameter.
| var lockOwner = GenerateUniqueOwner(); | ||
| await daprClient.Unlock(storeName, resourceId, lockOwner, cancellationToken); |
There was a problem hiding this comment.
The ReleaseLockAsync method is now logically broken. It generates a new unique owner (containing a random GUID) for the Unlock call. Since Dapr requires the owner ID to match the one used during acquisition, this call will always fail to release the lock. If this method is intended to be deprecated and non-functional, it should be documented as such or return false immediately to avoid a useless network call. Otherwise, it should use a stable identifier.
| { | ||
| var database = redisConnection.GetDatabase(); | ||
| var lockOwner = GetClientIdentifier(); | ||
| var lockOwner = GenerateUniqueOwner(); |
There was a problem hiding this comment.
Similar to the Dapr implementation, ReleaseLockAsync is now broken because it generates a random unique owner for the release script. The owner must match the one that acquired the lock for the Lua script to successfully delete the key. This method will now always fail to release locks.
| efOptions.SetDbStatementForText = true; | ||
| efOptions.SetDbStatementForStoredProcedure = true; | ||
| }); | ||
| tracing.AddEntityFrameworkCoreInstrumentation(); |
There was a problem hiding this comment.
Removing the explicit configuration for Entity Framework Core instrumentation will result in SQL statements no longer being captured in traces. By default, SetDbStatementForText and SetDbStatementForStoredProcedure are false in the OpenTelemetry instrumentation to prevent accidental exposure of sensitive data. If capturing the SQL queries is still desired for observability, these options should be restored.
tracing.AddEntityFrameworkCoreInstrumentation(efOptions =>
{
efOptions.SetDbStatementForText = true;
efOptions.SetDbStatementForStoredProcedure = true;
});| string lockKey, | ||
| string owner, | ||
| ILogger logger) | ||
| : IDistributedLockHandle, IDisposable |
There was a problem hiding this comment.
The RedisDistributedLockHandle implements IDisposable in addition to IAsyncDisposable, but the synchronous Dispose implementation (line 141) uses GetAwaiter().GetResult(). This is a sync-over-async anti-pattern that can lead to deadlocks or thread pool starvation. Since the IDistributedLockHandle interface only requires IAsyncDisposable, it is recommended to remove the IDisposable implementation and the synchronous Dispose method.
: IDistributedLockHandle| @@ -17,15 +17,15 @@ public class RedisDistributedLockService( | |||
| IApplicationInfoAccessor applicationInfoAccessor) | |||
There was a problem hiding this comment.
The applicationInfoAccessor parameter is no longer used in this class. It should be removed from the constructor.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 102 |
| Duplication | 51 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The obsolete
ReleaseLockAsyncimplementations for both Dapr and Redis now generate a new owner per call (GenerateUniqueOwner), which means they can no longer successfully release locks acquired earlier; if you need to keep them usable for legacy callers, consider reusing the original owner or clearly documenting that they will effectively never release a lock. - The README examples and interface snippet show
ExecuteWithLockAsync<T>returning a(bool Acquired, T? Result)tuple, but the coreIDistributedLockServiceinterface still returnsTask<T?>; aligning the public API or adjusting the samples would avoid confusion and compilation issues for consumers copying the examples.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The obsolete `ReleaseLockAsync` implementations for both Dapr and Redis now generate a new owner per call (`GenerateUniqueOwner`), which means they can no longer successfully release locks acquired earlier; if you need to keep them usable for legacy callers, consider reusing the original owner or clearly documenting that they will effectively never release a lock.
- The README examples and interface snippet show `ExecuteWithLockAsync<T>` returning a `(bool Acquired, T? Result)` tuple, but the core `IDistributedLockService` interface still returns `Task<T?>`; aligning the public API or adjusting the samples would avoid confusion and compilation issues for consumers copying the examples.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary by Sourcery
Introduce handle-based distributed lock lifecycle management with TTL extension support and per-acquisition owners, and align docs, telemetry, and tests with the new API.
New Features:
Bug Fixes:
Enhancements:
Tests: