tests: Add ConcurrentCommandGuardExtensions and improve exclusivity#418
Merged
Conversation
samtrion
added
state:ready for merge
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughThis PR updates the codebase with XML documentation refinements, introduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Introduce extension methods for registering ConcurrentCommandGuardInterceptor with IMediatorBuilder, supporting open-generic, closed-generic, and void command overloads. Refactor the interceptor to implement IDisposable for proper SemaphoreSlim disposal and restrict it to IExclusiveCommand<TResponse> only. Update and expand tests to cover registration and deduplication scenarios. Apply minor doc and type improvements, and ensure consistent usage of Extensibility.Void. Resolves #256
samtrion
force-pushed
the
test/ConcurrentCommandGuardExtensions
branch
from
April 17, 2026 11:04
399dcd9 to
7c523e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/NetEvolve.Pulse.Tests.Integration/Idempotency/IdempotencyTestsBase.cs (1)
288-293: Minor inconsistency: line 293 still usesExtensibility.Void.Completed.Now that
Voidis unqualified in the signature and registration, consider usingVoid.Completedhere too for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/NetEvolve.Pulse.Tests.Integration/Idempotency/IdempotencyTestsBase.cs` around lines 288 - 293, The HandleAsync implementation in TestIdempotentVoidCommandHandler currently returns Extensibility.Void.Completed while the signature and registration use the unqualified Void type; update the return expression in HandleAsync to use Void.Completed instead to match the rest of the codebase and maintain consistency for the ICommandHandler<TestIdempotentVoidCommand, Void> implementation.tests/NetEvolve.Pulse.Tests.Unit/Interceptors/ConcurrentCommandGuardInterceptorTests.cs (1)
121-127: Max-concurrency tracking is racy and could mask a broken guard.
maxConcurrentis read non-atomically before theCompareExchange, andmax = maxConcurrentis re-read from the field rather than the CAS result. If the interceptor ever regressed and allowed concurrent execution, two threads could both observe a stalemaxand lose an update, weakening this test. Consider a simpleInterlocked.Exchange/InterlockedMax-style loop driven off the CAS return value.♻️ Suggested tightening
- var current = Interlocked.Increment(ref currentConcurrent); - var max = maxConcurrent; - while (current > max) - { - _ = Interlocked.CompareExchange(ref maxConcurrent, current, max); - max = maxConcurrent; - } + var current = Interlocked.Increment(ref currentConcurrent); + int max; + do + { + max = Volatile.Read(ref maxConcurrent); + if (current <= max) + { + break; + } + } while (Interlocked.CompareExchange(ref maxConcurrent, current, max) != max);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/NetEvolve.Pulse.Tests.Unit/Interceptors/ConcurrentCommandGuardInterceptorTests.cs` around lines 121 - 127, The max-concurrency tracking loop is racy because it reads maxConcurrent non-atomically and then re-reads the field instead of using the CAS return value; replace the current CompareExchange loop so it uses the CAS return value (or an InterlockedMax-style loop) to update maxConcurrent atomically based on the returned previous value from Interlocked.CompareExchange, referencing the currentConcurrent and maxConcurrent variables and the Interlocked.CompareExchange call in the ConcurrentCommandGuardInterceptorTests to ensure no lost updates when multiple threads try to raise the max.src/NetEvolve.Pulse/Interceptors/ConcurrentCommandGuardInterceptor.cs (1)
70-93: Optional: simplify the disposal pattern for a sealed type.Because the class is
sealed, the fullDispose(bool disposing)virtual-style pattern with_disposedValueis unnecessary and adds boilerplate/finalizer complexity you don't need. A simpler pattern suffices:♻️ Proposed simplification
- private bool _disposedValue; + private bool _disposed; ... - private void Dispose(bool disposing) - { - if (!_disposedValue) - { - if (disposing) - { - // Dispose managed state (managed objects) - foreach (var semaphore in _semaphores.Values) - { - semaphore.Dispose(); - } - _semaphores.Clear(); - } - - _disposedValue = true; - } - } - - /// <inheritdoc /> - public void Dispose() - { - Dispose(disposing: true); - GC.SuppressFinalize(this); - } + /// <inheritdoc /> + public void Dispose() + { + if (_disposed) + { + return; + } + + foreach (var semaphore in _semaphores.Values) + { + semaphore.Dispose(); + } + _semaphores.Clear(); + _disposed = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NetEvolve.Pulse/Interceptors/ConcurrentCommandGuardInterceptor.cs` around lines 70 - 93, The disposal pattern is overengineered for a sealed class: remove the private Dispose(bool disposing) method and the _disposedValue flag, and replace with a single public Dispose() that iterates _semaphores.Values disposing each SemaphoreSlim (or semaphore type), clears _semaphores, and calls GC.SuppressFinalize(this); update any references to Dispose(bool) to call Dispose() and keep the existing semaphore loop and Clear() logic from the current Dispose(bool) implementation; leave no finalizer or virtual disposal hooks since the class is sealed.
🤖 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/NetEvolve.Pulse/ConcurrentCommandGuardExtensions.cs`:
- Around line 26-41: Update the XML summary for the
ConcurrentCommandGuardExtensions method to correct the lifetime description:
replace the word "scoped" with "singleton" and reword the first sentence to
state that it registers an open-generic
ConcurrentCommandGuardInterceptor{TRequest,TResponse} as a singleton
IRequestInterceptor{TRequest,TResponse} for all IExclusiveCommand{TResponse}
implementations; ensure the summary aligns with the remarks and the actual
registration using ServiceLifetime.Singleton and TryAddEnumerable on the
IMediatorBuilder so consumers are not misled.
- Line 104: The registration currently calls
services.TryAdd(ServiceDescriptor.Singleton(typeof(ConcurrentCommandGuardInterceptor<,>)))
which incorrectly registers System.Type; change the registration to register the
open-generic interceptor type as a service implementation (for example use
services.TryAddSingleton(typeof(IRequestInterceptor<,>),
typeof(ConcurrentCommandGuardInterceptor<,>)) or
services.TryAdd(ServiceDescriptor.Singleton(typeof(IRequestInterceptor<,>),
typeof(ConcurrentCommandGuardInterceptor<,>))) so the DI container can resolve
ConcurrentCommandGuardInterceptor<TRequest,TResponse>; update the factory usage
around the existing factory code that resolves the interceptor (the code
referencing ConcurrentCommandGuardInterceptor<,> in the registration block) to
rely on the IRequestInterceptor<TRequest,TResponse> service, and add a unit test
that builds a provider and calls
provider.GetRequiredService<IRequestInterceptor<ExclusiveCommand, string>>() to
prevent regression.
- Around line 83-88: The XML doc is wrong: registering both
AddConcurrentCommandGuard() (open-generic) and
AddConcurrentCommandGuard<TRequest,TResponse>() currently produces two separate
ConcurrentCommandGuardInterceptor instances with independent _semaphores; fix by
changing the typed overload AddConcurrentCommandGuard<TRequest,TResponse>() to
first detect whether an open-generic descriptor for IRequestInterceptor<,> →
ConcurrentCommandGuardInterceptor<,> is already registered in the
IServiceCollection and skip adding the self-mapped descriptor if so (or
alternatively use TryAddEnumerable for the closed
IRequestInterceptor<TRequest,TResponse> to avoid creating a duplicate
singleton); update AddConcurrentCommandGuard<TRequest,TResponse>(),
ConcurrentCommandGuardInterceptor, and related registration logic accordingly or
update the XML docs to reflect the actual two-instance behavior if you prefer
the documented outcome.
In `@src/NetEvolve.Pulse/Interceptors/ConcurrentCommandGuardInterceptor.cs`:
- Around line 40-46: The open-generic
ConcurrentCommandGuardInterceptor<TRequest,TResponse> narrows the generic
constraint to TRequest : IExclusiveCommand<TResponse> whereas
ICommandInterceptor<TRequest,TResponse> only requires TRequest :
ICommand<TResponse>, which can cause DI to skip descriptors for non-exclusive
commands; add an integration test that registers the open-generic
ConcurrentCommandGuardInterceptor<,> (the class name) as IRequestInterceptor<,>
and then resolves IEnumerable<IRequestInterceptor<NonExclusiveCommand,
SomeResponse>> (use a concrete NonExclusiveCommand type that implements
ICommand<SomeResponse> but not IExclusiveCommand) to assert the DI container
returns a pipeline without throwing or missing required interceptors, ensuring
the registration and resolution behavior is validated.
In `@tests/NetEvolve.Pulse.Tests.Unit/ConcurrentCommandGuardExtensionsTests.cs`:
- Around line 170-200: The test
AddConcurrentCommandGuard_Typed_CombinedWithOpenGeneric_DoesNotDuplicateInterfaceRegistrations
should also assert actual resolution uniqueness: after registering both
AddConcurrentCommandGuard() and
AddConcurrentCommandGuard<ExclusiveCommand,string>(), build a service provider
(services.BuildServiceProvider()), resolve
IEnumerable<IRequestInterceptor<ExclusiveCommand,string>> via
provider.GetServices<IRequestInterceptor<ExclusiveCommand,string>>() and assert
that the resulting collection contains a single instance (instance identity), so
the test verifies the open‑generic mapping to
ConcurrentCommandGuardInterceptor<,> and the closed generic factory produce the
same underlying instance rather than two separate interceptor objects.
- Around line 18-24: The test methods
AddConcurrentCommandGuard_WithNullConfigurator_ThrowsArgumentNullException,
AddConcurrentCommandGuard_TypedWithNullConfigurator_..., and
AddConcurrentCommandGuard_VoidWithNullConfigurator_... are declared async but
contain no awaits, causing CS1998; fix each by either removing the async
modifier and returning Task.CompletedTask, or keeping async and adding a minimal
await such as await Task.Yield(); so the signature remains async. Update the
test method declarations (the ones above) and their return statements
accordingly to eliminate the compiler warning while preserving the existing
Assert logic.
- Around line 78-97: Add runtime resolution checks to the typed tests: after
calling ConcurrentCommandGuardExtensions.AddConcurrentCommandGuard<TCommand,
TResponse>() and verifying the service descriptor, call
services.BuildServiceProvider(), resolve the service via
provider.GetRequiredService<IRequestInterceptor<ExclusiveCommand, string>>() and
assert the resolved instance is a
ConcurrentCommandGuardInterceptor<ExclusiveCommand, string> (verifying the typed
overload actually registers a resolvable implementation and matches the
open-generic registration).
---
Nitpick comments:
In `@src/NetEvolve.Pulse/Interceptors/ConcurrentCommandGuardInterceptor.cs`:
- Around line 70-93: The disposal pattern is overengineered for a sealed class:
remove the private Dispose(bool disposing) method and the _disposedValue flag,
and replace with a single public Dispose() that iterates _semaphores.Values
disposing each SemaphoreSlim (or semaphore type), clears _semaphores, and calls
GC.SuppressFinalize(this); update any references to Dispose(bool) to call
Dispose() and keep the existing semaphore loop and Clear() logic from the
current Dispose(bool) implementation; leave no finalizer or virtual disposal
hooks since the class is sealed.
In `@tests/NetEvolve.Pulse.Tests.Integration/Idempotency/IdempotencyTestsBase.cs`:
- Around line 288-293: The HandleAsync implementation in
TestIdempotentVoidCommandHandler currently returns Extensibility.Void.Completed
while the signature and registration use the unqualified Void type; update the
return expression in HandleAsync to use Void.Completed instead to match the rest
of the codebase and maintain consistency for the
ICommandHandler<TestIdempotentVoidCommand, Void> implementation.
In
`@tests/NetEvolve.Pulse.Tests.Unit/Interceptors/ConcurrentCommandGuardInterceptorTests.cs`:
- Around line 121-127: The max-concurrency tracking loop is racy because it
reads maxConcurrent non-atomically and then re-reads the field instead of using
the CAS return value; replace the current CompareExchange loop so it uses the
CAS return value (or an InterlockedMax-style loop) to update maxConcurrent
atomically based on the returned previous value from
Interlocked.CompareExchange, referencing the currentConcurrent and maxConcurrent
variables and the Interlocked.CompareExchange call in the
ConcurrentCommandGuardInterceptorTests to ensure no lost updates when multiple
threads try to raise the max.
🪄 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: CHILL
Plan: Pro
Run ID: 7101bf89-886b-4efb-84f1-27f46078a627
📒 Files selected for processing (11)
src/NetEvolve.Pulse.AspNetCore/EndpointRouteBuilderExtensions.cssrc/NetEvolve.Pulse.EntityFramework/Configurations/MySqlOutboxMessageConfiguration.cssrc/NetEvolve.Pulse.EntityFramework/EntityFrameworkIdempotencyExtensions.cssrc/NetEvolve.Pulse.EntityFramework/ModelBuilderExtensions.cssrc/NetEvolve.Pulse.Extensibility/IExclusiveCommand1.cs`src/NetEvolve.Pulse.MySql/MySqlIdempotencyMediatorBuilderExtensions.cssrc/NetEvolve.Pulse/ConcurrentCommandGuardExtensions.cssrc/NetEvolve.Pulse/Interceptors/ConcurrentCommandGuardInterceptor.cstests/NetEvolve.Pulse.Tests.Integration/Idempotency/IdempotencyTestsBase.cstests/NetEvolve.Pulse.Tests.Unit/ConcurrentCommandGuardExtensionsTests.cstests/NetEvolve.Pulse.Tests.Unit/Interceptors/ConcurrentCommandGuardInterceptorTests.cs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
==========================================
+ Coverage 92.09% 92.20% +0.11%
==========================================
Files 145 146 +1
Lines 5475 5494 +19
Branches 515 517 +2
==========================================
+ Hits 5042 5066 +24
+ Misses 287 281 -6
- Partials 146 147 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Martin Stühmer <me@samtrion.net>
Refactored ConcurrentCommandGuardInterceptor to simplify the Dispose pattern by removing Dispose(bool), renaming _disposedValue to _disposed, and consolidating disposal logic into the public Dispose() method. Now ensures proper resource cleanup and prevents multiple disposals.
Updated three test methods in ConcurrentCommandGuardExtensionsTests.cs to be synchronous, removing unnecessary async/await usage for ArgumentNullException checks. Also added missing using directives for required namespaces.
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
1 participant
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.
Introduce extension methods for registering ConcurrentCommandGuardInterceptor with IMediatorBuilder, supporting open-generic, closed-generic, and void command overloads. Refactor the interceptor to implement IDisposable for proper SemaphoreSlim disposal and restrict it to IExclusiveCommand only. Update and expand tests to cover registration and deduplication scenarios. Apply minor doc and type improvements, and ensure consistent usage of Extensibility.Void.
Resolves #256
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests