fix(client): adopt the runner's re-scoped CDN token from the queue result - #1136
Draft
hanchchch wants to merge 1 commit into
Draft
fix(client): adopt the runner's re-scoped CDN token from the queue result#1136hanchchch wants to merge 1 commit into
hanchchch wants to merge 1 commit into
Conversation
…sult ``handle.get()`` fetched the result and returned ``response.json()`` without looking at the headers, so the queue path silently dropped the ``x-fal-cdn-token`` the runner returns. Since ``subscribe`` is built on ``submit`` + ``handle.get()``, every chaining app that uses it lost the token. The result response is the only one that can carry it. ``submit`` already calls ``handle_response_headers``, but on the *enqueue* response -- written before the runner has run -- so its scope can never cover the request ids the run ended up with access to. This matters once CDN tokens are request-scoped: an inner app's output is tagged with the *inner* request id, so the outer app's own token does not grant read on it. The runner returns a token whose scope accumulated every hop, the gateway forwards it upstream (``_build_worker_response_headers`` treats the worker's token as overriding the platform default), and the caller has to adopt it. That last step was missing here. Verified against prod: both ``fal.run`` and the queue result endpoint return ``x-fal-cdn-token`` with ``read:request_id:<that run's id>``. Design: "CDN Access Control", Request-Scoped Access / Flow step 4.
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
1 participant
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
handle.get()fetched the queue result and returnedresponse.json()without looking at the headers, so the queue path silently dropped thex-fal-cdn-tokenthe runner returns.subscribeis built onsubmit+handle.get(), so every chaining app using it lost the token.submitdoes callhandle_response_headers— but on the enqueue response, written before the runner has even run. Its scope can never cover the request ids the run ended up with access to. Only the result response can.Why it matters
Once CDN tokens are request-scoped, an inner app's output is tagged with the inner request id, so the outer app's own token grants no read on it. The design ("CDN Access Control", Request-Scoped Access → Flow step 4) handles this by having the runner return a token whose scope accumulated every hop, which the gateway forwards upstream. That's needed rather than gateway-side reconstruction because in an A→B→C chain the gateway only knows the hop it proxied.
Everything except this step is already in place:
_get_cdn_tokenfal.Appmiddleware_build_worker_response_headers— worker's token overrides the platform defaulthandle_response_headersrun/stream, ✗ queue pathVerified against prod by forwarding a token as the caller: both
fal.runand the queue result endpoint returnx-fal-cdn-token, scopedread:request_id:<that run's id>.Change
One call in each of
SyncRequestHandle.get()andAsyncRequestHandle.get(), after_raise_for_status.handle_response_headersis already a no-op outside a fal app context, so non-app SDK users are unaffected.Testing
TestQueueResultAdoptsCdnToken— sync and async adoption, no-token-in-response leaves the existing token untouched, and no-app-context does not raise.pytest tests/unit→ 148 passedpre-commit run --files ...→ clean (ruff, ruff-format)Not covered —
fal.appshas no token handling at allfal.apps.run/submitsend onlycreds.to_headers(): they neither forwardx-fal-cdn-tokenon the way out nor adopt it on the way back. This PR does not reach them.Four registry callers use that path:
registry/video/fast_svd.pyfal-ai/fast-sdxlregistry/video/fast_svd_lcm.pyfal-ai/fast-sdxlregistry/fine_tuning/personalization/inference.pyfal-ai/moondream/batchedregistry/image/supir/inference.pyfal-ai/llava-nextThe two
fast-sdxlchains keep working post-flip precisely because the intermediate is not caller-scoped, but the caller does not own their own intermediate and it is world-readable. Suggested follow-up is migrating those callers toregistry.client(which forwards, see fal-ai/registry#12943) rather than teachingfal.appsthe protocol — one less client to maintain.Part of INFRA-4420.