Skip to content

feat(fal): route upload-policy uploads through the node-local uploader (PLAT-5890) - #1213

Merged
dennybritz merged 2 commits into
cdn-uploaderfrom
denny/plat-5890-route-upload-policy-uploads-through-the-node-local-uploader
Oct 12, 2026
Merged

dennybritz merged 2 commits into
cdn-uploaderfrom
denny/plat-5890-route-upload-policy-uploads-through-the-node-local-uploader

Conversation

@dennybritz

@dennybritz dennybritz commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Part of PLAT-5823 (bring-your-own storage). Linear: PLAT-5890. Stacked on #1182 (cdn-uploader). Needs the uploader side, fal-ai/isolate-cloud#10730.

Problem

A caller can send x-app-fal-upload-policy (an S3 presigned POST from boto3 generate_presigned_post) to have outputs written to their own bucket instead of the fal CDN. The SDK handles this in-process: it POSTs to the bucket from a background thread on the runner and returns the URL right away.

That check runs before repository selection, so on runners opted in to the node-local uploader (FAL_USE_LOCAL_UPLOADER=1) a policy upload never reaches the uploader. The runner keeps doing its own S3 POSTs, and those uploads are lost if the runner exits before they finish.

What this changes

When the runner is opted in, the policy upload is handed to the node-local uploader as a second kind of destination. The uploader spools the bytes on the node and POSTs them to the caller's bucket. There is no fal CDN copy.

File.from_bytes / from_path  (request carries x-app-fal-upload-policy)
  │
  ├─ checks that answer 422 before any upload (unchanged):
  │    size <= 5 GB, no "/" or "\" in the file name,
  │    signed Content-Type matches, app did not name another repository
  │
  ├─ FAL_USE_LOCAL_UPLOADER not set ──────────────► in-process POST (as today)
  │
  └─ POST /uploads on the uploader socket
       ├─ 202 ────────────────────────────────────► file_url from the uploader
       ├─ 429 x3, 401, socket missing ────────────► in-process POST
       ├─ 413 / 422 ──────────────────────────────► UploadPolicyError (424)
       └─ 5xx, connection lost mid-body ──────────► LocalUploadError, no retry

It never falls back to FalFileRepositoryV3: a caller who asked for their own bucket must not get fal storage.

The request

POST /uploads
Content-Type: text/plain
Content-Length: 5
X-App-Fal-Upload-Policy: {"url": "https://bucket.s3.amazonaws.com/", "fields": {"key": "outputs/${filename}", ...}}
X-Fal-File-Name: 00000000000000000000000000000007-cat.txt
X-Fal-Request-ID: <request id>
  • No Authorization or X-Fal-CDN-Token. An uploader that predates #10730 ignores the policy header. Without credentials it answers 401 and the SDK uploads in-process, instead of the old uploader writing the output to the fal CDN.
  • X-Fal-File-Name carries the uuid prefix. The uploader substitutes it for ${filename}, so the key and URL are the same ones the in-process path builds:
    https://bucket.s3.amazonaws.com/outputs/00000000000000000000000000000007-cat.txt

Non-ASCII file names

Header values must be ASCII: httpx raises UnicodeEncodeError on vidéo.mp4, and the uploader rejects non-ASCII headers with 400. On the policy path the SDK therefore percent-encodes the whole name, and the uploader decodes it before substituting ${filename} (being added to isolate-cloud#10730). Both paths store the same key:

file name X-Fal-File-Name key on both paths
cat.txt <uuid>-cat.txt <uuid>-cat.txt
vidéo (1).mp4 <uuid>-vid%C3%A9o%20%281%29.mp4 <uuid>-vidéo (1).mp4

The fal CDN path keeps rewriting non-ASCII characters to ?, which matches what the REST API does.

Code

  • providers/local.py: new save_with_policy(policy, file_name, content, content_type). It sends X-Fal-File-Name percent-encoded. It picks the uploader or the in-process path, as described above. file.py calls it in place of upload_bytes_with_policy / upload_path_with_policy. from_path still keeps small files' bytes for File.as_bytes() and streams large ones from disk.
  • _upload_policy.py: the name and Content-Type checks move out of _prepare_upload into _upload_file_name, so both paths run the same checks and use the same uuid naming.
  • _local_uploader.py: new LocalUploadInvalid for 413/422. It subclasses LocalUploadRefused, so the normal CDN path keeps falling back to direct CDN on those statuses as before.

Tradeoffs and gaps

  • Large outputs are sent before from_path returns. The uploader path streams the file over the local socket first. The in-process path copies it and uploads in the background.
  • ${filename} in other fields. The uploader substitutes it in every string field of the policy, while the in-process path only substitutes it in key. This only matters for policies that use ${filename} outside key.
  • 422 messages are generic. The 424 says Local uploader returned HTTP 422. and drops the uploader's detail (for example "Upload policy expired"), as the client does for all uploader errors.
  • The in-process path stays as a fallback until PLAT-5856 puts the uploader on every node.
  • feat(fal): read FAL_API_SOCKET and call /v1 uploader routes (PLAT-5971) #1211. It merges cleanly with this branch. Once it is folded into feat(fal): opt in to node-local SDK uploads #1182, the "/uploads" path assertions in the new unit tests become /v1/uploads. isolate-cloud#10730 also needs a rebase onto isolate-cloud main to serve the policy on the /v1 routes.

Testing

Unit tests (tests/unit/toolkit/file/test_local_uploader.py, 18 new cases) cover:

  • the policy upload reaching the socket, and its exact headers;
  • the URL matching what _prepare_upload builds for the same uuid;
  • each response above, including that it never calls FalFileRepositoryV3;
  • the input checks running before the handoff;
  • non-ASCII file names: vidéo (1).mp4 is sent as vid%C3%A9o%20%281%29.mp4, and the returned URL equals the in-process one.
pytest -n auto projects/fal/tests/unit/toolkit
pre-commit run --all-files

Integration tests run against a real cdn-uploader binary built from isolate-cloud#10730:

CDN_UPLOADER_TEST_BINARY=.../target/debug/cdn-uploader \
  pytest projects/fal/tests/integration/toolkit/test_local_uploader.py \
  -k "upload_policy or expired"
  • test_upload_policy_posts_to_the_caller_bucket (uses vidéo (1).txt, so it needs the decode in #10730): the real uploader accepts the request without fal credentials and returns the expected URL. It then connects to bucket.s3.us-west-1.amazonaws.com:443 and makes no CDN reservation.
  • test_expired_upload_policy_is_refused: an expired policy comes back as a 422 and is raised as UploadPolicyError.

The fake S3 endpoint is an HTTPS proxy that records which host the uploader connects to. It can't receive the POST body: the uploader only accepts HTTPS S3 hostnames and trusts only its bundled root certificates.

Not yet run: e2e on a preview against a real bucket.

Known failures on this machine, identical on the base branch:

  • test_serialized_sdk_reads_runner_environment and test_runner_exit_after_acceptance fail with "Failed to fetch user info" in their subprocess.
  • The two ?wait= integration tests fail because #10730 predates that endpoint.

…r (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.
@linear-code

linear-code Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

PLAT-5890

PLAT-5823

@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.

No new bugs found in this automated pass. Beyond the inline findings, I checked whether the local-uploader path streams large files synchronously on the request thread (vs. the old background-thread POST) and whether a 5xx/dropped connection now hard-fails instead of retrying — both are explicitly called out as intended tradeoffs in the PR description, not regressions, so I'm not raising them as findings. Given the size and sensitivity of this change (new upload destination routing, exception handling, header construction sent to an external uploader socket), a human look is still worthwhile.

Extended reasoning...

The change adds a second upload destination (node-local uploader) for policy-based (bring-your-own-bucket) uploads, refactors shared validation into _upload_file_name, and adds a new LocalUploadInvalid exception for 413/422 responses; it touches file.py, providers/local.py, _upload_policy.py and _local_uploader.py plus new unit/integration tests. Security-sensitive surface is limited to header construction (policy JSON, request ID, content-type) sent over a local Unix-socket HTTP client — no new network-facing auth or crypto logic, and the diff matches the PR's own described design. There are no outstanding third-party review objections in the available timeline, and no bugs were reported by the hunt beyond one dropped duplicate candidate.

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.
@dennybritz
dennybritz merged commit 683200e into cdn-uploader Oct 12, 2026
11 of 26 checks passed
@dennybritz
dennybritz deleted the denny/plat-5890-route-upload-policy-uploads-through-the-node-local-uploader branch October 12, 2026 08:08
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
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.

1 participant