Optimizes Service Bus admin API concurrency and auto-refresh#86
Merged
soliktomasz merged 3 commits intomainfrom Apr 10, 2026
Merged
Optimizes Service Bus admin API concurrency and auto-refresh#86soliktomasz merged 3 commits intomainfrom
soliktomasz merged 3 commits intomainfrom
Conversation
- Introduce `BoundedAdminProjector` to throttle parallel metadata requests to Service Bus, preventing resource exhaustion when fetching entity details. - Implement an interlocked gate in `MainWindowViewModel` to ensure auto-refresh ticks do not overlap. - Remove redundant message loading when toggling the dead-letter view to prevent duplicate API calls.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a new BoundedAdminProjector utility to run async projections with bounded concurrency, updates admin retrieval to use it, refactors MainWindowViewModel to prevent overlapping auto-refresh and coordinate dead-letter reloads, and adds tests validating concurrency and ordering behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Bounded as BoundedAdminProjector
participant Gate as SemaphoreSlim
participant Projector as Projector Func
participant Results as Results Array
Caller->>Bounded: Call SelectAsync(source, projector, maxConcurrency)
Bounded->>Gate: init SemaphoreSlim(min(maxConcurrency, count))
loop schedule one task per source item
Caller->>Bounded: enqueue ProjectAsync(item, index)
end
par up to maxConcurrency
Bounded->>Gate: await WaitAsync(ct)
activate Projector
Projector->>Projector: execute projector(item, ct)
Projector-->>Results: store result[index]
deactivate Projector
Gate->>Gate: Release()
end
Bounded->>Caller: Task.WhenAll completes -> return ordered Results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
BusLane.Tests/Services/ServiceBus/ServiceBusOperationsFactoryTests.cs (1)
27-41: These tests pin an implementation detail, not the factory contract.
ServiceBusOperationsFactoryalready returns fixed concrete types inBusLane/Services/ServiceBus/ServiceBusOperationsFactory.cs:40-62, so these assertions only prove that two reflected static properties currently match. They will still pass if the returned types stop usingBoundedAdminProjector, and they will fail if the two implementations ever need different budgets for valid reasons. Prefer keeping factory tests focused on the returned type and keeping concurrency-budget verification withBoundedAdminProjector/operations behavior tests instead.Also applies to: 106-124, 147-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BusLane.Tests/Services/ServiceBus/ServiceBusOperationsFactoryTests.cs` around lines 27 - 41, The test is asserting a reflected static concurrency-limit implementation detail instead of the factory contract; update the ServiceBusOperationsFactory.CreateFromConnectionString test to assert the concrete returned type (e.g., that factory.CreateFromConnectionString(...) returns an instance of AzureCredentialOperations or the expected concrete class) rather than comparing GetAdminProjectionConcurrencyLimit values, and move any concurrency-budget verification into tests that exercise BoundedAdminProjector or the specific operations behavior; apply the same change pattern to the other similar tests referenced (the other CreateFromConnectionString and CreateFromCredential tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BusLane.Tests/Services/ServiceBus/ServiceBusOperationsTests.cs`:
- Around line 21-31: The inline projector passed to
InvokeBoundedAdminProjectorAsync can skip Interlocked.Decrement(ref
activeWorkers) if an exception or cancellation occurs, inflating activeWorkers
and invalidating concurrency assertions; wrap the increment/decrement around a
try/finally in the lambda so that Interlocked.Decrement(ref activeWorkers)
always runs (use try { await Task.Delay(..., ct); return item * 2; } finally {
Interlocked.Decrement(ref activeWorkers); }) and keep UpdateMaxValue(ref
maxConcurrentWorkers, inFlight) immediately after Interlocked.Increment to
preserve the max-tracking behavior.
In `@BusLane.Tests/ViewModels/MainWindowViewModelTests.cs`:
- Around line 329-330: The test in MainWindowViewModelTests currently uses
Task.Delay(TimeSpan.FromMilliseconds(2600)) which is brittle; replace the
wall-clock sleep with a deterministic trigger: inject a controllable/fake timer
or scheduler (e.g., a TestScheduler or an ITimer stub used by
MainWindowViewModel) and advance it twice to simulate two ticks, or modify the
stubbed alert evaluation (e.g., your EvaluateAlertsAsync stub) to complete a
TaskCompletionSource when the second evaluation occurs and await that signal
instead of sleeping; then perform the assertions immediately after advancing the
fake timer or awaiting the TCS.
- Around line 255-257: Replace the brittle Task.Delay(50) in the test with a
TaskCompletionSource that the PeekMessagesAsync stub completes when it runs:
modify the test to create a TaskCompletionSource (tcs), inject that tcs into the
PeekMessagesAsync stub implementation (the stub used by
ToggleDeadLetterViewCommand.ExecuteAsync) so that when PeekMessagesAsync is
invoked it calls tcs.SetResult(null) (or appropriate result), then await
tcs.Task after executing sut.ToggleDeadLetterViewCommand.ExecuteAsync(null)
instead of Task.Delay; this ensures the assertion runs only after the
fire-and-forget reload (PeekMessagesAsync) has completed.
In `@BusLane/ViewModels/MainWindowViewModel.cs`:
- Around line 1275-1278: ToggleDeadLetterViewAsync currently flips
CurrentNavigation.ShowDeadLetter and returns immediately while the actual reload
runs via FireAndForget in the property's change handlers, losing
AsyncRelayCommand tracking; fix it by invoking and awaiting the real reload task
directly inside ToggleDeadLetterViewAsync instead of returning
Task.CompletedTask and instead of relying on FireAndForget (e.g., after toggling
CurrentNavigation.ShowDeadLetter call the same reload method the property-change
handler uses, await that Task, and remove/avoid the FireAndForget path so
AsyncRelayCommand observes execution/errors).
- Around line 1652-1655: Do not reopen the auto-refresh gate during teardown:
remove the Interlocked.Exchange(ref _autoRefreshTickInProgress, 0) calls in the
dispose/teardown paths (references: _autoRefreshTimer,
_autoRefreshTickInProgress) so the gate remains closed, and ensure
HandleAutoRefreshTickAsync checks the _disposed flag at its start and
short-circuits immediately if _disposed is true; apply the same change for the
similar block around the other dispose/unsubscribe area (the block noted at
lines 1676-1679).
---
Nitpick comments:
In `@BusLane.Tests/Services/ServiceBus/ServiceBusOperationsFactoryTests.cs`:
- Around line 27-41: The test is asserting a reflected static concurrency-limit
implementation detail instead of the factory contract; update the
ServiceBusOperationsFactory.CreateFromConnectionString test to assert the
concrete returned type (e.g., that factory.CreateFromConnectionString(...)
returns an instance of AzureCredentialOperations or the expected concrete class)
rather than comparing GetAdminProjectionConcurrencyLimit values, and move any
concurrency-budget verification into tests that exercise BoundedAdminProjector
or the specific operations behavior; apply the same change pattern to the other
similar tests referenced (the other CreateFromConnectionString and
CreateFromCredential tests).
🪄 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: 93c89c8a-fc01-4657-8209-5de4c39d668a
📒 Files selected for processing (7)
BusLane.Tests/Services/ServiceBus/ServiceBusOperationsFactoryTests.csBusLane.Tests/Services/ServiceBus/ServiceBusOperationsTests.csBusLane.Tests/ViewModels/MainWindowViewModelTests.csBusLane/Services/ServiceBus/AzureCredentialOperations.csBusLane/Services/ServiceBus/BoundedAdminProjector.csBusLane/Services/ServiceBus/ConnectionStringOperations.csBusLane/ViewModels/MainWindowViewModel.cs
- Prevent redundant message reloads when toggling the dead-letter view by implementing a suppression flag. - Add disposal checks to `HandleAutoRefreshTickAsync` to prevent background processing on closed tabs. - Simplify Service Bus operation classes by removing redundant internal concurrency limit properties. - Replace fragile `Task.Delay` calls in unit tests with `TaskCompletionSource` to ensure deterministic execution and reduce flakiness. - Ensure concurrency counters in tests are correctly decremented using `try-finally` blocks.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
BusLane.Tests/Services/ServiceBus/ServiceBusOperationsTests.cs (1)
3-8:⚠️ Potential issue | 🟡 MinorReorder the
usingblock to keepSystem.*imports first.
System.Reflectionshould be grouped with the standard-library imports, ahead ofBusLane.*and third-party namespaces.As per coding guidelines, "Order
usingdirectives as: System/standard imports → BusLane imports → third-party imports (Avalonia, CommunityToolkit, Serilog, etc.)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BusLane.Tests/Services/ServiceBus/ServiceBusOperationsTests.cs` around lines 3 - 8, Move the System namespace import so standard-library usings appear first: reorder the using block in ServiceBusOperationsTests (where ServiceBusOperationsTests class is defined) to place System.Reflection immediately after other System usings (i.e., put System.* imports before BusLane.* and third-party usings such as Azure.Messaging.ServiceBus, BusLane.Services.ServiceBus, FluentAssertions, NSubstitute, and Xunit) to match the project's using-directive ordering convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BusLane.Tests/Services/ServiceBus/ServiceBusOperationsTests.cs`:
- Around line 21-37: The test uses Task.Delay which makes concurrency
nondeterministic; replace the timer-driven work lambda passed to
InvokeBoundedAdminProjectorAsync with a TaskCompletionSource-based coordination:
create a list/array of TaskCompletionSource for each input item, have the worker
increment Interlocked activeWorkers and call UpdateMaxValue, then await the
per-item tcs.Task instead of Task.Delay so you can deterministically control
when each item completes; in the test drive signal the first N tcs (N =
maxConcurrency, here 3) to allow workers to enter and assert
maxConcurrentWorkers reached 3, then release remaining tcs in input order to
assert outputs preserve source order; reference the existing
InvokeBoundedAdminProjectorAsync invocation, the activeWorkers/UpdateMaxValue
usage and the maxConcurrency parameter when making the change.
---
Outside diff comments:
In `@BusLane.Tests/Services/ServiceBus/ServiceBusOperationsTests.cs`:
- Around line 3-8: Move the System namespace import so standard-library usings
appear first: reorder the using block in ServiceBusOperationsTests (where
ServiceBusOperationsTests class is defined) to place System.Reflection
immediately after other System usings (i.e., put System.* imports before
BusLane.* and third-party usings such as Azure.Messaging.ServiceBus,
BusLane.Services.ServiceBus, FluentAssertions, NSubstitute, and Xunit) to match
the project's using-directive ordering convention.
🪄 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: 00b40c11-03e4-4f21-887e-187b4f9d96dd
📒 Files selected for processing (4)
BusLane.Tests/Services/ServiceBus/ServiceBusOperationsTests.csBusLane.Tests/ViewModels/MainWindowViewModelTests.csBusLane/Services/ServiceBus/ConnectionStringOperations.csBusLane/ViewModels/MainWindowViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- BusLane/ViewModels/MainWindowViewModel.cs
- BusLane.Tests/ViewModels/MainWindowViewModelTests.cs
- Replace `Task.Delay` with `TaskCompletionSource` to precisely control and verify worker concurrency. - Release workers in batches to ensure the concurrency limit is respected throughout the operation. - Prevent potential test hangs by adding timeouts to asynchronous waits.
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.
Improves application performance and stability by implementing two key optimizations:
These changes are supported by new unit tests validating the bounded concurrency and the non-overlapping refresh logic.
Summary by CodeRabbit
Bug Fixes
Tests