ryankert01 opened a new issue, #4168: URL: https://github.com/apache/iggy/issues/4168
### Description `HttpRetryMiddleware` mishandles `Retry-After`, the "come back in N seconds" hint a server sends when it is overloaded, in two ways. **1. We only read it on 429.** [`retry.rs:425`](https://github.com/apache/iggy/blob/78151b95111a7bc4427e7defa2b90260aa0e6c05/core/connectors/sdk/src/retry.rs#L425) checks for `TOO_MANY_REQUESTS` and ignores the header otherwise, but we retry 5xx too ([`retry.rs:351`](https://github.com/apache/iggy/blob/78151b95111a7bc4427e7defa2b90260aa0e6c05/core/connectors/sdk/src/retry.rs#L351)), and [RFC 9110 ยง10.2.3](https://www.rfc-editor.org/rfc/rfc9110.html#name-retry-after) 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`](https://github.com/apache/iggy/blob/78151b95111a7bc4427e7defa2b90260aa0e6c05/core/connectors/sdk/src/retry.rs#L173) also drops. **2. We honor it with no upper limit.** [`retry.rs:439`](https://github.com/apache/iggy/blob/78151b95111a7bc4427e7defa2b90260aa0e6c05/core/connectors/sdk/src/retry.rs#L439) 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`](https://github.com/apache/iggy/blob/78151b95111a7bc4427e7defa2b90260aa0e6c05/core/connectors/sdk/src/retry.rs#L494): 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](https://github.com/urllib3/urllib3/blob/main/src/urllib3/util/retry.py) | yes | `retry_after_max`, 6h default, separate from `backoff_max` | | [OpenTelemetry OTLP](https://opentelemetry.io/docs/specs/otel/protocol/exporter/) | yes | throttle overrides backoff; the backoff clamp does not apply | | [AWS SDK](https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html) | `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`](https://github.com/apache/iggy/blob/78151b95111a7bc4427e7defa2b90260aa0e6c05/core/connectors/runtime/src/state/http.rs#L567) does today. None of the libraries above do this, and Quickwit's [`DEFAULT_RETRY_MAX_DELAY`](https://github.com/apache/iggy/blob/78151b95111a7bc4427e7defa2b90260aa0e6c05/core/connectors/sinks/quickwit_sink/src/lib.rs#L39) 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 - [x] 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
