From 89986b9f3c3c00a266985c134bcfbcd1cc60310b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 21:08:22 +0000 Subject: [PATCH 1/2] Honor Retry-After as a floor under the backoff (#189) The retry loop was pure full-jitter exponential and never read the header, so a 429 asking for 30 seconds was retried inside max_backoff's 20-second window: an attempt spent arriving early to be refused again. A service that documents its throttling, which is the case that started this, was the one the client handled worst. Both RFC 9110 forms are read, delta-seconds and HTTP-date, the latter through the runtime's own IMF-fixdate parser rather than a second one. A date already past asks for nothing rather than for negative time, and a value that does not parse is ignored in favor of ordinary backoff: a peer's malformed hint is not worth failing a call over. Floor, never ceiling. Retry-After is a minimum, so it can raise the wait and never shorten the backoff that exists to stop a client coming back early. retry_after_cap (default 60s) bounds how far a number the peer sent is trusted. Separate from max_backoff because they bound different things: one a guess this client made, the other a number it was sent. Higher by default because truncating a documented 30 to 20 is worse than useless. Written test-first: the six positive cases were red against a stubbed RetryAfterDelay, and the absent/malformed ones passed against it from the start, which is the shape that says they pin the negative space. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jj5X2fKdgrYurHmbwLzUiQ --- CHANGELOG.md | 13 ++ docs/production-guide.md | 31 ++++- docs/research/client-third-party-api-gaps.md | 2 +- runtime/include/opal/client/retry.h | 28 ++++ runtime/src/client/retry.cc | 65 ++++++++- runtime/tests/client/retry_test.cc | 134 +++++++++++++++++++ 6 files changed, 270 insertions(+), 3 deletions(-) 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/src/client/retry.cc b/runtime/src/client/retry.cc index ab0f4026..d63736d6 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,69 @@ 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, read with the runtime's own + // IMF-fixdate parser rather than a second one written here. + const auto when = Timestamp::Parse(*value, TimestampFormat::kHttpDate); + if (!when) { + return std::nullopt; + } + const std::int64_t delta = when->epoch_milliseconds() - now.epoch_milliseconds(); + // A date already past asks for nothing, not for negative time. + return std::chrono::milliseconds(std::max(delta, 0)); +} + 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 +144,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/tests/client/retry_test.cc b/runtime/tests/client/retry_test.cc index 6db65495..8f876620 100644 --- a/runtime/tests/client/retry_test.cc +++ b/runtime/tests/client/retry_test.cc @@ -10,6 +10,8 @@ #include #include "opal/core/error.h" +#include "opal/core/timestamp.h" +#include "opal/http/headers.h" namespace opal { namespace { @@ -57,6 +59,138 @@ 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, 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"), From 9908404a560b23a00421098809a3f0b449c526b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 03:13:48 +0000 Subject: [PATCH 2/2] Accept all three HTTP-date formats, and stop overflowing the subtraction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 9110 §5.6.7 requires a recipient to accept IMF-fixdate and the two obsolete forms, and Retry-After's HTTP-date alternative inherits that. Only the first was read, so a server still emitting rfc850 or asctime was silently ignored and the client came back early — the bug this change exists to fix, for exactly the old servers most likely to hit it. ParseHttpDate lives in the http layer because it is an HTTP field concern, not a Smithy one: Timestamp::Parse(kHttpDate) stays strict IMF-fixdate, which is what @timestampFormat http-date means and what the protocol conformance suites pin. The obsolete forms are normalized into IMF-fixdate text and handed to that same parser, so there is no second civil-date arithmetic to disagree with the first. The two-digit year follows §5.6.7's fifty-year rule, without which a 1994 timestamp reads as 2094 and a delay in the past becomes seventy years in the future. The delta was a signed subtraction of two Timestamps. The function is public and Timestamp's unchecked factory can build instants whose difference exceeds int64, where that is undefined rather than large. Ordering is checked first; the unsigned difference of an ordered pair is exact, then saturated. Written test-first. The weekday check in the IMF-fixdate parser earned its keep immediately by rejecting test data that carried 1994's Sunday over to a 2030 date. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jj5X2fKdgrYurHmbwLzUiQ --- runtime/include/opal/http/headers.h | 24 +++++ runtime/src/client/retry.cc | 26 ++++-- runtime/src/http/headers.cc | 130 ++++++++++++++++++++++++++++ runtime/tests/client/retry_test.cc | 36 ++++++++ runtime/tests/http/http_test.cc | 55 ++++++++++++ 5 files changed, 264 insertions(+), 7 deletions(-) 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 d63736d6..610e6ae4 100644 --- a/runtime/src/client/retry.cc +++ b/runtime/src/client/retry.cc @@ -52,15 +52,27 @@ std::optional RetryAfterDelay(const http::Headers& he return std::chrono::seconds(static_cast(seconds)); } - // The other form: an absolute HTTP-date, read with the runtime's own - // IMF-fixdate parser rather than a second one written here. - const auto when = Timestamp::Parse(*value, TimestampFormat::kHttpDate); - if (!when) { + // 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; } - const std::int64_t delta = when->epoch_milliseconds() - now.epoch_milliseconds(); - // A date already past asks for nothing, not for negative time. - return std::chrono::milliseconds(std::max(delta, 0)); + // 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) { 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 8f876620..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 @@ -93,6 +95,40 @@ TEST(RetryAfterDelayTest, ReadsAnHttpDateAgainstNow) { 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); 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