From 0bfe2f6eadb1a234ae095a2c520bf05c5d174bee Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 18:06:23 +0200 Subject: [PATCH] [client] Answer terminalLoginError's nil case on its own terms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A successful Login reaches terminalLoginError with a nil error, and nothing covered that. It happens to work on grpc v1.80.0 — gstatus.FromError(nil) answers (nil, true), and Status.Code tolerates a nil receiver by returning codes.OK, which is not in the terminal set — but that is a chain of internal details to be relying on for the common path, and none of it was asserted. Now the nil error is handled where it is obvious, and the table covers it. Reported by CodeRabbit on PR #7398, which called it a panic; measured on v1.80.0 it is not one. The gap was the untested reliance, not a crash. --- client/cmd/root.go | 8 ++++++++ client/cmd/up_setconfig_refusal_test.go | 1 + 2 files changed, 9 insertions(+) diff --git a/client/cmd/root.go b/client/cmd/root.go index b599ca859..2ca14c39c 100644 --- a/client/cmd/root.go +++ b/client/cmd/root.go @@ -299,6 +299,14 @@ func DialClientGRPCServer(ctx context.Context, addr string) (*grpc.ClientConn, e // they each carried their own copy of this list — which is how one of them // ended up retrying a refusal the other treated as final. func terminalLoginError(err error) bool { + // A successful Login reaches here with a nil error, and that is not a + // terminal failure. Handled explicitly rather than left to + // gstatus.FromError, which answers (nil, true) for a nil error and leans on + // Status.Code tolerating a nil receiver to come back as codes.OK. + if err == nil { + return false + } + s, ok := gstatus.FromError(err) if !ok { return false diff --git a/client/cmd/up_setconfig_refusal_test.go b/client/cmd/up_setconfig_refusal_test.go index 74623e434..fdf580102 100644 --- a/client/cmd/up_setconfig_refusal_test.go +++ b/client/cmd/up_setconfig_refusal_test.go @@ -74,6 +74,7 @@ func TestTerminalLoginError(t *testing.T) { {name: "daemon unreachable, worth retrying", err: gstatus.Errorf(codes.Unavailable, "connection refused"), wantTerminal: false}, {name: "transient internal failure", err: gstatus.Errorf(codes.Internal, "boom"), wantTerminal: false}, {name: "not a status error", err: errors.New("boom"), wantTerminal: false}, + {name: "no error at all, the login succeeded", err: nil, wantTerminal: false}, } for _, tt := range tests {