apache / apache/iggy

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

Open
#4,168 1 comment 0 reactions 1 assignee Claimed by @ryankert01 View on GitHub
connectors
Dominant language
Rust
Stars
4.9k
Forks
432
Avg merge
2d 10h
Merged PRs (30d)
173

Description

### Description

**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

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.