feat: add distributed counting semaphore with multi-provider support#117
Open
feat: add distributed counting semaphore with multi-provider support#117
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds full distributed semaphore support: new core interfaces, models, options, exceptions, a SemaphoreService, eight provider implementations, DI extensions, a sample app and sample service, README updates, many unit/integration tests, and minor solution/project metadata changes. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,220,255,0.5)
participant App as Application
end
rect rgba(200,255,200,0.5)
participant Svc as SemaphoreService
participant Timer as Timer
end
rect rgba(255,230,200,0.5)
participant Prov as ISemaphoreProvider
participant Backend as DistributedBackend
end
App->>Svc: StartAsync()
Svc->>Timer: start acquisition loop
App->>Svc: TryAcquireAsync()/WaitForSlotAsync()
Svc->>Prov: TryAcquireAsync(...)(with retry/backoff)
Prov->>Backend: read/update holder state
Backend-->>Prov: success / failure + counts
Prov-->>Svc: acquisition result
alt Acquired
Svc->>Timer: start heartbeat timer
Timer->>Prov: UpdateHeartbeatAsync()
Prov->>Backend: renew holder heartbeat
else Not acquired
Svc->>Svc: schedule retry / wait
end
App->>Svc: ReleaseAsync()
Svc->>Prov: ReleaseAsync()
Prov->>Backend: remove holder
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Changes SummaryThis PR introduces a distributed counting semaphore abstraction to the MultiLock library, complementing its existing distributed leader election feature. It adds Type: feature Components Affected: MultiLock core (ISemaphoreProvider, ISemaphoreService, SemaphoreService), InMemory provider, Redis provider, PostgreSQL provider, SQL Server provider, FileSystem provider, Consul provider, Azure Blob Storage provider, Unit tests, Integration tests, Sample application Files Changed
Architecture Impact
Risk Areas: Busy-wait in DisposeAsync: the disposal loop Suggestions
Full review in progress... | Powered by diffray |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a distributed counting semaphore abstraction to MultiLock (mirroring the existing leader-election pattern) and implements multiple provider backends, along with tests, documentation updates, and a runnable sample.
Changes:
- Introduces new semaphore core APIs/types (
ISemaphoreService,ISemaphoreProvider, options, status/info/events, acquisition wrapper) and DI registration helpers. - Adds semaphore providers for InMemory, FileSystem, Redis, SQL Server, PostgreSQL, Azure Blob Storage, Consul, and ZooKeeper (plus provider-specific options + DI extensions).
- Adds unit + integration tests for semaphore behavior across providers and includes a rate-limiting sample app + README updates.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MultiLock.Tests/SemaphoreServiceTests.cs | Unit tests for SemaphoreService basic behaviors and status changes. |
| tests/MultiLock.Tests/SemaphoreParameterValidationTests.cs | Unit tests for new semaphore-related parameter validation helpers. |
| tests/MultiLock.Tests/SemaphoreConcurrencyTests.cs | Concurrency-focused tests for the in-memory provider. |
| tests/MultiLock.Tests/InMemorySemaphoreProviderTests.cs | Unit tests for in-memory semaphore provider semantics and disposal behavior. |
| tests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cs | Integration tests for ZooKeeper semaphore provider. |
| tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs | Integration tests for SQL Server semaphore provider. |
| tests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cs | Integration tests for Redis semaphore provider. |
| tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs | Integration tests for PostgreSQL semaphore provider. |
| tests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cs | Integration tests for filesystem semaphore provider. |
| tests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cs | Integration tests for Consul semaphore provider. |
| tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs | Integration tests for Azure Blob Storage semaphore provider (Azurite). |
| src/Providers/MultiLock.ZooKeeper/ZooKeeperServiceCollectionExtensions.cs | Adds DI wiring for ZooKeeper semaphore provider. |
| src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cs | New options type for ZooKeeper semaphore provider. |
| src/Providers/MultiLock.ZooKeeper/MultiLock.ZooKeeper.csproj | Package metadata updated to include semaphores. |
| src/Providers/MultiLock.SqlServer/SqlServerServiceCollectionExtensions.cs | Adds DI wiring for SQL Server semaphore provider. |
| src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs | Implements semaphore operations using SQL Server + sp_getapplock. |
| src/Providers/MultiLock.SqlServer/SqlServerSemaphoreOptions.cs | New options type for SQL Server semaphore provider. |
| src/Providers/MultiLock.SqlServer/MultiLock.SqlServer.csproj | Package metadata updated to include semaphores. |
| src/Providers/MultiLock.Redis/RedisServiceCollectionExtensions.cs | Adds DI wiring for Redis semaphore provider. |
| src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs | Implements semaphore operations using Redis ZSET + Lua scripts. |
| src/Providers/MultiLock.Redis/RedisSemaphoreOptions.cs | New options type for Redis semaphore provider. |
| src/Providers/MultiLock.Redis/MultiLock.Redis.csproj | Package metadata updated to include semaphores. |
| src/Providers/MultiLock.PostgreSQL/PostgreSQLServiceCollectionExtensions.cs | Adds DI wiring for PostgreSQL semaphore provider. |
| src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs | Implements semaphore operations using PostgreSQL advisory locks + table storage. |
| src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cs | New options type for PostgreSQL semaphore provider. |
| src/Providers/MultiLock.PostgreSQL/MultiLock.PostgreSQL.csproj | Package metadata updated to include semaphores. |
| src/Providers/MultiLock.InMemory/SemaphoreSlotRecord.cs | Internal slot record type for in-memory semaphore provider. |
| src/Providers/MultiLock.InMemory/MultiLock.InMemory.csproj | Package metadata updated to include semaphores. |
| src/Providers/MultiLock.InMemory/InMemoryServiceCollectionExtensions.cs | Adds DI wiring for in-memory semaphore provider. |
| src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs | In-process semaphore provider implementation. |
| src/Providers/MultiLock.FileSystem/MultiLock.FileSystem.csproj | Package metadata updated to include semaphores. |
| src/Providers/MultiLock.FileSystem/HolderFileContent.cs | File payload model for filesystem semaphore holders. |
| src/Providers/MultiLock.FileSystem/FileSystemServiceCollectionExtensions.cs | Adds DI wiring for filesystem semaphore provider. |
| src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs | Filesystem-based semaphore provider implementation. |
| src/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cs | New options type for filesystem semaphore provider. |
| src/Providers/MultiLock.Consul/MultiLock.Consul.csproj | Package metadata updated to include semaphores. |
| src/Providers/MultiLock.Consul/ConsulServiceCollectionExtensions.cs | Adds DI wiring for Consul semaphore provider. |
| src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs | Consul KV + session based semaphore provider implementation. |
| src/Providers/MultiLock.Consul/ConsulSemaphoreOptions.cs | New options type for Consul semaphore provider. |
| src/Providers/MultiLock.AzureBlobStorage/MultiLock.AzureBlobStorage.csproj | Package metadata updated to include semaphores. |
| src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageServiceCollectionExtensions.cs | Adds DI wiring for Azure Blob Storage semaphore provider. |
| src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs | Blob-based semaphore provider with optimistic concurrency (ETags). |
| src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cs | New options type for Azure Blob Storage semaphore provider. |
| src/MultiLock/SemaphoreStatus.cs | New status record for a participant’s semaphore state. |
| src/MultiLock/SemaphoreServiceExtensions.cs | Adds core DI registration helpers for semaphore service/provider. |
| src/MultiLock/SemaphoreOptions.cs | New options type for semaphore service behavior (heartbeat/retry/etc.). |
| src/MultiLock/SemaphoreInfo.cs | New info record for describing semaphore state and holders. |
| src/MultiLock/SemaphoreHolder.cs | New record for describing a single holder. |
| src/MultiLock/SemaphoreChangedEventArgs.cs | New event args record for status-change notifications. |
| src/MultiLock/SemaphoreAcquisition.cs | New disposable wrapper representing an acquired slot. |
| src/MultiLock/ParameterValidation.cs | Adds semaphore + SQL identifier validation helpers. |
| src/MultiLock/MultiLock.csproj | Package metadata updated to include semaphores. |
| src/MultiLock/ISemaphoreService.cs | New service interface contract for semaphore operations. |
| src/MultiLock/ISemaphoreProvider.cs | New provider interface contract for semaphore backends. |
| src/MultiLock/Exceptions/SemaphoreTimeoutException.cs | New semaphore-specific timeout exception type. |
| src/MultiLock/Exceptions/SemaphoreProviderException.cs | New semaphore provider exception type. |
| src/MultiLock/Exceptions/SemaphoreFullException.cs | New semaphore “full capacity” exception type. |
| src/MultiLock/Exceptions/SemaphoreException.cs | New base semaphore exception type. |
| samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs | Sample worker demonstrating rate-limited work via semaphores. |
| samples/MultiLock.SemaphoreSample/Program.cs | Sample host + provider selection wiring. |
| samples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csproj | New sample project. |
| README.md | Updates docs to include distributed semaphores. |
| MultiLock.slnx | Adds new sample project and updates solution metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs
Show resolved
Hide resolved
src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cs-53-57 (1)
53-57:⚠️ Potential issue | 🟡 MinorError messages are misleading about valid range boundaries.
The validation rejects
TimeSpan.Zero(via<= TimeSpan.Zero), but the error messages state "between 0 and X minutes", which implies 0 is valid. The messages should say "greater than 0" to match the actual validation logic.📝 Proposed fix
if (SessionTimeout <= TimeSpan.Zero || SessionTimeout > TimeSpan.FromMinutes(60)) - throw new ArgumentException("Session timeout must be between 0 and 60 minutes.", nameof(SessionTimeout)); + throw new ArgumentException("Session timeout must be greater than 0 and at most 60 minutes.", nameof(SessionTimeout)); if (ConnectionTimeout <= TimeSpan.Zero || ConnectionTimeout > TimeSpan.FromMinutes(10)) - throw new ArgumentException("Connection timeout must be between 0 and 10 minutes.", nameof(ConnectionTimeout)); + throw new ArgumentException("Connection timeout must be greater than 0 and at most 10 minutes.", nameof(ConnectionTimeout));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cs` around lines 53 - 57, The ArgumentException messages for the SessionTimeout and ConnectionTimeout validations are misleading because the checks use <= TimeSpan.Zero (disallowing zero); update the messages in the validation that references SessionTimeout and ConnectionTimeout (in ZooKeeperSemaphoreOptions validation/constructor) to state the correct boundaries (e.g., "must be greater than 0 and no more than 60 minutes" for SessionTimeout and "must be greater than 0 and no more than 10 minutes" for ConnectionTimeout) so the text matches the actual logic.README.md-733-735 (1)
733-735:⚠️ Potential issue | 🟡 MinorFix named argument mismatch in FileSystem semaphore sample.
Line 734 uses
basePath, but the extension method parameter isdirectoryPath, so this snippet won’t compile.Suggested patch
builder.Services.AddFileSystemSemaphore( - basePath: "/var/locks", + directoryPath: "/var/locks", options => { options.SemaphoreName = "my-semaphore"; options.MaxCount = 10; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 733 - 735, The README sample uses the wrong named parameter for the FileSystem semaphore extension: change the call to match the extension's parameter name by replacing the named argument basePath with directoryPath in the AddFileSystemSemaphore invocation (keep the same values and the options configuration for SemaphoreName and MaxCount so the call to AddFileSystemSemaphore(directoryPath: "/var/locks", options => { options.SemaphoreName = "my-semaphore"; options.MaxCount = 10; }) compiles).samples/MultiLock.SemaphoreSample/Program.cs-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorValidate
maxConcurrentis > 0 before configuring semaphore options.At Line 24, values like
0or negative numbers are accepted and then propagated tooptions.MaxCount, which can break expected acquisition behavior.Proposed fix
int maxConcurrent = args.Length > 1 && int.TryParse(args[1], out int max) ? max : 3; +if (maxConcurrent <= 0) +{ + Console.Error.WriteLine("max_concurrent must be a positive integer."); + return; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/MultiLock.SemaphoreSample/Program.cs` at line 24, The parsed maxConcurrent value may be zero or negative; before using it to configure the semaphore (e.g., assigning to options.MaxCount) validate that maxConcurrent > 0 and otherwise replace it with a sane default (use 3 as the current default) or clamp to a minimum of 1; update the code that computes/uses maxConcurrent (the int maxConcurrent = ... expression and the code that sets SemaphoreOptions.MaxCount) to enforce this check so invalid inputs are not propagated to the semaphore configuration.tests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cs-18-20 (1)
18-20:⚠️ Potential issue | 🟡 MinorDispose
ILoggerFactoryin test lifecycle.The factory created in the constructor is not retained/disposed. Keep it as a field and dispose it in
DisposeAsyncto avoid resource leakage across integration runs.♻️ Suggested fix
public class ConsulSemaphoreIntegrationTests : IAsyncLifetime { + private readonly ILoggerFactory loggerFactory; private const string Address = "http://localhost:8500"; private readonly ILogger<ConsulSemaphoreProvider> logger; ... - ILoggerFactory loggerFactory = LoggerFactory.Create(builder => builder.AddConsole().SetMinimumLevel(LogLevel.Warning)); + loggerFactory = LoggerFactory.Create(builder => builder.AddConsole().SetMinimumLevel(LogLevel.Warning)); logger = loggerFactory.CreateLogger<ConsulSemaphoreProvider>(); ... - public Task DisposeAsync() => Task.CompletedTask; + public Task DisposeAsync() + { + loggerFactory.Dispose(); + return Task.CompletedTask; + }Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cs` around lines 18 - 20, The ILoggerFactory created for tests (loggerFactory) is not stored or disposed, causing resource leaks; make loggerFactory a private field on the test class, assign it where you currently call LoggerFactory.Create (instead of a local variable), keep creating logger via loggerFactory.CreateLogger<ConsulSemaphoreProvider>(), and then dispose or call loggerFactory.Dispose() (or await disposal) inside the test class's DisposeAsync method (alongside existing cleanup) checking for null to avoid exceptions; update any other similar creation (line ~29) the same way so all factories are retained and disposed.tests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cs-16-18 (1)
16-18:⚠️ Potential issue | 🟡 MinorDispose
ILoggerFactoryalong with filesystem cleanup.The logger factory instance is currently not disposed, which can leak console logger resources in repeated integration runs.
♻️ Suggested fix
public class FileSystemSemaphoreIntegrationTests : IAsyncLifetime { + private readonly ILoggerFactory loggerFactory; private readonly ILogger<FileSystemSemaphoreProvider> logger; ... - ILoggerFactory loggerFactory = LoggerFactory.Create(builder => builder.AddConsole().SetMinimumLevel(LogLevel.Warning)); + loggerFactory = LoggerFactory.Create(builder => builder.AddConsole().SetMinimumLevel(LogLevel.Warning)); logger = loggerFactory.CreateLogger<FileSystemSemaphoreProvider>(); ... public Task DisposeAsync() { + loggerFactory.Dispose(); // Clean up test directory if (Directory.Exists(directoryPath)) Directory.Delete(directoryPath, true); return Task.CompletedTask; }Also applies to: 23-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cs` around lines 16 - 18, The loggerFactory created in FileSystemSemaphoreIntegrationTests (variable loggerFactory) is not disposed and can leak console logger resources; update the test cleanup routine (the class TearDown/Dispose or [OneTimeTearDown]/[TearDown] method used for directoryPath cleanup) to call loggerFactory.Dispose() (or await loggerFactory.DisposeAsync() if available) and null out logger and loggerFactory after disposing so both the ILoggerFactory and ILogger<FileSystemSemaphoreProvider> are properly released; apply the same disposal change to the other test setup instances where loggerFactory is created.src/MultiLock/SemaphoreOptions.cs-106-111 (1)
106-111:⚠️ Potential issue | 🟡 MinorReject
RetryMaxDelayvalues belowRetryBaseDelay.Current checks allow
RetryMaxDelay < RetryBaseDelay, which produces an invalid backoff envelope.🛠️ Minimal fix
if (RetryMaxDelay <= TimeSpan.Zero) throw new ArgumentException("Retry max delay must be positive.", nameof(RetryMaxDelay)); + + if (RetryMaxDelay < RetryBaseDelay) + throw new ArgumentException( + "Retry max delay must be greater than or equal to retry base delay.", + nameof(RetryMaxDelay));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreOptions.cs` around lines 106 - 111, Add a validation that rejects RetryMaxDelay values smaller than RetryBaseDelay in the same validation block that currently checks RetryBaseDelay and RetryMaxDelay: if RetryMaxDelay < RetryBaseDelay throw a new ArgumentException with a clear message (e.g., "Retry max delay must be greater than or equal to retry base delay.") and nameof(RetryMaxDelay) so callers know which parameter is invalid; ensure this check sits after the existing positive-time checks for RetryBaseDelay and RetryMaxDelay.tests/MultiLock.Tests/InMemorySemaphoreProviderTests.cs-263-273 (1)
263-273:⚠️ Potential issue | 🟡 MinorExpiration test is timing-fragile under slow CI.
Using a fixed
Task.Delay(200)after a100mstimeout can intermittently fail. Prefer bounded polling until acquire succeeds (or timeout) to reduce flakiness.💡 Suggested stabilization
- // Wait for the slot to expire - await Task.Delay(200); - - // Act - second holder should be able to acquire - bool result = await provider.TryAcquireAsync(semaphoreName, "holder-2", maxCount, metadata, shortTimeout); + // Wait (bounded) until slot expires + bool result = false; + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2)); + while (!cts.Token.IsCancellationRequested) + { + result = await provider.TryAcquireAsync(semaphoreName, "holder-2", maxCount, metadata, shortTimeout); + if (result) break; + await Task.Delay(25, cts.Token); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.Tests/InMemorySemaphoreProviderTests.cs` around lines 263 - 273, The test uses a fixed Task.Delay(200) after acquiring with shortTimeout which is fragile; replace the fixed delay with a bounded polling loop that repeatedly calls provider.TryAcquireAsync(semaphoreName, "holder-2", maxCount, metadata, remainingTimeout) until it returns true or the original timeout window elapses, computing remaining timeout from the initial shortTimeout and the time elapsed since the first acquire; ensure you still await the first TryAcquireAsync for "holder-1" and use the same semaphoreName, maxCount and metadata identifiers so the test asserts acquisition deterministically instead of relying on Task.Delay.tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs-270-271 (1)
270-271:⚠️ Potential issue | 🟡 MinorCorrect the isolation-strategy comment to avoid drift from implementation.
Line 270 and Line 271 mention
SERIALIZABLE, but the PR’s intended strategy is advisory locking underReadCommitted. Please align this comment with current behavior.📝 Suggested comment update
- // The provider uses SERIALIZABLE isolation which prevents over-admission. - // With holderCount >> maxCount all slots should be filled. + // The provider uses advisory transaction locking to serialize slot-admission checks. + // With holderCount >> maxCount all slots should be filled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs` around lines 270 - 271, Update the misleading comment in the PostgreSQLSemaphoreIntegrationTests class: replace the mention of "SERIALIZABLE" with the actual strategy "advisory locking under ReadCommitted" and reword the two-line comment so it states that the provider uses advisory locks with ReadCommitted isolation which prevents over-admission, and that with holderCount >> maxCount all slots should be filled; locate and edit the comment near the test that describes isolation strategy (the comment block currently mentioning SERIALIZABLE).tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs-259-260 (1)
259-260:⚠️ Potential issue | 🟡 MinorUpdate stale locking-strategy comment to match current implementation.
Line 259 and Line 260 still describe
SERIALIZABLE + UPDLOCK/HOLDLOCK, but this PR documentssp_getapplockunderReadCommitted. The comment is now misleading.📝 Suggested comment update
- // SERIALIZABLE isolation + UPDLOCK/HOLDLOCK prevents both over-admission and spurious failures, - // so with holderCount >> maxCount all slots must be filled. + // Application-level locking (sp_getapplock) under ReadCommitted prevents over-admission, + // so with holderCount >> maxCount all slots should be filled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs` around lines 259 - 260, The comment in SqlServerSemaphoreIntegrationTests describing "SERIALIZABLE isolation + UPDLOCK/HOLDLOCK" is stale: update the test comment to reflect the current locking strategy that uses sp_getapplock under ReadCommitted. Locate the comment in the SqlServerSemaphoreIntegrationTests class and replace the SERIALIZABLE/UPDLOCK/HOLDLOCK wording with a concise description stating that the implementation uses sp_getapplock with ReadCommitted to prevent over-admission and spurious failures (and mention holderCount >> maxCount behavior if still relevant).src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs-158-176 (1)
158-176:⚠️ Potential issue | 🟡 MinorMissing
slotTimeoutvalidation.Other providers (Redis, PostgreSQL, SQL Server) call
ParameterValidation.ValidateSlotTimeout(slotTimeout)inGetCurrentCountAsync, but this implementation omits it. This could lead to inconsistent behavior if an invalidTimeSpan(e.g., negative) is passed.🔧 Proposed fix
public Task<int> GetCurrentCountAsync( string semaphoreName, TimeSpan slotTimeout, CancellationToken cancellationToken = default) { ThrowIfDisposed(); ParameterValidation.ValidateSemaphoreName(semaphoreName); + ParameterValidation.ValidateSlotTimeout(slotTimeout); lock (`@lock`) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs` around lines 158 - 176, In InMemorySemaphoreProvider.GetCurrentCountAsync, add the missing slot timeout validation by calling ParameterValidation.ValidateSlotTimeout(slotTimeout) (after ParameterValidation.ValidateSemaphoreName(semaphoreName) and before entering the lock) so invalid TimeSpan values are rejected consistently with other providers; keep the existing ThrowIfDisposed() and the rest of the method logic unchanged.src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs-476-477 (1)
476-477:⚠️ Potential issue | 🟡 MinorIndex name may exceed PostgreSQL's 63-character identifier limit.
The index name
idx_{options.TableName}_semaphore_heartbeatcould exceed PostgreSQL's 63-character limit ifTableNameis long (PR allows up to 128 chars). PostgreSQL silently truncates identifiers, which could cause naming collisions with long table names.Consider using a shorter or hash-based suffix:
🔧 Suggested fix
- CREATE INDEX IF NOT EXISTS idx_{options.TableName}_semaphore_heartbeat - ON {options.SchemaName}.{options.TableName} (semaphore_name, last_heartbeat);"; + CREATE INDEX IF NOT EXISTS idx_{options.TableName}_hb + ON {options.SchemaName}.{options.TableName} (semaphore_name, last_heartbeat);";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs` around lines 476 - 477, The index name construction using "idx_{options.TableName}_semaphore_heartbeat" can exceed PostgreSQL's 63-char identifier limit; update the code that builds the CREATE INDEX SQL (in the PostgreSqlSemaphoreProvider where options.TableName and options.SchemaName are used) to generate a safe deterministic short name: either truncate options.TableName to fit within the 63-char limit combined with the fixed suffix or compute a short hash (e.g., SHA1/base36) of options.TableName and use "idx_semaphore_heartbeat_{hash}" so the resulting identifier is ≤63 chars and uses only safe characters; ensure the same generated name is used for CREATE INDEX and any DROP/IF EXISTS operations so names remain consistent.src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs-67-97 (1)
67-97:⚠️ Potential issue | 🟡 MinorRenewal overwrites
AcquiredAttimestamp.When renewing an existing holder (lines 73-78 in Lua), the
HolderDataobject created at lines 91-96 always setsAcquiredAt = DateTimeOffset.UtcNow. This overwrites the original acquisition timestamp, which differs from theInMemorySemaphoreProviderbehavior that preservesexistingSlot.AcquiredAtduring renewal.If consistent
AcquiredAtsemantics are desired, the Lua script should fetch and preserve the existingAcquiredAtvalue on renewal.🔧 Suggested approach
Modify the Lua script to return the existing holder data when renewing, then preserve the original
AcquiredAtin the updatedHolderData:-- Check if holder already exists local existingScore = redis.call('ZSCORE', KEYS[1], ARGV[2]) if existingScore then -- Renew existing holder redis.call('ZADD', KEYS[1], ARGV[3], ARGV[2]) - redis.call('HSET', KEYS[2], ARGV[2], ARGV[4]) - return 1 + local existingData = redis.call('HGET', KEYS[2], ARGV[2]) + redis.call('HSET', KEYS[2], ARGV[2], ARGV[4]) + return {1, existingData} endThen parse the existing data client-side to preserve
AcquiredAt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs` around lines 67 - 97, The renewal path in the Redis semaphore Lua script currently overwrites the original acquisition timestamp because HolderData is always re-created with AcquiredAt = DateTimeOffset.UtcNow; update the Lua script to fetch and return the existing holder data on renewal (e.g., call HGET KEYS[2] ARGV[2] before ZADD/HSET) so the caller in RedisSemaphoreProvider can parse that JSON and preserve existing AcquiredAt when building holderDataJson for renewals; modify the renewal branch in the script to return the existing holder payload and adjust the client-side logic that constructs HolderData (the HolderData object and holderDataJson creation) to use the parsed existing AcquiredAt instead of always using UtcNow.src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs-141-160 (1)
141-160:⚠️ Potential issue | 🟡 MinorSilent failure after retry exhaustion in
ReleaseAsync.If all retry attempts fail due to ETag conflicts, the method exits silently without logging a warning or throwing an exception. This could leave the caller unaware that the release didn't complete successfully.
🔧 Proposed fix
for (int attempt = 0; attempt < options.MaxRetryAttempts; attempt++) { // ... existing code ... } + + logger.LogWarning("Failed to release slot for holder {HolderId} in semaphore {SemaphoreName} after {MaxRetries} attempts", + holderId, semaphoreName, options.MaxRetryAttempts); } catch (Exception ex)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs` around lines 141 - 160, ReleaseAsync currently retries options.MaxRetryAttempts using GetBlobClientAsync, ReadSemaphoreDataAsync and TryWriteSemaphoreDataAsync but returns silently if all attempts fail; update ReleaseAsync to log a warning (with semaphoreName, holderId and attempt count) and/or throw a specific exception after the retry loop to surface the failure to callers—include context from the last ETag/conflict if available; keep existing retry logic and only add the fallback logging/exception path after the loop to avoid changing retry behavior.src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs-473-477 (1)
473-477:⚠️ Potential issue | 🟡 MinorIndex/constraint names may exceed SQL Server's 128-character identifier limit.
The index name
IX_{options.TableName}_SemaphoreName_LastHeartbeatand constraint namePK_{options.TableName}could exceed 128 characters ifTableNameis long (PR allows up to 128 chars). Unlike PostgreSQL, SQL Server will error on this rather than truncating.Consider using shorter suffixes:
🔧 Suggested fix
- CONSTRAINT PK_{options.TableName} PRIMARY KEY (SemaphoreName, HolderId) + CONSTRAINT PK_{options.TableName}_Sem PRIMARY KEY (SemaphoreName, HolderId) ); - CREATE INDEX IX_{options.TableName}_SemaphoreName_LastHeartbeat + CREATE INDEX IX_{options.TableName}_HB ON [{options.SchemaName}].[{options.TableName}] (SemaphoreName, LastHeartbeat);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs` around lines 473 - 477, The CREATE statements in SqlServerSemaphoreProvider.cs generate identifier names PK_{options.TableName} and IX_{options.TableName}_SemaphoreName_LastHeartbeat which can exceed SQL Server's 128-char limit when options.TableName is long; update the SQL name generation logic (where the PK and IX names are composed) to produce shorter deterministic names (e.g., truncate options.TableName to a safe length, or use a short prefix + hash of options.TableName) and replace occurrences used for constraint creation and any later DROP/ALTER references so the same shortened identifier is used consistently for the primary key (PK_...) and index (IX_..._HB or similar) generation.
🧹 Nitpick comments (13)
MultiLock.slnx (1)
13-15: Inconsistent path separator and missingTypeattribute.Line 13 uses forward slashes (
samples/MultiLock.SemaphoreSample/...) while all other project entries use backslashes. Additionally, line 13 is missing theType="C#"attribute that other projects have.🔧 Proposed fix for consistency
- <Project Path="samples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csproj" /> + <Project Path="samples\MultiLock.SemaphoreSample\MultiLock.SemaphoreSample.csproj" Type="C#" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MultiLock.slnx` around lines 13 - 15, The Project entry currently uses mixed path separators and is missing the Type attribute; update the Project element for the MultiLock.SemaphoreSample entry so its Path uses backslashes (matching the other entries) and add Type="C#" (e.g., make the Project element for MultiLock.SemaphoreSample mirror the other Project elements by using backslashes and including the Type="C#" attribute).src/MultiLock/SemaphoreStatus.cs (1)
21-21: Consider defensive guard against negativeAvailableSlots.If
CurrentCountever exceedsMaxCountdue to race conditions or stale data,AvailableSlotswould return a negative value. WhileHasAvailableSlotshandles this correctly (returnsfalse), consumers directly usingAvailableSlotsmight not expect negative values.🛡️ Optional defensive fix
- public int AvailableSlots => MaxCount - CurrentCount; + public int AvailableSlots => Math.Max(0, MaxCount - CurrentCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreStatus.cs` at line 21, In SemaphoreStatus update the AvailableSlots property so it never returns negative values by clamping the computed slots to zero (i.e., replace the simple subtraction MaxCount - CurrentCount with a non-negative result), keeping HasAvailableSlots behavior unchanged; locate the AvailableSlots getter in the SemaphoreStatus class and use a clamp (e.g., Math.Max(0, MaxCount - CurrentCount)) so consumers always receive 0+ for available slots.src/MultiLock/SemaphoreInfo.cs (1)
16-19: Consider defensive computation forAvailableSlots.If
CurrentCountever exceedsMaxCountdue to a race condition or transient inconsistency,AvailableSlotswould return a negative value. Consider usingMath.Max(0, MaxCount - CurrentCount)for consistency with the defensive approach mentioned in the PR objectives forCurrentCount.🛡️ Optional defensive fix
- public int AvailableSlots => MaxCount - CurrentCount; + public int AvailableSlots => Math.Max(0, MaxCount - CurrentCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreInfo.cs` around lines 16 - 19, AvailableSlots currently computes MaxCount - CurrentCount which can go negative if CurrentCount briefly exceeds MaxCount; update the property in SemaphoreInfo (AvailableSlots) to defensively clamp the result to zero (use Math.Max(0, MaxCount - CurrentCount)) so it never returns a negative value while still reflecting available slots.src/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cs (1)
35-36: HardenFileExtensionvalidation to reject path separators and enforce extension format.
FileExtensioncurrently only checks non-empty. Consider failing fast when it doesn't start with.or includes directory separators to avoid invalid holder file paths.Suggested patch
if (string.IsNullOrWhiteSpace(FileExtension)) throw new ArgumentException("File extension cannot be null or empty.", nameof(FileExtension)); + + if (!FileExtension.StartsWith('.', StringComparison.Ordinal)) + throw new ArgumentException("File extension must start with '.'.", nameof(FileExtension)); + + if (FileExtension.IndexOfAny([Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar]) >= 0) + throw new ArgumentException("File extension cannot contain path separators.", nameof(FileExtension));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cs` around lines 35 - 36, The FileExtension validation in FileSystemSemaphoreOptions is too permissive; update the validation for the FileExtension property (in the FileSystemSemaphoreOptions constructor/initializer) to reject values that don't start with '.' or that contain directory separators or path separator characters (e.g., Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar, Path.PathSeparator) or are just "."; if any check fails, throw an ArgumentException (include nameof(FileExtension) in the exception) to prevent invalid holder file paths.src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cs (1)
40-41: Add validation for Azure Blob Storage container naming rules at options-validation time.The current check only validates for non-empty values. Azure Blob Storage enforces strict naming rules: 3-63 characters, lowercase letters/numbers/hyphens only, must start and end with a letter or number, and no consecutive hyphens. Add explicit validation to catch naming violations during startup rather than at runtime when the container is accessed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cs` around lines 40 - 41, Add Azure Blob Storage container name validation inside AzureBlobStorageSemaphoreOptions (where ContainerName is checked): ensure ContainerName length is between 3 and 63, contains only lowercase letters, numbers and hyphens, does not begin or end with a hyphen, does not contain consecutive hyphens, and starts/ends with a letter or number; if validation fails throw an ArgumentException describing the rule violation referencing ContainerName so startup fails early. Use a single helper method (e.g., ValidateContainerName) called from the existing guard to keep the constructor/validation clean.src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cs (1)
29-35: Changeinittosetfor consistency with other provider options.The PostgreSQL options classes use
initfor these properties while the equivalent SqlServer options useset. For configuration options classes,setis the conventional choice and aligns with the rest of the codebase.Suggested patch
- public bool AutoCreateTable { get; init; } = true; + public bool AutoCreateTable { get; set; } = true; /// <summary> /// Gets or sets the command timeout in seconds for PostgreSQL operations. /// Default is 30 seconds. /// </summary> - public int CommandTimeoutSeconds { get; init; } = 30; + public int CommandTimeoutSeconds { get; set; } = 30;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cs` around lines 29 - 35, Change the two properties on PostgreSqlSemaphoreOptions from init-only setters to mutable setters: replace "public bool AutoCreateTable { get; init; } = true;" with a set accessor and replace "public int CommandTimeoutSeconds { get; init; } = 30;" with a set accessor so they read "public bool AutoCreateTable { get; set; } = true;" and "public int CommandTimeoutSeconds { get; set; } = 30;"; this makes them consistent with the SqlServer options and other provider option classes.samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs (1)
35-35: Consider observing the monitor task to avoid silent failures.At Line 35, the fire-and-forget monitor can fault silently for non-cancellation exceptions. Capturing and observing the task would improve sample robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs` at line 35, The fire-and-forget call "_ = MonitorStatusChangesAsync(stoppingToken);" can fault silently; capture the Task returned by MonitorStatusChangesAsync (e.g., var monitorTask = MonitorStatusChangesAsync(stoppingToken)) and either await it on shutdown or attach a continuation that logs non-cancellation exceptions so failures are observed; update ExecuteAsync/StopAsync to await or handle monitorTask and use the existing logger to surface exceptions from MonitorStatusChangesAsync rather than discarding the Task.src/MultiLock/SemaphoreOptions.cs (1)
85-111: Prefer sharedParameterValidationmethods here to avoid rule drift.
Validate()currently duplicates partial logic and omits some shared constraints (identifier shape/length and metadata validation). ReuseParameterValidationentry points for consistency across service and provider boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreOptions.cs` around lines 85 - 111, The Validate() method in SemaphoreOptions duplicates basic checks and omits shared constraints; replace the inline checks by invoking the shared ParameterValidation entry points (e.g., call the appropriate ParameterValidation.ValidateIdentifier/ValidateName/ValidateMetadata and timeout/delay validation helpers) from within SemaphoreOptions.Validate(), retaining only any semaphore-specific rules (like MaxCount) and removing duplicated checks for SemaphoreName, HeartbeatInterval/Timeout, AcquisitionInterval, RetryBase/MaxDelay and MaxRetryAttempts; ensure you call the identifier shape/length validator for SemaphoreName and the metadata validator so all common constraints are enforced consistently across services/providers.src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs (1)
595-605: Consider clearing all tracked nodes inHandleSessionExpired.When a ZooKeeper session expires, ALL ephemeral nodes from that session are deleted, not just the specific semaphore's nodes.
HandleSessionExpiredcurrently only removes one semaphore's nodes (line 603), but the watcher'sprocess()method (line 424) clears all nodes. This redundancy is harmless since both paths execute, but aligning the behavior would be clearer.♻️ Suggested simplification
private void HandleSessionExpired(KeeperException.SessionExpiredException ex, string operation, string holderId, string semaphoreName) { logger.LogWarning(ex, "ZooKeeper session expired while {Operation} for holder {HolderId} in semaphore {SemaphoreName}", operation, holderId, semaphoreName); sessionExpired = true; isInitialized = false; - lock (nodeLock) - { - activeNodes.Remove(semaphoreName); - } + // All ephemeral nodes are gone on session expiry; process() watcher clears activeNodes }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs` around lines 595 - 605, HandleSessionExpired currently only removes the entry for a single semaphoreName from activeNodes; change it to clear all tracked nodes to reflect that a session expiry removes all ephemeral nodes. Inside HandleSessionExpired (which sets sessionExpired and isInitialized and locks nodeLock), replace activeNodes.Remove(semaphoreName) with activeNodes.Clear() while keeping the existing log, sessionExpired = true, isInitialized = false, and the lock(nodeLock) scope so all entries are removed consistently with the watcher process() behavior.src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs (1)
256-281: Consider batching Redis calls forGetHoldersAsync.The current implementation makes N+1 Redis round trips (1
ZRANGE+ NHGETcalls). For semaphores with larger slot counts, this could impact performance.Consider using
HMGETor a Lua script to fetch all holder data in a single call.♻️ Example optimization using HMGET
SortedSetEntry[] entries = await database.SortedSetRangeByRankWithScoresAsync(holdersKey); + // Batch fetch all holder data + RedisValue[] holderIds = entries.Select(e => e.Element).ToArray(); + RedisValue[] dataValues = await database.HashGetAsync(dataKey, holderIds); var holders = new List<SemaphoreHolder>(); - foreach (SortedSetEntry entry in entries) + for (int i = 0; i < entries.Length; i++) { - string holderId = entry.Element!; - DateTimeOffset lastHeartbeat = DateTimeOffset.FromUnixTimeMilliseconds((long)entry.Score); - - // Get holder data - RedisValue dataValue = await database.HashGetAsync(dataKey, holderId); + string holderId = entries[i].Element!; + DateTimeOffset lastHeartbeat = DateTimeOffset.FromUnixTimeMilliseconds((long)entries[i].Score); + RedisValue dataValue = dataValues[i];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs` around lines 256 - 281, GetHoldersAsync currently does one ZRANGE (holdersKey) then N HashGetAsync calls causing N+1 round trips; instead batch the hash reads by collecting all holder IDs from the SortedSetEntry[] and calling the multi-field hash fetch (HashGetAsync(dataKey, RedisValue[])) once, then deserialize each corresponding RedisValue into HolderData and construct SemaphoreHolder instances; update the loop that builds holders to use the single HashGetAsync result array and keep symbols: GetHoldersAsync, holdersKey, dataKey, SemaphoreHolder, HolderData.src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs (3)
370-382: Consider handling null session creation result.If Consul fails to create a session,
createResult.Responsecould be null, but the method returns a non-nullablestring. This would propagate null to callers that don't expect it.🛡️ Proposed defensive handling
WriteResult<string>? createResult = await consulClient.Session.Create(sessionEntry, cancellationToken); - return createResult.Response; + return createResult?.Response + ?? throw new SemaphoreProviderException("Failed to create Consul session");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs` around lines 370 - 382, CreateSessionAsync currently returns createResult.Response which may be null if Consul fails; update CreateSessionAsync to defensively check that the result and createResult.Response are not null after calling consulClient.Session.Create(sessionEntry, cancellationToken) and throw a clear exception (e.g., InvalidOperationException or custom ConsulException) including context like holderId and any available error info when they are null so callers never receive a null string from CreateSessionAsync.
407-414: CAS result is not checked.
consulClient.KV.CASreturns a boolean indicating success, but the result is discarded. If the CAS fails (e.g., due to a concurrent modification), the heartbeat update silently fails while appearing successful to the caller.If this is intentional best-effort behavior, consider adding a debug log when CAS returns false.
📝 Proposed logging for failed CAS
- await consulClient.KV.CAS(kvPair, cancellationToken); + WriteResult<bool> casResult = await consulClient.KV.CAS(kvPair, cancellationToken); + if (!casResult.Response) + { + logger.LogDebug("CAS update failed for holder {HolderId} in semaphore {SemaphoreName} - concurrent modification", + holderId, semaphoreName); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs` around lines 407 - 414, The CAS call on consulClient.KV.CAS is unchecked: when creating the KVPair (holderKey with Value, Session and ModifyIndex from getResult.Response.ModifyIndex) you must capture the boolean result and handle failure; update the code around KVPair and consulClient.KV.CAS to await and store the returned bool, and if false either retry (e.g., refresh getResult and attempt CAS again) or at minimum emit a debug/error log that includes holderKey, sessionId and the ModifyIndex to surface the concurrent-modification failure instead of silently dropping it.
359-362: Sync-over-async disposal pattern is documented but worth noting.The
GetAwaiter().GetResult()pattern can deadlock if aSynchronizationContextis present (e.g., UI or ASP.NET classic). The remarks correctly document this risk. Consider whether this class should implementIDisposableat all, or only exposeIAsyncDisposable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs` around lines 359 - 362, The synchronous Dispose implementation calls DisposeAsync().AsTask().GetAwaiter().GetResult(), which can deadlock under a SynchronizationContext; replace the sync-over-async pattern by removing the IDisposable.Dispose implementation and surface only IAsyncDisposable.DisposeAsync (i.e., delete the public void Dispose() method), or if you must keep IDisposable, throw NotSupportedException and document it; update the ConsulSemaphoreProvider class to rely on the existing DisposeAsync method (reference DisposeAsync and the class ConsulSemaphoreProvider) so consumers use async disposal instead of the blocking Dispose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e370645c-324f-43bc-9a49-daba234cfb98
📒 Files selected for processing (65)
MultiLock.slnxREADME.mdsamples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csprojsamples/MultiLock.SemaphoreSample/Program.cssamples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cssrc/MultiLock/Exceptions/SemaphoreException.cssrc/MultiLock/Exceptions/SemaphoreFullException.cssrc/MultiLock/Exceptions/SemaphoreProviderException.cssrc/MultiLock/Exceptions/SemaphoreTimeoutException.cssrc/MultiLock/ISemaphoreProvider.cssrc/MultiLock/ISemaphoreService.cssrc/MultiLock/MultiLock.csprojsrc/MultiLock/ParameterValidation.cssrc/MultiLock/SemaphoreAcquisition.cssrc/MultiLock/SemaphoreChangedEventArgs.cssrc/MultiLock/SemaphoreHolder.cssrc/MultiLock/SemaphoreInfo.cssrc/MultiLock/SemaphoreOptions.cssrc/MultiLock/SemaphoreService.cssrc/MultiLock/SemaphoreServiceExtensions.cssrc/MultiLock/SemaphoreStatus.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageServiceCollectionExtensions.cssrc/Providers/MultiLock.AzureBlobStorage/MultiLock.AzureBlobStorage.csprojsrc/Providers/MultiLock.Consul/ConsulSemaphoreOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cssrc/Providers/MultiLock.Consul/ConsulServiceCollectionExtensions.cssrc/Providers/MultiLock.Consul/MultiLock.Consul.csprojsrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cssrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cssrc/Providers/MultiLock.FileSystem/FileSystemServiceCollectionExtensions.cssrc/Providers/MultiLock.FileSystem/HolderFileContent.cssrc/Providers/MultiLock.FileSystem/MultiLock.FileSystem.csprojsrc/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cssrc/Providers/MultiLock.InMemory/InMemoryServiceCollectionExtensions.cssrc/Providers/MultiLock.InMemory/MultiLock.InMemory.csprojsrc/Providers/MultiLock.InMemory/SemaphoreSlotRecord.cssrc/Providers/MultiLock.PostgreSQL/MultiLock.PostgreSQL.csprojsrc/Providers/MultiLock.PostgreSQL/PostgreSQLServiceCollectionExtensions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cssrc/Providers/MultiLock.Redis/MultiLock.Redis.csprojsrc/Providers/MultiLock.Redis/RedisSemaphoreOptions.cssrc/Providers/MultiLock.Redis/RedisSemaphoreProvider.cssrc/Providers/MultiLock.Redis/RedisServiceCollectionExtensions.cssrc/Providers/MultiLock.SqlServer/MultiLock.SqlServer.csprojsrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreOptions.cssrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cssrc/Providers/MultiLock.SqlServer/SqlServerServiceCollectionExtensions.cssrc/Providers/MultiLock.ZooKeeper/MultiLock.ZooKeeper.csprojsrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperServiceCollectionExtensions.cstests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cstests/MultiLock.Tests/InMemorySemaphoreProviderTests.cstests/MultiLock.Tests/SemaphoreConcurrencyTests.cstests/MultiLock.Tests/SemaphoreParameterValidationTests.cstests/MultiLock.Tests/SemaphoreServiceTests.cs
Comment on lines
+11
to
12
| private const int maxSqlIdentifierLength = 128; | ||
| private const int maxMetadataKeyLength = 255; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the maximum identifier length for PostgreSQL table/schema names versus SQL Server identifiers, and does PostgreSQL truncate identifiers longer than its limit?
💡 Result:
PostgreSQL: maximum identifier length for table and schema names is 63 bytes (default NAMEDATALEN-1=63). SQL Server: maximum identifier length is 128 characters. PostgreSQL truncates identifiers longer than 63 bytes, issuing a NOTICE but proceeding with the truncated name; longer names can be written but are stored truncated.
Citations:
- 1: https://www.postgresql.org/docs/current/sql-syntax-lexical.html
- 2: https://www.postgresql.org/docs/current/limits.html
- 3: https://www.postgresql.org/docs/15/sql-syntax-lexical.html
- 4: https://learn.microsoft.com/en-us/sql/relational-databases/databases/database-identifiers?view=sql-server-ver17
🏁 Script executed:
cat -n src/MultiLock/ParameterValidation.cs | head -200Repository: steingran/MultiLock
Length of output: 11317
🏁 Script executed:
rg -n "ValidateSqlIdentifier" --type cs -B 2 -A 2Repository: steingran/MultiLock
Length of output: 2007
ValidateSqlIdentifier must enforce provider-specific identifier length limits.
The validator currently uses a hard-coded 128-character limit for all backends. PostgreSQL's actual identifier limit is 63 bytes; identifiers longer than 63 bytes are silently truncated by the database. This allows two distinct 128-character names to pass validation but become identical after PostgreSQL's truncation, creating a collision and correctness risk.
Pass an explicit max length parameter from each provider's options class: SqlServerSemaphoreOptions (use 128) and PostgreSqlSemaphoreOptions (use 63).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MultiLock/ParameterValidation.cs` around lines 11 - 12, The
ValidateSqlIdentifier method currently uses a hard-coded maxSqlIdentifierLength
(128); change its signature to accept an explicit maxLength parameter and use
that instead of the constant, update callers to pass provider-specific limits
from the options classes (SqlServerSemaphoreOptions -> 128,
PostgreSqlSemaphoreOptions -> 63), and remove or deprecate the hard-coded
maxSqlIdentifierLength constant; ensure the validation logic in
ValidateSqlIdentifier still checks null/empty and throws the same exception
types but uses the provided maxLength to enforce provider-specific identifier
limits.
src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs
Outdated
Show resolved
Hide resolved
tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cs
Outdated
Show resolved
Hide resolved
![diffray[bot]](https://avatars.githubusercontent.com/in/2250818?s=60&v=4){kind=small}
src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs
Outdated
Show resolved
Hide resolved
Review SummaryValidated 108 issues: 49 kept, 59 filtered Issues Found: 49💬 See 49 individual line comment(s) for details. 📊 29 unique issue type(s) across 49 location(s) 📋 Full issue list (click to expand)🟠 HIGH - ExecuteWithRetryAsync returns default on failure silentlyAgent: architecture Category: bug File: Description: ExecuteWithRetryAsync returns Suggestion: On retry exhaustion, throw the last captured exception (lastException is already stored at line 427). Callers wanting fault-tolerant behavior can catch it. Confidence: 92% Rule: 🟠 HIGH - ZooKeeper connection timeout has no effect (3 occurrences)Agent: bugs Category: bug 📍 View all locations
Rule: 🟠 HIGH - Consul session leaks on exception after creation (2 occurrences)Agent: bugs Category: bug 📍 View all locations
Rule: 🟠 HIGH - File name doesn't match public class nameAgent: csharp Category: quality File: Description: File is Suggestion: Rename the file to Confidence: 80% Rule: 🟠 HIGH - Retry count inconsistency across providersAgent: refactoring Category: bug File: Description: SemaphoreService's ExecuteWithRetryAsync uses Suggestion: Align retry semantics: either change to Confidence: 78% Rule: 🟠 HIGH - Weak concurrent acquisition assertion in Redis testAgent: testing Category: quality File: Description: ConcurrentAcquire_ShouldRespectMaxCount asserts Suggestion: Replace the loose bounds check with Confidence: 88% Rule: 🟠 HIGH - Unconditional Task.Delay used as synchronization guard (2 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟠 HIGH - Missing UpdateHeartbeatAsync and GetHoldersAsync tests (4 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - ReleaseAsync updates state even on provider failureAgent: bugs Category: bug File: Description: ReleaseAsync calls the void ExecuteWithRetryAsync overload (line 169-172) which silently returns on exhaustion (returns Suggestion: Either make the void ExecuteWithRetryAsync throw on exhaustion, or check a return value before updating local state. At minimum, log a warning about potential state inconsistency. Confidence: 85% Rule: 🟡 MEDIUM - N+1 Redis round trips in GetHoldersAsyncAgent: csharp Category: performance File: Description: GetHoldersAsync fetches all sorted set entries (line 257) then issues a separate awaited HashGetAsync per entry inside a foreach loop (line 266). This results in N+1 Redis round trips, degrading latency linearly with holder count. Suggestion: Use IBatch to pipeline all HGET commands in a single round trip, or use a Lua script to atomically retrieve both sorted set entries and hash data. Confidence: 92% Rule: 🟡 MEDIUM - N+1 ZooKeeper round trips in GetHoldersAsync (2 occurrences)Agent: performance Category: performance 📍 View all locations
Rule: 🟡 MEDIUM - Synchronous file I/O blocks thread pool in async method (2 occurrences)Agent: csharp Category: performance 📍 View all locations
Rule: 🟡 MEDIUM - JsonSerializerOptions allocated on every write callAgent: csharp Category: performance File: Description: WriteHolderFile allocates Suggestion: Declare Confidence: 95% Rule: 🟡 MEDIUM - Consul session ID returned without null checkAgent: bugs Category: bug File: Description: CreateSessionAsync returns Suggestion: Add a null check: Confidence: 70% Rule: 🟡 MEDIUM - Direct ConsulClient instantiation in constructor (3 occurrences)Agent: architecture Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Retry errors invisible by defaultAgent: architecture Category: quality File: Description: ExecuteWithRetryAsync logs retries only at Debug level behind Suggestion: Log retry attempts at Warning level by default. Expose an optional onRetry callback in SemaphoreOptions for consumers who need alerting or circuit-breaking. Confidence: 65% Rule: 🟡 MEDIUM - Magic HTTP status code 409 used inline (2 occurrences)Agent: csharp Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Empty catch block swallows all file read exceptions (2 occurrences)Agent: csharp Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Missing ConfigureAwait(false) throughout ZooKeeper provider (7 occurrences)Agent: csharp Category: performance 📍 View all locations
await connection.OpenAsync(cance... | 80% |
| `src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs:52-151` | All `await` calls in `ConsulSemaphoreProvider` omit `ConfigureAwait(false)`. Library code should not... | Add `.ConfigureAwait(false)` to every `await` expression. | 80% |
| `src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs:43-125` | All `await` calls in `AzureBlobStorageSemaphoreProvider` omit `ConfigureAwait(false)`. Library code ... | Add `.ConfigureAwait(false)` to every `await` expression. | 80% |
| `src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs:39-120` | All `await` calls in `RedisSemaphoreProvider` omit `ConfigureAwait(false)`. Library code should not ... | Add `.ConfigureAwait(false)` to every `await` expression. | 80% |
| `src/MultiLock/SemaphoreService.cs:83-103` | `SemaphoreService` is library/infrastructure code. ConfigureAwait(false) is used in some places (lin... | Add `.ConfigureAwait(false)` to every `await` expression that does not need to resume on the origina... | 75% |
</details>
**Rule:** `csharp_configureawait`
---
#### 🟡 MEDIUM - TryAcquireAsync is 140 lines with multiple abstraction levels (2 occurrences)
**Agent:** csharp
**Category:** quality
<details>
<summary>📍 View all locations</summary>
| File | Description | Suggestion | Confidence |
|------|-------------|------------|------------|
| `src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs:36-176` | `TryAcquireAsync` handles validation, advisory lock acquisition, expired slot cleanup, existing-hold... | Extract into private helper methods such as `AcquireAdvisoryLockAsync`, `CleanupExpiredSlotsAsync`, ... | 60% |
| `src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs:36-192` | `TryAcquireAsync` in `SqlServerSemaphoreProvider` mixes: parameter validation, `sp_getapplock` execu... | Extract helper methods: `AcquireApplicationLockAsync`, `CleanupExpiredHoldersAsync`, `RenewHolderIfE... | 60% |
</details>
**Rule:** `cs_refactor_large_methods_into_smaller_func`
---
#### 🟡 MEDIUM - Opaque PostgreSQL SQL state code magic strings
**Agent:** csharp
**Category:** quality
**File:** `src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs:486-494`
**Description:** Error codes "42P07", "42710", and "23505" are used directly in a catch clause filter. While comments on lines 488-491 explain them, named constants would provide compile-time safety and self-documentation at the usage site.
**Suggestion:** Extract to named constants: `private const string RelationAlreadyExists = "42P07";` etc., then use them in the catch filter.
**Confidence:** 65%
**Rule:** `cs_use_enums_instead_of_magic_strings`
---
#### 🟡 MEDIUM - Duplicated file-enumeration logic across 4 methods
**Agent:** csharp
**Category:** quality
**File:** `src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs:384-422`
**Description:** The pattern `Directory.GetFiles(semaphoreDir, $"*{options.FileExtension}")` followed by `ReadHolderFile` is duplicated across CleanupExpiredHolders, CountHolders, CountActiveHolders, and GetHoldersAsync.
**Suggestion:** Extract a private helper: `private IEnumerable<HolderFileContent?> ReadAllHolderFiles(string semaphoreDir)` that centralises file enumeration and deserialization.
**Confidence:** 72%
**Rule:** `cs_extract_duplicated_business_logic`
---
#### 🟡 MEDIUM - Synchronous BeginTransaction used in async method
**Agent:** csharp
**Category:** bug
**File:** `src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs:65`
**Description:** `connection.BeginTransaction()` (synchronous overload) is called inside an async method. SqlConnection supports BeginTransactionAsync since .NET 5+. The PostgreSQL equivalent already uses BeginTransactionAsync (line 65-66 of PostgreSqlSemaphoreProvider.cs).
**Suggestion:** Replace with: `await using var transaction = await connection.BeginTransactionAsync(System.Data.IsolationLevel.ReadCommitted, cancellationToken).ConfigureAwait(false);`
**Confidence:** 72%
**Rule:** `cs_use_proper_naming_conventions`
---
#### 🟡 MEDIUM - Public override 'process' violates PascalCase naming
**Agent:** csharp
**Category:** quality
**File:** `src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs:408`
**Description:** The public override method `process` on line 408 uses lowercase, violating .NET naming conventions. This is constrained by the Java-origin ZooKeeper client library's `Watcher` base class.
**Suggestion:** Add an XML doc comment explaining the naming deviation is due to the third-party library constraint, e.g., `/// <remarks>Name mandated by org.apache.zookeeper.Watcher base class.</remarks>`
**Confidence:** 60%
**Rule:** `cs_use_pascalcase_for_methods_and_propertie`
---
#### 🟡 MEDIUM - Discarded CAS return value silently ignores failures
**Agent:** csharp
**Category:** quality
**File:** `src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs:414`
**Description:** The result of `consulClient.KV.CAS(kvPair, cancellationToken)` on line 414 is discarded. CAS returns a bool indicating whether the compare-and-swap succeeded. If CAS fails due to concurrent modification (ModifyIndex mismatch), the heartbeat update is silently lost.
**Suggestion:** Capture and check the result: `var casResult = await consulClient.KV.CAS(kvPair, cancellationToken); if (!casResult.Response) { /* log warning or retry */ }`
**Confidence:** 75%
**Rule:** `cs_avoid_returning_unused_values`
---
#### 🟡 MEDIUM - PostgreSQL identifiers lack SQL quoting
**Agent:** [security](https://diffray.ai/agents/security)
**Category:** security
**File:** `src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs:83-85`
**Description:** SchemaName and TableName are interpolated bare into SQL (e.g., `{options.SchemaName}.{options.TableName}`) across 15+ occurrences with no double-quote wrapping. While ValidateSqlIdentifier regex is applied at construction, PostgreSQL has no bracket quoting unlike the SQL Server provider which uses `[{identifier}]`.
**Suggestion:** Add PostgreSQL standard double-quote identifier quoting as defense-in-depth: `\"{options.SchemaName}\".\"{options.TableName}\"` in addition to the existing validation.
**Confidence:** 65%
**Rule:** `sec_semgrep_scan`
---
#### 🟡 MEDIUM - TryAcquireAsync has 6 parameters
**Agent:** csharp
**Category:** quality
**File:** `src/MultiLock/ISemaphoreProvider.cs:19-25`
**Description:** The `TryAcquireAsync` method on the public `ISemaphoreProvider` interface has 6 parameters. The first five represent semaphore acquisition context and could be grouped into a parameter object for better API ergonomics.
**Suggestion:** Introduce a parameter object: `public record SemaphoreAcquisitionRequest(string SemaphoreName, string HolderId, int MaxCount, IReadOnlyDictionary<string, string> Metadata, TimeSpan SlotTimeout)` to reduce parameter count.
**Confidence:** 60%
**Rule:** `cs_group_related_parameters_in_objects`
---
#### 🔵 LOW - ZooKeeper timeout error message omits configured timeout value
**Agent:** [bugs](https://diffray.ai/agents/bugs)
**Category:** bug
**File:** `src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs:489`
**Description:** Line 489: throw new SemaphoreProviderException("Failed to connect to ZooKeeper within timeout") — does not include the configured timeout value (options.ConnectionTimeout) or connection string. Operators cannot determine if the timeout is misconfigured vs server unreachable.
**Suggestion:** Include the configured timeout and connection string in the message: $"Failed to connect to ZooKeeper within timeout ({connectionTimeout.TotalSeconds}s) at '{options.ConnectionString}'"
**Confidence:** 72%
**Rule:** `bug_generic_error_message`
---
#### 🔵 LOW - DateTime.UtcNow used for elapsed-time measurement
**Agent:** csharp
**Category:** quality
**File:** `src/MultiLock/SemaphoreService.cs:308-312`
**Description:** DateTime.UtcNow is used for duration measurement in DisposeAsync polling loop. While technically susceptible to NTP clock adjustments, the practical impact is negligible in a 10-second disposal timeout. Stopwatch would be the idiomatic choice.
**Suggestion:** Replace with System.Diagnostics.Stopwatch for correctness, though practical risk is low in this context.
**Confidence:** 62%
**Rule:** `cs_avoid_datetime_now_for_timing_operations`
---
</details>
<details>
<summary>🔇 4 low-severity issue(s) not posted (min: medium)</summary>
> Issues below `medium` severity are saved but not posted as comments.
> View all issues in the full review details.
</details>
<details>
<summary>📝 35 additional issue(s) shown in summary only (max: 10 inline)</summary>
> To reduce noise, only 10 inline comments are posted.
> All issues are listed above in the full issue list.
</details>
🔗 [View full review details](https://app.diffray.ai/dashboard/reviews/adee64ee-1df9-40b2-b3b3-56dfaf0941ce)
---
<sub>Review ID: `adee64ee-1df9-40b2-b3b3-56dfaf0941ce`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a href="https://diffray.ai?utm_source=github-private-note">diffray</a></sub> |
Owner
Author
Code reviewFound 3 issues:
MultiLock/src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs Lines 481 to 492 in 4494795
MultiLock/src/MultiLock/SemaphoreService.cs Lines 188 to 202 in 4494795
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
steingran
force-pushed
the
feat/semaphore
branch
from
4494795 to
3dd1975
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionProvider.cs (1)
537-543:⚠️ Potential issue | 🟠 MajorClose the ZooKeeper client on timed-out initialization.
Line 539 can now cancel correctly, but on the timeout branch (Lines 541-543) the already-created
zooKeeperinstance is left open. This can leak connections/threads across retries.🔧 Suggested fix
try { // WaitAsync links the TaskCompletionSource to the timeout/caller token so that // cancellation actually propagates; awaiting the Task directly never cancels. await connectionCompletionSource.Task.WaitAsync(timeoutCts.Token).ConfigureAwait(false); } catch (OperationCanceledException) when (timeoutCts.Token.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { + if (zooKeeper != null) + { + try + { + await zooKeeper.closeAsync().ConfigureAwait(false); + } + catch (Exception closeEx) + { + logger.LogWarning(closeEx, "Error closing ZooKeeper connection after connection timeout"); + } + finally + { + zooKeeper = null; + } + } throw new LeaderElectionProviderException("Failed to connect to ZooKeeper within timeout"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionProvider.cs` around lines 537 - 543, The catch for OperationCanceledException on timeout currently throws LeaderElectionProviderException but leaves the created zooKeeper instance open; update the catch block so that if the local zooKeeper variable is non-null you close/dispose it (e.g., call zooKeeper.Close/Dispose/CloseAsync as appropriate for the client API) before throwing the LeaderElectionProviderException, and swallow or log any exceptions from the close operation to avoid masking the original timeout; ensure you reference the existing connectionCompletionSource, timeoutCts, cancellationToken and zooKeeper variables in the cleanup.
♻️ Duplicate comments (5)
tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs (1)
69-70:⚠️ Potential issue | 🟠 MajorAssert setup acquires to avoid false-positive tests.
Precondition acquisitions should be asserted immediately so setup failures cannot silently invalidate test intent.
Proposed fix
- await provider.TryAcquireAsync("test-semaphore", "holder-1", maxCount, metadata, TimeSpan.FromMinutes(5)); - await provider.TryAcquireAsync("test-semaphore", "holder-2", maxCount, metadata, TimeSpan.FromMinutes(5)); + (await provider.TryAcquireAsync("test-semaphore", "holder-1", maxCount, metadata, TimeSpan.FromMinutes(5))).ShouldBeTrue(); + (await provider.TryAcquireAsync("test-semaphore", "holder-2", maxCount, metadata, TimeSpan.FromMinutes(5))).ShouldBeTrue(); - await provider.TryAcquireAsync("test-semaphore", "holder-1", 3, metadata, TimeSpan.FromMinutes(5)); + (await provider.TryAcquireAsync("test-semaphore", "holder-1", 3, metadata, TimeSpan.FromMinutes(5))).ShouldBeTrue(); - await provider.TryAcquireAsync("test-semaphore", "holder-1", maxCount, metadata, TimeSpan.FromMinutes(5)); + (await provider.TryAcquireAsync("test-semaphore", "holder-1", maxCount, metadata, TimeSpan.FromMinutes(5))).ShouldBeTrue(); - await provider.TryAcquireAsync("test-semaphore", "holder-1", 5, metadata, TimeSpan.FromMinutes(5)); - await provider.TryAcquireAsync("test-semaphore", "holder-2", 5, metadata, TimeSpan.FromMinutes(5)); + (await provider.TryAcquireAsync("test-semaphore", "holder-1", 5, metadata, TimeSpan.FromMinutes(5))).ShouldBeTrue(); + (await provider.TryAcquireAsync("test-semaphore", "holder-2", 5, metadata, TimeSpan.FromMinutes(5))).ShouldBeTrue();Also applies to: 87-87, 107-107, 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs` around lines 69 - 70, In the test setup in AzureBlobStorageSemaphoreIntegrationTests where you call provider.TryAcquireAsync("test-semaphore", "holder-1", ...) and similar precondition acquisitions, immediately assert the acquisition succeeded instead of ignoring the boolean result; replace the bare awaits with an asserted form (use Assert.True(await provider.TryAcquireAsync(...), "precondition acquire failed")) for each precondition call (the instances of TryAcquireAsync at the shown locations and the other occurrences referenced) so setup failures fail the test rather than causing silent false positives.src/MultiLock/ParameterValidation.cs (1)
11-12:⚠️ Potential issue | 🟠 MajorUse provider-specific SQL identifier limits instead of a global
128.A single 128-char limit can allow PostgreSQL identifiers that are truncated by the database, creating collision risk.
Proposed fix direction
- private const int maxSqlIdentifierLength = 128; ... - public static void ValidateSqlIdentifier(string value, string paramName) + public static void ValidateSqlIdentifier(string value, string paramName, int maxLength) { if (string.IsNullOrWhiteSpace(value)) throw new ArgumentException("SQL identifier cannot be null, empty, or whitespace.", paramName); - if (value.Length > maxSqlIdentifierLength) + if (value.Length > maxLength) throw new ArgumentException( - $"SQL identifier cannot exceed {maxSqlIdentifierLength} characters.", paramName); + $"SQL identifier cannot exceed {maxLength} characters.", paramName);Then pass provider-specific limits from options validation (e.g., SQL Server 128, PostgreSQL 63).
Also applies to: 177-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/ParameterValidation.cs` around lines 11 - 12, Replace the hardcoded maxSqlIdentifierLength constant with a provider-specific limit passed in from options validation: stop using the global private const int maxSqlIdentifierLength = 128 and instead accept a configured SqlIdentifierMaxLength (or similar) from the options/validation flow that ValidateOptions/ParameterValidation uses; ensure ParameterValidation (and any checks around maxMetadataKeyLength and the related checks at the other occurrences referenced) use this provider-specific value (e.g., SQL Server 128, PostgreSQL 63) so identifiers are validated against the provider limit supplied by the options rather than a fixed 128.src/MultiLock/SemaphoreServiceExtensions.cs (1)
27-29:⚠️ Potential issue | 🟠 MajorRemove runtime cast risk in hosted-service wiring.
Casting
ISemaphoreServicetoSemaphoreServicecan throw if a customISemaphoreServiceis already registered.Safer DI wiring
using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Hosting; ... - services.TryAddSingleton<ISemaphoreService, SemaphoreService>(); - services.AddHostedService<SemaphoreService>(provider => - (SemaphoreService)provider.GetRequiredService<ISemaphoreService>()); + services.TryAddSingleton<SemaphoreService>(); + services.TryAddSingleton<ISemaphoreService>(sp => sp.GetRequiredService<SemaphoreService>()); + services.TryAddEnumerable( + ServiceDescriptor.Singleton<IHostedService>(sp => sp.GetRequiredService<SemaphoreService>()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreServiceExtensions.cs` around lines 27 - 29, The current code casts provider.GetRequiredService<ISemaphoreService>() to SemaphoreService which can throw if a different ISemaphoreService is registered; instead, register the concrete SemaphoreService and resolve it directly for the hosted service. Replace the registration so SemaphoreService is registered as the concrete singleton (e.g. services.TryAddSingleton<SemaphoreService>(); services.TryAddSingleton<ISemaphoreService>(sp => sp.GetRequiredService<SemaphoreService>())), and change the hosted service wiring to services.AddHostedService<SemaphoreService>(provider => provider.GetRequiredService<SemaphoreService>()), referencing ISemaphoreService, SemaphoreService, TryAddSingleton and AddHostedService in the diff.src/MultiLock/SemaphoreAcquisition.cs (1)
59-63:⚠️ Potential issue | 🟠 MajorMake the dispose guard atomic to prevent double release.
The
if (isDisposed) return; isDisposed = true;sequence is racy under concurrent disposal and can invokeReleaseAsyncmore than once.Proposed fix
- private volatile bool isDisposed; + private int disposeState; ... - public bool IsDisposed => isDisposed; + public bool IsDisposed => Volatile.Read(ref disposeState) == 1; ... - if (isDisposed) + if (Interlocked.Exchange(ref disposeState, 1) == 1) return; - - isDisposed = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreAcquisition.cs` around lines 59 - 63, The current non-atomic dispose guard (if (isDisposed) return; isDisposed = true;) is racy and can allow multiple concurrent disposals to call ReleaseAsync; make the guard atomic by replacing the bool isDisposed with an int (e.g., disposedFlag) and use Interlocked.CompareExchange/Interlocked.Exchange in the SemaphoreAcquisition.Dispose method to set the flag from 0 to 1 and only call ReleaseAsync when the atomic operation indicates this call performed the transition; reference isDisposed (rename to disposedFlag), Dispose (or SemaphoreAcquisition.Dispose), and ReleaseAsync to locate and update the logic.src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs (1)
95-109:⚠️ Potential issue | 🔴 CriticalIndexOf returning -1 bypasses maxCount check.
If the created node is deleted between
createAsync(line 92) andgetChildrenAsync(line 96)—for example, due to session expiration—IndexOfreturns -1. The conditionposition >= maxCountevaluates to false for -1, causing the method to returntruedespite the slot no longer existing.🐛 Proposed fix
int position = sortedChildren.IndexOf(createdNodeName); - if (position >= maxCount) + if (position < 0 || position >= maxCount) { // We're over the limit, release our slot - await zooKeeper.deleteAsync(createdPath); + try + { + await zooKeeper.deleteAsync(createdPath); + } + catch (KeeperException.NoNodeException) + { + // Already deleted, ignore + } RemoveNode(semaphoreName, holderId); - logger.LogDebug("Semaphore {SemaphoreName} became full during acquisition, releasing slot", - semaphoreName); + logger.LogDebug("Semaphore {SemaphoreName} acquisition failed (position={Position}), releasing slot", + semaphoreName, position); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs` around lines 95 - 109, The code that checks position after calling zooKeeper.getChildrenAsync can see IndexOf return -1 if the created node was removed (e.g., session expired), which bypasses the maxCount check; update the logic in the semaphore acquisition path (around createdPath/createdNodeName, the call to zooKeeper.getChildrenAsync and the position calculation) to explicitly handle position == -1 as a failure: if position == -1 then treat the slot as gone (attempt cleanup if necessary via zooKeeper.deleteAsync/RemoveNode but do not assume success) and return false; otherwise keep the existing position >= maxCount check to release the slot and return false when appropriate.
🧹 Nitpick comments (5)
src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs (1)
34-35: Synchronous connection in Lazy initializer may block indefinitely.
ConnectionMultiplexer.Connectis synchronous and can block for a long time (default timeout ~60s) if Redis is unreachable. Since this is in aLazy<T>initializer called from async methods, it may block thread pool threads. Consider usingLazy<Task<ConnectionMultiplexer>>withConnectAsyncfor non-blocking initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs` around lines 34 - 35, Replace the blocking Lazy<ConnectionMultiplexer> initialization with an asynchronous lazy pattern: change the field from connectionMultiplexer = new Lazy<ConnectionMultiplexer>(...) to a Lazy<Task<ConnectionMultiplexer>> that calls ConnectionMultiplexer.ConnectAsync(this.options.ConnectionString) (optionally specifying LazyThreadSafetyMode.ExecutionAndPublication). Update all usages in methods such as any Acquire/Release or helper methods that access connectionMultiplexer.Value to instead await connectionMultiplexer.Value (and await the returned Task<ConnectionMultiplexer>) so initialization is non-blocking and asynchronous.tests/MultiLock.Tests/SemaphoreServiceTests.cs (2)
145-158: Race window in event collection test.The 100ms delay before
TryAcquireAsyncis a timing-based workaround to allow the subscription to start. While acceptable for in-memory testing, this pattern can cause flaky tests under load. Consider using a synchronization primitive (e.g.,TaskCompletionSourcesignaled when the first iteration ofawait foreachbegins) for deterministic behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.Tests/SemaphoreServiceTests.cs` around lines 145 - 158, Replace the timing-based Task.Delay with an explicit synchronization between the collector and the test: create a TaskCompletionSource (e.g., tcs) that the collector code inside the Task.Run signals when the first iteration of await foreach in GetStatusChangesAsync begins, await tcs.Task in the test before calling service.TryAcquireAsync(), and then let the collector continue and complete (the collectTask and service.GetStatusChangesAsync remain unchanged); this removes the 100ms delay and makes the test deterministic and robust under load.
114-122: Async test methods without await.These test methods are marked
asyncbut contain noawaitexpressions. While this works (the compiler returns a completedTask), consider removing theasyncmodifier to avoid compiler warnings and clarify intent.♻️ Suggested fix
[Fact] - public async Task SemaphoreName_ShouldBeSetFromOptions() + public Task SemaphoreName_ShouldBeSetFromOptions() { // Arrange const string expectedName = "my-semaphore"; service = CreateService(expectedName, 3); // Assert service.SemaphoreName.ShouldBe(expectedName); + return Task.CompletedTask; }Also applies to: 125-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.Tests/SemaphoreServiceTests.cs` around lines 114 - 122, The test method SemaphoreName_ShouldBeSetFromOptions is marked async but contains no await; remove the async modifier and change its return type from Task to void (i.e., make it a synchronous test) to avoid compiler warnings; apply the same change to the other test at lines 125-133 (the other async test in SemaphoreServiceTests) so both methods are non-async void tests.src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs (1)
239-248: Consider logging expired slot cleanup.The
CleanupExpiredSlotsmethod silently removes expired holders. For debugging purposes in development/testing scenarios, consider adding debug-level logging when slots are expired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs` around lines 239 - 248, The CleanupExpiredSlots method currently removes expired entries silently; update it to emit debug-level logs when expirations are found by either using an existing ILogger on the class or adding an optional ILogger parameter to CleanupExpiredSlots, then after computing expiredKeys (from holders.Where(...) into expiredKeys) log a debug message containing the number of expired slots and their keys (or identifiers from SemaphoreSlotRecord) along with expiryTime; guard the logging with IsEnabled(LogLevel.Debug) (or equivalent) to avoid overhead in production.src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs (1)
401-404: Sync-over-async pattern carries deadlock risk.The warning comment is helpful, but callers may still invoke
Dispose()inadvertently. Consider implementingIDisposableonly if truly required by the DI container; otherwise, removing it forces callers to useDisposeAsync().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs` around lines 401 - 404, The synchronous Dispose() implementation in ZooKeeperSemaphoreProvider uses sync-over-async (DisposeAsync().AsTask().GetAwaiter().GetResult()) which can deadlock; remove the IDisposable/Dispose() surface so callers must use DisposeAsync() instead (or if your DI requires IDisposable, replace the sync stub with a safe non-blocking pattern such as registering DisposeAsync with the container or calling DisposeAsync().ConfigureAwait(false).GetAwaiter().GetResult() only after confirming no sync context — preferably remove Dispose() altogether). Update the class to expose only DisposeAsync() (and remove any IDisposable interface implementation) unless the DI container mandates IDisposable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MultiLock/SemaphoreAcquisition.cs`:
- Around line 81-84: The synchronous Dispose() currently blocks on
DisposeAsync().AsTask().GetAwaiter().GetResult(), which can deadlock if the
underlying provider's ReleaseAsync() does I/O and tries to resume on a captured
synchronization context; change Dispose() to run the async disposal on the
thread-pool to avoid context capture by invoking DisposeAsync() inside Task.Run
(so the awaited task executes off the current sync context) and then
synchronously wait for completion; update the Dispose() implementation that
calls DisposeAsync() to use Task.Run(() =>
DisposeAsync().AsTask()).GetAwaiter().GetResult() (affecting the Dispose and
DisposeAsync interaction and ensuring ReleaseAsync() cannot deadlock the
caller).
In
`@tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs`:
- Around line 18-21: LoggerFactory.Create(...) in the test setup allocates a
disposable ILoggerFactory stored as loggerFactory but it is never disposed;
modify the test class AzureBlobStorageSemaphoreIntegrationTests to dispose the
loggerFactory in the test teardown (implement IDisposable.Dispose or a
[TearDown]/DisposeAsync method used by the test framework) and ensure you call
loggerFactory.Dispose() (or await DisposeAsync) and null it after use; apply the
same disposal change for the other occurrence that initializes loggerFactory
(the second setup at the other test case referenced).
In `@tests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cs`:
- Around line 74-75: The two precondition calls to
provider.TryAcquireAsync("test-semaphore", "holder-1", ...) and
provider.TryAcquireAsync("test-semaphore", "holder-2", ...) should be asserted
so failures surface immediately; change each blind await to capture the boolean
result from TryAcquireAsync and assert it (e.g. Assert.True(result, "failed to
acquire holder-1/holder-2")), and make the same change for the other occurrences
of TryAcquireAsync referenced (lines around the other test preconditions) so
every pre-acquisition uses an Assert on the returned value.
In `@tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs`:
- Around line 23-27: The ILoggerFactory created in the
SqlServerSemaphoreIntegrationTests constructor is not disposed; make it a
private field (e.g., ILoggerFactory loggerFactory), assign it in the constructor
instead of a local variable, and then dispose it in the test class's
DisposeAsync method (call loggerFactory.Dispose() or await
loggerFactory.DisposeAsync() if available) after disposing other resources;
ensure you still create the logger via
loggerFactory.CreateLogger<SqlServerSemaphoreProvider>() and that DisposeAsync
handles null-checks.
- Around line 90-91: The test setup currently calls TryAcquireAsync without
asserting the result; change those lines to assert acquisition succeeded (use
Assert.True(await provider.TryAcquireAsync(semaphoreName, "holder-1", maxCount,
metadata, TimeSpan.FromMinutes(5)), "holder-1 should acquire") and similarly for
"holder-2") so preconditions fail the test immediately if acquire fails; apply
the same pattern for all occurrences of TryAcquireAsync in this file (including
the other call sites noted) using Assert.True (or the project's assertion
helper) and an explanatory message.
- Around line 13-15: The ConnectionString fallback currently embeds hardcoded
credentials in the static field ConnectionString; replace that fallback with a
strict check that throws a clear exception when the
MULTILOCK_SQLSERVER_CONNECTION environment variable is missing or empty (e.g.,
in the static initializer for SqlServerSemaphoreIntegrationTests or where
ConnectionString is assigned) so tests fail fast in CI/non-local environments
instead of using hardcoded secrets.
In `@tests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cs`:
- Around line 16-21: The ILoggerFactory created in the
ZooKeeperSemaphoreIntegrationTests constructor is not disposed; make the
loggerFactory a private field (e.g., private readonly ILoggerFactory
loggerFactory), assign it in the constructor where you currently create it, and
then dispose it inside DisposeAsync (call loggerFactory.Dispose() or await using
if switching to IAsyncDisposable) to release logging resources; keep the
existing logger assignment (logger =
loggerFactory.CreateLogger<ZooKeeperSemaphoreProvider>()) and ensure
DisposeAsync disposes loggerFactory before returning.
---
Outside diff comments:
In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionProvider.cs`:
- Around line 537-543: The catch for OperationCanceledException on timeout
currently throws LeaderElectionProviderException but leaves the created
zooKeeper instance open; update the catch block so that if the local zooKeeper
variable is non-null you close/dispose it (e.g., call
zooKeeper.Close/Dispose/CloseAsync as appropriate for the client API) before
throwing the LeaderElectionProviderException, and swallow or log any exceptions
from the close operation to avoid masking the original timeout; ensure you
reference the existing connectionCompletionSource, timeoutCts, cancellationToken
and zooKeeper variables in the cleanup.
---
Duplicate comments:
In `@src/MultiLock/ParameterValidation.cs`:
- Around line 11-12: Replace the hardcoded maxSqlIdentifierLength constant with
a provider-specific limit passed in from options validation: stop using the
global private const int maxSqlIdentifierLength = 128 and instead accept a
configured SqlIdentifierMaxLength (or similar) from the options/validation flow
that ValidateOptions/ParameterValidation uses; ensure ParameterValidation (and
any checks around maxMetadataKeyLength and the related checks at the other
occurrences referenced) use this provider-specific value (e.g., SQL Server 128,
PostgreSQL 63) so identifiers are validated against the provider limit supplied
by the options rather than a fixed 128.
In `@src/MultiLock/SemaphoreAcquisition.cs`:
- Around line 59-63: The current non-atomic dispose guard (if (isDisposed)
return; isDisposed = true;) is racy and can allow multiple concurrent disposals
to call ReleaseAsync; make the guard atomic by replacing the bool isDisposed
with an int (e.g., disposedFlag) and use
Interlocked.CompareExchange/Interlocked.Exchange in the
SemaphoreAcquisition.Dispose method to set the flag from 0 to 1 and only call
ReleaseAsync when the atomic operation indicates this call performed the
transition; reference isDisposed (rename to disposedFlag), Dispose (or
SemaphoreAcquisition.Dispose), and ReleaseAsync to locate and update the logic.
In `@src/MultiLock/SemaphoreServiceExtensions.cs`:
- Around line 27-29: The current code casts
provider.GetRequiredService<ISemaphoreService>() to SemaphoreService which can
throw if a different ISemaphoreService is registered; instead, register the
concrete SemaphoreService and resolve it directly for the hosted service.
Replace the registration so SemaphoreService is registered as the concrete
singleton (e.g. services.TryAddSingleton<SemaphoreService>();
services.TryAddSingleton<ISemaphoreService>(sp =>
sp.GetRequiredService<SemaphoreService>())), and change the hosted service
wiring to services.AddHostedService<SemaphoreService>(provider =>
provider.GetRequiredService<SemaphoreService>()), referencing ISemaphoreService,
SemaphoreService, TryAddSingleton and AddHostedService in the diff.
In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs`:
- Around line 95-109: The code that checks position after calling
zooKeeper.getChildrenAsync can see IndexOf return -1 if the created node was
removed (e.g., session expired), which bypasses the maxCount check; update the
logic in the semaphore acquisition path (around createdPath/createdNodeName, the
call to zooKeeper.getChildrenAsync and the position calculation) to explicitly
handle position == -1 as a failure: if position == -1 then treat the slot as
gone (attempt cleanup if necessary via zooKeeper.deleteAsync/RemoveNode but do
not assume success) and return false; otherwise keep the existing position >=
maxCount check to release the slot and return false when appropriate.
In
`@tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs`:
- Around line 69-70: In the test setup in
AzureBlobStorageSemaphoreIntegrationTests where you call
provider.TryAcquireAsync("test-semaphore", "holder-1", ...) and similar
precondition acquisitions, immediately assert the acquisition succeeded instead
of ignoring the boolean result; replace the bare awaits with an asserted form
(use Assert.True(await provider.TryAcquireAsync(...), "precondition acquire
failed")) for each precondition call (the instances of TryAcquireAsync at the
shown locations and the other occurrences referenced) so setup failures fail the
test rather than causing silent false positives.
---
Nitpick comments:
In `@src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs`:
- Around line 239-248: The CleanupExpiredSlots method currently removes expired
entries silently; update it to emit debug-level logs when expirations are found
by either using an existing ILogger on the class or adding an optional ILogger
parameter to CleanupExpiredSlots, then after computing expiredKeys (from
holders.Where(...) into expiredKeys) log a debug message containing the number
of expired slots and their keys (or identifiers from SemaphoreSlotRecord) along
with expiryTime; guard the logging with IsEnabled(LogLevel.Debug) (or
equivalent) to avoid overhead in production.
In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs`:
- Around line 34-35: Replace the blocking Lazy<ConnectionMultiplexer>
initialization with an asynchronous lazy pattern: change the field from
connectionMultiplexer = new Lazy<ConnectionMultiplexer>(...) to a
Lazy<Task<ConnectionMultiplexer>> that calls
ConnectionMultiplexer.ConnectAsync(this.options.ConnectionString) (optionally
specifying LazyThreadSafetyMode.ExecutionAndPublication). Update all usages in
methods such as any Acquire/Release or helper methods that access
connectionMultiplexer.Value to instead await connectionMultiplexer.Value (and
await the returned Task<ConnectionMultiplexer>) so initialization is
non-blocking and asynchronous.
In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs`:
- Around line 401-404: The synchronous Dispose() implementation in
ZooKeeperSemaphoreProvider uses sync-over-async
(DisposeAsync().AsTask().GetAwaiter().GetResult()) which can deadlock; remove
the IDisposable/Dispose() surface so callers must use DisposeAsync() instead (or
if your DI requires IDisposable, replace the sync stub with a safe non-blocking
pattern such as registering DisposeAsync with the container or calling
DisposeAsync().ConfigureAwait(false).GetAwaiter().GetResult() only after
confirming no sync context — preferably remove Dispose() altogether). Update the
class to expose only DisposeAsync() (and remove any IDisposable interface
implementation) unless the DI container mandates IDisposable.
In `@tests/MultiLock.Tests/SemaphoreServiceTests.cs`:
- Around line 145-158: Replace the timing-based Task.Delay with an explicit
synchronization between the collector and the test: create a
TaskCompletionSource (e.g., tcs) that the collector code inside the Task.Run
signals when the first iteration of await foreach in GetStatusChangesAsync
begins, await tcs.Task in the test before calling service.TryAcquireAsync(), and
then let the collector continue and complete (the collectTask and
service.GetStatusChangesAsync remain unchanged); this removes the 100ms delay
and makes the test deterministic and robust under load.
- Around line 114-122: The test method SemaphoreName_ShouldBeSetFromOptions is
marked async but contains no await; remove the async modifier and change its
return type from Task to void (i.e., make it a synchronous test) to avoid
compiler warnings; apply the same change to the other test at lines 125-133 (the
other async test in SemaphoreServiceTests) so both methods are non-async void
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd5eabe3-6606-414e-b26d-5b5ad6c0e23f
📒 Files selected for processing (68)
MultiLock.slnxREADME.mdsamples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csprojsamples/MultiLock.SemaphoreSample/Program.cssamples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cssrc/MultiLock/Exceptions/SemaphoreException.cssrc/MultiLock/Exceptions/SemaphoreFullException.cssrc/MultiLock/Exceptions/SemaphoreProviderException.cssrc/MultiLock/Exceptions/SemaphoreTimeoutException.cssrc/MultiLock/ISemaphoreProvider.cssrc/MultiLock/ISemaphoreService.cssrc/MultiLock/MultiLock.csprojsrc/MultiLock/ParameterValidation.cssrc/MultiLock/SemaphoreAcquisition.cssrc/MultiLock/SemaphoreChangedEventArgs.cssrc/MultiLock/SemaphoreHolder.cssrc/MultiLock/SemaphoreInfo.cssrc/MultiLock/SemaphoreOptions.cssrc/MultiLock/SemaphoreService.cssrc/MultiLock/SemaphoreServiceExtensions.cssrc/MultiLock/SemaphoreStatus.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageServiceCollectionExtensions.cssrc/Providers/MultiLock.AzureBlobStorage/MultiLock.AzureBlobStorage.csprojsrc/Providers/MultiLock.Consul/ConsulLeaderElectionOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cssrc/Providers/MultiLock.Consul/ConsulServiceCollectionExtensions.cssrc/Providers/MultiLock.Consul/MultiLock.Consul.csprojsrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cssrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cssrc/Providers/MultiLock.FileSystem/FileSystemServiceCollectionExtensions.cssrc/Providers/MultiLock.FileSystem/HolderFileContent.cssrc/Providers/MultiLock.FileSystem/MultiLock.FileSystem.csprojsrc/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cssrc/Providers/MultiLock.InMemory/InMemoryServiceCollectionExtensions.cssrc/Providers/MultiLock.InMemory/MultiLock.InMemory.csprojsrc/Providers/MultiLock.InMemory/SemaphoreSlotRecord.cssrc/Providers/MultiLock.PostgreSQL/MultiLock.PostgreSQL.csprojsrc/Providers/MultiLock.PostgreSQL/PostgreSQLServiceCollectionExtensions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cssrc/Providers/MultiLock.Redis/MultiLock.Redis.csprojsrc/Providers/MultiLock.Redis/RedisSemaphoreOptions.cssrc/Providers/MultiLock.Redis/RedisSemaphoreProvider.cssrc/Providers/MultiLock.Redis/RedisServiceCollectionExtensions.cssrc/Providers/MultiLock.SqlServer/MultiLock.SqlServer.csprojsrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreOptions.cssrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cssrc/Providers/MultiLock.SqlServer/SqlServerServiceCollectionExtensions.cssrc/Providers/MultiLock.ZooKeeper/MultiLock.ZooKeeper.csprojsrc/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperServiceCollectionExtensions.cstests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cstests/MultiLock.Tests/InMemorySemaphoreProviderTests.cstests/MultiLock.Tests/SemaphoreConcurrencyTests.cstests/MultiLock.Tests/SemaphoreParameterValidationTests.cstests/MultiLock.Tests/SemaphoreServiceTests.cs
✅ Files skipped from review due to trivial changes (30)
- src/MultiLock/MultiLock.csproj
- src/MultiLock/Exceptions/SemaphoreException.cs
- samples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csproj
- src/Providers/MultiLock.Consul/MultiLock.Consul.csproj
- src/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionOptions.cs
- src/Providers/MultiLock.SqlServer/MultiLock.SqlServer.csproj
- src/Providers/MultiLock.Redis/MultiLock.Redis.csproj
- src/Providers/MultiLock.FileSystem/MultiLock.FileSystem.csproj
- src/Providers/MultiLock.AzureBlobStorage/MultiLock.AzureBlobStorage.csproj
- src/MultiLock/Exceptions/SemaphoreProviderException.cs
- MultiLock.slnx
- src/Providers/MultiLock.FileSystem/HolderFileContent.cs
- src/Providers/MultiLock.InMemory/SemaphoreSlotRecord.cs
- src/Providers/MultiLock.ZooKeeper/MultiLock.ZooKeeper.csproj
- src/MultiLock/SemaphoreHolder.cs
- src/MultiLock/SemaphoreInfo.cs
- src/Providers/MultiLock.InMemory/MultiLock.InMemory.csproj
- src/Providers/MultiLock.PostgreSQL/MultiLock.PostgreSQL.csproj
- src/MultiLock/Exceptions/SemaphoreTimeoutException.cs
- src/Providers/MultiLock.Redis/RedisSemaphoreOptions.cs
- src/MultiLock/SemaphoreChangedEventArgs.cs
- src/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cs
- src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cs
- src/Providers/MultiLock.Consul/ConsulSemaphoreOptions.cs
- src/Providers/MultiLock.SqlServer/SqlServerSemaphoreOptions.cs
- src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cs
- src/MultiLock/SemaphoreStatus.cs
- src/MultiLock/Exceptions/SemaphoreFullException.cs
- tests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cs
- src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs
🚧 Files skipped from review as they are similar to previous changes (19)
- README.md
- src/Providers/MultiLock.FileSystem/FileSystemServiceCollectionExtensions.cs
- samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs
- src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageServiceCollectionExtensions.cs
- src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cs
- src/Providers/MultiLock.Redis/RedisServiceCollectionExtensions.cs
- src/Providers/MultiLock.Consul/ConsulServiceCollectionExtensions.cs
- src/Providers/MultiLock.PostgreSQL/PostgreSQLServiceCollectionExtensions.cs
- tests/MultiLock.Tests/SemaphoreConcurrencyTests.cs
- src/MultiLock/ISemaphoreService.cs
- tests/MultiLock.Tests/InMemorySemaphoreProviderTests.cs
- src/MultiLock/ISemaphoreProvider.cs
- tests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cs
- samples/MultiLock.SemaphoreSample/Program.cs
- src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs
- tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs
- src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs
- src/MultiLock/SemaphoreService.cs
- src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs
Comment on lines
+81
to
+84
| public void Dispose() | ||
| { | ||
| DisposeAsync().AsTask().GetAwaiter().GetResult(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "SemaphoreAcquisition.cs" -type fRepository: steingran/MultiLock
Length of output: 102
🏁 Script executed:
cat -n ./src/MultiLock/SemaphoreAcquisition.csRepository: steingran/MultiLock
Length of output: 3276
🏁 Script executed:
find . -name "*.cs" -type f | xargs grep -l "interface ISemaphoreProvider"Repository: steingran/MultiLock
Length of output: 100
🏁 Script executed:
cat -n ./src/MultiLock/ISemaphoreProvider.csRepository: steingran/MultiLock
Length of output: 5146
🏁 Script executed:
find . -name "*.cs" -type f | xargs grep -l "class.*ISemaphoreProvider"Repository: steingran/MultiLock
Length of output: 638
🏁 Script executed:
cat -n ./src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs | head -100Repository: steingran/MultiLock
Length of output: 4733
🏁 Script executed:
cat -n ./src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs | sed -n '91,130p'Repository: steingran/MultiLock
Length of output: 1696
🏁 Script executed:
cat -n ./src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs | grep -A 30 "public Task ReleaseAsync"Repository: steingran/MultiLock
Length of output: 45
🏁 Script executed:
find ./src/Providers/MultiLock.Redis -name "*.cs" -type fRepository: steingran/MultiLock
Length of output: 370
🏁 Script executed:
cat -n ./src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs | head -150Repository: steingran/MultiLock
Length of output: 6805
Synchronous dispose can deadlock when called from a synchronization context.
Blocking on DisposeAsync().AsTask().GetAwaiter().GetResult() can deadlock if the provider's ReleaseAsync() method performs actual async I/O (such as in Redis, SQL Server, Consul, Azure Blob Storage, and other network/database backends) and attempts to resume on the captured synchronization context. The current thread blocks waiting for completion while the async operation cannot resume on a blocked context.
The existing remarks (lines 77-80) acknowledge this limitation. The recommended fix wraps the async call in Task.Run() to execute on the thread pool, preventing synchronization context capture and eliminating the deadlock risk:
Proposed mitigation
public void Dispose()
{
- DisposeAsync().AsTask().GetAwaiter().GetResult();
+ Task.Run(() => DisposeAsync().AsTask()).GetAwaiter().GetResult();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MultiLock/SemaphoreAcquisition.cs` around lines 81 - 84, The synchronous
Dispose() currently blocks on DisposeAsync().AsTask().GetAwaiter().GetResult(),
which can deadlock if the underlying provider's ReleaseAsync() does I/O and
tries to resume on a captured synchronization context; change Dispose() to run
the async disposal on the thread-pool to avoid context capture by invoking
DisposeAsync() inside Task.Run (so the awaited task executes off the current
sync context) and then synchronously wait for completion; update the Dispose()
implementation that calls DisposeAsync() to use Task.Run(() =>
DisposeAsync().AsTask()).GetAwaiter().GetResult() (affecting the Dispose and
DisposeAsync interaction and ensuring ReleaseAsync() cannot deadlock the
caller).
tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs
Outdated
Show resolved
Hide resolved
steingran
force-pushed
the
feat/semaphore
branch
from
3dd1975 to
12c8d95
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (6)
samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs (1)
47-60:⚠️ Potential issue | 🟠 MajorAlways release in
finally, and avoid cancelling the release call.If
SimulateApiCallAsyncthrows (including on cancellation), the slot is never released. Additionally, passingstoppingTokentoReleaseAsyncrisks cancelling the release itself during shutdown, leaving the slot orphaned until heartbeat timeout.Proposed fix
if (acquired) { logger.LogInformation( "[Request {RequestId}] ✓ Acquired slot! Currently holding: {IsHolding}", requestId, semaphoreService.IsHolding); - // Simulate API call work - await SimulateApiCallAsync(requestId, stoppingToken); - - // Release the slot - await semaphoreService.ReleaseAsync(stoppingToken); - logger.LogInformation("[Request {RequestId}] Released slot", requestId); + try + { + // Simulate API call work + await SimulateApiCallAsync(requestId, stoppingToken); + } + finally + { + await semaphoreService.ReleaseAsync(CancellationToken.None); + logger.LogInformation("[Request {RequestId}] Released slot", requestId); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs` around lines 47 - 60, The acquired slot must always be released even if SimulateApiCallAsync throws, and ReleaseAsync must not be cancellable by stoppingToken; move the await semaphoreService.ReleaseAsync(...) into a finally block that runs only when acquired is true, and call ReleaseAsync without passing stoppingToken so the release completes during shutdown; update the try/await SimulateApiCallAsync(requestId, stoppingToken) block around the simulated work and ensure the logger.LogInformation("[Request {RequestId}] Released slot", requestId) runs after a successful release in that finally.src/MultiLock/SemaphoreAcquisition.cs (2)
77-83:⚠️ Potential issue | 🟠 MajorStill unresolved:
Dispose()can deadlock on async providers.
DisposeAsync().AsTask().GetAwaiter().GetResult()at Line 83 still blocks on real async I/O. The XML remark documents the hazard, but it does not remove it. Please verify this against a provider whoseReleaseAsyncawaits network or database I/O from a single-threaded synchronization context.Suggested fix
public void Dispose() { - DisposeAsync().AsTask().GetAwaiter().GetResult(); + Task.Run(() => DisposeAsync().AsTask()).GetAwaiter().GetResult(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreAcquisition.cs` around lines 77 - 83, The synchronous Dispose implementation still blocks on async I/O and can deadlock; replace the blocking call in Dispose with a non-blocking failure: have Dispose throw NotSupportedException (or InvalidOperationException) instructing callers to use DisposeAsync instead, and remove the GetAwaiter().GetResult() call so no synchronous wrapper awaits ReleaseAsync; reference the SemaphoreAcquisition.Dispose method and the existing DisposeAsync / ReleaseAsync flow so reviewers can verify callers use await using or call DisposeAsync when the provider performs network/database I/O.
10-10:⚠️ Potential issue | 🟠 MajorStill unresolved: make the dispose guard atomic.
Lines 59-62 are still racy. Two concurrent disposals can both pass the check and call
provider.ReleaseAsync, duplicating the release path under contention.Suggested fix
- private volatile bool isDisposed; + private int disposeState; ... - public bool IsDisposed => isDisposed; + public bool IsDisposed => Volatile.Read(ref disposeState) == 1; ... - if (isDisposed) + if (Interlocked.Exchange(ref disposeState, 1) == 1) return; - - isDisposed = true;Also applies to: 52-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreAcquisition.cs` at line 10, The dispose guard on the SemaphoreAcquisition class is racy: the volatile isDisposed check allows two concurrent disposals to both call provider.ReleaseAsync; change the guard to an atomic compare/exchange (e.g., replace the volatile bool isDisposed with an integer flag and use Interlocked.Exchange/CompareExchange) inside DisposeAsync/Dispose so only the first caller proceeds to call provider.ReleaseAsync and subsequent calls return immediately; keep the same provider.ReleaseAsync call and semantics but gate it behind the atomic transition of the flag.src/MultiLock/SemaphoreServiceExtensions.cs (1)
27-29:⚠️ Potential issue | 🟠 MajorStill unresolved: avoid casting
ISemaphoreServiceback toSemaphoreService.If a consumer pre-registers a custom
ISemaphoreService,TryAddSingletonleaves it in place and Line 29 throwsInvalidCastExceptionwhen the hosted-service factory runs. Please verify this by pre-registering a stubISemaphoreServiceand resolvingIHostedServicefrom the built container.Safer DI wiring
- services.TryAddSingleton<ISemaphoreService, SemaphoreService>(); - services.AddHostedService<SemaphoreService>(provider => - (SemaphoreService)provider.GetRequiredService<ISemaphoreService>()); + services.TryAddSingleton<SemaphoreService>(); + services.TryAddSingleton<ISemaphoreService>( + sp => sp.GetRequiredService<SemaphoreService>()); + services.TryAddEnumerable( + ServiceDescriptor.Singleton<Microsoft.Extensions.Hosting.IHostedService>( + sp => sp.GetRequiredService<SemaphoreService>()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreServiceExtensions.cs` around lines 27 - 29, The current factory casts provider.GetRequiredService<ISemaphoreService>() to SemaphoreService which throws if a custom ISemaphoreService was pre-registered; change the AddHostedService registration to resolve ISemaphoreService and (a) if the resolved instance already implements IHostedService return it, otherwise (b) wrap the resolved ISemaphoreService in a small adapter that implements IHostedService and delegates StartAsync/StopAsync to the semaphore service methods. Update the registration in AddHostedService to use that resolver (and add the adapter type if needed) instead of casting to SemaphoreService; keep TryAddSingleton<ISemaphoreService, SemaphoreService>() unchanged.src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs (1)
95-149:⚠️ Potential issue | 🟠 MajorClean up the newly created Consul session on every failure path.
This is still vulnerable to leaking a live session: once Line 96 succeeds, any exception before Line 140 leaves a session that is not yet tracked in
activeSessions, soDisposeAsync()cannot recover it later. That leaked session/key can hold a slot until TTL expiry.tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs (1)
13-15:⚠️ Potential issue | 🟠 MajorRemove the checked-in PostgreSQL fallback.
MULTILOCK_POSTGRES_CONNECTIONstill falls back to embedded credentials, so the secret is still in source control and the tests can silently bind to a developer-local database. It also makes the Line 33 localhost-specific failure message misleading when the env var points somewhere else.🔐 Suggested change
- private static readonly string connectionString = - Environment.GetEnvironmentVariable("MULTILOCK_POSTGRES_CONNECTION") - ?? "Host=localhost;Database=leaderelection;Username=leaderelection;Password=leaderelection123"; + private static readonly string connectionString = + Environment.GetEnvironmentVariable("MULTILOCK_POSTGRES_CONNECTION") + ?? throw new InvalidOperationException( + "Set MULTILOCK_POSTGRES_CONNECTION before running PostgreSQL integration tests."); ... - throw new InvalidOperationException("PostgreSQL is not available. Make sure PostgreSQL is running on localhost:5432"); + throw new InvalidOperationException( + "PostgreSQL is not available using MULTILOCK_POSTGRES_CONNECTION.");Also applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs` around lines 13 - 15, The connectionString static (connectionString) currently falls back to embedded credentials — remove that hardcoded fallback and require MULTILOCK_POSTGRES_CONNECTION to be set; update any test setup or failure messages that assume "localhost" (e.g., the localhost-specific failure text around the connection check) so they report the actual environment value or a generic missing/invalid connection message, and fail fast (throw or Assert) when the env var is null/empty to avoid silently binding to a developer DB.
🧹 Nitpick comments (7)
samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs (1)
34-35: Fire-and-forget task may swallow unhandled exceptions.Using
_ = MonitorStatusChangesAsync(stoppingToken)discards the task, meaning any unexpected exception (other thanOperationCanceledException) will be silently lost. For a sample this is acceptable, but consider logging or observing faulted tasks in production code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs` around lines 34 - 35, The fire-and-forget call to MonitorStatusChangesAsync(stoppingToken) can swallow exceptions; instead start/observe the task and ensure faults are logged or rethrown. Replace the discarded invocation by capturing the Task (e.g., var monitorTask = MonitorStatusChangesAsync(stoppingToken)) and either await it from Stop/Dispose or attach a continuation (monitorTask.ContinueWith(..., TaskContinuationOptions.OnlyOnFaulted)) or wrap the call in Task.Run with a try/catch that logs exceptions via your logger; reference MonitorStatusChangesAsync and stoppingToken to locate where to add capture/continuation/try-catch and ensure non-OperationCanceledException faults are logged.src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs (2)
341-349: Dispose pattern is correct but synchronous connection closure may block.
ConnectionMultiplexer.Dispose()can block while closing connections. For high-throughput scenarios, consider implementingIAsyncDisposablewithDisposeAsync()usingCloseAsync()for cleaner shutdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs` around lines 341 - 349, Implement IAsyncDisposable on RedisSemaphoreProvider and add a DisposeAsync() method that mirrors Dispose but performs an asynchronous shutdown: check isDisposed, set it, and if connectionMultiplexer.IsValueCreated call connectionMultiplexer.Value.CloseAsync() (awaiting it) instead of Dispose(), so closing the ConnectionMultiplexer does not block; keep the existing Dispose() as a synchronous fallback that calls connectionMultiplexer.Value.Dispose() when necessary, and ensure both methods reference the same isDisposed flag and connectionMultiplexer to avoid double-close.
176-195: Simplify JSON handling in UpdateHeartbeatAsync to avoid mixing serialization approaches.
UpdateHeartbeatAsyncis the only method that manipulates stored holder data through Lua'scjson, whileTryAcquireAsyncstores withSystem.Text.JsonandGetHoldersAsyncdeserializes withSystem.Text.Json. Though both libraries handle ISO 8601DateTimeOffsetformat compatibly, this mixing creates unnecessary complexity.Refactor
UpdateHeartbeatAsyncto either:
- Pass the complete serialized
HolderDataobject from C# (likeTryAcquireAsyncdoes), or- Keep the Lua-based approach but ensure all holder data is always encoded/decoded via the Lua script for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs` around lines 176 - 195, UpdateHeartbeatAsync currently decodes and re-encodes JSON inside the Lua script with cjson while TryAcquireAsync and GetHoldersAsync use System.Text.Json, causing mixed serialization; change UpdateHeartbeatAsync to serialize the full HolderData in C# (including updated Metadata and heartbeat timestamp) using System.Text.Json and pass that serialized string into the Lua script so the script only updates the ZSET score and HSET value without using cjson. Locate UpdateHeartbeatAsync and its script/args (holdersKey, dataKey, holderId, metadataJson, script) and modify the call so the ARG for the hash value is the complete serialized HolderData (mirror how TryAcquireAsync stores HolderData) and adjust the Lua script to skip cjson.decode/encode and directly HSET the provided value.tests/MultiLock.Tests/ServiceExtensionsValidationTests.cs (1)
236-279: Add a regression test for pre-registeredISemaphoreService.The new assertions only cover the default registration path, so the cast failure in
src/MultiLock/SemaphoreServiceExtensions.csstill slips through. A test that registers a non-SemaphoreServiceimplementation beforeAddSemaphore()and then resolves the hosted service would lock that down.tests/MultiLock.Tests/SemaphoreAcquisitionTests.cs (1)
82-93: Add a concurrent-dispose idempotency test.This test validates sequential double-dispose only. A parallel
DisposeAsyncrace test would better protect against duplicate release under concurrent callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.Tests/SemaphoreAcquisitionTests.cs` around lines 82 - 93, Add a concurrency test that ensures DisposeAsync is idempotent under parallel calls: create/acquire via provider.TryAcquireAsync(SemaphoreName, HolderId, maxCount, metadata, ...) and CreateAcquisition() as in DisposeAsync_CalledTwice_ShouldNotDoubleRelease, then start multiple concurrent tasks that call acquisition.DisposeAsync() (e.g., Task.WhenAll over several DisposeAsync calls) and assert no exceptions are thrown and the semaphore count/state remains correct; name the test e.g., DisposeAsync_ConcurrentCalls_ShouldNotDoubleRelease and reuse SemaphoreName, HolderId and provider to locate the related logic.tests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cs (1)
136-137: Clean up ad-hoc temp directories created in disposal tests.The per-test
ml-d-*directories created here are not deleted in test teardown. This can leave temp artifacts over repeated runs.Also applies to: 143-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cs` around lines 136 - 137, The test creates ad-hoc temp directories via CreateProvider(Path.Combine(Path.GetTempPath(), $"ml-d-{Guid.NewGuid():N}")) and never deletes them; update the test(s) around the p.Dispose() assertions (the ones calling CreateProvider and Should.NotThrow(() => p.Dispose())) to capture the generated directory path, call p.Dispose(), then remove the directory (e.g., Directory.Delete(generatedPath, true) or equivalent) inside a try/finally so cleanup always runs; alternatively create the provider with a tempDir variable and delete that tempDir in test teardown or immediately after disposing to ensure no ml-d-* artifacts remain.tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs (1)
245-249:HealthCheckAsync()is not initializing the table here.
src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.csLines 399-405 show thatHealthCheckAsync()only runsSELECT 1. Lines 248-249 therefore still leave the first concurrentTryAcquireAsync()calls racing throughCreateTableIfNotExistsAsync(), which makes this test less targeted and potentially flaky.🧪 Suggested change
- // Initialize the table by doing a health check - await mainProvider.HealthCheckAsync(); + // Force table creation before starting the acquisition race. + (await mainProvider.TryAcquireAsync( + semaphoreName, + "warmup-holder", + maxCount, + metadata, + TimeSpan.FromMinutes(5))).ShouldBeTrue(); + await mainProvider.ReleaseAsync(semaphoreName, "warmup-holder");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs` around lines 245 - 249, HealthCheckAsync() on PostgreSqlSemaphoreProvider only runs SELECT 1 and doesn't create the table, so the test still races on the first concurrent TryAcquireAsync() which calls CreateTableIfNotExistsAsync(); update the test to ensure the table exists before starting concurrent callers by invoking the provider's table-creation routine (call CreateTableIfNotExistsAsync() on the PostgreSqlSemaphoreProvider instance or, if that method is non-public, add and call a public Initialize/EnsureTableCreatedAsync() wrapper on PostgreSqlSemaphoreProvider) so the table is created deterministically before running the concurrent TryAcquireAsync() calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs`:
- Around line 62-117: The code currently treats ETag conflicts (412 Precondition
Failed) as normal false outcomes causing caller methods like TryAcquireAsync,
ReleaseAsync, and UpdateHeartbeatAsync to report "full"/"not holding" instead of
retrying; update the logic so that when TryWriteSemaphoreDataAsync detects an
ETag conflict (or when Azure Blob throws a RequestFailedException with Status ==
412) it does not swallow it but surfaces it as an exception (or rethrow a
RequestFailedException) so SemaphoreService.ExecuteWithRetryAsync can retry;
specifically, change TryWriteSemaphoreDataAsync to throw or propagate the 412
RequestFailedException instead of returning false on conflict, and update
TryAcquireAsync, ReleaseAsync, and UpdateHeartbeatAsync to treat write-conflict
outcomes as exceptions to propagate rather than as normal false/log-only returns
(preserving the current logging for non-conflict failures).
- Around line 119-123: The catch blocks that currently log and rethrow all
exceptions (e.g., the catch surrounding logger.LogError(...) and throw new
SemaphoreProviderException(...)) are wrapping OperationCanceledException and
breaking cancellation semantics; add a dedicated catch
(OperationCanceledException) { throw; } immediately before each existing catch
(Exception) to let cancellation propagate unchanged. Update each of the six
locations in AzureBlobStorageSemaphoreProvider (the catch blocks around the blob
SDK calls such as the AcquireSlotAsync/ReleaseSlotAsync/related methods where
logger.LogError is called and SemaphoreProviderException is thrown) by inserting
a catch for OperationCanceledException that rethrows, then keep the existing
catch (Exception) for other errors. Ensure the new catch appears before the
general Exception handler to preserve behavior for non-cancellation errors.
In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs`:
- Around line 165-181: Currently RemoveSession(semaphoreName, holderId) deletes
the local mapping before calling consulClient.Session.Destroy(sessionId); change
the flow so you retrieve the session id without removing it first (e.g., add or
use a GetSessionId/PeekSession method or a non-destructive lookup instead of
RemoveSession) then call await consulClient.Session.Destroy(sessionId,
cancellationToken); only after Destroy completes successfully remove the local
mapping (call RemoveSession) and log the info; if Destroy throws, leave the
local mapping intact so DisposeAsync() or retries can still find and clean up
the session, and still log and wrap the exception as before.
- Around line 227-242: GetCurrentCountAsync currently ignores the slotTimeout
and thus can return counts inconsistent with TryAcquireAsync; update
GetCurrentCountAsync to validate that the provided slotTimeout matches the
configured ConsulSemaphoreOptions.SessionTtl (or is within an acceptable
tolerance) and throw an ArgumentException if it doesn’t, and then continue to
use GetSemaphorePrefix and consulClient.KV.List as before; reference the
ConsulSemaphoreOptions.SessionTtl setting when validating slotTimeout so
capacity decisions stay consistent with TryAcquireAsync.
In `@src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs`:
- Around line 69-70: CountHolders is counting raw filenames while the acquire
path only removes/readable holders via CleanupExpiredHolders, so partially
written or corrupt JSON files still consume capacity; update CountHolders (and
the other counting usages noted around the Acquire path and the ranges
referenced) to only count holders that can be successfully read/parsed with the
same validation as CleanupExpiredHolders (reuse the same read/parse/IsReadable
logic or call the same helper used by CleanupExpiredHolders) so corrupt or
unreadable files are ignored when computing capacity.
- Around line 150-165: ReleaseAsync and UpdateHeartbeatAsync currently only use
the in-process monitor (lock(fileLock)) when mutating holder files, allowing
cross-process races; wrap their holder-file reads/writes with the same
cross-process ".lock" file protection used by TryAcquireAsync. Specifically,
before touching holder files in ReleaseAsync and UpdateHeartbeatAsync, acquire
the semaphore's file lock (the same ".lock" lock file used by TryAcquireAsync),
perform the existence/delete/update logic while holding that file lock, then
release it; keep any existing in-process lock(fileLock) as a secondary guard if
present. Ensure both methods use the identical lock acquisition/release helper
so the create/cleanup/count/heartbeat paths are serialized across processes.
- Around line 111-115: The generic catch(Exception) blocks in TryAcquireAsync,
ReleaseAsync, UpdateHeartbeatAsync, GetCurrentCountAsync, GetHoldersAsync, and
IsHoldingAsync are currently wrapping OperationCanceledException into
SemaphoreProviderException; add a dedicated catch(OperationCanceledException)
immediately before each generic catch to rethrow the cancellation unchanged
(preserve the original exception), so awaits from
EnsureInitializedAsync(cancellationToken) and other awaited calls can propagate
cancellation correctly instead of being wrapped.
In `@src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs`:
- Around line 225-231: Dispose() currently clears the semaphores dictionary
without taking the same synchronization lock used elsewhere, risking races;
modify Dispose() to acquire the same lock (the `@lock` used for all accesses to
the semaphores Dictionary) before checking/setting isDisposed and calling
semaphores.Clear(), i.e. perform the isDisposed check, set isDisposed = true and
semaphores.Clear() while holding `@lock` so Dispose(),
Acquire/Release/Heartbeat/Count are properly serialized.
In `@src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs`:
- Around line 170-175: The catch-all handler that catches Exception (e.g.,
"catch (Exception ex) when (ex is not SemaphoreException)") is currently
wrapping OperationCanceledException and ObjectDisposedException into
SemaphoreProviderException; update each such block in
PostgreSqlSemaphoreProvider so you first add explicit catches for
OperationCanceledException and ObjectDisposedException that simply rethrow
(preserve the original exception), then keep the existing catch that logs via
logger.LogError(...) and throws new SemaphoreProviderException("Failed to
acquire semaphore slot", ex); apply this change to all occurrences of the
generic catch (the blocks referencing logger, holderId, semaphoreName and
throwing SemaphoreProviderException) so cancellation and disposal exceptions
propagate unchanged.
- Around line 57-60: The code currently uses DateTimeOffset.UtcNow and
caller-side expiry calculation (e.g., DateTimeOffset now =
DateTimeOffset.UtcNow; DateTimeOffset expiryTime = now.Subtract(slotTimeout);
metadataJson = JsonSerializer.Serialize(metadata)) to set last_heartbeat and
expiry; instead use PostgreSQL server time and compute expiry in SQL so
timestamps are based on the DB clock: update the SQL statements in
UpdateHeartbeatAsync and GetCurrentCountAsync to use now() or CURRENT_TIMESTAMP
for last_heartbeat and compute expiry using now() - slotTimeout (or INTERVAL)
within the query, remove reliance on the local DateTimeOffset now/expiryTime
values, and keep metadataJson for parameterized metadata only; apply the same
change to the other occurrences mentioned (lines ~96-99, 142-145, 238-243,
285-289) so all last_heartbeat and expiry logic is DB-server-time-based.
In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs`:
- Around line 296-320: IsHoldingAsync currently returns true for holders whose
heartbeat score has expired; change it to accept a slotTimeout (update
ISemaphoreProvider.IsHoldingAsync signature) and compute the cutoff =
CurrentUnixTimeSeconds - slotTimeout.TotalSeconds, then use
GetDatabase().SortedSetScoreAsync(GetHoldersKey(semaphoreName), holderId) and
return true only if score.HasValue && score.Value >= cutoff (mirroring
GetCurrentCountAsync's filtering); ensure ThrowIfDisposed, ParameterValidation,
logging (logger.LogError) and exception wrapping remain, and update any
callers/tests that rely on the old signature (also consider adding an
overload/backwards-compat shim if needed).
In `@src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs`:
- Around line 186-190: The catch-all Exception handlers in
SqlServerSemaphoreProvider.cs (inside methods that log via logger.LogError then
throw new SemaphoreProviderException) are intercepting
OperationCanceledException and breaking cancellation propagation; add a specific
catch (OperationCanceledException) { throw; } immediately before each existing
catch (Exception ex) so cancellation exceptions propagate unchanged (do this for
the catch sites shown around the logger.LogError/throw new
SemaphoreProviderException blocks).
- Around line 464-483: The CREATE TABLE/CREATE INDEX batch is racy and can leave
the index uncreated on partial failures; update the SQL in
SqlServerSemaphoreProvider (the sql string built around
options.TableName/options.SchemaName) to wrap the CREATE TABLE statement in a
TRY...CATCH that swallows “already exists” errors (duplicate object) so
concurrent CREATE TABLE races are harmless, and move the index creation into a
separate idempotent statement that checks for the index's existence (IF NOT
EXISTS for IX_{...}_SemaphoreName_LastHeartbeat) before creating it; keep using
the same SqlCommand/ExecuteNonQueryAsync flow (command.CommandTimeout and
ExecuteNonQueryAsync) but execute the safer SQL batch(s) instead.
In `@tests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cs`:
- Around line 71-72: The test setup calls to TryAcquireAsync are currently
awaited but not asserted; change each setup invocation (the calls to
provider.TryAcquireAsync(...) in FileSystemSemaphoreIntegrationTests — e.g., the
pairs at the locations shown) to assert that acquisition succeeded by capturing
the boolean result and calling Assert.True(result) (or use Assert.True(await
provider.TryAcquireAsync(...))) so failed preconditions are surfaced; apply this
change to the occurrences around the existing calls (including the ones
referenced at the other locations).
- Around line 33-153: Add an integration test that verifies cross-process
exclusivity by spawning a separate OS process which uses
FileSystemSemaphoreProvider pointing at the same backing directory and attempts
to acquire slots; specifically, create a new test (e.g.,
CrossProcess_MaxCountEnforced) which starts a helper process that calls
TryAcquireAsync("test-semaphore", "remote-holder", maxCount, ...) and then in
the test process acquire up to maxCount slots via FileSystemSemaphoreProvider
(use the same CreateOptions() so both processes share the same directory),
assert the helper fails when max is reached (or the main test fails if helper
holds all slots), and ensure cleanup by signaling the helper to release or
exiting it; reference FileSystemSemaphoreProvider, TryAcquireAsync, ReleaseAsync
and GetCurrentCountAsync to locate where to wire the interprocess scenario and
reuse existing timeout values (TimeSpan.FromMinutes(5)).
In `@tests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cs`:
- Around line 46-47: The setup calls to provider.TryAcquireAsync are currently
unasserted and may silently fail, so update each precondition acquisition (the
initial provider.TryAcquireAsync calls that precede the actual Assert checks) to
assert success (e.g., Assert.True or equivalent) instead of just awaiting them;
specifically, assert the boolean result from provider.TryAcquireAsync for the
initial owner/acquire attempts (the calls that prepare the semaphore state
before checking the test case result) so failures in setup cause test
failures—apply this to the setup calls around the existing result checks and the
other similar occurrences in this test class.
In `@tests/MultiLock.Tests/SemaphoreAcquisitionTests.cs`:
- Line 72: The setup calls to TryAcquireAsync in the disposal tests ignore the
result; change both setup lines that call TryAcquireAsync(SemaphoreName,
HolderId, 3, metadata, TimeSpan.FromMinutes(5)) to capture the boolean result
and assert it succeeded (e.g., var acquired = await
provider.TryAcquireAsync(...); Assert.True(acquired);) so the test fails if the
initial acquisition did not succeed and the disposal test exercises the intended
path.
---
Duplicate comments:
In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs`:
- Around line 47-60: The acquired slot must always be released even if
SimulateApiCallAsync throws, and ReleaseAsync must not be cancellable by
stoppingToken; move the await semaphoreService.ReleaseAsync(...) into a finally
block that runs only when acquired is true, and call ReleaseAsync without
passing stoppingToken so the release completes during shutdown; update the
try/await SimulateApiCallAsync(requestId, stoppingToken) block around the
simulated work and ensure the logger.LogInformation("[Request {RequestId}]
Released slot", requestId) runs after a successful release in that finally.
In `@src/MultiLock/SemaphoreAcquisition.cs`:
- Around line 77-83: The synchronous Dispose implementation still blocks on
async I/O and can deadlock; replace the blocking call in Dispose with a
non-blocking failure: have Dispose throw NotSupportedException (or
InvalidOperationException) instructing callers to use DisposeAsync instead, and
remove the GetAwaiter().GetResult() call so no synchronous wrapper awaits
ReleaseAsync; reference the SemaphoreAcquisition.Dispose method and the existing
DisposeAsync / ReleaseAsync flow so reviewers can verify callers use await using
or call DisposeAsync when the provider performs network/database I/O.
- Line 10: The dispose guard on the SemaphoreAcquisition class is racy: the
volatile isDisposed check allows two concurrent disposals to both call
provider.ReleaseAsync; change the guard to an atomic compare/exchange (e.g.,
replace the volatile bool isDisposed with an integer flag and use
Interlocked.Exchange/CompareExchange) inside DisposeAsync/Dispose so only the
first caller proceeds to call provider.ReleaseAsync and subsequent calls return
immediately; keep the same provider.ReleaseAsync call and semantics but gate it
behind the atomic transition of the flag.
In `@src/MultiLock/SemaphoreServiceExtensions.cs`:
- Around line 27-29: The current factory casts
provider.GetRequiredService<ISemaphoreService>() to SemaphoreService which
throws if a custom ISemaphoreService was pre-registered; change the
AddHostedService registration to resolve ISemaphoreService and (a) if the
resolved instance already implements IHostedService return it, otherwise (b)
wrap the resolved ISemaphoreService in a small adapter that implements
IHostedService and delegates StartAsync/StopAsync to the semaphore service
methods. Update the registration in AddHostedService to use that resolver (and
add the adapter type if needed) instead of casting to SemaphoreService; keep
TryAddSingleton<ISemaphoreService, SemaphoreService>() unchanged.
In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs`:
- Around line 13-15: The connectionString static (connectionString) currently
falls back to embedded credentials — remove that hardcoded fallback and require
MULTILOCK_POSTGRES_CONNECTION to be set; update any test setup or failure
messages that assume "localhost" (e.g., the localhost-specific failure text
around the connection check) so they report the actual environment value or a
generic missing/invalid connection message, and fail fast (throw or Assert) when
the env var is null/empty to avoid silently binding to a developer DB.
---
Nitpick comments:
In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs`:
- Around line 34-35: The fire-and-forget call to
MonitorStatusChangesAsync(stoppingToken) can swallow exceptions; instead
start/observe the task and ensure faults are logged or rethrown. Replace the
discarded invocation by capturing the Task (e.g., var monitorTask =
MonitorStatusChangesAsync(stoppingToken)) and either await it from Stop/Dispose
or attach a continuation (monitorTask.ContinueWith(...,
TaskContinuationOptions.OnlyOnFaulted)) or wrap the call in Task.Run with a
try/catch that logs exceptions via your logger; reference
MonitorStatusChangesAsync and stoppingToken to locate where to add
capture/continuation/try-catch and ensure non-OperationCanceledException faults
are logged.
In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs`:
- Around line 341-349: Implement IAsyncDisposable on RedisSemaphoreProvider and
add a DisposeAsync() method that mirrors Dispose but performs an asynchronous
shutdown: check isDisposed, set it, and if connectionMultiplexer.IsValueCreated
call connectionMultiplexer.Value.CloseAsync() (awaiting it) instead of
Dispose(), so closing the ConnectionMultiplexer does not block; keep the
existing Dispose() as a synchronous fallback that calls
connectionMultiplexer.Value.Dispose() when necessary, and ensure both methods
reference the same isDisposed flag and connectionMultiplexer to avoid
double-close.
- Around line 176-195: UpdateHeartbeatAsync currently decodes and re-encodes
JSON inside the Lua script with cjson while TryAcquireAsync and GetHoldersAsync
use System.Text.Json, causing mixed serialization; change UpdateHeartbeatAsync
to serialize the full HolderData in C# (including updated Metadata and heartbeat
timestamp) using System.Text.Json and pass that serialized string into the Lua
script so the script only updates the ZSET score and HSET value without using
cjson. Locate UpdateHeartbeatAsync and its script/args (holdersKey, dataKey,
holderId, metadataJson, script) and modify the call so the ARG for the hash
value is the complete serialized HolderData (mirror how TryAcquireAsync stores
HolderData) and adjust the Lua script to skip cjson.decode/encode and directly
HSET the provided value.
In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs`:
- Around line 245-249: HealthCheckAsync() on PostgreSqlSemaphoreProvider only
runs SELECT 1 and doesn't create the table, so the test still races on the first
concurrent TryAcquireAsync() which calls CreateTableIfNotExistsAsync(); update
the test to ensure the table exists before starting concurrent callers by
invoking the provider's table-creation routine (call
CreateTableIfNotExistsAsync() on the PostgreSqlSemaphoreProvider instance or, if
that method is non-public, add and call a public
Initialize/EnsureTableCreatedAsync() wrapper on PostgreSqlSemaphoreProvider) so
the table is created deterministically before running the concurrent
TryAcquireAsync() calls.
In `@tests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cs`:
- Around line 136-137: The test creates ad-hoc temp directories via
CreateProvider(Path.Combine(Path.GetTempPath(), $"ml-d-{Guid.NewGuid():N}")) and
never deletes them; update the test(s) around the p.Dispose() assertions (the
ones calling CreateProvider and Should.NotThrow(() => p.Dispose())) to capture
the generated directory path, call p.Dispose(), then remove the directory (e.g.,
Directory.Delete(generatedPath, true) or equivalent) inside a try/finally so
cleanup always runs; alternatively create the provider with a tempDir variable
and delete that tempDir in test teardown or immediately after disposing to
ensure no ml-d-* artifacts remain.
In `@tests/MultiLock.Tests/SemaphoreAcquisitionTests.cs`:
- Around line 82-93: Add a concurrency test that ensures DisposeAsync is
idempotent under parallel calls: create/acquire via
provider.TryAcquireAsync(SemaphoreName, HolderId, maxCount, metadata, ...) and
CreateAcquisition() as in DisposeAsync_CalledTwice_ShouldNotDoubleRelease, then
start multiple concurrent tasks that call acquisition.DisposeAsync() (e.g.,
Task.WhenAll over several DisposeAsync calls) and assert no exceptions are
thrown and the semaphore count/state remains correct; name the test e.g.,
DisposeAsync_ConcurrentCalls_ShouldNotDoubleRelease and reuse SemaphoreName,
HolderId and provider to locate the related logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbe0268a-db5a-4346-af1b-e8de8657fd00
📒 Files selected for processing (77)
MultiLock.slnxREADME.mdsamples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csprojsamples/MultiLock.SemaphoreSample/Program.cssamples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cssrc/MultiLock/Exceptions/SemaphoreException.cssrc/MultiLock/Exceptions/SemaphoreFullException.cssrc/MultiLock/Exceptions/SemaphoreProviderException.cssrc/MultiLock/Exceptions/SemaphoreTimeoutException.cssrc/MultiLock/ISemaphoreProvider.cssrc/MultiLock/ISemaphoreService.cssrc/MultiLock/MultiLock.csprojsrc/MultiLock/ParameterValidation.cssrc/MultiLock/SemaphoreAcquisition.cssrc/MultiLock/SemaphoreChangedEventArgs.cssrc/MultiLock/SemaphoreHolder.cssrc/MultiLock/SemaphoreInfo.cssrc/MultiLock/SemaphoreOptions.cssrc/MultiLock/SemaphoreService.cssrc/MultiLock/SemaphoreServiceExtensions.cssrc/MultiLock/SemaphoreStatus.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageServiceCollectionExtensions.cssrc/Providers/MultiLock.AzureBlobStorage/MultiLock.AzureBlobStorage.csprojsrc/Providers/MultiLock.Consul/ConsulLeaderElectionOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cssrc/Providers/MultiLock.Consul/ConsulServiceCollectionExtensions.cssrc/Providers/MultiLock.Consul/MultiLock.Consul.csprojsrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cssrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cssrc/Providers/MultiLock.FileSystem/FileSystemServiceCollectionExtensions.cssrc/Providers/MultiLock.FileSystem/HolderFileContent.cssrc/Providers/MultiLock.FileSystem/MultiLock.FileSystem.csprojsrc/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cssrc/Providers/MultiLock.InMemory/InMemoryServiceCollectionExtensions.cssrc/Providers/MultiLock.InMemory/MultiLock.InMemory.csprojsrc/Providers/MultiLock.InMemory/SemaphoreSlotRecord.cssrc/Providers/MultiLock.PostgreSQL/MultiLock.PostgreSQL.csprojsrc/Providers/MultiLock.PostgreSQL/PostgreSQLServiceCollectionExtensions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cssrc/Providers/MultiLock.Redis/MultiLock.Redis.csprojsrc/Providers/MultiLock.Redis/RedisSemaphoreOptions.cssrc/Providers/MultiLock.Redis/RedisSemaphoreProvider.cssrc/Providers/MultiLock.Redis/RedisServiceCollectionExtensions.cssrc/Providers/MultiLock.SqlServer/MultiLock.SqlServer.csprojsrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreOptions.cssrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cssrc/Providers/MultiLock.SqlServer/SqlServerServiceCollectionExtensions.cssrc/Providers/MultiLock.ZooKeeper/MultiLock.ZooKeeper.csprojsrc/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperServiceCollectionExtensions.cstests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cstests/MultiLock.Tests/FileSystemSemaphoreOptionsTests.cstests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cstests/MultiLock.Tests/FileSystemServiceCollectionExtensionsTests.cstests/MultiLock.Tests/InMemorySemaphoreProviderTests.cstests/MultiLock.Tests/InMemoryServiceCollectionExtensionsTests.cstests/MultiLock.Tests/SemaphoreAcquisitionTests.cstests/MultiLock.Tests/SemaphoreConcurrencyTests.cstests/MultiLock.Tests/SemaphoreExceptionTests.cstests/MultiLock.Tests/SemaphoreModelTests.cstests/MultiLock.Tests/SemaphoreOptionsValidationTests.cstests/MultiLock.Tests/SemaphoreParameterValidationTests.cstests/MultiLock.Tests/SemaphoreServiceTests.cstests/MultiLock.Tests/ServiceExtensionsValidationTests.cs
✅ Files skipped from review due to trivial changes (34)
- src/Providers/MultiLock.Consul/MultiLock.Consul.csproj
- src/MultiLock/Exceptions/SemaphoreProviderException.cs
- src/Providers/MultiLock.SqlServer/MultiLock.SqlServer.csproj
- src/Providers/MultiLock.InMemory/SemaphoreSlotRecord.cs
- src/MultiLock/MultiLock.csproj
- src/Providers/MultiLock.Consul/ConsulLeaderElectionOptions.cs
- samples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csproj
- src/Providers/MultiLock.InMemory/MultiLock.InMemory.csproj
- src/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionOptions.cs
- src/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionProvider.cs
- src/MultiLock/SemaphoreHolder.cs
- src/Providers/MultiLock.PostgreSQL/MultiLock.PostgreSQL.csproj
- src/Providers/MultiLock.FileSystem/HolderFileContent.cs
- src/Providers/MultiLock.AzureBlobStorage/MultiLock.AzureBlobStorage.csproj
- src/Providers/MultiLock.Redis/MultiLock.Redis.csproj
- src/MultiLock/Exceptions/SemaphoreFullException.cs
- src/Providers/MultiLock.ZooKeeper/MultiLock.ZooKeeper.csproj
- src/MultiLock/SemaphoreChangedEventArgs.cs
- src/MultiLock/Exceptions/SemaphoreTimeoutException.cs
- src/Providers/MultiLock.Redis/RedisSemaphoreOptions.cs
- src/Providers/MultiLock.Consul/ConsulSemaphoreOptions.cs
- src/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cs
- src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cs
- src/MultiLock/ISemaphoreService.cs
- src/MultiLock/Exceptions/SemaphoreException.cs
- src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cs
- tests/MultiLock.Tests/SemaphoreParameterValidationTests.cs
- src/Providers/MultiLock.SqlServer/SqlServerSemaphoreOptions.cs
- src/MultiLock/SemaphoreInfo.cs
- src/MultiLock/SemaphoreStatus.cs
- MultiLock.slnx
- src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cs
- src/Providers/MultiLock.ZooKeeper/ZooKeeperServiceCollectionExtensions.cs
- src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs
🚧 Files skipped from review as they are similar to previous changes (15)
- src/Providers/MultiLock.FileSystem/MultiLock.FileSystem.csproj
- src/Providers/MultiLock.InMemory/InMemoryServiceCollectionExtensions.cs
- src/Providers/MultiLock.FileSystem/FileSystemServiceCollectionExtensions.cs
- src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageServiceCollectionExtensions.cs
- src/Providers/MultiLock.Redis/RedisServiceCollectionExtensions.cs
- src/Providers/MultiLock.Consul/ConsulServiceCollectionExtensions.cs
- src/MultiLock/ISemaphoreProvider.cs
- src/MultiLock/SemaphoreOptions.cs
- samples/MultiLock.SemaphoreSample/Program.cs
- src/MultiLock/ParameterValidation.cs
- tests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cs
- tests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cs
- README.md
- tests/MultiLock.Tests/SemaphoreServiceTests.cs
- src/MultiLock/SemaphoreService.cs
src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs
Show resolved
Hide resolved
src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs
Show resolved
Hide resolved
steingran
force-pushed
the
feat/semaphore
branch
from
12c8d95 to
4453050
Compare
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (12)
tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs (1)
74-75:⚠️ Potential issue | 🟠 MajorStill assert every state-establishing acquire.
These
TryAcquireAsynccalls still ignore the returned boolean. If an acquire fails and the queried method regresses in the same direction, the test can pass without ever creating the intended semaphore state.Also applies to: 92-92, 112-112, 130-131, 150-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs` around lines 74 - 75, The test currently ignores the boolean result of TryAcquireAsync calls (e.g., the calls to TryAcquireAsync("test-semaphore", "holder-1", ...), TryAcquireAsync("test-semaphore", "holder-2", ...) and the other similar calls flagged) which can hide failed acquires; update each state-establishing call to assert the returned value is true (e.g., Assert.True(await provider.TryAcquireAsync(...), "Acquire should succeed")) so the test fails immediately if an expected acquire did not succeed; apply this change to every TryAcquireAsync invocation mentioned (including the duplicates at the other locations).tests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cs (1)
74-75:⚠️ Potential issue | 🟠 MajorStill assert every state-establishing acquire.
These
TryAcquireAsynccalls still ignore the returned boolean. If an acquire fails and the queried method regresses in the same direction, the test can pass without ever creating the intended semaphore state.Also applies to: 92-92, 112-112, 130-131, 150-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cs` around lines 74 - 75, The test calls to provider.TryAcquireAsync ("TryAcquireAsync") are ignoring the returned boolean, so failing acquires can silently leave the semaphore state incorrect; update each occurrence (e.g., the calls at lines shown and duplicates) to capture the result and assert it is true (use the test framework's assert, e.g., Assert.True or equivalent) immediately after each TryAcquireAsync call to ensure the acquire actually succeeded before proceeding with subsequent assertions.src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs (1)
225-231:⚠️ Potential issue | 🔴 CriticalSerialize
Dispose()with the same lock as the dictionary.
Dispose()clearssemaphoresoutside@lock, while every other access to thatDictionaryis synchronized. A concurrent acquire/release/heartbeat/count can race with shutdown and observe corrupted in-memory state.Suggested fix
public void Dispose() { - if (isDisposed) return; - isDisposed = true; - semaphores.Clear(); + lock (`@lock`) + { + if (isDisposed) return; + isDisposed = true; + semaphores.Clear(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs` around lines 225 - 231, The Dispose() method must be serialized with the same synchronization used for the semaphores dictionary; change Dispose() so the isDisposed check, setting isDisposed = true, and semaphores.Clear() all execute while holding the same lock object (the `@lock` used elsewhere in InMemorySemaphoreProvider), e.g., enter the monitor/lock around those operations to prevent races with Acquire/Release/Heartbeat/Count methods that access semaphores concurrently.src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs (1)
456-460:⚠️ Potential issue | 🟠 MajorDo not treat unreadable holder files as live capacity.
CleanupExpiredHolders()only removes files it can deserialize, butCountHolders()counts raw filenames. A partial/corrupt holder file can therefore consume a slot forever and make the semaphore look full even though no readable holder exists.Suggested fix
private int CountHolders(string semaphoreDir) { if (!Directory.Exists(semaphoreDir)) return 0; - return Directory.GetFiles(semaphoreDir, $"*{options.FileExtension}").Length; + return Directory.GetFiles(semaphoreDir, $"*{options.FileExtension}") + .Select(ReadHolderFile) + .Count(holder => holder != null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs` around lines 456 - 460, CountHolders currently counts filenames and can treat corrupt/unreadable holder files as live holders; change CountHolders to iterate files in semaphoreDir matching options.FileExtension, attempt to open and deserialize each file using the same deserialization logic as CleanupExpiredHolders (or call the same helper used there), and only increment the count for files that successfully deserialize and are not expired; skip (and optionally log) files that fail to read/deserialize so corrupt files don’t occupy capacity. Ensure you reference semaphoreDir, options.FileExtension, and the existing CleanupExpiredHolders deserialization routine to share behavior.src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs (2)
95-149:⚠️ Potential issue | 🟠 MajorDestroy the new session on exception paths before
AddSession().Once
CreateSessionAsync()succeeds, any exception before the local mapping is added exits through the outer catch without cleaning upsessionId. Because that ID is not yet inactiveSessions, disposal cannot recover it either, and a successfully acquired KV slot can stay occupied until the Consul TTL expires.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs` around lines 95 - 149, After CreateSessionAsync succeeds we must ensure the new sessionId is destroyed on any exception paths before AddSession() is called; modify the Acquire logic in ConsulSemaphoreProvider (methods CreateSessionAsync, AddSession usage) to track whether sessionId was created but not yet added and call consulClient.Session.Destroy(sessionId, cancellationToken) on all exception/early-return paths (including the outer catch or a local try/finally) so sessions aren’t leaked when an exception occurs before AddSession().
232-246:⚠️ Potential issue | 🟠 MajorDo not silently ignore
slotTimeoutin the live-count path.
TryAcquireAsync()uses this method for both the fast-fail check and the post-acquisition rollback. Counting raw KV entries here means admission decisions followoptions.SessionTtl, not the caller’sslotTimeout, and a config mismatch changes capacity behavior without any validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs` around lines 232 - 246, GetCurrentCountAsync currently ignores the slotTimeout parameter; add explicit validation to prevent silent mismatch with the Consul session TTL by checking slotTimeout against the provider's configured session TTL (options.SessionTtl) at the start of GetCurrentCountAsync (the same place TryAcquireAsync calls it), and if they differ in a way that would change admission behavior throw an ArgumentException or ArgumentOutOfRangeException; keep the existing KV listing via consulClient.KV.List(GetSemaphorePrefix(semaphoreName)) only for the case where slotTimeout equals options.SessionTtl, and reference GetCurrentCountAsync, TryAcquireAsync, options.SessionTtl, GetSemaphorePrefix, and consulClient.KV.List when making the change.src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs (1)
412-433:⚠️ Potential issue | 🟠 MajorBubble write conflicts instead of flattening them into
false.Returning
falsehere makes repeated optimistic-concurrency collisions look like normal semaphore outcomes after the caller exhausts its local loop, so higher layers can misreport transient contention as “full”, “not holding”, or a completed release instead of retrying it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs` around lines 412 - 433, The TryWriteSemaphoreDataAsync method currently swallows ETag precondition failures by returning false; instead surface those write-conflict errors so callers can retry or handle them explicitly: in the catch (RequestFailedException ex) when ex.Status == 412 (inside TryWriteSemaphoreDataAsync) do not return false—rethrow the RequestFailedException (or wrap and throw a specific Concurrency/OptimisticLock exception) so higher layers detect an optimistic-concurrency collision; keep the rest of the method behavior the same and ensure callers of TryWriteSemaphoreDataAsync handle this exception path rather than interpreting a boolean false as a normal semaphore outcome.src/MultiLock/SemaphoreService.cs (1)
411-457:⚠️ Potential issue | 🔴 CriticalThrow on retry exhaustion instead of returning
default.Returning
false/nullhere makes provider failures indistinguishable from normal semaphore states.TryAcquireInternalAsync()reports backend errors as “full”,ReleaseAsync()moves local state to waiting after a failed remote release, and heartbeat outages look like a lost slot.Suggested fix
- // Return default value on failure (false for bool, null for reference types) - return default!; + throw lastException ?? new InvalidOperationException( + $"Failed to {operationName} for holder {HolderId} after {attempt} attempts.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreService.cs` around lines 411 - 457, ExecuteWithRetryAsync currently returns default on retry exhaustion which hides provider failures; instead, when attempt > options.MaxRetryAttempts, throw the final exception (or wrap it with context) so callers like TryAcquireInternalAsync and ReleaseAsync can distinguish backend errors from normal semaphore states. Locate ExecuteWithRetryAsync, use the captured lastException (or create a new Exception that includes operationName, HolderId and attempts) and throw it after logging; preserve the existing logging calls and remove the final "return default!" so failures propagate to callers.src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs (1)
464-483:⚠️ Potential issue | 🟠 MajorMake the startup DDL idempotent under concurrent initializers.
This batch is still racy across multiple nodes: two initializers can both pass the existence check before either
CREATE TABLEruns. It also couples index creation to table creation, so an index failure after the table succeeds is never repaired because later runs skip the whole block once the table exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs` around lines 464 - 483, The DDL block in SqlServerSemaphoreProvider that builds the `sql` string is racy and couples index creation to table creation; change it so table/index/constraint creation are idempotent under concurrent initializers by (1) executing CREATE TABLE in a TRY/CATCH that ignores the "already exists" error (2714) instead of relying on a separate IF NOT EXISTS check, and (2) create the primary key and the index in separate statements that only run when they don't exist (e.g., check sys.key_constraints/sys.indexes or use TRY/CATCH to ignore "already exists" errors). Update the code that builds `sql` (the block referencing options.SchemaName and options.TableName and the PK/IX names) to perform these separate safe operations, then execute the resulting batch via the existing SqlCommand/ExecuteNonQueryAsync call so concurrent initializers can't race to leave the schema inconsistent.src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs (1)
95-102:⚠️ Potential issue | 🟠 MajorTreat
IndexOf(...) == -1as a failed acquisition.If the refreshed child list does not contain
createdNodeName,positionbecomes-1and the current condition reports success. Handleposition < 0the same as over-capacity and delete the created znode.🐛 Proposed fix
- if (position >= maxCount) + if (position < 0 || position >= maxCount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs` around lines 95 - 102, The code in ZooKeeperSemaphoreProvider.cs should treat position < 0 the same as position >= maxCount: after refreshing children and computing createdNodeName and position, check if position < 0 || position >= maxCount and in that case delete the created znode (use the existing delete call pattern, e.g., zooKeeper.deleteAsync(createdPath) or the provider's removal method) and return/fail the acquisition; update the if condition that currently checks only position >= maxCount to use position < 0 || position >= maxCount so missing nodes are handled as failed acquisitions.src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs (2)
57-60:⚠️ Potential issue | 🟠 MajorUse PostgreSQL server time for lease timestamps and expiry.
These paths still derive
last_heartbeatand expiry fromDateTimeOffset.UtcNow, so clock skew between application nodes can expire a live holder early or keep a dead holder counted longer. UseCURRENT_TIMESTAMPin the writes and compute the expiry window in SQL fromslotTimeoutinstead of a caller-side timestamp.🕒 Suggested direction
- DateTimeOffset now = DateTimeOffset.UtcNow; string metadataJson = JsonSerializer.Serialize(metadata); - DateTimeOffset expiryTime = now.Subtract(slotTimeout); string cleanupSql = $@" DELETE FROM {options.SchemaName}.{options.TableName} - WHERE semaphore_name = `@SemaphoreName` AND last_heartbeat < `@ExpiryTime`"; + WHERE semaphore_name = `@SemaphoreName` + AND last_heartbeat < (CURRENT_TIMESTAMP - `@SlotTimeout`)"; await using (var cleanupCommand = new NpgsqlCommand(cleanupSql, connection, transaction)) { cleanupCommand.CommandTimeout = options.CommandTimeoutSeconds; cleanupCommand.Parameters.AddWithValue("@SemaphoreName", semaphoreName); - cleanupCommand.Parameters.AddWithValue("@ExpiryTime", expiryTime); + cleanupCommand.Parameters.AddWithValue("@SlotTimeout", slotTimeout); await cleanupCommand.ExecuteNonQueryAsync(cancellationToken); } string checkExistingSql = $@" UPDATE {options.SchemaName}.{options.TableName} - SET last_heartbeat = `@Now`, metadata = `@Metadata`::jsonb + SET last_heartbeat = CURRENT_TIMESTAMP, metadata = `@Metadata`::jsonb WHERE semaphore_name = `@SemaphoreName` AND holder_id = `@HolderId` RETURNING 1"; ... - VALUES (`@SemaphoreName`, `@HolderId`, `@Now`, `@Now`, `@Metadata`::jsonb)"; + VALUES (`@SemaphoreName`, `@HolderId`, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP, `@Metadata`::jsonb)";Also applies to: 83-92, 96-108, 142-153, 238-251, 285-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs` around lines 57 - 60, The code currently uses DateTimeOffset.UtcNow to set last_heartbeat and compute expiryTime (e.g., the snippet that creates metadataJson and expiryTime using now.Subtract(slotTimeout)), which causes clock-skew issues; update all affected write/update SQL operations (those that set last_heartbeat or compare expiry windows) to use PostgreSQL's CURRENT_TIMESTAMP in the INSERT/UPDATE and compute the expiry window inside SQL using the provided slotTimeout parameter (pass slotTimeout as an interval/seconds value to SQL and use CURRENT_TIMESTAMP - slotTimeout for expiry checks) instead of computing expiryTime in C#; replace any use of now/DateTimeOffset.UtcNow and caller-side expiry calculation (variables like now, expiryTime, last_heartbeat) in methods that perform leasing, heartbeats, and lease checks so the database determines timestamps and expirations atomically.
170-175:⚠️ Potential issue | 🟠 MajorLet cancellation and disposal propagate unchanged.
These handlers still wrap
OperationCanceledExceptionandObjectDisposedExceptionintoSemaphoreProviderException, which makes cancellation/disposal indistinguishable from a real PostgreSQL failure. Add explicit catches for those two exception types before the generic wrapper.🐛 Representative fix
+ catch (OperationCanceledException) + { + throw; + } + catch (ObjectDisposedException) + { + throw; + } catch (Exception ex) { logger.LogError(ex, "..."); throw new SemaphoreProviderException("...", ex); }Also applies to: 212-217, 261-266, 299-303, 348-352, 383-388, 495-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs` around lines 170 - 175, The catch-all that wraps exceptions into SemaphoreProviderException is also catching OperationCanceledException and ObjectDisposedException; add explicit catch clauses for OperationCanceledException and ObjectDisposedException before the generic catch in the semaphore acquisition/release methods so those two exceptions are rethrown unchanged (do not wrap), then keep the existing logger.LogError + throw new SemaphoreProviderException(...) for all other exceptions; apply the same pattern to the other similar catch blocks in this class that currently catch Exception ex when (ex is not SemaphoreException) so cancellation and disposal propagate normally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@samples/MultiLock.SemaphoreSample/Program.cs`:
- Line 24: The parsed variable maxConcurrent (from args) can be zero or negative
causing runtime errors; update the parsing/validation around the maxConcurrent
assignment so that after TryParse you also check the parsed value > 0 and
otherwise fall back to the default (3) or throw a clear ArgumentException;
change the single-line parse logic that sets maxConcurrent to validate the
parsed int (e.g., ensure parsed 'max' > 0) and produce a descriptive error or
use the default when invalid before using maxConcurrent to construct the
semaphore.
- Around line 68-72: The sample currently embeds a hardcoded PostgreSQL password
in the default postgresConnectionString; update the default to avoid real
credentials by replacing the literal password with a clear placeholder (e.g.,
"Password=YOUR_PASSWORD_HERE") or read the password from an environment variable
and compose postgresConnectionString accordingly; locate the
postgresConnectionString declaration and the call to
builder.Services.AddPostgreSqlSemaphore(...) in Program.cs and change the
default value so it uses a placeholder or
Environment.GetEnvironmentVariable("PG_PASSWORD") instead of
"leaderelection123".
In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs`:
- Line 35: The fire-and-forget call to MonitorStatusChangesAsync(stoppingToken)
discards exceptions; change it to capture the Task (e.g., store in a field like
_monitorTask) and either attach a continuation to log
non-OperationCanceledException errors (using your ILogger instance) or await the
task during shutdown (e.g., in StopAsync or Dispose) so unhandled exceptions are
surfaced and logged; reference MonitorStatusChangesAsync, stoppingToken, the
created Task variable (e.g., _monitorTask), and the service shutdown method
(StopAsync/Dispose) when making the change.
In `@src/MultiLock/ISemaphoreProvider.cs`:
- Around line 59-84: GetHoldersAsync and IsHoldingAsync are underspecified for
expiry-sensitive semantics: they must either accept the same slotTimeout used by
GetCurrentCountAsync or have their XML docs relaxed. Update the public contract
by adding a TimeSpan slotTimeout parameter (with CancellationToken default) to
both GetHoldersAsync and IsHoldingAsync so providers can use the same heartbeat
expiry window, and update their XML docs to mention that holders/liveness are
evaluated using that slotTimeout; ensure method signatures match
GetCurrentCountAsync's pattern (include parameter name slotTimeout and keep
cancellationToken default).
In `@src/MultiLock/SemaphoreService.cs`:
- Around line 186-203: Both overloads of WaitForSlotAsync register for status
changes after checking currentStatus.IsHolding, creating a race where an
acquisition can be missed; move the subscriber registration (use the existing
subscriberRegistered hook or stateLock) so you subscribe before checking
currentStatus.IsHolding, then immediately re-check currentStatus.IsHolding (or
currentStatus.IsHolding under the same stateLock) after registration and return
if true; ensure the GetStatusChangesAsync await/foreach remains unchanged and
that the same pattern is applied to the other overload mentioned (the one around
lines 206-230).
- Around line 303-326: DisposeAsync currently disposes stateLock while public
methods may still be executing; add a simple in-flight public-call counter
(e.g., publicCalls or publicCallCount) incremented at the start of each public
method StartAsync, TryAcquireAsync, ReleaseAsync, and StopAsync and decremented
in their finally blocks, then in DisposeAsync after cancelling timers and
waiting for activeCallbacks, also wait for publicCallCount to reach zero (with a
similar max-wait loop) before calling stateLock.Dispose() and
cancellationTokenSource.Dispose(); update the mentioned public methods and
DisposeAsync to use this counter to prevent ObjectDisposedException when a
public method later calls stateLock.Release().
In `@src/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cs`:
- Around line 35-36: The FileExtension property validation only checks for
null/whitespace; update the FileSystemSemaphoreOptions validation to also ensure
FileExtension starts with a '.' and is a valid extension (e.g., not just "."),
by checking FileExtension.StartsWith('.') and length > 1 before throwing;
reference the FileExtension property and the existing validation block so you
add the extra checks and throw ArgumentException with a clear message when the
format is invalid.
In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs`:
- Around line 67-89: The Lua script in the RedisSemaphoreProvider.cs (the const
string script used for acquire) removes expired members from the ZSET
(holdersKey) but leaves their JSON in the hash (dataKey); update the script so
it captures expired member IDs (e.g. via redis.call('ZRANGEBYSCORE', KEYS[1],
'-inf', ARGV[1])), removes them from the ZSET and then HDEL(s) those IDs from
KEYS[2] to avoid orphaned hash fields and memory leaks; make this change inside
the same script string (keep the existing renew and add logic intact) so expired
holders are always removed from both holdersKey and dataKey.
In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs`:
- Around line 141-149: Remove the call to RemoveNode before performing the
remote delete: instead read the tracked path (use the same lookup logic that
RemoveNode uses but do not mutate activeNodes), call
zooKeeper.deleteAsync(nodePath), and only call RemoveNode (or remove from
activeNodes) after deleteAsync completes successfully or if you catch a
NoNodeException; on other exceptions keep the path in activeNodes and
surface/log the error so retries can still find the node. Apply the same change
to both code blocks that currently call RemoveNode before deleteAsync (the one
referencing RemoveNode and the later block at lines ~153-168).
- Around line 49-57: TryAcquireAsync and GetCurrentCountAsync currently validate
slotTimeout but never use HolderData.LastHeartbeat when computing capacity, so
dead holders that stopped heartbeating still count; update all places that
compute the live-holder set (e.g., TryAcquireAsync, GetCurrentCountAsync and any
helper that builds the holders list) to filter holders by comparing
HolderData.LastHeartbeat to DateTime.UtcNow - slotTimeout and only count/admit
those whose heartbeat is within the slotTimeout window, ensuring admission logic
uses the filtered live set when enforcing maxCount and returning the current
count.
- Around line 472-495: The connection timeout handler leaves the newly created
org.apache.zookeeper.ZooKeeper instance (zooKeeper) open, causing socket/watcher
leaks; modify the OperationCanceledException catch for the WaitAsync in
EnsureInitializedAsync so it closes and clears the zooKeeper field (call its
close/Dispose pattern used in the sessionExpired cleanup) before throwing the
SemaphoreProviderException, and ensure connectionCompletionSource is still
nulled in the finally block; reference the zooKeeper field,
connectionCompletionSource, EnsureInitializedAsync and the sessionExpired
cleanup logic to mirror the same cleanup steps.
In `@tests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cs`:
- Around line 69-70: The calls to provider.TryAcquireAsync(...) are ignoring
their boolean result which can hide failures; update each state-establishing
acquire (the TryAcquireAsync invocations for "holder-1"/"holder-2" and the other
listed occurrences) to assert the return value (e.g., Assert.True or
Assert.False as appropriate) immediately after awaiting so the test fails fast
if an acquire didn't succeed; search for TryAcquireAsync usages in this test
class and replace the bare awaits with assertions against the returned bool to
validate the setup.
- Around line 233-239: The test currently only asserts 0 < successCount <=
maxCount which won't detect under-admission; change the assertions after
awaiting Task.WhenAll(tasks) to require exact admission: assert successCount ==
maxCount (replace the Count assertions) and then call and assert that await
semaphore.GetCurrentCountAsync() equals maxCount to verify the semaphore reports
the same current count; locate and update the logic using the variables/results
and the GetCurrentCountAsync call in RedisSemaphoreIntegrationTests.
- Around line 16-29: The test constructor creates a LoggerFactory via
LoggerFactory.Create(...) but assigns it to a local variable that is never
disposed; change the constructor to store the factory in a private field (e.g.,
loggerFactory) instead of a local variable and update DisposeAsync() to dispose
it (call loggerFactory.Dispose() or await disposing wrapper) while keeping
existing behavior for logger and keyPrefix; reference the
RedisSemaphoreIntegrationTests constructor, the local loggerFactory variable,
and the DisposeAsync method when making this change.
In `@tests/MultiLock.Tests/FileSystemServiceCollectionExtensionsTests.cs`:
- Around line 220-225: The test redundantly calls
provider.GetService<ISemaphoreProvider>() twice; fix by resolving once into a
local variable (e.g., var semaphore = provider.GetService<ISemaphoreProvider>()
) after ServiceProvider provider = services.BuildServiceProvider() and then
assert semaphore.ShouldNotBeNull() and
semaphore.ShouldBeOfType<FileSystemSemaphoreProvider>(); apply the same
single-resolution change for the other occurrences that repeat the pattern.
- Around line 181-184: The test calls provider.GetService<ISemaphoreProvider>()
twice; capture the resolved ISemaphoreProvider into a local variable (e.g., var
semaphoreProvider = provider.GetService<ISemaphoreProvider>()) and then assert
semaphoreProvider.ShouldNotBeNull() and
semaphoreProvider.ShouldBeOfType<FileSystemSemaphoreProvider>(), leaving the
existing provider.GetService<ISemaphoreService>().ShouldNotBeNull() unchanged;
this avoids redundant resolution and makes the intent clearer.
In `@tests/MultiLock.Tests/InMemoryServiceCollectionExtensionsTests.cs`:
- Around line 122-125: Store the resolved ISemaphoreProvider in a local variable
instead of calling provider.GetService<ISemaphoreProvider>() twice: after
ServiceProvider provider = services.BuildServiceProvider(); assign var
semaphoreProvider = provider.GetService<ISemaphoreProvider>(); then assert
semaphoreProvider.ShouldNotBeNull() and
semaphoreProvider.ShouldBeOfType<InMemorySemaphoreProvider>(); keep the existing
provider.GetService<ISemaphoreService>().ShouldNotBeNull() unchanged; this
reduces redundant resolution and makes assertions clearer.
In `@tests/MultiLock.Tests/SemaphoreServiceTests.cs`:
- Around line 176-215: Add a regression test that starts a pending
WaitForSlotAsync (call service.WaitForSlotAsync(TimeSpan.FromSeconds(5)) or the
non-timeout overload), then dispose the service (call service.Dispose()) while
the wait is pending, and assert the wait task completes with the expected
disposal outcome (e.g., returns false) rather than being canceled or throwing
TaskCanceledException; reference the WaitForSlotAsync method and the
service.Dispose call in your test to ensure the code path that previously
misreported disposal as caller cancellation is covered.
---
Duplicate comments:
In `@src/MultiLock/SemaphoreService.cs`:
- Around line 411-457: ExecuteWithRetryAsync currently returns default on retry
exhaustion which hides provider failures; instead, when attempt >
options.MaxRetryAttempts, throw the final exception (or wrap it with context) so
callers like TryAcquireInternalAsync and ReleaseAsync can distinguish backend
errors from normal semaphore states. Locate ExecuteWithRetryAsync, use the
captured lastException (or create a new Exception that includes operationName,
HolderId and attempts) and throw it after logging; preserve the existing logging
calls and remove the final "return default!" so failures propagate to callers.
In
`@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs`:
- Around line 412-433: The TryWriteSemaphoreDataAsync method currently swallows
ETag precondition failures by returning false; instead surface those
write-conflict errors so callers can retry or handle them explicitly: in the
catch (RequestFailedException ex) when ex.Status == 412 (inside
TryWriteSemaphoreDataAsync) do not return false—rethrow the
RequestFailedException (or wrap and throw a specific Concurrency/OptimisticLock
exception) so higher layers detect an optimistic-concurrency collision; keep the
rest of the method behavior the same and ensure callers of
TryWriteSemaphoreDataAsync handle this exception path rather than interpreting a
boolean false as a normal semaphore outcome.
In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs`:
- Around line 95-149: After CreateSessionAsync succeeds we must ensure the new
sessionId is destroyed on any exception paths before AddSession() is called;
modify the Acquire logic in ConsulSemaphoreProvider (methods CreateSessionAsync,
AddSession usage) to track whether sessionId was created but not yet added and
call consulClient.Session.Destroy(sessionId, cancellationToken) on all
exception/early-return paths (including the outer catch or a local try/finally)
so sessions aren’t leaked when an exception occurs before AddSession().
- Around line 232-246: GetCurrentCountAsync currently ignores the slotTimeout
parameter; add explicit validation to prevent silent mismatch with the Consul
session TTL by checking slotTimeout against the provider's configured session
TTL (options.SessionTtl) at the start of GetCurrentCountAsync (the same place
TryAcquireAsync calls it), and if they differ in a way that would change
admission behavior throw an ArgumentException or ArgumentOutOfRangeException;
keep the existing KV listing via
consulClient.KV.List(GetSemaphorePrefix(semaphoreName)) only for the case where
slotTimeout equals options.SessionTtl, and reference GetCurrentCountAsync,
TryAcquireAsync, options.SessionTtl, GetSemaphorePrefix, and
consulClient.KV.List when making the change.
In `@src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs`:
- Around line 456-460: CountHolders currently counts filenames and can treat
corrupt/unreadable holder files as live holders; change CountHolders to iterate
files in semaphoreDir matching options.FileExtension, attempt to open and
deserialize each file using the same deserialization logic as
CleanupExpiredHolders (or call the same helper used there), and only increment
the count for files that successfully deserialize and are not expired; skip (and
optionally log) files that fail to read/deserialize so corrupt files don’t
occupy capacity. Ensure you reference semaphoreDir, options.FileExtension, and
the existing CleanupExpiredHolders deserialization routine to share behavior.
In `@src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs`:
- Around line 225-231: The Dispose() method must be serialized with the same
synchronization used for the semaphores dictionary; change Dispose() so the
isDisposed check, setting isDisposed = true, and semaphores.Clear() all execute
while holding the same lock object (the `@lock` used elsewhere in
InMemorySemaphoreProvider), e.g., enter the monitor/lock around those operations
to prevent races with Acquire/Release/Heartbeat/Count methods that access
semaphores concurrently.
In `@src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs`:
- Around line 57-60: The code currently uses DateTimeOffset.UtcNow to set
last_heartbeat and compute expiryTime (e.g., the snippet that creates
metadataJson and expiryTime using now.Subtract(slotTimeout)), which causes
clock-skew issues; update all affected write/update SQL operations (those that
set last_heartbeat or compare expiry windows) to use PostgreSQL's
CURRENT_TIMESTAMP in the INSERT/UPDATE and compute the expiry window inside SQL
using the provided slotTimeout parameter (pass slotTimeout as an
interval/seconds value to SQL and use CURRENT_TIMESTAMP - slotTimeout for expiry
checks) instead of computing expiryTime in C#; replace any use of
now/DateTimeOffset.UtcNow and caller-side expiry calculation (variables like
now, expiryTime, last_heartbeat) in methods that perform leasing, heartbeats,
and lease checks so the database determines timestamps and expirations
atomically.
- Around line 170-175: The catch-all that wraps exceptions into
SemaphoreProviderException is also catching OperationCanceledException and
ObjectDisposedException; add explicit catch clauses for
OperationCanceledException and ObjectDisposedException before the generic catch
in the semaphore acquisition/release methods so those two exceptions are
rethrown unchanged (do not wrap), then keep the existing logger.LogError + throw
new SemaphoreProviderException(...) for all other exceptions; apply the same
pattern to the other similar catch blocks in this class that currently catch
Exception ex when (ex is not SemaphoreException) so cancellation and disposal
propagate normally.
In `@src/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cs`:
- Around line 464-483: The DDL block in SqlServerSemaphoreProvider that builds
the `sql` string is racy and couples index creation to table creation; change it
so table/index/constraint creation are idempotent under concurrent initializers
by (1) executing CREATE TABLE in a TRY/CATCH that ignores the "already exists"
error (2714) instead of relying on a separate IF NOT EXISTS check, and (2)
create the primary key and the index in separate statements that only run when
they don't exist (e.g., check sys.key_constraints/sys.indexes or use TRY/CATCH
to ignore "already exists" errors). Update the code that builds `sql` (the block
referencing options.SchemaName and options.TableName and the PK/IX names) to
perform these separate safe operations, then execute the resulting batch via the
existing SqlCommand/ExecuteNonQueryAsync call so concurrent initializers can't
race to leave the schema inconsistent.
In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs`:
- Around line 95-102: The code in ZooKeeperSemaphoreProvider.cs should treat
position < 0 the same as position >= maxCount: after refreshing children and
computing createdNodeName and position, check if position < 0 || position >=
maxCount and in that case delete the created znode (use the existing delete call
pattern, e.g., zooKeeper.deleteAsync(createdPath) or the provider's removal
method) and return/fail the acquisition; update the if condition that currently
checks only position >= maxCount to use position < 0 || position >= maxCount so
missing nodes are handled as failed acquisitions.
In
`@tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs`:
- Around line 74-75: The test currently ignores the boolean result of
TryAcquireAsync calls (e.g., the calls to TryAcquireAsync("test-semaphore",
"holder-1", ...), TryAcquireAsync("test-semaphore", "holder-2", ...) and the
other similar calls flagged) which can hide failed acquires; update each
state-establishing call to assert the returned value is true (e.g.,
Assert.True(await provider.TryAcquireAsync(...), "Acquire should succeed")) so
the test fails immediately if an expected acquire did not succeed; apply this
change to every TryAcquireAsync invocation mentioned (including the duplicates
at the other locations).
In `@tests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cs`:
- Around line 74-75: The test calls to provider.TryAcquireAsync
("TryAcquireAsync") are ignoring the returned boolean, so failing acquires can
silently leave the semaphore state incorrect; update each occurrence (e.g., the
calls at lines shown and duplicates) to capture the result and assert it is true
(use the test framework's assert, e.g., Assert.True or equivalent) immediately
after each TryAcquireAsync call to ensure the acquire actually succeeded before
proceeding with subsequent assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c22ed7b-404c-45dd-9fd2-04feee2516fe
📒 Files selected for processing (77)
MultiLock.slnxREADME.mdsamples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csprojsamples/MultiLock.SemaphoreSample/Program.cssamples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cssrc/MultiLock/Exceptions/SemaphoreException.cssrc/MultiLock/Exceptions/SemaphoreFullException.cssrc/MultiLock/Exceptions/SemaphoreProviderException.cssrc/MultiLock/Exceptions/SemaphoreTimeoutException.cssrc/MultiLock/ISemaphoreProvider.cssrc/MultiLock/ISemaphoreService.cssrc/MultiLock/MultiLock.csprojsrc/MultiLock/ParameterValidation.cssrc/MultiLock/SemaphoreAcquisition.cssrc/MultiLock/SemaphoreChangedEventArgs.cssrc/MultiLock/SemaphoreHolder.cssrc/MultiLock/SemaphoreInfo.cssrc/MultiLock/SemaphoreOptions.cssrc/MultiLock/SemaphoreService.cssrc/MultiLock/SemaphoreServiceExtensions.cssrc/MultiLock/SemaphoreStatus.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageServiceCollectionExtensions.cssrc/Providers/MultiLock.AzureBlobStorage/MultiLock.AzureBlobStorage.csprojsrc/Providers/MultiLock.Consul/ConsulLeaderElectionOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cssrc/Providers/MultiLock.Consul/ConsulServiceCollectionExtensions.cssrc/Providers/MultiLock.Consul/MultiLock.Consul.csprojsrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cssrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cssrc/Providers/MultiLock.FileSystem/FileSystemServiceCollectionExtensions.cssrc/Providers/MultiLock.FileSystem/HolderFileContent.cssrc/Providers/MultiLock.FileSystem/MultiLock.FileSystem.csprojsrc/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cssrc/Providers/MultiLock.InMemory/InMemoryServiceCollectionExtensions.cssrc/Providers/MultiLock.InMemory/MultiLock.InMemory.csprojsrc/Providers/MultiLock.InMemory/SemaphoreSlotRecord.cssrc/Providers/MultiLock.PostgreSQL/MultiLock.PostgreSQL.csprojsrc/Providers/MultiLock.PostgreSQL/PostgreSQLServiceCollectionExtensions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cssrc/Providers/MultiLock.Redis/MultiLock.Redis.csprojsrc/Providers/MultiLock.Redis/RedisSemaphoreOptions.cssrc/Providers/MultiLock.Redis/RedisSemaphoreProvider.cssrc/Providers/MultiLock.Redis/RedisServiceCollectionExtensions.cssrc/Providers/MultiLock.SqlServer/MultiLock.SqlServer.csprojsrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreOptions.cssrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cssrc/Providers/MultiLock.SqlServer/SqlServerServiceCollectionExtensions.cssrc/Providers/MultiLock.ZooKeeper/MultiLock.ZooKeeper.csprojsrc/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperServiceCollectionExtensions.cstests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cstests/MultiLock.Tests/FileSystemSemaphoreOptionsTests.cstests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cstests/MultiLock.Tests/FileSystemServiceCollectionExtensionsTests.cstests/MultiLock.Tests/InMemorySemaphoreProviderTests.cstests/MultiLock.Tests/InMemoryServiceCollectionExtensionsTests.cstests/MultiLock.Tests/SemaphoreAcquisitionTests.cstests/MultiLock.Tests/SemaphoreConcurrencyTests.cstests/MultiLock.Tests/SemaphoreExceptionTests.cstests/MultiLock.Tests/SemaphoreModelTests.cstests/MultiLock.Tests/SemaphoreOptionsValidationTests.cstests/MultiLock.Tests/SemaphoreParameterValidationTests.cstests/MultiLock.Tests/SemaphoreServiceTests.cstests/MultiLock.Tests/ServiceExtensionsValidationTests.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-and-test
- GitHub Check: Analyze (csharp)
🧰 Additional context used
🪛 Betterleaks (1.1.1)
tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs
[high] 12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| semaphoreService.MaxCount); | ||
|
|
||
| // Start monitoring semaphore status changes in background | ||
| _ = MonitorStatusChangesAsync(stoppingToken); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fire-and-forget task discards exceptions.
The discarded task _ = MonitorStatusChangesAsync(stoppingToken) swallows any exception other than OperationCanceledException. For a sample app this is acceptable, but in production consider logging unhandled exceptions or awaiting the task during shutdown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs` at line 35,
The fire-and-forget call to MonitorStatusChangesAsync(stoppingToken) discards
exceptions; change it to capture the Task (e.g., store in a field like
_monitorTask) and either attach a continuation to log
non-OperationCanceledException errors (using your ILogger instance) or await the
task during shutdown (e.g., in StopAsync or Dispose) so unhandled exceptions are
surfaced and logged; reference MonitorStatusChangesAsync, stoppingToken, the
created Task variable (e.g., _monitorTask), and the service shutdown method
(StopAsync/Dispose) when making the change.
Comment on lines
+59
to
+84
| Task<int> GetCurrentCountAsync( | ||
| string semaphoreName, | ||
| TimeSpan slotTimeout, | ||
| CancellationToken cancellationToken = default); | ||
|
|
||
| /// <summary> | ||
| /// Gets information about all current holders of the semaphore. | ||
| /// </summary> | ||
| /// <param name="semaphoreName">The name of the semaphore.</param> | ||
| /// <param name="cancellationToken">A token to cancel the operation.</param> | ||
| /// <returns>A collection of holder information, or an empty collection if no holders exist.</returns> | ||
| Task<IReadOnlyList<SemaphoreHolder>> GetHoldersAsync( | ||
| string semaphoreName, | ||
| CancellationToken cancellationToken = default); | ||
|
|
||
| /// <summary> | ||
| /// Checks if the specified holder currently holds a slot in the semaphore. | ||
| /// </summary> | ||
| /// <param name="semaphoreName">The name of the semaphore.</param> | ||
| /// <param name="holderId">The unique identifier of the holder to check.</param> | ||
| /// <param name="cancellationToken">A token to cancel the operation.</param> | ||
| /// <returns>True if the holder currently holds a slot; otherwise, false.</returns> | ||
| Task<bool> IsHoldingAsync( | ||
| string semaphoreName, | ||
| string holderId, | ||
| CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Expiry-sensitive queries are under-specified.
GetCurrentCountAsync needs slotTimeout to decide whether a heartbeat is expired, but GetHoldersAsync and IsHoldingAsync still promise "current" semantics without taking that same window. That makes these APIs impossible to keep consistent across providers that derive liveness from heartbeat age. Either pass the expiry window through both methods or relax the XML docs before this public contract ships.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MultiLock/ISemaphoreProvider.cs` around lines 59 - 84, GetHoldersAsync
and IsHoldingAsync are underspecified for expiry-sensitive semantics: they must
either accept the same slotTimeout used by GetCurrentCountAsync or have their
XML docs relaxed. Update the public contract by adding a TimeSpan slotTimeout
parameter (with CancellationToken default) to both GetHoldersAsync and
IsHoldingAsync so providers can use the same heartbeat expiry window, and update
their XML docs to mention that holders/liveness are evaluated using that
slotTimeout; ensure method signatures match GetCurrentCountAsync's pattern
(include parameter name slotTimeout and keep cancellationToken default).
Comment on lines
+181
to
+184
| ServiceProvider provider = services.BuildServiceProvider(); | ||
| provider.GetService<ISemaphoreProvider>().ShouldNotBeNull(); | ||
| provider.GetService<ISemaphoreProvider>().ShouldBeOfType<FileSystemSemaphoreProvider>(); | ||
| provider.GetService<ISemaphoreService>().ShouldNotBeNull(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid redundant service resolution.
GetService<ISemaphoreProvider>() is called twice. Store the result in a variable to avoid redundant resolution.
♻️ Suggested fix
ServiceProvider provider = services.BuildServiceProvider();
- provider.GetService<ISemaphoreProvider>().ShouldNotBeNull();
- provider.GetService<ISemaphoreProvider>().ShouldBeOfType<FileSystemSemaphoreProvider>();
+ ISemaphoreProvider? semaphoreProvider = provider.GetService<ISemaphoreProvider>();
+ semaphoreProvider.ShouldNotBeNull();
+ semaphoreProvider.ShouldBeOfType<FileSystemSemaphoreProvider>();
provider.GetService<ISemaphoreService>().ShouldNotBeNull();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ServiceProvider provider = services.BuildServiceProvider(); | |
| provider.GetService<ISemaphoreProvider>().ShouldNotBeNull(); | |
| provider.GetService<ISemaphoreProvider>().ShouldBeOfType<FileSystemSemaphoreProvider>(); | |
| provider.GetService<ISemaphoreService>().ShouldNotBeNull(); | |
| ServiceProvider provider = services.BuildServiceProvider(); | |
| ISemaphoreProvider? semaphoreProvider = provider.GetService<ISemaphoreProvider>(); | |
| semaphoreProvider.ShouldNotBeNull(); | |
| semaphoreProvider.ShouldBeOfType<FileSystemSemaphoreProvider>(); | |
| provider.GetService<ISemaphoreService>().ShouldNotBeNull(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/MultiLock.Tests/FileSystemServiceCollectionExtensionsTests.cs` around
lines 181 - 184, The test calls provider.GetService<ISemaphoreProvider>() twice;
capture the resolved ISemaphoreProvider into a local variable (e.g., var
semaphoreProvider = provider.GetService<ISemaphoreProvider>()) and then assert
semaphoreProvider.ShouldNotBeNull() and
semaphoreProvider.ShouldBeOfType<FileSystemSemaphoreProvider>(), leaving the
existing provider.GetService<ISemaphoreService>().ShouldNotBeNull() unchanged;
this avoids redundant resolution and makes the intent clearer.
Comment on lines
+220
to
+225
|
|
||
| // Only verify the provider itself is registered (the service is lazy-validated) | ||
| ServiceProvider provider = services.BuildServiceProvider(); | ||
| provider.GetService<ISemaphoreProvider>().ShouldNotBeNull(); | ||
| provider.GetService<ISemaphoreProvider>().ShouldBeOfType<FileSystemSemaphoreProvider>(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same redundant resolution pattern.
Lines 222-224 and 238-240 follow the same pattern of calling GetService<ISemaphoreProvider>() twice. Apply the same fix for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/MultiLock.Tests/FileSystemServiceCollectionExtensionsTests.cs` around
lines 220 - 225, The test redundantly calls
provider.GetService<ISemaphoreProvider>() twice; fix by resolving once into a
local variable (e.g., var semaphore = provider.GetService<ISemaphoreProvider>()
) after ServiceProvider provider = services.BuildServiceProvider() and then
assert semaphore.ShouldNotBeNull() and
semaphore.ShouldBeOfType<FileSystemSemaphoreProvider>(); apply the same
single-resolution change for the other occurrences that repeat the pattern.
Implement a distributed counting semaphore abstraction (ISemaphoreService / ISemaphoreProvider) mirroring the existing leader-election design, with providers for PostgreSQL, SQL Server, Redis, Azure Blob Storage, Consul, ZooKeeper, FileSystem, and InMemory. Key design decisions and fixes included in this commit: - AutoStart=false defers acquisition to the caller; StartAsync drives acquisition when true; ExecuteAsync is a no-op stub (BackgroundService requirement) - GetCurrentCountAsync filters by heartbeat age (slotTimeout) so expired holders are excluded from the live count across all providers - PostgreSQL uses pg_advisory_xact_lock(hashtext(semaphoreName)) under ReadCommitted instead of SERIALIZABLE + FOR UPDATE, which deadlocked on empty tables - SQL Server uses sp_getapplock(@resource, 'Exclusive', 'Transaction') under ReadCommitted instead of SERIALIZABLE + range locks, which caused deadlocks when the table was empty - Consul and ZooKeeper apply a post-acquisition count recheck to close the TOCTOU window between the count read and the slot creation - FileSystem provider validates canonical paths to prevent directory traversal via semaphoreName or holderId - Schema and table names are validated against ^[a-zA-Z_][a-zA-Z0-9_]*$ (max 128 chars) before interpolation into SQL to prevent injection - InMemory provider replaced ConcurrentDictionary with plain Dictionary guarded by a single lock, removing redundant thread-safety layering - SemaphoreAcquisition.Dispose() delegates to DisposeAsync() to avoid sync-context deadlocks from direct GetAwaiter().GetResult() on the task - SemaphoreService.DisposeAsync() awaits broadcastTask before completing - UpdateStatus() is a plain void method; the previous async overload only awaited Task.CompletedTask and accepted an unused CancellationToken - HeartbeatTimeout (not the now-removed SlotTimeout) is passed as the expiry window to all provider calls - IAsyncDisposable removed from SemaphoreService class declaration (already satisfied via ISemaphoreService) - CurrentCount decrements are guarded with Math.Max(0, ...) to prevent negative counts on spurious releases - Integration tests: each test instance uses a unique table name and semaphore name (Guid-per-instance); DisposeAsync drops the table; ConcurrentAcquire asserts successCount == maxCount and cross-checks via GetCurrentCountAsync Fixes #100
steingran
force-pushed
the
feat/semaphore
branch
from
4453050 to
a1769a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (16)
src/MultiLock/ParameterValidation.cs (1)
11-12:⚠️ Potential issue | 🟠 MajorUse provider-specific SQL identifier length limits in shared validator.
Line 11 and Line 182 enforce a universal 128-character SQL identifier limit, but PostgreSQL identifiers are shorter, so distinct long names can collide after server-side truncation. Please make the max length explicit per provider at call time.
🔧 Proposed fix
- private const int maxSqlIdentifierLength = 128; + private const int defaultSqlIdentifierMaxLength = 128; ... - public static void ValidateSqlIdentifier(string value, string paramName) + public static void ValidateSqlIdentifier( + string value, + string paramName, + int maxLength = defaultSqlIdentifierMaxLength) { if (string.IsNullOrWhiteSpace(value)) throw new ArgumentException("SQL identifier cannot be null, empty, or whitespace.", paramName); - if (value.Length > maxSqlIdentifierLength) + if (value.Length > maxLength) throw new ArgumentException( - $"SQL identifier cannot exceed {maxSqlIdentifierLength} characters.", paramName); + $"SQL identifier cannot exceed {maxLength} characters.", paramName);Then pass provider-specific limits from options validation (e.g., PostgreSQL 63, SQL Server 128).
What are the official maximum identifier lengths for PostgreSQL schema/table names and SQL Server schema/table names?Also applies to: 177-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/ParameterValidation.cs` around lines 11 - 12, Replace the hard-coded universal maxSqlIdentifierLength (and its use in ParameterValidation methods around the former lines 177-185) with a provider-specific limit passed into the validator from options/connection info; update the signature of the validator entry point (e.g., ValidateOptions/ValidateIdentifiers or the ParameterValidation class constructor) to accept an int identifierMaxLength and use that instead of maxSqlIdentifierLength, and retain maxMetadataKeyLength for metadata; for reference enforce PostgreSQL identifiers to 63 and SQL Server identifiers to 128 when calling the validator so provider-specific truncation is correctly handled.tests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cs (1)
46-47:⚠️ Potential issue | 🟡 MinorAssert setup acquisitions to avoid false-positive tests.
At Line 46 and the other setup sites, precondition
TryAcquireAsyncresults are ignored; if setup fails, downstream assertions can pass for the wrong reason.Proposed fix pattern
- await provider.TryAcquireAsync("sem1", "h1", 1, NoMeta, TimeSpan.FromMinutes(5)); + (await provider.TryAcquireAsync("sem1", "h1", 1, NoMeta, TimeSpan.FromMinutes(5))).ShouldBeTrue();Also applies to: 54-55, 64-65, 79-80, 94-95, 103-104, 114-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cs` around lines 46 - 47, The test setup calls to provider.TryAcquireAsync in FileSystemSemaphoreProviderTests currently ignore their boolean results, which can hide setup failures and produce false-positive assertions; update each setup call (e.g., the initial TryAcquireAsync calls at the start of tests around lines where provider.TryAcquireAsync("sem1", ... ) is used) to capture and assert the returned bool is true (use Assert.True or equivalent) before proceeding, so that any failure to acquire during test setup fails the test immediately.src/MultiLock/SemaphoreAcquisition.cs (2)
10-10:⚠️ Potential issue | 🟠 MajorUse an atomic dispose guard to prevent concurrent double-release.
At Line 59–Line 63, the check/set pattern is racy; two callers can both proceed to
ReleaseAsync.Proposed fix
- private volatile bool isDisposed; + private int disposeState; ... - public bool IsDisposed => isDisposed; + public bool IsDisposed => Volatile.Read(ref disposeState) == 1; ... - if (isDisposed) + if (Interlocked.Exchange(ref disposeState, 1) == 1) return; - - isDisposed = true;Also applies to: 52-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreAcquisition.cs` at line 10, The current dispose/check pattern using the volatile field isDisposed is racy and can allow concurrent double-release; replace the check/set with an atomic operation (e.g., use Interlocked.Exchange or CompareExchange) to ensure only one thread wins the dispose path in ReleaseAsync and any Dispose/DisposeAsync logic. Specifically, update the ReleaseAsync method and any Dispose-related code that reads/writes isDisposed to atomically set the flag and only proceed with release logic when the exchange indicates the caller is the first to flip the flag (use the unique symbol isDisposed and method ReleaseAsync to locate the code).
81-84:⚠️ Potential issue | 🟠 MajorAvoid sync-over-async deadlock in
Dispose().At Line 83, blocking directly on
DisposeAsync()can deadlock when continuations need the current synchronization context.Proposed fix
public void Dispose() { - DisposeAsync().AsTask().GetAwaiter().GetResult(); + Task.Run(() => DisposeAsync().AsTask()).GetAwaiter().GetResult(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreAcquisition.cs` around lines 81 - 84, The current Dispose() blocks on DisposeAsync() which can deadlock; change it to call DisposeAsync().AsTask().ConfigureAwait(false).GetAwaiter().GetResult() so the async continuation doesn't capture the ambient synchronization context—update the SemaphoreAcquisition.Dispose() implementation to use AsTask().ConfigureAwait(false).GetAwaiter().GetResult() instead of plain AsTask().GetAwaiter().GetResult().samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs (2)
47-59:⚠️ Potential issue | 🟠 MajorAlways release in
finally, and don’t cancel the release call.At Line 55–Line 59, if work throws/cancels, the slot may never be released; and using
stoppingTokenfor release can cancel cleanup itself.Proposed fix
if (acquired) { logger.LogInformation( "[Request {RequestId}] ✓ Acquired slot! Currently holding: {IsHolding}", requestId, semaphoreService.IsHolding); - // Simulate API call work - await SimulateApiCallAsync(requestId, stoppingToken); - - // Release the slot - await semaphoreService.ReleaseAsync(stoppingToken); - logger.LogInformation("[Request {RequestId}] Released slot", requestId); + try + { + // Simulate API call work + await SimulateApiCallAsync(requestId, stoppingToken); + } + finally + { + await semaphoreService.ReleaseAsync(CancellationToken.None); + logger.LogInformation("[Request {RequestId}] Released slot", requestId); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs` around lines 47 - 59, The acquired-slot branch in RateLimitedWorkerService (around SimulateApiCallAsync and semaphoreService.ReleaseAsync) must guarantee release: move the ReleaseAsync call into a finally block so the slot is always released even if SimulateApiCallAsync throws or is canceled, and do not pass the stoppingToken into semaphoreService.ReleaseAsync (call ReleaseAsync without a cancellation token) so cleanup cannot be canceled; ensure the logger messages remain but that the release logging occurs after the awaited ReleaseAsync in the finally.
35-35: 🧹 Nitpick | 🔵 TrivialDon’t discard the monitor task without observing faults.
At Line 35, fire-and-forget can hide non-cancellation exceptions from
MonitorStatusChangesAsync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs` at line 35, Fire-and-forget of MonitorStatusChangesAsync(stoppingToken) hides exceptions; capture the returned Task (e.g., monitorTask = MonitorStatusChangesAsync(stoppingToken)) and observe faults instead of discarding it: either await it where appropriate or attach a continuation/try-catch to log and rethrow or swallow exceptions (e.g., monitorTask.ContinueWith(..., OnlyOnFaulted) or wrap in Task.Run with try/catch and use the service logger). Reference MonitorStatusChangesAsync and the stoppingToken so the task is observed and exceptions are logged.tests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cs (1)
33-153:⚠️ Potential issue | 🟠 MajorAdd a cross-process contention test.
This suite only exercises providers inside the same test process, so it will not catch a filesystem semaphore implementation that admits more than
maxCountholders across separate processes sharing the same directory. Add a helper-process test that reuses the sameDirectoryPathfrom another OS process and asserts total successful acquires stop atmaxCount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cs` around lines 33 - 153, The tests lack a cross-process contention case; add an integration test that starts a helper process which constructs a FileSystemSemaphoreProvider with the same Options.Create(options).DirectoryPath and attempts multiple TryAcquireAsync calls, then assert that across both processes the total successful acquires never exceeds the configured maxCount; implement this by adding a new test method (e.g., CrossProcess_Contention_ShouldRespectMaxCount) that (1) creates options and a provider in the test process, (2) spawns a child process (same test binary or a small helper executable) passing the DirectoryPath and desired attempts, (3) have the child use FileSystemSemaphoreProvider.TryAcquireAsync to attempt N acquires and return the number of successes, (4) wait for the child to exit and parse its result, and (5) assert that successes in parent + child <= maxCount; reuse existing symbols FileSystemSemaphoreProvider, TryAcquireAsync, CreateOptions, and DirectoryPath to locate where to hook this behavior.src/MultiLock/ISemaphoreProvider.cs (1)
64-84:⚠️ Potential issue | 🟠 MajorAPI consistency concern:
GetHoldersAsyncandIsHoldingAsynclack expiry semantics.
GetCurrentCountAsyncacceptsslotTimeoutto filter expired holders, butGetHoldersAsyncandIsHoldingAsyncdon't take this parameter. This creates an inconsistency where:
GetCurrentCountAsyncreturns count of live holders (filtered by heartbeat age)GetHoldersAsyncreturns all holders (possibly including expired ones)IsHoldingAsyncchecks holder status without considering expiryEither add the
slotTimeoutparameter to these methods for consistency, or clarify in the XML docs that these methods intentionally return/check unfiltered data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/ISemaphoreProvider.cs` around lines 64 - 84, GetHoldersAsync and IsHoldingAsync are inconsistent with GetCurrentCountAsync because they lack the slotTimeout expiry filter; add a CancellationToken-optional TimeSpan slotTimeout parameter to both GetHoldersAsync and IsHoldingAsync (matching signature style of GetCurrentCountAsync), update their XML docs to state that results are filtered by slotTimeout (expired holders are ignored), and ensure implementations that call these interface methods use the slotTimeout to filter out holders by heartbeat age just like GetCurrentCountAsync does; reference the methods GetHoldersAsync, IsHoldingAsync and GetCurrentCountAsync when updating interface, docs, and implementations.src/MultiLock/SemaphoreServiceExtensions.cs (1)
27-29:⚠️ Potential issue | 🟠 MajorHard cast can fail at runtime if a custom
ISemaphoreServiceis pre-registered.The cast
(SemaphoreService)provider.GetRequiredService<ISemaphoreService>()on line 29 will throwInvalidCastExceptionifISemaphoreServiceis registered to a different implementation beforeAddSemaphoreis called. Consider registering the concreteSemaphoreServicefirst, then mapping both interfaces to that instance.🔧 Safer DI wiring
- services.TryAddSingleton<ISemaphoreService, SemaphoreService>(); - services.AddHostedService<SemaphoreService>(provider => - (SemaphoreService)provider.GetRequiredService<ISemaphoreService>()); + services.TryAddSingleton<SemaphoreService>(); + services.TryAddSingleton<ISemaphoreService>(sp => sp.GetRequiredService<SemaphoreService>()); + services.AddHostedService(sp => sp.GetRequiredService<SemaphoreService>());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreServiceExtensions.cs` around lines 27 - 29, The hard cast (SemaphoreService)provider.GetRequiredService<ISemaphoreService>() can throw if a different ISemaphoreService implementation was pre-registered; instead ensure the concrete SemaphoreService is registered and reused: register SemaphoreService as the singleton implementation (TryAddSingleton<SemaphoreService> / add concrete), then map the interface to that same instance (TryAddSingleton<ISemaphoreService>(provider => provider.GetRequiredService<SemaphoreService>())), and resolve the hosted service from the concrete SemaphoreService when calling AddHostedService so no invalid cast occurs; update the registration calls around ISemaphoreService and SemaphoreService accordingly.tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs (1)
13-15:⚠️ Potential issue | 🟠 MajorRemove the hardcoded PostgreSQL fallback.
This still keeps DB credentials in source, and
??will not catch an empty env var. Prefer a null/whitespace check with a clear startup failure instead of falling back to localhost credentials.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs` around lines 13 - 15, The hardcoded fallback in the static readonly connectionString (used by PostgreSQLSemaphoreIntegrationTests) leaks credentials and ignores empty env vars; change to read the env var into connectionString without a hardcoded default, then validate with string.IsNullOrWhiteSpace and throw a clear startup exception (e.g., InvalidOperationException) naming the missing MULTILOCK_POSTGRES_CONNECTION so tests fail fast and don’t use localhost credentials.src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs (1)
255-269:⚠️ Potential issue | 🟠 MajorHonor
slotTimeoutinGetCurrentCountAsync.
TryAcquireAsyncuses this count for both the pre-check and the post-acquisition rollback, but this implementation silently ignores the caller's expiry window. If this provider intentionally relies onSessionTtl, validate thatslotTimeoutmatches it and fail fast instead of making admission decisions against a different timeout than the caller requested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs` around lines 255 - 269, GetCurrentCountAsync ignores the caller's slotTimeout and simply lists KV entries; update it to honor slotTimeout by either filtering entries by their remaining TTL or validating slotTimeout against the provider's configured session TTL and failing fast. Locate GetCurrentCountAsync and use GetSemaphorePrefix and consulClient.KV.List to fetch candidates, then: if the provider uses a fixed SessionTtl, compare the provided slotTimeout to SessionTtl and throw/return an error when they differ; otherwise compute remaining lifetime for each KV/session and only count entries whose remaining TTL >= slotTimeout (or whose session expiry would cover the requested slotTimeout) so TryAcquireAsync's pre-check and rollback use a consistent timeout expectation. Ensure ThrowIfDisposed and ParameterValidation paths remain unchanged.src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs (1)
112-117:⚠️ Potential issue | 🟠 MajorLet cancellation escape from
TryAcquireAsync.
AcquireLockFileAsynchonorscancellationToken, but the catch-all here wraps thatOperationCanceledExceptioninSemaphoreProviderException.SemaphoreServicewill then treat a canceled lock wait as a provider failure and retry it instead of stopping promptly.Also applies to: 122-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs` around lines 112 - 117, The catch-all in FileSystemSemaphoreProvider.TryAcquireAsync is wrapping OperationCanceledException thrown by AcquireLockFileAsync and turning cancellations into SemaphoreProviderException; change the catch in TryAcquireAsync so that if the caught exception is OperationCanceledException or TaskCanceledException (or the token signals cancellation) you rethrow the original exception immediately, otherwise log via logger.LogError and throw new SemaphoreProviderException("Failed to acquire semaphore slot", ex). Apply the same change to the similar block covering lines 122-134 so cancellation can escape instead of being treated as provider failure.src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs (1)
62-117:⚠️ Potential issue | 🟠 MajorDon't report exhausted ETag conflicts as normal semaphore outcomes.
After the optimistic-concurrency loop gives up,
TryAcquireAsyncreturnsfalse,ReleaseAsynconly logs, andUpdateHeartbeatAsyncreturnsfalse.SemaphoreService.ExecuteWithRetryAsynconly retries exceptions, so repeated 412s are misreported as "full", "released", or "lost slot" instead of transient blob write contention.Also applies to: 145-167, 196-220, 429-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs` around lines 62 - 117, The optimistic-concurrency loops in TryAcquireAsync (and similarly in ReleaseAsync and UpdateHeartbeatAsync) currently swallow persistent ETag conflict (412 PreconditionFailed) failures and return false/log instead of signaling a transient contention; modify these methods to detect when all retry attempts failed due to ETag/precondition conflicts from TryWriteSemaphoreDataAsync (or the underlying RequestFailedException.Status==412) and, after the retry loop, rethrow a transient exception (e.g., a specific ConcurrencyConflictException or rethrow the original RequestFailedException) instead of returning normal false/log messages so SemaphoreService.ExecuteWithRetryAsync can retry; update the logic in TryAcquireAsync, ReleaseAsync, and UpdateHeartbeatAsync to track if any write failure was a 412 and raise that exception when retries are exhausted.tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs (1)
13-15:⚠️ Potential issue | 🟠 MajorRemove the hardcoded SQL Server fallback.
This still embeds a real password in source, and
??also accepts an empty environment variable. Fail fast on null/whitespaceMULTILOCK_SQLSERVER_CONNECTIONinstead of falling back to localhost credentials.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs` around lines 13 - 15, The ConnectionString constant currently falls back to a hardcoded SQL Server credential; replace this by reading MULTILOCK_SQLSERVER_CONNECTION into a variable (using Environment.GetEnvironmentVariable("MULTILOCK_SQLSERVER_CONNECTION")), validate it with string.IsNullOrWhiteSpace, and if it's null/empty/whitespace throw an InvalidOperationException (or similar) with a clear message requiring the env var; update references to use the validated variable instead of the hardcoded fallback so no credentials are embedded in source.src/MultiLock/SemaphoreService.cs (1)
83-103:⚠️ Potential issue | 🔴 CriticalDon't dispose
stateLockwhile public methods can still release it.The wait loop only tracks timer callbacks.
StartAsync,StopAsync,TryAcquireAsync, andReleaseAsyncalso holdstateLock, so disposing it here can turn theirfinally { stateLock.Release(); }blocks intoObjectDisposedException.Also applies to: 107-139, 142-183, 351-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MultiLock/SemaphoreService.cs` around lines 83 - 103, The code disposes stateLock while public methods (StartAsync, StopAsync, TryAcquireAsync, ReleaseAsync) may still enter and call stateLock.Release(), causing ObjectDisposedException; fix by ensuring stateLock is only disposed after no public methods can access it: introduce/observe the existing disposed flag and only call stateLock.Dispose() in the final Dispose/DisposeAsync path after taking stateLock to quiesce operations (or after stopping timers and setting AutoStart/state so no new callers run), and remove any intermediate disposals (from the timer/callback paths) so that stateLock.Dispose() happens exactly once when the service is fully shut down and no methods will call ReleaseAsync/Release on stateLock. Reference symbols: stateLock, StartAsync, StopAsync, TryAcquireAsync, ReleaseAsync, Dispose/DisposeAsync, heartbeatTimer, acquisitionTimer.src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs (1)
57-60:⚠️ Potential issue | 🟠 MajorUse PostgreSQL time for heartbeats and expiry.
These paths stamp
last_heartbeatwith each caller'sUtcNowand compare expiry against each caller's local clock. In a distributed semaphore, clock skew between nodes can expire a live slot early or keep a dead slot counted longer. Move the timestamping and expiry math into SQL withCURRENT_TIMESTAMP/CURRENT_TIMESTAMP -@slotTimeout``.Also applies to: 83-92, 96-108, 142-153, 246-259, 297-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs` around lines 57 - 60, The code currently uses DateTimeOffset.UtcNow and local expiry math when stamping last_heartbeat and checking expiry; change those to use PostgreSQL server time by setting last_heartbeat = CURRENT_TIMESTAMP in your INSERT/UPDATE SQL and perform expiry comparisons in SQL (e.g., WHERE last_heartbeat < CURRENT_TIMESTAMP - `@slotTimeout` OR WHERE CURRENT_TIMESTAMP - `@slotTimeout` < last_heartbeat) so all time math happens in the database. Update the SQL used in methods of PostgreSqlSemaphoreProvider (e.g., the Acquire/Renew/Release/Cleanup SQL statements and any SELECTs that filter by expiry) to accept slotTimeout as an interval parameter and replace client-side DateTimeOffset variables (now, expiryTime) with database-side CURRENT_TIMESTAMP arithmetic; remove or stop using the local now/expiryTime variables in those code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MultiLock.slnx`:
- Around line 12-16: The Project Path entry in the solution XML uses
inconsistent path separators (forward slashes) compared to the other Project
Path entries; update the Project Path value in the <Project
Path="samples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csproj" />
element to use backslashes to match the other entries (i.e.,
samples\MultiLock.SemaphoreSample\MultiLock.SemaphoreSample.csproj) so all
<Project Path="..."> attributes are consistent.
In `@src/MultiLock/SemaphoreOptions.cs`:
- Around line 84-114: Refactor SemaphoreOptions.Validate to delegate its
validation to the shared ParameterValidation.Validate so all common rules
(including HolderId and Metadata checks) are applied consistently: remove the
duplicated checks in SemaphoreOptions.Validate and call
ParameterValidation.Validate(this) (or the appropriate
ParameterValidation.Validate overload) so HolderId and Metadata are validated
centrally along with the existing timing/count constraints; keep any
semaphore-specific validations only if they are unique and not already handled
by ParameterValidation.
In `@src/MultiLock/SemaphoreService.cs`:
- Around line 130-135: StopAsync currently completes broadcastChannel but leaves
per-subscriber channels open so callers of WaitForSlotAsync and
GetStatusChangesAsync can hang; modify StopAsync (the method shutting down the
service) to also complete/close all per-subscriber channels (the collection used
to hold subscriber ChannelWriter/Reader instances) immediately when stopping
rather than waiting for DisposeAsync, by iterating the subscriber channel
collection and calling TryComplete/Complete on each subscriber channel writer
(or otherwise signaling their readers) so blocked callers unblock; ensure you
reference the same channel objects used by WaitForSlotAsync and
GetStatusChangesAsync and avoid disposing timers until after these channels are
completed.
In `@src/MultiLock/SemaphoreStatus.cs`:
- Around line 21-26: The Unknown(maxCount) factory currently sets CurrentCount=0
which makes AvailableSlots and HasAvailableSlots report capacity; change the
representation so "unknown" is explicit and conservative: add an IsUnknown flag
(or make CurrentCount nullable) on SemaphoreStatus and update
AvailableSlots/HasAvailableSlots to return 0/false when IsUnknown (or
CurrentCount is null), and ensure Unknown(...) constructs a status with
IsUnknown=true; also update the other Unknown() overloads/uses (the other
occurrence referenced) to follow the same pattern so no availability is reported
until a real provider read sets IsUnknown=false and a real CurrentCount.
In
`@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cs`:
- Around line 35-45: The Validate method in AzureBlobStorageSemaphoreOptions
currently checks ConnectionString, ContainerName non-emptiness, and
MaxRetryAttempts but lacks Azure container name format validation; update
AzureBlobStorageSemaphoreOptions.Validate to additionally verify ContainerName
follows Azure rules (3-63 characters, lowercase letters, numbers and hyphens
only, starts and ends with a letter or number, no consecutive hyphens or
leading/trailing hyphens) and throw an ArgumentException with a clear message
referencing ContainerName when invalid; keep existing checks for
ConnectionString and MaxRetryAttempts and include the container-format check
before returning so consumers get an early, descriptive error from Validate().
In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs`:
- Around line 169-174: The Consul provider currently wraps
OperationCanceledException in SemaphoreProviderException which breaks caller
cancellation; update each generic catch filter in ConsulSemaphoreProvider (the
catch blocks that currently read like 'catch (Exception ex) when (ex is not
SemaphoreException)') to also exclude OperationCanceledException (e.g., change
the filter to 'when (ex is not SemaphoreException && ex is not
OperationCanceledException)'). Apply the same change to the other five catch
blocks flagged in the review so OperationCanceledException is re-thrown
unwrapped.
In `@src/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cs`:
- Around line 36-43: The code calls ThrowIfDisposed() before entering the
critical sections guarded by lock(`@lock`), which allows a Dispose() to run while
the caller is waiting and then re-enter a locked section on a disposed provider;
fix each affected critical section (the blocks that lock on `@lock` and manipulate
semaphores) by re-validating disposal inside the lock — e.g., immediately after
acquiring lock(`@lock`) call ThrowIfDisposed() again or check isDisposed and throw
ObjectDisposedException/return as appropriate so no code mutates semaphores
after Dispose() has run; apply this change to every lock block that currently
does pre-lock ThrowIfDisposed() (the blocks around semaphores usage referenced
in the review).
In `@src/Providers/MultiLock.Redis/RedisSemaphoreOptions.cs`:
- Around line 29-39: The Validate method in RedisSemaphoreOptions should also
enforce a safe-character rule for KeyPrefix: update
RedisSemaphoreOptions.Validate() to check KeyPrefix against a whitelist regex
(e.g. start with letter or digit and allow letters, digits, underscores, colons
and hyphens like ^[A-Za-z0-9][A-Za-z0-9_: -]*$ or a stricter variant) and throw
an ArgumentException with a clear message if it fails; reference the KeyPrefix
property and the Validate method when adding this check so callers get
consistent validation similar to the SQL providers.
In `@src/Providers/MultiLock.Redis/RedisSemaphoreProvider.cs`:
- Around line 126-131: The catch-all Exception handlers in
RedisSemaphoreProvider (methods AcquireAsync, ReleaseAsync,
UpdateHeartbeatAsync, GetCurrentCountAsync, GetHoldersAsync, IsHoldingAsync) are
wrapping OperationCanceledException and breaking cancellation propagation;
change each catch to rethrow OperationCanceledException unchanged (e.g., add an
explicit catch (OperationCanceledException) { throw; }) before the general
catch(Exception ex) so cancellation flows up, then keep logging/wrapping other
exceptions as SemaphoreProviderException for the remaining catch blocks.
In `@src/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cs`:
- Around line 242-252: GetCurrentCountAsync in ZooKeeperSemaphoreProvider
(method GetCurrentCountAsync) currently ignores slotTimeout and simply counts
ephemeral children, which can diverge from other providers that filter stale
holders by heartbeat age; update the public API/docs (e.g., the
IMultiLockSemaphoreProvider/GetCurrentCountAsync XML doc or interface-level
documentation) to explicitly state that the ZooKeeper implementation counts live
ephemeral sessions and does not apply slotTimeout/heartbeat filtering so callers
understand provider-specific semantics, and add a one-line remark in
ZooKeeperSemaphoreProvider's method comment to reference this documented
behavior.
In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs`:
- Around line 97-98: The precondition TryAcquireAsync calls
(provider.TryAcquireAsync(semaphoreName, "holder-1", ...),
provider.TryAcquireAsync(semaphoreName, "holder-2", ...), etc.) are currently
discarding their boolean results; update each of these setup invocations to
assert the return value (e.g., Assert.True or equivalent) so the test fails at
the setup step if acquisition fails. Locate all occurrences of TryAcquireAsync
used as preconditions (including the calls with "holder-1"/"holder-2" and the
other listed occurrences) and wrap them in assertions that verify they returned
true, keeping the same parameters and timeouts.
In `@tests/MultiLock.Tests/SemaphoreAcquisitionTests.cs`:
- Around line 99-134: The test calls provider.TryAcquireAsync(SemaphoreName,
HolderId, maxCount, metadata, TimeSpan.FromMinutes(5)) to set up the acquisition
but never asserts its return value; add an assertion immediately after that call
to ensure the initial acquire succeeded (e.g., assert the boolean is true)
before creating the acquisition via CreateAcquisition() so the test precondition
is validated and subsequent DisposeAsync() concurrency checks are meaningful.
In `@tests/MultiLock.Tests/SemaphoreConcurrencyTests.cs`:
- Around line 161-165: The loop always calls ReleaseAsync even when
TryAcquireAsync may fail; change it to capture the boolean result of
provider.TryAcquireAsync into a variable (e.g., acquired) and call
provider.ReleaseAsync(semaphoreName, holderId, cts.Token) only when acquired is
true; additionally assert that at least one acquire succeeded across the loop
(monitor a progress counter or assert acquired was true at least once) to ensure
forward progress; update references around provider.TryAcquireAsync,
provider.ReleaseAsync, semaphoreName, holderId, maxCount, metadata, slotTimeout,
cts.Token and cycles accordingly.
- Around line 36-40: The tests currently call provider methods directly (e.g.,
TryAcquireAsync, ReleaseAsync) which execute synchronously under locks and
return completed Tasks, so build the task list by scheduling the calls on
thread-pool threads to force real concurrency; change each Select(...) that
currently does provider.TryAcquireAsync(...) (and similar blocks for
ReleaseAsync, TryAcquireManyAsync, etc.) to wrap the provider call in Task.Run
or Task.Factory.StartNew so that the provider method executes on a worker thread
and returns the Task it produces, ensuring true parallel contention for the
semaphore; apply this pattern to all affected sites (the TryAcquireAsync,
ReleaseAsync and other provider method invocations listed in the review).
In `@tests/MultiLock.Tests/SemaphoreServiceTests.cs`:
- Around line 114-123: The three test methods HolderId_ShouldBeSetFromOptions,
SemaphoreName_ShouldBeSetFromOptions, and MaxCount_ShouldBeSetFromOptions in
SemaphoreServiceTests are marked async but contain no awaits; remove the
unnecessary async modifier (and the returned Task if you prefer, leaving the
method as a synchronous Fact) or alternatively add an awaited noop (e.g., await
Task.CompletedTask) inside each test to suppress CS1998 — update the method
signatures for those named tests accordingly so the compiler warning is
eliminated.
---
Duplicate comments:
In `@samples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cs`:
- Around line 47-59: The acquired-slot branch in RateLimitedWorkerService
(around SimulateApiCallAsync and semaphoreService.ReleaseAsync) must guarantee
release: move the ReleaseAsync call into a finally block so the slot is always
released even if SimulateApiCallAsync throws or is canceled, and do not pass the
stoppingToken into semaphoreService.ReleaseAsync (call ReleaseAsync without a
cancellation token) so cleanup cannot be canceled; ensure the logger messages
remain but that the release logging occurs after the awaited ReleaseAsync in the
finally.
- Line 35: Fire-and-forget of MonitorStatusChangesAsync(stoppingToken) hides
exceptions; capture the returned Task (e.g., monitorTask =
MonitorStatusChangesAsync(stoppingToken)) and observe faults instead of
discarding it: either await it where appropriate or attach a
continuation/try-catch to log and rethrow or swallow exceptions (e.g.,
monitorTask.ContinueWith(..., OnlyOnFaulted) or wrap in Task.Run with try/catch
and use the service logger). Reference MonitorStatusChangesAsync and the
stoppingToken so the task is observed and exceptions are logged.
In `@src/MultiLock/ISemaphoreProvider.cs`:
- Around line 64-84: GetHoldersAsync and IsHoldingAsync are inconsistent with
GetCurrentCountAsync because they lack the slotTimeout expiry filter; add a
CancellationToken-optional TimeSpan slotTimeout parameter to both
GetHoldersAsync and IsHoldingAsync (matching signature style of
GetCurrentCountAsync), update their XML docs to state that results are filtered
by slotTimeout (expired holders are ignored), and ensure implementations that
call these interface methods use the slotTimeout to filter out holders by
heartbeat age just like GetCurrentCountAsync does; reference the methods
GetHoldersAsync, IsHoldingAsync and GetCurrentCountAsync when updating
interface, docs, and implementations.
In `@src/MultiLock/ParameterValidation.cs`:
- Around line 11-12: Replace the hard-coded universal maxSqlIdentifierLength
(and its use in ParameterValidation methods around the former lines 177-185)
with a provider-specific limit passed into the validator from options/connection
info; update the signature of the validator entry point (e.g.,
ValidateOptions/ValidateIdentifiers or the ParameterValidation class
constructor) to accept an int identifierMaxLength and use that instead of
maxSqlIdentifierLength, and retain maxMetadataKeyLength for metadata; for
reference enforce PostgreSQL identifiers to 63 and SQL Server identifiers to 128
when calling the validator so provider-specific truncation is correctly handled.
In `@src/MultiLock/SemaphoreAcquisition.cs`:
- Line 10: The current dispose/check pattern using the volatile field isDisposed
is racy and can allow concurrent double-release; replace the check/set with an
atomic operation (e.g., use Interlocked.Exchange or CompareExchange) to ensure
only one thread wins the dispose path in ReleaseAsync and any
Dispose/DisposeAsync logic. Specifically, update the ReleaseAsync method and any
Dispose-related code that reads/writes isDisposed to atomically set the flag and
only proceed with release logic when the exchange indicates the caller is the
first to flip the flag (use the unique symbol isDisposed and method ReleaseAsync
to locate the code).
- Around line 81-84: The current Dispose() blocks on DisposeAsync() which can
deadlock; change it to call
DisposeAsync().AsTask().ConfigureAwait(false).GetAwaiter().GetResult() so the
async continuation doesn't capture the ambient synchronization context—update
the SemaphoreAcquisition.Dispose() implementation to use
AsTask().ConfigureAwait(false).GetAwaiter().GetResult() instead of plain
AsTask().GetAwaiter().GetResult().
In `@src/MultiLock/SemaphoreService.cs`:
- Around line 83-103: The code disposes stateLock while public methods
(StartAsync, StopAsync, TryAcquireAsync, ReleaseAsync) may still enter and call
stateLock.Release(), causing ObjectDisposedException; fix by ensuring stateLock
is only disposed after no public methods can access it: introduce/observe the
existing disposed flag and only call stateLock.Dispose() in the final
Dispose/DisposeAsync path after taking stateLock to quiesce operations (or after
stopping timers and setting AutoStart/state so no new callers run), and remove
any intermediate disposals (from the timer/callback paths) so that
stateLock.Dispose() happens exactly once when the service is fully shut down and
no methods will call ReleaseAsync/Release on stateLock. Reference symbols:
stateLock, StartAsync, StopAsync, TryAcquireAsync, ReleaseAsync,
Dispose/DisposeAsync, heartbeatTimer, acquisitionTimer.
In `@src/MultiLock/SemaphoreServiceExtensions.cs`:
- Around line 27-29: The hard cast
(SemaphoreService)provider.GetRequiredService<ISemaphoreService>() can throw if
a different ISemaphoreService implementation was pre-registered; instead ensure
the concrete SemaphoreService is registered and reused: register
SemaphoreService as the singleton implementation
(TryAddSingleton<SemaphoreService> / add concrete), then map the interface to
that same instance (TryAddSingleton<ISemaphoreService>(provider =>
provider.GetRequiredService<SemaphoreService>())), and resolve the hosted
service from the concrete SemaphoreService when calling AddHostedService so no
invalid cast occurs; update the registration calls around ISemaphoreService and
SemaphoreService accordingly.
In
`@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cs`:
- Around line 62-117: The optimistic-concurrency loops in TryAcquireAsync (and
similarly in ReleaseAsync and UpdateHeartbeatAsync) currently swallow persistent
ETag conflict (412 PreconditionFailed) failures and return false/log instead of
signaling a transient contention; modify these methods to detect when all retry
attempts failed due to ETag/precondition conflicts from
TryWriteSemaphoreDataAsync (or the underlying
RequestFailedException.Status==412) and, after the retry loop, rethrow a
transient exception (e.g., a specific ConcurrencyConflictException or rethrow
the original RequestFailedException) instead of returning normal false/log
messages so SemaphoreService.ExecuteWithRetryAsync can retry; update the logic
in TryAcquireAsync, ReleaseAsync, and UpdateHeartbeatAsync to track if any write
failure was a 412 and raise that exception when retries are exhausted.
In `@src/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cs`:
- Around line 255-269: GetCurrentCountAsync ignores the caller's slotTimeout and
simply lists KV entries; update it to honor slotTimeout by either filtering
entries by their remaining TTL or validating slotTimeout against the provider's
configured session TTL and failing fast. Locate GetCurrentCountAsync and use
GetSemaphorePrefix and consulClient.KV.List to fetch candidates, then: if the
provider uses a fixed SessionTtl, compare the provided slotTimeout to SessionTtl
and throw/return an error when they differ; otherwise compute remaining lifetime
for each KV/session and only count entries whose remaining TTL >= slotTimeout
(or whose session expiry would cover the requested slotTimeout) so
TryAcquireAsync's pre-check and rollback use a consistent timeout expectation.
Ensure ThrowIfDisposed and ParameterValidation paths remain unchanged.
In `@src/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cs`:
- Around line 112-117: The catch-all in
FileSystemSemaphoreProvider.TryAcquireAsync is wrapping
OperationCanceledException thrown by AcquireLockFileAsync and turning
cancellations into SemaphoreProviderException; change the catch in
TryAcquireAsync so that if the caught exception is OperationCanceledException or
TaskCanceledException (or the token signals cancellation) you rethrow the
original exception immediately, otherwise log via logger.LogError and throw new
SemaphoreProviderException("Failed to acquire semaphore slot", ex). Apply the
same change to the similar block covering lines 122-134 so cancellation can
escape instead of being treated as provider failure.
In `@src/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cs`:
- Around line 57-60: The code currently uses DateTimeOffset.UtcNow and local
expiry math when stamping last_heartbeat and checking expiry; change those to
use PostgreSQL server time by setting last_heartbeat = CURRENT_TIMESTAMP in your
INSERT/UPDATE SQL and perform expiry comparisons in SQL (e.g., WHERE
last_heartbeat < CURRENT_TIMESTAMP - `@slotTimeout` OR WHERE CURRENT_TIMESTAMP -
`@slotTimeout` < last_heartbeat) so all time math happens in the database. Update
the SQL used in methods of PostgreSqlSemaphoreProvider (e.g., the
Acquire/Renew/Release/Cleanup SQL statements and any SELECTs that filter by
expiry) to accept slotTimeout as an interval parameter and replace client-side
DateTimeOffset variables (now, expiryTime) with database-side CURRENT_TIMESTAMP
arithmetic; remove or stop using the local now/expiryTime variables in those
code paths.
In `@tests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cs`:
- Around line 33-153: The tests lack a cross-process contention case; add an
integration test that starts a helper process which constructs a
FileSystemSemaphoreProvider with the same Options.Create(options).DirectoryPath
and attempts multiple TryAcquireAsync calls, then assert that across both
processes the total successful acquires never exceeds the configured maxCount;
implement this by adding a new test method (e.g.,
CrossProcess_Contention_ShouldRespectMaxCount) that (1) creates options and a
provider in the test process, (2) spawns a child process (same test binary or a
small helper executable) passing the DirectoryPath and desired attempts, (3)
have the child use FileSystemSemaphoreProvider.TryAcquireAsync to attempt N
acquires and return the number of successes, (4) wait for the child to exit and
parse its result, and (5) assert that successes in parent + child <= maxCount;
reuse existing symbols FileSystemSemaphoreProvider, TryAcquireAsync,
CreateOptions, and DirectoryPath to locate where to hook this behavior.
In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs`:
- Around line 13-15: The hardcoded fallback in the static readonly
connectionString (used by PostgreSQLSemaphoreIntegrationTests) leaks credentials
and ignores empty env vars; change to read the env var into connectionString
without a hardcoded default, then validate with string.IsNullOrWhiteSpace and
throw a clear startup exception (e.g., InvalidOperationException) naming the
missing MULTILOCK_POSTGRES_CONNECTION so tests fail fast and don’t use localhost
credentials.
In `@tests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cs`:
- Around line 13-15: The ConnectionString constant currently falls back to a
hardcoded SQL Server credential; replace this by reading
MULTILOCK_SQLSERVER_CONNECTION into a variable (using
Environment.GetEnvironmentVariable("MULTILOCK_SQLSERVER_CONNECTION")), validate
it with string.IsNullOrWhiteSpace, and if it's null/empty/whitespace throw an
InvalidOperationException (or similar) with a clear message requiring the env
var; update references to use the validated variable instead of the hardcoded
fallback so no credentials are embedded in source.
In `@tests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cs`:
- Around line 46-47: The test setup calls to provider.TryAcquireAsync in
FileSystemSemaphoreProviderTests currently ignore their boolean results, which
can hide setup failures and produce false-positive assertions; update each setup
call (e.g., the initial TryAcquireAsync calls at the start of tests around lines
where provider.TryAcquireAsync("sem1", ... ) is used) to capture and assert the
returned bool is true (use Assert.True or equivalent) before proceeding, so that
any failure to acquire during test setup fails the test immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5409e9d-589c-4d5d-be97-452b5b1831ef
📒 Files selected for processing (77)
MultiLock.slnxREADME.mdsamples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csprojsamples/MultiLock.SemaphoreSample/Program.cssamples/MultiLock.SemaphoreSample/RateLimitedWorkerService.cssrc/MultiLock/Exceptions/SemaphoreException.cssrc/MultiLock/Exceptions/SemaphoreFullException.cssrc/MultiLock/Exceptions/SemaphoreProviderException.cssrc/MultiLock/Exceptions/SemaphoreTimeoutException.cssrc/MultiLock/ISemaphoreProvider.cssrc/MultiLock/ISemaphoreService.cssrc/MultiLock/MultiLock.csprojsrc/MultiLock/ParameterValidation.cssrc/MultiLock/SemaphoreAcquisition.cssrc/MultiLock/SemaphoreChangedEventArgs.cssrc/MultiLock/SemaphoreHolder.cssrc/MultiLock/SemaphoreInfo.cssrc/MultiLock/SemaphoreOptions.cssrc/MultiLock/SemaphoreService.cssrc/MultiLock/SemaphoreServiceExtensions.cssrc/MultiLock/SemaphoreStatus.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreProvider.cssrc/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageServiceCollectionExtensions.cssrc/Providers/MultiLock.AzureBlobStorage/MultiLock.AzureBlobStorage.csprojsrc/Providers/MultiLock.Consul/ConsulLeaderElectionOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreOptions.cssrc/Providers/MultiLock.Consul/ConsulSemaphoreProvider.cssrc/Providers/MultiLock.Consul/ConsulServiceCollectionExtensions.cssrc/Providers/MultiLock.Consul/MultiLock.Consul.csprojsrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreOptions.cssrc/Providers/MultiLock.FileSystem/FileSystemSemaphoreProvider.cssrc/Providers/MultiLock.FileSystem/FileSystemServiceCollectionExtensions.cssrc/Providers/MultiLock.FileSystem/HolderFileContent.cssrc/Providers/MultiLock.FileSystem/MultiLock.FileSystem.csprojsrc/Providers/MultiLock.InMemory/InMemorySemaphoreProvider.cssrc/Providers/MultiLock.InMemory/InMemoryServiceCollectionExtensions.cssrc/Providers/MultiLock.InMemory/MultiLock.InMemory.csprojsrc/Providers/MultiLock.InMemory/SemaphoreSlotRecord.cssrc/Providers/MultiLock.PostgreSQL/MultiLock.PostgreSQL.csprojsrc/Providers/MultiLock.PostgreSQL/PostgreSQLServiceCollectionExtensions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreOptions.cssrc/Providers/MultiLock.PostgreSQL/PostgreSqlSemaphoreProvider.cssrc/Providers/MultiLock.Redis/MultiLock.Redis.csprojsrc/Providers/MultiLock.Redis/RedisSemaphoreOptions.cssrc/Providers/MultiLock.Redis/RedisSemaphoreProvider.cssrc/Providers/MultiLock.Redis/RedisServiceCollectionExtensions.cssrc/Providers/MultiLock.SqlServer/MultiLock.SqlServer.csprojsrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreOptions.cssrc/Providers/MultiLock.SqlServer/SqlServerSemaphoreProvider.cssrc/Providers/MultiLock.SqlServer/SqlServerServiceCollectionExtensions.cssrc/Providers/MultiLock.ZooKeeper/MultiLock.ZooKeeper.csprojsrc/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperLeaderElectionProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreOptions.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperSemaphoreProvider.cssrc/Providers/MultiLock.ZooKeeper/ZooKeeperServiceCollectionExtensions.cstests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ConsulSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/FileSystemSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/RedisSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/SqlServerSemaphoreIntegrationTests.cstests/MultiLock.IntegrationTests/ZooKeeperSemaphoreIntegrationTests.cstests/MultiLock.Tests/FileSystemSemaphoreOptionsTests.cstests/MultiLock.Tests/FileSystemSemaphoreProviderTests.cstests/MultiLock.Tests/FileSystemServiceCollectionExtensionsTests.cstests/MultiLock.Tests/InMemorySemaphoreProviderTests.cstests/MultiLock.Tests/InMemoryServiceCollectionExtensionsTests.cstests/MultiLock.Tests/SemaphoreAcquisitionTests.cstests/MultiLock.Tests/SemaphoreConcurrencyTests.cstests/MultiLock.Tests/SemaphoreExceptionTests.cstests/MultiLock.Tests/SemaphoreModelTests.cstests/MultiLock.Tests/SemaphoreOptionsValidationTests.cstests/MultiLock.Tests/SemaphoreParameterValidationTests.cstests/MultiLock.Tests/SemaphoreServiceTests.cstests/MultiLock.Tests/ServiceExtensionsValidationTests.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-and-test
- GitHub Check: Analyze (csharp)
🧰 Additional context used
🪛 Betterleaks (1.1.1)
tests/MultiLock.IntegrationTests/AzureBlobStorageSemaphoreIntegrationTests.cs
[high] 12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Comment on lines
12
to
16
| <Folder Name="/Samples/"> | ||
| <Project Path="samples\MultiLock.MultiProvider\MultiLock.MultiProvider.csproj" Type="Classic C#" /> | ||
| <Project Path="samples\MultiLock.Sample\MultiLock.Sample.csproj" Type="Classic C#" /> | ||
| <Project Path="samples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csproj" /> | ||
| <Project Path="samples\MultiLock.MultiProvider\MultiLock.MultiProvider.csproj" Type="C#" /> | ||
| <Project Path="samples\MultiLock.Sample\MultiLock.Sample.csproj" Type="C#" /> | ||
| </Folder> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent path separators in solution file.
Line 13 uses forward slashes (samples/MultiLock.SemaphoreSample/...) while other project paths use backslashes (samples\MultiLock.MultiProvider\...). Consider normalizing to backslashes for consistency with the rest of the solution file.
Suggested fix
<Folder Name="/Samples/">
- <Project Path="samples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csproj" />
+ <Project Path="samples\MultiLock.SemaphoreSample\MultiLock.SemaphoreSample.csproj" Type="C#" />
<Project Path="samples\MultiLock.MultiProvider\MultiLock.MultiProvider.csproj" Type="C#" />
<Project Path="samples\MultiLock.Sample\MultiLock.Sample.csproj" Type="C#" />
</Folder>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MultiLock.slnx` around lines 12 - 16, The Project Path entry in the solution
XML uses inconsistent path separators (forward slashes) compared to the other
Project Path entries; update the Project Path value in the <Project
Path="samples/MultiLock.SemaphoreSample/MultiLock.SemaphoreSample.csproj" />
element to use backslashes to match the other entries (i.e.,
samples\MultiLock.SemaphoreSample\MultiLock.SemaphoreSample.csproj) so all
<Project Path="..."> attributes are consistent.
Comment on lines
+84
to
+114
| { | ||
| if (string.IsNullOrWhiteSpace(SemaphoreName)) | ||
| throw new ArgumentException("Semaphore name cannot be null or empty.", nameof(SemaphoreName)); | ||
|
|
||
| if (MaxCount < 1) | ||
| throw new ArgumentException("Max count must be at least 1.", nameof(MaxCount)); | ||
|
|
||
| if (HeartbeatInterval <= TimeSpan.Zero) | ||
| throw new ArgumentException("Heartbeat interval must be positive.", nameof(HeartbeatInterval)); | ||
|
|
||
| if (HeartbeatTimeout <= TimeSpan.Zero) | ||
| throw new ArgumentException("Heartbeat timeout must be positive.", nameof(HeartbeatTimeout)); | ||
|
|
||
| if (HeartbeatTimeout <= HeartbeatInterval) | ||
| throw new ArgumentException("Heartbeat timeout must be greater than heartbeat interval.", nameof(HeartbeatTimeout)); | ||
|
|
||
| if (AcquisitionInterval <= TimeSpan.Zero) | ||
| throw new ArgumentException("Acquisition interval must be positive.", nameof(AcquisitionInterval)); | ||
|
|
||
| if (MaxRetryAttempts < 0) | ||
| throw new ArgumentException("Max retry attempts cannot be negative.", nameof(MaxRetryAttempts)); | ||
|
|
||
| if (RetryBaseDelay <= TimeSpan.Zero) | ||
| throw new ArgumentException("Retry base delay must be positive.", nameof(RetryBaseDelay)); | ||
|
|
||
| if (RetryMaxDelay <= TimeSpan.Zero) | ||
| throw new ArgumentException("Retry max delay must be positive.", nameof(RetryMaxDelay)); | ||
|
|
||
| if (RetryMaxDelay < RetryBaseDelay) | ||
| throw new ArgumentException("Retry max delay must be greater than or equal to retry base delay.", nameof(RetryMaxDelay)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Centralize Validate() on ParameterValidation to prevent rule drift.
Line 85 onward re-implements checks but does not validate HolderId/Metadata. Reusing shared validators keeps behavior consistent and fails fast at options validation time.
♻️ Proposed refactor
public void Validate()
{
- if (string.IsNullOrWhiteSpace(SemaphoreName))
- throw new ArgumentException("Semaphore name cannot be null or empty.", nameof(SemaphoreName));
+ ParameterValidation.ValidateSemaphoreName(SemaphoreName, nameof(SemaphoreName));
- if (MaxCount < 1)
- throw new ArgumentException("Max count must be at least 1.", nameof(MaxCount));
+ ParameterValidation.ValidateMaxCount(MaxCount, nameof(MaxCount));
- if (HeartbeatInterval <= TimeSpan.Zero)
- throw new ArgumentException("Heartbeat interval must be positive.", nameof(HeartbeatInterval));
+ ParameterValidation.ValidateSlotTimeout(HeartbeatInterval, nameof(HeartbeatInterval));
- if (HeartbeatTimeout <= TimeSpan.Zero)
- throw new ArgumentException("Heartbeat timeout must be positive.", nameof(HeartbeatTimeout));
+ ParameterValidation.ValidateSlotTimeout(HeartbeatTimeout, nameof(HeartbeatTimeout));
if (HeartbeatTimeout <= HeartbeatInterval)
throw new ArgumentException("Heartbeat timeout must be greater than heartbeat interval.", nameof(HeartbeatTimeout));
- if (AcquisitionInterval <= TimeSpan.Zero)
- throw new ArgumentException("Acquisition interval must be positive.", nameof(AcquisitionInterval));
+ ParameterValidation.ValidateSlotTimeout(AcquisitionInterval, nameof(AcquisitionInterval));
+ if (!string.IsNullOrWhiteSpace(HolderId))
+ ParameterValidation.ValidateHolderId(HolderId, nameof(HolderId));
+
+ ParameterValidation.ValidateMetadata(Metadata, nameof(Metadata));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MultiLock/SemaphoreOptions.cs` around lines 84 - 114, Refactor
SemaphoreOptions.Validate to delegate its validation to the shared
ParameterValidation.Validate so all common rules (including HolderId and
Metadata checks) are applied consistently: remove the duplicated checks in
SemaphoreOptions.Validate and call ParameterValidation.Validate(this) (or the
appropriate ParameterValidation.Validate overload) so HolderId and Metadata are
validated centrally along with the existing timing/count constraints; keep any
semaphore-specific validations only if they are unique and not already handled
by ParameterValidation.
Comment on lines
+130
to
+135
| broadcastChannel.Writer.TryComplete(); | ||
| await broadcastTask.WaitAsync(cancellationToken); | ||
| await cancellationTokenSource.CancelAsync(); | ||
|
|
||
| await DisposeTimersAsync(); | ||
| } |
There was a problem hiding this comment.
Complete subscriber channels when the service stops.
StopAsync completes broadcastChannel, but existing WaitForSlotAsync / GetStatusChangesAsync callers are blocked on their per-subscriber channels, not on the broadcast reader. Those channels stay open until DisposeAsync, so a plain stop can leave waiters hanging indefinitely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MultiLock/SemaphoreService.cs` around lines 130 - 135, StopAsync
currently completes broadcastChannel but leaves per-subscriber channels open so
callers of WaitForSlotAsync and GetStatusChangesAsync can hang; modify StopAsync
(the method shutting down the service) to also complete/close all per-subscriber
channels (the collection used to hold subscriber ChannelWriter/Reader instances)
immediately when stopping rather than waiting for DisposeAsync, by iterating the
subscriber channel collection and calling TryComplete/Complete on each
subscriber channel writer (or otherwise signaling their readers) so blocked
callers unblock; ensure you reference the same channel objects used by
WaitForSlotAsync and GetStatusChangesAsync and avoid disposing timers until
after these channels are completed.
Comment on lines
+21
to
+26
| public int AvailableSlots => Math.Max(0, MaxCount - CurrentCount); | ||
|
|
||
| /// <summary> | ||
| /// Gets a value indicating whether the semaphore has available slots. | ||
| /// </summary> | ||
| public bool HasAvailableSlots => AvailableSlots > 0; |
There was a problem hiding this comment.
Unknown() currently reports free capacity.
Unknown(maxCount) sets CurrentCount to 0, so AvailableSlots becomes MaxCount and HasAvailableSlots becomes true even though the docs say no semaphore information is available. That can trigger false "slots available" decisions before the first provider read. Represent the unknown state explicitly, or at minimum make Unknown() conservative so availability stays false until a real count is observed.
Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MultiLock/SemaphoreStatus.cs` around lines 21 - 26, The Unknown(maxCount)
factory currently sets CurrentCount=0 which makes AvailableSlots and
HasAvailableSlots report capacity; change the representation so "unknown" is
explicit and conservative: add an IsUnknown flag (or make CurrentCount nullable)
on SemaphoreStatus and update AvailableSlots/HasAvailableSlots to return 0/false
when IsUnknown (or CurrentCount is null), and ensure Unknown(...) constructs a
status with IsUnknown=true; also update the other Unknown() overloads/uses (the
other occurrence referenced) to follow the same pattern so no availability is
reported until a real provider read sets IsUnknown=false and a real
CurrentCount.
Comment on lines
+35
to
+45
| public void Validate() | ||
| { | ||
| if (string.IsNullOrWhiteSpace(ConnectionString)) | ||
| throw new ArgumentException("Connection string cannot be null or empty.", nameof(ConnectionString)); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(ContainerName)) | ||
| throw new ArgumentException("Container name cannot be null or empty.", nameof(ContainerName)); | ||
|
|
||
| if (MaxRetryAttempts < 1) | ||
| throw new ArgumentException("Max retry attempts must be at least 1.", nameof(MaxRetryAttempts)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding Azure container name format validation.
Azure Blob container names have specific requirements (3-63 lowercase alphanumeric characters and hyphens, must start with letter or number). While the Azure SDK will reject invalid names at runtime, early validation in Validate() could provide clearer error messages.
♻️ Optional enhancement
if (string.IsNullOrWhiteSpace(ContainerName))
throw new ArgumentException("Container name cannot be null or empty.", nameof(ContainerName));
+
+ // Azure container names: 3-63 chars, lowercase alphanumeric and hyphens, start with letter/number
+ if (ContainerName.Length < 3 || ContainerName.Length > 63)
+ throw new ArgumentException("Container name must be between 3 and 63 characters.", nameof(ContainerName));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void Validate() | |
| { | |
| if (string.IsNullOrWhiteSpace(ConnectionString)) | |
| throw new ArgumentException("Connection string cannot be null or empty.", nameof(ConnectionString)); | |
| if (string.IsNullOrWhiteSpace(ContainerName)) | |
| throw new ArgumentException("Container name cannot be null or empty.", nameof(ContainerName)); | |
| if (MaxRetryAttempts < 1) | |
| throw new ArgumentException("Max retry attempts must be at least 1.", nameof(MaxRetryAttempts)); | |
| } | |
| public void Validate() | |
| { | |
| if (string.IsNullOrWhiteSpace(ConnectionString)) | |
| throw new ArgumentException("Connection string cannot be null or empty.", nameof(ConnectionString)); | |
| if (string.IsNullOrWhiteSpace(ContainerName)) | |
| throw new ArgumentException("Container name cannot be null or empty.", nameof(ContainerName)); | |
| // Azure container names: 3-63 chars, lowercase alphanumeric and hyphens, start with letter/number | |
| if (ContainerName.Length < 3 || ContainerName.Length > 63) | |
| throw new ArgumentException("Container name must be between 3 and 63 characters.", nameof(ContainerName)); | |
| if (MaxRetryAttempts < 1) | |
| throw new ArgumentException("Max retry attempts must be at least 1.", nameof(MaxRetryAttempts)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Providers/MultiLock.AzureBlobStorage/AzureBlobStorageSemaphoreOptions.cs`
around lines 35 - 45, The Validate method in AzureBlobStorageSemaphoreOptions
currently checks ConnectionString, ContainerName non-emptiness, and
MaxRetryAttempts but lacks Azure container name format validation; update
AzureBlobStorageSemaphoreOptions.Validate to additionally verify ContainerName
follows Azure rules (3-63 characters, lowercase letters, numbers and hyphens
only, starts and ends with a letter or number, no consecutive hyphens or
leading/trailing hyphens) and throw an ArgumentException with a clear message
referencing ContainerName when invalid; keep existing checks for
ConnectionString and MaxRetryAttempts and include the container-format check
before returning so consumers get an early, descriptive error from Validate().
Comment on lines
+97
to
+98
| await provider.TryAcquireAsync(semaphoreName, "holder-1", maxCount, metadata, TimeSpan.FromMinutes(5)); | ||
| await provider.TryAcquireAsync(semaphoreName, "holder-2", maxCount, metadata, TimeSpan.FromMinutes(5)); |
There was a problem hiding this comment.
Assert the setup acquires in these tests.
These precondition calls currently ignore the return value. If one of them fails, the renew/release cases can still pass or fail at a later assertion, which hides the actual regression.
Also applies to: 115-115, 135-135, 153-153, 187-188, 206-207, 228-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/MultiLock.IntegrationTests/PostgreSQLSemaphoreIntegrationTests.cs`
around lines 97 - 98, The precondition TryAcquireAsync calls
(provider.TryAcquireAsync(semaphoreName, "holder-1", ...),
provider.TryAcquireAsync(semaphoreName, "holder-2", ...), etc.) are currently
discarding their boolean results; update each of these setup invocations to
assert the return value (e.g., Assert.True or equivalent) so the test fails at
the setup step if acquisition fails. Locate all occurrences of TryAcquireAsync
used as preconditions (including the calls with "holder-1"/"holder-2" and the
other listed occurrences) and wrap them in assertions that verify they returned
true, keeping the same parameters and timeouts.
Comment on lines
+99
to
+134
| [Fact] | ||
| public async Task DisposeAsync_ConcurrentCalls_ShouldNotDoubleRelease() | ||
| { | ||
| // Arrange: acquire a slot then fire many concurrent DisposeAsync calls. | ||
| // The volatile-bool guard has a read-then-write pattern that is not atomically | ||
| // compare-and-swap, so multiple callers may race past the isDisposed check. | ||
| // The test verifies that even in the worst case the semaphore ends up in a | ||
| // consistent state: the slot is released exactly once, the acquisition is | ||
| // marked disposed, and no exception escapes. | ||
| const string otherHolder = "holder-2"; | ||
| const int maxCount = 1; | ||
| const int concurrency = 8; | ||
|
|
||
| await provider.TryAcquireAsync(SemaphoreName, HolderId, maxCount, metadata, TimeSpan.FromMinutes(5)); | ||
| var acquisition = CreateAcquisition(); | ||
|
|
||
| // Act: Task.WhenAll rethrows the first faulted task; any unexpected exception | ||
| // here is a test failure (DisposeAsync is specified never to throw). | ||
| Task[] disposeTasks = Enumerable | ||
| .Range(0, concurrency) | ||
| .Select(_ => acquisition.DisposeAsync().AsTask()) | ||
| .ToArray(); | ||
| await Task.WhenAll(disposeTasks); | ||
|
|
||
| // Assert: the acquisition must be marked disposed regardless of which task won. | ||
| acquisition.IsDisposed.ShouldBeTrue(); | ||
|
|
||
| // The slot must be free: no phantom holders left from double-release races. | ||
| int count = await provider.GetCurrentCountAsync(SemaphoreName, TimeSpan.FromMinutes(5)); | ||
| count.ShouldBe(0, "holder-1 slot should be fully released after concurrent disposal"); | ||
|
|
||
| // A new holder should be able to acquire the now-free slot. | ||
| bool acquired = await provider.TryAcquireAsync( | ||
| SemaphoreName, otherHolder, maxCount, metadata, TimeSpan.FromMinutes(5)); | ||
| acquired.ShouldBeTrue("slot should be available for a new holder after concurrent disposal"); | ||
| } |
There was a problem hiding this comment.
Assert acquisition precondition in concurrent disposal test.
Line 112's TryAcquireAsync result should be asserted for test setup validation.
💚 Proposed fix
const int maxCount = 1;
const int concurrency = 8;
- await provider.TryAcquireAsync(SemaphoreName, HolderId, maxCount, metadata, TimeSpan.FromMinutes(5));
+ bool acquired = await provider.TryAcquireAsync(SemaphoreName, HolderId, maxCount, metadata, TimeSpan.FromMinutes(5));
+ acquired.ShouldBeTrue("setup: slot acquisition must succeed");
var acquisition = CreateAcquisition();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [Fact] | |
| public async Task DisposeAsync_ConcurrentCalls_ShouldNotDoubleRelease() | |
| { | |
| // Arrange: acquire a slot then fire many concurrent DisposeAsync calls. | |
| // The volatile-bool guard has a read-then-write pattern that is not atomically | |
| // compare-and-swap, so multiple callers may race past the isDisposed check. | |
| // The test verifies that even in the worst case the semaphore ends up in a | |
| // consistent state: the slot is released exactly once, the acquisition is | |
| // marked disposed, and no exception escapes. | |
| const string otherHolder = "holder-2"; | |
| const int maxCount = 1; | |
| const int concurrency = 8; | |
| await provider.TryAcquireAsync(SemaphoreName, HolderId, maxCount, metadata, TimeSpan.FromMinutes(5)); | |
| var acquisition = CreateAcquisition(); | |
| // Act: Task.WhenAll rethrows the first faulted task; any unexpected exception | |
| // here is a test failure (DisposeAsync is specified never to throw). | |
| Task[] disposeTasks = Enumerable | |
| .Range(0, concurrency) | |
| .Select(_ => acquisition.DisposeAsync().AsTask()) | |
| .ToArray(); | |
| await Task.WhenAll(disposeTasks); | |
| // Assert: the acquisition must be marked disposed regardless of which task won. | |
| acquisition.IsDisposed.ShouldBeTrue(); | |
| // The slot must be free: no phantom holders left from double-release races. | |
| int count = await provider.GetCurrentCountAsync(SemaphoreName, TimeSpan.FromMinutes(5)); | |
| count.ShouldBe(0, "holder-1 slot should be fully released after concurrent disposal"); | |
| // A new holder should be able to acquire the now-free slot. | |
| bool acquired = await provider.TryAcquireAsync( | |
| SemaphoreName, otherHolder, maxCount, metadata, TimeSpan.FromMinutes(5)); | |
| acquired.ShouldBeTrue("slot should be available for a new holder after concurrent disposal"); | |
| } | |
| [Fact] | |
| public async Task DisposeAsync_ConcurrentCalls_ShouldNotDoubleRelease() | |
| { | |
| // Arrange: acquire a slot then fire many concurrent DisposeAsync calls. | |
| // The volatile-bool guard has a read-then-write pattern that is not atomically | |
| // compare-and-swap, so multiple callers may race past the isDisposed check. | |
| // The test verifies that even in the worst case the semaphore ends up in a | |
| // consistent state: the slot is released exactly once, the acquisition is | |
| // marked disposed, and no exception escapes. | |
| const string otherHolder = "holder-2"; | |
| const int maxCount = 1; | |
| const int concurrency = 8; | |
| bool acquired = await provider.TryAcquireAsync(SemaphoreName, HolderId, maxCount, metadata, TimeSpan.FromMinutes(5)); | |
| acquired.ShouldBeTrue("setup: slot acquisition must succeed"); | |
| var acquisition = CreateAcquisition(); | |
| // Act: Task.WhenAll rethrows the first faulted task; any unexpected exception | |
| // here is a test failure (DisposeAsync is specified never to throw). | |
| Task[] disposeTasks = Enumerable | |
| .Range(0, concurrency) | |
| .Select(_ => acquisition.DisposeAsync().AsTask()) | |
| .ToArray(); | |
| await Task.WhenAll(disposeTasks); | |
| // Assert: the acquisition must be marked disposed regardless of which task won. | |
| acquisition.IsDisposed.ShouldBeTrue(); | |
| // The slot must be free: no phantom holders left from double-release races. | |
| int count = await provider.GetCurrentCountAsync(SemaphoreName, TimeSpan.FromMinutes(5)); | |
| count.ShouldBe(0, "holder-1 slot should be fully released after concurrent disposal"); | |
| // A new holder should be able to acquire the now-free slot. | |
| bool acquired = await provider.TryAcquireAsync( | |
| SemaphoreName, otherHolder, maxCount, metadata, TimeSpan.FromMinutes(5)); | |
| acquired.ShouldBeTrue("slot should be available for a new holder after concurrent disposal"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/MultiLock.Tests/SemaphoreAcquisitionTests.cs` around lines 99 - 134,
The test calls provider.TryAcquireAsync(SemaphoreName, HolderId, maxCount,
metadata, TimeSpan.FromMinutes(5)) to set up the acquisition but never asserts
its return value; add an assertion immediately after that call to ensure the
initial acquire succeeded (e.g., assert the boolean is true) before creating the
acquisition via CreateAcquisition() so the test precondition is validated and
subsequent DisposeAsync() concurrency checks are meaningful.
Comment on lines
+36
to
+40
| var tasks = Enumerable.Range(0, totalHolders) | ||
| .Select(i => provider.TryAcquireAsync(semaphoreName, $"holder-{i}", maxCount, metadata, slotTimeout)) | ||
| .ToList(); | ||
|
|
||
| bool[] results = await Task.WhenAll(tasks); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd 'InMemorySemaphoreProvider.cs$' | head -n 1)
[ -n "$file" ] || { echo "InMemorySemaphoreProvider.cs not found"; exit 1; }
echo "Inspecting $file"
rg -n -C2 'TryAcquireAsync|GetCurrentCountAsync|GetHoldersAsync|IsHoldingAsync|ReleaseAsync|Task\.FromResult|Task\.CompletedTask|lock\s*\(' "$file"Repository: steingran/MultiLock
Length of output: 4608
These concurrency tests do not create actual parallelism—the provider methods are lock-based and return synchronously completed tasks.
The test code builds task lists by calling the provider directly on the current thread. Since TryAcquireAsync, ReleaseAsync, and other methods execute synchronously within lock blocks and return Task.FromResult() or Task.CompletedTask, the entire sequence runs serially on the calling thread. Task.WhenAll then waits for tasks that are already completed. Schedule the provider calls on worker threads explicitly to create real concurrent contention.
Example fix
- var tasks = Enumerable.Range(0, totalHolders)
- .Select(i => provider.TryAcquireAsync(semaphoreName, $"holder-{i}", maxCount, metadata, slotTimeout))
+ var tasks = Enumerable.Range(0, totalHolders)
+ .Select(i => Task.Run(async () =>
+ await provider.TryAcquireAsync(semaphoreName, $"holder-{i}", maxCount, metadata, slotTimeout)))
.ToList();Applies to lines 36–40, 58–68, 91–95, 113–117, 136–140, 158–166.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/MultiLock.Tests/SemaphoreConcurrencyTests.cs` around lines 36 - 40, The
tests currently call provider methods directly (e.g., TryAcquireAsync,
ReleaseAsync) which execute synchronously under locks and return completed
Tasks, so build the task list by scheduling the calls on thread-pool threads to
force real concurrency; change each Select(...) that currently does
provider.TryAcquireAsync(...) (and similar blocks for ReleaseAsync,
TryAcquireManyAsync, etc.) to wrap the provider call in Task.Run or
Task.Factory.StartNew so that the provider method executes on a worker thread
and returns the Task it produces, ensuring true parallel contention for the
semaphore; apply this pattern to all affected sites (the TryAcquireAsync,
ReleaseAsync and other provider method invocations listed in the review).
Comment on lines
+161
to
+165
| for (int i = 0; i < cycles && !cts.Token.IsCancellationRequested; i++) | ||
| { | ||
| await provider.TryAcquireAsync(semaphoreName, holderId, maxCount, metadata, slotTimeout, cts.Token); | ||
| await provider.ReleaseAsync(semaphoreName, holderId, cts.Token); | ||
| } |
There was a problem hiding this comment.
Only release after a successful acquire.
Even with the parallel scheduling fixed, this loop still releases unconditionally. Because spurious releases are now clamped, the test can pass even if every TryAcquireAsync call returns false. Gate ReleaseAsync on acquired and assert some forward progress.
Representative fix
+ int successfulCycles = 0;
var tasks = Enumerable.Range(0, 4).Select(async holderIndex =>
{
string holderId = $"holder-{holderIndex}";
for (int i = 0; i < cycles && !cts.Token.IsCancellationRequested; i++)
{
- await provider.TryAcquireAsync(semaphoreName, holderId, maxCount, metadata, slotTimeout, cts.Token);
- await provider.ReleaseAsync(semaphoreName, holderId, cts.Token);
+ bool acquired = await provider.TryAcquireAsync(semaphoreName, holderId, maxCount, metadata, slotTimeout, cts.Token);
+ if (!acquired)
+ continue;
+
+ System.Threading.Interlocked.Increment(ref successfulCycles);
+ await provider.ReleaseAsync(semaphoreName, holderId, cts.Token);
}
}).ToList();
// Assert - should complete without deadlock
await Task.WhenAll(tasks);
+ successfulCycles.ShouldBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/MultiLock.Tests/SemaphoreConcurrencyTests.cs` around lines 161 - 165,
The loop always calls ReleaseAsync even when TryAcquireAsync may fail; change it
to capture the boolean result of provider.TryAcquireAsync into a variable (e.g.,
acquired) and call provider.ReleaseAsync(semaphoreName, holderId, cts.Token)
only when acquired is true; additionally assert that at least one acquire
succeeded across the loop (monitor a progress counter or assert acquired was
true at least once) to ensure forward progress; update references around
provider.TryAcquireAsync, provider.ReleaseAsync, semaphoreName, holderId,
maxCount, metadata, slotTimeout, cts.Token and cycles accordingly.
Comment on lines
+114
to
+123
| [Fact] | ||
| public async Task SemaphoreName_ShouldBeSetFromOptions() | ||
| { | ||
| // Arrange | ||
| const string expectedName = "my-semaphore"; | ||
| service = CreateService(expectedName, 3); | ||
|
|
||
| // Assert | ||
| service.SemaphoreName.ShouldBe(expectedName); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Async methods without await are unnecessary.
HolderId_ShouldBeSetFromOptions, SemaphoreName_ShouldBeSetFromOptions, and MaxCount_ShouldBeSetFromOptions are marked async but contain no await expressions. The compiler will generate a warning (CS1998). Either remove the async keyword or add await Task.CompletedTask if async is required by convention.
♻️ Proposed fix
[Fact]
- public async Task HolderId_ShouldBeSetFromOptions()
+ public void HolderId_ShouldBeSetFromOptions()
{
// Arrange
const string expectedHolderId = "custom-holder-id";
...
}
[Fact]
- public async Task SemaphoreName_ShouldBeSetFromOptions()
+ public void SemaphoreName_ShouldBeSetFromOptions()
{
...
}
[Fact]
- public async Task MaxCount_ShouldBeSetFromOptions()
+ public void MaxCount_ShouldBeSetFromOptions()
{
...
}Also applies to: 125-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/MultiLock.Tests/SemaphoreServiceTests.cs` around lines 114 - 123, The
three test methods HolderId_ShouldBeSetFromOptions,
SemaphoreName_ShouldBeSetFromOptions, and MaxCount_ShouldBeSetFromOptions in
SemaphoreServiceTests are marked async but contain no awaits; remove the
unnecessary async modifier (and the returned Task if you prefer, leaving the
method as a synchronous Fact) or alternatively add an awaited noop (e.g., await
Task.CompletedTask) inside each test to suppress CS1998 — update the method
signatures for those named tests accordingly so the compiler warning is
eliminated.
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
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.
Implement a distributed counting semaphore abstraction (ISemaphoreService / ISemaphoreProvider) mirroring the existing leader-election design, with providers for PostgreSQL, SQL Server, Redis, Azure Blob Storage, Consul, ZooKeeper, FileSystem, and InMemory.
Key design decisions and fixes included in this commit:
Fixes #100