Skip to content

fix(client): eliminate resolver probation starvation in LossThenLatency balancing strategy - #206

Open
taskkillstar wants to merge 2 commits into
masterking32:mainfrom
taskkillstar:fix/strategy6-probation-starvation
Open

fix(client): eliminate resolver probation starvation in LossThenLatency balancing strategy#206
taskkillstar wants to merge 2 commits into
masterking32:mainfrom
taskkillstar:fix/strategy6-probation-starvation

Conversation

@taskkillstar

Copy link
Copy Markdown

Summary of Changes

This PR fixes a starvation / deadlock issue in the client balancer where newly reactivated or low-traffic resolvers are permanently locked out of selection when using Strategy 6 (BalancingLossThenLatency) or scoring strategies.


Root Cause Analysis

In upstream internal/client/balancer.go:

  1. The 5-Packet Probation Penalty:
    In lossScoreLocked(idx int):

    sent, _, lost, _, _ := b.stats[idx].snapshot()
    if sent < 5 {
        return 200 // Initial probation
    }

    Any resolver with sent < 5 is assigned a synthetic loss penalty score of 200.

  2. Candidate Filtering in Strategy 6 (lossThenLatencyCandidatesLocked):
    Strategy 6 computes the loss cutoff relative to the best resolver in the pool:

    lossTolerance := uint64(25)
    if bestLoss >= 200 {
        lossTolerance = 0
    }
    lossCutoff := bestLoss + lossTolerance
    • As soon as an active resolver reaches $\ge 5$ packets with 0 loss, bestLoss becomes 0.
    • The loss cutoff is locked at 25 (0 + 25 = 25).
    • Any other resolver with sent < 5 (e.g. newly reactivated from health checks or standby) has lossScore = 200.
    • Because 200 > 25, the resolver is strictly discarded (cand.loss > lossCutoff).
  3. Deadlock / Starvation:
    Because the unproven resolver is filtered out of selection, it is never routed any tunnel packets. Its sent counter never reaches 5, causing it to remain trapped in probation forever while traffic continues piling onto established resolvers even if their latency degrades.

  4. Latency Floor in latencyScoreLocked:
    latencyScoreLocked similarly returned 999000 ($\mu s$) when count < 5, ignoring valid RTT measurements for resolvers with $1 \le count &lt; 5$.


Changes Made

  1. Dynamic Loss Calculation (lossScoreLocked):

    • Removed the artificial sent < 5 return of 200.
    • When sent == 0 or lost == 0, return 0 (clean/unpenalized).
    • When sent > 0, return (lost * 1000) / sent.
  2. Dynamic Latency Calculation (latencyScoreLocked):

    • If count > 0, compute sum / count using the actual RTT observations.
    • Only return 999000 if count == 0 (no samples recorded).
  3. Signal Checks (hasLossSignalLocked / hasLatencySignalLocked):

    • Updated threshold to sent > 0 and count > 0 so runtime statistics take effect as soon as real samples are available.
  4. Unit Test:

    • Added TestBalancerLossThenLatency_NoProbationStarvation in internal/client/balancer_test.go to ensure a newly activated low-latency resolver with fewer than 5 packets is successfully selected over an established high-latency resolver.

Verification

  • Ran go test -v -run TestBalancer ./internal/client/... $\rightarrow$ PASS (all tests passed).
  • Verified no regressions in round-robin fallback behavior when no stats exist.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Zero-RTT resolvers can still starve after the actual seeded reactivation flow.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates resolver scoring to use available packet and RTT samples immediately, reducing probation starvation.

Changes:

  • Removes five-sample scoring thresholds.
  • Adds a Strategy 6 regression test.
File summaries
File Description
internal/client/balancer.go Revises loss, latency, and signal scoring.
internal/client/balancer_test.go Tests low-sample resolver selection.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/client/balancer.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants