From 6ff81e628763861565ea8386fa77df884548ab5b Mon Sep 17 00:00:00 2001 From: None Date: Tue, 3 Mar 2026 17:18:16 -0600 Subject: [PATCH] Fix modular mode deployment issues (#1045) - Fix Postgres version check failing with restricted DB users (use $POSTGRES_DB instead of hardcoded 'postgres') - Fix DVR recording broken in modular mode (respect DISPATCHARR_PORT instead of hardcoding 9191) - Remove flushdb() from wait_for_redis.py to prevent Redis data loss on container restart - Add DISPATCHARR_PORT to celery environment in docker-compose.yml - Add depends_on health conditions for proper service startup ordering - Add extra_hosts for host.docker.internal resolution on Linux - Harden celery entrypoint with timeouts for JWT wait (120s) and migration wait (300s) - Replace fragile showmigrations grep with migrate --check - Add unit tests for DVR port resolution and flushdb removal regression --- apps/channels/tasks.py | 44 ++++++---- .../tests/test_dvr_port_resolution.py | 59 ++++++++++++++ docker/docker-compose.yml | 21 +++-- docker/entrypoint.celery.sh | 28 ++++++- docker/init/02-postgres.sh | 9 ++- scripts/wait_for_redis.py | 1 - tests/__init__.py | 0 tests/test_wait_for_redis.py | 81 +++++++++++++++++++ 8 files changed, 215 insertions(+), 28 deletions(-) create mode 100644 apps/channels/tests/test_dvr_port_resolution.py create mode 100644 tests/__init__.py create mode 100644 tests/test_wait_for_redis.py diff --git a/apps/channels/tasks.py b/apps/channels/tasks.py index 8d49287b..a95e9ad6 100755 --- a/apps/channels/tasks.py +++ b/apps/channels/tasks.py @@ -1486,6 +1486,33 @@ def _build_output_paths(channel, program, start_time, end_time): return final_path, temp_ts_path, os.path.basename(final_path) +def build_dvr_candidates(): + """Build ordered list of candidate base URLs for DVR TS streaming. + + Reads environment variables to determine which URLs to try: + - DISPATCHARR_INTERNAL_TS_BASE_URL: explicit override (first priority) + - DISPATCHARR_PORT: the external port (default 9191) + - DISPATCHARR_ENV/DISPATCHARR_DEBUG/REDIS_HOST: dev-mode detection + - DISPATCHARR_INTERNAL_API_BASE: override for the docker service URL + """ + explicit = os.environ.get('DISPATCHARR_INTERNAL_TS_BASE_URL') + dispatcharr_port = os.environ.get('DISPATCHARR_PORT', '9191') + is_dev = (os.environ.get('DISPATCHARR_ENV', '').lower() == 'dev') or \ + (os.environ.get('DISPATCHARR_DEBUG', '').lower() == 'true') or \ + (os.environ.get('REDIS_HOST', 'redis') in ('localhost', '127.0.0.1')) + candidates = [] + if explicit: + candidates.append(explicit) + if is_dev: + # Debug container typically exposes API on 5656 (uwsgi internal port) + candidates.extend(['http://127.0.0.1:5656', f'http://127.0.0.1:{dispatcharr_port}']) + # Docker service name fallback — use DISPATCHARR_PORT so modular mode works with custom ports + candidates.append(os.environ.get('DISPATCHARR_INTERNAL_API_BASE', f'http://web:{dispatcharr_port}')) + # Last-resort localhost ports + candidates.extend(['http://localhost:5656', f'http://localhost:{dispatcharr_port}']) + return candidates + + @shared_task def run_recording(recording_id, channel_id, start_time_str, end_time_str): """ @@ -1790,22 +1817,7 @@ def run_recording(recording_id, channel_id, start_time_str, end_time_str): from requests.exceptions import ReadTimeout, ConnectionError as ReqConnectionError, ChunkedEncodingError - # Determine internal base URL(s) for TS streaming - # Prefer explicit override, then try common ports for debug and docker - explicit = os.environ.get('DISPATCHARR_INTERNAL_TS_BASE_URL') - is_dev = (os.environ.get('DISPATCHARR_ENV', '').lower() == 'dev') or \ - (os.environ.get('DISPATCHARR_DEBUG', '').lower() == 'true') or \ - (os.environ.get('REDIS_HOST', 'redis') in ('localhost', '127.0.0.1')) - candidates = [] - if explicit: - candidates.append(explicit) - if is_dev: - # Debug container typically exposes API on 5656 - candidates.extend(['http://127.0.0.1:5656', 'http://127.0.0.1:9191']) - # Docker service name fallback - candidates.append(os.environ.get('DISPATCHARR_INTERNAL_API_BASE', 'http://web:9191')) - # Last-resort localhost ports - candidates.extend(['http://localhost:5656', 'http://localhost:9191']) + candidates = build_dvr_candidates() chosen_base = None last_error = None diff --git a/apps/channels/tests/test_dvr_port_resolution.py b/apps/channels/tests/test_dvr_port_resolution.py new file mode 100644 index 00000000..c7373959 --- /dev/null +++ b/apps/channels/tests/test_dvr_port_resolution.py @@ -0,0 +1,59 @@ +import os +from django.test import SimpleTestCase +from unittest.mock import patch + +from apps.channels.tasks import build_dvr_candidates + + +class DVRPortResolutionTests(SimpleTestCase): + """ + Tests that DVR recording candidate URLs respect the DISPATCHARR_PORT + environment variable instead of hardcoding port 9191. + """ + + @patch.dict(os.environ, {'REDIS_HOST': 'redis'}, clear=True) + def test_default_port_uses_9191(self): + """Without DISPATCHARR_PORT set, candidates default to 9191.""" + candidates = build_dvr_candidates() + self.assertIn('http://web:9191', candidates) + self.assertIn('http://localhost:9191', candidates) + + @patch.dict(os.environ, {'DISPATCHARR_PORT': '8080', 'REDIS_HOST': 'redis'}, clear=True) + def test_custom_port_reflected_in_candidates(self): + """DISPATCHARR_PORT=8080 replaces all hardcoded 9191 references.""" + candidates = build_dvr_candidates() + self.assertIn('http://web:8080', candidates) + self.assertIn('http://localhost:8080', candidates) + self.assertNotIn('http://web:9191', candidates) + self.assertNotIn('http://localhost:9191', candidates) + + @patch.dict(os.environ, { + 'DISPATCHARR_PORT': '7777', + 'DISPATCHARR_ENV': 'dev', + 'REDIS_HOST': 'redis', + }, clear=True) + def test_dev_mode_includes_5656_and_custom_port(self): + """Dev mode includes both uwsgi internal port (5656) and custom port.""" + candidates = build_dvr_candidates() + self.assertIn('http://127.0.0.1:5656', candidates) + self.assertIn('http://127.0.0.1:7777', candidates) + + @patch.dict(os.environ, { + 'DISPATCHARR_INTERNAL_TS_BASE_URL': 'http://custom:1234', + 'REDIS_HOST': 'redis', + }, clear=True) + def test_explicit_override_is_first(self): + """DISPATCHARR_INTERNAL_TS_BASE_URL should be the first candidate.""" + candidates = build_dvr_candidates() + self.assertEqual(candidates[0], 'http://custom:1234') + + @patch.dict(os.environ, { + 'DISPATCHARR_PORT': '3000', + 'DISPATCHARR_INTERNAL_API_BASE': 'http://myhost:4000', + 'REDIS_HOST': 'redis', + }, clear=True) + def test_internal_api_base_overrides_web_fallback(self): + """DISPATCHARR_INTERNAL_API_BASE replaces the http://web:{port} default.""" + candidates = build_dvr_candidates() + self.assertIn('http://myhost:4000', candidates) + self.assertNotIn('http://web:3000', candidates) diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index 8d086500..d674460a 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -15,8 +15,12 @@ services: volumes: - ./data:/data depends_on: - - db - - redis + db: + condition: service_healthy + redis: + condition: service_healthy + extra_hosts: + - "host.docker.internal:host-gateway" # --- Environment Configuration --- environment: @@ -83,9 +87,12 @@ services: container_name: dispatcharr_celery restart: unless-stopped depends_on: - - db - - redis - - web + db: + condition: service_healthy + redis: + condition: service_healthy + web: + condition: service_started volumes: - ./data:/data extra_hosts: @@ -97,6 +104,10 @@ services: # Deployment Mode - DISPATCHARR_ENV=modular + # Internal Service Communication + # Must match the web service port for DVR recording and internal API calls + - DISPATCHARR_PORT=9191 + # PostgreSQL Connection - POSTGRES_HOST=db - POSTGRES_PORT=5432 diff --git a/docker/entrypoint.celery.sh b/docker/entrypoint.celery.sh index e5c2f340..1c14f9b6 100644 --- a/docker/entrypoint.celery.sh +++ b/docker/entrypoint.celery.sh @@ -9,9 +9,19 @@ echo_with_timestamp() { echo "$(date '+%Y-%m-%d %H:%M:%S') - $1" } -# Wait for Django secret key +# Wait for Django secret key (generated by the web container on startup) +JWT_TIMEOUT=120 +JWT_WAITED=0 echo 'Waiting for Django secret key...' -while [ ! -f /data/jwt ]; do sleep 1; done +while [ ! -f /data/jwt ]; do + if [ $JWT_WAITED -ge $JWT_TIMEOUT ]; then + echo "❌ ERROR: Timed out waiting for /data/jwt after ${JWT_TIMEOUT}s." + echo " Is the web container running? Does it have the /data volume mounted?" + exit 1 + fi + sleep 1 + JWT_WAITED=$((JWT_WAITED + 1)) +done export DJANGO_SECRET_KEY="$(tr -d '\r\n' < /data/jwt)" # --- NumPy version switching for legacy hardware --- @@ -26,11 +36,21 @@ if [ "$USE_LEGACY_NUMPY" = "true" ]; then fi fi -# Wait for migrations to complete (check that NO unapplied migrations remain) +# Wait for migrations to complete +# Uses 'migrate --check' which exits 0 only when all migrations are applied, +# and exits 1 on unapplied migrations OR connection errors (safe either way) +MIG_TIMEOUT=300 +MIG_WAITED=0 echo 'Waiting for migrations to complete...' -until ! python manage.py showmigrations 2>&1 | grep -q '\[ \]'; do +until python manage.py migrate --check >/dev/null 2>&1; do + if [ $MIG_WAITED -ge $MIG_TIMEOUT ]; then + echo "❌ ERROR: Timed out waiting for migrations after ${MIG_TIMEOUT}s." + echo " Check web container logs for migration errors." + exit 1 + fi echo_with_timestamp 'Migrations not ready yet, waiting...' sleep 2 + MIG_WAITED=$((MIG_WAITED + 2)) done # Start Celery diff --git a/docker/init/02-postgres.sh b/docker/init/02-postgres.sh index 87aa94ef..2ece526f 100644 --- a/docker/init/02-postgres.sh +++ b/docker/init/02-postgres.sh @@ -204,14 +204,19 @@ check_external_postgres_version() { MIN_REQUIRED_VERSION=$PG_VERSION # Query external PostgreSQL version - EXTERNAL_VERSION=$(PGPASSWORD="$POSTGRES_PASSWORD" psql -w -h "$POSTGRES_HOST" -p "$POSTGRES_PORT" -U "$POSTGRES_USER" -d "postgres" -tAc "SHOW server_version;" 2>/dev/null | grep -oE '^[0-9]+') + # Use $POSTGRES_DB — restricted users may not have access to the default 'postgres' database + PG_VERSION_ERR=$(mktemp) + EXTERNAL_VERSION=$(PGPASSWORD="$POSTGRES_PASSWORD" psql -w -h "$POSTGRES_HOST" -p "$POSTGRES_PORT" -U "$POSTGRES_USER" -d "$POSTGRES_DB" -tAc "SHOW server_version;" 2>"$PG_VERSION_ERR" | grep -oE '^[0-9]+') if [ -z "$EXTERNAL_VERSION" ]; then echo "❌ ERROR: Unable to determine external PostgreSQL version" - echo " Could not connect to database at ${POSTGRES_HOST}:${POSTGRES_PORT}" + echo " Could not connect to database '$POSTGRES_DB' at ${POSTGRES_HOST}:${POSTGRES_PORT} as user '$POSTGRES_USER'" + echo " Error: $(cat "$PG_VERSION_ERR")" echo " Please verify your database connection settings." + rm -f "$PG_VERSION_ERR" return 1 fi + rm -f "$PG_VERSION_ERR" # Compare versions if [[ "$EXTERNAL_VERSION" -lt "$MIN_REQUIRED_VERSION" ]]; then diff --git a/scripts/wait_for_redis.py b/scripts/wait_for_redis.py index 0d278150..41b66757 100644 --- a/scripts/wait_for_redis.py +++ b/scripts/wait_for_redis.py @@ -31,7 +31,6 @@ def wait_for_redis(host='localhost', port=6379, db=0, password='', username='', socket_connect_timeout=2 ) redis_client.ping() - redis_client.flushdb() # Flush the database to ensure it's clean logger.info(f"✅ Redis at {host}:{port}/{db} is now available!") return True except (redis.exceptions.ConnectionError, redis.exceptions.TimeoutError) as e: diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/test_wait_for_redis.py b/tests/test_wait_for_redis.py new file mode 100644 index 00000000..55e623eb --- /dev/null +++ b/tests/test_wait_for_redis.py @@ -0,0 +1,81 @@ +import sys +import os +import importlib +from django.test import SimpleTestCase +from unittest.mock import patch, MagicMock + +import redis as redis_module + +# Ensure the scripts directory is importable +sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', 'scripts')) + + +def _import_wait_for_redis(): + """Import (or reimport) the wait_for_redis function from scripts/.""" + import wait_for_redis as module + importlib.reload(module) + return module.wait_for_redis + + +class WaitForRedisTests(SimpleTestCase): + """ + Tests for scripts/wait_for_redis.py. + + The critical regression test is test_successful_connection_does_not_call_flushdb + which ensures the removed flushdb() call is never re-added. + """ + + @patch('wait_for_redis.redis.Redis') + def test_successful_connection_does_not_call_flushdb(self, mock_redis_cls): + """After connecting successfully, flushdb must NOT be called.""" + mock_client = MagicMock() + mock_client.ping.return_value = True + mock_redis_cls.return_value = mock_client + + wait_for_redis = _import_wait_for_redis() + result = wait_for_redis(max_retries=1, retry_interval=0) + + self.assertTrue(result) + mock_client.ping.assert_called_once() + mock_client.flushdb.assert_not_called() + + @patch('wait_for_redis.redis.Redis') + def test_retries_on_connection_error(self, mock_redis_cls): + """Should retry on ConnectionError and eventually succeed.""" + mock_client = MagicMock() + mock_client.ping.side_effect = [ + redis_module.exceptions.ConnectionError("refused"), + redis_module.exceptions.ConnectionError("refused"), + True, + ] + mock_redis_cls.return_value = mock_client + + wait_for_redis = _import_wait_for_redis() + result = wait_for_redis(max_retries=5, retry_interval=0) + + self.assertTrue(result) + self.assertEqual(mock_client.ping.call_count, 3) + + @patch('wait_for_redis.redis.Redis') + def test_returns_false_after_max_retries(self, mock_redis_cls): + """Should return False when max retries are exhausted.""" + mock_client = MagicMock() + mock_client.ping.side_effect = redis_module.exceptions.ConnectionError("refused") + mock_redis_cls.return_value = mock_client + + wait_for_redis = _import_wait_for_redis() + result = wait_for_redis(max_retries=2, retry_interval=0) + + self.assertFalse(result) + + @patch('wait_for_redis.redis.Redis') + def test_unexpected_error_returns_false(self, mock_redis_cls): + """Generic exceptions should return False immediately.""" + mock_client = MagicMock() + mock_client.ping.side_effect = RuntimeError("unexpected") + mock_redis_cls.return_value = mock_client + + wait_for_redis = _import_wait_for_redis() + result = wait_for_redis(max_retries=5, retry_interval=0) + + self.assertFalse(result)