Repository navigation
Feat youtube live integration - #20
Conversation
zZMathSP
left a comment
There was a problem hiding this comment.
Review of the YouTube Live integration. Two of the findings below (invalid YAML in the module config and the missing migration for existing databases) will break the deploy of this branch, so I'd hold the merge until those are fixed. The rest are ordered by severity.
| admin_token_file: "/data/admin_token.txt" | ||
| homeserver: "http://syapse:3000" | ||
| - module: modules.youtube_live_service.module.YoutubeLiveServiceModule | ||
| config: |
There was a problem hiding this comment.
Blocker: invalid YAML. config: is indented 8 spaces under a - module: key that sits at column 4, so it is not a sibling of module. PyYAML fails with mapping values are not allowed here and Synapse refuses to load the config. Since docker-compose runs --config-path /data/homeserver.yaml, the homeserver container will crash-loop on every deploy of this branch and take every existing module down with it, not just this one.
It should match the other entries:
- module: modules.youtube_live_service.module.YoutubeLiveServiceModule
config:
youtube_client_id: "..."| admin_token_file: "/data/admin_token.txt" | ||
| homeserver: "http://localhost:3000" | ||
| - module: modules.youtube_live_service.module.YoutubeLiveServiceModule | ||
| config: |
There was a problem hiding this comment.
Same indentation problem as in infra/synapse-docker/data/homeserver.yaml: config: must be at the same column as module:, otherwise this is not valid YAML.
| @@ -76,10 +76,16 @@ CREATE TABLE public.room_calendars ( | |||
| ALTER TABLE room_calendars OWNER TO <synapse_user>; | |||
|
|
|||
| CREATE TABLE IF NOT EXISTS room_streams ( | |||
There was a problem hiding this comment.
Blocker for existing environments. The new provider column and the nullable playback_url only exist here in 001_init.sql, which Postgres runs solely on first initdb (/docker-entrypoint-initdb.d). Any environment whose db_data volume is already populated will never get these changes.
On upgrade, db.get_stream runs SELECT playback_url, provider FROM room_streams and hits psycopg2.errors.UndefinedColumn, so every get_stream / set_stream call returns 500 and the fixed-channel Live feature that already works today breaks for all rooms.
We need a 002_*.sql (or equivalent) with ALTER TABLE room_streams ADD COLUMN provider ... and ALTER COLUMN playback_url DROP NOT NULL, and a way to apply it to running databases.
|
|
||
| # imports aqui dentro (não no topo do arquivo) para não exigir as | ||
| # libs do Google instaladas quando só o MockYoutubeClient é usado | ||
| from google.oauth2.credentials import Credentials |
There was a problem hiding this comment.
Missing dependencies. RealYoutubeClient imports google.oauth2.credentials and googleapiclient.discovery, but neither google-auth nor google-api-python-client is installed in infra/synapse-docker/Dockerfile or declared in pyproject.toml.
In production with all three credentials configured, the first start_broadcast raises ModuleNotFoundError inside the thread, which is swallowed by the bare except Exception in service.py and surfaces as a misleading 502 "Failed to create YouTube broadcast". Please add both packages to the image/pyproject.
| if client_id and client_secret and refresh_token: | ||
| logger.info("YoutubeLiveServiceModule: using RealYoutubeClient") | ||
| youtube_client = RealYoutubeClient(client_id, client_secret, refresh_token) | ||
| else: |
There was a problem hiding this comment.
Silent downgrade to the mock in production. Any partial misconfiguration (typo in a config key, secret mounted at the wrong path, unreadable file) only logs a WARNING and falls back to MockYoutubeClient, which returns HTTP 200 with a fabricated watch_url and stream_key.
Scenario: ops mounts the secret at the wrong path in prod, an admin clicks "Start broadcast", gets a plausible-looking RTMP key, pastes it into OBS, and the stream goes nowhere while the widget embeds a nonexistent video.
Note also that the 503 "YouTube integration not configured yet" path in service.py is unreachable: nothing ever raises NotImplementedError. I'd make the mock opt-in (e.g. youtube_use_mock: true) and fail startup, or at least return 503, when credentials are incomplete.
| from twisted.internet.threads import deferToThread | ||
|
|
||
| try: | ||
| info = await deferToThread(self._create_broadcast_sync, title) |
There was a problem hiding this comment.
This awaits a raw twisted.internet.threads.deferToThread Deferred. Synapse already exposes ModuleApi.defer_to_thread (see synapse/module_api/__init__.py), which wraps the call in make_deferred_yieldable and uses Synapse's thread pool.
Bypassing it leaks log contexts across requests: under concurrency you'll see Expected logging context ... but found ... errors and request_id attribution in logs becomes wrong for unrelated requests handled while the Google call is in flight. Suggest passing api into the client and using await self.api.defer_to_thread(self._create_broadcast_sync, title).
| from google.oauth2.credentials import Credentials | ||
| from googleapiclient.discovery import build | ||
|
|
||
| credentials = Credentials( |
There was a problem hiding this comment.
_get_client lazily builds and caches a single googleapiclient Resource (backed by one httplib2.Http) that is then shared across worker threads. httplib2 / googleapiclient objects are documented as not thread-safe, and the lazy init itself has no lock.
Two admins starting broadcasts at the same moment can trigger a double build() (one client discarded mid-use) or share the same HTTP connection, producing interleaved socket reads (HttpLib2Error, garbled JSON, IncompleteRead) that show up as random 502s exactly when several rooms go live together. Building a fresh client per call (or per thread), as the googleapiclient thread-safety guide recommends, avoids this.
| "room_id": "!r:localhost", | ||
| "playback_url": "https://cdn.example/live/r.m3u8", | ||
| } | ||
| svc.store.db_pool.runInteraction.return_value = None |
There was a problem hiding this comment.
The rewrite dropped five existing tests without replacement:
get_streamreturning a saved URL (the happy path)get_stream(None)→ 400set_streamwithroom_id=None→ 400- whitespace-only
playback_url→ 400 - the
assert_not_awaited/assert_awaited_oncechecks that the DB is not touched on 403
Without them, a future change that drops the room_id guard, stops stripping whitespace, or writes to the DB before the admin check would pass CI unnoticed, and the new SELECT ... provider shape has no happy-path coverage at all. Could we bring them back (adapted to the new return shape)?
| try: | ||
| with open(path, "r") as f: | ||
| return f.read().strip() | ||
| except FileNotFoundError: |
There was a problem hiding this comment.
_read_secret_file is the fourth copy of this helper (room_features_service/module.py, vod_service/module.py and schedule_service/module.py have their own), and this one only catches FileNotFoundError. A secret mounted as a directory or with root-only permissions raises IsADirectoryError / PermissionError at module init and aborts Synapse startup with a traceback instead of the intended warn-and-fallback.
Suggest extracting a single shared read_secret_file under modules/ (catching OSError) and using it in all four modules.
There was a problem hiding this comment.
Agreed, a shared helper makes sense. Would it be okay if I did it in a separate PR?
My concern is the admin token: the admin user is created automatically when Synapse starts, and its token is written to a file at that moment. So the file doesn't exist yet when the modules load, and they wait up to 30s for it to appear. A plain "read the file" helper might break that, if I understood the flow correctly.
The YouTube secrets are created by hand before starting, so a plain read should be enough there. It now catches OSError, so an unreadable file no longer stops Synapse from starting.
What do you think about building the helper with a "wait for the file" option, and migrating the other modules in a follow-up? They already run in production, so I'd like to keep that change separate. I'll create a card on Basecamp for it.
| **Errors:** | ||
| - `400` — missing `room_id` or `title`. | ||
| - `403` — the caller is not an admin. | ||
| - `503` — YouTube credentials not configured yet (running in mock/dev mode). |
There was a problem hiding this comment.
The README documents behaviour the code doesn't have:
- 503 when credentials are missing: in practice the mock is selected and returns 200 with fake data (see comment on
module.py). - The test file is named
test_real_youtube_client.pywith 2 tests here, but the actual file istest_youtube_client.pywith 6. RealYoutubeClientis described as verified end-to-end, while the PR description says it is blocked bylivePermissionBlocked.
A frontend dev coding the "integration not configured" UX against the documented 503 will never see it. Please align the README with the actual behaviour, or fix the code to match.
Beyond the review: changes that followed from the persistence fixWhile addressing the
Caveats on (3):
Out of scope, still open: an admin who closes the modal can't retrieve the |
Live streaming: YouTube integration (create and end broadcasts)
Summary
Adds a second way to power a room's Live: instead of only pointing at a manually-configured playback URL (the existing "fixed channel" model), a room can be set to create a YouTube broadcast automatically every time someone starts a transmission, and to end it on YouTube when the transmission is stopped. Both models coexist; the choice is per room.
The frontend is in element-web element-hq#21.
What's new
youtube_live_service(new module)Both endpoints are admin-only, same pattern as the rest of the system (creating a room/space, enabling features, configuring the fixed channel).
POST /_synapse/youtube_live_service/start_broadcast(room_id,title)provider = 'youtube'(viaroom_streams_service) before contacting YouTube:404if the room has no stream configuration,400if it uses another provider. This avoids creating orphan broadcasts on the company channel.liveBroadcasts.insert(the "event"),liveStreams.insert(the ingestion pipe, where YouTube generates the RTMP server and stream key) andliveBroadcasts.bind.broadcast_id/watch_urlon the room. If that write fails the request still returns200(logged at ERROR): the broadcast already exists on YouTube, and withholding the stream key would be worse than losing the association.Returns
watch_url(to build the widget) plusingestion_addressandstream_key(for the streamer to paste into OBS). The module never touches OBS; that step is inherently manual, same as with any streaming provider.POST /_synapse/youtube_live_service/stop_broadcast(room_id)Looks up the room's active
broadcast_id, ends it on YouTube(
liveBroadcasts.transition→complete) and, only if that succeeds, clears the local record. If YouTube refuses, the record is kept: clearing it anyway would make Element report the room as live-free while the broadcast could still be on air.400if there is no active broadcast (YouTube is not called).If YouTube rejects the transition with
reason = invalidTransition, it is treated as success. The docs define that reason only as "can't transition from its current status", so it covers both a broadcast that already ended (encoder disconnected, ended in YouTube Studio) and one that never went live. Nothing ison air in either case. Any other error, including an unreadable error body, surfaces as
502and the record is kept.Errors:
400,403,404(start only),502(the YouTube call failed),503(integration not configured).Client selection.
module.pypicks one of three clients:youtube_use_mock: true→MockYoutubeClient(fake data, a WARNING on every use; local development only).youtube_client_id,youtube_client_secret_fileandyoutube_refresh_token_fileall present and readable →RealYoutubeClient.UnconfiguredYoutubeClient: logs an ERROR naming the missing keys and both endpoints return503. It never falls back to the mock silently, so a secret mounted at the wrong path can't hand an admin a fake stream key. Synapse still starts.Threading. The Google client library is synchronous. The call goes through
ModuleApi.defer_to_thread(Synapse's thread pool, which keeps the request's logging context) instead ofrun_in_executor(unreliable on Twisted's reactor) or a rawdeferToThread. A fresh Google client is built on every call, sincegoogleapiclient/httplib2objects are not thread-safe.Dependencies.
google-authandgoogle-api-python-clientare declared inpyproject.toml(a clean virtualenv install pulls both in).room_streams_service: two providers, and sole owner ofroom_streamsprovidercolumn (fixed|youtube, defaultfixed).playback_urlis nullable and only required whenprovider = 'fixed'(check constraint);youtuberooms don't store a fixed URL.youtube_broadcast_id/youtube_watch_urlcolumns hold the broadcast currently associated with the room (both null or both set). Without them the live had no server-side record: if the response was lost, nobody could recover the watch URL, andget_streamkept reporting no live.get_streamreturns the new fields.set_streamvalidatesplayback_urlonly forfixed, and clears the two YouTube fields whenever a room's configuration changes, so a stale broadcast is never reported as current.room_streamsyoutube_live_servicegoes through three methods on its service instance:require_youtube_room,set_youtube_broadcastandclear_youtube_broadcast.clear_youtube_broadcastis deliberately not exposed over HTTP. It only clears the local record, so a standalone endpoint would let any admin put the record out of sync with YouTube; the only caller isstop_broadcast, after YouTube confirms.api._hs.room_streams_service, the same patternroom_serviceuses, soroom_streams_servicemust be listed beforeyoutube_live_serviceinhomeserver.yaml.YoutubeLiveServiceModuleraisesRuntimeErrorat startup otherwise.Database
001_init.sql:provider, nullableplayback_url, the YouTube columns and their constraints (room_streams_provider_check,room_streams_fixed_requires_url,room_streams_youtube_fields_match), for fresh environments.002_add_stream_provider.sqland003_youtube_broadcast_columns.sql: the equivalent migrations for existing databases, idempotent (safe to run twice).001_init.sqlonly runs on the first initdb, so these are needed on any database that already has data.Deploy notes
002if the table predatesprovider, then003. The productionroom_streamswas created already withprovider(Infra Dummy issue element-hq/synapse#80), so there it only needs003.get_stream/set_streamfail withUndefinedColumnand the fixed-channel Live breaks for every room, not only the new path.client_secretandrefresh_tokenfollow the same file pattern asadmin_token_file. They should come from a Buzzlabs Google account, and the OAuth app needs to be published: in "Testing" mode the refresh token expires after 7 days.Configuration
homeserver.yamlandhomeserver.yaml.exampleare updated with the entry.Tests
room_streams_service: 21 tests. Both providers, required-field validation, the admin check (including that the DB is not touched when it fails),require_youtube_room's three outcomes,set_youtube_broadcastandclear_youtube_broadcast.youtube_live_service: 43 tests. Start (room validation before calling YouTube, persistence, a persistence failure not failing the request,503), the Google client (request bodies, thread-pool dispatch, one client per call), client selection (mock only when opted in, a secret at the wrong path gives503, not the mock) andstop_broadcast(a YouTube failure keeps the record, onlyinvalidTransitioncounts as success).Verification status
invalidTransition, which is handled as described above.Notes and open points
privacyStatusis hardcoded to"unlisted". Worth revisiting: a paid Live'swatch_urlis directly accessible outside the platform via YouTube itself (access control only applies within Element).read_secret_filehelper will come in a separate PR (Basecamp card).