From afd04c742cea2281c56bbe922b042f086ff2c6e8 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 5 Sep 2026 13:31:45 -0400 Subject: [PATCH 001/104] ci: skip pipeline steps for areas a pull request does not touch A new `changes` job diffs the PR against its base branch and emits four flags (go, js, i18n, build) that gate the steps of the test, lint and build jobs. A docs-only PR now runs no Go tests and no 11-platform build. The flags gate steps rather than jobs on purpose: a job-level skip propagates through the needs chain (actions/runner#491) and would take the release jobs down with it on tag pushes. Only upload-packages and the push-manifest jobs skip at the job level, where nothing depends on them. Master pushes and tags always get every flag, so a release can never be built from a partially validated tree. coverage-on-pr.yml now probes for the coverage artifact before downloading it, since a PR with no Go changes produces none. --- .github/workflows/coverage-on-pr.yml | 14 +++ .github/workflows/detect-changes.sh | 56 ++++++++++++ .github/workflows/pipeline.yml | 130 ++++++++++++++++++++++----- 3 files changed, 180 insertions(+), 20 deletions(-) create mode 100755 .github/workflows/detect-changes.sh diff --git a/.github/workflows/coverage-on-pr.yml b/.github/workflows/coverage-on-pr.yml index 03260cacc..6f982b33a 100644 --- a/.github/workflows/coverage-on-pr.yml +++ b/.github/workflows/coverage-on-pr.yml @@ -15,9 +15,20 @@ jobs: env: COVERAGE_COMMENT: 'true' steps: + # The pipeline skips its coverage steps when a PR touches no Go code. + - name: Check the run produced a coverage artifact + id: artifact + env: + GH_TOKEN: ${{ github.token }} + run: | + count=$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/${{ github.event.workflow_run.id }}/artifacts" \ + --jq '[.artifacts[] | select(.name == "octocov-pr")] | length') + echo "count=$count" >> "$GITHUB_OUTPUT" + # Only the config, from the base branch: this job holds a write token, so # it must never check out the fork. - name: Check out the octocov config + if: steps.artifact.outputs.count != '0' uses: actions/checkout@v7 with: sparse-checkout: .octocov.yml @@ -27,6 +38,7 @@ jobs: # Into a subdirectory. A pull_request run executes the fork's own copy of # pipeline.yml, so every file in here is attacker-controlled. - uses: actions/download-artifact@v8 + if: steps.artifact.outputs.count != '0' with: name: octocov-pr path: untrusted @@ -35,6 +47,7 @@ jobs: - name: Verify the artifact and take the coverage profile id: pr + if: steps.artifact.outputs.count != '0' env: GH_TOKEN: ${{ github.token }} HEAD_SHA: ${{ github.event.workflow_run.head_sha }} @@ -51,6 +64,7 @@ jobs: echo "number=$number" >> "$GITHUB_OUTPUT" - uses: k1LoW/octocov-action@v1 + if: steps.artifact.outputs.count != '0' env: # A workflow_run job looks like a push to the default branch. Point # octocov back at the pull request and at the run that produced it. diff --git a/.github/workflows/detect-changes.sh b/.github/workflows/detect-changes.sh new file mode 100755 index 000000000..2e7a42586 --- /dev/null +++ b/.github/workflows/detect-changes.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# +# Emits per-area change flags to $GITHUB_OUTPUT so the pipeline can skip work a +# pull request cannot affect: +# +# go - Go sources, module files, linter config, embedded resources +# js - anything under ui/ +# i18n - translation files and their validation script +# build - anything that ends up in a binary, image or package (i.e. every +# change except the doc-only paths in $DOC_ONLY_RE) +# +# Only pull requests are narrowed. Master pushes and tags always get every flag, +# so a release can never be built from a partially validated tree. +# +# Flags gate STEPS, not jobs: a job-level skip propagates through the needs +# chain (actions/runner#491) and would take the release jobs down with it. +# +# Compares HEAD against $BASE_REF (default master). Requires full history +# (fetch-depth: 0 in CI). +set -uo pipefail +export LC_ALL=C + +GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/)' +JS_RE='^ui/' +I18N_RE='(^resources/i18n/|^\.github/workflows/validate-translations\.sh$)' +DOC_ONLY_RE='(\.md$|^LICENSE$|^\.git-blame-ignore-revs$|^\.gitignore$|^\.devcontainer/)' + +emit() { printf '%s=%s\n' "$1" "$2" | tee -a "${GITHUB_OUTPUT:-/dev/null}"; } + +if [ "${GITHUB_EVENT_NAME:-}" != "pull_request" ]; then + echo "Not a pull request — running everything." + for area in go js i18n build; do emit "$area" true; done + exit 0 +fi + +BASE_REF="${BASE_REF:-master}" +git fetch --no-tags --quiet origin "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" + +files="$(git diff --name-only "origin/${BASE_REF}...HEAD")" +echo "Changed files:" +printf '%s\n' "$files" | sed 's/^/ /' +echo + +flag() { # $1=name $2=regex + if printf '%s\n' "$files" | grep -qE "$2"; then emit "$1" true; else emit "$1" false; fi +} + +flag go "$GO_RE" +flag js "$JS_RE" +flag i18n "$I18N_RE" + +if printf '%s\n' "$files" | grep -vE "$DOC_ONLY_RE" | grep -q '[^[:space:]]'; then + emit build true +else + emit build false +fi diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml index aa8e29e49..bb318bf5c 100644 --- a/.github/workflows/pipeline.yml +++ b/.github/workflows/pipeline.yml @@ -58,13 +58,36 @@ jobs: echo "GIT_TAG=$GIT_TAG" echo "GIT_SHA=$GIT_SHA" + # Outputs gate steps, never jobs: a job-level skip propagates through the needs + # chain (actions/runner#491) and would skip all release jobs on tag pushes. + changes: + name: Detect changed areas + runs-on: ubuntu-latest + outputs: + go: ${{ steps.detect.outputs.go }} + js: ${{ steps.detect.outputs.js }} + i18n: ${{ steps.detect.outputs.i18n }} + build: ${{ steps.detect.outputs.build }} + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + - name: Detect changed areas + id: detect + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: ./.github/workflows/detect-changes.sh + go-lint: name: Lint Go code runs-on: ubuntu-latest + needs: [changes] steps: - uses: actions/checkout@v7 + if: needs.changes.outputs.go == 'true' - uses: actions/setup-go@v6 + if: needs.changes.outputs.go == 'true' with: go-version-file: go.mod @@ -72,9 +95,11 @@ jobs: # cannot turn red in CI just because a new golangci-lint was released. - name: Resolve golangci-lint version id: golangci-version + if: needs.changes.outputs.go == 'true' run: echo "version=$(grep '^GOLANGCI_LINT_VERSION' Makefile | cut -d ' ' -f 3)" >> "$GITHUB_OUTPUT" - name: golangci-lint + if: needs.changes.outputs.go == 'true' uses: golangci/golangci-lint-action@v9 with: version: ${{ steps.golangci-version.outputs.version }} @@ -82,9 +107,12 @@ jobs: args: --timeout 2m - name: Run go goimports + if: needs.changes.outputs.go == 'true' run: go run golang.org/x/tools/cmd/goimports@latest -w `find . -name '*.go' | grep -v '_gen.go$' | grep -v '.pb.go$'` - - run: go mod tidy + - if: needs.changes.outputs.go == 'true' + run: go mod tidy - name: Verify no changes from goimports and go mod tidy + if: needs.changes.outputs.go == 'true' run: | git status --porcelain if [ -n "$(git status --porcelain)" ]; then @@ -93,8 +121,10 @@ jobs: fi - name: Run go generate + if: needs.changes.outputs.go == 'true' run: go generate ./... - name: Verify no changes from go generate + if: needs.changes.outputs.go == 'true' run: | git status --porcelain if [ -n "$(git status --porcelain)" ]; then @@ -126,23 +156,29 @@ jobs: go: name: Test Go code runs-on: ubuntu-latest + needs: [changes] steps: - name: Check out code into the Go module directory + if: needs.changes.outputs.go == 'true' uses: actions/checkout@v7 - uses: actions/setup-go@v6 + if: needs.changes.outputs.go == 'true' with: go-version-file: go.mod - name: Download dependencies + if: needs.changes.outputs.go == 'true' run: go mod download # Name must stay unique across the workflow: octocov matches step names # by name across every job, and waits for each match to finish. - name: Test with coverage + if: needs.changes.outputs.go == 'true' run: go test -shuffle=on -tags netgo,sqlite_fts5 -race -v -covermode=atomic -coverprofile=coverage.out $(go list ./... | grep -v '/plugins$') - name: Test ndpgen + if: needs.changes.outputs.go == 'true' run: | cd plugins/cmd/ndpgen go test -shuffle=on -v @@ -150,6 +186,7 @@ jobs: ./ndpgen --help - name: Upload coverage profile + if: needs.changes.outputs.go == 'true' uses: actions/upload-artifact@v7 with: name: octocov-go @@ -159,18 +196,22 @@ jobs: go-plugins: name: Test Go plugins runs-on: ubuntu-latest + needs: [changes] steps: - name: Check out code into the Go module directory + if: needs.changes.outputs.go == 'true' uses: actions/checkout@v7 - uses: actions/setup-go@v6 id: setup-go + if: needs.changes.outputs.go == 'true' with: go-version-file: go.mod # Without this, the suite recompiles every test plugin WASM module, # which dominates its runtime under -race. - name: Cache the WASM compilation cache + if: needs.changes.outputs.go == 'true' uses: actions/cache@v6 with: path: plugins/testdata/.wazero-cache @@ -178,9 +219,11 @@ jobs: restore-keys: wazero-${{ runner.os }}- - name: Test plugins + if: needs.changes.outputs.go == 'true' run: go tool ginkgo -p -race -tags netgo,sqlite_fts5 --cover --covermode=atomic --coverprofile=coverage.out --output-dir=. ./plugins/ - name: Upload coverage profile + if: needs.changes.outputs.go == 'true' uses: actions/upload-artifact@v7 with: name: octocov-plugins @@ -190,7 +233,7 @@ jobs: coverage: name: Report coverage runs-on: ubuntu-latest - needs: [go, go-plugins] + needs: [changes, go, go-plugins] permissions: contents: read actions: write @@ -198,27 +241,31 @@ jobs: COVERAGE_COMMENT: 'false' steps: - uses: actions/checkout@v7 + if: needs.changes.outputs.go == 'true' - uses: actions/download-artifact@v8 + if: needs.changes.outputs.go == 'true' with: pattern: octocov-* # Merge here rather than letting octocov do it: octocov reports statement # coverage for a single profile, but switches to line counting for several. - name: Merge coverage profiles + if: needs.changes.outputs.go == 'true' run: | echo "mode: atomic" > coverage.out awk 'FNR==1 && /^mode:/ {next} {k=$1" "$2; c[k]+=$3} END {for (k in c) print k, c[k]}' \ octocov-*/coverage.out | sort >> coverage.out - uses: k1LoW/octocov-action@v1 + if: needs.changes.outputs.go == 'true' - name: Save the PR number for the comment workflow - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request' && needs.changes.outputs.go == 'true' run: echo "${{ github.event.pull_request.number }}" > pr_number - name: Upload the merged profile for the comment workflow - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request' && needs.changes.outputs.go == 'true' uses: actions/upload-artifact@v7 with: name: octocov-pr @@ -230,27 +277,33 @@ jobs: go-windows: name: Test Go code (Windows) runs-on: windows-2022 + needs: [changes] env: FFMPEG_VERSION: "7.1" FFMPEG_REPOSITORY: navidrome/ffmpeg-windows-builds steps: - uses: actions/checkout@v7 + if: needs.changes.outputs.go == 'true' - uses: actions/setup-go@v6 + if: needs.changes.outputs.go == 'true' with: go-version-file: go.mod - uses: msys2/setup-msys2@v2 + if: needs.changes.outputs.go == 'true' with: msystem: MINGW64 install: mingw-w64-x86_64-gcc update: false - name: Add mingw64 to PATH + if: needs.changes.outputs.go == 'true' shell: bash run: echo "C:/msys64/mingw64/bin" >> $GITHUB_PATH - name: Cache ffmpeg + if: needs.changes.outputs.go == 'true' id: ffmpeg-cache uses: actions/cache@v6 with: @@ -258,7 +311,7 @@ jobs: key: ffmpeg-${{ env.FFMPEG_VERSION }}-win64 - name: Download ffmpeg - if: steps.ffmpeg-cache.outputs.cache-hit != 'true' + if: needs.changes.outputs.go == 'true' && steps.ffmpeg-cache.outputs.cache-hit != 'true' shell: pwsh run: | $asset = "ffmpeg-n${env:FFMPEG_VERSION}-latest-win64-gpl-${env:FFMPEG_VERSION}" @@ -270,10 +323,12 @@ jobs: Copy-Item "C:\ffmpeg-extracted\$asset\bin\ffprobe.exe" C:\ffmpeg\bin - name: Add ffmpeg to PATH + if: needs.changes.outputs.go == 'true' shell: bash run: echo "C:/ffmpeg/bin" >> $GITHUB_PATH - name: Verify toolchain + if: needs.changes.outputs.go == 'true' shell: pwsh run: | go version @@ -283,16 +338,19 @@ jobs: ffprobe -version - name: Download dependencies + if: needs.changes.outputs.go == 'true' shell: bash run: go mod download - name: Test + if: needs.changes.outputs.go == 'true' shell: bash env: CGO_ENABLED: "1" run: go test -shuffle=on -tags netgo,sqlite_fts5 ./... -v - name: Test ndpgen + if: needs.changes.outputs.go == 'true' shell: bash run: | cd plugins/cmd/ndpgen @@ -303,32 +361,39 @@ jobs: js: name: Test JS code runs-on: ubuntu-latest + needs: [changes] env: NODE_OPTIONS: "--max_old_space_size=4096" steps: - uses: actions/checkout@v7 + if: needs.changes.outputs.js == 'true' - uses: actions/setup-node@v6 + if: needs.changes.outputs.js == 'true' with: node-version: 24 cache: "npm" cache-dependency-path: "**/package-lock.json" - name: npm install dependencies + if: needs.changes.outputs.js == 'true' run: | cd ui npm ci - name: npm lint + if: needs.changes.outputs.js == 'true' run: | cd ui npm run check-formatting && npm run lint - name: npm test + if: needs.changes.outputs.js == 'true' run: | cd ui npm test - name: npm build + if: needs.changes.outputs.js == 'true' run: | cd ui npm run build @@ -336,9 +401,12 @@ jobs: i18n-lint: name: Lint i18n files runs-on: ubuntu-latest + needs: [changes] steps: - uses: actions/checkout@v7 - - run: | + if: needs.changes.outputs.i18n == 'true' + - if: needs.changes.outputs.i18n == 'true' + run: | set -e for file in resources/i18n/*.json; do echo "Validating $file" @@ -350,6 +418,7 @@ jobs: fi done - run: ./.github/workflows/validate-translations.sh -v + if: needs.changes.outputs.i18n == 'true' check-push-enabled: @@ -364,7 +433,7 @@ jobs: build: name: Build - needs: [js, go, go-plugins, go-windows, go-lint, i18n-lint, git-version, check-push-enabled, validate-migrations] + needs: [changes, js, go, go-plugins, go-windows, go-lint, i18n-lint, git-version, check-push-enabled, validate-migrations] strategy: matrix: platform: [ linux/amd64, linux/arm64, linux/arm/v5, linux/arm/v6, linux/arm/v7, linux/386, linux/riscv64, darwin/amd64, darwin/arm64, windows/amd64, windows/386 ] @@ -373,19 +442,23 @@ jobs: IS_LINUX: ${{ startsWith(matrix.platform, 'linux/') && 'true' || 'false' }} IS_ARMV5: ${{ matrix.platform == 'linux/arm/v5' && 'true' || 'false' }} IS_DOCKER_PUSH_CONFIGURED: ${{ needs.check-push-enabled.outputs.is_enabled == 'true' }} + SHOULD_BUILD: ${{ needs.changes.outputs.build }} DOCKER_BUILD_SUMMARY: false GIT_SHA: ${{ needs.git-version.outputs.git_sha }} GIT_TAG: ${{ needs.git-version.outputs.git_tag }} steps: - name: Sanitize platform name + if: env.SHOULD_BUILD == 'true' id: set-platform run: | PLATFORM=$(echo ${{ matrix.platform }} | tr '/' '_') echo "PLATFORM=$PLATFORM" >> $GITHUB_ENV - uses: actions/checkout@v7 + if: env.SHOULD_BUILD == 'true' - name: Prepare Docker Buildx + if: env.SHOULD_BUILD == 'true' uses: ./.github/actions/prepare-docker id: docker with: @@ -395,6 +468,7 @@ jobs: hub_password: ${{ secrets.DOCKER_HUB_PASSWORD }} - name: Build Binaries + if: env.SHOULD_BUILD == 'true' uses: docker/build-push-action@v7 with: context: . @@ -408,14 +482,14 @@ jobs: GIT_TAG=${{ env.GIT_TAG }} - name: Set up QEMU for smoke test - if: env.IS_LINUX == 'true' + if: env.SHOULD_BUILD == 'true' && env.IS_LINUX == 'true' uses: docker/setup-qemu-action@v4 # The binary is static, so binfmt+qemu runs it directly on the runner. # Catches startup crashes in cross-compiled binaries before they ship, # e.g. the broken ifunc relocations on 32-bit arm from issue #5738. - name: Smoke-test binary - if: env.IS_LINUX == 'true' + if: env.SHOULD_BUILD == 'true' && env.IS_LINUX == 'true' run: | BIN=./output/${{ env.PLATFORM }}/navidrome chmod +x "$BIN" @@ -423,6 +497,7 @@ jobs: echo "OK: ${{ matrix.platform }} binary starts" - name: Upload Binaries + if: env.SHOULD_BUILD == 'true' uses: actions/upload-artifact@v7 with: name: navidrome-${{ env.PLATFORM }} @@ -431,7 +506,7 @@ jobs: - name: Build and push image by digest id: push-image - if: env.IS_LINUX == 'true' && env.IS_DOCKER_PUSH_CONFIGURED == 'true' && env.IS_ARMV5 == 'false' + if: env.SHOULD_BUILD == 'true' && env.IS_LINUX == 'true' && env.IS_DOCKER_PUSH_CONFIGURED == 'true' && env.IS_ARMV5 == 'false' uses: docker/build-push-action@v7 with: context: . @@ -446,7 +521,7 @@ jobs: type=image,name=ghcr.io/${{ github.repository }},push-by-digest=true,name-canonical=true,push=true - name: Export digest - if: env.IS_LINUX == 'true' && env.IS_DOCKER_PUSH_CONFIGURED == 'true' && env.IS_ARMV5 == 'false' + if: env.SHOULD_BUILD == 'true' && env.IS_LINUX == 'true' && env.IS_DOCKER_PUSH_CONFIGURED == 'true' && env.IS_ARMV5 == 'false' run: | mkdir -p /tmp/digests digest="${{ steps.push-image.outputs.digest }}" @@ -454,7 +529,7 @@ jobs: - name: Upload digest uses: actions/upload-artifact@v7 - if: env.IS_LINUX == 'true' && env.IS_DOCKER_PUSH_CONFIGURED == 'true' && env.IS_ARMV5 == 'false' + if: env.SHOULD_BUILD == 'true' && env.IS_LINUX == 'true' && env.IS_DOCKER_PUSH_CONFIGURED == 'true' && env.IS_ARMV5 == 'false' with: name: digests-${{ env.PLATFORM }} path: /tmp/digests/* @@ -467,8 +542,8 @@ jobs: contents: read packages: write runs-on: ubuntu-latest - needs: [build, check-push-enabled] - if: needs.check-push-enabled.outputs.is_enabled == 'true' + needs: [changes, build, check-push-enabled] + if: needs.check-push-enabled.outputs.is_enabled == 'true' && needs.changes.outputs.build == 'true' env: REGISTRY_IMAGE: ghcr.io/${{ github.repository }} steps: @@ -502,8 +577,8 @@ jobs: runs-on: ubuntu-latest permissions: contents: read - needs: [build, check-push-enabled] - if: needs.check-push-enabled.outputs.is_enabled == 'true' && vars.DOCKER_HUB_REPO != '' + needs: [changes, build, check-push-enabled] + if: needs.check-push-enabled.outputs.is_enabled == 'true' && vars.DOCKER_HUB_REPO != '' && needs.changes.outputs.build == 'true' continue-on-error: true steps: - uses: actions/checkout@v7 @@ -555,22 +630,26 @@ jobs: msi: name: Build Windows installers - needs: [build, git-version] + needs: [changes, build, git-version] runs-on: ubuntu-24.04 steps: - uses: actions/checkout@v7 + if: needs.changes.outputs.build == 'true' - uses: actions/download-artifact@v8 + if: needs.changes.outputs.build == 'true' with: path: ./binaries pattern: navidrome-windows* merge-multiple: true - name: Install Wix + if: needs.changes.outputs.build == 'true' run: sudo apt-get install -y wixl jq - name: Build MSI + if: needs.changes.outputs.build == 'true' env: GIT_TAG: ${{ needs.git-version.outputs.git_tag }} run: | @@ -580,6 +659,7 @@ jobs: du -h binaries/msi/*.msi - name: Upload MSI files + if: needs.changes.outputs.build == 'true' uses: actions/upload-artifact@v7 with: name: navidrome-windows-installers @@ -588,29 +668,33 @@ jobs: release: name: Package/Release - needs: [build, msi] + needs: [changes, build, msi] runs-on: ubuntu-latest outputs: package_list: ${{ steps.set-package-list.outputs.package_list }} steps: - uses: actions/checkout@v7 + if: needs.changes.outputs.build == 'true' with: fetch-depth: 0 fetch-tags: true - uses: actions/download-artifact@v8 + if: needs.changes.outputs.build == 'true' with: path: ./binaries pattern: navidrome-* merge-multiple: true - run: ls -lR ./binaries + if: needs.changes.outputs.build == 'true' - name: Set RELEASE_FLAGS for snapshot releases - if: env.IS_RELEASE == 'false' + if: needs.changes.outputs.build == 'true' && env.IS_RELEASE == 'false' run: echo 'RELEASE_FLAGS=--skip=publish --snapshot' >> $GITHUB_ENV - name: Run GoReleaser + if: needs.changes.outputs.build == 'true' uses: goreleaser/goreleaser-action@v7 with: version: '2.16.0' @@ -619,17 +703,20 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: Remove build artifacts + if: needs.changes.outputs.build == 'true' run: | ls -l ./dist rm ./dist/*.tar.gz ./dist/*.zip - name: Upload all-packages artifact + if: needs.changes.outputs.build == 'true' uses: actions/upload-artifact@v7 with: name: packages path: dist/navidrome_0* - id: set-package-list + if: needs.changes.outputs.build == 'true' name: Export list of generated packages run: | cd dist @@ -641,7 +728,10 @@ jobs: upload-packages: name: Upload Linux PKG runs-on: ubuntu-latest - needs: [release] + needs: [changes, release] + # Job-level skip is safe here: nothing needs this job, and fromJson would + # error on the empty package_list a skipped release leaves behind. + if: needs.changes.outputs.build == 'true' strategy: matrix: item: ${{ fromJson(needs.release.outputs.package_list) }} From 16b9f60a21cdbbc4cc11ec253199a5446e53a126 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 5 Sep 2026 13:47:20 -0400 Subject: [PATCH 002/104] ci: close change-detection gaps found in review Four changes, three of them gaps in the filters added by the previous commit. Go tests execute db/migrations/*.sql for real: persistence_suite_test.go calls db.Init, which runs goose against an in-memory database. `make migration-sql` produces a .sql-only diff, and six such commits exist in history, so those PRs would have run zero Go tests. Added ^db/migrations/ to the Go filter. validate-translations.sh reads ui/src/i18n/en.json as its reference, not resources/i18n/en.json, which does not exist. A commit that only removes an English key (dd4802c0c is one) would have skipped the only check that reports the orphaned keys left in all 36 translations. The coverage-artifact probe was unpaginated. A full Go pipeline run produces exactly 30 artifacts, the API default page size, and octocov-pr sorts to index 27 because the test jobs finish first. Run 33503006884 already produced 33 and pushed all three coverage artifacts off page one. Server-side ?name= filtering has no count ceiling. The script also now fails closed if the base ref cannot be resolved. The fetch itself stays non-fatal: actions/checkout already created the ref, so a failed refresh is harmless, and making it fatal would turn a transient blip into a red pipeline. Also adds workflow_dispatch, so a manual run is possible and gets every flag. --- .github/workflows/coverage-on-pr.yml | 4 ++-- .github/workflows/detect-changes.sh | 16 +++++++++++----- .github/workflows/pipeline.yml | 1 + 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/.github/workflows/coverage-on-pr.yml b/.github/workflows/coverage-on-pr.yml index 6f982b33a..7c6c026f4 100644 --- a/.github/workflows/coverage-on-pr.yml +++ b/.github/workflows/coverage-on-pr.yml @@ -21,8 +21,8 @@ jobs: env: GH_TOKEN: ${{ github.token }} run: | - count=$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/${{ github.event.workflow_run.id }}/artifacts" \ - --jq '[.artifacts[] | select(.name == "octocov-pr")] | length') + count=$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/${{ github.event.workflow_run.id }}/artifacts?name=octocov-pr" \ + --jq '.total_count') echo "count=$count" >> "$GITHUB_OUTPUT" # Only the config, from the base branch: this job holds a write token, so diff --git a/.github/workflows/detect-changes.sh b/.github/workflows/detect-changes.sh index 2e7a42586..d1a6760ad 100755 --- a/.github/workflows/detect-changes.sh +++ b/.github/workflows/detect-changes.sh @@ -9,8 +9,8 @@ # build - anything that ends up in a binary, image or package (i.e. every # change except the doc-only paths in $DOC_ONLY_RE) # -# Only pull requests are narrowed. Master pushes and tags always get every flag, -# so a release can never be built from a partially validated tree. +# Only pull requests are narrowed. Master pushes, tags and manual runs always get +# every flag, so a release can never be built from a partially validated tree. # # Flags gate STEPS, not jobs: a job-level skip propagates through the needs # chain (actions/runner#491) and would take the release jobs down with it. @@ -20,9 +20,9 @@ set -uo pipefail export LC_ALL=C -GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/)' +GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/|^db/migrations/)' JS_RE='^ui/' -I18N_RE='(^resources/i18n/|^\.github/workflows/validate-translations\.sh$)' +I18N_RE='(^resources/i18n/|^ui/src/i18n/en\.json$|^\.github/workflows/validate-translations\.sh$)' DOC_ONLY_RE='(\.md$|^LICENSE$|^\.git-blame-ignore-revs$|^\.gitignore$|^\.devcontainer/)' emit() { printf '%s=%s\n' "$1" "$2" | tee -a "${GITHUB_OUTPUT:-/dev/null}"; } @@ -34,7 +34,13 @@ if [ "${GITHUB_EVENT_NAME:-}" != "pull_request" ]; then fi BASE_REF="${BASE_REF:-master}" -git fetch --no-tags --quiet origin "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" +git fetch --no-tags --quiet origin "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" || true +# Guard the diff, not the fetch: checkout already created the ref, so a failed +# refresh is harmless, but an unresolvable ref would emit every flag as false. +if ! git rev-parse --verify --quiet "origin/${BASE_REF}" >/dev/null; then + printf '::error::Cannot resolve origin/%s. In CI, check out with fetch-depth: 0.\n' "$BASE_REF" >&2 + exit 1 +fi files="$(git diff --name-only "origin/${BASE_REF}...HEAD")" echo "Changed files:" diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml index bb318bf5c..cc90b370a 100644 --- a/.github/workflows/pipeline.yml +++ b/.github/workflows/pipeline.yml @@ -8,6 +8,7 @@ on: pull_request: branches: - master + workflow_dispatch: concurrency: group: ${{ startsWith(github.ref, 'refs/tags/v') && 'tag' || 'branch' }}-${{ github.ref }} From 596bbf91b43ae27aeeecddbb15e080b096dfe728 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 5 Sep 2026 13:50:44 -0400 Subject: [PATCH 003/104] ci: do not treat an empty artifact list as an error on a PR A pull request that changes nothing reaching a binary now builds nothing, so the download-link workflow finding no artifacts is an expected outcome rather than a problem worth an error annotation. --- .github/workflows/download-link-on-pr.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/download-link-on-pr.yml b/.github/workflows/download-link-on-pr.yml index 80ec18e90..fdfc82e08 100644 --- a/.github/workflows/download-link-on-pr.yml +++ b/.github/workflows/download-link-on-pr.yml @@ -35,8 +35,10 @@ jobs: const {data: {artifacts}} = await github.rest.actions.listWorkflowRunArtifacts({owner, repo, run_id}); const downloadable = artifacts.filter((art) => !art.name.startsWith('octocov-')); + // A PR that changes nothing reaching a binary builds nothing, so an + // empty list is expected rather than a problem. if (!downloadable.length) { - return core.error(`No artifacts found`); + return core.info(`No artifacts found`); } const header = `Download the artifacts for this pull request:`; let body = `${header}\n`; From 98e435490df32c9c19e882891b12680c04d8fe61 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 5 Sep 2026 13:59:47 -0400 Subject: [PATCH 004/104] ci: treat everything under tests/ as a Go change tests/ holds only Go test scaffolding: the shared mocks, tests/fixtures, and tests/navidrome-test.toml, which tests.Init reads for every suite. A change to any of it can alter a Go test result, and the directory can never collide with a frontend or docs path. This deliberately does not cover testdata/ generally. Measured over the full history, 99 of 103 pull requests touching testdata/ or tests/fixtures/ already matched the Go filter, and the single missed pull request was a frontend one that a broader pattern would have made worse by running the whole Go matrix. --- .github/workflows/detect-changes.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/detect-changes.sh b/.github/workflows/detect-changes.sh index d1a6760ad..3146abf66 100755 --- a/.github/workflows/detect-changes.sh +++ b/.github/workflows/detect-changes.sh @@ -20,7 +20,7 @@ set -uo pipefail export LC_ALL=C -GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/|^db/migrations/)' +GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/|^db/migrations/|^tests/)' JS_RE='^ui/' I18N_RE='(^resources/i18n/|^ui/src/i18n/en\.json$|^\.github/workflows/validate-translations\.sh$)' DOC_ONLY_RE='(\.md$|^LICENSE$|^\.git-blame-ignore-revs$|^\.gitignore$|^\.devcontainer/)' From 86e6c62cf186c50820b2ee181803f59d9fdf1a12 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 5 Sep 2026 14:09:39 -0400 Subject: [PATCH 005/104] ci: count the source path of a rename in change detection Rename detection makes `git diff --name-only` report only the destination, so moving a file out of a gated area dropped the source from every filter. A rename of ui/src/a.js to docs/a.js.md emitted all four flags as false, skipping the build for a change that deleted UI source. --no-renames turns the rename back into a delete plus an add, so both paths participate. It is also about 8x faster on a large diff, since rename detection is the expensive part, and it can only ever move a flag from false to true. --- .github/workflows/detect-changes.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/detect-changes.sh b/.github/workflows/detect-changes.sh index 3146abf66..138db4871 100755 --- a/.github/workflows/detect-changes.sh +++ b/.github/workflows/detect-changes.sh @@ -42,7 +42,9 @@ if ! git rev-parse --verify --quiet "origin/${BASE_REF}" >/dev/null; then exit 1 fi -files="$(git diff --name-only "origin/${BASE_REF}...HEAD")" +# --no-renames: rename detection reports only the destination, so moving a file +# out of a gated area would drop the source path from every filter. +files="$(git diff --no-renames --name-only "origin/${BASE_REF}...HEAD")" echo "Changed files:" printf '%s\n' "$files" | sed 's/^/ /' echo From d3bed0936315628438b498c3c3f2ecb643063602 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 5 Sep 2026 14:25:03 -0400 Subject: [PATCH 006/104] ci: fix three change-detection gaps found in review `grep -q` exits on its first match, which closes the pipe under the still- writing `printf`. Under `pipefail` the killed writer, not the successful match, sets the exit status, so a large diff emitted `false` for an area that had changed. It is a race rather than a buffer threshold: it starts firing at around 1200 paths, well under the 64 KiB pipe buffer, and it kills the second `grep` in the build pipeline too. Both pipelines are now here-strings. The largest diff in this repo's history is 519 files, so this was latent, but a repo-wide sweep or a mass directory move reaches it. pipeline.yml and this script are now inputs to the go, js and i18n filters. A pipeline-only change previously skipped every suite that pipeline.yml defines, so a broken test command merged green and surfaced on master. Worse, a wrong gating expression is invisible in every run: `outputs.golang` instead of `outputs.go` reads as empty on master pushes too, and the suite disappears silently and permanently. This change adds 63 such expressions. Of 82 historical pipeline-only commits, 38 edited a job that is now gated, and the measured cost is about 12 extra full runs a year. The download-link workflow returned before looking for its own previous comment, so a PR that built binaries and then became docs-only kept advertising artifacts from a commit that is no longer the head. That branch was unreachable until this PR gated the build. It now deletes the stale comment. Deleting rather than rewording is deliberate: the comment is matched by its header, so a reworded body would either have to keep a header that contradicts it or start posting duplicates. The lookup is also paginated now; it saw only the first 30 comments. --- .github/workflows/detect-changes.sh | 13 ++++++++----- .github/workflows/download-link-on-pr.yml | 19 +++++++++++++------ 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/.github/workflows/detect-changes.sh b/.github/workflows/detect-changes.sh index 138db4871..30987013d 100755 --- a/.github/workflows/detect-changes.sh +++ b/.github/workflows/detect-changes.sh @@ -9,6 +9,9 @@ # build - anything that ends up in a binary, image or package (i.e. every # change except the doc-only paths in $DOC_ONLY_RE) # +# pipeline.yml and this script are inputs to every suite they gate: a wrong +# gating expression skips a suite green, in every run, with no other signal. +# # Only pull requests are narrowed. Master pushes, tags and manual runs always get # every flag, so a release can never be built from a partially validated tree. # @@ -20,9 +23,9 @@ set -uo pipefail export LC_ALL=C -GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/|^db/migrations/|^tests/)' -JS_RE='^ui/' -I18N_RE='(^resources/i18n/|^ui/src/i18n/en\.json$|^\.github/workflows/validate-translations\.sh$)' +GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/|^db/migrations/|^tests/|^\.github/workflows/(pipeline\.yml|detect-changes\.sh)$)' +JS_RE='(^ui/|^\.github/workflows/(pipeline\.yml|detect-changes\.sh)$)' +I18N_RE='(^resources/i18n/|^ui/src/i18n/en\.json$|^\.github/workflows/validate-translations\.sh$|^\.github/workflows/(pipeline\.yml|detect-changes\.sh)$)' DOC_ONLY_RE='(\.md$|^LICENSE$|^\.git-blame-ignore-revs$|^\.gitignore$|^\.devcontainer/)' emit() { printf '%s=%s\n' "$1" "$2" | tee -a "${GITHUB_OUTPUT:-/dev/null}"; } @@ -50,14 +53,14 @@ printf '%s\n' "$files" | sed 's/^/ /' echo flag() { # $1=name $2=regex - if printf '%s\n' "$files" | grep -qE "$2"; then emit "$1" true; else emit "$1" false; fi + if grep -qE "$2" <<< "$files"; then emit "$1" true; else emit "$1" false; fi } flag go "$GO_RE" flag js "$JS_RE" flag i18n "$I18N_RE" -if printf '%s\n' "$files" | grep -vE "$DOC_ONLY_RE" | grep -q '[^[:space:]]'; then +if [ -n "$(grep -vE "$DOC_ONLY_RE" <<< "$files")" ]; then emit build true else emit build false diff --git a/.github/workflows/download-link-on-pr.yml b/.github/workflows/download-link-on-pr.yml index fdfc82e08..115eb2e34 100644 --- a/.github/workflows/download-link-on-pr.yml +++ b/.github/workflows/download-link-on-pr.yml @@ -35,20 +35,27 @@ jobs: const {data: {artifacts}} = await github.rest.actions.listWorkflowRunArtifacts({owner, repo, run_id}); const downloadable = artifacts.filter((art) => !art.name.startsWith('octocov-')); - // A PR that changes nothing reaching a binary builds nothing, so an - // empty list is expected rather than a problem. + const header = `Download the artifacts for this pull request:`; + + const comments = await github.paginate(github.rest.issues.listComments, {repo, owner, issue_number}); + // Match on the body too: octocov also comments as github-actions[bot]. + const existing_comment = comments.find((c) => c.user.login === 'github-actions[bot]' && c.body.startsWith(header)); + + // A PR that changes nothing reaching a binary builds nothing. Delete + // rather than reword: the matcher above keys off the header. if (!downloadable.length) { + if (existing_comment) { + core.info(`Deleting stale comment ${existing_comment.id}`); + await github.rest.issues.deleteComment({repo, owner, comment_id: existing_comment.id}); + } return core.info(`No artifacts found`); } - const header = `Download the artifacts for this pull request:`; + let body = `${header}\n`; for (const art of downloadable) { body += `\n* [${art.name}.zip](https://nightly.link/${owner}/${repo}/actions/artifacts/${art.id}.zip)`; } - const {data: comments} = await github.rest.issues.listComments({repo, owner, issue_number}); - // Match on the body too: octocov also comments as github-actions[bot]. - const existing_comment = comments.find((c) => c.user.login === 'github-actions[bot]' && c.body.startsWith(header)); if (existing_comment) { core.info(`Updating comment ${existing_comment.id}`); await github.rest.issues.updateComment({repo, owner, comment_id: existing_comment.id, body}); From 4f3e12738a42bef07ba823d92726663ebb05f62d Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 5 Sep 2026 14:33:36 -0400 Subject: [PATCH 007/104] ci: delete the stale coverage comment when a run produces no profile A PR that reverts its Go changes but keeps a doc change produces no coverage artifact, so every step after the probe skips, octocov never runs, and its comment for the earlier head stays on the PR reporting a delta that is zero by construction. Gating those steps is what made this reachable. The comment is matched on ``, the signature octocov appends and keys `updatePrevious` off itself, which is also what distinguishes it from the download-link comment posted by the same bot. The PR number comes from the base repo's workflow_run head sha rather than from the artifact, so the fork-controlled-input cross-check that guards the profile path does not apply: nothing is downloaded or executed on this path. --- .github/workflows/coverage-on-pr.yml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/.github/workflows/coverage-on-pr.yml b/.github/workflows/coverage-on-pr.yml index 7c6c026f4..66405beda 100644 --- a/.github/workflows/coverage-on-pr.yml +++ b/.github/workflows/coverage-on-pr.yml @@ -25,6 +25,23 @@ jobs: --jq '.total_count') echo "count=$count" >> "$GITHUB_OUTPUT" + # A PR that reverts its Go changes produces no artifact, but octocov's + # comment for the earlier head stays. Match the marker it keys off itself. + - name: Delete the stale coverage comment + if: steps.artifact.outputs.count == '0' + env: + GH_TOKEN: ${{ github.token }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + run: | + number=$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls?state=open&per_page=100" \ + --jq ".[] | select(.head.sha == \"$HEAD_SHA\") | .number" | head -n1) + [ -n "$number" ] || exit 0 + id=$(gh api --paginate "repos/$GITHUB_REPOSITORY/issues/$number/comments" \ + --jq '.[] | select(.user.login == "github-actions[bot]" and (.body | contains(""))) | .id' | head -n1) + if [ -n "$id" ]; then + gh api -X DELETE "repos/$GITHUB_REPOSITORY/issues/comments/$id" + fi + # Only the config, from the base branch: this job holds a write token, so # it must never check out the fork. - name: Check out the octocov config From dd6d9b7891476b360beaa4bdc1eeb249c3d5634b Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 5 Sep 2026 14:40:35 -0400 Subject: [PATCH 008/104] ci: treat the plugin manifest schema as a Go change plugins/manifest.go:37 declares manifest-schema.json as the input to a real go:generate directive producing manifest_gen.go, so a schema-only edit skipped the very check that catches a forgotten regeneration. Verified by adding a property to the schema and re-running go generate, which changed the output. All nine commits that ever touched the schema were already caught, but that is the check working rather than evidence it is unnecessary: an author who forgets to regenerate cannot reach merged history while the check runs. This is the only gap of its kind. The repo has two go:generate directives, and wire's inputs are all Go source. --- .github/workflows/detect-changes.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/detect-changes.sh b/.github/workflows/detect-changes.sh index 30987013d..c70c0c0c0 100755 --- a/.github/workflows/detect-changes.sh +++ b/.github/workflows/detect-changes.sh @@ -3,7 +3,8 @@ # Emits per-area change flags to $GITHUB_OUTPUT so the pipeline can skip work a # pull request cannot affect: # -# go - Go sources, module files, linter config, embedded resources +# go - Go sources, module files, linter config, embedded resources and +# the go:generate inputs whose generated output CI verifies # js - anything under ui/ # i18n - translation files and their validation script # build - anything that ends up in a binary, image or package (i.e. every @@ -23,7 +24,7 @@ set -uo pipefail export LC_ALL=C -GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/|^db/migrations/|^tests/|^\.github/workflows/(pipeline\.yml|detect-changes\.sh)$)' +GO_RE='(\.go$|(^|/)go\.(mod|sum)$|^Makefile$|^\.golangci\.yml$|^resources/|^db/migrations/|^tests/|^plugins/manifest-schema\.json$|^\.github/workflows/(pipeline\.yml|detect-changes\.sh)$)' JS_RE='(^ui/|^\.github/workflows/(pipeline\.yml|detect-changes\.sh)$)' I18N_RE='(^resources/i18n/|^ui/src/i18n/en\.json$|^\.github/workflows/validate-translations\.sh$|^\.github/workflows/(pipeline\.yml|detect-changes\.sh)$)' DOC_ONLY_RE='(\.md$|^LICENSE$|^\.git-blame-ignore-revs$|^\.gitignore$|^\.devcontainer/)' From 27abc83ac8b1466362a9148813f3ab5c988d93e9 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 6 Sep 2026 23:49:28 -0400 Subject: [PATCH 009/104] ci: add the detect-changes test suite Thirty assertions over the change shapes the filters have to get right, including the ones that only appeared under review: a large diff that used to lose flags to SIGPIPE, renames that hide their source path, and an unresolvable base ref that must fail closed rather than emit every flag as false. It builds a throwaway repo per case, so it needs no fixtures and leaves nothing behind. Nothing runs it in CI yet; it is a development tool for whoever changes a regex next. --- .github/workflows/detect-changes_test.sh | 110 +++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100755 .github/workflows/detect-changes_test.sh diff --git a/.github/workflows/detect-changes_test.sh b/.github/workflows/detect-changes_test.sh new file mode 100755 index 000000000..18e1e8d4a --- /dev/null +++ b/.github/workflows/detect-changes_test.sh @@ -0,0 +1,110 @@ +#!/usr/bin/env bash +# +# Tests detect-changes.sh against a throwaway repo: builds a synthetic PR for +# each change shape and asserts the four emitted flags. +# +# ./.github/workflows/detect-changes_test.sh # tests the sibling script +# ./.github/workflows/detect-changes_test.sh # tests another copy +# +# To confirm a case still bites, edit a pattern out of detect-changes.sh and +# re-run: exactly the case that covers it should fail. +set -uo pipefail +SCRIPT="${1:-$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/detect-changes.sh}" +T=$(mktemp -d); O=$(mktemp -d) +cd "$T" +git init -q -b master . +git config user.email t@t; git config user.name t +mkdir -p ui/src/i18n resources/i18n .github/workflows db/migrations +echo x > README.md; echo x > main.go; echo x > ui/src/a.js +echo x > resources/i18n/pt.json; echo x > ui/src/i18n/en.json; echo x > go.mod +git add -A; git commit -qm base +git clone -q --bare . "$O/origin.git" +git remote add origin "$O/origin.git" +git fetch -q origin + +fails=0 +run() { # $1=label $2=expected "go js i18n build" ; rest=files + label="$1"; want="$2"; shift 2 + git checkout -q -B test master + for f in "$@"; do mkdir -p "$(dirname "$f")"; echo change >> "$f"; done + git add -A >/dev/null; git commit -qm "$label" + got=$(GITHUB_EVENT_NAME=pull_request BASE_REF=master bash "$SCRIPT" 2>&1 \ + | grep -E '^(go|js|i18n|build)=' | cut -d= -f2 | tr '\n' ' ' | sed 's/ $//') + if [ "$got" = "$want" ]; then printf 'ok %-32s %s\n' "$label" "$got" + else printf 'FAIL %-32s got[%s] want[%s]\n' "$label" "$got" "$want"; fails=$((fails+1)); fi +} + +# go js i18n build +run "docs only" "false false false false" README.md +run "gitignore only" "false false false false" .gitignore +run "go only" "true false false true" core/thing.go +run "ui only" "false true false true" ui/src/b.js +run "i18n resources" "true false true true" resources/i18n/fr.json +run "ui en.json" "false true true true" ui/src/i18n/en.json +run "ui other i18n" "false true false true" ui/src/i18n/provider.js +run "db migration sql" "true false false true" db/migrations/20260101000000_x.sql +run "tests fixture" "true false false true" tests/fixtures/playlist.m3u +run "tests toml" "true false false true" tests/navidrome-test.toml +run "conf testdata" "false false false true" conf/testdata/cfg.toml +run "manifest schema" "true false false true" plugins/manifest-schema.json +run "other plugin json" "false false false true" plugins/testdata/fake/manifest-schema.json +run "nested go.mod" "true false false true" plugins/testdata/x/go.mod +run "Dockerfile only" "false false false true" Dockerfile +run "pipeline.yml" "true true true true" .github/workflows/pipeline.yml +run "detect-changes.sh" "true true true true" .github/workflows/detect-changes.sh +run "other workflow" "false false false true" .github/workflows/stale.yml +run "validate-trans.sh" "false false true true" .github/workflows/validate-translations.sh + +echo "--- large diff must not lose flags to SIGPIPE ---" +git checkout -q -B test master +mkdir -p big/pkg +python3 -c " +import os +os.makedirs('big/pkg', exist_ok=True) +open('big/pkg/aaa_first.go','w').write('x') +for i in range(2500): open('big/pkg/filler_%04d.txt' % i,'w').write('x') +" +git add -A >/dev/null; git commit -qm big +nfiles=$(git diff --no-renames --name-only master...HEAD | wc -l | tr -d ' ') +for i in 1 2 3 4 5; do + got=$(GITHUB_EVENT_NAME=pull_request BASE_REF=master bash "$SCRIPT" 2>&1 \ + | grep -E '^(go|js|i18n|build)=' | cut -d= -f2 | tr '\n' ' ' | sed 's/ $//') + if [ "$got" = "true false false true" ]; then printf 'ok %-32s %s (%s files)\n' "large diff run $i" "$got" "$nfiles" + else printf 'FAIL %-32s got[%s] want[true false false true] (%s files)\n' "large diff run $i" "$got" "$nfiles"; fails=$((fails+1)); fi +done + +echo "--- renames must count the source path ---" +rn() { # $1=label $2=expected $3=from $4=to + git checkout -q -B test master + mkdir -p "$(dirname "$4")"; git mv "$3" "$4" + git add -A >/dev/null; git commit -qm "$1" + got=$(GITHUB_EVENT_NAME=pull_request BASE_REF=master bash "$SCRIPT" 2>&1 \ + | grep -E '^(go|js|i18n|build)=' | cut -d= -f2 | tr '\n' ' ' | sed 's/ $//') + if [ "$got" = "$2" ]; then printf 'ok %-32s %s\n' "$1" "$got" + else printf 'FAIL %-32s got[%s] want[%s]\n' "$1" "$got" "$2"; fails=$((fails+1)); fi +} +# go js i18n build +rn "ui .js -> docs .md" "false true false true" ui/src/a.js docs/a.js.md +rn "go -> docs .md" "true false false true" main.go docs/main.go.md + +echo "--- non-PR events ---" +git checkout -q master +for ev in push workflow_dispatch; do + got=$(GITHUB_EVENT_NAME=$ev bash "$SCRIPT" 2>&1 | grep -E '^(go|js|i18n|build)=' | cut -d= -f2 | tr '\n' ' ' | sed 's/ $//') + if [ "$got" = "true true true true" ]; then printf 'ok %-32s %s\n' "$ev" "$got" + else printf 'FAIL %-32s got[%s]\n' "$ev" "$got"; fails=$((fails+1)); fi +done + +echo "--- unresolvable base ref must fail closed ---" +git checkout -q -B test master; echo x >> main.go; git add -A >/dev/null; git commit -qm x +out=$(GITHUB_EVENT_NAME=pull_request BASE_REF=does-not-exist bash "$SCRIPT" 2>&1); rc=$? +if [ "$rc" != "0" ] && ! grep -qE '^(go|js|i18n|build)=' <<<"$out"; then + printf 'ok %-32s exit=%s, no flags emitted\n' "bad base ref" "$rc" +else + printf 'FAIL %-32s exit=%s out[%s]\n' "bad base ref" "$rc" "$out"; fails=$((fails+1)) +fi + +echo +echo "failures: $fails" +cd /; rm -rf "$T" "$O" +exit "$fails" From 48af781b82524fcb1dd9e4b1a0a3d35c7405428b Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 13:47:45 -0400 Subject: [PATCH 010/104] fix(reflex): exclude .worktrees from the reflex configuration regex --- reflex.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reflex.conf b/reflex.conf index 47dd775ab..1cbaa7bb7 100644 --- a/reflex.conf +++ b/reflex.conf @@ -1 +1 @@ --s -r "(\.go$$|\.cpp$$|\.h$$|navidrome.toml|resources|token_received.html)" -R "(^ui|^data|^db/migrations)" -R "_test\.go$$" -- go run -race -tags netgo,sqlite_fts5 . +-s -r "(\.go$$|\.cpp$$|\.h$$|navidrome.toml|resources|token_received.html)" -R "(^ui|^data|^db/migrations)" -R "_test\.go$$" -R "^\.worktrees" -- go run -race -tags netgo,sqlite_fts5 . From 404837799b9e7fc378625a1e8e1834df5c3533a3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 13:43:26 -0400 Subject: [PATCH 011/104] fix(subsonic): don't re-encode a source already in the player's forced format When a player has a forced transcoding format, ClientInfo.ForceFormat cleared DirectPlayProfiles unconditionally. A FLAC source on a player configured to transcode to FLAC was therefore re-encoded to FLAC, wasting CPU and bandwidth for no gain. Worse, the transcoder pipes ffmpeg output to stdout, so the resulting FLAC has total_samples=0 and no seek table -- an offline copy of it can never be seeked. Reported against getTranscodeDecision by the Symfonium author. ForceFormat now rebuilds DirectPlayProfiles from the matching transcoding profiles instead of dropping them: a client declaring a transcoding profile for a format is proof it can consume that format, so a source already in it is served as-is. Container and codec come from resolveTargetFormat, so a legacy "oga" target_format yields an ogg/opus profile, and the profile's MaxAudioChannels is carried across. DirectPlayProfile has no bitrate field, so restoring direct play needs a ceiling to keep an over-bitrate source out of it. GetTranscodeDecision now seeds that ceiling from the transcoding row's DefaultBitRate when a format was successfully forced, with the player's own MaxBitRate still taking precedence. This also closes a gap where the new endpoint ignored DefaultBitRate entirely: an mp3 320 source on a player forced to mp3@192 was served at 320, while the legacy /rest/stream path correctly gave 192. Applied via CapBitrate, which only ever lowers, so a client declaring a stricter limit keeps it. The legacy path (applyServerOverride) is untouched -- ForceFormat has no other callers. --- core/stream/decider_test.go | 76 +++++++++++++++++++++++++++++++ core/stream/types.go | 26 +++++++---- core/stream/types_test.go | 32 ++++++++++++- server/subsonic/transcode.go | 25 ++++++---- server/subsonic/transcode_test.go | 45 +++++++++++++++++- 5 files changed, 183 insertions(+), 21 deletions(-) diff --git a/core/stream/decider_test.go b/core/stream/decider_test.go index 577207636..01fef1249 100644 --- a/core/stream/decider_test.go +++ b/core/stream/decider_test.go @@ -1144,6 +1144,82 @@ var _ = Describe("Decider", func() { }) }) + Context("Player-forced format", func() { + symfonium := func() *ClientInfo { + return &ClientInfo{ + Name: "Symfonium", + DirectPlayProfiles: []DirectPlayProfile{ + {Containers: []string{"mp3", "flac", "ogg"}, Protocols: []string{ProtocolHTTP}}, + }, + TranscodingProfiles: []Profile{ + {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, + {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, + }, + } + } + + It("direct plays a flac source forced to flac", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1026, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) + ci := symfonium() + Expect(ci.ForceFormat("flac")).To(BeTrue()) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeTrue()) + }) + + It("still transcodes a 24-bit flac when the client caps bit depth", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 4600, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) + ci := symfonium() + ci.CodecProfiles = []CodecProfile{{ + Type: CodecProfileTypeAudio, Name: "flac", + Limitations: []Limitation{{Name: LimitationAudioBitdepth, Comparison: ComparisonLessThanEqual, Values: []string{"16"}, Required: true}}, + }} + Expect(ci.ForceFormat("flac")).To(BeTrue()) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeFalse()) + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TranscodeStream.BitDepth).To(Equal(16)) + }) + + It("still transcodes a 320 mp3 forced to mp3 at a lower bitrate", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 320, Channels: 2, SampleRate: 44100}) + ci := symfonium() + Expect(ci.ForceFormat("mp3")).To(BeTrue()) + ci.CapBitrate(192) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeFalse()) + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TargetBitrate).To(Equal(192)) + }) + + It("direct plays a 128 mp3 forced to mp3 at a higher bitrate", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 128, Channels: 2, SampleRate: 44100}) + ci := symfonium() + Expect(ci.ForceFormat("mp3")).To(BeTrue()) + ci.CapBitrate(192) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeTrue()) + }) + + It("transcodes a flac source forced to mp3", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1026, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) + ci := symfonium() + Expect(ci.ForceFormat("mp3")).To(BeTrue()) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeFalse()) + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TargetFormat).To(Equal("mp3")) + }) + }) }) Describe("ensureProbed", func() { diff --git a/core/stream/types.go b/core/stream/types.go index 19474dd91..300017c01 100644 --- a/core/stream/types.go +++ b/core/stream/types.go @@ -59,28 +59,38 @@ func (ci *ClientInfo) CapBitrate(maxKbps int) bool { return changed } -// ForceFormat narrows the client to transcoding to targetFormat and suppresses -// direct play, but only if the client already declares a profile for that -// format. All matching profiles are kept so negotiation can still pick among -// them (e.g. by protocol). Returns false (no-op) when targetFormat is empty or -// unsupported. +// ForceFormat narrows the client to transcoding to targetFormat, but only if the +// client already declares a profile for it. All matching profiles are kept so +// negotiation can still pick among them (e.g. by protocol). Direct play is rebuilt +// from those profiles rather than dropped, since declaring a transcoding profile +// for a format is proof the client can play it. Returns false when unsupported. func (ci *ClientInfo) ForceFormat(targetFormat string) bool { if targetFormat == "" { return false } var matched []Profile + var directPlay []DirectPlayProfile for i := range ci.TranscodingProfiles { + p := &ci.TranscodingProfiles[i] // matchesContainer is alias-aware, so a forced "oga" (legacy Opus // target_format) still matches a resolved "opus" profile. - if _, format := resolveTargetFormat(&ci.TranscodingProfiles[i]); matchesContainer(format, []string{targetFormat}) { - matched = append(matched, ci.TranscodingProfiles[i]) + container, format := resolveTargetFormat(p) + if !matchesContainer(format, []string{targetFormat}) { + continue } + matched = append(matched, *p) + directPlay = append(directPlay, DirectPlayProfile{ + Containers: []string{container}, + AudioCodecs: []string{format}, + Protocols: []string{ProtocolHTTP}, + MaxAudioChannels: p.MaxAudioChannels, + }) } if len(matched) == 0 { return false } ci.TranscodingProfiles = matched - ci.DirectPlayProfiles = nil + ci.DirectPlayProfiles = directPlay return true } diff --git a/core/stream/types_test.go b/core/stream/types_test.go index eff408362..88ad904a7 100644 --- a/core/stream/types_test.go +++ b/core/stream/types_test.go @@ -58,7 +58,7 @@ var _ = Describe("ClientInfo", func() { }) Describe("ForceFormat", func() { - It("restricts to the forced format and clears direct play when supported", func() { + It("restricts direct play to the forced format when supported", func() { ci := &ClientInfo{ DirectPlayProfiles: []DirectPlayProfile{{Containers: []string{"flac"}, AudioCodecs: []string{"flac"}}}, TranscodingProfiles: []Profile{ @@ -71,7 +71,35 @@ var _ = Describe("ClientInfo", func() { Expect(ok).To(BeTrue()) Expect(ci.TranscodingProfiles).To(HaveLen(1)) Expect(ci.TranscodingProfiles[0].AudioCodec).To(Equal("opus")) - Expect(ci.DirectPlayProfiles).To(BeEmpty()) + Expect(ci.DirectPlayProfiles).To(ConsistOf(DirectPlayProfile{ + Containers: []string{"ogg"}, AudioCodecs: []string{"opus"}, Protocols: []string{ProtocolHTTP}, + })) + }) + + It("keeps direct play for a source already in the forced format", func() { + ci := &ClientInfo{ + DirectPlayProfiles: []DirectPlayProfile{{Containers: []string{"flac"}, AudioCodecs: []string{"flac"}}}, + TranscodingProfiles: []Profile{ + {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, + {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, + }, + } + ok := ci.ForceFormat("flac") + Expect(ok).To(BeTrue()) + Expect(ci.DirectPlayProfiles).To(ConsistOf(DirectPlayProfile{ + Containers: []string{"flac"}, AudioCodecs: []string{"flac"}, Protocols: []string{ProtocolHTTP}, + })) + }) + + It("carries the channel limit of the forced profile into direct play", func() { + ci := &ClientInfo{ + TranscodingProfiles: []Profile{ + {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP, MaxAudioChannels: 2}, + }, + } + Expect(ci.ForceFormat("flac")).To(BeTrue()) + Expect(ci.DirectPlayProfiles).To(HaveLen(1)) + Expect(ci.DirectPlayProfiles[0].MaxAudioChannels).To(Equal(2)) }) It("matches a container-only forced format (mp3)", func() { diff --git a/server/subsonic/transcode.go b/server/subsonic/transcode.go index 9eb2af160..d64bce605 100644 --- a/server/subsonic/transcode.go +++ b/server/subsonic/transcode.go @@ -280,12 +280,19 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) return stream.IsAACCodec(p.Container) }) + player, hasPlayer := request.PlayerFrom(ctx) + // Honor the player's forced transcoding format, falling back to normal // negotiation when the client can't play it (issue #5583). + maxBitRate := 0 if trc, ok := request.TranscodingFrom(ctx); ok && trc.TargetFormat != "" { - if !clientInfo.ForceFormat(trc.TargetFormat) { + if clientInfo.ForceFormat(trc.TargetFormat) { + // DirectPlayProfile carries no bitrate, so this ceiling is the only + // thing keeping an over-bitrate source out of direct play. + maxBitRate = trc.DefaultBitRate + } else { clientName := clientInfo.Name - if player, ok := request.PlayerFrom(ctx); ok && player.Client != "" { + if hasPlayer && player.Client != "" { clientName = player.Client } log.Debug(ctx, "Player forced format not supported by client; falling back to negotiation", @@ -293,13 +300,13 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) } } - // Apply the player's MaxBitRate as a ceiling on the client's declared - // limits (issue #5583). Both fields are capped because the client sends - // them independently here; capping only MaxAudioBitrate would let an - // independent MaxTranscodingAudioBitrate slip through computeBitrate. - if player, ok := request.PlayerFrom(ctx); ok && clientInfo.CapBitrate(player.MaxBitRate) { - log.Debug(ctx, "Applied player MaxBitRate cap to transcode decision", - "playerMaxBitRate", player.MaxBitRate, "client", clientInfo.Name) + // The player's own MaxBitRate outranks the forced-format default (issue #5583). + if hasPlayer && player.MaxBitRate > 0 { + maxBitRate = player.MaxBitRate + } + if clientInfo.CapBitrate(maxBitRate) { + log.Debug(ctx, "Applied bitrate ceiling to transcode decision", + "maxBitRate", maxBitRate, "client", clientInfo.Name) } // Get media file diff --git a/server/subsonic/transcode_test.go b/server/subsonic/transcode_test.go index 8d5cbb974..0f3c24832 100644 --- a/server/subsonic/transcode_test.go +++ b/server/subsonic/transcode_test.go @@ -369,7 +369,7 @@ var _ = Describe("Transcode endpoints", func() { mockTD.token = "token" }) - It("forces a supported format and clears direct play", func() { + It("forces a supported format and narrows direct play to it", func() { body := `{"directPlayProfiles":[{"containers":["flac"],"audioCodecs":["flac"],"protocols":["http"]}], "transcodingProfiles":[{"container":"ogg","audioCodec":"opus","protocol":"http"}, {"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` @@ -380,7 +380,11 @@ var _ = Describe("Transcode endpoints", func() { Expect(err).ToNot(HaveOccurred()) Expect(mockTD.capturedClient.TranscodingProfiles).To(HaveLen(1)) Expect(mockTD.capturedClient.TranscodingProfiles[0].AudioCodec).To(Equal("opus")) - Expect(mockTD.capturedClient.DirectPlayProfiles).To(BeEmpty()) + Expect(mockTD.capturedClient.DirectPlayProfiles).To(ConsistOf(stream.DirectPlayProfile{ + Containers: []string{"ogg"}, + AudioCodecs: []string{"opus"}, + Protocols: []string{"http"}, + })) }) It("falls back to negotiation when the forced format is unsupported", func() { @@ -416,6 +420,43 @@ var _ = Describe("Transcode endpoints", func() { Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(128)) Expect(mockTD.capturedClient.MaxTranscodingAudioBitrate).To(Equal(128)) }) + + withForcedBitRate := func(r *http.Request, format string, defaultBitRate, playerMaxBitRate int) *http.Request { + ctx := request.WithTranscoding(r.Context(), model.Transcoding{TargetFormat: format, DefaultBitRate: defaultBitRate}) + ctx = request.WithPlayer(ctx, model.Player{Client: "NavidromeUI", MaxBitRate: playerMaxBitRate}) + return r.WithContext(ctx) + } + + It("applies the transcoding default bitrate when the player sets no maxBitRate", func() { + body := `{"transcodingProfiles":[{"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedBitRate(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "mp3", 192, 0) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(192)) + Expect(mockTD.capturedClient.MaxTranscodingAudioBitrate).To(Equal(192)) + }) + + It("prefers the player maxBitRate over the transcoding default bitrate", func() { + body := `{"transcodingProfiles":[{"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedBitRate(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "mp3", 192, 320) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(320)) + }) + + It("ignores the transcoding default bitrate when the forced format is unsupported", func() { + body := `{"transcodingProfiles":[{"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedBitRate(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "opus", 192, 0) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(BeZero()) + }) }) }) From 89026012ab3d9aeddcd7bbdf965fa9d6e0806b62 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 14:19:45 -0400 Subject: [PATCH 012/104] fix(transcoding): make piped FLAC transcodes seekable The FLAC muxer writes STREAMINFO before it knows the stream length, then rewinds at the end to fill total_samples in. Navidrome pipes ffmpeg's stdout (-f flac -), which is not seekable, so ffmpeg logs "unable to rewrite FLAC header" and the field stays 0. A decoder needs total_samples to turn a timestamp into a byte offset, so it reports an unknown duration and refuses to seek. Online playback hides this because the client re-requests with a new offset each time, but an offline copy is permanently unseekable, the symptom reported against Symfonium where seeking a downloaded track jumps back to the start. Transcode now wraps its own output and rewrites total_samples as the first bytes flow past. This lives in core/ffmpeg because the unseekable pipe is that package's doing: buildDynamicArgs is what appends the trailing '-'. core/stream only learns a target format and hands back an io.ReadCloser, so compensating there leaked a transcoder implementation detail one layer up. TranscodeOptions grows a Duration field alongside the existing Offset, which also puts the duration-minus-offset arithmetic in the same function that emits -ss. The wrapper runs on every transcode rather than only FLAC targets: the format on a transcoding row is a declared target that nothing validates against the command's actual -f, so a custom command can emit FLAC under any target_format. The magic-byte check inside the wrapper is the authoritative test and costs a 26-byte peek. The output sample rate is read back out of the header ffmpeg just wrote rather than taken from the transcode options, so a resampled (-ar) output still gets the right count. Anything that is not a FLAC stream with an unset total_samples passes through byte for byte. Measured on a 177s source: before, total_samples=0 and ffprobe reported duration N/A; after, total_samples=7807023 and duration 177.03s, with the audio payload byte-identical. This affects every piped FLAC regardless of the source format; only FLAC stores an authoritative "unknown", which is why mp3, opus and aac survive the same pipe. No SEEKTABLE is synthesised and the MD5 is left zero: both are optional, and decoders binary-search using total_samples alone. --- core/ffmpeg/ffmpeg.go | 17 ++-- core/ffmpeg/ffmpeg_test.go | 35 +++++++ core/ffmpeg/flac_streaminfo.go | 66 +++++++++++++ core/ffmpeg/flac_streaminfo_test.go | 142 ++++++++++++++++++++++++++++ core/stream/media_streamer.go | 1 + 5 files changed, 255 insertions(+), 6 deletions(-) create mode 100644 core/ffmpeg/flac_streaminfo.go create mode 100644 core/ffmpeg/flac_streaminfo_test.go diff --git a/core/ffmpeg/ffmpeg.go b/core/ffmpeg/ffmpeg.go index af2dab647..cc38dd9de 100644 --- a/core/ffmpeg/ffmpeg.go +++ b/core/ffmpeg/ffmpeg.go @@ -27,11 +27,12 @@ type TranscodeOptions struct { Command string // DB command template (used to detect custom vs default) Format string // Target format (mp3, opus, aac, flac) FilePath string - BitRate int // kbps, 0 = codec default - SampleRate int // 0 = no constraint - Channels int // 0 = no constraint - BitDepth int // 0 = no constraint; valid values: 16, 24, 32 - Offset int // seconds + BitRate int // kbps, 0 = codec default + SampleRate int // 0 = no constraint + Channels int // 0 = no constraint + BitDepth int // 0 = no constraint; valid values: 16, 24, 32 + Offset int // seconds + Duration float32 // seconds; 0 = unknown. Only used to repair a piped FLAC header. } // AudioProbeResult contains authoritative audio stream properties from ffprobe. @@ -86,7 +87,11 @@ func (e *ffmpeg) Transcode(ctx context.Context, opts TranscodeOptions) (io.ReadC } else { args = buildTemplateArgs(opts) } - return e.start(ctx, args) + out, err := e.start(ctx, args) + if err != nil { + return nil, err + } + return patchFLACDuration(out, opts.Duration-float32(opts.Offset)), nil } func (e *ffmpeg) ConvertAnimatedImage(ctx context.Context, reader io.Reader, maxSize int, quality int) (io.ReadCloser, error) { diff --git a/core/ffmpeg/ffmpeg_test.go b/core/ffmpeg/ffmpeg_test.go index 0fa3de111..dbc8fa3c8 100644 --- a/core/ffmpeg/ffmpeg_test.go +++ b/core/ffmpeg/ffmpeg_test.go @@ -3,6 +3,7 @@ package ffmpeg import ( "context" "errors" + "io" "os" "os/exec" "path/filepath" @@ -684,6 +685,40 @@ var _ = Describe("ffmpeg", func() { }) Expect(err).To(MatchError(context.Canceled)) }) + + It("fills in total_samples on a piped FLAC transcode", func() { + stream, err := ff.Transcode(GinkgoT().Context(), TranscodeOptions{ + Command: "ffmpeg -i %s -map 0:a:0 -v 0 -c:a flac -f flac -", + Format: "flac", + FilePath: "tests/fixtures/test.flac", + Duration: 1, // the fixture is exactly 1s at 44100Hz + }) + Expect(err).ToNot(HaveOccurred()) + defer stream.Close() + + out, err := io.ReadAll(stream) + Expect(err).ToNot(HaveOccurred()) + Expect(string(out[:4])).To(Equal("fLaC")) + Expect(readTotalSamples(out)).To(Equal(uint64(44100))) + }) + + It("patches the duration net of the requested offset", func() { + // The command has no %t, so ffmpeg still emits the whole fixture. + // What is under test is the header arithmetic, not the audio. + stream, err := ff.Transcode(GinkgoT().Context(), TranscodeOptions{ + Command: "ffmpeg -i %s -map 0:a:0 -v 0 -c:a flac -f flac -", + Format: "flac", + FilePath: "tests/fixtures/test.flac", + Duration: 3, + Offset: 1, + }) + Expect(err).ToNot(HaveOccurred()) + defer stream.Close() + + out, err := io.ReadAll(stream) + Expect(err).ToNot(HaveOccurred()) + Expect(readTotalSamples(out)).To(Equal(uint64(2 * 44100))) + }) }) Context("stderr capture", func() { diff --git a/core/ffmpeg/flac_streaminfo.go b/core/ffmpeg/flac_streaminfo.go new file mode 100644 index 000000000..878c28718 --- /dev/null +++ b/core/ffmpeg/flac_streaminfo.go @@ -0,0 +1,66 @@ +package ffmpeg + +import ( + "bytes" + "encoding/binary" + "errors" + "io" + "math" +) + +const ( + flacPrefixLen = 26 // through the last total_samples byte + flacMaxTotalSamples = 1<<36 - 1 +) + +// patchFLACDuration fills in the STREAMINFO total_samples that ffmpeg leaves at 0 +// when writing to a pipe, since a decoder cannot seek a cached FLAC without it. +func patchFLACDuration(r io.ReadCloser, duration float32) io.ReadCloser { + if duration <= 0 { + return r + } + return &flacPatcher{ReadCloser: r, duration: duration} +} + +type flacPatcher struct { + io.ReadCloser + duration float32 + // Peeking here rather than in the constructor keeps Transcode from blocking + // until ffmpeg has emitted its first bytes. + stream io.Reader +} + +func (f *flacPatcher) Read(p []byte) (int, error) { + if f.stream == nil { + prefix := make([]byte, flacPrefixLen) + n, err := io.ReadFull(f.ReadCloser, prefix) + if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { + return 0, err + } + prefix = prefix[:n] + if err == nil { + setFLACTotalSamples(prefix, f.duration) + } + f.stream = io.MultiReader(bytes.NewReader(prefix), f.ReadCloser) + } + return f.stream.Read(p) +} + +// setFLACTotalSamples takes the rate from the header rather than the transcode +// options, so a resampled (-ar) output still gets the right count. +func setFLACTotalSamples(prefix []byte, duration float32) { + if string(prefix[:4]) != "fLaC" || prefix[4]&0x7F != 0 { + return + } + // 20-bit rate | 3-bit channels | 5-bit depth | 36-bit total_samples + info := binary.BigEndian.Uint64(prefix[18:]) + rate := info >> 44 + if rate == 0 || info&flacMaxTotalSamples != 0 { + return + } + total := math.Round(float64(duration) * float64(rate)) + if total > flacMaxTotalSamples { + return + } + binary.BigEndian.PutUint64(prefix[18:], info|uint64(total)) +} diff --git a/core/ffmpeg/flac_streaminfo_test.go b/core/ffmpeg/flac_streaminfo_test.go new file mode 100644 index 000000000..6bf3503d7 --- /dev/null +++ b/core/ffmpeg/flac_streaminfo_test.go @@ -0,0 +1,142 @@ +package ffmpeg + +import ( + "bytes" + "errors" + "io" + "os" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// Decoded independently so the specs do not mirror the production bit-twiddling. +func readSampleRate(b []byte) int { + return int(b[18])<<12 | int(b[19])<<4 | int(b[20])>>4 +} + +func readTotalSamples(b []byte) uint64 { + return uint64(b[21]&0x0F)<<32 | uint64(b[22])<<24 | uint64(b[23])<<16 | uint64(b[24])<<8 | uint64(b[25]) +} + +var _ = Describe("patchFLACDuration", func() { + var fileFLAC []byte + + // Zeroing total_samples reproduces what a piped transcode emits. + pipedFLAC := func() []byte { + b := bytes.Clone(fileFLAC) + b[21] &= 0xF0 + clear(b[22:26]) + return b + } + + readAll := func(in []byte, duration float32) []byte { + out, err := io.ReadAll(patchFLACDuration(io.NopCloser(bytes.NewReader(in)), duration)) + Expect(err).ToNot(HaveOccurred()) + return out + } + + BeforeEach(func() { + var err error + fileFLAC, err = os.ReadFile("tests/fixtures/test.flac") + Expect(err).ToNot(HaveOccurred()) + Expect(readSampleRate(fileFLAC)).To(Equal(44100)) // specs below hard-code this rate + }) + + It("fills in total_samples from the duration", func() { + out := readAll(pipedFLAC(), 1.0) + Expect(readTotalSamples(out)).To(Equal(uint64(44100))) + }) + + It("takes the sample rate from the header, not from the source file", func() { + in := pipedFLAC() + // Rewrite the header's rate to 48000, as -ar would. + in[18], in[19] = 0x0B, 0xB8 + in[20] &= 0x0F + + out := readAll(in, 2.0) + + Expect(readSampleRate(out)).To(Equal(48000)) + Expect(readTotalSamples(out)).To(Equal(uint64(96000))) + }) + + It("rounds to the nearest sample rather than truncating", func() { + // float32(0.7)*44100 is 30869.9995, so truncation would lose a sample. + out := readAll(pipedFLAC(), 0.7) + Expect(readTotalSamples(out)).To(Equal(uint64(30870))) + }) + + It("passes through when the duration overflows the 36-bit field", func() { + in := pipedFLAC() + Expect(readAll(in, 2e6)).To(Equal(in)) + }) + + It("leaves everything after the header untouched", func() { + in := pipedFLAC() + out := readAll(in, 1.0) + Expect(out).To(HaveLen(len(in))) + Expect(out[26:]).To(Equal(in[26:])) + Expect(out[:18]).To(Equal(in[:18])) + }) + + It("leaves an already-populated total_samples alone", func() { + out := readAll(fileFLAC, 99.0) + Expect(out).To(Equal(fileFLAC)) + }) + + It("passes through a stream that is not FLAC", func() { + in := []byte("ID3\x04\x00\x00\x00\x00\x00\x00 not a flac stream at all, just bytes") + Expect(readAll(in, 1.0)).To(Equal(in)) + }) + + It("passes through when the first metadata block is not STREAMINFO", func() { + in := pipedFLAC() + in[4] = 0x04 // VORBIS_COMMENT + Expect(readAll(in, 1.0)).To(Equal(in)) + }) + + It("passes through a stream shorter than the STREAMINFO fields it patches", func() { + in := pipedFLAC()[:20] + Expect(readAll(in, 1.0)).To(Equal(in)) + }) + + It("passes through an empty stream", func() { + Expect(readAll(nil, 1.0)).To(BeEmpty()) + }) + + It("passes through when the duration is zero or negative", func() { + in := pipedFLAC() + Expect(readAll(in, 0)).To(Equal(in)) + Expect(readAll(in, -5)).To(Equal(in)) + }) + + It("passes through when the header declares no sample rate", func() { + in := pipedFLAC() + in[18], in[19] = 0, 0 + in[20] &= 0x0F + Expect(readAll(in, 1.0)).To(Equal(in)) + }) + + It("propagates a read error from the underlying stream", func() { + _, err := io.ReadAll(patchFLACDuration(io.NopCloser(io.MultiReader( + bytes.NewReader(pipedFLAC()[:10]), &errReader{})), 1.0)) + Expect(err).To(MatchError("boom")) + }) + + It("closes the underlying stream", func() { + c := &closeSpy{Reader: bytes.NewReader(pipedFLAC())} + Expect(patchFLACDuration(c, 1.0).Close()).To(Succeed()) + Expect(c.closed).To(BeTrue()) + }) +}) + +type errReader struct{} + +func (e *errReader) Read([]byte) (int, error) { return 0, errors.New("boom") } + +type closeSpy struct { + io.Reader + closed bool +} + +func (c *closeSpy) Close() error { c.closed = true; return nil } diff --git a/core/stream/media_streamer.go b/core/stream/media_streamer.go index aaa3126b4..6db2f6338 100644 --- a/core/stream/media_streamer.go +++ b/core/stream/media_streamer.go @@ -268,6 +268,7 @@ func NewTranscodingCache() TranscodingCache { BitDepth: job.bitDepth, Channels: job.channels, Offset: job.offset, + Duration: job.mf.Duration, }) if err != nil { release() From bea9715001abc956c712f76bf87cd93fd41bd6f1 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 8 Sep 2026 18:51:49 -0400 Subject: [PATCH 013/104] refactor(ui): replace icons in LibraryScanButton with react-icons --- ui/src/library/LibraryScanButton.jsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/src/library/LibraryScanButton.jsx b/ui/src/library/LibraryScanButton.jsx index 50d90e615..b793844fd 100644 --- a/ui/src/library/LibraryScanButton.jsx +++ b/ui/src/library/LibraryScanButton.jsx @@ -8,8 +8,8 @@ import { useUnselectAll, } from 'react-admin' import { useSelector } from 'react-redux' -import SyncIcon from '@material-ui/icons/Sync' -import CachedIcon from '@material-ui/icons/Cached' +import { GiMagnifyingGlass } from 'react-icons/gi' +import { VscSync } from 'react-icons/vsc' import subsonic from '../subsonic' const LibraryScanButton = ({ fullScan, selectedIds, className }) => { @@ -54,7 +54,7 @@ const LibraryScanButton = ({ fullScan, selectedIds, className }) => { ? translate('resources.library.actions.fullScan') : translate('resources.library.actions.quickScan') - const icon = fullScan ? : + const icon = fullScan ? : return (