Repository navigation
feat(fal): read FAL_API_SOCKET and call /v1 uploader routes (PLAT-5971) - #1211
Conversation
There was a problem hiding this comment.
Looks good — straightforward, mechanical rename of the socket env var/path and REST routes to the versioned /v1 scheme, with matching test updates. Reviewed the client in _local_uploader.py (env var read, default socket path, all REST call sites: /v1/uploads, /v1/upload-sessions[...], /v1/uploads/{id}), confirmed no leftover references to CDN_UPLOADER_SOCKET_PATH or the old /run/fal-upload/upload.sock path anywhere in the repo, and checked that the three test files (unit, unit-socket, integration) consistently assert the new names/paths.
Extended reasoning...
The change touches only projects/fal/src/fal/toolkit/file/_local_uploader.py and its three test files: it renames an env var (CDN_UPLOADER_SOCKET_PATH -> FAL_API_SOCKET), changes a default Unix socket path, updates a sync-with comment, and adds a /v1 prefix to three REST paths used over a local Unix socket. No new security-sensitive surface is introduced (same trust boundary as before — a local socket), and a repo-wide grep confirms no other code or docs still reference the old env var or socket path. Tests were updated in lockstep to match, including a new path-capture assertion in the fake HTTP handler. Size and risk are small and mechanical, which supports approval.
The SDK finds the fal API socket through FAL_API_SOCKET, default /run/fal/api.sock, and calls /v1/uploads and /v1/upload-sessions.
099ec9f to
a6eea62
Compare
…ead-fal_api_socket-and-call-v1-routes # Conflicts: # projects/fal/src/fal/toolkit/file/_local_uploader.py
…from the uploader
Problem
fal#1182 sends SDK uploads through the node-local uploader. As written, it finds the uploader under uploader-specific names and calls unversioned routes:
PLAT-5965 turns that socket into the generic fal API socket, so later node-local features can share it. The uploader (isolate-cloud#10889, merged) already serves both sockets, and serves its routes both with and without
/v1. If fal#1182 shipped as is, a released SDK would depend on the old names, and they could never be removed.Change
The SDK reads only
FAL_API_SOCKETand calls/v1routes:CDN_UPLOADER_SOCKET_PATHFAL_API_SOCKET/run/fal-upload/upload.sock/run/fal/api.sockPOST /uploadsPOST /v1/uploadsGET /uploads/{id}?wait=60GET /v1/uploads/{id}?wait=60/upload-sessions[/{id}/body|/finish]/v1/upload-sessions[/{id}/body|/finish]The old variable is not read at all, so there is no fallback path.
Rollout
This branch is stacked on
cdn-uploader(fal#1182) and must stay off it until PLAT-5970 (the controller mounts/run/faland setsFAL_API_SOCKET) is fully rolled out:cdn-uploaderbranch. On a runner started by the old controller there is no/run/fal, so uploads would go straight to the CDN, andsave_stream(), which has no fallback, would raise.6eb8bb3and is not affected.Fold this into fal#1182 before fal#1182 merges, so no released SDK reads
CDN_UPLOADER_SOCKET_PATHor calls unversioned routes.Testing
Unit tests (
pytest -n auto projects/fal/tests/unit/toolkit): 489 passed. They assert the new env var, default path and/v1routes, includingPOST /v1/uploadsover a real Unix socket.Integration tests against the real uploader built from isolate-cloud
main, which bindsFAL_API_SOCKETand serves/v1/health:cargo build -p cdn-uploader # in isolate-cloud/monorust CDN_UPLOADER_TEST_BINARY=.../target/debug/cdn-uploader \ pytest projects/fal/tests/integration/toolkit/test_local_uploader.py5 passed.
Note
Medium Risk
Changes how every local upload discovers the socket and which API paths are called; misaligned runners or old daemons without
/v1will refuse or fall back to CDN instead of the node uploader.Overview
The node-local upload client now targets the shared fal API Unix socket instead of the uploader-only socket. It reads
FAL_API_SOCKET(default/run/fal/api.sock) and no longer honorsCDN_UPLOADER_SOCKET_PATH.All uploader HTTP calls use
/v1paths (/v1/uploads,/v1/upload-sessions, status polling, and integration/v1/health). Tests were updated to match, treat 404 as a refusal when/v1is missing, and add coverage that a missing socket (including when only the old env var is set) falls back to direct CDN upload.Reviewed by Cursor Bugbot for commit 495eb77. Bugbot is set up for automated code reviews on this repo. Configure here.