Repository navigation
feat(fal): opt in to node-local SDK uploads - #1182
Conversation
* feat(fal): opt in to node-local SDK uploads * docs(fal): keep local uploader details internal * fix(fal): prevent fallback after local upload failures
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.
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked LocalFileRepository.save_file for a stat/stream size race (local.py:113) — os.path.getsize() is read before the file is reopened for streaming, but since falls_back=False on this repository, a size mismatch there was ruled out as a real risk for the intended single-write-then-upload runner workflow rather than a new bug.
Extended reasoning...
This PR adds an opt-in node-local upload path (LocalUploader, LocalFileRepository, save_stream) touching file upload headers, a new falls_back/no-retry semantic, and Unix-socket protocol code; two confirmed findings (UnicodeEncodeError on non-ASCII file names, and an incorrect upload-policy-conflict rejection when repository=LocalFileRepository() is passed explicitly) are already queued as inline comments. I additionally reviewed the save_file size-race candidate (stat then reopen) and treated it as ruled out given the expected write-then-upload usage pattern rather than a newly introduced defect worth flagging again. No third-party CHANGES_REQUESTED review is visible in the timeline, but given the confirmed findings a human should still land the fixes before merge.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
projects/fal/src/fal/toolkit/file/file.py— Apps that pass repository=LocalFileRepository() explicitly (a supported, tested pattern, e.g. test_errors_do_not_retry_or_fall_back) get a caller's upload-policy override wrongly rejected, unlike apps using the default "fal_v3" string or an explicit FalFileRepositoryV3(). _DEFAULT_REPOSITORY_TYPES (file.py:99) lists only "FalFileRepositoryV3", so _is_default_repository (file.py:105) treats LocalFileRepository as a foreign destination and _require_no_repository_conflict raises UploadPolicyInputError naming "LocalFileRepository" to the caller, even though it is the very destination the env-var path treats as the default fal_v3 target. …Why this was flagged
…Fix: have _is_default_repository also recognize LocalFileRepository (or, more generally, whatever get_builtin_repository substitutes for "fal_v3") as default-equivalent.
Trigger: an app explicitly constructs File.from_bytes/from_path(..., repository=LocalFileRepository()) and a caller request also carries the x-fal-upload-policy header (get_upload_policy(request) is not None). _require_no_repository_conflict (file.py:108) calls _is_default_repository(repository); since type(repository).name == "LocalFileRepository" is not in _DEFAULT_REPOSITORY_TYPES = {"FalFileRepositoryV3"} (file.py:99), it returns False and the function raises UploadPolicyInputError. On base/main, an app using the equivalent FalFileRepositoryV3() object or the "fal_v3" string id would pass this check via _DEFAULT_REPOSITORY_TYPES or _DEFAULT_REPOSITORY_IDS and let the caller's policy proceed. The safeguard this bypasses is the intended one: _DEFAULT_REPOSITORY_TYPES exists precisely to treat pickle-identity-unstable stand-ins for the default destination as default, but LocalFileRepository,…
Verification: nit. The mechanism is real.
_DEFAULT_REPOSITORY_TYPES = frozenset({"FalFileRepositoryV3"})(file.py:99) omitsLocalFileRepository, so_is_default_repository(file.py:105:type(repository).__name__ in _DEFAULT_REPOSITORY_TYPES) returns False for aLocalFileRepository()instance, and_require_no_repository_conflict(file.py:114 -> 120) raisesUploadPolicyInputErrorat the two call…
* refactor(fal): define the local uploader socket directory * refactor(fal): keep only the consumed socket default
…ck (#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
…ctions 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.
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.
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.
…itory 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.
…d 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.
* 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
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.
Move the session protocol into _stream next to _accept so both public functions read the same way, and give _request explicit keyword arguments.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6eb8bb3. Configure here.
…r (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.
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.
2 participants
Resolves PLAT-5240.
SDK uploads currently keep the runner waiting for remote storage. This adds an internal, opt-in path to the node-local uploader:
File.from_path("/tmp/video.mp4")returns once the node has durably accepted the complete file, while the uploader finishes the CDN transfer in the background.Everything is gated by
FAL_USE_LOCAL_UPLOADER. With the variable unset,0,false, or empty, uploads still useFalFileRepositoryV3. The flag is read on the runner at upload time.Upload path
The
fal_v3registry factory selects the repository. Deprecatedfal_v2andcdnIDs resolve through the same factory. Explicit repository objects keep their chosen destination. A caller upload policy still takes precedence over the default destination, including an explicitly constructedLocalFileRepository().LocalFileRepositoryadapts the existing bytes/file methods to_local_uploader.upload(file_name, content, headers). The client accepts bytes or aPath, sendsContent-Length, and waits for the uploader'saccepted_localreceipt./run/fal-upload/upload.sock;CDN_UPLOADER_SOCKET_PATHoverrides it. Proxy settings are ignored.FileData, preservingFile.as_bytes()behavior. Files selected for multipart handling stream in explicit 64 KiB reads. This bounds reads even on older supported httpx versions. Each rejected file submission reopens the source from byte zero.?, control characters become?, and surrounding whitespace is removed. For example,vidéo.mp4is sent asvid?o.mp4.Settings needing account defaults or recipient lookup remain available to the uploader's REST reservation path.
Retry and fallback
Fallback is allowed only when the SDK knows the uploader did not take the work. It uses
FalFileRepositoryV3with the original save arguments.LocalUploadError; do not submit the file again.Errors name the HTTP status or transport exception class without echoing response bodies, signed URLs, or credentials.
LocalUploadError.falls_back = Falsealso stops the outerFilefallback chain. This matters because the usualFile.from_bytesandFile.from_pathcalls otherwise retry against the legacyfalrepository. The exception attribute survives toolkit serialization, so the same rule applies on the runner. Definite refusals permit fallback; other repository errors keep their existing fallback behavior.For example, with the flag enabled:
A missing socket uses direct V3 upload. A lost acceptance reply raises: the node may already be transferring the file, so resubmitting could create a second copy. This trades availability on uncertain failures for avoiding automatic duplicate uploads.
Generated streams
For output whose size is unknown until it is fully produced,
LocalFileRepository.save_streamuses the uploader's session API. Nothing calls it yet; it is there for a future public entry point such asFile.from_stream.DELETE /upload-sessions/s-1and re-raises the original error.wait_for_completion=Truewaits after finish, the same way as for bytes and files.Limits and follow-ups
LocalFileRepository.save_stream, which has no publicFileentry point yet.Validation
The toolkit and serialization suites pass: 495 passed, 1 skipped (missing
GCLOUD_SA_JSON). The 74 local-uploader/socket tests also pass with httpx 0.20.0. All pre-commit hooks pass.Tests cover routing, authentication and metadata, bounded file reads, 429 replay, missing-httpx fallback, and uncertainty through the public
Fileentrypoints, plus the stream session sequence and abort on producer failure. Against a realcdn-uploaderbuilt from isolate-cloud main (CDN_UPLOADER_TEST_BINARY), the 5 integration tests pass. They include a generated stream that waits for CDN completion. The socket tests serialize the toolkit in a separate process, then verify both successful acceptance and a dropped reply after the server receives the complete body. The dropped-reply case must raise without calling another repository.Optional process tests use a locally built Rust uploader and controlled HTTP storage peer to check 80 and 101 MiB uploads after caller exit, hashes, and full-queue fallback. They were not rerun for this fix because no built uploader binary was available:
Note
Medium Risk
Changes default CDN upload routing and duplicate-upload semantics on runners with the flag enabled; fallback behavior is carefully gated but upload failures after local acceptance no longer retry automatically.
Overview
Adds an opt-in node-local upload path for the Python SDK (
FAL_USE_LOCAL_UPLOADER), so defaultfal_v3uploads can hand bytes/files to a Unix-socket HTTP uploader and return once the node has durably accepted the payload (CDN transfer continues in the background).Routing: New
_local_uploaderclient (POST /uploads, session API for unknown-size streams, optionalwait_for_completionpolling) andLocalFileRepositoryadapter.LocalUploadRefused(connect failures, 429 after brief replay, missing httpx) falls back to directFalFileRepositoryV3; uncertain failures setfalls_back = FalsesoFile’s repository fallback chain does not duplicate uploads.Upload policy:
Filepolicy saves go throughsave_with_policy, which can delegate to the local uploader when opted in (policy JSON in headers, no fal CDN creds), with in-process S3 POST as fallback._upload_file_nameis factored out of_prepare_uploadfor shared validation/naming.Tests cover routing, refusals vs hard errors, policy handoff, streams, serialization/socket behavior, and optional integration against the Rust
cdn-uploaderbinary.Reviewed by Cursor Bugbot for commit bbd031c. Bugbot is set up for automated code reviews on this repo. Configure here.