diff --git a/core/ffmpeg/ffmpeg.go b/core/ffmpeg/ffmpeg.go index 3d4cd0e72..af2dab647 100644 --- a/core/ffmpeg/ffmpeg.go +++ b/core/ffmpeg/ffmpeg.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "io/fs" "os" "os/exec" "path/filepath" @@ -67,7 +68,7 @@ var ErrAnimatedWebPUnsupported = errors.New("ffmpeg lacks libwebp_anim encoder const ( extractImageCmd = "ffmpeg -i %s -map 0:v -map -0:V -vcodec copy -f image2pipe -" probeCmd = "ffmpeg %s -f ffmetadata" - probeAudioStreamCmd = "ffprobe -v quiet -select_streams a:0 -print_format json -show_streams -show_format %s" + probeAudioStreamCmd = "ffprobe -v error -select_streams a:0 -print_format json -show_streams -show_format %s" ) type ffmpeg struct{} @@ -159,16 +160,80 @@ func (e *ffmpeg) ProbeAudioStream(ctx context.Context, filePath string) (*AudioP return nil, err } if err := fileExists(filePath); err != nil { - return nil, err + return nil, &ProbeError{Path: filePath, Reason: fileAccessReason(err), + NotFound: errors.Is(err, fs.ErrNotExist), err: err} } args := createFFmpegCommand(probeAudioStreamCmd, filePath, 0, 0) log.Trace(ctx, "Executing ffprobe command", "args", args) cmd := exec.CommandContext(ctx, args[0], args[1:]...) // #nosec output, err := cmd.Output() if err != nil { - return nil, fmt.Errorf("running ffprobe on %q: %w", filePath, err) + return nil, &ProbeError{Path: filePath, Reason: probeClientReason(err, filePath), err: err} } - return parseProbeOutput(output) + result, err := parseProbeOutput(output) + if err != nil { + return nil, &ProbeError{Path: filePath, Reason: err.Error(), err: err} + } + return result, nil +} + +// ProbeError reports an ffprobe failure. Reason is a path-free message safe to +// expose to clients; the wrapped cause carries the full detail for logging. +// NotFound marks the media file itself as missing — a launch failure of a +// deleted ffprobe binary also wraps fs.ErrNotExist, so callers must not infer +// it from the error chain. +type ProbeError struct { + Path string + Reason string + NotFound bool + err error +} + +func (e *ProbeError) Error() string { + if e.err == nil { + return fmt.Sprintf("probe failed on %q: %s", e.Path, e.Reason) + } + return fmt.Sprintf("probe failed on %q: %s", e.Path, probeDetail(e.err)) +} + +// Unwrap exposes the underlying cause so callers can test it with errors.Is +// (e.g. fs.ErrNotExist to detect a missing file). +func (e *ProbeError) Unwrap() error { return e.err } + +// SafeReason returns the path-free reason, safe to send to clients. +func (e *ProbeError) SafeReason() string { return e.Reason } + +// fileAccessReason maps a stat failure to a clear, path-free reason, so a moved +// or unreadable file reads as "file not found" rather than a raw ffprobe message. +func fileAccessReason(err error) string { + switch { + case errors.Is(err, fs.ErrNotExist): + return "file not found" + case errors.Is(err, fs.ErrPermission): + return "permission denied" + default: + return "file not accessible" + } +} + +// probeDetail returns the full diagnostic for logging (may contain paths): +// ffprobe's stderr when present, otherwise the raw error text. +func probeDetail(err error) string { + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok && len(exitErr.Stderr) > 0 { + return strings.TrimSpace(string(exitErr.Stderr)) + } + return err.Error() +} + +// probeClientReason returns a path-free reason for an ffprobe execution failure: +// ffprobe's stderr with the file path stripped, or a generic reason when ffprobe +// couldn't run at all (its launch error may embed the binary path). +func probeClientReason(err error, path string) string { + exitErr, ok := errors.AsType[*exec.ExitError](err) + if !ok || len(exitErr.Stderr) == 0 { + return "could not read file" + } + return strings.TrimSpace(strings.ReplaceAll(string(exitErr.Stderr), path, "the file")) } type probeOutput struct { diff --git a/core/ffmpeg/ffmpeg_test.go b/core/ffmpeg/ffmpeg_test.go index 9c20e6c05..0fa3de111 100644 --- a/core/ffmpeg/ffmpeg_test.go +++ b/core/ffmpeg/ffmpeg_test.go @@ -2,6 +2,7 @@ package ffmpeg import ( "context" + "errors" "os" "os/exec" "path/filepath" @@ -553,6 +554,65 @@ var _ = Describe("ffmpeg", func() { }) }) + Describe("ProbeError", func() { + It("uses the underlying cause in Error() so logs keep the full detail", func() { + e := &ProbeError{Path: "/music/foo.flac", + err: errors.New("/music/foo.flac: Invalid data found when processing input")} + Expect(e.Error()).To(ContainSubstring("/music/foo.flac")) + Expect(e.Error()).To(ContainSubstring("Invalid data found when processing input")) + }) + + It("returns the path-free reason from SafeReason()", func() { + e := &ProbeError{Path: "/music/foo.flac", Reason: "the file: Invalid data found when processing input"} + Expect(e.SafeReason()).To(Equal("the file: Invalid data found when processing input")) + Expect(e.SafeReason()).ToNot(ContainSubstring("/music/foo.flac")) + }) + + It("unwraps to the underlying cause so errors.Is detects a missing file", func() { + e := &ProbeError{Path: "/music/foo.flac", Reason: "file not found", err: os.ErrNotExist} + Expect(errors.Is(e, os.ErrNotExist)).To(BeTrue()) + }) + }) + + Describe("probeClientReason", func() { + It("strips the file path from ffprobe stderr", func() { + if runtime.GOOS == "windows" { + Skip("uses /bin/sh") + } + _, err := exec.Command("/bin/sh", "-c", "echo '/music/foo.flac: Invalid data found' >&2; exit 1").Output() + Expect(err).To(HaveOccurred()) + Expect(probeClientReason(err, "/music/foo.flac")).To(Equal("the file: Invalid data found")) + }) + + It("returns a generic reason for launch failures, without leaking the binary path", func() { + err := errors.New("fork/exec /opt/navidrome/bin/ffprobe: no such file or directory") + Expect(probeClientReason(err, "/music/foo.flac")).To(Equal("could not read file")) + }) + }) + + Describe("probeDetail", func() { + It("surfaces ffprobe stderr for logging", func() { + if runtime.GOOS == "windows" { + Skip("uses /bin/sh") + } + _, err := exec.Command("/bin/sh", "-c", "echo 'boom detail' >&2; exit 1").Output() + Expect(err).To(HaveOccurred()) + Expect(probeDetail(err)).To(Equal("boom detail")) + }) + }) + + Describe("fileAccessReason", func() { + It("reports a missing file as 'file not found', not a raw stat message", func() { + _, err := os.Stat("/no/such/dir/really-missing.flac") + Expect(err).To(HaveOccurred()) + Expect(fileAccessReason(err)).To(Equal("file not found")) + }) + + It("falls back to a generic reason for other access errors", func() { + Expect(fileAccessReason(errors.New("boom"))).To(Equal("file not accessible")) + }) + }) + Describe("FFmpeg", func() { Context("when FFmpeg is available", func() { var ff FFmpeg @@ -566,6 +626,16 @@ var _ = Describe("ffmpeg", func() { } }) + It("ProbeAudioStream returns a not-found ProbeError for a missing file", func() { + _, err := ff.ProbeAudioStream(GinkgoT().Context(), "/no/such/dir/really-missing.flac") + Expect(err).To(HaveOccurred()) + Expect(errors.Is(err, os.ErrNotExist)).To(BeTrue()) + var pe *ProbeError + Expect(errors.As(err, &pe)).To(BeTrue()) + Expect(pe.SafeReason()).To(Equal("file not found")) + Expect(pe.NotFound).To(BeTrue()) + }) + It("should interrupt transcoding when context is cancelled", func() { ctx, cancel := context.WithTimeout(GinkgoT().Context(), 5*time.Second) defer cancel() diff --git a/server/subsonic/transcode.go b/server/subsonic/transcode.go index 511db2b85..9eb2af160 100644 --- a/server/subsonic/transcode.go +++ b/server/subsonic/transcode.go @@ -8,6 +8,7 @@ import ( "slices" "strconv" + "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/core/stream" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -315,7 +316,8 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) decision, err := api.transcodeDecision.MakeDecision(ctx, mf, clientInfo, stream.TranscodeOptions{}) if err != nil { log.Error(ctx, "Failed to make transcode decision", "mediaID", mediaID, err) - return nil, newError(responses.ErrorGeneric, "failed to make transcode decision") + code, reason := transcodeFailure(err) + return nil, newError(code, "failed to make transcode decision: %s", reason) } // Only create a token when there is a valid playback path @@ -346,6 +348,19 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) return response, nil } +// transcodeFailure maps a decision error to a Subsonic error code and a reason +// safe to send to clients, omitting server file paths. +func transcodeFailure(err error) (int32, string) { + pe, ok := errors.AsType[*ffmpeg.ProbeError](err) + if !ok { + return responses.ErrorGeneric, "internal error" + } + if pe.NotFound { + return responses.ErrorDataNotFound, pe.SafeReason() + } + return responses.ErrorGeneric, pe.SafeReason() +} + // GetTranscodeStream handles the OpenSubsonic getTranscodeStream endpoint. // It streams media using the decision encoded in the transcodeParams JWT token. // All errors are returned as proper HTTP status codes (not Subsonic error responses). diff --git a/server/subsonic/transcode_test.go b/server/subsonic/transcode_test.go index 7e36ab243..8d5cbb974 100644 --- a/server/subsonic/transcode_test.go +++ b/server/subsonic/transcode_test.go @@ -4,12 +4,16 @@ import ( "bytes" "context" "errors" + "fmt" + "io/fs" "net/http" "net/http/httptest" + "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/core/stream" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -77,6 +81,47 @@ var _ = Describe("Transcode endpoints", func() { Expect(err.Error()).To(ContainSubstring("error retrieving media file")) }) + It("enriches the decision error with the reason, without leaking the file path", func() { + mockMFRepo.SetData(model.MediaFiles{{ID: "song-1", Suffix: "flac"}}) + mockTD.decisionErr = fmt.Errorf("probing media file song-1: %w", + &ffmpeg.ProbeError{Path: "/music/secret/foo.flac", Reason: "the file: Invalid data found when processing input"}) + r := newJSONPostRequest("mediaId=song-1&mediaType=song", "{}") + _, err := router.GetTranscodeDecision(w, r) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to make transcode decision")) + Expect(err.Error()).To(ContainSubstring("Invalid data found when processing input")) + Expect(err.Error()).ToNot(ContainSubstring("/music/secret")) + var subErr subError + Expect(errors.As(err, &subErr)).To(BeTrue()) + Expect(subErr.code).To(Equal(responses.ErrorGeneric)) + }) + + It("returns ErrorDataNotFound when the source file is missing on disk", func() { + mockMFRepo.SetData(model.MediaFiles{{ID: "song-1", Suffix: "flac"}}) + mockTD.decisionErr = fmt.Errorf("probing media file song-1: %w", + &ffmpeg.ProbeError{Path: "/music/gone.flac", Reason: "file not found", NotFound: true}) + r := newJSONPostRequest("mediaId=song-1&mediaType=song", "{}") + _, err := router.GetTranscodeDecision(w, r) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("file not found")) + var subErr subError + Expect(errors.As(err, &subErr)).To(BeTrue()) + Expect(subErr.code).To(Equal(responses.ErrorDataNotFound)) + }) + + It("keeps ErrorGeneric when ffprobe is missing, even though the cause wraps fs.ErrNotExist", func() { + mockMFRepo.SetData(model.MediaFiles{{ID: "song-1", Suffix: "flac"}}) + pe := &ffmpeg.ProbeError{Path: "/music/song.flac", Reason: "could not read file"} + mockTD.decisionErr = fmt.Errorf("probing media file song-1: %w (%w)", pe, fs.ErrNotExist) + Expect(errors.Is(mockTD.decisionErr, fs.ErrNotExist)).To(BeTrue()) + r := newJSONPostRequest("mediaId=song-1&mediaType=song", "{}") + _, err := router.GetTranscodeDecision(w, r) + Expect(err).To(HaveOccurred()) + var subErr subError + Expect(errors.As(err, &subErr)).To(BeTrue()) + Expect(subErr.code).To(Equal(responses.ErrorGeneric)) + }) + It("returns error when body is empty", func() { r := newJSONPostRequest("mediaId=song-1&mediaType=song", "") _, err := router.GetTranscodeDecision(w, r) @@ -516,6 +561,7 @@ func newJSONPostRequest(queryParams string, jsonBody string) *http.Request { // mockTranscodeDecision is a test double for stream.TranscodeDecider type mockTranscodeDecision struct { decision *stream.TranscodeDecision + decisionErr error token string tokenErr error resolvedReq stream.Request @@ -525,6 +571,9 @@ type mockTranscodeDecision struct { func (m *mockTranscodeDecision) MakeDecision(_ context.Context, _ *model.MediaFile, ci *stream.ClientInfo, _ stream.TranscodeOptions) (*stream.TranscodeDecision, error) { m.capturedClient = ci + if m.decisionErr != nil { + return nil, m.decisionErr + } if m.decision != nil { return m.decision, nil }