Skip to content

feat(fal-client): add backup domains for fal run requests - #1160

Open
badayvedat wants to merge 5 commits into
mainfrom
vedat/fea-fal-client-backup-domains
Open

feat(fal-client): add backup domains for fal run requests#1160
badayvedat wants to merge 5 commits into
mainfrom
vedat/fea-fal-client-backup-domains

Conversation

@badayvedat

@badayvedat badayvedat commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Assuming at t=0 we send a request to fal.run and it fails due to a Connection Timeout which is set to 5 seconds, the timeline of the events will look like below.

Time, approximately What happens
t=0 Connect to fal.run.
t=5s Connection times out. Immediately try falrun.com; emit a warning.
t=10s Backup connection also times out. First attempt failed.
t=10.05–10.15s After a short randomized delay, start attempt 2 on fal.run again.
~5s later If primary still fails, try backup again.
Both fail again Wait 0.1–0.3s, then start attempt 3 (upper bound is 10 attempts)

Note

Medium Risk
Changes core HTTP transport for all run/queue traffic; behavior is well-tested but alters connection timing, error chaining, and failure modes for every inference request.

Overview
Adds automatic domain failover for sync and async HTTP clients when fal.run or queue.fal.run cannot be reached. On ConnectError or ConnectTimeout, the client retries the same request against falrun.com / queue.falrun.com, logs a host-only warning, and preserves existing retry behavior (primary error type wins when both hosts fail to connect).

Connect timeouts on those mapped hosts are capped at 5 seconds so fallback is not delayed by long caller timeouts; shorter timeouts still apply. Fallback is connect-time only—HTTP errors, read/write failures, and mid-stream failures do not trigger a second host.

run() / stream() docstrings now state the default has no read timeout (unbounded generations) while submit() keeps the client default_timeout. A large unit test suite covers URL rewriting, queue/run/stream paths, redirect handling, and retry policy.

Reviewed by Cursor Bugbot for commit d3738df. Bugbot is set up for automated code reviews on this repo. Configure here.

@badayvedat

Copy link
Copy Markdown
Contributor Author

@claude review
@cursor review

@badayvedat
badayvedat marked this pull request as ready for review September 4, 2026 20:18

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it's a non-trivial change to core HTTP transport/retry/timeout behavior in the client, a human look would still be worthwhile.

What was reviewed: the new BackupDomainTransport/AsyncBackupDomainTransport fallback logic and connect-timeout capping in client.py; redirect-policy interaction between the outer httpx.Client/AsyncClient (follow_redirects, pre-existing asymmetry between sync/async) and the inner non-redirecting client; and the accompanying test_backup_domains.py coverage of fallback/no-fallback paths, streaming, and header/timeout preservation. One design point was checked and is not a correctness bug: each request independently re-attempts the primary host before falling back (no memoized "primary is down" state), so a sustained outage costs the connect-timeout ceiling (5s) per request rather than being remembered across calls.

Extended reasoning...

Overview

The PR adds a backup-domain fallback mechanism to fal_client's HTTP layer: a host-to-backup-host map, two new httpx transport classes (sync/async) that wrap an inner non-redirecting client, retry-on-ConnectError/ConnectTimeout logic that replays the request against a mapped backup host, a connect-timeout ceiling for mapped domains, and doc/comment updates reflecting the now-unbounded run() read timeout. It's confined to projects/fal_client/src/fal_client/client.py plus a new 512-line test file.

Security risks

No injection, auth-bypass, or data-exposure concerns: the backup host mapping is a small static dict, the Host header rewrite only targets known mapped hosts, and headers/auth are copied verbatim between primary and backup requests (not altered or logged — the log statement is host-only per the test asserting no path/secret leakage). No secrets or credential-shaped literals appear in the diff.

Level of scrutiny

This warrants a careful human review despite the extensive test suite, because it changes core network transport behavior for every request the client makes (new transport wrapping httpx.Client/AsyncClient, altered redirect handling boundary, timeout semantics change for run() becoming unbounded by default). Subtle bugs in retry/timeout/redirect interaction tend to surface only under real network conditions that unit tests with MockTransport can't fully replicate (e.g., interaction with actual DNS/TCP timing, proxies, or partial reads).

Other factors

The bug-hunting system ran to a dry streak and reported no bugs; I independently traced the transport code and confirmed the one candidate issue already investigated (no cross-request memory of primary-host failure, causing repeated ceiling-capped connect attempts under sustained outage) is a design tradeoff, not a functional bug — it degrades gracefully rather than breaking. The PR conversation timeline shows no reviewer feedback or CHANGES_REQUESTED to reconcile, and this is the first review pass. Test coverage is thorough (domain mapping, fallback triggers, non-fallback cases, streaming vs. mid-stream, timeout capping, redirect policy, header/log content), which raises confidence but doesn't eliminate the value of a human sanity check given the surface area touched.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to include this in GH actions or is it auto included?

Also, you can add a test where _BACKUP_DOMAINS has a non existent one like wrong.fal.run -> wrong.falrun.com so you can see that if the domain is just handled nowhere, how does it handle it?

Hard to replicate the case of domain being routed to an IP and then that IP refusing the connection.

Maybe we setup such an example in staging env?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the tests will be run by itself since all unit directory is declared to run. i also added a test that points to a non-existing primary and backup domain. and it just fails as expected

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 19e0418. Configure here.

Comment thread projects/fal_client/src/fal_client/client.py Outdated
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.

2 participants