From e15f8cec57c503807c8907b67bceb2c5f11f5715 Mon Sep 17 00:00:00 2001 From: Bharat Kunwar Date: Thu, 29 Oct 2026 15:38:48 +0545 Subject: [PATCH] PLAT-1314/feat: expose per-request retry limits in twoctl --- README.md | 16 +++++ cmd/twoctl/cli/operations.go | 20 +++++- cmd/twoctl/cli/operations_test.go | 1 + cmd/twoctl/cli/paginate.go | 6 +- cmd/twoctl/cli/retries_test.go | 101 ++++++++++++++++++++++++++++++ cmd/twoctl/cli/root.go | 12 ++-- internal/httpx/client.go | 7 ++- internal/httpx/retry_test.go | 9 +++ 8 files changed, 163 insertions(+), 9 deletions(-) create mode 100644 cmd/twoctl/cli/retries_test.go diff --git a/README.md b/README.md index 24b0592..a608679 100644 --- a/README.md +++ b/README.md @@ -184,6 +184,22 @@ Top-level non-resource commands: | 6 | server error (5xx) | | 7 | network / transport failure | +## Retries + +API requests retry transient network errors, HTTP 429, and HTTP 5xx up to +three times by default (four attempts including the initial request). +Use `--max-retries N` to set the number of retries per request, or +`--max-retries 0` to make a single attempt: + +```sh +twoctl order get --order-id abc-123 --max-retries 0 +twoctl billing-statement get --all --max-retries 1 +``` + +The setting applies to each page when using `--all`. Retries keep the same +idempotency key and request body. The existing 60-second request timeout +still bounds the attempts and backoff for each request. + ## Self-upgrade `twoctl` checks for new releases at most once every 24 hours and prompts when one is available. The prompt offers: diff --git a/cmd/twoctl/cli/operations.go b/cmd/twoctl/cli/operations.go index 2386000..a8ce9cf 100644 --- a/cmd/twoctl/cli/operations.go +++ b/cmd/twoctl/cli/operations.go @@ -362,7 +362,25 @@ func runOperation(c *cobra.Command, method, path string, op *openapi3.Operation, if dryRun { return printDryRun(c, req, body != nil) } - return executeRequest(c, httpx.New(resolved.APIKey), req) + client, err := operationClient(resolved.APIKey) + if err != nil { + return err + } + return executeRequest(c, client, req) +} + +// operationClient applies the same retry policy to ordinary and paginated calls. +func operationClient(apiKey string) (*http.Client, error) { + if flagMaxRetries < 0 { + return nil, &apiError{ + code: "invalid_max_retries", + message: "--max-retries must be zero or greater", + exit: ExitUsage, + } + } + return httpx.NewWithOptions(httpx.Options{ + APIKey: apiKey, MaxRetries: flagMaxRetries, DisableRetries: flagMaxRetries == 0, + }), nil } func resolveForRun(dryRun bool) (*config.Resolved, error) { diff --git a/cmd/twoctl/cli/operations_test.go b/cmd/twoctl/cli/operations_test.go index a2d6e98..d9b23ef 100644 --- a/cmd/twoctl/cli/operations_test.go +++ b/cmd/twoctl/cli/operations_test.go @@ -34,6 +34,7 @@ func resetGlobals(t *testing.T) { flagContext = "" flagURL = "" flagOutput = "json" + flagMaxRetries = 3 resetCobraFlags(Root()) dir := t.TempDir() t.Setenv("XDG_CONFIG_HOME", dir) diff --git a/cmd/twoctl/cli/paginate.go b/cmd/twoctl/cli/paginate.go index 098291e..658542d 100644 --- a/cmd/twoctl/cli/paginate.go +++ b/cmd/twoctl/cli/paginate.go @@ -19,7 +19,6 @@ import ( "github.com/getkin/kin-openapi/openapi3" "github.com/spf13/cobra" - "github.com/two-inc/twoctl-cli/internal/httpx" "github.com/two-inc/twoctl-cli/internal/output" ) @@ -56,7 +55,10 @@ func runPaginated(c *cobra.Command, method, path string, op *openapi3.Operation, if err != nil { return err } - client := httpx.New(resolved.APIKey) + client, err := operationClient(resolved.APIKey) + if err != nil { + return err + } var combined []any cursorPtr := b.stringFlags[cursorFlagName] diff --git a/cmd/twoctl/cli/retries_test.go b/cmd/twoctl/cli/retries_test.go new file mode 100644 index 0000000..726e6b5 --- /dev/null +++ b/cmd/twoctl/cli/retries_test.go @@ -0,0 +1,101 @@ +package cli + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strconv" + "testing" +) + +func TestMaxRetriesAppliesToEveryAPIPath(t *testing.T) { + for _, paginated := range []bool{false, true} { + for _, retries := range []int{0, 1, -1} { + t.Run(fmt.Sprintf("paginated=%t/retries=%d", paginated, retries), func(t *testing.T) { + checkRetryAttempts(t, paginated, retries) + }) + } + } +} + +// A negative value here means omit the flag and assert the default policy. +func checkRetryAttempts(t *testing.T, paginated bool, retries int) { + t.Helper() + resetGlobals(t) + calls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + w.WriteHeader(http.StatusServiceUnavailable) + fmt.Fprint(w, `{"error":"temporary"}`) + })) + defer srv.Close() + args := []string{"--url", srv.URL, "--api-key", "secret_test_x", "billing-statement", "get"} + if paginated { + args = append(args, "--all") + } + want := 4 + if retries >= 0 { + args = append(args, "--max-retries", strconv.Itoa(retries)) + want = retries + 1 + } + _, _, err := runRoot(t, args...) + ae, ok := err.(*apiError) + if !ok || ae.status != http.StatusServiceUnavailable { + t.Fatalf("expected HTTP 503 apiError, got %v", err) + } + if calls != want { + t.Fatalf("got %d HTTP attempts, want %d", calls, want) + } +} + +func TestNegativeMaxRetriesDoesNotSendRequest(t *testing.T) { + for _, paginated := range []bool{false, true} { + t.Run(fmt.Sprintf("paginated=%t", paginated), func(t *testing.T) { + resetGlobals(t) + calls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + })) + defer srv.Close() + args := []string{"--url", srv.URL, "--api-key", "secret_test_x", "--max-retries", "-1", "billing-statement", "get"} + if paginated { + args = append(args, "--all") + } + _, _, err := runRoot(t, args...) + ae, ok := err.(*apiError) + if !ok || ae.code != "invalid_max_retries" || ae.exit != ExitUsage { + t.Fatalf("expected usage error, got %v", err) + } + if calls != 0 { + t.Fatalf("sent %d requests with invalid retry setting", calls) + } + }) + } +} + +func TestMaxRetriesAppliesToEveryPage(t *testing.T) { + resetGlobals(t) + calls := map[string]int{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + cursor := r.URL.Query().Get("page_cursor") + calls[cursor]++ + if calls[cursor] == 1 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + if cursor == "" { + fmt.Fprint(w, `{"items":[{"id":"a"}],"next_page_cursor":"second"}`) + return + } + fmt.Fprint(w, `{"items":[{"id":"b"}]}`) + })) + defer srv.Close() + _, _, err := runRoot(t, "--url", srv.URL, "--api-key", "secret_test_x", + "billing-statement", "get", "--all", "--max-retries", "1") + if err != nil { + t.Fatal(err) + } + if calls[""] != 2 || calls["second"] != 2 || len(calls) != 2 { + t.Fatalf("expected one retry for each page, got %v", calls) + } +} diff --git a/cmd/twoctl/cli/root.go b/cmd/twoctl/cli/root.go index d5da2ab..854b7f6 100644 --- a/cmd/twoctl/cli/root.go +++ b/cmd/twoctl/cli/root.go @@ -10,11 +10,12 @@ import ( // Global flags. Populated by cobra during Execute(). var ( - flagAPIKey string - flagEnv string - flagContext string - flagURL string - flagOutput string + flagAPIKey string + flagEnv string + flagContext string + flagURL string + flagOutput string + flagMaxRetries int ) // activeEnv returns the context-selecting flag value, preferring --context @@ -53,6 +54,7 @@ func init() { rootCmd.PersistentFlags().StringVar(&flagContext, "context", "", "alias of --env (kubectl-style)") rootCmd.PersistentFlags().StringVar(&flagURL, "url", "", "raw API base URL (overrides the context's URL)") rootCmd.PersistentFlags().StringVarP(&flagOutput, "output", "o", "auto", "output format: table, json, yaml (auto = table on TTY, json when piped)") + rootCmd.PersistentFlags().IntVar(&flagMaxRetries, "max-retries", httpx.DefaultMaxRetries, "maximum retries per API request (0 disables retries)") rootCmd.Version = httpx.Version } diff --git a/internal/httpx/client.go b/internal/httpx/client.go index 2246744..a0770ff 100644 --- a/internal/httpx/client.go +++ b/internal/httpx/client.go @@ -35,6 +35,9 @@ type Options struct { APIKey string Timeout time.Duration MaxRetries int + // DisableRetries takes precedence over MaxRetries, whose zero value + // otherwise selects DefaultMaxRetries. + DisableRetries bool } // New returns an *http.Client wired with auth + identity headers, idempotency @@ -49,7 +52,9 @@ func NewWithOptions(opt Options) *http.Client { if opt.Timeout == 0 { opt.Timeout = DefaultTimeout } - if opt.MaxRetries == 0 { + if opt.DisableRetries { + opt.MaxRetries = 0 + } else if opt.MaxRetries == 0 { opt.MaxRetries = DefaultMaxRetries } return &http.Client{ diff --git a/internal/httpx/retry_test.go b/internal/httpx/retry_test.go index fe4a30e..23ed0b8 100644 --- a/internal/httpx/retry_test.go +++ b/internal/httpx/retry_test.go @@ -183,3 +183,12 @@ func TestContextCancellationStopsRetry(t *testing.T) { t.Error("expected context cancellation error") } } + +func TestDisableRetriesOverridesDefaultAndExplicitCount(t *testing.T) { + for _, retries := range []int{0, 3} { + client := NewWithOptions(Options{DisableRetries: true, MaxRetries: retries}) + if got := client.Transport.(*transport).maxRetries; got != 0 { + t.Errorf("DisableRetries with MaxRetries=%d: got %d retries", retries, got) + } + } +}