Skip to content

fix(connectors): honor Retry-After on 5xx and cap the honored value #4168

Description

@ryankert01

Description

1. We only read it on 429. retry.rs:425 checks for TOO_MANY_REQUESTS and ignores the header otherwise, but we retry 5xx too (retry.rs:351), and RFC 9110 §10.2.3 allows the header on any 5xx. The server tells us when it will be back and we guess instead. The same section allows an HTTP-date form, which parse_retry_after also drops.

2. We honor it with no upper limit. retry.rs:439 sleeps the value directly, and max_delay bounds only the backoff we compute ourselves. Retry-After: 86400 sleeps a connector for a day behind one warn! line.

Affects every connector built with build_retry_client: the Quickwit sink, the InfluxDB sink, and the InfluxDB source.

Affected area / component

Connectors

Proposed solution

Read the header on any status we already retry, accept the date form, and cap the honored value with a fixed MAX_RETRY_AFTER.

The cap guards against an absurd value; it is not a tuning knob. Prior art agrees it should be separate from, and much larger than, the ordinary backoff ceiling:

Honors it Limit on the honored value
urllib3 yes retry_after_max, 6h default, separate from backoff_max
OpenTelemetry OTLP yes throttle overrides backoff; the backoff clamp does not apply
AWS SDK x-amz-retry-after computed delay + 5s; the 20s backoff cap does not apply

urllib3's comment on its default: "This is undocumented in the RFC. Setting to 6 hours matches other popular libraries."

The one open question is the value. I suggest 1 hour: it honors any realistic rate-limit window and still bounds an absurd one. 6 hours would match urllib3, but connectors move data continuously, so tighter seems better. Either works.

Alternatives considered

  • Clamp to max_delay, as state/http.rs:567 does today. None of the libraries above do this, and Quickwit's DEFAULT_RETRY_MAX_DELAY is 5s, so it would cut a genuine rate-limit window short and spend the retry budget for nothing.
  • A per-connector config field instead of a constant. It is a safety bound, not a dial, and is easy to add later if a real case appears.

Contribution

  • I'm willing to submit a pull request to implement this feature

Good first issue

  • I think this could be a good first issue for a new contributor

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions