Skip to content

fix(connectors): honor Retry-After on every retried status - #4192

Draft
ryankert01 wants to merge 1 commit into
apache:masterfrom
ryankert01:fix/connectors-retry-after
Draft

ryankert01 wants to merge 1 commit into
apache:masterfrom
ryankert01:fix/connectors-retry-after

Conversation

@ryankert01

@ryankert01 ryankert01 commented Sep 15, 2026

Copy link
Copy Markdown
Member

Which issue does this PR address?

Closes #4168

Rationale

HttpRetryMiddleware read Retry-After only on 429, but it retries any 5xx too.

What changed?

The middleware now reads Retry-After on every status it already retries, so a
503 is no longer retried with the server's own timing thrown away. InfluxDB OSS
v2 documents the header on 503 and does not list 429, so the connector this
middleware was written for hit that directly; InfluxDB Cloud uses 429, which is
what the original code covered.

The honored value is bounded by the same max_delay as the computed backoff.
That bound is the operator's configured ceiling rather than a constant of our
own, because the sleep runs inside the sink consume FFI call, which no
shutdown signal cancels. A value the remote server picks must not decide how
long a connector parks a runtime worker thread. Operators who want to honor
longer windows raise retry_max_delay.

Net effect in both directions: 429 previously honored the header with no
ceiling, and is now bounded. 5xx previously ignored it and is now
server-informed under the same ceiling as before.

Two smaller fixes in the same code: a Retry-After: 0 counted as a real delay
and spent the whole attempt budget inside one round trip, so zero now counts as
absent; and the header lookup uses reqwest::header::RETRY_AFTER rather than a
string literal, matching the rest of the repo.

Follow-up: parse_retry_after
accepts only the integer-seconds form, so the HTTP-date form that
RFC 9110 §10.2.3
also allows falls back to the computed backoff. Adding it needs an httpdate or
chrono dependency, so it stays a separate PR. Tracked in #4168.

Local Execution

  • Passed — fmt, clippy -D warnings on the SDK and the three connectors using
    build_retry_client, 174 SDK tests, cargo doc under RUSTDOCFLAGS=-D warnings.
  • Both new middleware tests confirmed failing against the old behaviour first:
    the 503 test, and the ceiling test on a 429. The two pure tests over
    bounded_retry_after cover a function this PR adds, so they pin its edges
    rather than catch the old bug.
  • prek ran and passes, except markdownlint, license-headers,
    trailing-whitespace, trailing-newline and binary-artifacts, which need
    bash >= 4.2; ran those directly instead.

AI Usage

  • Claude Code.
  • Implementation, tests and docs.
  • Local chain above; prior art checked against urllib3, OTLP and the AWS SDK,
    then set aside, because those clients block one caller thread while this sleep
    parks a shared runtime worker.
  • Yes.

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.63%. Comparing base (9061596) to head (2e5e329).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@              Coverage Diff              @@
##             master    #4192       +/-   ##
=============================================
- Coverage     87.42%   68.63%   -18.80%     
  Complexity     1575     1575               
=============================================
  Files          1280     1278        -2     
  Lines        222704   183591    -39113     
  Branches     186067   146954    -39113     
=============================================
- Hits         194706   125999    -68707     
- Misses        23287    52814    +29527     
- Partials       4711     4778       +67     
Components Coverage Δ
Rust Core 64.89% <100.00%> (-23.61%) ⬇️
Java SDK 68.68% <ø> (ø)
C# SDK 77.42% <ø> (ø)
Python SDK 90.97% <ø> (ø)
PHP SDK 85.67% <ø> (ø)
Node SDK 96.43% <ø> (ø)
Go SDK 70.08% <ø> (ø)
Files with missing lines Coverage Δ
core/connectors/sdk/src/retry.rs 97.17% <100.00%> (+1.00%) ⬆️

... and 411 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ryankert01
ryankert01 force-pushed the fix/connectors-retry-after branch from 92e136d to cdf188f Compare September 15, 2026 09:32
`HttpRetryMiddleware` read `Retry-After` only on 429, but it retries any
5xx as well, and RFC 9110 allows the header on any 5xx. InfluxDB OSS
documents it on 503 in particular, so the connector this middleware was
written for asked us to come back at a known time and we guessed instead.

The header now replaces the computed backoff on every status the
middleware already retries, bounded by the same `max_delay` as the
backoff itself. The bound is the operator's ceiling rather than a
constant of our own, because the sleep runs inside the sink `consume`
FFI call, which no shutdown signal cancels: a value the remote server
picks must not decide how long a connector parks a runtime worker
thread. Operators who want to honor longer windows raise
`retry_max_delay`.

A zero value now counts as absent. Honoring it spent the whole attempt
budget inside one round trip, which is the opposite of what a server
asking for an immediate retry wants.

This bounds 429 as well, which had no ceiling at all before.
@ryankert01
ryankert01 force-pushed the fix/connectors-retry-after branch from cdf188f to 2e5e329 Compare September 15, 2026 18:59
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.

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

1 participant