Skip to content

fix(HTTPClient): always emit RETRY_AFTER for 429s; recognise OpenAI headers#228

Open
plenarius wants to merge 1 commit into
masterfrom
fix/httpclient-retry-after
Open

fix(HTTPClient): always emit RETRY_AFTER for 429s; recognise OpenAI headers#228
plenarius wants to merge 1 commit into
masterfrom
fix/httpclient-retry-after

Conversation

@plenarius

Copy link
Copy Markdown
Owner

Summary

The HTTPClient plugin only broadcast RETRY_AFTER for Discord-shaped responses (those carrying X-RateLimit-Limit) or Slack-shaped ones (those carrying Retry-After). OpenAI 429s satisfy neither pattern — especially insufficient_quota 429s, which omit retry hints entirely — so the script-side handler read an undefined event tag (which logs Tried to access event data with invalid tag: 'RETRY_AFTER'), parsed it to 0, and immediately re-fired the request. Sustained 429 traffic from OpenAI tipped Layonara's nwserver over today after a credit grant expired.

What this PR does

  • New ComputeRetryAfterMs(response) helper that probes hint headers in priority order:
    1. Retry-After-Ms (OpenAI extension — most precise)
    2. X-RateLimit-Reset-After (Discord float-seconds — more precise than Retry-After)
    3. Retry-After (RFC 7231 delay-seconds; HTTP-date form is ignored rather than crashed on)
    4. OpenAI per-bucket X-RateLimit-Reset-Requests / -Tokens parsed via a new ParseDurationToMs() that handles "1.5s" / "200ms" / "1m30s" formats
  • Always broadcasts RETRY_AFTER (in milliseconds) for every 429, falling back to a 5s default when no header is parseable. Floored at 1s and capped 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 cannot terminate the response task with an unhandled exception. (The existing stof() on Slack Retry-After had the same crash exposure.)
  • Preserves Discord X-RateLimit-Limit/Remaining/Reset metadata broadcasts unchanged — callers can still pace requests preemptively from successful responses.
  • Includes the computed retry delay in the rate-limit log line for easier debugging.

Test plan

  • Builds cleanly: cmake --build . --target HTTPClient produces Binaries/NWNX_HTTPClient.so
  • Deploy to ee64 and verify Discord webhooks, OpenAI calls, and GitHub API calls all still work
  • Force a 429 from OpenAI (or simulate via test endpoint) and confirm RETRY_AFTER is emitted with a sensible non-zero value
  • Confirm Discord rate-limit pacing still works on RATELIMIT_LIMIT/REMAINING/RESET metadata

…eaders

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant