diff --git a/CHANGELOG.md b/CHANGELOG.md index 21c8a39b..24f9c6db 100644 --- a/CHANGELOG.md +++ b/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)` diff --git a/docs/production-guide.md b/docs/production-guide.md index 9cc6a14f..ad5c2ea4 100644 --- a/docs/production-guide.md +++ b/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 diff --git a/docs/research/client-third-party-api-gaps.md b/docs/research/client-third-party-api-gaps.md index c86c1e08..87f110e8 100644 --- a/docs/research/client-third-party-api-gaps.md +++ b/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 | diff --git a/runtime/include/opal/client/retry.h b/runtime/include/opal/client/retry.h index ab93f2b4..ff2513a6 100644 --- a/runtime/include/opal/client/retry.h +++ b/runtime/include/opal/client/retry.h @@ -4,10 +4,13 @@ #include #include #include +#include #include #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 sleep; std::function 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 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. diff --git a/runtime/include/opal/http/headers.h b/runtime/include/opal/http/headers.h index 2451b458..a8d420a2 100644 --- a/runtime/include/opal/http/headers.h +++ b/runtime/include/opal/http/headers.h @@ -7,6 +7,8 @@ #include #include +#include "opal/core/timestamp.h" + namespace opal::http { // HTTP header collection: case-insensitive names, repeated names preserved in @@ -52,6 +54,28 @@ std::vector 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 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. diff --git a/runtime/src/client/retry.cc b/runtime/src/client/retry.cc index ab0f4026..610e6ae4 100644 --- a/runtime/src/client/retry.cc +++ b/runtime/src/client/retry.cc @@ -1,7 +1,13 @@ #include "opal/client/retry.h" #include +#include +#include +#include +#include +#include #include +#include #include namespace opal { @@ -24,12 +30,81 @@ std::chrono::milliseconds RetryDelay(const RetryPolicy& policy, int retry, doubl static_cast(static_cast(ceiling.count()) * jitter01)); } +std::optional 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::max() / 1000; + if (seconds > static_cast(kMaxSeconds)) { + return std::chrono::milliseconds::max(); + } + return std::chrono::seconds(static_cast(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(when->epoch_milliseconds()) - + static_cast(now.epoch_milliseconds()); + constexpr auto kRepresentable = + static_cast(std::numeric_limits::max()); + return std::chrono::milliseconds(static_cast(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::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& 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 SendWithRetriesImpl( http::HttpClient& transport, const http::HttpRequest& request, const RetryPolicy& policy, const std::vector>& interceptors, const http::BodySink* sink) { @@ -81,7 +156,7 @@ Outcome SendWithRetriesImpl( if (!retryable) { return outcome; } - sleep(RetryDelay(policy, retry, jitter())); + sleep(DelayBefore(policy, retry, jitter(), outcome)); outcome = attempt_send(retry + 1); } return outcome; diff --git a/runtime/src/http/headers.cc b/runtime/src/http/headers.cc index b881a3c8..301a3abc 100644 --- a/runtime/src/http/headers.cc +++ b/runtime/src/http/headers.cc @@ -1,7 +1,12 @@ #include "opal/http/headers.h" #include +#include #include +#include +#include +#include +#include namespace opal::http { namespace { @@ -49,6 +54,131 @@ bool HeaderNameStartsWith(std::string_view name, std::string_view prefix) { [](char x, char y) { return AsciiLower(x) == AsciiLower(y); }); } +namespace { + +constexpr std::array kAbbreviatedDays = {"Sun", "Mon", "Tue", "Wed", + "Thu", "Fri", "Sat"}; +constexpr std::array kFullDays = {"Sunday", "Monday", "Tuesday", "Wednesday", + "Thursday", "Friday", "Saturday"}; + +bool AllDigits(std::string_view text) { + return !text.empty() && text.find_first_not_of("0123456789") == std::string_view::npos; +} + +// The whole of `text` as an int, or nullopt. from_chars rather than stoi: +// this library builds under -fno-exceptions too, and a conversion that +// reports failure in its return value has nothing to throw. +std::optional ParseInt(std::string_view text) { + int value = 0; + const auto [end, ec] = std::from_chars(text.data(), text.data() + text.size(), value); + if (ec != std::errc{} || end != text.data() + text.size()) { + return std::nullopt; + } + return value; +} + +// The reference's civil year, via the format the runtime already tests, so no +// second civil decomposition exists to disagree with the first. Negative when +// the instant falls outside the representable window, where a two-digit year +// cannot be resolved against anything. +int ReferenceYear(Timestamp reference) { + const std::string rendered = reference.Format(TimestampFormat::kDateTime); + if (rendered.size() < 4) { + return -1; + } + // AllDigits before the conversion because from_chars would take a leading + // '-', and a negative year here is garbage rather than an instant. + const std::string_view year = std::string_view(rendered).substr(0, 4); + return AllDigits(year) ? ParseInt(year).value_or(-1) : -1; +} + +// RFC 9110 §5.6.7: a two-digit year more than fifty years ahead of the +// reference is the most recent past year ending in those digits. Without it a +// 1994 timestamp reads as 2094, and a delay in the past becomes seventy years +// in the future. +std::string ResolveTwoDigitYear(int two_digits, int reference_year) { + const int century = (reference_year / 100) * 100; + int year = century + two_digits; + if (year > reference_year + 50) { + year -= 100; + } + std::string text = std::to_string(year); + return std::string(4 - text.size(), '0') + text; +} + +// "Sunday, 06-Nov-94 08:49:37 GMT" as IMF-fixdate, or empty when it is not +// that shape. The weekday is carried across rather than recomputed, so the +// IMF-fixdate parser's own weekday check still has something to check. +std::string Rfc850AsFixdate(std::string_view text, Timestamp reference) { + const auto comma = text.find(','); + if (comma == std::string_view::npos) return {}; + std::size_t day = 0; + while (day < kFullDays.size() && kFullDays[day] != text.substr(0, comma)) ++day; + if (day == kFullDays.size()) return {}; + + // " 06-Nov-94 08:49:37 GMT" is fixed-width once the day name is off. + const std::string_view rest = text.substr(comma + 1); + if (rest.size() != 23 || rest[0] != ' ' || rest[3] != '-' || rest[7] != '-' || rest[10] != ' ' || + rest.substr(19) != " GMT") { + return {}; + } + const std::string_view day_of_month = rest.substr(1, 2); + const std::string_view month = rest.substr(4, 3); + const std::string_view two_digit_year = rest.substr(8, 2); + const std::string_view time_of_day = rest.substr(11, 8); + if (!AllDigits(day_of_month) || !AllDigits(two_digit_year)) return {}; + const int reference_year = ReferenceYear(reference); + const auto year_digits = ParseInt(two_digit_year); + if (reference_year < 0 || !year_digits.has_value()) return {}; + + return std::string(kAbbreviatedDays[day]) + ", " + std::string(day_of_month) + " " + + std::string(month) + " " + ResolveTwoDigitYear(*year_digits, reference_year) + " " + + std::string(time_of_day) + " GMT"; +} + +// "Sun Nov 6 08:49:37 1994" as IMF-fixdate, or empty. Fixed-width, with the +// day of the month space-padded rather than zero-padded. +std::string AsctimeAsFixdate(std::string_view text) { + if (text.size() != 24 || text[3] != ' ' || text[7] != ' ' || text[10] != ' ' || text[19] != ' ') { + return {}; + } + std::size_t day = 0; + while (day < kAbbreviatedDays.size() && kAbbreviatedDays[day] != text.substr(0, 3)) ++day; + if (day == kAbbreviatedDays.size()) return {}; + + const std::string_view month = text.substr(4, 3); + const std::string_view time_of_day = text.substr(11, 8); + const std::string_view year = text.substr(20, 4); + const std::string day_of_month = + text[8] == ' ' ? "0" + std::string(text.substr(9, 1)) : std::string(text.substr(8, 2)); + if (!AllDigits(day_of_month) || !AllDigits(year)) return {}; + + return std::string(kAbbreviatedDays[day]) + ", " + day_of_month + " " + std::string(month) + " " + + std::string(year) + " " + std::string(time_of_day) + " GMT"; +} + +} // namespace + +std::optional ParseHttpDate(std::string_view text, Timestamp reference) { + // IMF-fixdate first: the only form a sender is allowed to produce, and so + // the only one worth trying before the two kept alive for old peers. + if (const auto fixdate = Timestamp::Parse(text, TimestampFormat::kHttpDate); fixdate.ok()) { + return *fixdate; + } + std::string normalized = Rfc850AsFixdate(text, reference); + if (normalized.empty()) { + normalized = AsctimeAsFixdate(text); + } + if (normalized.empty()) { + return std::nullopt; + } + const auto parsed = Timestamp::Parse(normalized, TimestampFormat::kHttpDate); + if (!parsed.ok()) { + return std::nullopt; + } + return *parsed; +} + bool HeaderNameEquals(std::string_view a, std::string_view b) { return a.size() == b.size() && std::equal(a.begin(), a.end(), b.begin(), [](char x, char y) { return AsciiLower(x) == AsciiLower(y); diff --git a/runtime/tests/client/retry_test.cc b/runtime/tests/client/retry_test.cc index 6db65495..17510b79 100644 --- a/runtime/tests/client/retry_test.cc +++ b/runtime/tests/client/retry_test.cc @@ -3,6 +3,8 @@ #include #include +#include +#include #include #include #include @@ -10,6 +12,8 @@ #include #include "opal/core/error.h" +#include "opal/core/timestamp.h" +#include "opal/http/headers.h" namespace opal { namespace { @@ -57,6 +61,172 @@ TEST(RetryableStatusTest, TransientStatusesOnly) { } } +// A response carrying the server's own idea of when to come back. +http::HttpResponse Throttled(int status, const std::string& retry_after) { + http::HttpResponse response; + response.status = status; + if (!retry_after.empty()) response.headers.Set("retry-after", retry_after); + response.body = "slow down"; + return response; +} + +Timestamp At(const char* http_date) { + auto parsed = Timestamp::Parse(http_date, TimestampFormat::kHttpDate); + return parsed.ok() ? *parsed : Timestamp{}; +} + +TEST(RetryAfterDelayTest, ReadsDeltaSeconds) { + http::Headers headers; + headers.Set("retry-after", "30"); + EXPECT_EQ(RetryAfterDelay(headers, Timestamp{}), milliseconds(30000)); + + headers.Set("retry-after", "0"); + EXPECT_EQ(RetryAfterDelay(headers, Timestamp{}), milliseconds(0)); +} + +TEST(RetryAfterDelayTest, ReadsAnHttpDateAgainstNow) { + const Timestamp now = At("Fri, 31 Dec 1999 23:59:00 GMT"); + http::Headers headers; + headers.Set("retry-after", "Fri, 31 Dec 1999 23:59:30 GMT"); + EXPECT_EQ(RetryAfterDelay(headers, now), milliseconds(30000)); + + // Already past: the server is asking for nothing, not for negative time. + headers.Set("retry-after", "Fri, 31 Dec 1999 23:58:00 GMT"); + EXPECT_EQ(RetryAfterDelay(headers, now), milliseconds(0)); +} + +TEST(RetryAfterDelayTest, ReadsTheObsoleteHttpDateFormatsToo) { + // RFC 9110 §5.6.7 requires a recipient to accept all three HTTP-date + // formats, and Retry-After's HTTP-date alternative inherits that. Ignoring + // the obsolete two means coming back earlier than a server asked, silently + // — the bug this whole change exists to fix, just for older servers. + const Timestamp now = At("Sun, 06 Nov 1994 08:49:00 GMT"); + for (const char* value : {"Sun, 06 Nov 1994 08:49:37 GMT", "Sunday, 06-Nov-94 08:49:37 GMT", + "Sun Nov 6 08:49:37 1994"}) { + http::Headers headers; + headers.Set("retry-after", value); + EXPECT_EQ(RetryAfterDelay(headers, now), milliseconds(37000)) << "value: " << value; + } +} + +TEST(RetryAfterDelayTest, AnExtremeReferenceDoesNotOverflowTheSubtraction) { + // The function is public, and Timestamp::FromEpochMilliseconds is an + // unchecked factory, so the difference of two legal Timestamps can exceed + // int64. Signed overflow is undefined behavior, not a large number. + http::Headers headers; + headers.Set("retry-after", "Fri, 31 Dec 9999 23:59:59 GMT"); + const auto delay = RetryAfterDelay( + headers, Timestamp::FromEpochMilliseconds(std::numeric_limits::min())); + ASSERT_TRUE(delay.has_value()); + EXPECT_GT(*delay, milliseconds(0)) << "a far-future date read as no delay at all"; + EXPECT_GE(*delay, milliseconds(86400000)); + + // The mirror image: a far-past date against a far-future reference is zero, + // not a wrapped positive. + headers.Set("retry-after", "Thu, 01 Jan 1970 00:00:00 GMT"); + EXPECT_EQ(RetryAfterDelay(headers, + Timestamp::FromEpochMilliseconds(std::numeric_limits::max())), + milliseconds(0)); +} + +TEST(RetryAfterDelayTest, AbsentOrMalformedAsksForNothing) { + const http::Headers none; + EXPECT_EQ(RetryAfterDelay(none, Timestamp{}), std::nullopt); + + // A peer's malformed hint is not worth failing a call over; ordinary + // backoff is the safe answer. Note "30s" and "+30": near-misses that a lax + // parser would accept and RFC 9110 does not. + for (const char* value : {"", "soon", "-5", "30s", "+30", "0x10", " 30", "30 ", "3.5"}) { + http::Headers headers; + headers.Set("retry-after", value); + EXPECT_EQ(RetryAfterDelay(headers, Timestamp{}), std::nullopt) << "value: " << value; + } +} + +TEST(RetryAfterDelayTest, AnAbsurdDelaySaturatesRatherThanOverflowing) { + // Digits are syntactically fine however many there are. The cap is what + // makes this harmless, so the parse must not wrap into a small or negative + // duration on the way there. + http::Headers headers; + headers.Set("retry-after", "999999999999999999999"); + const auto delay = RetryAfterDelay(headers, Timestamp{}); + ASSERT_TRUE(delay.has_value()); + EXPECT_GT(*delay, milliseconds(0)); + EXPECT_GE(*delay, milliseconds(86400000)); +} + +TEST(SendWithRetriesTest, RetryAfterRaisesTheBackoffItDoesNotLowerIt) { + // The header is a floor under this client's own backoff, per RFC 9110: + // the server may ask it to wait longer, never to come back sooner. + ScriptedTransport transport; + transport.script = {Throttled(429, "2"), http::HttpResponse{200, {}, "ok"}}; + std::vector slept; + const auto outcome = SendWithRetries(transport, {}, InstantPolicy(&slept)); + ASSERT_TRUE(outcome.ok()) << outcome.error().message(); + ASSERT_EQ(slept.size(), 1u); + EXPECT_EQ(slept[0], milliseconds(2000)) << "the server asked for 2s and got the 100ms backoff"; + + // Asking for less than the backoff changes nothing: the backoff is already + // the longer of the two, and coming back early is what it exists to stop. + ScriptedTransport impatient; + impatient.script = {Throttled(503, "0"), http::HttpResponse{200, {}, "ok"}}; + std::vector impatient_slept; + ASSERT_TRUE(SendWithRetries(impatient, {}, InstantPolicy(&impatient_slept)).ok()); + ASSERT_EQ(impatient_slept.size(), 1u); + EXPECT_EQ(impatient_slept[0], milliseconds(100)); +} + +TEST(SendWithRetriesTest, ClampsRetryAfterToItsOwnCap) { + // How far this client will trust a number the peer sent. An hour is not + // an offer a caller has to accept. + ScriptedTransport transport; + transport.script = {Throttled(429, "3600"), http::HttpResponse{200, {}, "ok"}}; + std::vector slept; + RetryPolicy policy = InstantPolicy(&slept); + policy.retry_after_cap = milliseconds(5000); + ASSERT_TRUE(SendWithRetries(transport, {}, policy).ok()); + ASSERT_EQ(slept.size(), 1u); + EXPECT_EQ(slept[0], milliseconds(5000)); +} + +TEST(SendWithRetriesTest, AMalformedOrAbsentRetryAfterLeavesTheBackoffAlone) { + ScriptedTransport transport; + transport.script = {Throttled(429, "whenever"), Throttled(503, ""), + http::HttpResponse{200, {}, "ok"}}; + std::vector slept; + ASSERT_TRUE(SendWithRetries(transport, {}, InstantPolicy(&slept)).ok()); + EXPECT_EQ(slept, (std::vector{milliseconds(100), milliseconds(200)})); +} + +TEST(SendWithRetriesTest, ATransportErrorHasNoHeaderToHonor) { + ScriptedTransport transport; + transport.script = {Error::Transport("refused"), http::HttpResponse{200, {}, "ok"}}; + std::vector slept; + ASSERT_TRUE(SendWithRetries(transport, {}, InstantPolicy(&slept)).ok()); + EXPECT_EQ(slept, std::vector{milliseconds(100)}); +} + +TEST(SendWithRetriesTest, AnHttpDateRetryAfterIsMeasuredAgainstTheRealClock) { + // The loop reads the date form against the wall clock, which the pure + // parser tests above cannot pin because they supply `now` themselves. + const auto in_two_seconds = + Timestamp::FromEpochMilliseconds(std::chrono::duration_cast( + std::chrono::system_clock::now().time_since_epoch()) + .count() + + 2000) + .Format(TimestampFormat::kHttpDate); + + ScriptedTransport transport; + transport.script = {Throttled(429, in_two_seconds), http::HttpResponse{200, {}, "ok"}}; + std::vector slept; + ASSERT_TRUE(SendWithRetries(transport, {}, InstantPolicy(&slept)).ok()); + ASSERT_EQ(slept.size(), 1u); + // A second of slack either way: the date has whole-second resolution and + // the clock moves between building the header and reading it. + EXPECT_GE(slept[0], milliseconds(500)); + EXPECT_LE(slept[0], milliseconds(3000)); +} + TEST(SendWithRetriesTest, RetriesTransportErrorsThenSucceeds) { ScriptedTransport transport; transport.script = {Error::Transport("refused"), Error::Transport("refused"), diff --git a/runtime/tests/http/http_test.cc b/runtime/tests/http/http_test.cc index ac2c10b6..8843f398 100644 --- a/runtime/tests/http/http_test.cc +++ b/runtime/tests/http/http_test.cc @@ -1,5 +1,6 @@ #include +#include "opal/core/timestamp.h" #include "opal/http/headers.h" #include "opal/http/loopback.h" #include "opal/http/message.h" @@ -289,3 +290,57 @@ TEST(HeadersTest, RequestLineFieldPredicateRejectsSpaceAndControls) { } // namespace } // namespace opal::http + +namespace opal::http { +namespace { + +// RFC 9110 §5.6.7's own example: three spellings of one instant. +constexpr char kImfFixdate[] = "Sun, 06 Nov 1994 08:49:37 GMT"; +constexpr char kRfc850[] = "Sunday, 06-Nov-94 08:49:37 GMT"; +constexpr char kAsctime[] = "Sun Nov 6 08:49:37 1994"; + +Timestamp In(const char* date_time) { + auto parsed = Timestamp::Parse(date_time, TimestampFormat::kDateTime); + return parsed.ok() ? *parsed : Timestamp{}; +} + +TEST(ParseHttpDateTest, AcceptsAllThreeFormatsAsTheSameInstant) { + // A recipient MUST accept all three (§5.6.7), obsolete or not: a server + // still emitting rfc850 is exactly the server whose Retry-After a client + // would otherwise ignore and come back too early. + const Timestamp reference = In("2026-09-12T00:00:00Z"); + const Timestamp expected = In("1994-11-06T08:49:37Z"); + + EXPECT_EQ(ParseHttpDate(kImfFixdate, reference), expected); + EXPECT_EQ(ParseHttpDate(kRfc850, reference), expected); + EXPECT_EQ(ParseHttpDate(kAsctime, reference), expected); +} + +TEST(ParseHttpDateTest, ResolvesTheObsoleteTwoDigitYearAgainstTheReference) { + // "94" read naively as 2094 would turn a long-past timestamp into a + // seventy-year delay. §5.6.7: more than fifty years ahead means it is the + // most recent past year with those digits. + EXPECT_EQ(ParseHttpDate(kRfc850, In("2026-09-12T00:00:00Z")), In("1994-11-06T08:49:37Z")); + + // Within fifty years it is the future year it appears to be. The weekday + // is Wednesday because 2030-11-06 is one: the IMF-fixdate parser checks the + // weekday against the date, which caught this test carrying 1994's Sunday + // over to a 2030 date. + EXPECT_EQ(ParseHttpDate("Wednesday, 06-Nov-30 08:49:37 GMT", In("2026-09-12T00:00:00Z")), + In("2030-11-06T08:49:37Z")); + + // The rule is relative to the reference, not to a hardcoded century. + EXPECT_EQ(ParseHttpDate(kRfc850, In("1996-01-01T00:00:00Z")), In("1994-11-06T08:49:37Z")); +} + +TEST(ParseHttpDateTest, RejectsWhatIsNoneOfTheThree) { + const Timestamp reference = In("2026-09-12T00:00:00Z"); + for (const char* text : {"", "soon", "1994-11-06T08:49:37Z", "Sun, 06 Nov 1994 08:49:37", + "Sun, 06 Nov 1994 08:49:37 PST", "Sun, 32 Nov 1994 08:49:37 GMT", + "Sun, 06 Xxx 1994 08:49:37 GMT", "Sun Nov 6 08:49:37"}) { + EXPECT_EQ(ParseHttpDate(text, reference), std::nullopt) << "text: " << text; + } +} + +} // namespace +} // namespace opal::http