From 0ff4931e100030d2d31d2ab6edf07c74bbb6a59b Mon Sep 17 00:00:00 2001 From: plenarius Date: Sat, 2 May 2026 13:46:58 -0400 Subject: [PATCH] fix(HTTPClient): always emit RETRY_AFTER for 429s; recognise OpenAI headers Before this change, the plugin only broadcast RETRY_AFTER when the response matched a Discord shape (X-RateLimit-Limit present) or a Slack shape (Retry-After present). OpenAI 429 responses do neither -- especially insufficient_quota 429s, which carry no retry hint at all -- so downstream NWScript handlers read an undefined event tag (logged as "Tried to access event data with invalid tag: 'RETRY_AFTER'"), parsed it to 0, and immediately re-fired requests in a tight loop, crashing the NWN server after sustained 429 traffic. This refactor: - Adds ComputeRetryAfterMs() that probes hint headers in priority order: Retry-After-Ms (OpenAI extension), X-RateLimit-Reset-After (Discord float-seconds, more precise than Retry-After), Retry-After (RFC 7231 delay-seconds), then OpenAI's per-bucket X-RateLimit-Reset-Requests / -Tokens with their "1.5s" / "200ms" / "1m30s" duration format. - Always broadcasts RETRY_AFTER (in milliseconds) for every 429, falling back to a 5s default when no header is parseable. Floors at 1s and caps at 1h so a misbehaving server can never busy-loop callers OR park them for absurd durations. - Wraps every numeric parse in try/catch so a malformed header value cannot terminate the response task with an unhandled exception. - Preserves the existing X-RateLimit-Limit/Remaining/Reset Discord metadata broadcasts unchanged. - Includes the computed retry delay in the rate-limit log line for easier debugging. --- Plugins/HTTPClient/HTTPClient.cpp | 127 ++++++++++++++++++++++++------ 1 file changed, 101 insertions(+), 26 deletions(-) diff --git a/Plugins/HTTPClient/HTTPClient.cpp b/Plugins/HTTPClient/HTTPClient.cpp index 726f7249608..5458b381c0d 100644 --- a/Plugins/HTTPClient/HTTPClient.cpp +++ b/Plugins/HTTPClient/HTTPClient.cpp @@ -73,6 +73,87 @@ namespace Core { using namespace NWNXLib; using namespace NWNXLib::API; +namespace { + +// Parse a duration string like "1.5s", "200ms", "1m30s" into milliseconds. +// Used for OpenAI-style x-ratelimit-reset-* headers. Returns -1 if unparseable. +int64_t ParseDurationToMs(const std::string& s) +{ + int64_t totalMs = 0; + size_t i = 0; + bool any = false; + while (i < s.size()) + { + size_t numStart = i; + while (i < s.size() && (std::isdigit(static_cast(s[i])) || s[i] == '.')) + ++i; + if (numStart == i) return -1; + double value; + try { value = std::stod(s.substr(numStart, i - numStart)); } + catch (...) { return -1; } + std::string unit; + while (i < s.size() && std::isalpha(static_cast(s[i]))) + unit += static_cast(std::tolower(static_cast(s[i++]))); + if (unit == "ms") totalMs += static_cast(value); + else if (unit == "s" || unit.empty()) totalMs += static_cast(value * 1000.0); + else if (unit == "m") totalMs += static_cast(value * 60.0 * 1000.0); + else if (unit == "h") totalMs += static_cast(value * 3600.0 * 1000.0); + else return -1; + any = true; + } + return any ? totalMs : -1; +} + +// Try every retry-hint header we recognize, in priority order, and return ms. +// Returns -1 if no usable signal is present. +// +// Why every provider needs its own probe: +// - OpenAI 429s (especially insufficient_quota) often omit Retry-After +// entirely but include `retry-after-ms` and per-bucket `x-ratelimit-reset-*`. +// - Discord uses float-seconds in `X-RateLimit-Reset-After` (more precise +// than the integer-seconds `Retry-After`). +// - RFC 7231 Retry-After can be either delay-seconds OR an HTTP-date; we +// handle the numeric case and ignore dates rather than crash. +int64_t ComputeRetryAfterMs(const httplib::Response& response) +{ + if (response.has_header("Retry-After-Ms")) + { + try { return std::stoll(response.get_header_value("Retry-After-Ms")); } + catch (...) {} + } + if (response.has_header("X-RateLimit-Reset-After")) + { + try { return static_cast(std::stof(response.get_header_value("X-RateLimit-Reset-After")) * 1000.0f); } + catch (...) {} + } + if (response.has_header("Retry-After")) + { + try { return static_cast(std::stof(response.get_header_value("Retry-After")) * 1000.0f); } + catch (...) {} // HTTP-date form — fall through + } + int64_t openAiMs = -1; + for (const char* h : {"X-RateLimit-Reset-Requests", "X-RateLimit-Reset-Tokens"}) + { + if (response.has_header(h)) + { + int64_t v = ParseDurationToMs(response.get_header_value(h)); + if (v >= 0 && (openAiMs < 0 || v < openAiMs)) + openAiMs = v; + } + } + return openAiMs; +} + +// Bounds applied to whatever ComputeRetryAfterMs returns before we emit it. +// Default: a sane wait when no header is parseable (the original bug — the +// scripts saw 0 and busy-looped). Min: never busy-loop. Max: never park +// requests for absurd durations even if a server returns garbage. +constexpr int64_t kRetryDefaultMs = 5000; +constexpr int64_t kRetryMinMs = 1000; +constexpr int64_t kRetryMaxMs = 60LL * 60 * 1000; + +} // namespace + static std::unique_ptr GetResult(const Request&); static httplib::Headers ParseHeaderString(const std::string&); static int s_clientRequestId = 0; @@ -202,9 +283,8 @@ void PerformRequest(const Request &client_req) response.status == 201 || response.status == 204 || response.status == 429) { - // Discord sends your rate limit information even on success so you can stagger calls if you want - // This header also lets us know it's Discord not Slack, important because Discord sends RETRY_AFTER - // in milliseconds and Slack sends it as seconds. + // Discord rate-limit metadata: emitted on every response (incl. + // success) so callers can pace requests preemptively. if (response.has_header("X-RateLimit-Limit")) { MessageBus::Broadcast( @@ -222,32 +302,26 @@ void PerformRequest(const Request &client_req) {"RATELIMIT_RESET", response.get_header_value( "X-RateLimit-Reset")}); - if (response.has_header("Retry-After")) - MessageBus::Broadcast( - "NWNX_EVENT_PUSH_EVENT_DATA", - {"RETRY_AFTER", - response.get_header_value( - "Retry-After")}); - else if (response.has_header("Retry-At")) - { - MessageBus::Broadcast( - "NWNX_EVENT_PUSH_EVENT_DATA", - {"RETRY_AFTER", - response.get_header_value( - "Retry-At")}); - } } - // Slack rate limited - else if (response.has_header("Retry-After")) + + // Compute retry delay (ms) from any provider's hint headers. + // For non-429 responses we only emit RETRY_AFTER if the server + // actually sent a hint (Discord does this on successes too). + // For 429 we ALWAYS emit a value, falling back to a sane + // default — otherwise downstream scripts read an undefined + // event tag, get 0, and busy-loop the main thread. + int64_t retryMs = ComputeRetryAfterMs(response); + if (response.status == 429 && retryMs <= 0) + retryMs = kRetryDefaultMs; + if (retryMs > 0) { - float fSlackRetry = - stof(response.get_header_value( - "Retry-After")) * 1000.0f; + if (retryMs < kRetryMinMs) retryMs = kRetryMinMs; + if (retryMs > kRetryMaxMs) retryMs = kRetryMaxMs; MessageBus::Broadcast( "NWNX_EVENT_PUSH_EVENT_DATA", - {"RETRY_AFTER", - std::to_string(fSlackRetry)}); + {"RETRY_AFTER", std::to_string(retryMs)}); } + if (response.status != 429) { MessageBus::Broadcast( @@ -265,8 +339,9 @@ void PerformRequest(const Request &client_req) {"NWNX_ON_HTTPCLIENT_FAILED", moduleOid}); LOG_WARNING( - "HTTP Client Request to '%s%s' failed, rate limited.", - client_req.host, client_req.path); + "HTTP Client Request to '%s%s' failed, rate limited (retry in %lldms).", + client_req.host, client_req.path, + static_cast(retryMs)); } } else