Honor Retry-After as a floor under the backoff (#189)

CHANGELOG.md

@@ -82,6 +82,19 @@ policy in [docs/versioning.md](docs/versioning.md).
 
 ### Added
 
+- **`Retry-After` is honored** (#189). When a retried response carries the
+  header, that delay becomes a floor under the backoff for that attempt: both
+  RFC 9110 §10.2.3 forms are read, delta-seconds and HTTP-date, with a date
+  already past asking for nothing and a malformed value ignored in favor of
+  ordinary backoff. Before this the retry loop was pure full-jitter
+  exponential, so a 429 saying `Retry-After: 30` was retried inside
+  `max_backoff`'s 20-second window, arriving early to be refused again. It is
+  a floor and never a ceiling, so `Retry-After: 0` cannot shorten the backoff
+  that exists to prevent exactly that. `RetryPolicy::retry_after_cap`
+  (default 60 s) bounds how far a peer's number is trusted, separate from
+  `max_backoff` because that one bounds a guess this client made rather than
+  a number it was sent. `opal::RetryAfterDelay` is public for callers driving
+  their own retry loop.
 - **A response body sink: `HttpClient::SendStreaming`** (#213, slice 1 of
   `@streaming` blob support). A caller that does not want a response body
   buffered passes an `opal::http::BodySink` — an `accept(status, headers)`

docs/production-guide.md

@@ -119,15 +119,44 @@ hit by a synchronized thundering herd.
 config.retry.max_attempts = 3;                              // total tries; 1 disables retries
 config.retry.initial_backoff = std::chrono::milliseconds(100);
 config.retry.max_backoff = std::chrono::milliseconds(20000);
+config.retry.retry_after_cap = std::chrono::milliseconds(60000);
 ```
 
+### Retry-After
+
+When a retried response carries `Retry-After`, that delay becomes a **floor**
+under the backoff for that attempt (issue #189). Both RFC 9110 §10.2.3 forms
+are read: delta-seconds (`30`) and an HTTP-date
+(`Fri, 31 Dec 1999 23:59:59 GMT`), the second measured against the wall clock,
+with a date already past asking for nothing. A header that does not parse is
+ignored and ordinary backoff applies; a peer's malformed hint is not worth
+failing a call over.
+
+Floor, never ceiling. The server can ask this client to wait longer, never to
+come back sooner, so a `Retry-After: 0` does not shorten the backoff that
+exists to stop exactly that.
+
+`retry_after_cap` (default 60 s) bounds how far a number the peer sent is
+trusted, and is deliberately separate from `max_backoff`, which bounds a guess
+this client made. It defaults higher for a reason: a service documenting
+`Retry-After: 30` wants 30 seconds, and truncating that to `max_backoff` would
+spend an attempt arriving early to be refused again. Lower it when a slow retry
+is worse for you than a failed one.
+
+Total wall-clock across attempts is still `max_attempts` multiplied by what
+each sleep and timeout allow, with no ceiling of its own — an overall deadline
+is the remaining piece of #189.
+
 Guidance:
 
 - **Interactive paths:** keep `max_attempts` low (2–3) and cap
   `max_backoff` near your latency budget; a user-facing call gains nothing
   from a 20-second sleep.
 - **Batch/background paths:** raise `max_attempts` and let `max_backoff`
-  breathe; throttling (429) resolves on its own if you back off.
+  breathe; throttling (429) resolves on its own if you back off. Against an
+  API that documents its throttling (a published `Retry-After`), leave
+  `retry_after_cap` above the largest value the service advertises, or the
+  client will keep arriving early.
 - **Idempotency:** retries resend the same serialized request.
   `@idempotencyToken` members are generated once per call and reused across
   attempts, so the server can deduplicate. For non-idempotent operations

docs/research/client-third-party-api-gaps.md

@@ -16,7 +16,7 @@ side: ADR-0005 (dep-light transports), ADR-0007 (Beast + BoringSSL TLS),
 | 1 | Boost-free TLS | **A.** `//runtime:http_tls` — socket + BoringSSL (already a direct dep) | New transport target; keep Beast for server/WS |
 | 2 | Proxy + CA add | Env + explicit proxy on that transport; **`ca_pem` append mode** | CONNECT is the hard part; CA fix is small |
 | 3 | Response decompress | Client-layer gunzip via existing `GzipDecompress` + `accept_encoding` | Plumbing only; no new deps |
-| 4 | `Retry-After` | Floor under delay with separate `retry_after_cap` | Local `retry.cc` change |
+| 4 | `Retry-After` | **Done** — floor under delay with separate `retry_after_cap` | Local `retry.cc` change |
 | 5 | First-class 304 | Document `@httpResponseCode` now; optional `http_status` on `Error` later | Mostly model/docs; small Error API |
 | 6 | Streaming bodies | Defer for unary JSON; sink/spill later for blobs | Large if full codegen streaming |
 | 7 | Overall deadline | `ClientConfig::overall_timeout_ms` in `SendWithRetries` | Small config + retry loop |

runtime/include/opal/client/retry.h

@@ -4,10 +4,13 @@
 #include <chrono>
 #include <functional>
 #include <memory>
+#include <optional>
 #include <vector>
 
 #include "opal/client/interceptor.h"
 #include "opal/core/outcome.h"
+#include "opal/core/timestamp.h"
+#include "opal/http/headers.h"
 #include "opal/http/message.h"
 #include "opal/http/transport.h"
 
@@ -22,6 +25,17 @@ struct RetryPolicy {
   std::chrono::milliseconds initial_backoff{100};
   std::chrono::milliseconds max_backoff{20000};
 
+  // Ceiling on a delay the *server* asked for via Retry-After (issue #189).
+  // Separate from max_backoff because the two bound different things:
+  // max_backoff bounds a guess this client made, and this bounds how far it
+  // will trust a number the peer sent. It is the larger of the two by
+  // default because the header exists to ask for longer than a client would
+  // choose — a service documenting "Retry-After: 30" wants 30, and truncating
+  // that to max_backoff would spend an attempt arriving early to be refused
+  // again. Total wall-clock across attempts is still unbounded here; that is
+  // issue #189 item 7.
+  std::chrono::milliseconds retry_after_cap{60000};
+
   // Overrides for tests; null means a real sleep / a thread-local uniform [0,1).
   std::function<void(std::chrono::milliseconds)> sleep;
   std::function<double()> jitter;
@@ -34,6 +48,20 @@ std::chrono::milliseconds RetryDelay(const RetryPolicy& policy, int retry, doubl
 // 429 (throttling) and 500/502/503/504.
 bool RetryableStatus(int status);
 
+// The delay a response's Retry-After header asks for, or nullopt when the
+// header is absent or does not parse. RFC 9110 §10.2.3 gives it two forms:
+// delta-seconds ("30") and an HTTP-date ("Fri, 31 Dec 1999 23:59:59 GMT"),
+// the second measured against `now`. A date already past asks for nothing,
+// so it reads as zero rather than as negative.
+//
+// A header that does not parse is ignored rather than fatal: a peer's
+// malformed hint is not worth failing a call over, and ordinary backoff is
+// a safe answer. SendWithRetries takes the result as a *floor* under its own
+// backoff, clamped to RetryPolicy::retry_after_cap — the server can ask this
+// client to wait longer, never to come back sooner.
+std::optional<std::chrono::milliseconds> RetryAfterDelay(const http::Headers& headers,
+                                                         Timestamp now);
+
 // Sends through the transport with retries: transport failures flagged
 // retryable (connection, timeout) and transient response statuses are
 // retried up to policy.max_attempts, sleeping the backoff in between.

runtime/include/opal/http/headers.h

@@ -7,6 +7,8 @@
 #include <utility>
 #include <vector>
 
+#include "opal/core/timestamp.h"
+
 namespace opal::http {
 
 // HTTP header collection: case-insensitive names, repeated names preserved in
@@ -52,6 +54,28 @@ std::vector<std::string> SplitHeaderListValues(std::string_view value);
 // "application/json").
 std::string MediaTypeOf(std::string_view content_type);
 
+// An HTTP field timestamp, in any of the three formats RFC 9110 §5.6.7
+// requires a recipient to accept:
+//
+//   Sun, 06 Nov 1994 08:49:37 GMT    IMF-fixdate, the only one a sender may emit
+//   Sunday, 06-Nov-94 08:49:37 GMT   obsolete RFC 850
+//   Sun Nov  6 08:49:37 1994         obsolete ANSI C asctime()
+//
+// nullopt when the text is none of them. All three are GMT by definition; a
+// zone other than GMT on the first two is a rejection rather than an offset.
+//
+// `reference` resolves rfc850's two-digit year, and nothing else: per §5.6.7 a
+// year that would land more than fifty years ahead of the reference is read as
+// the most recent past year with those digits, which is what keeps a 1994
+// timestamp from reading as 2094.
+//
+// Deliberately not Timestamp::Parse(kHttpDate): that one is Smithy's
+// @timestampFormat http-date, strict IMF-fixdate, and the protocol
+// conformance suites pin it that way. Wire-format strictness for a modeled
+// member and recipient leniency for an HTTP field are different rules that
+// happen to share a spelling.
+std::optional<Timestamp> ParseHttpDate(std::string_view text, Timestamp reference);
+
 // Splits a list-valued header of HTTP-dates, which themselves contain one
 // comma ("Mon, 16 Dec 2019 23:48:18 GMT, Tue, 17 Dec ..."): consecutive
 // comma-separated tokens are re-joined two at a time.

runtime/src/client/retry.cc

@@ -1,7 +1,13 @@
 #include "opal/client/retry.h"
 
 #include <algorithm>
+#include <chrono>
+#include <cstdint>
+#include <cstdlib>
+#include <limits>
+#include <optional>
 #include <random>
+#include <string>
 #include <thread>
 
 namespace opal {
@@ -24,12 +30,81 @@ std::chrono::milliseconds RetryDelay(const RetryPolicy& policy, int retry, doubl
       static_cast<std::int64_t>(static_cast<double>(ceiling.count()) * jitter01));
 }
 
+std::optional<std::chrono::milliseconds> RetryAfterDelay(const http::Headers& headers,
+                                                         Timestamp now) {
+  const auto value = headers.Get("retry-after");
+  if (!value.has_value() || value->empty()) {
+    return std::nullopt;
+  }
+
+  // delta-seconds. RFC 9110 §10.2.3 spells it as digits and nothing else:
+  // strtoull on its own would also take "+30", " 30" and the "3.5" of a
+  // client that guessed, so the shape is checked before the value is read.
+  if (value->find_first_not_of("0123456789") == std::string::npos) {
+    const unsigned long long seconds = std::strtoull(value->c_str(), nullptr, 10);
+    // However many digits a peer sends, the result must not wrap into a
+    // small — or negative — duration on its way to the cap that makes it
+    // harmless.
+    constexpr std::int64_t kMaxSeconds = std::numeric_limits<std::int64_t>::max() / 1000;
+    if (seconds > static_cast<unsigned long long>(kMaxSeconds)) {
+      return std::chrono::milliseconds::max();
+    }
+    return std::chrono::seconds(static_cast<std::int64_t>(seconds));
+  }
+
+  // The other form: an absolute HTTP-date, in any of the three spellings a
+  // recipient must accept (RFC 9110 §5.6.7). `now` resolves the obsolete
+  // two-digit year, which is the only thing it is used for there.
+  const auto when = http::ParseHttpDate(*value, now);
+  if (!when.has_value()) {
+    return std::nullopt;
+  }
+  // A date already past asks for nothing, not for negative time. The ordering
+  // is checked before the subtraction rather than after it: this function is
+  // public and Timestamp's unchecked factory can build instants whose
+  // difference exceeds int64, where a signed subtraction is undefined rather
+  // than merely large. Once the pair is ordered, the unsigned difference is
+  // exact for any two int64 instants.
+  if (*when <= now) {
+    return std::chrono::milliseconds(0);
+  }
+  const std::uint64_t ahead = static_cast<std::uint64_t>(when->epoch_milliseconds()) -
+                              static_cast<std::uint64_t>(now.epoch_milliseconds());
+  constexpr auto kRepresentable =
+      static_cast<std::uint64_t>(std::numeric_limits<std::int64_t>::max());
+  return std::chrono::milliseconds(static_cast<std::int64_t>(std::min(ahead, kRepresentable)));
+}
+
 bool RetryableStatus(int status) {
   return status == 429 || status == 500 || status == 502 || status == 503 || status == 504;
 }
 
 namespace {
 
+Timestamp Now() {
+  return Timestamp::FromEpochMilliseconds(std::chrono::duration_cast<std::chrono::milliseconds>(
+                                              std::chrono::system_clock::now().time_since_epoch())
+                                              .count());
+}
+
+// The wait before the next attempt: this client's own backoff, raised to
+// whatever the response asked for and clamped to what the policy will trust.
+// Raised, never lowered — Retry-After is a minimum (RFC 9110 §10.2.3), and
+// coming back sooner than the backoff is the thing the backoff exists to
+// prevent.
+std::chrono::milliseconds DelayBefore(const RetryPolicy& policy, int retry, double jitter01,
+                                      const Outcome<http::HttpResponse>& outcome) {
+  const auto backoff = RetryDelay(policy, retry, jitter01);
+  if (!outcome.ok()) {
+    return backoff;  // a transport error has no headers to ask with
+  }
+  const auto asked = RetryAfterDelay(outcome->headers, Now());
+  if (!asked.has_value()) {
+    return backoff;
+  }
+  return std::max(backoff, std::min(*asked, policy.retry_after_cap));
+}
+
 Outcome<http::HttpResponse> SendWithRetriesImpl(
     http::HttpClient& transport, const http::HttpRequest& request, const RetryPolicy& policy,
     const std::vector<std::shared_ptr<Interceptor>>& interceptors, const http::BodySink* sink) {
@@ -81,7 +156,7 @@ Outcome<http::HttpResponse> SendWithRetriesImpl(
     if (!retryable) {
       return outcome;
     }
-    sleep(RetryDelay(policy, retry, jitter()));
+    sleep(DelayBefore(policy, retry, jitter(), outcome));
     outcome = attempt_send(retry + 1);
   }
   return outcome;