fix: wire settings.json to actually drive runtime behavior

Settings page saved to /config/settings.json but nothing downstream read that file. Schedule changes were silently ignored; max_quality and sleep_interval changes were silently ignored. The "Settings saved successfully" flash was a lie. Fix: - sync.sh reads max_quality + sleep_interval from settings.json on each run (jq -er ... // empty, falling back to env vars on missing/malformed file) - entrypoint.sh reads sync_schedule from settings.json before setting up cron, and writes the crond PID to /var/run/crond.pid so Flask can SIGHUP it - app.py adds apply_schedule(): rewrites /etc/crontabs/root, signals crond via the recorded PID, restarts crond if the PID is stale, drops the crontab when schedule is set to "manual". save_settings_route invokes it only when the schedule actually changed; any failure flashes a warning so the save still succeeds with the user informed - bare `except: pass` in get_settings replaced with explicit exception types + stderr warning so debugging malformed settings is possible - sync.sh: one bad channel no longer aborts the whole loop under set -e - Dockerfile adds jq for the JSON reads in sync.sh / entrypoint.sh - README: two stale github.com URLs fixed to Gitea; new Running Tests section under Building From Source - tests/test_settings.py: 3 pytest cases covering get_settings()'s three branches (missing file, valid file, malformed JSON) Settings hierarchy unchanged: env-var defaults seed the UI; settings.json wins when present and parseable. Timezone (TZ) is not applied live - tzdata is locked in at process start. Same behavior as before; not in scope for this commit.
2026-05-31 19:21:43 -07:00
parent 71ba63ffe3
commit ef903c86d1
8 changed files with 216 additions and 5 deletions
--- a/1
+++ b/1
@@ -7,6 +7,7 @@ RUN apk add --no-cache \
    ffmpeg \
    bash \
    curl \
+    jq \
    tzdata \
    dcron \
    && pip3 install --no-cache-dir yt-dlp flask --break-system-packages
--- a/README.md
+++ b/README.md
@@ -175,14 +175,24 @@ YouTube is blocking downloads. Solution: Add cookies.txt (see Configuration sect
 ## Building From Source

 ```bash
-git clone https://github.com/azcomputerguru/youtube-sync-docker
+git clone https://git.azcomputerguru.com/azcomputerguru/youtube-sync-docker
 cd youtube-sync-docker
 docker build -t youtube-sync .
 ```

+### Running Tests
+
+Tests are dev-only and not bundled into the production image. Install the
+dev dependencies and run pytest from the repo root:
+
+```bash
+pip install -r requirements-dev.txt
+pytest tests/
+```
+
 ## Support

-Issues and pull requests: https://github.com/azcomputerguru/youtube-sync-docker
+Issues and pull requests: https://git.azcomputerguru.com/azcomputerguru/youtube-sync-docker

 ## License

--- a/app.py
+++ b/app.py
@@ -5,7 +5,9 @@ Provides a web UI for managing YouTube channel downloads
 """

 import os
+import signal
 import subprocess
+import sys
 import json
 import re
 from datetime import datetime
@@ -23,6 +25,8 @@ CHANNELS_FILE = os.path.join(CONFIG_DIR, 'channels.txt')
 COOKIES_FILE = os.path.join(CONFIG_DIR, 'cookies.txt')
 SETTINGS_FILE = os.path.join(CONFIG_DIR, 'settings.json')
 LOG_FILE = '/var/log/youtube-sync.log'
+CRONTAB_FILE = '/etc/crontabs/root'
+CROND_PID_FILE = '/var/run/crond.pid'

 def get_settings():
    """Load settings from file or return defaults"""
@@ -37,8 +41,14 @@ def get_settings():
        try:
            with open(SETTINGS_FILE, 'r') as f:
                return json.load(f)
-        except:
-            pass
+        except (OSError, json.JSONDecodeError) as e:
+            # Malformed or unreadable settings.json: log and fall back to defaults
+            # rather than crashing the UI. The user can re-save from the Settings page
+            # to rewrite a clean file.
+            print(
+                f"[WARNING] Could not read {SETTINGS_FILE}: {e}; using env-var defaults",
+                file=sys.stderr,
+            )

    return defaults

@@ -47,6 +57,75 @@ def save_settings(settings):
    with open(SETTINGS_FILE, 'w') as f:
        json.dump(settings, f, indent=2)

+def apply_schedule(new_schedule):
+    """
+    Rewrite /etc/crontabs/root with the new schedule and reload crond so the
+    change takes effect without a container restart.
+
+    - new_schedule == "manual": remove the crontab entirely.
+    - otherwise: write a single line invoking /app/sync.sh on the given schedule.
+
+    crond is reloaded by SIGHUP'ing the PID recorded by entrypoint.sh. If crond
+    isn't running (e.g. container started in manual mode) and we now have a real
+    schedule, start it.
+
+    Raises whatever the underlying OS calls raise; callers are expected to
+    catch and surface a warning to the user.
+    """
+    if new_schedule == 'manual':
+        # Drop the crontab so crond stops firing the job. Leave crond running —
+        # cheaper than killing/restarting it and it'll just idle.
+        if os.path.exists(CRONTAB_FILE):
+            os.remove(CRONTAB_FILE)
+    else:
+        os.makedirs(os.path.dirname(CRONTAB_FILE), exist_ok=True)
+        line = f"{new_schedule} /app/sync.sh >> {LOG_FILE} 2>&1\n"
+        with open(CRONTAB_FILE, 'w') as f:
+            f.write(line)
+
+    # Reload crond. dcron (Alpine) re-reads crontabs on SIGHUP.
+    pid = None
+    if os.path.exists(CROND_PID_FILE):
+        try:
+            with open(CROND_PID_FILE, 'r') as f:
+                pid = int(f.read().strip())
+        except (OSError, ValueError) as e:
+            print(
+                f"[WARNING] Could not read crond pid file {CROND_PID_FILE}: {e}",
+                file=sys.stderr,
+            )
+            pid = None
+
+    if pid is not None:
+        try:
+            os.kill(pid, signal.SIGHUP)
+            return
+        except ProcessLookupError:
+            # PID file is stale — crond exited. Fall through to start it fresh
+            # below if we have a non-manual schedule.
+            print(
+                f"[WARNING] crond pid {pid} no longer running; restarting",
+                file=sys.stderr,
+            )
+            pid = None
+
+    # No live crond. Start one if we have an active schedule to run.
+    if new_schedule != 'manual':
+        proc = subprocess.Popen(
+            ['crond', '-f', '-l', '2'],
+            stdout=subprocess.DEVNULL,
+            stderr=subprocess.DEVNULL,
+        )
+        try:
+            with open(CROND_PID_FILE, 'w') as f:
+                f.write(str(proc.pid))
+        except OSError as e:
+            # Non-fatal: cron is running, we just can't reload it on the next change.
+            print(
+                f"[WARNING] crond started (pid {proc.pid}) but pid file write failed: {e}",
+                file=sys.stderr,
+            )
+
 def extract_channel_id(url_or_id):
    """
    Extract channel ID from various YouTube URL formats or validate direct ID
@@ -280,6 +359,7 @@ def settings():
@app.route('/settings/save', methods=['POST'])
 def save_settings_route():
    """Save settings"""
+    previous = get_settings()
    settings = {
        'sync_schedule': request.form.get('sync_schedule', '0 2 * * *'),
        'max_quality': request.form.get('max_quality', '1080'),
@@ -289,6 +369,27 @@ def save_settings_route():

    save_settings(settings)
    flash('Settings saved successfully', 'success')
+
+    # Apply the new cron schedule immediately if it changed. max_quality /
+    # sleep_interval are read fresh by sync.sh on each run, so no action needed
+    # for them. Timezone changes still require a container restart (system tzdata
+    # is locked in at container start); we don't claim otherwise.
+    if settings['sync_schedule'] != previous.get('sync_schedule'):
+        try:
+            apply_schedule(settings['sync_schedule'])
+        except (OSError, subprocess.SubprocessError) as e:
+            # Save succeeded; the schedule reload didn't. Let the user know
+            # it'll take effect on next restart rather than silently failing.
+            print(
+                f"[WARNING] Could not reload cron schedule: {e}",
+                file=sys.stderr,
+            )
+            flash(
+                'Schedule saved, but live reload failed. '
+                'New schedule will take effect after container restart.',
+                'warning',
+            )
+
    return redirect(url_for('settings'))

@app.route('/cookies', methods=['GET', 'POST'])
--- a/entrypoint.sh
+++ b/entrypoint.sh
@@ -8,6 +8,16 @@ echo "YouTube Channel Sync Docker"
 echo "=========================================="
 echo "Download Directory: $DOWNLOAD_DIR"
 echo "Config Directory: $CONFIG_DIR"
+
+# settings.json (managed via the web UI) overrides the env-var defaults
+# baked into the image / docker-compose. Same hierarchy as app.py:get_settings().
+SETTINGS_FILE="${CONFIG_DIR}/settings.json"
+if [ -f "$SETTINGS_FILE" ]; then
+    if VAL=$(jq -er '.sync_schedule // empty' "$SETTINGS_FILE" 2>/dev/null); then
+        SYNC_SCHEDULE="$VAL"
+    fi
+fi
+
 echo "Sync Schedule: $SYNC_SCHEDULE"
 echo "Max Quality: ${MAX_QUALITY}p"
 echo "Sleep Interval: ${SLEEP_INTERVAL}s"
@@ -45,6 +55,8 @@ if [ "$SYNC_SCHEDULE" != "manual" ]; then
    # Start crond in the background
    crond -f -l 2 &
    CRON_PID=$!
+    # Record PID so the Flask UI can SIGHUP crond when the schedule changes.
+    echo "$CRON_PID" > /var/run/crond.pid
    echo "[INFO] Cron daemon started (PID: $CRON_PID)"
 else
    echo "[INFO] Manual mode enabled. Use Web UI to trigger syncs."
--- a/requirements-dev.txt
+++ b/requirements-dev.txt
@@ -0,0 +1 @@
+pytest>=7.0
--- a/sync.sh
+++ b/sync.sh
@@ -8,9 +8,21 @@ DOWNLOAD_DIR="${DOWNLOAD_DIR:-/downloads}"
 CONFIG_DIR="${CONFIG_DIR:-/config}"
 COOKIES_FILE="${CONFIG_DIR}/cookies.txt"
 CHANNELS_FILE="${CONFIG_DIR}/channels.txt"
+SETTINGS_FILE="${CONFIG_DIR}/settings.json"
 MAX_QUALITY="${MAX_QUALITY:-1080}"
 SLEEP_INTERVAL="${SLEEP_INTERVAL:-2}"

+# Override env-var defaults with values from settings.json when present and parseable.
+# Missing or malformed settings.json silently falls back to env-var defaults.
+if [ -f "$SETTINGS_FILE" ]; then
+    if VAL=$(jq -er '.max_quality // empty' "$SETTINGS_FILE" 2>/dev/null); then
+        MAX_QUALITY="$VAL"
+    fi
+    if VAL=$(jq -er '.sleep_interval // empty' "$SETTINGS_FILE" 2>/dev/null); then
+        SLEEP_INTERVAL="$VAL"
+    fi
+fi
+
 # Check if channels file exists
 if [ ! -f "$CHANNELS_FILE" ]; then
    echo "[ERROR] Channels file not found: $CHANNELS_FILE"
@@ -57,6 +69,9 @@ while IFS='|' read -r CHANNEL_ID CHANNEL_NAME || [ -n "$CHANNEL_ID" ]; do
    echo "Channel ID: $CHANNEL_ID"
    echo "=========================================="

+    # Don't let one bad channel kill the whole loop. yt-dlp can exit non-zero
+    # for individual videos even with --ignore-errors (e.g. age-gated, geo-blocked,
+    # member-only). We log and continue rather than aborting the entire sync.
    yt-dlp \
        $COOKIES_PARAM \
        --format "bestvideo[ext=mp4][height<=${MAX_QUALITY}]+bestaudio[ext=m4a]/best[ext=mp4]/best" \
@@ -73,7 +88,8 @@ while IFS='|' read -r CHANNEL_ID CHANNEL_NAME || [ -n "$CHANNEL_ID" ]; do
        --ignore-errors \
        --sleep-interval "$SLEEP_INTERVAL" \
        --max-sleep-interval 5 \
-        "https://www.youtube.com/channel/$CHANNEL_ID/videos"
+        "https://www.youtube.com/channel/$CHANNEL_ID/videos" \
+        || echo "[WARNING] channel $CHANNEL_NAME ($CHANNEL_ID) failed; continuing with remaining channels"

    # Create tvshow.nfo for Emby/Plex
    echo ""
--- a/tests/init.py
+++ b/tests/init.py
--- a/tests/test_settings.py
+++ b/tests/test_settings.py
@@ -0,0 +1,70 @@
+"""
+Tests for the settings hierarchy in app.get_settings().
+
+Hierarchy under test: env-var defaults are returned when settings.json is
+missing or malformed; the file wins when it exists and parses cleanly.
+"""
+
+import json
+import os
+import sys
+import tempfile
+
+import pytest
+
+# Make the app module importable without installing it.
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import app as app_module  # noqa: E402
+
+
+@pytest.fixture
+def tmp_settings_path(monkeypatch, tmp_path):
+    """Point app.SETTINGS_FILE at a temp path for the duration of the test."""
+    path = tmp_path / "settings.json"
+    monkeypatch.setattr(app_module, "SETTINGS_FILE", str(path))
+    return path
+
+
+def test_get_settings_returns_defaults_when_file_missing(tmp_settings_path, monkeypatch):
+    monkeypatch.setenv("SYNC_SCHEDULE", "5 5 * * *")
+    monkeypatch.setenv("MAX_QUALITY", "720")
+    monkeypatch.setenv("SLEEP_INTERVAL", "7")
+    monkeypatch.setenv("TZ", "UTC")
+    assert not tmp_settings_path.exists()
+
+    result = app_module.get_settings()
+
+    assert result == {
+        "sync_schedule": "5 5 * * *",
+        "max_quality": "720",
+        "sleep_interval": "7",
+        "timezone": "UTC",
+    }
+
+
+def test_get_settings_returns_file_contents_when_present(tmp_settings_path):
+    payload = {
+        "sync_schedule": "0 4 * * *",
+        "max_quality": "1440",
+        "sleep_interval": "3",
+        "timezone": "America/Phoenix",
+    }
+    tmp_settings_path.write_text(json.dumps(payload))
+
+    assert app_module.get_settings() == payload
+
+
+def test_get_settings_falls_back_on_malformed_json(tmp_settings_path, monkeypatch):
+    monkeypatch.setenv("SYNC_SCHEDULE", "0 2 * * *")
+    monkeypatch.setenv("MAX_QUALITY", "1080")
+    monkeypatch.setenv("SLEEP_INTERVAL", "2")
+    monkeypatch.setenv("TZ", "America/Phoenix")
+    tmp_settings_path.write_text("{ this is not valid json ")
+
+    result = app_module.get_settings()
+
+    assert result["sync_schedule"] == "0 2 * * *"
+    assert result["max_quality"] == "1080"
+    assert result["sleep_interval"] == "2"
+    assert result["timezone"] == "America/Phoenix"