fix(client): исключение из OnConnected больше не убивает клиент навсегда (10.10.1.0) - #72
fix(client): исключение из OnConnected больше не убивает клиент навсегда (10.10.1.0)#72Platonenkov wants to merge 5 commits into
Conversation
…гда (10.10.1.0) Connection.OnceOpen ловил любое исключение потребительского обработчика OnConnected и звал Disconnect() - пользовательский путь отключения: _permanentlyDisconnected = true плюс ClearReconnectState(). После этого клиент мёртв навсегда: реконнект-цикл не перезапускается, новых сокетов не открывается, OnConnected больше не поднимается, а любой запрос падает с NotConnectedException. Наружу при этом не выходило ничего - ни события, ни лога. Триггер - самый штатный сценарий: перезапуск ноды. OnConnected - типовое место восстановления подписок (SDK их после реконнекта не восстанавливает), а нода принимает TCP раньше, чем начинает отвечать на запросы, поэтому первый subscribe уходит в RequestTimeout и падает. В проде так встал флот ботов - по четыре часа тишины после обновления ноды. Теперь сбой обработчика трактуется как отказ СОЕДИНЕНИЯ, а не как воля пользователя: сокет сносится, дальше работает обычный реконнект с экспоненциальным бэкоффом, флаг постоянного отключения не ставится. Защита от вечного цикла: OnceOpen чистит реконнект-состояние до вызова обработчика, поэтому счётчик попыток самого цикла обнуляется на каждом успешном TCP-коннекте и сойтись не может. Подряд идущие сбои обработчика считаются отдельно (_connectHandlerFailures, сбрасывается при успешном прогоне обработчика, в Connect() и в ChangeServer()); при достижении MaxReconnectAttempts с StopAfterMaxAttempts клиент осознанно сдаётся - сразу понятный NotConnectedException вместо пятиминутного молчания. Причина теперь наблюдаема: исключение поднимается через OnError с errorMessage = "connectHandlerError" и через OnConnectionStatus. Попутно: WebSocketClient.SendMessageAsync (async void, вызывается без await) больше не глотает ошибку отправки молча - она трассируется. Поведение не меняется, запрос по-прежнему ограничен RequestTimeout, но причина перестала быть невидимой при диагностике. TestUOnConnectedHandlerFailure закрывает все четыре свойства против mock rippled: восстановление после разового сбоя, работоспособность клиента после восстановления, отчёт через OnError и остановка вечно падающего обработчика.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe client now bounds ChangesConnection recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OnConnected
participant ReconnectLoopAsync
participant NewServer
Client->>OnConnected: Invoke after connection
OnConnected-->>Client: Throw exception
Client->>ReconnectLoopAsync: Start bounded reconnect
ReconnectLoopAsync->>NewServer: Attempt connection
NewServer-->>ReconnectLoopAsync: Accept or reject connection
ReconnectLoopAsync-->>Client: Restore connection or report permanent failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Xrpl/Client/connection.cs`:
- Around line 1690-1704: The give-up path loses its detailed notification and
does not promptly unblock waiting callers. In the giveUp branch of the
connection failure handler, call SetConnectionState with the detailed
disconnected message before await Disconnect(), then update
WaitForConnectionAsync to check _permanentlyDisconnected on every loop iteration
and throw NotConnectedException immediately; preserve the existing
reconnect-attempt guard and messages for other termination cases.
- Around line 1716-1721: The failed-connection cleanup must preserve the
original socket identity instead of potentially closing a newly connected
socket. In the cleanup block, use failedSocket as the socketToClose value and
clear ws only when it still references failedSocket; update the logic around the
disconnect lock without altering unrelated reconnect behavior.
In `@Xrpl/Client/WebSocketClient.cs`:
- Around line 311-314: Replace the DEBUG-only Debug.WriteLine in the WebSocket
send failure handler with a production-visible error path. Route the exception
through the WebSocket error callback and connection.OnError, or the project’s
established production logger, while preserving the existing diagnostic message
and ensuring fire-and-forget send failures are reported immediately.
In `@Xrpl/Xrpl.csproj`:
- Line 17: Update the PackageVersion property in the Xrpl.AddressCodec,
Xrpl.BinaryCodec, and Xrpl.Keypairs project files from 10.9.0.0 to 10.10.1.0,
matching the release version already declared in Xrpl.csproj.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 631b8cf1-fb0a-4e15-8417-218a5e5e9302
📒 Files selected for processing (5)
CHANGES.mdTests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.csXrpl/Client/WebSocketClient.csXrpl/Client/connection.csXrpl/Xrpl.csproj
1. Детальная причина отказа терялась. SetConnectionState после Disconnect() - no-op: Disconnect() уже перевёл состояние в Disconnected, а SetConnectionState уведомляет только при смене состояния. Подписчик видел "Disconnected by user request." вместо причины. Уведомление перенесено перед Disconnect(). 2. WaitForConnectionAsync проверял _permanentlyDisconnected один раз на входе и никогда - внутри цикла ожидания. Ждущий в Connect() вызывающий досиживал весь ConnectionAcquisitionTimeout (по умолчанию 5 минут) и получал общий TimeoutException вместо реальной причины. Проверка перенесена в цикл. 3. Личность сокета при разборе. WebSocketClient.Connect вызывает OnConnect без await, поэтому connect-лок отпускается, пока обработчик ещё выполняется, и к моменту разбора ws может указывать уже на новый сокет. Закрывается ровно тот сокет, для которого падал обработчик; ws обнуляется, только если всё ещё ссылается на него; если сокет уже не текущий - реконнект-состояние принадлежит новому соединению и не трогается. При этом тест на вечно падающий обработчик вскрыл гонку в самом фиксе: проверка "цикл реконнекта уже работает" гонится с выходом этого цикла - он прерывается сразу, как сокет отрапортовал Open, то есть ДО того, как обработчик успел упасть. Проигрыш гонки оставлял клиента без реконнекта - ровно тот клин, который правится. Вместо проверки путь теперь безусловно забирает владение: гасит текущий цикл и запускает новый, а пришедший позже OnceClose видит живой цикл и корректно отступает. Побочно: восстановление стало занимать сотни миллисекунд вместо десятков секунд. 4. Ошибка отправки: Debug.WriteLine вырезается без DEBUG, которого нет в релизной сборке. Мёртвый колбэк ошибки WebSocketClient (его никто не звал и не подписывал) теперь доносит исключение до Connection.OnError с errorMessage = "socketSendError". Только уведомление: неудачная отправка сама по себе не означает потерю соединения, реконнект не запускается, запрос по-прежнему ограничен RequestTimeout. Отклонено: подъём версий Xrpl.AddressCodec / Xrpl.BinaryCodec / Xrpl.Keypairs до 10.10.1.0. Эти пакеты в этом PR не менялись, версионируются независимо и уже расходятся с основным на NuGet осознанно (Xrpl 10.10.0 против base 10.9.0; так же было в 10.9.1.0 и 10.10.0.0). Публикация идёт с --skip-duplicate, так что неизменные пакеты просто пропускаются, а подъём выложил бы побайтово те же артефакты под новым номером. Тест TestPermanentlyFailingOnConnectedHandlerStops усилен: теперь проверяет, что ожидающий вызывающий разблокируется именно NotConnectedException.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Xrpl/Client/connection.cs (1)
1657-1674: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider narrowing the
tryblock to the consumerOnConnectedinvocation.The
tryblock also coversSetConnectionStateat Line 1666.SetConnectionStateinvokes the consumerOnConnectionStatusevent. If anOnConnectionStatussubscriber throws, the catch classifies the failure as anOnConnectedhandler failure. The client then increments_connectHandlerFailures, reportsconnectHandlerError, and tears down a healthy socket. The reported reason is then wrong, and a broken status subscriber can drive the bounded retry to the give-up state.♻️ Proposed narrowing
try { connectionManager.ResolveAllAwaiting(); if (OnConnected is not null) { await OnConnected?.Invoke(); } - - Interlocked.Exchange(ref _connectHandlerFailures, value: 0); - SetConnectionState(XrpConnectionState.Connected, message: $"Connected {url}"); } catch (Exception error) { connectionManager.RejectAllAwaiting(error); await OnConnectHandlerFailedAsync(connectedSocket, error); return; // Don't start ping timer if connection failed } + + Interlocked.Exchange(ref _connectHandlerFailures, value: 0); + SetConnectionState(XrpConnectionState.Connected, message: $"Connected {url}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Client/connection.cs` around lines 1657 - 1674, Restrict the try/catch in the connection-success flow to ResolveAllAwaiting and the OnConnected invocation only. Move resetting _connectHandlerFailures and SetConnectionState outside that catch so exceptions from OnConnectionStatus are not handled by OnConnectHandlerFailedAsync or used to tear down the healthy socket; preserve the existing failure handling for OnConnected errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs`:
- Around line 25-59: Add teardown support for the mock server created in
MyTestInitialize by retaining its Server instance and exposing a method that
calls Server.Stop(). Update MyTestCleanup to invoke this teardown after
disconnecting and clearing _client, ensuring the listener is stopped for every
test.
In `@Xrpl/Client/connection.cs`:
- Around line 1775-1783: Guard reconnect-loop cleanup so a retired loop cannot
modify the session created by StartReconnectLoop. In the reconnect-loop worker
and its tail cleanup, capture the loop-owned reconnect state and only update
_reconnectMode, _reconnectAttempts, or dispose _reconnectCts when _reconnectLoop
still identifies that same active loop; otherwise leave the replacement session
untouched. Preserve StopReconnectLoop’s cancellation behavior while making
teardown ownership-aware.
---
Nitpick comments:
In `@Xrpl/Client/connection.cs`:
- Around line 1657-1674: Restrict the try/catch in the connection-success flow
to ResolveAllAwaiting and the OnConnected invocation only. Move resetting
_connectHandlerFailures and SetConnectionState outside that catch so exceptions
from OnConnectionStatus are not handled by OnConnectHandlerFailedAsync or used
to tear down the healthy socket; preserve the existing failure handling for
OnConnected errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: febcc39f-611c-4d57-8d6e-d4776825ddf3
📒 Files selected for processing (5)
CHANGES.mdTests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.csXrpl/Client/WebSocketClient.csXrpl/Client/connection.csXrpl/Xrpl.csproj
1. Реконнект-цикл писал в чужую сессию. StopReconnectLoop() отменяет токен, но не дожидается цикла, поэтому снятый цикл мог добраться до тела или хвоста уже после того, как установлена замена, и обнулить _reconnectMode живого цикла, сбросить его _reconnectAttempts или выбросить его _reconnectCts. Дефект существовал и раньше (RetireCurrentSessionAndReconnectAsync снимает циклы ровно так же), но путь сбоя OnConnected делает его гораздо более достижимым. ReconnectLoopAsync теперь принимает CancellationTokenSource, которым владеет, и трогает общее состояние, только пока этот источник остаётся активным. 2. Mock-сервер в тестах не останавливался: CreateMockRippled.Start() создавал Server (его конструктор поднимает слушателя) и терял ссылку. Добавлен Stop(), TestUOnConnectedHandlerFailure зовёт его в TestCleanup.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 26 minutes. |
Тот же класс клина, что и основной фикс PR, но другой путь. Найдено при прогоне Blazor-демо: переключение селектора сети на выключенную ноду. ChangeServer ставил ГЛОБАЛЬНЫЙ _isIntentionalDisconnect = true, чтобы отфильтровать поздние колбэки уходящего сокета, а сбрасывался этот флаг только в OnceOpen. Если новый сервер не поднимался, OnceOpen не выполнялся никогда: OnConnectionFailed читал отказ НОВОГО соединения как отключение по воле пользователя, писал "Connection closed permanently.", не запускал реконнект-цикл, и дальше всё - включая сам ChangeServer - падало с вводящим в заблуждение "No connection attempt in progress. Call Connect() first.". Поднятие сервера потом ничего не меняло: клиент был мёртв. Поздние колбэки теперь фильтруются исключительно по-сокетному отслеживанием, которое здесь и так уже было (_userInitiatedSockets плюс собственный флаг сокета, выставляемый в RetireOldSessionAsync) - ровно так же, как всегда делал путь ping-timeout / network-drop; в его коде даже висит комментарий, предостерегающий от глобального флага именно по этой причине. Дополнительно флаг явно гасится на входе, чтобы ChangeServer после пользовательского Disconnect() не подавлялся оставшимся от него значением. Дефект существовал до этого PR - проверено дважды: ни одна из строк этого пути им не менялась, и одинаковый диагностический тест на чистом dev (f17fc11) даёт байт-в-байт тот же вывод. TestUChangeServerFailure закрывает оба случая: клиент доходит до нового сервера, когда тот появляется, с предшествующим Disconnect() и без него. Оба теста падают на dev и проходят с фиксом.
|
Добавлен второй фикс того же класса — 120a5ba. Как нашёлся. Прогон Blazor-демо ( Причина. Исправление. Поздние колбэки фильтруются только по-сокетным отслеживанием, которое здесь и так уже было ( Дефект существовал до этого PR, проверено дважды: ни одна строка этого пути PR-ом не менялась, и одинаковый диагностический тест на чистом Проверка. @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
Xrpl/Client/connection.cs (1)
1784-1792: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake reconnect-loop ownership checks atomic with state changes.
ReferenceEquals(_reconnectCts, ownCts)is only a time-of-check. A replacement loop can start after Line 2015, Line 2104, or Line 2142 and before the following write.A retired loop can then reset the replacement loop’s attempt counter or clear its reconnect mode. It can also retire the replacement session and clear its
wsreference in the later session-cleanup block.Serialize loop replacement and loop-owned state changes with one reconnect lifecycle lock. Check ownership and mutate
_reconnectAttempts,_reconnectMode,_reconnectCts,_activeSession, andwsin that protected operation.Based on learnings:
ReconnectLoopAsynccan be replaced while its prior task is still completing, so ownership must prevent the retired loop from changing replacement-session state.Also applies to: 2015-2020, 2104-2108, 2139-2158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Client/connection.cs` around lines 1784 - 1792, Protect reconnect-loop replacement and all loop-owned state transitions with a single reconnect lifecycle lock. In ReconnectLoopAsync and the related cleanup paths around the referenced ownership checks, perform the ReferenceEquals validation and mutations of _reconnectAttempts, _reconnectMode, _reconnectCts, _activeSession, and ws atomically under that lock, so retired loops cannot alter replacement-loop or replacement-session state. Apply the same synchronization to the replacement flow using StopReconnectLoop, _reconnectLoop, and StartReconnectLoop.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/Xrpl.Tests/CreateMockRippled.cs`:
- Around line 71-96: Synchronize CreateMockRippled startup and shutdown by
protecting _server access with a shared lock and recording that Stop() has begun
before checking the current server. Update Start() so a server created after
shutdown is immediately stopped rather than assigned to _server, while
preserving the existing cleanup and exception behavior.
---
Outside diff comments:
In `@Xrpl/Client/connection.cs`:
- Around line 1784-1792: Protect reconnect-loop replacement and all loop-owned
state transitions with a single reconnect lifecycle lock. In ReconnectLoopAsync
and the related cleanup paths around the referenced ownership checks, perform
the ReferenceEquals validation and mutations of _reconnectAttempts,
_reconnectMode, _reconnectCts, _activeSession, and ws atomically under that
lock, so retired loops cannot alter replacement-loop or replacement-session
state. Apply the same synchronization to the replacement flow using
StopReconnectLoop, _reconnectLoop, and StartReconnectLoop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2df686f3-4407-459e-87df-eac4524def91
📒 Files selected for processing (5)
CHANGES.mdTests/Xrpl.Tests/Client/TestUChangeServerFailure.csTests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.csTests/Xrpl.Tests/CreateMockRippled.csXrpl/Client/connection.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.cs
Start() выполняется на фоновом потоке, поэтому Stop() из TestCleanup мог увидеть _server == null до присваивания и оставить живого слушателя. Останов теперь фиксируется под локом: старт, завершившийся после него, гасит собственный Server вместо того, чтобы его сохранить.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Проблема
Connection.OnceOpenловил любое исключение потребительского обработчикаOnConnectedи звалDisconnect()— то есть пользовательский путь отключения:_permanentlyDisconnected = trueплюсClearReconnectState().После этого клиент мёртв навсегда: реконнект-цикл не перезапускается, новых сокетов не открывается,
OnConnectedбольше не поднимается, а любой запрос падает сNotConnectedException("Client has been disconnected. Call Connect() to reconnect."). Наружу не выходило ничего — ни события, ни лога.Триггер — самый штатный сценарий: перезапуск ноды.
OnConnected— типовое место восстановления подписок (SDK их после реконнекта не восстанавливает), а нода принимает TCP раньше, чем начинает отвечать на запросы. Первый жеsubscribeуходит вRequestTimeout(40 с) и падает. Потребитель, который «упал громко, пусть SDK переподключится», получал ровно обратное — тихую смерть клиента. В проде так встал флот ботов: по четыре часа тишины после обновления ноды, один из них похоронил себя за 69 секунд до того, как нода стала доступна.Что сделано
OnceOpenчистит реконнект-состояние до вызова обработчика, поэтому счётчик попыток самого цикла обнуляется на каждом успешном TCP-коннекте и сойтись не может. Подряд идущие сбои обработчика считаются отдельно (_connectHandlerFailures; сбрасывается при успешном прогоне обработчика, вConnect()и вChangeServer()). При достиженииMaxReconnectAttemptsсStopAfterMaxAttemptsклиент осознанно сдаётся — сразу понятныйNotConnectedExceptionвместо пятиминутного молчания;Connect()обнуляет счётчик, так что восстановление остаётся возможным. СStopAfterMaxAttempts = falseпопытки продолжаются — это ровно то, о чём просит опция.OnErrorсerrorMessage = "connectHandlerError"(та же форма, что уже используется для сбоев stream-обработчиков) и черезOnConnectionStatus. Раньше причина смерти не сообщалась нигде.WebSocketClient.SendMessageAsync(async void, вызывается безawaitизConnection.WebsocketSendAsync) больше не глотает ошибку отправки молча — она трассируется. Поведение не меняется (запрос по-прежнему ограниченRequestTimeout), но причина перестала быть невидимой при диагностике.Совместимость
Потребители, сознательно бросавшие из
OnConnectedради «жёсткой остановки», получат другое поведение. Судя по комментарию в коде (Don't start ping timer if connection failed), задумка была «не считать соединение установленным», а не «убить клиент». Жёсткая остановка при этом сохранена как бэкстоп: с дефолтнымиStopAfterMaxAttempts = true/MaxReconnectAttempts = 5вечно падающий обработчик всё равно приводит к терминальному состоянию, только после 5 попыток и с внятным сообщением, а не с первой.Проверка
Дефект сначала воспроизведён тестом на текущем
dev— падал ровно с той ошибкой из прода:TestUOnConnectedHandlerFailure(mock rippled) закрывает четыре свойства:TestTransientOnConnectedFailureRecoversOnConnectedподнимается сноваTestClientIsUsableAfterOnConnectedFailureTestPermanentlyFailingOnConnectedHandlerStopsMaxReconnectAttemptsTestOnConnectedFailureIsReportedThroughOnErrorOnErrorсconnectHandlerErrorЛокально, до и после:
dotnet test --filter "TestU");.ci-config/docker-compose.ci.yml(xrpld 3.2.0 в Docker) — 219 пройдено, 40 пропущено, 0 упало (dotnet test --filter "TestI"), прогон повторён на финальной сборке.Версия:
10.10.0.0→10.10.1.0.Summary by CodeRabbit
Bug Fixes
Tests
Documentation