Make dual-mode supervisor port configurable#237
Make dual-mode supervisor port configurable#237DL6ER wants to merge 2 commits intoselkies-project:mainfrom
Conversation
The supervisor API server (/switch, /status) was hardcoded to port 8082, which collides with the data websocket server's default port (also 8082). Users enabling SELKIES_ENABLE_DUAL_MODE=true would either get a bind error in selkies, or had to manually set CUSTOM_WS_PORT to a different value to avoid the collision. This adds a `supervisor_port` setting (default 8083) with an env-var override `SELKIES_SUPERVISOR_PORT`, and uses it in `run()` instead of the hardcoded value. Default 8083 matches the existing control_port default and avoids the collision out of the box. Users who currently rely on `CUSTOM_WS_PORT=8081` to work around the conflict can keep that, or switch to the cleaner default. No backwards-incompatible behaviour for single-mode setups.
There was a problem hiding this comment.
Pull request overview
This PR makes the dual-mode supervisor API server port configurable via settings, so dual-mode can bind the /switch and /status endpoints on a non-hardcoded port.
Changes:
- Add a new
supervisor_portsetting intended to control the dual-mode supervisor API bind port. - Update
run()insrc/selkies/__main__.pyto usesettings.supervisor_portinstead of a hardcoded port. - Update logging to print the configured supervisor port.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/selkies/settings.py | Adds a supervisor_port setting definition. |
| src/selkies/main.py | Uses settings.supervisor_port when starting the supervisor API task and logs the chosen port. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if settings.enable_dual_mode[0]: | ||
| api_task = asyncio.create_task(create_api_server(manager, host='localhost', port=8082)) | ||
| logger.info("Dual mode enabled: Supervisor API server started on port 8082") | ||
| supervisor_port = settings.supervisor_port |
| # Server Startup & Operational Settings | ||
| {'name': 'port', 'type': 'int', 'default': 8081, 'env_var': 'CUSTOM_WS_PORT', 'help': 'Port for the data websocket server.'}, | ||
| {'name': 'control_port', 'type': 'int', 'default': 8083, 'help': 'Port for the internal control plane API.'}, | ||
| {'name': 'supervisor_port', 'type': 'int', 'default': 8083, 'env_var': 'SELKIES_SUPERVISOR_PORT', 'help': 'Port for the dual-mode supervisor API (handles /switch and /status endpoints when enable_dual_mode is true).'}, |
| # Server Startup & Operational Settings | ||
| {'name': 'port', 'type': 'int', 'default': 8081, 'env_var': 'CUSTOM_WS_PORT', 'help': 'Port for the data websocket server.'}, | ||
| {'name': 'control_port', 'type': 'int', 'default': 8083, 'help': 'Port for the internal control plane API.'}, | ||
| {'name': 'supervisor_port', 'type': 'int', 'default': 8083, 'env_var': 'SELKIES_SUPERVISOR_PORT', 'help': 'Port for the dual-mode supervisor API (handles /switch and /status endpoints when enable_dual_mode is true).'}, |
| # Server Startup & Operational Settings | ||
| {'name': 'port', 'type': 'int', 'default': 8081, 'env_var': 'CUSTOM_WS_PORT', 'help': 'Port for the data websocket server.'}, | ||
| {'name': 'control_port', 'type': 'int', 'default': 8083, 'help': 'Port for the internal control plane API.'}, | ||
| {'name': 'supervisor_port', 'type': 'int', 'default': 8083, 'env_var': 'SELKIES_SUPERVISOR_PORT', 'help': 'Port for the dual-mode supervisor API (handles /switch and /status endpoints when enable_dual_mode is true).'}, |
| {'name': 'port', 'type': 'int', 'default': 8081, 'env_var': 'CUSTOM_WS_PORT', 'help': 'Port for the data websocket server.'}, | ||
| {'name': 'control_port', 'type': 'int', 'default': 8083, 'help': 'Port for the internal control plane API.'}, | ||
| {'name': 'supervisor_port', 'type': 'int', 'default': 8083, 'env_var': 'SELKIES_SUPERVISOR_PORT', 'help': 'Port for the dual-mode supervisor API (handles /switch and /status endpoints when enable_dual_mode is true).'}, |
There was a problem hiding this comment.
Code Review
This pull request replaces the hardcoded port for the dual-mode supervisor API with a configurable supervisor_port setting. Feedback indicates that the current implementation will cause an AttributeError at runtime because the setting was added to the websocket-specific configuration instead of common settings. Additionally, the default port value of 8083 conflicts with the existing control_port, which would lead to binding failures if both features are enabled.
| if settings.enable_dual_mode[0]: | ||
| api_task = asyncio.create_task(create_api_server(manager, host='localhost', port=8082)) | ||
| logger.info("Dual mode enabled: Supervisor API server started on port 8082") | ||
| supervisor_port = settings.supervisor_port |
There was a problem hiding this comment.
This access will raise an AttributeError at runtime because supervisor_port was only added to the websockets settings definition (SETTING_DEFINITIONS_WEBSOCKETS), but __main__.py imports and uses settings_webrtc. The definition in src/selkies/settings.py must be moved to COMMON_SETTING_DEFINITIONS to be available in both modes.
| # Server Startup & Operational Settings | ||
| {'name': 'port', 'type': 'int', 'default': 8081, 'env_var': 'CUSTOM_WS_PORT', 'help': 'Port for the data websocket server.'}, | ||
| {'name': 'control_port', 'type': 'int', 'default': 8083, 'help': 'Port for the internal control plane API.'}, | ||
| {'name': 'supervisor_port', 'type': 'int', 'default': 8083, 'env_var': 'SELKIES_SUPERVISOR_PORT', 'help': 'Port for the dual-mode supervisor API (handles /switch and /status endpoints when enable_dual_mode is true).'}, |
There was a problem hiding this comment.
There are two issues with this addition:
- Port Collision: The default value
8083is identical to the defaultcontrol_port(line 116). If a user enables both the dual-mode supervisor and the secure control plane (by providing amaster_token), the two API servers will attempt to bind to the same port, causing a startup failure. I suggest using8084as the default. - Incorrect Scope: As noted in
__main__.py, this setting should be moved toCOMMON_SETTING_DEFINITIONS(around line 98) because it is accessed by the supervisor logic in__main__.py, which uses the WebRTC settings object.
| {'name': 'supervisor_port', 'type': 'int', 'default': 8083, 'env_var': 'SELKIES_SUPERVISOR_PORT', 'help': 'Port for the dual-mode supervisor API (handles /switch and /status endpoints when enable_dual_mode is true).'}, | |
| {'name': 'supervisor_port', 'type': 'int', 'default': 8084, 'env_var': 'SELKIES_SUPERVISOR_PORT', 'help': 'Port for the dual-mode supervisor API (handles /switch and /status endpoints when enable_dual_mode is true).'}, |
Three issues raised in review: 1. supervisor_port was added to SETTING_DEFINITIONS_WEBSOCKETS only, but __main__.run() reads it from `settings_webrtc` (imported as `settings`). In dual mode this would AttributeError at runtime. Move the definition into COMMON_SETTING_DEFINITIONS so it appears in both settings_ws and settings_webrtc. 2. Default 8083 collided with control_port (also default 8083). Bump to 8084 so the dual-mode supervisor and the secure-mode control plane can coexist on default ports. 3. The explicit env_var='SELKIES_SUPERVISOR_PORT' was redundant: the AppSettings auto-derives `SELKIES_<UPPER>` already, so the help line would render "Env: SELKIES_SUPERVISOR_PORT (or SELKIES_SUPERVISOR_PORT)". Drop the explicit env_var; the auto-derived name is identical.
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
This PR makes the dual-mode supervisor API server port configurable via settings to avoid binding collisions when enable_dual_mode is enabled, replacing the previously hard-coded supervisor port in __main__.py.
Changes:
- Add a new
supervisor_portinteger setting (intended to backSELKIES_SUPERVISOR_PORT) for configuring the supervisor API server port. - Update
run()to readsettings.supervisor_portand log the actual configured port instead of using a hard-coded value.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/selkies/settings.py | Adds the supervisor_port setting definition and default value. |
| src/selkies/main.py | Uses settings.supervisor_port when starting the supervisor API server and updates logging accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {'name': 'enable_dual_mode', 'type': 'bool', 'default': False, 'help': 'Enable switching Streaming modes from UI'}, | ||
| {'name': 'supervisor_port', 'type': 'int', 'default': 8084, 'help': 'Port for the dual-mode supervisor API (/switch and /status endpoints) when enable_dual_mode is true.'}, | ||
| {'name': 'audio_device_name', 'type': 'str', 'default': 'output.monitor', 'help': 'Audio device name for pcmflux capture.'}, |
|
Thanks for the PR. But we have an ongoing effort to unify all ports into a single port #236. |
|
Related to #236. |
|
Superseded by #236. |
Reference the issue numbers and reviewers
(no existing issue I'd be aware of)
Explain relevant issues and how this pull request solves them
When
SELKIES_ENABLE_DUAL_MODE=true,run()insrc/selkies/__main__.pystarts the supervisor API server (the/switchand/statusendpoints that drive the dashboard's runtime mode switch) on a hard-coded port:That hardcoded
8082collides with the default of theportsetting (SELKIES_PORT, the data websocket server), which is also8082.Result: turning dual mode on out of the box races two listeners against the same port. In the typical container deployment (e.g.
linuxserver/baseimage-selkies, whereCUSTOM_WS_PORTdefaults to8082) users have to know to overrideCUSTOM_WS_PORT=8081(or similar) or one of the two services fails silently. There is no env var today to move the supervisor instead.This PR makes the supervisor port a first-class setting so it can be moved without changing the data port:
supervisor_portsetting (SELKIES_SUPERVISOR_PORT), default8083run()readssettings.supervisor_portinstead of the hardcoded80828083matches the existingcontrol_portdefault and removes the collision out of the box, while leaving every other default unchanged.Describe the changes in code and its dependencies and justify that they work as intended after testing
Two files touched, both in
src/selkies/:settings.py— one new entry in the settings list:{'name': 'supervisor_port', 'type': 'int', 'default': 8083, 'env_var': 'SELKIES_SUPERVISOR_PORT', 'help': 'Port for the dual-mode supervisor API (handles /switch and /status endpoints when enable_dual_mode is true).'},__main__.py— replace the hardcoded port in the dual-mode branch:No new dependencies. No change to the supervisor server itself, only where it binds.
Tested manually inside a
linuxserver/baseimage-selkies-derived container (debian-trixie + xfce):Default (no env var):
Override:
Single-mode (
enable_dual_mode=false, default): the supervisor task is not started either way, so the new setting has no effect — confirmed by running the previous default config and observing only the data ws listener.Describe alternatives you've considered
8082. Rejected: the data port is far more widely referenced in downstream nginx configs, deployment scripts and docs (CUSTOM_WS_PORT,proxy_pass http://127.0.0.1:8082), so changing it would be the more disruptive break.port + 1. Rejected as kinda unintuitive8082as the supervisor default and document thatCUSTOM_WS_PORTmust be moved. Rejected: this is the current state and the source of the bug report from downstream.Additional context
This unblocks the corresponding nginx routing in
linuxserver/docker-baseimage-selkies(separate PR) which addslocation ~ ^/(switch|status)$and a configurableSELKIES_SUPERVISOR_PORTenv var on the nginx side; with that PR plusthis one,
linuxserver/webtopusers get working runtime mode switching without any custom port juggling.