fix(selector/ewma): do not penalise nodes on context.Canceled (fixes #3834) - #3836
Open
akpradheeph wants to merge 2 commits into
Open
fix(selector/ewma): do not penalise nodes on context.Canceled (fixes #3834)#3836akpradheeph wants to merge 2 commits into
akpradheeph wants to merge 2 commits into
Conversation
The EWMA node health callback marked a node success=0 (fully degraded)
whenever di.Err was context.Canceled, treating it identically to
context.DeadlineExceeded. These two errors have different origins:
context.DeadlineExceeded — backend was too slow to respond within the
client timeout. May indicate backend overload or latency issues.
Penalising the node is reasonable.
context.Canceled — the *caller* cancelled the in-flight RPC (user
navigated away, upstream HTTP request was aborted, load test
interrupted mid-flight, parent context cancelled). The backend is
completely unaware of this cancellation and may be perfectly healthy.
Penalising the backend node is incorrect.
Impact of the bug:
Any workload with frequent client-side cancellations — mobile apps,
frontend SPAs with AbortController, aggressive per-request timeouts on
the ingress side — causes the EWMA balancer to continuously lower the
health score of otherwise healthy backends. Under sustained cancellation
load all nodes can reach success≈0 simultaneously, producing erratic
load distribution that recovers only via the 600ms EWMA decay window.
Fix:
Remove errors.Is(context.Canceled, di.Err) from the success=0
condition. Canceled is now treated the same as any other non-fatal
error: the node's lag EWMA is updated (reflecting real latency up to
the cancellation point) but its health score is not penalised.
Tests:
TestCanceledDoesNotDegradeNode asserts that repeated context.Canceled
errors do not reduce node weight, while context.DeadlineExceeded still
does (regression guard for the existing behaviour).
Fixes go-kratos#3834
dosubot
Bot
added
the
size:XS
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the EWMA node health calculation so that context.Canceled (caller-initiated cancellation) no longer fully degrades a backend node’s health, while preserving degradation on backend-relevant failures like context.DeadlineExceeded.
Changes:
- Exclude
context.Canceledfrom the EWMA “success = 0” degradation condition in the node pick callback. - Add a new regression test intended to ensure
context.Canceleddoes not degrade nodes, whilecontext.DeadlineExceededstill does.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
selector/node/ewma/node.go |
Removes context.Canceled from the error conditions that set EWMA success to 0. |
selector/node/ewma/node_test.go |
Adds a test covering cancellation vs deadline exceeded behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
156
to
164
| if errors.Is(context.DeadlineExceeded, di.Err) || | ||
| // context.Canceled is intentionally excluded: it means the caller | ||
| // cancelled the request (user navigation, upstream timeout, etc.) and | ||
| // says nothing about whether the backend is healthy. Penalising nodes | ||
| // for client-side cancellations causes healthy backends to lose weight | ||
| // under normal frontend workloads with frequent in-flight cancellations. | ||
| errors.IsServiceUnavailable(di.Err) || errors.IsGatewayTimeout(di.Err) || errors.As(di.Err, &netErr) { | ||
| success = 0 | ||
| } |
Author
There was a problem hiding this comment.
Fixed in b444c58. Changed to errors.Is(di.Err, context.DeadlineExceeded) — correct stdlib/Kratos signature so wrapped deadline errors are matched properly.
Comment on lines
+112
to
+123
| baseline := wn.Weight() | ||
|
|
||
| // Several picks that all report context.Canceled. | ||
| for i := 0; i < 4; i++ { | ||
| done = wn.Pick() | ||
| time.Sleep(time.Millisecond * 20) | ||
| done(context.Background(), selector.DoneInfo{Err: context.Canceled}) | ||
| } | ||
| if wn.Weight() < baseline*0.9 { | ||
| t.Errorf("context.Canceled should not degrade node weight: before=%.2f after=%.2f", | ||
| baseline, wn.Weight()) | ||
| } |
Author
There was a problem hiding this comment.
Fixed in b444c58. Now type-asserts wn.(*Node) and asserts directly on n.success.Load() (the health EWMA) rather than Weight(). Since the test is in package ewma it has direct access to the unexported field.
Comment on lines
+130
to
+140
| baseline2 := wn2.Weight() | ||
|
|
||
| for i := 0; i < 4; i++ { | ||
| done = wn2.Pick() | ||
| time.Sleep(time.Millisecond * 20) | ||
| done(context.Background(), selector.DoneInfo{Err: context.DeadlineExceeded}) | ||
| } | ||
| if wn2.Weight() >= baseline2 { | ||
| t.Errorf("context.DeadlineExceeded should degrade node weight: before=%.2f after=%.2f", | ||
| baseline2, wn2.Weight()) | ||
| } |
Author
There was a problem hiding this comment.
Fixed in b444c58. Same approach — asserts on n2.success.Load() directly to verify the health EWMA drops on context.DeadlineExceeded, independent of lag/load fluctuations.
…not weight - errors.Is(context.DeadlineExceeded, di.Err) had arguments reversed; fixed to errors.Is(di.Err, context.DeadlineExceeded) so wrapped deadline errors are matched correctly. - TestCanceledDoesNotDegradeNode now asserts on the node's success EWMA (n.success.Load()) rather than Weight(), which also varies with lag/load and can fluctuate even when health is unchanged. Accessing the internal field directly is valid since the test is in package ewma. Addresses reviewer feedback on PR go-kratos#3836.
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
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.
Problem
Fixes #3834.
The EWMA node health callback treated
context.Canceledidentically tocontext.DeadlineExceeded, settingsuccess = 0(fully degraded) for both. These have different origins:context.DeadlineExceededcontext.CanceledImpact
Any workload with frequent client-side cancellations — mobile apps, frontend SPAs, aggressive ingress timeouts — causes the EWMA balancer to lower the health score of healthy backends. Under sustained load all nodes can reach
success ≈ 0simultaneously, producing erratic distribution that recovers only via the 600 ms EWMA decay window.Change
Remove
errors.Is(context.Canceled, di.Err)from thesuccess = 0condition.Cancelednow updates the lag EWMA (reflecting real latency up to cancellation) but does not penalise node health.Tests
TestCanceledDoesNotDegradeNodeasserts:context.Cancelederrors do not reduce node weightcontext.DeadlineExceededstill degrades node weight (regression guard)All existing
selector/node/ewmatests pass.