Repository navigation
test(fal): document and verify the uploader socket default - #1198
Merged
dennybritz merged 2 commits intoOct 5, 2026
Merged
dennybritz merged 2 commits into
dennybritz merged 2 commits into
Conversation
dennybritz
changed the title
dennybritz
marked this pull request as ready for review
October 5, 2026 01:30
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good, straightforward documentation and test-coverage addition.
What was reviewed: the comment added above DEFAULT_SOCKET_PATH (purely documentation, no behavior change) and the new test_client_socket_path test, which correctly exercises _new_client() with the CDN_UPLOADER_SOCKET_PATH env var set and unset against the real default socket path string.
Extended reasoning...
The diff touches only a comment in _local_uploader.py and adds one parametrized unit test asserting the httpx transport's uds kwarg matches the expected socket path for both the env-override and default cases. No security-sensitive surface (auth, crypto, permissions) is touched, and the change is small, self-contained, and mechanical with no logic modifications. The test logic itself is correct and matches the existing _new_client implementation's precedence of CDN_UPLOADER_SOCKET_PATH over DEFAULT_SOCKET_PATH.
dennybritz
deleted the
denny/plat-5707-use-the-volume-helper-and-share-socket-paths-for-the
branch
October 5, 2026 01:44
dennybritz
added a commit
that referenced
this pull request
Oct 13, 2026
* 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The SDK default
/run/fal-upload/upload.sockmust match the controller mount and Nomad uploader job, but its definition does not identify where those equivalents are maintained. Existing socket tests only exercise an environment override.Change
Add a comment beside
DEFAULT_SOCKET_PATHpointing to the controller and infra definitions. Keep the existing single default constant; the SDK has no consumer for a separate mount-directory constant.The new test checks the Unix transport receives
/run/fal-upload/upload.sockwhen the environment is unset, and the override whenCDN_UPLOADER_SOCKET_PATHis set. This preserves resolution when the client is created on the runner.Validation
pre-commit run --all-files, including mypy: passed.Stacked on #1182. Tracks PLAT-5707. Companion PRs: controller #10632, infra #16510.
CI status
Pre-commit and six unit variants pass. The Python 3.13 unit run reproduces the parent PR's
FakeGauge.incmiddleware leak. Integration jobs reproduce missing Python executables in cached remote environments, and e2e jobs reproduce backend lifecycle and timeout failures seen on main. Evidence: parent unit failure, main integration failure, main e2e failures. These remain outside this change.