diff --git a/internal/app/app.go b/internal/app/app.go index 14145f2..1d9e882 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -2,7 +2,9 @@ package app import ( "context" + "errors" "fmt" + "os/exec" "path/filepath" "regexp" "sort" @@ -903,6 +905,10 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall } } if err = m.Tagger.TagFLAC(outPath, tagMeta, coverPath); err != nil { + if isFFmpegMissingError(err) { + _ = m.Store.MarkFailed(ctx, source, "track", id) + return fmt.Errorf("id=%s title=%q tag: %w", id, title, err) + } m.logf("warning: tag failed for %s: %v\n", filepath.Base(outPath), err) } @@ -1342,3 +1348,13 @@ func applyPlaylistMetadataOverrides(meta map[string]any, cfg config.MetadataConf } artist["name"] = "Various Artists" } + +func isFFmpegMissingError(err error) bool { + if err == nil { + return false + } + if errors.Is(err, exec.ErrNotFound) { + return true + } + return strings.Contains(strings.ToLower(err.Error()), "ffmpeg not found") +} diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 728611b..31db4dd 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -2,9 +2,11 @@ package app import ( "context" + "fmt" "net/http" "net/http/httptest" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -20,6 +22,12 @@ type noopTagger struct{} func (n noopTagger) TagFLAC(string, tag.Metadata, string) error { return nil } +type failingTagger struct { + err error +} + +func (f failingTagger) TagFLAC(string, tag.Metadata, string) error { return f.err } + type fakeProvider struct { url string } @@ -237,6 +245,56 @@ func TestTrackRipPipeline(t *testing.T) { } } +func TestTrackRipFailsWhenTaggerReportsMissingFFmpeg(t *testing.T) { + tmp := t.TempDir() + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("audio-bytes")) + })) + defer ts.Close() + + d := config.DefaultConfigData() + d.Downloads.Folder = tmp + d.Downloads.SourceSubdirectories = false + cfg := &config.Config{File: d, Session: d} + + sqlite, err := store.NewSQLite(filepath.Join(tmp, "db.sqlite")) + if err != nil { + t.Fatalf("NewSQLite() error = %v", err) + } + defer func() { _ = sqlite.Close() }() + + m := &Main{ + Config: cfg, + Providers: map[string]provider.Client{ + "qobuz": &fakeProvider{url: ts.URL}, + }, + Store: sqlite, + DL: download.NewWithOptions(true, false), + Tagger: failingTagger{err: fmt.Errorf("ffmpeg not found: %w", exec.ErrNotFound)}, + } + + ctx := context.Background() + if err = m.AddByID(ctx, "qobuz", "track", "19512574"); err != nil { + t.Fatalf("AddByID() error = %v", err) + } + if err = m.Resolve(ctx); err != nil { + t.Fatalf("Resolve() error = %v", err) + } + err = m.Rip(ctx) + if err == nil { + t.Fatalf("expected rip failure") + } + + ok, err := sqlite.IsDownloaded(ctx, "qobuz", "19512574") + if err != nil { + t.Fatalf("IsDownloaded() error = %v", err) + } + if ok { + t.Fatalf("expected track not marked downloaded") + } +} + func TestAlbumRipPipeline(t *testing.T) { tmp := t.TempDir() diff --git a/internal/audio/convert/convert.go b/internal/audio/convert/convert.go index 89ca70e..6dcf75a 100644 --- a/internal/audio/convert/convert.go +++ b/internal/audio/convert/convert.go @@ -52,13 +52,13 @@ func Convert(path string, cfg config.ConversionConfig) (string, error) { return path, fmt.Errorf("conversion failed: %w: %s", err, string(output)) } - if path != finalPath { - _ = os.Remove(path) - } if err = os.Rename(tmpPath, finalPath); err != nil { _ = os.Remove(tmpPath) return path, err } + if path != finalPath { + _ = os.Remove(path) + } return finalPath, nil } diff --git a/internal/audio/convert/convert_test.go b/internal/audio/convert/convert_test.go index 8d3ffcb..243b390 100644 --- a/internal/audio/convert/convert_test.go +++ b/internal/audio/convert/convert_test.go @@ -1,6 +1,8 @@ package convert import ( + "os" + "path/filepath" "strings" "testing" @@ -59,3 +61,43 @@ func TestBuildFFmpegArgsNoCoverForOpus(t *testing.T) { t.Fatalf("unexpected cover map args=%s", joined) } } + +func TestConvertKeepsSourceWhenRenameFails(t *testing.T) { + tmp := t.TempDir() + in := filepath.Join(tmp, "song.flac") + if err := os.WriteFile(in, []byte("src"), 0o644); err != nil { + t.Fatalf("write input: %v", err) + } + + fakeBin := filepath.Join(tmp, "bin") + if err := os.MkdirAll(fakeBin, 0o755); err != nil { + t.Fatalf("mkdir bin: %v", err) + } + fakeFFmpeg := filepath.Join(fakeBin, "ffmpeg") + script := "#!/bin/sh\nout=\"\"\nfor arg in \"$@\"; do out=\"$arg\"; done\n: > \"$out\"\n" + if err := os.WriteFile(fakeFFmpeg, []byte(script), 0o755); err != nil { + t.Fatalf("write fake ffmpeg: %v", err) + } + t.Setenv("PATH", fakeBin) + + finalPath := strings.TrimSuffix(in, filepath.Ext(in)) + ".m4a" + if err := os.Mkdir(finalPath, 0o755); err != nil { + t.Fatalf("mkdir final path: %v", err) + } + + cfg := config.ConversionConfig{Enabled: true, Codec: "ALAC"} + out, err := Convert(in, cfg) + if err == nil { + t.Fatalf("expected rename failure") + } + if out != in { + t.Fatalf("returned path = %q, want %q", out, in) + } + if _, statErr := os.Stat(in); statErr != nil { + t.Fatalf("expected source to remain, stat err=%v", statErr) + } + tmpPath := finalPath + ".tmp.m4a" + if _, statErr := os.Stat(tmpPath); !os.IsNotExist(statErr) { + t.Fatalf("expected temp output cleanup, stat err=%v", statErr) + } +} diff --git a/internal/download/downloader.go b/internal/download/downloader.go index 273461b..5cdaaf8 100644 --- a/internal/download/downloader.go +++ b/internal/download/downloader.go @@ -323,17 +323,7 @@ func (d *Downloader) streamManifestWithFFmpeg(ctx context.Context, sourceURL, ou return fmt.Errorf("ffmpeg not found for manifest stream: %w", err) } - args := []string{ - "-y", - "-protocol_whitelist", "file,http,https,tcp,tls,crypto,data", - "-i", sourceURL, - } - if includeVideo { - args = append(args, "-map", "0") - } else { - args = append(args, "-map", "0:a:0") - } - args = append(args, "-c", "copy", "-hide_banner", "-nostats", "-progress", "pipe:2", outputPath) + args := buildFFmpegStreamArgs(sourceURL, outputPath, includeVideo) if !d.ProgressEnabled() { cmd := exec.CommandContext(ctx, "ffmpeg", args...) @@ -458,6 +448,24 @@ func (d *Downloader) streamManifestWithFFmpeg(ctx context.Context, sourceURL, ou return nil } +func buildFFmpegStreamArgs(sourceURL, outputPath string, includeVideo bool) []string { + args := []string{ + "-y", + "-protocol_whitelist", "file,http,https,tcp,tls,crypto,data", + "-i", sourceURL, + } + if includeVideo { + args = append(args, + "-map", "0:v:0?", + "-map", "0:a:0?", + ) + } else { + args = append(args, "-map", "0:a:0") + } + args = append(args, "-c", "copy", "-hide_banner", "-nostats", "-progress", "pipe:2", outputPath) + return args +} + type scanState struct { totalMS int64 currentMS int64 diff --git a/internal/download/downloader_test.go b/internal/download/downloader_test.go index a696d6c..ace490b 100644 --- a/internal/download/downloader_test.go +++ b/internal/download/downloader_test.go @@ -260,3 +260,38 @@ func TestParseClockDurationMSInvalid(t *testing.T) { t.Fatalf("expected short duration to fail") } } + +func TestBuildFFmpegStreamArgsAudioOnly(t *testing.T) { + args := buildFFmpegStreamArgs("https://example.com/master.m3u8", "/tmp/out.m4a", false) + if !containsArgPair(args, "-map", "0:a:0") { + t.Fatalf("expected audio map in args: %v", args) + } + if containsArgPair(args, "-map", "0:v:0?") { + t.Fatalf("did not expect video map in audio-only args: %v", args) + } + if containsArgPair(args, "-map", "0") { + t.Fatalf("did not expect broad map=0 in args: %v", args) + } +} + +func TestBuildFFmpegStreamArgsIncludeVideo(t *testing.T) { + args := buildFFmpegStreamArgs("https://example.com/master.m3u8", "/tmp/out.mp4", true) + if !containsArgPair(args, "-map", "0:v:0?") { + t.Fatalf("expected video map in args: %v", args) + } + if !containsArgPair(args, "-map", "0:a:0?") { + t.Fatalf("expected audio map in args: %v", args) + } + if containsArgPair(args, "-map", "0") { + t.Fatalf("did not expect broad map=0 in args: %v", args) + } +} + +func containsArgPair(args []string, key, value string) bool { + for i := 0; i+1 < len(args); i++ { + if args[i] == key && args[i+1] == value { + return true + } + } + return false +}