Skip to content

fix: Handle non-429 retriable errors in backoff_wait_generator#61

Open
ReubenFrankel wants to merge 2 commits into
masterfrom
fix/backoff-non-429-retriable-errors
Open

fix: Handle non-429 retriable errors in backoff_wait_generator#61
ReubenFrankel wants to merge 2 commits into
masterfrom
fix/backoff-non-429-retriable-errors

Conversation

@ReubenFrankel

Copy link
Copy Markdown
Contributor

Problem

A transient requests.exceptions.ConnectionError (RemoteDisconnected) crashed the tap during retry handling:

File ".../tap_outbrain/client.py", line 30, in _backoff_from_headers
    if retriable_api_error.response.status_code != HTTPStatus.TOO_MANY_REQUESTS:
AttributeError: 'NoneType' object has no attribute 'status_code'

The backoff wait generator assumed every exception it received was a RetriableAPIError with a populated .response. But the SDK's backoff.on_exception retries on a wider family — ConnectionResetError, RetriableAPIError, Timeout, ConnectionError, ChunkedEncodingError, ContentDecodingError — and the connection-level ones carry no response, so .response.status_code raised AttributeError.

Fix

backoff_wait_generator now decides per-exception:

  • HTTP 429 → wait based on the rate-limit-msec-left header (unchanged behaviour).
  • Anything else → fall back to the default exponential backoff, driven via an inline backoff.expo generator.

This replaces the previous backoff_runtime raise/suppress handshake, which only recognised RetriableAPIError and couldn't fall back cleanly for connection-level errors. Because the check runs on every retry, a connection blip followed by a real 429 now correctly honours the 429's header instead of being locked into exponential backoff.

Verification

Drove the generator manually across the relevant exception sequences — all return sensible waits with no crash:

Sequence Result
ConnErr first (the traceback) exponential fallback ✅
429, 429 header-based ✅
500 first exponential fallback ✅
429 then 500 header, then fallback ✅
ConnErr then 429 fallback, then header ✅
Timeout first exponential fallback ✅

Unit/schema tests pass. The 6 failing tests in test_core.py are pre-existing live-API integration tests (record-limit / schema-match against real data) that fail identically on the base commit and don't exercise backoff.

🤖 Generated with Claude Code

The backoff wait generator assumed every exception it received was a
RetriableAPIError with a populated response and read
`response.status_code` directly. But the SDK retries on a wider set of
exceptions (ConnectionError, Timeout, ConnectionResetError, etc.), none
of which carry a response, so a dropped connection crashed the tap with
`AttributeError: 'NoneType' object has no attribute 'status_code'`.

Rewrite `backoff_wait_generator` to decide per-exception: HTTP 429s use
the `rate-limit-msec-left` header wait, everything else falls back to the
default exponential backoff (driven via an inline `backoff.expo`
generator). This removes the previous raise/`suppress` handshake, which
only recognised RetriableAPIError and could not fall back cleanly for
connection-level errors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ReubenFrankel ReubenFrankel added the bug Something isn't working label Jun 2, 2026
@ReubenFrankel ReubenFrankel changed the title fix: handle non-429 retriable errors in backoff wait generator fix: Handle non-429 retriable errors in backoff_wait_generator Jun 2, 2026
@ReubenFrankel ReubenFrankel self-assigned this Jun 2, 2026
@ReubenFrankel

Copy link
Copy Markdown
Contributor Author

@edgarrmondragon Do you know if there is a better way we can be handling backoff with respect to rate-limit response headers here?

@edgarrmondragon

Copy link
Copy Markdown

@edgarrmondragon Do you know if there is a better way we can be handling backoff with respect to rate-limit response headers here?

No, I don't think there's a better way to respect RateLimit headers when they're available, and fall back to exponential backoff when they're not.

@ReubenFrankel

Copy link
Copy Markdown
Contributor Author

Curious if meltano/sdk#2012 (comment) would make this easier... 🤔

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants