RPC pool: park a node that keeps failing, drop the unreachable one, per-node stats - #77
Conversation
…drop the unreachable node, per-node stats Closes #76
PR Summary by QodoRPC pool hygiene: failure parking, remove unreachable node, add per-node stats
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e68bee4f84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| RecordLatency(h, elapsedMs); | ||
| } | ||
| if (h.ConsecutiveFailures >= FailureParkThreshold) |
There was a problem hiding this comment.
When the default failoverThreshold of 2 is used and a node enters a call with two consecutive failures, the first failed attempt parks it here, but HiveRpcClient.Send has already captured the ordered node list and continues its inner retry loop because timeouts and connection failures do not set AdvanceImmediately. The supposedly parked node therefore receives a fourth attempt before failover, adding another full timeout and immediately extending its initial 30-second park to 60 seconds. The retry loop should stop when RecordFailure transitions the node into the parked state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Applied in a64dbf9: RecordFailure now returns whether the node is parked after this failure, and both failure branches in Send break out of the same-node retry on true. Test: with failoverThreshold 2 the third consecutive failure makes exactly one attempt and parks for 30s, not two attempts and 60s.
| counter.RecordUpstream(Environment.TickCount64 - started); | ||
| var elapsed = Environment.TickCount64 - started; | ||
| counter.RecordUpstream(elapsed); | ||
| if (elapsed > BudgetMs) Interlocked.Increment(ref counter.SlowFill); |
There was a problem hiding this comment.
When Client.CallMethod exceeds BudgetMs and then throws, such as after a node timeout or slow transport failure, execution jumps directly to the catch block before this increment. Every waiting reader can therefore time out while slow_fill remains zero, causing the new stats to hide slow fills precisely during upstream outages. Record the elapsed duration and update this counter on failed upstream calls as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Applied in a64dbf9: the budget check moved into a finally around the upstream call, so a fill that outruns the budget and then throws increments slow_fill as well. Test: a 400ms upstream that answers an RPC error under a 100ms budget is one reader timeout and one slow fill.
|
Warning Review limit reached
Next review available in: 41 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds injectable node-health tracking with failure parking, timeout classification, per-node statistics, and slow-fill metrics. It removes an unreachable default node and adds integration tests for failover, fallback, health reporting, and coalesced SSR requests. ChangesNode health and failover
SSR statistics
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR improves RPC node failure handling and adds per-node statistics without any identified merge-blocking issue; it is merge-ready after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…throws is still a slow fill
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…, 429s do not count toward parking
Code Review by Qodo
1. Parking permits probe bursts
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
1 participant
Closes #76.
Found while attributing the SSR cache's lookup timeouts on one origin: a single sample of the service's upstream sockets showed 83 half-open connects to
hive-api.arcange.euat once, with the failover target's connection count jumping right after. That node never completes a TCP connect from any host this service runs on. The tracker let it happen: a timeout below the 2s "slow failure" floor left no latency sample, so the node kept its unexplored standing (neutral 1s prior); only 429s parked a node, and a failure demoted one for 30s at most. So each time the leading nodes each recorded one transient failure, the dead node was next in line and every in-flight fill went to it at the full per-node timeout.NodeHealthTracker: a timeout is recorded as a latency sample whatever the floor (the floor still tells instant refusals from slow 5xx); three consecutive failures park the node for 30s, doubling to 120s, a success clears the streak; a failure-parked node is skipped while any other node is available and probed once its park lapses; when every node is parked all are offered. Per-node lifetime counters (calls, ok, failures, timeouts, rate-limited) and aSnapshot(). Clock is an injectable seam per tracker (internal client constructor) so the windows are tested deterministically.HiveRpcClient: the timeout branch tags its exception;HealthSnapshot()(host names only);hive-api.arcange.euremoved from the default pool with the reason next to the existing exclusions.SsrRpc:slow_fillper method (fills that outran the budget;timeoutstays per waiting reader, so one slow fill on a hot key is one slow fill and many timeouts) and anodesarray in/private-api/ssr/stats.Summary by CodeRabbit
/statsresponse, including availability, latency, failures, timeouts, and rate limits.