Give PATH and yt-dlp the verified FFmpeg, and split the e2e job

.github/workflows/ci.yml

@@ -61,8 +61,21 @@ jobs:
   # Browser tests for static/js (#339). node --check above is a syntax parse;
   # this drives the real UI against the real backend, including the Tauri code
   # path, which is where #335 hid because it was invisible in a browser.
+  # Split three ways, each shard on its own runner (#686). The suite runs
+  # serially inside a shard -- workers: 1 in playwright.config -- because it
+  # shares one backend and one jobs directory, and several specs change server
+  # state. A shard is a separate machine with its own backend, so splitting
+  # here adds parallelism without adding sharing. Files stay whole, since
+  # fullyParallel is off, so tests that rely on order within a file still get
+  # it. Free on a public repository; each shard pays its own setup.
   frontend-e2e:
     runs-on: ubuntu-latest
+    strategy:
+      # One failing shard must not cancel the others: the report is only
+      # useful whole.
+      fail-fast: false
+      matrix:
+        shard: [1, 2, 3]
     steps:
       - uses: actions/checkout@v7.0.1
       - uses: actions/setup-node@v7.0.0
@@ -74,11 +87,12 @@ jobs:
       - run: uv sync --frozen --all-extras
       - run: npm ci
       - run: npx playwright install --with-deps chromium
-      - run: npx playwright test
+      - run: npx playwright test --shard=${{ matrix.shard }}/3
       - if: failure()
         uses: actions/upload-artifact@v7.0.1
         with:
-          name: playwright-report
+          # Per shard: a second upload under the same name fails the job.
+          name: playwright-report-${{ matrix.shard }}
           path: |
             playwright-report/
             test-results/

app/core/config.py

@@ -1,10 +1,13 @@
 import json
+import logging
 import os
 import re
 import shutil
 import sys
 from pathlib import Path
 
+logger = logging.getLogger("stemdeck.config")
+
 
 def _env_int(name: str, default: int) -> int:
     raw = os.environ.get(name, "").strip()
@@ -456,17 +459,74 @@ def ffprobe_executable() -> str:
     return str(FFPROBE_BIN) if FFPROBE_BIN.is_file() else "ffprobe"
 
 
+def ffmpeg_dir() -> Path | None:
+    """The directory holding the FFmpeg this process actually runs, or None.
+
+    PATH and yt-dlp both need a directory, and they used to be given
+    FFMPEG_DIR: in a desktop install always data/ffmpeg, whatever setup had
+    concluded about what lives there. Setup can reject that copy and settle on
+    another -- an Intel pair on Apple Silicon (#637), a build missing an
+    encoder -- and tell the backend through STEMDECK_FFMPEG/STEMDECK_FFPROBE,
+    which ffmpeg_executable() honours. PATH and yt-dlp never got the message,
+    so separation used the verified binary while every YouTube import ran the
+    rejected one (#651).
+
+    So the directory is derived from the binary, which makes it one answer
+    everywhere. It also covers data/ffmpeg/bin/, the nested layout setup
+    accepts (#248): yt-dlp was handed data/ffmpeg there, which holds no binary
+    at all, and yt-dlp does not fall back to PATH from a directory it was
+    given.
+
+    None when there is no single directory to hand over: no binaries (Docker,
+    or a source checkout with FFmpeg on PATH, where PATH is already right), or
+    ffmpeg and ffprobe in different places. yt-dlp takes one directory and
+    looks for both in it, so a split pair is left to PATH, where the desktop
+    shell has already put the verified directory first.
+
+    A hand-set STEMDECK_FFMPEG_DIR is still honoured: FFMPEG_BIN defaults to a
+    file inside it.
+
+    Evaluated per call, not bound at import, so nothing has to agree with a
+    value captured before the environment was final.
+    """
+    if not (FFMPEG_BIN.is_file() and FFPROBE_BIN.is_file()):
+        return None
+    directory = FFMPEG_BIN.resolve().parent
+    if FFPROBE_BIN.resolve().parent != directory:
+        return None
+    return directory
+
+
 def configure_portable_environment() -> None:
     """Keep generated caches inside the portable data folder when requested.
 
     This is intentionally best-effort. It only sets variables that are still
     unset, so explicit caller/env choices win.
     """
-    if FFMPEG_DIR.is_dir():
+    # FFmpeg first on PATH for anything that runs a bare `ffmpeg`, Demucs
+    # decoding a compressed source among them. See ffmpeg_dir() for why this is
+    # the verified binary's directory and not FFMPEG_DIR.
+    directory = ffmpeg_dir()
+    if directory is not None:
         path = os.environ.get("PATH", "")
-        ffmpeg_path = str(FFMPEG_DIR)
+        ffmpeg_path = str(directory)
         if ffmpeg_path not in path.split(os.pathsep):
             os.environ["PATH"] = ffmpeg_path + (os.pathsep + path if path else "")
+        logger.info("FFmpeg directory: %s", directory)
+    else:
+        logger.info("FFmpeg directory: none of our own, resolving from PATH")
+    # Said out loud because the failure it prevents was silent: a data-directory
+    # FFmpeg that is not the one in use is the copy setup rejected, and the
+    # error a user sees if it is ever run names that binary (#651). Not said
+    # for data/ffmpeg/bin/, which is the same install in its nested layout.
+    if FFMPEG_DIR.is_dir() and (
+        directory is None or not directory.is_relative_to(FFMPEG_DIR.resolve())
+    ):
+        logger.info(
+            "Not using %s: the FFmpeg in use is %s",
+            FFMPEG_DIR,
+            directory or "whatever PATH resolves",
+        )
 
     if PORTABLE_DATA_DIR_ENABLED:
         os.environ.setdefault("XDG_CACHE_HOME", str(CACHE_DIR))

app/pipeline/download.py

@@ -8,7 +8,7 @@
 
 from yt_dlp import YoutubeDL
 
-from app.core.config import FFMPEG_DIR, bundled_js_runtime, js_solver_available
+from app.core.config import bundled_js_runtime, ffmpeg_dir, js_solver_available
 from app.core.models import Job, JobCancelled, _set
 from app.core.settings import get_cookies_file, get_max_duration_sec, get_video_max_height
 
@@ -212,9 +212,11 @@ def _base_ydl_opts(extractors: list[str], *, use_cookies: bool = False) -> dict:
         "socket_timeout": _SOCKET_TIMEOUT_SEC,
     }
     # Portable builds have no ffmpeg on PATH; needed wherever a DASH stream
-    # might be remuxed. Inert for the metadata-only calls.
-    if FFMPEG_DIR.is_dir():
-        opts["ffmpeg_location"] = str(FFMPEG_DIR)
+    # might be remuxed. Inert for the metadata-only calls. The directory of the
+    # FFmpeg the backend verified, not FFMPEG_DIR, which can be the copy setup
+    # rejected (#651); yt-dlp runs whatever it finds there and never falls back.
+    if (directory := ffmpeg_dir()) is not None:
+        opts["ffmpeg_location"] = str(directory)
     # YouTube's n-challenge solver needs a JS runtime. Absent outside portable
     # builds, where yt-dlp resolves its own from PATH instead (#432).
     if (runtime := bundled_js_runtime()) is not None:

tests/test_ffmpeg_dir.py

@@ -0,0 +1,208 @@
+"""Which FFmpeg directory the backend hands to PATH and to yt-dlp (#651).
+
+Setup can reject the FFmpeg in the data directory and settle on another -- an
+Intel pair on Apple Silicon (#637), a build missing an encoder -- and it tells
+the backend through STEMDECK_FFMPEG / STEMDECK_FFPROBE. Separation honoured
+that. PATH and yt-dlp were given FFMPEG_DIR instead, which in a desktop install
+is always data/ffmpeg, so every YouTube import ran the binary setup had thrown
+out, and the error named a binary the app had supposedly stopped using.
+
+These build real files in a temporary data directory and reload the config
+module under the environment the desktop shell would give it, because what is
+under test is exactly what the backend concludes from that environment.
+"""
+
+from __future__ import annotations
+
+import importlib
+import logging
+import os
+import sys
+from pathlib import Path
+
+import pytest
+
+EXE = ".exe" if sys.platform.startswith("win") else ""
+ENV = ("STEMDECK_DATA_DIR", "STEMDECK_FFMPEG", "STEMDECK_FFPROBE", "STEMDECK_FFMPEG_DIR")
+
+
+def _pair(directory: Path) -> tuple[Path, Path]:
+    """An ffmpeg and an ffprobe, side by side, as a real install has them."""
+    directory.mkdir(parents=True, exist_ok=True)
+    ffmpeg = directory / f"ffmpeg{EXE}"
+    ffprobe = directory / f"ffprobe{EXE}"
+    ffmpeg.write_bytes(b"")
+    ffprobe.write_bytes(b"")
+    return ffmpeg, ffprobe
+
+
+@pytest.fixture
+def load(monkeypatch):
+    """Reload config as the backend would start under the given environment."""
+    import app.core.config as config
+
+    def _load(data_dir: Path, **env: Path | None):
+        monkeypatch.setenv("STEMDECK_DATA_DIR", str(data_dir))
+        for name in ENV[1:]:
+            value = env.get(name.removeprefix("STEMDECK_").lower())
+            if value is None:
+                monkeypatch.delenv(name, raising=False)
+            else:
+                monkeypatch.setenv(name, str(value))
+        return importlib.reload(config)
+
+    yield _load
+    monkeypatch.undo()
+    importlib.reload(config)
+
+
+@pytest.fixture
+def config_log():
+    """What config logged. Attached to its own logger rather than through
+    caplog, because the app's logging setup may stop propagation upward."""
+    records: list[logging.LogRecord] = []
+
+    class Keep(logging.Handler):
+        def emit(self, record):
+            records.append(record)
+
+    logger = logging.getLogger("stemdeck.config")
+    handler = Keep(level=logging.INFO)
+    previous = logger.level
+    logger.addHandler(handler)
+    logger.setLevel(logging.INFO)
+    yield records
+    logger.removeHandler(handler)
+    logger.setLevel(previous)
+
+
+def _ydl_location(config) -> str | None:
+    # Imported here: download.py holds the function it imported from config,
+    # and a reload of config re-executes the same module dict, so it sees the
+    # reloaded values.
+    from app.pipeline.download import _base_ydl_opts
+
+    return _base_ydl_opts(["youtube"]).get("ffmpeg_location")
+
+
+# ── the bug ──────────────────────────────────────────────────────────────────
+
+
+def test_a_rejected_data_dir_ffmpeg_is_not_the_directory_used(load, tmp_path):
+    data = tmp_path / "data"
+    _pair(data / "ffmpeg")  # the pair setup rejected
+    ffmpeg, ffprobe = _pair(tmp_path / "system")  # the one it verified
+
+    config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe)
+
+    assert config.ffmpeg_dir() == (tmp_path / "system").resolve()
+
+
+def test_path_gets_the_verified_directory_and_never_the_rejected_one(load, monkeypatch, tmp_path):
+    data = tmp_path / "data"
+    _pair(data / "ffmpeg")
+    ffmpeg, ffprobe = _pair(tmp_path / "system")
+    monkeypatch.setenv("PATH", str(tmp_path / "elsewhere"))
+
+    config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe)
+    config.configure_portable_environment()
+
+    entries = [Path(p) for p in os.environ["PATH"].split(os.pathsep)]
+    assert entries[0] == (tmp_path / "system").resolve()
+    assert (data / "ffmpeg").resolve() not in [p.resolve() for p in entries]
+
+
+def test_yt_dlp_is_given_the_verified_directory(load, tmp_path):
+    data = tmp_path / "data"
+    _pair(data / "ffmpeg")
+    ffmpeg, ffprobe = _pair(tmp_path / "system")
+
+    config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe)
+
+    assert _ydl_location(config) == str((tmp_path / "system").resolve())
+
+
+def test_skipping_the_data_dir_copy_is_logged(load, config_log, monkeypatch, tmp_path):
+    # The failure was silent; the next report should find this in the log.
+    data = tmp_path / "data"
+    _pair(data / "ffmpeg")
+    ffmpeg, ffprobe = _pair(tmp_path / "system")
+    monkeypatch.setenv("PATH", "")
+
+    config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe)
+    config.configure_portable_environment()
+
+    messages = [r.getMessage() for r in config_log]
+    assert any(m.startswith("Not using") and "ffmpeg" in m for m in messages), messages
+
+
+# ── the nested layout (#248) ─────────────────────────────────────────────────
+
+
+def test_the_nested_bin_layout_hands_over_bin(load, config_log, monkeypatch, tmp_path):
+    # yt-dlp was given data/ffmpeg here, which holds no binary, and it runs
+    # dir/ffmpeg literally without falling back to PATH.
+    data = tmp_path / "data"
+    ffmpeg, ffprobe = _pair(data / "ffmpeg" / "bin")
+    monkeypatch.setenv("PATH", "")
+
+    config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe)
+    config.configure_portable_environment()
+
+    assert config.ffmpeg_dir() == (data / "ffmpeg" / "bin").resolve()
+    assert _ydl_location(config) == str((data / "ffmpeg" / "bin").resolve())
+    # The same install, nested: nothing is being skipped.
+    assert not any(r.getMessage().startswith("Not using") for r in config_log)
+
+
+# ── what has to keep working ─────────────────────────────────────────────────
+
+
+def test_a_verified_data_dir_ffmpeg_is_still_used(load, tmp_path):
+    data = tmp_path / "data"
+    ffmpeg, ffprobe = _pair(data / "ffmpeg")
+
+    config = load(data, ffmpeg=ffmpeg, ffprobe=ffprobe)
+
+    assert config.ffmpeg_dir() == (data / "ffmpeg").resolve()
+    assert _ydl_location(config) == str((data / "ffmpeg").resolve())
+
+
+def test_docker_and_source_runs_leave_it_to_path(load, monkeypatch, tmp_path):
+    # No FFmpeg of our own and no override: PATH is already right there.
+    monkeypatch.setenv("PATH", str(tmp_path / "usr-bin"))
+
+    config = load(tmp_path / "data")
+    config.configure_portable_environment()
+
+    assert config.ffmpeg_dir() is None
+    assert os.environ["PATH"] == str(tmp_path / "usr-bin")
+    assert _ydl_location(config) is None
+
+
+def test_a_hand_set_ffmpeg_dir_is_still_honoured(load, tmp_path):
+    # Source runs point STEMDECK_FFMPEG_DIR at a bundled build when FFmpeg is
+    # not on PATH, with no STEMDECK_FFMPEG. The binary defaults into it.
+    bundled = tmp_path / "bundled-ffmpeg"
+    _pair(bundled)
+
+    config = load(tmp_path / "data", ffmpeg_dir=bundled)
+
+    assert config.ffmpeg_dir() == bundled.resolve()
+    assert _ydl_location(config) == str(bundled.resolve())
+
+
+def test_a_split_pair_is_left_to_path(load, monkeypatch, tmp_path):
+    # yt-dlp takes one directory and looks for both binaries in it, so there
+    # is no directory to give it. PATH decides, where the desktop shell has
+    # already put the verified FFmpeg's directory first.
+    ffmpeg, _ = _pair(tmp_path / "a")
+    _, ffprobe = _pair(tmp_path / "b")
+    monkeypatch.setenv("PATH", str(tmp_path / "shell-path"))
+
+    config = load(tmp_path / "data", ffmpeg=ffmpeg, ffprobe=ffprobe)
+    config.configure_portable_environment()
+
+    assert config.ffmpeg_dir() is None
+    assert os.environ["PATH"] == str(tmp_path / "shell-path")
+    assert _ydl_location(config) is None