diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 5792e62..3c2f1b8 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -19,6 +19,7 @@ internal/ httpapi/ authenticated retrying HTTP transport logging/ opt-in diagnostic logging abstraction output/ terminal and JSON/JSONL rendering + retry/ shared bounded retry and backoff policy search/ WebDAV search-files REPORT client and response mapping sharing/ OCS share discovery, public-link, and capability client sync/ deterministic one-way and bidirectional planning model @@ -80,10 +81,14 @@ without starting a subprocess. public-link permission facets; and mutate resource tags by stable resource ID. Identity and authorization policy remain on the server. - `internal/httpapi`: send replayable authenticated API requests with bounded - retries for non-WebDAV protocols. + retries for non-WebDAV protocols, using the shared `internal/retry` policy. - `internal/logging`: provide an injected no-op or text diagnostic logger. - `internal/output`: render human-readable output and versioned JSON/JSONL envelopes through injected writers. +- `internal/retry`: decide which responses may be retried and how long to wait, + honoring a server-requested delay exactly within one ceiling and refusing a + longer one, so no response can suspend an operation indefinitely and no retry + arrives before a throttling server allows it. - `internal/search`: issue bounded `search-files` REPORT requests and decode ranked WebDAV multistatus responses without depending on Cobra or Space selection policy. @@ -209,6 +214,13 @@ Fast package tests remain Docker-independent. planner; job configuration never implements a second transfer path. - An unavailable persisted Space is cleared and reported; commands never silently fall back to personal files. +- Retry policy lives only in `internal/retry`. Every wait between attempts, + whether requested by a server through `Retry-After` or produced by + exponential backoff, is bounded by one ceiling. A response never controls how + long the CLI pauses, because no request timeout applies while it waits. A + server-requested delay is honored exactly or refused, never shortened: an + exponential delay is the CLI's own choice and may be clamped, but retrying + before a throttling server allows can extend a rate-limit ban. - Cancellation propagates through Cobra contexts, application use cases, HTTP requests, and transfer workers and maps to exit code 130. - New behavior requires tests at its narrowest package boundary. diff --git a/Makefile b/Makefile index 868d689..fcb618c 100644 --- a/Makefile +++ b/Makefile @@ -38,7 +38,8 @@ check: fmt coverage: go run ./tools/covercheck -min $(COVERAGE_MIN) \ - app auth graph httpapi search sharing sync trash transfer versions webdav + app auth graph httpapi retry search sharing sync trash transfer versions \ + webdav fmt: gofmt -w . diff --git a/README.md b/README.md index 4f6c9d9..d497027 100644 --- a/README.md +++ b/README.md @@ -629,8 +629,13 @@ ocis download /notes/hello.txt - > hello.txt ``` Temporary network errors, HTTP `429`, and HTTP `5xx` responses are retried with -bounded exponential backoff. Global reliability controls are available on -every command: +bounded exponential backoff, never waiting longer than 30 seconds between +attempts. A server may ask for a specific delay with `Retry-After`; the CLI +honors it exactly when it is within that limit. A longer delay stops the command +with an error naming the requested wait, because retrying sooner than a +throttling server allows can extend a rate-limit ban — run the command again +later. Either way no response can stall a command indefinitely. Global +reliability controls are available on every command: ```sh ocis --timeout 2m --retries 5 --concurrency 8 upload --recursive ./photos /photos diff --git a/internal/httpapi/client.go b/internal/httpapi/client.go index dfcb311..289c07b 100644 --- a/internal/httpapi/client.go +++ b/internal/httpapi/client.go @@ -13,6 +13,7 @@ import ( "time" "github.com/mzner/ocis-cli/internal/logging" + "github.com/mzner/ocis-cli/internal/retry" ) // Config contains shared authentication and retry settings. @@ -89,7 +90,7 @@ func (client *Client) Do( } client.authenticate(request) response, err := client.http.Do(request) - if err == nil && (!retryableStatus(response.StatusCode) || attempt >= client.config.Retries) { + if err == nil && (!retry.RetryableStatus(response.StatusCode) || attempt >= client.config.Retries) { return response, nil } if err != nil && attempt >= client.config.Retries { @@ -97,14 +98,14 @@ func (client *Client) Do( } delay, reason := time.Duration(0), "transport_error" if response != nil { - delay, reason = retryAfter(response), response.Status + delay, reason = retry.After(response), response.Status _ = response.Body.Close() } client.config.Logger.Debug( "retrying API request", "method", method, "attempt", attempt+2, "reason", reason, ) - if err := waitRetry(ctx, client.config.RetryWait, attempt, delay); err != nil { + if err := retry.Wait(ctx, client.config.RetryWait, attempt, delay); err != nil { return nil, err } } @@ -156,34 +157,3 @@ func (client *Client) authenticate(request *http.Request) { request.Header.Set("User-Agent", client.config.UserAgent) } } - -func retryableStatus(status int) bool { - return status == http.StatusTooManyRequests || status >= 500 -} - -func retryAfter(response *http.Response) time.Duration { - value := response.Header.Get("Retry-After") - if seconds, err := time.ParseDuration(value + "s"); err == nil && seconds > 0 { - return seconds - } - if when, err := http.ParseTime(value); err == nil { - return max(time.Until(when), 0) - } - return 0 -} - -func waitRetry( - ctx context.Context, base time.Duration, attempt int, delay time.Duration, -) error { - if delay <= 0 { - delay = base * time.Duration(1< retry.MaxDelay { + t.Fatalf( + "elapsed: got %v, want the one-second Retry-After honored within %v", + elapsed, retry.MaxDelay, + ) + } +} + func TestRetryHonorsContext(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func( writer http.ResponseWriter, _ *http.Request, diff --git a/internal/retry/retry.go b/internal/retry/retry.go new file mode 100644 index 0000000..1374fc1 --- /dev/null +++ b/internal/retry/retry.go @@ -0,0 +1,136 @@ +// Package retry provides the shared bounded-retry policy used by every +// protocol adapter: which responses may be retried, and how long to wait. +package retry + +import ( + "context" + "errors" + "fmt" + "math" + "net/http" + "strconv" + "strings" + "time" +) + +// MaxDelay bounds every wait between attempts. A retry delay is derived from +// values the server controls, so an unbounded delay would let a hostile or +// misconfigured server suspend the CLI for an arbitrary time; the request +// timeout cannot recover from it because no request is in flight while waiting. +// +// Waiting longer than this is indistinguishable from a hang. Retrying sooner is +// not the alternative: a server that asks for a long delay is usually +// throttling, and a follow-up request before the delay expires can worsen the +// throttling or extend a rate-limit ban. A server-requested delay beyond this +// ceiling therefore stops the operation, while a delay the CLI chose for itself +// is simply clamped. +const MaxDelay = 30 * time.Second + +// DelayTooLongError reports that the server asked the CLI to wait longer than it +// is willing to. It names both durations so the caller can decide whether to run +// the command again later. +type DelayTooLongError struct { + Requested time.Duration + Ceiling time.Duration +} + +func (err *DelayTooLongError) Error() string { + return fmt.Sprintf( + "server asked to retry after %v, which exceeds the %v limit; "+ + "run the command again later", + err.Requested, err.Ceiling, + ) +} + +// RetryableStatus reports whether a response status may be retried. Only +// throttling and server-side failures qualify; a client error is a decision +// the server will repeat. +func RetryableStatus(status int) bool { + return status == http.StatusTooManyRequests || status >= 500 +} + +// After returns the delay requested by a response's Retry-After header. It +// returns zero when the header is absent, unparsable, or already elapsed, +// leaving the caller's backoff to choose the delay. A value beyond MaxDelay is +// reported as-is rather than reduced, so that Delay can refuse it and say what +// was asked for; only the saturation needed to stay inside time.Duration is +// applied. +func After(response *http.Response) time.Duration { + if response == nil { + return 0 + } + // RFC 9110 defines exactly two forms: delta-seconds and an HTTP-date. + value := strings.TrimSpace(response.Header.Get("Retry-After")) + if seconds, ok := deltaSeconds(value); ok { + switch { + case seconds <= 0: + return 0 + // Compared in seconds because converting first would overflow + // time.Duration and wrap to a negative delay. + case seconds >= int64(math.MaxInt64/time.Second): + return math.MaxInt64 + default: + return time.Duration(seconds) * time.Second + } + } + if when, err := http.ParseTime(value); err == nil { + return max(time.Until(when), 0) + } + return 0 +} + +// deltaSeconds decodes the delta-seconds form. A value too large for int64 +// saturates instead of being rejected, so that an absurd hint is capped by the +// caller rather than silently treated as "no delay requested". +func deltaSeconds(value string) (int64, bool) { + seconds, err := strconv.ParseInt(value, 10, 64) + switch { + case err == nil: + return seconds, true + case errors.Is(err, strconv.ErrRange): + return seconds, true + default: + return 0, false + } +} + +// Delay returns how long to wait before the next attempt. A positive +// server-requested delay wins and is honored exactly, or refused with +// *DelayTooLongError when it exceeds MaxDelay; shortening it would send a +// follow-up request the server asked us not to send. Otherwise the base wait is +// doubled per attempt up to a fixed number of doublings and clamped at MaxDelay. +func Delay( + base time.Duration, attempt int, serverDelay time.Duration, +) (time.Duration, error) { + if serverDelay > 0 { + if serverDelay > MaxDelay { + return 0, &DelayTooLongError{Requested: serverDelay, Ceiling: MaxDelay} + } + return serverDelay, nil + } + return min(base*time.Duration(1< 5*time.Second { + t.Fatalf("delay: got %v, want a positive value near 4s", got) + } +} + +// TestAfterReportsAnExcessiveDelayWithoutSaturating checks that a delay beyond +// anything worth waiting for is still reported as the large value it is, so the +// policy can refuse it and name it, rather than being silently rounded down to +// the ceiling or wrapping to a negative duration. +func TestAfterReportsAnExcessiveDelayWithoutSaturating(t *testing.T) { + values := []string{ + "86400", + "999999999999999999999", + time.Now().Add(72 * time.Hour).UTC().Format(http.TimeFormat), + } + for _, value := range values { + if got := retry.After(responseWith(value)); got <= retry.MaxDelay { + t.Fatalf( + "Retry-After %q: got %v, want a value above the %v ceiling", + value, got, retry.MaxDelay, + ) + } + } +} + +// TestDelayRefusesToRetryBeforeTheServerAllows covers the case where honoring +// Retry-After would exceed the local ceiling. Waiting that long is +// indistinguishable from a hang, and retrying sooner contradicts legitimate +// throttling guidance and can extend a rate-limit ban, so the operation stops +// with both durations named. +func TestDelayRefusesToRetryBeforeTheServerAllows(t *testing.T) { + _, err := retry.Delay(time.Millisecond, 0, 24*time.Hour) + if err == nil { + t.Fatal("expected an excessive Retry-After to stop the operation") + } + var excessive *retry.DelayTooLongError + if !errors.As(err, &excessive) { + t.Fatalf("error type: got %T, want *retry.DelayTooLongError", err) + } + if excessive.Requested != 24*time.Hour || excessive.Ceiling != retry.MaxDelay { + t.Fatalf("durations: got %v and %v", excessive.Requested, excessive.Ceiling) + } + if !strings.Contains(err.Error(), "24h0m0s") || + !strings.Contains(err.Error(), retry.MaxDelay.String()) { + t.Fatalf("message must name both durations: %v", err) + } +} + +func TestDelayHonorsAServerDelayWithinTheCeiling(t *testing.T) { + got, err := retry.Delay(time.Millisecond, 0, retry.MaxDelay) + if err != nil || got != retry.MaxDelay { + t.Fatalf("delay: got %v, %v; want the ceiling honored", got, err) + } +} + +func TestWaitRefusesAnExcessiveServerDelayPromptly(t *testing.T) { + started := time.Now() + err := retry.Wait(context.Background(), time.Millisecond, 0, time.Hour) + if err == nil { + t.Fatal("expected an excessive Retry-After to stop the operation") + } + if elapsed := time.Since(started); elapsed > time.Second { + t.Fatalf("elapsed: got %v, want a prompt refusal", elapsed) + } +} + +func TestAfterIgnoresUnusableValues(t *testing.T) { + for _, value := range []string{"", "soon", "-5", "0", "5m3", "1h", "1.5"} { + if got := retry.After(responseWith(value)); got != 0 { + t.Fatalf("Retry-After %q: got %v, want 0", value, got) + } + } +} + +func TestAfterToleratesSurroundingWhitespace(t *testing.T) { + if got := retry.After(responseWith(" 2 ")); got != 2*time.Second { + t.Fatalf("delay: got %v, want 2s", got) + } +} + +func TestDelayGrowsExponentiallyFromTheBase(t *testing.T) { + base := 200 * time.Millisecond + for attempt, want := range []time.Duration{ + 200 * time.Millisecond, + 400 * time.Millisecond, + 800 * time.Millisecond, + 1600 * time.Millisecond, + 3200 * time.Millisecond, + 3200 * time.Millisecond, + } { + got, err := retry.Delay(base, attempt, 0) + if err != nil || got != want { + t.Fatalf("Delay(attempt %d): got %v, %v; want %v", attempt, got, err, want) + } + } +} + +func TestDelayPrefersServerDelay(t *testing.T) { + got, err := retry.Delay(time.Millisecond, 0, 7*time.Second) + if err != nil || got != 7*time.Second { + t.Fatalf("delay: got %v, %v; want 7s", got, err) + } +} + +// TestDelayCapsLocalBackoff covers the ceiling for a delay the CLI chose +// itself. Unlike a server-requested delay, shortening it contradicts nothing, +// so it is clamped rather than refused. +func TestDelayCapsLocalBackoff(t *testing.T) { + got, err := retry.Delay(time.Hour, 3, 0) + if err != nil || got != retry.MaxDelay { + t.Fatalf("backoff delay: got %v, %v; want the %v ceiling", got, err, retry.MaxDelay) + } +} + +func TestWaitSleepsTheComputedDelay(t *testing.T) { + started := time.Now() + if err := retry.Wait(context.Background(), 10*time.Millisecond, 2, 0); err != nil { + t.Fatal(err) + } + if elapsed := time.Since(started); elapsed < 40*time.Millisecond { + t.Fatalf("elapsed: got %v, want at least 40ms", elapsed) + } +} + +func TestWaitReturnsContextError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if err := retry.Wait( + ctx, time.Hour, 0, time.Hour, + ); !errors.Is(err, context.Canceled) { + t.Fatalf("got %v, want the canceled context error", err) + } +} + +func responseWith(retryAfter string) *http.Response { + header := http.Header{} + if retryAfter != "" { + header.Set("Retry-After", retryAfter) + } + return &http.Response{Header: header} +} diff --git a/internal/webdav/client.go b/internal/webdav/client.go index ea3a16a..bd82647 100644 --- a/internal/webdav/client.go +++ b/internal/webdav/client.go @@ -18,6 +18,7 @@ import ( "time" "github.com/mzner/ocis-cli/internal/logging" + "github.com/mzner/ocis-cli/internal/retry" "github.com/mzner/ocis-cli/internal/transfer" ) @@ -404,8 +405,8 @@ func (client *Client) DownloadWithOptions(ctx context.Context, remote, local str } return err } - if retryableStatus(response.StatusCode) && attempt < client.config.Retries { - delay := retryAfter(response) + if retry.RetryableStatus(response.StatusCode) && attempt < client.config.Retries { + delay := retry.After(response) _ = response.Body.Close() if err := client.waitRetry(ctx, attempt, delay); err != nil { return err @@ -701,7 +702,7 @@ func (client *Client) doWithRetry(ctx context.Context, build func() (*http.Reque return nil, err } response, err := client.http.Do(request) - if err == nil && (!retryableStatus(response.StatusCode) || attempt >= client.config.Retries) { + if err == nil && (!retry.RetryableStatus(response.StatusCode) || attempt >= client.config.Retries) { return response, nil } if err != nil && attempt >= client.config.Retries { @@ -710,7 +711,7 @@ func (client *Client) doWithRetry(ctx context.Context, build func() (*http.Reque delay := time.Duration(0) status := "transport_error" if response != nil { - delay = retryAfter(response) + delay = retry.After(response) status = response.Status _ = response.Body.Close() } @@ -724,33 +725,10 @@ func (client *Client) doWithRetry(ctx context.Context, build func() (*http.Reque } } +// waitRetry pauses before the next attempt using the configured base wait and +// the shared bounded-delay policy. func (client *Client) waitRetry(ctx context.Context, attempt int, delay time.Duration) error { - if delay <= 0 { - delay = client.config.RetryWait * time.Duration(1<= 500 -} - -func retryAfter(response *http.Response) time.Duration { - value := response.Header.Get("Retry-After") - if seconds, err := time.ParseDuration(value + "s"); err == nil && seconds > 0 { - return seconds - } - if when, err := http.ParseTime(value); err == nil { - return max(time.Until(when), 0) - } - return 0 + return retry.Wait(ctx, client.config.RetryWait, attempt, delay) } func parseContentRange(value string) (start, size int64, ok bool) { diff --git a/internal/webdav/client_test.go b/internal/webdav/client_test.go index a8d2d7f..c6c1f5b 100644 --- a/internal/webdav/client_test.go +++ b/internal/webdav/client_test.go @@ -16,6 +16,7 @@ import ( "time" "github.com/mzner/ocis-cli/internal/logging" + "github.com/mzner/ocis-cli/internal/retry" ) func TestClientRetriesTemporaryResponse(t *testing.T) { @@ -46,6 +47,71 @@ func TestClientRetriesTemporaryResponse(t *testing.T) { } } +// TestClientRetryAppliesBoundedServerRequestedDelay proves the WebDAV retry +// loop routes Retry-After through the shared bounded policy: the header is +// honored, and the wait cannot exceed the ceiling regardless of the value sent. +// internal/retry covers clamping of an excessive value directly, because +// observing the full ceiling here would mean a test that sleeps for it. +func TestClientRetryAppliesBoundedServerRequestedDelay(t *testing.T) { + var attempts atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + if attempts.Add(1) == 1 { + writer.Header().Set("Retry-After", "1") + writer.WriteHeader(http.StatusTooManyRequests) + return + } + writeDAVFile(writer, request.URL.Path, 4) + })) + defer server.Close() + client := NewClient(Config{ + Server: server.URL, Username: "alice", Retries: 1, + RetryWait: time.Millisecond, + }, server.Client()) + started := time.Now() + if _, err := client.Stat(context.Background(), "/report.txt"); err != nil { + t.Fatal(err) + } + elapsed := time.Since(started) + if elapsed < time.Second || elapsed > retry.MaxDelay { + t.Fatalf( + "elapsed: got %v, want the one-second Retry-After honored within %v", + elapsed, retry.MaxDelay, + ) + } + if got := attempts.Load(); got != 2 { + t.Fatalf("attempts: got %d, want 2", got) + } +} + +// TestClientStopsWhenRetryAfterExceedsTheCeiling proves the WebDAV retry loop +// neither waits out an excessive Retry-After nor retries before it expires: the +// throttled endpoint must receive no follow-up request. +func TestClientStopsWhenRetryAfterExceedsTheCeiling(t *testing.T) { + var attempts atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) { + attempts.Add(1) + writer.Header().Set("Retry-After", "86400") + writer.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + client := NewClient(Config{ + Server: server.URL, Username: "alice", Retries: 3, + RetryWait: time.Millisecond, + }, server.Client()) + started := time.Now() + _, err := client.Stat(context.Background(), "/report.txt") + var excessive *retry.DelayTooLongError + if !errors.As(err, &excessive) { + t.Fatalf("error: got %v, want a refused retry delay", err) + } + if elapsed := time.Since(started); elapsed > retry.MaxDelay { + t.Fatalf("elapsed: got %v, want a prompt refusal", elapsed) + } + if got := attempts.Load(); got != 1 { + t.Fatalf("attempts: got %d, want no follow-up request", got) + } +} + func TestClientRetryHonorsContext(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) { writer.WriteHeader(http.StatusServiceUnavailable) diff --git a/internal/webdav/tus.go b/internal/webdav/tus.go index b9a684e..866b06f 100644 --- a/internal/webdav/tus.go +++ b/internal/webdav/tus.go @@ -18,6 +18,8 @@ import ( "time" "github.com/bdragon300/tusgo" + + "github.com/mzner/ocis-cli/internal/retry" ) const maxTUSResponseBody = 4096 @@ -149,7 +151,7 @@ func (client *Client) uploadTUS( } delay := time.Duration(0) if stream.LastResponse != nil { - delay = retryAfter(stream.LastResponse) + delay = retry.After(stream.LastResponse) } if err := client.waitRetry(ctx, attempt, delay); err != nil { return err @@ -401,7 +403,7 @@ func retryableTUSError(err error, response *http.Response) bool { if errors.As(err, &networkErr) { return true } - return response != nil && retryableStatus(response.StatusCode) + return response != nil && retry.RetryableStatus(response.StatusCode) } func (client *Client) wrapTUSError(