fix: forced health check and re-check storm on failure bursts - #2984
fix: forced health check and re-check storm on failure bursts#2984IsoLeyN wants to merge 3 commits into
Conversation
|
Problem 1 Today I noticed different behavior in the program. It turns out that the GUI I'm using doesn't automatically populate the URL for url-test groups. I manually set it, and tests started running automatically when I launch the core. However, this doesn't solve the issue of having to wait for the fallback interval. In the video, I demonstrated the core's operation both without my code and with it. In the second part of the video, you can see how fallback immediately selects a group that has at least some content. Demonstration video: https://youtu.be/MRBBIPpImy8. Problem 2 Demonstration video: https://youtu.be/Vkm76fPFLiE. I placed a stopwatch nearby to track the time elapsed between tests. In my config, the interval is set to 900 seconds. In the first example, you can see that the servers start being tested in less than 2 minutes, which is incorrect. In the second example, I show the corrected code where the proper 900-second wait time is enforced. |
olicesx
left a comment
There was a problem hiding this comment.
Overall this is a high-quality fix. Both bugs are addressed correctly: I verified the Unwrap chain in resolvesToReject terminates (via Base.Unwrap returning nil at adapter/outbound/base.go:142), that scheduled health checks go through the provider's own ticker (adapter/provider/healthcheck.go:45) and are not affected by the new cooldown, and that fallback.go's ListenPacketContext got the same forced-check treatment as DialContext. Just a few minor suggestions below, none blocking.
Detailed review
minor
M1 - forcedHealthCheckNeeded takes a testUrl parameter it never uses
groupbase.go: func forcedHealthCheckNeeded(proxy C.Proxy, testUrl string) bool { return resolvesToReject(proxy) }. The testUrl argument is dead, but all 4 call sites (urltest.go DialContext/ListenPacketContext, fallback.go DialContext/ListenPacketContext) pass u.testUrl/f.testUrl. Consider dropping the parameter to forcedHealthCheckNeeded(proxy C.Proxy) bool, or note in the comment that it is reserved for future use.
M2 - The forced-check snippet is duplicated across 4 sites
The same if forcedHealthCheckNeeded(proxy, xx.testUrl) { go xx.healthCheck() } block appears in urltest.go (DialContext L57, ListenPacketContext L84) and fallback.go (DialContext L33, ListenPacketContext L59). Since both *URLTest and *Fallback embed *GroupBase, this could collapse to a single method on GroupBase, e.g. func (gb *GroupBase) maybeForceHealthCheck(proxy C.Proxy) { if forcedHealthCheckNeeded(proxy) { go gb.healthCheck() } }, which also keeps the testUrl removal from M1 consistent. Acceptable as-is, but future edits will need to touch 4 places.
nit
N1 - forcedHealthCheckNeeded is currently a thin wrapper around resolvesToReject
Single-line return resolvesToReject(proxy). The comment explains "plain dead member deliberately excluded", which implies room for future expansion. Fine to keep as a seam; merge into resolvesToReject if no expansion is planned.
Test gap (non-blocking)
T1 - gb.failedTimes = 0 reset in onDialFailed is not directly covered
Existing tests cover cooldown suppression and the REJECT-dial trigger, but not the new "reset failedTimes after firing so subsequent bursts in the same window don't re-emit Warnln" behavior. Logic is simple and low regression risk, but a small unit test would lock it in.
Points I verified (no issue)
resolvesToReject'sfor p := proxy; p != nil; p = p.Unwrap(nil, false)relies onBase.Unwrapreturning nil to terminate (adapter/outbound/base.go:142) - the chain is finite, no infinite loop.- Selector/URLTest/Fallback
Unwrapimplementations do not dereference metadata, so passing nil is safe. - Scheduled health checks use the provider's own ticker (adapter/provider/healthcheck.go:45
time.NewTicker(hc.interval)) and do not route throughgb.healthCheck(), so the new cooldown does not throttle scheduled checks - consistent with the PR description. fallback.go'sListenPacketContextincludes the same forced-check asDialContext.- The 30s floor applies only when no interval is configured; when
intervalis set it respects the user's configured value.
|
@olicesx Thanks for the review. Both findings are fixed, but digging into them I found that the first half of this PR didn't actually work, so I've removed it. The diff looks pretty different now. What went wrong. func (hc *HealthCheck) check() {
if len(hc.proxies) == 0 {
return
}A group only serves its empty-fallback when So the forced-check-on-REJECT machinery cost a tree walk and a goroutine per dial and did nothing. What actually fixed things in my demo video was the other half - Cooldown. Tying it to
Two bugs in my own patch, caught while re-testing:
On M2 - just so it doesn't come up again elsewhere: a helper on Concurrency: 12 tests, green under Not covered here:
|
This PR fixes two related bugs in outboundgroup (fallback / url-test group logic) that affect setups using empty-fallback: REJECT.
Problem 1: fallback/url-test never recovers from an empty group
When a url-test or fallback group has no alive members, it resolves to its empty-fallback proxy (in this config, REJECT). The issue is that dialing through REJECT is not treated as a failure by the group: REJECT's DialContext returns a no-op connection with a nil error (it never actually connects anywhere), so onDialFailed is never called, the group's failure counter never increments, and the forced health check that would normally recover the group is never triggered.
In practice, this meant that if a url-test group's underlying provider was temporarily empty or unreachable at startup, the group would get stuck silently serving REJECT forever, only recovering at the next scheduled interval (up to 15 minutes in this config), instead of noticing the problem and re-checking its members.
Fix: added forcedHealthCheckNeeded(), which detects when the proxy actually being used to dial is REJECT/REJECT-DROP or is marked dead. When that's the case, DialContext/ListenPacketContext in both fallback.go and urltest.go now proactively kick off an async health check, so the group re-evaluates its members instead of waiting for the next scheduled interval.
Video Demonstration: https://youtu.be/watch?v=LoVrG-ICByg.
Problem 2: failure-triggered health checks can storm the network
GroupBase.onDialFailed already had logic to trigger a forced health check after N consecutive failed dials (maxFailedTimes). However, there was no cooldown between these forced checks. With a group containing ~200 proxies, a burst of failed dials (e.g. during a flaky connection) could re-trigger a full health check of every proxy in the group every few seconds, causing repeated storms of health-check traffic against all providers - visible in logs as "because failed multiple times, activate health check" firing every 6-30 seconds.
You might have noticed that in
url-testgroups, the ping fluctuates SEVERAL times right before your eyes. This is precisely the issue I'm referring to.Fix: added a 30-second cooldown (forcedHealthCheckCooldown) around GroupBasegered path, tracked via lastForcedCheck (atomic.TypedValue[time.Time]).Scheduled health checks (via interval config) are unaffected - only the failure-triggered path is throttled. Also reset the failure counter after a forced check fires, so it doesn't keep re-triggering warnings on every subsequent failed dial within the same window.
Testing
Added forcedcheck_test.go with three tests (written test-first, confirmed fafore the fix):