Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion selector/node/ewma/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,12 @@ func (n *Node) Pick() selector.DoneFunc {
success = 0
}
var netErr net.Error
if errors.Is(context.DeadlineExceeded, di.Err) || errors.Is(context.Canceled, di.Err) ||
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
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b444c58. Changed to errors.Is(di.Err, context.DeadlineExceeded) — correct stdlib/Kratos signature so wrapped deadline errors are matched properly.

Expand Down
55 changes: 55 additions & 0 deletions selector/node/ewma/node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,61 @@ func TestDirectError(t *testing.T) {
}
}

// TestCanceledDoesNotDegradeNode verifies that context.Canceled does not
// lower a node's health score. Canceled means the caller gave up — it is
// not evidence of backend failure. context.DeadlineExceeded (backend too
// slow) should still degrade the node as before.
func TestCanceledDoesNotDegradeNode(t *testing.T) {
newNode := func() selector.WeightedNode {
return (&Builder{}).Build(selector.NewNode(
"http",
"127.0.0.1:9090",
&registry.ServiceInstance{
ID: "127.0.0.1:9090",
Name: "helloworld",
Version: "v1.0.0",
Endpoints: []string{"http://127.0.0.1:9090"},
Metadata: map[string]string{"weight": "10"},
}))
}

// --- context.Canceled must NOT degrade health ---
wn := newNode()
// One successful pick to initialise EWMA state.
done := wn.Pick()
time.Sleep(time.Millisecond * 20)
done(context.Background(), selector.DoneInfo{})
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())
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.


// --- context.DeadlineExceeded still degrades health ---
wn2 := newNode()
done = wn2.Pick()
time.Sleep(time.Millisecond * 20)
done(context.Background(), selector.DoneInfo{})
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())
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

}

func TestDirectErrorHandler(t *testing.T) {
b := &Builder{
ErrHandler: func(err error) bool {
Expand Down
Loading