-
Notifications
You must be signed in to change notification settings - Fork 175
fix: Fatal Server Error when creating room in video component #1409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds defensive None-handling around room configuration lookups in WebSocket flows and pre-warms the channel relationship on room creation to prevent async ORM lazy-loading crashes during audit logging and broadcasting. Sequence diagram for room creation and AuditLog with pre-warmed channelsequenceDiagram
actor User
participant Frontend
participant ApiServer
participant RoomService
participant ORM
participant AuditLogService
User->>Frontend: Click create room
Frontend->>ApiServer: POST /rooms with with_channel=true
ApiServer->>RoomService: create_room(event, data, creator)
RoomService->>RoomService: _create_room(data, with_channel=True, creator)
RoomService->>ORM: Room.objects.create(...)
ORM-->>RoomService: room
RoomService->>ORM: Channel.objects.create(event_id=room.event_id, room=room)
ORM-->>RoomService: channel
RoomService->>RoomService: room.channel = channel
RoomService->>AuditLogService: AuditLog.objects.create(..., data={"room": room}, ...)
AuditLogService->>ORM: Serialize room including channel
ORM-->>AuditLogService: Serialized data (no lazy-load)
AuditLogService-->>RoomService: AuditLog entry created
RoomService-->>ApiServer: room details
ApiServer-->>Frontend: 200 OK room info
Frontend-->>User: Room created and UI updates
Sequence diagram for WebSocket room info push with None-safe config lookupsequenceDiagram
actor User
participant Frontend
participant WebSocketConsumer
participant RoomService
participant RoomRepository
participant PermissionService
User->>Frontend: Open room page
Frontend->>WebSocketConsumer: send push_room_info {room: room_id}
WebSocketConsumer->>RoomService: get_room_config_for_user(room_id, event_id, user)
RoomService->>RoomRepository: get_room(id=room_id, event_id=event_id)
alt Room_not_yet_available
RoomRepository-->>RoomService: None
RoomService-->>WebSocketConsumer: None
WebSocketConsumer->>WebSocketConsumer: conf is None, return
WebSocketConsumer-->>Frontend: No room_info broadcast
else Room_available
RoomRepository-->>RoomService: room
RoomService->>PermissionService: room.event.get_all_permissions(user)
PermissionService-->>RoomService: permissions
RoomService->>RoomService: get_room_config(room, permissions[room] | permissions[room.event])
RoomService-->>WebSocketConsumer: conf
WebSocketConsumer->>WebSocketConsumer: Check conf[permissions] contains room:view
WebSocketConsumer-->>Frontend: send_json room_info
end
Sequence diagram for WebSocket schedule data push with None-safe config lookupsequenceDiagram
actor User
participant Frontend
participant WebSocketConsumer
participant RoomService
participant RoomRepository
participant PermissionService
User->>Frontend: View room schedule
Frontend->>WebSocketConsumer: send push_schedule_data {room: room_id}
WebSocketConsumer->>RoomService: get_room_config_for_user(room_id, event_id, user)
RoomService->>RoomRepository: get_room(id=room_id, event_id=event_id)
alt Room_not_yet_available
RoomRepository-->>RoomService: None
RoomService-->>WebSocketConsumer: None
WebSocketConsumer->>WebSocketConsumer: config is None, return
WebSocketConsumer-->>Frontend: No schedule_data broadcast
else Room_available
RoomRepository-->>RoomService: room
RoomService->>PermissionService: room.event.get_all_permissions(user)
PermissionService-->>RoomService: permissions
RoomService->>RoomService: get_room_config(room, permissions[room] | permissions[room.event])
RoomService-->>WebSocketConsumer: config
WebSocketConsumer->>WebSocketConsumer: Check config[permissions] contains room:view
WebSocketConsumer-->>Frontend: send_json schedule_data
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Now that
get_room_config_for_usercan returnNone, consider updating its type hints (and any associated callers) to reflect the optional return type so this new behavior is explicit to future readers. - When skipping broadcasts in
push_room_infoandpush_schedule_databecause the room config isNone, consider adding lightweight logging so that unexpected missing rooms can be diagnosed without impacting users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `get_room_config_for_user` can return `None`, consider updating its type hints (and any associated callers) to reflect the optional return type so this new behavior is explicit to future readers.
- When skipping broadcasts in `push_room_info` and `push_schedule_data` because the room config is `None`, consider adding lightweight logging so that unexpected missing rooms can be diagnosed without impacting users.
## Individual Comments
### Comment 1
<location> `app/eventyay/base/services/event.py:363-365` </location>
<code_context>
async def get_room_config_for_user(room: str, event_id: str, user):
room = await get_room(id=room, event_id=event_id)
+ if room is None:
+ return None
permissions = await database_sync_to_async(room.event.get_all_permissions)(user)
</code_context>
<issue_to_address>
**suggestion:** `get_room_config_for_user` now returns `None` in some cases; consider tightening type hints and callers to reflect the optional return.
Since the function can now return `None`, its return type should be updated to `Optional[...]`, and all callers should be checked (beyond `push_room_info` and `push_schedule_data`) to ensure they correctly handle the `None` case. Making this optionality explicit in the signature reduces the risk of future callers assuming a non-`None` config.
Suggested implementation:
```python
from typing import Optional # if there's already a typing import, extend it instead of adding a new line
async def get_room_config_for_user(room: str, event_id: str, user) -> Optional[dict]:
room = await get_room(id=room, event_id=event_id)
if room is None:
return None
permissions = await database_sync_to_async(room.event.get_all_permissions)(user)
return get_room_config(room, permissions[room] | permissions[room.event])
```
1. If `event.py` already has a `from typing import ...` line, extend it instead of adding a second import, e.g.:
- `from typing import Any, Dict, Optional`
2. Replace `dict` in the return type with the concrete type used by `get_room_config` if it already has a more precise annotation, for example:
- `-> Optional[RoomConfig]`
- or `-> Optional[Dict[str, Any]]`
3. Update all call sites of `get_room_config_for_user` (beyond `push_room_info` and `push_schedule_data`) to handle the `None` case explicitly, for example:
- Early-return if the config is `None`.
- Or branch logic to only use the config when it is non-`None`.
4. If you use a static type checker (e.g. mypy), run it to confirm all callers now respect the `Optional[...]` return type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| async def get_room_config_for_user(room: str, event_id: str, user): | ||
| room = await get_room(id=room, event_id=event_id) | ||
| if room is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: get_room_config_for_user now returns None in some cases; consider tightening type hints and callers to reflect the optional return.
Since the function can now return None, its return type should be updated to Optional[...], and all callers should be checked (beyond push_room_info and push_schedule_data) to ensure they correctly handle the None case. Making this optionality explicit in the signature reduces the risk of future callers assuming a non-None config.
Suggested implementation:
from typing import Optional # if there's already a typing import, extend it instead of adding a new line
async def get_room_config_for_user(room: str, event_id: str, user) -> Optional[dict]:
room = await get_room(id=room, event_id=event_id)
if room is None:
return None
permissions = await database_sync_to_async(room.event.get_all_permissions)(user)
return get_room_config(room, permissions[room] | permissions[room.event])- If
event.pyalready has afrom typing import ...line, extend it instead of adding a second import, e.g.:from typing import Any, Dict, Optional
- Replace
dictin the return type with the concrete type used byget_room_configif it already has a more precise annotation, for example:-> Optional[RoomConfig]- or
-> Optional[Dict[str, Any]]
- Update all call sites of
get_room_config_for_user(beyondpush_room_infoandpush_schedule_data) to handle theNonecase explicitly, for example:- Early-return if the config is
None. - Or branch logic to only use the config when it is non-
None.
- Early-return if the config is
- If you use a static type checker (e.g. mypy), run it to confirm all callers now respect the
Optional[...]return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical server crash that occurred when creating new video rooms (Stage, Chat, or BBB), which previously caused the frontend to hang in a perpetual loading state. The fix addresses two root causes: a lazy-loading ORM conflict during audit log serialization and missing None handling in WebSocket broadcast logic.
Key Changes:
- Pre-warm the channel relationship after room creation to prevent lazy-loading issues during async serialization
- Add defensive None checks in room config retrieval and WebSocket handlers to gracefully handle transient lookup failures
- Skip WebSocket broadcasts when room configuration is temporarily unavailable instead of crashing
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
app/eventyay/base/services/event.py |
Pre-warms the channel relationship after creation and adds None check in get_room_config_for_user to handle room lookup failures |
app/eventyay/features/live/modules/room.py |
Adds None checks in push_room_info and push_schedule_data WebSocket handlers to gracefully skip broadcasts when room config is unavailable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I am still not able to create video rooms after the merge. |
* Restructure configuration * More settings * More settings * More settings * More settings * Use fixed DATA_DIR * Specify how to provide secret settings * Add more settings * Delete reference to settings.TALK_CONFIG * Set default running environment in some entry files * Fix Docker build * Fix missing software in Docker build * Fix Docker build for prod * No need to copy egg-info in Docker build * Delete duplication in settings * Update README with the guide for this new configuration system * Print the list of configration files with colors * Add .env support for config file * Ignore .env file Also print that we load .env file. * Add "process" cache store * Receover PRETIX_ADMIN_AUDIT_COMMENTS * Recover PRETIX_xxx settings * Fix crashing when encountering unsupported language * Remove the --exclude option from Docker "COPY" command * Do not add __pycache__ to the docker build * Disable VueCompiler, not needed anymore * Rework Dockerfile/entrypoint jobs * move some python manage.py calls into Dockerfile to speed up generation of files. * Keep compress in entrypoint.prod.sh. * work around the fact that we need to have static.dist on a shared volume in docker (compose) so that nginx can share the files. * Bring back VueCompiler Because we still have Vue 2 code. * feature: added hidden rooms and ensure sync (#1184) * added unscheduled rooms * implemented suggestions * added suggestions by copilot * fix(migrations): add missing migrations into base/0002 --------- Co-authored-by: Sak1012 <[email protected]> * fix: add JSON_FIELD_AVAILABLE setting and fix Event.items reference - Add JSON_FIELD_AVAILABLE setting based on database backend (postgresql = True) - Fix checkinlists exporter using old Event.items instead of Event.products - Resolves AttributeError when accessing export functionality * fix rooms not working on video (#1339) Co-authored-by: Srivatsav Auswin <[email protected]> Co-authored-by: Mario Behling <[email protected]> * Added translation using Weblate (Persian) * Added translation using Weblate (Indonesian) * Added translation using Weblate (Japanese) * Added translation using Weblate (Korean) * Added translation using Weblate (Malay) * Added translation using Weblate (Vietnamese) * fix(translations): remove old translation files and merge redundant files (#1370) * fix(locale): Remove old translation files introduced by weblate The updated translation files are already available in the new `enext` system under `app/eventyay/locale`. * fix(locale): Merge redundant language files * fix: Fatal Server Error when creating room in video component (#1409) * room creation null fix (#1414) * add logging for room problem (#1415) * add logging * add defaults via migration * fix(migrations): Add missing migrations (#1426) * room complete setup pass (#1425) * chore: save room fix try (#1429) * save room fix try and added logging * implemented suggestions --------- Co-authored-by: Mario Behling <[email protected]> * Don't install Rollup to "static" dir This Rollup is for Django-Compressor. This "static" folder is for JS/CSS files that we will copy to Nginx serving folder. It must not contain node_modules or JS tools. * Update Makefile to install Rollup to new location * Add missing 'crispy_forms_tags' * Update pydantic-settings * Delete the frontend built product files from "static" folder * Delete duplicate static files * Use "data/compiled-frontend" folder as output for Vue 3 apps * Recover datetimepicker SCSS and JS files They are needed by pretixcontrol app. * Delete out-of-sync DB migration files. These files are left after "enext" branch was force-pushed. * Fix package-lock compatibility * Revert unwanted changes due to enext force-push * Fix ouput dir for building Vue 3 app in Dockerfile.prod * Fix installing tool to build webcheckin in Dockerfile.prod * Temporarily not run compilemessages in Dockerfile.prod * Delete more duplicate static files * Recover and relocate static files under "eventyay-common" * Add diagram of how static files should be in folders * Prevent DisallowedHost raised from MultiDomainMiddleware * Don't install JS tools to "static" folder when building Docker * Update `DJANGO_SETTINGS_MODULE` to new value in entire code base * Set default `EVY_RUNNING_ENVIRONMENT` env var for Celery * Changes to the deployment setup * Raise the priority of env var for configuration --------- Co-authored-by: Norbert Preining <[email protected]> Co-authored-by: Saksham Sirohi <[email protected]> Co-authored-by: Sak1012 <[email protected]> Co-authored-by: Arnav Angarkar <[email protected]> Co-authored-by: Srivatsav Auswin <[email protected]> Co-authored-by: Mario Behling <[email protected]> Co-authored-by: abbas davarpanah <[email protected]> Co-authored-by: suhail nadaf <[email protected]>
Summary
Fixes a crash that occurred when creating new rooms (Stage, Chat, or BBB) in the video component, which caused the frontend to remain in a perpetual loading state.
Root Cause
Two related issues triggered the failure:
AttributeErrorif room lookup temporarily returned None due to transaction timing.Fixes Implemented
Pre-warm
room.channelafter creation to avoid lazy-loading inside async flow.Added None checks in:
get_room_config_for_user()to safely handle transient lookup failurespush_room_info,push_schedule_data) to skip broadcasts when room config is unavailableNotes
Issue was more visible in production due to async behavior, connection pooling, and concurrency patterns different from local environments.
Related
Closes #1343
Related: #1339
Includes: #1379
Summary by Sourcery
Prevent crashes and loading hangs when creating new video rooms by hardening room creation and WebSocket room lookups.
Bug Fixes:
Enhancements: