From 3cfac9172d6d0ce0b8ad97e83cb1173f44f36a47 Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Thu, 6 Aug 2026 22:15:07 -0300 Subject: [PATCH 1/5] =?UTF-8?q?fix(client):=20=D0=BD=D0=B5=20=D1=82=D1=80?= =?UTF-8?q?=D0=BE=D0=B3=D0=B0=D1=82=D1=8C=20=D1=87=D1=83=D0=B6=D0=BE=D0=B9?= =?UTF-8?q?=20=D0=B8=D1=81=D1=82=D0=BE=D1=87=D0=BD=D0=B8=D0=BA=20=D0=BE?= =?UTF-8?q?=D1=82=D0=BC=D0=B5=D0=BD=D1=8B=20=D0=B2=20=D0=B1=D1=8B=D1=81?= =?UTF-8?q?=D1=82=D1=80=D0=BE=D0=BC=20=D0=BF=D0=B5=D1=80=D0=B5=D0=BF=D0=BE?= =?UTF-8?q?=D0=B4=D0=BA=D0=BB=D1=8E=D1=87=D0=B5=D0=BD=D0=B8=D0=B8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Замечания CodeRabbit к релизному мержу. Все пять проверены по актуальному коду, все оказались в силе. * Ветка успеха RetireCurrentSessionAndReconnectAsync безусловно забирала _reconnectCts и диспозила его. За время await ConnectInternalAsync и WaitForConnectionAsync конкурентный путь (RestartReconnectLoop из падающего OnConnected-хендлера) успевает установить НОВЫЙ источник — и его отменяли вместе с обнулением счётчика, оставляя новую последовательность без источника. Метод теперь запоминает созданный им самим источник и снимает состояние только если _reconnectCts всё ещё ссылается на него — так же, как проверки владения в ReconnectLoopAsync * StartReconnectLoop: параметр initialAttempts стал мёртвым после появления RestartReconnectLoop (все три вызова шли без аргумента), удалён вместе с документацией; свежая последовательность начинается с нуля явно * TestUOnConnectedHandlerFailure: комментарий к ассерту роста бэкоффа врал. С ReconnectBaseDelay 100 мс и ReconnectMaxDelay 1 с задержки идут 400, 800, 1000 мс — третья упирается в cap, то есть первый-к-последнему это ~2.5x, а не 4x. Добавлено, что ассерт держится лишь пока cap выше более ранних значений: при cap 400 мс все интервалы легли бы на него и сравнялись * TestUReconnectSessionRaces: laterPort выдаётся до await'ов, поэтому перед StartMock добавлена проверка IsPortStillFree — иначе занятый порт проявился бы таймаутом финального ассерта, а не внятным сообщением * Blazor-демо: client.connection.GetUrl() заменён на client.Url() во всех трёх оставшихся местах — интерфейсный метод, и он же уже использовался в правке смены сервера Гонку из первого пункта тестом не воспроизвести: нужно попасть в окно между await'ами, а через публичный API оно недостижимо — с убранной проверкой владения все тесты переподключения проходят (проверено мутацией). Правка обоснована тем же инвариантом, что и остальные проверки владения в этом файле. Проверено: сборка решения без ошибок, юнит-тесты 883/883. --- .../Blazor-WebAssembly/Pages/Index.razor | 6 ++-- .../Client/TestUOnConnectedHandlerFailure.cs | 8 +++-- .../Client/TestUReconnectSessionRaces.cs | 6 ++++ Xrpl/Client/connection.cs | 36 +++++++++++-------- 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor b/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor index e2eee217..0a213b84 100644 --- a/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor +++ b/Tests/TestsClients/Blazor-WebAssembly/Pages/Index.razor @@ -397,7 +397,7 @@ await InvokeAsync(async () => { IsConnected = true; - CurrentServerUrl = client.connection.GetUrl(); + CurrentServerUrl = client.Url(); CurrentReconnectInfo = null; SessionStartTime = DateTime.Now; @@ -617,7 +617,7 @@ client.connection.OnTransaction += _onTransactionHandler; client.connection.OnLedgerClosed += _onLedgerClosedHandler; - CurrentServerUrl = client.connection.GetUrl(); + CurrentServerUrl = client.Url(); CurrentConnectionState = client.connection.CurrentConnectionState; await base.OnInitializedAsync(); @@ -627,7 +627,7 @@ if (client.connection.IsConnected()) { IsConnected = true; - CurrentServerUrl = client.connection.GetUrl(); + CurrentServerUrl = client.Url(); CurrentConnectionState = client.connection.CurrentConnectionState; StateHasChanged(); } diff --git a/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs b/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs index c8c372e5..efc97fb5 100644 --- a/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs +++ b/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs @@ -298,8 +298,12 @@ public async Task TestRepeatedOnConnectedFailuresBackOff() } } - // ReconnectBaseDelay is 100ms and CalcBackoff doubles per attempt with 25% jitter, - // so the third gap is ~4x the first even at the extremes of the jitter range. + // CalcBackoff doubles per attempt off ReconnectBaseDelay (100ms), capped at + // ReconnectMaxDelay (1s), with 25% jitter. The handler-failure path seeds the counter + // with its consecutive-failure count, so the delays run 400ms, 800ms, then 1s (capped) + // — first to last is ~2.5x nominally, and still grows at the jitter extremes. + // This holds only while the configured cap stays above the earlier backoff values: with + // a cap at or below 400ms every gap would sit on the cap and compare equal. // Comparing first vs last rather than each consecutive pair keeps the assertion // robust: what regressed before was a flat sequence, not the exact multiplier. Assert.IsTrue( diff --git a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs index 7d434a45..8896ce43 100644 --- a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs +++ b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs @@ -265,6 +265,12 @@ public async Task TestFailedConnectDuringReconnectLeavesLoopRunning() } // The server appears. Nobody touches the client from here on. + // The port was handed out before the awaits above, so confirm it is still free: the mock + // binds without throwing to the caller, and a port taken meanwhile would surface as the + // reconnect assertion below timing out instead of a clear conflict. + Assert.IsTrue( + TestUtils.IsPortStillFree(laterPort), + $"Port {laterPort} was taken by another process while the test held it — rerun."); _secondRippled = StartMock(laterPort); DateTime deadline = DateTime.UtcNow + TimeSpan.FromSeconds(40); diff --git a/Xrpl/Client/connection.cs b/Xrpl/Client/connection.cs index 261bfdf0..cbb17003 100644 --- a/Xrpl/Client/connection.cs +++ b/Xrpl/Client/connection.cs @@ -572,12 +572,14 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // so a concurrent stop/start cannot dispose the source created here. Cancellation and // disposal of the old source happen after the lock is released. CancellationTokenSource oldCts; + CancellationTokenSource ownCts; lock (_reconnectStateLock) { oldCts = _reconnectCts; _reconnectLoop = null; // Clear old loop reference so StartReconnectLoop can start a new one _reconnectAttempts = 1; - _reconnectCts = new CancellationTokenSource(); + ownCts = new CancellationTokenSource(); + _reconnectCts = ownCts; } oldCts?.Cancel(); @@ -664,12 +666,19 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // Note: _reconnectMode will be cleared in OnceOpen when connection is fully established _isFastReconnectActive = false; - CancellationTokenSource settled; + // Only tear down the source this method installed. The awaits above give a concurrent + // path (RestartReconnectLoop from a failing OnConnected handler, say) room to install a + // newer one; cancelling and disposing that would strand the sequence it belongs to, + // which is the same wedge the ownership checks in ReconnectLoopAsync guard against. + CancellationTokenSource settled = null; lock (_reconnectStateLock) { - settled = _reconnectCts; - _reconnectCts = null; - _reconnectAttempts = 0; + if (ReferenceEquals(_reconnectCts, ownCts)) + { + settled = ownCts; + _reconnectCts = null; + _reconnectAttempts = 0; + } } settled?.Cancel(); @@ -2050,14 +2059,13 @@ private void ClearReconnectState() _reconnectMode = ReconnectMode.None; } - /// - /// Value to seed _reconnectAttempts with when a fresh reconnect sequence starts. - /// Defaults to 0 — a genuine new sequence begins at the base delay. The OnConnected-handler - /// path passes its own consecutive-failure count instead: that path tears the loop down and - /// starts it again on every failure, so with a 0 seed the backoff would restart at - /// ReconnectBaseDelay each time and never grow. - /// - private void StartReconnectLoop(int initialAttempts = 0) + /// + /// Starts a reconnect loop unless one is already running, reusing a pre-created cancellation + /// source when there is one. A fresh sequence always begins at the base delay; the + /// OnConnected-handler path needs a seeded counter instead and uses + /// . + /// + private void StartReconnectLoop() { // Set reconnect mode to LoopReconnect (upgrades from FastReconnect or sets from None) _reconnectMode = ReconnectMode.LoopReconnect; @@ -2091,7 +2099,7 @@ private void StartReconnectLoop(int initialAttempts = 0) // Retire the old CTS after the lock is released - see _reconnectStateLock retired = existingCts; _reconnectCts = new CancellationTokenSource(); - _reconnectAttempts = initialAttempts; + _reconnectAttempts = 0; } // else: Reuse existing valid CTS (pre-created for fast reconnect) // Don't reset _reconnectAttempts - this is continuation of existing reconnect sequence From 5f5ce9269c88949b5cfd4f31648d9e3dcc23506d Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Thu, 6 Aug 2026 22:35:20 -0300 Subject: [PATCH 2/5] =?UTF-8?q?fix(client):=20=D1=81=D0=BD=D0=B8=D0=BC?= =?UTF-8?q?=D0=B0=D1=82=D1=8C=20=D1=81=D1=81=D1=8B=D0=BB=D0=BA=D1=83=20?= =?UTF-8?q?=D0=BD=D0=B0=20=D1=86=D0=B8=D0=BA=D0=BB=20=D0=B8=20=D0=B2=20?= =?UTF-8?q?=D0=B2=D0=B5=D1=82=D0=BA=D0=B5=20=D1=83=D1=81=D0=BF=D0=B5=D1=85?= =?UTF-8?q?=D0=B0=20=D0=B1=D1=8B=D1=81=D1=82=D1=80=D0=BE=D0=B3=D0=BE=20?= =?UTF-8?q?=D0=BF=D0=B5=D1=80=D0=B5=D0=BF=D0=BE=D0=B4=D0=BA=D0=BB=D1=8E?= =?UTF-8?q?=D1=87=D0=B5=D0=BD=D0=B8=D1=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Селф-ревью PR #79 тремя независимыми проходами (Opus) — нашлось одно блокирующее и четыре неточности в моих же комментариях. Блокирующее: ветка успеха RetireCurrentSessionAndReconnectAsync забирала _reconnectCts, но оставляла _reconnectLoop — та же дыра, которую PR #78 закрыл в StopReconnectLoop, и здесь я её не заметил. Метод обнуляет _reconnectLoop на входе, поэтому за время await'ов конкурентный OnConnectionFailed видит «цикла нет», а StartReconnectLoop переиспользует ещё валидный ownCts и ставит на него новый цикл. Дальше ветка успеха отменяла источник, но ссылку на задачу оставляла: все проверки loopIsRunning видели выходящую задачу и отступали, а сам цикл вставал по проверке владения. Переподключаться было некому. Ссылка теперь снимается в той же транзакции. Неточности комментариев, все мои: * обоснование guard'а порта было скопировано из TestUChangeServerFailure, где StartMock уходит в фоновый поток. В этом файле он теперь синхронный (я сам так сделал по прошлому нитпику), поэтому занятый порт даёт SocketException сразу, а не таймаут ассерта. Плюс отмечено, что guard — диагностика: он сам биндит и отпускает, TOCTOU остаётся * XML-док StartReconnectLoop обещал «always begins at the base delay». Счётчик свежей последовательности равен нулю, инкремент идёт до расчёта, значит первая задержка — CalcBackoff(1), то есть вдвое больше базовой; а на путях ping-timeout и network-drop первая попытка вообще без задержки * remarks у _reconnectStateLock перечисляли известные исключения, но пропускали предпроверки «цикл уже идёт?» в OnConnectionFailed и OnceClose — они читают невалатильный _reconnectLoop вне лока * комментарий в catch-ветке утверждал, что источник заведомо на месте и будет переиспользован, — ровно то допущение, которое ветка успеха теперь объявляет неверным. Переписан с разбором обоих исходов * добавлена явная фраза, что при потере владения ownCts освобождать не нужно: его отменил и задиспозил тот, кто вытеснил Проверено: сборка решения без ошибок, юнит-тесты 883/883. Живой прогон Blazor-демо против локальной ноды 3.2.1 — обрыв соединения (нода убита при активном клиенте, код 1005) даёт восстановление следующей же попыткой после подъёма ноды, отключение/подключение, переключения на мёртвые порты и возврат на живой проходят, стрим ledger-ов идёт, необработанных исключений в консоли нет. --- .../Client/TestUOnConnectedHandlerFailure.cs | 3 +- .../Client/TestUReconnectSessionRaces.cs | 8 +++-- Xrpl/Client/connection.cs | 30 +++++++++++++++---- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs b/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs index efc97fb5..a47f8a46 100644 --- a/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs +++ b/Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs @@ -303,7 +303,8 @@ public async Task TestRepeatedOnConnectedFailuresBackOff() // with its consecutive-failure count, so the delays run 400ms, 800ms, then 1s (capped) // — first to last is ~2.5x nominally, and still grows at the jitter extremes. // This holds only while the configured cap stays above the earlier backoff values: with - // a cap at or below 400ms every gap would sit on the cap and compare equal. + // a cap at or below 400ms every gap would sit on the cap, and the comparison would come + // down to which way the jitter fell — a coin flip, not a stable result. // Comparing first vs last rather than each consecutive pair keeps the assertion // robust: what regressed before was a flat sequence, not the exact multiplier. Assert.IsTrue( diff --git a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs index 8896ce43..1ce93c07 100644 --- a/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs +++ b/Tests/Xrpl.Tests/Client/TestUReconnectSessionRaces.cs @@ -265,9 +265,11 @@ public async Task TestFailedConnectDuringReconnectLeavesLoopRunning() } // The server appears. Nobody touches the client from here on. - // The port was handed out before the awaits above, so confirm it is still free: the mock - // binds without throwing to the caller, and a port taken meanwhile would surface as the - // reconnect assertion below timing out instead of a clear conflict. + // The port was handed out before the awaits above, so check it is still free. StartMock + // binds on this thread, so a port taken meanwhile would come out as a raw SocketException + // from the line below; this turns it into a statement of the actual cause. Diagnostics, + // not a fix: the check itself binds and releases, so the port can still be lost between + // here and StartMock. Assert.IsTrue( TestUtils.IsPortStillFree(laterPort), $"Port {laterPort} was taken by another process while the test held it — rerun."); diff --git a/Xrpl/Client/connection.cs b/Xrpl/Client/connection.cs index cbb17003..3482ca56 100644 --- a/Xrpl/Client/connection.cs +++ b/Xrpl/Client/connection.cs @@ -296,9 +296,11 @@ private static WebSocketClient CreateWebSocket(string url, ConnectionOptions con /// /// /// Not every touch of these fields is covered: the per-iteration _reconnectAttempts++ in - /// , and the plain resets in ChangeServer and - /// OnceClose, still run outside it. Those predate this lock; do not read the list above - /// as "all three fields are always synchronized". + /// , the plain resets in ChangeServer and + /// OnceClose, and the "is a loop already running" pre-checks in + /// OnConnectionFailed and OnceClose (which read _reconnectLoop, a + /// non-volatile field, outside the lock) all still run outside it. Those predate this lock; do + /// not read the list above as "all three fields are always synchronized". /// /// /// @@ -670,6 +672,8 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // path (RestartReconnectLoop from a failing OnConnected handler, say) room to install a // newer one; cancelling and disposing that would strand the sequence it belongs to, // which is the same wedge the ownership checks in ReconnectLoopAsync guard against. + // When ownership is lost, ownCts needs no cleanup here: whoever evicted it from the + // field cancelled and disposed it as part of doing so. CancellationTokenSource settled = null; lock (_reconnectStateLock) { @@ -678,6 +682,15 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) settled = ownCts; _reconnectCts = null; _reconnectAttempts = 0; + + // Drop the task reference in the same transaction, for the same reason + // StopReconnectLoop does: a loop may have been started on this very source + // while the awaits above were running (OnConnectionFailed sees no live loop - + // the entry above cleared the reference - and StartReconnectLoop reuses a + // still-valid source). Cancelling that source without clearing the reference + // leaves every loopIsRunning check looking at a task that is exiting, so + // nobody starts a replacement and nobody reconnects. + _reconnectLoop = null; } } @@ -689,7 +702,12 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // If Connect fails, transition to loop reconnect mode // Keep _reconnectMode set (will be LoopReconnect after StartReconnectLoop) - // _reconnectCts is already set, so StartReconnectLoop will reuse it + // StartReconnectLoop reuses the source installed above when it is still there. It may + // not be: the awaits could have let another path replace or clear it, the same way the + // success branch above can no longer assume it still owns ownCts. Either outcome is + // survivable here - a live foreign loop makes the call return early, a cleared source + // makes it start a fresh sequence (losing only the seeded first delay) - so this path + // does not need an ownership check of its own. SetConnectionState( XrpConnectionState.RestoringConnection, message: $"Reconnection failed: {ex.Message}. Retrying...", @@ -2061,7 +2079,9 @@ private void ClearReconnectState() /// /// Starts a reconnect loop unless one is already running, reusing a pre-created cancellation - /// source when there is one. A fresh sequence always begins at the base delay; the + /// source when there is one. A fresh sequence starts its attempt counter at zero, so the first + /// delay is CalcBackoff(1) — twice ReconnectBaseDelay — except on the + /// ping-timeout and network-drop paths, where the first attempt skips the delay entirely. The /// OnConnected-handler path needs a seeded counter instead and uses /// . /// From 612aaffc378f9f8e7d2ff5e21790159feff5f83e Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Thu, 6 Aug 2026 22:42:59 -0300 Subject: [PATCH 3/5] =?UTF-8?q?fix(client):=20=D0=BD=D0=B5=20=D1=82=D0=B5?= =?UTF-8?q?=D1=80=D1=8F=D1=82=D1=8C=20=D1=86=D0=B8=D0=BA=D0=BB=20=D0=BD?= =?UTF-8?q?=D0=B0=20=D0=B8=D1=81=D0=BA=D0=BB=D1=8E=D1=87=D0=B5=D0=BD=D0=B8?= =?UTF-8?q?=D0=B8=20=D0=BF=D0=BE=D1=82=D1=80=D0=B5=D0=B1=D0=B8=D1=82=D0=B5?= =?UTF-8?q?=D0=BB=D1=8F=20=D0=B8=20=D0=BD=D0=B5=20=D0=BE=D0=B6=D0=B8=D0=B2?= =?UTF-8?q?=D0=BB=D1=8F=D1=82=D1=8C=20=D0=BF=D0=BE=D1=81=D0=BB=D0=B5=20Dis?= =?UTF-8?q?connect?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Два предсуществующих дефекта в RetireCurrentSessionAndReconnectAsync, найденные селф-ревью PR #79. Оба в том же методе, чей жизненный цикл этот PR и приводит в порядок, поэтому закрываются здесь же. 1. Исключение из потребительского обработчика уносило сессию с собой. SetConnectionState синхронно зовёт OnConnectionStatus, а метод исполняется только на ping-путях, где вызывающие глушат всё. Бросок из обработчика вылетал из метода, оставляя установленный источник отмены без цикла и без владельца: клиент замирал в RestoringConnection, а CheckIfNotConnected по непустому _reconnectCts считал, что попытка идёт, из-за чего WaitForConnectionAsync выжигал весь ConnectionAcquisitionTimeout. Обе нотификации обёрнуты. В catch-ветке вдобавок изменён порядок: StartReconnectLoop вызывается ДО уведомления — без цикла клиент не вернётся, а сообщение вторично. 2. Метод оживлял клиент после пользовательского Disconnect(). Disconnect ставит _permanentlyDisconnected, чистит состояние переподключения и ждёт ping-задачу — ту самую, внутри которой крутится этот метод, то есть он ещё не завершился. Метод же безусловно сбрасывал флаг и поднимал новую сессию: Disconnect возвращался с отчётом об успехе, пока за ним собиралось живое соединение. Теперь флаг проверяется перед сбросом: если пользователь отключился, метод освобождает свой источник (по проверке владения) и уходит. Та же проверка добавлена в catch-ветку перед запуском цикла. Тестами эти пути не покрыты, и это не оговорка: все три вызова метода — на ping-путях, а во всех юнит-тестах клиента стоит UseCustomPing = false, то есть метод в тестах не исполняется вовсе. Тест возможен (UseCustomPing = true плюс мок, не отвечающий на ping), но интервал пинга захардкожен 20 с в трёх местах и конфигом не управляется, так что прогон вышел бы на 25-40 с. Дешёвый тест появится, если интервал сделать настраиваемым — отдельная задача. Проверено: сборка решения без ошибок, юнит-тесты 883/883. --- Xrpl/Client/connection.cs | 82 +++++++++++++++++++++++++++++++++------ 1 file changed, 71 insertions(+), 11 deletions(-) diff --git a/Xrpl/Client/connection.cs b/Xrpl/Client/connection.cs index 3482ca56..d828c6df 100644 --- a/Xrpl/Client/connection.cs +++ b/Xrpl/Client/connection.cs @@ -588,11 +588,22 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) oldCts?.Dispose(); // 4. Now send first notification - IsReconnectActive() will return true - SetConnectionState( - XrpConnectionState.RestoringConnection, - message: $"{reason} Reconnecting immediately...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); + // Guarded: SetConnectionState invokes the consumer's OnConnectionStatus synchronously, and + // this method only ever runs on a ping path whose callers swallow everything. An exception + // from a consumer handler would therefore leave the source installed above with no loop and + // nobody to dispose it - the client would sit in RestoringConnection forever. + try + { + SetConnectionState( + XrpConnectionState.RestoringConnection, + message: $"{reason} Reconnecting immediately...", + ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo()); + } + catch (Exception notifyError) + { + Debug.WriteLine($"{DateTime.Now}OnConnectionStatus handler threw while entering fast reconnect: {notifyError.Message}"); + } // ===================================================== // FAST RECONNECT with PER-SESSION ISOLATION (same as ChangeServer) @@ -650,7 +661,34 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) _pingTimeoutSocket = null; _networkDropSocket = null; - // 11. Reset permanentlyDisconnected for new connection + // 11. Reset permanentlyDisconnected for new connection - unless the user asked to disconnect + // while the awaits above were running. Disconnect() sets the flag, clears the reconnect + // state and then waits on the ping task this method runs inside, so it is still blocked + // here and cannot have finished its teardown. Clearing its flag and reconnecting anyway + // would resurrect a client the consumer explicitly took down - and Disconnect() would + // return reporting success while a fresh session was being built behind it. + if (_permanentlyDisconnected) + { + CancellationTokenSource abandoned = null; + lock (_reconnectStateLock) + { + if (ReferenceEquals(_reconnectCts, ownCts)) + { + abandoned = ownCts; + _reconnectCts = null; + _reconnectAttempts = 0; + _reconnectLoop = null; + } + } + + abandoned?.Cancel(); + abandoned?.Dispose(); + _isFastReconnectActive = false; + + Debug.WriteLine($"{DateTime.Now}Fast reconnect abandoned before connecting - the client was disconnected by the user"); + return; + } + _permanentlyDisconnected = false; // Note: _reconnectAttempts and _reconnectCts already set at the start of this method @@ -702,18 +740,40 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // If Connect fails, transition to loop reconnect mode // Keep _reconnectMode set (will be LoopReconnect after StartReconnectLoop) + // A user Disconnect() can land while the awaits above are running - and it will wait on + // the very ping task this method runs inside, so it cannot have finished yet. Handing + // the client back to a reconnect loop then would undo an explicit disconnect. The flag + // is the authority: leave the state alone and let Disconnect() finish its teardown. + if (_permanentlyDisconnected) + { + Debug.WriteLine($"{DateTime.Now}Fast reconnect abandoned - the client was disconnected by the user: {ex.Message}"); + return; + } + + // Start the loop BEFORE notifying: SetConnectionState calls into consumer code, and an + // exception from a handler must not cost us the reconnect loop. Ordering matters more + // than the message here - without the loop the client never comes back. + // // StartReconnectLoop reuses the source installed above when it is still there. It may // not be: the awaits could have let another path replace or clear it, the same way the // success branch above can no longer assume it still owns ownCts. Either outcome is // survivable here - a live foreign loop makes the call return early, a cleared source // makes it start a fresh sequence (losing only the seeded first delay) - so this path // does not need an ownership check of its own. - SetConnectionState( - XrpConnectionState.RestoringConnection, - message: $"Reconnection failed: {ex.Message}. Retrying...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); StartReconnectLoop(); + + try + { + SetConnectionState( + XrpConnectionState.RestoringConnection, + message: $"Reconnection failed: {ex.Message}. Retrying...", + ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo()); + } + catch (Exception notifyError) + { + Debug.WriteLine($"{DateTime.Now}OnConnectionStatus handler threw while reporting a failed fast reconnect: {notifyError.Message}"); + } } } From d87a5c4737b83b89f57afa46de90170d02eeffa8 Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Thu, 6 Aug 2026 23:21:30 -0300 Subject: [PATCH 4/5] =?UTF-8?q?refactor(client):=20=D0=B2=D1=8B=D0=BD?= =?UTF-8?q?=D0=B5=D1=81=D1=82=D0=B8=20=D0=B8=D0=BD=D1=82=D0=B5=D1=80=D0=B2?= =?UTF-8?q?=D0=B0=D0=BB=20=D0=B8=20=D0=BF=D0=BE=D1=80=D0=BE=D0=B3=20health?= =?UTF-8?q?-check=20=D0=B2=20=D0=BA=D0=BE=D0=BD=D1=84=D0=B8=D0=B3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Проверка живости соединения жила на двух захардкоженных числах: интервал таймера 20 с (в трёх местах — обычный Timer и WASM-таймер) и порог неактивности 60 с. Теперь это HealthCheckInterval и InactivityTimeout в ConnectionOptions, значения по умолчанию прежние. Мотивация — тестируемость fast-reconnect. Все вызовы RetireCurrentSessionAndReconnectAsync стоят за этим таймером, поэтому путь, правленный в трёх PR подряд (#72, #78, #79), ни разу не исполнялся ни в одном тесте: во всех тестах клиента UseCustomPing = false. С настраиваемыми порогами он достижим за миллисекунды вместо минуты. Тесты этим коммитом НЕ добавлены, и вот почему: воспроизвести обрыв на CreateMockRippled не удалось. mock.Stop() гасит listener, но клиентский ClientWebSocket остаётся в состоянии Open — он узнаёт о смерти пира только на следующем вводе-выводе, а ошибка keepalive-пинга состояние не меняет. Пробовал паузы до 8 с: клиент так и не входит в RestoringConnection, то есть тест зелёный на несработавшем сценарии — хуже, чем никакого. Ветка порога неактивности тоже недостижима: мок отвечает даже на неизвестные команды, поэтому активность обновляется каждый тик и порог не переступается. Чтобы покрыть путь по-настоящему, нужно одно из двух: научить мок обрывать соединение (сейчас у него только Start/Stop) или поставить в тестах TCP-прокси между клиентом и сервером и рвать его. Обе опции — отдельная задача; эти два свойства делают её выполнимой и попутно убирают магические числа. Проверено: сборка решения без ошибок, юнит-тесты 883/883. --- Xrpl/Client/connection.cs | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/Xrpl/Client/connection.cs b/Xrpl/Client/connection.cs index d828c6df..a2a0d582 100644 --- a/Xrpl/Client/connection.cs +++ b/Xrpl/Client/connection.cs @@ -212,6 +212,32 @@ public class ConnectionOptions /// public bool UseCheckHealth { get; set; } = false; + /// + /// Gets or sets how often the background health check runs — the timer that notices a socket + /// which is no longer Open and hands the client to the fast-reconnect path.
+ /// Default: 20 seconds, the interval this check has always used. + /// + /// + /// Exposed primarily so tests can exercise the ping and fast-reconnect paths without waiting + /// out the default interval; those paths were previously unreachable from a unit test, which + /// is why they went uncovered through several fixes. Lowering it in production only makes the + /// state check more frequent — it sends no network requests of its own. + /// + public TimeSpan HealthCheckInterval { get; set; } = TimeSpan.FromSeconds(20); + + /// + /// Gets or sets how long a connection may go without any inbound activity before the health + /// check treats it as dead and hands it to the fast-reconnect path.
+ /// Default: 60 seconds, the threshold this check has always used. + /// + /// + /// A socket whose peer vanished stays Open until the next I/O, so silence is the only + /// signal available without sending traffic. Exposed together with + /// so the fast-reconnect path is reachable from a test in + /// under a second instead of over a minute. + /// + public TimeSpan InactivityTimeout { get; set; } = TimeSpan.FromSeconds(60); + /// /// Gets or sets the policy that determines how failed requests are handled. /// @@ -2421,8 +2447,8 @@ private void StartWasmPingTimer(CancellationTokenSource cts) _ = ExecutePingCheckAndReleaseAsync(innerCts, tcs); }, state: cts, - dueTime: 20000, - period: 20000); + dueTime: (int)config.HealthCheckInterval.TotalMilliseconds, + period: (int)config.HealthCheckInterval.TotalMilliseconds); } private async Task ExecutePingCheckAndReleaseAsync(CancellationTokenSource cts, TaskCompletionSource tcs) @@ -2491,11 +2517,13 @@ private async Task ExecutePingCheckAsync(CancellationTokenSource cts) return; } - if (timeSinceLastActivity > 60) + double inactivityLimit = config.InactivityTimeout.TotalSeconds; + if (timeSinceLastActivity > inactivityLimit) { _pingTimeoutSocket = ws; - await RetireCurrentSessionAndReconnectAsync("Connection timeout (no activity for 60+ seconds)."); + await RetireCurrentSessionAndReconnectAsync( + $"Connection timeout (no activity for {inactivityLimit:F0}+ seconds)."); return; } @@ -2606,7 +2634,7 @@ private void StartPingTimer() } else { - pingTimer = new Timer(20000); + pingTimer = new Timer(config.HealthCheckInterval.TotalMilliseconds); pingTimer.Elapsed += (sender, e) => { if (cts.IsCancellationRequested) From f58633b83891097c29cc6b208c437fb02c3b3690 Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Sat, 8 Aug 2026 17:42:44 -0300 Subject: [PATCH 5/5] =?UTF-8?q?fix(client):=20=D0=B3=D0=B0=D1=81=D0=B8?= =?UTF-8?q?=D1=82=D1=8C=20=D0=B8=D1=81=D0=BA=D0=BB=D1=8E=D1=87=D0=B5=D0=BD?= =?UTF-8?q?=D0=B8=D1=8F=20=D0=BD=D0=BE=D1=82=D0=B8=D1=84=D0=B8=D0=BA=D0=B0?= =?UTF-8?q?=D1=86=D0=B8=D0=B8=20=D0=B2=20=D0=BE=D0=B4=D0=BD=D0=BE=D0=B9=20?= =?UTF-8?q?=D1=82=D0=BE=D1=87=D0=BA=D0=B5=20=D0=B8=20=D0=B2=D0=B0=D0=BB?= =?UTF-8?q?=D0=B8=D0=B4=D0=B8=D1=80=D0=BE=D0=B2=D0=B0=D1=82=D1=8C=20health?= =?UTF-8?q?-check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Три замечания CodeRabbit к PR #79. 1. (Major) Исключение обработчика внутри ReconnectLoopAsync. В прошлом коммите я обернул нотификации в fast-reconnect, но сам цикл переподключения тоже зовёт SetConnectionState — перед первой попыткой соединения. Бросок из OnConnectionStatus ронял задачу цикла, оставляя _reconnectCts установленным без живого цикла: восстанавливать соединение становилось некому. Латать по местам бессмысленно — защита перенесена в саму SetConnectionState, через которую проходят все уведомления класса. Две локальные обёртки убраны как избыточные; обработчик потребителя больше не может уронить машину состояний ниоткуда. 2. (Major, частично) Пользовательский Disconnect против летящего fast reconnect. ConnectInternalAsync и WaitForConnectionAsync теперь получают токен сессии, которой владеет метод: Disconnect отменяет её, и попытка прекращается вместо открытия сокета за спиной у отключённого клиента. Проверки флага для этого недостаточно — Disconnect ждёт ping-задачу считанные секунды, а получение соединения может длиться дольше. Полностью замечание не закрыто: остаётся принципиальный вопрос, вправе ли автоматические пути вообще сбрасывать _permanentlyDisconnected, — это отдельная задача, здесь взято то, что устраняет главное окно. 3. (Minor) Валидация новых опций. HealthCheckInterval на WASM-пути кастится в int миллисекунд: ноль выстреливает один раз и больше не повторяется, значения вне диапазона таймер отвергает сам. Оба свойства проверяются в ValidateConfig, то есть в конструкторе Connection, с указанием имени опции. TestUHealthCheckOptions покрывает границы: ноль, отрицательное, за пределами int.MaxValue, неположительный InactivityTimeout, и приёмку 1 мс и значений по умолчанию. Проверено: сборка решения без ошибок, юнит-тесты 888/888. --- .../Client/TestUHealthCheckOptions.cs | 95 +++++++++++++++++++ Xrpl/Client/connection.cs | 94 ++++++++++-------- 2 files changed, 151 insertions(+), 38 deletions(-) create mode 100644 Tests/Xrpl.Tests/Client/TestUHealthCheckOptions.cs diff --git a/Tests/Xrpl.Tests/Client/TestUHealthCheckOptions.cs b/Tests/Xrpl.Tests/Client/TestUHealthCheckOptions.cs new file mode 100644 index 00000000..f8f18fd3 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUHealthCheckOptions.cs @@ -0,0 +1,95 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Threading.Tasks; + +using Xrpl.Client; + +using XrplTests; + +namespace Xrpl.Tests +{ + /// + /// Boundary checks for the health-check timing options. Both feed a timer directly — + /// HealthCheckInterval is cast to an int of milliseconds on the WASM path, where zero + /// fires once and never repeats and out-of-range values are rejected by the timer itself — so a + /// bad value has to fail on the way in, naming the option, rather than quietly disabling the + /// check that recovers dead connections. + /// + [TestClass] + public class TestUHealthCheckOptions + { + private static XrplClient CreateClient(TimeSpan? healthCheckInterval = null, TimeSpan? inactivityTimeout = null) => + new XrplClient("ws://127.0.0.1:1", new XrplClient.ClientOptions + { + UseCustomPing = true, + UseCheckHealth = true, + HealthCheckInterval = healthCheckInterval ?? TimeSpan.FromSeconds(20), + InactivityTimeout = inactivityTimeout ?? TimeSpan.FromSeconds(60), + ConnectionAttemptTimeout = TimeSpan.FromSeconds(1), + ConnectionAcquisitionTimeout = TimeSpan.FromSeconds(1), + }); + + [TestMethod] + public void TestZeroHealthCheckIntervalIsRejected() + { + // Validation runs while the connection is being constructed, so the bad value is rejected + // before a client exists to connect with. + ArgumentException error = Helper.ThrowsException( + () => CreateClient(healthCheckInterval: TimeSpan.Zero)); + StringAssert.Contains(error.Message, "HealthCheckInterval"); + } + + [TestMethod] + public void TestNegativeHealthCheckIntervalIsRejected() + { + ArgumentException error = Helper.ThrowsException( + () => CreateClient(healthCheckInterval: TimeSpan.FromMilliseconds(-1))); + StringAssert.Contains(error.Message, "HealthCheckInterval"); + } + + [TestMethod] + public void TestOutOfRangeHealthCheckIntervalIsRejected() + { + // Past int.MaxValue milliseconds - the WASM timer cannot represent it + ArgumentException error = Helper.ThrowsException( + () => CreateClient(healthCheckInterval: TimeSpan.FromDays(30))); + StringAssert.Contains(error.Message, "HealthCheckInterval"); + } + + [TestMethod] + public void TestNonPositiveInactivityTimeoutIsRejected() + { + ArgumentException error = Helper.ThrowsException( + () => CreateClient(inactivityTimeout: TimeSpan.Zero)); + StringAssert.Contains(error.Message, "InactivityTimeout"); + } + + /// + /// The lower bound is 1ms, and the defaults must keep working — otherwise every existing + /// consumer would start failing on connect. + /// + [TestMethod] + public async Task TestBoundaryAndDefaultValuesAreAccepted() + { + // 1ms is the documented minimum: validation must let it through. Nothing is listening on + // port 1, so the connect attempt fails on the transport - not on config validation. + XrplClient atMinimum = CreateClient( + healthCheckInterval: TimeSpan.FromMilliseconds(1), + inactivityTimeout: TimeSpan.FromMilliseconds(1)); + + Exception minimumError = await Helper.ThrowsExceptionAsync(() => atMinimum.Connect()); + Assert.IsNotInstanceOfType( + minimumError, + typeof(ArgumentException), + $"1ms should pass validation, but connect failed with: {minimumError.Message}"); + + XrplClient atDefaults = CreateClient(); + Exception defaultError = await Helper.ThrowsExceptionAsync(() => atDefaults.Connect()); + Assert.IsNotInstanceOfType( + defaultError, + typeof(ArgumentException), + $"The default options should pass validation, but connect failed with: {defaultError.Message}"); + } + } +} diff --git a/Xrpl/Client/connection.cs b/Xrpl/Client/connection.cs index a2a0d582..84afbe80 100644 --- a/Xrpl/Client/connection.cs +++ b/Xrpl/Client/connection.cs @@ -263,6 +263,23 @@ private void ValidateConfig() throw new ArgumentException( $"ConnectionAcquisitionTimeout ({config.ConnectionAcquisitionTimeout.TotalSeconds}s) must be >= ConnectionAttemptTimeout ({config.ConnectionAttemptTimeout.TotalSeconds}s) to allow at least one full connection attempt."); } + + // The WASM timer takes this as an int of milliseconds: zero fires once and never repeats, + // and anything past int.MaxValue or below zero is rejected outright by the timer itself. + // Fail here instead, where the message can say which option is wrong. + double healthCheckMs = config.HealthCheckInterval.TotalMilliseconds; + if (healthCheckMs < 1 || healthCheckMs > int.MaxValue) + { + throw new ArgumentException( + $"HealthCheckInterval ({config.HealthCheckInterval}) must be between 1ms and {int.MaxValue}ms."); + } + + if (config.InactivityTimeout <= TimeSpan.Zero) + { + throw new ArgumentException( + $"InactivityTimeout ({config.InactivityTimeout}) must be positive - a non-positive value would " + + "treat every connection as dead on the first health check."); + } } // https://github.com/XRPLF/xrpl.js/blob/main/packages/xrpl/src/client/connection.ts createWebSocket @@ -433,14 +450,27 @@ private void SetConnectionState( _previousNotifiedMessage = message; - OnConnectionStatus?.Invoke( - new ConnectionStatusInfo - { - Message = message, - Severity = severity, - Reconnect = reconnect, - ConnectionState = newState, - }); + // Contained here, once, rather than at each call site. Every state notification in this class + // funnels through this method, and several call sites are places where an escaping exception + // costs the client its reconnect: the fast-reconnect path (running on a ping task whose + // callers swallow everything) and ReconnectLoopAsync, which notifies before its first + // connection attempt and would fault with _reconnectCts still installed and no live loop. + // A consumer's status handler must not be able to take the connection down. + try + { + OnConnectionStatus?.Invoke( + new ConnectionStatusInfo + { + Message = message, + Severity = severity, + Reconnect = reconnect, + ConnectionState = newState, + }); + } + catch (Exception notifyError) + { + Debug.WriteLine($"{DateTime.Now}OnConnectionStatus handler threw for state {newState}: {notifyError.Message}"); + } } private ReconnectInfo BuildReconnectInfo(int? explicitAttempt = null, TimeSpan? delay = null) @@ -614,22 +644,13 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) oldCts?.Dispose(); // 4. Now send first notification - IsReconnectActive() will return true - // Guarded: SetConnectionState invokes the consumer's OnConnectionStatus synchronously, and - // this method only ever runs on a ping path whose callers swallow everything. An exception - // from a consumer handler would therefore leave the source installed above with no loop and - // nobody to dispose it - the client would sit in RestoringConnection forever. - try - { - SetConnectionState( - XrpConnectionState.RestoringConnection, - message: $"{reason} Reconnecting immediately...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); - } - catch (Exception notifyError) - { - Debug.WriteLine($"{DateTime.Now}OnConnectionStatus handler threw while entering fast reconnect: {notifyError.Message}"); - } + // Consumer handler exceptions are contained inside SetConnectionState - an escaping throw + // here would leave the source installed above with no loop and nobody to dispose it. + SetConnectionState( + XrpConnectionState.RestoringConnection, + message: $"{reason} Reconnecting immediately...", + ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo()); // ===================================================== // FAST RECONNECT with PER-SESSION ISOLATION (same as ChangeServer) @@ -725,8 +746,12 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // and Connecting would overwrite ReconnectInfo, confusing consuming apps try { - await ConnectInternalAsync().ConfigureAwait(false); - await WaitForConnectionAsync(config.ConnectionAcquisitionTimeout, CancellationToken.None).ConfigureAwait(false); + // Pass the token of the session this method owns: a user Disconnect() cancels it, so the + // attempt below stops instead of opening a socket behind a client that was taken down. + // Disconnect() waits only briefly for the ping task, while acquisition can run much + // longer, so the flag check above cannot cover this window on its own. + await ConnectInternalAsync(ownCts.Token).ConfigureAwait(false); + await WaitForConnectionAsync(config.ConnectionAcquisitionTimeout, ownCts.Token).ConfigureAwait(false); // Connect succeeded - cleanup reconnect state // Note: _reconnectMode will be cleared in OnceOpen when connection is fully established @@ -788,18 +813,11 @@ private async Task RetireCurrentSessionAndReconnectAsync(string reason) // does not need an ownership check of its own. StartReconnectLoop(); - try - { - SetConnectionState( - XrpConnectionState.RestoringConnection, - message: $"Reconnection failed: {ex.Message}. Retrying...", - ConnectionCloseSeverity.Warning, - reconnect: BuildReconnectInfo()); - } - catch (Exception notifyError) - { - Debug.WriteLine($"{DateTime.Now}OnConnectionStatus handler threw while reporting a failed fast reconnect: {notifyError.Message}"); - } + SetConnectionState( + XrpConnectionState.RestoringConnection, + message: $"Reconnection failed: {ex.Message}. Retrying...", + ConnectionCloseSeverity.Warning, + reconnect: BuildReconnectInfo()); } }