From f5195852e478f6f2c74a4d1db5ba5b40cf87761e Mon Sep 17 00:00:00 2001 From: Thales <> Date: Wed, 23 Sep 2026 17:54:48 +0100 Subject: [PATCH 1/2] Give PATH and yt-dlp the FFmpeg setup verified, not the data directory Setup can decide the FFmpeg in the data directory is unusable and settle on another: an Intel pair on Apple Silicon (#637), a build missing an encoder. It tells the backend through STEMDECK_FFMPEG and STEMDECK_FFPROBE, and separation honours that. PATH and yt-dlp were given FFMPEG_DIR instead, which in a desktop install is always data/ffmpeg, whatever setup concluded about it. So local files separated fine while every YouTube import ran the binary setup had thrown out, and the error named a binary the app had supposedly stopped using (#651). ffmpeg_dir() now answers for both: the directory holding the ffmpeg this process actually runs, provided ffprobe is beside it. yt-dlp takes one directory and looks for both there, so a split pair gets no directory at all and is left to PATH, where the desktop shell has already put the verified FFmpeg first. So do Docker and source runs, which have no FFmpeg of their own and were already right. A hand-set STEMDECK_FFMPEG_DIR is still honoured, because the binary defaults into it. The same fault had a second victim, not in the report. Setup also accepts FFmpeg nested under data/ffmpeg/bin/ (#248). yt-dlp was handed data/ffmpeg there, which holds no binary, and it runs dir/ffmpeg literally, with no fallback to PATH, so those installs had no FFmpeg for any import at all. The nested directory is what gets handed over now. The directory in use is logged at startup, and so is a data-directory FFmpeg that is being passed over, since the failure this prevents was silent. download.py also stops importing FFMPEG_DIR as a value. It was bound when the module first loaded, which is right in production and was the reason one of the new tests could not show main's behaviour on its own. A cold start with the #651 environment does: main gives yt-dlp data\ffmpeg, the rejected copy; this gives it the verified directory. Nine tests over real files: the rejected-copy case for ffmpeg_dir, PATH, yt-dlp and the log line, the nested layout, and the four setups that have to keep working. --- app/core/config.py | 64 +++++++++++- app/pipeline/download.py | 10 +- tests/test_ffmpeg_dir.py | 208 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 276 insertions(+), 6 deletions(-) create mode 100644 tests/test_ffmpeg_dir.py diff --git a/app/core/config.py b/app/core/config.py index fe9f86ea..0a9f0bc9 100644 --- a/app/core/config.py +++ b/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)) diff --git a/app/pipeline/download.py b/app/pipeline/download.py index 12afb24f..440a3d34 100644 --- a/app/pipeline/download.py +++ b/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: diff --git a/tests/test_ffmpeg_dir.py b/tests/test_ffmpeg_dir.py new file mode 100644 index 00000000..2741b807 --- /dev/null +++ b/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 From 9c1683295910f5ba53d5e1cbb66d8eefe52127ab Mon Sep 17 00:00:00 2001 From: Thales <> Date: Wed, 23 Sep 2026 18:25:30 +0100 Subject: [PATCH 2/2] Split the frontend e2e job across three runners The e2e job had become the slowest check on every pull request, about six minutes, growing with every spec (#686). It runs serially on purpose: the suite shares one backend and one jobs directory, and several specs change server state, so more workers against that one backend would interfere and fail intermittently. Raising workers was never the fix. Each shard is its own runner with its own backend, so this adds parallelism without adding sharing. Files stay whole within a shard, fullyParallel being off, so tests that rely on order inside a file still get it. One failing shard does not cancel the others, and each uploads its report under its own name, since a second upload under the same name fails the job. Run locally, all three against a fresh server each: 87, 90 and 81 tests, the whole 258, all passing, so no spec leaned on another file's leftovers. Free on a public repository; each shard pays its own setup, about a minute and a half. No required status check names frontend-e2e, so the per-shard names block nothing. --- .github/workflows/ci.yml | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 08d74051..e2fe693a 100644 --- a/.github/workflows/ci.yml +++ b/.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/