From 27470c61294d06557bd59a1180065d782aa634e6 Mon Sep 17 00:00:00 2001 From: ivanauth Date: Wed, 15 Oct 2025 17:37:55 -0400 Subject: [PATCH] fix: suppress retry logs in version command when SpiceDB unavailable When running 'zed version' with an unavailable SpiceDB instance, the command would retry multiple times and log noisy error messages. This fix: - Disables retries by default for the version command - Respects user-specified --max-retries if explicitly set - Restores both flag value and Changed state to preserve semantics - Logs connection failures at debug level instead of error - Always prints 'service: (unknown)' for consistent output - Adds test coverage for unavailable server scenario Fixes #554 --- internal/cmd/version.go | 39 ++++++++++++++--- internal/cmd/version_test.go | 83 ++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 7 deletions(-) diff --git a/internal/cmd/version.go b/internal/cmd/version.go index dc37fb4b..d5204284 100644 --- a/internal/cmd/version.go +++ b/internal/cmd/version.go @@ -3,10 +3,12 @@ package cmd import ( "fmt" "os" + "strconv" "github.com/gookit/color" "github.com/jzelinskie/cobrautil/v2" "github.com/mattn/go-isatty" + "github.com/rs/zerolog/log" "github.com/spf13/cobra" "google.golang.org/grpc" "google.golang.org/grpc/metadata" @@ -63,18 +65,41 @@ func versionCmdFunc(cmd *cobra.Command, _ []string) error { _, err := client.GetCurrentTokenWithCLIOverride(cmd, configStore, secretStore) if err == nil { - spiceClient, err := client.NewClient(cmd) - if err != nil { - return err + // Temporarily disable retries for version check to avoid noisy error logs + // when SpiceDB is unavailable + userSetRetries := cmd.Flags().Changed("max-retries") + var originalValue uint + + if !userSetRetries { + originalValue, _ = cmd.Flags().GetUint("max-retries") + _ = cmd.Flags().Set("max-retries", "0") } - serverVersion, err := getServerVersion(cmd, spiceClient) - if err != nil { - return err + + spiceClient, err := client.NewClient(cmd) + + // Restore original max-retries value and Changed state + if !userSetRetries { + _ = cmd.Flags().Set("max-retries", strconv.FormatUint(uint64(originalValue), 10)) + cmd.Flags().Lookup("max-retries").Changed = false } blue := color.FgLightBlue.Render fmt.Print(blue("service: ")) - console.Println(serverVersion) + + if err != nil { + // Log at debug level and continue with unknown version + log.Debug().Err(err).Msg("failed to connect to SpiceDB") + console.Println("(unknown)") + } else { + serverVersion, err := getServerVersion(cmd, spiceClient) + if err != nil { + // Log at debug level and continue with unknown version + log.Debug().Err(err).Msg("failed to get server version") + console.Println("(unknown)") + } else { + console.Println(serverVersion) + } + } } } diff --git a/internal/cmd/version_test.go b/internal/cmd/version_test.go index 1e972cd3..20335cfd 100644 --- a/internal/cmd/version_test.go +++ b/internal/cmd/version_test.go @@ -1,16 +1,22 @@ package cmd import ( + "bytes" "context" + "os" "testing" + "github.com/spf13/cobra" "github.com/stretchr/testify/require" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" "github.com/authzed/authzed-go/pkg/responsemeta" v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" + "github.com/authzed/zed/internal/client" zedtesting "github.com/authzed/zed/internal/testing" ) @@ -138,3 +144,80 @@ func (m *mockSchemaServiceClient) ReadSchema(_ context.Context, _ *v1.ReadSchema func (m *mockSchemaServiceClient) WriteSchema(_ context.Context, _ *v1.WriteSchemaRequest, _ ...grpc.CallOption) (*v1.WriteSchemaResponse, error) { return nil, nil } + +func TestVersionCommandWithUnavailableServer(t *testing.T) { + // Note: Not running in parallel because we modify globals (client.NewClient, os.Stdout, os.Stderr) + + // This test verifies issue #554 is fixed: when SpiceDB is unavailable, + // zed version should not produce noisy retry error logs + + // Save and restore original client.NewClient + originalNewClient := client.NewClient + t.Cleanup(func() { + client.NewClient = originalNewClient + }) + + // Mock client.NewClient to return Unavailable error + client.NewClient = func(_ *cobra.Command) (client.Client, error) { + return nil, status.Error(codes.Unavailable, "connection refused") + } + + // Capture stdout to check output + oldStdout := os.Stdout + rOut, wOut, _ := os.Pipe() + os.Stdout = wOut + t.Cleanup(func() { + os.Stdout = oldStdout + }) + + // Capture stderr to verify no retry errors are logged + oldStderr := os.Stderr + rErr, wErr, _ := os.Pipe() + os.Stderr = wErr + t.Cleanup(func() { + os.Stderr = oldStderr + }) + + // Create command with all required flags + cmd := zedtesting.CreateTestCobraCommandWithFlagValue(t, + zedtesting.BoolFlag{FlagName: "include-remote-version", FlagValue: true}, + zedtesting.BoolFlag{FlagName: "include-deps", FlagValue: false}, + zedtesting.UintFlag{FlagName: "max-retries", FlagValue: 10}, + zedtesting.StringFlag{FlagName: "endpoint", FlagValue: ""}, + zedtesting.StringFlag{FlagName: "token", FlagValue: ""}, + zedtesting.StringFlag{FlagName: "certificate-path", FlagValue: ""}, + zedtesting.StringFlag{FlagName: "hostname-override", FlagValue: ""}, + zedtesting.StringFlag{FlagName: "permissions-system", FlagValue: ""}, + zedtesting.StringFlag{FlagName: "proxy", FlagValue: ""}, + zedtesting.StringFlag{FlagName: "request-id", FlagValue: ""}, + zedtesting.BoolFlag{FlagName: "insecure", FlagValue: false}, + zedtesting.BoolFlag{FlagName: "no-verify-ca", FlagValue: false}, + zedtesting.BoolFlag{FlagName: "skip-version-check", FlagValue: false}, + zedtesting.IntFlag{FlagName: "max-message-size", FlagValue: 0}, + ) + + // Run the version command + err := versionCmdFunc(cmd, nil) + + // Close writers and read captured outputs + wOut.Close() + wErr.Close() + var stdout bytes.Buffer + _, _ = stdout.ReadFrom(rOut) + var stderr bytes.Buffer + _, _ = stderr.ReadFrom(rErr) + + // Command should succeed despite unavailable server + require.NoError(t, err) + + // Stdout should contain client version and service: (unknown) + output := stdout.String() + require.Contains(t, output, "zed") + require.Contains(t, output, "service:") + require.Contains(t, output, "(unknown)") + + // Stderr should be empty (no retry error logs) + stderrOutput := stderr.String() + require.NotContains(t, stderrOutput, "retrying gRPC call") + require.NotContains(t, stderrOutput, "Unavailable") +}