fix(client): бэкофф при повторных сбоях OnConnected + замечания CodeRabbit к релизному PR #76 - #77
Merged
Merged
Conversation
…abbit к PR #76 * Бэкофф не рос при повторяющихся сбоях OnConnected-хендлера. Путь OnConnectHandlerFailedAsync сносит цикл переподключения и запускает заново на каждом сбое; StopReconnectLoop обнуляет _reconnectAttempts, свежая последовательность обнуляет его ещё раз, а CalcBackoff считает задержку только по этому счётчику. При StopAfterMaxAttempts = false ветки сдачи нет вовсе, и клиент бесконечно повторял connect -> сбой хендлера -> teardown с постоянной ReconnectBaseDelay — устойчивая нагрузка ровно на ту ноду, которая ещё не умеет обслуживать запросы. StartReconnectLoop получил параметр начального значения счётчика, путь сбоя хендлера засевает его своим числом последовательных сбоев. TestRepeatedOnConnectedFailuresBackOff это пинит: с откаченным фиксом тест показывает ~100 переподключений за 20 с с ровным интервалом ~200 мс * _reconnectCts объявлен volatile: цикл сравнивает его по ссылке, решая, владеет ли он ещё состоянием переподключения, а пишут его три метода из других потоков. Устаревшее чтение дало бы retired-циклу лишнюю итерацию либо увело бы владеющий цикл раньше времени. Соседние кросс-потоковые поля уже volatile * Гайд по кредитованию (обе языковые версии): таблица Loan Fields перечисляла четыре имени, которых у ledger-объекта нет — Account (заёмщик лежит в Borrower), а также Counterparty, PrincipalRequested и PaymentTotal, которые являются полями транзакции LoanSet. После удаления PrincipalRequested из LOLoan гайд обещал бы несуществующее свойство * TestUtils.GetFreePort больше не выдаёт один порт дважды в пределах процесса: ОС вправе вернуть только что освобождённый порт, классы тестов идут параллельно, и второй mock падал бы при бинде в фоновом потоке — это выглядело как таймаут, а не как конфликт. TestUChangeServerFailure дополнительно проверяет порт прямо перед стартом второго mock, чтобы остаточная внешняя гонка падала внятно * RippledLedgerFlags.Parse падает на повторно объявленном объекте, как это уже делает RippledLedgerEntryFormats.Parse. Проверено мутацией (дублирование блока Offer), фикстура после проверки восстановлена * Фикстуры в тестовом .csproj подключены через None Update вместо None Include — дефолтный glob SDK их уже включает Версия не бампится: 10.11.0.0 ещё не выпущен, правки дописаны в его раздел. Проверено: сборка решения без ошибок, юнит-тесты 880/880; обе вендоренные фикстуры сверены с пинами через curl | diff.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change preserves reconnect backoff after repeated ChangesReliability and documentation updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Connection
participant OnConnectedHandler
participant ReconnectLoop
Connection->>OnConnectedHandler: invoke after connection
OnConnectedHandler-->>Connection: return failure
Connection->>ReconnectLoop: restart with prior failure count
ReconnectLoop->>Connection: retry after increasing backoff
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai rate limit |
|
You're currently rate limited 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. Your next review will be available in 41 minutes. |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview 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: 2
🤖 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/Models/RippledLedgerFlags.cs`:
- Around line 95-106: The duplicate-name check in Parse must run before the
flags.Count == 0 early-skip so flagless duplicate LEDGER_OBJECT definitions are
rejected. Add and update a separate HashSet<string> for every parsed object name
before that branch, then retain objects solely for flagged entries and preserve
the existing duplicate exception behavior.
In `@Xrpl/Client/connection.cs`:
- Around line 290-294: Synchronize the full _reconnectCts lifecycle across
StopReconnectLoop(), StartReconnectLoop(),
RetireCurrentSessionAndReconnectAsync(), and the reconnect-loop creation path.
Use one shared lock or equivalent mechanism to atomically capture the current
source, cancel and dispose that captured instance, replace or clear the field,
and create/install new loops; do not read _reconnectCts separately for
cancellation, disposal, or assignment, and preserve the existing ownership
checks while preventing a stop path from touching a newly installed source.
🪄 Autofix
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: c18155d3-b73f-407f-a776-6c131765e6a3
📒 Files selected for processing (9)
CHANGES.mdDocFx/LendingProtocol-Guide.mdDocFx/LendingProtocol-Guide.ru.mdTests/Xrpl.Tests/Client/TestUChangeServerFailure.csTests/Xrpl.Tests/Client/TestUOnConnectedHandlerFailure.csTests/Xrpl.Tests/Models/RippledLedgerFlags.csTests/Xrpl.Tests/TestUtils.csTests/Xrpl.Tests/Xrpl.Tests.csprojXrpl/Client/connection.cs
Comment on lines
+95
to
+106
| // Indexer assignment would let a second block of the same name replace the | ||
| // first, dropping that object from the conformance table while flagCount still | ||
| // grew — the minimum-count guard below would not notice. Same rule as | ||
| // RippledLedgerEntryFormats.Parse, so the two parsers stay consistent | ||
| if (objects.ContainsKey(name)) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"{name}: declared twice in LedgerFormats.h — the parser would drop one " + | ||
| "definition, update it before trusting this test"); | ||
| } | ||
|
|
||
| objects.Add(name, flags); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check duplicate names before skipping flagless objects.
The flags.Count == 0 branch runs before this duplicate check. Therefore, Parse() still accepts duplicate LEDGER_OBJECT names when one definition has no parsed flags. Track every object name in a separate HashSet<string> before the empty-flags check. Keep objects for flagged entries.
Proposed fix
+ HashSet<string> seenObjectNames = new(StringComparer.Ordinal);
foreach (Match block in ObjectBlock.Matches(header))
{
string name = block.Groups["name"].Value;
+ if (!seenObjectNames.Add(name))
+ {
+ throw new InvalidOperationException(
+ $"{name}: declared twice in LedgerFormats.h — update it before trusting this test");
+ }
+
Dictionary<string, uint> flags = new();
...
- if (objects.ContainsKey(name))
- {
- throw new InvalidOperationException(...);
- }
-
objects.Add(name, flags);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Indexer assignment would let a second block of the same name replace the | |
| // first, dropping that object from the conformance table while flagCount still | |
| // grew — the minimum-count guard below would not notice. Same rule as | |
| // RippledLedgerEntryFormats.Parse, so the two parsers stay consistent | |
| if (objects.ContainsKey(name)) | |
| { | |
| throw new InvalidOperationException( | |
| $"{name}: declared twice in LedgerFormats.h — the parser would drop one " + | |
| "definition, update it before trusting this test"); | |
| } | |
| objects.Add(name, flags); | |
| HashSet<string> seenObjectNames = new(StringComparer.Ordinal); | |
| foreach (Match block in ObjectBlock.Matches(header)) | |
| { | |
| string name = block.Groups["name"].Value; | |
| if (!seenObjectNames.Add(name)) | |
| { | |
| throw new InvalidOperationException( | |
| $"{name}: declared twice in LedgerFormats.h — update it before trusting this test"); | |
| } | |
| Dictionary<string, uint> flags = new(); | |
| // ... | |
| objects.Add(name, flags); | |
| } |
🤖 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 `@Tests/Xrpl.Tests/Models/RippledLedgerFlags.cs` around lines 95 - 106, The
duplicate-name check in Parse must run before the flags.Count == 0 early-skip so
flagless duplicate LEDGER_OBJECT definitions are rejected. Add and update a
separate HashSet<string> for every parsed object name before that branch, then
retain objects solely for flagged entries and preserve the existing duplicate
exception behavior.
Comment on lines
+290
to
+294
| // Volatile: StopReconnectLoop, StartReconnectLoop and RetireCurrentSessionAndReconnectAsync | ||
| // write this from other threads, and the loop compares it by reference to decide whether it | ||
| // still owns the reconnect state. A stale read would let a retired loop run one more | ||
| // iteration, or make the owning loop stand down. Matches the other cross-thread fields here. | ||
| private volatile CancellationTokenSource _reconnectCts; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Synchronize the _reconnectCts lifecycle.
volatile does not make the cancellation-source transition atomic. StopReconnectLoop() reads _reconnectCts separately for Cancel(), Dispose(), and assignment to null. If StartReconnectLoop() installs a new source between these reads, the stale stop path can dispose and clear the new source. The new loop can then receive null or a disposed source and stop reconnecting.
Protect cancellation-source replacement, cancellation, disposal, and loop creation with one shared synchronization mechanism. Capture the source once before cancelling and disposing it.
Also applies to: 1794-1802, 1968-2003
🤖 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 290 - 294, Synchronize the full
_reconnectCts lifecycle across StopReconnectLoop(), StartReconnectLoop(),
RetireCurrentSessionAndReconnectAsync(), and the reconnect-loop creation path.
Use one shared lock or equivalent mechanism to atomically capture the current
source, cancel and dispose that captured instance, replace or clear the field,
and create/install new loops; do not read _reconnectCts separately for
cancellation, disposal, or assignment, and preserve the existing ownership
checks while preventing a stop path from touching a newly installed source.
Замечание CodeRabbit к PR #77: проверка дубликата стояла ПОСЛЕ `if (flags.Count == 0) continue`, поэтому имя, объявленное дважды, проскакивало, если одно из объявлений разбиралось без флагов. Имена теперь отслеживаются отдельным HashSet до этой ветки, а `objects` по-прежнему хранит только флагованные записи. Проверено мутацией именно этого сценария: вставка второго `LEDGER_OBJECT(Offer, )` с пустым телом теперь даёт "Offer: declared twice in LedgerFormats.h", а до правки проходила молча. Фикстура после проверки восстановлена и сверена с пином через curl | diff. Побочно всплыло, что PreserveNewest не обновляет копию фикстуры в bin, когда исходник возвращают из git (у восстановленного файла время правки старше копии): после мутационных проверок каталог Fixtures в bin нужно удалять, иначе тесты идут против подделанного файла. На этом и попались два прогона. Проверено: юнит-тесты 880/880.
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.
Разбор всех шести замечаний CodeRabbit к релизному PR #76 (
dev→release). Все приняты, ни одно не отклонено.Главное: бэкофф не рос при повторных сбоях
OnConnected🟠 Major, и единственное замечание про поведение продукта.
Путь
OnConnectHandlerFailedAsyncсносит цикл переподключения и запускает заново на каждом сбое хендлера.StopReconnectLoopобнуляет_reconnectAttempts, свежая последовательность обнуляет его ещё раз, аCalcBackoffсчитает задержку только по этому счётчику. Ветка сдачи бывает только приStopAfterMaxAttempts = true; приfalseеё нет вовсе — и клиент бесконечно повторял connect → сбой хендлера → teardown с постояннойReconnectBaseDelay. То есть устойчивая нагрузка ровно на ту ноду, которая приняла TCP, но ещё не обслуживает запросы, — сценарий, ради которого фикс #72 и писался.StartReconnectLoopтеперь принимает начальное значение счётчика, а путь сбоя хендлера засевает его собственным числом последовательных сбоев (_connectHandlerFailures), которое и так уже считается и сбрасывается при успешном хендлере,Connect()иChangeServer().Покрыто тестом и проверено мутацией.
TestRepeatedOnConnectedFailuresBackOffтребует, чтобы последний интервал между вызовами хендлера был больше первого. С откаченным фиксом тест падает и печатает всю картину:— около ста переподключений за 20 секунд с ровным интервалом ~200 мс. Ровно то, что описал ревьюер.
Остальные пять
_reconnectCts→volatile(🔵). Цикл сравнивает его по ссылке, решая, владеет ли он ещё состоянием переподключения, а пишут его три метода из других потоков. Устаревшее чтение дало бы retired-циклу лишнюю итерацию либо увело бы владеющий цикл раньше времени. Соседние кросс-потоковые поля (_permanentlyDisconnected,_reconnectMode,_isIntentionalDisconnect) ужеvolatile— этот выбивался.Гайд по кредитованию (🟡, outside-diff). Ревьюер нашёл одну неверную строку, при сверке с фикстурой их оказалось четыре:
Account(заёмщик лежит вBorrower), а такжеCounterparty,PrincipalRequestedиPaymentTotal— все три поля транзакцииLoanSet, а не ledger-объекта. После удаленияPrincipalRequestedизLOLoanв этом релизе гайд обещал бы свойство, которого больше нет. Исправлены обе языковые версии, добавлено примечание, куда эти три поля относятся на самом деле.Гонка свободного порта (🟡). Чинил в корне:
TestUtils.GetFreePortбольше не выдаёт один порт дважды в пределах процесса. ОС вправе вернуть только что освобождённый порт, а классы тестов идут параллельно (test.runsettings) — два вызова могли получить один порт, и второй mock падал бы при бинде в фоновом потоке, то есть наружу это выглядело бы 30-секундным таймаутом, а не конфликтом. Полностью закрыть гонку нельзя: тестам нужен именно закрытый порт, так что окно между «отдали порт» и «забиндили» неустранимо — зато внутрипроцессные коллизии, единственная достижимая половина, исчезли. ПлюсTestUChangeServerFailureпроверяет порт прямо перед стартом второго mock, чтобы остаточный внешний случай падал внятно.Дубликат в
RippledLedgerFlags.Parse(🔵). Моя недоделка из #75: guard добавлен в парсер полей, а в парсер флагов не перенесён. Теперь оба ведут себя одинаково. Проверено мутацией — дублирование блокаOfferдаётOffer: declared twice in LedgerFormats.h; фикстура после проверки восстановлена.None Include→None Update(🟠 по их шкале). Дефолтный glob SDK эти файлы уже включает. Практического сбоя не было — я специально собрал с-v nи грепнул: ниNETSDK1022, ни предупреждений о дублях. Но объявление корректнее, и после правки фикстуры по-прежнему копируются (.ref— нет, они и не должны).Версия
Не бампится.
10.11.0.0ещё не выпущен — он и есть содержимое релизного PR #76, поэтому правки дописаны в его же разделCHANGES.md, а не заводят новый номер.Проверка
curl … | diffпосле мутаций — байт-в-байтПосле мержа этого PR релизный #76 подхватит изменения автоматически (он из
dev).Summary by CodeRabbit
Bug Fixes
Documentation