feat(wma): add experimental WebRTC session apps (fal.wma) - #1153
feat(wma): add experimental WebRTC session apps (fal.wma)#1153noahgsolomon wants to merge 6 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
76e35bd to
9102eea
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also looked at RunnerIceConfig.build_ice_servers_async mutating self.status without a lock across concurrent session negotiations (projects/fal/src/fal/wma/ice.py) — it's used purely for status reporting/telemetry, not as correctness-critical state, so a race there yields a stale reported status rather than a security or session-handling bug.
Extended reasoning...
The inline finding (AiortcPeer's initial_connect_timeout_seconds defaulting to None, silently disabling the stall-detection watchdog described in _raw.py) is a real, confirmed issue and is already flagged inline, so it is not restated here. I additionally examined the RunnerIceConfig.status mutation pattern flagged as a candidate in the prior investigation: build_ice_servers_async writes self.status at several branches (ICE_STATUS_MISCONFIGURED, ICE_STATUS_TURN, ICE_STATUS_STUN_ONLY, ICE_STATUS_UNREACHABLE) with no lock, and RunnerIceConfig instances can be shared/long-lived across concurrent sessions. Reading the surrounding code, self.status is only consumed for external status reporting (e.g. exposed for observability/telemetry), not for branching logic that would affect session correctness or security — so a race here can only produce a momentarily stale or overwritten status value, not an exploitable bug. This matches the prior ruled-out assessment and I confirm it independently from the code.
Port the connection-oriented WMA surface from fal-ai/registry @ bf407fcae (noah/wma-app-contracts) into a self-contained experimental subpackage. Users subclass fal.wma.App, implement create_backend(), and deploy with `fal deploy`; the browser side is the wma() extension for fal.realtime.open() in @fal-ai/client. Surface: App/Session/SessionParams/PeerBackend/AiortcPeer and the StartSessionRequest/SessionAnswer negotiation contract (one POST /start-session SSE endpoint owns the session lifetime), RunnerIceConfig (bridge/env/server ICE resolution) with Metered TURN helpers, RealtimeContract rendering to OpenAPI x-fal-realtime + AsyncAPI 3.1, bounded connection_report telemetry, and deferred billing (Session.add_billable_units settles once at close). Severed registry edges as private modules: _errors.py (wire-identical 422/500 shapes and billing/retry headers), _ssrf.py (vendored is_globally_routable_ip for the ICE candidate filter), _request_id.py (x-fal-request-id UUID canonicalization), _billing.py (deferred-billing REST report loop). Compatibility: importable and tested across py3.8-3.14 and pydantic v1/v2 (typing generics at runtime-evaluated sites, fal.compat.run_in_thread, asyncio.wait_for, lazy TypeAdapter — only RealtimeContract rendering requires pydantic v2). aiortc stays a lazy session-time import; wrap_app injects WMA_APP_REQUIREMENTS (aiortc>=1.9,<2) into WMA runner envs via a sys.modules check, so `import fal` never imports fal.wma. Also fixes a latent wrap_app bug this made observable: cls.requirements (often the shared App.requirements class default) was passed by reference and extended in place by add_requirements, leaking injected requirements across apps wrapped in the same process. Now copied. fal.wma is experimental: APIs may change in a minor release, and it is deliberately not exported from fal/__init__.py.
9102eea to
363fb85
Compare
| create_default_channel: bool = True, | ||
| rtc_configuration: Any = None, | ||
| peer_connection_factory: Callable[[], Any] | None = None, | ||
| disconnected_grace_seconds: float | None = 0, |
There was a problem hiding this comment.
Default disconnect ends sessions early
Medium Severity
AiortcPeer defaults disconnected_grace_seconds to 0, so any connectionState == "disconnected" immediately sets the closed event and ends the SSE session. The raw helper documents the opposite default (None): treat disconnected as a transient ICE blip, matching client policy. The documented AiortcPeer(session, on_connect) usage therefore tears down recoverable sessions on brief network jitter.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit b7a50bd. Configure here.
CI runners have no globally routable interface, so with iceServers=[] every host candidate was stripped by the SSRF filter and the test timed out. Pin host discovery to loopback and bypass the candidate classifier inside this one test (its strictness is pinned by dedicated tests), so the end-to-end negotiation is deterministic and offline on any runner.
| await session.close() | ||
| ice.Connection.close = original_ice_close | ||
| ice.get_host_addresses = original_get_host_addresses | ||
| _raw.is_globally_routable_ip = original_classifier |
There was a problem hiding this comment.
Monkeypatches can leak across tests
Medium Severity
The new process-global patches to ice.get_host_addresses and _raw.is_globally_routable_ip (which disables the SSRF candidate filter) are applied before the try and restored only after await client.close() / await session.close(). A failure during setup or either close leaves host discovery pinned to loopback and the SSRF classifier always-true for the rest of the worker, which can cascade into unrelated ICE/security tests. Nearby aioice teardown tests restore globals in an outer finally around the whole scenario instead.
Reviewed by Cursor Bugbot for commit 2399f7f. Configure here.
A module-level urllib OpenerDirector holds an SSLContext on Python 3.12+, and the whole fal package is cloudpickled by value to runners at deploy — so any app referencing fal.wma.metered failed to serialize with "cannot pickle SSLContext object". Mints happen at most once per cache TTL, so per-call construction costs nothing measurable.
ada8594 to
0d4c726
Compare
| # the intermediate WMA base's inherited value ("app"), every concrete | ||
| # subclass ignores both ``name=...`` and its own class-derived default. | ||
| cls.app_name = None | ||
| super().__init_subclass__(**kwargs) |
There was a problem hiding this comment.
Subclass app_name cleared too aggressively
Medium Severity
The new __init_subclass__ always assigns cls.app_name = None before calling fal.App.__init_subclass__. That fixes the poisoned inherited "app" from the WMA base, but also wipes a subclass app_name ClassVar and any name inherited from an intermediate WMA base. fal.App normally preserves those via getattr(cls, "app_name") or app_name, so WMA apps can silently deploy under the wrong name when using the ClassVar or inheritance patterns that work for plain fal.App.
Reviewed by Cursor Bugbot for commit 41f097f. Configure here.
sdk.py carried `from __future__ import annotations`, which stringifies every annotation. fal apps are cloudpickled to runners, and cloudpickle ships only the globals referenced by code — so the endpoint signature's names (Optional, the request model) never reached the runner, FastAPI's dependency resolution hit an unresolvable ForwardRef, and every fal.wma.App /start-session answered 500 at the first request (verified against a live deployment; works locally because the module is really imported there). The registry original omitted the future import for exactly this reason. Annotations are now real objects evaluated at def time: typing forms where 3.8 can evaluate them, explicit strings for runtime-unsafe subscripts (asyncio.Task) in non-FastAPI positions. A regression test pins that no start_session annotation is a string.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 40683d5. Configure here.
| request: StartSessionRequest, | ||
| *, | ||
| caller_user_id: Optional[str] = None, | ||
| request_id: Optional[str] = None, |
There was a problem hiding this comment.
Constructors crash on older Python
High Severity
Dropping postponed annotation evaluation makes instance-attribute hints run at construction time. Several self.* annotations still use PEP 585 subscripting and PEP 604 unions, so creating Session, SessionParams, or AiortcPeer raises TypeError on the SDK's Python 3.8 and 3.9 floor. Function signatures were converted; these attribute annotations were not.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 40683d5. Configure here.
2 participants
What
Adds
fal.wma— an experimental subpackage for connection-oriented WebRTC session apps (world models, live video transforms, realtime avatars). Users subclassfal.wma.App, implementcreate_backend(), and deploy withfal deploy; the browser connects through the WMA bridge via thewma()extension forfal.realtime.open()in@fal-ai/client(branchnoah/realtime-wmain fal-js).Ported from
fal-ai/registry@bf407fcae(noah/wma-app-contracts), whoseregistry.wmapackage was written as a forward-compatible shim for exactly this move. Two commits: ① byte-for-byte port, ② adaptations — review effort belongs on ②.Experimental: APIs may change in a minor release.
fal.wmais deliberately not exported fromfal/__init__.py;import falis unchanged.Surface
App/Session/SessionParams/PeerBackend/AiortcPeer/StartSessionRequest/SessionAnswer— onePOST /start-sessionSSE endpoint owns the session lifetimeRunnerIceConfig(from_bridge/from_env/from_server) + Metered TURN helpers — bring-your-own ICE for apps outside fal's managed-TURN allowlistRealtimeContract→ OpenAPIx-fal-realtimediscovery + standalone AsyncAPI 3.1 (pydantic v2 only, lazily gated)connection_reporttelemetry (v1)Session.add_billable_units()→x-fal-billable-units-webhook: 1→ one settle report at closeAdaptations (commit 2)
_errors.py(422 atbody.sdp, billing-safe 500s),_ssrf.py,_request_id.py,_billing.pytypinggenerics at runtime-evaluated sites,fal.compat.run_in_thread,asyncio.wait_for, lazyTypeAdapterwrap_appinjectsWMA_APP_REQUIREMENTS = ["aiortc>=1.9,<2"]into WMA runner envs (theREALTIME_APP_REQUIREMENTSpattern); detection viasys.modules, so the package loads only when importedwrap_appbug this made observable:cls.requirements(often the sharedApp.requirementsdefault) was passed by reference and extended in place byadd_requirements, leaking injected requirements across apps in one process. Now copied.Verification
test_file_syncerrors reproduce on cleanmain)make docsbuilds, wheel containsfal/wma/Rollout context
noah/wma-app-contractsshould land in registry first so the port source is settled; registry then swapsregistry.wmainternals forfal.wmare-exports (net −2k lines) after the next fal release.noah/realtime-wma(client 1.11.0 + server-proxy) ships alongside.🤖 Generated with Claude Code
Note
Medium Risk
Adds a large experimental WebRTC/billing surface and runner dependency injection; mitigations are opt-in import, input validation on ICE/billing paths, and isolated changes to wrap_app/discovery.
Overview
Introduces
fal.wma, an opt-in experimental package for long-lived browser↔runner WebRTC sessions overPOST /start-session(SSE answer, then held open until teardown). Subclasses implementcreate_backend(); the SDK suppliesSession,AiortcPeer, ICE/TURN plumbing (bridge-forwarded, Metered env, or app-owned providers), wire-shaped errors, deferred session billing, connection telemetry, and optional OpenAPI/AsyncAPI contracts.Core fal integration:
wrap_appno longer mutates sharedhost_kwargs/requirementson the app class (fixes leaked injected deps across multiple wraps); WMA subclasses getaiortc>=1.9,<2in the runner env unless they already pinaiortc. Module discovery ignores the re-exportedfal.wma.Appbase so importing it does not count as a deployable app. Test extras add optionalaiortcfor negotiation tests.Security-oriented behavior in the new stack includes SDP candidate filtering (SSRF), UUID validation before billing REST paths, and strict validation of forwarded/Metered ICE URLs. An aioice teardown shim reduces orphaned STUN retry noise on failed negotiations.
Reviewed by Cursor Bugbot for commit 40683d5. Bugbot is set up for automated code reviews on this repo. Configure here.