From 9f17da58864d3733350f81f63fd54fd03dfa0f2b Mon Sep 17 00:00:00 2001 From: Scott Wilson Date: Wed, 26 Aug 2026 18:10:24 -0700 Subject: [PATCH 1/5] feat: implement and refactor network request retries --- packages/agentproxy/ca.go | 8 +- packages/agentproxy/cache.go | 7 +- packages/agentproxy/leases.go | 16 +- packages/agentproxy/proxy.go | 9 +- packages/cmd/agent.go | 6 +- packages/cmd/agent_proxy.go | 6 +- packages/cmd/agent_proxy_run.go | 6 +- packages/cmd/login.go | 2 - packages/pam/agent/run.go | 5 +- packages/pam/local/access.go | 6 +- packages/util/common.go | 11 +- packages/util/helper.go | 8 - packages/util/retry.go | 331 ++++++++++++++++++++++++ packages/util/retry_test.go | 441 ++++++++++++++++++++++++++++++++ 14 files changed, 832 insertions(+), 30 deletions(-) create mode 100644 packages/util/retry.go create mode 100644 packages/util/retry_test.go diff --git a/packages/agentproxy/ca.go b/packages/agentproxy/ca.go index a05af03d..f38e3783 100644 --- a/packages/agentproxy/ca.go +++ b/packages/agentproxy/ca.go @@ -16,7 +16,7 @@ import ( "time" "github.com/Infisical/infisical-merge/packages/api" - "github.com/go-resty/resty/v2" + "github.com/Infisical/infisical-merge/packages/util" "github.com/rs/zerolog/log" ) @@ -166,7 +166,11 @@ func (c *caManager) resignIntermediateLocked() error { } pubPem := pem.EncodeToMemory(&pem.Block{Type: "PUBLIC KEY", Bytes: pubDer}) - client := resty.New().SetAuthToken(c.token()) + client, err := util.GetRestyClientWithCustomHeaders() + if err != nil { + return err + } + client.SetAuthToken(c.token()) resp, err := api.CallSignAgentProxyIntermediateCa(client, api.SignAgentProxyIntermediateCaRequest{ PublicKey: string(pubPem), }) diff --git a/packages/agentproxy/cache.go b/packages/agentproxy/cache.go index abc254b7..710e8e21 100644 --- a/packages/agentproxy/cache.go +++ b/packages/agentproxy/cache.go @@ -11,7 +11,6 @@ import ( "github.com/Infisical/infisical-merge/packages/api" "github.com/Infisical/infisical-merge/packages/util" - "github.com/go-resty/resty/v2" "github.com/rs/zerolog/log" ) @@ -231,7 +230,11 @@ type resolveParams struct { // resolveServices lists the proxied services for a scope and attaches credential values. Shared by // both resolvers; the differences live in resolveParams. func resolveServices(scope agentScope, p resolveParams) ([]*resolvedService, error) { - client := resty.New().SetAuthToken(p.discoveryToken) + client, err := util.GetRestyClientWithCustomHeaders() + if err != nil { + return nil, err + } + client.SetAuthToken(p.discoveryToken) listResp, err := api.CallListProxiedServices(client, api.ListProxiedServicesRequest{ ProjectID: scope.projectID, Environment: scope.environment, diff --git a/packages/agentproxy/leases.go b/packages/agentproxy/leases.go index d8d91341..59eb9000 100644 --- a/packages/agentproxy/leases.go +++ b/packages/agentproxy/leases.go @@ -11,7 +11,7 @@ import ( "time" "github.com/Infisical/infisical-merge/packages/api" - "github.com/go-resty/resty/v2" + "github.com/Infisical/infisical-merge/packages/util" "github.com/rs/zerolog/log" "golang.org/x/sync/singleflight" ) @@ -99,7 +99,11 @@ func newLeaseStore(proxyToken func() string) *leaseStore { func defaultLeaseMinter(proxyToken func() string) leaseMinter { return func(args leaseMintArgs) (leaseMintResult, error) { - client := resty.New().SetAuthToken(proxyToken()) + client, err := util.GetRestyClientWithCustomHeaders() + if err != nil { + return leaseMintResult{}, err + } + client.SetAuthToken(proxyToken()) resp, err := api.CallCreateDynamicSecretLeaseV1(client, api.CreateDynamicSecretLeaseV1Request{ ProjectSlug: args.projectSlug, Environment: args.environment, @@ -116,8 +120,12 @@ func defaultLeaseMinter(proxyToken func() string) leaseMinter { func defaultLeaseRevoker(proxyToken func() string) leaseRevoker { return func(leaseID, projectSlug, environment, path string) error { - client := resty.New().SetAuthToken(proxyToken()) - _, err := api.CallRevokeDynamicSecretLeaseV1(client, api.RevokeDynamicSecretLeaseV1Request{ + client, err := util.GetRestyClientWithCustomHeaders() + if err != nil { + return err + } + client.SetAuthToken(proxyToken()) + _, err = api.CallRevokeDynamicSecretLeaseV1(client, api.RevokeDynamicSecretLeaseV1Request{ LeaseID: leaseID, ProjectSlug: projectSlug, Environment: environment, diff --git a/packages/agentproxy/proxy.go b/packages/agentproxy/proxy.go index aa7df7c9..4edf0388 100644 --- a/packages/agentproxy/proxy.go +++ b/packages/agentproxy/proxy.go @@ -19,7 +19,7 @@ import ( "time" "github.com/Infisical/infisical-merge/packages/api" - "github.com/go-resty/resty/v2" + "github.com/Infisical/infisical-merge/packages/util" "github.com/rs/zerolog" "github.com/rs/zerolog/log" ) @@ -175,7 +175,12 @@ func (ps *proxyServer) flushUsage() { ps.usage = make(map[string]struct{}) ps.usageMu.Unlock() - client := resty.New().SetAuthToken(ps.opts.ProxyToken()).SetTimeout(usageReportTimeout) + client, err := util.GetRestyClientWithPolicy(util.BestEffortRetryPolicy()) + if err != nil { + log.Debug().Err(err).Msg("failed to build usage-reporting client; dropping batch") + return + } + client.SetAuthToken(ps.opts.ProxyToken()).SetTimeout(usageReportTimeout) for serviceID := range snapshot { if err := api.CallReportProxiedServiceUsage(client, serviceID); err != nil { // Warn once: the usual cause is a missing Report Usage permission, which fails every attempt. diff --git a/packages/cmd/agent.go b/packages/cmd/agent.go index 93ce80e5..8a411360 100644 --- a/packages/cmd/agent.go +++ b/packages/cmd/agent.go @@ -1684,15 +1684,11 @@ func (tm *AgentManager) RevokeCredentials() error { // Refreshes the existing access token func (tm *AgentManager) RefreshAccessToken(accessToken string) error { - httpClient, err := util.GetRestyClientWithCustomHeaders() + httpClient, err := util.GetRestyClientWithPolicy(util.AgentRetryPolicy()) if err != nil { return err } - httpClient.SetRetryCount(10000). - SetRetryMaxWaitTime(20 * time.Second). - SetRetryWaitTime(5 * time.Second) - response, err := api.CallMachineIdentityRefreshAccessToken(httpClient, api.UniversalAuthRefreshRequest{AccessToken: accessToken}) if err != nil { return err diff --git a/packages/cmd/agent_proxy.go b/packages/cmd/agent_proxy.go index ad908bce..72075a84 100644 --- a/packages/cmd/agent_proxy.go +++ b/packages/cmd/agent_proxy.go @@ -172,7 +172,11 @@ func runAgentProxyConnect(cmd *cobra.Command, args []string) { Set("credentialSource", tokenSource). Set("allowReadableBrokeredSecrets", allowReadableBrokered)) - httpClient := resty.New().SetAuthToken(token.Token) + httpClient, err := util.GetRestyClientWithCustomHeaders() + if err != nil { + util.HandleError(err, "Failed to build the API client") + } + httpClient.SetAuthToken(token.Token) caResp, err := api.CallGetAgentProxyCa(httpClient) if err != nil { diff --git a/packages/cmd/agent_proxy_run.go b/packages/cmd/agent_proxy_run.go index dfc134bf..20f74a50 100644 --- a/packages/cmd/agent_proxy_run.go +++ b/packages/cmd/agent_proxy_run.go @@ -85,7 +85,11 @@ func runAgentProxyRun(cmd *cobra.Command, args []string) { // The single identity for the run: fetches config and secret values in the parent. The child gets none of it. src := resolveDeveloperTokenSource(cmd) - httpClient := resty.New().SetAuthToken(src.token()) + httpClient, err := util.GetRestyClientWithCustomHeaders() + if err != nil { + util.HandleError(err, "Failed to build the API client") + } + httpClient.SetAuthToken(src.token()) placeholders := fetchLocalProxiedServiceConfig(httpClient, projectID, environment, secretPath) local := &agentproxy.LocalOptions{ diff --git a/packages/cmd/login.go b/packages/cmd/login.go index 873efce5..b695a40f 100644 --- a/packages/cmd/login.go +++ b/packages/cmd/login.go @@ -638,7 +638,6 @@ func getFreshUserCredentials(email string, password string) (*api.GetLoginV3Resp if err != nil { return nil, err } - httpClient.SetRetryCount(5) loginV3Response, err := api.CallLoginV3(httpClient, api.GetLoginV3Request{ Email: email, @@ -658,7 +657,6 @@ func getFreshUserCredentialsWithSrp(email string, password string) (*api.GetLogi if err != nil { return nil, nil, err } - httpClient.SetRetryCount(5) params := srp.GetParams(4096) secret1 := srp.GenKey() diff --git a/packages/pam/agent/run.go b/packages/pam/agent/run.go index c17d812b..c65c380b 100644 --- a/packages/pam/agent/run.go +++ b/packages/pam/agent/run.go @@ -54,7 +54,10 @@ type Options struct { // Run binds a proxy per account and launches the agent. It returns the child's exit code. func Run(opts Options) (int, error) { - httpClient := resty.New() + httpClient, err := util.GetRestyClientWithCustomHeaders() + if err != nil { + return 1, fmt.Errorf("failed to build the API client: %w", err) + } httpClient.SetHeader("User-Agent", api.USER_AGENT) // Read the token per request rather than fixing it once. Sessions are created lazily and ended at diff --git a/packages/pam/local/access.go b/packages/pam/local/access.go index d0debefa..6e50ab90 100644 --- a/packages/pam/local/access.go +++ b/packages/pam/local/access.go @@ -68,7 +68,11 @@ func StartPAMAccess(accessToken, path, reason, durationStr, targetHost string, p log.Info().Msgf("Starting PAM access for: %s", strings.TrimPrefix(displayPath, "/")) log.Info().Msgf("Session duration: %s", durationStr) - httpClient := resty.New() + httpClient, err := util.GetRestyClientWithCustomHeaders() + if err != nil { + util.HandleError(err, "Failed to build the API client") + return + } httpClient.SetAuthToken(accessToken) httpClient.SetHeader("User-Agent", api.USER_AGENT) diff --git a/packages/util/common.go b/packages/util/common.go index 125db9e6..83ed576b 100644 --- a/packages/util/common.go +++ b/packages/util/common.go @@ -45,7 +45,16 @@ func ValidateInfisicalAPIConnection() (ok bool) { return err == nil } +// GetRestyClientWithCustomHeaders is the single place API clients are built. Retries are applied +// here so request sites don't have to opt in, which means new api.Call* usage gets them for free. +// Do not construct resty clients directly; TestNoDirectRestyConstruction enforces this. func GetRestyClientWithCustomHeaders() (*resty.Client, error) { + return GetRestyClientWithPolicy(DefaultRetryPolicy()) +} + +// GetRestyClientWithPolicy builds an API client with a specific retry policy. Long-running commands +// pass AgentRetryPolicy() to ride out an outage instead of exiting on one. +func GetRestyClientWithPolicy(policy RetryPolicy) (*resty.Client, error) { httpClient := resty.New() customHeaders := os.Getenv("INFISICAL_CUSTOM_HEADERS") if customHeaders != "" { @@ -56,7 +65,7 @@ func GetRestyClientWithCustomHeaders() (*resty.Client, error) { httpClient.SetHeaders(headers) } - return httpClient, nil + return applyRetryPolicy(httpClient, policy), nil } func GetInfisicalCustomHeadersMap() (map[string]string, error) { diff --git a/packages/util/helper.go b/packages/util/helper.go index 2c8625bb..92bb763d 100644 --- a/packages/util/helper.go +++ b/packages/util/helper.go @@ -334,10 +334,6 @@ func UniversalAuthLogin(clientId string, clientSecret string) (api.UniversalAuth return api.UniversalAuthLoginResponse{}, err } - httpClient.SetRetryCount(10000). - SetRetryMaxWaitTime(20 * time.Second). - SetRetryWaitTime(5 * time.Second) - tokenResponse, err := api.CallUniversalAuthLogin(httpClient, api.UniversalAuthLoginRequest{ClientId: clientId, ClientSecret: clientSecret}) if err != nil { return api.UniversalAuthLoginResponse{}, err @@ -353,10 +349,6 @@ func RenewMachineIdentityAccessToken(accessToken string) (string, error) { return "", err } - httpClient.SetRetryCount(10000). - SetRetryMaxWaitTime(20 * time.Second). - SetRetryWaitTime(5 * time.Second) - request := api.UniversalAuthRefreshRequest{ AccessToken: accessToken, } diff --git a/packages/util/retry.go b/packages/util/retry.go new file mode 100644 index 00000000..178ef772 --- /dev/null +++ b/packages/util/retry.go @@ -0,0 +1,331 @@ +package util + +import ( + "context" + "crypto/tls" + "crypto/x509" + "errors" + "io" + "net" + "net/http" + "os" + "strconv" + "strings" + "syscall" + "time" + + "github.com/go-resty/resty/v2" + "github.com/rs/zerolog/log" +) + +// Retries live here so request sites don't configure them. Every resty client used to talk to the +// Infisical API is built by GetRestyClientWithCustomHeaders, which applies DefaultRetryPolicy, so a +// new api.Call* inherits retries without doing anything. Long-running commands that should ride out +// an outage instead of exiting use GetRestyClientWithPolicy(AgentRetryPolicy()). + +const ( + defaultRetryMaxRetries = 3 + defaultRetryBaseDelay = 500 * time.Millisecond + defaultRetryMaxDelay = 10 * time.Second + + // Long-running commands (agent, gateway, relay) keep trying for roughly a quarter hour so a + // brief upstream outage doesn't take the process down with it. + agentRetryMaxRetries = 30 + agentRetryMaxDelay = 30 * time.Second +) + +// retryableStatusCodes are the responses worth repeating. Deliberately narrow: a 401 or 404 does not +// improve on the third attempt, and retrying it just multiplies the cost of a bad token or a typo. +var retryableStatusCodes = map[int]bool{ + http.StatusTooManyRequests: true, // 429 + http.StatusBadGateway: true, // 502 + http.StatusServiceUnavailable: true, // 503 + http.StatusGatewayTimeout: true, // 504 +} + +// RetryPolicy bounds how a resty client retries transient API failures. Time spent sleeping between +// attempts is at most MaxRetries * MaxDelay, so those two fields together decide how long a command +// keeps trying before it gives up. +type RetryPolicy struct { + // MaxRetries counts attempts after the first, matching resty's SetRetryCount. Zero disables + // retries. + MaxRetries int + BaseDelay time.Duration + MaxDelay time.Duration +} + +// DefaultRetryPolicy suits one-shot commands, where a user or script is waiting on the result and +// would rather see the error than sit through a long backoff. +func DefaultRetryPolicy() RetryPolicy { + return RetryPolicy{ + MaxRetries: defaultRetryMaxRetries, + BaseDelay: defaultRetryBaseDelay, + MaxDelay: defaultRetryMaxDelay, + }.withEnvOverrides() +} + +// BestEffortRetryPolicy suits calls whose failure is tolerable and whose latency is not, such as +// usage reporting that also runs on the shutdown path. It absorbs a single blip without making the +// user wait on a full backoff for a result nobody reads. +func BestEffortRetryPolicy() RetryPolicy { + return RetryPolicy{ + MaxRetries: 1, + BaseDelay: 200 * time.Millisecond, + MaxDelay: time.Second, + }.withEnvOverrides() +} + +// AgentRetryPolicy suits processes expected to outlive a transient outage rather than exit on one. +func AgentRetryPolicy() RetryPolicy { + return RetryPolicy{ + MaxRetries: agentRetryMaxRetries, + BaseDelay: defaultRetryBaseDelay, + MaxDelay: agentRetryMaxDelay, + }.withEnvOverrides() +} + +// withEnvOverrides applies the INFISICAL_RETRY_* variables on top of whichever defaults were chosen. +// A malformed value is reported and skipped rather than fatal, since this runs while building a +// client for a command that would otherwise work fine. +func (p RetryPolicy) withEnvOverrides() RetryPolicy { + if raw := os.Getenv(INFISICAL_RETRY_BASE_DELAY_NAME); raw != "" { + if delay, err := ParseTimeDurationString(raw, true); err == nil { + p.BaseDelay = delay + } else { + log.Warn().Msgf("ignoring %s: %v", INFISICAL_RETRY_BASE_DELAY_NAME, err) + } + } + + if raw := os.Getenv(INFISICAL_RETRY_MAX_DELAY_NAME); raw != "" { + if delay, err := ParseTimeDurationString(raw, true); err == nil { + p.MaxDelay = delay + } else { + log.Warn().Msgf("ignoring %s: %v", INFISICAL_RETRY_MAX_DELAY_NAME, err) + } + } + + if raw := os.Getenv(INFISICAL_RETRY_MAX_RETRIES_NAME); raw != "" { + if maxRetries, err := strconv.Atoi(raw); err == nil && maxRetries >= 0 { + p.MaxRetries = maxRetries + } else { + log.Warn().Msgf("ignoring %s: must be a non-negative integer, got %q", INFISICAL_RETRY_MAX_RETRIES_NAME, raw) + } + } + + if p.MaxDelay > 0 && p.BaseDelay > p.MaxDelay { + p.BaseDelay = p.MaxDelay + } + + return p +} + +// applyRetryPolicy installs the policy on a client. Safe to call on a client the caller will go on +// to configure further; only retry settings are touched. +func applyRetryPolicy(httpClient *resty.Client, policy RetryPolicy) *resty.Client { + if policy.MaxRetries <= 0 { + return httpClient + } + + httpClient. + SetRetryCount(policy.MaxRetries). + SetRetryWaitTime(policy.BaseDelay). + SetRetryMaxWaitTime(policy.MaxDelay). + SetRetryAfter(func(_ *resty.Client, res *resty.Response) (time.Duration, error) { + return retryDelay(res, policy), nil + }) + + // Resty drops its built-in "retry when err != nil" default as soon as a condition is added + // (retry.go Backoff), so this one condition has to cover transport errors and status codes both. + httpClient.AddRetryCondition(func(res *resty.Response, err error) bool { + return shouldRetryRequest(res, err) + }) + + httpClient.AddRetryHook(retryLogger(policy)) + + return httpClient +} + +// shouldRetryRequest decides whether a failed attempt is worth repeating. It works from an allow +// list, so anything unrecognised falls through to "don't retry" and surfaces to the caller +// immediately. +func shouldRetryRequest(res *resty.Response, err error) bool { + // The caller cancelled or its deadline passed. Further attempts cannot help and would ignore + // what the caller asked for. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return false + } + + // A transport error means no usable response arrived. res may be nil here, so decide on the + // error alone and do not touch res. + if err != nil { + return isRetryableTransportError(err) + } + + if res == nil { + return false + } + + if !retryableStatusCodes[res.StatusCode()] { + return false + } + + return methodAllowsStatusRetry(res.Request.Method, res.StatusCode()) +} + +// methodAllowsStatusRetry gates status-code retries on whether repeating the request is safe. +// +// A 429 is always safe: the server is stating it rejected the request without acting on it. The 5xx +// codes are not, for methods that aren't idempotent. A 504 can mean the server did process the write +// and only the response was lost, so replaying a POST risks double-applying it, for instance minting +// a second dynamic secret lease. Those surface to the caller instead. +func methodAllowsStatusRetry(method string, statusCode int) bool { + if statusCode == http.StatusTooManyRequests { + return true + } + + switch method { + case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodPut, http.MethodDelete: + return true + default: // POST, PATCH, anything unrecognised + return false + } +} + +// isRetryableTransportError reports whether a request failed in a way a retry could plausibly fix. +// Checks are on error types and syscall errnos rather than message substrings, so they don't depend +// on how the runtime happens to phrase things. +func isRetryableTransportError(err error) bool { + if err == nil { + return false + } + + // TLS trust failures are deterministic. They satisfy net.Error below, so rule them out first; + // otherwise a misconfigured CA bundle costs the user every attempt and every backoff before it + // reports the real problem. + var certErr *tls.CertificateVerificationError + if errors.As(err, &certErr) { + return false + } + + var unknownAuthorityErr x509.UnknownAuthorityError + if errors.As(err, &unknownAuthorityErr) { + return false + } + + var hostnameErr x509.HostnameError + if errors.As(err, &hostnameErr) { + return false + } + + var certInvalidErr x509.CertificateInvalidError + if errors.As(err, &certInvalidErr) { + return false + } + + // Covers dial timeouts, DNS failures and refused connections: http.Client wraps transport + // failures in *url.Error, which satisfies net.Error. + var netErr net.Error + if errors.As(err, &netErr) { + return true + } + + // Connection torn down mid-flight, most often a load balancer recycling a keep-alive connection. + for _, errno := range []syscall.Errno{ + syscall.ECONNRESET, + syscall.ECONNREFUSED, + syscall.ECONNABORTED, + syscall.EPIPE, + syscall.EHOSTUNREACH, + syscall.ENETUNREACH, + syscall.ETIMEDOUT, + } { + if errors.Is(err, errno) { + return true + } + } + + // Keep-alive connection closed between our write and the server's response. + return errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) +} + +// retryDelay returns how long to wait before the next attempt. Zero hands the decision back to +// resty's jittered exponential backoff, which is what we want whenever the server gave no guidance. +func retryDelay(res *resty.Response, policy RetryPolicy) time.Duration { + if res == nil { + return 0 + } + + wait, ok := parseRetryAfter(res.Header().Get("Retry-After")) + if !ok { + return 0 + } + + // The server told us exactly how long to wait, so prefer it over our own guess. Still cap it: + // a misconfigured or hostile Retry-After shouldn't be able to park the CLI indefinitely. + if policy.MaxDelay > 0 && wait > policy.MaxDelay { + return policy.MaxDelay + } + + return wait +} + +// parseRetryAfter reads a Retry-After header in either RFC 9110 form, a delay in seconds or an +// absolute HTTP date. ok is false when the header is absent, unparseable, or already in the past, +// leaving the caller on its default backoff. +func parseRetryAfter(value string) (time.Duration, bool) { + value = strings.TrimSpace(value) + if value == "" { + return 0, false + } + + if seconds, err := strconv.Atoi(value); err == nil { + if seconds <= 0 { + return 0, false + } + return time.Duration(seconds) * time.Second, true + } + + if deadline, err := http.ParseTime(value); err == nil { + if wait := time.Until(deadline); wait > 0 { + return wait, true + } + } + + return 0, false +} + +// retryLogger records each retryable failure at debug level. Retries are routine on a flaky network +// and warning on every one would be noise for scripted use, so this stays behind --log-level debug. +func retryLogger(policy RetryPolicy) resty.OnRetryFunc { + return func(res *resty.Response, err error) { + event := log.Debug() + exhausted := false + + if res != nil && res.Request != nil { + // Attempt counts from 1, so the last one lands on MaxRetries+1. Resty runs retry hooks + // on that attempt too, and calling it a retry there would be a lie. + exhausted = res.Request.Attempt > policy.MaxRetries + + event = event. + Str("method", res.Request.Method). + Str("url", res.Request.URL). + Int("attempt", res.Request.Attempt). + Int("maxRetries", policy.MaxRetries) + + if res.StatusCode() != 0 { + event = event.Int("status", res.StatusCode()) + } + } + + if err != nil { + event = event.Err(err) + } + + if exhausted { + event.Msg("request failed and retries are exhausted") + return + } + + event.Msg("request failed, retrying") + } +} diff --git a/packages/util/retry_test.go b/packages/util/retry_test.go new file mode 100644 index 00000000..14a118ef --- /dev/null +++ b/packages/util/retry_test.go @@ -0,0 +1,441 @@ +package util + +import ( + "context" + "crypto/x509" + "errors" + "fmt" + "io/fs" + "net" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/go-resty/resty/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testPolicy keeps the delays negligible so the suite exercises retry decisions rather than backoff. +func testPolicy(maxRetries int) RetryPolicy { + return RetryPolicy{ + MaxRetries: maxRetries, + BaseDelay: time.Millisecond, + MaxDelay: 5 * time.Millisecond, + } +} + +// newTestClient returns a client on testPolicy plus a counter of attempts actually dispatched. +// Counting client-side rather than in the handler also covers failures that never reach a server. +func newTestClient(t *testing.T, policy RetryPolicy) (*resty.Client, *atomic.Int32) { + t.Helper() + + var attempts atomic.Int32 + client := applyRetryPolicy(resty.New(), policy) + client.OnBeforeRequest(func(_ *resty.Client, _ *resty.Request) error { + attempts.Add(1) + return nil + }) + + return client, &attempts +} + +func TestRetryStatusCodes(t *testing.T) { + const maxRetries = 2 + + tests := []struct { + name string + status int + wantAttempts int32 + }{ + {"429 too many requests is retried", http.StatusTooManyRequests, maxRetries + 1}, + {"502 bad gateway is retried", http.StatusBadGateway, maxRetries + 1}, + {"503 service unavailable is retried", http.StatusServiceUnavailable, maxRetries + 1}, + {"504 gateway timeout is retried", http.StatusGatewayTimeout, maxRetries + 1}, + + // Permanent failures must surface on the first attempt. Retrying them multiplies the cost of + // a bad token or a typo and delays the error the user needs to see. + {"400 bad request is not retried", http.StatusBadRequest, 1}, + {"401 unauthorized is not retried", http.StatusUnauthorized, 1}, + {"403 forbidden is not retried", http.StatusForbidden, 1}, + {"404 not found is not retried", http.StatusNotFound, 1}, + {"422 unprocessable is not retried", http.StatusUnprocessableEntity, 1}, + {"500 internal server error is not retried", http.StatusInternalServerError, 1}, + {"200 ok is not retried", http.StatusOK, 1}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(test.status) + })) + defer server.Close() + + client, attempts := newTestClient(t, testPolicy(maxRetries)) + res, err := client.R().Get(server.URL) + + require.NoError(t, err, "a status-code failure should surface as a response, not an error") + assert.Equal(t, test.status, res.StatusCode()) + assert.Equal(t, test.wantAttempts, attempts.Load()) + }) + } +} + +// POST is not safely repeatable. A 502/503/504 can mean the server did process the write and only +// the response was lost, so replaying it risks double-applying, for instance minting a second +// dynamic secret lease. A 429 is safe because the server states it rejected the request outright. +func TestRetryMethodSafety(t *testing.T) { + const maxRetries = 2 + + tests := []struct { + method string + status int + wantAttempts int32 + }{ + {http.MethodGet, http.StatusGatewayTimeout, maxRetries + 1}, + {http.MethodPut, http.StatusGatewayTimeout, maxRetries + 1}, + {http.MethodDelete, http.StatusGatewayTimeout, maxRetries + 1}, + {http.MethodPost, http.StatusGatewayTimeout, 1}, + {http.MethodPost, http.StatusBadGateway, 1}, + {http.MethodPost, http.StatusServiceUnavailable, 1}, + {http.MethodPost, http.StatusTooManyRequests, maxRetries + 1}, + {http.MethodPatch, http.StatusGatewayTimeout, 1}, + {http.MethodPatch, http.StatusTooManyRequests, maxRetries + 1}, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%s %d", test.method, test.status), func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(test.status) + })) + defer server.Close() + + client, attempts := newTestClient(t, testPolicy(maxRetries)) + _, err := client.R().Execute(test.method, server.URL) + + require.NoError(t, err) + assert.Equal(t, test.wantAttempts, attempts.Load()) + }) + } +} + +func TestRetrySucceedsAfterTransientFailure(t *testing.T) { + var hits atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if hits.Add(1) < 3 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + defer server.Close() + + client, attempts := newTestClient(t, testPolicy(3)) + res, err := client.R().Get(server.URL) + + require.NoError(t, err) + assert.Equal(t, http.StatusOK, res.StatusCode()) + assert.Equal(t, `{"ok":true}`, res.String()) + assert.Equal(t, int32(3), attempts.Load(), "should stop retrying as soon as a request succeeds") +} + +func TestRetryDisabledWhenMaxRetriesIsZero(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + client, attempts := newTestClient(t, testPolicy(0)) + _, err := client.R().Get(server.URL) + + require.NoError(t, err) + assert.Equal(t, int32(1), attempts.Load()) +} + +func TestRetryOnTransportError(t *testing.T) { + // Bind then release a port so the address is routable but nothing is listening, which is the + // connection-refused case the CLI hits when an instance is down. + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + deadURL := fmt.Sprintf("http://%s", listener.Addr().String()) + require.NoError(t, listener.Close()) + + client, attempts := newTestClient(t, testPolicy(2)) + _, err = client.R().Get(deadURL) + + require.Error(t, err) + assert.Equal(t, int32(3), attempts.Load()) +} + +// A TLS trust failure is deterministic, so retrying it only delays the real error. It satisfies +// net.Error, which is why the policy rules certificate errors out explicitly. +func TestNoRetryOnTLSTrustFailure(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + client, attempts := newTestClient(t, testPolicy(3)) + _, err := client.R().Get(server.URL) + + require.Error(t, err) + assert.Equal(t, int32(1), attempts.Load()) +} + +func TestNoRetryOnContextCancellation(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + })) + defer server.Close() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + client, attempts := newTestClient(t, testPolicy(3)) + _, err := client.R().SetContext(ctx).Get(server.URL) + + require.Error(t, err) + assert.LessOrEqual(t, attempts.Load(), int32(1)) +} + +func TestRetryHonorsRetryAfterHeader(t *testing.T) { + const retryAfterSeconds = 1 + + var hits atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if hits.Add(1) == 1 { + w.Header().Set("Retry-After", fmt.Sprint(retryAfterSeconds)) + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + // MaxDelay has to exceed Retry-After, otherwise the cap is what we would be measuring. + policy := RetryPolicy{MaxRetries: 2, BaseDelay: time.Millisecond, MaxDelay: 5 * time.Second} + client, _ := newTestClient(t, policy) + + start := time.Now() + res, err := client.R().Get(server.URL) + elapsed := time.Since(start) + + require.NoError(t, err) + assert.Equal(t, http.StatusOK, res.StatusCode()) + assert.GreaterOrEqual(t, elapsed, retryAfterSeconds*time.Second, + "should wait as long as the server asked rather than using its own backoff") +} + +func TestParseRetryAfter(t *testing.T) { + tests := []struct { + name string + value string + want time.Duration + ok bool + }{ + {"empty header", "", 0, false}, + {"seconds", "30", 30 * time.Second, true}, + {"seconds with surrounding space", " 5 ", 5 * time.Second, true}, + {"zero seconds falls back to default backoff", "0", 0, false}, + {"negative seconds falls back to default backoff", "-5", 0, false}, + {"unparseable value falls back to default backoff", "soon", 0, false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, ok := parseRetryAfter(test.value) + assert.Equal(t, test.ok, ok) + assert.Equal(t, test.want, got) + }) + } + + t.Run("future http date", func(t *testing.T) { + got, ok := parseRetryAfter(time.Now().Add(30 * time.Second).UTC().Format(http.TimeFormat)) + require.True(t, ok) + // The header has second granularity and time passes during the call, so allow slack. + assert.InDelta(t, (30 * time.Second).Seconds(), got.Seconds(), 2) + }) + + t.Run("past http date falls back to default backoff", func(t *testing.T) { + _, ok := parseRetryAfter(time.Now().Add(-time.Minute).UTC().Format(http.TimeFormat)) + assert.False(t, ok) + }) +} + +// A misconfigured or hostile Retry-After must not be able to park the CLI indefinitely. +func TestRetryDelayCapsRetryAfterAtMaxDelay(t *testing.T) { + policy := RetryPolicy{MaxRetries: 3, BaseDelay: time.Second, MaxDelay: 10 * time.Second} + + res := &resty.Response{RawResponse: &http.Response{Header: http.Header{}}} + res.RawResponse.Header.Set("Retry-After", "3600") + + assert.Equal(t, policy.MaxDelay, retryDelay(res, policy)) +} + +func TestRetryDelayFallsBackWithoutHeader(t *testing.T) { + policy := RetryPolicy{MaxRetries: 3, BaseDelay: time.Second, MaxDelay: 10 * time.Second} + + assert.Zero(t, retryDelay(nil, policy), "a nil response should defer to resty's own backoff") + + res := &resty.Response{RawResponse: &http.Response{Header: http.Header{}}} + assert.Zero(t, retryDelay(res, policy), "an absent header should defer to resty's own backoff") +} + +func TestIsRetryableTransportError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"connection reset", syscall.ECONNRESET, true}, + {"connection refused", syscall.ECONNREFUSED, true}, + {"broken pipe", syscall.EPIPE, true}, + {"host unreachable", syscall.EHOSTUNREACH, true}, + {"network unreachable", syscall.ENETUNREACH, true}, + {"dns failure", &net.DNSError{Err: "no such host", Name: "app.infisical.com"}, true}, + {"wrapped connection reset", fmt.Errorf("posting secret: %w", syscall.ECONNRESET), true}, + + {"untrusted certificate authority", x509.UnknownAuthorityError{}, false}, + {"certificate hostname mismatch", x509.HostnameError{Host: "app.infisical.com"}, false}, + {"json marshal failure", errors.New("json: unsupported type"), false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.want, isRetryableTransportError(test.err)) + }) + } +} + +func TestRetryPolicyEnvOverrides(t *testing.T) { + t.Run("defaults apply when unset", func(t *testing.T) { + t.Setenv(INFISICAL_RETRY_BASE_DELAY_NAME, "") + t.Setenv(INFISICAL_RETRY_MAX_DELAY_NAME, "") + t.Setenv(INFISICAL_RETRY_MAX_RETRIES_NAME, "") + + policy := DefaultRetryPolicy() + assert.Equal(t, defaultRetryMaxRetries, policy.MaxRetries) + assert.Equal(t, defaultRetryBaseDelay, policy.BaseDelay) + assert.Equal(t, defaultRetryMaxDelay, policy.MaxDelay) + }) + + t.Run("env overrides are applied", func(t *testing.T) { + t.Setenv(INFISICAL_RETRY_BASE_DELAY_NAME, "250ms") + t.Setenv(INFISICAL_RETRY_MAX_DELAY_NAME, "45s") + t.Setenv(INFISICAL_RETRY_MAX_RETRIES_NAME, "7") + + policy := DefaultRetryPolicy() + assert.Equal(t, 7, policy.MaxRetries) + assert.Equal(t, 250*time.Millisecond, policy.BaseDelay) + assert.Equal(t, 45*time.Second, policy.MaxDelay) + }) + + t.Run("env overrides also apply to the agent policy", func(t *testing.T) { + t.Setenv(INFISICAL_RETRY_BASE_DELAY_NAME, "") + t.Setenv(INFISICAL_RETRY_MAX_DELAY_NAME, "") + t.Setenv(INFISICAL_RETRY_MAX_RETRIES_NAME, "2") + + policy := AgentRetryPolicy() + assert.Equal(t, 2, policy.MaxRetries, "env should win over the agent's higher default") + assert.Equal(t, agentRetryMaxDelay, policy.MaxDelay) + }) + + // A bad value in the environment should not stop a command that would otherwise work. + t.Run("malformed values are ignored", func(t *testing.T) { + t.Setenv(INFISICAL_RETRY_BASE_DELAY_NAME, "soon") + t.Setenv(INFISICAL_RETRY_MAX_DELAY_NAME, "") + t.Setenv(INFISICAL_RETRY_MAX_RETRIES_NAME, "lots") + + policy := DefaultRetryPolicy() + assert.Equal(t, defaultRetryMaxRetries, policy.MaxRetries) + assert.Equal(t, defaultRetryBaseDelay, policy.BaseDelay) + }) + + t.Run("base delay is clamped to max delay", func(t *testing.T) { + t.Setenv(INFISICAL_RETRY_BASE_DELAY_NAME, "30s") + t.Setenv(INFISICAL_RETRY_MAX_DELAY_NAME, "5s") + t.Setenv(INFISICAL_RETRY_MAX_RETRIES_NAME, "") + + policy := DefaultRetryPolicy() + assert.Equal(t, 5*time.Second, policy.BaseDelay) + assert.Equal(t, 5*time.Second, policy.MaxDelay) + }) +} + +// The constructors are only a single source of truth while nothing bypasses them, and a bypass is +// invisible in review: the client works, it just silently has no retries. +func TestClientsAreBuiltThroughTheSharedConstructor(t *testing.T) { + t.Setenv(INFISICAL_RETRY_MAX_RETRIES_NAME, "") + + client, err := GetRestyClientWithCustomHeaders() + require.NoError(t, err) + assert.Equal(t, defaultRetryMaxRetries, client.RetryCount, + "GetRestyClientWithCustomHeaders must apply the default retry policy") + + agentClient, err := GetRestyClientWithPolicy(AgentRetryPolicy()) + require.NoError(t, err) + assert.Equal(t, agentRetryMaxRetries, agentClient.RetryCount) +} + +// TestNoDirectRestyConstruction is the mechanism that keeps the retry policy single-sourced. A +// direct resty.New() compiles, runs, and looks correct in review; it just silently has no retries, +// which is exactly the bug this package exists to prevent. Add new clients via +// GetRestyClientWithCustomHeaders or GetRestyClientWithPolicy instead. +func TestNoDirectRestyConstruction(t *testing.T) { + // Files allowed to construct a client directly, relative to the repo root. + allowed := map[string]bool{ + filepath.Join("packages", "util", "common.go"): true, + } + + packagesDir := filepath.Join("..", "..", "packages") + repoRoot := filepath.Join("..", "..") + + var offenders []string + + err := filepath.WalkDir(packagesDir, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + if entry.IsDir() || !strings.HasSuffix(path, ".go") { + return nil + } + // Tests build throwaway clients on purpose and never talk to the real API. + if strings.HasSuffix(path, "_test.go") { + return nil + } + + relative, err := filepath.Rel(repoRoot, path) + if err != nil { + return err + } + if allowed[relative] { + return nil + } + + contents, err := os.ReadFile(path) + if err != nil { + return err + } + + for i, line := range strings.Split(string(contents), "\n") { + if strings.Contains(line, "resty.New(") { + offenders = append(offenders, fmt.Sprintf("%s:%d", relative, i+1)) + } + } + + return nil + }) + require.NoError(t, err) + + assert.Empty(t, offenders, + "these sites construct a resty client directly and so have no retry policy; "+ + "use util.GetRestyClientWithCustomHeaders or util.GetRestyClientWithPolicy instead") +} From 958d7b13ef554272c4d0e80cb6c028be0246121c Mon Sep 17 00:00:00 2001 From: Scott Wilson Date: Thu, 27 Aug 2026 10:11:41 -0700 Subject: [PATCH 2/5] improvement: address feedback --- packages/util/retry.go | 53 ++++++++++++-- packages/util/retry_test.go | 133 ++++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 4 deletions(-) diff --git a/packages/util/retry.go b/packages/util/retry.go index 178ef772..613370a2 100644 --- a/packages/util/retry.go +++ b/packages/util/retry.go @@ -155,10 +155,22 @@ func shouldRetryRequest(res *resty.Response, err error) bool { return false } - // A transport error means no usable response arrived. res may be nil here, so decide on the - // error alone and do not touch res. + // A transport error means no usable response arrived. if err != nil { - return isRetryableTransportError(err) + if !isRetryableTransportError(err) { + return false + } + + // A failure that proves the request was never delivered is safe to replay whatever the + // method. res is nil when resty failed in pre-request middleware, which is likewise before + // anything went out. + if res == nil || res.Request == nil || requestNeverReachedServer(err) { + return true + } + + // Otherwise the connection died mid-flight and we cannot tell whether the server already + // committed the request, so only replay methods that are safe to repeat. + return isIdempotentMethod(res.Request.Method) } if res == nil { @@ -183,14 +195,47 @@ func methodAllowsStatusRetry(method string, statusCode int) bool { return true } + return isIdempotentMethod(method) +} + +// isIdempotentMethod reports whether sending a request more than once is equivalent to sending it +// once, per RFC 9110 9.2.2. POST and PATCH are not, so replaying them can double-apply the write. +func isIdempotentMethod(method string) bool { switch method { - case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodPut, http.MethodDelete: + case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace, + http.MethodPut, http.MethodDelete: return true default: // POST, PATCH, anything unrecognised return false } } +// requestNeverReachedServer reports whether a transport error proves the request was never delivered, +// which makes replaying it safe regardless of method. Connection establishment failures qualify: if +// the dial never completed or the name never resolved, the server cannot have acted on anything. +// +// Mid-flight failures deliberately do not qualify. A reset or EOF after the request was written is +// ambiguous, because the server may have committed the write and only the response was lost. Keeping +// this distinction is what lets the agent still retry its token-refresh POST across a backend +// restart, which is the case a blanket "never replay POST" rule would give up. +func requestNeverReachedServer(err error) bool { + // Resolution never produced an address, so nothing was sent. + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) { + return true + } + + // Op is "dial" only while establishing the connection. A request already in flight reports + // "read" or "write" instead. + var opErr *net.OpError + if errors.As(err, &opErr) && opErr.Op == "dial" { + return true + } + + // A refused connection is always rejected at establishment time. + return errors.Is(err, syscall.ECONNREFUSED) +} + // isRetryableTransportError reports whether a request failed in a way a retry could plausibly fix. // Checks are on error types and syscall errnos rather than message substrings, so they don't depend // on how the runtime happens to phrase things. diff --git a/packages/util/retry_test.go b/packages/util/retry_test.go index 14a118ef..820e2e8a 100644 --- a/packages/util/retry_test.go +++ b/packages/util/retry_test.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "errors" "fmt" + "io" "io/fs" "net" "net/http" @@ -439,3 +440,135 @@ func TestNoDirectRestyConstruction(t *testing.T) { "these sites construct a resty client directly and so have no retry policy; "+ "use util.GetRestyClientWithCustomHeaders or util.GetRestyClientWithPolicy instead") } + +// newAbruptCloseServer accepts connections, reads the request, then drops the connection without +// responding. That is the ambiguous mid-flight failure: the request was delivered, so the server may +// already have acted on it, and only the response was lost. +func newAbruptCloseServer(t *testing.T) (serverURL string, connections func() int32) { + t.Helper() + + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = listener.Close() }) + + var count atomic.Int32 + go func() { + for { + conn, err := listener.Accept() + if err != nil { + return + } + count.Add(1) + _ = conn.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, _ = conn.Read(make([]byte, 4096)) + _ = conn.Close() + } + }() + + return "http://" + listener.Addr().String(), count.Load +} + +// deadAddress returns an address that is routable but has nothing listening, so dialing it is +// refused at connection establishment and the request provably never reaches a server. +func deadAddress(t *testing.T) string { + t.Helper() + + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := listener.Addr().String() + require.NoError(t, listener.Close()) + + return "http://" + addr +} + +// Transport failures have to respect method safety too, but only when they are ambiguous. Treating +// every transport error as unsafe for POST would give up the case this feature mainly exists for: +// retrying across a backend that is briefly down. +func TestTransportErrorMethodSafety(t *testing.T) { + const maxRetries = 2 + + t.Run("mid-flight failure does not replay POST", func(t *testing.T) { + serverURL, _ := newAbruptCloseServer(t) + + client, attempts := newTestClient(t, testPolicy(maxRetries)) + _, err := client.R().SetBody(`{"lease":"request"}`).Post(serverURL) + + require.Error(t, err) + assert.Equal(t, int32(1), attempts.Load(), + "an ambiguous mid-flight failure must not replay a write: the server may have committed it") + }) + + t.Run("mid-flight failure does not replay PATCH", func(t *testing.T) { + serverURL, _ := newAbruptCloseServer(t) + + client, attempts := newTestClient(t, testPolicy(maxRetries)) + _, err := client.R().SetBody(`{}`).Patch(serverURL) + + require.Error(t, err) + assert.Equal(t, int32(1), attempts.Load()) + }) + + t.Run("mid-flight failure replays GET", func(t *testing.T) { + serverURL, _ := newAbruptCloseServer(t) + + client, attempts := newTestClient(t, testPolicy(maxRetries)) + _, err := client.R().Get(serverURL) + + require.Error(t, err) + assert.Equal(t, int32(maxRetries+1), attempts.Load(), + "a read is idempotent, so an ambiguous failure is still safe to repeat") + }) + + t.Run("connection refused replays POST", func(t *testing.T) { + client, attempts := newTestClient(t, testPolicy(maxRetries)) + _, err := client.R().SetBody(`{}`).Post(deadAddress(t)) + + require.Error(t, err) + assert.Equal(t, int32(maxRetries+1), attempts.Load(), + "the dial never completed, so the server cannot have seen the write") + }) + + t.Run("unresolvable host replays POST", func(t *testing.T) { + client, attempts := newTestClient(t, testPolicy(maxRetries)) + _, err := client.R().SetBody(`{}`).Post("http://this-host-does-not-exist.invalid") + + require.Error(t, err) + assert.Equal(t, int32(maxRetries+1), attempts.Load(), + "resolution never produced an address, so nothing was sent") + }) +} + +func TestRequestNeverReachedServer(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"connection refused", syscall.ECONNREFUSED, true}, + {"wrapped connection refused", fmt.Errorf("post: %w", syscall.ECONNREFUSED), true}, + {"dns failure", &net.DNSError{Err: "no such host", Name: "app.infisical.com"}, true}, + {"dial failure", &net.OpError{Op: "dial", Err: syscall.ETIMEDOUT}, true}, + + // Delivered, then the connection died. The server may already have committed the write. + {"read failure mid-flight", &net.OpError{Op: "read", Err: syscall.ECONNRESET}, false}, + {"write failure mid-flight", &net.OpError{Op: "write", Err: syscall.EPIPE}, false}, + {"bare connection reset", syscall.ECONNRESET, false}, + {"unexpected eof", io.ErrUnexpectedEOF, false}, + {"eof", io.EOF, false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.want, requestNeverReachedServer(test.err)) + }) + } +} + +func TestIsIdempotentMethod(t *testing.T) { + for _, method := range []string{http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace, http.MethodPut, http.MethodDelete} { + assert.True(t, isIdempotentMethod(method), method) + } + for _, method := range []string{http.MethodPost, http.MethodPatch, "PROPFIND", ""} { + assert.False(t, isIdempotentMethod(method), method) + } +} From bbd8ead3badbc217ce0d09fc06dd185c7258d89c Mon Sep 17 00:00:00 2001 From: Scott Wilson Date: Thu, 27 Aug 2026 18:03:35 -0700 Subject: [PATCH 3/5] improvement: address feedback --- packages/cmd/agent.go | 5 +- packages/util/common.go | 8 +- packages/util/helper.go | 9 +- packages/util/retry.go | 216 +++++++++++------------------------- packages/util/retry_test.go | 145 +++++++++++++----------- 5 files changed, 158 insertions(+), 225 deletions(-) diff --git a/packages/cmd/agent.go b/packages/cmd/agent.go index 8a411360..9b82db17 100644 --- a/packages/cmd/agent.go +++ b/packages/cmd/agent.go @@ -1684,7 +1684,10 @@ func (tm *AgentManager) RevokeCredentials() error { // Refreshes the existing access token func (tm *AgentManager) RefreshAccessToken(accessToken string) error { - httpClient, err := util.GetRestyClientWithPolicy(util.AgentRetryPolicy()) + policy := util.AgentRetryPolicy() + policy.ReplaySafe = true // renewal extends the presented token, so a replay cannot double-apply + + httpClient, err := util.GetRestyClientWithPolicy(policy) if err != nil { return err } diff --git a/packages/util/common.go b/packages/util/common.go index 83ed576b..3849e5c8 100644 --- a/packages/util/common.go +++ b/packages/util/common.go @@ -45,15 +45,13 @@ func ValidateInfisicalAPIConnection() (ok bool) { return err == nil } -// GetRestyClientWithCustomHeaders is the single place API clients are built. Retries are applied -// here so request sites don't have to opt in, which means new api.Call* usage gets them for free. -// Do not construct resty clients directly; TestNoDirectRestyConstruction enforces this. +// GetRestyClientWithCustomHeaders is the single place API clients are built, which is what applies +// the retry policy everywhere. Do not construct resty clients directly; TestNoDirectRestyConstruction +// enforces this. func GetRestyClientWithCustomHeaders() (*resty.Client, error) { return GetRestyClientWithPolicy(DefaultRetryPolicy()) } -// GetRestyClientWithPolicy builds an API client with a specific retry policy. Long-running commands -// pass AgentRetryPolicy() to ride out an outage instead of exiting on one. func GetRestyClientWithPolicy(policy RetryPolicy) (*resty.Client, error) { httpClient := resty.New() customHeaders := os.Getenv("INFISICAL_CUSTOM_HEADERS") diff --git a/packages/util/helper.go b/packages/util/helper.go index 92bb763d..7d3a6cc6 100644 --- a/packages/util/helper.go +++ b/packages/util/helper.go @@ -329,7 +329,10 @@ func GetInfisicalToken(cmd *cobra.Command) (token *models.TokenDetails, err erro } func UniversalAuthLogin(clientId string, clientSecret string) (api.UniversalAuthLoginResponse, error) { - httpClient, err := GetRestyClientWithCustomHeaders() + policy := DefaultRetryPolicy() + policy.ReplaySafe = true // a replayed login only mints another TTL-bound token + + httpClient, err := GetRestyClientWithPolicy(policy) if err != nil { return api.UniversalAuthLoginResponse{}, err } @@ -343,8 +346,10 @@ func UniversalAuthLogin(clientId string, clientSecret string) (api.UniversalAuth } func RenewMachineIdentityAccessToken(accessToken string) (string, error) { + policy := DefaultRetryPolicy() + policy.ReplaySafe = true // renewal extends the presented token, so a replay cannot double-apply - httpClient, err := GetRestyClientWithCustomHeaders() + httpClient, err := GetRestyClientWithPolicy(policy) if err != nil { return "", err } diff --git a/packages/util/retry.go b/packages/util/retry.go index 613370a2..cd842fbc 100644 --- a/packages/util/retry.go +++ b/packages/util/retry.go @@ -3,7 +3,6 @@ package util import ( "context" "crypto/tls" - "crypto/x509" "errors" "io" "net" @@ -11,51 +10,42 @@ import ( "os" "strconv" "strings" - "syscall" "time" "github.com/go-resty/resty/v2" "github.com/rs/zerolog/log" ) -// Retries live here so request sites don't configure them. Every resty client used to talk to the -// Infisical API is built by GetRestyClientWithCustomHeaders, which applies DefaultRetryPolicy, so a -// new api.Call* inherits retries without doing anything. Long-running commands that should ride out -// an outage instead of exiting use GetRestyClientWithPolicy(AgentRetryPolicy()). - const ( defaultRetryMaxRetries = 3 defaultRetryBaseDelay = 500 * time.Millisecond defaultRetryMaxDelay = 10 * time.Second - // Long-running commands (agent, gateway, relay) keep trying for roughly a quarter hour so a - // brief upstream outage doesn't take the process down with it. agentRetryMaxRetries = 30 agentRetryMaxDelay = 30 * time.Second ) -// retryableStatusCodes are the responses worth repeating. Deliberately narrow: a 401 or 404 does not -// improve on the third attempt, and retrying it just multiplies the cost of a bad token or a typo. var retryableStatusCodes = map[int]bool{ - http.StatusTooManyRequests: true, // 429 - http.StatusBadGateway: true, // 502 - http.StatusServiceUnavailable: true, // 503 - http.StatusGatewayTimeout: true, // 504 + http.StatusTooManyRequests: true, + http.StatusBadGateway: true, + http.StatusServiceUnavailable: true, + http.StatusGatewayTimeout: true, } -// RetryPolicy bounds how a resty client retries transient API failures. Time spent sleeping between -// attempts is at most MaxRetries * MaxDelay, so those two fields together decide how long a command -// keeps trying before it gives up. +// RetryPolicy bounds how a client retries transient failures. type RetryPolicy struct { - // MaxRetries counts attempts after the first, matching resty's SetRetryCount. Zero disables - // retries. + // MaxRetries counts attempts after the first. Zero disables retries. MaxRetries int BaseDelay time.Duration MaxDelay time.Duration + + // ReplaySafe asserts that every request sent through this client is safe to send more than + // once, letting POST and PATCH retry like idempotent methods. Set it only where a replay + // cannot double-apply a write. + ReplaySafe bool } -// DefaultRetryPolicy suits one-shot commands, where a user or script is waiting on the result and -// would rather see the error than sit through a long backoff. +// DefaultRetryPolicy suits one-shot commands where a user or script is waiting on the result. func DefaultRetryPolicy() RetryPolicy { return RetryPolicy{ MaxRetries: defaultRetryMaxRetries, @@ -64,18 +54,7 @@ func DefaultRetryPolicy() RetryPolicy { }.withEnvOverrides() } -// BestEffortRetryPolicy suits calls whose failure is tolerable and whose latency is not, such as -// usage reporting that also runs on the shutdown path. It absorbs a single blip without making the -// user wait on a full backoff for a result nobody reads. -func BestEffortRetryPolicy() RetryPolicy { - return RetryPolicy{ - MaxRetries: 1, - BaseDelay: 200 * time.Millisecond, - MaxDelay: time.Second, - }.withEnvOverrides() -} - -// AgentRetryPolicy suits processes expected to outlive a transient outage rather than exit on one. +// AgentRetryPolicy suits long-running processes that should ride out an outage rather than exit. func AgentRetryPolicy() RetryPolicy { return RetryPolicy{ MaxRetries: agentRetryMaxRetries, @@ -84,9 +63,17 @@ func AgentRetryPolicy() RetryPolicy { }.withEnvOverrides() } -// withEnvOverrides applies the INFISICAL_RETRY_* variables on top of whichever defaults were chosen. -// A malformed value is reported and skipped rather than fatal, since this runs while building a -// client for a command that would otherwise work fine. +// BestEffortRetryPolicy suits fire-and-forget calls. Not env-tunable so paths that run during +// shutdown stay time-bounded. +func BestEffortRetryPolicy() RetryPolicy { + return RetryPolicy{ + MaxRetries: 1, + BaseDelay: 200 * time.Millisecond, + MaxDelay: time.Second, + } +} + +// withEnvOverrides applies INFISICAL_RETRY_*, which affect every command run in the environment. func (p RetryPolicy) withEnvOverrides() RetryPolicy { if raw := os.Getenv(INFISICAL_RETRY_BASE_DELAY_NAME); raw != "" { if delay, err := ParseTimeDurationString(raw, true); err == nil { @@ -119,8 +106,6 @@ func (p RetryPolicy) withEnvOverrides() RetryPolicy { return p } -// applyRetryPolicy installs the policy on a client. Safe to call on a client the caller will go on -// to configure further; only retry settings are touched. func applyRetryPolicy(httpClient *resty.Client, policy RetryPolicy) *resty.Client { if policy.MaxRetries <= 0 { return httpClient @@ -131,13 +116,13 @@ func applyRetryPolicy(httpClient *resty.Client, policy RetryPolicy) *resty.Clien SetRetryWaitTime(policy.BaseDelay). SetRetryMaxWaitTime(policy.MaxDelay). SetRetryAfter(func(_ *resty.Client, res *resty.Response) (time.Duration, error) { - return retryDelay(res, policy), nil + return retryDelay(res), nil }) - // Resty drops its built-in "retry when err != nil" default as soon as a condition is added - // (retry.go Backoff), so this one condition has to cover transport errors and status codes both. + // The first AddRetryCondition replaces resty's built-in retry-on-error default, so this one + // condition must cover transport errors and status codes both. httpClient.AddRetryCondition(func(res *resty.Response, err error) bool { - return shouldRetryRequest(res, err) + return shouldRetryRequest(policy, res, err) }) httpClient.AddRetryHook(retryLogger(policy)) @@ -145,178 +130,102 @@ func applyRetryPolicy(httpClient *resty.Client, policy RetryPolicy) *resty.Clien return httpClient } -// shouldRetryRequest decides whether a failed attempt is worth repeating. It works from an allow -// list, so anything unrecognised falls through to "don't retry" and surfaces to the caller -// immediately. -func shouldRetryRequest(res *resty.Response, err error) bool { - // The caller cancelled or its deadline passed. Further attempts cannot help and would ignore - // what the caller asked for. +func shouldRetryRequest(policy RetryPolicy, res *resty.Response, err error) bool { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return false } - // A transport error means no usable response arrived. if err != nil { if !isRetryableTransportError(err) { return false } - // A failure that proves the request was never delivered is safe to replay whatever the - // method. res is nil when resty failed in pre-request middleware, which is likewise before - // anything went out. - if res == nil || res.Request == nil || requestNeverReachedServer(err) { + // Pre-delivery failures are safe to replay for any method. res is nil when resty failed + // before sending; a non-nil response always carries its Request. + if res == nil || requestNeverReachedServer(err) { return true } - // Otherwise the connection died mid-flight and we cannot tell whether the server already - // committed the request, so only replay methods that are safe to repeat. - return isIdempotentMethod(res.Request.Method) + // Mid-flight failure: the server may have committed the request with only the response + // lost, so replaying a non-idempotent method could double-apply it. + return policy.ReplaySafe || isIdempotentMethod(res.Request.Method) } - if res == nil { + if res == nil || !retryableStatusCodes[res.StatusCode()] { return false } - if !retryableStatusCodes[res.StatusCode()] { + // Retrying sooner than the server asked for would only get rejected again. + if wait, ok := parseRetryAfter(res.Header().Get("Retry-After")); ok && wait > policy.MaxDelay { return false } - return methodAllowsStatusRetry(res.Request.Method, res.StatusCode()) -} - -// methodAllowsStatusRetry gates status-code retries on whether repeating the request is safe. -// -// A 429 is always safe: the server is stating it rejected the request without acting on it. The 5xx -// codes are not, for methods that aren't idempotent. A 504 can mean the server did process the write -// and only the response was lost, so replaying a POST risks double-applying it, for instance minting -// a second dynamic secret lease. Those surface to the caller instead. -func methodAllowsStatusRetry(method string, statusCode int) bool { - if statusCode == http.StatusTooManyRequests { + // A 429 was rejected before the server acted on it, so any method may repeat it. + if res.StatusCode() == http.StatusTooManyRequests { return true } - return isIdempotentMethod(method) + return policy.ReplaySafe || isIdempotentMethod(res.Request.Method) } -// isIdempotentMethod reports whether sending a request more than once is equivalent to sending it -// once, per RFC 9110 9.2.2. POST and PATCH are not, so replaying them can double-apply the write. +// Per RFC 9110 9.2.2. func isIdempotentMethod(method string) bool { switch method { case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace, http.MethodPut, http.MethodDelete: return true - default: // POST, PATCH, anything unrecognised + default: return false } } -// requestNeverReachedServer reports whether a transport error proves the request was never delivered, -// which makes replaying it safe regardless of method. Connection establishment failures qualify: if -// the dial never completed or the name never resolved, the server cannot have acted on anything. -// -// Mid-flight failures deliberately do not qualify. A reset or EOF after the request was written is -// ambiguous, because the server may have committed the write and only the response was lost. Keeping -// this distinction is what lets the agent still retry its token-refresh POST across a backend -// restart, which is the case a blanket "never replay POST" rule would give up. func requestNeverReachedServer(err error) bool { - // Resolution never produced an address, so nothing was sent. var dnsErr *net.DNSError if errors.As(err, &dnsErr) { return true } - // Op is "dial" only while establishing the connection. A request already in flight reports - // "read" or "write" instead. var opErr *net.OpError - if errors.As(err, &opErr) && opErr.Op == "dial" { - return true - } - - // A refused connection is always rejected at establishment time. - return errors.Is(err, syscall.ECONNREFUSED) + return errors.As(err, &opErr) && opErr.Op == "dial" } -// isRetryableTransportError reports whether a request failed in a way a retry could plausibly fix. -// Checks are on error types and syscall errnos rather than message substrings, so they don't depend -// on how the runtime happens to phrase things. func isRetryableTransportError(err error) bool { if err == nil { return false } - // TLS trust failures are deterministic. They satisfy net.Error below, so rule them out first; - // otherwise a misconfigured CA bundle costs the user every attempt and every backoff before it - // reports the real problem. + // Deterministic, but wrapped in *url.Error like everything else, so this must come before the + // net.Error check. crypto/tls wraps every x509 verification error in this type. var certErr *tls.CertificateVerificationError if errors.As(err, &certErr) { return false } - var unknownAuthorityErr x509.UnknownAuthorityError - if errors.As(err, &unknownAuthorityErr) { - return false - } - - var hostnameErr x509.HostnameError - if errors.As(err, &hostnameErr) { - return false - } - - var certInvalidErr x509.CertificateInvalidError - if errors.As(err, &certInvalidErr) { - return false - } - - // Covers dial timeouts, DNS failures and refused connections: http.Client wraps transport - // failures in *url.Error, which satisfies net.Error. var netErr net.Error if errors.As(err, &netErr) { return true } - // Connection torn down mid-flight, most often a load balancer recycling a keep-alive connection. - for _, errno := range []syscall.Errno{ - syscall.ECONNRESET, - syscall.ECONNREFUSED, - syscall.ECONNABORTED, - syscall.EPIPE, - syscall.EHOSTUNREACH, - syscall.ENETUNREACH, - syscall.ETIMEDOUT, - } { - if errors.Is(err, errno) { - return true - } - } - - // Keep-alive connection closed between our write and the server's response. + // A truncated response body surfaces as a bare io error, not a net.Error. return errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) } -// retryDelay returns how long to wait before the next attempt. Zero hands the decision back to -// resty's jittered exponential backoff, which is what we want whenever the server gave no guidance. -func retryDelay(res *resty.Response, policy RetryPolicy) time.Duration { +// retryDelay returns the server-requested wait; zero tells resty to use its jittered backoff. +// No cap needed: shouldRetryRequest declines waits beyond MaxDelay. +func retryDelay(res *resty.Response) time.Duration { if res == nil { return 0 } - wait, ok := parseRetryAfter(res.Header().Get("Retry-After")) - if !ok { - return 0 + if wait, ok := parseRetryAfter(res.Header().Get("Retry-After")); ok { + return wait } - // The server told us exactly how long to wait, so prefer it over our own guess. Still cap it: - // a misconfigured or hostile Retry-After shouldn't be able to park the CLI indefinitely. - if policy.MaxDelay > 0 && wait > policy.MaxDelay { - return policy.MaxDelay - } - - return wait + return 0 } -// parseRetryAfter reads a Retry-After header in either RFC 9110 form, a delay in seconds or an -// absolute HTTP date. ok is false when the header is absent, unparseable, or already in the past, -// leaving the caller on its default backoff. +const maxRetryAfter = 24 * time.Hour + func parseRetryAfter(value string) (time.Duration, bool) { value = strings.TrimSpace(value) if value == "" { @@ -324,31 +233,32 @@ func parseRetryAfter(value string) (time.Duration, bool) { } if seconds, err := strconv.Atoi(value); err == nil { - if seconds <= 0 { + switch { + case seconds <= 0: return 0, false + case seconds > int(maxRetryAfter/time.Second): + return maxRetryAfter, true } return time.Duration(seconds) * time.Second, true } if deadline, err := http.ParseTime(value); err == nil { if wait := time.Until(deadline); wait > 0 { - return wait, true + return min(wait, maxRetryAfter), true } } return 0, false } -// retryLogger records each retryable failure at debug level. Retries are routine on a flaky network -// and warning on every one would be noise for scripted use, so this stays behind --log-level debug. +// Debug level: warning on every retry would be noise for scripted use. func retryLogger(policy RetryPolicy) resty.OnRetryFunc { return func(res *resty.Response, err error) { event := log.Debug() exhausted := false - if res != nil && res.Request != nil { - // Attempt counts from 1, so the last one lands on MaxRetries+1. Resty runs retry hooks - // on that attempt too, and calling it a retry there would be a lie. + if res != nil { + // Resty runs retry hooks on the final attempt too. exhausted = res.Request.Attempt > policy.MaxRetries event = event. diff --git a/packages/util/retry_test.go b/packages/util/retry_test.go index 820e2e8a..1e8c6f94 100644 --- a/packages/util/retry_test.go +++ b/packages/util/retry_test.go @@ -10,6 +10,7 @@ import ( "net" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "strings" @@ -23,7 +24,6 @@ import ( "github.com/stretchr/testify/require" ) -// testPolicy keeps the delays negligible so the suite exercises retry decisions rather than backoff. func testPolicy(maxRetries int) RetryPolicy { return RetryPolicy{ MaxRetries: maxRetries, @@ -32,8 +32,7 @@ func testPolicy(maxRetries int) RetryPolicy { } } -// newTestClient returns a client on testPolicy plus a counter of attempts actually dispatched. -// Counting client-side rather than in the handler also covers failures that never reach a server. +// Attempts are counted client-side so failures that never reach a server still count. func newTestClient(t *testing.T, policy RetryPolicy) (*resty.Client, *atomic.Int32) { t.Helper() @@ -60,8 +59,6 @@ func TestRetryStatusCodes(t *testing.T) { {"503 service unavailable is retried", http.StatusServiceUnavailable, maxRetries + 1}, {"504 gateway timeout is retried", http.StatusGatewayTimeout, maxRetries + 1}, - // Permanent failures must surface on the first attempt. Retrying them multiplies the cost of - // a bad token or a typo and delays the error the user needs to see. {"400 bad request is not retried", http.StatusBadRequest, 1}, {"401 unauthorized is not retried", http.StatusUnauthorized, 1}, {"403 forbidden is not retried", http.StatusForbidden, 1}, @@ -88,9 +85,7 @@ func TestRetryStatusCodes(t *testing.T) { } } -// POST is not safely repeatable. A 502/503/504 can mean the server did process the write and only -// the response was lost, so replaying it risks double-applying, for instance minting a second -// dynamic secret lease. A 429 is safe because the server states it rejected the request outright. +// A 5xx on POST may have been committed server-side, so only 429 is safe to repeat there. func TestRetryMethodSafety(t *testing.T) { const maxRetries = 2 @@ -161,22 +156,13 @@ func TestRetryDisabledWhenMaxRetriesIsZero(t *testing.T) { } func TestRetryOnTransportError(t *testing.T) { - // Bind then release a port so the address is routable but nothing is listening, which is the - // connection-refused case the CLI hits when an instance is down. - listener, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(t, err) - deadURL := fmt.Sprintf("http://%s", listener.Addr().String()) - require.NoError(t, listener.Close()) - client, attempts := newTestClient(t, testPolicy(2)) - _, err = client.R().Get(deadURL) + _, err := client.R().Get(deadAddress(t)) require.Error(t, err) assert.Equal(t, int32(3), attempts.Load()) } -// A TLS trust failure is deterministic, so retrying it only delays the real error. It satisfies -// net.Error, which is why the policy rules certificate errors out explicitly. func TestNoRetryOnTLSTrustFailure(t *testing.T) { server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) @@ -220,7 +206,7 @@ func TestRetryHonorsRetryAfterHeader(t *testing.T) { })) defer server.Close() - // MaxDelay has to exceed Retry-After, otherwise the cap is what we would be measuring. + // MaxDelay must exceed the header value or the request would not be retried at all. policy := RetryPolicy{MaxRetries: 2, BaseDelay: time.Millisecond, MaxDelay: 5 * time.Second} client, _ := newTestClient(t, policy) @@ -247,6 +233,8 @@ func TestParseRetryAfter(t *testing.T) { {"zero seconds falls back to default backoff", "0", 0, false}, {"negative seconds falls back to default backoff", "-5", 0, false}, {"unparseable value falls back to default backoff", "soon", 0, false}, + // Uncapped, this multiplies into a negative Duration that would bypass the fail-fast. + {"overflowing seconds are capped", "9999999999", maxRetryAfter, true}, } for _, test := range tests { @@ -264,29 +252,41 @@ func TestParseRetryAfter(t *testing.T) { assert.InDelta(t, (30 * time.Second).Seconds(), got.Seconds(), 2) }) + t.Run("far future http date is capped", func(t *testing.T) { + got, ok := parseRetryAfter(time.Now().Add(100000 * time.Hour).UTC().Format(http.TimeFormat)) + require.True(t, ok) + assert.Equal(t, maxRetryAfter, got) + }) + t.Run("past http date falls back to default backoff", func(t *testing.T) { _, ok := parseRetryAfter(time.Now().Add(-time.Minute).UTC().Format(http.TimeFormat)) assert.False(t, ok) }) } -// A misconfigured or hostile Retry-After must not be able to park the CLI indefinitely. -func TestRetryDelayCapsRetryAfterAtMaxDelay(t *testing.T) { - policy := RetryPolicy{MaxRetries: 3, BaseDelay: time.Second, MaxDelay: 10 * time.Second} +func TestRetryDelayFallsBackWithoutHeader(t *testing.T) { + assert.Zero(t, retryDelay(nil), "a nil response should defer to resty's own backoff") res := &resty.Response{RawResponse: &http.Response{Header: http.Header{}}} - res.RawResponse.Header.Set("Retry-After", "3600") - - assert.Equal(t, policy.MaxDelay, retryDelay(res, policy)) + assert.Zero(t, retryDelay(res), "an absent header should defer to resty's own backoff") } -func TestRetryDelayFallsBackWithoutHeader(t *testing.T) { - policy := RetryPolicy{MaxRetries: 3, BaseDelay: time.Second, MaxDelay: 10 * time.Second} +func TestRetryAfterBeyondMaxDelayFailsFast(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "60") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + client, attempts := newTestClient(t, testPolicy(3)) - assert.Zero(t, retryDelay(nil, policy), "a nil response should defer to resty's own backoff") + start := time.Now() + res, err := client.R().Get(server.URL) - res := &resty.Response{RawResponse: &http.Response{Header: http.Header{}}} - assert.Zero(t, retryDelay(res, policy), "an absent header should defer to resty's own backoff") + require.NoError(t, err) + assert.Equal(t, http.StatusTooManyRequests, res.StatusCode()) + assert.Equal(t, int32(1), attempts.Load()) + assert.Less(t, time.Since(start), time.Second) } func TestIsRetryableTransportError(t *testing.T) { @@ -296,13 +296,12 @@ func TestIsRetryableTransportError(t *testing.T) { want bool }{ {"nil", nil, false}, + // Bare errnos satisfy net.Error, which is what makes an explicit errno list unnecessary. {"connection reset", syscall.ECONNRESET, true}, - {"connection refused", syscall.ECONNREFUSED, true}, {"broken pipe", syscall.EPIPE, true}, - {"host unreachable", syscall.EHOSTUNREACH, true}, - {"network unreachable", syscall.ENETUNREACH, true}, {"dns failure", &net.DNSError{Err: "no such host", Name: "app.infisical.com"}, true}, {"wrapped connection reset", fmt.Errorf("posting secret: %w", syscall.ECONNRESET), true}, + {"url-wrapped transport failure", &url.Error{Op: "Post", URL: "http://x", Err: &net.OpError{Op: "read", Err: syscall.ECONNRESET}}, true}, {"untrusted certificate authority", x509.UnknownAuthorityError{}, false}, {"certificate hostname mismatch", x509.HostnameError{Host: "app.infisical.com"}, false}, @@ -349,7 +348,6 @@ func TestRetryPolicyEnvOverrides(t *testing.T) { assert.Equal(t, agentRetryMaxDelay, policy.MaxDelay) }) - // A bad value in the environment should not stop a command that would otherwise work. t.Run("malformed values are ignored", func(t *testing.T) { t.Setenv(INFISICAL_RETRY_BASE_DELAY_NAME, "soon") t.Setenv(INFISICAL_RETRY_MAX_DELAY_NAME, "") @@ -360,6 +358,12 @@ func TestRetryPolicyEnvOverrides(t *testing.T) { assert.Equal(t, defaultRetryBaseDelay, policy.BaseDelay) }) + t.Run("best-effort policy ignores env overrides", func(t *testing.T) { + t.Setenv(INFISICAL_RETRY_MAX_RETRIES_NAME, "50") + + assert.Equal(t, 1, BestEffortRetryPolicy().MaxRetries) + }) + t.Run("base delay is clamped to max delay", func(t *testing.T) { t.Setenv(INFISICAL_RETRY_BASE_DELAY_NAME, "30s") t.Setenv(INFISICAL_RETRY_MAX_DELAY_NAME, "5s") @@ -371,27 +375,21 @@ func TestRetryPolicyEnvOverrides(t *testing.T) { }) } -// The constructors are only a single source of truth while nothing bypasses them, and a bypass is -// invisible in review: the client works, it just silently has no retries. func TestClientsAreBuiltThroughTheSharedConstructor(t *testing.T) { t.Setenv(INFISICAL_RETRY_MAX_RETRIES_NAME, "") client, err := GetRestyClientWithCustomHeaders() require.NoError(t, err) - assert.Equal(t, defaultRetryMaxRetries, client.RetryCount, - "GetRestyClientWithCustomHeaders must apply the default retry policy") + assert.Equal(t, defaultRetryMaxRetries, client.RetryCount) agentClient, err := GetRestyClientWithPolicy(AgentRetryPolicy()) require.NoError(t, err) assert.Equal(t, agentRetryMaxRetries, agentClient.RetryCount) } -// TestNoDirectRestyConstruction is the mechanism that keeps the retry policy single-sourced. A -// direct resty.New() compiles, runs, and looks correct in review; it just silently has no retries, -// which is exactly the bug this package exists to prevent. Add new clients via +// A direct resty.New() compiles and works but silently has no retry policy; add new clients via // GetRestyClientWithCustomHeaders or GetRestyClientWithPolicy instead. func TestNoDirectRestyConstruction(t *testing.T) { - // Files allowed to construct a client directly, relative to the repo root. allowed := map[string]bool{ filepath.Join("packages", "util", "common.go"): true, } @@ -408,7 +406,6 @@ func TestNoDirectRestyConstruction(t *testing.T) { if entry.IsDir() || !strings.HasSuffix(path, ".go") { return nil } - // Tests build throwaway clients on purpose and never talk to the real API. if strings.HasSuffix(path, "_test.go") { return nil } @@ -441,9 +438,8 @@ func TestNoDirectRestyConstruction(t *testing.T) { "use util.GetRestyClientWithCustomHeaders or util.GetRestyClientWithPolicy instead") } -// newAbruptCloseServer accepts connections, reads the request, then drops the connection without -// responding. That is the ambiguous mid-flight failure: the request was delivered, so the server may -// already have acted on it, and only the response was lost. +// newAbruptCloseServer simulates the ambiguous mid-flight failure: request delivered, connection +// dropped before any response. func newAbruptCloseServer(t *testing.T) (serverURL string, connections func() int32) { t.Helper() @@ -468,8 +464,7 @@ func newAbruptCloseServer(t *testing.T) (serverURL string, connections func() in return "http://" + listener.Addr().String(), count.Load } -// deadAddress returns an address that is routable but has nothing listening, so dialing it is -// refused at connection establishment and the request provably never reaches a server. +// deadAddress binds then releases a port, so dialing it is refused before anything is sent. func deadAddress(t *testing.T) string { t.Helper() @@ -481,9 +476,6 @@ func deadAddress(t *testing.T) string { return "http://" + addr } -// Transport failures have to respect method safety too, but only when they are ambiguous. Treating -// every transport error as unsafe for POST would give up the case this feature mainly exists for: -// retrying across a backend that is briefly down. func TestTransportErrorMethodSafety(t *testing.T) { const maxRetries = 2 @@ -494,8 +486,7 @@ func TestTransportErrorMethodSafety(t *testing.T) { _, err := client.R().SetBody(`{"lease":"request"}`).Post(serverURL) require.Error(t, err) - assert.Equal(t, int32(1), attempts.Load(), - "an ambiguous mid-flight failure must not replay a write: the server may have committed it") + assert.Equal(t, int32(1), attempts.Load()) }) t.Run("mid-flight failure does not replay PATCH", func(t *testing.T) { @@ -515,8 +506,7 @@ func TestTransportErrorMethodSafety(t *testing.T) { _, err := client.R().Get(serverURL) require.Error(t, err) - assert.Equal(t, int32(maxRetries+1), attempts.Load(), - "a read is idempotent, so an ambiguous failure is still safe to repeat") + assert.Equal(t, int32(maxRetries+1), attempts.Load()) }) t.Run("connection refused replays POST", func(t *testing.T) { @@ -524,8 +514,7 @@ func TestTransportErrorMethodSafety(t *testing.T) { _, err := client.R().SetBody(`{}`).Post(deadAddress(t)) require.Error(t, err) - assert.Equal(t, int32(maxRetries+1), attempts.Load(), - "the dial never completed, so the server cannot have seen the write") + assert.Equal(t, int32(maxRetries+1), attempts.Load()) }) t.Run("unresolvable host replays POST", func(t *testing.T) { @@ -533,8 +522,37 @@ func TestTransportErrorMethodSafety(t *testing.T) { _, err := client.R().SetBody(`{}`).Post("http://this-host-does-not-exist.invalid") require.Error(t, err) - assert.Equal(t, int32(maxRetries+1), attempts.Load(), - "resolution never produced an address, so nothing was sent") + assert.Equal(t, int32(maxRetries+1), attempts.Load()) + }) +} + +func TestReplaySafePolicy(t *testing.T) { + const maxRetries = 2 + + replaySafe := testPolicy(maxRetries) + replaySafe.ReplaySafe = true + + t.Run("mid-flight failure replays POST", func(t *testing.T) { + serverURL, _ := newAbruptCloseServer(t) + + client, attempts := newTestClient(t, replaySafe) + _, err := client.R().SetBody(`{"accessToken":"x"}`).Post(serverURL) + + require.Error(t, err) + assert.Equal(t, int32(maxRetries+1), attempts.Load()) + }) + + t.Run("503 on POST is retried", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + })) + defer server.Close() + + client, attempts := newTestClient(t, replaySafe) + _, err := client.R().SetBody(`{}`).Post(server.URL) + + require.NoError(t, err) + assert.Equal(t, int32(maxRetries+1), attempts.Load()) }) } @@ -544,15 +562,14 @@ func TestRequestNeverReachedServer(t *testing.T) { err error want bool }{ - {"connection refused", syscall.ECONNREFUSED, true}, - {"wrapped connection refused", fmt.Errorf("post: %w", syscall.ECONNREFUSED), true}, + // Refused connections and dial timeouts arrive as OpError{Op: "dial"}, never bare errnos. + {"refused connection", &net.OpError{Op: "dial", Err: &os.SyscallError{Syscall: "connect", Err: syscall.ECONNREFUSED}}, true}, + {"wrapped dial failure", fmt.Errorf("post: %w", &net.OpError{Op: "dial", Err: syscall.ETIMEDOUT}), true}, {"dns failure", &net.DNSError{Err: "no such host", Name: "app.infisical.com"}, true}, - {"dial failure", &net.OpError{Op: "dial", Err: syscall.ETIMEDOUT}, true}, - // Delivered, then the connection died. The server may already have committed the write. {"read failure mid-flight", &net.OpError{Op: "read", Err: syscall.ECONNRESET}, false}, {"write failure mid-flight", &net.OpError{Op: "write", Err: syscall.EPIPE}, false}, - {"bare connection reset", syscall.ECONNRESET, false}, + {"bare errno lacks dial context", syscall.ECONNREFUSED, false}, {"unexpected eof", io.ErrUnexpectedEOF, false}, {"eof", io.EOF, false}, } From be1cbe6f67c40a1098480ed17ccf8d4b28f3e3fc Mon Sep 17 00:00:00 2001 From: Scott Wilson Date: Fri, 28 Aug 2026 12:04:45 -0700 Subject: [PATCH 4/5] improvement: resty debug logging and remove safe reply from ua login --- packages/util/helper.go | 5 +---- packages/util/retry.go | 20 ++++++++++++++++++++ packages/util/retry_test.go | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/packages/util/helper.go b/packages/util/helper.go index 7d3a6cc6..867d64c9 100644 --- a/packages/util/helper.go +++ b/packages/util/helper.go @@ -329,10 +329,7 @@ func GetInfisicalToken(cmd *cobra.Command) (token *models.TokenDetails, err erro } func UniversalAuthLogin(clientId string, clientSecret string) (api.UniversalAuthLoginResponse, error) { - policy := DefaultRetryPolicy() - policy.ReplaySafe = true // a replayed login only mints another TTL-bound token - - httpClient, err := GetRestyClientWithPolicy(policy) + httpClient, err := GetRestyClientWithCustomHeaders() if err != nil { return api.UniversalAuthLoginResponse{}, err } diff --git a/packages/util/retry.go b/packages/util/retry.go index cd842fbc..f05f3bde 100644 --- a/packages/util/retry.go +++ b/packages/util/retry.go @@ -107,6 +107,8 @@ func (p RetryPolicy) withEnvOverrides() RetryPolicy { } func applyRetryPolicy(httpClient *resty.Client, policy RetryPolicy) *resty.Client { + httpClient.SetLogger(restyLogAdapter{}) + if policy.MaxRetries <= 0 { return httpClient } @@ -251,6 +253,24 @@ func parseRetryAfter(value string) (time.Duration, bool) { return 0, false } +// restyLogAdapter feeds resty's internal logging through zerolog instead of resty's default +// unstructured stderr logger. Every severity maps to debug: on the request path resty warns per +// failed attempt and errors once retries are exhausted, both of which retryLogger and the error +// returned to the caller already cover. +type restyLogAdapter struct{} + +func (restyLogAdapter) Errorf(format string, v ...any) { + log.Debug().Str("component", "resty").Msgf(format, v...) +} + +func (restyLogAdapter) Warnf(format string, v ...any) { + log.Debug().Str("component", "resty").Msgf(format, v...) +} + +func (restyLogAdapter) Debugf(format string, v ...any) { + log.Debug().Str("component", "resty").Msgf(format, v...) +} + // Debug level: warning on every retry would be noise for scripted use. func retryLogger(policy RetryPolicy) resty.OnRetryFunc { return func(res *resty.Response, err error) { diff --git a/packages/util/retry_test.go b/packages/util/retry_test.go index 1e8c6f94..fdb59aea 100644 --- a/packages/util/retry_test.go +++ b/packages/util/retry_test.go @@ -1,6 +1,7 @@ package util import ( + "bytes" "context" "crypto/x509" "errors" @@ -20,6 +21,8 @@ import ( "time" "github.com/go-resty/resty/v2" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -438,6 +441,38 @@ func TestNoDirectRestyConstruction(t *testing.T) { "use util.GetRestyClientWithCustomHeaders or util.GetRestyClientWithPolicy instead") } +// Resty's default logger writes unstructured lines straight to stderr on each failed attempt and +// on final failure; applyRetryPolicy must route those through zerolog instead. +func TestRestyInternalLoggingIsRoutedThroughZerolog(t *testing.T) { + stderrReader, stderrWriter, err := os.Pipe() + require.NoError(t, err) + + // Resty binds os.Stderr into its default logger at construction, so the swap must happen + // before the client is built to catch anything bypassing the adapter. + originalStderr := os.Stderr + os.Stderr = stderrWriter + t.Cleanup(func() { os.Stderr = originalStderr }) + + var structured bytes.Buffer + originalLogger := log.Logger + log.Logger = zerolog.New(&structured) + t.Cleanup(func() { log.Logger = originalLogger }) + + client, _ := newTestClient(t, testPolicy(1)) + _, err = client.R().Get(deadAddress(t)) + require.Error(t, err) + + os.Stderr = originalStderr + require.NoError(t, stderrWriter.Close()) + captured, err := io.ReadAll(stderrReader) + require.NoError(t, err) + + assert.NotContains(t, string(captured), "RESTY", + "resty wrote its own unstructured log lines instead of going through zerolog") + assert.Contains(t, structured.String(), `"component":"resty"`, + "resty's internal messages should surface as structured debug events tagged with their source") +} + // newAbruptCloseServer simulates the ambiguous mid-flight failure: request delivered, connection // dropped before any response. func newAbruptCloseServer(t *testing.T) (serverURL string, connections func() int32) { From a86bd3db1fd3ab679a580f5fe2e71ca868ce8884 Mon Sep 17 00:00:00 2001 From: Scott Wilson Date: Fri, 28 Aug 2026 12:07:27 -0700 Subject: [PATCH 5/5] chore: make comment more succinct --- packages/util/retry.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/util/retry.go b/packages/util/retry.go index f05f3bde..fe5d6b30 100644 --- a/packages/util/retry.go +++ b/packages/util/retry.go @@ -253,10 +253,8 @@ func parseRetryAfter(value string) (time.Duration, bool) { return 0, false } -// restyLogAdapter feeds resty's internal logging through zerolog instead of resty's default -// unstructured stderr logger. Every severity maps to debug: on the request path resty warns per -// failed attempt and errors once retries are exhausted, both of which retryLogger and the error -// returned to the caller already cover. +// restyLogAdapter routes resty's internal logging through zerolog. Everything maps to debug: +// resty's request-path warnings and errors duplicate retryLogger and the returned error. type restyLogAdapter struct{} func (restyLogAdapter) Errorf(format string, v ...any) {