Repository navigation
fix(fal): retry definite uploader rejections before direct CDN fallback - #1199
Conversation
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the retry/fallback logic across three upload paths (save, save_file, save_stream) in a critical file-upload code path, a human look would still be worthwhile.
What was reviewed:
- New
LocalUploadRejectedmarker parsing (HTTP 503 +rejectionfield) and its scoping to onlyqueue_full/draining, leaving other 503s conservative (acceptance_uncertain) - Replay semantics: byte bodies, file bodies (seek(0) + fresh iterator per attempt), and unknown-size stream sessions (retry only before body is sent)
- Fallback to
FalFileRepositoryV3insave/save_file/save_stream, including the stream-to-tempfile spooling path, which only happens after rejection and before the producer is touched - Checked whether unknown-size streaming uploads could leak an unhandled
LocalUploadRejected—save_streamitself catches it and falls back, so this is handled; checked whether asave()-internal direct-CDN failure bypasses the caller's ownfallback_repository=['fal']list — it does, but that list's'fal'entry is the same repository already attempted, so no fallback capability is actually lost
Extended reasoning...
The diff adds an explicit-rejection exception type and retry/fallback logic (3 attempts with 0.1s/0.2s backoff) across LocalFileRepository.save, save_file, and save_stream, including a spool-to-tempfile path for unknown-size streams, in a core opt-in local-CDN upload path with no auth/crypto surface touched. Test coverage is substantial (unit, integration against a real Rust uploader binary, and a Unix-socket subprocess scenario) and the replay/retry semantics check out against the code; no CODEOWNERS restricts this path and no prior reviewer feedback is outstanding (no prior review events in the timeline). Despite finding nothing wrong after a thorough multi-round bug hunt, the change is non-trivial reliability-critical logic spanning several code paths, so a human sanity check is still warranted before merge.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs in the retry/fallback logic. Because it changes how node-local upload rejections are retried and fall back to direct CDN (including auth and multipart-option handling), and the approach was substantially refactored since an earlier review of this PR, a human look would still be worthwhile.
What was reviewed:
_retry_or_fallbackinproviders/local.py: 3 attempts with 0.1s/0.2s backoff before callingFalFileRepositoryV3directly, matching the documented policy;LocalUploadRejectedis scoped to explicit HTTP 429 only, other failures keep existingacceptance_uncertainhandling.- The CDN fallback path requires
fetch_auth_credentials()to succeed, so a request authorized only via a per-requestX-Fal-CDN-Token(no FAL_KEY) still fails on fallback — but this matches pre-PR behavior where no fallback existed at all, so the overall request outcome is unchanged, only the error differs. save_file's non-multipart branch now forwardsmultipart/threshold/chunk/concurrency kwargs tosuper().save_fileso the fallback path keeps the caller's multipart settings.
Extended reasoning...
The diff touches fal/toolkit/file/_local_uploader.py (unix-socket HTTP client, classifies 429 as a new LocalUploadRejected) and fal/toolkit/file/providers/local.py (adds a retry/fallback decorator around save/save_file that replays the whole upload up to 3 times then falls back directly to the CDN repository), plus matching unit/integration tests; it touches auth-header construction indirectly (credential lookup on the fallback path) but no injection or permission surface. Three candidate concerns (auth-only-via-CDN-token on fallback, same-body replay on every 429, full-body replay for large files) were checked against the code and PR description and found to match intended, documented behavior rather than regressions. I'm deferring rather than approving because this is a second review of the PR with a substantial refactor since the last pass, and the prior review's content isn't available to me to confirm its concerns were addressed.
* feat(fal): opt in to node-local SDK uploads (#1181) * feat(fal): opt in to node-local SDK uploads * docs(fal): keep local uploader details internal * fix(fal): prevent fallback after local upload failures * refactor(fal): route local uploads through the repository resolver Move the FAL_USE_LOCAL_UPLOADER check into get_builtin_repository, where repository ids already resolve, and replace the isinstance no-fallback guard with a FileRepository.falls_back class attribute that survives pickle-by-value. Restore the _try_with_fallback name. LocalFileRepository.save_file now reads small files once and delegates to save, streaming only above the multipart threshold. The socket client gets a single _new_client factory, verify=False on the Unix transport, and loses the unreachable size validator, response URL re-validation, and the test-only status_code attribute. Tests patch the client factory instead of httpx globally, split the branching error parametrize, and wait on a server-side event instead of wrapping the client class. * feat(fal): add unexposed stream upload on the local repository LocalFileRepository.save_stream uploads output whose size is unknown until the producer is exhausted. It carries the same caller headers as save and save_file, counts bytes as they stream, and finishes with the count so the uploader can reject a truncated body. A producer that raises aborts the session. Reachable only through the local repository; File has no stream entry point yet. * fix(fal): percent-encode file names that cannot travel in a header httpx encodes header values as ASCII, so a non-ASCII file name raised UnicodeEncodeError with FAL_USE_LOCAL_UPLOADER on while the REST path, which sends the name in a JSON body, accepted it. The uploader and the CDN reject non-ASCII header bytes too, so encode on the client. Only bytes outside printable ASCII are encoded; ordinary names are sent unchanged, as the Rust client sends them. * fix(fal): name the transport error class in local upload failures A timeout, a peer close, and a write failure all read as "connection was interrupted", so logs could not tell them apart. Include the httpx exception class name; the exception text stays out because it can carry credentials or signed URLs. * test(fal): document and verify the uploader socket default (#1198) * refactor(fal): define the local uploader socket directory * refactor(fal): keep only the consumed socket default * fix(fal): retry definite uploader rejections before direct CDN fallback (#1199) * fix(fal): retry definite local upload rejections before direct CDN * refactor(fal): replay upload bodies through one local client * test(fal): trim redundant large-file fallback assertions * refactor(fal): simplify definite rejection retries and fallback * test(fal): expect definite 429 rejections from the uploader * test(fal): consolidate rejection fallback coverage * refactor(fal): replace the local uploader session object with two functions The streaming session had one caller that always ran begin, body and finish in order, so its state flags and misuse guards protected nothing. upload() and upload_stream() now open their own client, count streamed bytes, and abort on any failure. Reuse the shared retry helper for 429 replays and let httpx read large files directly. * refactor(fal): upload small files without a nested retry layer save_file read small files through the base class into the retrying save, so a rejection was replayed inside the outer save_file retry. Upload the bytes directly; each entry point now has exactly one retry and fallback. * refactor(fal): fall back to direct CDN on any local uploader failure Any uploader error now uploads through FalFileRepositoryV3 after the 429 retries, so a missing socket or lost response no longer fails the save. This removes the FileRepository.falls_back flag and the acceptance uncertainty tracking, which only existed to block fallback. A lost response can now publish a second URL. * fix(fal): let caller upload policies override an explicit local repository LocalFileRepository is what FAL_USE_LOCAL_UPLOADER substitutes for the default destination, so passing it explicitly must not reject a caller's upload policy as a foreign-destination conflict. * fix(fal): preserve local upload fallback and bound file reads * fix(fal): only fall back to direct CDN when the local uploader refused the work Sanitize file names as the REST API does so both upload paths publish the same name. Raise instead of re-uploading after the uploader may have accepted the bytes. Replay rejections around the request only, read the file once per attempt, and build headers from the V3 repository. Select the local repository from the fal_v3 registry factory. Drop the unused stream API and overlapping tests. * fix(fal): stop repository fallback after uncertain local acceptance * feat(fal): let local uploads wait for CDN completion (PLAT-5925) (#1208) * feat(fal): let local uploads wait for CDN completion (PLAT-5925) save_kwargs={"wait_for_completion": True} makes LocalFileRepository long-poll the uploader's GET /uploads/{id}?wait= until the CDN has the file. A failed or unknown transfer falls back to a direct CDN upload; streams raise. * refactor(fal): make wait_for_completion keyword-only in the local uploader client * feat(fal): add generated-stream uploads back to the local repository LocalFileRepository.save_stream drives the uploader's session API: reserve, stream the body, then finish with the counted byte count. Any failure, including a raising producer, aborts the session; streams never fall back because the producer cannot be replayed. wait_for_completion applies after finish. Uploads and sessions share one request classifier. * refactor(fal): mirror upload and upload_stream around one session helper Move the session protocol into _stream next to _accept so both public functions read the same way, and give _request explicit keyword arguments. * feat(fal): route upload-policy uploads through the node-local uploader (PLAT-5890) (#1213) * feat(fal): route upload-policy uploads through the node-local uploader (PLAT-5890) Opted-in runners hand outputs carrying x-app-fal-upload-policy to the node-local uploader, which POSTs them to the caller's bucket. The policy is forwarded without fal credentials, so an uploader without policy support refuses it instead of writing to the fal CDN. Refusals upload in-process as before; a 413 or 422 from the uploader is raised as UploadPolicyError. * fix(fal): percent-encode policy upload file names for the local uploader Header values must be ASCII, so non-ASCII names were rewritten to "?" and keyed differently from the in-process path. The uploader decodes the name, so both paths now store the same key and return the same URL. The fal CDN path keeps the "?" rewrite that matches the REST API. * test(fal): merge overlapping local uploader failure tests Fold the refusal and possibly-accepted tables into one classification table. Drop the 429 row, the explicit-repository dimension, the missing-socket test and the real-socket 429 replay, which repeat other tests. * test(fal): isolate local uploader socket tests from user config * test(fal): inline sleep patch target in upload policy fixture
With
FAL_USE_LOCAL_UPLOADER=1,File.from_path("out.mp4")currently fails when the node-local uploader is full or draining. Those requests were never accepted, so retrying or uploading directly to CDN is safe. A lost response or storage failure can already have accepted work and must stop instead.This combines PLAT-5704 and PLAT-5748, stacked on #1182 with base
cdn-uploader. The companion HTTP status change is isolate-cloud#10637.Behavior
HTTP 429 raises
LocalUploadRejected.saveandsave_fileshare one retry policy: try locally three times, waiting 100 ms and 200 ms, then callFalFileRepositoryV3directly with the original arguments. This bypasses the local-uploader flag and preserves multipart and lifecycle settings.Every attempt repeats the complete save operation. Bytes are reusable; large files reopen at the beginning. Small files retain their data for
File.as_bytes(). The low-level HTTP client classifies responses without implementing retry policy.Generic 503s, lost responses, and invalid acceptance receipts stop immediately, even after a 429.
falls_back=Falsestill prevents File's outer fallback list from creating a second URL. Missing sockets and other validation/authentication failures keep their existing behavior.Scope
Unknown-size streams retain the parent PR's behavior: no retries, temporary-file buffering, or direct CDN fallback. This removes the extra stream workflow, replay factories, and rejection JSON protocol. Older uploaders still return 503 for capacity and remain conservative until the companion backend update is deployed.
Validation
GCLOUD_SA_JSONunset).pre-commit run --all-filespassed, including mypy.Run locally with
FAL_CONFIG_PATHpointing to an empty config and a dummyFAL_KEY, plusPYTHONPATH=projects/fal/src:projects/isolate_proto/src:projects/fal_client/src:projects/fal/openapi-fal-rest, thenpython -m pytest projects/fal/tests/unit/toolkit -q. The real-uploader regression usesCDN_UPLOADER_TEST_BINARY=<isolate-cloud>/monorust/target/debug/cdn-uploaderand a controlled local storage peer; it needs no cloud credentials.Note
Medium Risk
Changes file upload routing when the local uploader rejects work, but retries are limited to explicit 429s and direct CDN fallback preserves prior safeguards against duplicate URLs on uncertain outcomes.
Overview
When the node-local uploader is full or draining, uploads no longer fail outright: HTTP 429 is treated as a definite admission rejection via new
LocalUploadRejected, while ambiguous failures (503, lost connections, uncertain acceptance) still stop immediately.LocalFileRepository.save/save_filenow retry the full local upload up to three times (100 ms / 200 ms backoff), then bypass the local uploader and callFalFileRepositoryV3with the same multipart and lifecycle options. The low-levelLocalUploaderclient only classifies 429; it does not implement retries. Unknown-sizesave_streambehavior is unchanged.Tests cover replay-then-CDN for bytes/small/large files, halting when acceptance becomes uncertain, integration with a real uploader binary, and serialized-runner socket retries.
Reviewed by Cursor Bugbot for commit 09b873e. Bugbot is set up for automated code reviews on this repo. Configure here.