refactor: complete async/await migration#70
Merged
Conversation
|
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 5 minutes and 17 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThis pull request completes the async/await migration across the codebase by adding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
IntelliTrader.Exchange.Binance/Services/BinanceWebSocketService.cs (1)
162-167:⚠️ Potential issue | 🟠 MajorHandle cancellation separately in connect/reconnect failure paths.
Lines 162-167 and 322-326 catch all exceptions, so cancellation can incorrectly trigger REST fallback / reconnect-failure logic during intentional shutdown.
💡 Proposed fix
private async Task ConnectInternalAsync(CancellationToken cancellationToken) { ... try { ... } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + ConnectionState = WebSocketConnectionState.Disconnected; + _loggingService.Debug("WebSocket connect canceled"); + throw; + } catch (Exception ex) { _loggingService.Error("Failed to connect to Binance WebSocket", ex); ConnectionState = WebSocketConnectionState.Disconnected; await StartRestFallbackAsync(cancellationToken).ConfigureAwait(false); throw; } } public async Task ReconnectAsync(CancellationToken cancellationToken = default) { ... try { ... } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + _loggingService.Debug("WebSocket reconnect canceled"); + } catch (Exception ex) { _loggingService.Error("Failed to reconnect to Binance WebSocket", ex); await HandleReconnectFailureAsync(cancellationToken).ConfigureAwait(false); } ... }Also applies to: 322-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@IntelliTrader.Exchange.Binance/Services/BinanceWebSocketService.cs` around lines 162 - 167, The catch-all exception handlers around connection logic (the catch(Exception ex) that logs via _loggingService.Error, sets ConnectionState, calls StartRestFallbackAsync, and the similar catch block near Reconnect logic) must not treat operation cancellation as an error; update those handlers to rethrow immediately when cancellation occurred by checking for OperationCanceledException or cancellationToken.IsCancellationRequested (e.g., if (ex is OperationCanceledException || cancellationToken.IsCancellationRequested) throw;), and only perform the logging, set ConnectionState = WebSocketConnectionState.Disconnected, and call StartRestFallbackAsync or other reconnect/failure logic for non-cancellation exceptions in the methods containing this catch, so intentional shutdown does not trigger fallback/reconnect behavior.IntelliTrader.Infrastructure/Adapters/Exchange/BinanceExchangeAdapter.cs (1)
33-37:⚠️ Potential issue | 🟠 Major
CancellationTokenis accepted but not honored in adapter methods.At Line 36/77/116/149/180/205/235/273/303/381, the token is part of the signature but not checked or propagated before legacy calls. This weakens cancellation during trading flows.
Suggested patch pattern
public async Task<Result<ExchangeOrderResult>> PlaceMarketBuyAsync( TradingPair pair, Money cost, CancellationToken cancellationToken = default) { + cancellationToken.ThrowIfCancellationRequested(); ArgumentNullException.ThrowIfNull(pair); ArgumentNullException.ThrowIfNull(cost); try { + cancellationToken.ThrowIfCancellationRequested(); var currentPrice = await _legacyExchange.GetLastPrice(pair.Symbol).ConfigureAwait(false);Apply the same pre-check pattern in the other adapter methods that accept
CancellationToken.Also applies to: 74-78, 112-117, 145-150, 178-181, 203-206, 233-236, 272-274, 300-304, 381-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@IntelliTrader.Infrastructure/Adapters/Exchange/BinanceExchangeAdapter.cs` around lines 33 - 37, The adapter methods (e.g., PlaceMarketBuyAsync) accept a CancellationToken but never honor it; update each method that takes cancellationToken to (1) call cancellationToken.ThrowIfCancellationRequested() at the start and before any long-running or legacy blocking call, and (2) propagate the token into any awaited or legacy API calls that accept a CancellationToken (or wrap calls with Task.Run/WithCancellation where supported) so cancellation is respected; apply the same pattern to the other adapter methods in this file that accept CancellationToken.
🧹 Nitpick comments (1)
tests/IntelliTrader.Web.Tests/HomeControllerTests.cs (1)
346-350: Assert the exact cancellation token instead ofIt.IsAny<CancellationToken>().Current verifications confirm an async call happened, but not that the controller propagated request cancellation.
💡 Example tightening (apply similarly to Sell/Swap/BuyDefault)
+var cts = new CancellationTokenSource(); +_sut.ControllerContext.HttpContext.RequestAborted = cts.Token; var result = await _sut.Buy(model); _tradingServiceMock.Verify(x => x.BuyAsync(It.Is<BuyOptions>(o => o.Pair == "BTCUSDT" && o.Amount == 0.01m && o.ManualOrder == true && - o.IgnoreExisting == true), It.IsAny<CancellationToken>()), Times.Once); + o.IgnoreExisting == true), It.Is<CancellationToken>(ct => ct == cts.Token)), Times.Once);Also applies to: 378-381, 409-412, 443-446
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/IntelliTrader.Web.Tests/HomeControllerTests.cs` around lines 346 - 350, The Verify call for _tradingServiceMock currently uses It.IsAny<CancellationToken>() so the test doesn't assert the controller propagated the incoming CancellationToken; update the Verify for BuyAsync (and similarly for SellAsync, SwapAsync, BuyDefaultAsync at the other locations) to match the exact token passed by the controller test (use the same CancellationToken instance/variable used when invoking the controller) instead of It.IsAny<CancellationToken>(), keeping the same predicate on BuyOptions (o.Pair, o.Amount, o.ManualOrder, o.IgnoreExisting) and Times.Once to ensure the controller forwards the exact cancellation token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@IntelliTrader.Core/Services/CoreService.cs`:
- Around line 91-95: The delayed startup is fire-and-forget and can start tasks
after shutdown; wrap the Task.Run delay with the service lifecycle
CancellationToken (the same token used by Stop()/Restart()) by passing it into
Task.Run and into Task.Delay(Constants.Timeouts.StartupDelayMs, lifecycleToken),
then check lifecycleToken.IsCancellationRequested (or call
lifecycleToken.ThrowIfCancellationRequested()) before calling StartAllTasks();
reference the StartAllTasks method and ensure the cancellation token used by
Stop()/Restart() is the one supplied to this delayed task so the startup is
skipped if the service is stopping.
In `@IntelliTrader.Infrastructure/Adapters/Legacy/LegacyTradingServiceAdapter.cs`:
- Around line 93-98: The current catch-all Exception handlers in
PlaceBuyOrderAsync, PlaceSellOrderAsync, and PlaceSwapOrderAsync swallow
OperationCanceledException and turn cancellations into exchange errors; change
the catch ordering so you first catch OperationCanceledException and rethrow (or
detect and rethrow) before the generic catch, then keep the existing
_logger.LogError(...) and Result<...>.Failure(...) behavior in the generic catch
block to preserve cancellation semantics while still logging other exceptions.
In `@IntelliTrader.Web/Controllers/HomeController.cs`:
- Around line 552-564: The Sell controller action is not propagating the HTTP
cancellation token to the service call; update the call to
_tradingService.SellAsync(...) to pass HttpContext.RequestAborted (i.e., call
SellAsync(new SellOptions(model.Pair){...}, HttpContext.RequestAborted)) so the
operation cancels if the client disconnects, and make the same change for the
other controller endpoints that call SellAsync/related async service methods
(the other occurrences noted in the review) ensuring the service methods accept
a CancellationToken parameter if they don't already.
---
Outside diff comments:
In `@IntelliTrader.Exchange.Binance/Services/BinanceWebSocketService.cs`:
- Around line 162-167: The catch-all exception handlers around connection logic
(the catch(Exception ex) that logs via _loggingService.Error, sets
ConnectionState, calls StartRestFallbackAsync, and the similar catch block near
Reconnect logic) must not treat operation cancellation as an error; update those
handlers to rethrow immediately when cancellation occurred by checking for
OperationCanceledException or cancellationToken.IsCancellationRequested (e.g.,
if (ex is OperationCanceledException ||
cancellationToken.IsCancellationRequested) throw;), and only perform the
logging, set ConnectionState = WebSocketConnectionState.Disconnected, and call
StartRestFallbackAsync or other reconnect/failure logic for non-cancellation
exceptions in the methods containing this catch, so intentional shutdown does
not trigger fallback/reconnect behavior.
In `@IntelliTrader.Infrastructure/Adapters/Exchange/BinanceExchangeAdapter.cs`:
- Around line 33-37: The adapter methods (e.g., PlaceMarketBuyAsync) accept a
CancellationToken but never honor it; update each method that takes
cancellationToken to (1) call cancellationToken.ThrowIfCancellationRequested()
at the start and before any long-running or legacy blocking call, and (2)
propagate the token into any awaited or legacy API calls that accept a
CancellationToken (or wrap calls with Task.Run/WithCancellation where supported)
so cancellation is respected; apply the same pattern to the other adapter
methods in this file that accept CancellationToken.
---
Nitpick comments:
In `@tests/IntelliTrader.Web.Tests/HomeControllerTests.cs`:
- Around line 346-350: The Verify call for _tradingServiceMock currently uses
It.IsAny<CancellationToken>() so the test doesn't assert the controller
propagated the incoming CancellationToken; update the Verify for BuyAsync (and
similarly for SellAsync, SwapAsync, BuyDefaultAsync at the other locations) to
match the exact token passed by the controller test (use the same
CancellationToken instance/variable used when invoking the controller) instead
of It.IsAny<CancellationToken>(), keeping the same predicate on BuyOptions
(o.Pair, o.Amount, o.ManualOrder, o.IgnoreExisting) and Times.Once to ensure the
controller forwards the exact cancellation token.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00124e82-2afb-4892-834e-6b978d2b87f1
📒 Files selected for processing (20)
IntelliTrader.Core/Services/CoreService.csIntelliTrader.Exchange.Base/Services/SecretRotationService.csIntelliTrader.Exchange.Binance/Services/BinanceExchangeService.csIntelliTrader.Exchange.Binance/Services/BinanceWebSocketService.csIntelliTrader.Infrastructure/Adapters/Exchange/BinanceExchangeAdapter.csIntelliTrader.Infrastructure/Adapters/Legacy/LegacyTradingServiceAdapter.csIntelliTrader.Infrastructure/Adapters/Persistence/Json/JsonPositionRepository.csIntelliTrader.Infrastructure/BackgroundServices/OrderExecutionService.csIntelliTrader.Infrastructure/BackgroundServices/SignalRuleProcessorService.csIntelliTrader.Infrastructure/BackgroundServices/TimedBackgroundService.csIntelliTrader.Infrastructure/BackgroundServices/TradingRuleProcessorService.csIntelliTrader.Infrastructure/Dispatching/InMemoryCommandDispatcher.csIntelliTrader.Infrastructure/Dispatching/InMemoryQueryDispatcher.csIntelliTrader.Infrastructure/Events/InMemoryDomainEventDispatcher.csIntelliTrader.Infrastructure/Migration/LegacyAccountMigrator.csIntelliTrader.Trading/Services/TradingService.csIntelliTrader.Web/BackgroundServices/SignalRBroadcasterService.csIntelliTrader.Web/Controllers/HomeController.csIntelliTrader.Web/Hubs/TradingHub.cstests/IntelliTrader.Web.Tests/HomeControllerTests.cs
Comment on lines
91
to
95
| _ = Task.Run(async () => | ||
| { | ||
| await Task.Delay(Constants.Timeouts.StartupDelayMs); | ||
| await Task.Delay(Constants.Timeouts.StartupDelayMs).ConfigureAwait(false); | ||
| StartAllTasks(); | ||
| }); |
There was a problem hiding this comment.
Guard delayed startup with a lifecycle cancellation token.
Line 93 schedules delayed StartAllTasks() as fire-and-forget; if Stop()/Restart() runs before the delay completes, tasks can be started after shutdown.
💡 Proposed fix
+private CancellationTokenSource? _startupDelayCts;
public void Start()
{
+ _startupDelayCts?.Cancel();
+ _startupDelayCts?.Dispose();
+ _startupDelayCts = new CancellationTokenSource();
+ var startupToken = _startupDelayCts.Token;
+
_ = Task.Run(async () =>
{
- await Task.Delay(Constants.Timeouts.StartupDelayMs).ConfigureAwait(false);
- StartAllTasks();
- });
+ try
+ {
+ await Task.Delay(Constants.Timeouts.StartupDelayMs, startupToken).ConfigureAwait(false);
+ if (!startupToken.IsCancellationRequested && Running)
+ {
+ StartAllTasks();
+ }
+ }
+ catch (OperationCanceledException) when (startupToken.IsCancellationRequested) { }
+ }, startupToken);
Running = true;
}
public void Stop()
{
+ _startupDelayCts?.Cancel();
Running = false;
...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@IntelliTrader.Core/Services/CoreService.cs` around lines 91 - 95, The delayed
startup is fire-and-forget and can start tasks after shutdown; wrap the Task.Run
delay with the service lifecycle CancellationToken (the same token used by
Stop()/Restart()) by passing it into Task.Run and into
Task.Delay(Constants.Timeouts.StartupDelayMs, lifecycleToken), then check
lifecycleToken.IsCancellationRequested (or call
lifecycleToken.ThrowIfCancellationRequested()) before calling StartAllTasks();
reference the StartAllTasks method and ensure the cancellation token used by
Stop()/Restart() is the one supplied to this delayed task so the startup is
skipped if the service is stopping.
Comment on lines
93
to
98
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error placing buy order for {Pair}", pair); | ||
| return Task.FromResult(Result<PlaceBuyOrderResult>.Failure( | ||
| Error.ExchangeError($"Failed to place buy order: {ex.Message}"))); | ||
| return Result<PlaceBuyOrderResult>.Failure( | ||
| Error.ExchangeError($"Failed to place buy order: {ex.Message}")); | ||
| } |
There was a problem hiding this comment.
Preserve cancellation semantics in async order placement methods.
catch (Exception) on Lines 93, 156, and 214 also catches OperationCanceledException, turning cancellations into exchange errors.
💡 Proposed fix
try
{
await _tradingService.BuyAsync(options, cancellationToken).ConfigureAwait(false);
...
}
+ catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
+ {
+ throw;
+ }
catch (Exception ex)
{
_logger.LogError(ex, "Error placing buy order for {Pair}", pair);
return Result<PlaceBuyOrderResult>.Failure(
Error.ExchangeError($"Failed to place buy order: {ex.Message}"));
}Apply the same catch ordering to PlaceSellOrderAsync and PlaceSwapOrderAsync.
Also applies to: 156-161, 214-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@IntelliTrader.Infrastructure/Adapters/Legacy/LegacyTradingServiceAdapter.cs`
around lines 93 - 98, The current catch-all Exception handlers in
PlaceBuyOrderAsync, PlaceSellOrderAsync, and PlaceSwapOrderAsync swallow
OperationCanceledException and turn cancellations into exchange errors; change
the catch ordering so you first catch OperationCanceledException and rethrow (or
detect and rethrow) before the generic catch, then keep the existing
_logger.LogError(...) and Result<...>.Failure(...) behavior in the generic catch
block to preserve cancellation semantics while still logging other exceptions.
Comment on lines
+552
to
564
| public async Task<IActionResult> Sell([FromForm] SellInputModel model) | ||
| { | ||
| if (!ModelState.IsValid) | ||
| { | ||
| return BadRequest(ModelState); | ||
| } | ||
|
|
||
| _tradingService.Sell(new SellOptions(model.Pair) | ||
| await _tradingService.SellAsync(new SellOptions(model.Pair) | ||
| { | ||
| Amount = model.Amount, | ||
| ManualOrder = true | ||
| }); | ||
| }).ConfigureAwait(false); | ||
| return Ok(); |
There was a problem hiding this comment.
Propagate request cancellation token from controller endpoints.
At Line 559, Line 578, Line 598, and Line 622, async service calls omit HttpContext.RequestAborted. If the client disconnects, these operations may continue unnecessarily.
Suggested patch
- await _tradingService.SellAsync(new SellOptions(model.Pair)
+ await _tradingService.SellAsync(new SellOptions(model.Pair)
{
Amount = model.Amount,
ManualOrder = true
- }).ConfigureAwait(false);
+ }, HttpContext.RequestAborted).ConfigureAwait(false);
- await _tradingService.BuyAsync(new BuyOptions(model.Pair)
+ await _tradingService.BuyAsync(new BuyOptions(model.Pair)
{
Amount = model.Amount,
IgnoreExisting = true,
ManualOrder = true
- }).ConfigureAwait(false);
+ }, HttpContext.RequestAborted).ConfigureAwait(false);
- await _tradingService.BuyAsync(new BuyOptions(model.Pair)
+ await _tradingService.BuyAsync(new BuyOptions(model.Pair)
{
MaxCost = _tradingService.GetPairConfig(model.Pair).BuyMaxCost,
IgnoreExisting = true,
ManualOrder = true,
Metadata = new OrderMetadata
{
BoughtGlobalRating = _signalsService.GetGlobalRating()
}
- }).ConfigureAwait(false);
+ }, HttpContext.RequestAborted).ConfigureAwait(false);
- await _tradingService.SwapAsync(new SwapOptions(model.Pair, model.Swap, new OrderMetadata())
+ await _tradingService.SwapAsync(new SwapOptions(model.Pair, model.Swap, new OrderMetadata())
{
ManualOrder = true
- }).ConfigureAwait(false);
+ }, HttpContext.RequestAborted).ConfigureAwait(false);Also applies to: 571-584, 591-608, 615-626
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@IntelliTrader.Web/Controllers/HomeController.cs` around lines 552 - 564, The
Sell controller action is not propagating the HTTP cancellation token to the
service call; update the call to _tradingService.SellAsync(...) to pass
HttpContext.RequestAborted (i.e., call SellAsync(new
SellOptions(model.Pair){...}, HttpContext.RequestAborted)) so the operation
cancels if the client disconnects, and make the same change for the other
controller endpoints that call SellAsync/related async service methods (the
other occurrences noted in the review) ensuring the service methods accept a
CancellationToken parameter if they don't already.
- Web controllers (Buy, Sell, Swap, BuyDefault): migrated from sync to async, now call BuyAsync/SellAsync/SwapAsync instead of sync wrappers - Web services (SignalRBroadcasterService, TradingHub): migrated GetCurrentPrice to GetCurrentPriceAsync - LegacyTradingServiceAdapter: migrated PlaceBuyOrderAsync, PlaceSellOrderAsync, PlaceSwapOrderAsync from sync wrappers to proper async calls with CancellationToken propagation - Added ConfigureAwait(false) to all await calls in library projects: CoreService, NotificationService, BinanceExchangeService, BinanceWebSocketService, SecretRotationService, TradingService, OrderExecutionService, SignalRuleProcessorService, TradingRuleProcessorService, TimedBackgroundService, BinanceExchangeAdapter, JsonPositionRepository, InMemoryCommandDispatcher, InMemoryQueryDispatcher, InMemoryDomainEventDispatcher, LegacyAccountMigrator - Updated web controller tests to verify async method calls Sync wrapper methods (GetTickers, GetCurrentPrice, PlaceOrder, etc.) are preserved for callers in lock blocks (TradingAccountBase, ExchangeAccount) and HighResolutionTimedTask - those are scope for issue #22. Closes #21 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add InternalsVisibleTo for IntelliTrader.Performance.Tests to fix CS0122 build error (TradingService inaccessible) - Inject default TradingConfig via reflection in performance tests to fix ArgumentNullException for 'configuration' parameter These issues pre-date this PR but were masked by the build error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
blackms
force-pushed
the
refactor/issue-21-async-migration
branch
from
76290e6 to
f575d56
Compare
…ntainer
Add [Collection("E2E")] to all E2E test classes to prevent xUnit from
running them in parallel. The tests share a static Startup.Container
reference which causes race conditions when multiple TestWebHostFactory
instances are created concurrently.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 24, 2026
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.
Summary
BuyAsync/SellAsync/SwapAsyncinstead of blocking sync wrappersLegacyTradingServiceAdapterfrom sync Buy/Sell/Swap calls to proper async with CancellationToken propagationGetCurrentPricetoGetCurrentPriceAsyncConfigureAwait(false)to allawaitcalls in library projects (20 files across Core, Exchange, Trading, Infrastructure layers) to prevent potential deadlocksScope decisions
GetTickers,GetCurrentPrice,PlaceOrder, etc. in TradingService) are preserved because their callers inTradingAccountBase/ExchangeAccountoperate insidelockblocks -- converting those is a larger refactorHighResolutionTimedTask.Run()pattern is not changed (scope of issue Migrate to IHostedService #22)BinanceExchangeService.Start()/ConnectTickersWebsocket()retainGetAwaiter().GetResult()since they run during startup with no SynchronizationContextTest plan
Closes #21
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Tests